Update deprecated usage of attribute_changed? in callbacks (#2929)

Co-authored-by: Aleksandar N. Kostadinov <[email protected]>

app/events/oidc/proxy_changed_event.rb

@@ -21,6 +21,8 @@ def self.create(proxy)
   def self.valid?(proxy)
     service = proxy.try(:service)
     return unless service
-    service.backend_version.oauth? || service.backend_version_change&.include?('oauth')
+
+    # TODO: second assertion is probably useless, or not. Revisit this in the future once we've seen this is safe. See https://github.com/3scale/porta/pull/2929/files#r846345372
+    service.backend_version.oauth? || service.backend_version_change_to_be_saved&.include?('oauth') || service.saved_change_to_backend_version&.include?('oauth')
   end
 end

app/lib/authentication/by_password.rb

@@ -39,7 +39,7 @@ def transparently_migrate_password(unencrypted_password)
       end
 
       def just_changed_password?
-        previous_changes.key?('password_digest') || super
+        saved_change_to_password_digest? || super
       end
 
       private
@@ -113,7 +113,7 @@ def password_required?
       end
 
       def just_changed_password?
-        previous_changes.key?('crypted_password')
+        saved_change_to_crypted_password?
       end
 
       private

app/lib/backend/model_extensions/cinstance.rb

@@ -44,9 +44,9 @@ def delete_backend_application
       end
 
       def update_backend_user_key_to_application_id_mapping
-        user_key_was, current_user_key = previous_changes[:user_key]
+        user_key_was, current_user_key = saved_change_to_user_key
 
-        if previously_changed?(:user_key) && service.id.present? && user_key_was.present?
+        if saved_change_to_user_key? && service.id.present? && user_key_was.present?
           ThreeScale::Core::Application.delete_id_by_key(service.backend_id, user_key_was)
         end
 
@@ -78,8 +78,8 @@ def set_application_id
       end
 
       def update_provider_backend_service_if_user_key_changed
-        if previously_changed?(:user_key)
-          user_key_was, user_key = previous_changes[:user_key]
+        if saved_change_to_user_key?
+          user_key_was, user_key = saved_change_to_user_key
 
           if user_account && user_account.provider? && user_key_was.present? && user_account.services.present?
             ThreeScale::Core::Service.change_provider_key!(user_key_was, user_key)

app/lib/backend/model_extensions/provider.rb

@@ -4,12 +4,11 @@ module Provider
       extend ActiveSupport::Concern
 
       included do
-        after_commit :update_backend_default_service_id, :if => :provider?, :unless => :master?
+        after_commit :update_backend_default_service_id, if: -> { provider? && saved_change_to_default_service_id? }, unless: :master?
       end
 
       def update_backend_default_service_id
         return if destroyed?
-        return unless previously_changed?(:default_service_id)
         services.default.update_backend_service
       end
     end

app/lib/backend/model_extensions/usage_limit.rb

@@ -27,7 +27,7 @@ def update_backend_usage_limit
 
       def delete_backend_usage_limit
         if plan_and_service?
-          original_period = previously_changed?(:period) ? period_previous_change.compact.first : period
+          original_period = saved_change_to_period? ? saved_change_to_period.compact.first : period
           ThreeScale::Core::UsageLimit.delete(service.backend_id, plan.backend_id, metric_id, original_period)
         end
 

app/lib/permalink_fu.rb

@@ -25,7 +25,8 @@ def permalink(attr_name, options = {})
   private
 
   def create_unique_permalink
-    return if permalink.present? && !permalink_changed?
+    return if permalink.present? && !will_save_change_to_permalink?
+
     base_permalink = build_permalink_from_attribute
     count = where_match_permalink_with_conditions(base_permalink).count
     self.permalink = count.positive? ? "#{base_permalink}-#{count + 1}" : base_permalink

app/lib/redhat_customer_portal_support.rb

@@ -14,18 +14,18 @@ def redhat_customer_authentication_provider
   end
 
   def redhat_account_recently_verified?
-    extra_fields_change = previous_changes['extra_fields']
+    extra_fields_change = saved_change_to_extra_fields
 
     return false unless extra_fields_change
 
-    verified_by_was = extra_fields_change.first['red_hat_account_verified_by']
-    verified_by = extra_fields_change.last['red_hat_account_verified_by']
+    verified_by_was = extra_fields_change.first[:red_hat_account_verified_by]
+    verified_by = extra_fields_change.last[:red_hat_account_verified_by]
 
     verified_by_was.blank? && verified_by.present?
   end
 
   def recently_suspended?
-    previous_changes['state'] && suspended?
+    saved_change_to_attribute(:state) && suspended?
   end
 
   private

app/models/account/billing.rb

@@ -5,7 +5,7 @@ module Account::Billing
 
   included do
     has_many :invoices, :foreign_key => 'buyer_account_id'
-    after_save :update_invoices_vat_rates
+    after_save :update_invoices_vat_rates, if: :saved_change_to_vat_rate?
     before_destroy :check_unresolved_invoices
   end
 
@@ -33,9 +33,7 @@ def billable_contracts_with_trial_period_expired(now)
   protected
 
   def update_invoices_vat_rates
-    if previously_changed?(:vat_rate) || changes.key?(:vat_rate)
-      self.invoices.not_frozen.reorder('').update_all(:vat_rate => self.vat_rate)
-    end
+    invoices.not_frozen.reorder('').update_all(vat_rate: vat_rate)
   end
 
   # Will prevent the buyer from destroying if there are unresolved

app/models/account/credit_card.rb

@@ -1,5 +1,7 @@
+# frozen_string_literal: true
+
 # TODO: will become a CreditCard model by itself soon
-module Account::CreditCard
+module Account::CreditCard # rubocop:disable Metrics/ModuleLength(RuboCop)
   extend ActiveSupport::Concern
 
   included do
@@ -113,9 +115,9 @@ def unstore_credit_card!
   end
 
   def notify_credit_card_change
-    credit_card_changes = previous_changes.slice(credit_card_stored_attribute,
-                                     :credit_card_partial_number,
-                                     :credit_card_expires_on)
+    credit_card_changes = saved_changes.slice(credit_card_stored_attribute,
+                                              :credit_card_partial_number,
+                                              :credit_card_expires_on)
 
     return unless credit_card_changes.present?
 

app/models/account/provider_domains.rb

@@ -1,9 +1,9 @@
 # frozen_string_literal: true
 
-module Account::ProviderDomains
+module Account::ProviderDomains # rubocop:disable Metrics/ModuleLength
   extend ActiveSupport::Concern
 
-  included do
+  included do # rubocop:disable Metrics/BlockLength
     include ThreeScale::DomainSubstitution::Account
 
     with_options :if => :validate_domains? do |provider|
@@ -78,7 +78,7 @@ def same_domain(domain)
   end
 
   def domains_changed?
-    attribute_changed?(:domain) || attribute_changed?(:self_domain)
+    saved_change_to_domain? || saved_change_to_self_domain?
   end
 
   def publish_domain_events

app/models/cinstance.rb

@@ -257,7 +257,7 @@ def change_user_key!
   end
 
   def user_key_updated?
-    self.previous_changes.select { |a| a == "user_key"}.count > 0
+    saved_change_to_user_key?
   end
 
   def push_webhook_key_updated

app/models/cms/builtin.rb

@@ -235,7 +235,7 @@ def system_name_rules
         unless self.class.system_name_whitelist.include?(system_name)
           errors.add(:system_name, :not_reserved)
         end
-      elsif system_name_changed? && attribute_was('system_name') != system_name
+      elsif will_save_change_to_system_name?
         errors.add(:system_name, :cannot_be_changed)
       end
     end

app/models/contract.rb

@@ -20,7 +20,7 @@ class Contract < ApplicationRecord
   include Logic::PlanChanges::Contract
 
   after_destroy :destroy_customized_plan
-  after_commit :notify_plan_changed
+  after_commit :notify_plan_changed, if: :saved_change_to_plan_id?
 
   belongs_to :plan
   validate   :correct_plan_subclass?
@@ -310,7 +310,7 @@ def update_counter_cache?(association_name)
   end
 
   def notify_plan_changed
-    if previously_changed?(:plan_id) && @old_plan
+    if @old_plan
       notify_observers(:bill_variable_for_plan_changed, @old_plan)
       notify_observers(:plan_changed)
 

app/models/invoice.rb

@@ -592,7 +592,7 @@ def set_friendly_id
   private :set_friendly_id
 
   def update_counter
-    return unless previous_changes.key?(:friendly_id)
+    return unless saved_change_to_friendly_id?
     counter.update_count(id_sufix.to_i)
   end
 

app/models/payment_detail.rb

@@ -77,6 +77,6 @@ def do_not_notify
   private
 
   def notify_credit_card_changes
-    CreditCardChangeNotifier.new(account, previous_changes).call
+    CreditCardChangeNotifier.new(account, saved_changes).call
   end
 end

app/models/proxy.rb

@@ -3,7 +3,7 @@
 require 'ipaddr'
 require 'resolv'
 
-class Proxy < ApplicationRecord
+class Proxy < ApplicationRecord # rubocop:disable Metrics/ClassLength
   include AfterCommitQueue
   include BackendApiLogic::ProxyExtension
   prepend BackendApiLogic::RoutingPolicy
@@ -106,7 +106,7 @@ class Proxy < ApplicationRecord
   after_save :publish_events
   before_destroy :publish_events
 
-  after_save :track_apicast_version_change, if: :apicast_configuration_driven_changed?
+  after_save :track_apicast_version_change, if: :saved_change_to_apicast_configuration_driven?
 
   alias_attribute :production_endpoint, :endpoint
   alias_attribute :staging_endpoint, :sandbox_endpoint
@@ -269,7 +269,7 @@ def self.credentials_collection
   end
 
   def set_correct_endpoints?
-    apicast_configuration_driven_changed? || new_record?
+    will_save_change_to_apicast_configuration_driven? || new_record?
   end
 
   def publish_events
@@ -278,15 +278,21 @@ def publish_events
     nil
   end
 
-  DEPLOYMENT_OPTION_CHANGED = ->(record) { record.changed_attributes.key?(:deployment_option) }
+  def will_save_change_to_deployment_option?
+    [self, service].any? do |record|
+      record.will_save_change_to_attribute?(:deployment_option)
+    end
+  end
 
-  def deployment_option_changed?
-    [ self, service ].any?(&DEPLOYMENT_OPTION_CHANGED)
+  def saved_change_to_deployment_option?
+    [self, service].any? do |record|
+      record.saved_change_to_attribute?(:deployment_option)
+    end
   end
 
   # We want to autosave when Service#deployment_option changed
   def changed_for_autosave?
-    deployment_option_changed? or super
+    will_save_change_to_deployment_option? or super
   end
 
   def self.config

app/models/service.rb

@@ -2,7 +2,7 @@
 
 require 'backend_client'
 
-class Service < ApplicationRecord
+class Service < ApplicationRecord # rubocop:disable Metrics/ClassLength
   include Searchable
   include Backend::ModelExtensions::Service
   include Logic::Contracting::Service
@@ -41,7 +41,7 @@ class Service < ApplicationRecord
   validates :kubernetes_service_link, length: {maximum: 255}
 
   after_create :create_default_metrics, :create_default_service_plan, :create_default_proxy
-  after_commit :update_notification_settings
+  after_commit :update_notification_settings, if: :saved_change_to_notification_settings?
 
   after_commit :create_and_publish_service_created_event, on: :create
   after_commit :create_and_publish_service_deleted_event, on: :destroy
@@ -491,7 +491,7 @@ def deployment_option=(value)
     super
   ensure
     # always set correct proxy endpoints when deployment option changes
-    (proxy || build_proxy).try(:set_correct_endpoints) if deployment_option_changed?
+    (proxy || build_proxy).try(:set_correct_endpoints) if will_save_change_to_deployment_option?
   end
 
   def backend_version=(backend_version)
@@ -538,7 +538,7 @@ def deleted_by_state_machine
   end
 
   def deleted_without_state_machine
-    if state_changed? && deleted? && !@deleted_by_state_machine
+    if saved_change_to_attribute?(:state) && deleted? && !@deleted_by_state_machine
       System::ErrorReporting.report_error('Service has been deleted without using State Machine')
     end
   end
@@ -558,8 +558,6 @@ def default_service_plan_state
   end
 
   def update_notification_settings
-    return unless previously_changed?(:notification_settings)
-
     current_alert_limits = alert_limits
 
     delete_alert_limits(current_alert_limits - notification_settings_levels)

app/models/web_hook/event.rb

@@ -144,7 +144,7 @@ def push_event?
     # :reek:NilCheck
     # That is fine as we want really to check for nil value in the changes
     def guess_event
-      resource_changes = @resource.previous_changes
+      resource_changes = @resource.previous_changes # TODO: use saved_changes instead?
       case
       when @resource.destroyed?
         'deleted'

lib/tasks/proxy.rake

@@ -33,11 +33,6 @@ namespace :proxy do
     end
   end
 
-  desc 'Fixing nil endpoint for hosted'
-  task :set_correct_endpoint_hosted => :environment do
-    Service.includes(:proxy).where(proxies: {endpoint: nil}, deployment_option: 'hosted').find_each(&:deployment_option_changed)
-  end
-
   desc 'Resets proxy config tracking object'
   task :reset_config_change_history, [:account_id] => :environment do |_, args|
     account_id = args[:account_id]

test/events/oidc/proxy_changed_event_test.rb

@@ -12,7 +12,7 @@ def setup
 
   def test_create_and_publish!
     proxy = FactoryBot.create(:simple_proxy, oidc_issuer_endpoint: 'http://example.com/auth/realm')
-    refute OIDC::ProxyChangedEvent.create_and_publish!(proxy), 'service is not oauth'
+    assert_not OIDC::ProxyChangedEvent.create_and_publish!(proxy), 'service is not oauth'
 
     proxy.service.backend_version = 'oauth'
     assert OIDC::ProxyChangedEvent.create_and_publish!(proxy),'event should be created for OAuth service'
@@ -31,14 +31,18 @@ def test_create
 
   test '#valid? when service has been deleted' do
     proxy = FactoryBot.create(:simple_proxy, oidc_issuer_endpoint: 'http://example.com/auth/realm')
+    assert_not OIDC::ProxyChangedEvent.valid?(proxy)
+
     proxy.service.backend_version = 'oauth'
     assert OIDC::ProxyChangedEvent.valid?(proxy)
 
     proxy.service.save
+    assert OIDC::ProxyChangedEvent.valid?(proxy)
+
     proxy.service.backend_version = "2"
     assert OIDC::ProxyChangedEvent.valid?(proxy)
 
     proxy.stubs(service: nil)
-    refute OIDC::ProxyChangedEvent.valid?(proxy)
+    assert_not OIDC::ProxyChangedEvent.valid?(proxy)
   end
 end