-
-
Notifications
You must be signed in to change notification settings - Fork 199
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: #3271 email validation #3273
base: beta
Are you sure you want to change the base?
fix: #3271 email validation #3273
Conversation
✅ Deploy Preview for oss-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
PR Compliance Checks Passed!
✅ Deploy Preview for design-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
lib/utils/validate-email.ts
Outdated
); | ||
}; | ||
const emailRegexp = | ||
/^(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-z\-0-9]+\.)+[a-z]{2,}))$/i; |
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.
Which regex is this? On the backend API, we use the HTML 5 email validator from the web standard spec:
It looks like this drifts abit. And I'd prefer if we aligned these.
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.
Hi, this is the same one as before, I just extracted it to prevent it from being rebuilt . Will update it to the one from the backend.
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.
Perfect, sounds great 👍
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.
Found the failure issue as well, the new regexp allows top level domains for emails, so adjusted the test to match that
Switch failure to success expectation due to changed regexp
Description
This PR fixes the email validation
Related Tickets & Documents
Fixes #3271
Mobile & Desktop Screenshots/Recordings
N/A
Steps to QA
This cleans up some code and adds regression tests, so running
npm test
is sufficient.Tier (staff will fill in)
[optional] Are there any post-deployment tasks we need to perform?
None I know of.
[optional] What gif best describes this PR or how it makes you feel?