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

support Japanese 0x0 non-Geographic area codes with 11 digits numbers #239

Merged
merged 1 commit into from Mar 29, 2015

Conversation

mcaramello
Copy link
Contributor

Basing on https://en.wikipedia.org/wiki/Telephone_numbers_in_Japan#Non-geographic_area_codes + https://en.wikipedia.org/wiki/Telephone_numbers_in_Japan#Length_of_numbers
there are 11 digits long Japanese numbers that have Non geographic area codes 020, 030, 040, 050, 060, 070, 080, 090 (note that 010 is not included since it has special purpose).

This means valid phone numbers such as '+81 070 1234 5789' are rejected.

According to the same section, also 10 digits numbers with the same non geographic area codes could exist but when I tried modifying like this:

+ one_of(ndcs_with_11_subscriber_numbers) >> split(3,4) |
  one_of(ndcs_with_11_subscriber_numbers) >> split(4,4) | # 3-digit non-Geographic area codes (excluding 010)

'+81 070 1234 5789' -like numbers started getting rejected whereas '+81 070 123 5789' were accepted. Leaving those aside for now while I figure out where these types of numbers are coming from.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 99.26% when pulling 5ad7e80 on mcaramello:master into 10a6be7 on floere:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 99.29% when pulling 5ad7e80 on mcaramello:master into 10a6be7 on floere:master.

floere added a commit that referenced this pull request Mar 29, 2015
support Japanese 0x0 non-Geographic area codes with 11 digits numbers
@floere floere merged commit 3c43b77 into floere:master Mar 29, 2015
@floere
Copy link
Owner

floere commented Mar 29, 2015

Thanks for your contribution! :)
Released in 2.12.6.

@floere
Copy link
Owner

floere commented Mar 31, 2015

Would you mind adding some specs for these cases? I'm starting to wonder if it actually does what you'd like.

@mcaramello
Copy link
Contributor Author

Will certainly do.

I am honestly on the fence with this change as well and I have been pondering whether it's semantically correct or not (in spite of the fact that I am sure it works to cover my specific original use case).

As I discovered yesterday, a valid JP cell phone looks like this:

070 1234 5678

that is an 11 digits number with 070 as non geographical national code.
However when it's dialed from abroad with an international code, it has to be entered as:

+81 70 1234 5678

that is the initial 0 of 070 must be omitted. That makes it a 10 digits phone number.

Since this gem wants number always preceded by country codes, my change might be not necessary or incorrect.
However when used with the 'phony_rails' gem and with default country code 'JP', one would want to accept the former number (with 070 without international country code) and not the latter.

I'll investigate more on how phony and phony_rails interact and devise a plan.

Thanks for your patience.

@floere
Copy link
Owner

floere commented Apr 1, 2015

Hi,

That zero is the trunk prefix (0 in Japan). Sorry for not spotting this in your PR - I misremembered how Japan's numbers work.

Would you mind changing this to use a trunk code, removing/fixing the 0x0, and adding specs for calls from inside Japan (with the trunk prefix)?

Cheers!

On Wed, Apr 1, 2015 at 6:03 AM, Michele Caramello
notifications@github.com wrote:

Will certainly do.
I am honestly on the fence with this change as well and I have been pondering whether it's semantically correct or not (in spite of the fact that I am sure it works to cover my specific original use case).
As I discovered yesterday, a valid JP cell phone looks like this:

070 1234 5678

that is an 11 digits number with 070 as non geographical national code.
However when it's dialed from abroad with an international code, it has to be entered as:

+81 70 1234 5678

that is the initial 0 of 070 must be omitted. That makes it a 10 digits phone number.
Since this gem wants number always preceded by country codes, my change might be not necessary or incorrect.
However when used with the 'phony_rails' gem and with default country code 'JP', one would want to accept the former number (with 070 without international country code) and not the latter.
I'll investigate more and devise a plan.

Thanks for your patience.

Reply to this email directly or view it on GitHub:
#239 (comment)

@mcaramello
Copy link
Contributor Author

I was looking into this and I am having a senior moment. I hope you can clarify for me.

Based on my late finding and confirmed by your understanding, my change should be entirely removed because the Japan country definition already specifies a 0 'trunk' code: https://github.com/floere/phony/blob/master/lib/phony/countries/japan.rb#L423

Let me check my understanding with you:
given 070 1234 5678 is a valid internal Japanese number,
+81 70 1234 5678 is a valid Japan international number (trunk code 0 stripped out) => Phony.plausible?(number) should return true.
+81 070 1234 5678 is an invalid Japan international number (trunk code 0 not removed) => Phony.plausible?(number) should return false but Phony.plausible?(Phony.normalize(number)) should return true (because it stripped out the trunk code as an effect of the normalization).
Is this the correct/expected behavior?

Would you mind changing this to use a trunk code, removing/fixing the 0x0, and adding specs for calls > from inside Japan (with the trunk prefix)?

I want to add a spec for this but the last bit confuses me: does this gem support internal phone numbers that don't contain a country code? Through this format: https://github.com/floere/phony/blob/master/spec/functional/normalize_spec.rb#L115?
If any, the best place for a spec using a number without a country code is the normalize_spec - would there be a better place you can point me to?

Thanks for your help.

@floere
Copy link
Owner

floere commented Apr 7, 2015

Re the first part: your understanding is completely correct.

Currently the best place is the countries_spec.rb for country specific specs. The API using countries Phony['81'] is not yet public, though, but I am using it as an internal API to develop against. So specs there are still very much appreciated.

@floere
Copy link
Owner

floere commented Apr 7, 2015

I will revert your change, but will be happy to accept other pull requests. Thanks for your interest, help and work :)

@mcaramello
Copy link
Contributor Author

Sounds reasonable.
This PR turned out to be negative help, I am sorry...

@floere
Copy link
Owner

floere commented Apr 7, 2015

Not a problem at all, I really appreciate your work! It actually turned our to inspire somebody else (in #240) to also give input. Cheers!

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

Successfully merging this pull request may close these issues.

None yet

3 participants