[HOLD for payment 2024-04-15] [$500] BA - On the phone number page, user can not enter data as the placeholder shows

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.4.47-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team

Action Performed:

Pre-requisite: user must be logged in and have created a workspace.

1. Go to Workspace > Bank account.
2. Initiate the add BA flow with the testing credentials.
3. On Company information section, reach the Phone number page.
4. Verify the placeholder is displayed as "(xxx)xxx-xxxx".
5. Enter a random US number in that same format, like "(440)458-9784".
6. Click on "Next".
7. Verify an error appears.

Expected Result:

The user should be able to enter the phone number as the placeholder shows.

Actual Result:

The user can not enter the phone number as the placeholder shows.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6402382_1709617407384.bandicam_2024-03-04_23-44-34-559.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f2cb151e80502fdb
• Upwork Job ID: 1765154798992896000
• Last Price Increase: 2024-03-05

Issue Owner

Current Issue Owner: @mallenexpensify

[melvin-bot]

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[izarutskaya]

@mallenexpensify I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The user can not enter the phone number as the placeholder shows.

What is the root cause of that problem?

We already have the logic to check the phone number is valid or not in here

but it's not really correct since it can cause some confused cases: #34766

so we added the additional logic to check phone number in

App/src/libs/ValidationUtils.ts

Line 287 in ffa731a

if (regionCode && !Str.isValidPhone(phone)) {

but isValidPhone just use the simple e164 regex

https://github.com/Expensify/expensify-common/blob/77d0b150ba6bfbe7a64b3c3e30b65592b2e58c4a/lib/CONST.jsx#L571-L572

and that doesn't cover other format like (440)458-9784

What changes do you think we should make in order to solve the problem?

• We shouldn't update isValidPhone logic since it's used in many places in the App, instead of that we should change its name to isValidE164Phone then replace it in all places

• Create other function isValidPhone to handle all possible phone number

Here's the possible format:

significant: 4404589784
international: +1 440-458-9784
e164: +14404589784
national: (440) 458-978
123.456.7890

so we can use the following regex:

const phoneNumberRegex = /(\+\d{1,2}\s?)?(\(\d{3}\)|\d{3})[\s.-]?\d{3}[\s.-]?\d{4}/;

What alternative solutions did you explore? (Optional)

NA

[dragnoir]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Phone number in the format (440)458-9784 is marked as invalid

What is the root cause of that problem?

Issue is within the function isValidUSPhone
https://github.com/Expensify/App/blob/578769197ddd4039f46b949d83a7e85ad2476abc/src/libs/ValidationUtils.ts#L281C10-L293

When the input is in phormat like: (440)458-9784
The condition below:

App/src/libs/ValidationUtils.ts

Lines 287 to 289 in 5787691

	if (regionCode && !Str.isValidPhone(phone)) {
	return false;
	}

will turn:
regionCode: US and !Str.isValidPhone(phone): true
Then the execution will exit without approving the phone number

What changes do you think we should make in order to solve the problem?

We need to fix this condition

App/src/libs/ValidationUtils.ts

Lines 285 to 289 in 5787691

	// When we pass regionCode as an option to parsePhoneNumber it wrongly assumes inputs like '=15123456789' as valid
	// so we need to check if it is a valid phone.
	if (regionCode && !Str.isValidPhone(phone)) {
	return false;
	}

It's built to fix an issue as mentioned in the comment:

// When we pass regionCode as an option to parsePhoneNumber it wrongly assumes inputs like '=15123456789' as valid

Currently we use Str.isValidPhone(phone) to check if the input is a valid phone number. isValidPhone is a function from expensify-common. We can replace it with another function we create to check all possible format and return a true for valid US phone numbers.

What alternative solutions did you explore? (Optional)

We can fix the function isValidPhone from expensify-common to consider the format (xxx)xxx-xxxx as a valid phone number
https://github.com/Expensify/expensify-common/blob/77d0b150ba6bfbe7a64b3c3e30b65592b2e58c4a/lib/str.js#L934-L936

[allgandalf]

I think this should be closed, as seen in the video, we don't enter space as specified in the placeholder and hence we face error as the regex doesn't match

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f2cb151e80502fdb

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Pujan92 (External)

[mallenexpensify]

Was able to reproduce and seems like we'd want to fix. I, personally, would find it frustrating.
Let me find which wave to drop it into.
@Pujan92 , can you also please try to repro? Bank account deets here

Checking for #wave-collect to add.

[melvin-bot]

📣 @waleedj1! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[waleedj1]

This issue is due to Validation For Phone Number Input Field Is Failing, Due To Some Issue In The Regex, We can solve this by improving our regex

Contributor details
Expensify Account: [email protected]
upwork Account: https://www.upwork.com/freelancers/~0124f16df0b524ada4

[Pujan92]

Yes @mallenexpensify , I am able to reproduce here and for enable wallet screen too as both uses the same common function.

Do we need to allow users to enter in placeholder format((xxx) xxx-xxxx) or should it be like the way we allow for the login("Please enter a valid phone number, with the country code (e.g. +15005550006)")?
Asking above to decide whether a placeholder change is needed or the pattern support needed.

[mallenexpensify]

Thanks for testing @Pujan92 . I've added this to the wave-collect project as Polish.

Do we need to allow users to enter in placeholder format((xxx) xxx-xxxx) or should it be like the way we allow for the login

Are there other examples of where a phone number is entered and accepted in multiple formats? Ideally want input to be consistent with otheres (and... to also allow for multiple formats, if possible).

[Pujan92]

Do we need to allow users to enter in placeholder format((xxx) xxx-xxxx) or should it be like the way we allow for the login

Are there other examples of where a phone number is entered and accepted in multiple formats? Ideally want input to be consistent with otheres (and... to also allow for multiple formats, if possible).

Earlier it used to be working with this format((xxx) xxx-xxxx) and many more. I think this problem arises after this PR. To allow multiple format case we need a solution which solves this issue as well as #34766.

[dragnoir]

@Pujan92 my proposal allows multiple formats

[Pujan92]

Thanks @dragnoir , I will review the proposals tomorrow.

[tienifr]

Ideally want input to be consistent with otheres (and... to also allow for multiple formats, if possible).

@Pujan92 I updated my proposal to support multiple formats:

[Pujan92]

All proposals RCA are correct, I like the @tienifr's proposal as it is more detailed and agree with the fact that isValidPhone is used at multiple places and some places we are not supporting multiple formats(for eg. login). So renaming it to isValidE164Phone and creating other function for supporting multiple formats makes more sense to me.

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Validate different phone number formats #38993
• Bump expensify-common #37723 #39634

If no regressions arise, payment will be issued on 2024-04-15. 🎊

For reference, here are some details about the assignees on this issue:

• @dragnoir requires payment (Needs manual offer from BZ)
• @Pujan92 requires payment (Needs manual offer from BZ)
• @tienifr requires payment (Needs manual offer from BZ)

[dragnoir]

@Pujan92 the new regex fails with a common US phone number like "1 212 555 0194". But it's valid with parsePhoneNumber .

Also, the new function is called isValidPhone where it checks only US phone formats!

Pls think again about this #37723 (comment)

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @dragnoir paid $(AMOUNT) via Upwork (LINK)
• ROLE: @Pujan92 paid $(AMOUNT) via Upwork (LINK)
• ROLE: @tienifr paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@mallenexpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1765154798992896000/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[mallenexpensify]

Checking on something, will issue payment shortly
@tienifr , @Pujan92 @dragnoir , can you please apply to the job and reply here once you have. Thx
https://www.upwork.com/jobs/~01b909addf4ed7ece2

[tienifr]

@mallenexpensify I've applied, thank you

[dragnoir]

@mallenexpensify applied. Thank you

[mallenexpensify]

@Pujan92 , @tienifr , @dragnoir can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01b909addf4ed7ece2

[dragnoir]

Accepted

[Pujan92]

Accepted!

[mallenexpensify]

Contributor: @dragnoir paid $200 via Upwork
Contributor: @tienifr paid $300 via Upwork
Contributor+: @Pujan92 paid $250 via Upwork (reflects deduction from regression).

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

@Pujan92 , can you please fill out the BZ checklist? One wasn't auto posted, so pasting below.

• [@] The PR that introduced the bug has been identified. Link to the PR:
• [@] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@] Determine if we should create a regression test for this bug.
• [@] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@bz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:
• [ ]

[Pujan92]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@Pujan92] The PR that introduced the bug has been identified. Link to the PR: Fix - Able to save phone number starting from = not + via WS Bank account connection #35293
• [@Pujan92] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: Fix - Able to save phone number starting from = not + via WS Bank account connection #35293 (comment)
• [@Pujan92] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: NA
• [@Pujan92] Determine if we should create a regression test for this bug. Yes
• [@Pujan92] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps

1. Go to Workspace > Bank account.
2. Initiate the add BA flow and reach to the Company information screen.
3. Try to enter phone number with the format (xxx)xxx-xxxx
4. Verify the number gets validated and won't show any error

Do you agree 👍 or 👎

[mallenexpensify]

Thanks @Pujan92
TR GH is here

• https://github.com/Expensify/Expensify/issues/392197