Merge branch '12607-remove-mr-approval-rule-source-restriction' into 'master'

Remove validation of MR level approval rules in merge requests

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

ee/app/models/approval_merge_request_rule.rb

@@ -39,6 +39,7 @@ class ApprovalMergeRequestRule < ApplicationRecord
   alias_method :source_rule, :approval_project_rule
 
   validate :validate_approvals_required, unless: :report_approver?
+  validate :validate_approval_project_rule
 
   enum rule_type: {
     regular: 1,
@@ -123,4 +124,11 @@ class ApprovalMergeRequestRule < ApplicationRecord
       errors.add(:approvals_required, :greater_than_or_equal_to, count: approval_project_rule.approvals_required)
     end
   end
+
+  def validate_approval_project_rule
+    return if approval_project_rule.blank?
+    return if merge_request.project == approval_project_rule.project
+
+    errors.add(:approval_project_rule, 'must be for the same project')
+  end
 end

ee/app/models/ee/merge_request.rb

@@ -35,8 +35,6 @@ module EE
 
       has_many :blocked_merge_requests, through: :blocks_as_blocker
 
-      validate :validate_approval_rule_source
-
       delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
       delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
       delegate :merge_requests_author_approval?, to: :target_project, allow_nil: true
@@ -106,21 +104,6 @@ module EE
       hidden.count
     end
 
-    def validate_approval_rule_source
-      return unless approval_rules.any?
-
-      local_project_rule_ids = approval_rules.map { |rule| rule.approval_merge_request_rule_source&.approval_project_rule_id }
-      local_project_rule_ids.compact!
-
-      invalid = if new_record?
-                  local_project_rule_ids.to_set != project.visible_regular_approval_rules.pluck(:id).to_set
-                else
-                  (local_project_rule_ids - project.approval_rule_ids).present?
-                end
-
-      errors.add(:approval_rules, :invalid_sourcing_to_project_rules) if invalid
-    end
-
     def participant_approvers
       strong_memoize(:participant_approvers) do
         next [] unless approval_needed?

ee/changelogs/unreleased/12607-remove-mr-approval-rule-source-restriction.yml

+---
+title: Remove validation of MR level approval rules in merge requests
+merge_request: 14968
+author:
+type: changed

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -16,6 +16,25 @@ describe ApprovalMergeRequestRule do
       expect(build(:approval_merge_request_rule, name: nil)).not_to be_valid
     end
 
+    context 'approval_project_rule is set' do
+      let(:approval_project_rule) { build(:approval_project_rule) }
+      let(:merge_request_rule) { build(:approval_merge_request_rule, merge_request: merge_request, approval_project_rule: approval_project_rule) }
+
+      context 'when project of approval_project_rule and merge request matches' do
+        let(:merge_request) { build(:merge_request, project: approval_project_rule.project) }
+
+        it 'is valid' do
+          expect(merge_request_rule).to be_valid
+        end
+      end
+
+      context 'when the project of approval_project_rule and merge request does not match' do
+        it 'is invalid' do
+          expect(merge_request_rule).to be_invalid
+        end
+      end
+    end
+
     context 'code owner rules' do
       it 'is valid' do
         expect(build(:code_owner_rule)).to be_valid

ee/spec/models/merge_request_spec.rb

@@ -50,92 +50,6 @@ describe MergeRequest do
     end
   end
 
-  describe 'approval_rules' do
-    before do
-      stub_licensed_features(multiple_approval_rules: true)
-    end
-
-    context 'when project contains approval_rules' do
-      let!(:project_rule1) {  project.approval_rules.create(name: 'p1') }
-      let!(:project_rule2) {  project.approval_rules.create(name: 'p2') }
-
-      context "when creating" do
-        subject(:merge_request) { build(:merge_request, source_project: project, target_project: project) }
-
-        context "when MR has no rule" do
-          it 'is valid as project rule will be active' do
-            expect(merge_request).to be_valid
-          end
-        end
-
-        context "when MR rules exists but do not reference all project rules" do
-          it 'is invalid' do
-            merge_request.approval_rules.build(name: 'mr1', approval_project_rule_id: project_rule1.id)
-
-            expect(merge_request).to be_invalid
-            expect(merge_request.errors.added?(:approval_rules, :invalid_sourcing_to_project_rules)).to eq(true)
-          end
-        end
-
-        context "when MR rules exists but reference rules other than the project's" do
-          let(:other_project_rule) { create(:approval_project_rule) }
-
-          it 'is invalid' do
-            merge_request.approval_rules.build(name: 'mr1', approval_project_rule_id: project_rule1.id)
-            merge_request.approval_rules.build(name: 'mr2', approval_project_rule_id: project_rule2.id)
-            merge_request.approval_rules.build(name: 'mr3', approval_project_rule_id: other_project_rule.id)
-
-            expect(merge_request).to be_invalid
-            expect(merge_request.errors.added?(:approval_rules, :invalid_sourcing_to_project_rules)).to eq(true)
-          end
-        end
-
-        context "when MR's rules exists and reference all project's rules" do
-          it 'is valid' do
-            merge_request.approval_rules.build(name: 'mr1', approval_project_rule_id: project_rule1.id)
-            merge_request.approval_rules.build(name: 'mr2', approval_project_rule_id: project_rule2.id)
-
-            expect(merge_request).to be_valid
-          end
-        end
-      end
-
-      context "when updating" do
-        subject!(:merge_request) do
-          merge_request = build(:merge_request, source_project: project, target_project: project)
-          merge_request.approval_rules.build(name: 'mr1', approval_project_rule_id: project_rule1.id)
-          merge_request.approval_rules.build(name: 'mr2', approval_project_rule_id: project_rule2.id)
-          merge_request.save!
-          merge_request
-        end
-
-        context "when MR rules reference rules other than the project's" do
-          let(:other_project_rule) { create(:approval_project_rule) }
-
-          it 'is invalid' do
-            merge_request.approval_rules.build(name: 'mr3', approval_project_rule_id: other_project_rule.id)
-
-            expect(merge_request).to be_invalid
-            expect(merge_request.errors.added?(:approval_rules, :invalid_sourcing_to_project_rules)).to eq(true)
-          end
-        end
-
-        context 'when project later added a new rule' do
-          before do
-            project.approval_rules.create(name: 'p3')
-          end
-
-          it 'can still be saved' do
-            subject.reload
-            subject.title = 'foobar'
-
-            expect(subject.save).to eq(true)
-          end
-        end
-      end
-    end
-  end
-
   describe '#participant_approvers' do
     let(:approvers) { create_list(:user, 2) }
     let(:code_owners) { create_list(:user, 2) }