Fall back to the session email when account.primaryLogin is missing in BookTravelButton - #100931
Fall back to the session email when account.primaryLogin is missing in BookTravelButton#100931MelvinBot wants to merge 3 commits into
Conversation
…n BookTravelButton Co-authored-by: daledah <daledah@users.noreply.github.com>
Co-authored-by: daledah <daledah@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
🤖 Web test steps verification results: I could not run these steps in a browser. Web prep failed on this run (no dev SSL cert / web backend), so NewDot web could not be driven and no screenshots or recordings were captured. Instead I verified the behavior each step asserts by running the component tests against the branch at One note before the table: the
Test outputAgainst the branch as-is: Scratch cases for step 6 (since deleted): Negative control, with the pre-fix lines restored in Code the steps exerciseThe guard is if (!primaryContactMethod || Str.isSMSLogin(primaryContactMethod)) {
If the Web environment comes back up, re-run with the |
|
|
|
Nothing to fix here — the failing check is pre-existing on
This PR only touches It is already tracked as #100888 ( The other red check, Evidence
Per the "don't fix unrelated failures" rule I left it alone — patching a Search test from a travel-button PR would muddy this diff and collide with whoever fixes #100888. Once that lands, merge One caveat: I could not drive NewDot in a browser on this run, so the travel flow itself is unverified by me. That analysis is entirely from CI logs and the diff. The manual |
|
@MelvinBot please open this PR |
|
🤖 Done — this PR is out of draft and open for review. Two things still need a human before a reviewer can sign off, and both will fail the checklist checks as-is:
The PR Author Checklist is also mostly unchecked, including the all-platforms screenshots and the per-platform test runs. On testing: I could not verify anything in a browser on this run — web prep failed, so NewDot web couldn't be driven and no screenshots or recordings exist. What I did verify locally is in my earlier comment: the regression test genuinely fails on the pre-fix code, and a phone-number primary login still gets blocked as intended. The browser steps and the |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safariweb.mov |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b44d4739fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const [travelSettings] = useOnyx(ONYXKEYS.NVP_TRAVEL_SETTINGS); | ||
| const [sessionEmail] = useOnyx(ONYXKEYS.SESSION, {selector: emailSelector}); | ||
| const primaryContactMethod = primaryLogin ?? sessionEmail ?? ''; | ||
| const primaryContactMethod = usePrimaryContactMethod(); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@MelvinBot i've completed my checklist, please complete the author checklist |
|
🤖 Author checklist updated — 13 of the 30 boxes are now checked. The 17 I left unchecked all need a browser or need you, and I've listed every one below. I could not verify anything in a browser on this run either. Web prep failed again, so NewDot web could not be driven and no screenshots or recordings exist. Everything I checked off was verified from the diff, local unit tests, and green CI checks. What I checked off
What I did not check, and whyNeeds you — the
Needs a real app session, which I never got:
Deliberate:
Also worth knowing
What I verified locally about the fix itself is in this comment: the regression test fails on the pre-fix code, and a phone-number primary login is still correctly blocked. |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
dylanexpensify
left a comment
dylanexpensify
left a comment
There was a problem hiding this comment.
Looks good from a product perspective 👍
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot please check this codex comment |
account.primaryLogin is sometimes written as an empty string rather than being absent. Because '' is not nullish, ?? never fell through to the session email, so the hook returned '' and BookTravelButton still showed the contact-method error for those users. Use || so an empty primaryLogin falls back to the session email, matching the existing precedent in VerifyAccountPageBase. Co-authored-by: daledah <daledah@users.noreply.github.com>
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@carlosmiceli Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
No code fix needed — the last red check is a human review gate, not a failure in your code. I pushed nothing. On the current head ( The only remaining red check is Nobody has approved the PR yet. That clears when an eligible reviewer approves — there is no commit that can fix it. What changed since my last comment
One thing worth a reviewer's eye: that change edits a shared hook, not just the travel button. Widening the fallback there also affects the card and validate-code pages that call Still outstanding for a human: the |
3 participants
Explanation of Change
BookTravelButtonblocked travel enablement with "Please add a work email as your primary login to book travel." for users who already had a work email as their primary login.The intended fallback to the session email was dead code:
primaryLoginhad already been defaulted to''. Because''is not nullish,?? sessionEmail ?? ''never evaluated, so the session-email fallback was unreachable.account.primaryLoginis optional in Onyx and is only written by sign-in responses,setContactMethodAsDefault, and the HybridApp delegate path — the app-init payload does not populate it. Any session that reached the Travel page without one of those writes (a reload, a session restored from storage, a copilot switch, a new account) therefore had it undefined,primaryContactMethodresolved to'', and the!primaryContactMethodguard rendered the error.Str.isSMSLogin('')isfalse, so this was unambiguously the empty-value branch rather than real SMS-login detection. That explains why the failure looked intermittent and per-session, and why Expensify Classic was unaffected.This replaces both lines with the existing
usePrimaryContactMethod()hook, which returns exactly the fallback the original code was trying to express (account?.primaryLogin ?? sessionEmail ?? ''). Several card and validate-code pages already use it, so this also makes travel consistent with the rest of the app and lets the two now-unuseduseOnyxcalls be dropped. As a side effect, the public-domain check on the next branch stops evaluating against an empty string.A regression test was added covering the case where
account.primaryLoginis absent but the session email is present. It fails onmainand passes with this change.Deliberately not included, so they can be scoped separately:
travel.phoneErrorstring says "add a work email as your primary login" while the condition it guards means "primary login is missing or is a phone number". Once this fix lands the string only renders for genuine SMS logins, where the wording is accurate, so the copy change is no longer load-bearing for this bug. Rewording it also needs translations across all 12 locales and marketing sign-off.primaryLoginin the app-init account payload would close the underlying data gap. That is tracked internally and is out of scope for this issue.Fixed Issues
$ #100870
PROPOSAL: #100870
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate your change worked.
//
// Suggested starting point, matching the reproduction steps on the linked issue:
// 1. Sign in with an account whose primary login is a work email (not a phone number, not a public domain).
// 2. Hard-reload the app so the session is restored from storage rather than from a fresh sign-in response.
// 3. Confirm in the Onyx debug view that
account.primaryLoginis absent.// 4. Open a workspace -> Travel and press the travel button.
// 5. Verify travel enablement proceeds and no "Please add a work email as your primary login to book travel" error renders.
// 6. Verify an account whose primary login IS a phone number still sees that error.
Offline tests
The change only alters which already-cached Onyx values the travel guard reads, so offline behavior is unchanged: the guard reads
session.emailfrom local storage, which is present offline, and the travel button behaves the same as when online.QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
// The human co-author must fill out the QA steps before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Notes on the boxes left unchecked
Everything above that is checked, I verified. The rest I could not, and they are left for the human co-author rather than checked optimistically:
TestsandQA Stepssections are human-owned and still carry their// TODO:placeholders.BookTravelButtonrenders on deeplinkable travel routes, so this item does apply — it just needs a browser I did not have.Designlabel /@Expensify/design. This change alters no UI, so I deliberately did not tag the design team. The parent conditional is checked because it does not apply; this sub-item is left unchecked rather than claiming an action I did not take.AI Tests
Run locally by MelvinBot on this branch:
npm run typechecknpm test -- tests/unit/components/BookTravelButtonTest.tsxnpm test -- --testPathPattern "([Tt]ravel|EmptySearchView|ManageTrips|MyTrips)"BookTravelButtonconsumersnpm run spell-changednpm run react-compiler-compliance-check -- check src/components/BookTravelButton.tsxnpx eslint src/components/BookTravelButton.tsx tests/unit/components/BookTravelButtonTest.tsxESLint checkis green.npm run prettierprettierscript or Prettier config; formatting is enforced through ESLint andOxfmt check(green in CI).npm run storybook -- --smoke-test --ciBookTravelButton. CI'sStorybook testsis green.The new regression test was verified to fail on the pre-fix code and pass after the change.
Browser verification: none. No screenshots or recordings were captured on any run.
Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari