Skip to content

Updated mobile phone number validator and test#1167

Merged
profnandaa merged 4 commits into
validatorjs:masterfrom
MladenZeljic:master
Jun 2, 2020
Merged

Updated mobile phone number validator and test#1167
profnandaa merged 4 commits into
validatorjs:masterfrom
MladenZeljic:master

Conversation

@MladenZeljic

Copy link
Copy Markdown
Contributor

Added verification for bosnian mobile phone numbers

Added verification for bosnian mobile phone numbers
Fixed bs locale
@MladenZeljic

Copy link
Copy Markdown
Contributor Author

@ezkemboi @profnandaa Hello! I hope that my changes are ok codewise :-)

@ezkemboi

ezkemboi commented Oct 20, 2019

Copy link
Copy Markdown
Member

I think those are the files that are required to be changed.
Did you run tests?
Also for faster review, you can send a referral link where we can get information on Bosnian locale mobile phone numbers (Not A must).

@MladenZeljic

Copy link
Copy Markdown
Contributor Author

@ezkemboi Hey, thanks for reviewing. Yes I did run the tests, they are ending with the most beautiful color (green) x-D. I am from Bosnia and these are the most used, standard mobile phone numbers in our country.

@profnandaa profnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your PR! 🎉

Comment thread src/lib/isMobilePhone.js
'ar-SA': /^(!?(\+?966)|0)?5\d{8}$/,
'ar-SY': /^(!?(\+?963)|0)?9\d{8}$/,
'ar-TN': /^(\+?216)?[2459]\d{7}$/,
'bs-BA': /^((((\+|0{2})3876)|06)[0-6])((?<=4)\d{7}|(?<!4)\d{6})$/,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like this regex could be simplified? Please check how the rest were written...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can rewrite and simplify it, but with that, this regex will leave out some forms of mobile phone numbers which are valid in my country. Its your choice :-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, I see. I'll take this in...

@profnandaa profnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for your PR! 🎉

Will just request one extra pair of eyes on this and we land it. /cc. @ezkemboi @tux-tn

Comment thread README.md Outdated
**isMD5(str)** | check if the string is a MD5 hash.
**isMimeType(str)** | check if the string matches to a valid [MIME type](https://en.wikipedia.org/wiki/Media_type) format
**isMobilePhone(str [, locale [, options]])** | check if the string is a mobile phone number,<br/><br/>(locale is either an array of locales (e.g `['sk-SK', 'sr-RS']`) OR one of `['ar-AE', 'ar-BH', 'ar-DZ', 'ar-EG', 'ar-IQ', ar-JO', 'ar-KW', 'ar-SA', 'ar-SY', 'ar-TN', 'be-BY', 'bg-BG', 'bn-BD', 'cs-CZ', 'de-DE', 'de-AT', 'da-DK', 'el-GR', 'en-AU', 'en-CA', 'en-GB', 'en-GG', 'en-GH', 'en-HK', 'en-IE', 'en-IN', 'en-KE', 'en-MT', 'en-MU', 'en-NG', 'en-NZ', 'en-RW', 'en-SG', 'en-UG', 'en-US', 'en-TZ', 'en-ZA', 'en-ZM', 'en-PK', 'es-ES', 'es-MX', 'es-PA', 'es-PY', 'es-UY', 'et-EE', 'fa-IR', 'fi-FI', 'fj-FJ', 'fr-FR', 'fr-GF', 'fr-GP', 'fr-MQ', 'fr-RE', 'he-IL', 'hu-HU', 'id-ID', 'it-IT', 'ja-JP', 'kk-KZ', 'ko-KR', 'lt-LT', 'ms-MY', 'nb-NO', 'nl-BE', 'nl-NL', 'nn-NO', 'pl-PL', 'pt-PT', 'pt-BR', 'ro-RO', 'ru-RU', 'sl-SI', 'sk-SK', 'sr-RS', 'sv-SE', 'th-TH', 'tr-TR', 'uk-UA', 'vi-VN', 'zh-CN', 'zh-HK', 'zh-TW']` OR defaults to 'any'. If 'any' or a falsey value is used, function will check if any of the locales match).<br/><br/>`options` is an optional object that can be supplied with the following keys: `strictMode`, if this is set to `true`, the mobile phone number must be supplied with the country code and therefore must start with `+`. Locale list is `validator.isMobilePhoneLocales`.
**isMobilePhone(str [, locale [, options]])** | check if the string is a mobile phone number,<br/><br/>(locale is either an array of locales (e.g `['sk-SK', 'sr-RS']`) OR one of `['ar-AE', 'ar-BH', 'ar-DZ', 'ar-EG', 'ar-IQ', ar-JO', 'ar-KW', 'ar-SA', 'ar-SY', 'ar-TN', 'ba-BS', 'be-BY', 'bg-BG', 'bn-BD', 'cs-CZ', 'de-DE', 'de-AT', 'da-DK', 'el-GR', 'en-AU', 'en-CA', 'en-GB', 'en-GG', 'en-GH', 'en-HK', 'en-IE', 'en-IN', 'en-KE', 'en-MT', 'en-MU', 'en-NG', 'en-NZ', 'en-RW', 'en-SG', 'en-UG', 'en-US', 'en-TZ', 'en-ZA', 'en-ZM', 'en-PK', 'es-ES', 'es-MX', 'es-PA', 'es-PY', 'es-UY', 'et-EE', 'fa-IR', 'fi-FI', 'fj-FJ', 'fr-FR', 'fr-GF', 'fr-GP', 'fr-MQ', 'fr-RE', 'he-IL', 'hu-HU', 'id-ID', 'it-IT', 'ja-JP', 'kk-KZ', 'ko-KR', 'lt-LT', 'ms-MY', 'nb-NO', 'nl-BE', 'nl-NL', 'nn-NO', 'pl-PL', 'pt-PT', 'pt-BR', 'ro-RO', 'ru-RU', 'sl-SI', 'sk-SK', 'sr-RS', 'sv-SE', 'th-TH', 'tr-TR', 'uk-UA', 'vi-VN', 'zh-CN', 'zh-HK', 'zh-TW']` OR defaults to 'any'. If 'any' or a falsey value is used, function will check if any of the locales match).<br/><br/>`options` is an optional object that can be supplied with the following keys: `strictMode`, if this is set to `true`, the mobile phone number must be supplied with the country code and therefore must start with `+`. Locale list is `validator.isMobilePhoneLocales`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You have a little typo, locale is bs-BA and not ba-BS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops D-:
I'll update it :-D

Updated locale in readme
@profnandaa

Copy link
Copy Markdown
Member

@MladenZeljic -- sorry for my delay in re-checking this. Could you just fix the m/c and we should be ready to land this?

@profnandaa profnandaa added mc-to-land Just merge-conflict standing between the PR and landing. 🧹 needs-update For PRs that need to be updated before landing and removed needs-more-review labels Mar 22, 2020
@MladenZeljic

MladenZeljic commented Jun 1, 2020

Copy link
Copy Markdown
Contributor Author

Hey @profnandaa, sorry about the delay, I was very busy these past couple of months. I can try to get to this in next few days if thats ok?

@MladenZeljic

MladenZeljic commented Jun 1, 2020

Copy link
Copy Markdown
Contributor Author

Hey @profnandaa I was able to get this done today. If everything is cool you can merge this. Sorry for waiting this long and thank you :-)

@profnandaa profnandaa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for the contrib! 🎉

@profnandaa profnandaa merged commit c5cab7d into validatorjs:master Jun 2, 2020
profnandaa added a commit that referenced this pull request Jun 11, 2020
coz of backward compatibility issue #1354
@profnandaa profnandaa mentioned this pull request Jun 11, 2020
3 tasks
chriso pushed a commit that referenced this pull request Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mc-to-land Just merge-conflict standing between the PR and landing. 🧹 needs-update For PRs that need to be updated before landing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants