Fix ApprovalState approval_needed?

This is supposed to be a static state,
not depending on current approvals count.

ee/app/models/approval_state.rb

@@ -45,7 +45,7 @@ class ApprovalState
   def approval_needed?
     return false unless project.feature_available?(:merge_request_approvers)
 
-    !approved?
+    overall_approvals_required > 0 || wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 }
   end
 
   def overall_approvals_required

ee/spec/models/approval_state_spec.rb

@@ -91,57 +91,97 @@ describe ApprovalState do
         end
       end
 
-      context 'when approved' do
-        it 'returns false' do
-          expect(subject).to receive(:approved?).and_return(true)
+      context 'when overall approvals required is not zero' do
+        before do
+          project.update(approvals_before_merge: 1)
+        end
 
-          expect(subject.approval_needed?).to eq(false)
+        it 'returns true' do
+          expect(subject.approval_needed?).to eq(true)
         end
+      end
 
-        context 'when overall_approvals_required is not met' do
-          before do
-            project.update(approvals_before_merge: 1)
-          end
+      context "when any rule's approvals required is not zero" do
+        it 'returns false' do
+          create_rule(approvals_required: 1)
 
-          it 'returns false' do
-            expect(subject.approved?).to eq(false)
-          end
+          expect(subject.approval_needed?).to eq(true)
         end
       end
 
-      context 'when not approved' do
-        it 'returns true' do
-          expect(subject).to receive(:approved?).and_return(false)
+      context "when overall approvals required and all rule's approvals_required are zero" do
+        it 'returns false' do
+          create_rule(approvals_required: 0)
 
-          expect(subject.approval_needed?).to eq(true)
+          expect(subject.approval_needed?).to eq(false)
         end
       end
-    end
 
-    describe '#approved?' do
-      before do
-        2.times { create_rule }
+      context "when overall approvals required is zero, and there is no rule" do
+        it 'returns false' do
+          expect(subject.approval_needed?).to eq(false)
+        end
       end
+    end
 
-      context 'when all rules are approved' do
+    describe '#approved?' do
+      context 'when no rules' do
         before do
-          subject.wrapped_approval_rules.each do |rule|
-            allow(rule).to receive(:approved?).and_return(true)
+          project.update(approvals_before_merge: 1)
+        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)
           end
         end
 
-        it 'returns true' do
-          expect(subject.approved?).to eq(true)
+        context 'when overall_approvals_required is met' do
+          it 'returns true' do
+            create(:approval, merge_request: merge_request)
+
+            expect(subject.wrapped_approval_rules.size).to eq(0)
+            expect(subject.approved?).to eq(true)
+          end
         end
       end
 
-      context 'when some rules are not approved' do
+      context 'when rules are present' do
         before do
-          allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
+          2.times { create_rule(users: [create(:user)]) }
         end
 
-        it 'returns false' do
-          expect(subject.approved?).to eq(false)
+        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
+
+          it 'returns true' do
+            expect(subject.approved?).to eq(true)
+          end
+
+          context 'when overall_approvals_required is not met' do
+            before do
+              project.update(approvals_before_merge: 3)
+            end
+
+            it 'returns false' do
+              expect(subject.approved?).to eq(false)
+            end
+          end
+        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 'returns false' do
+            expect(subject.approved?).to eq(false)
+          end
         end
       end
     end
@@ -595,57 +635,97 @@ describe ApprovalState do
         end
       end
 
-      context 'when approved' do
-        it 'returns false' do
-          expect(subject).to receive(:approved?).and_return(true)
+      context 'when overall approvals required is not zero' do
+        before do
+          project.update(approvals_before_merge: 1)
+        end
 
-          expect(subject.approval_needed?).to eq(false)
+        it 'returns true' do
+          expect(subject.approval_needed?).to eq(true)
         end
       end
 
-      context 'when not approved' do
-        it 'returns true' do
-          expect(subject).to receive(:approved?).and_return(false)
+      context "when any rule's approvals required is not zero" do
+        it 'returns false' do
+          create_rule(approvals_required: 1)
 
           expect(subject.approval_needed?).to eq(true)
         end
       end
-    end
 
-    describe '#approved?' do
-      before do
-        2.times { create_rule }
+      context "when overall approvals required and all rule's approvals_required are zero" do
+        it 'returns false' do
+          create_rule(approvals_required: 0)
+
+          expect(subject.approval_needed?).to eq(false)
+        end
       end
 
-      context 'when all rules are approved' do
-        before do
-          subject.wrapped_approval_rules.each do |rule|
-            allow(rule).to receive(:approved?).and_return(true)
-          end
+      context "when overall approvals required is zero, and there is no rule" do
+        it 'returns false' do
+          expect(subject.approval_needed?).to eq(false)
         end
+      end
+    end
 
-        it 'returns true' do
-          expect(subject.approved?).to eq(true)
+    describe '#approved?' do
+      context 'when no rules' do
+        before do
+          project.update(approvals_before_merge: 1)
         end
 
         context 'when overall_approvals_required is not met' do
-          before do
-            project.update(approvals_before_merge: 1)
-          end
-
           it 'returns false' do
+            expect(subject.wrapped_approval_rules.size).to eq(0)
             expect(subject.approved?).to eq(false)
           end
         end
+
+        context 'when overall_approvals_required is met' do
+          it 'returns true' do
+            create(:approval, merge_request: merge_request)
+
+            expect(subject.wrapped_approval_rules.size).to eq(0)
+            expect(subject.approved?).to eq(true)
+          end
+        end
       end
 
-      context 'when some rules are not approved' do
+      context 'when rules are present' do
         before do
-          allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
+          2.times { create_rule(users: [create(:user)]) }
         end
 
-        it 'returns false' do
-          expect(subject.approved?).to eq(false)
+        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
+
+          it 'returns true' do
+            expect(subject.approved?).to eq(true)
+          end
+
+          context 'when overall_approvals_required is not met' do
+            before do
+              project.update(approvals_before_merge: 3)
+            end
+
+            it 'returns false' do
+              expect(subject.approved?).to eq(false)
+            end
+          end
+        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 'returns false' do
+            expect(subject.approved?).to eq(false)
+          end
         end
       end
     end