From 2f2e51e973153e21feef339531ee9e16d207e320 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Fri, 17 Mar 2023 10:17:50 -0300 Subject: [PATCH] Refactor dirty tracking conditionals for different versions We have an number of conditions due to how dirty tracking changed around Rails 5.1, that implement methods using one or another method call. I might need more of this for mongo upgrades based on an initial investigation, plus this makes the code really hard to reason about sometimes with these many conditionals. While I want to drop support for older versions of Rails soon, this centralization of dirty methods that are used by devise conditionally simplifies the usage considerably across the board, moves the version condition to a single place, and will make it easier to refactor later once we drop older Rails version by simply removing the `devise_*` versions of the methods, alongside the prefix on the method calls for the most part, since those methods follow the naming of the newer Rails versions. --- CHANGELOG.md | 1 + lib/devise.rb | 5 +- lib/devise/models.rb | 1 + lib/devise/models/confirmable.rb | 51 +++++------------ lib/devise/models/database_authenticatable.rb | 33 +++-------- lib/devise/models/recoverable.rb | 21 ++----- lib/devise/models/validatable.rb | 9 +-- lib/devise/orm_dirty_tracking.rb | 57 +++++++++++++++++++ test/rails_app/lib/shared_admin.rb | 6 +- 9 files changed, 90 insertions(+), 94 deletions(-) create mode 100644 lib/devise/orm_dirty_tracking.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 22b1b76358..73b834bb65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * enhancements * Allow resource class scopes to override the global configuration for `sign_in_after_reset_password` behaviour. [#5429](https://github.com/heartcombo/devise/pull/5429) [@mattr](https://github.com/mattr) + * Refactor conditional dirty tracking logic to a centralized module to simplify usage throughout the codebase. [#5575](https://github.com/heartcombo/devise/pull/5575) * bug fixes * Fix frozen string exception in validatable. [#5563](https://github.com/heartcombo/devise/pull/5563) [#5465](https://github.com/heartcombo/devise/pull/5465) [@mameier](https://github.com/mameier) diff --git a/lib/devise.rb b/lib/devise.rb index 1d9370cc4f..e0749eb824 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -13,6 +13,7 @@ module Devise autoload :Encryptor, 'devise/encryptor' autoload :FailureApp, 'devise/failure_app' autoload :OmniAuth, 'devise/omniauth' + autoload :OrmDirtyTracking, 'devise/orm_dirty_tracking' autoload :ParameterFilter, 'devise/parameter_filter' autoload :ParameterSanitizer, 'devise/parameter_sanitizer' autoload :TestHelpers, 'devise/test_helpers' @@ -307,10 +308,6 @@ module Test mattr_accessor :sign_in_after_change_password @@sign_in_after_change_password = true - def self.activerecord51? # :nodoc: - defined?(ActiveRecord) && ActiveRecord.gem_version >= Gem::Version.new("5.1.x") - end - # Default way to set up Devise. Run rails generate devise_install to create # a fresh initializer with all configuration values. def self.setup diff --git a/lib/devise/models.rb b/lib/devise/models.rb index 4d50fa2453..1dc5753b04 100644 --- a/lib/devise/models.rb +++ b/lib/devise/models.rb @@ -84,6 +84,7 @@ def devise(*modules) end devise_modules_hook! do + include Devise::OrmDirtyTracking include Devise::Models::Authenticatable selected_modules.each do |m| diff --git a/lib/devise/models/confirmable.rb b/lib/devise/models/confirmable.rb index 7faae516b6..0f74d07578 100644 --- a/lib/devise/models/confirmable.rb +++ b/lib/devise/models/confirmable.rb @@ -258,44 +258,23 @@ def generate_confirmation_token! generate_confirmation_token && save(validate: false) end - if Devise.activerecord51? - def postpone_email_change_until_confirmation_and_regenerate_confirmation_token - @reconfirmation_required = true - self.unconfirmed_email = self.email - self.email = self.email_in_database - self.confirmation_token = nil - generate_confirmation_token - end - else - def postpone_email_change_until_confirmation_and_regenerate_confirmation_token - @reconfirmation_required = true - self.unconfirmed_email = self.email - self.email = self.email_was - self.confirmation_token = nil - generate_confirmation_token - end + + def postpone_email_change_until_confirmation_and_regenerate_confirmation_token + @reconfirmation_required = true + self.unconfirmed_email = self.email + self.email = self.devise_email_in_database + self.confirmation_token = nil + generate_confirmation_token end - if Devise.activerecord51? - def postpone_email_change? - postpone = self.class.reconfirmable && - will_save_change_to_email? && - !@bypass_confirmation_postpone && - self.email.present? && - (!@skip_reconfirmation_in_callback || !self.email_in_database.nil?) - @bypass_confirmation_postpone = false - postpone - end - else - def postpone_email_change? - postpone = self.class.reconfirmable && - email_changed? && - !@bypass_confirmation_postpone && - self.email.present? && - (!@skip_reconfirmation_in_callback || !self.email_was.nil?) - @bypass_confirmation_postpone = false - postpone - end + def postpone_email_change? + postpone = self.class.reconfirmable && + devise_will_save_change_to_email? && + !@bypass_confirmation_postpone && + self.email.present? && + (!@skip_reconfirmation_in_callback || !self.devise_email_in_database.nil?) + @bypass_confirmation_postpone = false + postpone end def reconfirmation_required? diff --git a/lib/devise/models/database_authenticatable.rb b/lib/devise/models/database_authenticatable.rb index 8c0e22613d..fc6ad714e6 100644 --- a/lib/devise/models/database_authenticatable.rb +++ b/lib/devise/models/database_authenticatable.rb @@ -177,16 +177,9 @@ def authenticatable_salt encrypted_password[0,29] if encrypted_password end - if Devise.activerecord51? - # Send notification to user when email changes. - def send_email_changed_notification - send_devise_notification(:email_changed, to: email_before_last_save) - end - else - # Send notification to user when email changes. - def send_email_changed_notification - send_devise_notification(:email_changed, to: email_was) - end + # Send notification to user when email changes. + def send_email_changed_notification + send_devise_notification(:email_changed, to: devise_email_before_last_save) end # Send notification to user when password changes. @@ -205,24 +198,12 @@ def password_digest(password) Devise::Encryptor.digest(self.class, password) end - if Devise.activerecord51? - def send_email_changed_notification? - self.class.send_email_changed_notification && saved_change_to_email? && !@skip_email_changed_notification - end - else - def send_email_changed_notification? - self.class.send_email_changed_notification && email_changed? && !@skip_email_changed_notification - end + def send_email_changed_notification? + self.class.send_email_changed_notification && devise_saved_change_to_email? && !@skip_email_changed_notification end - if Devise.activerecord51? - def send_password_change_notification? - self.class.send_password_change_notification && saved_change_to_encrypted_password? && !@skip_password_change_notification - end - else - def send_password_change_notification? - self.class.send_password_change_notification && encrypted_password_changed? && !@skip_password_change_notification - end + def send_password_change_notification? + self.class.send_password_change_notification && devise_saved_change_to_encrypted_password? && !@skip_password_change_notification end module ClassMethods diff --git a/lib/devise/models/recoverable.rb b/lib/devise/models/recoverable.rb index 59f3a613d6..b17c42aae6 100644 --- a/lib/devise/models/recoverable.rb +++ b/lib/devise/models/recoverable.rb @@ -99,24 +99,13 @@ def send_reset_password_instructions_notification(token) send_devise_notification(:reset_password_instructions, token, {}) end - if Devise.activerecord51? - def clear_reset_password_token? - encrypted_password_changed = respond_to?(:will_save_change_to_encrypted_password?) && will_save_change_to_encrypted_password? - authentication_keys_changed = self.class.authentication_keys.any? do |attribute| - respond_to?("will_save_change_to_#{attribute}?") && send("will_save_change_to_#{attribute}?") - end - - authentication_keys_changed || encrypted_password_changed + def clear_reset_password_token? + encrypted_password_changed = devise_respond_to_and_will_save_change_to_attribute?(:encrypted_password) + authentication_keys_changed = self.class.authentication_keys.any? do |attribute| + devise_respond_to_and_will_save_change_to_attribute?(attribute) end - else - def clear_reset_password_token? - encrypted_password_changed = respond_to?(:encrypted_password_changed?) && encrypted_password_changed? - authentication_keys_changed = self.class.authentication_keys.any? do |attribute| - respond_to?("#{attribute}_changed?") && send("#{attribute}_changed?") - end - authentication_keys_changed || encrypted_password_changed - end + authentication_keys_changed || encrypted_password_changed end module ClassMethods diff --git a/lib/devise/models/validatable.rb b/lib/devise/models/validatable.rb index 5a190a7c36..1c22fb5fec 100644 --- a/lib/devise/models/validatable.rb +++ b/lib/devise/models/validatable.rb @@ -29,13 +29,8 @@ def self.included(base) base.class_eval do validates_presence_of :email, if: :email_required? - if Devise.activerecord51? - validates_uniqueness_of :email, allow_blank: true, case_sensitive: true, if: :will_save_change_to_email? - validates_format_of :email, with: email_regexp, allow_blank: true, if: :will_save_change_to_email? - else - validates_uniqueness_of :email, allow_blank: true, if: :email_changed? - validates_format_of :email, with: email_regexp, allow_blank: true, if: :email_changed? - end + validates_uniqueness_of :email, allow_blank: true, case_sensitive: true, if: :devise_will_save_change_to_email? + validates_format_of :email, with: email_regexp, allow_blank: true, if: :devise_will_save_change_to_email? validates_presence_of :password, if: :password_required? validates_confirmation_of :password, if: :password_required? diff --git a/lib/devise/orm_dirty_tracking.rb b/lib/devise/orm_dirty_tracking.rb new file mode 100644 index 0000000000..07391108fb --- /dev/null +++ b/lib/devise/orm_dirty_tracking.rb @@ -0,0 +1,57 @@ +module Devise + module OrmDirtyTracking # :nodoc: + def self.activerecord51? + defined?(ActiveRecord) && ActiveRecord.gem_version >= Gem::Version.new("5.1.x") + end + + if activerecord51? + def devise_email_before_last_save + email_before_last_save + end + + def devise_email_in_database + email_in_database + end + + def devise_saved_change_to_email? + saved_change_to_email? + end + + def devise_saved_change_to_encrypted_password? + saved_change_to_encrypted_password? + end + + def devise_will_save_change_to_email? + will_save_change_to_email? + end + + def devise_respond_to_and_will_save_change_to_attribute?(attribute) + respond_to?("will_save_change_to_#{attribute}?") && send("will_save_change_to_#{attribute}?") + end + else + def devise_email_before_last_save + email_was + end + + def devise_email_in_database + email_was + end + + def devise_saved_change_to_email? + email_changed? + end + + def devise_saved_change_to_encrypted_password? + encrypted_password_changed? + end + + def devise_will_save_change_to_email? + email_changed? + end + + def devise_respond_to_and_will_save_change_to_attribute?(attribute) + respond_to?("#{attribute}_changed?") && send("#{attribute}_changed?") + end + end + end +end diff --git a/test/rails_app/lib/shared_admin.rb b/test/rails_app/lib/shared_admin.rb index 3e6362a78d..374666ff52 100644 --- a/test/rails_app/lib/shared_admin.rb +++ b/test/rails_app/lib/shared_admin.rb @@ -10,11 +10,7 @@ module SharedAdmin allow_unconfirmed_access_for: 2.weeks, reconfirmable: true validates_length_of :reset_password_token, minimum: 3, allow_blank: true - if Devise::Test.rails51? - validates_uniqueness_of :email, allow_blank: true, if: :will_save_change_to_email? - else - validates_uniqueness_of :email, allow_blank: true, if: :email_changed? - end + validates_uniqueness_of :email, allow_blank: true, if: :devise_will_save_change_to_email? end def raw_confirmation_token