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

Replace E-mail Validator #1794

Closed
gmacdougall opened this issue Mar 23, 2017 · 6 comments · Fixed by #3395
Closed

Replace E-mail Validator #1794

gmacdougall opened this issue Mar 23, 2017 · 6 comments · Fixed by #3395

Comments

@gmacdougall
Copy link
Member

Our current e-mail validator is not good. It is a simplistic regular expression that incorrectly flags some e-mails as being valid though they are not.

The validates_email_format_of gem can be of assistance here. It has a superior implementation of e-mail validation, and superior test cases.

We should consider using this solution rather than our current validator.

@jasonfb
Copy link

jasonfb commented Mar 23, 2017

this is just one person's opinion about why you should just avoid email validation altogether. (I happen to agree with him but there are good arguments to made for validation)

https://davidcel.is/posts/stop-validating-email-addresses-with-regex/

@jasonfb
Copy link

jasonfb commented Mar 23, 2017

having said that, we really like this approach (which isn't really an email validator at all, more a suggester): https://github.com/mailcheck/mailcheck

You can check that out live by going to Kickstarter (log out if you are already logged in), then click 'Sign up'. Then enter your email but purposefully leave off the .com (or in your case .ca), you'll see it suggest the right email to you

Then again, now that I think about it, it probably suggests for you .com when your email is actually .ca which must be annoying and off-putting for anyone with a non-.com email (mostly people outside U.S.)

@mtomov
Copy link
Contributor

mtomov commented Mar 24, 2017

I think that so log as it does not flag correct emails as being wrong, it's fine as it is. I don't consider it to be a core responsibility of Solidus to handle such use cases with perfection. They can be implemented at the individual store level.

@gmacdougall
Copy link
Member Author

I tend to think that since we're providing an e-mail validator we should make it decent.

We can also try to get out of that business altogether, but that's a bit more of an invasive change.

@tvdeyen
Copy link
Member

tvdeyen commented Mar 28, 2017

We can also try to get out of that business altogether, but that's a bit more of an invasive change.

Why not let the user class provider handle this (in our case solidus_auth_devise could let Devise validate this email)?

That's definitely not something we should do in core.

@dividedharmony
Copy link
Contributor

This didn't have the Solidus Hackathon label, but I thought it was simple enough, so I put in PR #1965 if this issue is still relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants