diff options
author | Heinrich Lee Yu <heinrich@gitlab.com> | 2019-04-23 17:30:18 +0800 |
---|---|---|
committer | Heinrich Lee Yu <heinrich@gitlab.com> | 2019-04-30 15:24:25 +0800 |
commit | fc22626f453f64409ad5b2967cdec541dbcc041d (patch) | |
tree | 9686c1c160af34db2038c241ab7f0b665f1c51a0 | |
parent | a2543ee29a97f61f960994d473291c7224c50c3d (diff) | |
download | gitlab-ce-fc22626f453f64409ad5b2967cdec541dbcc041d.tar.gz |
Remove deprecated uses of attribute_changed?9932-fix-deprecated-attribute_changed-ce
Prepares us for upgrade to Rails 5.2
-rw-r--r-- | app/models/commit_status.rb | 2 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 2 | ||||
-rw-r--r-- | app/models/concerns/storage/legacy_namespace.rb | 16 | ||||
-rw-r--r-- | app/models/merge_request_diff.rb | 11 | ||||
-rw-r--r-- | app/models/namespace.rb | 28 | ||||
-rw-r--r-- | app/models/pages_domain.rb | 4 | ||||
-rw-r--r-- | app/models/project.rb | 4 | ||||
-rw-r--r-- | app/models/remote_mirror.rb | 8 | ||||
-rw-r--r-- | app/models/storage/legacy_project.rb | 2 | ||||
-rw-r--r-- | app/services/projects/update_service.rb | 5 | ||||
-rw-r--r-- | app/services/system_hooks_service.rb | 6 | ||||
-rw-r--r-- | app/uploaders/object_storage.rb | 2 | ||||
-rw-r--r-- | db/migrate/20170503140202_turn_nested_groups_into_regular_groups_for_mysql.rb | 7 | ||||
-rw-r--r-- | spec/models/commit_status_spec.rb | 4 | ||||
-rw-r--r-- | spec/models/namespace_spec.rb | 30 | ||||
-rw-r--r-- | spec/services/system_hooks_service_spec.rb | 12 |
16 files changed, 77 insertions, 66 deletions
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index f97dc38dab7..be6f3e9c5b0 100644 --- a/app/models/commit_status.rb +++ b/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 diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 775aaa44e5b..127430cc68f 100644 --- a/app/models/concerns/issuable.rb +++ b/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? diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index 1cbe27ad03a..a15dc19e07a 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/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 diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 0b787217410..5f5d92bc2f0 100644 --- a/app/models/merge_request_diff.rb +++ b/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 diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 7228aab2c2e..168f6bedd63 100644 --- a/app/models/namespace.rb +++ b/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 diff --git a/app/models/pages_domain.rb b/app/models/pages_domain.rb index d73b2889f30..0ad052646d2 100644 --- a/app/models/pages_domain.rb +++ b/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 ) } @@ -146,7 +146,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? || diff --git a/app/models/project.rb b/app/models/project.rb index a29100405f9..b0dbf09ecbf 100644 --- a/app/models/project.rb +++ b/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 diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index 535f772b6a1..cbfc1a7c1b2 100644 --- a/app/models/remote_mirror.rb +++ b/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 diff --git a/app/models/storage/legacy_project.rb b/app/models/storage/legacy_project.rb index 76ac5c13c18..b483c677be9 100644 --- a/app/models/storage/legacy_project.rb +++ b/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) diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 864ce4fa9f5..dfa7bd20254 100644 --- a/app/services/projects/update_service.rb +++ b/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? diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb index bd3907cdf8e..858e04f43b2 100644 --- a/app/services/system_hooks_service.rb +++ b/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 diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 5f8b89f2a24..0a44d60778d 100644 --- a/app/uploaders/object_storage.rb +++ b/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 diff --git a/db/migrate/20170503140202_turn_nested_groups_into_regular_groups_for_mysql.rb b/db/migrate/20170503140202_turn_nested_groups_into_regular_groups_for_mysql.rb index cfa63b65df4..65b2c6a57be 100644 --- a/db/migrate/20170503140202_turn_nested_groups_into_regular_groups_for_mysql.rb +++ b/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 diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index ca2f9e36c98..017cca0541e 100644 --- a/spec/models/commit_status_spec.rb +++ b/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 diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 95a4b0f6d71..dd5edca5059 100644 --- a/spec/models/namespace_spec.rb +++ b/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 diff --git a/spec/services/system_hooks_service_spec.rb b/spec/services/system_hooks_service_spec.rb index 5dc7394b84f..f5c6e972953 100644 --- a/spec/services/system_hooks_service_spec.rb +++ b/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 |