Skip to content

Fix duplicate tax handling to preserve original tax data when unchanged #88125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MelvinBot wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix duplicate tax handling to preserve original tax data when unchanged #88125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b716146
Fix duplicate tax handling to preserve original tax data when unchanged
MelvinBot Apr 16, 2026
c2e683f
Preserve taxAmount when tax code is unchanged during duplicate resolu…
MelvinBot Apr 16, 2026
fed3eb7
Compare tax code against duplicatedTransaction instead of current tra…
MelvinBot Apr 16, 2026
47d2c27
Use duplicated transaction's policy for tax rate lookup in Confirmation
MelvinBot Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libs/API/parameters/MergeDuplicatesParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ type MergeDuplicatesParams = {
billable: boolean;
reimbursable: boolean;
tag: string;
taxCode: string;
taxCode?: string;
receiptID: number;
reportID: string | undefined;
reportActionID?: string | undefined;
Expand Down
5 changes: 2 additions & 3 deletions src/libs/TransactionUtils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2559,8 +2559,8 @@ function buildNewTransactionAfterReviewingDuplicates(reviewDuplicateTransaction:
modifiedMerchant: reviewDuplicateTransaction?.merchant,
merchant: reviewDuplicateTransaction?.merchant,
comment: {...reviewDuplicateTransaction?.comment, comment: reviewDuplicateTransaction?.description},
// Clear stale taxName/taxValue so MoneyRequestView derives them fresh from the policy using the updated taxCode
...(reviewDuplicateTransaction?.taxCode !== undefined && {taxName: undefined, taxValue: undefined}),
// If the taxCode changes, clear stale taxName/taxValue so MoneyRequestView derives them fresh from the policy using the updated taxCode
...(reviewDuplicateTransaction?.taxCode !== undefined && reviewDuplicateTransaction?.taxCode !== duplicatedTransaction?.taxCode && {taxName: undefined, taxValue: undefined}),
};
}

Expand All @@ -2581,7 +2581,6 @@ function buildMergeDuplicatesParams(
reimbursable: reviewDuplicates?.reimbursable ?? false,
category: reviewDuplicates?.category ?? '',
tag: reviewDuplicates?.tag ?? '',
taxCode: reviewDuplicates?.taxCode ?? '',
merchant: reviewDuplicates?.merchant ?? '',
comment: reviewDuplicates?.description ?? '',
};
Expand Down
16 changes: 8 additions & 8 deletions src/libs/actions/IOU/Duplicate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ function getIOUActionForTransactions(transactionIDList: Array<string | undefined
}

/** Merge several transactions into one by updating the fields of the one we want to keep and deleting the rest */
function mergeDuplicates({transactionThreadReportID: optimisticTransactionThreadReportID, taxAmount, taxValue, ...params}: MergeDuplicatesParams & {taxAmount: number; taxValue: string}) {
function mergeDuplicates({transactionThreadReportID: optimisticTransactionThreadReportID, taxAmount, taxValue, ...params}: MergeDuplicatesParams & {taxAmount?: number; taxValue?: string}) {
const allParams: MergeDuplicatesParams = {...params};
const allTransactions = getAllTransactions();
const allTransactionViolations = getAllTransactionViolations();
Expand All @@ -100,9 +100,9 @@ function mergeDuplicates({transactionThreadReportID: optimisticTransactionThread
modifiedMerchant: params.merchant,
reimbursable: params.reimbursable,
tag: params.tag,
taxCode: params.taxCode,
taxAmount,
taxValue,
taxCode: params.taxCode ?? originalSelectedTransaction?.taxCode,
taxAmount: taxAmount ?? originalSelectedTransaction?.taxAmount,
taxValue: taxValue ?? originalSelectedTransaction?.taxValue,
},
};

Expand Down Expand Up @@ -351,7 +351,7 @@ function mergeDuplicates({transactionThreadReportID: optimisticTransactionThread
}

/** Instead of merging the duplicates, it updates the transaction we want to keep and puts the others on hold without deleting them */
function resolveDuplicates({taxAmount, taxValue, ...params}: MergeDuplicatesParams & {taxAmount: number; taxValue: string}) {
function resolveDuplicates({taxAmount, taxValue, ...params}: MergeDuplicatesParams & {taxAmount?: number; taxValue?: string}) {
if (!params.transactionID) {
return;
}
Expand All @@ -376,9 +376,9 @@ function resolveDuplicates({taxAmount, taxValue, ...params}: MergeDuplicatesPara
modifiedMerchant: params.merchant,
reimbursable: params.reimbursable,
tag: params.tag,
taxCode: params.taxCode,
taxAmount,
taxValue,
taxCode: params.taxCode ?? originalSelectedTransaction?.taxCode,
taxAmount: taxAmount ?? originalSelectedTransaction?.taxAmount,
taxValue: taxValue ?? originalSelectedTransaction?.taxValue,
},
};

Expand Down
15 changes: 11 additions & 4 deletions src/pages/TransactionDuplicate/Confirmation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -75,12 +76,18 @@ 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) {
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Gate tax updates with selected report policy

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 👍 / 👎.

Copy link
Copy Markdown
Contributor

@dmkt9 dmkt9 Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>

Copy link
Copy Markdown
Contributor

@stitesExpensify stitesExpensify Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MelvinBot bump ^

Copy link
Copy Markdown
Contributor Author

@MelvinBot MelvinBot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
Comment on lines +80 to +82
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve tax-code updates when policy rates are not loaded

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 👍 / 👎.

Copy link
Copy Markdown
Contributor

@dmkt9 dmkt9 Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return {
taxAmount: -(reviewDuplicates?.taxAmount ?? 0),
taxValue: taxRate?.value ?? '',
taxAmount: -reviewDuplicates.taxAmount,
taxValue: taxRate?.value,
taxCode,
};
}, [reviewDuplicates?.taxCode, reviewDuplicates?.taxAmount, policy?.taxRates?.taxes]);
}, [reviewDuplicates?.taxCode, reviewDuplicates?.taxAmount, duplicatedTransactionPolicy?.taxRates?.taxes, duplicatedTransaction?.taxCode]);
const isReportOwner = iouReport?.ownerAccountID === currentUserPersonalDetails?.accountID;

const handleMergeDuplicates = useCallback(() => {
Expand Down
7 changes: 6 additions & 1 deletion src/pages/TransactionDuplicate/ReviewTaxCode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ?? [],
Expand Down Expand Up @@ -64,11 +65,15 @@ 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 === duplicatedTransaction?.taxCode) {
return;
Comment on lines +69 to +70
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear stale taxAmount when tax code is reselected

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 👍 / 👎.

}
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(
Expand Down
Loading