Exclude all-held reports from the Inbox to-do, keep them for whoever placed the hold#96178
Exclude all-held reports from the Inbox to-do, keep them for whoever placed the hold#96178allgandalf wants to merge 5 commits into
Conversation
|
@parasharrajat 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: a1ef2f1838
ℹ️ 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".
| // An all-held report can't move to its next state, so it isn't a to-do. Keep it only for an Approve or Pay report | ||
| // where the current user placed a hold, since only they can unhold it. Submit stays excluded even then, because on | ||
| // an open report only the owner can hold, so the exception would apply to every all-held report and defeat it. | ||
| const isExcludedForHeldExpenses = hasOnlyHeldExpenses(transactions) && (actionBadge === CONST.REPORT.ACTION_BADGE.SUBMIT ? true : !currentUserPlacedHold); |
There was a problem hiding this comment.
When the chat reaches the fallback path above (hasOutstandingChildRequest/iouReportID but no REPORT_PREVIEW action loaded), actionBadge is undefined. For an open submit report whose only expense is held by the current owner, currentUserPlacedHold is true, so this expression does not exclude it and hasValidIOUAction still raises a GBR for the policy expense chat, even though submit all-held reports are supposed to stay excluded (and TodosUtils excludes them unconditionally). This leaves the Inbox/LHN to-do inconsistent for the no-preview fallback case already handled by this function.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const hasOnlyPendingTransactions = transactions.length > 0 && transactions.every((t) => isPending(t)); | ||
|
|
||
| const iouReportActions = getAllReportActions(iouReportID); | ||
| const currentUserPlacedHold = didCurrentUserPlaceHoldOnReportExpense(iouReportActions, transactions, currentUserAccountID); |
There was a problem hiding this comment.
❌ PERF-2 (docs)
didCurrentUserPlaceHoldOnReportExpense is the expensive part of this block — it iterates every report action and, for each money-request action, scans reportTransactions.find(...) plus a getReportAction lookup (roughly O(actions × transactions)). It runs unconditionally here, but its result (currentUserPlacedHold) is only ever consumed when hasOnlyHeldExpenses(transactions) is true (the short-circuit && on the next line). Since getReasonAndReportActionThatRequiresAttention is called across the LHN/report-attributes derivation for many reports, this overhead is paid on every report even when no expense is held.
Gate the expensive call behind the cheap hasOnlyHeldExpenses(transactions) check so it only runs when it can actually change the outcome:
const hasOnlyHeld = hasOnlyHeldExpenses(transactions);
const currentUserPlacedHold = hasOnlyHeld && didCurrentUserPlaceHoldOnReportExpense(getAllReportActions(iouReportID), transactions, currentUserAccountID);
const isExcludedForHeldExpenses = hasOnlyHeld && (actionBadge === CONST.REPORT.ACTION_BADGE.SUBMIT ? true : !currentUserPlacedHold);Reviewed at: a1ef2f1 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
1 participant
Explanation of Change
The initial fix excluded all-held reports from the Submit/Approve/Pay to-dos on Home "For you", the Search type-menu badges, and the Spend page. But the Inbox "To-do" tab still showed the Approve to-do for those reports. The Inbox tab and the green action badge come from a separate client-derived value,
reportAttributes.requiresAttention(viagetReasonAndReportActionThatRequiresAttentionthengetIOUReportActionWithBadge), which the initial fix never touched. That path only screened out all-pending transactions, never all-held ones.This excludes all-held reports from that Inbox derivation too, so the Inbox matches Home and the Spend page.
It also applies the refinement discussed on the issue: an all-held report stays in the to-do of whoever placed the hold, since only they can remove it. Hold every expense on a report as the approver and it stays in your Approve/Pay to-do; if someone else placed the hold, it drops out of your to-do. Submit stays excluded whenever every expense is held, because on an open report only the report owner can place a hold and the owner is the one who submits, so a "keep it for the holder" exception there would keep every all-held open report.
The exclusion lives in one shared helper (
didCurrentUserPlaceHoldOnReportExpenseinReportUtils) that both the Inbox derivation andTodosUtils(Home/Spend) call, so the two to-do surfaces stay in sync.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/637331
PROPOSAL:
Tests
action:approve(all three surfaces match).Offline tests
The to-do classification is computed on-device from data already in Onyx, so it behaves identically offline. Repeat the Tests steps with the network disabled and verify the same outcomes.
QA Steps
Same as tests.
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