Only check for valid phone number in the deposit bank account flow - #97801
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Review — LGTM ✅The change is clean, minimal, and correct. It strictly loosens validation: the phone step previously required a number to pass both Verified:
Two things worth confirming (neither a blocker)
Optional: no unit test was added for the widened behavior (the removed tests covered NANP). A one-line assertion that In-app verification: I attempted to exercise the flow on web, but the tester was blocked before the phone step — EvidenceWallet page with Add bank account buttonValidate your account security-code wall blocking the flow |
|
@luacmartins 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-08-10.12.15.57.movAndroid: mWeb Chrome2026-08-10.12.09.39.moviOS: HybridApp2026-08-10.12.15.57.moviOS: mWeb Safari2026-08-10.12.09.39.movMacOS: Chrome / Safari2026-08-10.12.01.34.mov |
|
I suppose we have a mismatch here
const parsed = parsePhoneNumber(rawPhone, {regionCode: CONST.COUNTRY.US});
const phoneNumber = parsed.number?.significant ?? '';
I think we need to use |
|
And about these steps
Get reimbursed + UK doesn't require a phone number |
The update flow stripped the calling code with parsePhoneNumber().number.significant, so a UK number was sent as 10 bare digits and the BE read it as NANP. Reuse formatE164PhoneNumber() so this flow matches the add flow.
|
Good catch, you are right. Fixed in 82c600e. I went with It also upgrades legacy values. A stored Added coverage for both helpers, since removing
There is a related backend limitation on the same field that this PR does not touch: https://github.com/Expensify/Expensify/issues/669521 |
|
@ZhenjaHorbach can you check again please? |
Will check on the weekend or on Monday morning! |
| expect(parsedPhone).toBe('+12345678901'); | ||
| }); | ||
| }); | ||
| describe('formatE164PhoneNumber', () => { |
There was a problem hiding this comment.
Minor comment
But strange that we don't have empty lines before describe in this test file
|
But overall changes look good! |
|
@puneetlath 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] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.52-0 🚀
|
Help site review — no docs changes requiredI reviewed the changes in this PR against the help site files under Why: This PR is a bug fix. The deposit bank account phone number step was incorrectly rejecting valid non‑US/Canada numbers (it ran The relevant help article,
I also grepped all of Since no changes are required, I did not open a draft help site PR. @mountiny, please confirm you agree no help site update is needed. If you'd like a doc change anyway (e.g. to explicitly call out international phone support on the deposit flow), let me know and I'll open a draft PR. |
|
Hi @mountiny. QA team failed this PR on iOS with an original issue. Step 23 fails: "Please enter a valid phone number" message appears when entering +442071234567 1786465218554.97801-iOS-2.mp4 |
|
@MelvinBot can you check this flow #97801 (comment) and investigate why the phone number is not accepted? |
|
TL;DR: The PR's validation change is correct — a cleanly-entered
Investigation details1. The error is client-side validation, not the backend. 2. A clean 3. Leading hypothesis for why only the update flow failed. To confirm / fix:
Next Steps: Reply with |
|
@MelvinBot Yeah looks like it was correct +442071234567 |
|
Good catch — and The screenshot is the USD business bank account flow, not the personal deposit flow this PR fixed. The title "What's your company's phone number?" is This PR never touched that screen — and deliberately so. The PR description says the "genuinely US only callers … the USD The screen this PR actually fixed is the personal deposit step titled "What's your phone number?" ( Bottom line: this isn't a regression from PR — QA validated the wrong screen (the "company's phone number" business step). To validate this PR, test the deposit step titled "What's your phone number?" (Wallet → Add bank account → Get reimbursed → personal deposit), not the "What's your company's phone number?" business step. If you also want the USD business flow to accept non-US numbers, that's a separate change and a separate discussion. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.52-11 🚀
Bundle Size Analysis (Sentry): |
|
Ah right.. @IuliiaHerets can you please make sure to retest with the international flow? |
@trjExpensify in this US business bank account, should we allow non-USD phone numbers as well? |
|
@mountiny, steps 1-18 are now a pass, but we are unable to check steps 19-20 because the bank account is in Verifying status. Checking this PR was especially difficult for us because of vague test steps. Retest-97801-2.mp4Retest-97801-1.mp4 |
Good question. I'll defer to @joekaufmanexpensify on that for US VBBAs |
|
Yeah, I'm not aware we need to restrict this at all. Couldn't find any backend restriction that requires a US phone number here. It seems like it's only required on the client side. |
|
Great, works for me then! |




Explanation of Change
The phone number step in the deposit bank account flow validated the number twice. It ran
isValidPhoneNumber()on the number with the country code appended, and it also ranisValidNANPPhone()on the E.164 formatted number.isValidNANPPhone()only accepted numbers that parse to a North American Numbering Plan region:US,PR,GU,VI,AS,MP, andCA. This is the international deposit account flow, so the account holder can live anywhere. A user with a valid number such as+44 20 7123 4567sawPlease enter a valid phone numberand could not finish the step.This PR keeps
isValidPhoneNumber()as the only phone check in the step. It uses libphonenumber'spossiblecheck, so it accepts a valid number from any country and still rejects malformed input.isValidNANPPhone()is removed fromValidationUtilsbecause this step was its only caller. The genuinely US only callers, in the EnablePayments wallet flows and the USDReimbursementAccountflow, useisValidUSPhone()and are unchanged.Widening the step exposed a second bug in the update flow, which renders the same step.
UpdatePersonalBankAccountPagenormalized the phone withparsePhoneNumber().number.significant, which strips the calling code. That was harmless while the step guaranteed+1, becausesignificantthen yields exactly the 10 NANP digits. Once any country is accepted,+442071234567becomes2071234567and the backend reads it as a NANP number. The add flow already sent E.164, so the two flows disagreed on the format of the samecompanyPhonefield.Both flows now send E.164 through the shared
formatE164PhoneNumber()helper. The helper appends the country code first, which matters because the form draft holds whatever the user typed and that can have no calling code.Changed files:
src/pages/AddPersonalBankAccountPage/substeps/PhoneNumberStep.tsx— drop theisValidNANPPhone()check and the E.164 formatting it needed.src/pages/settings/Wallet/UpdatePersonalBankAccountPage.tsx— send E.164 instead of the national significant number, matching the add flow.src/libs/ValidationUtils.ts— removeisValidNANPPhone()and its export.tests/unit/ValidationUtilsTest.ts— replace theisValidNANPPhonedescribe block with anisValidPhoneNumberblock covering US, CA, GB, and AU numbers plus malformed input.tests/unit/LoginUtilsTest.ts— add aformatE164PhoneNumberdescribe block.Fixed Issues
$ #97806
PROPOSAL:
Tests
+44 20 7123 4567.+1 201 867 5309.+1 416 555 1234.123.Please enter a valid phone numberand the step does not continue.abcdefg.Please enter a valid phone numberand the step does not continue.+44 20 7123 4567, and finish the flow.UpdatePersonalBankAccountInforequest, and verify thatcompanyPhoneis+442071234567and not2071234567.201 867 5309, and verify thatcompanyPhoneis+12018675309.npm run test -- tests/unit/ValidationUtilsTest.ts tests/unit/LoginUtilsTest.tsand verify that both suites pass.Offline tests
+44 20 7123 4567.123.Please enter a valid phone number.QA Steps
+44 20 7123 4567.+61 2 5550 1234.+1 201 867 5309.123.Please enter a valid phone numberand the step does not continue.+44 20 7123 4567, and finish the flow.+44.201 867 5309.+1.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.Screenshots/Videos
Android: Native
Not yet tested — needs manual QA.
Android: mWeb Chrome
Not yet tested — needs manual QA.
iOS: Native
Not yet tested — needs manual QA.
iOS: mWeb Safari
Not yet tested — needs manual QA.
MacOS: Chrome / Safari
Not yet tested — needs manual QA.