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

Doesn't handle short codes properly #69

Open
johns10 opened this issue Jul 8, 2023 · 5 comments
Open

Doesn't handle short codes properly #69

johns10 opened this issue Jul 8, 2023 · 5 comments

Comments

@johns10
Copy link

johns10 commented Jul 8, 2023

This library doesn't appear to handle short codes properly.

Check this case:

iex(1)> {:ok, phone_number} = ExPhoneNumber.parse("88811", "US")        
{:ok,
 %ExPhoneNumber.Model.PhoneNumber{
   country_code: 1,
   national_number: 88811,
   extension: nil,
   italian_leading_zero: nil,
   number_of_leading_zeros: nil,
   raw_input: nil,
   country_code_source: nil,
   preferred_domestic_carrier_code: nil
 }}
iex(2)> ExPhoneNumber.is_valid_number?(phone_number)
false

That is a valid shortcode, from which I received a confirmation code.

I'd be willing to take this on. A little guidance on where to read the code to port from in libphonenumber, and where it should go in ex_phone_number would be super helpful.

@johns10
Copy link
Author

johns10 commented Jul 21, 2023

My first question here is:

Should the PhoneNumber struct land with a country code?

It seems like a significant portion of this library leans on having a country code.

https://en.wikipedia.org/wiki/Short_code#:~:text=the%20shortcode%20number.-,United%20States,Short%20Code%20Administration%20and%20CTIA.

However, in terms of dialing, they don't appear to be used. So, I'm not sure what the right choice is here.

@johns10
Copy link
Author

johns10 commented Jul 21, 2023

Ah, we don't have to define this, do we?

Apparently there is a short code standard defined alongside PhoneNumberMetaData.xml

https://github.com/google/libphonenumber/blob/master/resources/ShortNumberMetadata.xml

@johns10
Copy link
Author

johns10 commented Jul 21, 2023

I was initially looking at creating new metadata for this, but the more I look, there is a pretty standard format, and we can probably just lean on the PhoneNumberDescription in ShortNumberMetadata.xml

@johns10
Copy link
Author

johns10 commented Jul 21, 2023

I took a stab at this by fetching the ShortNumberMetadata and naively merging it in to the metadata struct. This broke 89 tests right off the bat, so maybe not the best approach?

@johns10
Copy link
Author

johns10 commented Jul 22, 2023

I made a PR for this. Its in no way complete or clean. I wanted to share what I've done so far for feedback. This is a bit of a yak shave for me so I implemented short code validation before creating phone numbers. Ya'll let me know what you think.

#71

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

2 participants