Skip reimbursable step in duplicate resolver for managed card transactions#94777
Skip reimbursable step in duplicate resolver for managed card transactions#94777MelvinBot wants to merge 4 commits into
Conversation
…tions Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
@MelvinBot complete the checklist and test steps, and mark screenshots as done since they’ll be added in the reviewer checklist. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariBefore: Screen.Recording.2026-06-28.at.18.28.04.movAfter: Screen.Recording.2026-06-28.at.19.50.40.mov |
|
@MelvinBot merge into latest main and mark the PR as ready |
…ursableManagedCard Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
@abzokhattab 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70c733f425
ℹ️ 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 (transactions.some(isManagedCardTransaction)) { | ||
| keep[fieldName] = false; |
There was a problem hiding this comment.
Only force non-reimbursable for selected managed cards
If the user selects the cash expense to keep while one of its duplicates is a managed card transaction, getReviewNavigationRoute passes that cash transaction as the selected transaction and resolveDuplicates updates params.transactionID; this branch still sees the managed card elsewhere in transactions and writes keep.reimbursable = false, so a valid reimbursable cash expense is silently changed to non-reimbursable. Please gate this on the selected/reviewing transaction being managed, not on any duplicate in the set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in 454c290.
The resolved reimbursable value is applied to the kept transaction (reviewDuplicates.transactionID, which buildMergeDuplicatesParams merges into), so the gate is now on whether that transaction is managed, not on any duplicate in the set:
if (isManagedCardTransaction(reviewingTransaction)) {
keep[fieldName] = false;
} else if (areAllFieldsEqualForKey) {
...
}- Keep the managed card →
keep.reimbursable = false, step hidden (original bug fixed). - Keep the cash expense (even when a duplicate is a managed card) → no longer forced to
false; the differing values surface as the reimbursable step, so a valid reimbursable cash expense is preserved.
Tests updated in tests/unit/TransactionUtilsTest.ts (reimbursable field comparison): one case for keeping the managed card (forced false, no step) and a new case for keeping the cash expense (not forced; change.reimbursable is surfaced).
…card Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
@marcochavezf 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] |
|
There is a back-navigation bug in the current fix. Root cause: Every Steps to reproduce:
Fix:
} else if (fieldName === 'reimbursable') {
const selectedTransaction = transactions.find((t) => t?.transactionID === selectedTransactionID) ?? firstTransaction;
if (isManagedCardTransaction(selectedTransaction)) {
keep[fieldName] = false;
// Before
compareDuplicateTransactionFields(policyTags ?? {}, transaction, allDuplicates, reviewDuplicatesReport, undefined, policy, policyCategories)
// After
compareDuplicateTransactionFields(policyTags ?? {}, transaction, allDuplicates, reviewDuplicatesReport, reviewDuplicates?.transactionID, policy, policyCategories)Affected files: |
…k navigation Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot review the PR |
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. The resolvedreimbursablevalue is always applied to the transaction that is kept (the reviewing transaction —reviewDuplicates.transactionID, whichbuildMergeDuplicatesParamsmerges into), so the branch forcesreimbursableintokeepasfalseand never adds it tochangeonly when that kept transaction is itself a managed card (isManagedCardTransaction(reviewingTransaction)). Because everyReview*page derives its step list fromObject.keys(change), droppingreimbursablefromchangeremoves the step everywhere, and seedingkeep.reimbursable = falseflows throughsetReviewDuplicatesKeyso the transaction is merged as non-reimbursable. When the kept transaction is not a managed card, the existing equal/differ behavior is preserved — so a kept cash expense that legitimately is reimbursable is never silently converted to non-reimbursable, even when one of its duplicates is a managed card.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.Fixed Issues
$ #94679
PROPOSAL: #94679 (comment)
Tests
Automated coverage in
tests/unit/TransactionUtilsTest.ts(compareDuplicateTransactionFields→reimbursable field comparison): keeping a managed-card transaction yieldskeep.reimbursable === falsewith nochange.reimbursable; keeping a cash expense whose duplicate is a managed card surfaces the differing values inchange.reimbursableinstead of forcingfalse; and existing cash-vs-cash behavior is unchanged.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