Merge branch '9932-fix-deprecated-attribute_changed' into 'master'

Remove deprecated usage of `attribute_changed?`

Closes #9932

See merge request gitlab-org/gitlab-ee!11407

app/models/commit_status.rb

@@ -142,7 +142,7 @@ class CommitStatus < ApplicationRecord
   end
 
   def locking_enabled?
-    status_changed?
+    will_save_change_to_status?
   end
 
   def before_sha

app/models/concerns/issuable.rb

@@ -117,7 +117,7 @@ module Issuable
     # We want to use optimistic lock for cases when only title or description are involved
     # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html
     def locking_enabled?
-      title_changed? || description_changed?
+      will_save_change_to_title? || will_save_change_to_description?
     end
 
     def allows_multiple_assignees?

app/models/concerns/storage/legacy_namespace.rb

@@ -13,20 +13,20 @@ module Storage
         raise Gitlab::UpdatePathError.new("Namespace #{name} (#{id}) cannot be moved because at least one project (e.g. #{proj_with_tags.name} (#{proj_with_tags.id})) has tags in container registry")
       end
 
-      parent_was = if parent_changed? && parent_id_before_last_save.present?
+      parent_was = if saved_change_to_parent? && parent_id_before_last_save.present?
                      Namespace.find(parent_id_before_last_save) # raise NotFound early if needed
                    end
 
       move_repositories
 
-      if parent_changed?
+      if saved_change_to_parent?
         former_parent_full_path = parent_was&.full_path
         parent_full_path = parent&.full_path
         Gitlab::UploadsTransfer.new.move_namespace(path, former_parent_full_path, parent_full_path)
         Gitlab::PagesTransfer.new.move_namespace(path, former_parent_full_path, parent_full_path)
       else
-        Gitlab::UploadsTransfer.new.rename_namespace(full_path_was, full_path)
-        Gitlab::PagesTransfer.new.rename_namespace(full_path_was, full_path)
+        Gitlab::UploadsTransfer.new.rename_namespace(full_path_before_last_save, full_path)
+        Gitlab::PagesTransfer.new.rename_namespace(full_path_before_last_save, full_path)
       end
 
       # If repositories moved successfully we need to
@@ -38,7 +38,7 @@ module Storage
         write_projects_repository_config
       rescue => e
         # Raise if development/test environment, else just notify Sentry
-        Gitlab::Sentry.track_exception(e, extra: { full_path_was: full_path_was, full_path: full_path, action: 'move_dir' })
+        Gitlab::Sentry.track_exception(e, extra: { full_path_before_last_save: full_path_before_last_save, full_path: full_path, action: 'move_dir' })
       end
 
       true # false would cancel later callbacks but not rollback
@@ -57,14 +57,14 @@ module Storage
       # Move the namespace directory in all storages used by member projects
       repository_storages.each do |repository_storage|
         # Ensure old directory exists before moving it
-        gitlab_shell.add_namespace(repository_storage, full_path_was)
+        gitlab_shell.add_namespace(repository_storage, full_path_before_last_save)
 
         # Ensure new directory exists before moving it (if there's a parent)
         gitlab_shell.add_namespace(repository_storage, parent.full_path) if parent
 
-        unless gitlab_shell.mv_namespace(repository_storage, full_path_was, full_path)
+        unless gitlab_shell.mv_namespace(repository_storage, full_path_before_last_save, full_path)
 
-          Rails.logger.error "Exception moving path #{repository_storage} from #{full_path_was} to #{full_path}"
+          Rails.logger.error "Exception moving path #{repository_storage} from #{full_path_before_last_save} to #{full_path}"
 
           # if we cannot move namespace directory we should rollback
           # db changes in order to prevent out of sync between db and fs

app/models/merge_request_diff.rb

@@ -134,7 +134,7 @@ class MergeRequestDiff < ApplicationRecord
   # It allows you to override variables like head_commit_sha before getting diff.
   after_create :save_git_content, unless: :importing?
 
-  after_save :update_external_diff_store, if: -> { !importing? && external_diff_changed? }
+  after_save :update_external_diff_store, if: -> { !importing? && saved_change_to_external_diff? }
 
   def self.find_by_diff_refs(diff_refs)
     find_by(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha)
@@ -154,7 +154,14 @@ class MergeRequestDiff < ApplicationRecord
     ensure_commit_shas
     save_commits
     save_diffs
+
+    # Another set of `after_save` hooks will be called here when we update the record
     save
+    # We need to reset so that dirty tracking is reset when running the original set
+    # of `after_save` hooks that come after this 'after_create` hook. Otherwise, the
+    # hooks that run when an attribute was changed are run twice.
+    reset
+
     keep_around_commits
   end
 
@@ -348,7 +355,7 @@ class MergeRequestDiff < ApplicationRecord
       has_attribute?(:external_diff_store)
   end
 
-  def external_diff_changed?
+  def saved_change_to_external_diff?
     super if has_attribute?(:external_diff)
   end
 

app/models/namespace.rb

@@ -63,7 +63,7 @@ class Namespace < ApplicationRecord
 
   # Legacy Storage specific hooks
 
-  after_update :move_dir, if: :path_or_parent_changed?
+  after_update :move_dir, if: :saved_change_to_path_or_parent?
   before_destroy(prepend: true) { prepare_for_destroy }
   after_destroy :rm_dir
 
@@ -144,7 +144,7 @@ class Namespace < ApplicationRecord
 
   def send_update_instructions
     projects.each do |project|
-      project.send_move_instructions("#{full_path_was}/#{project.path}")
+      project.send_move_instructions("#{full_path_before_last_save}/#{project.path}")
     end
   end
 
@@ -229,10 +229,6 @@ class Namespace < ApplicationRecord
     [owner_id]
   end
 
-  def parent_changed?
-    parent_id_changed?
-  end
-
   # Includes projects from this namespace and projects from all subgroups
   # that belongs to this namespace
   def all_projects
@@ -262,12 +258,12 @@ class Namespace < ApplicationRecord
     false
   end
 
-  def full_path_was
-    if parent_id_was.nil?
-      path_was
+  def full_path_before_last_save
+    if parent_id_before_last_save.nil?
+      path_before_last_save
     else
-      previous_parent = Group.find_by(id: parent_id_was)
-      previous_parent.full_path + '/' + path_was
+      previous_parent = Group.find_by(id: parent_id_before_last_save)
+      previous_parent.full_path + '/' + path_before_last_save
     end
   end
 
@@ -293,7 +289,15 @@ class Namespace < ApplicationRecord
 
   private
 
-  def path_or_parent_changed?
+  def parent_changed?
+    parent_id_changed?
+  end
+
+  def saved_change_to_parent?
+    saved_change_to_parent_id?
+  end
+
+  def saved_change_to_path_or_parent?
     saved_change_to_path? || saved_change_to_parent_id?
   end
 

app/models/pages_domain.rb

@@ -26,7 +26,7 @@ class PagesDomain < ApplicationRecord
 
   after_initialize :set_verification_code
   after_create :update_daemon
-  after_update :update_daemon, if: :pages_config_changed?
+  after_update :update_daemon, if: :saved_change_to_pages_config?
   after_destroy :update_daemon
 
   scope :enabled, -> { where('enabled_until >= ?', Time.now ) }
@@ -148,7 +148,7 @@ class PagesDomain < ApplicationRecord
   end
   # rubocop: enable CodeReuse/ServiceClass
 
-  def pages_config_changed?
+  def saved_change_to_pages_config?
     saved_change_to_project_id? ||
       saved_change_to_domain? ||
       saved_change_to_certificate? ||

app/models/project.rb

@@ -1912,8 +1912,8 @@ class Project < ApplicationRecord
     false
   end
 
-  def full_path_was
-    File.join(namespace.full_path, previous_changes['path'].first)
+  def full_path_before_last_save
+    File.join(namespace.full_path, path_before_last_save)
   end
 
   alias_method :name_with_namespace, :full_name

app/models/remote_mirror.rb

@@ -22,8 +22,8 @@ class RemoteMirror < ApplicationRecord
   before_save :set_new_remote_name, if: :mirror_url_changed?
 
   after_save :set_override_remote_mirror_available, unless: -> { Gitlab::CurrentSettings.current_application_settings.mirror_available }
-  after_save :refresh_remote, if: :mirror_url_changed?
-  after_update :reset_fields, if: :mirror_url_changed?
+  after_save :refresh_remote, if: :saved_change_to_mirror_url?
+  after_update :reset_fields, if: :saved_change_to_mirror_url?
 
   after_commit :remove_remote, on: :destroy
 
@@ -265,6 +265,10 @@ class RemoteMirror < ApplicationRecord
   def mirror_url_changed?
     url_changed? || credentials_changed?
   end
+
+  def saved_change_to_mirror_url?
+    saved_change_to_url? || saved_change_to_credentials?
+  end
 end
 
 RemoteMirror.prepend(EE::RemoteMirror)

app/models/storage/legacy_project.rb

@@ -30,7 +30,7 @@ module Storage
     end
 
     def rename_repo(old_full_path: nil, new_full_path: nil)
-      old_full_path ||= project.full_path_was
+      old_full_path ||= project.full_path_before_last_save
       new_full_path ||= project.build_full_path
 
       if gitlab_shell.mv_repository(repository_storage, old_full_path, new_full_path)

app/services/projects/update_service.rb

@@ -79,10 +79,7 @@ module Projects
     end
 
     def after_rename_service(project)
-      # The path slug the project was using, before the rename took place.
-      path_before = project.previous_changes['path'].first
-
-      AfterRenameService.new(project, path_before: path_before, full_path_before: project.full_path_was)
+      AfterRenameService.new(project, path_before: project.path_before_last_save, full_path_before: project.full_path_before_last_save)
     end
 
     def changing_pages_related_config?

app/services/system_hooks_service.rb

@@ -47,7 +47,7 @@ class SystemHooksService
 
       case event
       when :rename
-        data[:old_username] = model.username_was
+        data[:old_username] = model.username_before_last_save
       when :failed_login
         data[:state] = model.state
       end
@@ -58,8 +58,8 @@ class SystemHooksService
 
       if event == :rename
         data.merge!(
-          old_path: model.path_was,
-          old_full_path: model.full_path_was
+          old_path: model.path_before_last_save,
+          old_full_path: model.full_path_before_last_save
         )
       end
     when GroupMember

app/uploaders/object_storage.rb

@@ -117,7 +117,7 @@ module ObjectStorage
 
         next unless uploader
         next unless uploader.exists?
-        next unless send(:"#{mounted_as}_changed?") # rubocop:disable GitlabSecurity/PublicSend
+        next unless send(:"saved_change_to_#{mounted_as}?") # rubocop:disable GitlabSecurity/PublicSend
 
         mount
       end.keys

db/migrate/20170503140202_turn_nested_groups_into_regular_groups_for_mysql.rb

@@ -46,13 +46,6 @@ class TurnNestedGroupsIntoRegularGroupsForMysql < ActiveRecord::Migration[4.2]
 
       bulk_insert_members(rows)
 
-      # This method relies on the parent to determine the proper path.
-      # Because we reset "parent_id" this method will not return the right path
-      # when moving namespaces.
-      full_path_was = namespace.send(:full_path_was)
-
-      namespace.define_singleton_method(:full_path_was) { full_path_was }
-
       namespace.update!(parent_id: nil, path: new_path_for(namespace))
     end
   end

ee/app/models/ee/ci/build.rb

@@ -30,7 +30,7 @@ module EE
       end
 
       def stick_build_if_status_changed
-        return unless status_changed?
+        return unless saved_change_to_status?
         return unless running?
 
         ::Gitlab::Database::LoadBalancing::Sticking.stick(:build, id)

ee/app/models/ee/namespace.rb

@@ -88,7 +88,7 @@ module EE
     end
 
     def old_path_with_namespace_for(project)
-      project.full_path.sub(/\A#{Regexp.escape(full_path)}/, full_path_was)
+      project.full_path.sub(/\A#{Regexp.escape(full_path)}/, full_path_before_last_save)
     end
 
     # This makes the feature disabled by default, in contrary to how

ee/app/models/ee/project.rb

@@ -373,7 +373,7 @@ module EE
 
     def import_url_updated?
       # check if import_url has been updated and it's not just the first assignment
-      import_url_changed? && changes['import_url'].first
+      saved_change_to_import_url? && saved_changes['import_url'].first
     end
 
     def remove_mirror_repository_reference

ee/app/models/operations/feature_flag.rb

@@ -97,7 +97,7 @@ module Operations
     end
 
     def update_default_scope
-      default_scope.update(active: self.active) if self.active_changed?
+      default_scope.update(active: self.active) if saved_change_to_active?
     end
   end
 end

ee/app/models/packages/package_file.rb

@@ -11,7 +11,7 @@ class Packages::PackageFile < ApplicationRecord
 
   mount_uploader :file, Packages::PackageFileUploader
 
-  after_save :update_file_store, if: :file_changed?
+  after_save :update_file_store, if: :saved_change_to_file?
 
   def update_file_store
     # The file.object_store is set during `uploader.store!`

ee/app/services/ee/milestones/update_service.rb

@@ -10,7 +10,7 @@ module EE
       def execute(milestone)
         super
 
-        if dates_changed?(milestone)
+        if saved_change_to_dates?(milestone)
           ::Epic.update_start_and_due_dates(
             ::Epic.joins(:issues).where(issues: { milestone_id: milestone.id })
           )
@@ -22,9 +22,8 @@ module EE
 
       private
 
-      def dates_changed?(milestone)
-        changes = milestone.previous_changes
-        changes.include?(:start_date) || changes.include?(:due_date)
+      def saved_change_to_dates?(milestone)
+        milestone.saved_change_to_start_date? || milestone.saved_change_to_due_date?
       end
     end
   end

ee/app/services/epics/update_service.rb

@@ -16,7 +16,7 @@ module Epics
 
       update_task_event(epic) || update(epic)
 
-      if have_epic_dates_changed?(epic)
+      if saved_change_to_epic_dates?(epic)
         epic.update_start_and_due_dates
         epic.reset
       end
@@ -43,8 +43,8 @@ module Epics
 
     private
 
-    def have_epic_dates_changed?(epic)
-      (epic.previous_changes.keys.map(&:to_sym) & EPIC_DATE_FIELDS).present?
+    def saved_change_to_epic_dates?(epic)
+      (epic.saved_changes.keys.map(&:to_sym) & EPIC_DATE_FIELDS).present?
     end
   end
 end

ee/spec/models/namespace_spec.rb

@@ -137,18 +137,18 @@ describe Namespace do
       let!(:project_legacy) { create(:project_empty_repo, :legacy_storage, namespace: parent_group) }
       let!(:project_child_hashed) { create(:project, namespace: child_group) }
       let!(:project_child_legacy) { create(:project_empty_repo, :legacy_storage, namespace: child_group) }
-      let!(:full_path_was) { "#{parent_group.full_path}_old" }
+      let!(:full_path_before_last_save) { "#{parent_group.full_path}_old" }
 
       before do
         new_path = parent_group.full_path
 
         allow(parent_group).to receive(:gitlab_shell).and_return(gitlab_shell)
         allow(parent_group).to receive(:path_changed?).and_return(true)
-        allow(parent_group).to receive(:full_path_was).and_return(full_path_was)
+        allow(parent_group).to receive(:full_path_before_last_save).and_return(full_path_before_last_save)
         allow(parent_group).to receive(:full_path).and_return(new_path)
 
         allow(gitlab_shell).to receive(:mv_namespace)
-          .with(project_legacy.repository_storage, full_path_was, new_path)
+          .with(project_legacy.repository_storage, full_path_before_last_save, new_path)
           .and_return(true)
 
         stub_current_geo_node(primary)
@@ -163,9 +163,9 @@ describe Namespace do
 
         actual = Geo::RepositoryRenamedEvent.last(3).map(&:old_path_with_namespace)
         expected = %W[
-          #{full_path_was}/#{project_legacy.path}
-          #{full_path_was}/child/#{project_child_hashed.path}
-          #{full_path_was}/child/#{project_child_legacy.path}
+          #{full_path_before_last_save}/#{project_legacy.path}
+          #{full_path_before_last_save}/child/#{project_child_hashed.path}
+          #{full_path_before_last_save}/child/#{project_child_legacy.path}
         ]
 
         expect(actual).to match_array(expected)

spec/models/commit_status_spec.rb

@@ -449,7 +449,7 @@ describe CommitStatus do
       end
 
       it "lock" do
-        is_expected.to be true
+        is_expected.to be_truthy
       end
 
       it "raise exception when trying to update" do
@@ -463,7 +463,7 @@ describe CommitStatus do
       end
 
       it "do not lock" do
-        is_expected.to be false
+        is_expected.to be_falsey
       end
 
       it "save correctly" do

spec/models/namespace_spec.rb

@@ -743,22 +743,25 @@ describe Namespace do
     end
   end
 
-  describe '#full_path_was' do
+  describe '#full_path_before_last_save' do
     context 'when the group has no parent' do
-      it 'returns the path was' do
-        group = create(:group, parent: nil)
-        expect(group.full_path_was).to eq(group.path_was)
+      it 'returns the path before last save' do
+        group = create(:group)
+
+        group.update(parent: nil)
+
+        expect(group.full_path_before_last_save).to eq(group.path_before_last_save)
       end
     end
 
     context 'when a parent is assigned to a group with no previous parent' do
-      it 'returns the path was' do
+      it 'returns the path before last save' do
         group = create(:group, parent: nil)
-
         parent = create(:group)
-        group.parent = parent
 
-        expect(group.full_path_was).to eq("#{group.path_was}")
+        group.update(parent: parent)
+
+        expect(group.full_path_before_last_save).to eq("#{group.path_before_last_save}")
       end
     end
 
@@ -766,9 +769,10 @@ describe Namespace do
       it 'returns the parent full path' do
         parent = create(:group)
         group = create(:group, parent: parent)
-        group.parent = nil
 
-        expect(group.full_path_was).to eq("#{parent.full_path}/#{group.path}")
+        group.update(parent: nil)
+
+        expect(group.full_path_before_last_save).to eq("#{parent.full_path}/#{group.path}")
       end
     end
 
@@ -777,8 +781,10 @@ describe Namespace do
         parent = create(:group)
         group = create(:group, parent: parent)
         new_parent = create(:group)
-        group.parent = new_parent
-        expect(group.full_path_was).to eq("#{parent.full_path}/#{group.path}")
+
+        group.update(parent: new_parent)
+
+        expect(group.full_path_before_last_save).to eq("#{parent.full_path}/#{group.path}")
       end
     end
   end

spec/services/system_hooks_service_spec.rb

@@ -86,20 +86,20 @@ describe SystemHooksService do
 
     context 'group_rename' do
       it 'contains old and new path' do
-        allow(group).to receive(:path_was).and_return('old-path')
+        allow(group).to receive(:path_before_last_save).and_return('old-path')
 
         data = event_data(group, :rename)
 
         expect(data).to include(:event_name, :name, :created_at, :updated_at, :full_path, :path, :group_id, :old_path, :old_full_path)
         expect(data[:path]).to eq(group.path)
         expect(data[:full_path]).to eq(group.path)
-        expect(data[:old_path]).to eq(group.path_was)
-        expect(data[:old_full_path]).to eq(group.path_was)
+        expect(data[:old_path]).to eq(group.path_before_last_save)
+        expect(data[:old_full_path]).to eq(group.path_before_last_save)
       end
 
       it 'contains old and new full_path for subgroup' do
         subgroup = create(:group, parent: group)
-        allow(subgroup).to receive(:path_was).and_return('old-path')
+        allow(subgroup).to receive(:path_before_last_save).and_return('old-path')
 
         data = event_data(subgroup, :rename)
 
@@ -110,13 +110,13 @@ describe SystemHooksService do
 
     context 'user_rename' do
       it 'contains old and new username' do
-        allow(user).to receive(:username_was).and_return('old-username')
+        allow(user).to receive(:username_before_last_save).and_return('old-username')
 
         data = event_data(user, :rename)
 
         expect(data).to include(:event_name, :name, :created_at, :updated_at, :email, :user_id, :username, :old_username)
         expect(data[:username]).to eq(user.username)
-        expect(data[:old_username]).to eq(user.username_was)
+        expect(data[:old_username]).to eq(user.username_before_last_save)
       end
     end