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

feat(isMobilePhone): Added regex for Sudan ar-SD #2246

Merged
merged 3 commits into from Jul 24, 2023

Conversation

Hussienma
Copy link
Contributor

feat(isMobilePhone): Added regex for Sudan ar-SD

Added validation for Sudanese mobile (ar-SD)

Resourses

Wikipedia
International Telecommunication Union

Checklist

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

@@ -16,6 +16,7 @@ const phones = {
'ar-OM': /^((\+|00)968)?(9[1-9])\d{6}$/,
'ar-PS': /^(\+?970|0)5[6|9](\d{7})$/,
'ar-SA': /^(!?(\+?966)|0)?5\d{8}$/,
'ar-SD': /^(!?(\+?249)|0)?(9[0123569]|1[01258])\d{7}$/,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'ar-SD': /^(!?(\+?249)|0)?(9[0123569]|1[01258])\d{7}$/,
'ar-SD': /^(!?(\+?249)|0)?(9[123569])\d{7}$/,

I do not see +249 90 XXX XXXX in the ITU document nor the other reference that Wikipedia uses, but we can add those if you have an alternative source (possibly from the Zain Sudan website). The numbers +249 1X XXX XXXX are not mobile phones so they should not be a part of this validator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+249 90 XXX XXXX is valid and +249 95 XXX XXXX isn't according to ITU's Sudan Numbering Plan, according to the same doc Sudani/Sudatel Sudan provides mobile services. The Wikiperdia page seems outdated, I will try to update it.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for that new source. Do you happen to have an update to that as well? The presentation mentions problems with overlapping +249 100/11/12 and a planned solution but I'd like to know if that was implemented and what the current state is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I couldn't find any updated documents or articles concerning the overlapping issue. Either the solution wasn't publicated or not implemented yet. Cannot get in touch with either companies due to the current affairs.

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep it like this for now then, and we can see if in the future we can find more documentation on this. Just need to fix the tests and then we should be good

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'm sorry but I don't know why the tests aren't passing. How can I fix it?

invalid: [
'12345',
'',
'+249962662622',
Copy link
Member

Choose a reason for hiding this comment

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

This phone number is not invalid due to the regular expression so that's why the test is failing

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2440c39) 99.95% compared to head (d1bab4e) 99.95%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2246   +/-   ##
=======================================
  Coverage   99.95%   99.95%           
=======================================
  Files         107      107           
  Lines        2419     2419           
  Branches      608      608           
=======================================
  Hits         2418     2418           
  Partials        1        1           
Impacted Files Coverage Δ
src/lib/isMobilePhone.js 100.00% <ø> (ø)

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

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 contribution!

@profnandaa profnandaa merged commit 2ef9a83 into validatorjs:master Jul 24, 2023
9 checks passed
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.

None yet

3 participants