review(suggestion): better code with reviews, normalize Champs::EmailChamp.value, simplier default strict validation activation

Co-authored-by: Colin Darie <colin@darie.eu>
This commit is contained in:
Martin 2024-02-15 09:45:20 +01:00 committed by mfo
parent 5f77c0cd06
commit 8488e74bbb
7 changed files with 33 additions and 20 deletions

View file

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

View file

@ -1,4 +1,6 @@
class ContactInformation < ApplicationRecord class ContactInformation < ApplicationRecord
include EmailSanitizableConcern
belongs_to :groupe_instructeur belongs_to :groupe_instructeur
validates :nom, presence: { message: 'doit être renseigné' }, allow_nil: false validates :nom, presence: { message: 'doit être renseigné' }, allow_nil: false
@ -8,6 +10,7 @@ class ContactInformation < ApplicationRecord
validates :horaires, presence: { message: 'doivent être renseignés' }, allow_nil: false validates :horaires, presence: { message: 'doivent être renseignés' }, allow_nil: false
validates :adresse, presence: { message: 'doit être renseignée' }, allow_nil: false validates :adresse, presence: { message: 'doit être renseignée' }, allow_nil: false
validates :groupe_instructeur, presence: { message: 'doit être renseigné' }, allow_nil: false validates :groupe_instructeur, presence: { message: 'doit être renseigné' }, allow_nil: false
before_validation -> { sanitize_email(:email) }
def pretty_nom def pretty_nom
nom nom

View file

@ -37,8 +37,9 @@ class User < ApplicationRecord
before_validation -> { sanitize_email(:email) } before_validation -> { sanitize_email(:email) }
validate :does_not_merge_on_self, if: :requested_merge_into_id_changed? 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 # plug our custom validation a la devise (same options) https://github.com/heartcombo/devise/blob/main/lib/devise/models/validatable.rb#L30
validates :email, strict_email: true, allow_blank: true, if: :devise_will_save_change_to_email?
def validate_password_complexity? def validate_password_complexity?
administrateur? administrateur?
@ -272,7 +273,9 @@ class User < ApplicationRecord
Invite.where(email: email).update_all(user_id: id) Invite.where(email: email).update_all(user_id: id)
end 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) } _validators[:email]&.reject! { _1.is_a?(ActiveModel::Validations::FormatValidator) }
_validate_callbacks.each do |callback| _validate_callbacks.each do |callback|
next if !callback.filter.is_a?(ActiveModel::Validations::FormatValidator) next if !callback.filter.is_a?(ActiveModel::Validations::FormatValidator)

View file

@ -4,6 +4,7 @@ class StrictEmailValidator < ActiveModel::EachValidator
# but we want more, we want to ensure it's a domain with extension # but we want more, we want to ensure it's a domain with extension
# so we append \.[A-Za-z]{2,} # so we append \.[A-Za-z]{2,}
REGEXP = /\A[^@\s]+@[^@\s\.]+\.[^@\s]{2,}\z/ 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) def validate_each(record, attribute, value)
if value.present? && !regexp_for(record).match?(value) if value.present? && !regexp_for(record).match?(value)
@ -12,26 +13,20 @@ class StrictEmailValidator < ActiveModel::EachValidator
end end
def regexp_for(record) def regexp_for(record)
if StrictEmailValidator.elligible_to_new_validation?(record) if StrictEmailValidator.eligible_to_new_validation?(record)
REGEXP REGEXP
else else
Devise.email_regexp Devise.email_regexp
end end
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 !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 true
end end
def self.strict_validation_enabled? def self.strict_validation_enabled?
ENV.key?('STRICT_EMAIL_VALIDATION_STARTS_AT') ENV.key?('STRICT_EMAIL_VALIDATION_STARTS_ON')
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
end end
end end

View file

@ -78,13 +78,13 @@ RSpec.describe Avis, type: :model do
describe 'email validation' do describe 'email validation' do
let(:now_invalid_email) { "toto@tps" } let(:now_invalid_email) { "toto@tps" }
context 'new avis' do context 'new avis' do
before { allow(StrictEmailValidator).to receive(:elligible_to_new_validation?).and_return(true) } before { allow(StrictEmailValidator).to receive(:eligible_to_new_validation?).and_return(true) }
it { expect(build(:avis, email: now_invalid_email).valid?).to be_falsey } it { expect(build(:avis, email: now_invalid_email).valid?).to be_falsey }
it { expect(build(:avis, email: nil).valid?).to be_truthy } it { expect(build(:avis, email: nil).valid?).to be_truthy }
end end
context 'old avis' do context 'old avis' do
before { allow(StrictEmailValidator).to receive(:elligible_to_new_validation?).and_return(false) } before { allow(StrictEmailValidator).to receive(:eligible_to_new_validation?).and_return(false) }
it { expect(build(:avis, email: now_invalid_email).valid?).to be_truthy } it { expect(build(:avis, email: now_invalid_email).valid?).to be_truthy }
it { expect(build(:avis, email: nil).valid?).to be_truthy } it { expect(build(:avis, email: nil).valid?).to be_truthy }

View file

@ -3,7 +3,9 @@ describe Champs::EmailChamp do
let(:now) { Time.zone.now } let(:now) { Time.zone.now }
let(:before) { now + 1.day } let(:before) { now + 1.day }
let(:after) { 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 context 'when value is username' do
let(:value) { 'username' } let(:value) { 'username' }
@ -31,5 +33,13 @@ describe Champs::EmailChamp do
let(:value) { 'username@mailserver.domain' } let(:value) { 'username@mailserver.domain' }
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
end 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
end end

View file

@ -534,20 +534,20 @@ describe User, type: :model do
it_behaves_like "validation of users.email was flacky" it_behaves_like "validation of users.email was flacky"
end 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) } let(:user) { build(:user, email: email, created_at: before) }
before do before do
allow(StrictEmailValidator).to receive(:strict_validation_enabled?).and_return(true).at_least(1) 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 end
it_behaves_like "validation of users.email was flacky" it_behaves_like "validation of users.email was flacky"
end 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) } let(:user) { build(:user, email: email, created_at: after) }
before do before do
allow(StrictEmailValidator).to receive(:strict_validation_enabled?).and_return(true).at_least(1) 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 end
context 'when value is username' do context 'when value is username' do
let(:email) { 'username' } let(:email) { 'username' }