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(mls-migration): set correct CS when Kalium start migration (WPB-9638) #2812

Conversation

mchenani
Copy link
Contributor


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?

When Android/Kalium starts the migration flow, we actually creating the MLS Conversation, and due to the agreement that the BE will not set the Correct CS in the Create Conversation Response, we need to ignore that value; in the normal conversation creation this was considered, but not in the migration flow.

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

  • Have a team without MLS enabled
  • Login on android and web
  • Create a Proteus group and send some messages
  • Enable MLS and set the migration to be started
  • Log out and log in on Android, the android app must start the migration to MLS
  • Web ( without reloading the web-app) and android app must be able to send and receive messages and the group must be established as MLS

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.

mythsunwind and others added 12 commits May 13, 2024 17:02
* feat: use case to send button action confirmations (WPB-2633)

* Fixed issue with domain of button sender
* feat: testservice can send poll message buttons (WPB-2633) (#2377)

* feat: testservice can send poll message buttons (WPB-2633)

* Remove not needed nullable & check for parameter

* Add test for button change

* Use specific use case for composite message

* Revert change in SendTextMessageUseCase

---------

Co-authored-by: Mojtaba Chenani <[email protected]>

* fix: Solved merge conflict that removed PR 2393 and 2377

* feat: use case to send button action confirmations (WPB-2633)

* Fixed issue with domain of button sender

---------

Co-authored-by: Mojtaba Chenani <[email protected]>
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@@ -619,6 +620,7 @@ internal class MLSConversationDataSource(
externalSenders: ByteArray,
allowPartialMemberList: Boolean = false,
): Either<CoreFailure, MLSAdditionResult> = withContext(serialDispatcher) {
kaliumLogger.d("establish MLS group: $groupID")
Copy link
Member

Choose a reason for hiding this comment

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

is it fine to log the full group id here ?

Copy link
Contributor Author

@mchenani mchenani Jun 17, 2024

Choose a reason for hiding this comment

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

yes. there is no harm in it. hmm, but I can remove it if we want.

Copy link
Contributor

github-actions bot commented Jun 17, 2024

Test Results

2 874 tests   2 856 ✔️  4m 6s ⏱️
       7 suites       18 💤
       7 files           0

Results for commit 75db67d.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.76923% with 10 lines in your changes missing coverage. Please review.

Please upload report for BASE (testservice-release-cycle-4.6@8c9a70c). Learn more about missing BASE report.

Additional details and impacted files
@@                       Coverage Diff                        @@
##             testservice-release-cycle-4.6    #2812   +/-   ##
================================================================
  Coverage                                 ?   58.39%           
  Complexity                               ?        7           
================================================================
  Files                                    ?     1187           
  Lines                                    ?    46713           
  Branches                                 ?     4431           
================================================================
  Hits                                     ?    27278           
  Misses                                   ?    17452           
  Partials                                 ?     1983           
Files Coverage Δ
...gic/data/conversation/MLSConversationRepository.kt 82.37% <100.00%> (ø)
...ium/logic/data/featureConfig/FeatureConfigModel.kt 100.00% <100.00%> (ø)
...e/kalium/logic/feature/mlsmigration/MLSMigrator.kt 78.37% <100.00%> (ø)
...henticated/featureConfigs/FeatureConfigResponse.kt 84.83% <100.00%> (ø)
...ire/kalium/persistence/config/UserConfigStorage.kt 72.22% <100.00%> (ø)
...om/wire/kalium/logic/configuration/E2EISettings.kt 87.50% <83.33%> (ø)
...feature/featureConfig/handler/E2EIConfigHandler.kt 88.23% <75.00%> (ø)
...c/data/e2ei/CertificateRevocationListRepository.kt 87.09% <88.88%> (ø)
...um/logic/data/featureConfig/FeatureConfigMapper.kt 69.30% <50.00%> (ø)
...re/kalium/network/api/base/unbound/acme/ACMEApi.kt 26.15% <0.00%> (ø)

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 8c9a70c...75db67d. Read the comment docs.

@datadog-wireapp
Copy link

Datadog Report

All test runs bee73b5 🔗

2 Total Test Services: 0 Failed, 2 Passed

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Test Service View
kalium-ios 0 0 0 2761 123 1.47s Link
kalium-jvm 0 0 0 2902 107 6.29s Link

@mchenani mchenani requested a review from MohamadJaara June 17, 2024 12:34
@mchenani mchenani changed the base branch from testservice-release-cycle-4.6 to release/android-cycle-4.6 June 18, 2024 08:14
@mchenani mchenani closed this Jun 18, 2024
@mchenani mchenani deleted the fix/set-correct-cs-when-kalium-start-migration branch June 18, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants