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: sub conversation epoch verification [WPB-15647] #3244

Merged
merged 5 commits into from
Jan 24, 2025

Conversation

Garzas
Copy link
Contributor

@Garzas Garzas commented Jan 24, 2025

BugWPB-15647 [Android] epoch check for sub conversations is verifying the main 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

The previous implementation did not handle epoch verification for sub-conversations. As a result, the verification process always checked the parent conversation's epoch, leading to incorrect behavior when the sub-conversation epoch was out of sync while the parent conversation's epoch remained valid.

Causes

  1. Lack of Sub-Conversation Handling: Sub-conversations were not considered during epoch verification.
  2. Missing Models and Logic: There were no models or logic in place to support sub-conversation-specific checks.
  3. Incorrect Parent Conversation Focus: Epoch verification defaulted to the parent conversation, ignoring sub-conversations entirely.

Solutions

  1. Added verifySubConversationEpoch:

    • A dedicated method to verify epochs for sub-conversations, ensuring accurate context for verification.
  2. Sub-Conversation Repository Arrangement:

    • Created SubconversationRepositoryArrangement to mock and test sub-conversation-related methods efficiently.
  3. Introduced Sub-Conversation Models:

    • Added models in the logic module to handle sub-conversations as first-class entities.
  4. Updated Epoch Verification Logic:

    • Refactored NewMessageEventHandler to call verifySubConversationEpoch when a subConversationId is provided.
  5. Added Comprehensive Tests:

    • Verified that fetchConversation is not called when a subConversationId is present.
    • Ensured correct behavior for valid epochs, stale epochs, and failures.

@echoes-hq echoes-hq bot added the echoes: product-roadmap/bug Work contributing to resolve a bug not critical enough to have raised an incident. label Jan 24, 2025
Copy link
Contributor

github-actions bot commented Jan 24, 2025

Test Results

3 353 tests  +5   3 245 ✅ +5   5m 41s ⏱️ +27s
  572 suites ±0     108 💤 ±0 
  572 files   ±0       0 ❌ ±0 

Results for commit bdec069. ± Comparison against base commit b471e2a.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Jan 24, 2025

Datadog Report

Branch report: fix/subconversation-epoch-verifier
Commit report: ca7e851
Test service: kalium-jvm

✅ 0 Failed, 3245 Passed, 108 Skipped, 1m 0.71s Total Time

@Garzas Garzas enabled auto-merge January 24, 2025 11:25
@Garzas Garzas added this pull request to the merge queue Jan 24, 2025
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 45.61404% with 31 lines in your changes missing coverage. Please review.

Project coverage is 54.51%. Comparing base (b471e2a) to head (bdec069).

Files with missing lines Patch % Lines
.../kalium/logic/data/conversation/SubConversation.kt 0.00% 12 Missing ⚠️
...m/logic/data/conversation/SubConversationMapper.kt 0.00% 12 Missing ⚠️
...gic/data/conversation/SubconversationRepository.kt 0.00% 3 Missing ⚠️
...authenticated/conversation/ConversationResponse.kt 0.00% 2 Missing ⚠️
...c/data/conversation/LeaveSubconversationUseCase.kt 0.00% 0 Missing and 1 partial ⚠️
.../kotlin/com/wire/kalium/logic/data/id/IdMappers.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           release/candidate    #3244      +/-   ##
=====================================================
- Coverage              54.52%   54.51%   -0.02%     
=====================================================
  Files                   1250     1252       +2     
  Lines                  36555    36605      +50     
  Branches                3699     3702       +3     
=====================================================
+ Hits                   19932    19954      +22     
- Misses                 15222    15250      +28     
  Partials                1401     1401              
Files with missing lines Coverage Δ
...kalium/logic/feature/message/StaleEpochVerifier.kt 100.00% <100.00%> (ø)
...ver/conversation/message/NewMessageEventHandler.kt 84.76% <100.00%> (+0.14%) ⬆️
...c/data/conversation/LeaveSubconversationUseCase.kt 82.14% <0.00%> (ø)
.../kotlin/com/wire/kalium/logic/data/id/IdMappers.kt 93.33% <0.00%> (-6.67%) ⬇️
...authenticated/conversation/ConversationResponse.kt 0.00% <0.00%> (ø)
...gic/data/conversation/SubconversationRepository.kt 0.00% <0.00%> (ø)
.../kalium/logic/data/conversation/SubConversation.kt 0.00% <0.00%> (ø)
...m/logic/data/conversation/SubConversationMapper.kt 0.00% <0.00%> (ø)

... and 1 file 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 b471e2a...bdec069. Read the comment docs.

Merged via the queue into release/candidate with commit 8466dc4 Jan 24, 2025
21 checks passed
@Garzas Garzas deleted the fix/subconversation-epoch-verifier branch January 24, 2025 14:19
github-actions bot pushed a commit that referenced this pull request Jan 24, 2025
* fix: sub conversation epoch verification

* test fix

* detekt fix

* test fix
github-merge-queue bot pushed a commit that referenced this pull request Jan 27, 2025
* fix: sub conversation epoch verification

* test fix

* detekt fix

* test fix

Co-authored-by: Jakub Żerko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: product-roadmap/bug Work contributing to resolve a bug not critical enough to have raised an incident. 🚨 Potential breaking changes 👕 size: L type: bug / fix 🐞
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants