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

Unify Spree::User and Spree::Order email validation #3427

Open
spaghetticode opened this issue Nov 14, 2019 · 3 comments
Open

Unify Spree::User and Spree::Order email validation #3427

spaghetticode opened this issue Nov 14, 2019 · 3 comments
Labels
type:bug Error, flaw or fault

Comments

@spaghetticode
Copy link
Member

(not sure if this is more a bug or a new feature... I'm going for the latter as I think the solution will end up being a new feature 😄 )

At the moment we have a custom validator for the email in Spree::Order. This validator has different logic than the one used in solidus_auth_devise, so there may be instances when the user is valid, but the order is not. This creates confusion and can affect shop sales negatively when the issue goes unnoticed.

One possible course of action may be to use the same validator from solidus_auth_devise, which happens to be the one used by devise, as it's more used (if not respectful of standards) than ours... but only when the application is using solidus_auth_devise as well. More in general, if a user validator for email exists, the order should use that one instead of the one provided by Solidus.

email = '[email protected]'

u = Spree::User.new(email: email)
u.valid?
u.errors[:email]
# => [] 

o = Spree::Order.new(email: email)
o.valid?
o.errors[:email]
# => ["is invalid"] 
@spaghetticode
Copy link
Member Author

A bit more context on why this can be insidious, again from Solidus sandbox:

u = Spree::User.create email: '[email protected]', password: 'password', password_confirmation: 'password'

o = u.orders.create
o.persisted?
# => true
o.valid?
# => false
o.errors[:email]
 => ["is invalid"] 

The order is valid before being persisted, as the email field is blank. After the order is persisted, it becomes invalid as the email field, copied from the user, is not valid according to the validator.

@kennyadsl kennyadsl added the type:enhancement Proposed or newly added feature label Nov 28, 2019
@rubenochiavone
Copy link
Contributor

Hi there, I took a look at devise email validation and it turns out to be a regexp similar to Spree::EmailValidator::EMAIL_REGEXP. See

https://github.com/plataformatec/devise/blob/b52e642c0131f7b0d9f2dd24d8607a186f18223e/lib/devise.rb#L116

and

https://github.com/plataformatec/devise/blob/07f2712a22aa05b8da61c85307b80a3bd2ed6c4c/lib/devise/models/validatable.rb#L34

Then, replacing Spree::EmailValidator::EMAIL_REGEXP by Devise::email_regexp when Devise is defined would fix the issue and all commands you presented would be okay.

core/lib/spree/core/validators/email.rb

-      unless EMAIL_REGEXP.match? value
+      email_regexp = defined?(Devise) && defined?(Devise::email_regexp) ? Devise::email_regexp : EMAIL_REGEXP
+      unless email_regexp.match? value

WDYT?

@waiting-for-dev
Copy link
Contributor

For what it's worth, the email regexp used in orders can now be configured:

preference :default_email_regexp, :regexp, default: URI::MailTo::EMAIL_REGEXP
We should probably have solidus_auth_devise use the same one.

@waiting-for-dev waiting-for-dev added type:bug Error, flaw or fault and removed type:enhancement Proposed or newly added feature labels Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error, flaw or fault
Projects
None yet
Development

No branches or pull requests

4 participants