Skip reimbursable step in duplicate resolver for managed card transactions - #94789
Skip reimbursable step in duplicate resolver for managed card transactions#94789abzokhattab wants to merge 2 commits into
Conversation
…tions Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
Only force reimbursable=false when the transaction being kept is itself a managed card transaction. Previously, the check fired if any duplicate in the set was a managed card — silently setting a valid cash expense to non-reimbursable when the user chose to keep the cash duplicate instead.
|
@QichenZhu Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| // Managed card transactions are always non-reimbursable. Only suppress the reimbursable | ||
| // review step and force false when the transaction being kept is itself a managed card — | ||
| // if the user keeps the cash duplicate, the managed card is discarded and the cash | ||
| // transaction's reimbursable value should be left intact. |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This transactions.find((t) => t?.transactionID === selectedTransactionID) lookup duplicates the identical lookup already performed earlier in the same function (where keep.comment is set for the selected transaction). The same selection logic now lives in two places, so a future change to how the selected transaction is resolved must be made twice.
Hoist the lookup once near the top of the function and reuse the result in both branches:
// Computed once, after `transactions` is built
const selectedTransaction = selectedTransactionID ? transactions.find((t) => t?.transactionID === selectedTransactionID) : undefined;
if (selectedTransactionID) {
keep.comment = selectedTransaction?.comment ?? {};
}
// ...inside the loop:
} else if (fieldName === 'reimbursable') {
if (isManagedCardTransaction(selectedTransaction ?? firstTransaction)) {
keep[fieldName] = false;
} else if (areAllFieldsEqualForKey) {
keep[fieldName] = firstTransaction?.[keys[0]] ?? firstTransaction?.[keys[1]];
} else {
processChanges(fieldName, transactions, keys);
}
}Reviewed at: c8f8adc | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Explanation of Change
The duplicate resolver decides which review steps to show and what values to merge from
compareDuplicateTransactionFieldsinsrc/libs/TransactionUtils/index.ts.reimbursablewas treated as a generic comparable field: when a manually-added cash expense (reimbursable: true) was a duplicate of a managed company-card transaction (reimbursable: false), the values differed, soreimbursablewas added to thechangemap — surfacing the "Choose if transaction is reimbursable" step. The user could pick reimbursable, and the merge persistedreimbursable: trueonto a managed card transaction, an invalid state with no in-product way to correct it.This change adds a dedicated
reimbursablebranch in the field-comparison loop that gates on the selected transaction (the one the user chose to keep), not on any transaction in the duplicate set:isManagedCardTransaction(selectedTransaction)),reimbursableis forced intokeepasfalseand never added tochange. Because everyReview*page derives its step list fromObject.keys(change), droppingreimbursablefromchangeremoves the step, and seedingkeep.reimbursable = falseflows throughsetReviewDuplicatesKeysobuildMergeDuplicatesParamsmerges the transaction as non-reimbursable.transactions.some(isManagedCardTransaction)).isManagedCardTransaction(!!transaction?.managedCard) is used deliberately rather than a broader card-import check, so personal/CSV card imports that can legitimately be reimbursable are unaffected — only centrally managed cards are constrained to non-reimbursable.Supersedes #94777.
Fixed Issues
$ #94679
PROPOSAL: #94679 (comment)
Tests
Automated coverage added in
tests/unit/TransactionUtilsTest.ts(compareDuplicateTransactionFields→reimbursable field comparison):Keeping the managed card duplicate →
keep.reimbursable === false, nochange.reimbursable.Keeping the cash duplicate → reimbursable step shown with both values, cash value preserved.
Existing cash-vs-cash behavior unchanged.
Verify that no errors appear in the JS console
Offline tests
Same as tests.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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