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

feat: (isMobilePhone) add El Salvador validation #1766

Closed
wants to merge 2 commits into from
Closed

feat: (isMobilePhone) add El Salvador validation #1766

wants to merge 2 commits into from

Conversation

hereje
Copy link
Contributor

@hereje hereje commented Oct 9, 2021 •
edited
Loading

feat: (isMobilePhone) add El Salvador validation

Add mobile phone validation for El Salvador

  • country code with ISO: 'es-SV'
  • add entry to README.md file
  • add test case fixture entry for valid and invalid scenarios

Related to issue: #1761

Checklist

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

@codecov
Copy link

codecov bot commented Oct 9, 2021 •
edited
Loading

Codecov Report

Merging #1766 (6c1a29a) into master (13651ea) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1766   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          102       102           
  Lines         2029      2029           
  Branches       457       457           
=========================================
  Hits          2029      2029
Impacted Files Coverage Δ
src/lib/isMobilePhone.js 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 13651ea...6c1a29a. Read the comment docs.

tux-tn
tux-tn requested changes Oct 9, 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.

Hello @hereje and thank you for your PR!

The regex should only validate mobile phone numbers, looks like yours is also validating landline numbers. Please make the necessary changes to allow only mobile phones.

You can use this document from ITU or the Wikipieda article to know how mobile phones work in El Salvador

@hereje
fix: (isMobilePhone) add El Salvador validation
6c1a29a 
* Solve issue with wrong validation on El Salvador mobile phones
* update tests accordingly

in order to define the right regEx, the following site was
consulted.

https://www.siget.gob.sv/guia-de-servicios/consulta-el-plan-de-numeracion/numeros-moviles/
@hereje hereje requested a review from tux-tn October 10, 2021 03:27
@hereje
Copy link
Contributor Author

hereje commented Oct 13, 2021

Done

tux-tn
tux-tn approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@tux-tn tux-tn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for making the necessary changes @hereje ! LGTM 🎉
Is your contribution related to hacktoberfest?

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed and removed 🧹 needs-update For PRs that need to be updated before landing labels Oct 14, 2021
@hereje
Copy link
Contributor Author

hereje commented Oct 18, 2021

@tux-tn Yes it is!
I would appreciate if you mark my PR as hacktoberfest-accepted

@tux-tn
Copy link
Member

tux-tn commented Oct 18, 2021

Yes sir ! That's why i asked 😃

profnandaa
profnandaa approved these changes Oct 30, 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.

LGTM, thanks for your contrib! 🎉

@profnandaa
Copy link
Member

@hereje -- please fix the merge conflict on README and we should be good.

@profnandaa profnandaa added the mc-to-land Just merge-conflict standing between the PR and landing. label Oct 30, 2021
@profnandaa
Copy link
Member

Will be merged in #1851, no worries your commit contribution will be preserved. Thanks!

@profnandaa profnandaa closed this Oct 31, 2021
@profnandaa profnandaa mentioned this pull request Oct 31, 2021
Closed
@hereje hereje deleted the feat/add-validator-on-isMobilePhone-for-El_Salvador branch February 13, 2022 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@profnandaa profnandaa profnandaa approved these changes

Assignees
No one assigned
Labels
🎉 first-pr hacktoberfest-accepted mc-to-land Just merge-conflict standing between the PR and landing. 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.
3 participants
@hereje @tux-tn @profnandaa