VBA Fields validation #5531

akshayasalvi · 2021-09-25T19:12:38Z

@puneetlath Can you please review this?

Details

Added regex for phone validation
Added maxLength and keyboardType for fields

Fixed Issues

$ #5184

Tests

Tested the phone number with brackets and with long digits
Tested ssn with more than 4 digits
Tested taxIDNumber with more than 9 digits
Tested zipCode with more than 5 digits

QA Steps

There are two forms where the validation is added.

Company Step

Got to bank-account/company-step
Enter invalid values for zipCode, taxIDNumber and phoneNumber
Click on Save & Continue
Validate that fields show the error

Requestor Step

Got to bank-account/personal-information
Enter invalid values for ssn
Click on Save & Continue
Validate that fields show the error

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

akshayasalvi · 2021-09-25T19:58:55Z

@puneetlath We might have to update the placeholder for Phone number field. At the moment it says 10 digits, no hyphens. Can you or @SofiedeVreese help me with the new placeholder that I can update?

Luke9389 · 2021-09-27T22:01:07Z

@akshayasalvi Regarding the phone number placeholder, I think it's fine to just use Phone Number (xxx)xxx-xxxx .

Also, I think you should use Str.isValidPhone from expensify-common in the validate() method on this form.

Luke9389 · 2021-09-28T03:01:30Z

Hey @akshayasalvi, great job so far on this issue. 👍

We are wanting to push through all issues related to this flow very quickly, so it's possible that I'll end up taking this PR and finishing it personally. Rest assured, you will still get paid as if you completed this issue. I'll check back in tomorrow and make a determination then.

akshayasalvi · 2021-09-28T05:01:00Z

@akshayasalvi Regarding the phone number placeholder, I think it's fine to just use Phone Number (xxx)xxx-xxxx .

Same placeholder in Spanish too?

Also, I think you should use Str.isValidPhone from expensify-common in the validate() method on this form.

I had seen that but it seems our current isValidPhone only allows digits and not parenthesis? I had even raised it in the discussion here #5184 (comment)

Hey @akshayasalvi, great job so far on this issue. 👍

Thank you

We are wanting to push through all issues related to this flow very quickly, so it's possible that I'll end up taking this PR and finishing it personally. Rest assured, you will still get paid as if you completed this issue. I'll check back in tomorrow and make a determination then.

Okay both the changes that you've mentioned have follow-up questions. Can you help so that I'll update the PR at the earliest?

Luke9389 · 2021-09-28T23:06:35Z

Same placeholder in Spanish too?

That's a good point. For Spanish, lets do (prefijo) + (número).

Luke9389 · 2021-09-28T23:08:27Z

I had seen that but it seems our current isValidPhone only allows digits and not parenthesis? I had even raised it in the discussion here #5184 (comment)

You also offered taking out the parentheses in that comment, which sounds like a fine solution to me. We don't need to make that a separate method, just do it in one line.

src/CONST.js

src/libs/ValidationUtils.js

Luke9389

I think this is looking really good. Let me know what you think about my concern with de-coupling the max length of phone numbers and their symbols.

…-validation # Conflicts: # src/pages/ReimbursementAccount/CompanyStep.js # src/pages/ReimbursementAccount/IdentityForm.js

akshayasalvi · 2021-09-29T18:09:37Z

I think this is looking really good. Let me know what you think about my concern with de-coupling the max length of phone numbers and their symbols.

@Luke9389 I've updated the regex with length check on the numbers. I've also added an additional max length check in the function itself.

Also, I saw two conflicts where I've picked my changes. I insist on keeping my change keyboardType={CONST.KEYBOARD_TYPE.NUMERIC} whereas the incoming change had it set as KEYBOARD_TYPE.PHONE_PAD with fields like zipCode and ssnLast4 being numbers only.

src/CONST.js

Luke9389

Getting closer, just a few more questions. Thanks!

…-validation # Conflicts: # src/libs/ValidationUtils.js # src/pages/ReimbursementAccount/CompanyStep.js

akshayasalvi · 2021-09-30T03:32:38Z

PR updated

Luke9389

Before merging I'd like to verify that you've tested these changes since the most recent commit. I ask all my contributors this question because it's easy to assume our commits don't break anything, and then they cause regressions. Lets get you paid on time :)

src/CONST.js

akshayasalvi · 2021-10-01T02:56:49Z

Yes tested :)

…-validation # Conflicts: # src/libs/ValidationUtils.js

akshayasalvi · 2021-10-04T16:47:57Z

@Luke9389 Resolved and tested again

Luke9389 · 2021-10-04T17:39:58Z

@akshayasalvi Looks like there's another conflict.

akshayasalvi · 2021-10-04T17:48:00Z

@akshayasalvi Looks like there's another conflict.

Ohh no :(

Luke9389 · 2021-10-04T17:49:06Z

Heh, it's OK! No worries, just be sure you're merging with the most recent version of our main branch.
Usually, I run git fetch to help with this. (Though, if you want to be 200% sure, checkout main and run git fetch && git pull)

…-validation # Conflicts: # src/pages/ReimbursementAccount/IdentityForm.js

akshayasalvi · 2021-10-04T17:54:54Z

Resolved again and tested

Heh, it's OK! No worries, just be sure you're merging with the most recent version of our main branch. Usually, I run git fetch to help with this. (Though, if you want to be 200% sure, checkout main and run git fetch && git pull)

Yeah, I do it every time. It is just other changes are getting merged and resulting in conflicts. I am guessing these screens are under active development.

OSBotify · 2021-10-04T19:58:28Z

✋ 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 · 2021-10-06T18:18:20Z

🚀 Deployed to staging by @Luke9389 in version: 1.1.5-2 🚀

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

isagoico · 2021-10-07T14:41:53Z

This issue is failing this PR #5705

puneetlath · 2021-10-07T15:33:53Z

Hm, that seems like a separate issue rather than a regression caused by this PR. This PR just enforces that what is entered is the right type and length. But doesn't do any validation on whether the numbers entered are valid. So I don't think we need to block this PR based on that.

isagoico · 2021-10-07T15:41:59Z

oh ok then the PR is a pass! Checking it off and removing the deploy blocker label

Luke9389 · 2021-10-07T17:06:19Z

As long as @kavimuru used the correct number of zeros in the phone number. If they were able to pass this step with just one zero for a phone number, then I think that's a problem for this PR.

OSBotify · 2021-10-07T19:34:11Z

🚀 Deployed to production by @chiragsalian in version: 1.1.6-0 🚀

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

marcaaron · 2021-10-15T17:26:07Z

Just noticed the regex initially added here allows people to enter phone numbers that are only 3 digits ?

Seems like something is wrong? Or is that expected..?

akshayasalvi · 2021-10-15T17:33:25Z

Just noticed the regex initially added here allows people to enter phone numbers that are only 3 digits?

Yeah, it allows because our current regex also allows 3 digit numbers. This one adds support for brackets, etc. in US format, yet allows you to add 2-3 digit numbers as per E164

marcaaron · 2021-10-15T21:24:40Z

Yes, that's good. But what 3 digit number are they entering that we should accept as a phone number?

akshayasalvi · 2021-10-16T10:24:36Z

Yes, that's good. But what 3 digit number are they entering that we should accept as a phone number?

Good question. I am not sure then what would be the expected values. Can you help me and accordingly I'll update the regex. This is the current regex: /^\+?[1-9]\d{1,14}$/

Luke9389 · 2021-10-18T18:53:52Z

Yea, we should have enforced a minimum character amount. A quick search turned up with 5 as the absolute minimum number of digits (Solomon Islands). Runner up is Sweden at 7.
https://stackoverflow.com/questions/14894899/what-is-the-minimum-length-of-a-valid-international-phone-number

marcaaron · 2021-10-18T20:50:39Z

Nice. Should we create a new issue? It'd be great to try and avoid any situation where a user is able to do something that we can catch with front end validation instead of in the back end.

Luke9389 · 2021-10-18T21:09:21Z

Yea, I'm fine with just spinning one up real quick and doing it. I don't think it's worth Exporting.

Before I do that though, do you think we should try to factor in location to this check? We could look at the address they've provided (if they've done that field) and/or use an IP goelocation lookup?

As I'm writing this it sounds like overkill, but I'd like to prevent someone from providing a 5 digit phone number (likely as an accident). What do you think?

marcaaron · 2021-10-18T21:26:47Z

Good thoughts, but agree with it being overkill for now. The case I was running into specifically is that I had an area code previously saved auto-filling in this input so adding a minimum like 5 digits here seems like a high value improvement given the complexity added.

Luke9389 · 2021-10-18T21:45:06Z

K, I'll just spin this up and do it. 👍

akshayasalvi added 3 commits September 25, 2021 23:19

Added validation for phone, ssn and taxt_id_number

08de2ed

Added maxlength and keyboard type for zipcode and phone number

26d2e62

Added maxlength to zipcode

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

f0f9680

akshayasalvi requested a review from a team as a code owner September 25, 2021 19:12

MelvinBot requested review from puneetlath and removed request for a team September 25, 2021 19:12

Luke9389 self-assigned this Sep 28, 2021

Luke9389 reviewed Sep 28, 2021

View reviewed changes

src/CONST.js Outdated Show resolved Hide resolved

Luke9389 reviewed Sep 28, 2021

View reviewed changes

src/libs/ValidationUtils.js Outdated Show resolved Hide resolved

Luke9389 reviewed Sep 28, 2021

View reviewed changes

akshayasalvi added 3 commits September 29, 2021 23:22

Added translations for phone number placeholder

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

830b7f9

Updated regex validation for length and added length check in function

9fb2257

Merge branch 'main' of github-personal:akshayasalvi/App into vba-flow…

d1a2119

…-validation # Conflicts: # src/pages/ReimbursementAccount/CompanyStep.js # src/pages/ReimbursementAccount/IdentityForm.js

akshayasalvi requested a review from Luke9389 September 29, 2021 18:12

Luke9389 reviewed Sep 29, 2021

View reviewed changes

src/CONST.js Outdated Show resolved Hide resolved

Luke9389 reviewed Sep 29, 2021

View reviewed changes

src/CONST.js Outdated Show resolved Hide resolved

Luke9389 reviewed Sep 29, 2021

View reviewed changes

akshayasalvi added 2 commits September 30, 2021 08:53

Updated length to 12

6ed6a77

Merge branch 'main' of github-personal:akshayasalvi/App into vba-flow…

7b68fa9

…-validation # Conflicts: # src/libs/ValidationUtils.js # src/pages/ReimbursementAccount/CompanyStep.js

akshayasalvi requested a review from Luke9389 September 30, 2021 03:32

Luke9389 previously approved these changes Sep 30, 2021

View reviewed changes

src/CONST.js Outdated Show resolved Hide resolved

Merge branch 'main' of github-personal:akshayasalvi/App into vba-flow…

870c41c

…-validation # Conflicts: # src/libs/ValidationUtils.js

Merge branch 'main' of github-personal:akshayasalvi/App into vba-flow…

43f209a

…-validation # Conflicts: # src/pages/ReimbursementAccount/IdentityForm.js

Luke9389 approved these changes Oct 4, 2021

View reviewed changes

Luke9389 merged commit 817240d into Expensify:main Oct 4, 2021

OSBotify mentioned this pull request Oct 6, 2021

Deploy Checklist: New Expensify 2021-10-06 #5693

Closed

13 tasks

SofiedeVreese mentioned this pull request Oct 6, 2021

[HOLD for payment 2021-10-14] [HOLD for payment 2021-10-14] Prevent inaccurate information by enforcing a character limit during the VBA flow #5184

Closed

kavimuru mentioned this pull request Oct 6, 2021

[HOLD for payment 2021-11-04] VBA - User can proceed to next steps when entering 0's to the fields without any error message #5705

Closed

parasharrajat mentioned this pull request Oct 18, 2021

[HOLD for payment 2021-11-04] Fake growl about changes being made in profile is displayed when saving a name that's over 50 characters long #5091

Closed

Luke9389 mentioned this pull request Oct 18, 2021

Don't allow phone numbers that are less than 5 digits long #5936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VBA Fields validation #5531

VBA Fields validation #5531

akshayasalvi commented Sep 25, 2021 •

edited

Loading

akshayasalvi commented Sep 25, 2021

Luke9389 commented Sep 27, 2021

Luke9389 commented Sep 28, 2021

akshayasalvi commented Sep 28, 2021

Luke9389 commented Sep 28, 2021

Luke9389 commented Sep 28, 2021

Luke9389 left a comment

akshayasalvi commented Sep 29, 2021

Luke9389 left a comment

akshayasalvi commented Sep 30, 2021

Luke9389 left a comment

akshayasalvi commented Oct 1, 2021

akshayasalvi commented Oct 4, 2021

Luke9389 commented Oct 4, 2021

akshayasalvi commented Oct 4, 2021

Luke9389 commented Oct 4, 2021 •

edited

Loading

akshayasalvi commented Oct 4, 2021

OSBotify commented Oct 4, 2021

OSBotify commented Oct 6, 2021

isagoico commented Oct 7, 2021

puneetlath commented Oct 7, 2021

isagoico commented Oct 7, 2021

Luke9389 commented Oct 7, 2021

OSBotify commented Oct 7, 2021

marcaaron commented Oct 15, 2021

akshayasalvi commented Oct 15, 2021

marcaaron commented Oct 15, 2021

akshayasalvi commented Oct 16, 2021

Luke9389 commented Oct 18, 2021 •

edited

Loading

marcaaron commented Oct 18, 2021

Luke9389 commented Oct 18, 2021

marcaaron commented Oct 18, 2021

Luke9389 commented Oct 18, 2021

VBA Fields validation #5531

VBA Fields validation #5531

Conversation

akshayasalvi commented Sep 25, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

akshayasalvi commented Sep 25, 2021

Luke9389 commented Sep 27, 2021

Luke9389 commented Sep 28, 2021

akshayasalvi commented Sep 28, 2021

Luke9389 commented Sep 28, 2021

Luke9389 commented Sep 28, 2021

Luke9389 left a comment

Choose a reason for hiding this comment

akshayasalvi commented Sep 29, 2021

Luke9389 left a comment

Choose a reason for hiding this comment

akshayasalvi commented Sep 30, 2021

Luke9389 left a comment

Choose a reason for hiding this comment

akshayasalvi commented Oct 1, 2021

akshayasalvi commented Oct 4, 2021

Luke9389 commented Oct 4, 2021

akshayasalvi commented Oct 4, 2021

Luke9389 commented Oct 4, 2021 • edited Loading

akshayasalvi commented Oct 4, 2021

OSBotify commented Oct 4, 2021

OSBotify commented Oct 6, 2021

isagoico commented Oct 7, 2021

puneetlath commented Oct 7, 2021

isagoico commented Oct 7, 2021

Luke9389 commented Oct 7, 2021

OSBotify commented Oct 7, 2021

marcaaron commented Oct 15, 2021

akshayasalvi commented Oct 15, 2021

marcaaron commented Oct 15, 2021

akshayasalvi commented Oct 16, 2021

Luke9389 commented Oct 18, 2021 • edited Loading

marcaaron commented Oct 18, 2021

Luke9389 commented Oct 18, 2021

marcaaron commented Oct 18, 2021

Luke9389 commented Oct 18, 2021

akshayasalvi commented Sep 25, 2021 •

edited

Loading

Luke9389 commented Oct 4, 2021 •

edited

Loading

Luke9389 commented Oct 18, 2021 •

edited

Loading