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

isValidNumber() passing on invalid number #1535

Closed
NickSutton opened this issue Feb 27, 2024 · 14 comments
Closed

isValidNumber() passing on invalid number #1535

NickSutton opened this issue Feb 27, 2024 · 14 comments

Comments

@NickSutton
Copy link

Steps to reproduce

  1. Select UK ('gb') as the intended country
  2. Enter 7 digits of the mobile number
  3. I am using:
      $("#tel").on("keyup", function () {
        if (iti.isValidNumber()) {
          $(".swal2-confirm").focus();
        }
      });

which focuses the confirm button at length = 7 instead of the full length number for the UK.

Expected behaviour

Should only pass after 11 digits (ie, 07123 012345) or 10 digits if the number begins +44 +447123 012345

Actual behaviour

isValidNumber() is passing when length is 7 for UK format numbers

Initialisation options

        autoInsertDialCode: true,
        initialCountry: 'gb',
        nationalMode: false,
        utilsScript: "assets/InternationalTel_dev/build/js/utils.js",
@jackocnr
Copy link
Owner

We use libphonenumber for formatting, validation and generating placeholder numbers - if your issue relates to one of these things, please check their test site first and if you see the problem there please file an issue with them instead.

If it's working correctly in libphonenumber, the next step is to check that we're using the latest version of libphonenumber in the plugin - check the version history here. If we need to upgrade, it only takes 5 minutes to do this yourself and submit a Pull Request - instructions here: Updating to a new version of libphonenumber.

@NickSutton
Copy link
Author

NickSutton commented Feb 28, 2024

Hi,

Thanks for this, tests pass correctly on the test site, and I've just pulled the latest update files and tested again and the problem would appear to persist!

@jackocnr
Copy link
Owner

jackocnr commented Feb 29, 2024

Thanks for raising this. I can confirm that isValidNumber is incorrectly returning true for UK numbers of 7-10 digits (as you say it should only be true for UK numbers of 11 digits). I think this is because technically, in the UK, you can dial local numbers of 7 digits and they work fine. But obviously, that is not useful for us in this context.

The quick fix is to switch to isValidNumberPrecise which should work as expected. You can check the readme for more info on the difference between these two methods.

I will explore this issue further with libphonenumber, the lib we use for validation, and see if we can find a way to fix it. So I'll leave this issue open for now.

UPDATE: I've asked the question on the libphonenumber discussion group here: https://groups.google.com/g/libphonenumber-discuss/c/H-VVzK0AArM

Some test results from libphonenumber below:
(NOTE that our isValidNumber actually uses their isPossibleNumber)

0740 - is possible number (local only)
07400 - is possible number (local only)
074001 - is possible number (local only)
0740012 - is possible number
07400123 - is possible number
074001234 - is possible number
0740012345 - is possible number
07400123456 - is possible number (this is actually a valid number)

+447400 - is possible number (local only)
+4474001 - is possible number (local only)
+44740012 - is possible number (local only)
+447400123 - is possible number
+4474001234 - is possible number (local only)
+44740012345 - is possible number
+447400123456 - is possible number (this is actually a valid number)

We are able to filter out the local-only numbers and mark them as invalid, but the problem is that libphonenumber isn't consistently marking them as local-only. So either this is a bug with LPN, which is unlikely, or maybe there's another way to check if they're local only (which I've asked in the discussion), or else maybe there's a way to simplify the metadata so it only includes patterns that are relevant to mobile numbers.

Failing that, it would actually be fairly straightforward to add some custom code here, which says if it's a GB number, then check its 11 digits (while also handling the possibility of +44, or the use of separateDialCode etc). Though I wonder if this issue applies to any other countries as well...

@jackocnr
Copy link
Owner

jackocnr commented Mar 2, 2024

I think it's unlikely we will get any help from LPN any time soon, so I've added a workaround for this for now, which fixes isValidNumber for UK mobile numbers at least. Released in v19.5.5.

@jackocnr
Copy link
Owner

jackocnr commented Mar 6, 2024

The below screenshot is from the Wikipedia page for Telephone numbers in the United Kingdom shows that there are valid UK numbers (after the leading 0) of length 7, 9 and 10. (It also shows valid numbers of shorter lengths e.g. dialing "999" for emergency services, but I think libphonenumber excludes numbers like those)

So in this case, we would have to find a way to exclude 08 (toll free) numbers lengths from the validation. Perhaps we only want to include mobile numbers? I think that would be fine for most use cases. So I'll look into this next. Ideally we'd find a way to exclude all the meta data for other number types, to reduce the bundle size, but I don't know if this is possible - I'll ask on the LPN discussion thread, linked above.

Screenshot 2024-03-06 at 16 46 50

@jackocnr
Copy link
Owner

jackocnr commented Mar 6, 2024

Ok I'm going to re-open this issue, as I've found a way to move forwards, but don't have time to implement it right now.

In utils.js, in the isPossibleNumber method, we should be using the LPN method isPossibleNumberForTypeWithReason and specifying a number type each time e.g. MOBILE or FIXED_LINE. I think that by default it should be mobile-only, which I think will be the use case for the vast majority of people, but we can have an argument mobileOnly which for now will default to false (so it's not a breaking change), but in the next major release will probably just default to true, as that will be what most people want. This will obviously need to be well documented in the readme and on the demo site.

@jackocnr jackocnr reopened this Mar 6, 2024
@NickSutton
Copy link
Author

Hi, I think the last comment hits the nail on the head.

For the UK, mobile numbers can be 07 or +44 if using the international prefix. For example: 07111 012345 = +447111 012345

01 numbers will be a landline of a residence or business. I've never actually heard of a 7-digit number, it looks like they are newer toll-free options.

I think the mobileOnly argument would help, for sure.

@jackocnr
Copy link
Owner

jackocnr commented Mar 7, 2024

Just noting here that LPN have said there is no way to include/exclude the metadata for specific number types unfortunately.

@jackocnr
Copy link
Owner

jackocnr commented Mar 7, 2024

isValidNumber(mobileOnly) option released in v19.5.6

@NickSutton
Copy link
Author

isValidNumber(mobileOnly) option released in v19.5.6

Unfortunately, I am still seeing the original issue, when called iti.isValidNumber(true) on UK registered mobile numbers such as +447123012345 and 07123456789

@jackocnr
Copy link
Owner

I'm confused, the original issue was about isValidNumber returning true for invalid numbers, but the two number examples you just shared are valid, no?

@NickSutton
Copy link
Author

Those two numbers are valid, but isValidNumber(true) is passing earlier than it should, as per the initial post, it would seem, at '07123 01'

@jackocnr
Copy link
Owner

That's strange, I can't reproduce that on the validation demo - if I select GB and enter "07123 01" and click Validate it says "Invalid number". (Note that this demo is using v20.0.5, where mobileOnly is true by default, so no need to pass true to isValidNumber). Can you make sure you're using the latest version?

@NickSutton
Copy link
Author

NickSutton commented Mar 21, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants