Fix tax field showing wrong rate after cross-workspace merge#86178
Fix tax field showing wrong rate after cross-workspace merge#86178
Conversation
…aces When the same tax code exists in multiple workspaces but refers to different rates, the policy lookup returns the wrong rate name. Add a percentage-based mismatch check to detect cross-workspace tax code collisions and fall back to showing the raw tax value instead. Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| const categoryCopyValue = !canEdit ? decodedCategoryName : undefined; | ||
| const cardCopyValue = cardProgramName; | ||
| const taxRateValue = transaction?.taxName ?? taxRateTitle ?? fallbackTaxRateTitle; | ||
| const taxRateValue = hasConfirmedTaxMismatch ? fallbackTaxRateTitle : (transaction?.taxName ?? taxRateTitle ?? fallbackTaxRateTitle); |
There was a problem hiding this comment.
@MelvinBot do we need to change in this line? I think we can keep as it is
There was a problem hiding this comment.
Good question! The change on this line is needed — setting taxRateTitle to undefined alone isn't sufficient because of the fallback chain priority.
The original code is:
const taxRateValue = transaction?.taxName ?? taxRateTitle ?? fallbackTaxRateTitle;transaction?.taxName is checked before taxRateTitle. Since taxName is a stored field on the transaction (set when the transaction was created in the original workspace), it doesn't update when the transaction moves to a different workspace. So even though we correctly set taxRateTitle = undefined when mismatch is detected, transaction?.taxName still holds the stale name from the source workspace and would be displayed first.
For example:
- Transaction created in Workspace B with
taxName = "Custom Tax (3%)",taxCode = "id_TAX_RATE_1" - After merge/move to Workspace A,
transaction.taxNameis still"Custom Tax (3%)" - In the fallback chain,
transaction?.taxNameis truthy → it wins, even thoughtaxRateTitlewas set toundefined
The fix bypasses transaction?.taxName entirely when mismatch is confirmed, falling back to fallbackTaxRateTitle (transaction?.taxValue, the raw percentage like "3%") which is always accurate.
Evidence: taxName is stored on the transaction type (Transaction.ts:462-463), and in merge flows it's set from the source policy (MergeTransactionUtils.ts:631), not the destination.
| const defaultTaxCode = getDefaultTaxCode(policy, transaction); | ||
| const taxRatePercentage = getTaxValue(policy, transaction, transaction?.taxCode ?? defaultTaxCode ?? ''); | ||
| const hasConfirmedTaxMismatch = taxRatePercentage !== undefined && transaction?.taxValue !== undefined && taxRatePercentage !== transaction?.taxValue; |
There was a problem hiding this comment.
@MelvinBot I think we can put this one into a shareable util and add unit test for this one. Wdyt>
There was a problem hiding this comment.
Good idea! Done — I've extracted the mismatch detection logic into a hasConfirmedTaxMismatch(policy, transaction) utility in TransactionUtils/index.ts and added 7 unit tests covering:
- Mismatch detected (different percentages for same tax code)
- No mismatch (matching percentages)
undefinedtransactiontaxValue(graceful false)- Non-existent tax code in policy (graceful false)
undefinedtransaction or policy (graceful false)- Default tax code fallback when transaction has no
taxCode
MoneyRequestView.tsx now imports and calls the util directly instead of inlining the logic.
Move the cross-workspace tax mismatch detection logic from MoneyRequestView into TransactionUtils as a reusable hasConfirmedTaxMismatch function, and add comprehensive unit tests covering match, mismatch, undefined, and default tax code scenarios. Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
| * This happens in cross-workspace merge/move flows where the same tax code (e.g., `id_TAX_RATE_1`) maps to | ||
| * different rates in different workspaces. Returns true only when both values are defined and they differ. | ||
| */ | ||
| function hasConfirmedTaxMismatch(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transaction>): boolean { |
There was a problem hiding this comment.
@MelvinBot The method name is confusing to me. Do you have any alternative names? What do you think of hasTaxValueMismatch?
There was a problem hiding this comment.
Good call — hasTaxValueMismatch is cleaner and more descriptive. Renamed it across all three files (TransactionUtils, MoneyRequestView, and the tests) in 8f63254.
Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
@codex review |
|
@MelvinBot can you resolve conflict on this PR? |
…heck Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
Resolved the merge conflict in The utility function and tests added in earlier commits still exist in |
![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: 8f63254404
ℹ️ 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".
| */ | ||
| function hasTaxValueMismatch(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transaction>): boolean { | ||
| const defaultTaxCode = getDefaultTaxCode(policy, transaction); | ||
| const taxRatePercentage = getTaxValue(policy, transaction, transaction?.taxCode ?? defaultTaxCode ?? ''); |
There was a problem hiding this comment.
The mismatch check uses transaction?.taxCode ?? defaultTaxCode, so an empty-string taxCode is treated as a real code instead of falling back to the policy default. This diverges from getTaxName() (which explicitly treats empty string as missing), and causes hasTaxValueMismatch() to return false when the transaction has taxCode: '' but a taxValue from another workspace. In that case, MoneyRequestView can still show the current workspace tax name instead of the raw percentage, so the cross-workspace fix is bypassed for this input shape.
Useful? React with 👍 / 👎.
2 participants
Explanation of Change
When the same tax code (e.g.,
id_TAX_RATE_1) exists in multiple workspaces but refers to different tax rates,MoneyRequestViewresolves the name from the current policy, which returns the wrong rate. This happens when merging expenses across workspaces or moving an expense to a different workspace's report.This PR adds a percentage-based mismatch check: before displaying the policy-resolved tax name, we compare the policy rate's percentage with the transaction's stored
taxValue. If both are defined and they differ, we know the tax code refers to a different rate in the current workspace, so we fall back to showing the raw tax percentage instead of a misleading rate name.Fixed Issues
$ #85164
PROPOSAL: #85164 (comment)
Tests
Alternative test (change report flow):
Offline tests
No change to offline behavior — this fix only affects how the tax rate name is resolved for display.
QA Steps
Same as tests above.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari