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

Updated mobile phone number validator and test #1167

Merged
merged 4 commits into from
Jun 2, 2020

Conversation

MladenZeljic
Copy link
Contributor

Added verification for bosnian mobile phone numbers

Added verification for bosnian mobile phone numbers
Fixed bs locale
@MladenZeljic
Copy link
Contributor Author

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

@ezkemboi
Copy link
Member

ezkemboi commented Oct 20, 2019

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
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.

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.

Thanks for your PR! 🎉

@@ -12,6 +12,7 @@ const phones = {
'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
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
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
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...

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 PR! 🎉

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

README.md Outdated
@@ -122,7 +122,7 @@ Validator | Description
**isMagnetURI(str)** | check if the string is a [magnet uri format](https://en.wikipedia.org/wiki/Magnet_URI_scheme).
**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
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
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
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
Copy link
Contributor Author

MladenZeljic commented Jun 1, 2020

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
Copy link
Contributor Author

MladenZeljic commented Jun 1, 2020

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 :-)

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