Fix duplicate tax handling to preserve original tax data when unchanged#88125
Fix duplicate tax handling to preserve original tax data when unchanged#88125
Conversation
When resolving duplicates, stop sending taxCode/taxAmount/taxValue if the tax code hasn't changed or was deleted. This prevents overwriting existing tax data with empty values, which caused empty tax rate rows and incorrect tax amounts after confirming duplicate resolution. Co-authored-by: dmkt9 <[email protected]>
|
@codex review |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![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: b7161465a0
ℹ️ 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".
| // Preserve taxAmount and taxValue if taxCode is deleted or remains unchanged compared to duplicatedTransaction?.taxCode. | ||
| if (!taxRate || (taxCode && duplicatedTransaction?.taxCode === taxCode) || reviewDuplicates?.taxAmount === undefined) { | ||
| return; |
There was a problem hiding this comment.
The new guard in taxData treats a missing taxRate as a reason to skip sending tax updates, but taxRate is also undefined while policy data is still loading. In that state, selecting a different tax code is dropped: taxData becomes undefined, buildMergeDuplicatesParams() no longer includes taxCode, and mergeDuplicates/resolveDuplicates fall back to the original transaction tax fields. This makes the user’s explicit tax-code choice silently not apply on confirm when the policy cache is not yet hydrated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I believe this is acceptable because the policy is fully retrieved from the server when the report is opened. Additionally, on the confirmation page, the user must click "Confirm" to apply changes, and the policy will have been loaded beforehand.
|
@MelvinBot, apply this patch to preserve the taxAmount when the tax code remains unchanged. diff --git a/src/pages/TransactionDuplicate/ReviewTaxCode.tsx b/src/pages/TransactionDuplicate/ReviewTaxCode.tsx
index 96dd7a8405e..aea66a5e68a 100644
--- a/src/pages/TransactionDuplicate/ReviewTaxCode.tsx
+++ b/src/pages/TransactionDuplicate/ReviewTaxCode.tsx
@@ -64,6 +64,10 @@ function ReviewTaxRate() {
);
const getTaxAmount = useCallback(
(taxID: string) => {
+ // If the tax code remains unchanged, preserve the tax amount to avoid resetting it to the default value when resolving duplicates.
+ if (taxID === transaction?.taxCode) {
+ return;
+ }
const taxPercentage = getTaxValue(policy, transaction, taxID);
const decimals = getCurrencyDecimals(transaction?.currency);
return convertToBackendAmount(calculateTaxAmount(taxPercentage ?? '', getAmount(transaction), decimals));
|
…tion Co-authored-by: dmkt9 <[email protected]>
|
Applied the patch to preserve |
|
@MelvinBot Update the QA steps with the following: **Test 1:**
_Precondition:_ Enable Taxes in workspace settings.
1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Create an expense with 0% tax rate.
4. Create another expense with the same amount and 5% tax rate.
5. Go to workspace settings > Taxes.
6. Delete 5% tax rate.
7. Open the expense report.
8. Click Review duplicates.
9. Click Keep this one on expense with 5% tax rate.
10. Proceed to confirm page > click Confirm.
11. Open the expense.
12. Click Tax rate.
13. Note that the tax rate will show the disabled tax rate as selected.
**Test 2:**
_Precondition:_ Enable Taxes in workspace settings.
1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Create two expenses with same amount, same tax rate but different tax amount.
4. Open the expense report.
5. Click Review duplicates.
6. Click Keep this one on any expense.
7. Proceed to confirm page.
→ Confirm page shows the expense tax amount instead of the default tax amount.
8. Click Confirm.
9. Open the expense.
10. Verify that the original tax amount will be preserved (similar to tax amount on confirm page in Step 7). |
|
Updated the QA Steps section in the PR description with the provided test steps. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariTest1.mp4Test2.mp4 |
|
@MelvinBot I have tested this PR. Please complete your checklist. |
|
@codex review |
|
Updated the PR Author Checklist. Checked off all N/A conditional items (no new files, no CSS, no assets, no UI changes, no new pages, no deeplinks, no Storybook, no generic components, no message editing code) and the code quality items I can verify from the diff. Still needs your input before marking ready for review:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2e683f1c7
ℹ️ 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".
| if (taxID === transaction?.taxCode) { | ||
| return; |
There was a problem hiding this comment.
The new early return in getTaxAmount treats a tax choice as “unchanged” by comparing taxID to transaction?.taxCode, but transaction is read from route.params.threadReportID (the current thread) rather than the duplicate selected with “Keep this one”. In that flow, choosing a tax code that matches the original thread (but differs from the selected duplicate) now returns undefined here, so reviewDuplicates.taxAmount stays undefined and Confirmation omits taxData, causing merge/resolve to preserve the selected duplicate’s old tax fields instead of the user’s choice.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot apply this patch:
patch
diff --git a/src/pages/TransactionDuplicate/ReviewTaxCode.tsx b/src/pages/TransactionDuplicate/ReviewTaxCode.tsx
index 492c447bfd1..bcca68cbd62 100644
--- a/src/pages/TransactionDuplicate/ReviewTaxCode.tsx
+++ b/src/pages/TransactionDuplicate/ReviewTaxCode.tsx
@@ -31,6 +31,7 @@ function ReviewTaxRate() {
const [transactionThreadReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${route.params.threadReportID}`);
const transactionID = getTransactionID(transactionThreadReport);
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${getNonEmptyStringOnyxID(transactionID)}`);
+ const [duplicatedTransaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${getNonEmptyStringOnyxID(reviewDuplicates?.transactionID)}`);
const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`);
const allDuplicateIDs = useMemo(
() => transactionViolations?.find((violation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION)?.data?.duplicates ?? [],
@@ -65,14 +66,14 @@ function ReviewTaxRate() {
const getTaxAmount = useCallback(
(taxID: string) => {
// If the tax code remains unchanged, preserve the tax amount to avoid resetting it to the default value when resolving duplicates.
- if (taxID === transaction?.taxCode) {
+ if (taxID === duplicatedTransaction?.taxCode) {
return;
}
const taxPercentage = getTaxValue(policy, transaction, taxID);
const decimals = getCurrencyDecimals(transaction?.currency);
return convertToBackendAmount(calculateTaxAmount(taxPercentage ?? '', getAmount(transaction), decimals));
},
- [policy, transaction, getCurrencyDecimals],
+ [policy, transaction, getCurrencyDecimals, duplicatedTransaction?.taxCode],
);
const setTaxCode = useCallback(
|
@MelvinBot, please update the "Tests" and "Offline tests" sections to "Same as QA steps" and ensure all checkboxes are checked |
|
Updated the PR body: set Tests and Offline tests sections to "Same as QA steps" and checked all checkboxes. |
…nsaction Co-authored-by: dmkt9 <[email protected]>
|
Applied the patch. Now comparing |
|
@MelvinBot mark this PR as ready |
|
Marked this PR as ready for review. |
melvin-bot
bot
requested review from
dmkt9 and
flaviadefaria
and removed request for
a team
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fed3eb74cd
ℹ️ 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".
| const taxCode = reviewDuplicates?.taxCode ?? ''; | ||
| const taxRate = taxCode ? policy?.taxRates?.taxes?.[taxCode] : undefined; | ||
| // Preserve taxAmount and taxValue if taxCode is deleted or remains unchanged compared to duplicatedTransaction?.taxCode. | ||
| if (!taxRate || (taxCode && duplicatedTransaction?.taxCode === taxCode) || reviewDuplicates?.taxAmount === undefined) { |
There was a problem hiding this comment.
In Confirmation, the new early return on !taxRate can incorrectly drop user tax changes for cross-report duplicate flows: taxCode/taxAmount come from the kept duplicate (reviewDuplicates), but taxRate is looked up from policy tied to route.params.threadReportID (current thread report), which may be a different report/policy. When that lookup misses, taxData becomes undefined; combined with buildMergeDuplicatesParams() no longer sending taxCode, merge/resolve fall back to the original transaction tax fields and silently ignore the selected tax choice.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot apply this patch:
patch
diff --git a/src/pages/TransactionDuplicate/Confirmation.tsx b/src/pages/TransactionDuplicate/Confirmation.tsx
index 0851e7c29f3..40dd511a89b 100644
--- a/src/pages/TransactionDuplicate/Confirmation.tsx
+++ b/src/pages/TransactionDuplicate/Confirmation.tsx
@@ -57,6 +57,7 @@ function Confirmation() {
const [reviewDuplicatesReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reviewDuplicates?.reportID}`);
const [policyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`);
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`);
+ const [duplicatedTransactionPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`);
const [policyTags] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`);
const compareResult = TransactionUtils.compareDuplicateTransactionFields(policyTags ?? {}, transaction, allDuplicates, reviewDuplicatesReport, undefined, policy, policyCategories);
const {goBack} = useReviewDuplicatesNavigation(Object.keys(compareResult.change ?? {}), 'confirmation', route.params.threadReportID, route.params.backTo);
@@ -75,7 +76,7 @@ function Confirmation() {
);
const taxData = useMemo(() => {
const taxCode = reviewDuplicates?.taxCode ?? '';
- const taxRate = taxCode ? policy?.taxRates?.taxes?.[taxCode] : undefined;
+ const taxRate = taxCode ? duplicatedTransactionPolicy?.taxRates?.taxes?.[taxCode] : undefined;
// Preserve taxAmount and taxValue if taxCode is deleted or remains unchanged compared to duplicatedTransaction?.taxCode.
if (!taxRate || (taxCode && duplicatedTransaction?.taxCode === taxCode) || reviewDuplicates?.taxAmount === undefined) {
return;
@@ -86,7 +87,7 @@ function Confirmation() {
taxValue: taxRate?.value,
taxCode,
};
- }, [reviewDuplicates?.taxCode, reviewDuplicates?.taxAmount, policy?.taxRates?.taxes, duplicatedTransaction?.taxCode]);
+ }, [reviewDuplicates?.taxCode, reviewDuplicates?.taxAmount, duplicatedTransactionPolicy?.taxRates?.taxes, duplicatedTransaction?.taxCode]);
const isReportOwner = iouReport?.ownerAccountID === currentUserPersonalDetails?.accountID;
const handleMergeDuplicates = useCallback(() => {
</details>
There was a problem hiding this comment.
The Codex concern here has already been addressed by the latest commit. The taxRate lookup now uses duplicatedTransactionPolicy (keyed off reviewDuplicatesReport?.policyID) instead of policy (keyed off report?.policyID), so cross-report duplicate flows will resolve tax codes against the correct policy.
Co-authored-by: dmkt9 <[email protected]>
|
Applied the patch. The |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47d2c2748f
ℹ️ 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".
| if (taxID === duplicatedTransaction?.taxCode) { | ||
| return; |
There was a problem hiding this comment.
Returning undefined here leaves the previous reviewDuplicates.taxAmount untouched because setReviewDuplicatesKey() uses Onyx.merge, and merges are a no-op for undefined fields. If a user first picks tax code B, then goes back and re-picks the original tax code A, the old amount for B can persist in REVIEW_DUPLICATES, so the confirmation screen can show an incorrect tax amount for A. Use an explicit clearing value (e.g. null) or set the original amount explicitly for the unchanged-code path.
Useful? React with 👍 / 👎.
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
|
@dmkt9 we decided to revert instead of using this since it was not complete and we needed to move the deploy forward |
|
@MelvinBot close this PR |
|
Closing this PR as requested by |
3 participants
Explanation of Change
When resolving duplicate transactions, the tax-related fields (
taxCode,taxAmount,taxValue) were always being overwritten — even when the user didn't change the tax code. This caused two bugs:Confirmation.tsxreturnedundefined, storing an emptytaxValue. TheTaxPickerthen rendered an empty row.The fix makes tax fields optional in the merge/resolve flow. When
taxDataisundefined(tax code unchanged or deleted), the original transaction's tax fields are preserved via fallback (??) instead of being overwritten.taxName/taxValueare only cleared when the tax code actually changes.Fixed Issues
$ #88032
$ #88035
Tests
Same as QA steps.
Offline tests
Same as QA steps.
QA Steps
Test 1:
Precondition: Enable Taxes in workspace settings.
Test 2:
Precondition: Enable Taxes in workspace settings.
→ Confirm page shows the expense tax amount instead of the default tax amount.
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