[HOLD BE] Exclude reports with all expenses on hold from Submit/Approve/Pay to-dos#92499
[HOLD BE] Exclude reports with all expenses on hold from Submit/Approve/Pay to-dos#92499allgandalf wants to merge 10 commits into
Conversation
melvin-bot
Bot
requested review from
a team,
Eskalifer1 and
JmillsExpensify
and removed request for
a team
|
@Eskalifer1 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] |
|
@marufsharifi 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: b8c7d91c39
ℹ️ 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".
| const reportTransactions = transactionsByReportID[report.reportID] ?? []; | ||
| const reportMetadata = allReportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${report.reportID}`]; | ||
| if (isSubmitAction(report, reportTransactions, reportMetadata, policy, reportNameValuePair, undefined, login, currentUserAccountID)) { | ||
| if (isSubmitAction(report, reportTransactions, reportMetadata, policy, reportNameValuePair, undefined, login, currentUserAccountID) && !hasOnlyHeldExpenses(reportTransactions)) { |
There was a problem hiding this comment.
When a report has only held active expenses plus an unheld transaction that is pending deletion (for example, after deleting/moving an expense while offline or before the delete request syncs), transactionsByReportID still includes that pending-delete transaction, so this new hasOnlyHeldExpenses(reportTransactions) guard returns false and the report stays in Submit/Approve/Pay to-dos. The report header path already filters pending deletes before computing the primary action (useReportPrimaryAction.ts:46-53), so the to-do counts can disagree with the actionable UI in that state; filter pending-delete transactions before applying the new held-only checks.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const reportTransactions = transactionsByReportID[report.reportID] ?? []; | ||
| const reportMetadata = allReportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${report.reportID}`]; | ||
| if (isSubmitAction(report, reportTransactions, reportMetadata, policy, reportNameValuePair, undefined, login, currentUserAccountID)) { | ||
| if (isSubmitAction(report, reportTransactions, reportMetadata, policy, reportNameValuePair, undefined, login, currentUserAccountID) && !hasOnlyHeldExpenses(reportTransactions)) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
hasOnlyHeldExpenses(reportTransactions) is called three times within the same loop iteration (lines 61, 64, and 78) with the identical argument. Since reportTransactions does not change between calls, the result should be computed once and reused to avoid redundant work and duplication.
Compute it once at the top of the loop body:
const allExpensesHeld = hasOnlyHeldExpenses(reportTransactions);Then use !allExpensesHeld in each of the three conditions.
Reviewed at: b8c7d91 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| reportActions, | ||
| }) && hasOnlyHeldExpenses(reportTransactions); | ||
| const expensesToHold = getAllExpensesToHoldIfApplicable(report, reportActions, reportTransactions, policy, currentUserAccountID); | ||
| const shouldRemoveHoldForAllHeldExpenses = hasOnlyHeldExpenses(reportTransactions) && !!expensesToHold.length; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
hasOnlyHeldExpenses(reportTransactions) is already called on line 483 as part of isPayActionWithAllExpensesHeld. The new line 485 calls it again redundantly. Extract the result into a shared variable to avoid the duplicate call.
For example:
const allExpensesHeld = hasOnlyHeldExpenses(reportTransactions);
const isPayActionWithAllExpensesHeld = isPrimaryPayAction({...}) && allExpensesHeld;
const shouldRemoveHoldForAllHeldExpenses = allExpensesHeld && !!expensesToHold.length;Reviewed at: b8c7d91 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
allgandalf
changed the title
|
Hi @allgandalf I'll be partially unavailable this Friday (after lunch) and OOO all next week, so I won't be able to work on this PR. I see that it’s on HOLD, so we can wait, but if the PR is ready to work on (the backend will be ready) and I won’t be around for a few more days (since I don’t think it makes sense to reassign it, say, on Friday evening), then feel free to reassign this issue, for example, to the second C+ who was assigned here. Thanks! |
|
I'm re-assigning to @Krishna2323 since they were already working on #85861 |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Reported this issue. The testing steps also look good. Approved.
4 participants
Explanation of Change
Reports where every expense is on hold were still counted in the Submit, Approve, and Pay to-dos on Home (For You), the Inbox type-menu badges, and the Search expense-reports tab, even though the user can't act on them until the hold is removed. All three counts come from
ONYXKEYS.DERIVED.TODOS(createTodosReportsAndTransactions), and itsisSubmitAction/isApproveAction/isPrimaryPayActiongates never looked at hold state. Added a!hasOnlyHeldExpenses(reportTransactions)guard to each of the three pushes, matching the existing!hasOnlyNonReimbursableTransactionsguard already on the pay push.Also updated
getReportPrimaryActionso an all-held report the user can unhold surfaces Remove Hold on the report header instead of Approve or Submit, reusing the existing Remove Hold fallback that already handled the pay case.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/637331
PROPOSAL: N/A (internal bug fix, BE contributor assigned)
Tests
Approve case:
Submit case:
Pay case:
Offline tests
The to-do counts are a client-side Onyx derived value with no network dependency, so the behavior is identical offline. With an all-held report already in Onyx, go offline and confirm the report is still excluded from the Submit/Approve/Pay counts and that the report header still shows Remove Hold.
QA Steps
Same as Tests.
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))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.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