[CP Staging] Revert PR #94784 to stop loginToAccountIDMap massive logs#95233
[CP Staging] Revert PR #94784 to stop loginToAccountIDMap massive logs#95233mountiny wants to merge 2 commits into
Conversation
Removes the loginToAccountIDMap derived Onyx key and restores the pre-94784 Attendee type/shape. Resolves conflicts with #94299 by keeping the flaggedExpenses hook migration. Co-authored-by: Cursor <cursoragent@cursor.com>
melvin-bot
Bot
requested review from
JmillsExpensify and
thesahindia
and removed request for
a team
|
@thesahindia 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d877b2cd26
ℹ️ 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".
| }).length; | ||
| if (ownerAccountID !== undefined) { | ||
| // Normal case: filter by accountID | ||
| attendeesMinusOwnerCount = attendees.filter((a) => a?.accountID !== ownerAccountID).length; |
There was a problem hiding this comment.
When iouReport.ownerAccountID is available but comment.attendees comes from older/server data that only has email (the shape used before this change), this branch treats every attendee with no accountID as a non-owner because undefined !== ownerAccountID. A control-policy expense with attendee tracking required and only the report owner in the attendee list will therefore clear/avoid the missingAttendees violation, even though there is no non-owner attendee. Please fall back to comparing the owner login/email when an attendee lacks accountID rather than counting it as non-owner.
Useful? React with 👍 / 👎.
mountiny
changed the title
Aligns expectations with the reverted Attendee fields restored in IOUUtils after reverting PR #94784. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
1 participant
Explanation of Change
Reverts PR #94784, which introduced the
loginToAccountIDMapderived Onyx key. That key maps every personal-details email to an account ID and is written viaOnyx.set()whenever personal details change. react-native-onyx logs every top-level property name on each set, so accounts with many contacts produce log lines over 1 MiB. Web-Expensify'sLogAPIdrops those messages and raisesLogAPI - Massive log message given(Expensify/Expensify#655280).This revert removes
loginToAccountIDMapand restores the pre-94784 Attendee type/shape (lookup viapersonalDetailsListbyaccountID). Conflicts with #94299 were resolved by keeping the flagged-expenses hook migration (no re-add ofFLAGGED_EXPENSESderived key).Tradeoff: Attendee type no longer matches the BE-aligned shape from #94784 / #82191 until re-landed with a logging-safe approach.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/655280
PROPOSAL:
Tests
LogAPI - Massive log message givenalerts are triggered from[Onyx] set called for key: loginToAccountIDMapmessages.Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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