isMobilePhone - fixed validation for am-AM by AlexKrupko · Pull Request #2140 · validatorjs/validator.js

isMobilePhone - fixed validation for am-AM by AlexKrupko · Pull Request #2140 · validatorjs/validator.js

Conversation

Added support of some codes of Ucom mobile network - 55, 50, 66

Wiki: Armenian mobile network codes

Checklist

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

Added support of some codes of Ucom mobile network - 55, 50, 66

Codecov Report

Patch and project coverage have no change.

Comparison is base (f074abd) 99.95% compared to head (158489b) 99.95%.

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

Files Changed	Coverage Δ
src/lib/isMobilePhone.js	`100.00% <ø> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

		/* 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}$)/,

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

ok, got it
I've removed validation for 60xxxxxx numbers

		],
		invalid: [
		'12345',
		'+37411498855',

Choose a reason for hiding this comment

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

This is sort of minor "breaking change", can you move the removed test-cases to the valid side then?

Choose a reason for hiding this comment

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

done

		/* 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}$/,

Choose a reason for hiding this comment

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

nit: wondering if this regex could be any simpler?

Choose a reason for hiding this comment

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

I don't think, I did all my best to combine and simplify similar parts.

		/* 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}$/,

Choose a reason for hiding this comment

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

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}$/,

Choose a reason for hiding this comment

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

@WikiRik,
pattern for fixed phone numbers has been removed, only mobile numbers are covered now there.
Thank you for suggestion

Choose a reason for hiding this comment

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

LGTM, thanks for your contrib! 🎉

vky5 mentioned this pull request

This was referenced

Sep 20, 2024

This was referenced

Sep 22, 2024