[$1000] Phone number and company URL field isn’t highlighted when it fails validation upon form submission.

[kavimuru]

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:

1. Go to Settings -> workspaces -> bank account
2. Fill in all required info and go to the company info page
3. In the Company website, in field phone number type “123435345/5/” or 1234353455?” or “?_@1234353455” .
   Notice that upon form submission, the field number isn’t red highlighted.
   Similar case occurs on company url too . Try “https://example.com:10000000”

Expected Result:

Form field with errors should be highlighted in red even if the error comes form backend.

Actual Result:

Errors got on form submission aren’t highlighted in red.

Workaround:

unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.92-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-03-30.at.5.55.56.PM.mov

Recording.104.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ashimsharma10
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680178415219629

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0170d7d8b2289c9967
• Upwork Job ID: 1643651811770814464
• Last Price Increase: 2023-05-26

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

@greg-schroeder Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[MelvinBot]

@greg-schroeder Huh... This is 4 days overdue. Who can take care of this?

[greg-schroeder]

Reviewing now, sorry for the delay

[greg-schroeder]

I don't think this is a duplicate, and I was able to reproduce on web.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0170d7d8b2289c9967

[MelvinBot]

Triggered auto assignment to @Christinadobrzyn (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[MelvinBot]

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

[MelvinBot]

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

[jayeshmangwani]

Proposal

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

when adding the wrong phone number or company URL then border is not highlighted red color for these 2 fields

What is the root cause of that problem?

1. For the Phone number field
   we are getting the wrong result in isValidUSPhone function as we are passing the 123435345/5/ and it is returning true for this number that's wrong because this number should not be sent from the frontend, we should change the frontend regex same as the backend (we will need backend regex for checking here or we can change the current regex where we should not remove the all alphanumeric values when comparing against the CONST.REGEX.PHONE_E164_PLUS regex)
   

   App/src/libs/ValidationUtils.js

   Lines 287 to 294 in ef6977e

   	function isValidUSPhone(phoneNumber = '', isCountryCodeOptional) {
   	// Remove non alphanumeric characters from the phone number
   	const sanitizedPhone = (phoneNumber || '').replace(CONST.REGEX.NON_ALPHA_NUMERIC, '');
   	const isUsPhone = isCountryCodeOptional
   	? CONST.REGEX.US_PHONE_WITH_OPTIONAL_COUNTRY_CODE.test(sanitizedPhone) : CONST.REGEX.US_PHONE.test(sanitizedPhone);
   	return CONST.REGEX.PHONE_E164_PLUS.test(sanitizedPhone) && isUsPhone;
   	}

   
   here in the above isValidUSPhone function, we are passing the sanitizedPhone to regex for testing but we should check the original phoneNumber in CONST.REGEX.PHONE_E164_PLUS.test(sanitizedPhone) or simply we should use the backend regex here

2. for the website field we need to make changes to the backend as more than 6 characters after the colon are showing the wrong website from the backend one more thing is that when I am testing I Found that Https://google.com is also not working so the backend regex should be case insensitive
   For e.g. https://google.com:12 is working but https://google.com:12343321

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

1. For the phone number field we should use the same regex as the backend as the current isValidUSPhone function is passing an incorrect phone number to the backend

App/src/libs/ValidationUtils.js

Line 289 in ef6977e

const sanitizedPhone = (phoneNumber || '').replace(CONST.REGEX.NON_ALPHA_NUMERIC, '');

here for sanitizedPhone we are replacing all the non alpha numeric values with empty string, but from testing it looks like the backend only replace space, new line, (,), and - , so we should use this /[\s()\-]+/g regex instead of CONST.REGEX.NON_ALPHA_NUMERIC regex

2. For the website invalid message issue we should update the backend regex that accepts the links like this https://google.com:12343321 more than 6 characters after the colon

What alternative solutions did you explore? (Optional)

none

[allroundexperts]

I think we should handle this with #16874. The two are very similar.

[greg-schroeder]

Hey @allroundexperts - good catch. Going to ask if we should combine or handle separately. I'll remove Help Wanted for now until we decide.

[pecanoro]

@greg-schroeder Should we group the issues then? I see the discussion in the other issue kept going 😄

[jayeshmangwani]

@allroundexperts Not any proposals accepted for the mentioned issue, @rushatgabhane we should Hold issue #17205

I think there are more proposals listed there and one of them is very close to being accepted.

@rushatgabhane can you please look into this if we should HOLD the current issue for the mentioned issue?

[huzaifa-99]

Proposal

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

Form fields for company information page are not highlighted with red when there is an error from api after form submission

What is the root cause of that problem?

We are addressing 2 problems here

1. For phone number field, when doing frontend validation we are sanitizing the value here but we are not sending the sanitized value to the api, so the api returns an error.
2. For company url field, the regex we use to match for valid url does not ATM check for the port number range 0-65535.

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

1. For phone number field, we need to send the sanitized value to the api. The phone number is already sanitized by this function but it does not remove non alpha numeric chars. We need to update this function to also sanitize the value for non-alphanumeric chars, like this

function getPhoneNumberWithoutUSCountryCodeAndSpecialChars(phone) {
    const phoneWithoutSpecialChars = (phone || '').replace(CONST.REGEX.NON_ALPHA_NUMERIC, '');
    return getPhoneNumberWithoutSpecialChars(phoneWithoutSpecialChars.replace(/^\+1/, ''));
}

2. I agree with @jayeshmangwani to update the company url regex. But the suggested regex will also allow port 0, for example https://example.com:0 will pass the validation. I think we should not allow port 0 as it is a wildcard port (and in general networking terms when port 0 is used the system will find an available port to use instead of it). We should use this updated regex instead that does not allow port 0

/^((https?|ftp):\/\/)(([a-z\d]([a-z\d-]*[a-z\d])*)\.)+[a-z]{2,}(:(([1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5]))?)?(\/[-a-z\d%_.~+]*)*(\?[;&a-z\d%_.~+=-]*)?(#[-a-z\d_]*)?$/i

I would also suggest to update the backend to not use/allow port 0 with urls

What alternative solutions did you explore? (Optional)

N/A

[rushatgabhane]

@pecanoro I think we should close/hold this issue.

#17205 is fixing the URL errors, and #17461 can take care of phone number validations.

[huzaifa-99]

@rushatgabhane I tried the accepted proposal fix for #17205, it doesn't allow port numbers in url, maybe that's the desired behaviour at the end?

#17461 adds validations to phone numbers but the validations are already passing for phone numbers in this issue. We are just not sending the sanitized values to the api which is causing the issue.

[allroundexperts]

@rushatgabhane I tried the accepted proposal fix for #17205, it doesn't allow port numbers in url, maybe that's the desired behaviour at the end?

#17461 adds validations to phone numbers but the validations are already passing for phone numbers in this issue. We are just not sending the sanitized values to the api which is causing the issue.

@huzaifa-99 It does allow port numbers that are valid ie 80, 8080, 9000 etc

[huzaifa-99]

@allroundexperts Hm, interesting. I did another test and it seems like it will pass any port that is between 2 and 4 digits. Notice the d{2,4} in the regex here

But it fails with a 1 digit or 5 digit port

1. https://example.com:5 - any 1 digit port
2. https://example.com:65535 any 5 digits port

It will pass this case but notice the leading 0
3. https://example.com:0111

It also does not check for valid port range 0-65535.

[allroundexperts]

@allroundexperts Hm, interesting. I did another test and it seems like it will pass any port that is between 2 and 4 digits. Notice the d{2,4} in the regex here

But it fails with a 1 digit or 5 digit port

1. https://example.com:5 - any 1 digit port
2. https://example.com:65535 any 5 digits port

It will pass this case but notice the leading 0 3. https://example.com:0111

It also does not check for valid port range 0-65535.

Thanks for your insights @huzaifa-99. I think it might be better to create a PR that adds these missing port number range to the expensify-common URL validator instead. I think my PR will require some changes to the library itself (read more here). I'll update the port range there as well.

[MelvinBot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[pecanoro]

Let's close this issue then if it will get fixed in the other one. Also, I saw the person reporting is the same for both and the errors are so similar that I think it's ok to pay just the other reporting bonus.

[kbecciv]

Reopening this issue as it's happening. Contributor reported in the #expensify-bugs channel here https://expensify.slack.com/archives/C049HHMV9SM/p1684745553939399 and I'm still able to reproduce it.

Recording.2828.mp4

[greg-schroeder]

Looking

[greg-schroeder]

@rushatgabhane @pecanoro Hmm - both #17461 and #17205 are complete, but this appears to still be a thing. I'm curious what went wrong as far as those not fixing this incidentally

[rushatgabhane]

cc @thesahindia @s77rt any idea what went wrong in the PR for the above issues?

They should have fixed this issue. (im out rn, so can't investigate)

[pecanoro]

Hmm, I wonder if some PR brought this back as a regression.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[s77rt]

I don't think the same issue is reproducible here. We are looking at a similar yet different issue. The first one was about getting a RBR error from backend even though the number is said to be correct. This behaviour is fixed, now as long as you don't see an error message in the phone number then it will work.

The reason some numbers that contain letters work as seen in the above video is due to how to the awesome-phonenumber lib validates them, e.g. if you type +12313hi441232 this will be treated as +12313441232 this is probably done to support phone-numbers that contain letters e.g. +1800AUPAIRS will be treated as +18002872477. Check demo.

[melvin-bot]

@pecanoro, @greg-schroeder, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[pecanoro]

@s77rt I saw the video again and you are right. It seems even the back-end takes the number as valid or at least I don't see any errors being thrown. If that's the case, we can just close this issue again.

[greg-schroeder]

All right - I will re-close per above.

[s77rt]

@pecanoro

It seems even the back-end takes the number

We always send the backend the number in e164 format. This will always work.

[pecanoro]

Got it, so this is working as expected then.