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.

Sign up for GitHub

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

Jump to bottom

fix(isEmail): replace all dots in gmail length validation #1718

Merged
merged 1 commit into from
Sep 26, 2021
Merged

fix(isEmail): replace all dots in gmail length validation #1718

merged 1 commit into from
Sep 26, 2021

Conversation

DasDingGehtNicht
Copy link
Contributor

@DasDingGehtNicht DasDingGehtNicht commented Aug 30, 2021 •
edited
Loading

The gmail domain specific validator checks the length of the username part and replaces dots as they don't count against the limit. But the replace function only replaced the first dot. Fixed by using regex with g switch.

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Aug 30, 2021 •
edited
Loading

Codecov Report

Merging #1718 (89f02c2) into master (8c4b3b3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1718   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          101       101           
  Lines         2005      2005           
  Branches       452       452           
=========================================
  Hits          2005      2005
Impacted Files Coverage Δ
src/lib/isEmail.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c4b3b3...89f02c2. Read the comment docs.

fedeci
fedeci reviewed Aug 30, 2021
View reviewed changes
Copy link
Contributor

@fedeci fedeci left a comment

Choose a reason for hiding this comment

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

Could you add test cases?

@DasDingGehtNicht DasDingGehtNicht force-pushed the fix-gmail-length-validation branch from d0aa34a to 89f02c2 Compare August 30, 2021 15:12
@DasDingGehtNicht
Copy link
Contributor Author

Could you add test cases?

modified existing test case.
test case: 'some.name.midd.leNa.me.and.locality+extension@GoogleMail.com',
pre-fix: invalid (somename.midd.leNa.me.and.locality = 35 chars)
post-fix: valid (somenamemiddleNameandlocality = 29 chars)

tux-tn
tux-tn approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

LGTM 🎉
Nice catch!

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed 🎉 first-pr labels Aug 30, 2021
fedeci
fedeci reviewed Aug 30, 2021
View reviewed changes
test/validators.js
@@ -70,7 +70,7 @@ describe('Validators', () => {
'hans@m端ller.com',
'test|123@m端ller.com',
'test123+ext@gmail.com',
'some.name.midd.leNa.me+extension@GoogleMail.com',
'some.name.midd.leNa.me.and.locality+extension@GoogleMail.com',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it was passing the test before? Is the GoogleMail domain part of gmail?

Copy link
Member

Choose a reason for hiding this comment

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

Max length is supposed to be 30 characters without the dots and the email alias (any string after the + character) . Even after removing only one dot the old string length was less than 30

@03-65
Copy link

03-65 commented Sep 1, 2021

I have no idea how to code

@03-65
Copy link

03-65 commented Sep 1, 2021

Could you add test cases?

modified existing test case.
test case: 'some.name.midd.leNa.me.and.locality+extension@GoogleMail.com',
pre-fix: invalid (somename.midd.leNa.me.and.locality = 35 chars)
post-fix: valid (somenamemiddleNameandlocality = 29 chars)

I don't know what you mean and I'm unable to Upload files it tells me

@03-65
Copy link

03-65 commented Sep 9, 2021

Thank you

profnandaa
profnandaa approved these changes Sep 26, 2021
View reviewed changes
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Good catch, thanks! 🎉

@profnandaa profnandaa merged commit a3497bd into validatorjs:master Sep 26, 2021
@profnandaa profnandaa mentioned this pull request Oct 31, 2021
Closed
@DasDingGehtNicht DasDingGehtNicht deleted the fix-gmail-length-validation branch November 19, 2021 00:53
@abdulrahman-khankan abdulrahman-khankan mentioned this pull request Nov 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedeci fedeci fedeci left review comments

@tux-tn tux-tn tux-tn approved these changes

@profnandaa profnandaa profnandaa approved these changes

Assignees
No one assigned
Labels
🎉 first-pr ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DasDingGehtNicht @03-65 @profnandaa @tux-tn @fedeci