auto suggestion

CHANGELOG-EE

 v 7.14
   - Don't send "Added to group" notifications when group is LDAP synched
+  - Automatic approver suggestions (based on an authority of the code)  
 
 v 7.13.2
   - Fix group web hook

app/controllers/projects/merge_requests_controller.rb

@@ -94,12 +94,16 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @diffs = @merge_request.compare_diffs
     @note_counts = Note.where(commit_id: @commits.map(&:id)).
       group(:commit_id).count
+
+    set_suggested_approvers
   end
 
   def edit
     @source_project = @merge_request.source_project
     @target_project = @merge_request.target_project
     @target_branches = @merge_request.target_project.repository.branch_names
+
+    set_suggested_approvers
   end
 
   def create
@@ -288,6 +292,20 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render 'invalid'
   end
 
+  def set_suggested_approvers
+    if @target_project.approvals_before_merge.nonzero?
+      @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
+        },
+        current_user
+      ).calculate(@target_project.approvals_before_merge)
+    end
+  end
+
   def merge_request_params
     params.require(:merge_request).permit(
       :title, :assignee_id, :source_project_id, :source_branch,

app/models/approver.rb

@@ -13,4 +13,6 @@
 class Approver < ActiveRecord::Base
   belongs_to :target, polymorphic: true
   belongs_to :user
+
+  validates :user_id, presence: true
 end

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

@@ -90,14 +90,11 @@
         .help-block
           Merge Request should be approved by these users. 
           You can override the project settings by setting your own list of approvers.
-        
+
         .panel.panel-default.prepend-top-10
           .panel-heading
             Approvers
-            %small#approvers-counter
-              - approvers = @merge_request.new_record? ? @merge_request.target_project.approvers : @merge_request.approvers
-              (#{approvers.count(:all)})
-          %ul.well-list
+          %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)}
@@ -107,7 +104,7 @@
                       = icon("sign-out")
                       Remove
               - if @merge_request.target_project.approvers.empty?
-                %li There are no approvers
+                %li.no-approvers There are no approvers
             - else
               - @merge_request.approvers.each do |approver|
                 %li
@@ -117,7 +114,12 @@
                       = icon("sign-out")
                       Remove
               - if @merge_request.approvers.empty?
-                %li There are no approvers
+                %li.no-approvers There are no approvers
+        .help-block.suggested-approvers
+          - if @suggested_approvers.any?
+            Suggested approvers: 
+            = raw @suggested_approvers.map{|approver| link_to sanitize(approver.name), "#", id: dom_id(approver) }.join(", ")
+
   %hr
     - if @merge_request.new_record?
       .form-group
@@ -152,17 +154,35 @@
     - cancel_project = issuable.project
   = link_to 'Cancel', [cancel_project.namespace.becomes(Namespace), cancel_project, issuable], class: 'btn btn-cancel'
 
+%li.project-approvers.hide.approver-template{id: "user_{user_id}"}
+  = link_to "{approver_name}", "#"
+  .pull-right
+    = 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
+
 :coffeescript
-  $(".project-approvers .btn-remove").click ->
-    approvers_list = $(this).closest("ul")
+  $(".approver-list").on "click", ".project-approvers .btn-remove", ->
     $(this).closest("li").remove()
-    approvers_count = approvers_list.find("li").size()
-    $("#approvers-counter").text("(" + approvers_count + ")")
     return false
-  $("form#new_merge_request").submit ->
-    approver_ids = $.map $("li.project-approvers"), (li, i) ->
+  $("form.merge-request-form").submit ->
+    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(approver_ids.join(","))
+    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
+
 

features/project/merge_requests.feature

@@ -260,6 +260,15 @@ Feature: Project Merge Requests
     When I click link "New Merge Request"
     And I select "fix" 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
+    And there is one auto-suggested approver
+    When I click link "New Merge Request"
+    And I select "fix" as source
+    Then I see auto-suggested approver
+    And I can add it to approver list
     
 
   Scenario: I should see rebase checkbox

features/steps/project/merge_requests.rb

@@ -343,12 +343,38 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     project.approvers.create(user_id: current_user.id)
   end
 
+  step 'there is one auto-suggested approver' do
+    @user = create :user
+    allow_any_instance_of(Gitlab::AuthorityAnalyzer).to receive(:calculate).and_return([@user])
+  end
+
   step 'I see suggested approver' do
-    page.within '.project-approvers' do
+    page.within 'ul .project-approvers' do
       expect(page).to have_content(current_user.name)
     end
   end
 
+  step 'I see auto-suggested approver' do
+    page.within '.suggested-approvers' do
+      expect(page).to have_content(@user.name)
+    end
+  end
+
+  step 'I can add it to approver list' do
+    click_link @user.name
+
+    page.within 'ul.approver-list' do
+      expect(page).to have_content(@user.name)
+    end
+
+    click_button "Submit new merge request"
+    click_link "Edit"
+
+    page.within 'ul.approver-list' do
+      expect(page).to have_content(@user.name)
+    end    
+  end
+
   step 'merge request \'Bug NS-04\' must be approved' do
     merge_request = MergeRequest.find_by!(title: "Bug NS-04")
     project = merge_request.target_project

lib/gitlab/authority_analyzer.rb

+module Gitlab
+  class AuthorityAnalyzer
+    COMMITS_TO_CONSIDERATION = 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]
+      @current_user = current_user
+      @users = {}
+    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)
+    end
+
+    private
+
+    def involved_users
+      @repo = @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?
+        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
+        end
+      end
+
+      []
+    end
+  end
+end