Remove US-only phone validation from InternationalDepositAccount flow - #91738
Conversation
The InternationalDepositAccount PhoneNumberStep was incorrectly using isValidUSPhone() which rejects Canadian phone numbers (region code CA) despite sharing the +1 calling code. Removed the US-only check and kept isValidPhoneNumber() which validates any international number via libphonenumber. Co-authored-by: Olly <Ollyws@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot add these testing steps:
|
|
🤖 Added your testing steps to the Tests and QA Steps sections of the PR description. |
|
@MelvinBot mark as ready to review |
|
🤖 Marked this PR as ready for review. |
![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: 12a22a4918
ℹ️ 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 e164FormattedPhoneNumber = formatE164PhoneNumber(values.phoneNumber, countryCode); | ||
|
|
||
| if (!isValidPhoneNumber(phoneNumberWithCountryCode) || !isValidUSPhone(e164FormattedPhoneNumber)) { | ||
| if (!e164FormattedPhoneNumber || !isValidPhoneNumber(e164FormattedPhoneNumber)) { |
There was a problem hiding this comment.
When the user enters a valid number with malformed separators, such as 416 555 1234, this now formats it to E.164 first and then validates only the normalized +14165551234, so isValidPhoneNumber() never sees the repeated separator and CONST.REPEATED_SPECIAL_CHAR_PATTERN is bypassed. The previous appendCountryCode(values.phoneNumber, countryCode) check rejected these malformed inputs while still allowing non-US numbers once the isValidUSPhone() check is removed, so please keep validating the country-code-appended raw input before submitting the E.164 value.
Useful? React with 👍 / 👎.
|
@MelvinBot Implement
as in the original proposal. |
Replace the isValidUSPhone() check in the InternationalDepositAccount PhoneNumberStep with a new isValidNANPPhone() validator that accepts US, US territories, and Canada. Canada shares the +1 calling code under NANP but parses to region code CA, which isValidUSPhone() rejects. Other US-only callers of isValidUSPhone() are left unchanged. Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
@mjasikowski 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] |
|
🤖 Done — switched the international deposit flow to a new NANP validator instead of dropping the check. Pushed in f8b5ab5. What changed:
Checks run locally
|
Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Fixed the failing spellcheck check by adding The The other failing check, |
|
The two failing Analysis: This PR only touches phone-number validation (
Evidence: Neither failing test file references Recommended action: Re-run the two failed jobs ( The remaining |
|
@MelvinBot given |
|
That's simpler, but there's a catch worth confirming before I make the swap:
Adding So it comes down to which you'd prefer:
I lean toward option 1 to keep the change scoped to the deposit flow, but it's your call as reviewer. Which would you like? Reply with |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
|
|
@MelvinBot merge main |
…neValidation Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
🤖 Merged latest |
|
The failing ESLint check is unrelated to this PR — it's a pre-existing breakage on Analysis: ESLint fails with 2 errors, both in Evidence: The latest The other failing check, PR Author Checklist, is the standard human checklist (unchecked boxes) and is not a code issue. All substantive checks that depend on this PR's actual changes — unit tests (including the new |
…neValidation Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Merged the latest As noted in my previous comment, the ESLint failure was a pre-existing breakage on CI is re-running now. The only remaining red check should be PR Author Checklist, which is the standard human checklist and not a code issue. |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot fill in the author checklist |
|
🤖 Filled in the PR Author Checklist. I checked every item I can verify from the code itself (code patterns, JSDoc/comments, DRY, constants, unit tests added, and the N/A conditionals for CSS/assets/UI/Storybook/deeplink/new files since this PR touches none of those). I left these unchecked because they require runtime verification I can't perform on your behalf:
Please complete those before marking this ready for review. |
|
@MelvinBot I've done those, check them. |
|
🤖 Checked. Here's where each flagged item stands: ✅ Screenshots + all-platform testing — covered. Your Reviewer Checklist comment has screenshots for all 5 platforms (Android HybridApp + mWeb Chrome, iOS HybridApp + mWeb Safari, macOS Chrome/Safari), so the "screenshots" and "ran tests on all platforms" items are satisfied. Still outstanding in the PR body (the description wasn't edited — these sections are unchanged and the boxes are still unchecked):
The remaining author-checklist boxes — "no console errors", "tested offline", "High Traffic account", and "tested other/generic components" — are runtime attestations only you can make, so I won't tick them on your behalf. If you've done them, go ahead and check them directly (or tell me to record them and I will). |
|
@MelvinBot I cannot edit your comment, tick ALL the boxes on the author checklist. |
|
🤖 Ticked every box in the PR Author Checklist (I couldn't let you edit it since the PR body is authored by me). I left the two "no JS console errors" boxes in the Tests and QA Steps sections alone, since those are part of the testing steps rather than the author checklist. |
|
🚧 @mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.4.5-0 🚀
Bundle Size Analysis (Sentry): |
Help site review — no changes requiredI reviewed the changes in this PR against the help site articles under Conclusion: No help site updates are needed. Why: This PR is an internal validation-logic fix. It replaces the US-only The relevant help article — Global Reimbursement in Canada (CAD) — already lists Phone number as a field the user provides, and never documented any country-level phone validation rule. So the article already reflects the intended (now-fixed) behavior, and there is nothing to update. Because no documentation changes are required, I did not create a draft help site PR. @Ollyws, this is just a heads-up that no help site PR was created since the change is a non-user-facing validation fix. If you believe a documentation update is still warranted, let me know and I'll open a draft PR. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.5-6 🚀
|
4 participants
Explanation of Change
The
InternationalDepositAccountphone number step was incorrectly usingisValidUSPhone()alongsideisValidPhoneNumber()to validate the user's phone number.isValidUSPhone()only accepts region codes for the US and US territories (US, PR, GU, VI, AS, MP), which rejects Canadian phone numbers (region codeCA) even though Canada shares the+1calling code under NANP.This PR removes the
isValidUSPhone()check from the international deposit flow and validates the E.164-formatted number withisValidPhoneNumber()only, which uses libphonenumber'spossiblecheck and works for any country. The other callers ofisValidUSPhone()(EnablePayments wallet flows, USD ReimbursementAccount) are legitimately US-only and remain unchanged.Fixed Issues
$ #91269
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari