Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2021-12-06] [$250] When creating new accounts, allow users to enter their phone numbers with - and/or ( ) and spaces #6227

Closed
mallenexpensify opened this issue Nov 4, 2021 · 16 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Reviewing Has a PR in review Weekly KSv2

Comments

@mallenexpensify
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

Flow 1:

  1. Navigate to NewDot
  2. Input a phone number as login using any of symbols -, (, ) or spaces (e.g. +1 (222) 222 2222)

Flow 2:

  1. Open concierge chat in NewDot
  2. Click the call icon on the top right
  3. Input a phone number as login using any of symbols -, (, ) or spaces (e.g. +1 (222) 222 2222)

Expected Result:

The user should be able to sign in or request a call from concierge just fine . The number input by the user should be cleaned before being sent to the back-end, stripping -, (, ) and spaces.

Actual Result:

We get validation errors

Screen Shot 2021-11-04 at 10 13 54 AM

Screen Shot 2021-11-04 at 10 14 32 AM

Screen Shot 2021-11-04 at 10 16 01 AM

## Platform: Where is this issue occurring? ALL

Version Number:
Reproducible in staging?:
Reproducible in production?:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/183582#
Issue reported by:
Slack conversation:

View all open jobs on GitHub

@mallenexpensify mallenexpensify added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Nov 4, 2021
@MelvinBot
Copy link

Triggered auto assignment to @sakluger (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Nov 4, 2021
@mallenexpensify
Copy link
Contributor Author

Upwork Job posted. cc @aldo-expensify in case you want to snag this, auto-assigning via Exported tag now.
https://www.upwork.com/jobs/~01e3a9fa28fc98c96a

@botify botify removed the Daily KSv2 label Nov 5, 2021
@MelvinBot MelvinBot added Weekly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 5, 2021
@MelvinBot
Copy link

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

@Bojan996
Copy link

Bojan996 commented Nov 5, 2021

After following the code, I see that the main issue is the regex that is used to validate the phone number.
Because you are using the expensify-common git repo for validation you will either have to change the code directly in the repo or add a custom regex to your project (for this specific case only). I would suggest changing the code on expensify-common (I see it's the same company name so I guess you have access), because this will improve future use.

The function that you are using for validation can be found in expensify-common/lib/str.js 921
This uses the regex found in expensify-common/lib/CONST.jsx 518

This can be improved, I have a regex that will solve it but I need to know are you restricting the users to follow a country specific phone pattern, would you allow users to not put in spaces, parenthesis and a plus sign or do they have to?

If you provide some more details about what kind of patterns would you allow, I am more than happy to solve the issue for you.

@parasharrajat
Copy link
Member

parasharrajat commented Nov 6, 2021

@mallenexpensify I added this as a feature @mountiny do you have any comments? Oops didn't see that we want to add it.

Proposal

Two approaches

1

I allowed the bracket (( )) and - in the login in the following commit. 500ba78.

  1. So we will update the replacement regex here allow spaces as well. So that validation will pass for Logic form.
/^\+?\d*$/.test(input.replace(/((?!\n)[()-\s\t])/g, ''))
  1. We just run this replacement before matching the input in Str.isvalidPhone or before passing to backend.
    if (!Str.isValidEmail(this.state.login) && !Str.isValidPhone(this.state.login)) {
    .
    fetchAccountDetails(this.state.login);

2

OR we can just use the CONST.REGEX.PHONE_WITH_SPECIAL_CHARS regex instead of Str.isValidPhonein Login Page validations which works well. And remove these values before sending them to the backend using the above replacement.

@mountiny
Copy link
Contributor

mountiny commented Nov 6, 2021

@parasharrajat is right, this has been introduced when we wanted to unify the error messages in this PR #5864.

We should be able to just revert the commit Rajat mentioned to allow the (, ) and - characters.

@parasharrajat
Copy link
Member

Sorry, @mountiny for tagging you. I didn't correctly see the issue. We want to add it not remove it so I have updated the proposal. Now we want to extend that change and allow spaces as well.

@akshayasalvi
Copy link
Contributor

Proposal

I had changed the phone number regex in the company VBA flow. I would want to change the validation check to isValidPhoneWithSpecialChars in LoginForm.validateAndSubmitForm

if (!Str.isValidEmail(this.state.login) && !isValidPhoneWithSpecialChars(this.state.login)) { // updated func here
            if (isNumericWithSpecialChars(this.state.login)) {
        

@mallenexpensify
Copy link
Contributor Author

mallenexpensify commented Nov 8, 2021

We are placing all non-critical issues on hold while we're on a company offsite. The hold is expected to be lifted on 11/19 (but could go longer). For any PRs that are submitted before or during the hold, we will add a $250 bonus payment.

@kadiealexander kadiealexander changed the title When creating new accounts, allow users to enter their phone numbers with - and/or ( ) and spaces [$250] When creating new accounts, allow users to enter their phone numbers with - and/or ( ) and spaces Nov 9, 2021
@chiragsalian
Copy link
Contributor

I'm going to go with @parasharrajat because he captured the importance of replacing the character values as well before sending it to the backend. If we send the characters to the backend I'm pretty sure it will error out.

As for your 1 and 2 proposal choices @parasharrajat. I think 1 would be easier and cleaner. I don't mind solution 2 but in my limited testing CONST.REGEX.PHONE_WITH_SPECIAL_CHARS or isValidPhoneWithSpecialChars didn't work as well as I expected. For example, take this number without special characters +15857527441, with special characters it would be +1 (585) 752-7441 and the regex fails and the length checks in isValidPhoneWithSpecialChars also fail for it. Something to keep in mind if you want to pursue solution 2.

Anyway feel free to create a PR @parasharrajat and @mallenexpensify feel free to hire Rajath in Upwork.

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 11, 2021
@mallenexpensify
Copy link
Contributor Author

@parasharrajat , can you apply here please? https://www.upwork.com/jobs/~01e3a9fa28fc98c96a

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels Nov 29, 2021
@botify
Copy link

botify commented Nov 29, 2021

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

If no regressions arise, payment will be issued on 2021-12-06. 🎊

@botify botify changed the title [$250] When creating new accounts, allow users to enter their phone numbers with - and/or ( ) and spaces [HOLD for payment 2021-12-06] [$250] When creating new accounts, allow users to enter their phone numbers with - and/or ( ) and spaces Nov 29, 2021
@parasharrajat
Copy link
Member

Ping for
image
@mallenexpensify

@mallenexpensify
Copy link
Contributor Author

@parasharrajat Can you accept the offer here please and confirm when you have?
https://www.upwork.com/jobs/~01b50ba5b54d346056

@parasharrajat
Copy link
Member

Accepted it @mallenexpensify

@mallenexpensify
Copy link
Contributor Author

Paid @parasharrajat $250 for the job and $250 for company offsite hold bonus.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants