diff options
author | Heinrich Lee Yu <heinrich@gitlab.com> | 2019-04-23 17:01:43 +0800 |
---|---|---|
committer | Heinrich Lee Yu <heinrich@gitlab.com> | 2019-04-30 15:24:20 +0800 |
commit | 35834940519f297259312fbbe54dd88beda28123 (patch) | |
tree | 04be3f817959c8a78c352ecec46ab1b59bb0727d | |
parent | 9d8394cc3451463d9dcf1a07c16a782c12dc682c (diff) | |
download | gitlab-ce-35834940519f297259312fbbe54dd88beda28123.tar.gz |
Remove deprecated uses of attribute_changed?
Prepares us for upgrade to Rails 5.2
24 files changed, 94 insertions, 84 deletions
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index a1d1ba2264f..2dd314b28cb 100644 --- a/app/models/commit_status.rb +++ b/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 diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index f7e18a12136..f7640651168 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..5edc33fc637 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 9b223ac06ed..685c1afd853 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 c46405e2725..b9448ce5421 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 6f3f25c72a9..bf1aad03184 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,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) 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 e47376d05df..377b2f36700 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 4b806a75205..efad76321f8 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 d7d9bd1b2bd..cc8fa216954 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/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 910765770ac..c0497927ef6 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/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) diff --git a/ee/app/models/ee/namespace.rb b/ee/app/models/ee/namespace.rb index 469619addb3..21c14b4a27f 100644 --- a/ee/app/models/ee/namespace.rb +++ b/ee/app/models/ee/namespace.rb @@ -80,7 +80,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 diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index c6633d6ece2..90a9e3373ef 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -372,7 +372,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 diff --git a/ee/app/models/operations/feature_flag.rb b/ee/app/models/operations/feature_flag.rb index 9d970bf40b6..de918dd54da 100644 --- a/ee/app/models/operations/feature_flag.rb +++ b/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 diff --git a/ee/app/models/packages/package_file.rb b/ee/app/models/packages/package_file.rb index c540fb37842..74d5aad236d 100644 --- a/ee/app/models/packages/package_file.rb +++ b/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!` diff --git a/ee/app/services/ee/milestones/update_service.rb b/ee/app/services/ee/milestones/update_service.rb index 95fa84fe49a..b6fc2b02ab9 100644 --- a/ee/app/services/ee/milestones/update_service.rb +++ b/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 diff --git a/ee/app/services/epics/update_service.rb b/ee/app/services/epics/update_service.rb index 3def5fa3779..621996a2881 100644 --- a/ee/app/services/epics/update_service.rb +++ b/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 diff --git a/ee/spec/models/namespace_spec.rb b/ee/spec/models/namespace_spec.rb index 5c5959dabbe..9f087305327 100644 --- a/ee/spec/models/namespace_spec.rb +++ b/ee/spec/models/namespace_spec.rb @@ -113,18 +113,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) end @@ -137,9 +137,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) 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 |