Repository navigation
[Payment due @eh2077] [No QA] Prefer live account on personal details login collision - #96469
Conversation
|
@codex review! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28a107b821
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ed-login-collision
…ed-login-collision
Auth no longer needs the validated tie-break now that isClosed is deployed, so the login-collision preference simplifies to first-live-entry-wins. Apply the same preference to the loginToAccountIDMap derived value per review feedback.
Trim to the single reason this field exists on the client: detecting a merged-away login collision.
|
@codex review |
…ed-login-collision
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Use accountID/login variables instead of literal numeric and email object keys.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: deffd206a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-29.at.8.11.45.PM.mov |
An optimistic personal detail can collide with a real one on the same login before the real entry replaces it in Onyx, so treat isOptimisticPersonalDetail like isClosed in both the imperative cache and the derived login map.
|
@codex review. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
🎯 @eh2077, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
| const login = personalDetails.login.toLowerCase(); | ||
| const existingAccountID = loginToAccountIDMap[login]; | ||
| const existingDetail = existingAccountID === undefined ? undefined : personalDetailsList[existingAccountID]; | ||
| if (!existingDetail || existingDetail.isClosed || existingDetail.isOptimisticPersonalDetail) { | ||
| loginToAccountIDMap[login] = personalDetails.accountID; | ||
| } |
There was a problem hiding this comment.
I hope this wont have significant impact on performance cc @TMisiukiewicz @adhorodyski
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.46-0 🚀
|
Help site review — no changes requiredI reviewed the changes in this PR against the help site articles under WhyThis PR changes how the in-memory
There is:
The help site documents product behavior and workflows, none of which this correctness fix alters. It simply prevents an approver's email from resolving to a stale merged-away account ID. Since no changes are required, I did not create a draft help site PR. @c3024, let me know if you believe any specific help article is affected and I'll take another look. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Payment issue created: #97540 |
Explanation of Change
When an account is merged away, the closed account's personal details are still served for old chat participants with the
MERGED_prefix stripped from itslogin, so the client can hold two personal details with the same login (the closed one and the live one). TheemailToPersonalDetailsCacheinPersonalDetailsUtilsis keyed by login and previously let whichever entry came last (the higher accountID) win, sogetAccountIDsByLoginscould resolve an approver's email to the closed account's ID. Commands likeSubmitReportthen convert that stale ID back to the rawMERGED_0@...email and fail the workspace membership check ("Please select another approver...", see the linked issue — 18 workspaces affected).This change makes the cache prefer the live entry on a login collision:
isClosed(new optional flag stamped by Auth in https://github.com/Expensify/Auth/pull/23052, now deployed).The same preference is applied to the
loginToAccountIDMapderived value (src/libs/actions/OnyxDerived/configs/loginToAccountIDMap.ts), which is consumed by components like attendee cells/fields and would otherwise still resolve a colliding login to the closed account.Server-side counterpart that hardens SubmitReport regardless of client state: https://github.com/Expensify/Web-Expensify/pull/54680.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/660923
PROPOSAL:
Tests
npx jest tests/unit/libs/PersonalDetailsUtilsTest.ts tests/unit/OnyxDerived/loginToAccountIDMapTest.ts.Offline tests
None — the change only affects how the in-memory email→personal details cache and the derived login map pick between two entries with the same login; there is no network behavior change.
QA Steps
Cannot be QA'd organically: reproducing requires an account whose personal details contain a closed merged-away account colliding with a live login, which fresh test accounts never have. Covered by unit tests; validated in production via the linked issue's affected workspaces once deployed.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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