Treat an explicit $0.00 max expense amount as configured, not unset - #99994
Conversation
Co-authored-by: Krishna <Krishna2323@users.noreply.github.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.
|
Android test results — pass (4/4)Saving a The PR's
Two notes on scope:
Evidence (4)Rules > Expenses page before changes
Itemized receipt required amount saved and displayed as 0.00
Validation error blocking Receipt required amount = 50
Itemized receipt required amount reopened, prefilled with 0.00
I did not capture a screen recording — the run only produced stills. |
Web test results — pass (4/4)On dev NewDot web, a saved The PR's
One caveat on the console check: direct console-log capture is not available on this web session, so that step relied on a network dump instead. Every API call in the session — including Setup note: no workspace existed, so the tester created one and enabled the Rules feature (which required an upgrade to Control, granted instantly in dev). The workspace defaults to €, not $ — the zero-vs-unset behavior under test is currency-agnostic. Evidence (3)Rules > Expenses showing Itemized receipt required amount as 0.00
Error blocking save of Receipt required amount = 50 because itemized amount is 0.00
Itemized receipt required amount field reopened, prefilled with 0.00
|
|
@MelvinBot add these steps in
|
|
Updated the PR description:
Two things worth a look before you mark it ready:
The reviewer checklist is untouched, so it's ready for your recordings. |
This comment was marked as outdated.
This comment was marked as outdated.
…ule errors Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@MelvinBot mark this PR ready for review |
|
No code fix needed — everything that can pass, passes. When you pinged me, the checks for
The one red check is Check independent approval, and it isn't a code failure. It fails because the PR has no approving review yet ( |
|
Both changes are pushed in 1. One error, on the edited field. The revamp validator now flags a single field: the one whose amount differs from its initial value, falling back to Require receipt when both (or neither) changed — using 2. Currency symbol. Swapped I applied the currency fix to the two non-revamp pages as well — The one-error-per-save rule is a UX call, so it is worth VerificationThree new UI tests in
The expected strings are built with Also green: Browser verification did not run. The web session came up as a blank page — 0 DOM nodes, no network activity — while the dev server itself served the bundles fine with a clean build. That is an environment failure, not a code failure, but it means nobody has yet seen these three cases in a real browser. Worth a manual pass before merge. |
|
@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] |
|
Marked ready for review. Before flipping it, I ran the three repro steps from #99109 on web (dev NewDot) — 3/3 pass — and added those screenshots plus the Android native ones from the earlier run into the Two things still need you, since I won't fill out human-authored sections on your behalf:
The only failing check is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b27e9560d
ℹ️ 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".
| function isMaxExpenseAmountSet(value: number | undefined): value is number { | ||
| return value !== undefined && value !== CONST.DISABLED_MAX_EXPENSE_VALUE; |
There was a problem hiding this comment.
Preserve zero receipt thresholds during Control upgrades
When a Collect/Submit policy carrying an explicit zero threshold is upgraded through upgradeToCorporate or the Control path of upgradeSubmit, getCorporateUpgradeReceiptThresholds() in src/libs/actions/Policy/Policy.ts still excludes 0 on line 5903. It consequently replaces the configured always-required rule with the default receipt thresholds in both optimistic and success Onyx data, contradicting this helper's new semantics. Reuse isMaxExpenseAmountSet() there so upgrading does not silently change the rule.
Useful? React with 👍 / 👎.
|
@dylanexpensify need a product call on this one. On the Rules revamp Require receipts page, both amounts are on the same screen and share one constraint: the receipt amount can't be greater than the itemized receipt amount. Originally, breaking it showed an error on both fields, each pointing to the other value, so it wasn't clear which one to change. I changed it in the last commit to only flag the field that was edited. That's better, but there's still an issue — if you previously touched the receipt amount and then edit the itemized amount, the error can show on the receipt field above, away from where you're working. Another option is the pattern we already use in I have that implemented locally and it feels cleaner, but it means replacing the two field-specific error strings with one. Wanted your call before switching. Happy to post screenshots of both if useful. Issue
After fix to show only one error
cc: @JmillsExpensify |
Nice update. I agree this feels a lot cleaner. |
@MelvinBot Lets do this instead. |
…flagging a field Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
No fix needed on this PR — the failing checks are not caused by these changes.
The typecheck failureAll four errors are in #99673 removed Every typecheck run across the repo flipped from pass to fail at that moment, on unrelated branches. EvidenceTypecheck runs on unrelated branches, ordered by time: This PR changes The two failing Jest ( What to doWait for #100080 ( I did not push anything. Fixing another PR's breakage inside this one would put an unrelated change in this diff, and the correct fix is already on its way to view run · no recording available |
|
Status unchanged — still nothing to fix on this PR. Same three failures as my previous analysis, from the same CI runs. No new checks have run since. The blocker is still
The fix, #100080, is open and awaiting review — mergeable, but I still haven't pushed anything. Cherry-picking that four-line deletion here would put an unrelated change in this diff and collide with #100080 when it lands. |
|
@MelvinBot merge main |
…ntZeroVsUnset Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@MelvinBot, two changes on the submit blocking:
Also apply the Polyglot Parrot patch and add the shared-message work to the Explanation of Change. |
|
Merged The branch is now up to date with The merge did bring dependency bumps (
|
…rough Control upgrades Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Checks are green — nothing left to fix. 36 checks pass, including The The one remaining red mark is The PR body still has the |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
Both changes are in 1. Submit blocking moved into
|
| Check | Result |
|---|---|
npm test -- tests/ui/RulesRequireReceiptsPageTest.tsx |
12 passed |
npm test -- tests/actions/PolicyTest.ts |
214 passed (incl. the new upgrade case) |
npm test -- tests/unit/PolicyUtilsTest.ts tests/unit/TranslateTest.ts |
391 passed |
npm run typecheck |
passed |
| ESLint on all 14 changed files | passed |
| React Compiler compliance | passed |
| cspell | 0 issues |
npm run lint-changed and npm run spell-changed both abort in this environment with git merge-base failed — the checkout is shallow, so they can't compute the diff base. I ran ESLint and cspell over the explicit file list instead, which is the same coverage.
Browser verification failed to run for the third time. The web session came up blank again — 0 accessibility nodes, unresponsive — while the dev server was healthy and had lazily compiled RulesRequireReceiptsPage.tsx without errors. So the shared-message behavior is still only covered by the Jest UI tests. The stuck-spinner case in particular is worth one manual pass: save a conflicting pair, then check the Save button still responds.
|
@MelvinBot add these steps in TestLegacy pages, account without rules revamp beta
Revamp page, account with rules revamp beta
Regressions
|
|
PR description updated.
I checked your steps against the code currently on the branch (
One thing to be aware of before you mark it ready: the checklist now asserts several things this run didn't cover — the all-platform runs, the offline test, and the High Traffic account test — and the body's own |
|
@MelvinBot, revert the Restore the local |
…icate Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Reverted in
The comment is my inference, not your words — you didn't say why, so I wrote that the helper only picks what's displayed optimistically during the upgrade and that carrying a Collect-era Two things this leaves open:
Verification
I did not run a browser pass. This commit only changes optimistic Onyx data on the Control upgrade path and deletes a unit test; the UI behavior you listed in the |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_hybrid.mp4Android: mWeb Chromeandroid_mWeb.mp4iOS: HybridAppios_hybrid.mp4iOS: mWeb Safariios_mWeb.mp4MacOS: Chrome / Safariweb_chrome_rules_revamp.mp4web_chrome.mp4 |
|
🚧 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.69-0 🚀
|
|
Yes — help site changes are required. Draft PR: #100231 This PR settled two behaviors that the help site doesn't document, and I found two stale UI labels in the same article while checking. What needed documenting
Stale labels corrected I drove a workspace on web and confirmed the live Workspaces > [Workspace Name] > Rules > Expenses section against the article:
I also reworded the two receipt bullets to the product's own phrasing ("Require receipts when spend exceeds this amount"). The old wording — "Set the minimum amount that requires a receipt" — read as the opposite of how the setting works and would have contradicted the new Scope and what I left aloneAll edits are in Category-level rules (Require receipts over / Require itemized receipts over) are different settings with different labels, so they're untouched. Article structure follows
@Krishna2323, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.69-1 🚀
Bundle Size Analysis (Sentry): |











Explanation of Change
maxExpenseAmountNoReceiptandmaxExpenseAmountNoItemizedReceiptuseundefinedandCONST.DISABLED_MAX_EXPENSE_VALUEto mean "not set".0is a valid configured limit meaning "a receipt is always required". Several call sites used a falsy check or an explicit!== 0guard, which conflated a saved$0.00with "unset".That caused the reported bug: with the itemized limit saved as
$0.00, the receipt-required validator skipped its cross-field check, so entering$50passed client validation, saved optimistically, navigated back, and was then rejected by the server — leaving the field reverted with no visible error.This adds a single shared helper and routes every affected call site through it, so the semantics stay consistent:
Call sites updated:
RulesReceiptRequiredAmountPage.tsx(validator)?? 0plus a!== 0guard$50 > $0.00error now shows and blocks the saveRulesReceiptRequiredAmountPage.tsx(defaultValue)!policy?.maxExpenseAmountNoReceipt$0.00reopens as0.00instead of an empty fieldRulesItemizedReceiptRequiredAmountPage.tsx(defaultValue)RulesItemizedReceiptRequiredAmountPage.tsx(validator)?? 0< 0), changed so both validators read the same wayRulesRequireReceiptsPage.tsxisAmountEnabledwithvalue !== 0$0.00rendered the toggle OFF with a blank field, which gated off the cross-field check at submitIndividualExpenseRulesSectionRevamp.tsx!== 0$0.00limit instead of omitting itWorth calling out for review: the rulesRevamp page carried the same defect, so this is not resolved by the revamp rollout.
The
hasConfiguredRuleshelper inPolicyUtils.tshas a related!!policy.maxExpenseAmountNoReceiptcheck, but it also compares againstDEFAULT_MAX_AMOUNT_NO_RECEIPT— it answers "is this non-default", not "is this set" — so it was deliberately left alone.One shared message for the two amounts on the rulesRevamp page
Fixing the
$0.00bug made a second problem visible. OnRulesRequireReceiptsPage.tsxboth amounts sit on one screen and share one constraint — the require-receipt amount can't exceed the require-itemized-receipt amount — so breaking it flagged both fields, each message naming the other value, with no indication of which one to change.Following the two-fields-one-constraint pattern in
IOURequestStepDistanceOdometer, the violation is now surfaced as a singleFormHelpMessagebelow the pair, with neither input flagged:workspace.rules.requireReceipts.receiptAmountGreaterThanItemizedError— "The require receipt amount can't be greater than the require itemized receipt amount." Added to all 11 locales.onSubmitrather than fromvalidate, which also runs on every keystroke and blur.onValueChange, and by either toggle.shouldShowLoadingImmediatelyOnPress={false}is required alongside the early return: nothing on this screen flips the loading flag back off, so the default press-loading spinner would stick after a blocked save and swallow every later press. Same reason asMissingPersonalDetails/subPages/PIN.tsx.The two non-rulesRevamp pages keep their field-specific messages — they edit one amount at a time, so the double-error problem does not apply there. Their copy was changed to build the amount with
convertToDisplayString, so the message reads($31.00)and matches the currency symbol shown in the field.Product sign-off on the single-message approach: #99994 (comment)
Fixed Issues
$ #99109
PROPOSAL: #99109 (comment)
Tests
Legacy pages, account without rules revamp beta
Revamp page, account with rules revamp beta
Regressions
Offline tests
QA Steps
Same as tests
Verify that no errors appear in the JS console
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.AI Tests
Run locally on this branch:
npm test -- tests/ui/RulesRequireReceiptsPageTest.tsx— 12 passed, including 4 covering the shared amount messagenpm test -- tests/actions/PolicyTest.ts— 213 passednpm test -- tests/unit/PolicyUtilsTest.ts tests/unit/TranslateTest.ts— 391 passed (includes 4isMaxExpenseAmountSetcases)npm run typecheck— passednpm run react-compiler-compliance-check -- check src/pages/workspace/rules/RulesRequireReceiptsPage.tsx— passedAI Manual Verification
Automated UI runs of the three repro steps from #99109, on Android native (standalone NewDot developmentDebug APK) and on web (dev NewDot). Both passed 3/3. The workspaces were freshly created so their default currency renders as €, not $ — the zero-vs-unset behavior under test is currency-agnostic.
0.00→ rules page shows0.00, not blank50in "Receipt required amount" → error shown, save blocked, no navigation back, no silent revert0.00, not emptyOn both platforms the blocking error read
Amount can’t be greater than the itemized receipt required amount (0.00). On Android,adb logcatfiltered forReactNativeJS [error]/[fatal],FATAL EXCEPTION, and RedBox over the test window returned no matches. On web, console-stream capture was unavailable in the harness, so console cleanliness was checked indirectly — no error overlay appeared and every API call in the network dump returned 200.Those runs predate the shared-message change on the rulesRevamp page, which has only been verified by the Jest UI tests above — the browser session failed to come up on the two attempts since. Also not covered by the automated runs: Android mWeb Chrome, iOS native, iOS mWeb Safari, and the offline behavior.
Screenshots/Videos
Android: Native
Rules > Expenses page before changes
Itemized receipt required amount saved and displayed as 0.00
Validation error blocking Receipt required amount = 50
Itemized receipt required amount reopened, prefilled with 0.00
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Rules > Expenses showing Itemized receipt required amount as 0.00
Blocking validation error when entering 50
Itemized receipt required amount reopened, prefilled with 0.00