Refactoring

app/controllers/projects/merge_requests_controller.rb

@@ -293,16 +293,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def set_suggested_approvers
-    if @target_project.approvals_before_merge.nonzero?
+    if @merge_request.requires_approve?
       @suggested_approvers = Gitlab::AuthorityAnalyzer.new(
-        {
-          source_branch: @merge_request.source_branch,
-          source_project: @merge_request.source_project,
-          target_branch: @merge_request.target_branch,
-          target_project: @merge_request.target_project
-        },
+        @merge_request,
         current_user
-      ).calculate(@target_project.approvals_before_merge)
+      ).calculate(@merge_request.approvals_required)
     end
   end
 

app/models/approver.rb

@@ -14,5 +14,5 @@ class Approver < ActiveRecord::Base
   belongs_to :target, polymorphic: true
   belongs_to :user
 
-  validates :user_id, presence: true
+  validates :user, presence: true
 end

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

@@ -107,7 +107,7 @@
                 %li.no-approvers There are no approvers
             - else
               - @merge_request.approvers.each do |approver|
-                %li
+                %li{id: dom_id(approver.user)}
                   = link_to approver.user.name, approver.user
                   .pull-right
                     = 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
@@ -160,30 +160,30 @@
     = link_to "#", data: { confirm: "Are you sure you want to remove approver {approver_name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
       = icon("sign-out")
       Remove
+- if issuable.is_a?(MergeRequest)
+  :coffeescript
+    $(".approver-list").on "click", ".project-approvers .btn-remove", ->
+      $(this).closest("li").remove()
+      return false
+    $("form.merge-request-form").submit ->
+      if $("input#merge_request_approver_ids").length
+        approver_ids = $.map $("li.project-approvers").not(".approver-template"), (li, i) ->
+          li.id.replace("user_", "")
+        approvers_input = $(this).find("input#merge_request_approver_ids")
+        approver_ids = approver_ids.concat(approvers_input.val().split(","))
+        approvers_input.val(_.compact(approver_ids).join(","))
+    $(".suggested-approvers a").click ->
+      user_id = this.id.replace("user_", "")
+      user_name = this.text
+      return false if $(".approver-list #user_" + user_id).length
 
-:coffeescript
-  $(".approver-list").on "click", ".project-approvers .btn-remove", ->
-    $(this).closest("li").remove()
-    return false
-  $("form.merge-request-form").submit ->
-    if $("input#merge_request_approver_ids").length
-      approver_ids = $.map $("li.project-approvers").not(".approver-template"), (li, i) ->
-        li.id.replace("user_", "")
-      approvers_input = $(this).find("input#merge_request_approver_ids")
-      approver_ids = approver_ids.concat(approvers_input.val().split(","))
-      approvers_input.val(_.compact(approver_ids).join(","))
-  $(".suggested-approvers a").click ->
-    user_id = this.id.replace("user_", "")
-    user_name = this.text
-    return false if $(".approver-list #user_" + user_id).length
-
-    approver_item_html = $(".project-approvers.approver-template").clone().
-      removeClass("hide approver-template")[0].
-      outerHTML.
-      replace(/\{approver_name\}/g, user_name).
-      replace(/\{user_id\}/g, user_id)
-    $(".no-approvers").remove()
-    $(".approver-list").append(approver_item_html)
-    return false
+      approver_item_html = $(".project-approvers.approver-template").clone().
+        removeClass("hide approver-template")[0].
+        outerHTML.
+        replace(/\{approver_name\}/g, user_name).
+        replace(/\{user_id\}/g, user_id)
+      $(".no-approvers").remove()
+      $(".approver-list").append(approver_item_html)
+      return false
 
 

lib/gitlab/authority_analyzer.rb

 module Gitlab
   class AuthorityAnalyzer
-    COMMITS_TO_CONSIDERATION = 5
+    COMMITS_TO_CONSIDER = 5
 
-    def initialize(data, current_user)
-      @source_branch = data[:source_branch]
-      @source_project = data[:source_project]
-      @target_branch = data[:target_branch]
-      @target_project = data[:target_project]
+    def initialize(merge_request, current_user)
+      @merge_request = merge_request
       @current_user = current_user
-      @users = {}
+      @users = Hash.new(0)
     end
 
     def calculate(number_of_approvers)
       involved_users
 
       # Picks most active users from hash like: {user1: 2, user2: 6}
-      @users.to_a.sort{|a, b| b.last <=> a.last }[0..number_of_approvers].map(&:first)
+      @users.sort_by { |user, count| count }.map(&:first).take(number_of_approvers)
     end
 
     private
 
     def involved_users
-      @repo = @target_project.repository
+      @repo = @merge_request.target_project.repository
 
       list_of_involved_files.each do |path|
-        @repo.commits(@target_branch, path, COMMITS_TO_CONSIDERATION).each do |commit|
-          add_user_to_list(commit.author) unless commit.author.nil?
+        @repo.commits(@merge_request.target_branch, path, COMMITS_TO_CONSIDER).each do |commit|
+          @users[commit.author] += 1 if commit.author
         end
       end
     end
 
-    def add_user_to_list(user)
-      @users[user] = @users[user].nil? ? 1 : (@users[user] + 1)
-    end
-
     def list_of_involved_files
-      compare_result = CompareService.new.execute(
-        @current_user,
-        @source_project,
-        @source_branch,
-        @target_project,
-        @target_branch,
-      )
-
-      commits = compare_result.commits
-
-      if commits.present? && compare_result.diffs.present?
-        return compare_result.diffs.map do |diff|
-          case true
-          when diff.deleted_file, diff.renamed_file
-            diff.old_path
-          else
-            diff.new_path
-          end
+      compare_diffs = @merge_request.compare_diffs || @merge_request.diffs
+
+      return [] unless compare_diffs.present?
+      
+      compare_diffs.map do |diff|
+        if diff.deleted_file || diff.renamed_file
+          diff.old_path
+        else
+          diff.new_path
         end
       end
-
-      []
     end
   end
 end