Merge branch 'sh-fix-any-approvals-with-project-rules' into 'master'

Allow overriding of project approvers in merge request

See merge request gitlab-org/gitlab!34026

changelogs/unreleased/sh-fix-any-approvals-with-project-rules.yml

+---
+title: Fix approval rule type when project rule has users/groups
+merge_request: 34026
+author:
+type: fixed

ee/app/services/approval_rules/create_service.rb

@@ -7,6 +7,7 @@ module ApprovalRules
     # @param target [Project, MergeRequest]
     def initialize(target, user, params)
       @rule = target.approval_rules.build
+      @params = params
 
       # report_approver rule_type is currently auto-set according to rulename
       # Techdebt to be addressed with: https://gitlab.com/gitlab-org/gitlab/issues/12759
@@ -14,8 +15,10 @@ module ApprovalRules
         params.reverse_merge!(rule_type: :report_approver)
       end
 
-      handle_any_approver_rule_creation(target, @rule.project, params)
+      # If merge request approvers are specified, they take precedence over project
+      # approvers.
       copy_approval_project_rule_properties(params) if target.is_a?(MergeRequest)
+      handle_any_approver_rule_creation(target, @rule.project, params)
 
       super(@rule.project, user, params)
     end
@@ -29,17 +32,16 @@ module ApprovalRules
 
       return if approval_project_rule.blank?
 
-      # Remove the following from params so when set they'll be ignored
-      params.delete(:user_ids)
-      params.delete(:group_ids)
-
       params[:name] = approval_project_rule.name
-      params[:users] = approval_project_rule.users
-      params[:groups] = approval_project_rule.groups
+
+      unless approvers_set?
+        params[:users] = approval_project_rule.users
+        params[:groups] = approval_project_rule.groups
+      end
     end
 
     def handle_any_approver_rule_creation(target, project, params)
-      if params[:user_ids].blank? && params[:group_ids].blank?
+      unless approvers_present?
         params.reverse_merge!(rule_type: :any_approver, name: ApprovalRuleLike::ALL_MEMBERS)
 
         return
@@ -49,5 +51,13 @@ module ApprovalRules
 
       target.approval_rules.any_approver.delete_all
     end
+
+    def approvers_set?
+      @params.key?(:user_ids) || @params.key?(:group_ids)
+    end
+
+    def approvers_present?
+      %i(user_ids group_ids users groups).any? { |key| @params[key].present? }
+    end
   end
 end

ee/spec/requests/api/merge_request_approval_rules_spec.rb

@@ -146,6 +146,12 @@ RSpec.describe API::MergeRequestApprovalRules do
     let(:approver) { create(:user) }
     let(:group) { create(:group) }
     let(:approval_project_rule_id) { nil }
+    let(:approver_params) do
+      {
+        user_ids: user_ids,
+        group_ids: group_ids
+      }
+    end
     let(:user_ids) { [] }
     let(:group_ids) { [] }
 
@@ -153,10 +159,8 @@ RSpec.describe API::MergeRequestApprovalRules do
       {
         name: 'Test',
         approvals_required: 1,
-        approval_project_rule_id: approval_project_rule_id,
-        user_ids: user_ids,
-        group_ids: group_ids
-      }
+        approval_project_rule_id: approval_project_rule_id
+      }.merge(approver_params)
     end
 
     let(:action) { post api(url, current_user), params: params }
@@ -222,15 +226,32 @@ RSpec.describe API::MergeRequestApprovalRules do
 
         let(:approval_project_rule_id) { approval_project_rule.id }
 
-        it 'copies the attributes from the project rule execpt approvals_required' do
-          rule = json_response
+        context 'with blank approver params' do
+          it 'copies the attributes from the project rule except approvers' do
+            rule = json_response
 
-          expect(rule['name']).to eq(approval_project_rule.name)
-          expect(rule['approvals_required']).to eq(params[:approvals_required])
-          expect(rule['source_rule']['approvals_required']).to eq(approval_project_rule.approvals_required)
-          expect(rule['eligible_approvers']).to match([hash_including('id' => approver.id)])
-          expect(rule['users']).to match([hash_including('id' => approver.id)])
-          expect(rule['groups']).to match([hash_including('id' => group.id)])
+            expect(rule['name']).to eq(approval_project_rule.name)
+            expect(rule['approvals_required']).to eq(params[:approvals_required])
+            expect(rule['source_rule']['approvals_required']).to eq(approval_project_rule.approvals_required)
+            expect(rule['eligible_approvers']).to eq([])
+            expect(rule['users']).to eq([])
+            expect(rule['groups']).to eq([])
+          end
+        end
+
+        context 'with omitted approver params' do
+          let(:approver_params) { {} }
+
+          it 'copies the attributes from the project rule except approvals_required' do
+            rule = json_response
+
+            expect(rule['name']).to eq(approval_project_rule.name)
+            expect(rule['approvals_required']).to eq(params[:approvals_required])
+            expect(rule['source_rule']['approvals_required']).to eq(approval_project_rule.approvals_required)
+            expect(rule['eligible_approvers']).to match([hash_including('id' => approver.id)])
+            expect(rule['users']).to match([hash_including('id' => approver.id)])
+            expect(rule['groups']).to match([hash_including('id' => group.id)])
+          end
         end
       end
     end

ee/spec/services/approval_rules/create_service_spec.rb

@@ -195,14 +195,19 @@ RSpec.describe ApprovalRules::CreateService do
     it_behaves_like "creatable"
 
     context 'when project rule id is present' do
+      let_it_be(:project_user) { create(:user) }
+      let_it_be(:public_group) { create(:group, :public) }
+      let(:project_user_approvers) { [project_user] }
+      let(:group_user_approvers) { [public_group] }
+      let(:merge_request_approvers) { {} }
       let(:project_rule) do
         create(
           :approval_project_rule,
           project: project,
           name: 'bar',
           approvals_required: 1,
-          users: [create(:user)],
-          groups: [create(:group)]
+          users: project_user_approvers,
+          groups: group_user_approvers
         )
       end
 
@@ -210,26 +215,70 @@ RSpec.describe ApprovalRules::CreateService do
         described_class.new(target, user, {
           name: 'foo',
           approvals_required: 0,
-          approval_project_rule_id: project_rule.id,
-          user_ids: [],
-          group_ids: []
-        }).execute
+          approval_project_rule_id: project_rule.id
+        }.merge(merge_request_approvers)).execute
       end
 
       let(:rule) { result[:rule] }
 
-      it 'associates with project rule' do
+      before do
+        project.add_developer(project_user)
+      end
+
+      it 'associates with project rule and copies its properites' do
         expect(result[:status]).to eq(:success)
         expect(rule.approvals_required).to eq(0)
         expect(rule.approval_project_rule).to eq(project_rule)
-      end
-
-      it 'copies properties from the project rule' do
         expect(rule.name).to eq(project_rule.name)
+        expect(rule.rule_type).to eq('regular')
         expect(rule.users).to match(project_rule.users)
         expect(rule.groups).to match(project_rule.groups)
       end
 
+      context 'when project rule includes no specific approvers' do
+        let(:project_user_approvers) { User.none }
+        let(:group_user_approvers) { Group.none }
+
+        it 'associates with project rule and copies its properties' do
+          expect(result[:status]).to eq(:success)
+          expect(rule.approvals_required).to eq(0)
+          expect(rule.approval_project_rule).to eq(project_rule)
+          expect(rule.name).to eq(project_rule.name)
+          expect(rule.rule_type).to eq('any_approver')
+          expect(rule.users).to match([])
+          expect(rule.groups).to match([])
+        end
+      end
+
+      context 'when merge request includes empty approvers' do
+        let(:merge_request_approvers) do
+          {
+            user_ids: [],
+            group_ids: []
+          }
+        end
+
+        it 'sets any approver' do
+          expect(result[:status]).to eq(:success)
+          expect(rule.name).to eq(project_rule.name)
+          expect(rule.rule_type).to eq('any_approver')
+          expect(rule.users).to eq([])
+          expect(rule.groups).to eq([])
+        end
+      end
+
+      context 'when merge request overrides approvers' do
+        let(:merge_request_approvers) { { user_ids: [user.id] } }
+
+        it 'sets single user as the approver' do
+          expect(result[:status]).to eq(:success)
+          expect(rule.name).to eq(project_rule.name)
+          expect(rule.rule_type).to eq('regular')
+          expect(rule.users).to eq([user])
+          expect(rule.groups).to eq([])
+        end
+      end
+
       context 'when project rule is under the same project as MR' do
         let(:another_project) { create(:project) }