827 : emails validation in person #965
Conversation
app/models/person.rb
Outdated
| validates :email, email: true | ||
| validates :email_2, email: true |
There was a problem hiding this comment.
I like the validates_with methods a little better, because they use a class name. If the class name EmailValidator is here in this validation line, someone reading this code will have to do less thinking to guess where the logic exists. I'm sure there's other ways to write this that accomplish the same thing. What I care about is that these email: true arguments feel like too much "Rails magic" too me
spec/models/person_spec.rb
Outdated
| context 'when the email field is not valid' do | ||
| it { is_expected.not_to be_valid } | ||
|
|
||
| it 'generates an error on the correct field' do | ||
| person.valid? |
There was a problem hiding this comment.
Do you think these tests would be better as tests on the validator itself instead of on the model?
app/validators/email_validator.rb
Outdated
| class EmailValidator < ActiveModel::EachValidator | ||
| def validate_each(record, attribute, value) | ||
| return if value.nil? || value.strip.empty? | ||
| unless /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i.match?(value) |
There was a problem hiding this comment.
@exbinary went to see if any of our dependencies already have an email regex.
The Devise gem includes an email regex, but I think that one is way too permissive.
@exbinary pointed out that Ruby's standard library includes URI::MailTo::EMAIL_REGEXP, which may be good enough for us. What do you think?
There was a problem hiding this comment.
Oh, never mind — I see that URI::MailTo::EMAIL_REGEXP would permit the bad example described in the issue
|
I was able to pull this branch to my local dev environment, where it worked with no problems! |
Sorry for my english.
Why
We need to validate emails in person (#827).
What
person_spec.rbHow
The validator is in the guide about validations.
Testing
A test is available now in
person_spec.rb.Next Steps
Unknown.
Outstanding Questions, Concerns and Other Notes
Not seem to be concerned.
Accessibility
Not seem to be concerned.
Security
Not seem to be concerned.
Meta
Not seem to be concerned.
Pre-Merge Checklist
I think the reviewer will check this so i let it empty.