joost / phony_rails

This Gem adds useful methods to your Rails app to validate, display and save phone numbers. It uses the super awesome Phony gem (https://github.com/floere/phony).
MIT License
554 stars 111 forks source link

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

Open milesmatthias opened 6 years ago

milesmatthias commented 6 years ago

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 commented 6 years ago

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

joost commented 6 years ago

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

milesmatthias commented 6 years ago

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 commented 5 years ago

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

synion commented 5 years ago

This could be solved by it https://github.com/joost/phony_rails/pull/190.

joost commented 5 years ago

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

trueinviso commented 5 years ago

@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 commented 5 years ago

@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.