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

Nil return values for normalize cause validations to pass #92

Closed
vdolbilov opened this issue Apr 28, 2015 · 7 comments
Closed

Nil return values for normalize cause validations to pass #92

vdolbilov opened this issue Apr 28, 2015 · 7 comments

Comments

@vdolbilov
Copy link

Hi, I have in my User model:

phony_normalize :phone_number, default_country_code: 'US'
validates :phone_number, phony_plausible: true

One thing I noticed is that setting a string value that causes #normalize_number to return nil will pass validations:

u = User.find(blah)
u.phone_number = "Clearly invalid"
u.save!
=> true
u.phone_number
=> nil

However,

PhonyRails.plausible_number? nil
=> false

Am I doing the validation incorrectly? Setting a phone number like "12345" causes the validation to fail as expected.

@joost
Copy link
Owner

joost commented Apr 29, 2015

Have you tried allow_nil: false ?

@joost
Copy link
Owner

joost commented Apr 29, 2015

@pjg did most of the validator stuff so he might know the answer :)

@dmdeller
Copy link

The problem seems to be that PhonyRails attempts to normalize the number before validating, which transforms the value into nil if it was invalid, causing it to pass validation when blank is allowed and an invalid value is submitted.

I've had success by avoiding using both phony_normalize and validates_plausible_phone entirely, and using this approach instead:

PHONE_DEFAULT_COUNTRY_CODE = 'US'

validate :phone_looks_real

after_validation :normalize_phone

def phone_looks_real
  unless self.phone.blank? || PhonyRails.plausible_number?(self.phone, default_country_code: PHONE_DEFAULT_COUNTRY_CODE)
    errors.add(:phone, "doesn't look like a valid phone number")
  end
end

def normalize_phone
  self.phone = PhonyRails.normalize_number(self.phone, default_country_code: PHONE_DEFAULT_COUNTRY_CODE)
end

This seems to work with any US phone number I throw at it, with or without any of the following: dashes, spaces, parentheses, country code. It rejects strings like 'foo' as invalid. It also allows leaving the field blank, which is what I needed.

@vdolbilov
Copy link
Author

Thanks! I ended up doing basically the same thing since I needed to allow nil values. Closing this issue since this is a good solution.

@pjg
Copy link
Contributor

pjg commented Apr 30, 2015

I like when things get resolved before I have time to look into them ;) It's been ages since I've looked at phony's source code.

@bartoszkopinski
Copy link

I'm afraid it's not really resolved as the behaviour is still inconsistent if I don't specify presence: true:

phony_normalize :phone_number, default_country_code: 'GB'
validates :phone_number, phony_plausible: true
pry(main)> u.phone_number
=> "+447400018923"
pry(main)> u.phone_number = '123'
=> "123"
pry(main)> u.phone_number
=> "123"
pry(main)> u.valid?
=> false
pry(main)> u.phone_number = 'asd'
=> "asd"
pry(main)> u.valid?
=> true
pry(main)> u.phone_number
=> nil

@joost @pjg any chances of fixing this?

@pjg
Copy link
Contributor

pjg commented Oct 20, 2015

I don't use the gem any more, so I don't think I'll be able to help. Sorry!

joost pushed a commit that referenced this issue Mar 12, 2016
Only assigned normalize values if there is one. Fixes #92.
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