-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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: extend phone number validation to handle additional '+' characters #24133
Conversation
Reviewer Checklist
Screenshots/VideosWeb24133.Web.mp4Mobile Web - Chrome24133.mWeb-Chrome.mp4Mobile Web - Safari24133.mWeb-Safari.mp4Desktop24133.Desktop.mp4iOS24133.iOS.mp4AndroidScreen_Recording_20230806_221428_New.Expensify.1.mp4 |
src/pages/signin/LoginForm.js
Outdated
@@ -125,6 +125,12 @@ function LoginForm(props) { | |||
return; | |||
} | |||
|
|||
// Every input that starts with '+' should be validated as a phone number. | |||
if (loginTrim.startsWith('+') && !ValidationUtils.isNumericWithSpecialChars(loginTrim)) { |
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.
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.
@mollfpr This can be considered a valid email address, yes, but this is a very rare edge case that is unlikely to happen. Also, based on #23803 (comment), the input starting with a "+" should have the phone number error displayed (regardless of the characters that come afterwards).
It was also mentioned in previous comments in the issue thread that as long as the input starts with a "+" sign, it will be considered a phone number. See #23803 (comment) for instance.
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.
There is a slim chance that someone might interpret it as a phone number due to the presence of numbers and a plus sign. It does not adhere to a typical phone number format. It is more likely to be an incorrect or invalid email-like string.
I'm afraid that someone will think differently after this PR is merged.
If the value is not a standard email address that should have the "@" symbol, separating the local part (before "@"), and the domain part (after "@"), with plus sign leading, then we can consider this an invalid phone number. E.g. +3232asdas323232
.
I initially thought we only caught the invalid phone number for the numeric value.
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 clarify the change I should make here? Here's what I was able to understand:
- If the input contains an '@' even if it starts with '+', then it should be treated as an email, and show the corresponding error message for invalid email address. I can add another conditional check for this.
- In every other case where it doesn't look like an email, for example:
+aa228833aa.cc
it will be considered as an invalid phone number, and the phone number invalid message will be shown.
Are these assumptions correct?
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 think the second case is more appropriate with an email address error because it has a word and .
for the domain part of an email address.
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 think the second case is more appropriate with an email address error because it has a word and . for the domain part of an email address.
I don't think it really matters much, because an email address must always contain an '@' sign.
However, in this case, it looks like the solution would be to only consider inputs that contain digits (numeric 0-9) and the special symbols (+
, (
, (
) and -
) as a phone number (show phone number error), and then all other cases as an email address (show email address error), in which case, the only thing that needs to be updated is the regex, and I can remove this check: if (loginTrim.startsWith('+') && !ValidationUtils.isNumericWithSpecialChars(loginTrim)) {
.
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.
Yeah, the updated regex seems enough.
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.
Pushed a new commit. All tests run ok as expected.
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 and tests well 👍
✋ 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/thienlnam in version: 1.3.51-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.51-2 🚀
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.52-0 🚀
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.52-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
Details
With the regular expression updated to handle additional '+' signs in the login input, this validates these values as phone numbers and displays the corresponding error message.
Fixed Issues
$ #23803
PROPOSAL: $ #23803 (comment)
Tests
Offline tests
N/A
QA Steps
Same as tests.
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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)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
Web
23803-web.mp4
Mobile Web - Chrome
23803-android-chrome.webm
Mobile Web - Safari
23803-ios-safari.mp4
Desktop
23803-desktop.mp4
iOS
23803-ios-native.mp4
Android
23803-android-native.webm