-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: treat certain US numbers as invalid #31726
fix: treat certain US numbers as invalid #31726
Conversation
…cial case where a phone is valid but we want to make it invalid
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@@ -0,0 +1,18 @@ | |||
import {parsePhoneNumber} from '@libs/PhoneNumber'; | |||
|
|||
describe('PhoneNumber', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some more tests with non-US phone numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aimane-chnaif added a few more test numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some more test cases with blank space in various places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I’ll be spotty for the next 24 hours due to long haul traveling, but I’ll add it as soon as I can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aimane-chnaif added a few extra numbers with spaces between every number including country selector. Let me know if this is good enough.
} | ||
|
||
// eslint-disable-next-line import/prefer-default-export | ||
export {parsePhoneNumber}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't very happy with the export {*} from 'awesome-phone-number'
part. I I think it was unnecessary and added complexity to it. I'm now only exporting parsePhoneNumber
and I updated the lint rule to only restrict this function from awesome-phone-number
(we are not using anything else throughout the entire codebase anyways).
Please fix conflict |
Conflict fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 bugs in this video:
- error text glitch
- no +11 validation when there are spaces between letters
bug.mov
return parsedPhoneNumber; | ||
} | ||
|
||
const phoneNumberWithoutSpecialChars = phoneNumber.replace(CONST.REGEX.SPECIAL_CHARS_WITHOUT_NEWLINE, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for further reviewers: not using getPhoneNumberWithoutSpecialChars
from LoginUtils
because of dependency cycle
I believe the glitch is unrelated to this fix as it’s also happening on staging. I’ll try to find the cause and possibly fix it. I’ll look into the validation issue and report back. |
this is also not related to this fix. You can see this happening on staging with the following examples: +15555555555 (try adding a space after the +) this is because of the following line: App/src/libs/OptionsListUtils.js Line 1631 in 0a9f7d4
When a telephone is not valid (possible is false) it will check if the search string is only numbers and +, and if so then it will display the error message related to phone numbers. Because space (and all other non numeric chars) is not part of it, it will think the search string is not a phone number. We could add spaces, () and - to the list as well to address that (might not be as simple because it would then show the phone error if you type something like What are your thoughts? Should we try and address both issues you pointed out as part of this PR even though they are not really related? I’m fine either way, just want to make sure to follow the correct procedure here. |
I did a little more digging and found out what's the reason behind the error message glitch. App/src/components/OptionsList/BaseOptionsList.js Lines 261 to 267 in 573c5e8
As you can see above, this same issue have been dealt with in the past and the code above was added to solve it. Basically it will not display any error message while data is still being loaded, which makes total sense. But it doesn't work quite as expected, see below: Lines 86 to 89 in 573c5e8
Line 209 in 573c5e8
Lines 237 to 240 in 573c5e8
The code above is from the To solve this we'll need to move the https://github.com/Expensify/App/blob/573c5e812c5def1a4b18150ebca9e505766fe4b4/src/pages/SearchPage.js All these components suffer the same issue. IMO we should raise a new issue to handle this in order to get my eyes on it as the solution might not be as easy. I'd say ideally we would want to extract the search logic on a shared component used by all these pages so we don't have repeated logic. |
As far as the above, we can replace - DIGITS_AND_PLUS: /^\+?[0-9]*$/,
+ PHONE_NUMBER_CHARS: /^\+?(?=.*[0-9])([0-9() -]+)$/, Basically it will expand the list of characters to include I haven't made the change yet, just wanted to hear your thoughts first. |
For me, the glitch doesn't happen on main. Just before loading and while loading, not found message should not show. That regex change is fine to me. So the target we're trying to achieve is that: |
See screen recording. This is production app installed on my phone. It’s not as apparent because it’s a production build and things are a lot faster, but it’s still there. RPReplay_Final1701876362.mp4 |
ok thanks for confirmation. Agree out of scope then |
Let's just stick to this. If something is buggy, should happen on both this branch and main |
Just to confirm, do we want to update the regex to show invalid phone number error when there are spaces (parenthesis and dashes as well) on the search item, or leave it as is (where it won’t if there are spaces)? |
On main, +1 2 5 00 will say “no results” while +12500 will show the invalid phone number error. |
Good catch. I think current branch is in good shape then. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemchrome.moviOS: Nativeios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
@barros001 there's conflict |
@aimane-chnaif conflict resolved. We should try and merge this PR quickly because conflicts tend to happen the longer we wait due to the nature of the changes. |
@madmax330 kindly bump ^ |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/madmax330 in version: 1.4.14-0 🚀
|
🚀 Deployed to staging by https://github.com/madmax330 in version: 1.4.14-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.14-6 🚀
|
@barros001 @aimane-chnaif This was reverted as the additional phone check on the Search page has been causing the app to hang/ crash for Account Manager users who have thousands of people with sms number in their contacts With the next attempt, lets make an Adhoc build and ask internal users like @conorpendergrast or @muttmuure to test the build search page performance |
@mountiny I thought of this scenario when working on the PR and even tried to make it as efficient as possible. I guess it wasn't enough, I'll look again into it. |
No problem, this was super hard to catch because only some of the largest heavy users internally had this issue |
I'll run some performance tests against it. I think if I check for the length of the string and only comparing the first 3 digits (+11) instead of doing a regex test could make a difference here. |
@mountiny are we sure this PR caused the problem? I just run a quick benchmark test calling I tried to following: if (phoneNumberWithoutSpecialChars.length !== 13 || !phoneNumberWithoutSpecialChars.startsWith('+11') || !/^\+11[0-9]{10}$/.test(phoneNumberWithoutSpecialChars)) {
return parsedPhoneNumber;
} To delay calling While I agree that if this PR is causing issues it should be reverted, to me the root cause here is the search page being inefficient. It calls |
@barros001 we haven't confirmed 100%. But the issue seemed to have gone after reverting 2 PRs at the same time. |
@barros001 I must admit that there was not a clear reasoning due to the fact this was hard to experience locally, we ahve reverted two PRs, this one and #31010 so it could be that main problem was actually coming from the other PR. Nevertheless, you are right the search page needs optimizing and there is couple PRs already in works as well as Reassure tests so hopefully we will get into a better place in a week or so |
@aimane-chnaif @mountiny I opened a new PR here for us to verify if this is causing the performance bottleneck. How do we go about making an Adhoc build? |
Details
We have a special case of US phone numbers that are technically valid but we want to treat them as invalid.
Fixed Issues
$ #28492
PROPOSAL: #28492 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android_native.webm
Android: mWeb Chrome
android_web.webm
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-27.at.12.14.10.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-27.at.12.14.50.mp4
MacOS: Chrome / Safari
Screen.Recording.2023-11-27.at.12.05.17.PM.mov
MacOS: Desktop
Screen.Recording.2023-11-27.at.12.07.14.PM.mov