diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb index 9618d8f62befa2467801cf1e5e7df3e9851b1dcf..77cfb534ea026f15348d4489293c6ae5dbd6c3a3 100644 --- a/ee/app/models/approval_merge_request_rule.rb +++ b/ee/app/models/approval_merge_request_rule.rb @@ -41,6 +41,7 @@ class ApprovalMergeRequestRule < ApplicationRecord alias_method :source_rule, :approval_project_rule before_update :compare_with_project_rule + after_create :check_for_modification_from_project_rule validate :validate_approval_project_rule @@ -131,12 +132,52 @@ def refresh_required_approvals!(project_approval_rule) refresh_license_scanning_approvals(project_approval_rule) if license_scanning? end + # Since the ApprovalProjectRule (APR) that this rule is templated from might + # have changed, rather than attempt to update every rule related to the APR, + # we do a quick check of the APR to see if it and this rule have diverged. + # If so, we attempt to pull the value from the templating APR, falling back + # to the AMRR itself if the APR (as `source_rule`) is for some reason + # missing. + # + + # Since we're also redefining these associations, move them to the side so we + # still can access the original AR association when we need to. + # + alias_method :raw_users, :users + alias_method :raw_groups, :groups + + %i(name approvals_required users groups).each do |method_name| + define_method(method_name) do + return source_rule.send(method_name) if source_rule && !modified_from_project_rule # rubocop:disable GitlabSecurity/PublicSend + + retrieve_value_from_record(method_name) + end + end + private + def retrieve_value_from_record(method_name) + if attributes.key?(method_name.to_s) + read_attribute(method_name) + else + self.send("raw_#{method_name}".to_sym) # rubocop:disable GitlabSecurity/PublicSend + end + end + def compare_with_project_rule self.modified_from_project_rule = overridden? ? true : false end + def check_for_modification_from_project_rule + reset + + if source_rule + compare_with_project_rule + + save! if changed? + end + end + def validate_approval_project_rule return if approval_project_rule.blank? return if merge_request.project == approval_project_rule.project diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index 65b2543afec65d93d06c8c82ca631cf1288e5d05..2812f73d0d6f18db9ea0d4cdc7e688604ae44f99 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -70,8 +70,8 @@ def user_defined? def overridden? return false unless source_rule.present? - source_rule.name != name || - source_rule.approvals_required != approvals_required || + source_rule.name != self[:name] || + source_rule.approvals_required != self[:approvals_required] || source_rule.user_ids.to_set != user_ids.to_set || source_rule.group_ids.to_set != group_ids.to_set end diff --git a/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb index 62b0219e62fa0d3bea28d3d94d9f3f6fa4a8c199..27ac87ed97861a408e5ce08e26a142fc44900aae 100644 --- a/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb +++ b/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb @@ -51,6 +51,17 @@ def project merge_request.target_project end + # This code borrowed from ApprovalRuleLike + # + def overridden? + return false unless approval_project_rule.present? + + approval_project_rule.name != self[:name] || + approval_project_rule.approvals_required != self[:approvals_required] || + approval_project_rule.user_ids.to_set != user_ids.to_set || + approval_project_rule.group_ids.to_set != group_ids.to_set + end + def self.find_or_create_code_owner_rule(merge_request, entry) merge_request.approval_rules.safe_find_or_create_by( rule_type: :code_owner, @@ -198,6 +209,9 @@ def perform(target_type, target_id, sync_code_owner_rule: true) def handle_merge_request if rule = sync_rule rule.approval_project_rule = target.target_project.approval_rules.regular.first + + rule.modified_from_project_rule = rule.overridden? ? true : false + rule.save! if rule.changed? end target.sync_code_owners_with_approvers if @sync_code_owner_rule diff --git a/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb index ae51a36821c1f83f12bd1c3a6916c5d4670c691e..2aca27c0aaa9596e12a798d78e0fa8d60fe9e6d8 100644 --- a/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb +++ b/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb @@ -133,15 +133,52 @@ def create_member_in(member, *populate_in) context 'when project rule is present' do let!(:project_rule) { create(:approval_project_rule, project: target.target_project) } + let(:rule_under_test) { target.approval_rules.regular.first } - it "sets MR rule's source to project rule without duplication" do + before do user = create(:user) create_member_in(user, :old_schema) create_member_in(user, :old_schema) + end + it "sets MR rule's source to project rule" do described_class.new.perform(target_type, target.id) - expect(target.approval_rules.regular.first.approval_project_rule).to eq(project_rule) + expect(rule_under_test.approval_project_rule).to eq(project_rule) + end + + context "when modified from its project rule" do + # Due to how the BackgroundMigration interacts with the records + # created by FactoryBot, our default state is to report the newly + # created ApprovalMergeRequestRules as `overridden? == true` + # + it "sets modified_from_project_rule attribute as true" do + described_class.new.perform(target_type, target.id) + + expect(rule_under_test.modified_from_project_rule).to be_truthy + end + end + + context "when unmodified from its project rule" do + # Because the classes used in the BackgroundMigration are dynamically + # created, we can't interact with them here in RSpec without jumping + # through some intricate hoops, so instead we manually set the attrs + # of the related ApprovalProjectRule to match what FactoryBot will + # create. + # + before do + project_rule.update!( + approvals_required: 2, + name: ApprovalRuleLike::DEFAULT_NAME, + user_ids: [User.last.id] + ) + end + + it "sets modified_from_project_rule attribute as false" do + described_class.new.perform(target_type, target.id) + + expect(rule_under_test.modified_from_project_rule).to be_falsy + end end end diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb index 94f294994f6e79f083e6f40b9f0461f759b869c8..82cb8495b9acdd0e946bd6a5a30d05dc8e08d7eb 100644 --- a/ee/spec/models/approval_merge_request_rule_spec.rb +++ b/ee/spec/models/approval_merge_request_rule_spec.rb @@ -5,7 +5,11 @@ RSpec.describe ApprovalMergeRequestRule do let(:merge_request) { create(:merge_request) } - subject { create(:approval_merge_request_rule, merge_request: merge_request) } + subject(:amrr) do + create(:approval_merge_request_rule, + merge_request: merge_request + ) + end describe 'associations' do subject { build_stubbed(:approval_merge_request_rule) } @@ -108,6 +112,65 @@ end end + shared_examples_for 'attempts to read the value from the AMRR record itself' do + it 'as expected' do + expect(amrr) + .to receive(:retrieve_value_from_record) + .at_least(:once) + .and_call_original + + amrr.send(method_name) + end + end + + describe 'custom getters' do + let(:approval_project_rule) do + create(:approval_project_rule, project: merge_request.project) + end + + let(:amrr) do + create(:approval_merge_request_rule, + merge_request: merge_request, + approval_project_rule: approval_project_rule + ) + end + + %i(name approvals_required users groups).each do |method_name| + let(:method_name) { method_name } + + context 'when source_rule is present' do + context 'the AMRR rule has been modified' do + before do + amrr.modified_from_project_rule = true + end + + it_behaves_like 'attempts to read the value from the AMRR record itself' + end + + context 'the AMRR rule has not been modified' do + before do + amrr.modified_from_project_rule = false + end + + it 'attempts to read the value from the source rule (ApprovalProjectRule)' do + expect(amrr.approval_project_rule) + .to receive(method_name) + .at_least(:once) + .and_call_original + + amrr.send(method_name) + end + end + end + + context 'when source_rule is missing' do + let(:approval_project_rule) { nil } + + it_behaves_like 'attempts to read the value from the AMRR record itself' + end + end + end + context 'scopes' do let!(:rb_rule) { create(:code_owner_rule, name: '*.rb') } let!(:js_rule) { create(:code_owner_rule, name: '*.js') } @@ -276,17 +339,12 @@ expect(subject.approvers).to be_empty end - context 'when the rules users have already been loaded' do + context 'when the rule\'s users have already been loaded' do before do subject.users subject.group_users end - it 'does not perform any new queries when all users are loaded already' do - # single query is triggered for license check - expect { subject.approvers }.not_to exceed_query_limit(1) - end - it 'does not contain the author' do expect(subject.approvers).to be_empty end diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb index 69deb86b30fd83b24ca283e6bf65d511ad9d7b01..07d434b9ec671546ff04a054a97aa1ad948eae6e 100644 --- a/ee/spec/models/concerns/approval_rule_like_spec.rb +++ b/ee/spec/models/concerns/approval_rule_like_spec.rb @@ -40,11 +40,6 @@ subject.group_users end - it 'does not perform any new queries when all users are loaded already' do - # single query is triggered for license check - expect { subject.approvers }.not_to exceed_query_limit(1) - end - it_behaves_like 'approvers contains the right users' end diff --git a/ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb b/ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb index 86ca586adc63f6c6aa309bacc98f41191efb3ca9..42faf77569e2106ebabdff8a4d7afdfabba09586 100644 --- a/ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb @@ -24,6 +24,12 @@ let(:newrev) { commits.first.id } let(:approver) { create(:user) } let(:notification_service) { spy('notification_service') } + let(:approval_project_rule) do + create(:approval_project_rule, + project: merge_request.project, + users: [approver] + ) + end def approval_todos(merge_request) Todo.where(action: Todo::APPROVAL_REQUIRED, target: merge_request) @@ -40,6 +46,8 @@ def approval_todos(merge_request) end merge_request.approvals.create!(user_id: approver.id) + + merge_request.approval_rules.first.approval_project_rule = approval_project_rule end it 'resets approvals' do