Skip to content

[CP Staging] Source signerEmail from session instead of account.primaryLogin - #97226

Merged
mountiny merged 2 commits into
Expensify:mainfrom
callstack-internal:VickyStash/bugfix/97151-fix-blank-signer-email
Aug 3, 2026
Merged

[CP Staging] Source signerEmail from session instead of account.primaryLogin#97226
mountiny merged 2 commits into
Expensify:mainfrom
callstack-internal:VickyStash/bugfix/97151-fix-blank-signer-email

Conversation

@VickyStash

@VickyStash VickyStash commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

Non-USD (e.g. UK/GBP) bank account setup could get permanently stuck on the "hang tight" screen because SaveCorpayOnboardingDirectorInformation was sent with a blank signerEmail, failing backend required-field validation.

Both submission paths read the email from account.primaryLogin, which isn't reliable:

  • Delegate.connect calls clearOnyxForDelegateTransition, which wipes ONYXKEYS.ACCOUNT and only re-hydrates it asynchronously via OpenApp. ONYXKEYS.SESSION is in KEYS_TO_PRESERVE_DELEGATE_ACCESS and survives the switch.
  • So in a Copilot session account.primaryLogin can be blank or belong to the wrong account, while session.email is the account being acted on. DelegateAccessHandler already relies on this.
  • It can also be blank in a normal session whenever ONYXKEYS.ACCOUNT is unhydrated at submit time.

Fix: read signerEmail from ONYXKEYS.SESSION (via the existing emailSelector) in both paths — NonUSD/SignerInfo/index.tsx (in-line director flow) and EnterSignerInfo/index.tsx (emailed-signer flow).

Fixed Issues

$ #97151
PROPOSAL: N/A

Tests

  • Verify that no errors appear in the JS console
  1. Use a Copilot account.
  2. Open a workspace with a non-USD currency (e.g. GBP).
  3. Add console.log to src/pages/ReimbursementAccount/NonUSD/SignerInfo/index.tsx with currentUserEmail. Open link https://dev.new.expensify.com:8082/bank-account/new/global/signer-info/is-director?policyID=<POLICY_ID> . See, currentUserEmail isn't empty.
  4. Add console.log to src/pages/ReimbursementAccount/EnterSignerInfo/index.tsx with signerEmail. Open link https://dev.new.expensify.com:8082/bank-account/enter-signer-info?policyID=<POLICY_ID>&bankAccountID=1&isCompleted=false. See, signerEmail isn't empty.

Offline tests

N/A

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."

  • Verify that no errors appear in the JS console

  • Complete UK/non-USD bank account signer/director verification in a normal session → succeeds.

  • Complete it in a Copilot/delegate session → succeeds (previously stuck on "hang tight").

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

MacOS: Chrome / Safari image image

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
...ges/ReimbursementAccount/EnterSignerInfo/index.tsx 0.00% <0.00%> (ø)
...s/ReimbursementAccount/NonUSD/SignerInfo/index.tsx 0.00% <0.00%> (ø)
... and 10 files with indirect coverage changes

@VickyStash
VickyStash marked this pull request as ready for review July 28, 2026 12:09
@VickyStash
VickyStash requested review from a team as code owners July 28, 2026 12:09
@melvin-bot
melvin-bot Bot requested review from flaviadefaria and situchan July 28, 2026 12:09
@melvin-bot

melvin-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

@situchan 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]

@github-actions

Copy link
Copy Markdown
Contributor

🚧 joekaufmanexpensify has triggered a test Expensify/App build. You can view the workflow run here.

@github-actions

This comment has been minimized.

@situchan

situchan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
android.mov
Android: mWeb Chrome
iOS: HybridApp
ios.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov

@joekaufmanexpensify

Copy link
Copy Markdown
Contributor

Retested and working well. I was able to make it through the director flow to the bank account confirmation page from both a normal and copilot account.

image

@joekaufmanexpensify
joekaufmanexpensify requested review from joekaufmanexpensify and removed request for flaviadefaria July 28, 2026 16:47

@joekaufmanexpensify joekaufmanexpensify left a comment

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.

Good from my perspective.

@VickyStash

Copy link
Copy Markdown
Contributor Author

Kind bump @situchan on review here

@MelvinBot

Copy link
Copy Markdown
Contributor

Review

The fix is correct and well-scoped. Sourcing signerEmail from SESSION (via emailSelector) instead of account.primaryLogin is the right call, and I was able to confirm the stated root cause directly.

Verified root cause: ONYXKEYS.SESSION is in KEYS_TO_PRESERVE_DELEGATE_ACCESS but ONYXKEYS.ACCOUNT is not — so clearOnyxForDelegateTransition wipes ACCOUNT (re-hydrated async via OpenApp) while SESSION survives. In a Copilot/delegate session account.primaryLogin can therefore be blank at submit time, producing the empty signerEmail that failed backend required-field validation and stuck the flow on "hang tight". Both submission paths (NonUSD/SignerInfo and EnterSignerInfo) are correctly updated, useCallback dependency arrays are updated to match, and the useOnyx(SESSION, {selector: emailSelector}) + default '' pattern matches ~30 existing usages in the codebase. This also aligns with how DelegateAccessHandler already treats session.email as the acting account.

Two minor, non-blocking notes:

Details
  1. session.email vs account.primaryLogin for secondary logins. For a user who signed in with a secondary login, session.email is that secondary email while primaryLogin was the primary. So in a normal session this changes which email is submitted as the signer/director email (it's used directly as signerDetails.email). That's almost certainly the intended/more-correct behavior — the signer is the acting user, and backend validation is against the session — but worth a conscious confirmation since it's a behavior change beyond just the Copilot fix.

  2. Unit test checkbox. The author checklist has "I added unit tests…" checked, but the diff only touches the two index.tsx files — no test was added. This flow may be impractical to unit test, so not necessarily a blocker; just flagging the checkbox/diff mismatch.

Nothing here blocks merge — items above are just for the reviewer's awareness.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: b5e213de9c

ℹ️ 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".

@VickyStash

Copy link
Copy Markdown
Contributor Author

Hey @situchan, any ETA on reviewing this one?

@situchan

Copy link
Copy Markdown
Contributor

ETA today

@VickyStash

Copy link
Copy Markdown
Contributor Author

I'll be OOO next week (back Aug 10) 🌴
If any adjustments are needed, please ask @TMisiukiewicz so he can help to find someone

@situchan

Copy link
Copy Markdown
Contributor

QA Steps

Prerequisites

  • A workspace whose currency is non-USD (e.g. GBP/UK). Create one or use an existing one.
  • Access to a Copilot (delegate) relationship: Account A has added Account B as a Copilot (Settings → Security → Copilot). You'll log in as B and act on A's behalf.

Test 1 — Normal session (director completes signer info)

  1. Log in as the workspace owner.
  2. Go to Workspace → Bank account and start adding a bank account for the non-USD workspace.
  3. Proceed through the flow until the Signer information step.
  4. When asked "Are you a director?", select Yes and fill in the signer details (name, job title, date of birth, address, and upload the required documents).
  5. Submit the signer information.
  6. Verify: the flow advances to the next step and the account continues setting up. It does not get permanently stuck on the "Hang tight" screen.

Test 2 — Copilot / delegate session (the previously-broken case)

  1. Log in as Account B, then switch into Account A via Copilot (Settings → Security → Copilot → select Account A).
  2. While acting as Account A, open Account A's non-USD workspace and start / resume adding the bank account.
  3. Reach the Signer information step, select Yes to "Are you a director?", complete the signer details, and submit.
  4. Verify: the flow advances normally and does not hang on the "Hang tight" screen (this is the exact scenario that was permanently stuck before the fix).

Test 3 — Emailed second signer (AUD / two-signer path, if applicable)

  1. In a non-USD flow that requires a second signer, enter a second signer's email and send the invite.
  2. Open the emailed signer info link and log in as that signer.
  3. Complete and submit the signer details.
  4. Verify: submission succeeds and the flow continues without getting stuck on "Hang tight".

Expected result (all tests): the signer/director step completes successfully; the bank account setup never gets permanently stuck on the "Hang tight" screen.

@melvin-bot

melvin-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

We did not find an internal engineer to review this PR, trying to assign a random engineer to #97151 as well as to this PR... Please reach out for help on Slack if no one gets assigned!

@melvin-bot
melvin-bot Bot requested a review from jasperhuangg July 31, 2026 17:58

@situchan situchan left a comment

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.

LGTM

jasperhuangg
jasperhuangg previously approved these changes Jul 31, 2026
@jasperhuangg

Copy link
Copy Markdown
Contributor

@situchan mind completing the reviewer checklist? thanks

@situchan

situchan commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@situchan mind completing the reviewer checklist? thanks

It's already here. Not sure why it's failing

@melvin-bot
melvin-bot Bot requested a review from jasperhuangg August 1, 2026 02:18
@situchan

situchan commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@MelvinBot why PR Reviewer Checklist failing even if it's filled here?

@MelvinBot

Copy link
Copy Markdown
Contributor

The check is stale — it's not actually seeing your checked boxes. Just re-submit a review on the PR to re-run it and it'll pass.

What happened:

  • The failing run (30379895160) was triggered by a pull_request_review event on Jul 28 16:47Z, and at that moment your checklist comment still had empty boxes. The action's log says: You completed 0 out of 43 checklist items with 43 unfinished items.
  • You checked all the boxes later — the comment was last edited Jul 31 17:57Z (comment now shows all - [x]).

The workflow runs on: pull_request_review and reads the checklist comment at that moment. Editing the checklist comment does not re-trigger it, so the failing status is frozen from before you ticked the boxes.

Fix: submit any new review on the PR (e.g. re-approve, or a "comment" review). That fires a fresh pull_request_review event, the action re-reads the now-completed checklist, and the check goes green.

…51-fix-blank-signer-email

# Conflicts:
#	src/pages/ReimbursementAccount/EnterSignerInfo/index.tsx
@mountiny
mountiny requested review from mountiny and removed request for jasperhuangg August 3, 2026 08:20
@mountiny mountiny changed the title Source signerEmail from session instead of account.primaryLogin [CP Staging] Source signerEmail from session instead of account.primaryLogin Aug 3, 2026
@mountiny
mountiny merged commit 4da55ef into Expensify:main Aug 3, 2026
37 checks passed
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here.

OSBotify pushed a commit that referenced this pull request Aug 3, 2026
…7151-fix-blank-signer-email

[CP Staging] Source signerEmail from session instead of `account.primaryLogin`

(cherry picked from commit 4da55ef)

(cherry-picked to staging by mountiny)
@OSBotify OSBotify added the CP Staging marks PRs that have been CP'd to staging label Aug 3, 2026
@OSBotify

OSBotify commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

OSBotify commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.4.48-1 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 No help site changes required.

I reviewed the changes in this PR and no updates to the help site files under docs/articles are needed.

Why: This is a purely internal bug fix. It changes where signerEmail is sourced (from account.primaryLoginONYXKEYS.SESSION via emailSelector) so that non-USD (e.g. UK/GBP) signer/director submission no longer fails with a blank email and gets stuck on the "hang tight" screen. From the user's perspective the flow is unchanged:

  • No new or renamed UI, tabs, buttons, or settings labels.
  • No change to any documented step. The Global Reimbursement articles (e.g. Global Reimbursement – United Kingdom) describe entering director/signer information, and those steps are identical after this change.
  • The email-sourcing detail is an implementation concern that isn't (and shouldn't be) documented in the help site.

Since there's no user-facing behavior or copy change to document, I did not create a draft docs PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CP Staging marks PRs that have been CP'd to staging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants