Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/libs/CardUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1084,11 +1084,24 @@ function getCardSettings(cardSettings: OnyxEntry<ExpensifyCardSettings>, feedCou
return undefined;
}

if (feedCountry) {
const feedCountryCardSettings = cardSettings[feedCountry as keyof typeof cardSettings];
if (feedCountryCardSettings && typeof feedCountryCardSettings === 'object' && !Array.isArray(feedCountryCardSettings)) {
return feedCountryCardSettings as ExpensifyCardSettingsBase;
const tryNested = (key: string): ExpensifyCardSettingsBase | undefined => {
const nested = cardSettings[key as keyof typeof cardSettings];
if (nested && typeof nested === 'object' && !Array.isArray(nested)) {
return {...cardSettings, ...(nested as ExpensifyCardSettingsBase)} as ExpensifyCardSettingsBase;
}
return undefined;
};

if (feedCountry) {
return tryNested(feedCountry) ?? cardSettings;
}

// Auto-detect: try known card programs in priority order so callers that
// don't pass feedCountry still get the right program sub-object when the
// backend sends nested settings (Phase 2 of fixing shared Onyx key).
const result = tryNested(CONST.COUNTRY.US) ?? tryNested('CURRENT') ?? tryNested('GB');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-2 (docs)

The string literals 'CURRENT' and 'GB' are hardcoded magic strings. 'GB' already has a constant defined at CONST.COUNTRY.GB (the same line already uses CONST.COUNTRY.US, so this is inconsistent). 'CURRENT' has no constant defined anywhere and should be added to a relevant CONST object (e.g., CONST.EXPENSIFY_CARD.CARD_PROGRAM.CURRENT or similar) so its meaning is discoverable and centralized.

Additionally, 'CURRENT' and 'GB' are not represented in the ExpensifyCardSettings type definition (only US and TRAVEL_US are typed as optional nested keys). This means cardSettings[key as keyof typeof cardSettings] relies on an unsafe as cast to access keys the type system doesn't know about. Consider adding CURRENT?: ExpensifyCardSettingsBase and GB?: ExpensifyCardSettingsBase to the ExpensifyCardSettings type to make these accesses type-safe.

Suggested fix:

// 1. Add to CONST (or appropriate location):
CARD_PROGRAM: {
    CURRENT: 'CURRENT',
} as const,

// 2. Update ExpensifyCardSettings type to include GB and CURRENT:
type ExpensifyCardSettings = OnyxCommon.OnyxValueWithOfflineFeedback<
    ExpensifyCardSettingsBase & {
        US?: ExpensifyCardSettingsBase;
        GB?: ExpensifyCardSettingsBase;
        CURRENT?: ExpensifyCardSettingsBase;
        TRAVEL_US?: ExpensifyCardSettingsBase;
        hasOnceLoaded?: boolean;
    }
>;

// 3. Use constants on line 1102:
const result = tryNested(CONST.COUNTRY.US) ?? tryNested(CONST.EXPENSIFY_CARD.CARD_PROGRAM.CURRENT) ?? tryNested(CONST.COUNTRY.GB);

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 90e7daf — replaced magic strings with CONST.COUNTRY.GB and CONST.EXPENSIFY_CARD.CARD_PROGRAM.CURRENT, added GB? and CURRENT? to ExpensifyCardSettings type, renamed tryNested to getMergedProgramSettings

if (result) {
return result;
}

return cardSettings;
Expand Down
61 changes: 47 additions & 14 deletions tests/unit/CardUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3245,6 +3245,8 @@ describe('CardUtils', () => {
} as ExpensifyCardSettings;

const nestedSettings = {
domainName: 'example.com',
preferredPolicy: 'policyID',
paymentBankAccountID: 12345,
limit: 50000,
US: {
Expand All @@ -3268,42 +3270,73 @@ describe('CardUtils', () => {
expect(getCardSettings(null as unknown as undefined)).toBeUndefined();
});

it('should return flat root when feedCountry is not provided', () => {
it('should return flat root when feedCountry is not provided and no nested keys exist', () => {
const result = getCardSettings(flatSettings);
expect(result).toBe(flatSettings);
});

it('should return flat root when feedCountry is undefined', () => {
it('should return flat root when feedCountry is undefined and no nested keys exist', () => {
const result = getCardSettings(flatSettings, undefined);
expect(result).toBe(flatSettings);
});

it('should return nested object when feedCountry matches a nested key', () => {
it('should return merged root + nested when feedCountry matches a nested key', () => {
const result = getCardSettings(nestedSettings, 'US');
expect(result).toEqual({
paymentBankAccountID: 67890,
limit: 30000,
currentBalance: 500,
});
expect(result?.paymentBankAccountID).toBe(67890);
expect(result?.limit).toBe(30000);
expect(result?.currentBalance).toBe(500);
expect(result?.domainName).toBe('example.com');
});

it('should fall back to flat root when feedCountry key does not exist', () => {
it('should fall back to root when feedCountry key does not exist', () => {
const result = getCardSettings(nestedSettings, 'CA');
expect(result).toBe(nestedSettings);
});

it('should return TRAVEL_US nested settings when feedCountry is TRAVEL_US', () => {
it('should return merged root + TRAVEL_US when feedCountry is TRAVEL_US', () => {
const result = getCardSettings(nestedSettings, 'TRAVEL_US');
expect(result).toEqual({
paymentBankAccountID: 11111,
isEnabled: true,
});
expect(result?.paymentBankAccountID).toBe(11111);
expect(result?.isEnabled).toBe(true);
expect(result?.domainName).toBe('example.com');
});

it('should not return primitive values as nested settings', () => {
const result = getCardSettings(nestedSettings, 'limit');
expect(result).toBe(nestedSettings);
});

it('should auto-detect US program when no feedCountry is provided', () => {
const result = getCardSettings(nestedSettings);
expect(result?.paymentBankAccountID).toBe(67890);
expect(result?.limit).toBe(30000);
expect(result?.domainName).toBe('example.com');
});

it('should auto-detect GB program when only GB nested key exists', () => {
const gbOnlySettings = {
domainName: 'uk-example.com',
GB: {
paymentBankAccountID: 99999,
limit: 20000,
},
} as ExpensifyCardSettings;
const result = getCardSettings(gbOnlySettings);
expect(result?.paymentBankAccountID).toBe(99999);
expect(result?.domainName).toBe('uk-example.com');
});

it('should auto-detect CURRENT program for legacy pre-2024 nested format', () => {
const currentSettings = {
domainName: 'legacy.com',
CURRENT: {
paymentBankAccountID: 55555,
limit: 10000,
},
} as ExpensifyCardSettings;
const result = getCardSettings(currentSettings);
expect(result?.paymentBankAccountID).toBe(55555);
expect(result?.domainName).toBe('legacy.com');
});
});
});

Expand Down
Loading