Skip to content

Commit 29ddc6d

Browse files
stitesExpensifystitesExpensify
authored
Merge pull request #88765 from Expensify/claude-fixDuplicateTaxCodeReview
Fix empty tax code page in Review Duplicates flow
2 parents cab81ee + d26382a commit 29ddc6d

9 files changed

Lines changed: 175 additions & 58 deletions

File tree

‎config/eslint/eslint.seatbelt.tsv‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -353,8 +353,6 @@
353353
"../../src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx" "react-hooks/set-state-in-effect" 1
354354
"../../src/pages/Search/SearchPage.tsx" "react-hooks/refs" 31
355355
"../../src/pages/Search/SearchPage.tsx" "react-hooks/set-state-in-effect" 1
356-
"../../src/pages/TransactionDuplicate/Confirmation.tsx" "react-hooks/immutability" 2
357-
"../../src/pages/TransactionDuplicate/Confirmation.tsx" "react-hooks/preserve-manual-memoization" 1
358356
"../../src/pages/TransactionDuplicate/Confirmation.tsx" "react-hooks/refs" 12
359357
"../../src/pages/TransactionMerge/MergeFieldReview.tsx" "no-restricted-syntax" 1
360358
"../../src/pages/TransactionMerge/TransactionMergeReceipts.tsx" "no-restricted-syntax" 1

‎src/libs/API/parameters/MergeDuplicatesParams.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ type MergeDuplicatesParams = {
1010
billable: boolean;
1111
reimbursable: boolean;
1212
tag: string;
13+
taxCode?: string;
1314
receiptID: number;
1415
reportID: string | undefined;
1516
reportActionID?: string | undefined;

‎src/libs/API/parameters/ResolveDuplicatesParams.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ type ResolveDuplicatesParams = {
1313
billable: boolean;
1414
reimbursable: boolean;
1515
tag: string;
16+
taxCode?: string;
1617

1718
/** The reportActionID of the dismissed violation action in the kept transaction thread report */
1819
dismissedViolationReportActionID: string;

‎src/libs/TransactionUtils/index.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2581,14 +2581,17 @@ function getTransactionID(report?: OnyxEntry<Report>): string | undefined {
25812581
}
25822582

25832583
function buildNewTransactionAfterReviewingDuplicates(reviewDuplicateTransaction: OnyxEntry<ReviewDuplicates>, duplicatedTransaction: OnyxEntry<Transaction>): Partial<Transaction> {
2584-
const {duplicates, ...restReviewDuplicateTransaction} = reviewDuplicateTransaction ?? {};
2584+
const {duplicates, taxAmount, ...restReviewDuplicateTransaction} = reviewDuplicateTransaction ?? {};
2585+
const hasUpdatedTaxCode = reviewDuplicateTransaction?.taxCode !== undefined && reviewDuplicateTransaction?.taxCode !== duplicatedTransaction?.taxCode;
25852586

25862587
return {
25872588
...duplicatedTransaction,
25882589
...restReviewDuplicateTransaction,
25892590
modifiedMerchant: reviewDuplicateTransaction?.merchant,
25902591
merchant: reviewDuplicateTransaction?.merchant,
25912592
comment: {...reviewDuplicateTransaction?.comment, comment: reviewDuplicateTransaction?.description},
2593+
// If the taxCode changes, apply the reviewed tax amount and clear stale taxName/taxValue so MoneyRequestView derives them fresh from the policy.
2594+
...(hasUpdatedTaxCode && {taxAmount, taxName: undefined, taxValue: undefined}),
25922595
};
25932596
}
25942597

‎src/libs/actions/IOU/Duplicate.ts‎

Lines changed: 103 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -71,40 +71,105 @@ function getIOUActionForTransactions(transactionIDList: Array<string | undefined
7171
);
7272
}
7373

74-
type MergeDuplicatesFuncParams = MergeDuplicatesParams & {currentUserLogin: string; currentUserAccountID: number};
75-
76-
/** Merge several transactions into one by updating the fields of the one we want to keep and deleting the rest */
77-
function mergeDuplicates({transactionThreadReportID: optimisticTransactionThreadReportID, currentUserLogin, currentUserAccountID, ...params}: MergeDuplicatesFuncParams) {
78-
const allParams: MergeDuplicatesParams = {...params};
79-
const allTransactions = getAllTransactions();
80-
const allTransactionViolations = getAllTransactionViolations();
81-
const allReports = getAllReports();
82-
const originalSelectedTransaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`];
74+
type DuplicateTransactionParams = {
75+
transactionID: string | undefined;
76+
originalSelectedTransaction: OnyxEntry<OnyxTypes.Transaction>;
77+
billable: boolean;
78+
comment: string;
79+
category: string;
80+
created: string;
81+
currency: string;
82+
merchant: string;
83+
reimbursable: boolean;
84+
tag: string;
85+
taxCode?: string;
86+
taxAmount?: number;
87+
taxValue?: string;
88+
};
8389

84-
const optimisticTransactionData: OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION> = {
90+
function buildOptimisticTransactionData({
91+
transactionID,
92+
originalSelectedTransaction,
93+
billable,
94+
comment,
95+
category,
96+
created,
97+
currency,
98+
merchant,
99+
reimbursable,
100+
tag,
101+
taxCode,
102+
taxAmount,
103+
taxValue,
104+
}: DuplicateTransactionParams): OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION> {
105+
return {
85106
onyxMethod: Onyx.METHOD.MERGE,
86-
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`,
107+
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
87108
value: {
88109
...originalSelectedTransaction,
89-
billable: params.billable,
110+
billable,
90111
comment: {
91-
comment: params.comment,
112+
comment,
92113
},
93-
category: params.category,
94-
created: params.created,
95-
currency: params.currency,
96-
modifiedMerchant: params.merchant,
97-
reimbursable: params.reimbursable,
98-
tag: params.tag,
114+
category,
115+
created,
116+
currency,
117+
modifiedMerchant: merchant,
118+
reimbursable,
119+
tag,
120+
taxCode: taxCode ?? originalSelectedTransaction?.taxCode,
121+
taxAmount: taxAmount ?? originalSelectedTransaction?.taxAmount,
122+
taxValue: taxValue ?? originalSelectedTransaction?.taxValue,
123+
// Clear `taxName` to stay consistent with the server response,
124+
// and avoid retaining an outdated value that doesn't match the new `taxCode`.
125+
taxName: taxCode ? null : originalSelectedTransaction?.taxName,
99126
},
100127
};
128+
}
101129

102-
const failureTransactionData: OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION> = {
130+
function buildFailureTransactionData(transactionID: string | undefined, originalSelectedTransaction: OnyxEntry<OnyxTypes.Transaction>): OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION> {
131+
return {
103132
onyxMethod: Onyx.METHOD.MERGE,
104-
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`,
133+
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
105134
// eslint-disable-next-line @typescript-eslint/non-nullable-type-assertion-style
106135
value: originalSelectedTransaction as OnyxTypes.Transaction,
107136
};
137+
}
138+
139+
type MergeDuplicatesFuncParams = MergeDuplicatesParams & {currentUserLogin: string; currentUserAccountID: number; taxAmount?: number; taxValue?: string};
140+
141+
/** Merge several transactions into one by updating the fields of the one we want to keep and deleting the rest */
142+
function mergeDuplicates({
143+
transactionThreadReportID: optimisticTransactionThreadReportID,
144+
currentUserLogin,
145+
currentUserAccountID,
146+
taxAmount,
147+
taxValue,
148+
...params
149+
}: MergeDuplicatesFuncParams) {
150+
const allParams: MergeDuplicatesParams = {...params};
151+
const allTransactions = getAllTransactions();
152+
const allTransactionViolations = getAllTransactionViolations();
153+
const allReports = getAllReports();
154+
const originalSelectedTransaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`];
155+
156+
const optimisticTransactionData = buildOptimisticTransactionData({
157+
transactionID: params.transactionID,
158+
originalSelectedTransaction,
159+
billable: params.billable,
160+
comment: params.comment,
161+
category: params.category,
162+
created: params.created,
163+
currency: params.currency,
164+
merchant: params.merchant,
165+
reimbursable: params.reimbursable,
166+
tag: params.tag,
167+
taxCode: params.taxCode,
168+
taxAmount,
169+
taxValue,
170+
});
171+
172+
const failureTransactionData = buildFailureTransactionData(params.transactionID, originalSelectedTransaction);
108173

109174
const optimisticTransactionDuplicatesData: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION>> = params.transactionIDList.map((id) => ({
110175
onyxMethod: Onyx.METHOD.SET,
@@ -344,7 +409,7 @@ function mergeDuplicates({transactionThreadReportID: optimisticTransactionThread
344409
}
345410

346411
/** Instead of merging the duplicates, it updates the transaction we want to keep and puts the others on hold without deleting them */
347-
function resolveDuplicates(params: MergeDuplicatesParams) {
412+
function resolveDuplicates({taxAmount, taxValue, ...params}: MergeDuplicatesParams & {taxAmount?: number; taxValue?: string}) {
348413
if (!params.transactionID) {
349414
return;
350415
}
@@ -354,30 +419,23 @@ function resolveDuplicates(params: MergeDuplicatesParams) {
354419

355420
const originalSelectedTransaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`];
356421

357-
const optimisticTransactionData: OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION> = {
358-
onyxMethod: Onyx.METHOD.MERGE,
359-
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`,
360-
value: {
361-
...originalSelectedTransaction,
362-
billable: params.billable,
363-
comment: {
364-
comment: params.comment,
365-
},
366-
category: params.category,
367-
created: params.created,
368-
currency: params.currency,
369-
modifiedMerchant: params.merchant,
370-
reimbursable: params.reimbursable,
371-
tag: params.tag,
372-
},
373-
};
422+
const optimisticTransactionData = buildOptimisticTransactionData({
423+
transactionID: params.transactionID,
424+
originalSelectedTransaction,
425+
billable: params.billable,
426+
comment: params.comment,
427+
category: params.category,
428+
created: params.created,
429+
currency: params.currency,
430+
merchant: params.merchant,
431+
reimbursable: params.reimbursable,
432+
tag: params.tag,
433+
taxCode: params.taxCode,
434+
taxAmount,
435+
taxValue,
436+
});
374437

375-
const failureTransactionData: OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION> = {
376-
onyxMethod: Onyx.METHOD.MERGE,
377-
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${params.transactionID}`,
378-
// eslint-disable-next-line @typescript-eslint/non-nullable-type-assertion-style
379-
value: originalSelectedTransaction as OnyxTypes.Transaction,
380-
};
438+
const failureTransactionData = buildFailureTransactionData(params.transactionID, originalSelectedTransaction);
381439

382440
const optimisticTransactionViolations: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS>> = [...params.transactionIDList, params.transactionID].map((id) => {
383441
const violations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${id}`] ?? [];

‎src/pages/TransactionDuplicate/Confirmation.tsx‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ function Confirmation() {
5757
const [reviewDuplicatesReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reviewDuplicates?.reportID}`);
5858
const [policyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`);
5959
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`);
60+
const [duplicatedTransactionPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`);
6061
const [policyTags] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`);
6162
const compareResult = TransactionUtils.compareDuplicateTransactionFields(policyTags ?? {}, transaction, allDuplicates, reviewDuplicatesReport, undefined, policy, policyCategories);
6263
const {goBack} = useReviewDuplicatesNavigation(Object.keys(compareResult.change ?? {}), 'confirmation', route.params.threadReportID, route.params.backTo);
@@ -73,25 +74,44 @@ function Confirmation() {
7374
() => TransactionUtils.buildMergeDuplicatesParams(reviewDuplicates, duplicates ?? [], newTransaction),
7475
[duplicates, reviewDuplicates, newTransaction],
7576
);
77+
const reviewDuplicatesTaxCode = reviewDuplicates?.taxCode;
78+
const reviewDuplicatesTaxAmount = reviewDuplicates?.taxAmount;
79+
const duplicatedTransactionTaxCode = duplicatedTransaction?.taxCode;
80+
const taxRates = duplicatedTransactionPolicy?.taxRates?.taxes;
81+
const taxData = useMemo(() => {
82+
const taxCode = reviewDuplicatesTaxCode ?? '';
83+
const taxRate = taxCode ? taxRates?.[taxCode] : undefined;
84+
// Preserve taxAmount and taxValue if taxCode is deleted or remains unchanged compared to duplicatedTransaction?.taxCode.
85+
if (!taxRate || (taxCode && duplicatedTransactionTaxCode === taxCode) || reviewDuplicatesTaxAmount === undefined) {
86+
return;
87+
}
88+
89+
return {
90+
taxAmount: -reviewDuplicatesTaxAmount,
91+
taxValue: taxRate?.value,
92+
taxCode,
93+
};
94+
}, [reviewDuplicatesTaxCode, reviewDuplicatesTaxAmount, taxRates, duplicatedTransactionTaxCode]);
7695
const isReportOwner = iouReport?.ownerAccountID === currentUserPersonalDetails?.accountID;
96+
const currentUserAccountID = currentUserPersonalDetails.accountID;
97+
const currentUserLogin = currentUserPersonalDetails?.login;
98+
const childReportID = reportAction?.childReportID;
7799

78100
const handleMergeDuplicates = useCallback(() => {
79-
const transactionThreadReportID = reportAction?.childReportID ?? generateReportID();
80-
if (!reportAction?.childReportID) {
81-
transactionsMergeParams.transactionThreadReportID = transactionThreadReportID;
82-
}
83-
mergeDuplicates({...transactionsMergeParams, currentUserAccountID: currentUserPersonalDetails.accountID, currentUserLogin: currentUserPersonalDetails?.login ?? ''});
101+
const transactionThreadReportID = childReportID ?? generateReportID();
102+
const mergeParams = !childReportID ? {...transactionsMergeParams, transactionThreadReportID} : transactionsMergeParams;
103+
mergeDuplicates({...mergeParams, ...taxData, currentUserAccountID, currentUserLogin: currentUserLogin ?? ''});
84104
if (isSuperWideRHPDisplayed) {
85105
Navigation.dismissToSuperWideRHP();
86106
return;
87107
}
88108
Navigation.dismissModal();
89-
}, [reportAction?.childReportID, transactionsMergeParams, currentUserPersonalDetails.accountID, currentUserPersonalDetails?.login, isSuperWideRHPDisplayed]);
109+
}, [childReportID, transactionsMergeParams, taxData, currentUserAccountID, currentUserLogin, isSuperWideRHPDisplayed]);
90110

91111
const handleResolveDuplicates = useCallback(() => {
92-
resolveDuplicates(transactionsMergeParams);
112+
resolveDuplicates({...transactionsMergeParams, ...taxData});
93113
Navigation.dismissToSuperWideRHP();
94-
}, [transactionsMergeParams]);
114+
}, [transactionsMergeParams, taxData]);
95115

96116
const contextMenuStateValue = useMemo(
97117
() => ({

‎src/pages/TransactionDuplicate/ReviewTaxCode.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,10 @@ function ReviewTaxRate() {
2626
const {translate} = useLocalize();
2727
const {getCurrencyDecimals} = useCurrencyListActions();
2828
const [reviewDuplicates] = useOnyx(ONYXKEYS.REVIEW_DUPLICATES);
29-
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reviewDuplicates?.reportID ?? route.params.threadReportID}`);
29+
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reviewDuplicates?.reportID}`);
3030
const policy = usePolicy(report?.policyID);
31-
const transactionID = getTransactionID(report);
31+
const [transactionThreadReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${route.params.threadReportID}`);
32+
const transactionID = getTransactionID(transactionThreadReport);
3233
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${getNonEmptyStringOnyxID(transactionID)}`);
3334
const [transactionViolations] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`);
3435
const allDuplicateIDs = useMemo(

‎tests/actions/IOUTest/DuplicateTest.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,7 @@ describe('actions/Duplicate', () => {
198198
billable: true,
199199
reimbursable: false,
200200
tag: 'UpdatedProject',
201+
taxCode: '',
201202
receiptID: 123,
202203
reportID,
203204
};
@@ -532,6 +533,7 @@ describe('actions/Duplicate', () => {
532533
billable: true,
533534
reimbursable: false,
534535
tag: 'UpdatedProject',
536+
taxCode: '',
535537
receiptID: 123,
536538
reportID,
537539
};

‎tests/unit/TransactionUtilsTest.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2903,6 +2903,39 @@ describe('TransactionUtils', () => {
29032903
});
29042904
});
29052905

2906+
describe('buildNewTransactionAfterReviewingDuplicates', () => {
2907+
it('preserves the kept transaction tax amount when the selected tax code matches the existing tax code', () => {
2908+
const duplicatedTransaction = generateTransaction({
2909+
transactionID: 'transaction1',
2910+
reportID: 'report1',
2911+
taxCode: 'id_TAX_RATE_1',
2912+
taxAmount: -500,
2913+
taxValue: '5%',
2914+
});
2915+
2916+
const reviewDuplicates = {
2917+
duplicates: [],
2918+
transactionID: 'transaction1',
2919+
reportID: 'report1',
2920+
merchant: 'Updated Merchant',
2921+
category: 'Travel',
2922+
tag: 'Project',
2923+
taxCode: 'id_TAX_RATE_1',
2924+
taxAmount: 900,
2925+
description: 'Updated comment',
2926+
comment: duplicatedTransaction.comment ?? {},
2927+
reimbursable: false,
2928+
billable: true,
2929+
};
2930+
2931+
const updatedTransaction = TransactionUtils.buildNewTransactionAfterReviewingDuplicates(reviewDuplicates, duplicatedTransaction);
2932+
2933+
expect(updatedTransaction.taxCode).toBe('id_TAX_RATE_1');
2934+
expect(updatedTransaction.taxAmount).toBe(-500);
2935+
expect(updatedTransaction.taxValue).toBe('5%');
2936+
});
2937+
});
2938+
29062939
describe('getTagArrayFromName', () => {
29072940
it('splits simple tag by colon', () => {
29082941
expect(TransactionUtils.getTagArrayFromName('tag1:tag2:tag3')).toEqual(['tag1', 'tag2', 'tag3']);

0 commit comments

Comments
 (0)