From 6e1d8668da5a7f4d94a72b2781ba32b696b3c0f5 Mon Sep 17 00:00:00 2001 From: alberto Date: Wed, 12 Aug 2026 13:08:09 +0200 Subject: [PATCH 01/12] Use the right distance on merge --- tests/actions/MergeTransactionTest.ts | 39 +++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/actions/MergeTransactionTest.ts b/tests/actions/MergeTransactionTest.ts index adbdada939f9..2b2b5360a651 100644 --- a/tests/actions/MergeTransactionTest.ts +++ b/tests/actions/MergeTransactionTest.ts @@ -2,6 +2,7 @@ import {getReportPreviewAction} from '@libs/actions/IOU/MoneyRequestBuilder'; import {areTransactionsEligibleForMerge, getTransactionsForMerging, mergeTransactionRequest, setMergeTransactionKey, setupMergeTransactionData} from '@libs/actions/MergeTransaction'; import {addComment, openReport} from '@libs/actions/Report'; import {WRITE_COMMANDS} from '@libs/API/types'; +import {getMergeFieldUpdatedValues} from '@libs/MergeTransactionUtils'; import {getLoginsByAccountIDs} from '@libs/PersonalDetailsUtils'; import {getOriginalMessage, getReportAction} from '@libs/ReportActionsUtils'; import {buildTransactionThread} from '@libs/ReportUtils'; @@ -1544,6 +1545,44 @@ describe('setMergeTransactionKey', () => { description: 'New Description', // Added }); }); + + it('should not keep the commuter exclusion of a previously selected merchant when another one is selected', async () => { + // Given a merge of two distance expenses, where the merchant of the one on a workspace that excludes + // 1 commuter mile was selected first + const transactionID = 'merge-distance-transaction'; + const selectMerchantOf = async (transaction: Transaction) => { + setMergeTransactionKey( + transactionID, + getMergeFieldUpdatedValues({ + transaction, + field: 'merchant', + fieldValue: transaction.merchant, + getCurrencyDecimals: getCurrencyDecimalsLocal, + mergeTransaction: await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`), + }), + ); + await waitForBatchedUpdates(); + }; + + await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); + await selectMerchantOf({ + ...createRandomDistanceRequestTransaction(0), + comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 4.49, commuterExclusion: 1, reimbursableDistance: 3.49}}, + }); + + // When the merchant of the expense on the workspace with no commuter exclusion is selected instead + await selectMerchantOf({ + ...createRandomDistanceRequestTransaction(1), + comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 10.2}}, + }); + + // Then only that expense's distance is left, rather than the first one's reimbursable distance, which the + // distance field displays in place of the full distance whenever an exclusion is present + const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); + expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); + }); }); describe('areTransactionsEligibleForMerge', () => { From 6981278b2317383bce2ed7b6eeca661b6a574cdc Mon Sep 17 00:00:00 2001 From: alberto Date: Wed, 12 Aug 2026 13:08:39 +0200 Subject: [PATCH 02/12] properly handle exclusions --- src/libs/MergeTransactionUtils.ts | 18 +++++++++++++++--- tests/actions/MergeTransactionTest.ts | 16 ++++++++-------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/libs/MergeTransactionUtils.ts b/src/libs/MergeTransactionUtils.ts index a5cf273c0872..833ef1b8ce94 100644 --- a/src/libs/MergeTransactionUtils.ts +++ b/src/libs/MergeTransactionUtils.ts @@ -7,7 +7,7 @@ import ONYXKEYS from '@src/ONYXKEYS'; import type {MergeTransaction, Policy, Report, SearchResults, Transaction} from '@src/types/onyx'; import type {Attendee} from '@src/types/onyx/IOU'; -import type {OnyxEntry} from 'react-native-onyx'; +import type {NullishDeep, OnyxEntry} from 'react-native-onyx'; import type {TupleToUnion} from 'type-fest'; import {SafeString} from 'expensify-common'; @@ -64,7 +64,7 @@ type MergeFieldData = { }; /** Type for merge transaction values that can be null to clear existing values in Onyx */ -type MergeTransactionUpdateValues = Partial>; +type MergeTransactionUpdateValues = Partial | null>>; const MERGE_FIELD_TRANSLATION_KEYS = { amount: 'iou.amount', @@ -674,7 +674,19 @@ function getMergeFieldUpdatedValues({ const transactionDetails = getTransactionDetails(transaction); updatedValues.amount = getMergeFieldValue(transactionDetails, transaction, 'amount') as number; updatedValues.currency = getCurrency(transaction); - updatedValues.customUnit = transaction?.comment?.customUnit; + // Selections are stored with Onyx.merge, which deep merges the custom unit, so the commuter exclusion of the + // surviving expense's workspace stays applied. Its reimbursable distance describes the previously selected + // distance though, and the distance field displays that in place of the full distance, so recompute it here. + const selectedCustomUnit = transaction?.comment?.customUnit; + const commuterExclusion = selectedCustomUnit?.commuterExclusion ?? mergeTransaction?.customUnit?.commuterExclusion; + const selectedQuantity = selectedCustomUnit?.quantity; + const hasDistanceToExclude = !!commuterExclusion && typeof selectedQuantity === 'number'; + updatedValues.customUnit = { + ...selectedCustomUnit, + ...((hasDistanceToExclude || mergeTransaction?.customUnit?.reimbursableDistance !== undefined) && { + reimbursableDistance: hasDistanceToExclude ? Math.max(0, selectedQuantity - commuterExclusion) : null, + }), + }; updatedValues.iouRequestType = transaction?.iouRequestType; // For manual distance requests, set waypoints/routes and receipt to null to clear any existing values updatedValues.receipt = transaction?.receipt ?? null; diff --git a/tests/actions/MergeTransactionTest.ts b/tests/actions/MergeTransactionTest.ts index 2b2b5360a651..bc58927ebde3 100644 --- a/tests/actions/MergeTransactionTest.ts +++ b/tests/actions/MergeTransactionTest.ts @@ -1546,9 +1546,9 @@ describe('setMergeTransactionKey', () => { }); }); - it('should not keep the commuter exclusion of a previously selected merchant when another one is selected', async () => { - // Given a merge of two distance expenses, where the merchant of the one on a workspace that excludes - // 1 commuter mile was selected first + it('should apply the commuter exclusion to the newly selected distance rather than the previously selected one', async () => { + // Given a merge of two distance expenses onto a workspace that excludes 1 commuter mile, where the merchant of + // the 4.49 mile expense was selected first const transactionID = 'merge-distance-transaction'; const selectMerchantOf = async (transaction: Transaction) => { setMergeTransactionKey( @@ -1570,18 +1570,18 @@ describe('setMergeTransactionKey', () => { comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 4.49, commuterExclusion: 1, reimbursableDistance: 3.49}}, }); - // When the merchant of the expense on the workspace with no commuter exclusion is selected instead + // When the merchant of the 10.2 mile expense is selected instead await selectMerchantOf({ ...createRandomDistanceRequestTransaction(1), comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 10.2}}, }); - // Then only that expense's distance is left, rather than the first one's reimbursable distance, which the - // distance field displays in place of the full distance whenever an exclusion is present + // Then the workspace's exclusion still applies, but to the newly selected distance: the distance field renders + // the reimbursable distance in place of the full one, so keeping the previous 3.49 would show the wrong expense const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); - expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); - expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); }); }); From 0e68465a8b3b67ec841316056062b02d339c09f0 Mon Sep 17 00:00:00 2001 From: alberto Date: Wed, 12 Aug 2026 14:22:22 +0200 Subject: [PATCH 03/12] Do not deduct if not needed --- src/libs/MergeTransactionUtils.ts | 47 +++++++++--- .../DynamicDetailsReviewPage.tsx | 6 ++ tests/actions/MergeTransactionTest.ts | 73 +++++++++++++------ 3 files changed, 96 insertions(+), 30 deletions(-) diff --git a/src/libs/MergeTransactionUtils.ts b/src/libs/MergeTransactionUtils.ts index 833ef1b8ce94..31a96b17ef88 100644 --- a/src/libs/MergeTransactionUtils.ts +++ b/src/libs/MergeTransactionUtils.ts @@ -6,6 +6,7 @@ import type {TranslationPaths} from '@src/languages/types'; import ONYXKEYS from '@src/ONYXKEYS'; import type {MergeTransaction, Policy, Report, SearchResults, Transaction} from '@src/types/onyx'; import type {Attendee} from '@src/types/onyx/IOU'; +import type {TransactionCustomUnit} from '@src/types/onyx/Transaction'; import type {NullishDeep, OnyxEntry} from 'react-native-onyx'; import type {TupleToUnion} from 'type-fest'; @@ -18,6 +19,7 @@ import type {TransactionDetails} from './ReportUtils'; import {getDecodedLeafCategoryName} from './CategoryUtils'; import {convertToBackendAmount} from './CurrencyUtils'; import Parser from './Parser'; +import {isCommuterExclusionEnabled} from './PolicyDistanceRatesUtils'; import {getCommaSeparatedTagNameWithSanitizedColons} from './PolicyUtils'; import {constructReceiptSourceFromFilename} from './ReceiptUtils'; import {getIOUActionForReportID} from './ReportActionsUtils'; @@ -47,6 +49,7 @@ import { // Define the specific merge fields we want to handle const MERGE_FIELDS = ['amount', 'merchant', 'created', 'category', 'tag', 'description', 'taxValue', 'reimbursable', 'billable', 'attendees', 'reportID'] as const; +const COMMUTER_EXCLUSION_CUSTOM_UNIT_KEYS = ['commuterExclusion', 'reimbursableDistance', 'commuterExclusionType', 'commuterExclusionMethod'] as const; // Some fields are dependant on others. We need to automatically derive the correct field values depending on user selection. const DERIVED_MERGE_FIELDS = [...MERGE_FIELDS, 'taxCode', 'taxAmount'] as const; type MergeFieldKey = TupleToUnion; @@ -638,8 +641,36 @@ type GetMergeFieldUpdatedValuesParams = { mergeTransaction?: OnyxEntry; searchReports?: Array>; policy?: OnyxEntry; + + /** Workspace of the report the merged expense will live on, which is the one whose rules apply to it */ + destinationPolicy?: OnyxEntry; }; +/** + * Build the custom unit's commuter exclusion for a merge selection. + * + * A commuter exclusion belongs to the workspace the surviving expense ends up on, and always describes the distance + * that is selected. Selections are stored with Onyx.merge, which deep merges the custom unit, so both the exclusion of + * a workspace that is no longer the destination and the reimbursable distance of a previously selected merchant would + * otherwise survive — and the distance field shows the reimbursable distance in place of the full distance. + */ +function getCommuterExclusionCustomUnitUpdate( + selectedCustomUnit: TransactionCustomUnit | undefined, + previousCustomUnit: TransactionCustomUnit | undefined, + destinationPolicy: OnyxEntry, +): Record { + const commuterExclusion = selectedCustomUnit?.commuterExclusion ?? previousCustomUnit?.commuterExclusion; + const quantity = selectedCustomUnit?.quantity ?? previousCustomUnit?.quantity; + + if (isCommuterExclusionEnabled(destinationPolicy) && !!commuterExclusion && typeof quantity === 'number') { + return {commuterExclusion, reimbursableDistance: Math.max(0, quantity - commuterExclusion)}; + } + + // Null the exclusion keys that hold a value so Onyx removes them, leaving the ones that were never set out of the update + const keysToClear = COMMUTER_EXCLUSION_CUSTOM_UNIT_KEYS.filter((key) => selectedCustomUnit?.[key] !== undefined || previousCustomUnit?.[key] !== undefined); + return Object.fromEntries(keysToClear.map((key) => [key, null])); +} + /** * Build updated values for merge transaction field selection * Handles special cases like currency for amount field, report name, tax value and additional fields for distance requests @@ -652,6 +683,7 @@ function getMergeFieldUpdatedValues({ mergeTransaction, searchReports, policy, + destinationPolicy, }: GetMergeFieldUpdatedValuesParams): MergeTransactionUpdateValues { const updatedValues: MergeTransactionUpdateValues = { [field]: fieldValue, @@ -668,24 +700,21 @@ function getMergeFieldUpdatedValues({ if (field === 'reportID') { const reportName = transaction?.reportName?.length ? transaction?.reportName : getReportName(getReportOrDraftReport(getReportIDForExpense(transaction), searchReports)); updatedValues.reportName = reportName.length ? reportName : null; + + // Moving the expense to another workspace changes whether that workspace excludes commuter distance from it + if (isDistanceRequest(transaction)) { + updatedValues.customUnit = getCommuterExclusionCustomUnitUpdate(undefined, mergeTransaction?.customUnit, destinationPolicy); + } } if (field === 'merchant' && isDistanceRequest(transaction)) { const transactionDetails = getTransactionDetails(transaction); updatedValues.amount = getMergeFieldValue(transactionDetails, transaction, 'amount') as number; updatedValues.currency = getCurrency(transaction); - // Selections are stored with Onyx.merge, which deep merges the custom unit, so the commuter exclusion of the - // surviving expense's workspace stays applied. Its reimbursable distance describes the previously selected - // distance though, and the distance field displays that in place of the full distance, so recompute it here. const selectedCustomUnit = transaction?.comment?.customUnit; - const commuterExclusion = selectedCustomUnit?.commuterExclusion ?? mergeTransaction?.customUnit?.commuterExclusion; - const selectedQuantity = selectedCustomUnit?.quantity; - const hasDistanceToExclude = !!commuterExclusion && typeof selectedQuantity === 'number'; updatedValues.customUnit = { ...selectedCustomUnit, - ...((hasDistanceToExclude || mergeTransaction?.customUnit?.reimbursableDistance !== undefined) && { - reimbursableDistance: hasDistanceToExclude ? Math.max(0, selectedQuantity - commuterExclusion) : null, - }), + ...getCommuterExclusionCustomUnitUpdate(selectedCustomUnit, mergeTransaction?.customUnit, destinationPolicy), }; updatedValues.iouRequestType = transaction?.iouRequestType; // For manual distance requests, set waypoints/routes and receipt to null to clear any existing values diff --git a/src/pages/TransactionMerge/DynamicDetailsReviewPage.tsx b/src/pages/TransactionMerge/DynamicDetailsReviewPage.tsx index 4e3262563ead..fb6d86790fd7 100644 --- a/src/pages/TransactionMerge/DynamicDetailsReviewPage.tsx +++ b/src/pages/TransactionMerge/DynamicDetailsReviewPage.tsx @@ -109,6 +109,11 @@ function DynamicDetailsReviewPage({route}: DynamicDetailsReviewPageProps) { return newErrors; }); + // Selecting a report is what moves the merged expense to a workspace, so it decides which workspace's rules + // apply to it. Any other selection leaves it on the report already chosen. + const destinationReportID = field === 'reportID' ? transaction.reportID : mergeTransaction?.reportID; + const destinationPolicy = destinationReportID === sourceTransactionReport?.reportID ? sourceTransactionPolicy : targetTransactionPolicy; + // Update both the field value and track which transaction was selected (persisted in Onyx) const currentSelections = mergeTransaction?.selectedTransactionByField ?? {}; const updatedValues = getMergeFieldUpdatedValues({ @@ -119,6 +124,7 @@ function DynamicDetailsReviewPage({route}: DynamicDetailsReviewPageProps) { mergeTransaction, searchReports: [targetTransactionReport, sourceTransactionReport], policy: transaction.transactionID === targetTransaction?.transactionID ? targetTransactionPolicy : sourceTransactionPolicy, + destinationPolicy: destinationReportID === CONST.REPORT.UNREPORTED_REPORT_ID ? undefined : destinationPolicy, }); setMergeTransactionKey(transactionID, { diff --git a/tests/actions/MergeTransactionTest.ts b/tests/actions/MergeTransactionTest.ts index bc58927ebde3..660e4b2ca9a5 100644 --- a/tests/actions/MergeTransactionTest.ts +++ b/tests/actions/MergeTransactionTest.ts @@ -12,6 +12,7 @@ import ONYXKEYS from '@src/ONYXKEYS'; import type { MergeTransaction as MergeTransactionType, OriginalMessageIOU, + Policy, Report, ReportAction, ReportActions, @@ -27,6 +28,7 @@ import Onyx from 'react-native-onyx'; import type {MockFetch} from '../utils/TestHelper'; import createRandomMergeTransaction from '../utils/collections/mergeTransaction'; +import createRandomPolicy from '../utils/collections/policies'; import createRandomReportAction from '../utils/collections/reportActions'; import {createExpenseReport, createRandomReport} from '../utils/collections/reports'; import createRandomTransaction, {createRandomDistanceRequestTransaction} from '../utils/collections/transaction'; @@ -1546,42 +1548,71 @@ describe('setMergeTransactionKey', () => { }); }); - it('should apply the commuter exclusion to the newly selected distance rather than the previously selected one', async () => { - // Given a merge of two distance expenses onto a workspace that excludes 1 commuter mile, where the merchant of - // the 4.49 mile expense was selected first + describe('commuter exclusion on a distance merge', () => { const transactionID = 'merge-distance-transaction'; - const selectMerchantOf = async (transaction: Transaction) => { + const plainWorkspaceReportID = 'reportOnPlainWorkspace'; + const excludingWorkspace: Policy = { + ...createRandomPolicy(0, CONST.POLICY.TYPE.TEAM), + commuterExclusions: {method: CONST.POLICY.COMMUTER_EXCLUSION_METHOD.FIXED_DISTANCE, fixedDistance: 1, fixedDistanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES}, + }; + const plainWorkspace = createRandomPolicy(1, CONST.POLICY.TYPE.TEAM); + const fourMileExpense = { + ...createRandomDistanceRequestTransaction(0), + comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 4.49, commuterExclusion: 1, reimbursableDistance: 3.49}}, + }; + const tenMileExpense = { + ...createRandomDistanceRequestTransaction(1), + reportID: plainWorkspaceReportID, + comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 10.2}}, + }; + + const select = async (transaction: Transaction, field: 'merchant' | 'reportID', fieldValue: string, destinationPolicy: Policy) => { setMergeTransactionKey( transactionID, getMergeFieldUpdatedValues({ transaction, - field: 'merchant', - fieldValue: transaction.merchant, + field, + fieldValue, getCurrencyDecimals: getCurrencyDecimalsLocal, mergeTransaction: await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`), + destinationPolicy, }), ); await waitForBatchedUpdates(); }; - await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); - await selectMerchantOf({ - ...createRandomDistanceRequestTransaction(0), - comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 4.49, commuterExclusion: 1, reimbursableDistance: 3.49}}, + it('should apply the exclusion to the newly selected distance rather than the previously selected one', async () => { + // Given a merge onto a workspace that excludes 1 commuter mile, where the merchant of the 4.49 mile expense + // was selected first + await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); + await select(fourMileExpense, 'merchant', fourMileExpense.merchant, excludingWorkspace); + + // When the merchant of the 10.2 mile expense is selected instead + await select(tenMileExpense, 'merchant', tenMileExpense.merchant, excludingWorkspace); + + // Then the exclusion still applies, but to the newly selected distance: the distance field renders the + // reimbursable distance in place of the full one, so keeping the previous 3.49 would show the wrong expense + const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); + expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); }); - // When the merchant of the 10.2 mile expense is selected instead - await selectMerchantOf({ - ...createRandomDistanceRequestTransaction(1), - comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 10.2}}, - }); + it('should drop the exclusion when the report of a workspace that has none is selected', async () => { + // Given the same merge, with the exclusion applied to the selected 10.2 mile distance + await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); + await select(fourMileExpense, 'merchant', fourMileExpense.merchant, excludingWorkspace); + await select(tenMileExpense, 'merchant', tenMileExpense.merchant, excludingWorkspace); + + // When the report on the workspace that excludes nothing is selected for the merged expense + await select(tenMileExpense, 'reportID', plainWorkspaceReportID, plainWorkspace); - // Then the workspace's exclusion still applies, but to the newly selected distance: the distance field renders - // the reimbursable distance in place of the full one, so keeping the previous 3.49 would show the wrong expense - const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); - expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); - expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); - expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); + // Then nothing is deducted from it anymore, because the exclusion belongs to the workspace the expense left + const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); + expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); + }); }); }); From 079d8a05262724fb439c91341ee162dfd1ac7723 Mon Sep 17 00:00:00 2001 From: alberto Date: Wed, 12 Aug 2026 15:16:40 +0200 Subject: [PATCH 04/12] revamp of tests --- src/libs/DistanceRequestUtils.ts | 30 ++- src/libs/MergeTransactionUtils.ts | 78 ++++++- .../DynamicDetailsReviewPage.tsx | 18 +- tests/actions/MergeTransactionTest.ts | 23 +- .../ui/MergeTransactionDetailsReviewTest.tsx | 203 ++++++++++++++++++ 5 files changed, 325 insertions(+), 27 deletions(-) create mode 100644 tests/ui/MergeTransactionDetailsReviewTest.tsx diff --git a/src/libs/DistanceRequestUtils.ts b/src/libs/DistanceRequestUtils.ts index 7748d3bc0a3d..c933e379a850 100644 --- a/src/libs/DistanceRequestUtils.ts +++ b/src/libs/DistanceRequestUtils.ts @@ -337,6 +337,25 @@ function getCommuterExclusionDisplayData(customUnit: TransactionCustomUnit | und }; } +/** + * Returns the distance a workspace excludes from a distance of `distance` units, expressed in that same unit. + * + * Returns 0 when the workspace excludes nothing, so callers can treat it as "no exclusion applies". The exclusion never + * exceeds the distance itself, which is what keeps a reimbursable distance from going negative. + */ +function getPolicyCommuterExclusionForDistance(policy: OnyxEntry, distance: number, distanceUnit: Unit): number { + const commuterExclusions = policy?.commuterExclusions; + if (commuterExclusions?.method !== CONST.POLICY.COMMUTER_EXCLUSION_METHOD.FIXED_DISTANCE) { + return 0; + } + + const fixedDistanceUnit: Unit = + commuterExclusions.fixedDistanceUnit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_KILOMETERS ? CONST.CUSTOM_UNITS.DISTANCE_UNIT_KILOMETERS : CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES; + const fixedDistanceInRequestUnit = convertDistanceUnit(convertToDistanceInMeters(commuterExclusions.fixedDistance ?? 0, fixedDistanceUnit), distanceUnit); + + return Math.max(0, Math.min(fixedDistanceInRequestUnit, distance)); +} + function getTransactionCommuterExclusionData({ transaction, policy, @@ -385,15 +404,7 @@ function getTransactionCommuterExclusionData({ return; } - const fixedDistanceUnit: Unit = - policyCommuterExclusions.fixedDistanceUnit === CONST.CUSTOM_UNITS.DISTANCE_UNIT_KILOMETERS ? CONST.CUSTOM_UNITS.DISTANCE_UNIT_KILOMETERS : CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES; - const fixedDistanceInRequestUnit = convertDistanceUnit(convertToDistanceInMeters(policyCommuterExclusions.fixedDistance ?? 0, fixedDistanceUnit), requestDistanceUnit); - - if (fixedDistanceInRequestUnit <= 0) { - return; - } - - const commuterExclusion = Math.min(fixedDistanceInRequestUnit, routeDistance); + const commuterExclusion = getPolicyCommuterExclusionForDistance(policy, routeDistance, requestDistanceUnit); if (commuterExclusion <= 0) { return; } @@ -849,6 +860,7 @@ export default { getDistanceMerchant, getDistanceRequestAmount, getCommuterExclusionDisplayData, + getPolicyCommuterExclusionForDistance, getTransactionCommuterExclusionData, getDistanceDisplayDetailsWithCommuter, getFormattedRateValue, diff --git a/src/libs/MergeTransactionUtils.ts b/src/libs/MergeTransactionUtils.ts index 31a96b17ef88..dcfb34810c55 100644 --- a/src/libs/MergeTransactionUtils.ts +++ b/src/libs/MergeTransactionUtils.ts @@ -18,8 +18,8 @@ import type {TransactionDetails} from './ReportUtils'; import {getDecodedLeafCategoryName} from './CategoryUtils'; import {convertToBackendAmount} from './CurrencyUtils'; +import DistanceRequestUtils from './DistanceRequestUtils'; import Parser from './Parser'; -import {isCommuterExclusionEnabled} from './PolicyDistanceRatesUtils'; import {getCommaSeparatedTagNameWithSanitizedColons} from './PolicyUtils'; import {constructReceiptSourceFromFilename} from './ReceiptUtils'; import {getIOUActionForReportID} from './ReportActionsUtils'; @@ -50,6 +50,8 @@ import { // Define the specific merge fields we want to handle const MERGE_FIELDS = ['amount', 'merchant', 'created', 'category', 'tag', 'description', 'taxValue', 'reimbursable', 'billable', 'attendees', 'reportID'] as const; const COMMUTER_EXCLUSION_CUSTOM_UNIT_KEYS = ['commuterExclusion', 'reimbursableDistance', 'commuterExclusionType', 'commuterExclusionMethod'] as const; +/** The custom unit's commuter exclusion fields, where null clears the stored value through Onyx.merge */ +type CommuterExclusionCustomUnitUpdate = {[Key in TupleToUnion]?: TransactionCustomUnit[Key] | null}; // Some fields are dependant on others. We need to automatically derive the correct field values depending on user selection. const DERIVED_MERGE_FIELDS = [...MERGE_FIELDS, 'taxCode', 'taxAmount'] as const; type MergeFieldKey = TupleToUnion; @@ -658,17 +660,54 @@ function getCommuterExclusionCustomUnitUpdate( selectedCustomUnit: TransactionCustomUnit | undefined, previousCustomUnit: TransactionCustomUnit | undefined, destinationPolicy: OnyxEntry, -): Record { - const commuterExclusion = selectedCustomUnit?.commuterExclusion ?? previousCustomUnit?.commuterExclusion; +): CommuterExclusionCustomUnitUpdate { const quantity = selectedCustomUnit?.quantity ?? previousCustomUnit?.quantity; + const distanceUnit = selectedCustomUnit?.distanceUnit ?? previousCustomUnit?.distanceUnit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES; + // The destination workspace's setting decides the exclusion, so it applies to an expense that arrives from a + // workspace excluding nothing, and stops applying to one that leaves a workspace that excludes a distance + const commuterExclusion = typeof quantity === 'number' ? DistanceRequestUtils.getPolicyCommuterExclusionForDistance(destinationPolicy, quantity, distanceUnit) : 0; - if (isCommuterExclusionEnabled(destinationPolicy) && !!commuterExclusion && typeof quantity === 'number') { - return {commuterExclusion, reimbursableDistance: Math.max(0, quantity - commuterExclusion)}; + if (commuterExclusion > 0 && typeof quantity === 'number') { + return { + commuterExclusion, + reimbursableDistance: Math.max(0, quantity - commuterExclusion), + commuterExclusionMethod: CONST.POLICY.COMMUTER_EXCLUSION_METHOD.FIXED_DISTANCE, + }; } // Null the exclusion keys that hold a value so Onyx removes them, leaving the ones that were never set out of the update - const keysToClear = COMMUTER_EXCLUSION_CUSTOM_UNIT_KEYS.filter((key) => selectedCustomUnit?.[key] !== undefined || previousCustomUnit?.[key] !== undefined); - return Object.fromEntries(keysToClear.map((key) => [key, null])); + const clearedUpdate: CommuterExclusionCustomUnitUpdate = {}; + for (const key of COMMUTER_EXCLUSION_CUSTOM_UNIT_KEYS) { + if (selectedCustomUnit?.[key] === undefined && previousCustomUnit?.[key] === undefined) { + continue; + } + clearedUpdate[key] = null; + } + + return clearedUpdate; +} + +/** + * Scales an amount that pays for `billedDistance` so that it pays for `newBilledDistance` instead, keeping a merged + * distance expense's amount on the distance its workspace reimburses once a commuter exclusion is applied or dropped. + */ +function getAmountForBilledDistance(amount: number, billedDistance: number | undefined, newBilledDistance: number | undefined): number { + if (!amount || !billedDistance || newBilledDistance === undefined || billedDistance === newBilledDistance) { + return amount; + } + + return Math.round((amount / billedDistance) * newBilledDistance); +} + +/** + * The distance an amount pays for: the reimbursable distance when a commuter exclusion applies, otherwise the whole distance. + */ +function getBilledDistance(quantity: number | null | undefined, reimbursableDistance: number | null | undefined): number | undefined { + if (typeof reimbursableDistance === 'number') { + return reimbursableDistance; + } + + return typeof quantity === 'number' ? quantity : undefined; } /** @@ -701,21 +740,38 @@ function getMergeFieldUpdatedValues({ const reportName = transaction?.reportName?.length ? transaction?.reportName : getReportName(getReportOrDraftReport(getReportIDForExpense(transaction), searchReports)); updatedValues.reportName = reportName.length ? reportName : null; - // Moving the expense to another workspace changes whether that workspace excludes commuter distance from it + // Moving the expense to another workspace changes whether that workspace excludes commuter distance from it, + // and the amount has to follow the distance that is left to reimburse if (isDistanceRequest(transaction)) { - updatedValues.customUnit = getCommuterExclusionCustomUnitUpdate(undefined, mergeTransaction?.customUnit, destinationPolicy); + const previousCustomUnit = mergeTransaction?.customUnit; + const commuterExclusionUpdate = getCommuterExclusionCustomUnitUpdate(undefined, previousCustomUnit, destinationPolicy); + updatedValues.customUnit = commuterExclusionUpdate; + if (typeof mergeTransaction?.amount === 'number') { + updatedValues.amount = getAmountForBilledDistance( + mergeTransaction.amount, + getBilledDistance(previousCustomUnit?.quantity, previousCustomUnit?.reimbursableDistance), + getBilledDistance(previousCustomUnit?.quantity, commuterExclusionUpdate.reimbursableDistance), + ); + } } } if (field === 'merchant' && isDistanceRequest(transaction)) { const transactionDetails = getTransactionDetails(transaction); - updatedValues.amount = getMergeFieldValue(transactionDetails, transaction, 'amount') as number; updatedValues.currency = getCurrency(transaction); const selectedCustomUnit = transaction?.comment?.customUnit; + const commuterExclusionUpdate = getCommuterExclusionCustomUnitUpdate(selectedCustomUnit, mergeTransaction?.customUnit, destinationPolicy); updatedValues.customUnit = { ...selectedCustomUnit, - ...getCommuterExclusionCustomUnitUpdate(selectedCustomUnit, mergeTransaction?.customUnit, destinationPolicy), + ...commuterExclusionUpdate, }; + // The selected expense's amount pays for the distance its own workspace reimbursed, so it is re-scaled to the + // distance the destination workspace reimburses + updatedValues.amount = getAmountForBilledDistance( + getMergeFieldValue(transactionDetails, transaction, 'amount') as number, + getBilledDistance(selectedCustomUnit?.quantity, selectedCustomUnit?.reimbursableDistance), + getBilledDistance(selectedCustomUnit?.quantity, commuterExclusionUpdate.reimbursableDistance), + ); updatedValues.iouRequestType = transaction?.iouRequestType; // For manual distance requests, set waypoints/routes and receipt to null to clear any existing values updatedValues.receipt = transaction?.receipt ?? null; diff --git a/src/pages/TransactionMerge/DynamicDetailsReviewPage.tsx b/src/pages/TransactionMerge/DynamicDetailsReviewPage.tsx index fb6d86790fd7..d120d24726ec 100644 --- a/src/pages/TransactionMerge/DynamicDetailsReviewPage.tsx +++ b/src/pages/TransactionMerge/DynamicDetailsReviewPage.tsx @@ -62,6 +62,11 @@ function DynamicDetailsReviewPage({route}: DynamicDetailsReviewPageProps) { const sourceReportOwnerAsAttendee = useReportOwnerAsAttendee(sourceTransaction); const targetReportOwnerAsAttendee = useReportOwnerAsAttendee(targetTransaction); + // The workspace of the report the merged expense is currently headed to, which is the one whose rules apply to it. + // An unreported expense lands in the self-DM, which has no workspace, so nothing resolves here. + const [chosenReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getNonEmptyStringOnyxID(mergeTransaction?.reportID)}`); + const [chosenReportPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${getNonEmptyStringOnyxID(chosenReport?.policyID)}`); + const [hasErrors, setHasErrors] = useState>>({}); const conflictFields = useMemo(() => { @@ -109,10 +114,7 @@ function DynamicDetailsReviewPage({route}: DynamicDetailsReviewPageProps) { return newErrors; }); - // Selecting a report is what moves the merged expense to a workspace, so it decides which workspace's rules - // apply to it. Any other selection leaves it on the report already chosen. - const destinationReportID = field === 'reportID' ? transaction.reportID : mergeTransaction?.reportID; - const destinationPolicy = destinationReportID === sourceTransactionReport?.reportID ? sourceTransactionPolicy : targetTransactionPolicy; + const selectedTransactionPolicy = transaction.transactionID === targetTransaction?.transactionID ? targetTransactionPolicy : sourceTransactionPolicy; // Update both the field value and track which transaction was selected (persisted in Onyx) const currentSelections = mergeTransaction?.selectedTransactionByField ?? {}; @@ -123,8 +125,11 @@ function DynamicDetailsReviewPage({route}: DynamicDetailsReviewPageProps) { getCurrencyDecimals, mergeTransaction, searchReports: [targetTransactionReport, sourceTransactionReport], - policy: transaction.transactionID === targetTransaction?.transactionID ? targetTransactionPolicy : sourceTransactionPolicy, - destinationPolicy: destinationReportID === CONST.REPORT.UNREPORTED_REPORT_ID ? undefined : destinationPolicy, + policy: selectedTransactionPolicy, + // Selecting a report is what moves the merged expense to a workspace, so it decides which workspace's + // rules apply to it: the workspace of the expense whose report was picked. Any other selection leaves + // the expense on the report already chosen, whose workspace is resolved from Onyx above. + destinationPolicy: field === 'reportID' ? selectedTransactionPolicy : chosenReportPolicy, }); setMergeTransactionKey(transactionID, { @@ -143,6 +148,7 @@ function DynamicDetailsReviewPage({route}: DynamicDetailsReviewPageProps) { targetTransaction?.transactionID, targetTransactionPolicy, sourceTransactionPolicy, + chosenReportPolicy, getCurrencyDecimals, ], ); diff --git a/tests/actions/MergeTransactionTest.ts b/tests/actions/MergeTransactionTest.ts index 660e4b2ca9a5..115e66cbeb63 100644 --- a/tests/actions/MergeTransactionTest.ts +++ b/tests/actions/MergeTransactionTest.ts @@ -1558,10 +1558,12 @@ describe('setMergeTransactionKey', () => { const plainWorkspace = createRandomPolicy(1, CONST.POLICY.TYPE.TEAM); const fourMileExpense = { ...createRandomDistanceRequestTransaction(0), + amount: -449, comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 4.49, commuterExclusion: 1, reimbursableDistance: 3.49}}, }; const tenMileExpense = { ...createRandomDistanceRequestTransaction(1), + amount: -1020, reportID: plainWorkspaceReportID, comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 10.2}}, }; @@ -1598,6 +1600,23 @@ describe('setMergeTransactionKey', () => { expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); }); + it('should apply the exclusion to an expense arriving from a workspace that excludes nothing', async () => { + // Given a merge where the merchant of the 10.2 mile expense, which carries no exclusion of its own, is selected + await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); + await select(tenMileExpense, 'merchant', tenMileExpense.merchant, plainWorkspace); + + // When the report on the workspace that excludes 1 commuter mile is selected for the merged expense + await select(fourMileExpense, 'reportID', 'reportOnExcludingWorkspace', excludingWorkspace); + + // Then that workspace's exclusion is deducted from the selected distance, and the amount pays for what is + // left of it rather than for the whole trip + const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); + expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); + expect(mergeTransaction?.amount).toBe(920); + }); + it('should drop the exclusion when the report of a workspace that has none is selected', async () => { // Given the same merge, with the exclusion applied to the selected 10.2 mile distance await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); @@ -1607,11 +1626,13 @@ describe('setMergeTransactionKey', () => { // When the report on the workspace that excludes nothing is selected for the merged expense await select(tenMileExpense, 'reportID', plainWorkspaceReportID, plainWorkspace); - // Then nothing is deducted from it anymore, because the exclusion belongs to the workspace the expense left + // Then nothing is deducted from it anymore, because the exclusion belongs to the workspace the expense left, + // and the amount pays for the whole trip again const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); + expect(mergeTransaction?.amount).toBe(1020); }); }); }); diff --git a/tests/ui/MergeTransactionDetailsReviewTest.tsx b/tests/ui/MergeTransactionDetailsReviewTest.tsx new file mode 100644 index 000000000000..ff45717d8131 --- /dev/null +++ b/tests/ui/MergeTransactionDetailsReviewTest.tsx @@ -0,0 +1,203 @@ +import {act, fireEvent, render, screen} from '@testing-library/react-native'; + +import ComposeProviders from '@components/ComposeProviders'; +import {LocaleContextProvider} from '@components/LocaleContextProvider'; +import OnyxListItemProvider from '@components/OnyxListItemProvider'; + +import navigationRef from '@libs/Navigation/navigationRef'; +import createPlatformStackNavigator from '@libs/Navigation/PlatformStackNavigation/createPlatformStackNavigator'; + +import DynamicConfirmationPage from '@pages/TransactionMerge/DynamicConfirmationPage'; +import DynamicDetailsReviewPage from '@pages/TransactionMerge/DynamicDetailsReviewPage'; + +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; +import SCREENS from '@src/SCREENS'; +import type {Policy, Report, Transaction} from '@src/types/onyx'; + +import {NavigationContainer} from '@react-navigation/native'; +import React from 'react'; +import Onyx from 'react-native-onyx'; + +import createRandomPolicy from '../utils/collections/policies'; +import {createRandomReport} from '../utils/collections/reports'; +import {createRandomDistanceRequestTransaction} from '../utils/collections/transaction'; +import getOnyxValue from '../utils/getOnyxValue'; +import * as TestHelper from '../utils/TestHelper'; +import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; + +jest.mock('@hooks/useDynamicBackPath', () => jest.fn(() => '')); + +const Stack = createPlatformStackNavigator>(); + +// Expose each field row's description and title so the rendered distance can be read back +jest.mock('@components/MenuItemWithTopDescription', () => { + const RN = jest.requireActual>>('react-native'); + return ({description, title}: {description?: string; title?: string}) => ( + + {title} + + ); +}); + +TestHelper.setupGlobalFetchMock(); + +const MERGE_TRANSACTION_ID = 'mergeDistanceTransaction'; +const EXCLUDING_POLICY_ID = 'policyThatExcludesCommuterDistance'; +const PLAIN_POLICY_ID = 'policyThatExcludesNothing'; +const EXCLUDING_REPORT_ID = '4444'; +const PLAIN_REPORT_ID = '5555'; + +const excludingPolicy: Policy = { + ...createRandomPolicy(0, CONST.POLICY.TYPE.TEAM), + id: EXCLUDING_POLICY_ID, + name: 'Workspace that excludes 1 mile', + commuterExclusions: {method: CONST.POLICY.COMMUTER_EXCLUSION_METHOD.FIXED_DISTANCE, fixedDistance: 1, fixedDistanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES}, +}; +const plainPolicy: Policy = {...createRandomPolicy(1, CONST.POLICY.TYPE.TEAM), id: PLAIN_POLICY_ID, name: 'Workspace that excludes nothing'}; + +const buildReport = (reportID: string, policyID: string, reportName: string): Report => ({ + ...createRandomReport(Number(reportID)), + reportID, + policyID, + reportName, + type: CONST.REPORT.TYPE.EXPENSE, + stateNum: CONST.REPORT.STATE_NUM.OPEN, + statusNum: CONST.REPORT.STATUS_NUM.OPEN, +}); + +const buildDistanceExpense = (transactionID: string, reportID: string, merchant: string, quantity: number, commuterExclusion?: number): Transaction => ({ + ...createRandomDistanceRequestTransaction(quantity), + transactionID, + reportID, + merchant, + modifiedMerchant: merchant, + amount: -1020, + currency: CONST.CURRENCY.USD, + created: '2026-08-01', + iouRequestType: CONST.IOU.REQUEST_TYPE.DISTANCE_MAP, + comment: { + type: CONST.TRANSACTION.TYPE.CUSTOM_UNIT, + customUnit: { + name: CONST.CUSTOM_UNITS.NAME_DISTANCE, + quantity, + distanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, + ...(commuterExclusion !== undefined && {commuterExclusion, reimbursableDistance: quantity - commuterExclusion}), + }, + waypoints: {waypoint0: {address: `${merchant} start`}, waypoint1: {address: `${merchant} end`}}, + }, +}); + +// The expense being merged into lives on the workspace that excludes nothing, and the other one on the workspace that +// excludes a commuter mile, which is the combination that decides whether the merged expense gets a deduction +const targetExpense = buildDistanceExpense('targetTransaction', PLAIN_REPORT_ID, '10.20 mi @ $0.67 / mi', 10.2); +const sourceExpense = buildDistanceExpense('sourceTransaction', EXCLUDING_REPORT_ID, '4.49 mi @ $0.67 / mi', 4.49, 1); + +describe('Merging distance expenses across workspaces', () => { + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + }); + + beforeEach(async () => { + await act(async () => { + await Onyx.clear(); + await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${EXCLUDING_POLICY_ID}`, excludingPolicy); + await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${PLAIN_POLICY_ID}`, plainPolicy); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${EXCLUDING_REPORT_ID}`, buildReport(EXCLUDING_REPORT_ID, EXCLUDING_POLICY_ID, 'Report that deducts')); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${PLAIN_REPORT_ID}`, buildReport(PLAIN_REPORT_ID, PLAIN_POLICY_ID, 'Report that does not deduct')); + await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${targetExpense.transactionID}`, targetExpense); + await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${sourceExpense.transactionID}`, sourceExpense); + await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${MERGE_TRANSACTION_ID}`, { + targetTransactionID: targetExpense.transactionID, + sourceTransactionID: sourceExpense.transactionID, + eligibleTransactions: [targetExpense, sourceExpense], + }); + }); + await waitForBatchedUpdatesWithAct(); + }); + + const renderPage = async () => { + render( + + + + + + + , + ); + await waitForBatchedUpdatesWithAct(); + }; + + const press = async (label: string) => { + const option = screen.getAllByLabelText(label).at(0); + if (!option) { + throw new Error(`No option to select for ${label}`); + } + fireEvent.press(option); + await waitForBatchedUpdatesWithAct(); + }; + + it("applies the destination workspace's commuter exclusion to the distance that was selected", async () => { + await renderPage(); + + // When the distance of the expense from the workspace that excludes nothing is selected, and the merged expense + // is put on the report of the workspace that excludes a commuter mile + await press(targetExpense.merchant); + await press('Report that deducts'); + + // Then that workspace's exclusion is deducted from the selected 10.2 mile distance + const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${MERGE_TRANSACTION_ID}`); + expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); + // And the amount pays for the 9.2 miles that are left rather than the whole trip + expect(mergeTransaction?.amount).toBe(920); + }); + + it('deducts nothing when the merged expense is put on the report of a workspace that excludes nothing', async () => { + await renderPage(); + + // When the distance of the expense from the workspace that excludes a commuter mile is selected, but the merged + // expense is put on the report of the workspace that excludes nothing + await press(sourceExpense.merchant); + await press('Report that does not deduct'); + + // Then nothing is deducted from it + const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${MERGE_TRANSACTION_ID}`); + expect(mergeTransaction?.customUnit?.quantity).toBe(4.49); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); + }); + + it('shows the deducted distance on the confirmation page', async () => { + // Given the distance of the expense from the workspace that excludes nothing selected, on the report of the + // workspace that excludes a commuter mile + await renderPage(); + await press(targetExpense.merchant); + await press('Report that deducts'); + + // When the confirmation page is opened for that merge + render( + + + + + + + , + ); + await waitForBatchedUpdatesWithAct(); + + // Then the distance field shows the reimbursable distance, alongside the distance it was deducted from + expect(screen.getByTestId('field-Distance • Original: 10.20 mi')).toHaveTextContent('9.20 mi'); + }); +}); From 16da227b3652f9c9fb61a9f1a3a8c9ede912ac7c Mon Sep 17 00:00:00 2001 From: alberto Date: Wed, 12 Aug 2026 15:23:55 +0200 Subject: [PATCH 05/12] test simplification --- tests/actions/MergeTransactionTest.ts | 90 +++++-------------- .../ui/MergeTransactionDetailsReviewTest.tsx | 50 +++++------ 2 files changed, 44 insertions(+), 96 deletions(-) diff --git a/tests/actions/MergeTransactionTest.ts b/tests/actions/MergeTransactionTest.ts index 115e66cbeb63..22e99fcff21f 100644 --- a/tests/actions/MergeTransactionTest.ts +++ b/tests/actions/MergeTransactionTest.ts @@ -1548,92 +1548,50 @@ describe('setMergeTransactionKey', () => { }); }); - describe('commuter exclusion on a distance merge', () => { + it('should apply the commuter exclusion to the newly selected distance rather than the previously selected one', async () => { + // Given a merge onto a workspace that excludes 1 commuter mile, where the merchant of the 4.49 mile expense was + // selected first const transactionID = 'merge-distance-transaction'; - const plainWorkspaceReportID = 'reportOnPlainWorkspace'; const excludingWorkspace: Policy = { ...createRandomPolicy(0, CONST.POLICY.TYPE.TEAM), commuterExclusions: {method: CONST.POLICY.COMMUTER_EXCLUSION_METHOD.FIXED_DISTANCE, fixedDistance: 1, fixedDistanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES}, }; - const plainWorkspace = createRandomPolicy(1, CONST.POLICY.TYPE.TEAM); - const fourMileExpense = { - ...createRandomDistanceRequestTransaction(0), - amount: -449, - comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 4.49, commuterExclusion: 1, reimbursableDistance: 3.49}}, - }; - const tenMileExpense = { - ...createRandomDistanceRequestTransaction(1), - amount: -1020, - reportID: plainWorkspaceReportID, - comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 10.2}}, - }; - - const select = async (transaction: Transaction, field: 'merchant' | 'reportID', fieldValue: string, destinationPolicy: Policy) => { + const selectMerchantOf = async (transaction: Transaction) => { setMergeTransactionKey( transactionID, getMergeFieldUpdatedValues({ transaction, - field, - fieldValue, + field: 'merchant', + fieldValue: transaction.merchant, getCurrencyDecimals: getCurrencyDecimalsLocal, mergeTransaction: await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`), - destinationPolicy, + destinationPolicy: excludingWorkspace, }), ); await waitForBatchedUpdates(); }; - it('should apply the exclusion to the newly selected distance rather than the previously selected one', async () => { - // Given a merge onto a workspace that excludes 1 commuter mile, where the merchant of the 4.49 mile expense - // was selected first - await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); - await select(fourMileExpense, 'merchant', fourMileExpense.merchant, excludingWorkspace); - - // When the merchant of the 10.2 mile expense is selected instead - await select(tenMileExpense, 'merchant', tenMileExpense.merchant, excludingWorkspace); - - // Then the exclusion still applies, but to the newly selected distance: the distance field renders the - // reimbursable distance in place of the full one, so keeping the previous 3.49 would show the wrong expense - const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); - expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); - expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); - expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); + await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); + await selectMerchantOf({ + ...createRandomDistanceRequestTransaction(0), + amount: -349, + comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 4.49, commuterExclusion: 1, reimbursableDistance: 3.49}}, }); - it('should apply the exclusion to an expense arriving from a workspace that excludes nothing', async () => { - // Given a merge where the merchant of the 10.2 mile expense, which carries no exclusion of its own, is selected - await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); - await select(tenMileExpense, 'merchant', tenMileExpense.merchant, plainWorkspace); - - // When the report on the workspace that excludes 1 commuter mile is selected for the merged expense - await select(fourMileExpense, 'reportID', 'reportOnExcludingWorkspace', excludingWorkspace); - - // Then that workspace's exclusion is deducted from the selected distance, and the amount pays for what is - // left of it rather than for the whole trip - const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); - expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); - expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); - expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); - expect(mergeTransaction?.amount).toBe(920); + // When the merchant of the 10.2 mile expense is selected instead + await selectMerchantOf({ + ...createRandomDistanceRequestTransaction(1), + amount: -1020, + comment: {customUnit: {name: CONST.CUSTOM_UNITS.NAME_DISTANCE, quantity: 10.2}}, }); - it('should drop the exclusion when the report of a workspace that has none is selected', async () => { - // Given the same merge, with the exclusion applied to the selected 10.2 mile distance - await Onyx.set(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`, {targetTransactionID: transactionID}); - await select(fourMileExpense, 'merchant', fourMileExpense.merchant, excludingWorkspace); - await select(tenMileExpense, 'merchant', tenMileExpense.merchant, excludingWorkspace); - - // When the report on the workspace that excludes nothing is selected for the merged expense - await select(tenMileExpense, 'reportID', plainWorkspaceReportID, plainWorkspace); - - // Then nothing is deducted from it anymore, because the exclusion belongs to the workspace the expense left, - // and the amount pays for the whole trip again - const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); - expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); - expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); - expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); - expect(mergeTransaction?.amount).toBe(1020); - }); + // Then the exclusion still applies, but to the newly selected distance: the distance field renders the + // reimbursable distance in place of the full one, so keeping the previous 3.49 would show the wrong expense + const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${transactionID}`); + expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); + expect(mergeTransaction?.amount).toBe(920); }); }); diff --git a/tests/ui/MergeTransactionDetailsReviewTest.tsx b/tests/ui/MergeTransactionDetailsReviewTest.tsx index ff45717d8131..edf26feb5173 100644 --- a/tests/ui/MergeTransactionDetailsReviewTest.tsx +++ b/tests/ui/MergeTransactionDetailsReviewTest.tsx @@ -72,7 +72,8 @@ const buildDistanceExpense = (transactionID: string, reportID: string, merchant: reportID, merchant, modifiedMerchant: merchant, - amount: -1020, + // A dollar per billed mile, so the amount and the distance stay readable against each other + amount: -Math.round((commuterExclusion === undefined ? quantity : quantity - commuterExclusion) * 100), currency: CONST.CURRENCY.USD, created: '2026-08-01', iouRequestType: CONST.IOU.REQUEST_TYPE.DISTANCE_MAP, @@ -150,38 +151,14 @@ describe('Merging distance expenses across workspaces', () => { await press(targetExpense.merchant); await press('Report that deducts'); - // Then that workspace's exclusion is deducted from the selected 10.2 mile distance + // Then that workspace's exclusion is deducted from the selected 10.2 mile distance, and the amount pays for the + // 9.2 miles that are left rather than for the whole trip const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${MERGE_TRANSACTION_ID}`); - expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); - // And the amount pays for the 9.2 miles that are left rather than the whole trip expect(mergeTransaction?.amount).toBe(920); - }); - - it('deducts nothing when the merged expense is put on the report of a workspace that excludes nothing', async () => { - await renderPage(); - - // When the distance of the expense from the workspace that excludes a commuter mile is selected, but the merged - // expense is put on the report of the workspace that excludes nothing - await press(sourceExpense.merchant); - await press('Report that does not deduct'); - - // Then nothing is deducted from it - const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${MERGE_TRANSACTION_ID}`); - expect(mergeTransaction?.customUnit?.quantity).toBe(4.49); - expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); - expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); - }); - - it('shows the deducted distance on the confirmation page', async () => { - // Given the distance of the expense from the workspace that excludes nothing selected, on the report of the - // workspace that excludes a commuter mile - await renderPage(); - await press(targetExpense.merchant); - await press('Report that deducts'); - // When the confirmation page is opened for that merge + // And the confirmation page shows the reimbursable distance, alongside the distance it was deducted from render( @@ -196,8 +173,21 @@ describe('Merging distance expenses across workspaces', () => { , ); await waitForBatchedUpdatesWithAct(); - - // Then the distance field shows the reimbursable distance, alongside the distance it was deducted from expect(screen.getByTestId('field-Distance • Original: 10.20 mi')).toHaveTextContent('9.20 mi'); }); + + it('deducts nothing when the merged expense is put on the report of a workspace that excludes nothing', async () => { + await renderPage(); + + // When the distance of the expense from the workspace that excludes a commuter mile is selected, but the merged + // expense is put on the report of the workspace that excludes nothing + await press(sourceExpense.merchant); + await press('Report that does not deduct'); + + // Then nothing is deducted from it, and the amount pays for the whole trip + const mergeTransaction = await getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${MERGE_TRANSACTION_ID}`); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); + expect(mergeTransaction?.amount).toBe(449); + }); }); From c0115722a46e081554b8fe04527d5f203cf94849 Mon Sep 17 00:00:00 2001 From: alberto Date: Wed, 12 Aug 2026 17:15:32 +0200 Subject: [PATCH 06/12] better comment --- src/libs/MergeTransactionUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/MergeTransactionUtils.ts b/src/libs/MergeTransactionUtils.ts index dcfb34810c55..0eba00eaecac 100644 --- a/src/libs/MergeTransactionUtils.ts +++ b/src/libs/MergeTransactionUtils.ts @@ -654,7 +654,7 @@ type GetMergeFieldUpdatedValuesParams = { * A commuter exclusion belongs to the workspace the surviving expense ends up on, and always describes the distance * that is selected. Selections are stored with Onyx.merge, which deep merges the custom unit, so both the exclusion of * a workspace that is no longer the destination and the reimbursable distance of a previously selected merchant would - * otherwise survive — and the distance field shows the reimbursable distance in place of the full distance. + * otherwise survive. The distance field then shows the reimbursable distance in place of the full distance. */ function getCommuterExclusionCustomUnitUpdate( selectedCustomUnit: TransactionCustomUnit | undefined, From 35bb35c6b3058c64d67c81e2a37b488af6952e9c Mon Sep 17 00:00:00 2001 From: alberto Date: Thu, 13 Aug 2026 00:23:51 +0200 Subject: [PATCH 07/12] fix test --- tests/ui/MergeTransactionDetailsReviewTest.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/ui/MergeTransactionDetailsReviewTest.tsx b/tests/ui/MergeTransactionDetailsReviewTest.tsx index edf26feb5173..7d0c6499dda3 100644 --- a/tests/ui/MergeTransactionDetailsReviewTest.tsx +++ b/tests/ui/MergeTransactionDetailsReviewTest.tsx @@ -173,7 +173,8 @@ describe('Merging distance expenses across workspaces', () => { , ); await waitForBatchedUpdatesWithAct(); - expect(screen.getByTestId('field-Distance • Original: 10.20 mi')).toHaveTextContent('9.20 mi'); + // Matched loosely because the unit label reads as either "mi" or "miles", depending on the field's short form flag + expect(screen.getByTestId('field-Distance • Original: 10.20 mi')).toHaveTextContent(/^9\.20 (mi|miles)$/); }); it('deducts nothing when the merged expense is put on the report of a workspace that excludes nothing', async () => { From d0c96ef78dffdaab0d5bc4941c6d44ba8d76c1e6 Mon Sep 17 00:00:00 2001 From: alberto Date: Thu, 13 Aug 2026 00:48:18 +0200 Subject: [PATCH 08/12] better identical merge handling --- src/libs/MergeTransactionUtils.ts | 30 +++++++++++--- tests/unit/MergeTransactionUtilsTest.ts | 53 +++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/src/libs/MergeTransactionUtils.ts b/src/libs/MergeTransactionUtils.ts index 0eba00eaecac..5be44e21fc1c 100644 --- a/src/libs/MergeTransactionUtils.ts +++ b/src/libs/MergeTransactionUtils.ts @@ -235,6 +235,8 @@ function getMergeableDataAndConflictFields( ) { const conflictFields: string[] = []; const mergeableData: Record = {}; + // The same object, typed for the field builders that read the values merged so far + const mergedSoFar = mergeableData as MergeTransaction; // Resolve the report-owner fallback the same way the display path (buildMergeFieldsData) does, so an expense // with no stored attendee is compared as [owner] instead of [] and doesn't produce a false attendee conflict @@ -287,7 +289,16 @@ function getMergeableDataAndConflictFields( // We allow user to select unreported report if (field === 'reportID') { if (targetValue === sourceValue) { - const updatedValues = getMergeFieldUpdatedValues({transaction: targetTransaction, field, fieldValue: SafeString(targetValue), getCurrencyDecimals, searchReports}); + const updatedValues = getMergeFieldUpdatedValues({ + transaction: targetTransaction, + field, + fieldValue: SafeString(targetValue), + getCurrencyDecimals, + mergeTransaction: mergedSoFar, + searchReports, + // Both expenses share the report, so the merged expense stays on that report's workspace + destinationPolicy: targetTransactionPolicy, + }); Object.assign(mergeableData, updatedValues); } else { conflictFields.push(field); @@ -323,9 +334,11 @@ function getMergeableDataAndConflictFields( field, fieldValue: selectedFieldValue as MergeTransaction[typeof field], getCurrencyDecimals, - mergeTransaction: mergeableData as MergeTransaction, + mergeTransaction: mergedSoFar, searchReports, policy: selectedPolicy, + // Nothing conflicts, so the merged expense stays on the report it is already on + destinationPolicy: selectedPolicy, }); Object.assign(mergeableData, updatedValues); } else { @@ -644,8 +657,11 @@ type GetMergeFieldUpdatedValuesParams = { searchReports?: Array>; policy?: OnyxEntry; - /** Workspace of the report the merged expense will live on, which is the one whose rules apply to it */ - destinationPolicy?: OnyxEntry; + /** + * Workspace of the report the merged expense will live on, which is the one whose rules apply to it. Required so + * that an expense with no workspace, which no rule applies to, has to be passed as undefined rather than omitted. + */ + destinationPolicy: OnyxEntry; }; /** @@ -745,7 +761,11 @@ function getMergeFieldUpdatedValues({ if (isDistanceRequest(transaction)) { const previousCustomUnit = mergeTransaction?.customUnit; const commuterExclusionUpdate = getCommuterExclusionCustomUnitUpdate(undefined, previousCustomUnit, destinationPolicy); - updatedValues.customUnit = commuterExclusionUpdate; + // An empty update means the expense has no exclusion to apply or clear, and writing it would wipe the + // custom unit the merchant selection stored + if (Object.keys(commuterExclusionUpdate).length > 0) { + updatedValues.customUnit = commuterExclusionUpdate; + } if (typeof mergeTransaction?.amount === 'number') { updatedValues.amount = getAmountForBilledDistance( mergeTransaction.amount, diff --git a/tests/unit/MergeTransactionUtilsTest.ts b/tests/unit/MergeTransactionUtilsTest.ts index 360d8c373265..1587f04d2063 100644 --- a/tests/unit/MergeTransactionUtilsTest.ts +++ b/tests/unit/MergeTransactionUtilsTest.ts @@ -17,10 +17,12 @@ import {isFromCreditCardImport} from '@libs/TransactionUtils'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; +import type {Policy} from '@src/types/onyx'; import Onyx from 'react-native-onyx'; import createRandomMergeTransaction from '../utils/collections/mergeTransaction'; +import createRandomPolicy from '../utils/collections/policies'; import {createRandomReport} from '../utils/collections/reports'; import createRandomTransaction, {createRandomDistanceRequestTransaction} from '../utils/collections/transaction'; import {convertToDisplayString, translateLocal} from '../utils/TestHelper'; @@ -358,6 +360,49 @@ describe('MergeTransactionUtils', () => { }); }); + it('should keep the commuter exclusion when two identical distance expenses are merged without conflicts', () => { + // Given two identical distance expenses on a report of a workspace that excludes 1 commuter mile, so that + // nothing conflicts and every field is merged automatically + const excludingWorkspace: Policy = { + ...createRandomPolicy(0, CONST.POLICY.TYPE.TEAM), + commuterExclusions: {method: CONST.POLICY.COMMUTER_EXCLUSION_METHOD.FIXED_DISTANCE, fixedDistance: 1, fixedDistanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES}, + }; + const distanceExpense = { + ...createRandomDistanceRequestTransaction(0), + reportID: '9999', + merchant: '10.20 mi @ $1.00 / mi', + modifiedMerchant: '10.20 mi @ $1.00 / mi', + amount: -920, + comment: { + customUnit: { + name: CONST.CUSTOM_UNITS.NAME_DISTANCE, + quantity: 10.2, + distanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, + commuterExclusion: 1, + reimbursableDistance: 9.2, + }, + }, + }; + + // When they are merged + const result = getMergeableDataAndConflictFields( + distanceExpense, + {...distanceExpense, transactionID: 'secondTransaction'}, + mockLocaleCompare, + mockGetCurrencyDecimals, + [], + excludingWorkspace, + excludingWorkspace, + ); + + // Then the workspace's exclusion survives, and the amount still pays for the reimbursable distance only + expect(result.conflictFields).not.toContain('merchant'); + expect(result.mergeableData).toMatchObject({ + amount: 920, + customUnit: expect.objectContaining({commuterExclusion: 1, reimbursableDistance: 9.2}) as unknown, + }); + }); + it('should merge amount field correctly when they are same', () => { const targetTransaction = { ...createRandomTransaction(1), @@ -1022,7 +1067,7 @@ describe('MergeTransactionUtils', () => { const fieldValue = 'New Merchant Name'; // When we get updated values for merchant field - const result = getMergeFieldUpdatedValues({transaction, field: 'merchant', fieldValue, getCurrencyDecimals: mockGetCurrencyDecimals}); + const result = getMergeFieldUpdatedValues({transaction, field: 'merchant', fieldValue, getCurrencyDecimals: mockGetCurrencyDecimals, destinationPolicy: undefined}); // Then it should return an object with the field value expect(result).toEqual({ @@ -1039,7 +1084,7 @@ describe('MergeTransactionUtils', () => { const fieldValue = 2500; // When we get updated values for amount field - const result = getMergeFieldUpdatedValues({transaction, field: 'amount', fieldValue, getCurrencyDecimals: mockGetCurrencyDecimals}); + const result = getMergeFieldUpdatedValues({transaction, field: 'amount', fieldValue, getCurrencyDecimals: mockGetCurrencyDecimals, destinationPolicy: undefined}); // Then it should include both amount and currency expect(result).toEqual({ @@ -1058,7 +1103,7 @@ describe('MergeTransactionUtils', () => { const fieldValue = '456'; // When we get updated values for reportID field - const result = getMergeFieldUpdatedValues({transaction, field: 'reportID', fieldValue, getCurrencyDecimals: mockGetCurrencyDecimals}); + const result = getMergeFieldUpdatedValues({transaction, field: 'reportID', fieldValue, getCurrencyDecimals: mockGetCurrencyDecimals, destinationPolicy: undefined}); // Then it should include both reportID and reportName expect(result).toEqual({ @@ -1093,7 +1138,7 @@ describe('MergeTransactionUtils', () => { const fieldValue = 'New Distance Merchant'; // When we get updated values for merchant field - const result = getMergeFieldUpdatedValues({transaction, field: 'merchant', fieldValue, getCurrencyDecimals: mockGetCurrencyDecimals}); + const result = getMergeFieldUpdatedValues({transaction, field: 'merchant', fieldValue, getCurrencyDecimals: mockGetCurrencyDecimals, destinationPolicy: undefined}); // Then it should include merchant plus all distance-specific fields expect(result).toEqual({ From d03e4ac2d730fab94ac5b7686d6c5006e660d63f Mon Sep 17 00:00:00 2001 From: alberto Date: Thu, 13 Aug 2026 03:16:20 +0200 Subject: [PATCH 09/12] comment --- tests/unit/MergeTransactionUtilsTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/MergeTransactionUtilsTest.ts b/tests/unit/MergeTransactionUtilsTest.ts index 1587f04d2063..a318ff226722 100644 --- a/tests/unit/MergeTransactionUtilsTest.ts +++ b/tests/unit/MergeTransactionUtilsTest.ts @@ -399,7 +399,7 @@ describe('MergeTransactionUtils', () => { expect(result.conflictFields).not.toContain('merchant'); expect(result.mergeableData).toMatchObject({ amount: 920, - customUnit: expect.objectContaining({commuterExclusion: 1, reimbursableDistance: 9.2}) as unknown, + customUnit: expect.objectContaining({commuterExclusion: 1, reimbursableDistance: 9.2}), }); }); From 0a80a262d37fb1de24d0c4015b92e526774f191f Mon Sep 17 00:00:00 2001 From: alberto Date: Tue, 25 Aug 2026 22:53:30 +0200 Subject: [PATCH 10/12] fix equal merge --- src/libs/MergeTransactionUtils.ts | 4 +--- tests/unit/MergeTransactionUtilsTest.ts | 17 +++++++++++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/libs/MergeTransactionUtils.ts b/src/libs/MergeTransactionUtils.ts index c6c43305b6a3..1fcd3c6fed00 100644 --- a/src/libs/MergeTransactionUtils.ts +++ b/src/libs/MergeTransactionUtils.ts @@ -806,10 +806,8 @@ function getMergeFieldUpdatedValues({ if (isDistanceRequest(transaction)) { const previousCustomUnit = mergeTransaction?.customUnit; const commuterExclusionUpdate = getCommuterExclusionCustomUnitUpdate(undefined, previousCustomUnit, destinationPolicy); - // An empty update means the expense has no exclusion to apply or clear, and writing it would wipe the - // custom unit the merchant selection stored if (Object.keys(commuterExclusionUpdate).length > 0) { - updatedValues.customUnit = commuterExclusionUpdate; + updatedValues.customUnit = {...previousCustomUnit, ...commuterExclusionUpdate}; } if (typeof mergeTransaction?.amount === 'number') { updatedValues.amount = getAmountForBilledDistance( diff --git a/tests/unit/MergeTransactionUtilsTest.ts b/tests/unit/MergeTransactionUtilsTest.ts index c43c1b9709d1..ff2401bcc348 100644 --- a/tests/unit/MergeTransactionUtilsTest.ts +++ b/tests/unit/MergeTransactionUtilsTest.ts @@ -364,7 +364,7 @@ describe('MergeTransactionUtils', () => { }); }); - it('should keep the commuter exclusion when two identical distance expenses are merged without conflicts', () => { + it('should keep the distance, rate and commuter exclusion when two identical distance expenses are merged without conflicts', () => { // Given two identical distance expenses on a report of a workspace that excludes 1 commuter mile, so that // nothing conflicts and every field is merged automatically const excludingWorkspace: Policy = { @@ -380,6 +380,7 @@ describe('MergeTransactionUtils', () => { comment: { customUnit: { name: CONST.CUSTOM_UNITS.NAME_DISTANCE, + customUnitRateID: 'rate123', quantity: 10.2, distanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, commuterExclusion: 1, @@ -399,11 +400,19 @@ describe('MergeTransactionUtils', () => { excludingWorkspace, ); - // Then the workspace's exclusion survives, and the amount still pays for the reimbursable distance only - expect(result.conflictFields).not.toContain('merchant'); + // Then the distance and rate survive alongside the workspace's exclusion, and the amount still pays for + // the reimbursable distance only + expect(result.conflictFields).toHaveLength(0); expect(result.mergeableData).toMatchObject({ amount: 920, - customUnit: expect.objectContaining({commuterExclusion: 1, reimbursableDistance: 9.2}), + customUnit: { + name: CONST.CUSTOM_UNITS.NAME_DISTANCE, + customUnitRateID: 'rate123', + quantity: 10.2, + distanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, + commuterExclusion: 1, + reimbursableDistance: 9.2, + }, }); }); From bb17afec8c8d05b528e68d0ce83286ad058358c9 Mon Sep 17 00:00:00 2001 From: alberto Date: Tue, 25 Aug 2026 23:06:48 +0200 Subject: [PATCH 11/12] handle manual merges properly --- src/libs/DistanceRequestUtils.ts | 14 +++- src/libs/MergeTransactionUtils.ts | 21 ++++- tests/unit/MergeTransactionUtilsTest.ts | 105 ++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 4 deletions(-) diff --git a/src/libs/DistanceRequestUtils.ts b/src/libs/DistanceRequestUtils.ts index d495e3e088a2..b7b06304a6e4 100644 --- a/src/libs/DistanceRequestUtils.ts +++ b/src/libs/DistanceRequestUtils.ts @@ -2,6 +2,7 @@ import type {CurrencyListActionsContextType} from '@components/CurrencyListConte import type {LocaleContextProps} from '@components/LocaleContextProvider'; import CONST from '@src/CONST'; +import type {IOURequestType} from '@src/CONST'; import type {LastSelectedDistanceRates, OnyxInputOrEntry, Transaction} from '@src/types/onyx'; import type DefaultP2PMileageRate from '@src/types/onyx/DefaultP2PMileageRate'; import type {Unit} from '@src/types/onyx/Policy'; @@ -336,6 +337,16 @@ function getCommuterExclusionDisplayData(customUnit: TransactionCustomUnit | und }; } +/** + * Whether a workspace's commuter exclusion applies to a distance expense of this request type. + * + * Only a distance the app itself measured describes a route the workspace can recognise a commute in, so a manually + * entered or odometer distance is reimbursed in full. + */ +function isCommuterExclusionApplicableToRequestType(iouRequestType: IOURequestType | undefined): boolean { + return iouRequestType !== CONST.IOU.REQUEST_TYPE.DISTANCE_MANUAL && iouRequestType !== CONST.IOU.REQUEST_TYPE.DISTANCE_ODOMETER; +} + /** * Returns the distance a workspace excludes from a distance of `distance` units, expressed in that same unit. * @@ -374,7 +385,7 @@ function getTransactionCommuterExclusionData({ getCurrencySymbol?: CurrencyListActionsContextType['getCurrencySymbol']; personalPolicyOutputCurrency?: string; }): (Pick & {modifiedAmount: number; customUnit: TransactionCustomUnit}) | undefined { - if (transaction?.iouRequestType === CONST.IOU.REQUEST_TYPE.DISTANCE_MANUAL || transaction?.iouRequestType === CONST.IOU.REQUEST_TYPE.DISTANCE_ODOMETER) { + if (!isCommuterExclusionApplicableToRequestType(transaction?.iouRequestType)) { return; } @@ -874,6 +885,7 @@ export default { getDistanceRequestAmount, getCommuterExclusionDisplayData, getPolicyCommuterExclusionForDistance, + isCommuterExclusionApplicableToRequestType, getTransactionCommuterExclusionData, getDistanceDisplayDetailsWithCommuter, getFormattedRateValue, diff --git a/src/libs/MergeTransactionUtils.ts b/src/libs/MergeTransactionUtils.ts index 1fcd3c6fed00..fbad088f740a 100644 --- a/src/libs/MergeTransactionUtils.ts +++ b/src/libs/MergeTransactionUtils.ts @@ -2,6 +2,7 @@ import type {CurrencyListActionsContextType} from '@components/CurrencyListConte import type {LocaleContextProps} from '@components/LocaleContextProvider'; import CONST from '@src/CONST'; +import type {IOURequestType} from '@src/CONST'; import type {TranslationPaths} from '@src/languages/types'; import ONYXKEYS from '@src/ONYXKEYS'; import type {MergeTransaction, Policy, Report, ReportAction, SearchResults, Transaction} from '@src/types/onyx'; @@ -716,17 +717,24 @@ type GetMergeFieldUpdatedValuesParams = { * that is selected. Selections are stored with Onyx.merge, which deep merges the custom unit, so both the exclusion of * a workspace that is no longer the destination and the reimbursable distance of a previously selected merchant would * otherwise survive. The distance field then shows the reimbursable distance in place of the full distance. + * + * `iouRequestType` is the type the merged expense ends up with, so that merging reaches the same exclusion that + * creating the expense on the destination workspace would. */ function getCommuterExclusionCustomUnitUpdate( selectedCustomUnit: TransactionCustomUnit | undefined, previousCustomUnit: TransactionCustomUnit | undefined, destinationPolicy: OnyxEntry, + iouRequestType: IOURequestType | undefined, ): CommuterExclusionCustomUnitUpdate { const quantity = selectedCustomUnit?.quantity ?? previousCustomUnit?.quantity; const distanceUnit = selectedCustomUnit?.distanceUnit ?? previousCustomUnit?.distanceUnit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES; // The destination workspace's setting decides the exclusion, so it applies to an expense that arrives from a // workspace excluding nothing, and stops applying to one that leaves a workspace that excludes a distance - const commuterExclusion = typeof quantity === 'number' ? DistanceRequestUtils.getPolicyCommuterExclusionForDistance(destinationPolicy, quantity, distanceUnit) : 0; + const commuterExclusion = + typeof quantity === 'number' && DistanceRequestUtils.isCommuterExclusionApplicableToRequestType(iouRequestType) + ? DistanceRequestUtils.getPolicyCommuterExclusionForDistance(destinationPolicy, quantity, distanceUnit) + : 0; if (commuterExclusion > 0 && typeof quantity === 'number') { return { @@ -805,7 +813,14 @@ function getMergeFieldUpdatedValues({ // and the amount has to follow the distance that is left to reimburse if (isDistanceRequest(transaction)) { const previousCustomUnit = mergeTransaction?.customUnit; - const commuterExclusionUpdate = getCommuterExclusionCustomUnitUpdate(undefined, previousCustomUnit, destinationPolicy); + // The merchant selection is what sets the merged expense's type, so it is preferred over the type of the + // expense whose report was selected here + const commuterExclusionUpdate = getCommuterExclusionCustomUnitUpdate( + undefined, + previousCustomUnit, + destinationPolicy, + mergeTransaction?.iouRequestType ?? transaction?.iouRequestType, + ); if (Object.keys(commuterExclusionUpdate).length > 0) { updatedValues.customUnit = {...previousCustomUnit, ...commuterExclusionUpdate}; } @@ -823,7 +838,7 @@ function getMergeFieldUpdatedValues({ const transactionDetails = getTransactionDetails(transaction); updatedValues.currency = getCurrency(transaction); const selectedCustomUnit = transaction?.comment?.customUnit; - const commuterExclusionUpdate = getCommuterExclusionCustomUnitUpdate(selectedCustomUnit, mergeTransaction?.customUnit, destinationPolicy); + const commuterExclusionUpdate = getCommuterExclusionCustomUnitUpdate(selectedCustomUnit, mergeTransaction?.customUnit, destinationPolicy, transaction?.iouRequestType); updatedValues.customUnit = { ...selectedCustomUnit, ...commuterExclusionUpdate, diff --git a/tests/unit/MergeTransactionUtilsTest.ts b/tests/unit/MergeTransactionUtilsTest.ts index ff2401bcc348..d16e1dfdc493 100644 --- a/tests/unit/MergeTransactionUtilsTest.ts +++ b/tests/unit/MergeTransactionUtilsTest.ts @@ -416,6 +416,111 @@ describe('MergeTransactionUtils', () => { }); }); + it.each([CONST.IOU.REQUEST_TYPE.DISTANCE_MANUAL, CONST.IOU.REQUEST_TYPE.DISTANCE_ODOMETER])( + 'should not apply the commuter exclusion when merging a %s expense onto a workspace that excludes commuter distance', + (iouRequestType) => { + // Given two identical distance expenses whose distance the app did not measure, on a report of a + // workspace that excludes 1 commuter mile + const excludingWorkspace: Policy = { + ...createRandomPolicy(0, CONST.POLICY.TYPE.TEAM), + commuterExclusions: {method: CONST.POLICY.COMMUTER_EXCLUSION_METHOD.FIXED_DISTANCE, fixedDistance: 1, fixedDistanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES}, + }; + const distanceExpense = { + ...createRandomDistanceRequestTransaction(0), + iouRequestType, + reportID: '9999', + merchant: '10.20 mi @ $1.00 / mi', + modifiedMerchant: '10.20 mi @ $1.00 / mi', + amount: -1020, + comment: { + customUnit: { + name: CONST.CUSTOM_UNITS.NAME_DISTANCE, + customUnitRateID: 'rate123', + quantity: 10.2, + distanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, + }, + }, + }; + + // When they are merged + const result = getMergeableDataAndConflictFields( + distanceExpense, + {...distanceExpense, transactionID: 'secondTransaction'}, + mockLocaleCompare, + mockGetCurrencyDecimals, + [], + excludingWorkspace, + excludingWorkspace, + ); + + // Then the whole distance is reimbursed, the same as creating the expense on that workspace would do + expect(result.conflictFields).toHaveLength(0); + expect(result.mergeableData).toMatchObject({ + amount: 1020, + customUnit: { + name: CONST.CUSTOM_UNITS.NAME_DISTANCE, + customUnitRateID: 'rate123', + quantity: 10.2, + distanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, + }, + }); + expect(result.mergeableData.customUnit).not.toHaveProperty('commuterExclusion'); + expect(result.mergeableData.customUnit).not.toHaveProperty('reimbursableDistance'); + }, + ); + + it('should clear an exclusion a manual distance expense still carries when it is merged', () => { + // Given two identical manual distance expenses that carry an exclusion from an earlier workspace, so that + // their amount pays for 9.2 of the 10.2 miles + const excludingWorkspace: Policy = { + ...createRandomPolicy(0, CONST.POLICY.TYPE.TEAM), + commuterExclusions: {method: CONST.POLICY.COMMUTER_EXCLUSION_METHOD.FIXED_DISTANCE, fixedDistance: 1, fixedDistanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES}, + }; + const distanceExpense = { + ...createRandomDistanceRequestTransaction(0), + iouRequestType: CONST.IOU.REQUEST_TYPE.DISTANCE_MANUAL, + reportID: '9999', + merchant: '10.20 mi @ $1.00 / mi', + modifiedMerchant: '10.20 mi @ $1.00 / mi', + amount: -920, + comment: { + customUnit: { + name: CONST.CUSTOM_UNITS.NAME_DISTANCE, + customUnitRateID: 'rate123', + quantity: 10.2, + distanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, + commuterExclusion: 1, + reimbursableDistance: 9.2, + }, + }, + }; + + // When they are merged + const result = getMergeableDataAndConflictFields( + distanceExpense, + {...distanceExpense, transactionID: 'secondTransaction'}, + mockLocaleCompare, + mockGetCurrencyDecimals, + [], + excludingWorkspace, + excludingWorkspace, + ); + + // Then the exclusion keys are nulled so Onyx removes them, and the amount pays for the whole distance again + expect(result.conflictFields).toHaveLength(0); + expect(result.mergeableData).toMatchObject({ + amount: 1020, + customUnit: { + name: CONST.CUSTOM_UNITS.NAME_DISTANCE, + customUnitRateID: 'rate123', + quantity: 10.2, + distanceUnit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, + commuterExclusion: null, + reimbursableDistance: null, + }, + }); + }); + it('should merge amount field correctly when they are same', () => { const targetTransaction = { ...createRandomTransaction(1), From 3ab299ee2fa6e29e5037baa4ecf07160f6313e91 Mon Sep 17 00:00:00 2001 From: alberto Date: Tue, 25 Aug 2026 23:16:59 +0200 Subject: [PATCH 12/12] better tests --- src/libs/DistanceRequestUtils.ts | 2 +- .../ui/MergeTransactionDetailsReviewTest.tsx | 113 ++++++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/src/libs/DistanceRequestUtils.ts b/src/libs/DistanceRequestUtils.ts index b7b06304a6e4..182382b77192 100644 --- a/src/libs/DistanceRequestUtils.ts +++ b/src/libs/DistanceRequestUtils.ts @@ -340,7 +340,7 @@ function getCommuterExclusionDisplayData(customUnit: TransactionCustomUnit | und /** * Whether a workspace's commuter exclusion applies to a distance expense of this request type. * - * Only a distance the app itself measured describes a route the workspace can recognise a commute in, so a manually + * Only a distance the app itself measured describes a route the workspace can recognize a commute in, so a manually * entered or odometer distance is reimbursed in full. */ function isCommuterExclusionApplicableToRequestType(iouRequestType: IOURequestType | undefined): boolean { diff --git a/tests/ui/MergeTransactionDetailsReviewTest.tsx b/tests/ui/MergeTransactionDetailsReviewTest.tsx index 7d0c6499dda3..6cb6ffdf72f2 100644 --- a/tests/ui/MergeTransactionDetailsReviewTest.tsx +++ b/tests/ui/MergeTransactionDetailsReviewTest.tsx @@ -4,6 +4,7 @@ import ComposeProviders from '@components/ComposeProviders'; import {LocaleContextProvider} from '@components/LocaleContextProvider'; import OnyxListItemProvider from '@components/OnyxListItemProvider'; +import {setupMergeTransactionDataAndNavigate} from '@libs/actions/MergeTransaction'; import navigationRef from '@libs/Navigation/navigationRef'; import createPlatformStackNavigator from '@libs/Navigation/PlatformStackNavigation/createPlatformStackNavigator'; @@ -11,6 +12,7 @@ import DynamicConfirmationPage from '@pages/TransactionMerge/DynamicConfirmation import DynamicDetailsReviewPage from '@pages/TransactionMerge/DynamicDetailsReviewPage'; import CONST from '@src/CONST'; +import type {IOURequestType} from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import SCREENS from '@src/SCREENS'; import type {Policy, Report, Transaction} from '@src/types/onyx'; @@ -28,6 +30,15 @@ import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct' jest.mock('@hooks/useDynamicBackPath', () => jest.fn(() => '')); +// The auto-merge entry point navigates itself, which the test drives by rendering the destination page directly +jest.mock('@libs/Navigation/Navigation', () => { + const actualNavigation = jest.requireActual<{default: Record}>('@libs/Navigation/Navigation'); + return { + __esModule: true, + default: {...actualNavigation.default, navigate: jest.fn()}, + }; +}); + const Stack = createPlatformStackNavigator>(); // Expose each field row's description and title so the rendered distance can be read back @@ -43,6 +54,7 @@ jest.mock('@components/MenuItemWithTopDescription', () => { TestHelper.setupGlobalFetchMock(); const MERGE_TRANSACTION_ID = 'mergeDistanceTransaction'; +const DISTANCE_RATE_ID = 'distanceRateOfTheExcludingWorkspace'; const EXCLUDING_POLICY_ID = 'policyThatExcludesCommuterDistance'; const PLAIN_POLICY_ID = 'policyThatExcludesNothing'; const EXCLUDING_REPORT_ID = '4444'; @@ -192,3 +204,104 @@ describe('Merging distance expenses across workspaces', () => { expect(mergeTransaction?.amount).toBe(449); }); }); + +describe('Merging identical distance expenses without conflicts', () => { + // Identical expenses on the same report leave nothing to resolve, so the merge skips the details review page and + // builds the whole merge transaction in one pass + const buildIdenticalExpense = (iouRequestType: IOURequestType): Transaction => { + const expense = buildDistanceExpense('firstTransaction', EXCLUDING_REPORT_ID, '10.20 mi @ $1.00 / mi', 10.2); + return { + ...expense, + iouRequestType, + comment: {...expense.comment, customUnit: {...expense.comment?.customUnit, customUnitRateID: DISTANCE_RATE_ID}}, + }; + }; + + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + }); + + const setUpAndMerge = async (iouRequestType: IOURequestType) => { + // The second expense is a copy so that every field matches and the merge has nothing to resolve + const firstExpense = buildIdenticalExpense(iouRequestType); + const secondExpense = {...firstExpense, transactionID: 'secondTransaction'}; + const report = buildReport(EXCLUDING_REPORT_ID, EXCLUDING_POLICY_ID, 'Report that deducts'); + + await act(async () => { + await Onyx.clear(); + await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${EXCLUDING_POLICY_ID}`, excludingPolicy); + await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${EXCLUDING_REPORT_ID}`, report); + await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${firstExpense.transactionID}`, firstExpense); + await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${secondExpense.transactionID}`, secondExpense); + }); + await waitForBatchedUpdatesWithAct(); + + await act(async () => { + setupMergeTransactionDataAndNavigate( + MERGE_TRANSACTION_ID, + [firstExpense, secondExpense], + (a: string, b: string) => a.localeCompare(b), + () => 2, + [report], + false, + false, + [excludingPolicy, excludingPolicy], + ); + }); + await waitForBatchedUpdatesWithAct(); + + return getOnyxValue(`${ONYXKEYS.COLLECTION.MERGE_TRANSACTION}${MERGE_TRANSACTION_ID}`); + }; + + const renderConfirmationPage = async () => { + render( + + + + + + + , + ); + await waitForBatchedUpdatesWithAct(); + }; + + it('carries the distance and rate through to the confirmation page alongside the exclusion', async () => { + // Given two identical map distance expenses on a report of a workspace that excludes 1 commuter mile + // When they are merged with nothing to resolve + const mergeTransaction = await setUpAndMerge(CONST.IOU.REQUEST_TYPE.DISTANCE_MAP); + + // Then the distance and rate reach the merge transaction rather than being replaced by the exclusion alone + expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); + expect(mergeTransaction?.customUnit?.customUnitRateID).toBe(DISTANCE_RATE_ID); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBe(1); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBe(9.2); + expect(mergeTransaction?.amount).toBe(920); + + // And the confirmation page renders the reimbursable distance against the distance it was deducted from + await renderConfirmationPage(); + // Matched loosely because the unit label reads as either "mi" or "miles", depending on the field's short form flag + expect(screen.getByTestId('field-Distance • Original: 10.20 mi')).toHaveTextContent(/^9\.20 (mi|miles)$/); + }); + + it('deducts nothing from a manually entered distance, which the workspace cannot recognize a commute in', async () => { + // Given two identical manual distance expenses on a report of a workspace that excludes 1 commuter mile + // When they are merged with nothing to resolve + const mergeTransaction = await setUpAndMerge(CONST.IOU.REQUEST_TYPE.DISTANCE_MANUAL); + + // Then the whole distance is reimbursed, the same as creating the expense on that workspace would do + expect(mergeTransaction?.customUnit?.quantity).toBe(10.2); + expect(mergeTransaction?.customUnit?.customUnitRateID).toBe(DISTANCE_RATE_ID); + expect(mergeTransaction?.customUnit?.commuterExclusion).toBeUndefined(); + expect(mergeTransaction?.customUnit?.reimbursableDistance).toBeUndefined(); + expect(mergeTransaction?.amount).toBe(1020); + + // And the confirmation page renders the whole distance, with no distance deducted from it + await renderConfirmationPage(); + expect(screen.getByTestId('field-Distance')).toHaveTextContent(/^10\.20 (mi|miles)$/); + }); +});