Don't offer a zero-amount partial approve/pay option when the non-held amount isn't meaningful - #100641
Don't offer a zero-amount partial approve/pay option when the non-held amount isn't meaningful#100641MelvinBot wants to merge 2 commits into
Conversation
… hasNonHeldExpenses Co-authored-by: Marco Chávez <marcochavezf@users.noreply.github.com>
Co-authored-by: Marco Chávez <marcochavezf@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Android test steps verification results: ✅ pass Verified on standalone Android NewDot: the approve confirmation modal never offers a $0.00 partial-approve option for a report with a held $0.00 expense and a non-held -$5.00 expense; it only shows "Approve -$5.00", consistent from both the workspace chat/report and the Spend > Reports entry points. Note: the PR's |
|
🤖 Web test steps verification results: ✅ pass PR #100641's fix verified on dev NewDot web: with a $0.00 expense held and a -$5.00 expense remaining, the approve confirmation modal correctly offers only "Approve -$5.00" and never shows a zero-amount "$0.00" partial-approve option, in both the workspace chat and Spend > Reports. Setup notes: the fresh session had no workspace, so one was created to reach a workspace expense chat. Two expenses were created there — a $0.00 "Zero Expense" and a -$5.00 "Negative Expense" — the report was submitted, and the $0.00 expense was put on hold, leaving a non-held total of -$5.00. The PR's view run · no recording available |




Explanation of Change
The approve confirmation on Spend > Reports offered an "Approve only $0.00" option that the report/workspace-chat approve dropdown correctly omits.
getNonHeldAndFullAmountreturnshasValidNonHeldAmount, which isfalsewhen the unheld total isn't meaningfully different from the full total (for example, a held0.00expense alongside an unheld negative expense). Every caller ofHoldMenuModalWrapperandProcessMoneyReportHoldMenualready honours that flag and passesnonHeldAmount: undefinedto mean "there is no valid partial amount".Both modals then threw that intent away:
nonHeldAmount = '0'default parameter converted the caller'sundefinedback into a renderable0.hasNonHeldExpenses— a different boolean that only says whether an unheld expense exists at all — instead of on whether a valid partial amount was actually supplied.So a report with an unheld expense but no meaningful partial amount rendered "Approve only $0.00" / "Pay only $0.00".
This change drops the
'0'default and gates the option onnonHeldAmount !== undefined, so the callers' existingundefinedsentinel is respected. The prompt text stays gated onhasNonHeldExpenses, which matches how the approve dropdown gates its own header text onhasOnlyHeldExpenses— only the partial option was wrong.ProcessMoneyReportHoldMenuis a near-duplicate ofHoldMenuModalWrapperand had the identical defect on the Pay flow, reachable from the report preview and the selection toolbar. Both are fixed here so the two surfaces don't drift again.Regression introduced by: #83667, which rewrote the report/chat approve flow into a dropdown that gates correctly, but left the Search surface on the older modal. Before that PR both surfaces shared this modal, so they could not disagree.
Root cause analysis: #100639 (comment)
Automated checks run locally
npm run typechecknpx eslinton all three changed filesnpm run spell-changednpm run react-compiler-compliance-check checkon both componentsnpm test— newHoldMenuModalWrapperTest,ReportPreviewHoldMenuTest,DecisionModalTest,MoneyRequestReportPreview.testThe new test was confirmed to fail against the unfixed code (
firstOptionTextwas" 0") before the fix was restored.npm run react-compiler-compliance-check check-changedandnpm run lint-changedcould not resolve their base ref in this environment (GITHUB_BASE_REFis set to an empty string), so the equivalent per-file commands were run instead.npx prettier --checkcould not run either — it resolved a.prettierrc.jsoutside the App checkout that requires a plugin not installed there.Fixed Issues
$ #100639
PROPOSAL: #100639 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate your change worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas they need to test for regressions.
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