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

PM-12051: Fix sync error after logout and switch accounts #1100

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

matt-livefront
Copy link
Collaborator

@matt-livefront matt-livefront commented Oct 31, 2024

🎟️ Tracking

PM-12051

📔 Objective

Fixes an issue with automatic account switching when logging out if the two accounts are in separate environments.

  • Account A logged into environment X
  • Account B logged into environment Y
  • If account A logs out, the app will automatically switches to account B. When this occurred, the environment service wasn't loading the URLs for account B/environment Y. This caused any future networking requests to fail for account B in the current session.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Comment on lines -309 to -319
if let account = try? await services.authRepository.getAccount(),
userId == account.profile.userId {
return await handleAndRoute(
.accountBecameActive(
account,
animated: false,
attemptAutomaticBiometricUnlock: true,
didSwitchAccountAutomatically: false
)
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This logic here is the same as what's below, without the call to authRepository.setActiveAccount().

As far as I can tell, this was put in originally as a shortcut if you attempted to switch accounts to the already active account, then it would skip the setActiveAccount() call. However, this causes issues because we also have logic in StateService that makes the next account active when logging out. So what would happen in this scenario, is the first account would logout, StateService would make the next account active and eventually this switchAccountRedirect would be called. Since StateService has already updated the active account, this would skip the authRepository.setActiveAccount call, which does a few other things like reloads the URLs in the environment service. So I think it's better if we always call setActiveAccount() here. We have checks in place in the profile switcher if you try to switch to the already active account, so I couldn't find a scenario where this would be called and you wouldn't want to call setActiveAccount().

if state.activeUserId == knownUserId, userInitiated {
// Find the next account to make the active account.
state.activeUserId = state.accounts.first?.key

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.34%. Comparing base (defdd8d) to head (aa70a30).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1100   +/-   ##
=======================================
  Coverage   89.34%   89.34%           
=======================================
  Files         677      677           
  Lines       42771    42760   -11     
=======================================
- Hits        38214    38205    -9     
+ Misses       4557     4555    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matt-livefront matt-livefront merged commit 74e3bbb into main Nov 1, 2024
9 checks passed
@matt-livefront matt-livefront deleted the matt/PM-12051-sync-fail branch November 1, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants