Preserve disabled category/tag/taxCode during duplicate resolution#85612
Preserve disabled category/tag/taxCode during duplicate resolution#85612
Conversation
When resolving duplicate expenses where both have disabled categories (or tags/taxCodes), the field was silently dropped from both 'keep' and 'change' in compareDuplicateTransactionFields, resulting in an empty string being used during merge. This adds fallback else clauses to preserve the first transaction's value even when no enabled options are available. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
The PR intentionally preserves the first transaction's category/taxCode in keep when all options are disabled/deleted, instead of silently dropping the field. Update test expectations accordingly. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Fixed the failing check: Updated 5 test assertions in |
| keep[fieldName] = firstTransaction?.[keys[0]] ?? firstTransaction?.[keys[1]]; | ||
| } else { | ||
| // Preserve the first transaction's taxCode (even if disabled) to avoid blanking it during duplicate resolution | ||
| keep[fieldName] = firstTransaction?.[keys[0]] ?? firstTransaction?.[keys[1]]; |
There was a problem hiding this comment.
@MelvinBot It's the same code with the previous else block. Let's just combine it into 1 else block. Do for the others too
There was a problem hiding this comment.
Combined the else if (areAllFieldsEqualForKey) and else blocks into a single else block for all three fields (taxCode, category, and tag). Since both branches had the same code, they're now a single else that handles both cases. Pushed in 2ca4bde.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mp4 |
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
@MelvinBot mark this PR as ready |
|
@bernhardoj 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] |
|
Done — PR is now marked as ready for review. ✅ |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #82925 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Explanation of Change
In
compareDuplicateTransactionFields, when resolving duplicate expenses where both have different disabled categories (or tags/taxCodes), the field was silently dropped from bothkeepandchangebecause:changebranch requires enabled options to be availablekeepbranch requires all fields to be equalWhen neither condition is met (different values, all disabled), the field falls through without being stored anywhere. This causes
buildMergeDuplicatesParamsto readreviewDuplicates?.category ?? ''which resolves to an empty string, blanking the category.This PR adds fallback
elseclauses to preserve the first transaction's value (even if disabled) rather than silently dropping it. The same fix is applied tocategory,tag(non-multi-level), andtaxCodefields which all have the identical gap.Fixed Issues
$ #82925
PROPOSAL: #82925 (comment)
Tests
Offline tests
N/A - This change affects duplicate resolution logic which requires server interaction. The optimistic data flow is unchanged.
QA Steps
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