-
Notifications
You must be signed in to change notification settings - Fork 131
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
Validate phone numbers in different formats #672
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@Pujan92 Yes, I just added it. |
isValidPhone
to isValidE164Phone
Thanks! |
This looks good, but may silently break OldDot, so I am waiting a little to review and merge |
@tienifr Looks good to me, better if you add some tests here for expensify-common/__tests__/Str-test.js Lines 110 to 111 in 1247a82
Also some space lint fixes which look straightforward to me, from this PR only 1 error occurred but better if we fix all(5) space errors here. |
So, I was thinking about this, and how we can merge this without breaking other parts of the application that depend on it. What if we:
|
@Pujan92 What do you think? I think that's not neccessary because we already investigate all usages of the deprecated |
I added unit test. |
This is used in Web-Expensify and Web-Secure. My point is, if we merge this and someone updates the expensify-common lib in one of the repos before we merge the renaming, we will break this validation. |
@Pujan92 I marked |
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.
Thanks!
Current
isValidPhone
function only validates E.164 standard phone numbers but not others causing misunderstandings and worng usages. This PR:isValidPhone
as deprecatedisValidPhone
withisValidE164Phone
isValidPhoneFormat
to validate phone numbers in different formatsFixed Issues
$ Expensify/App#37723
Tests
1. What unit/integration tests cover your change? What autoQA tests cover your change?
This is a renaming refactor so no need to add new unit tests.
2. What tests did you perform that validates your changed worked?
(xxx) xxx-xxxx
, e.g., "(440) 458-9784"QA
1. What does QA need to do to validate your changes?
Test in Expensify App follow these steps:
(xxx) xxx-xxxx
, e.g., "(440) 458-9784"2. What areas to they need to test for regressions?
NA
Screenshots
Screen.Recording.2024-03-26.at.18.32.42-compressed.mov