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

Chore: Centralize email validation functionality #23816

Merged
merged 3 commits into from
Dec 20, 2021

Conversation

KevLehman
Copy link
Contributor

Proposed changes (including videos or screenshots)

  • Create lib for validating emails
  • Modify places that validate emails to use the new central function

Issue(s)

Steps to test or reproduce

Further comments

Comment on lines 6 to 11
case 'basic':
return basicEmailRegex.test(email);
case 'rfc':
return rfcEmailRegex.test(email);
default:
return basicEmailRegex.test(email);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case 'basic':
return basicEmailRegex.test(email);
case 'rfc':
return rfcEmailRegex.test(email);
default:
return basicEmailRegex.test(email);
case 'rfc':
return rfcEmailRegex.test(email);
case 'basic':
default:
return basicEmailRegex.test(email);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we had the no-fallthrough rule enabled 🤔

@@ -0,0 +1,13 @@
export const validateEmail = (email: string, options: { style: string } = { style: 'basic' }): boolean => {
const basicEmailRegex = /^.+@.+$/;
Copy link
Member

Choose a reason for hiding this comment

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

looking into some regex you modified to use this function I found this one interesting: [^@].*@[^@], it makes sure only a single @ is allowed.. wdyt? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, let me do the change 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Not sure 🙈 if we should use it tho

Copy link
Member

Choose a reason for hiding this comment

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

oopsie.. looks like I missread the regex 🙈

Copy link
Member

Choose a reason for hiding this comment

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

ok, this one 😬 ^[^@]+@[^@]+$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works way better 👀
image

@sampaiodiego
Copy link
Member

Although I have approved it already, what about writing some unit tests for it? maybe we can have an agreement that all functions in /lib should have unit tests.. wdyt?

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2021

CLA assistant check
All committers have signed the CLA.

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.

4 participants