Merge branch 'id-fix-self-approvals' into 'master'

Use fallback approval rule if no eligible rules exist

Closes gitlab-ce#61044

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

ee/app/models/approval_state.rb

@@ -51,12 +51,12 @@ class ApprovalState
   end
 
   def has_non_fallback_rules?
-    regular_rules.present? || code_owner_rules.present?
+    has_regular_rule_with_approvers? || code_owner_rules.present?
   end
 
   # Use the fallback rule if regular rules are empty
   def use_fallback?
-    regular_rules.empty?
+    !has_regular_rule_with_approvers?
   end
 
   def fallback_rule
@@ -65,7 +65,7 @@ class ApprovalState
 
   # Determines which set of rules to use (MR or project)
   def approval_rules_overwritten?
-    regular_merge_request_rules.any? ||
+    regular_merge_request_rules.any? { |rule| rule.approvers.present? } ||
       (project.can_override_approvers? && merge_request.approvals_before_merge.present?)
   end
   alias_method :approvers_overwritten?, :approval_rules_overwritten?
@@ -83,7 +83,7 @@ class ApprovalState
   end
 
   def any_approver_allowed?
-    regular_rules.empty? || approved?
+    !has_regular_rule_with_approvers? || approved?
   end
 
   def approvals_required
@@ -172,6 +172,10 @@ class ApprovalState
 
   private
 
+  def has_regular_rule_with_approvers?
+    regular_rules.any? { |rule| rule.approvers.present? }
+  end
+
   def regular_rules
     strong_memoize(:regular_rules) do
       rules = approval_rules_overwritten? ? regular_merge_request_rules : regular_project_rules

ee/changelogs/unreleased/id-fix-self-approvals.yml

+---
+title: Use fallback approval rule if no eligible rules exist
+merge_request: 14042
+author:
+type: fixed

ee/spec/features/merge_request/user_edits_approval_rules_mr_spec.rb

@@ -30,8 +30,10 @@ describe 'Merge request > User edits MR with approval rules', :js do
     project.update_attribute(:disable_overriding_approvers_per_merge_request, false)
     stub_licensed_features(multiple_approval_rules: true)
 
+    approver = create(:user)
     mr_rule_names.each do |name|
-      create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1, name: name)
+      create(:approval_merge_request_rule,
+        merge_request: merge_request, approvals_required: 1, name: name, users: [approver])
     end
 
     sign_in(author)

ee/spec/models/approval_state_spec.rb

@@ -4,7 +4,8 @@ require 'spec_helper'
 
 describe ApprovalState do
   def create_rule(additional_params = {})
-    params = additional_params.merge(merge_request: merge_request)
+    default_approver = create(:user)
+    params = additional_params.reverse_merge(merge_request: merge_request, users: [default_approver])
     factory = params.delete(:code_owner) ? :code_owner_rule : :approval_merge_request_rule
 
     create(factory, params)
@@ -101,11 +102,23 @@ describe ApprovalState do
 
     context 'when approval rule on the merge request exists' do
       before do
-        create(:approval_merge_request_rule, merge_request: merge_request)
+        create(:approval_merge_request_rule, merge_request: merge_request, users: approvers)
       end
 
-      it 'returns true' do
-        expect(subject.approval_rules_overwritten?).to eq(true)
+      context 'without approvers' do
+        let(:approvers) { [] }
+
+        it 'returns false' do
+          expect(subject.approval_rules_overwritten?).to eq(false)
+        end
+      end
+
+      context 'with approvers' do
+        let(:approvers) { [create(:user)] }
+
+        it 'returns true' do
+          expect(subject.approval_rules_overwritten?).to eq(true)
+        end
       end
     end
 
@@ -618,10 +631,11 @@ describe ApprovalState do
       end
 
       context 'when there is one approver required' do
-        let!(:rule) { create_rule(approvals_required: 1) }
+        let!(:rule) { create_rule(approvals_required: 1, users: []) }
 
         context 'when that approver is the MR author' do
           before do
+            project.update!(approvals_before_merge: 2)
             rule.users << author
           end
 
@@ -629,13 +643,45 @@ describe ApprovalState do
 
           it_behaves_like 'a MR that all members with write access can approve'
 
-          it 'requires one approval' do
-            expect(subject.approvals_left).to eq(1)
-          end
-
           it 'does not allow a logged-out user to approve the MR' do
             expect(subject.can_approve?(nil)).to be_falsey
           end
+
+          it 'fallback rule is used' do
+            expect(subject.approvals_left).to eq(2)
+          end
+
+          it 'is not approved' do
+            expect(subject.approved?).to eq(false)
+          end
+
+          context 'with project approval rule' do
+            before do
+              create(:approval_project_rule, project: project, approvals_required: 1, users: [approver])
+            end
+
+            context 'with approvers' do
+              let(:approver) { create(:user) }
+
+              it 'requires one approval' do
+                rule = subject.wrapped_approval_rules.last.approval_rule
+
+                expect(rule).to be_a(ApprovalProjectRule)
+                expect(subject.approvers).to eq([approver])
+              end
+            end
+
+            context 'without approvers' do
+              let(:approver) { author }
+
+              it 'requires one approval' do
+                rule = subject.wrapped_approval_rules.last
+
+                expect(rule).to be_a(ApprovalMergeRequestFallback)
+                expect(subject.approvers).to eq([])
+              end
+            end
+          end
         end
 
         context 'when that approver is not the MR author' do
@@ -1072,7 +1118,7 @@ describe ApprovalState do
 
       it 'includes approvers from first rule and code owner rule' do
         create_rules
-        approvers = rule1.users + [group_approver1]
+        approvers = rule1.users + code_owner_rule.users + [group_approver1]
 
         expect(subject.approvers).to contain_exactly(*approvers)
       end
@@ -1252,10 +1298,11 @@ describe ApprovalState do
       end
 
       context 'when there is one approver required' do
-        let!(:rule) { create_rule(approvals_required: 1) }
+        let!(:rule) { create_rule(approvals_required: 1, users: []) }
 
         context 'when that approver is the MR author' do
           before do
+            project.update!(approvals_before_merge: 1)
             rule.users << author
           end
 
@@ -1270,6 +1317,10 @@ describe ApprovalState do
           it 'does not allow a logged-out user to approve the MR' do
             expect(subject.can_approve?(nil)).to be_falsey
           end
+
+          it 'is not approved' do
+            expect(subject.approved?).to eq(false)
+          end
         end
 
         context 'when that approver is not the MR author' do

ee/spec/requests/api/merge_request_approvals_spec.rb

@@ -60,6 +60,7 @@ describe API::MergeRequestApprovals do
     context 'when private group approver' do
       before do
         private_group = create(:group, :private)
+        private_group.add_developer(create(:user))
         merge_request.approver_groups.create(group: private_group)
       end
 
@@ -138,7 +139,9 @@ describe API::MergeRequestApprovals do
     end
 
     it 'excludes private groups' do
-      private_group = create :group, :private
+      private_group = create(:group, :private)
+
+      rule.users << approver
       rule.groups << private_group
 
       get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user)