Skip to content

Fix Travel Invoicing toggle incorrectly enabled by root-level card settings #83248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rlinoz merged 1 commit into from
Feb 24, 2026
Merged

Fix Travel Invoicing toggle incorrectly enabled by root-level card settings #83248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 7 additions & 11 deletions src/libs/TravelInvoicingUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,17 @@ function isTravelCVVTestingEnabled(): boolean {
}

/**
* Gets the Travel Invoicing settings, handling both nested (TRAVEL_US) and root-level data.
* Backend may return data under cardSettings.TRAVEL_US or at the root level.
* We prioritize TRAVEL_US if it exists, otherwise fall back to root level.
* Gets the Travel Invoicing settings from the nested TRAVEL_US object.
* Only returns settings if cardSettings.TRAVEL_US exists — root-level cardSettings
* (e.g. paymentBankAccountID for the Expensify Card) must not be treated as Travel Invoicing data.
*/
function getTravelSettings(cardSettings: OnyxEntry<ExpensifyCardSettings>): ExpensifyCardSettingsBase | undefined {
if (!cardSettings) {
if (!cardSettings?.TRAVEL_US) {
return undefined;
Comment on lines +26 to 27
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 24, 2026

Choose a reason for hiding this comment

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

P1 Badge Preserve root fallback until enable flow stops reading root fields

Returning undefined whenever TRAVEL_US is missing breaks the current enable path for workspaces that only have a root-level paymentBankAccountID: WorkspaceTravelInvoicingSection.handleToggle() now always routes to settlement-account setup (hasTravelInvoicingSettlementAccount becomes false), but WorkspaceTravelInvoicingSettlementAccountPage.handleSelectAccount() still reads the same root-level account and treats selecting it as a no-op (value === paymentBankAccountID). In the common single-bank-account case, admins can get stuck unable to turn Travel Invoicing on because no call to setTravelInvoicingSettlementAccount/toggleTravelInvoicing is made.

Useful? React with 👍 / 👎.

Copy link
Contributor

@rlinoz rlinoz Mar 4, 2026

Choose a reason for hiding this comment

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

I think this actually broke enabling invoicing because we are still flattening the card settings?

}
// Prefer nested TRAVEL_US if it exists, but merge with root settings to ensure we have all properties.
// This handles optimistic updates where only a partial TRAVEL_US object (e.g. enabled state) is written.
if (cardSettings.TRAVEL_US) {
return {...cardSettings, ...cardSettings.TRAVEL_US};
}
// Fall back to root level (for optimistic updates and backward compat)
return cardSettings;
// Merge root settings with TRAVEL_US so partial optimistic updates (e.g. only isEnabled) still
// inherit other fields like monthlySettlementDate from the root.
return {...cardSettings, ...cardSettings.TRAVEL_US};
}

/**
Expand Down
105 changes: 81 additions & 24 deletions tests/unit/TravelInvoicingUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,28 +49,37 @@ describe('TravelInvoicingUtils', () => {
expect(result).toBe(false);
});

it('Should return false when isEnabled is explicitly false', () => {
it('Should return false when root-level isEnabled is false without TRAVEL_US', () => {
const cardSettings = {isEnabled: false} as ExpensifyCardSettings;
const result = getIsTravelInvoicingEnabled(cardSettings);
expect(result).toBe(false);
});

it('Should return true when isEnabled is explicitly true', () => {
it('Should return false when root-level isEnabled is true without TRAVEL_US (not travel settings)', () => {
const cardSettings = {isEnabled: true} as ExpensifyCardSettings;
const result = getIsTravelInvoicingEnabled(cardSettings);
expect(result).toBe(true);
expect(result).toBe(false);
});

it('Should return false when isEnabled is undefined and no paymentBankAccountID (new account)', () => {
it('Should return false when isEnabled is undefined and no TRAVEL_US (new account)', () => {
// Empty settings (like from loading state) should return false, not true
const cardSettings = {} as ExpensifyCardSettings;
const result = getIsTravelInvoicingEnabled(cardSettings);
expect(result).toBe(false);
});

it('Should return true when isEnabled is undefined with valid paymentBankAccountID', () => {
it('Should return false when root-level paymentBankAccountID exists but no TRAVEL_US (Expensify Card settlement account)', () => {
const cardSettings = {paymentBankAccountID: 12345} as ExpensifyCardSettings;
const result = getIsTravelInvoicingEnabled(cardSettings);
expect(result).toBe(false);
});

it('Should return true when TRAVEL_US has valid paymentBankAccountID and isEnabled is undefined', () => {
const cardSettings = {
// eslint-disable-next-line @typescript-eslint/naming-convention
Copy link
Contributor

@github-actions github-actions bot Feb 24, 2026

Choose a reason for hiding this comment

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

❌ CONSISTENCY-5 (docs)

This eslint-disable-next-line lacks a justification comment explaining why the rule is being disabled. This pattern repeats across all newly added TRAVEL_US test objects in this file.

Add a brief comment explaining the reason, for example:

// TRAVEL_US matches the backend API property name
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {paymentBankAccountID: 12345},

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

Copy link
Contributor

@rlinoz rlinoz Feb 24, 2026

Choose a reason for hiding this comment

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

We don't usually give a reason in tests, so all good.

Copy link
Contributor Author

@MelvinBot MelvinBot Feb 24, 2026

Choose a reason for hiding this comment

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

Added justification comments to all 21 eslint-disable-next-line directives in the test file:

  • TRAVEL_US properties: "TRAVEL_US matches the backend API property name"
  • Numeric string keys: "Card IDs use numeric string keys from the backend"

Pushed in commit bc532a0.

TRAVEL_US: {paymentBankAccountID: 12345},
} as ExpensifyCardSettings;
const result = getIsTravelInvoicingEnabled(cardSettings);
expect(result).toBe(true);
});

Expand Down Expand Up @@ -123,9 +132,18 @@ describe('TravelInvoicingUtils', () => {
expect(result).toBe(false);
});

it('Should return true when paymentBankAccountID is a valid non-zero value', () => {
it('Should return false when only root-level paymentBankAccountID exists (no TRAVEL_US)', () => {
const cardSettings = {paymentBankAccountID: 67890} as ExpensifyCardSettings;
const result = hasTravelInvoicingSettlementAccount(cardSettings);
expect(result).toBe(false);
});

it('Should return true when TRAVEL_US has a valid non-zero paymentBankAccountID', () => {
const cardSettings = {
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {paymentBankAccountID: 67890},
} as ExpensifyCardSettings;
const result = hasTravelInvoicingSettlementAccount(cardSettings);
expect(result).toBe(true);
});
});
Expand All @@ -136,14 +154,17 @@ describe('TravelInvoicingUtils', () => {
expect(result).toBe(0);
});

it('Should return 0 when remainingLimit is not set', () => {
const cardSettings = {} as ExpensifyCardSettings;
it('Should return 0 when no TRAVEL_US exists', () => {
const cardSettings = {remainingLimit: 50000} as ExpensifyCardSettings;
const result = getTravelLimit(cardSettings);
expect(result).toBe(0);
});

it('Should return the remainingLimit value when set', () => {
const cardSettings = {remainingLimit: 50000} as ExpensifyCardSettings;
it('Should return the limit value from TRAVEL_US when set', () => {
const cardSettings = {
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {remainingLimit: 50000},
} as ExpensifyCardSettings;
const result = getTravelLimit(cardSettings);
expect(result).toBe(50000);
});
Expand All @@ -155,14 +176,17 @@ describe('TravelInvoicingUtils', () => {
expect(result).toBe(0);
});

it('Should return 0 when currentBalance is not set', () => {
const cardSettings = {} as ExpensifyCardSettings;
it('Should return 0 when no TRAVEL_US exists', () => {
const cardSettings = {currentBalance: 25000} as ExpensifyCardSettings;
const result = getTravelSpend(cardSettings);
expect(result).toBe(0);
});

it('Should return the currentBalance value when set', () => {
const cardSettings = {currentBalance: 25000} as ExpensifyCardSettings;
it('Should return the currentBalance value from TRAVEL_US when set', () => {
const cardSettings = {
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {currentBalance: 25000},
} as ExpensifyCardSettings;
const result = getTravelSpend(cardSettings);
expect(result).toBe(25000);
});
Expand All @@ -174,14 +198,26 @@ describe('TravelInvoicingUtils', () => {
expect(result).toBe(CONST.EXPENSIFY_CARD.FREQUENCY_SETTING.MONTHLY);
});

it('Should return daily when monthlySettlementDate is not set', () => {
it('Should return monthly (default) when no TRAVEL_US exists', () => {
const cardSettings = {} as ExpensifyCardSettings;
const result = getTravelSettlementFrequency(cardSettings);
expect(result).toBe(CONST.EXPENSIFY_CARD.FREQUENCY_SETTING.MONTHLY);
});

it('Should return daily when TRAVEL_US has no monthlySettlementDate', () => {
const cardSettings = {
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {isEnabled: true},
} as ExpensifyCardSettings;
const result = getTravelSettlementFrequency(cardSettings);
expect(result).toBe(CONST.EXPENSIFY_CARD.FREQUENCY_SETTING.DAILY);
});

it('Should return monthly when monthlySettlementDate is set', () => {
const cardSettings = {monthlySettlementDate: new Date('2024-01-15')} as ExpensifyCardSettings;
it('Should return monthly when TRAVEL_US has monthlySettlementDate', () => {
const cardSettings = {
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {monthlySettlementDate: new Date('2024-01-15')},
} as ExpensifyCardSettings;
const result = getTravelSettlementFrequency(cardSettings);
expect(result).toBe(CONST.EXPENSIFY_CARD.FREQUENCY_SETTING.MONTHLY);
});
Expand Down Expand Up @@ -212,31 +248,49 @@ describe('TravelInvoicingUtils', () => {
expect(result).toBeUndefined();
});

it('Should use paymentBankAccountAddressName when available', () => {
it('Should return undefined when only root-level paymentBankAccountID exists (no TRAVEL_US)', () => {
const cardSettings = {
paymentBankAccountID: 12345,
paymentBankAccountAddressName: 'Custom Name',
paymentBankAccountNumber: '****5678',
} as ExpensifyCardSettings;
const result = getTravelSettlementAccount(cardSettings, mockBankAccountList);
expect(result).toBeUndefined();
});

it('Should use paymentBankAccountAddressName from TRAVEL_US when available', () => {
const cardSettings = {
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {
paymentBankAccountID: 12345,
paymentBankAccountAddressName: 'Custom Name',
paymentBankAccountNumber: '****5678',
},
} as ExpensifyCardSettings;
const result = getTravelSettlementAccount(cardSettings, mockBankAccountList);
expect(result).toBeDefined();
expect(result?.displayName).toBe('Custom Name');
expect(result?.last4).toBe('5678');
});

it('Should fallback to bank account data when paymentBankAccountAddressName is not set', () => {
it('Should fallback to bank account data when TRAVEL_US paymentBankAccountAddressName is not set', () => {
const cardSettings = {
paymentBankAccountID: 'bankAccountID' as unknown as number,
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {
paymentBankAccountID: 'bankAccountID' as unknown as number,
},
} as ExpensifyCardSettings;
const result = getTravelSettlementAccount(cardSettings, mockBankAccountList);
expect(result).toBeDefined();
expect(result?.displayName).toBe('Test Company');
expect(result?.last4).toBe('1234');
});

it('Should return bankAccountID in the result', () => {
it('Should return bankAccountID from TRAVEL_US in the result', () => {
const cardSettings = {
paymentBankAccountID: 12345,
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {
paymentBankAccountID: 12345,
},
} as ExpensifyCardSettings;
const result = getTravelSettlementAccount(cardSettings, mockBankAccountList);
expect(result).toBeDefined();
Expand All @@ -245,7 +299,10 @@ describe('TravelInvoicingUtils', () => {

it('Should handle missing bank account in list gracefully', () => {
const cardSettings = {
paymentBankAccountID: 99999,
// eslint-disable-next-line @typescript-eslint/naming-convention
TRAVEL_US: {
paymentBankAccountID: 99999,
},
} as ExpensifyCardSettings;
const result = getTravelSettlementAccount(cardSettings, mockBankAccountList);
expect(result).toBeDefined();
Expand Down
Loading