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

chore: one on one conversation details [WPB-15479] #3260

Merged
merged 3 commits into from
Jan 30, 2025

Conversation

Garzas
Copy link
Contributor

@Garzas Garzas commented Jan 29, 2025

TaskWPB-15479 [Android] Allow 1:1 conversations to be moved from user profile to folder


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?

  • updated GetOneToOneConversationUseCase to GetOneToOneConversationDetailsUseCase which will return
    ConversationDetails.OneOne to have access to information about folders

@Garzas Garzas self-assigned this Jan 29, 2025
@echoes-hq echoes-hq bot added the echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. label Jan 29, 2025
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.

Just a small log we need to remove, anyway lgtm !

Copy link
Contributor

github-actions bot commented Jan 29, 2025

Test Results

3 417 tests  +1   3 309 ✅ +1   6m 14s ⏱️ -10s
  584 suites ±0     108 💤 ±0 
  584 files   ±0       0 ❌ ±0 

Results for commit 1bd7602. ± Comparison against base commit e21b0a8.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.
com.wire.kalium.logic.data.conversation.ConversationRepositoryTest ‑ givenUserHasKnownContactAndConversation_WhenGettingConversationDetailsByExistingConversation_ReturnTheCorrectConversation[jvm]
com.wire.kalium.logic.data.conversation.ConversationRepositoryTest ‑ givenUserHasKnownContactAndConversation_WhenGettingConversationByExistingConversation_ReturnTheCorrectConversation[jvm]
com.wire.kalium.logic.data.conversation.ConversationRepositoryTest ‑ givenUserHasKnownContactAndConversation_WhenGettingConversationDetailsByExistingConversation_ReturnTheCorrectConversationDetails[jvm]

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jan 29, 2025

@datadog-wireapp
Copy link

datadog-wireapp bot commented Jan 29, 2025

Datadog Report

Branch report: chore/one-on-one-conversation-details
Commit report: e2c76af
Test service: kalium-jvm

✅ 0 Failed, 3309 Passed, 108 Skipped, 1m 4.52s Total Time

Comment on lines +162 to +164
selectActiveOneOnOneConversationDetails:
SELECT * FROM ConversationDetails
WHERE qualifiedId = (SELECT active_one_on_one_conversation_id FROM User WHERE qualified_id = :user_id);
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend using a JOIN instead of a subquery, it is more efficient.

SELECT ConversationDetails.*
FROM ConversationDetails
JOIN User ON ConversationDetails.qualifiedId = User.active_one_on_one_conversation_id
WHERE User.qualified_id = :user_id;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I analyzed the performance of both queries using EXPLAIN ANALYZE in SQLite. The execution plans are identical, both queries utilize the same indexes and have no significant performance difference.

Since there's no additional cost for the subquery, the choice between JOIN and WHERE (SELECT …) comes down to readability and preference.

Copy link
Member

Choose a reason for hiding this comment

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

ok.. thanks for checking

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 53.98%. Comparing base (e21b0a8) to head (1bd7602).

Files with missing lines Patch % Lines
...ersistence/dao/conversation/ConversationDAOImpl.kt 0.00% 4 Missing ⚠️
...versation/GetOneToOneConversationDetailsUseCase.kt 0.00% 3 Missing ⚠️
...um/logic/feature/conversation/ConversationScope.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3260      +/-   ##
===========================================
- Coverage    53.98%   53.98%   -0.01%     
===========================================
  Files         1303     1303              
  Lines        37446    37454       +8     
  Branches      3781     3781              
===========================================
+ Hits         20216    20220       +4     
- Misses       15809    15812       +3     
- Partials      1421     1422       +1     
Files with missing lines Coverage Δ
.../logic/data/conversation/ConversationRepository.kt 59.73% <100.00%> (+0.30%) ⬆️
...um/persistence/dao/conversation/ConversationDAO.kt 100.00% <ø> (ø)
...um/logic/feature/conversation/ConversationScope.kt 0.00% <0.00%> (ø)
...versation/GetOneToOneConversationDetailsUseCase.kt 0.00% <0.00%> (ø)
...ersistence/dao/conversation/ConversationDAOImpl.kt 71.97% <0.00%> (-1.02%) ⬇️

... and 2 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 e21b0a8...1bd7602. Read the comment docs.

@Garzas Garzas requested a review from ohassine January 30, 2025 11:34
@Garzas Garzas added this pull request to the merge queue Jan 30, 2025
Merged via the queue into develop with commit 76d2206 Jan 30, 2025
23 checks passed
@Garzas Garzas deleted the chore/one-on-one-conversation-details branch January 30, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. 🚨 Potential breaking changes 👕 size: S type: chore 🧹
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants