From 446d82d3742d25329e356af77ce2d21d057c777b Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 15 Feb 2024 09:45:20 +0100 Subject: [PATCH] review(suggestion): better code with reviews, normalize Champs::EmailChamp.value, simplier default strict validation activation Co-authored-by: Colin Darie --- app/models/champs/email_champ.rb | 2 ++ app/models/user.rb | 6 ++++-- app/validators/strict_email_validator.rb | 13 ++++--------- spec/models/champs/email_champ_spec.rb | 12 +++++++++++- spec/models/user_spec.rb | 8 ++++---- 5 files changed, 25 insertions(+), 16 deletions(-) diff --git a/app/models/champs/email_champ.rb b/app/models/champs/email_champ.rb index d22e9788755..d524626064d 100644 --- a/app/models/champs/email_champ.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 31b86a90b9f..ce0728d262a 100644 --- a/app/models/user.rb +++ b/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) diff --git a/app/validators/strict_email_validator.rb b/app/validators/strict_email_validator.rb index d56d4005d22..4d71c1f7f67 100644 --- a/app/validators/strict_email_validator.rb +++ b/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 diff --git a/spec/models/champs/email_champ_spec.rb b/spec/models/champs/email_champ_spec.rb index 5e8114e9dc5..f655dd4a7d7 100644 --- a/spec/models/champs/email_champ_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ba1e9e974d4..5a4b799077c 100644 --- a/spec/models/user_spec.rb +++ b/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' }