[HOLD for payment 2022-04-06] [$500] Split Money - Able to proceed with split request using first 3 digits of phone number

[kbecciv]

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 https://staging.new.expensify.com
2. Log in with any account
3. Tap on Fub menu and tap Split bill
4. Add one user
5. Tap on Search box and start typing the numbers
6. Delete some numbers and leave it only 3 digits
7. Click Split bill

Error message:

Unexpected error, please try it again

Expected Result:

Unable to proceed with split request using first 3 digits of phone number

Actual Result:

Able to proceed with split request using first 3 digits

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.38.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5447716_error.mp4

Recording.204.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @deetergp (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

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

[mdneyazahmad]

Proposal

Invalidate the phone number, if the number of digit is not atleast 4 digit.

App/src/libs/OptionsListUtils.js

Line 550 in d05fce8

&& ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) || Str.isValidPhone(searchValue))

-        && ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) || Str.isValidPhone(searchValue))

+        && ((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)) || (searchValue.length > 3 && Str.isValidPhone(searchValue)))

[MelvinBot]

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

[MelvinBot]

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

[stephanieelliott]

Posted to Upwork: https://www.upwork.com/jobs/~01531087688241047c

[iwiznia]

@mdneyazahmad 👎 on your proposal. Either the SMS regex is wrong and we should update it OR there's really some valid cases of 3 digits phone numbers.

[parasharrajat]

I think 911 🤖

[stephanieelliott]

Google tells me that the minimum number of digits for a phone number is 4. 911, 311, 411, etc. are actually considered "special dialing codes", they are not actually phone numbers so I am not so sure there is a valid use case for anything less than 4.

I guess this points to the SMS regex being wrong, @iwiznia do we need to update this issue to clarify that?

[iwiznia]

Sure why not.

[nbhargava]

Proposal

Update the E164 regex constant in https://github.com/Expensify/expensify-common/blob/main/lib/CONST.jsx so that only minimum 4-digit numbers are accepted by calls to Str.isValidPhone.

        // Regex that matches on a E.164 phone number starting with a '+'
-        E164_REGEX: /^\+?[1-9]\d{1,14}$/,
+        E164_REGEX: /^\+?[1-9]\d{3,14}$/,

[rushatgabhane]

Hmm I'm not sure about this one. What's our goal here?

The frontend will allow a split request for +11234, but the server will return error.
So changing the regex to allow 4 digits and above doesn't really add much value.

@iwiznia Do we wanna
A) Only allow proceeding with numbers that are valid. (requires some lib like google/libphonenumber (550kB) or this (150kB))
B) Or, let the server handle validation of phone number like it is now, and do nothing here.

[nbhargava]

Also happy to add in libphonenumber (or something equivalent) if that's what folks feel is the right move here!

[MelvinBot]

@iwiznia, @rushatgabhane, @jliexpensify Huh... This is 4 days overdue. Who can take care of this?

[iwiznia]

Were you not hired in upwork already? I asked @jliexpensify to do it and he reacted with 👍

[jliexpensify]

Hmm weird: I swear I hired @Santhosh-Sellavel the other day (I've noticed that Upworks sometimes crashes on me and I have to re-do everything...perhaps that's what happened?)

Anyway, you're officially hired now, sorry about that!

[MelvinBot]

📣 @Santhosh-Sellavel You have been assigned to this job by @jliexpensify!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[jliexpensify]

Adding @kadiealexander to help keep an eye on my GH's as I'll be OOO next week

[kadiealexander]

@Santhosh-Sellavel, could you please provide an update on this issue?

[Santhosh-Sellavel]

@kadiealexander #7688 (comment)

PR link-> #8019

[iwiznia]

Not overdue, PR was merged yesterday

[botify]

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

• IOU Invalid Phone number error handled #8019

If no regressions arise, payment will be issued on 2022-04-06. 🎊

[kadiealexander]

@jliexpensify leaving you assigned, please make payment on this one on the 6th/7th, as I'll be away. Thanks! 🎉

[jliexpensify]

Looks like there's a regression for iOS - is this fine to pay? cc @iwiznia @rushatgabhane

[rushatgabhane]

@jliexpensify there wasn't any regression here.

I commented in the deploy blocker that the culprit PR is #8087.

And not #8019. I hope this clears it up!

[jliexpensify]

Paid @rushatgabhane and @Santhosh-Sellavel ! Closed.