Fix: Tax name missing and tax unselected after changing tax code - #92750
Conversation
…ate tax calculations
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
This PR also fixes the regression reported in #92657. What we missed was that the renamed tax code was only being resolved in the display/selection paths. Some submit/update flows were still sending the old taxCode, so after an offline submit it could fall back to the default/0% tax rate. I updated those submit/share/split/merge/inline-edit paths to resolve the tax code against the current policy tax map before sending it. REC-20260605113531.mp4 |
|
@Krishna2323 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] |
| return !!tags && Object.values(tags).some((tag) => !!tag.enabled); | ||
| } | ||
|
|
||
| function resolveCurrentTaxCodeFromTaxRates(policyTaxRates: TaxRates, taxCode: string): string | undefined { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The new resolveCurrentTaxCodeFromTaxRates function duplicates the logic already present in getCurrentTaxID from PolicyUtils.ts, which is even imported in this same file. Both functions check if a tax code exists directly in the tax rates, and if not, search for a matching previousTaxCode. The only difference is the function signature (one takes TaxRates directly, the other takes Policy and extracts policy?.taxRates?.taxes).
Remove resolveCurrentTaxCodeFromTaxRates and use the already-imported getCurrentTaxID (or resolveCurrentTaxCode) instead. For example, at line 355:
// Instead of:
const currentTaxCode = resolveCurrentTaxCodeFromTaxRates(policyTaxRates, taxCode) ?? taxCode;
// Use the already-imported function:
const currentTaxCode = getCurrentTaxID({taxRates: {taxes: policyTaxRates}} as Policy, taxCode) ?? taxCode;Or better yet, refactor the call site to pass the policy object and use resolveCurrentTaxCode(policy, taxCode) directly.
Reviewed at: a6ec3e5 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5196fe23df
ℹ️ 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".
| [newTaxCode]: { | ||
| ...originalTaxRate, | ||
| code: newTaxCode, | ||
| previousTaxCode: oldTaxCode, |
There was a problem hiding this comment.
When a tax code that was already renamed is renamed again (for example A -> B -> C), this overwrites originalTaxRate.previousTaxCode with only B. The new resolution helper only checks one previousTaxCode, so historical expenses still storing A stop resolving after the second rename and the missing-name/unselected-tax behavior returns for those older expenses. Preserve the prior alias (or otherwise resolve the chain) when writing the renamed tax rate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
When a tax code that was already renamed is renamed again (for example A -> B -> C), this overwrites originalTaxRate.previousTaxCode with only B
What I found is that the expense can still store the original tax code, while the current tax rate only keeps the latest previousTaxCode. With only one alias, the frontend can’t reliably resolve the full rename chain.
The only complete fix I found is to preserve/return the tax-code alias history from the backend, then have the frontend resolve old transaction tax codes against that history. So this needs a backend change as well.
@Krishna2323 Can you please check and confirm?
There was a problem hiding this comment.
@jasperhuangg what do you think about this? I think we should only fix the reported bug for now and only store the last edited value instead of full chain.
There was a problem hiding this comment.
Ah I see, I think there's a gap in the back-end changes. We should store all previous tax codes and also emit previous tax codes whenever loading the full policy JSON. Let me see about the back-end changes, sorry for the back and forth.
There was a problem hiding this comment.
Thanks @jasperhuangg! One thing to consider on the backend: when storing previous tax codes, we should make sure that if an admin creates a new tax that reuses an old code (e.g., creates a new tax with code A after A was previously renamed to B), the backend removes A from B's previous codes list.
Otherwise, two taxes could end up claiming the same previous code, which would make the resolution ambiguous.
There was a problem hiding this comment.
Ah, sorry for missing this. The backend PR is currently still under review. Thanks for your patience.
There was a problem hiding this comment.
https://github.com/Expensify/Auth/pull/22394 for posterity's sake
|
Thanks for the v2 @marufsharifi. The submit/action paths look good now. I found a few remaining places in the duplicate transaction flow that still seem to use
Could you please take a look at those as well? |
|
While testing I also found this bug: When change the category of an expense the tax rate changes to default ( 0%) REC-20260609115449.mp4Which is also reproducible in staging. @Krishna2323 Should I also consider this or is out of scope for this PR ? |
It's not in scope for this PR. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMonosnap.screencast.2026-06-11.19-18-09.mp4 |
|
@marufsharifi please resolve conflicts. |
|
@MelvinBot review |
|
🤖 Review — looks solid. The rename-resolution approach is consistent and well-tested. No blocking issues found. A few notes below. What the PR does well
Verification of earlier review threads
Minor, non-blocking
I did not re-run the manual e2e flow (renaming a tax code requires a tax-enabled/upgraded workspace to set up); the author's and reviewer's recorded videos already demonstrate the tax name persisting, and the resolver logic is covered by the new unit tests. |
jasperhuangg
left a comment
There was a problem hiding this comment.
code changes look good @joekaufmanexpensify onto you for the product review, thanks!
|
@Krishna2323 Could you please check this bug : #92750 (comment) |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product.
d976f42
|
@Krishna2323 gentle bump on this comment, thanks |
@jasperhuangg what do you think about this? Should we fix it in this PR? |
|
Let's move forward without fixing this for now, since the PR has been open for a couple of months. We can follow up separately if needed. |
Agree, we can fix that in follow up. thanks |
|
apologies, was OOO, agree we can fix that in a follow-up @marufsharifi please fix the conflicts! |
|
@jasperhuangg main is merged now |
|
🚧 jasperhuangg 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/jasperhuangg in version: 9.4.57-0 🚀
|
|
🤖 Help site review: no changes required. This PR is an internal bug fix — it corrects how the app resolves a renamed tax code so historical expenses show the right tax name, keep the tax picker selected, and stop showing false tax out-of-policy violations. It adds a It does not add or change any user-facing feature, page, tab, setting, or button. The relevant help article already documents editing a tax rate's Name, Value, and Tax Code ( If you'd like a proactive note added anywhere (e.g. clarifying that historical expenses stay accurate after renaming a tax code), let me know and I'll draft one. |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.4.57-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
When a workspace admin renames a tax code, older expenses still store the old tax ID. This update makes the app resolve renamed tax codes before displaying tax names or validating tax rules. As a result, tax names show correctly, the tax picker stays selected, and tax out‑of‑policy violations no longer appear incorrectly for historical expenses.
Fixed Issues
$ #83111
PROPOSAL: #83111 (comment)
Tests
→ Verify in tax rate section that the tax name is displayed
Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Screen.Recording.2026-03-08.at.10.00.08.AM.mov
Android: mWeb Chrome
Screen.Recording.2026-03-08.at.10.06.27.AM.mov
iOS: Native
iosnative.mp4
iOS: mWeb Safari
Screen.Recording.2026-03-08.at.10.18.55.AM.mov
MacOS: Chrome / Safari
macchrome.mp4