Merge branch '959-too-many-approvers-on-merge-request' into 'master'

Ensure an MR never has negative approvals left

Closes #959.

See merge request !708

app/models/merge_request.rb

@@ -644,7 +644,10 @@ class MergeRequest < ActiveRecord::Base
   # choose the lower so the MR doesn't get 'stuck' in a state where it can't be approved.
   #
   def approvals_left
-    [approvals_required - approvals.count, number_of_potential_approvers].min
+    [
+      [approvals_required - approvals.count, number_of_potential_approvers].min,
+      0
+    ].max
   end
 
   def approvals_required

spec/models/merge_request_spec.rb

@@ -1010,6 +1010,22 @@ describe MergeRequest, models: true do
               expect(merge_request.can_approve?(nil)).to be_falsey
             end
           end
+
+          context 'when more than the number of approvers have approved the MR' do
+            before do
+              create(:approval, user: approver, merge_request: merge_request)
+              create(:approval, user: approver_2, merge_request: merge_request)
+              create(:approval, user: developer, merge_request: merge_request)
+            end
+
+            it 'marks the MR as approved' do
+              expect(merge_request).to be_approved
+            end
+
+            it 'clamps the approvals left at zero' do
+              expect(merge_request.approvals_left).to eq(0)
+            end
+          end
         end
 
         context 'when the approvers do not contain the MR author' do

spec/services/merge_requests/approval_service_spec.rb

@@ -29,12 +29,16 @@ describe MergeRequests::ApprovalService, services: true do
 
     context 'with valid approval' do
       it 'creates an approval note' do
+        allow(merge_request).to receive(:approvals_left).and_return(1)
+
         expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user)
 
         service.execute(merge_request)
       end
 
       it 'marks pending todos as done' do
+        allow(merge_request).to receive(:approvals_left).and_return(1)
+
         service.execute(merge_request)
 
         expect(todo.reload).to be_done