Allow overriding of project approvers in merge request

Previously specifying blank `user_ids` and `group_ids` during the
creation of a merge request approver rule would cause the merge request
to inherit from the project rule. However, that prevents users from
setting the merge request approvers to "any approver". To fix this, we
only set the "any approver" type if `user_ids` and `group_ids` is not
specified and no project approvers are present.

In addition, this commit also fixes the approval rule type when project
rule has users/groups.

Suppose a project has an approval rule that includes specific users or
groups as approvers. If a user attempts to create a merge request
approval rule based on this project rule but omits `user_ids` and
`group_ids`, previously this rule would be erroneously assigned "any
approver". However, the project approvers should be inherited.

To fix this, we:

1. First copy the project approval rules.

2. If the newly-modified params indicate specific users or groups, don't
assign the merge request rule the "any approver" type.

Found while investigating failures in upgrading Grape v1.3.3 in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33450.

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) }