isMobilePhone - fixed validation for am-AM #2140
Conversation
Added support of some codes of Ucom mobile network - 55, 50, 66
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #2140 +/- ##
=======================================
Coverage 99.95% 99.95%
=======================================
Files 107 107
Lines 2436 2436
Branches 615 615
=======================================
Hits 2435 2435
Partials 1 1
☔ View full report in Codecov by Sentry. |
src/lib/isMobilePhone.js
Outdated
| @@ -2,7 +2,7 @@ import assertString from './util/assertString'; | |||
|
|
|||
| /* eslint-disable max-len */ | |||
| const phones = { | |||
| 'am-AM': /^(\+?374|0)((10|[9|7][0-9])\d{6}$|[2-4]\d{7}$)/, | |||
| 'am-AM': /^(\+?374|0)((10|[5679][0-9])\d{6}$|[2-4]\d{7}$)/, | |||
There was a problem hiding this comment.
| 'am-AM': /^(\+?374|0)((10|[5679][0-9])\d{6}$|[2-4]\d{7}$)/, | |
| 'am-AM': /^(\+?374|0)((33|4[1|3|4]|55|77|88|9[1|3|4|5|6|8|9])\d{6}$)/, |
According to this reference from ITU (accessed from https://www.itu.int/oth/T020200000A/en) this should be the proper RegExp for mobile numbers.
Can you update the tests accordingly?
There was a problem hiding this comment.
Hi @WikiRik ,
Thanks for the suggestion. Unfortunately, your regexp doesn't cover all possible AM numbers.
I've changed regexp based on ITU spec and updated tests.
Thanks
There was a problem hiding this comment.
This validator is only for mobile phone numbers, see 'Non-geographic number for mobile services' on page 12 of above mentioned reference. Phone numbers that are marked as 'fixed telephony services' should not be valid in this validator.
There was a problem hiding this comment.
ok, got it
I've removed validation for 60xxxxxx numbers
test/validators.js
Outdated
| ], | ||
| invalid: [ | ||
| '12345', | ||
| '+37411498855', |
There was a problem hiding this comment.
This is sort of minor "breaking change", can you move the removed test-cases to the valid side then?
src/lib/isMobilePhone.js
Outdated
| @@ -2,7 +2,7 @@ import assertString from './util/assertString'; | |||
|
|
|||
| /* eslint-disable max-len */ | |||
| const phones = { | |||
| 'am-AM': /^(\+?374|0)((10|[9|7][0-9])\d{6}$|[2-4]\d{7}$)/, | |||
| 'am-AM': /^(\+?374|0)(1[0125][2-9]|22[2346]|23[1-7]|24[2-69]|25[2-7]|26[1-9]|28[13-7]|3[12]2|(33|4[134]|55|77|88|9[13-689])\d)\d{5}$/, | |||
There was a problem hiding this comment.
nit: wondering if this regex could be any simpler?
There was a problem hiding this comment.
I don't think, I did all my best to combine and simplify similar parts.
profnandaa
added
🎉 first-pr
🧹 needs-update
src/lib/isMobilePhone.js
Outdated
| @@ -2,7 +2,7 @@ import assertString from './util/assertString'; | |||
|
|
|||
| /* eslint-disable max-len */ | |||
| const phones = { | |||
| 'am-AM': /^(\+?374|0)((10|[9|7][0-9])\d{6}$|[2-4]\d{7}$)/, | |||
| 'am-AM': /^(\+?374|0)(1[0125][2-9]|22[2346]|23[1-7]|24[2-69]|25[2-7]|26[1-9]|28[13-7]|3[12]2|(33|4[134]|55|77|88|9[13-689])\d)\d{5}$/, | |||
There was a problem hiding this comment.
We're still validating fixed phone numbers in this RegExp. See the section for 'Non-geographic number for mobile services' on page 12 for the only mobile phone numbers; https://www.itu.int/dms_pub/itu-t/oth/02/02/T020200000A0007PDFE.pdf
| 'am-AM': /^(\+?374|0)(1[0125][2-9]|22[2346]|23[1-7]|24[2-69]|25[2-7]|26[1-9]|28[13-7]|3[12]2|(33|4[134]|55|77|88|9[13-689])\d)\d{5}$/, | |
| 'am-AM': /^(\+?374|0)(33|4[134]|55|77|88|9[13-689])\d{6}$/, |
There was a problem hiding this comment.
@WikiRik,
pattern for fixed phone numbers has been removed, only mobile numbers are covered now there.
Thank you for suggestion
profnandaa
added
✅ LGTM
and removed
🧹 needs-update
Added support of some codes of Ucom mobile network - 55, 50, 66
Wiki: Armenian mobile network codes
Checklist