Merge branch 'id-fix-api-for-mr-approval-rules' into 'master'

Fix create/delete API calls for approval rules

See merge request gitlab-org/gitlab!23107

ee/app/models/concerns/approval_rule_like.rb

@@ -50,4 +50,8 @@ module ApprovalRuleLike
   def any_approver?
     raise NotImplementedError
   end
+
+  def user_defined?
+    regular? || any_approver?
+  end
 end

ee/app/policies/approval_merge_request_rule_policy.rb

@@ -4,7 +4,7 @@ class ApprovalMergeRequestRulePolicy < BasePolicy
   delegate { @subject.merge_request }
 
   condition(:editable) do
-    can?(:update_merge_request, @subject.merge_request) && @subject.regular?
+    can?(:update_merge_request, @subject.merge_request) && @subject.user_defined?
   end
 
   rule { editable }.enable :edit_approval_rule

ee/app/services/approval_rules/create_service.rb

@@ -14,7 +14,7 @@ module ApprovalRules
         params.reverse_merge!(rule_type: :report_approver)
       end
 
-      handle_any_approver_rule_creation(target, params) if target.is_a?(Project)
+      handle_any_approver_rule_creation(target, @rule.project, params)
       copy_approval_project_rule_properties(params) if target.is_a?(MergeRequest)
 
       super(@rule.project, user, params)
@@ -38,14 +38,14 @@ module ApprovalRules
       params[:groups] = approval_project_rule.groups
     end
 
-    def handle_any_approver_rule_creation(target, params)
+    def handle_any_approver_rule_creation(target, project, params)
       if params[:user_ids].blank? && params[:group_ids].blank?
         params.reverse_merge!(rule_type: :any_approver, name: ApprovalRuleLike::ALL_MEMBERS)
 
         return
       end
 
-      return if target.multiple_approval_rules_available?
+      return if project.multiple_approval_rules_available?
 
       target.approval_rules.any_approver.delete_all
     end

ee/changelogs/unreleased/id-fix-api-for-mr-approval-rules.yml

+---
+title: Fix create/delete API calls for approval rules
+merge_request: 23107
+author:
+type: fixed

ee/spec/policies/approval_merge_request_rule_policy_spec.rb

@@ -15,7 +15,15 @@ describe ApprovalMergeRequestRulePolicy do
       expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule)
     end
 
-    context 'when rule is not regular type' do
+    context 'when rule is any-approval' do
+      let(:approval_rule) { build(:any_approver_rule, merge_request: merge_request) }
+
+      it 'allows updating approval rule' do
+        expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule)
+      end
+    end
+
+    context 'when rule is not user editable' do
       let(:approval_rule) { create(:code_owner_rule, merge_request: merge_request) }
 
       it 'disallows updating approval rule' do

ee/spec/services/approval_rules/create_service_spec.rb

@@ -76,6 +76,45 @@ describe ApprovalRules::CreateService do
         expect(result[:message]).to include('Prohibited')
       end
     end
+
+    context 'when approval rule with empty users and groups is being created' do
+      subject { described_class.new(target, user, { user_ids: [], group_ids: [] }) }
+
+      it 'sets default attributes for any-approver rule' do
+        rule = subject.execute[:rule]
+
+        expect(rule[:rule_type]).to eq('any_approver')
+        expect(rule[:name]).to eq('All Members')
+      end
+    end
+
+    context 'when any-approver rule exists' do
+      before do
+        target.approval_rules.create!(rule_type: :any_approver, name: 'All members')
+      end
+
+      context 'multiple approval rules are not enabled' do
+        subject { described_class.new(target, user, { user_ids: [1], group_ids: [] }) }
+
+        it 'removes the rule if a regular one is created' do
+          expect { subject.execute }.to change(
+            target.approval_rules.any_approver, :count
+          ).from(1).to(0)
+        end
+      end
+
+      context 'multiple approval rules are enabled' do
+        subject { described_class.new(target, user, { user_ids: [1], group_ids: [] }) }
+
+        before do
+          stub_licensed_features(multiple_approval_rules: true)
+        end
+
+        it 'does not remove any approval rule' do
+          expect { subject.execute }.not_to change(target.approval_rules.any_approver, :count)
+        end
+      end
+    end
   end
 
   context 'when target is project' do
@@ -148,37 +187,6 @@ describe ApprovalRules::CreateService do
         specify { expect(result[:rule].rule_type).to eq('report_approver') }
       end
     end
-
-    context 'when approval rule is being created' do
-      subject { described_class.new(target, user, { user_ids: [], group_ids: [] }) }
-
-      it 'sets default attributes for any-approver rule' do
-        rule = subject.execute[:rule]
-
-        expect(rule[:rule_type]).to eq('any_approver')
-        expect(rule[:name]).to eq('All Members')
-      end
-    end
-
-    context 'when any-approver rule exists' do
-      let!(:any_approver_rule) do
-        create(:approval_project_rule, project: target, rule_type: :any_approver)
-      end
-
-      context 'multiple approval rules are not enabled' do
-        subject { described_class.new(target, user, { user_ids: [1], group_ids: [] }) }
-
-        before do
-          stub_licensed_features(multiple_approval_rules: false)
-        end
-
-        it 'removes the rule if a regular one is created' do
-          expect { subject.execute }.to change(
-            target.approval_rules.any_approver, :count
-          ).from(1).to(0)
-        end
-      end
-    end
   end
 
   context 'when target is merge request' do