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

Country code is re-added if phone number is wrong length #180

Open
milesmatthias opened this issue Aug 30, 2018 · 9 comments
Open

Country code is re-added if phone number is wrong length #180

milesmatthias opened this issue Aug 30, 2018 · 9 comments

Comments

@milesmatthias
Copy link

A perfectly valid response to this is that it's the application's responsibility, but I think it could be a nice option here.

We have US & Australian users, and if an Australian user puts in a phone number that is longer than 9 digits (ie: an American doing Australian testing, so they put in their 10 digit US number), phony_rails re-adds the country code (+61 in the case of Australia) to the phone number every time the user record is saved.

So we ended up with a couple user records with super long phone numbers (+61616161616161615555555555).

It'd be great if there was a config option for how to handle this situation that phony_rails just handled. Ex: truncate first 9 digits, truncate last 9 digits, etc. Or maybe there is, and I just missed it in the docs.

@milesmatthias
Copy link
Author

Possible that ignore_record_country_number and ignore_record_country_code will help here? Need to test.

@joost
Copy link
Owner

joost commented Sep 5, 2018

Not sure why this happens. Please elaborate on your (model) setup.

@milesmatthias
Copy link
Author

Our app/models/user.rb looks something like this:

class User < ActiveRecord::Base
  include CountryCode

  phony_normalize :phone_number, default_country_code: 'US'
end

The user model has an attribute called "country", which is either "AUS" or "US", so we use a little library to translate that into the proper country codes of "AU" or "US", in lib/country_code.rb:

module CountryCode

  # country code necessary for going_postal & phony_rails gems
  def country_code
    if country.present? && country == "AUS"
      "AU"
    else
      "US"
    end
  end

end

@joost
Copy link
Owner

joost commented Oct 11, 2018

Thanks. If you could give an example (eg. an example like given in #187) it might help to find the problem.

@synion
Copy link
Contributor

synion commented Jan 11, 2019

This could be solved by it #190.

joost pushed a commit that referenced this issue Jan 14, 2019
@joost
Copy link
Owner

joost commented Jan 14, 2019

@milesmatthias is this still an issue? I added a test that shows it should not happen.

@trueinviso
Copy link

trueinviso commented Jan 31, 2019

@joost This is still an issue. Basically if you don't validate your phone number before normalizing, and the number is invalid it will append the country code to the number every time your model is updated. If you look at the normalize_number method you can see where it happens:

# (Force) add country_number if missing 
# NOTE: do we need to force adding country code? Otherwise we can share logic with next block
if !Phony.plausible?(number) || _country_number != country_code_from_number(number)
  number = "#{_country_number}#{number}"
end

I don't understand why it's appending _country_number here if the number is not plausible. At first glance it looks like maybe this should be changed to:

if !Phony.plausible?(number) || _country_number != country_code_from_number(number)
  temp = "#{_country_number}#{number}"
  number = Phony.plausible?(temp) ? temp : number
end

This seems to solve the issue when I run it in the rails console, but I haven't run it against your test suite yet. Thoughts?

P.S. #190 doesn't solve this issue because we aren't using validation here.

@trueinviso
Copy link

trueinviso commented Jan 31, 2019

@joost Also, to get your test fo fail in this commit: 22d06c8 , you need to pass country_code: "AU" to PhonyRails.normalize_number, we have country_code saved on the model. Here is a PR that shows a failing test: #191.

@scottgratton
Copy link

Hey, just confirming that I am seeing the same

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

5 participants