diff --git a/app/events/oidc/proxy_changed_event.rb b/app/events/oidc/proxy_changed_event.rb index 9415a6f6b4..4a713c4f00 100644 --- a/app/events/oidc/proxy_changed_event.rb +++ b/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 diff --git a/app/lib/authentication/by_password.rb b/app/lib/authentication/by_password.rb index a62559b59f..ff3e3329b8 100644 --- a/app/lib/authentication/by_password.rb +++ b/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 diff --git a/app/lib/backend/model_extensions/cinstance.rb b/app/lib/backend/model_extensions/cinstance.rb index 3b916a9bf5..60a7fb1d7e 100644 --- a/app/lib/backend/model_extensions/cinstance.rb +++ b/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) diff --git a/app/lib/backend/model_extensions/provider.rb b/app/lib/backend/model_extensions/provider.rb index 33a4a91cec..fdefd70b0a 100644 --- a/app/lib/backend/model_extensions/provider.rb +++ b/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 diff --git a/app/lib/backend/model_extensions/usage_limit.rb b/app/lib/backend/model_extensions/usage_limit.rb index 77f6ba31b5..38fd19e9d7 100644 --- a/app/lib/backend/model_extensions/usage_limit.rb +++ b/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 diff --git a/app/lib/permalink_fu.rb b/app/lib/permalink_fu.rb index d1589b542a..52034f9789 100644 --- a/app/lib/permalink_fu.rb +++ b/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 diff --git a/app/lib/redhat_customer_portal_support.rb b/app/lib/redhat_customer_portal_support.rb index 90b8d87af2..c349c6f733 100644 --- a/app/lib/redhat_customer_portal_support.rb +++ b/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 diff --git a/app/models/account/billing.rb b/app/models/account/billing.rb index e2d0c25a45..36d7129cd3 100644 --- a/app/models/account/billing.rb +++ b/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 diff --git a/app/models/account/credit_card.rb b/app/models/account/credit_card.rb index 0509ab9ecb..c6aed15cd5 100644 --- a/app/models/account/credit_card.rb +++ b/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? diff --git a/app/models/account/provider_domains.rb b/app/models/account/provider_domains.rb index fdc53380d5..8c62616e3f 100644 --- a/app/models/account/provider_domains.rb +++ b/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 diff --git a/app/models/cinstance.rb b/app/models/cinstance.rb index 07eb55fe21..936e80cee8 100644 --- a/app/models/cinstance.rb +++ b/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 diff --git a/app/models/cms/builtin.rb b/app/models/cms/builtin.rb index 2c462da600..cb9bfdc566 100644 --- a/app/models/cms/builtin.rb +++ b/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 diff --git a/app/models/contract.rb b/app/models/contract.rb index 21e84e0c12..d90d3171a9 100644 --- a/app/models/contract.rb +++ b/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) diff --git a/app/models/invoice.rb b/app/models/invoice.rb index eca76857a2..6384144d41 100644 --- a/app/models/invoice.rb +++ b/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 diff --git a/app/models/payment_detail.rb b/app/models/payment_detail.rb index 87779b60f8..40b9cdb2f8 100644 --- a/app/models/payment_detail.rb +++ b/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 diff --git a/app/models/proxy.rb b/app/models/proxy.rb index c3f171ce2d..1a80198a16 100644 --- a/app/models/proxy.rb +++ b/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 diff --git a/app/models/service.rb b/app/models/service.rb index 795e26f022..fdc2644c5d 100644 --- a/app/models/service.rb +++ b/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) diff --git a/app/models/web_hook/event.rb b/app/models/web_hook/event.rb index 1f44611a94..59242f20a0 100644 --- a/app/models/web_hook/event.rb +++ b/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' diff --git a/config/initializers/previously_changed.rb b/config/initializers/previously_changed.rb deleted file mode 100644 index 1b51350132..0000000000 --- a/config/initializers/previously_changed.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -ActiveSupport.on_load(:active_record) do - -require 'previously_changed' - -ActiveRecord::Base.send(:include, PreviouslyChanged) - -end diff --git a/lib/previously_changed.rb b/lib/previously_changed.rb deleted file mode 100644 index 820bedfa37..0000000000 --- a/lib/previously_changed.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -module PreviouslyChanged - extend ActiveSupport::Concern - - def previously_changed?(attribute) - Array(previous_changes[attribute.to_s]).any? - end -end diff --git a/lib/tasks/proxy.rake b/lib/tasks/proxy.rake index 99346a9ca0..e97468423d 100644 --- a/lib/tasks/proxy.rake +++ b/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] diff --git a/test/events/oidc/proxy_changed_event_test.rb b/test/events/oidc/proxy_changed_event_test.rb index bcfaa29fe8..592310bfed 100644 --- a/test/events/oidc/proxy_changed_event_test.rb +++ b/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 diff --git a/test/unit/account/provider_domains_test.rb b/test/unit/account/provider_domains_test.rb index a5e969163b..bbe17687cb 100644 --- a/test/unit/account/provider_domains_test.rb +++ b/test/unit/account/provider_domains_test.rb @@ -6,7 +6,7 @@ class Account::ProviderDomainsTest < ActiveSupport::TestCase test '#domain must be downcase' do account_one = FactoryBot.create(:simple_provider) account_one.subdomain = 'FOO' - refute account_one.valid? + assert_not account_one.valid? end test '#domain must be unique' do @@ -16,7 +16,7 @@ class Account::ProviderDomainsTest < ActiveSupport::TestCase account_two = FactoryBot.build(:provider_account) account_two.domain = account_one.domain - refute account_two.valid? + assert_not account_two.valid? assert account_two.errors[:subdomain].include? 'already taken' end @@ -27,7 +27,7 @@ class Account::ProviderDomainsTest < ActiveSupport::TestCase account_two = FactoryBot.build(:provider_account) account_two.domain = account_one.domain - refute account_two.valid? + assert_not account_two.valid? assert account_two.errors[:subdomain].include? 'already taken' account_one.destroy @@ -133,4 +133,32 @@ class Account::ProviderDomainsTest < ActiveSupport::TestCase assert_equal "master.#{ThreeScale.config.superdomain}", account.domain assert_equal "master.#{ThreeScale.config.superdomain}", account.self_domain end + + test '#publish_domain_events when domain changes' do + provider = FactoryBot.create(:simple_provider) + provider.expects(:publish_domain_events).once + + provider.domain = 'new.example.com' + provider.save + + provider.org_name = 'Do Not Change Domain Again' + provider.save + + provider.domain = 'new.example.com' + provider.save + end + + test '#publish_domain_events when subdomain changes' do + provider = FactoryBot.create(:simple_provider) + provider.expects(:publish_domain_events).once + + provider.domain = 'new.example.com' + provider.save + + provider.org_name = 'Do Not Change Domain Again' + provider.save + + provider.domain = 'new.example.com' + provider.save + end end diff --git a/test/unit/backend/model_extensions/usage_limit_test.rb b/test/unit/backend/model_extensions/usage_limit_test.rb index 919348f87f..be26c89169 100644 --- a/test/unit/backend/model_extensions/usage_limit_test.rb +++ b/test/unit/backend/model_extensions/usage_limit_test.rb @@ -107,7 +107,7 @@ def test_delete_backend_usage_limit usage_limit = FactoryBot.create(:usage_limit, :period => :month, :value => 2000) ThreeScale::Core::UsageLimit.expects(:delete).with( - usage_limit.service.backend_id, usage_limit.plan.id, usage_limit.metric.id, usage_limit.period) + usage_limit.service.backend_id, usage_limit.plan.id, usage_limit.metric.id, usage_limit.period.to_s) usage_limit.destroy end diff --git a/test/unit/proxy_test.rb b/test/unit/proxy_test.rb index 2e27e7d1bd..aba9aec075 100644 --- a/test/unit/proxy_test.rb +++ b/test/unit/proxy_test.rb @@ -659,6 +659,35 @@ def test_set_correct_endpoints @proxy.destroy end + test '#changed_for_autosave?' do + # no changes + assert_not @proxy.changed_for_autosave? + + # deployment option changed + @proxy.service.deployment_option = 'self_managed' + assert @proxy.changed_for_autosave? + end + + test '#deployment_option_changed?' do + service = FactoryBot.create(:simple_service, deployment_option: 'hosted') + proxy = FactoryBot.create(:proxy, service: service) + + assert_not proxy.saved_change_to_deployment_option? + assert_not proxy.will_save_change_to_deployment_option? + + proxy.service.deployment_option = 'hosted' + assert_not proxy.saved_change_to_deployment_option? + assert_not proxy.will_save_change_to_deployment_option? + + proxy.service.deployment_option = 'self_managed' + assert_not proxy.saved_change_to_deployment_option? + assert proxy.will_save_change_to_deployment_option? + + proxy.service.save! + assert proxy.saved_change_to_deployment_option? + assert_not proxy.will_save_change_to_deployment_option? + end + def analytics ThreeScale::Analytics::UserTracking.any_instance end diff --git a/test/unit/service_test.rb b/test/unit/service_test.rb index 296fcda168..b363cc4065 100644 --- a/test/unit/service_test.rb +++ b/test/unit/service_test.rb @@ -532,12 +532,6 @@ class AsynchronousDeletionOfService < ActiveSupport::TestCase end end - test 'don not call update endpoint if deployment option has not changed' do - service = FactoryBot.create(:simple_service, deployment_option: 'hosted') - service.expects(:deployment_option_changed).times(0) - service.update(deployment_option: 'hosted') - end - test 'call update endpoint on proxy after update when deployment option has changed' do service = FactoryBot.create(:simple_service, deployment_option: 'hosted')