Fix suggested approvers

Previously, this would pick the least-frequent contributors to the
changed files, rather than the most frequent.

app/controllers/projects/merge_requests_controller.rb

@@ -465,8 +465,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

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/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