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

Validation fails if record country code does not match code in phone number #55

Closed
juanpaco opened this issue Apr 23, 2014 · 8 comments
Closed

Comments

@juanpaco
Copy link

Use case: Suppose you moved to Thailand but still have a Japanese phone number (not contrived-- seeing this in production).

Heart of the issue: The validator has this line @record.errors.add(attribute, error_message) if not Phony.plausible?(value, cc: country_number). That call to country_number will first look to see if a country code was supplied in the options. Then it will check to see if the record has a country code. If it doesn't have that, then it checks to see if the record has a country, which it then uses to look up a country code.

So, passing in nil as the country_number option won't work, because the validator will fallback to looking things up on the record.

So, I realize I could rename columns on my models, but for obvious reasons I'd rather avoid that.

2 questions:

  • 1 - Am I off my rocker here and missing something simple?
  • 2 - Are you open to a PR that fixes this? I was thinking something along the lines of an option that allows the record's country and the number's to not match. I pick that direction, because it wouldn't change functionality for existing users. Something like, :allow_record_and_phone_country_mismatch. It's kind of long, but I think it communicates the issue.

Thanks!

@joost
Copy link
Owner

joost commented Apr 23, 2014

Couldn't you use something like a validate blabla if: :some_method?

@juanpaco
Copy link
Author

I don't think that would get the job done in this case. Even if the phone number's country code doesn't match the country stored on the record, we still want to run it through the validator and set error messages for obviously bad numbers.

We're not looking to not validate in certain cases. We just don't want to require that the country code on the phone matches the country stored on the record.

@joost
Copy link
Owner

joost commented Apr 23, 2014

How can you validate a number if you don't have a country_number? Can Phony.plausible validate without it?

@joost
Copy link
Owner

joost commented Apr 23, 2014

Anyway I don't use the validator myself that much.. maybe @cinconnu or @pjg have an opinion :)

@juanpaco
Copy link
Author

The country code is supplied in the number, but that number doesn't match the country stored on the record. When Phony gets a number to validate and a country code explicitly stated, if those 2 don't match, then it fails the number.

Here are some examples (number redacted for obvious reasons):

2.0.0-p353 :009 > Phony.plausible?('+639111111111')
 => true

And here's the equivalent of what's getting called in the validator:

2.0.0-p353 :009 > Phony.plausible?('+639111111111', cc: '81')
 => false

I'm looking to make supplying the country code via the cc option optional with the validator. I want Phony to just take the phone number on its own merits and not consider the record's country attribute.

@joost
Copy link
Owner

joost commented Apr 23, 2014

I guess the validator could use some options like: use_country_number and use_country_code or something? That would fix the issue in a more general way right?

Pull request welcome :)

@juanpaco
Copy link
Author

Do you see those as booleans? Like, they could be set to false and that would skip supplying them to Phony?

@pjg
Copy link
Contributor

pjg commented Apr 23, 2014

This is an interesting problem.

I would suggest something a little bit more global, i.e. a setting called ignore_country_number and it would just pass the number straight to Phony, without any cc option (regardless if it's in the options hash or in the record's attributes).

@joost joost closed this as completed in 1e8a51e Jan 21, 2015
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

3 participants