-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix: treat certain US numbers as invalid #31726
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
Changes from all commits
078bc2d
cfc9701
382aef3
e4ca376
5fca4aa
88e39f6
558daab
b1fca58
bacba18
c89f1ef
1394856
5fb03d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // eslint-disable-next-line no-restricted-imports | ||
| import {parsePhoneNumber as originalParsePhoneNumber, ParsedPhoneNumber, ParsedPhoneNumberInvalid, PhoneNumberParseOptions} from 'awesome-phonenumber'; | ||
| import CONST from '@src/CONST'; | ||
|
|
||
| /** | ||
| * Wraps awesome-phonenumber's parsePhoneNumber function to handle the case where we want to treat | ||
| * a US phone number that's technically valid as invalid. eg: +115005550009. | ||
| * See https://github.com/Expensify/App/issues/28492 | ||
| */ | ||
| function parsePhoneNumber(phoneNumber: string, options?: PhoneNumberParseOptions): ParsedPhoneNumber { | ||
| const parsedPhoneNumber = originalParsePhoneNumber(phoneNumber, options); | ||
| if (!parsedPhoneNumber.possible) { | ||
| return parsedPhoneNumber; | ||
| } | ||
|
|
||
| const phoneNumberWithoutSpecialChars = phoneNumber.replace(CONST.REGEX.SPECIAL_CHARS_WITHOUT_NEWLINE, ''); | ||
| if (!/^\+11[0-9]{10}$/.test(phoneNumberWithoutSpecialChars)) { | ||
| return parsedPhoneNumber; | ||
| } | ||
|
|
||
| const countryCode = phoneNumberWithoutSpecialChars.substring(0, 2); | ||
| const phoneNumberWithoutCountryCode = phoneNumberWithoutSpecialChars.substring(2); | ||
|
|
||
| return { | ||
| ...parsedPhoneNumber, | ||
| valid: false, | ||
| possible: false, | ||
| number: { | ||
| ...parsedPhoneNumber.number, | ||
|
|
||
| // mimic the behavior of awesome-phonenumber | ||
| e164: phoneNumberWithoutSpecialChars, | ||
| international: `${countryCode} ${phoneNumberWithoutCountryCode}`, | ||
| national: phoneNumberWithoutCountryCode, | ||
| rfc3966: `tel:${countryCode}-${phoneNumberWithoutCountryCode}`, | ||
| significant: phoneNumberWithoutCountryCode, | ||
| }, | ||
| } as ParsedPhoneNumberInvalid; | ||
| } | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export | ||
| export {parsePhoneNumber}; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't very happy with the |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import {parsePhoneNumber} from '@libs/PhoneNumber'; | ||
|
|
||
| describe('PhoneNumber', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add some more tests with non-US phone numbers?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aimane-chnaif added a few more test numbers
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| describe('parsePhoneNumber', () => { | ||
| it('Should return valid phone number', () => { | ||
| const validNumbers = [ | ||
| '+1 (234) 567-8901', | ||
| '+12345678901', | ||
| '+54 11 8765-4321', | ||
| '+49 30 123456', | ||
| '+44 20 8759 9036', | ||
| '+34 606 49 95 99', | ||
| ' + 1 2 3 4 5 6 7 8 9 0 1', | ||
| '+ 4 4 2 0 8 7 5 9 9 0 3 6', | ||
| '+1 ( 2 3 4 ) 5 6 7 - 8 9 0 1', | ||
| ]; | ||
|
|
||
| validNumbers.forEach((givenPhone) => { | ||
| const parsedPhone = parsePhoneNumber(givenPhone); | ||
| expect(parsedPhone.valid).toBe(true); | ||
| expect(parsedPhone.possible).toBe(true); | ||
| }); | ||
| }); | ||
| it('Should return invalid phone number if US number has extra 1 after country code', () => { | ||
| const validNumbers = ['+1 1 (234) 567-8901', '+112345678901', '+115550123355', '+ 1 1 5 5 5 0 1 2 3 3 5 5']; | ||
|
|
||
| validNumbers.forEach((givenPhone) => { | ||
| const parsedPhone = parsePhoneNumber(givenPhone); | ||
| expect(parsedPhone.valid).toBe(false); | ||
| expect(parsedPhone.possible).toBe(false); | ||
| }); | ||
| }); | ||
| it('Should return invalid phone number', () => { | ||
| const invalidNumbers = ['+165025300001', 'John Doe', '123', 'email@domain.com']; | ||
|
|
||
| invalidNumbers.forEach((givenPhone) => { | ||
| const parsedPhone = parsePhoneNumber(givenPhone); | ||
| expect(parsedPhone.valid).toBe(false); | ||
| expect(parsedPhone.possible).toBe(false); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
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
getPhoneNumberWithoutSpecialCharsfromLoginUtilsbecause of dependency cycle