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

[CE] Remove deprecated usage of `attribute_changed?`

See merge request gitlab-org/gitlab-ce!27577

app/models/commit_status.rb

@@ -140,7 +140,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,4 +265,8 @@ 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

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

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