review(suggestion): better code with reviews, normalize Champs::Email…

…Champ.value, simplier default strict validation activation

Co-authored-by: Colin Darie <colin@darie.eu>

app/models/champs/email_champ.rb

@@ -1,3 +1,5 @@
 class Champs::EmailChamp < Champs::TextChamp
+  include EmailSanitizableConcern
+  before_validation -> { sanitize_email(:value) }
   validates :value, format: { with: StrictEmailValidator::REGEXP }, if: :validate_champ_value?
 end

app/models/user.rb

@@ -37,7 +37,7 @@ class User < ApplicationRecord
   before_validation -> { sanitize_email(:email) }
   validate :does_not_merge_on_self, if: :requested_merge_into_id_changed?
 
-  before_validation :remove_devise_email_validator
+  before_validation :remove_devise_email_format_validator
   validates :email, strict_email: true
 
   def validate_password_complexity?
@@ -272,7 +272,9 @@ def link_invites!
     Invite.where(email: email).update_all(user_id: id)
   end
 
-  def remove_devise_email_validator
+  # we just want to remove the devise format validator
+  #   https://github.com/heartcombo/devise/blob/main/lib/devise/models/validatable.rb#L30
+  def remove_devise_email_format_validator
     _validators[:email]&.reject! { _1.is_a?(ActiveModel::Validations::FormatValidator) }
     _validate_callbacks.each do |callback|
       next if !callback.filter.is_a?(ActiveModel::Validations::FormatValidator)

app/validators/strict_email_validator.rb

@@ -4,6 +4,7 @@ class StrictEmailValidator < ActiveModel::EachValidator
   #   but we want more, we want to ensure it's a domain with extension
   #   so we append \.[A-Za-z]{2,}
   REGEXP = /\A[^@\s]+@[^@\s\.]+\.[^@\s]{2,}\z/
+  DATE_SINCE_STRICT_EMAIL_VALIDATION = Date.parse(ENV.fetch('STRICT_EMAIL_VALIDATION_STARTS_ON')) rescue 0
 
   def validate_each(record, attribute, value)
     if value.present? && !regexp_for(record).match?(value)
@@ -19,19 +20,13 @@ def regexp_for(record)
     end
   end
 
-  def self.elligible_to_new_validation?(record)
+  def self.eligible_to_new_validation?(record)
     return false if !strict_validation_enabled?
-    return false if (record.created_at || Time.zone.now) < date_since_strict_email_validation
+    return false if (record.created_at || Time.zone.now) < DATE_SINCE_STRICT_EMAIL_VALIDATION
     true
   end
 
   def self.strict_validation_enabled?
-    ENV.key?('STRICT_EMAIL_VALIDATION_STARTS_AT')
-  end
-
-  def self.date_since_strict_email_validation
-    DateTime.parse(ENV['STRICT_EMAIL_VALIDATION_STARTS_AT'])
-  rescue
-    DateTime.new(1789, 5, 5, 0, 0) # french revolution, ds was not yet launched
+    ENV.key?('STRICT_EMAIL_VALIDATION_STARTS_ON')
   end
 end

spec/models/champs/email_champ_spec.rb

@@ -3,7 +3,9 @@
     let(:now) { Time.zone.now }
     let(:before) { now + 1.day }
     let(:after) { now + 1.day }
-    subject { build(:champ_email, value: value).valid?(:validate_champ_value) }
+    let(:champ) { build(:champ_email, value: value) }
+
+    subject { champ.valid?(:validate_champ_value) }
 
     context 'when value is username' do
       let(:value) { 'username' }
@@ -31,5 +33,13 @@
       let(:value) { 'username@mailserver.domain' }
       it { is_expected.to be_truthy }
     end
+
+    context 'when value contains white spaces plus a standard email' do
+      let(:value) { "\r\n\t username@mailserver.domain\r\n\t " }
+      it { is_expected.to be_truthy }
+      it 'normalize value' do
+        expect { subject }.to change { champ.value }.from(value).to('username@mailserver.domain')
+      end
+    end
   end
 end

spec/models/user_spec.rb

@@ -534,20 +534,20 @@
       it_behaves_like "validation of users.email was flacky"
     end
 
-    context "record.created_at < ENV['STRICT_EMAIL_VALIDATION_STARTS_AT']" do
+    context "record.created_at < ENV['STRICT_EMAIL_VALIDATION_STARTS_ON']" do
       let(:user) { build(:user, email: email, created_at: before) }
       before do
         allow(StrictEmailValidator).to receive(:strict_validation_enabled?).and_return(true).at_least(1)
-        allow(StrictEmailValidator).to receive(:date_since_strict_email_validation).and_return(now).at_least(1)
+        stub_const("StrictEmailValidator::DATE_SINCE_STRICT_EMAIL_VALIDATION", now)
       end
       it_behaves_like "validation of users.email was flacky"
     end
 
-    context "record.created_at > ENV['STRICT_EMAIL_VALIDATION_STARTS_AT']" do
+    context "record.created_at > ENV['STRICT_EMAIL_VALIDATION_STARTS_ON']" do
       let(:user) { build(:user, email: email, created_at: after) }
       before do
         allow(StrictEmailValidator).to receive(:strict_validation_enabled?).and_return(true).at_least(1)
-        allow(StrictEmailValidator).to receive(:date_since_strict_email_validation).and_return(now).at_least(1)
+        stub_const("StrictEmailValidator::DATE_SINCE_STRICT_EMAIL_VALIDATION", now)
       end
       context 'when value is username' do
         let(:email) { 'username' }