Merge branch 'prevent-author-from-being-an-approver' into 'master'

Prevent author from being an approver

@dbalexandre can you review this please? Background: https://gitlab.com/gitlab-org/gitlab-ee/issues/388#note_12743940

I'd like to apply commit https://gitlab.com/gitlab-org/gitlab-ee/commit/3758f751906ad76a47a40c653c1b07c767772260 to CE, as it doesn't need to be EE-specific. I'd also really like to not have to do https://gitlab.com/gitlab-org/gitlab-ee/commit/ee751a5963937c25ba456311d26bf4103946ac5f but that comment requires it 😕 

See merge request !560

CHANGELOG-EE

@@ -4,6 +4,7 @@ v 8.10.0 (unreleased)
   - Rename Git Hooks to Push Rules
   - Fix EE keys fingerprint add index migration if came from CE
   - Add todos for MR approvers !547
+  - Prevent the author of an MR from being on the approvers list
   - Isolate EE LDAP library code in EE module (Part 1) !511
   - Make Elasticsearch indexer run as an async task
 

app/assets/javascripts/users_select.js.coffee

@@ -220,6 +220,7 @@ class @UsersSelect
       @showCurrentUser = $(select).data('current-user')
       @pushCodeToProtectedBranches = $(select).data('push-code-to-protected-branches')
       @authorId = $(select).data('author-id')
+      @skipUsers = $(select).data('skip-users')
       showNullUser = $(select).data('null-user')
       showAnyUser = $(select).data('any-user')
       showEmailUser = $(select).data('email-user')
@@ -328,6 +329,7 @@ class @UsersSelect
         current_user: @showCurrentUser
         push_code_to_protected_branches: @pushCodeToProtectedBranches
         author_id: @authorId
+        skip_users: @skipUsers
       dataType: "json"
     ).done (users) ->
       callback(users)

app/controllers/autocomplete_controller.rb

@@ -5,6 +5,7 @@ class AutocompleteController < ApplicationController
   def users
     @users = @users.non_ldap if params[:skip_ldap] == 'true'
     @users = @users.search(params[:search]) if params[:search].present?
+    @users = @users.where.not(id: params[:skip_users]) if params[:skip_users].present?
     @users = @users.active
     @users = @users.reorder(:name)
 

app/controllers/projects/merge_requests_controller.rb

@@ -462,8 +462,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def set_suggested_approvers
     if @merge_request.requires_approve?
       @suggested_approvers = Gitlab::AuthorityAnalyzer.new(
-        @merge_request,
-        current_user
+        @merge_request
       ).calculate(@merge_request.approvals_required)
     end
   end

app/helpers/selects_helper.rb

@@ -5,21 +5,9 @@ module SelectsHelper
     css_class << "skip_ldap " if opts[:skip_ldap]
     css_class << (opts[:class] || '')
     value = opts[:selected] || ''
-
-    first_user = opts[:first_user] && current_user ? current_user.username : false
-
     html = {
       class: css_class,
-      data: {
-        placeholder: opts[:placeholder]   || 'Search for a user',
-        null_user: opts[:null_user]       || false,
-        any_user: opts[:any_user]         || false,
-        email_user: opts[:email_user]     || false,
-        first_user: first_user,
-        current_user: opts[:current_user] || false,
-        "push-code-to-protected-branches" => opts[:push_code_to_protected_branches],
-        author_id: opts[:author_id] || ''
-      }
+      data: users_select_data_attributes(opts)
     }
 
     unless opts[:scope] == :all
@@ -85,4 +73,20 @@ module SelectsHelper
 
     hidden_field_tag(id, value, class: css_class)
   end
+
+  private
+
+  def users_select_data_attributes(opts)
+    {
+      placeholder: opts[:placeholder]   || 'Search for a user',
+      null_user: opts[:null_user]       || false,
+      any_user: opts[:any_user]         || false,
+      email_user: opts[:email_user]     || false,
+      first_user: opts[:first_user] && current_user ? current_user.username : false,
+      current_user: opts[:current_user] || false,
+      "push-code-to-protected-branches" => opts[:push_code_to_protected_branches],
+      author_id: opts[:author_id] || '',
+      skip_users: opts[:skip_users] ? opts[:skip_users].map(&:id) : nil,
+    }
+  end
 end

app/models/merge_request.rb

@@ -112,7 +112,7 @@ class MergeRequest < ActiveRecord::Base
   scope :references_project, -> { references(:target_project) }
 
   participant :approvers_left
- 
+
   after_save :keep_around_commit
 
   def self.reference_prefix
@@ -560,52 +560,95 @@ class MergeRequest < ActiveRecord::Base
     locked_at.nil? || locked_at < (Time.now - 1.day)
   end
 
-  def approvals_left
-    approvals_required - approvals.count
+  def requires_approve?
+    approvals_required.nonzero?
   end
 
-  def approvers_left
-    user_ids = overall_approvers.map(&:user_id) - approvals.map(&:user_id)
-    User.where id: user_ids
+  def approved?
+    approvals_left < 1
+  end
+
+  # Number of approvals remaining (excluding existing approvals) before the MR is
+  # considered approved. If there are fewer potential approvers than approvals left,
+  # 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
   end
 
   def approvals_required
     approvals_before_merge || target_project.approvals_before_merge
   end
 
-  def requires_approve?
-    approvals_required.nonzero?
-  end
+  # An MR can potentially be approved by:
+  # - anyone in the approvers list
+  # - any other project member with developer access or higher (if there are no approvers
+  #   left)
+  #
+  # It cannot be approved by:
+  # - a user who has already approved the MR
+  # - the MR author
+  #
+  def number_of_potential_approvers
+    has_access = ['access_level > ?', Member::REPORTER]
+    wheres = [
+      "id IN (#{overall_approvers.select(:user_id).to_sql})",
+      "id IN (#{project.members.where(has_access).select(:user_id).to_sql})"
+    ]
 
-  def overall_approvers
-    if approvers.any?
-      approvers
-    else
-      target_project.approvers
+    if project.group
+      wheres << "id IN (#{project.group.members.where(has_access).select(:user_id).to_sql})"
     end
-  end
 
-  def approved?
-    approvals_left < 1
+    User.where("(#{wheres.join(' OR ')}) AND id NOT IN (#{approvals.select(:user_id).to_sql}) AND id != #{author.id}").count
   end
 
-  def approved_by?(user)
-    approved_by_users.include?(user)
+  # Users in the list of approvers who have not already approved this MR.
+  #
+  def approvers_left
+    User.where(id: overall_approvers.select(:user_id)).where.not(id: approvals.select(:user_id))
   end
 
-  def approved_by_users
-    approvals.map(&:user)
+  # The list of approvers from either this MR (if they've been set on the MR) or the
+  # target project. Excludes the author by default.
+  #
+  # Before a merge request has been created, author will be nil, so pass the current user
+  # on the MR create page.
+  #
+  def overall_approvers(exclude_user: nil)
+    exclude_user ||= author
+    approvers_relation = approvers.any? ? approvers : target_project.approvers
+
+    exclude_user ? approvers_relation.where.not(user_id: exclude_user.id) : approvers_relation
   end
 
   def can_approve?(user)
-    approvers_left.include?(user) ||
-    (any_approver_allowed? && !approved_by?(user))
+    return true if approvers_left.include?(user)
+    return false if user == author
+    return false unless user.can?(:update_merge_request, self)
+
+    any_approver_allowed? && approvals.where(user: user).empty?
   end
 
+  # Once there are fewer approvers left in the list than approvals required, allow other
+  # project members to approve the MR.
+  #
   def any_approver_allowed?
     approvals_left > approvers_left.count
   end
 
+  def approved_by_users
+    approvals.map(&:user)
+  end
+
+  def approver_ids=(value)
+    value.split(",").map(&:strip).each do |user_id|
+      next if author && user_id == author.id
+
+      approvers.find_or_initialize_by(user_id: user_id, target_id: id)
+    end
+  end
+
   def has_ci?
     source_project.ci_service && commits.any?
   end
@@ -644,12 +687,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def approver_ids=(value)
-    value.split(",").map(&:strip).each do |user_id|
-      approvers.find_or_initialize_by(user_id: user_id, target_id: id)
-    end
-  end
-
   def state_icon_name
     if merged?
       "check"

app/views/shared/issuable/_form.html.haml

@@ -114,7 +114,9 @@
       = f.label :approver_ids, class: 'control-label' do
         Approvers
       .col-sm-10
-        = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
+        - author = @merge_request.author || current_user
+        - skip_users = @merge_request.overall_approvers.map(&:user) + [author]
+        = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true, skip_users: skip_users)
         .help-block
           This merge request must be approved by these users.
           You can override the project settings by setting your own list of approvers.
@@ -123,26 +125,22 @@
           .panel-heading
             Approvers
           %ul.well-list.approver-list
-            - if @merge_request.new_record?
-              - @merge_request.target_project.approvers.each do |approver|
-                %li.project-approvers{id: dom_id(approver.user)}
-                  = link_to approver.user.name, approver.user
-                  .pull-right
+            - using_project_approvers = @merge_request.approvers.empty?
+            - item_class = 'project-approvers' if using_project_approvers
+            - @merge_request.overall_approvers(exclude_user: author).each do |approver|
+              %li{id: dom_id(approver.user), class: item_class}
+                = link_to approver.user.name, approver.user
+                .pull-right
+                  - if using_project_approvers
                     = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
                       = icon("sign-out")
                       Remove
-              - if @merge_request.target_project.approvers.empty?
-                %li.no-approvers There are no approvers
-            - else
-              - @merge_request.approvers.each do |approver|
-                %li{id: dom_id(approver.user)}
-                  = link_to approver.user.name, approver.user
-                  .pull-right
+                  - else
                     = link_to namespace_project_merge_request_approver_path(@project.namespace, @project, @merge_request, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do
                       = icon("sign-out")
                       Remove
-              - if @merge_request.approvers.empty?
-                %li.no-approvers There are no approvers
+            - if @merge_request.overall_approvers.empty?
+              %li.no-approvers There are no approvers
         .help-block.suggested-approvers
           - if @suggested_approvers.any?
             Suggested approvers:

doc/workflow/merge_request_approvals.md

@@ -33,7 +33,8 @@ independent of changes to the merge request.
 ### Approvers
 
 At approvers you can define the default set of users that need to approve a
-merge request.
+merge request. The author of a merge request cannot be set as an approver for
+that merge request.
 
 If there are more approvers than required approvals, any subset of these users
 can approve the merge request.
@@ -64,4 +65,4 @@ yet the process is still enforced.
 
 To approve a merge request, simply press the button.
 
-![Merge request approval](merge_request_approvals/2_approvals.png)
\ No newline at end of file
+![Merge request approval](merge_request_approvals/2_approvals.png)

features/project/merge_requests.feature

@@ -320,28 +320,13 @@ Feature: Project Merge Requests
     And I click link "Close"
     Then I should see closed merge request "Bug NS-04"
 
-  Scenario: I approve merge request
-    Given merge request 'Bug NS-04' must be approved
-    And I click link "Bug NS-04"
-    And I should not see merge button
-    When I click link "Approve"
-    Then I should see approved merge request "Bug NS-04"
-
-  Scenario: Reporter can approve merge request
-    Given I am a "Shop" reporter
+  Scenario: Developer can approve merge request
+    Given I am a "Shop" developer
     And I visit project "Shop" merge requests page
     And merge request 'Bug NS-04' must be approved
     And I click link "Bug NS-04"
     And I should not see merge button
     When I click link "Approve"
-    Then I should see message that merge request can be merged
-
-  Scenario: I approve merge request if I am an approver
-    Given merge request 'Bug NS-04' must be approved by current user
-    And I click link "Bug NS-04"
-    And I should not see merge button
-    And I should see message that MR require an approval from me
-    When I click link "Approve"
     Then I should see approved merge request "Bug NS-04"
 
   Scenario: I can not approve merge request if I am not an approver
@@ -353,15 +338,19 @@ Feature: Project Merge Requests
 
   @javascript
   Scenario: I see suggested approvers on new merge request form
-    Given project settings contain list of approvers
+    Given I am a "Shop" developer
+    And project settings contain list of approvers
+    And I visit project "Shop" merge requests page
     When I click link "New Merge Request"
     And I select "feature_conflict" as source
     Then I see suggested approver
 
   @javascript
   Scenario: I see auto-suggested approvers on new merge request form
-    Given project settings contain list of approvers
+    Given I am a "Shop" developer
+    And project settings contain list of approvers
     And there is one auto-suggested approver
+    And I visit project "Shop" merge requests page
     When I click link "New Merge Request"
     And I select "feature_conflict" as source
     Then I see auto-suggested approver

features/steps/project/merge_requests.rb

@@ -641,10 +641,10 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     expect(page).to have_content('diff --git')
   end
 
-  step 'I am a "Shop" reporter' do
+  step 'I am a "Shop" developer' do
     user = create(:user, name: "Mike")
     project = Project.find_by(name: "Shop")
-    project.team << [user, :reporter]
+    project.team << [user, :developer]
 
     logout
     login_with user

lib/gitlab/authority_analyzer.rb

@@ -2,9 +2,8 @@ module Gitlab
   class AuthorityAnalyzer
     COMMITS_TO_CONSIDER = 5
 
-    def initialize(merge_request, current_user)
+    def initialize(merge_request)
       @merge_request = merge_request
-      @current_user = current_user
       @users = Hash.new(0)
     end
 
@@ -12,7 +11,7 @@ module Gitlab
       involved_users
 
       # Picks most active users from hash like: {user1: 2, user2: 6}
-      @users.sort_by { |user, count| count }.map(&:first).take(number_of_approvers)
+      @users.sort_by { |user, count| -count }.map(&:first).take(number_of_approvers)
     end
 
     private
@@ -22,7 +21,9 @@ module Gitlab
 
       list_of_involved_files.each do |path|
         @repo.commits(@merge_request.target_branch, path: path, limit: COMMITS_TO_CONSIDER).each do |commit|
-          @users[commit.author] += 1 if commit.author
+          if commit.author && commit.author != @merge_request.author
+            @users[commit.author] += 1
+          end
         end
       end
     end

spec/controllers/autocomplete_controller_spec.rb

@@ -176,4 +176,18 @@ describe AutocompleteController do
       expect(body.collect { |u| u['id'] }).not_to include(99999)
     end
   end
+
+  context 'skip_users parameter included' do
+    before { sign_in(user) }
+
+    let(:response_user_ids) { JSON.parse(response.body).map { |user| user['id'] } }
+
+    it 'skips the user IDs passed' do
+      get(:users, skip_users: [user, user2].map(&:id))
+
+      expect(response_user_ids).not_to include(user.id)
+      expect(response_user_ids).not_to include(user2.id)
+      expect(response_user_ids).not_to be_empty
+    end
+  end
 end

spec/features/merge_requests/approvals_spec.rb

+require 'rails_helper'
+
+feature 'Merge request approvals', js: true, feature: true do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, approvals_before_merge: 1) }
+
+  context 'when editing an MR with a different author' do
+    let(:author) { create(:user) }
+    let(:merge_request) { create(:merge_request, author: author, source_project: project) }
+
+    before do
+      project.team << [user, :developer]
+      project.team << [author, :developer]
+
+      login_as(user)
+      visit edit_namespace_project_merge_request_path(project.namespace, project, merge_request)
+
+      find('#s2id_merge_request_approver_ids .select2-input').click
+    end
+
+    it 'does not allow setting the author as an approver' do
+      expect(find('.select2-results')).not_to have_content(author.name)
+    end
+
+    it 'allows setting the current user as an approver' do
+      expect(find('.select2-results')).to have_content(user.name)
+    end
+  end
+
+  context 'when creating an MR' do
+    let(:other_user) { create(:user) }
+
+    before do
+      project.team << [user, :developer]
+      project.team << [other_user, :developer]
+
+      login_as(user)
+      visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_branch: 'feature' })
+
+      find('#s2id_merge_request_approver_ids .select2-input').click
+    end
+
+    it 'allows setting other users as approvers' do
+      expect(find('.select2-results')).to have_content(other_user.name)
+    end
+
+    it 'does not allow setting the current user as an approver' do
+      expect(find('.select2-results')).not_to have_content(user.name)
+    end
+  end
+end

spec/helpers/merge_request_helper_spec.rb

@@ -56,45 +56,47 @@ describe MergeRequestsHelper do
   end
 
   describe 'render_require_section' do
+    let(:merge_request) { create(:merge_request, target_project: project, source_project: project) }
+    let(:approver) { create(:user) }
+
+    before do
+      5.times { project.team << [create(:user), :developer] }
+      project.team << [approver, :developer]
+    end
+
     it "returns correct value in case - one approval" do
       project.update(approvals_before_merge: 1)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
+
       expect(render_require_section(merge_request)).to eq("Requires one more approval")
     end
 
     it "returns correct value in case - two approval" do
       project.update(approvals_before_merge: 2)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
+
       expect(render_require_section(merge_request)).to eq("Requires 2 more approvals")
     end
 
     it "returns correct value in case - one approver" do
       project.update(approvals_before_merge: 1)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
-      user = create :user
-      merge_request.approvers.create(user_id: user.id)
+      create(:approver, user: approver, target: merge_request)
 
-      expect(render_require_section(merge_request)).to eq("Requires one more approval (from #{user.name})")
+      expect(render_require_section(merge_request)).to eq("Requires one more approval (from #{approver.name})")
     end
 
     it "returns correct value in case - one approver and one more" do
       project.update(approvals_before_merge: 2)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
-      user = create :user
-      merge_request.approvers.create(user_id: user.id)
+      create(:approver, user: approver, target: merge_request)
 
-      expect(render_require_section(merge_request)).to eq("Requires 2 more approvals (from #{user.name} and 1 more)")
+      expect(render_require_section(merge_request)).to eq("Requires 2 more approvals (from #{approver.name} and 1 more)")
     end
 
     it "returns correct value in case - two approver and one more" do
       project.update(approvals_before_merge: 3)
-      merge_request = create(:merge_request, target_project: project, source_project: project)
-      user = create :user
-      user1 = create :user
-      merge_request.approvers.create(user_id: user.id)
-      merge_request.approvers.create(user_id: user1.id)
+      approver2 = create(:user)
+      create(:approver, user: approver, target: merge_request)
+      create(:approver, user: approver2, target: merge_request)
 
-      expect(render_require_section(merge_request)).to eq("Requires 3 more approvals (from #{user1.name}, #{user.name} and 1 more)")
+      expect(render_require_section(merge_request)).to eq("Requires 3 more approvals (from #{approver2.name}, #{approver.name} and 1 more)")
     end
   end
 end

spec/lib/gitlab/authority_analyzer_spec.rb

+require 'spec_helper'
+
+describe Gitlab::AuthorityAnalyzer, lib: true do
+  describe '#calculate' do
+    let(:project) { create(:project) }
+    let(:author) { create(:user) }
+    let(:user_a) { create(:user) }
+    let(:user_b) { create(:user) }
+    let(:merge_request) { create(:merge_request, target_project: project, source_project: project, author: author) }
+    let(:files) { [double(:file, deleted_file: true, old_path: 'foo')] }
+
+    let(:commits) do
+      [
+        double(:commit, author: author),
+        double(:commit, author: user_a),
+        double(:commit, author: user_a),
+        double(:commit, author: user_b),
+        double(:commit, author: author)
+      ]
+    end
+
+    let(:approvers) { Gitlab::AuthorityAnalyzer.new(merge_request).calculate(number_of_approvers) }
+
+    before do
+      merge_request.compare = double(:compare, diffs: files)
+      allow(merge_request.target_project.repository).to receive(:commits).and_return(commits)
+    end
+
+    context 'when there are fewer contributors than requested' do
+      let(:number_of_approvers) { 5 }
+
+      it 'returns the full number of users' do
+        expect(approvers.length).to eq(2)
+      end
+    end
+
+    context 'when there are more contributors than requested' do
+      let(:number_of_approvers) { 1 }
+
+      it 'returns only the top n contributors' do
+        expect(approvers).to contain_exactly(user_a)
+      end
+    end
+  end
+end

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 "#number_of_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.number_of_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.number_of_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.number_of_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.number_of_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.number_of_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) }
@@ -707,4 +757,175 @@ describe MergeRequest, models: true do
       subject.reload_diff
     end
   end
+
+  describe 'approvals' do
+    let(:project) { create(:empty_project) }
+    let(:merge_request) { create(:merge_request, source_project: project, author: author) }
+    let(:author) { create(:user) }
+    let(:approver) { create(:user) }
+
+    context 'on a project with only one member' 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]
+            create(:approver, user: approver, target: merge_request)
+          end
+
+          it 'requires one approval' do
+            expect(merge_request.approvals_left).to eq(1)
+          end
+
+          it 'allows the approver to approve the MR' do
+            expect(merge_request.can_approve?(approver)).to be_truthy
+          end
+        end
+      end
+    end
+
+    context 'on a project with several members' do
+      let(:approver_2) { create(:user) }
+      let(:developer) { create(:user) }
+      let(:reporter) { create(:user) }
+      let(:stranger) { create(:user) }
+
+      before do
+        project.team << [author, :developer]
+        project.team << [approver, :developer]
+        project.team << [approver_2, :developer]
+        project.team << [developer, :developer]
+        project.team << [reporter, :reporter]
+      end
+
+      context 'when there is one approver required' do
+        before { project.update_attributes(approvals_before_merge: 1) }
+
+        context 'when that approver is the MR author' do
+          before { create(:approver, user: author, target: merge_request) }
+
+          it 'requires one approval' do
+            expect(merge_request.approvals_left).to eq(1)
+          end
+
+          it 'does not allow the author to approve the MR' do
+            expect(merge_request.can_approve?(author)).to be_falsey
+          end
+
+          it 'allows any other project member with write access to approve the MR' do
+            expect(merge_request.can_approve?(developer)).to be_truthy
+
+            expect(merge_request.can_approve?(reporter)).to be_falsey
+            expect(merge_request.can_approve?(stranger)).to be_falsey
+          end
+        end
+
+        context 'when that approver is not the MR author' do
+          before { create(:approver, user: approver, target: merge_request) }
+
+          it 'requires one approval' do
+            expect(merge_request.approvals_left).to eq(1)
+          end
+
+          it 'only allows the approver to approve the MR' do
+            expect(merge_request.can_approve?(approver)).to be_truthy
+
+            expect(merge_request.can_approve?(author)).to be_falsey
+            expect(merge_request.can_approve?(developer)).to be_falsey
+            expect(merge_request.can_approve?(reporter)).to be_falsey
+            expect(merge_request.can_approve?(stranger)).to be_falsey
+          end
+        end
+      end
+
+      context 'when there are multiple approvers required' do
+        before { project.update_attributes(approvals_before_merge: 3) }
+
+        context 'when one of those approvers is the MR author' do
+          before do
+            create(:approver, user: author, target: merge_request)
+            create(:approver, user: approver, target: merge_request)
+            create(:approver, user: approver_2, target: merge_request)
+          end
+
+          it 'requires the original number of approvals' do
+            expect(merge_request.approvals_left).to eq(3)
+          end
+
+          it 'does not allow the author to approve the MR' do
+            expect(merge_request.can_approve?(author)).to be_falsey
+          end
+
+          it 'allows any other other approver to approve the MR' do
+            expect(merge_request.can_approve?(approver)).to be_truthy
+          end
+
+          context 'when all of the valid 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)
+            end
+
+            it 'requires the original number of approvals' do
+              expect(merge_request.approvals_left).to eq(1)
+            end
+
+            it 'does not allow the author to approve the MR' do
+              expect(merge_request.can_approve?(author)).to be_falsey
+            end
+
+            it 'does not allow the approvers to approve the MR again' do
+              expect(merge_request.can_approve?(approver)).to be_falsey
+              expect(merge_request.can_approve?(approver_2)).to be_falsey
+            end
+
+            it 'allows any other project member with write access to approve the MR' do
+              expect(merge_request.can_approve?(developer)).to be_truthy
+
+              expect(merge_request.can_approve?(reporter)).to be_falsey
+              expect(merge_request.can_approve?(stranger)).to be_falsey
+            end
+          end
+        end
+
+        context 'when the approvers do not contain the MR author' do
+          before do
+            create(:approver, user: developer, target: merge_request)
+            create(:approver, user: approver, target: merge_request)
+            create(:approver, user: approver_2, target: merge_request)
+          end
+
+          it 'requires the original number of approvals' do
+            expect(merge_request.approvals_left).to eq(3)
+          end
+
+          it 'only allows the approvers to approve the MR' do
+            expect(merge_request.can_approve?(developer)).to be_truthy
+            expect(merge_request.can_approve?(approver)).to be_truthy
+            expect(merge_request.can_approve?(approver_2)).to be_truthy
+
+            expect(merge_request.can_approve?(author)).to be_falsey
+            expect(merge_request.can_approve?(reporter)).to be_falsey
+            expect(merge_request.can_approve?(stranger)).to be_falsey
+          end
+        end
+      end
+    end
+  end
 end

spec/requests/api/merge_requests_spec.rb

@@ -704,14 +704,31 @@ describe API::API, api: true  do
   end
 
   describe 'POST :id/merge_requests/:merge_request_id/approve' do
-    it 'approves the merge request' do
-      project.update_attribute(:approvals_before_merge, 2)
+    before { project.update_attribute(:approvals_before_merge, 2) }
+
+    context 'as the author of the merge request' do
+      before { post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user) }
+
+      it 'returns a 401' do
+        expect(response).to have_http_status(401)
+      end
+    end
 
-      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user)
+    context 'as a valid approver' do
+      let(:approver) { create(:user) }
 
-      expect(response.status).to eq(201)
-      expect(json_response['approvals_left']).to eq(1)
-      expect(json_response['approved_by'][0]['user']['username']).to eq(user.username)
+      before do
+        project.team << [approver, :developer]
+        project.team << [create(:user), :developer]
+
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", approver)
+      end
+
+      it 'approves the merge request' do
+        expect(response.status).to eq(201)
+        expect(json_response['approvals_left']).to eq(1)
+        expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
+      end
     end
   end