isMobilePhone - fixed validation for am-AM - #2140
Conversation
Sorry, something went wrong.
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. |
Sorry, something went wrong.
| /* 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?
Sorry, something went wrong.
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
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
ok, got it
I've removed validation for 60xxxxxx numbers
Sorry, something went wrong.
| ], | ||
| 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?
Sorry, something went wrong.
There was a problem hiding this comment.
done
Sorry, something went wrong.
| /* 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?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think, I did all my best to combine and simplify similar parts.
Sorry, something went wrong.
| /* 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}$/, |
Sorry, something went wrong.
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
Sorry, something went wrong.
profnandaa
left a comment
There was a problem hiding this comment.
LGTM, thanks for your contrib! 🎉
Sorry, something went wrong.
Added support of some codes of Ucom mobile network - 55, 50, 66
Wiki: Armenian mobile network codes
Checklist