Skip to content

Fall back to the session email when account.primaryLogin is missing in BookTravelButton #100931

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

Open
MelvinBot wants to merge 3 commits into
base: main
Choose a base branch
from
+75 −7
Open

Fall back to the session email when account.primaryLogin is missing in BookTravelButton #100931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de99bd2
Fall back to the session email when account.primaryLogin is missing i…
MelvinBot Sep 11, 2026
b44d473
Remove the blank line left behind by the dropped account useOnyx call
MelvinBot Sep 11, 2026
22fa38d
Treat an empty primaryLogin as missing in usePrimaryContactMethod
MelvinBot Sep 11, 2026
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
8 changes: 2 additions & 6 deletions src/components/BookTravelButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import useLocalize from '@hooks/useLocalize';
import useOnyx from '@hooks/useOnyx';
import usePermissions from '@hooks/usePermissions';
import usePolicy from '@hooks/usePolicy';
import usePrimaryContactMethod from '@hooks/usePrimaryContactMethod';
import useScreenBoundDynamicRoute from '@hooks/useScreenBoundDynamicRoute';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
Expand All @@ -28,7 +29,6 @@ import type WithSentryLabel from '@src/types/utils/SentryLabel';

import type {ReactElement} from 'react';

import {emailSelector} from '@selectors/Session';
import {Str} from 'expensify-common';
import React, {useEffect, useState} from 'react';

Expand Down Expand Up @@ -67,15 +67,11 @@ function BookTravelButton({
const illustrations = useMemoizedLazyIllustrations(['RocketDude']);
const {translate} = useLocalize();
const {environmentURL} = useEnvironment();
const [account] = useOnyx(ONYXKEYS.ACCOUNT);
const primaryLogin = account?.primaryLogin ?? '';

const policy = usePolicy(activePolicyID);
const blockIfDefaultWorkspaceLacksTravel = useDefaultWorkspaceTravelGuard();
const [errorMessage, setErrorMessage] = useState<string | ReactElement>('');
const [travelSettings] = useOnyx(ONYXKEYS.NVP_TRAVEL_SETTINGS);
const [sessionEmail] = useOnyx(ONYXKEYS.SESSION, {selector: emailSelector});
const primaryContactMethod = primaryLogin ?? sessionEmail ?? '';
const primaryContactMethod = usePrimaryContactMethod();

@chatgpt-codex-connector chatgpt-codex-connector Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Handle an empty primaryLogin before booking travel

When Onyx contains account.primaryLogin === '', usePrimaryContactMethod() still returns the empty string because it uses ??, so it never falls back to the valid session email and bookATrip continues to show the contact-method error. This is a real account state—the repository explicitly handles it in src/pages/settings/VerifyAccountPageBase.tsx:41-43—so the reported travel-blocking bug remains for those users; treat an empty primary login as missing here or update the hook to use the session email for this case.

Useful? React with 👍 / 👎.

@daledah daledah Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MelvinBot please check this comment

@MelvinBot MelvinBot Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — this is correct, and I've fixed it in 22fa38d.

usePrimaryContactMethod() used ??, so an account.primaryLogin of '' short-circuited the session-email fallback and returned '' — the exact failure the PR set out to remove, just reached through a different Onyx state. The precedent you cite is real: src/pages/settings/VerifyAccountPageBase.tsx:41-43 already uses || with the comment "sometimes primaryLogin can be empty string".

I fixed the hook rather than patching BookTravelButton, because all 13 call sites are contact-method flows (validate-code pages, card verification, travel) where '' is never a usable value:

// primaryLogin is sometimes stored as an empty string rather than being absent, so treat it as missing and fall back to the session email.
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
return account?.primaryLogin || sessionEmail || '';

Added four regression tests: empty primaryLogin with and without a session email in tests/unit/hooks/usePrimaryContactMethod.test.ts, plus an empty-string case in tests/unit/components/BookTravelButtonTest.tsx.

The four pre-existing hook tests keep their expected values under ||, since none of them exercise a non-empty falsy input.

One caveat: the sandbox on this run blocked me from executing Jest, ESLint, and typecheck locally, so CI is the first real verification of this commit. I'd hold off merging until the checks come back green.

view run

@MelvinBot MelvinBot Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Codex is right, and this is already fixed — commit 22fa38d landed at 09:25 UTC, about 90 minutes after the comment, so the thread is just stale. No further change needed.

I re-ran it as a negative control to be sure the finding was real rather than speculative. Reverting src/hooks/usePrimaryContactMethod.ts:12 from || back to ?? fails exactly the two empty-string tests and nothing else:

● usePrimaryContactMethod › should fall back to session email when primaryLogin is an empty string
● BookTravelButton › … › falls back to the session email when primaryLogin is stored as an empty string
Tests: 2 failed, 17 passed, 19 total

With || in place, 19/19 pass. So primaryLogin === '' really did slip through the original fix, and the follow-up closes it.

On Codex's evidence, and one thing worth a reviewer's eye

The precedent Codex cites checks out. src/pages/settings/VerifyAccountPageBase.tsx:41-43 carries the comment "sometimes primaryLogin can be empty string" with an eslint-disable for prefer-nullish-coalescing — the same shape the fix now uses. src/pages/signin/SignInPage.tsx:247 also falsy-checks it (!account?.primaryLogin).

Caveat on how strong that evidence is: I could not find any code path in App that writes primaryLogin: ''. The two writers I can see — setContactMethodAsDefault and the HybridApp delegate path — both write a real value or undefined. The empty string would have to arrive from a sign-in API payload, so the two defensive sites above are the evidence, not a traceable write. The repo clone here is shallow, so I could not blame those lines back to the incident that prompted them. Either way the fix is harmless if the state never occurs, and correct if it does.

Worth a sanity check before merge: the fix is in the shared hook, so it changes behavior for all 14 consumers, not just travel — the validate-code and card pages in src/pages/settings/Wallet/, src/pages/workspace/expensifyCard/, and src/pages/workspace/companyCards/. In every one of those, an empty primaryLogin now resolves to the session email instead of '', which is the strictly better outcome (they use it to address a validation code). I see no consumer that wants ''. Fixing it in the hook rather than locally in BookTravelButton also matches the VerifyAccountPageBase precedent, so I think it is the right call — but the blast radius is wider than this PR's title suggests, and it is your call to confirm.

One note: I could not verify any of this in a browser. Web prep failed on this run, so NewDot web could not be driven. Everything above is from the test suite and static analysis on 22fa38d.

view run

const {isBetaEnabled} = usePermissions();
const {showConfirmModal} = useConfirmModal();
const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);
Expand Down
4 changes: 3 additions & 1 deletion src/hooks/usePrimaryContactMethod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import useOnyx from './useOnyx';
function usePrimaryContactMethod(): string {
const [account] = useOnyx(ONYXKEYS.ACCOUNT);
const [sessionEmail] = useOnyx(ONYXKEYS.SESSION, {selector: emailSelector});
return account?.primaryLogin ?? sessionEmail ?? '';
// primaryLogin is sometimes stored as an empty string rather than being absent, so treat it as missing and fall back to the session email.
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
return account?.primaryLogin || sessionEmail || '';
}

export default usePrimaryContactMethod;
46 changes: 46 additions & 0 deletions tests/unit/components/BookTravelButtonTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,52 @@ describe('BookTravelButton', () => {
});
});

describe('when account.primaryLogin is absent from Onyx (e.g. a session restored from storage after a reload)', () => {
it('falls back to the session email instead of blocking the user with the contact-method error', async () => {
// Given a validated admin whose work email is only known from the session, not from account.primaryLogin
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${POLICY_ID}`, provisionedPolicy);
await Onyx.merge(ONYXKEYS.ACCOUNT, {validated: true});
await Onyx.merge(ONYXKEYS.SESSION, {email: USER_LOGIN});
await Onyx.merge(ONYXKEYS.NVP_TRAVEL_SETTINGS, {hasAcceptedTerms: false});
await Onyx.merge(ONYXKEYS.PRIVATE_PERSONAL_DETAILS, {legalFirstName: 'Test', legalLastName: 'User'});
await waitForBatchedUpdatesWithAct();
});
renderBookTravelButton();
await waitForBatchedUpdatesWithAct();

// When the admin presses the book travel button
fireEvent.press(screen.getByText('Book a trip'));
await waitForBatchedUpdatesWithAct();

// Then travel enablement proceeds rather than surfacing the "add a work email" error
expect(Navigation.navigate).toHaveBeenCalledWith(ENABLE_TRAVEL_ROUTE);
expect(screen.queryByText(/add a work email as your primary login/)).toBeNull();
});

it('falls back to the session email when primaryLogin is stored as an empty string', async () => {
// Given a validated admin whose account.primaryLogin is present but empty, a state the app writes in practice
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${POLICY_ID}`, provisionedPolicy);
await Onyx.merge(ONYXKEYS.ACCOUNT, {validated: true, primaryLogin: ''});
await Onyx.merge(ONYXKEYS.SESSION, {email: USER_LOGIN});
await Onyx.merge(ONYXKEYS.NVP_TRAVEL_SETTINGS, {hasAcceptedTerms: false});
await Onyx.merge(ONYXKEYS.PRIVATE_PERSONAL_DETAILS, {legalFirstName: 'Test', legalLastName: 'User'});
await waitForBatchedUpdatesWithAct();
});
renderBookTravelButton();
await waitForBatchedUpdatesWithAct();

// When the admin presses the book travel button
fireEvent.press(screen.getByText('Book a trip'));
await waitForBatchedUpdatesWithAct();

// Then travel enablement proceeds rather than surfacing the "add a work email" error
expect(Navigation.navigate).toHaveBeenCalledWith(ENABLE_TRAVEL_ROUTE);
expect(screen.queryByText(/add a work email as your primary login/)).toBeNull();
});
});

describe('when the user has a personal-email login', () => {
it('shows the public-domain error even when legal details are missing', async () => {
// Given a user logged in with a public-domain email and no legal name set yet
Expand Down
24 changes: 24 additions & 0 deletions tests/unit/hooks/usePrimaryContactMethod.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,30 @@ describe('usePrimaryContactMethod', () => {
expect(result.current).toBe('session-only@expensify.com');
});

it('should fall back to session email when primaryLogin is an empty string', async () => {
await act(async () => {
await Onyx.merge(ONYXKEYS.ACCOUNT, {primaryLogin: ''});
await Onyx.merge(ONYXKEYS.SESSION, {email: 'session-only@expensify.com'});
await waitForBatchedUpdates();
});

const {result} = renderHook(() => usePrimaryContactMethod());

expect(result.current).toBe('session-only@expensify.com');
});

it('should return empty string when primaryLogin is empty and there is no session email', async () => {
await act(async () => {
await Onyx.merge(ONYXKEYS.ACCOUNT, {primaryLogin: ''});
await Onyx.merge(ONYXKEYS.SESSION, {});
await waitForBatchedUpdates();
});

const {result} = renderHook(() => usePrimaryContactMethod());

expect(result.current).toBe('');
});

it('should return empty string when neither primaryLogin nor session email exist', async () => {
await act(async () => {
await Onyx.merge(ONYXKEYS.ACCOUNT, {});
Expand Down
Loading