Update all usage of User(ONYXKEYS.USER), to use Account(ONYXKEYS.ACCOUNT) #60851
Conversation
inimaga
changed the title
|
Once this is reviewed and merged, the next step (D) will be App cleanup to remove all the User model in the App, including the hook introduced here. |
inimaga
changed the title
|
@ Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
inimaga
changed the title
|
@MariaHCD Fair point, it touches all the flows where user/account is invoked (All usage of User(ONYXKEYS.USER) has been replaced with Account(ONYXKEYS.ACCOUNT)). I initially added NO QA as it’s purely a cross-cutting refactor with no UI changes. But will nevertheless add a list of test cases so that potential issues / regressions can be surfaced. Regarding breaking the PR down into several smaller ones, I'm wary of doing this. Since every screen in this change uniformly uses ONYXKEYS.ACCOUNT, QA only needs to validate one consistent flow; splitting this into pieces would introduce mixed USER/ACCOUNT subscriptions and dramatically multiply the states they have to test. Keeping it as one atomic switch preserves a single end-to-end path, simplifies validation, and makes rollback trivial. |
inimaga
changed the title
Great, I think adding test cases makes a lot of sense—it’ll make it much easier to catch if there are specific flows where we’re missing the correct Onyx data from the backend, for example.
Fair point. I was initially wondering if we could split this by feature or flow to make testing easier, but I’m fine with the single-switch approach too. I agree that as long as we have all the relevant test scenarios clearly defined, it should help surface any issues during review and when QAing on staging. |
| const shouldUseStagingServer = user?.shouldUseStagingServer ?? isUsingStagingApi(); | ||
| const isDebugModeEnabled = !!user?.isDebugModeEnabled; | ||
| const [network] = useOnyx(ONYXKEYS.NETWORK, {canBeMissing: true}); | ||
| const [account = ACCOUNT_DEFAULT] = useOnyx(ONYXKEYS.ACCOUNT, {canBeMissing: true}); |
There was a problem hiding this comment.
For my own understanding, how do you determine if canBeMissing can safely be set to true?
There was a problem hiding this comment.
I looked at the usage of the key being called to see if there is a sensible default incase its not provided or if it is being accessed in a safe manner.
In the case of the code you've highlighted, account gets set to ACCOUNT_DEFAULT if ONYXKEYS.ACCOUNT is missing.
For network, its being accessed in a null safe way so if its missing, the places where it is used will still work okay.
| @@ -231,6 +231,9 @@ type Account = { | |||
|
|
|||
| /** Whether the debug mode is currently enabled */ | |||
| isDebugModeEnabled?: boolean; | |||
|
|
|||
| /** If user has accesible policies on a private domain */ | |||
| hasAccessibleDomainPolicies?: boolean; | |||
There was a problem hiding this comment.
Oh, was this a new prop added to onyx/User.ts?
Maybe let's add a comment to User.ts that it is being deprecated and should no longer be used? Or rather, should be just delete that model in this PR? 🤔
There was a problem hiding this comment.
Yes, it was a new prop introduced in this PR: #54912, merged last week.
I have added a deprecated comment saying it should no longer be used. Will delete in the next step as part of cleanup.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppuserAndroid.movAndroid: mWeb ChromeuserAndroidmWeb.moviOS: HybridAppuseriOS.moviOS: mWeb SafariuseriOSmWeb.movMacOS: Chrome / SafariuserChrome.mp4MacOS: DesktopuserDesktop.mov |
|
Code changes look good. I will test the flows to find if there is anything surpising. Lint is failing. It looks like we need to override lint at the export statement in |
MariaHCD
deleted the
issa-update-usage-of-ONYXKEYS_USER-to-ONYXKEYS_ACCOUNT-part1
branch
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 9.1.39-0 🚀
|
|
Due to the complexity of this single PR, we are working to limit the scope as some of the flows are tested independently as part of regular regression. @MariaHCD @Valforte Can you pls check if this makes sense not to retest separately and only run the ones we dont have explicit TCs for?
We will update the other TCs in a similar way shortly |
10 . Workspace & Company-Card Administration
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.1.39-8 🚀
|
|
@mvtglobally Answering your questions:
Yes this makes sense. We can skip separately retesting all the flows already covered, and only test the ones we don't have explicit coverage for. For context, given the cross-cutting nature of the refactor in this PR change, I listed the flows and highlighted all the components that the change touched so they could be covered in the QA process (for both existing flows tested independently as part of regular regression testing, and flows that are usually not tested).
This is the flow for 2 factor authentication using SMS (instead of Authy).
Yes, this is testable by assigning a secondary login to an account. And then removing that secondary login attached to the account.
Yes, this is related to the general 2FA process and will be covered by existing tests.
Here no. The existing tests have good coverage.
Yes, it is. |
|
Continued response:
No
For this, I looked at our current existing TC and they are good enough to cover it. link
Yes, this is fine.
Yes, this should cover it.
This is covered by an existing test, so skip this.
Here you just need to test that switching between workspaces for an account works fine.
No. This is good enough. |
|
Response continued: In-App UI Hooks (avatar, FAB)
This is somewhat covered by existing test but has to do with ensuring no issues with the avatar displayed in the sidebar.
Yes, this test should cover it.
This should be covered by some of the tests listed here, so skip |
|
@mvtglobally So that they are easily followable and to avoid such confusions. |
Explanation of Change
We have two models in the App that essentially represent the same thing: User and Account and this leads to a risk of duplicated information. We are thus in the process of deprecating User.ts and merge it into Account.ts to standardise and prevent duplicate information from being added to both models.
This change is step C of the entire process. i.e: Update all usage of User(ONYXKEYS.USER) in the frontend (App), to use Account(ONYXKEYS.ACCOUNT)
Fixed Issues
Part of #59277
Tests
Offline tests
QA Steps
We need to validate all the flows which are affected by this change. This means testing anything to do with the Account Model. These have been distilled below (grouped by feature area):
• SignInPage & BaseLoginForm (password login)
• ThirdPartySignInPage (Google/Apple)
• SAMLSignInPage (SSO)
• ValidateCodeForm (magic-link / one-time code)
• SMSDeliveryFailurePage (SMS 2FA fallback)
• RequireTwoFactorAuthenticationPage (force 2FA)
• LogInWithShortLivedAuthTokenPage (tokenized login)
• UnlinkLoginPage (remove a linked login)
• LogOutPreviousUserPage (logout existing session)
• TwoFactorAuthWrapper / TwoFactorAuthPage (enable 2FA)
• BaseTwoFactorAuthForm (enter code to enable)
• CopyCodesPage (display recovery codes)
• VerifyPage (verify magic codes)
• DisablePage (turn 2FA off)
• SecuritySettingsPage (overall security dashboard)
• Merge Accounts (AccountValidatePage + AccountDetailsPage)
• Add Delegate flow (AddDelegatePage + DelegateMagicCodeModal + UpdateDelegateRole modals)
• ProfilePage (name, avatar, calendar link)
• ContactMethodsPage + ContactMethodDetailsPage (phone/email on file)
• SubscriptionSettings (plan management)
• SubscriptionSize / Confirmation sub-step (change seats)
• CardSection in SubscriptionSettings (payment methods)
• EarlyCancellationMenuItem
• WalletPage (Expensify Card overview)
• ExpensifyCardPage (individual card settings)
• ReportVirtualCardFraudPage
• ReportCardLostPage
• VerifyAccountPage (identity check for card)
• ReimbursementAccountPage
• VerifiedBankAccountFlowEntryPoint
• ConnectBankAccount (USD)
• NonUSD SignerInfo
• OnboardingWorkEmail + WorkEmailValidation
• MissingPersonalDetailsMagicCodeModal
• ReferralDetailsPage (referral status)
• ConnectToXeroFlow (in Settings, via component)
• WorkspaceSwitcherPage
• PolicyAccountingPage
• WorkspaceCompanyCardsPage + EmptyState + AddNewCardPage + AssignCardFeedPage
• IssueNewCardPage + ConfirmationStep + WorkspaceExpensifyCardListPage / EmptyState
• NavigationTabBarAvatar (sidebar avatar display)
• FloatingActionButtonAndPopover (FAB shows “Request Money” or “Send Money” based on primaryLogin)
• HeaderView (greeting / account banner)
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop