Skip to content
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

Update email regex #562

Merged
merged 3 commits into from
Aug 1, 2023
Merged

Update email regex #562

merged 3 commits into from
Aug 1, 2023

Conversation

cristipaval
Copy link
Contributor

@cristipaval cristipaval commented Jul 31, 2023

We don't want the front end to pass emails that are not valid in the backend. We use FILTER_VALIDATE_EMAIL from php which validates the emails if they match RFC-822 standards. According to those standards, the username part of the email can't be longer than 64 characters. This PR adds this condition in our regex for the email validation in JS.

See here a discussion about the email validation inconsistency throughout the project.

Fixed Issues

Part of Expensify/App#23180

Tests

  1. In NewDot, go to Profile -> Contact Method -> New Contact Method
  2. In the email input field add an email with a long username, like usernamelongerthan64charactersshouldnotworkaccordingtorfc822whichisusedbyphp@gmail.com
  3. Click on Add button
  4. Verify that you get an Invalid contact method error

QA

We'll be QA'd with the App PR

@cristipaval cristipaval requested a review from a team as a code owner July 31, 2023 20:19
@cristipaval cristipaval self-assigned this Jul 31, 2023
@melvin-bot melvin-bot bot requested review from deetergp and removed request for a team July 31, 2023 20:19
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. I had one minor NAB comment.

it('Correctly detects a valid email', () => {
expect(Str.isValidEmail('[email protected]')).toBeTruthy();
expect(Str.isValidEmail('test@gmail')).toBeFalsy();
expect(Str.isValidEmail('usernamelongerthan64charactersshouldnotworkaccordingtorfc822whichisusedbyphp@gmail.com')).toBeFalsy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB: Should we add a test for usernames shorter than 1 character?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, it makes sense to add that test for completeness. Thanks 🙇
Follow-up PR here

@deetergp deetergp merged commit 56db2a0 into main Aug 1, 2023
3 checks passed
@deetergp deetergp deleted the cristi_update-email-regex branch August 1, 2023 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants