Merge branch 'auto_suggestion' into 'master'

Automatic approver suggestions



See merge request !466

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,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     render 'invalid'
   end
 
+  def set_suggested_approvers
+    if @merge_request.requires_approve?
+      @suggested_approvers = Gitlab::AuthorityAnalyzer.new(
+        @merge_request,
+        current_user
+      ).calculate(@merge_request.approvals_required)
+    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, 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,17 +104,22 @@
                       = 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
+                %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
                       = 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,36 @@
     - cancel_project = issuable.project
   = link_to 'Cancel', [cancel_project.namespace.becomes(Namespace), cancel_project, issuable], class: 'btn btn-cancel'
 
-:coffeescript
-  $(".project-approvers .btn-remove").click ->
-    approvers_list = $(this).closest("ul")
-    $(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) ->
-      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(","))
+%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
+- 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
+
+      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

@@ -325,6 +325,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   end
 
   step 'I click the "Target branch" dropdown' do
+    sleep 0.5
     first('.target_branch').click
   end
 
@@ -343,12 +344,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_CONSIDER = 5
+
+    def initialize(merge_request, current_user)
+      @merge_request = merge_request
+      @current_user = current_user
+      @users = Hash.new(0)
+    end
+
+    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)
+    end
+
+    private
+
+    def involved_users
+      @repo = @merge_request.target_project.repository
+
+      list_of_involved_files.each do |path|
+        @repo.commits(@merge_request.target_branch, path, COMMITS_TO_CONSIDER).each do |commit|
+          @users[commit.author] += 1 if commit.author
+        end
+      end
+    end
+
+    def list_of_involved_files
+      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