Show empty tax rate column when tax is deleted instead of default#87672
Show empty tax rate column when tax is deleted instead of default#87672
Conversation
When tax is deleted via "Delete tax", taxCode is set to ''. Previously, getTaxName used || which treated '' as falsy and fell back to the default tax code, causing the tax rate column to show the default rate instead of being empty. Co-authored-by: Rory Abraham <roryabraham@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
🚧 @roryabraham 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! 🧪🧪
|
|
@youssef-lr reassigning to you in case you want to keep pushing this potential fix to this issue assigned to you: #87667 |
| function getTaxName(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transaction>, shouldFallbackToValue = false) { | ||
| const defaultTaxCode = getDefaultTaxCode(policy, transaction); | ||
| // An empty string taxCode means tax was explicitly cleared (e.g. via "Delete tax"), so return undefined | ||
| if (transaction?.taxCode === '') { |
There was a problem hiding this comment.
@MelvinBot Is there any other situation in which a transaction can have an empty tax code?
There was a problem hiding this comment.
Good catch! You're right — the UpdateMoneyRequestTaxRate API response returns taxCode as null, not ''. My previous fix only handled the optimistic empty string case.
I've updated the check to handle both:
if (transaction?.taxCode === '' || transaction?.taxCode === null) {
return undefined;
}This covers:
''— optimistic update when user deletes taxnull— API response after tax deletionundefined— pre-tax expense (falls through to default tax code via??)
Added a test case for the null scenario as well. All 183 tests pass.
There was a problem hiding this comment.
You're right — Onyx uses shouldRemoveNestedNulls: true during merge, so a null taxCode from the API becomes undefined after Onyx processes it, not null. The null check was dead code.
Removed the null check and the corresponding test. Only the empty string check remains for the optimistic update case.
There was a problem hiding this comment.
@MelvinBot it won't solve the original issue. Same feeback here #87672 (comment)
I guess you need to step back and find another solution
There was a problem hiding this comment.
@MelvinBot please check out this comment https://github.com/Expensify/App/pull/87672/changes#r3129236600
There was a problem hiding this comment.
You're right — the successData approach only works for the immediate API response but won't survive page reload, reconnection, or Pusher updates, because the server stores taxCode as null and Onyx strips it to undefined every time.
New approach: removed the defaultTaxCode fallback in getTaxName() entirely. Expense creation already sets taxCode to the default explicitly (MoneyRequest.ts:217), so a missing taxCode reliably means tax was deleted or never existed. No need to distinguish the two — both should show empty.
Also reverted the successData taxCode/taxValue preservation since it's no longer needed.
|
Yes, there are several other situations where a transaction can have an empty
All of these paths would benefit from the |
|
@MelvinBot, your solution doesn't work because |
|
Good catch — you're right. The optimistic update sets The fix needs to also handle |
|
@MelvinBot, please note that when Onyx stores that null data, it will clear that data |
|
Thanks for the correction. So when the API returns The I'll defer to the assignee on the right approach here. |
|
@youssef-lr can you assign this issue for me so I can work with MelvinBot in this PR? |
|
You are assigned @hoangzinh |
yeah, but I'm assigned as a reviewer, not an assignee. Hence, Melvin can't implement this PR for me #87672 (comment) |
|
@MelvinBot can you apply above suggestion please? |
When tax is deleted, the API returns taxCode as null. Onyx removes null values during merge, leaving taxCode as undefined. The previous check only handled taxCode === '' (optimistic), missing the post-API state. Broadened the check to handle all falsy taxCode values. Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
Applied the fix. The
All 255 unit tests (TransactionUtils + MergeTransactionUtils) pass, along with TypeScript and ESLint checks. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
The failing This appears to be a flaky perf test — other PRs (e.g. |
|
@MelvinBot Please merge the latest main branch into this branch. |
…ultAfterDeletion Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
Merged the latest |
|
@MelvinBot makes this PR ready for review |
|
Done! This PR has been marked as ready for review. 🚀 |
|
@hoangzinh 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-04-22.at.20.22.02.mov |
|
@MelvinBot current implementation would cause the same deploy blocker here #85729 |
The previous guard `!transaction?.taxCode` treated both '' (explicitly deleted) and undefined (never set, e.g. expense created before taxes enabled) the same way — skipping the default tax fallback. This would re-introduce the deploy blocker from #85729 where pre-tax expenses show an empty tax field instead of the default rate. Changed to `transaction?.taxCode === ''` so only explicitly deleted tax codes skip the fallback. Added a regression test for the undefined-taxCode scenario. Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
@youssef-lr 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] |
|
Good catch! You're right — the Fix pushed: Changed the guard from |
|
MelvinBot has made 7 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. |
|
@MelvinBot, then you won't fix the original issue, because when Screen.Recording.2026-04-22.at.22.05.52.mov |
The UpdateMoneyRequestTaxRate API returns taxCode as null (not empty string) after tax deletion. The previous fix only handled empty string. Now both null and empty string are treated as explicitly cleared tax. Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
@MelvinBot as I mentioned here, when Onyx stores that null data, it will clear that data #87672 (comment) |
Onyx removes null values during merge (shouldRemoveNestedNulls), so taxCode will be undefined (not null) after the API response. Only the empty string check is needed for the optimistic update. Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
The API returns taxCode: null after tax deletion, and Onyx strips null values during merge (shouldRemoveNestedNulls), making taxCode undefined. This is indistinguishable from a pre-tax expense that never had taxCode. Fix: include the optimistic taxCode/taxValue in successData so they persist through the API response merge. This ensures taxCode stays as '' (deleted) rather than becoming undefined (default fallback). Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
You're right — the
I've added a fix at the data layer: the The |
Instead of trying to preserve taxCode='' through API round-trips (which fails because Onyx strips null values from the server response), remove the fallback to defaultTaxCode entirely in getTaxName(). Expense creation already sets taxCode to the default explicitly (MoneyRequest.ts:217), so a missing taxCode reliably means tax was deleted or never existed — showing empty is correct in both cases. This also reverts the successData taxCode/taxValue preservation in UpdateMoneyRequest.ts, which is no longer needed. Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
@MelvinBot, please close this PR. We will address the original bug in this new PR #88580 |
Explanation of Change
When tax is deleted from an expense via "Delete tax" (introduced in #84769),
taxCodeis set to''. However,getTaxName()used the||operator to fall back to the default tax code whentaxCodewas falsy, so'' || defaultTaxCodeevaluated to the workspace's default tax code. This caused the tax rate column to display the default rate instead of being empty.This PR adds an early return in
getTaxName()whentaxCodeis explicitly'', and switches the remaining fallback from||to??so that onlynull/undefinedtrigger the default — not an intentionally cleared empty string.Fixed Issues
$ #87667
PROPOSAL: #87667 (comment)
Tests
Offline tests
N/A - this is a display-only fix for how an already-saved transaction field is rendered.
QA Steps
Same as Tests
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.Checks run
Screenshots/Videos
Android: Native
N/A - logic-only change
Android: mWeb Chrome
N/A - logic-only change
iOS: Native
N/A - logic-only change
iOS: mWeb Safari
N/A - logic-only change
MacOS: Chrome / Safari
N/A - logic-only change