Fix: Tax name missing and tax unselected after changing tax code#92750
Fix: Tax name missing and tax unselected after changing tax code#92750marufsharifi wants to merge 6 commits into
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 |
melvin-bot
Bot
requested review from
Krishna2323 and
joekaufmanexpensify
and removed request for
a team
|
@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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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 |
![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: 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?
|
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 ? |
3 participants
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
#92657
PROPOSAL: #83111 (comment)
Tests
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