Check for code owner rules even when no regular rules present

fallback_approvals_required should be checked in conjunction with
code owner rules (if any)

ee/app/models/approval_state.rb

@@ -43,11 +43,9 @@ class ApprovalState
   def approval_needed?
     return false unless project.feature_available?(:merge_request_approvers)
 
-    if regular_rules.empty?
-      fallback_approvals_required > 0
-    else
-      wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 }
-    end
+    result = wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 }
+    result ||= fallback_approvals_required > 0 if regular_rules.empty?
+    result
   end
 
   def fallback_approvals_required
@@ -56,11 +54,9 @@ class ApprovalState
 
   def approved?
     strong_memoize(:approved) do
-      if regular_rules.empty?
-        approvals.size >= fallback_approvals_required
-      else
-        wrapped_approval_rules.all?(&:approved?)
-      end
+      result = wrapped_approval_rules.all?(&:approved?)
+      result &&= approvals.size >= fallback_approvals_required if regular_rules.empty?
+      result
     end
   end
 
@@ -70,11 +66,9 @@ class ApprovalState
 
   def approvals_required
     strong_memoize(:approvals_required) do
-      if regular_rules.empty?
-        [project.approvals_before_merge, merge_request.approvals_before_merge || 0].max
-      else
-        wrapped_approval_rules.sum(&:approvals_required)
-      end
+      result = wrapped_approval_rules.sum(&:approvals_required)
+      result = [result, fallback_approvals_required].max if regular_rules.empty?
+      result
     end
   end
 
@@ -82,11 +76,9 @@ class ApprovalState
   # considered approved.
   def approvals_left
     strong_memoize(:approvals_left) do
-      if regular_rules.empty?
-        [fallback_approvals_required - approved_approvers.size, 0].max
-      else
-        wrapped_approval_rules.sum(&:approvals_left)
-      end
+      result = wrapped_approval_rules.sum(&:approvals_left)
+      result = [result, fallback_approvals_required - approved_approvers.size].max if regular_rules.empty?
+      result
     end
   end
 

ee/spec/models/approval_state_spec.rb

@@ -127,64 +127,74 @@ describe ApprovalState do
     end
 
     describe '#approved?' do
-      context 'when no rules' do
-        before do
-          project.update(approvals_before_merge: 1)
-        end
+      shared_examples_for 'when rules are present' do
+        context 'when all rules are approved' do
+          before do
+            subject.wrapped_approval_rules.each do |rule|
+              create(:approval, merge_request: merge_request, user: rule.users.first)
+            end
+          end
 
-        context 'when overall_approvals_required is not met' do
-          it 'returns false' do
-            expect(subject.wrapped_approval_rules.size).to eq(0)
-            expect(subject.approved?).to eq(false)
+          it 'returns true' do
+            expect(subject.approved?).to eq(true)
           end
         end
 
-        context 'when overall_approvals_required is met' do
-          it 'returns true' do
-            create(:approval, merge_request: merge_request)
+        context 'when some rules are not approved' do
+          before do
+            allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
+          end
 
-            expect(subject.wrapped_approval_rules.size).to eq(0)
-            expect(subject.approved?).to eq(true)
+          it 'returns false' do
+            expect(subject.approved?).to eq(false)
           end
         end
       end
 
-      context 'when rules are present' do
+      shared_examples_for 'checking fallback_approvals_required' do
         before do
-          2.times { create_rule(users: [create(:user)]) }
+          project.update(approvals_before_merge: 1)
+
+          subject.wrapped_approval_rules.each do |rule|
+            allow(rule).to receive(:approved?).and_return(true)
+          end
         end
 
-        context 'when all rules are approved' do
-          before do
-            subject.wrapped_approval_rules.each do |rule|
-              create(:approval, merge_request: merge_request, user: rule.users.first)
-            end
+        context 'when it is not met' do
+          it 'returns false' do
+            expect(subject.approved?).to eq(false)
           end
+        end
 
+        context 'when it is met' do
           it 'returns true' do
+            create(:approval, merge_request: merge_request)
+
             expect(subject.approved?).to eq(true)
           end
+        end
+      end
 
-          context 'when overall_approvals_required is not met' do
-            before do
-              project.update(approvals_before_merge: 3)
-            end
+      context 'when no rules' do
+        it_behaves_like 'checking fallback_approvals_required'
+      end
 
-            it 'returns true as overall approvals_required is ignored' do
-              expect(subject.approved?).to eq(true)
-            end
-          end
+      context 'when only code owner rules present' do
+        before do
+          2.times { create_rule(users: [create(:user)], code_owner: true) }
         end
 
-        context 'when some rules are not approved' do
-          before do
-            allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
-          end
+        it_behaves_like 'when rules are present'
+        it_behaves_like 'checking fallback_approvals_required'
+      end
 
-          it 'returns false' do
-            expect(subject.approved?).to eq(false)
-          end
+      context 'when regular rules present' do
+        before do
+          project.update(approvals_before_merge: 999)
+          2.times { create_rule(users: [create(:user)]) }
         end
+
+        it_behaves_like 'when rules are present'
       end
     end
 
@@ -673,64 +683,74 @@ describe ApprovalState do
     end
 
     describe '#approved?' do
-      context 'when no rules' do
-        before do
-          project.update(approvals_before_merge: 1)
-        end
+      shared_examples_for 'when rules are present' do
+        context 'when all rules are approved' do
+          before do
+            subject.wrapped_approval_rules.each do |rule|
+              create(:approval, merge_request: merge_request, user: rule.users.first)
+            end
+          end
 
-        context 'when overall_approvals_required is not met' do
-          it 'returns false' do
-            expect(subject.wrapped_approval_rules.size).to eq(0)
-            expect(subject.approved?).to eq(false)
+          it 'returns true' do
+            expect(subject.approved?).to eq(true)
           end
         end
 
-        context 'when overall_approvals_required is met' do
-          it 'returns true' do
-            create(:approval, merge_request: merge_request)
+        context 'when some rules are not approved' do
+          before do
+            allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
+          end
 
-            expect(subject.wrapped_approval_rules.size).to eq(0)
-            expect(subject.approved?).to eq(true)
+          it 'returns false' do
+            expect(subject.approved?).to eq(false)
           end
         end
       end
 
-      context 'when rules are present' do
+      shared_examples_for 'checking fallback_approvals_required' do
         before do
-          2.times { create_rule(users: [create(:user)]) }
+          project.update(approvals_before_merge: 1)
+
+          subject.wrapped_approval_rules.each do |rule|
+            allow(rule).to receive(:approved?).and_return(true)
+          end
         end
 
-        context 'when all rules are approved' do
-          before do
-            subject.wrapped_approval_rules.each do |rule|
-              create(:approval, merge_request: merge_request, user: rule.users.first)
-            end
+        context 'when it is not met' do
+          it 'returns false' do
+            expect(subject.approved?).to eq(false)
           end
+        end
 
+        context 'when it is met' do
           it 'returns true' do
+            create(:approval, merge_request: merge_request)
+
             expect(subject.approved?).to eq(true)
           end
+        end
+      end
 
-          context 'when overall_approvals_required is not met' do
-            before do
-              project.update(approvals_before_merge: 3)
-            end
+      context 'when no rules' do
+        it_behaves_like 'checking fallback_approvals_required'
+      end
 
-            it 'returns true as overall approvals_required is ignored' do
-              expect(subject.approved?).to eq(true)
-            end
-          end
+      context 'when only code owner rules present' do
+        before do
+          2.times { create_rule(users: [create(:user)], code_owner: true) }
         end
 
-        context 'when some rules are not approved' do
-          before do
-            allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
-          end
+        it_behaves_like 'when rules are present'
+        it_behaves_like 'checking fallback_approvals_required'
+      end
 
-          it 'returns false' do
-            expect(subject.approved?).to eq(false)
-          end
+      context 'when regular rules present' do
+        before do
+          project.update(approvals_before_merge: 999)
+          2.times { create_rule(users: [create(:user)]) }
         end
+
+        it_behaves_like 'when rules are present'
       end
     end