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

fix: fallback to proteus if conversation already present but MLS is default (WPB-15191) #3200

Merged
merged 3 commits into from
Dec 31, 2024

Conversation

yamilmedina
Copy link
Contributor

@yamilmedina yamilmedina commented Dec 27, 2024

BugWPB-15191 [Android] User can not create or see MLS conversations, when they still have a proteus client registered to their account


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

There is case when two users on different teams (team A and team B) have already Proteus conversations.
They get lost if:

  • user A PROTEUS & MLS, user B PROTEUS, team MLS - no common protocol → 1:1 not shown
  • user A PROTEUS, user B PROTEUS, team MLS - no common protocol → 1:1 not shown

Causes (Optional)

When performing the creation of 1:1, we introduced a check for validating the team default protocol. #2787
Also when resolving 1:1 conversation protocol, we introduced the concept of active_one_to_one_id #2010 this runs on migration of Proteus to MLS conversations as well.

Long story short, the checks for these cases triggers that when enabling MLS as the protocol for the team, and no matching protocols between user A and B, those conversations won't be shown, and cannot interact, even though, the conversation is a valid Proteus one from the BE perspective, as WEB also shows double-checked with the DB.

Solutions

Since we introduced on #2841 the error type CoreFailure.NoCommonProtocolFound.OtherNeedToUpdate for cases when the other users are still on Proteus, we take advantage of this case and for existing Proteus conversations, we get them and update the active_one_on_one_id. A new function was introduced that does not create Proteus conversations in this case.

More notes

https://wearezeta.atlassian.net/wiki/x/lwIPYQ

Testing

Test Coverage (Optional)

WIP. (will add)

  • I have added automated test to this contribution

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

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.

@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Dec 27, 2024
Copy link
Contributor

github-actions bot commented Dec 27, 2024

Test Results

3 344 tests   - 3   3 237 ✅  - 3   6m 2s ⏱️ +45s
  572 suites ±0     107 💤 ±0 
  572 files   ±0       0 ❌ ±0 

Results for commit ab856f5. ± Comparison against base commit 4a84b76.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 54.15%. Comparing base (da1e7eb) to head (ab856f5).
Report is 2 commits behind head on release/candidate.

Files with missing lines Patch % Lines
...logic/feature/conversation/mls/OneOnOneMigrator.kt 85.00% 1 Missing and 2 partials ⚠️
...logic/feature/conversation/mls/OneOnOneResolver.kt 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           release/candidate    #3200   +/-   ##
==================================================
  Coverage              54.15%   54.15%           
==================================================
  Files                   1250     1250           
  Lines                  36477    36493   +16     
  Branches                3696     3699    +3     
==================================================
+ Hits                   19754    19763    +9     
- Misses                 15295    15298    +3     
- Partials                1428     1432    +4     
Files with missing lines Coverage Δ
...ersation/GetOrCreateOneToOneConversationUseCase.kt 91.30% <ø> (ø)
...logic/feature/conversation/mls/OneOnOneResolver.kt 68.33% <60.00%> (-1.31%) ⬇️
...logic/feature/conversation/mls/OneOnOneMigrator.kt 92.18% <85.00%> (-3.97%) ⬇️

... and 3 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 da1e7eb...ab856f5. Read the comment docs.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Dec 27, 2024

Datadog Report

Branch report: fix/protocol-resolver-oneonone
Commit report: 813e3c0
Test service: kalium-jvm

✅ 0 Failed, 3237 Passed, 107 Skipped, 1m 1.88s Total Time

@yamilmedina yamilmedina requested a review from ohassine December 30, 2024 12:08
@yamilmedina yamilmedina added this pull request to the merge queue Dec 31, 2024
Merged via the queue into release/candidate with commit d4e993d Dec 31, 2024
21 checks passed
@yamilmedina yamilmedina deleted the fix/protocol-resolver-oneonone branch December 31, 2024 09:28
github-actions bot pushed a commit that referenced this pull request Dec 31, 2024
…efault (WPB-15191) (#3200)

* fix: fallback to proteus if conversation already present and no common protocol

* fix: test coverage

* fix: test coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: unplanned Any work item that isn’t part of the product or technical roadmap. 🚨 Potential breaking changes 👕 size: M type: bug / fix 🐞
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants