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: not able to open some conversations [WPB-11325] #3705

Open
wants to merge 6 commits into
base: release/candidate
Choose a base branch
from

Conversation

saleniuk
Copy link
Contributor

@saleniuk saleniuk commented Dec 4, 2024

BugWPB-11325 [Android] Can not open conversations


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 there are 2 logged in accounts and user receives a full screen intent call for not currently selected account, then after that it's only possible to open conversations where both accounts are members of.

Causes (Optional)

When a CallActivity is created, it checks whether it's needed to switch account (if the call is for other account), so when user receives a call from a conversation where both accounts are in, it's 50% chance that full screen intent will appear for one or another account (depends which incoming call gets handled first). CallActivity lives in another thread, so when the account is switched and there is another WireActivity in the background, state of both is unsynced and WireActivity still holds previous account from before switching.

Solutions

Implement observer to keep both activities in sync, so when CallActivity switches account it informs WireActivity and it performs navigation to clear the back stack and recompose the home screen with switched account.
Also, functions to observe the screenshot censoring and sync state is fixed to observe account changes, previously it was taking only single first current account value so it wasn't working when account is switched.
Show incoming call as a full screen intent only for calls for current session, as if shown for another session it will switch to that session and the user won't know that he/she is receiving a call as a different account. For calls that are not for the current session it will show the notification as a heads up notification.
We should also consider showing current account name and/or avatar on CallActivity or even make it possible for the user to switch account on incoming call screen if the call can be answered by more than one account.

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

Have 2 logged in accounts, open the app, turn off the screen, receive a call for the second account and open the app again - it should be switched to that second account.


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 4, 2024
Copy link

sonarcloud bot commented Dec 4, 2024

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 61.64384% with 28 lines in your changes missing coverage. Please review.

Project coverage is 45.25%. Comparing base (f719727) to head (711b258).

Files with missing lines Patch % Lines
...lin/com/wire/android/util/SwitchAccountObserver.kt 0.00% 17 Missing ⚠️
...re/android/notification/CallNotificationManager.kt 33.33% 4 Missing and 2 partials ⚠️
...otlin/com/wire/android/ui/WireActivityViewModel.kt 88.37% 1 Missing and 4 partials ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           release/candidate    #3705      +/-   ##
=====================================================
- Coverage              45.31%   45.25%   -0.07%     
=====================================================
  Files                    471      472       +1     
  Lines                  15854    15856       +2     
  Branches                2653     2653              
=====================================================
- Hits                    7185     7176       -9     
- Misses                  7898     7906       +8     
- Partials                 771      774       +3     
Files with missing lines Coverage Δ
...rc/main/kotlin/com/wire/android/ui/WireActivity.kt 0.00% <ø> (ø)
...m/wire/android/ui/calling/CallActivityViewModel.kt 95.65% <100.00%> (+0.41%) ⬆️
...otlin/com/wire/android/ui/WireActivityViewModel.kt 71.42% <88.37%> (+0.09%) ⬆️
...re/android/notification/CallNotificationManager.kt 44.88% <33.33%> (-0.78%) ⬇️
...lin/com/wire/android/util/SwitchAccountObserver.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 f719727...711b258. Read the comment docs.

Copy link
Contributor

github-actions bot commented Dec 4, 2024

Built wire-android-staging-compat-pr-3705.apk is available for download

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. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants