From 8bdf633f51c4624af95dbb0150694084d20c3de4 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Mon, 13 Jun 2016 15:47:17 +0100
Subject: [PATCH 1/8] Fix suggested approvers

Previously, this would pick the least-frequent contributors to the
changed files, rather than the most frequent.
---
 .../projects/merge_requests_controller.rb     |  3 +-
 lib/gitlab/authority_analyzer.rb              |  9 ++--
 spec/lib/gitlab/authority_analyzer_spec.rb    | 45 +++++++++++++++++++
 3 files changed, 51 insertions(+), 6 deletions(-)
 create mode 100644 spec/lib/gitlab/authority_analyzer_spec.rb

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 5e86de1aee2ada..083b39fc0190c7 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -465,8 +465,7 @@ def invalid_mr
   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
diff --git a/lib/gitlab/authority_analyzer.rb b/lib/gitlab/authority_analyzer.rb
index a6fea82e754b11..01d43b72015057 100644
--- a/lib/gitlab/authority_analyzer.rb
+++ b/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 @@ def calculate(number_of_approvers)
       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 @@ def involved_users
 
       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
diff --git a/spec/lib/gitlab/authority_analyzer_spec.rb b/spec/lib/gitlab/authority_analyzer_spec.rb
new file mode 100644
index 00000000000000..19ec2deedc2dfd
--- /dev/null
+++ b/spec/lib/gitlab/authority_analyzer_spec.rb
@@ -0,0 +1,45 @@
+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
-- 
GitLab


From 3758f751906ad76a47a40c653c1b07c767772260 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 13 Jul 2016 15:38:21 +0100
Subject: [PATCH 2/8] Allow skipping users in autocomplete

Pass an array of user IDs in the `skip_users` param to have them
excluded from the results (unless they are explicitly included through
the `current_user` or `author_id` params).
---
 app/assets/javascripts/users_select.js.coffee |  2 ++
 app/controllers/autocomplete_controller.rb    |  1 +
 app/helpers/selects_helper.rb                 | 30 +++++++++++--------
 .../autocomplete_controller_spec.rb           | 14 +++++++++
 4 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/app/assets/javascripts/users_select.js.coffee b/app/assets/javascripts/users_select.js.coffee
index f4d9b100d53e77..a0659adb914309 100644
--- a/app/assets/javascripts/users_select.js.coffee
+++ b/app/assets/javascripts/users_select.js.coffee
@@ -213,6 +213,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')
@@ -321,6 +322,7 @@ class @UsersSelect
         current_user: @showCurrentUser
         push_code_to_protected_branches: @pushCodeToProtectedBranches
         author_id: @authorId
+        skip_users: @skipUsers
       dataType: "json"
     ).done (users) ->
       callback(users)
diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb
index d2ff371f0cf0c8..5a5fd5539440e5 100644
--- a/app/controllers/autocomplete_controller.rb
+++ b/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)
 
diff --git a/app/helpers/selects_helper.rb b/app/helpers/selects_helper.rb
index 1a2928fd08f263..094b945743e56f 100644
--- a/app/helpers/selects_helper.rb
+++ b/app/helpers/selects_helper.rb
@@ -5,21 +5,9 @@ def users_select_tag(id, opts = {})
     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 @@ def admin_email_select_tag(id, opts = {})
 
     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
diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb
index 546bf09ec977e5..b03133f40de639 100644
--- a/spec/controllers/autocomplete_controller_spec.rb
+++ b/spec/controllers/autocomplete_controller_spec.rb
@@ -176,4 +176,18 @@
       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
-- 
GitLab


From a250142ccd6b9ff38b2f5ba69c5719ea89c73612 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 13 Jul 2016 16:56:22 +0100
Subject: [PATCH 3/8] Prevent author from approving their own MR

- Don't include the author when checking approvers remaining.
- Don't allow the author to be added as an approver when editing an MR.
- Don't allow the current user (who will be the author) to be added as
  an approver when creating an MR.
---
 CHANGELOG-EE                                  |   1 +
 app/models/merge_request.rb                   |  26 +--
 app/views/shared/issuable/_form.html.haml     |  28 ++--
 doc/workflow/merge_request_approvals.md       |   5 +-
 features/project/merge_requests.feature       |  27 +--
 features/steps/project/merge_requests.rb      |   4 +-
 .../features/merge_requests/approvals_spec.rb |  51 ++++++
 spec/helpers/merge_request_helper_spec.rb     |  34 ++--
 spec/models/merge_request_spec.rb             | 156 ++++++++++++++++++
 spec/requests/api/merge_requests_spec.rb      |  29 +++-
 10 files changed, 290 insertions(+), 71 deletions(-)
 create mode 100644 spec/features/merge_requests/approvals_spec.rb

diff --git a/CHANGELOG-EE b/CHANGELOG-EE
index 79f96f9c3117de..5c010cf9f9373f 100644
--- a/CHANGELOG-EE
+++ b/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
 
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 0d0ef993ee8b9c..251b483633658d 100644
--- a/app/models/merge_request.rb
+++ b/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
@@ -565,8 +565,7 @@ def approvals_left
   end
 
   def approvers_left
-    user_ids = overall_approvers.map(&:user_id) - approvals.map(&:user_id)
-    User.where id: user_ids
+    User.where(id: overall_approvers.select(:user_id)).where.not(id: approvals.select(:user_id))
   end
 
   def approvals_required
@@ -577,12 +576,14 @@ def requires_approve?
     approvals_required.nonzero?
   end
 
-  def overall_approvers
-    if approvers.any?
-      approvers
-    else
-      target_project.approvers
-    end
+  # 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 approved?
@@ -598,8 +599,9 @@ def approved_by_users
   end
 
   def can_approve?(user)
-    approvers_left.include?(user) ||
-    (any_approver_allowed? && !approved_by?(user))
+    return true if approvers_left.include?(user)
+
+    any_approver_allowed? && !approved_by?(user) && user != author && user.can?(:update_merge_request, self)
   end
 
   def any_approver_allowed?
@@ -640,6 +642,8 @@ def state_human_name
 
   def approver_ids=(value)
     value.split(",").map(&:strip).each do |user_id|
+      next if user_id == author.id
+
       approvers.find_or_initialize_by(user_id: user_id, target_id: id)
     end
   end
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index c1f43197609dd5..41e3b32de7e12f 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -134,7 +134,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.
@@ -143,26 +145,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:
diff --git a/doc/workflow/merge_request_approvals.md b/doc/workflow/merge_request_approvals.md
index dc24ad4617d282..5fc32030597b6c 100644
--- a/doc/workflow/merge_request_approvals.md
+++ b/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)
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index d827b43a94121c..e1d5f9591db204 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -320,15 +320,8 @@ 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"
@@ -336,14 +329,6 @@ Feature: Project Merge Requests
     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
     Given merge request 'Bug NS-04' must be approved by some user
     And I click link "Bug NS-04"
@@ -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
diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index d2a73d277d7f14..9061c333986b04 100644
--- a/features/steps/project/merge_requests.rb
+++ b/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
diff --git a/spec/features/merge_requests/approvals_spec.rb b/spec/features/merge_requests/approvals_spec.rb
new file mode 100644
index 00000000000000..9c7eb1a418be59
--- /dev/null
+++ b/spec/features/merge_requests/approvals_spec.rb
@@ -0,0 +1,51 @@
+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
diff --git a/spec/helpers/merge_request_helper_spec.rb b/spec/helpers/merge_request_helper_spec.rb
index 700548cdfc87fc..287747aa1df365 100644
--- a/spec/helpers/merge_request_helper_spec.rb
+++ b/spec/helpers/merge_request_helper_spec.rb
@@ -56,45 +56,47 @@
   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
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 15656df44a9838..79d6f118316434 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -707,4 +707,160 @@
       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 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
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 8b5d405e2ca566..dc9779e111abd8 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/spec/requests/api/merge_requests_spec.rb
@@ -704,14 +704,31 @@ def create_merge_request(approvals_before_merge)
   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
 
-- 
GitLab


From ee751a5963937c25ba456311d26bf4103946ac5f Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Thu, 14 Jul 2016 14:11:00 +0100
Subject: [PATCH 4/8] 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       | 15 ++++++-
 spec/models/merge_request_spec.rb | 67 ++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 251b483633658d..cbe2cb4a00c85e 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -561,7 +561,20 @@ def locked_long_ago?
   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
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 79d6f118316434..93bc24d85e42f2 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -250,7 +250,7 @@
     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 @@
     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 @@
       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]
-- 
GitLab


From 904b4d3900f02a9ae6d2e1410c95a05d162c6829 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 15 Jul 2016 09:00:59 +0100
Subject: [PATCH 5/8] fixup! Prevent author from approving their own MR

---
 app/models/merge_request.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index cbe2cb4a00c85e..444768c9605210 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -613,8 +613,10 @@ def approved_by_users
 
   def can_approve?(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? && !approved_by?(user) && user != author && user.can?(:update_merge_request, self)
+    any_approver_allowed? && !approved_by?(user)
   end
 
   def any_approver_allowed?
-- 
GitLab


From f8908e680b878ac7e9c7ac89135c36b6bd39566c Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 15 Jul 2016 09:02:15 +0100
Subject: [PATCH 6/8] fixup! Ensure MR approvals don't get stuck

---
 app/models/merge_request.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 444768c9605210..1e0038dd957cc9 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -565,13 +565,14 @@ def approvals_left
   end
 
   def potential_approvers
+    has_access = ['access_level > ?', Member::REPORTER]
     wheres = [
       "id IN (#{overall_approvers.select(:user_id).to_sql})",
-      "id IN (#{project.members.where(['access_level > ?', Member::REPORTER]).select(:user_id).to_sql})"
+      "id IN (#{project.members.where(has_access).select(:user_id).to_sql})"
     ]
 
     if project.group
-      wheres << "id IN (#{project.group.members.where(['access_level > ?', Member::REPORTER]).select(:user_id).to_sql})"
+      wheres << "id IN (#{project.group.members.where(has_access).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}")
-- 
GitLab


From 871381cddd945b11fb93b373d8b98a84cea450bc Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 15 Jul 2016 10:09:06 +0100
Subject: [PATCH 7/8] fixup! Prevent author from approving their own MR

---
 app/models/merge_request.rb             | 2 +-
 features/project/merge_requests.feature | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 1e0038dd957cc9..f5f6855acb1406 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -658,7 +658,7 @@ def state_human_name
 
   def approver_ids=(value)
     value.split(",").map(&:strip).each do |user_id|
-      next if user_id == author.id
+      next if author && user_id == author.id
 
       approvers.find_or_initialize_by(user_id: user_id, target_id: id)
     end
diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature
index e1d5f9591db204..daff12416d487b 100644
--- a/features/project/merge_requests.feature
+++ b/features/project/merge_requests.feature
@@ -327,7 +327,7 @@ Feature: Project Merge Requests
     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
+    Then I should see approved merge request "Bug NS-04"
 
   Scenario: I can not approve merge request if I am not an approver
     Given merge request 'Bug NS-04' must be approved by some user
-- 
GitLab


From 0ab86d46e57ba7819fb9fa7b5243abe3ce1224c9 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Sun, 17 Jul 2016 11:22:19 +0100
Subject: [PATCH 8/8] Explain approvals logic better in MR model

---
 app/models/merge_request.rb       | 81 +++++++++++++++++++------------
 spec/models/merge_request_spec.rb | 12 ++---
 2 files changed, 55 insertions(+), 38 deletions(-)

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index f5f6855acb1406..cb32d5a30b699f 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -560,11 +560,36 @@ def locked_long_ago?
     locked_at.nil? || locked_at < (Time.now - 1.day)
   end
 
+  def requires_approve?
+    approvals_required.nonzero?
+  end
+
+  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, potential_approvers].min
+    [approvals_required - approvals.count, number_of_potential_approvers].min
   end
 
-  def potential_approvers
+  def approvals_required
+    approvals_before_merge || target_project.approvals_before_merge
+  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})",
@@ -575,21 +600,18 @@ def potential_approvers
       wheres << "id IN (#{project.group.members.where(has_access).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}")
+    User.where("(#{wheres.join(' OR ')}) AND id NOT IN (#{approvals.select(:user_id).to_sql}) AND id != #{author.id}").count
   end
 
+  # 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 approvals_required
-    approvals_before_merge || target_project.approvals_before_merge
-  end
-
-  def requires_approve?
-    approvals_required.nonzero?
-  end
-
+  # 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.
   #
@@ -600,30 +622,33 @@ def overall_approvers(exclude_user: nil)
     exclude_user ? approvers_relation.where.not(user_id: exclude_user.id) : approvers_relation
   end
 
-  def approved?
-    approvals_left < 1
-  end
-
-  def approved_by?(user)
-    approved_by_users.include?(user)
-  end
-
-  def approved_by_users
-    approvals.map(&:user)
-  end
-
   def can_approve?(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? && !approved_by?(user)
+    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
@@ -656,14 +681,6 @@ def state_human_name
     end
   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 state_icon_name
     if merged?
       "check"
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 93bc24d85e42f2..ffb9ef0f573e4b 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -264,7 +264,7 @@
     end
   end
 
-  describe "#potential_approvers" do
+  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) }
@@ -272,7 +272,7 @@
     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.to change { merge_request.number_of_potential_approvers }.by(1)
     end
 
     it "includes project members with developer access and up" do
@@ -281,7 +281,7 @@
         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.to change { merge_request.number_of_potential_approvers }.by(2)
     end
 
     it "excludes users who have already approved the MR" do
@@ -289,14 +289,14 @@
         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.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.potential_approvers }.by(1)
+      end.to change { merge_request.number_of_potential_approvers }.by(1)
     end
 
     context "when the project is part of a group" do
@@ -309,7 +309,7 @@
           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.to change { merge_request.number_of_potential_approvers }.by(2)
       end
     end
   end
-- 
GitLab