Skip to content

Fix missing tax amount limit error after money-request route migration - #93251

Closed
MelvinBot wants to merge 2 commits into
mainfrom
claude-taxAmountFormRouteCheck
Closed

Fix missing tax amount limit error after money-request route migration#93251
MelvinBot wants to merge 2 commits into
mainfrom
claude-taxAmountFormRouteCheck

Conversation

@MelvinBot

Copy link
Copy Markdown
Contributor

Explanation of Change

The tax-amount step stopped enforcing the tax limit after PR #92161 migrated the route from the camelCase taxAmount path to the kebab-case money-request/tax-amount path.

MoneyRequestAmountForm decides whether the amount input is a tax amount by checking whether the active route contains the substring taxAmount. After the route rename, that substring was never present, so isTaxAmountForm was always false and the over-limit check (isTaxAmountInvalid) was skipped — letting users save a tax amount above the allowed limit with no error.

This change:

  • Adds a constant CONST.IOU.TAX_AMOUNT_ROUTE_PREFIX = 'money-request/tax-amount' so the route string is no longer a magic literal.
  • Updates the detection in MoneyRequestAmountForm to check for that prefix, which matches the new route.
  • Adds a regression test that asserts the generated tax-amount route stays in sync with the constant, so a future route rename that would re-break this validation fails CI.

Fixed Issues

$ #93236
PROPOSAL: #93236 (comment)

Tests

  1. Open a workspace with Taxes enabled in a paid/Collect workspace.
  2. Go to the workspace chat → + → Create expense → Manual.
  3. Enter an amount → Next.
  4. On the confirmation page, tap Tax amount.
  5. Enter a tax amount that exceeds the limit and try to save.
  6. Verify an error appears (Tax amount cannot be greater than …) and the value cannot be saved.
  7. Enter a valid tax amount (including 0) and verify it saves with no error.
  • Verify that no errors appear in the JS console

Offline tests

Same as Tests.

QA Steps

  1. Precondition: Enable Taxes in workspace settings.
  2. Go to staging.new.expensify.com → workspace chat.
  3. + → Create expense → Manual → enter amount → Next.
  4. On confirm page, tap Tax amount, enter a large amount over the limit, save.
  5. Verify the over-limit error shows and the amount cannot be saved.
  6. Verify a valid tax amount (including 0) still saves correctly.
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.

Screenshots/Videos

N/A — logic-only fix. See Tests/QA for verification steps.

MelvinBot and others added 2 commits June 11, 2026 06:01
Co-authored-by: huult <huult@users.noreply.github.com>
Co-authored-by: huult <huult@users.noreply.github.com>
@MelvinBot
MelvinBot requested a review from a team June 11, 2026 06:06
@MelvinBot

Copy link
Copy Markdown
Contributor Author

I created this PR from an upstream branch since I don't have push access to your fork (huult/App).

To take ownership of this branch and be able to push updates, run:

git remote add upstream https://github.com/Expensify/App.git
git fetch upstream claude-taxAmountFormRouteCheck
git checkout -b claude-taxAmountFormRouteCheck upstream/claude-taxAmountFormRouteCheck
git push -u origin claude-taxAmountFormRouteCheck

Then you can close this PR and open a new one from your fork.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

The failing ESLint check is unrelated to this PR — it's a pre-existing baseline drift on main

Root cause: The ESLint failure is an eslint-seatbelt violation in tests/unit/ReportSecondaryActionUtilsTest.ts — a file this PR does not touch:

[eslint-seatbelt]: There are 466 errors of this type, but only 450 are allowed.
@typescript-eslint/no-unsafe-type-assertion

The seatbelt baseline at config/eslint/eslint.seatbelt.tsv:1821 allows 450, but the file now contains 466 of these errors. The baseline for that line was set on 2026-06-05, and the test file was modified by several PRs afterward (most recently Expensify/App#91753 on 2026-06-10) that raised the actual count without bumping the allowed baseline.

Why it's not this PR: This PR only changes src/CONST/index.ts (adds a string constant), src/pages/iou/MoneyRequestAmountForm.tsx, and a new test — none of which touch ReportSecondaryActionUtilsTest.ts or add type assertions. The same ESLint failure with the identical 466 / 450 count is currently hitting other unrelated open PRs (e.g. Expensify/App#93247, Expensify/App#93246, Expensify/App#93245), confirming it's a main-wide drift rather than a regression introduced here.

What's needed: A separate PR should reconcile the seatbelt baseline for that file (or fix the underlying type assertions). I'm intentionally not bumping the baseline here, since that's an unrelated change that shouldn't ride along on this PR.

The other failing check

PR Author Checklist / checklist is failing only because the author checklist in the PR description hasn't been completed yet — huult should fill that out. It's not a code issue.

@MelvinBot

Copy link
Copy Markdown
Contributor Author

⚠️ Something went wrong. This may be a transient issue — try again. If it persists, check status.anthropic.com.

Run details

@huult

huult commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot please close this PR

@huult

huult commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot Please close this PR

1 similar comment
@huult

huult commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot Please close this PR

@MelvinBot

Copy link
Copy Markdown
Contributor Author

🤖 huult requested closing this PR. I attempted to close it but the close action requires manual approval in my environment. huult, please close it directly, or re-confirm and I'll retry.

@huult

huult commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants