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

Phony has trouble validating luxembourg phone numbers #276

Closed
jdel opened this issue Sep 3, 2015 · 17 comments
Closed

Phony has trouble validating luxembourg phone numbers #276

jdel opened this issue Sep 3, 2015 · 17 comments

Comments

@jdel
Copy link
Contributor

jdel commented Sep 3, 2015

Hello,

I am using phony to validate phone numbers that users input with international format.
Some valid numbers are not plausible according to phony. Example numbers are:
+352 44 55 66
+352 54 52 58
+352 81 81 81
+352 35 72 14 1

I don't understand why some numbers will validate and not others. So far, I have seen that all the 6 digits numbers don't validate, some 7 don't, but some, like this one, will:
+352 40 22 66 1
I am pretty sure this one has a base phone number of 40 22 66 with extension 1.

I already looked in the code, but I'm not familiar with telephony in general so I'm pretty much useless at troubleshooting this issue.

Any advice would be much appreciated.

Thanks and regards.

@floere
Copy link
Owner

floere commented Sep 3, 2015

Hi Julien,

Phony operates on these rules: https://github.com/floere/phony/blob/master/lib/phony/countries.rb#L595-L602
It could well be that these rules do not cover all cases.

Can you have a look at them and let me know which ones are incorrect? (The rules are, hopefully, understandable – and if not, let me know)

Cheers & thanks!

@jdel
Copy link
Contributor Author

jdel commented Sep 10, 2015

The rules are understandable, however, I don't know what the specs are for luxembourg numbers, especially regarding length. Looks like the minimum length for landlines is 6 digits total excluding the country code (formatted by 3 groups of 2). Mobile numbers are always 9 digits (formatted by 3 groups of 3, or 1 group of 3 and 3 groups of 2).

The regexp /^(2[467]\d{2})$ only seems to match 4 number digits starting with 24, 26 or 27 but tries to split them in 3 groups of two. I'm not sure how this works as the numbers seem to be formed of exclusively 4 digits. However, some valid numbers containing 6 digits and starting with 26 are valid but plausible? returns false.

I don't understand how it validates the length in the first condition one_of('4') ?
The number 44 55 66 for example should be validated as it is a valid 6 digits number like many but plausible? returns false. However on 44 55 66 1 it will return true, on 44 55 66 11 it will return false.

If I try to format "+352 44 55 66" which should be formatted exactly how I've written it is formatted differently by Phony:

irb(main):015:0> Phony.format(Phony.normalize("+352445566"))
=> "+352 4 45 56 6"

On the upside, all mobile numbers seem to work fine.

While the formatting is ok to me, the length validation seems to be causing the issue with plausible? but I cannot find where this length validation is done.

@jdel
Copy link
Contributor Author

jdel commented Sep 10, 2015

After a bit of fiddling, I ended up with the following rules that seem to do what I want:

      match(/^(2[467]\d{2})\d{4}$/)         >> split(2,2)     | # 4-digit NDC
      match(/^(6\d[18])\d+$/)               >> split(3,3)     | # mobile
      match(/^(60\d{2})\d{8}$/)             >> split(2,2,2,2) | # mobile machine to machine
      match(/^([2-9][^467])\d{4}$/)         >> split(2,2)     | # 2-digit NDC Regular 6 didigts number          
      match(/^([2-9][^467])\d{5}$/)         >> split(2,2,1)   | # 2-digit NDC Regular 6 didigts number 
                                                                # w/ 1 digit extension
      match(/^([2-9][^467])\d{6}$/)         >> split(2,2,2)   | # 2-digit NDC Regular 8 didigts number
                                                                # or 6 digits with 2 digits extension
      match(/^([2-9][^468])\d{7}$/)         >> split(2,2,3)   | # 2-digit NDC Regular 6 digits 
                                                                # with 3 digit extension
      match(/^([2-9][^467])\d{8}$/)         >> split(2,2,4)     # 2-digit NDC Regular 6 didigts number
                                                                # with 4 digit extension

I am not sure it is interesting to separate numbers starting with 4 to give them a different formatting as they are more commonly formatted XX XX XX and not X XX XXX so I removed the first rule altogether.

I also added a few rules to match numbers with extensions.

I will monkey patch the gem on my test environment to do more tests in the next few days.

@floere
Copy link
Owner

floere commented Sep 10, 2015

Looking good! (Small typo: "didigts")

Do you know how to change the Phony code (fork etc.), how to run the tests (rake) and how to make a pull request? I'm happy to pull your change if the tests run :)

@floere
Copy link
Owner

floere commented Sep 10, 2015

(And if the tests don't run, feel free to adapt the tests – within reason – according to how Luxembourg is supposed to work)

@jdel
Copy link
Contributor Author

jdel commented Sep 11, 2015

Yes, I will fork and submit a PR if i can do that behind our company proxy, otherwise it will have to wait until next week.

@floere
Copy link
Owner

floere commented Sep 11, 2015

@jdel Thanks! (Protip: Use https)

@klaustopher
Copy link

Any update on this? @jdel? I just ran into the same issue and would put some time into fixing this, but if you already did the work, there's not much use in doing it again 😄

Number in question for me is +352 454 565 4001

@floere
Copy link
Owner

floere commented Jan 4, 2016

I'm happy for any and all PRs 😊

@floere
Copy link
Owner

floere commented Jan 4, 2016

@klaustopher Since it's been 4 months already, I think you can go ahead :)

@jdel
Copy link
Contributor Author

jdel commented Jan 7, 2016

Sorry guys, the last time I tried to download the github client on my corporate computer, I got flagged for potential virus and my computer was rebuilt :/

I have since monkey patched phony on my live environment and it is running happily with the following rules:

match(/^(2[467]\d{2})\d{4}$/)       >> split(2,2)       | # 4-digit NDC
match(/^(6\d[18])\d+$/)                 >> split(3,3)     | # mobile
match(/^(60\d{2})\d{8}$/)                   >> split(2,2,2,2) | # mobile machine to machine
match(/^((2[^467]|[3-9]\d))\d{4}$/) >> split(2,2)     | # 2-digit NDC Regular 6 digits number          
match(/^((2[^467]|[3-9]\d))\d{5}$/) >> split(2,2,1)   | # 2-digit NDC Regular 6 digits number w/ 1 digit extension
match(/^((2[^467]|[3-9]\d))\d{6}$/) >> split(2,2,2)   | # 2-digit NDC Regular 8 digits number or 6 digits with 2 digits extension
match(/^((2[^467]|[3-9]\d))\d{7}$/) >> split(2,2,3)   | # 2-digit NDC Regular 6 digits with 4 digits extension
match(/^((2[^467]|[3-9]\d))\d{8}$/) >> split(2,2,4)     # 2-digit NDC Regular 6 digits number with 4 digits extension

@klaustopher Feel free to submit a PR, as I will definitely not have time to look at the tests. Also, I tried the number you are having trouble with, and it validates fine with my rules.

@klaustopher
Copy link

Ok, thanks I will have a go at it in the next days ;)

@floere
Copy link
Owner

floere commented Jan 7, 2016

@jdel Shall we close #280?

@jdel
Copy link
Contributor Author

jdel commented Jan 12, 2016

@floere, yes feel free too, I will not have a chance to look at the tests anytime soon.

@floere
Copy link
Owner

floere commented Feb 19, 2016

@klaustopher Should I keep this open?

@klaustopher
Copy link

Yes, please, I will get on this probably next week ;)

@klaustopher
Copy link

So, last week got so many other things in front of this one, that I was actually planning to do this during the bug fixing day tomorrow, but now @bdewater has already completed it. Cool 😎 ... Works fine with the numbers I had trouble with

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

No branches or pull requests

3 participants