Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(mls): navigate to migrated conversation [WPB-4705] #2317

Conversation

vitorhugods
Copy link
Member

@vitorhugods vitorhugods commented Oct 11, 2023

TaskWPB-4705 [Android] Observe & navigate to active 1-1 conversation


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

When a 1:1 Proteus conversation is migrated to MLS, it actually creates a new conversation, and moves all the content to the new conversation.

If that happens while the user has the conversation open on the screen, a blank conversation would appear, and the user would interact with a now "defunct" conversation that should no longer be visible.

Solutions

Observe the details of the conversation, and if it is a 1:1 conversation, check the activeOneOnOneConversationId with the other user and navigate to it, if it differs from the conversation being observed.

Added a new ConversationMigrationViewModel so it can be specialised into observing this kind of things.

It doesn't work using the current version of Kalium, as Kalium always returns null for activeOneOnOneConversationId when observing the conversation details, but I'm creating a PR in Kalium to fix it.

Testing

Test Coverage

  • I have added automated test to this contribution

How to Test

You can use the DB inspector to change the activeOneOnOneConversationId of some user to some other conversation while you have it open.


PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@AndroidBob
Copy link
Collaborator

Build 1245 failed.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2023

Test Results

0 tests   - 641   0 ✔️  - 641   0s ⏱️ - 8m 22s
0 suites  -   93   0 💤 ±    0 
0 files    -   93   0 ±    0 

Results for commit b84d1e7. ± Comparison against base commit f15d696.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

@vitorhugods looks like you are rolling back kalium to a previous commitish.

This means that the PR's target branch (feat/migration-system-messages) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.

feat/migration-system-messages This PR
9e4e604df95f783b7c6a0b156312732978e28fbc 8c3cd30e80c08c93c92fdac46ef02c496470fff3

Is this intentional?

@AndroidBob
Copy link
Collaborator

Build 1246 succeeded.

The build produced the following APK's:

@typfel typfel force-pushed the feat/migration-system-messages branch from f15d696 to 080ef10 Compare October 11, 2023 21:21
typfel and others added 3 commits October 12, 2023 09:42
one-on-one conversation

This commit introduces a new ViewModel `ConversationMigrationViewModel.kt` for tracking conversation migrations. It observes details of a conversation and checks if this conversation was migrated to a different one-on-one conversation. If it was, it updates the target conversation with the ID of the active one-on-one conversation.
@vitorhugods vitorhugods force-pushed the feat/mls/navigate-to-migrated-conversation branch from b84d1e7 to d8b5264 Compare October 12, 2023 07:42
@github-actions
Copy link
Contributor

@vitorhugods looks like you are rolling back kalium to a previous commitish.

This means that the PR's target branch (feat/migration-system-messages) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.

feat/migration-system-messages This PR
d055503321f4021f0529f2d544522ec40f8f115a a77539da744cc9231e1ac0c8054c97915c19509a

Is this intentional?

Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking awesome 🧉

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #2317 (d8b5264) into feat/migration-system-messages (080ef10) will increase coverage by 0.05%.
The diff coverage is 95.23%.

@@                         Coverage Diff                          @@
##             feat/migration-system-messages    #2317      +/-   ##
====================================================================
+ Coverage                             41.02%   41.07%   +0.05%     
- Complexity                             1048     1049       +1     
====================================================================
  Files                                   328      327       -1     
  Lines                                 11947    11917      -30     
  Branches                               1590     1584       -6     
====================================================================
- Hits                                   4901     4895       -6     
+ Misses                                 6573     6549      -24     
  Partials                                473      473              
Files Coverage Δ
...rc/main/kotlin/com/wire/android/WireApplication.kt 0.00% <ø> (ø)
...onversations/banner/ConversationBannerViewModel.kt 55.81% <ø> (ø)
...ations/migration/ConversationMigrationViewModel.kt 95.23% <95.23%> (ø)

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 080ef10...d8b5264. Read the comment docs.

@vitorhugods vitorhugods merged commit 36b5ef6 into feat/migration-system-messages Oct 12, 2023
11 checks passed
@vitorhugods vitorhugods deleted the feat/mls/navigate-to-migrated-conversation branch October 12, 2023 08:07
@github-actions
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 1253 succeeded.

The build produced the following APK's:

@typfel typfel restored the feat/mls/navigate-to-migrated-conversation branch October 15, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants