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

fix(isMobilePhone): Fix wrong dv-MV mobile phone matching (issue #2101) #2109

Merged
merged 2 commits into from
Jan 29, 2023

Conversation

pano9000
Copy link
Contributor

@pano9000 pano9000 commented Dec 2, 2022

Hello,

As reported in #2101, isMobilePhone's dv-MV locale is not matching actually correct mobile phone numbers currently.

See my comments there, which show that that the original RegExp was wrong since it was originally committed, last October:
#2101 (comment)

The new RegExp is now based on the official "numbering plan" document from the Maldives Gov
(I've used the archived wayback machine's link, just in case the "live" document above changes in the future, as they seem to be updating that document regularly. Live version can be found here, dated June 2022, as of today).

In there they describe the following ranges as valid mobile phone ranges:
72X XXXX to 79X XXXX
91X XXXX to 99X XXXX

Further examples can also be found in the comment above.

This PR fixes #2101
I also updated the tests accordingly, as the previous tests of course are not valid either anymore.

Potential room for improvement here (but I feel like that maybe should be a separate PR as it is more a feature maybe?):
Also account for "local formatting" of the number - i.e. it appears to be common to use a space or hyphen, separating the (non international) digits into two blocks, e.g.

  • 722 1234
  • 723-4567

I'll create a separate PR for this, after this fix has been pulled in

Thank you

Checklist

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

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (fba5b32) compared to base (f97e8d4).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2109   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          105       105           
  Lines         2335      2335           
  Branches       586       586           
=========================================
  Hits          2335      2335           
Impacted Files Coverage Δ
src/lib/isMobilePhone.js 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 9531 to 9534
'9609112345',
'9609958973',
'9607212963',
'9607986963',
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these really valid? It seems strange to have a country code there without any indicator that it is a country code (+ or 00, for example). I couldn't find any examples of numbers like this from your links.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment, as you make a valid point.

Strictly speaking:
no, these are not valid phone numbers, if there is no + or 00 in front of the country code.

However, I just tried to stick to the same RegExp style like (most if not almost all) the other phone numbers, which are making the + optional, when you don't set the strictMode option to true.
And I just included these technically invalid phone numbers in the tests as well, to test that this "optional" matching works as well :-)

E.g. random examples from isMobilePhone.js:

  • nn-NO currently uses /^(\+?47)?[49]\d{7}$/
  • el-GR currently used /^(\+?30|0)?(69\d{8})$/

both of these will match phone numbers without 00 or + as valid.

a big side note on this topic

Generally speaking the isMobilePhone RegExp do not seem to be very constistent altogether in the way the RegExp are matching the phone numbers:

  • some are checking for the + as mandatory (regardless of strictMode)
  • some are checking for the + as optional
  • many do not check for the 00 prefix, which actually should be valid as well
  • etc.

There's a whole range of different formats in the isMobilePhone, which IMHO should be made a bit more consistent, across all of the different countries (or locales in this case).

However fixing that would be a whole new thingon its own, which I could try to tackle soon as well - the only problem I see is that there are a lot of unmerged PRs in regards to the isMobilePhone, so I am not sure if I should already try to add my changes or wait until these are added?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is certainly interesting. I don't use phone validation from this library, so I haven't looked much into that side of things. This should definitely be fixed somehow. Could you post this as a new issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll write something up later today and post as a separate issue, so that we can track this better

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's track this as a separate issue, did you post it? Will definitely be a breaking change but a necessary 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.

@profnandaa yeah, had posted it as #2124

WikiRik
WikiRik previously approved these changes Dec 13, 2022
rubiin
rubiin previously approved these changes Jan 21, 2023
@rubiin rubiin added ready-to-land For PRs that are reviewed and ready to be landed ✅ LGTM labels Jan 21, 2023
profnandaa
profnandaa previously approved these changes Jan 22, 2023
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

@profnandaa profnandaa added mc-to-land Just merge-conflict standing between the PR and landing. and removed ready-to-land For PRs that are reviewed and ready to be landed labels Jan 22, 2023
The RegExp and corresponding tests were testing a wrong format.
Correct format can be found in the numbering plan:
https://web.archive.org/web/20220614004138/https://cam.gov.mv/docs/Numbering_plan.pdf

fixes issue validatorjs#2101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ LGTM mc-to-land Just merge-conflict standing between the PR and landing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect validation for Maldives phone numbers
5 participants