Fix category tax value not syncing on expense creation#88477
Conversation
When selecting a category with a default tax rate, getCategoryTaxCodeAndAmount updates taxCode and taxAmount but not taxValue. This mismatch causes the tax field to display "0%" and triggers "Tax no longer valid" on submit. Return categoryTaxValue from getCategoryTaxCodeAndAmount and use setMoneyRequestTaxRateValues to set all three fields atomically. Co-authored-by: Fitsum Abebe <FitseTLT@users.noreply.github.com>
|
@MelvinBot merge main |
…ValueSync Co-authored-by: Fitsum Abebe <FitseTLT@users.noreply.github.com>
|
Merged |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chrome2026-04-28.16-40-23.mp4iOS: HybridApp2026-04-28.17-03-15.mp4iOS: mWeb Safari2026-04-28.16-38-38.mp4MacOS: Chrome / Safari2026-04-28.16-26-14.mp4 |
|
@MelvinBot make it ready for review |
|
PR is now marked as ready for review. |
| } | ||
|
|
||
| return {categoryTaxCode, categoryTaxAmount}; | ||
| return {categoryTaxCode, categoryTaxAmount, categoryTaxValue: categoryTaxPercentage}; |
There was a problem hiding this comment.
Now that we added categoryTaxValue on the return of getCategoryTaxCodeAndAmount I think we should change its name @MelvinBot
There was a problem hiding this comment.
Renamed to getCategoryTaxDetails across all call sites (TransactionUtils, IOU, and tests).
| if (categoryTaxCode && categoryTaxAmount !== undefined) { | ||
| setMoneyRequestTaxRate(transactionID, categoryTaxCode); | ||
| setMoneyRequestTaxAmount(transactionID, categoryTaxAmount); | ||
| setMoneyRequestTaxRateValues(transactionID, {taxCode: categoryTaxCode, taxAmount: categoryTaxAmount, taxValue: categoryTaxValue ?? null}); |
There was a problem hiding this comment.
U can replace the default value of null with checking for the existence of the value. @MelvinBot
There was a problem hiding this comment.
Updated to use an existence check: taxValue: categoryTaxValue ? categoryTaxValue : null. A conditional spread wasn't possible here because the TaxRateValues type requires taxValue to always be present.
There was a problem hiding this comment.
Nope what I meant is you can add a check categoryTaxValue in the if to avoid giving default null
@MelvinBot
There was a problem hiding this comment.
Done — added categoryTaxValue to the if condition so we no longer need the ?? null fallback.
| if (categoryTaxCode && categoryTaxAmount !== undefined) { | ||
| updatedTransaction.taxCode = categoryTaxCode; | ||
| updatedTransaction.taxAmount = categoryTaxAmount; | ||
| if (categoryTaxValue) { |
There was a problem hiding this comment.
Moved the if (categoryTaxValue) block above the if (categoryTaxCode && categoryTaxAmount !== undefined) block so taxValue is set independently.
…tence check for taxValue, move taxValue condition above Co-authored-by: Fitsum Abebe <FitseTLT@users.noreply.github.com>
Co-authored-by: Fitsum Abebe <FitseTLT@users.noreply.github.com>
|
Fixed the failing ESLint check: replaced |
| updatedTransaction.category = transactionChanges.category; | ||
| const {categoryTaxCode, categoryTaxAmount} = getCategoryTaxCodeAndAmount(transactionChanges.category, transaction, policy); | ||
| const {categoryTaxCode, categoryTaxAmount, categoryTaxValue} = getCategoryTaxDetails(transactionChanges.category, transaction, policy); | ||
| if (categoryTaxValue) { |
There was a problem hiding this comment.
u should take the assignment in the below if @MelvinBot
There was a problem hiding this comment.
Done — moved the taxValue assignment into the existing if (categoryTaxCode && categoryTaxAmount !== undefined) block and added categoryTaxValue to the condition.
|
@MelvinBot updated tests Precondition Open categories, click the Advertising push row, set the Default tax rate to a non-default tax rate from the workspace |
|
Merged |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Fix makes sense to me!
|
@MelvinBot merge latest main |
…ValueSync Co-authored-by: Fitsum Abebe <FitseTLT@users.noreply.github.com>
|
Merged latest main into this branch (b612b6c). |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Cover the new categoryTaxValue return property in all existing getCategoryTaxDetails and getUpdatedTransaction test cases to address the Codecov coverage gap. Co-authored-by: Fitsum Abebe <FitseTLT@users.noreply.github.com>
|
Added |
|
@MelvinBot complete your checklist |
|
Completed the PR Author Checklist. Items left unchecked require manual verification (platform testing, screenshots, offline testing, high-traffic account testing, console errors). |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #86473 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@Gonals while testing this pr I found a couple of BE bugs some of them might make this pr look it caused a regression because they are surfaced by it but in my opinion this pr can be merged as the behavior is not worse than main and then the BE fix can be made. U might find bc-contributors or you fix it. As you like. 👍 BUGS: Precondition:
The tax rate should update to the linked rate when category is changed to Advertising but now it is changed to the default tax rate of the workspace instead.
The tax rate will change to Also a question if this is a bug: when you create a receipt expense selecting a category that has linked tax rate in rules |
Can you report these bugs on Slack? |
|
🚧 @Gonals 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/Gonals in version: 9.3.66-0 🚀
|
|
No help site changes required. This PR is an internal bug fix that corrects how No user-facing behavior, terminology, settings, or workflows changed — the category default tax rate feature already existed and is already working as documented. This fix just resolves a sync issue between internal tax fields ( |
|
Deploy Blocker #89541 was identified to be related to this PR. |
Explanation of Change
When selecting a category with a default tax rate,
getCategoryTaxCodeAndAmountcomputes and setstaxCodeandtaxAmountbut never setstaxValue. This creates a mismatch wheretaxCodepoints to the correct rate (e.g. 5%) buttaxValueretains its stale value (e.g. "0%").This causes:
hasTaxRateWithMatchingValuereturns false (staletaxValuevs actual rate)TaxController/confirmation list resets tax to policy defaultFix: Return
categoryTaxValuefromgetCategoryTaxCodeAndAmountand usesetMoneyRequestTaxRateValuesto settaxCode,taxValue, andtaxAmountatomically. Also apply the same fix ingetUpdatedTransactionfor the edit-expense flow.Fixed Issues
$ #86473
PROPOSAL: #86473 (comment)
Tests
Precondition: A control workspace with Rules and Taxes enabled with additional non-default tax rate
Offline tests
N/A — this change only affects how tax values are set when selecting a category. No new offline-specific behavior is introduced.
QA Steps
Precondition: A control workspace with Rules and Taxes enabled with additional non-default tax rate
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop