solidus icon indicating copy to clipboard operation
solidus copied to clipboard

Unify `Spree::User` and `Spree::Order` email validation

Open spaghetticode opened this issue 5 years ago • 3 comments

(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 avatar Nov 14 '19 10:11 spaghetticode

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.

spaghetticode avatar Nov 14 '19 11:11 spaghetticode

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?

rubenochiavone avatar Dec 01 '19 18:12 rubenochiavone

For what it's worth, the email regexp used in orders can now be configured: https://github.com/solidusio/solidus/blob/38ef1644f8ae03df8c83f2f28b0258669453722f/core/lib/spree/app_configuration.rb#L156 We should probably have solidus_auth_devise use the same one.

waiting-for-dev avatar Sep 07 '22 08:09 waiting-for-dev