Ensure MR approvals don't get stuck

The potential approvers of an MR are:

- The users in the approvers list (excluding other permissions, as long
  as they can see the MR).
- Users with developer access or higher on the project or the group
  containing the project.

This number is reduced by:

- The author not being able to approve their own MR.
- Users who have already approved the MR.

If the number of potential approvers is less than the number of
approvals left by the normal count, pick the number of potential
approvers instead, so the MR isn't 'stuck' in a state where it needs
approval but no-one can approve it.

app/models/merge_request.rb

@@ -561,7 +561,20 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def approvals_left
-    approvals_required - approvals.count
+    [approvals_required - approvals.count, potential_approvers].min
+  end
+
+  def potential_approvers
+    wheres = [
+      "id IN (#{overall_approvers.select(:user_id).to_sql})",
+      "id IN (#{project.members.where(['access_level > ?', Member::REPORTER]).select(:user_id).to_sql})"
+    ]
+
+    if project.group
+      wheres << "id IN (#{project.group.members.where(['access_level > ?', Member::REPORTER]).select(:user_id).to_sql})"
+    end
+
+    User.count_by_sql("SELECT COUNT(*) FROM users WHERE (#{wheres.join(' OR ')}) AND id NOT IN (#{approvals.select(:user_id).to_sql}) AND id != #{author.id}")
   end
 
   def approvers_left

spec/models/merge_request_spec.rb

@@ -250,7 +250,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe "approvers_left" do
+  describe "#approvers_left" do
     let(:merge_request) {create :merge_request}
 
     it "returns correct value" do
@@ -264,6 +264,56 @@ describe MergeRequest, models: true do
     end
   end
 
+  describe "#potential_approvers" do
+    let(:project) { create(:empty_project) }
+    let(:author) { create(:user) }
+    let(:merge_request) { create(:merge_request, source_project: project, author: author) }
+
+    it "includes approvers set on the MR" do
+      expect do
+        create(:approver, user: create(:user), target: merge_request)
+      end.to change { merge_request.potential_approvers }.by(1)
+    end
+
+    it "includes project members with developer access and up" do
+      expect do
+        project.team << [create(:user), :guest]
+        project.team << [create(:user), :reporter]
+        project.team << [create(:user), :developer]
+        project.team << [create(:user), :master]
+      end.to change { merge_request.potential_approvers }.by(2)
+    end
+
+    it "excludes users who have already approved the MR" do
+      expect do
+        approver = create(:user)
+        create(:approver, user: approver, target: merge_request)
+        create(:approval, user: approver, merge_request: merge_request)
+      end.not_to change { merge_request.potential_approvers }
+    end
+
+    it "excludes the MR author" do
+      expect do
+        create(:approver, user: create(:user), target: merge_request)
+        create(:approver, user: author, target: merge_request)
+      end.to change { merge_request.potential_approvers }.by(1)
+    end
+
+    context "when the project is part of a group" do
+      let(:group) { create(:group) }
+      before { project.update_attributes(group: group) }
+
+      it "includes group members with developer access and up" do
+        expect do
+          group.add_guest(create(:user))
+          group.add_reporter(create(:user))
+          group.add_developer(create(:user))
+          group.add_master(create(:user))
+        end.to change { merge_request.potential_approvers }.by(2)
+      end
+    end
+  end
+
   describe "#approvals_required" do
     let(:merge_request) { build(:merge_request) }
     before { merge_request.target_project.update_attributes(approvals_before_merge: 3) }
@@ -718,6 +768,21 @@ describe MergeRequest, models: true do
       context 'when there is one approver' do
         before { project.update_attributes(approvals_before_merge: 1) }
 
+        context 'when that approver is the MR author' do
+          before do
+            project.team << [author, :developer]
+            create(:approver, user: author, target: merge_request)
+          end
+
+          it 'does not require approval for the merge request' do
+            expect(merge_request.approvals_left).to eq(0)
+          end
+
+          it 'does not allow the approver to approve the MR' do
+            expect(merge_request.can_approve?(author)).to be_falsey
+          end
+        end
+
         context 'when that approver is not the MR author' do
           before do
             project.team << [approver, :developer]