[No QA] update previousTaxCode in setPolicyTaxCode successData#94286
Conversation
melvin-bot
Bot
requested review from
dmkt9,
heyjennahay and
truph01
and removed request for
a team
![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: 996e18e9ce
ℹ️ 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".
| pendingFields: {...originalTaxRate.pendingFields, code: null}, | ||
| pendingAction: null, | ||
| errorFields: {code: null}, | ||
| previousTaxCode: oldTaxCode, |
There was a problem hiding this comment.
Keeping previousTaxCode in the confirmed Onyx state makes the old code a permanent alias for this rate. If the user later reuses that old code for another tax, validateTaxCode/isExistingTaxCode only reject existing object keys, and the old key was removed; then WorkspaceEditTaxPage calls getCurrentTaxID(), which can return this renamed rate via previousTaxCode before it reaches the real reused-code key. In that scenario the edit route for the newly reused code redirects to this renamed tax, so the new tax cannot be edited or deleted correctly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I introduced optimisticPreviousTaxCode to fix this issue cc @truph01
| pendingFields: {...originalTaxRate.pendingFields, code: null}, | ||
| pendingAction: null, | ||
| errorFields: {code: null}, | ||
| previousTaxCode: oldTaxCode, |
There was a problem hiding this comment.
@dukenv0307 I think we should update the previousTaxCode value for the policy in snapshot_ as well.
There was a problem hiding this comment.
@dmkt9 Do you mean update it optimistically or in BE?
There was a problem hiding this comment.
Yes, I mean we should update both.
There was a problem hiding this comment.
Can you give me the test steps to reproduce the bug (if we don't update snapshot)? I'll update it in the follow-up PR
There was a problem hiding this comment.
Sure. You can use my PR (#94007) for testing. Please refer to Test 4:
- Create a workspace.
- Enable Taxes and navigate to the Taxes page.
- Create two new tax rates (e.g., Tax A and Tax B).
- Open Tax A.
- Verify that the Tax Code field is empty.
- Click the Tax Code field, enter any value, and save the changes.
- Verify that the Tax Code field now contains a value.
- Create a couple of expenses using Tax A and Tax B.
- Navigate to Spend > Expenses.
- Enable the Tax Code column.
- Note that the Tax Code column displays empty values for those expenses
Expected Result:
Verify that the Tax Code column displays a value for expenses using Tax A, and is empty for expenses using Tax B.
There was a problem hiding this comment.
@MelvinBot can you please create the new issue in E/E repo?
There was a problem hiding this comment.
@dmkt9 I can't reproduce, can you please check again? previousTaxCode is already in snapshots
Screen.Recording.2026-06-30.at.4.25.05.PM.mov
There was a problem hiding this comment.
@dmkt9 I can't reproduce, can you please check again? previousTaxCode is already in snapshots
Screen.Recording.2026-06-30.at.4.25.05.PM.mov
Ah, thanks! It works well for me.
|
Kindly bump @truph01 |
|
Reviewing... |
|
@truph01 done |
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.
|
|
@truph01 bump |
|
@dukenv0307 I am still seeing the default tax code ID is shown in tax code column: 
Could you help check? |
|
@dukenv0307 Also, could you update the "Explanation of Change" section? I am assuming that your PR aims to fix: #94003 (comment) |
|
@truph01 I updated the PR description
No, I just made sure the |
@truph01 I think this PR only adds previousTaxCode |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-06.at.15.44.48.mov |
@dukenv0307 was any issue ever created? |
|
Yes — that issue was already created, so no new one is needed. In response to I won't open a duplicate. If you'd like the internal issue reference, I can share it internally (it's in a private repo, so I'm not linking it here on the public PR). |
|
@MonilBhavsar yes, but I closed bc we couldn't reproduce |
|
Nice, thanks 👍 |
MonilBhavsar
left a comment
MonilBhavsar
left a comment
There was a problem hiding this comment.
previousTaxCode is no longer used? Can we remove it?
Also mind adding QA steps please
dukenv0307
changed the title
|
@MonilBhavsar I think we don't need QA tests in this PR
We still need it in #94007 |
|
🚧 MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.4.32-0 🚀
|
|
🤖 Help site review: no changes required. I reviewed the changes in this PR and no updates to Expensify's help site files under Why: This is an internal state-management/data-correctness fix. It introduces an
There is no user-facing behavior change. Editing a tax rate's name, code, or rate works exactly as before from the user's perspective; only the underlying data bookkeeping is corrected. Help articles describe how to add/edit tax rates, not the internal Onyx fields, so no article is affected. No draft docs PR was created. @dukenv0307, please confirm you agree no help site changes are needed here. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.32-3 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
Currently, we are using
in successData, so
previousTaxCodewill be overridden to the outdated data.In this PR, I update
previousTaxCodeto the correct value.Fixed Issues
$ #94003
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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