Merge branch 'rs-revert-09ab819e' into 'master'

Revert "Merge branch '388-option-to-disallow-author-to-approve-merge-request' into 'master'"

This reverts commit 09ab819e, reversing
changes made to 041f3c27.

This reverts !455 as per https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/479#note_12609455

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/18759

See merge request !493

CHANGELOG-EE

@@ -5,7 +5,6 @@ v 8.9.0 (unreleased)
   - Fix nil user handling in UpdateMirrorService
   - Allow overriding the number of approvers for a merge request
   - Allow LDAP to mark users as external based on their group membership. !432
-  - Forbid MR authors from approving their own MRs
   - Instrument instance methods of Gitlab::InsecureKeyFingerprint class
   - Add API endpoint for Merge Request Approvals !449
   - Send notification email when merge request is approved

app/controllers/projects/merge_requests_controller.rb

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

app/models/merge_request.rb

@@ -492,7 +492,8 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def approvers_left
-    User.where(id: overall_approvers.select(:user_id)).where.not(id: approvals.select(:user_id))
+    user_ids = overall_approvers.map(&:user_id) - approvals.map(&:user_id)
+    User.where id: user_ids
   end
 
   def approvals_required
@@ -524,10 +525,8 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def can_approve?(user)
-    return false if user == self.author
-
     approvers_left.include?(user) ||
-      (any_approver_allowed? && !approved_by?(user))
+    (any_approver_allowed? && !approved_by?(user))
   end
 
   def any_approver_allowed?

doc/workflow/merge_request_approvals.md

@@ -33,7 +33,7 @@ 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. The author of a merge request cannot approve that merge request.
+merge request.
 
 If there are more approvers than required approvals, any subset of these users
 can approve the merge request.
@@ -64,4 +64,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)
+![Merge request approval](merge_request_approvals/2_approvals.png)
\ No newline at end of file

features/project/merge_requests.feature

@@ -327,6 +327,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
     And I visit project "Shop" merge requests page
@@ -336,12 +343,13 @@ Feature: Project Merge Requests
     When I click link "Approve"
     Then I should see message that merge request can be merged
 
-  Scenario: I can not approve Merge request if I am the author
-    Given merge request 'Bug NS-04' must be approved
+  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
-    When I should not see Approve button
-    And I should see message that MR require an approval
+    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

lib/gitlab/authority_analyzer.rb

@@ -2,8 +2,9 @@ module Gitlab
   class AuthorityAnalyzer
     COMMITS_TO_CONSIDER = 5
 
-    def initialize(merge_request)
+    def initialize(merge_request, current_user)
       @merge_request = merge_request
+      @current_user = current_user
       @users = Hash.new(0)
     end
 
@@ -11,7 +12,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
@@ -21,9 +22,7 @@ module Gitlab
 
       list_of_involved_files.each do |path|
         @repo.commits(@merge_request.target_branch, path: path, limit: COMMITS_TO_CONSIDER).each do |commit|
-          if commit.author && commit.author != @merge_request.author
-            @users[commit.author] += 1
-          end
+          @users[commit.author] += 1 if commit.author
         end
       end
     end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -335,49 +335,6 @@ describe Projects::MergeRequestsController do
     end
   end
 
-  describe 'POST #approve' do
-    def approve(user)
-      post :approve, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid, user: user
-    end
-
-    context 'when the user is the author of the MR' do
-      before { merge_request.approvers.create(user: merge_request.author) }
-
-      it "returns a 404" do
-        approve(merge_request.author)
-
-        expect(response).to have_http_status(404)
-      end
-    end
-
-    context 'when the user is not allowed to approve the MR' do
-      it "returns a 404" do
-        approve(user)
-
-        expect(response).to have_http_status(404)
-      end
-    end
-
-    context 'when the user is allowed to approve the MR' do
-      before { merge_request.approvers.create(user: user) }
-
-      it 'creates an approval' do
-        service = double(:approval_service)
-
-        expect(MergeRequests::ApprovalService).to receive(:new).with(project, anything).and_return(service)
-        expect(service).to receive(:execute).with(merge_request)
-
-        approve(user)
-      end
-
-      it 'redirects to the MR' do
-        approve(user)
-
-        expect(response).to redirect_to(namespace_project_merge_request_path)
-      end
-    end
-  end
-
   describe "DELETE #destroy" do
     it "denies access to users unless they're admin or project owner" do
       delete :destroy, namespace_id: project.namespace.path, project_id: project.path, id: merge_request.iid

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
-
-    def calculate_approvers(count)
-      described_class.new(merge_request).calculate(count)
-    end
-
-    before do
-      merge_request.compare = double(:compare, diffs: files)
-      allow(merge_request.target_project.repository).to receive(:commits).and_return(commits)
-    end
-
-    context 'when the MR author is in the top contributors' do
-      it 'does not include the MR author' do
-        approvers = calculate_approvers(2)
-
-        expect(approvers).not_to include(author)
-      end
-
-      it 'returns the correct number of contributors' do
-        approvers = calculate_approvers(2)
-
-        expect(approvers.length).to eq(2)
-      end
-    end
-
-    context 'when there are fewer contributors than requested' do
-      it 'returns the full number of users' do
-        approvers = calculate_approvers(5)
-
-        expect(approvers.length).to eq(2)
-      end
-    end
-
-    context 'when there are more contributors than requested' do
-      it 'returns only the top n contributors' do
-        approvers = calculate_approvers(1)
-
-        expect(approvers).to contain_exactly(user_a)
-      end
-    end
-  end
-end

spec/models/merge_request_spec.rb

@@ -223,7 +223,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
@@ -256,58 +256,6 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe "#can_approve?" do
-    let(:author) { create(:user) }
-    let(:user) { create(:user) }
-    let(:merge_request) { create(:merge_request, author: author) }
-
-    context "when the user is the MR author" do
-      it "returns false" do
-        expect(merge_request.can_approve?(author)).to eq(false)
-      end
-    end
-
-    context "when the user is not the MR author" do
-      context "when the user is in the approvers list" do
-        before { merge_request.approvers.create(user: user) }
-
-        context "when the user has not already approved the MR" do
-          it "returns true" do
-            expect(merge_request.can_approve?(user)).to eq(true)
-          end
-        end
-
-        context "when the user has already approved the MR" do
-          before { merge_request.approvals.create(user: user) }
-
-          it "returns false" do
-            expect(merge_request.can_approve?(user)).to eq(false)
-          end
-        end
-      end
-
-      context "when the user is not in the approvers list" do
-        context "when anyone is allowed to approve the MR" do
-          before { merge_request.target_project.update_attributes(approvals_before_merge: 1) }
-
-          context "when the user has not already approved the MR" do
-            it "returns true" do
-              expect(merge_request.can_approve?(user)).to eq(true)
-            end
-          end
-
-          context "when the user has already approved the MR" do
-            before { merge_request.approvals.create(user: user) }
-
-            it "returns false" do
-              expect(merge_request.can_approve?(user)).to eq(false)
-            end
-          end
-        end
-      end
-    end
-  end
-
   describe '#can_remove_source_branch?' do
     let(:user) { create(:user) }
     let(:user2) { create(:user) }

spec/requests/api/merge_requests_spec.rb

@@ -695,24 +695,14 @@ describe API::API, api: true  do
   end
 
   describe 'POST :id/merge_requests/:merge_request_id/approve' do
-    context 'when the user is an allowed approver' do
-      it 'approves the merge request' do
-        project.update_attribute(:approvals_before_merge, 2)
-
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", admin)
-
-        expect(response.status).to eq(201)
-        expect(json_response['approvals_left']).to eq(1)
-        expect(json_response['approved_by'][0]['user']['username']).to eq(admin.username)
-      end
-    end
+    it 'approves the merge request' do
+      project.update_attribute(:approvals_before_merge, 2)
 
-    context 'when the user is the MR author' do
-      it 'returns a not authorised response' do
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user)
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/approve", user)
 
-        expect(response.status).to eq(401)
-      end
+      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)
     end
   end