Skip to content

Commit 0ef827f

Browse files
committed
Address PR review: gate distance cleanup, dedupe default selection
Two pieces of bot feedback on PR #89867: 1. Gate the successData distance-rate cleanup on parts?.distance. When a user duplicates a workspace WITHOUT selecting distance rates, buildOptimisticDuplicatePolicy creates no distance custom unit for the target policy — but the previous successData unconditionally wrote customUnits[distanceCustomUnitID].rates if the source had distance rates, leaving a partial/orphan distance unit of nulled rate IDs in the duplicate. Compute sourceDistanceUnit only when parts.distance is true so cleanup runs only when the optimistic distance unit was actually inserted. 2. Extract a shared getDefaultDistanceRate(rates) helper. The default-rate selection (filter enabled, sort by index with CONST.DEFAULT_NUMBER_ID fallback, take first) was duplicated in cloneCustomUnitWithNewIDs and in buildDuplicatePolicyData. Pull it into a single helper and call it from both. (Note: getDefaultMileageRate in DistanceRequestUtils.ts uses the same algorithm but on a pre-filtered MileageRates structure with extra MileageRate construction; left untouched to avoid scope creep.) Adds two PolicyTest.ts integration tests covering the new code paths: the optimistic clone shape with distance selected (using mockFetch.pause to inspect the offline state, then resume to verify successData clears source IDs), and the no-orphan-distance-unit guarantee with distance deselected.
1 parent b90bf90 commit 0ef827f

3 files changed

Lines changed: 229 additions & 23 deletions

File tree

src/libs/PolicyUtils.ts

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -298,21 +298,31 @@ function hasEligibleActiveAdminFromWorkspaces(policies: OnyxCollection<Policy> |
298298
return false;
299299
}
300300

301+
/**
302+
* Pick the rate that the expense flow will treat as default for a distance custom unit's rates
303+
* dictionary: filter enabled rates, sort by `index` (treating missing index as CONST.DEFAULT_NUMBER_ID),
304+
* and take the first. Mirrors `getDefaultMileageRate`'s selection so optimistic state stays aligned
305+
* with the rate the expense flow later picks.
306+
*/
307+
function getDefaultDistanceRate(rates: Record<string, Rate> | undefined): Rate | undefined {
308+
if (!rates) {
309+
return undefined;
310+
}
311+
return Object.values(rates)
312+
.filter((rate) => rate.enabled !== false)
313+
.sort((a, b) => (a.index ?? CONST.DEFAULT_NUMBER_ID) - (b.index ?? CONST.DEFAULT_NUMBER_ID))
314+
.at(0);
315+
}
316+
301317
function cloneCustomUnitWithNewIDs(unit: CustomUnit, newCustomUnitID: string, newDefaultRateID?: string): CustomUnit {
302318
if (newDefaultRateID) {
303319
// The server-side DUPLICATE_POLICY assigns newDefaultRateID to the source's default rate.
304-
// Mirror getDefaultMileageRate's selection (enabled rates, sorted by index with
305-
// CONST.DEFAULT_NUMBER_ID for missing indexes) so the optimistic clone aligns with the
306-
// rate the expense flow will later treat as default. Non-default rates keep their source
307-
// IDs so they remain visible offline; successData clears them after the API succeeds so
308-
// the server response can repopulate with fresh server IDs without leaving duplicates.
309-
// Iteration order is preserved from the source (default's key swapped in place) so
310-
// getDefaultMileageRate's stable sort still picks the original default when other rates
311-
// have a missing index (which would otherwise tie at CONST.DEFAULT_NUMBER_ID).
312-
const defaultRate = Object.values(unit.rates)
313-
.filter((rate) => rate.enabled !== false)
314-
.sort((a, b) => (a.index ?? CONST.DEFAULT_NUMBER_ID) - (b.index ?? CONST.DEFAULT_NUMBER_ID))
315-
.at(0);
320+
// Non-default rates keep their source IDs so they remain visible offline; successData clears
321+
// them after the API succeeds so the server response can repopulate with fresh server IDs
322+
// without leaving duplicates. Iteration order is preserved from the source (default's key
323+
// swapped in place) so getDefaultMileageRate's stable sort still picks the original default
324+
// when other rates have a missing index (which would otherwise tie at CONST.DEFAULT_NUMBER_ID).
325+
const defaultRate = getDefaultDistanceRate(unit.rates);
316326
const rates: Record<string, Rate> = {};
317327
for (const rate of Object.values(unit.rates)) {
318328
if (rate.customUnitRateID === defaultRate?.customUnitRateID) {
@@ -2280,6 +2290,7 @@ export {
22802290
getManagerAccountID,
22812291
isPreferredExporter,
22822292
getCustomUnitsForDuplication,
2293+
getDefaultDistanceRate,
22832294
getCountOfRequiredTagLists,
22842295
getActiveEmployeeWorkspaces,
22852296
getPolicyRole,

src/libs/actions/Policy/Policy.ts

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,14 @@ import Permissions from '@libs/Permissions';
9090
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
9191
import * as PhoneNumber from '@libs/PhoneNumber';
9292
import * as PolicyUtils from '@libs/PolicyUtils';
93-
import {getCustomUnitsForDuplication, getMemberAccountIDsForWorkspace, goBackWhenEnableFeature, isControlPolicy, navigateToExpensifyCardPage} from '@libs/PolicyUtils';
93+
import {
94+
getCustomUnitsForDuplication,
95+
getDefaultDistanceRate,
96+
getMemberAccountIDsForWorkspace,
97+
goBackWhenEnableFeature,
98+
isControlPolicy,
99+
navigateToExpensifyCardPage,
100+
} from '@libs/PolicyUtils';
94101
import * as ReportUtils from '@libs/ReportUtils';
95102
import {hasValidModifiedAmount} from '@libs/TransactionUtils';
96103
import type {Feature} from '@pages/OnboardingInterestedFeatures/types';
@@ -3236,16 +3243,13 @@ function buildDuplicatePolicyData(policy: Policy, options: DuplicatePolicyDataOp
32363243
const {customUnitID: distanceCustomUnitID, customUnitRateID} = buildOptimisticDistanceRateCustomUnits(outputCurrency);
32373244
const perDiemCustomUnitID = generateCustomUnitID();
32383245

3239-
// Source non-default distance rate IDs — kept in optimistic data so they're visible offline,
3240-
// then nulled in successData so the server's Pusher response can repopulate with fresh server
3241-
// IDs without leaving optimistic-vs-server duplicates.
3242-
const sourceDistanceUnit = Object.values(policy?.customUnits ?? {}).find((unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE);
3243-
const sourceDistanceDefaultRate = sourceDistanceUnit
3244-
? Object.values(sourceDistanceUnit.rates)
3245-
.filter((rate) => rate.enabled !== false)
3246-
.sort((a, b) => (a.index ?? CONST.DEFAULT_NUMBER_ID) - (b.index ?? CONST.DEFAULT_NUMBER_ID))
3247-
.at(0)
3248-
: undefined;
3246+
// Source non-default distance rate IDs — kept in optimistic data (by cloneCustomUnitWithNewIDs)
3247+
// so they're visible offline, then nulled in successData so the server's Pusher response can
3248+
// repopulate with fresh server IDs without leaving optimistic-vs-server duplicates. Only
3249+
// computed when the user actually selected distance rates for duplication; otherwise no
3250+
// optimistic distance unit was created and there's nothing to clean up.
3251+
const sourceDistanceUnit = parts?.distance ? Object.values(policy?.customUnits ?? {}).find((unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE) : undefined;
3252+
const sourceDistanceDefaultRate = getDefaultDistanceRate(sourceDistanceUnit?.rates);
32493253
const sourceNonDefaultDistanceRateIDs = sourceDistanceUnit
32503254
? Object.values(sourceDistanceUnit.rates)
32513255
.filter((rate) => rate.customUnitRateID !== sourceDistanceDefaultRate?.customUnitRateID)

tests/actions/PolicyTest.ts

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -773,6 +773,197 @@ describe('actions/Policy', () => {
773773
expect(messages?.at(0)?.text).toBe(callerEmail);
774774
});
775775

776+
it('duplicate workspace with distance rates clones all source rates in optimistic data with the default rebound to the API-known rate ID', async () => {
777+
await Onyx.set(ONYXKEYS.SESSION, {email: ESH_EMAIL, accountID: ESH_ACCOUNT_ID});
778+
779+
const sourceDistanceUnitID = 'srcDistUnit';
780+
const sourceDefaultRateID = 'srcDefaultRate';
781+
const sourceExtraRateID = 'srcExtraRate';
782+
const fakePolicy: PolicyType = {
783+
...createRandomPolicy(19, CONST.POLICY.TYPE.TEAM),
784+
customUnits: {
785+
[sourceDistanceUnitID]: {
786+
customUnitID: sourceDistanceUnitID,
787+
name: CONST.CUSTOM_UNITS.NAME_DISTANCE,
788+
enabled: true,
789+
attributes: {unit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES},
790+
rates: {
791+
[sourceDefaultRateID]: {customUnitRateID: sourceDefaultRateID, name: 'Default Rate', rate: 72.5, currency: 'USD', enabled: true, index: 0},
792+
[sourceExtraRateID]: {customUnitRateID: sourceExtraRateID, name: 'Extra Rate', rate: 100, currency: 'USD', enabled: true, index: 1},
793+
},
794+
},
795+
},
796+
};
797+
await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${fakePolicy.id}`, fakePolicy);
798+
await waitForBatchedUpdates();
799+
800+
const policyID = Policy.generatePolicyID();
801+
const options = {
802+
currentUserAccountID: ESH_ACCOUNT_ID,
803+
currentUserEmail: ESH_EMAIL,
804+
policyName: 'Distance Rates Duplicate',
805+
policyID: fakePolicy.id,
806+
targetPolicyID: policyID,
807+
welcomeNote: 'Join my policy',
808+
parts: {
809+
people: false,
810+
reports: false,
811+
connections: false,
812+
categories: false,
813+
tags: false,
814+
taxes: false,
815+
perDiem: false,
816+
reimbursements: false,
817+
expenses: false,
818+
distance: true,
819+
invoices: false,
820+
exportLayouts: false,
821+
},
822+
localCurrency: 'USD',
823+
};
824+
825+
// Pause the API call so successData (which clears the optimistic source rate IDs) does not
826+
// run yet — this lets us inspect the optimistic state the user sees offline.
827+
mockFetch.pause();
828+
Policy.duplicateWorkspace(fakePolicy, options);
829+
await waitForBatchedUpdates();
830+
831+
const duplicatePolicy: OnyxEntry<PolicyType> = await new Promise((resolve) => {
832+
const connection = Onyx.connect({
833+
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
834+
callback: (workspace) => {
835+
Onyx.disconnect(connection);
836+
resolve(workspace);
837+
},
838+
});
839+
});
840+
841+
expect(duplicatePolicy?.areDistanceRatesEnabled).toBe(true);
842+
const distanceUnit = Object.values(duplicatePolicy?.customUnits ?? {}).find((unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE);
843+
if (!distanceUnit) {
844+
throw new Error('Expected duplicated distance unit');
845+
}
846+
// The duplicated unit's customUnitID should differ from the source's (fresh ID).
847+
expect(distanceUnit.customUnitID).not.toBe(sourceDistanceUnitID);
848+
// Both rates should be present in optimistic data: the extra rate keeps its source ID,
849+
// and the default is rebound to a fresh API-known customUnitRateID.
850+
const rateKeys = Object.keys(distanceUnit.rates);
851+
expect(rateKeys).toContain(sourceExtraRateID);
852+
expect(rateKeys).not.toContain(sourceDefaultRateID);
853+
expect(rateKeys).toHaveLength(2);
854+
// The Default Rate should be the one picked by getDefaultDistanceRate (lowest enabled index).
855+
const defaultRate = Object.values(distanceUnit.rates)
856+
.filter((rate) => rate.enabled !== false)
857+
.sort((a, b) => (a.index ?? CONST.DEFAULT_NUMBER_ID) - (b.index ?? CONST.DEFAULT_NUMBER_ID))
858+
.at(0);
859+
expect(defaultRate?.name).toBe('Default Rate');
860+
expect(defaultRate?.rate).toBe(72.5);
861+
// The default rate's customUnitRateID matches its dictionary key (rebound).
862+
expect(defaultRate?.customUnitRateID).not.toBe(sourceDefaultRateID);
863+
const defaultRateID = defaultRate?.customUnitRateID;
864+
if (!defaultRateID) {
865+
throw new Error('Expected default rate to have a customUnitRateID');
866+
}
867+
expect(distanceUnit.rates[defaultRateID]).toBe(defaultRate);
868+
869+
// Resume the mocked fetch so successData fires and clears the optimistic source rate IDs.
870+
// After success, only the rebound default remains until a Pusher response (not mocked here)
871+
// would repopulate the rates with fresh server IDs.
872+
await mockFetch.resume?.();
873+
await waitForBatchedUpdates();
874+
875+
const duplicateAfterSuccess: OnyxEntry<PolicyType> = await new Promise((resolve) => {
876+
const connection = Onyx.connect({
877+
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
878+
callback: (workspace) => {
879+
Onyx.disconnect(connection);
880+
resolve(workspace);
881+
},
882+
});
883+
});
884+
885+
const distanceUnitAfter = Object.values(duplicateAfterSuccess?.customUnits ?? {}).find((unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE);
886+
if (!distanceUnitAfter) {
887+
throw new Error('Expected duplicated distance unit to still exist after success');
888+
}
889+
const ratesAfter = Object.entries(distanceUnitAfter.rates).filter(([, rate]) => rate !== null);
890+
expect(ratesAfter).toHaveLength(1);
891+
expect(ratesAfter.at(0)?.[1]?.name).toBe('Default Rate');
892+
});
893+
894+
it('duplicate workspace without distance rates does not write any customUnits cleanup for the source distance rates', async () => {
895+
await Onyx.set(ONYXKEYS.SESSION, {email: ESH_EMAIL, accountID: ESH_ACCOUNT_ID});
896+
897+
const sourceDistanceUnitID = 'srcDistUnitB';
898+
const sourceExtraRateID = 'srcExtraRateB';
899+
const fakePolicy: PolicyType = {
900+
...createRandomPolicy(20, CONST.POLICY.TYPE.TEAM),
901+
customUnits: {
902+
[sourceDistanceUnitID]: {
903+
customUnitID: sourceDistanceUnitID,
904+
name: CONST.CUSTOM_UNITS.NAME_DISTANCE,
905+
enabled: true,
906+
attributes: {unit: CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES},
907+
rates: {
908+
srcDefaultRateB: {customUnitRateID: 'srcDefaultRateB', name: 'Default Rate', rate: 72.5, currency: 'USD', enabled: true, index: 0},
909+
[sourceExtraRateID]: {customUnitRateID: sourceExtraRateID, name: 'Extra Rate', rate: 100, currency: 'USD', enabled: true, index: 1},
910+
},
911+
},
912+
},
913+
};
914+
await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${fakePolicy.id}`, fakePolicy);
915+
await waitForBatchedUpdates();
916+
917+
const policyID = Policy.generatePolicyID();
918+
const options = {
919+
currentUserAccountID: ESH_ACCOUNT_ID,
920+
currentUserEmail: ESH_EMAIL,
921+
policyName: 'No Distance Duplicate',
922+
policyID: fakePolicy.id,
923+
targetPolicyID: policyID,
924+
welcomeNote: 'Join my policy',
925+
parts: {
926+
people: false,
927+
reports: false,
928+
connections: false,
929+
categories: false,
930+
tags: false,
931+
taxes: false,
932+
perDiem: false,
933+
reimbursements: false,
934+
expenses: false,
935+
distance: false,
936+
invoices: false,
937+
exportLayouts: false,
938+
},
939+
localCurrency: 'USD',
940+
};
941+
942+
Policy.duplicateWorkspace(fakePolicy, options);
943+
await waitForBatchedUpdates();
944+
945+
const duplicatePolicy: OnyxEntry<PolicyType> = await new Promise((resolve) => {
946+
const connection = Onyx.connect({
947+
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
948+
callback: (workspace) => {
949+
Onyx.disconnect(connection);
950+
resolve(workspace);
951+
},
952+
});
953+
});
954+
955+
// The duplicate should not have any distance custom unit because parts.distance is false.
956+
// In particular, it should not contain a partial/orphan distance unit consisting only of
957+
// nulled-out source rate IDs (which would be the regression the gating prevents).
958+
const distanceUnits = Object.values(duplicatePolicy?.customUnits ?? {}).filter((unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE);
959+
expect(distanceUnits).toHaveLength(0);
960+
// The source's extra rate ID must not appear anywhere in the duplicate's customUnits as
961+
// a stale "null" entry from successData cleanup.
962+
for (const unit of Object.values(duplicatePolicy?.customUnits ?? {})) {
963+
expect(Object.keys(unit.rates ?? {})).not.toContain(sourceExtraRateID);
964+
}
965+
});
966+
776967
it('creates a new workspace with BASIC approval mode if the introSelected is MANAGE_TEAM', async () => {
777968
const policyID = Policy.generatePolicyID();
778969
// When a new workspace is created with introSelected set to MANAGE_TEAM

0 commit comments

Comments
 (0)