Merge branch 'approvals_improvements' into 'master'

Approvals improvements

https://dev.gitlab.org/gitlab/gitlab-ee/issues/326


* Fix approvals for forks. Now you can change approvals settings on the new merge request form
* Suggested approvers are shown on the new merge request form


See merge request !464

CHANGELOG

@@ -12,6 +12,8 @@ v 7.14.0 (unreleased)
   - Add support for destroying project milestones (Stan Hu)
   - Add fetch command to the MR page.
   - Fix bug causing "Remove source-branch" option not to work for merge requests from the same project.
+  - Fix approvals for forks. Now you can change approvals settings on the new merge request form
+  - Suggested approvers are shown on the new merge request form
 
 v 7.13.1
   - Revert issue caching

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

@@ -81,7 +81,7 @@
         = link_to 'Create new label', new_namespace_project_label_path(issuable.project.namespace, issuable.project), target: :blank
 
 - if issuable.is_a?(MergeRequest)
-  - if @project.approvals_before_merge.nonzero?
+  - if @merge_request.requires_approve?
     .form-group
       = f.label :approver_ids, class: 'control-label' do
         Approvers
@@ -89,25 +89,35 @@
         = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
         .help-block
           Merge Request should be approved by these users. 
-          - if @project.approvers.any?
-            Suggested approver(s): #{render_items_list(@project.approvers.map{ |approver| approver.user.name})}.
-            You can override the project settings by setting your own list of approvers.
+          You can override the project settings by setting your own list of approvers.
         
         .panel.panel-default.prepend-top-10
           .panel-heading
             Approvers
-            %small
-              (#{@merge_request.approvers.count(:all)})
+            %small#approvers-counter
+              - approvers = @merge_request.new_record? ? @merge_request.target_project.approvers : @merge_request.approvers
+              (#{approvers.count(:all)})
           %ul.well-list
-            - @merge_request.approvers.each do |approver|
-              %li
-                = 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
+            - if @merge_request.new_record?
+              - @merge_request.target_project.approvers.each do |approver|
+                %li.project-approvers{id: dom_id(approver.user)}
+                  = link_to approver.user.name, approver.user
+                  .pull-right
+                    = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
+                      = icon("sign-out")
+                      Remove
+              - if @merge_request.target_project.approvers.empty?
+                %li There are no approvers
+            - else
+              - @merge_request.approvers.each do |approver|
+                %li
+                  = 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
   %hr
     - if @merge_request.new_record?
       .form-group
@@ -141,3 +151,18 @@
   - else
     - 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(","))
+

features/project/merge_requests.feature

@@ -255,6 +255,13 @@ Feature: Project Merge Requests
     When I should not see Approve button
     And I should see message that MR require an approval
 
+  Scenario: I see suggested approvers on new merge request form
+    Given project settings contain list of approvers
+    When I click link "New Merge Request"
+    And I select "fix" as source
+    Then I see suggested approver
+    
+
   Scenario: I should see rebase checkbox
     Given project "Shop" have "Bug NS-05" open merge request with diffs inside
     And rebase before merge enabled

features/steps/project/merge_requests.rb

@@ -338,6 +338,17 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     expect(page).to have_content 'Target branch changed from master to feature'
   end
 
+  step 'project settings contain list of approvers' do
+    project.update(approvals_before_merge: 1)
+    project.approvers.create(user_id: current_user.id)
+  end
+
+  step 'I see suggested approver' do
+    page.within '.project-approvers' do
+      expect(page).to have_content(current_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
@@ -350,7 +361,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     merge_request = MergeRequest.find_by!(title: "Bug NS-04")
     project = merge_request.target_project
     project.approvals_before_merge = 1
-    project.approvers.create(user_id: current_user.id)
+    merge_request.approvers.create(user_id: current_user.id)
     project.ensure_satellite_exists
     project.save!
   end
@@ -359,7 +370,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     merge_request = MergeRequest.find_by!(title: "Bug NS-04")
     project = merge_request.target_project
     project.approvals_before_merge = 1
-    project.approvers.create(user_id: create(:user).id)
+    merge_request.approvers.create(user_id: create(:user).id)
     project.ensure_satellite_exists
     project.save!
   end

spec/models/merge_request_spec.rb

@@ -181,8 +181,8 @@ describe MergeRequest do
     it "returns correct value" do
       user = create(:user)
       user1 = create(:user)
-      merge_request.target_project.approvers.create(user_id: user.id)
-      merge_request.target_project.approvers.create(user_id: user1.id)
+      merge_request.approvers.create(user_id: user.id)
+      merge_request.approvers.create(user_id: user1.id)
       merge_request.approvals.create(user_id: user1.id)
 
       expect(merge_request.approvers_left).to eq [user]