Merge branch 'dz-bulk-edit-mr' into 'master'

Add bulk update support for merge requests list

## What does this MR do?

Adds new feature: ability to update multiple merge requests at once from index page (same like we have for issues already)

## Are there points in the code the reviewer needs to double check?

I don't think so

## Why was this MR needed?

* consistency with issues
* save user time when you want set same label/milestone to 20 merge requests 

## Screenshots (if relevant)

![Screenshot_2016-09-08_11.48.49](/uploads/fe50b2472330e211eb8d6d6dafb15667/Screenshot_2016-09-08_11.48.49.png)![Screenshot_2016-09-08_11.48.46](/uploads/4833c3705120ec028f7a62a4e128998d/Screenshot_2016-09-08_11.48.46.png)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] ~~[Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- [x] ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/20087

See merge request !6226

CHANGELOG

@@ -104,6 +104,7 @@ v 8.12.0 (unreleased)
   - Show values of CI trigger variables only when clicked (Katarzyna Kobierska Ula Budziszewska)
   - Use default clone protocol on "check out, review, and merge locally" help page URL
   - API for Ci Lint !5953 (Katarzyna Kobierska Urszula Budziszewska)
+  - Allow bulk update merge requests from merge requests index page
 
 v 8.11.6 (unreleased)
 

app/assets/javascripts/dispatcher.js

@@ -23,6 +23,7 @@
         case 'projects:boards:show':
           shortcut_handler = new ShortcutsNavigation();
           break;
+        case 'projects:merge_requests:index':
         case 'projects:issues:index':
           Issuable.init();
           new IssuableBulkActions();
@@ -93,10 +94,6 @@
           break;
         case "projects:merge_requests:conflicts":
           window.mcui = new MergeConflictResolver()
-        case 'projects:merge_requests:index':
-          shortcut_handler = new ShortcutsNavigation();
-          Issuable.init();
-          break;
         case 'dashboard:activity':
           new Activities();
           break;

app/assets/javascripts/issuable.js.es6

@@ -77,7 +77,7 @@
     },
     checkChanged: function() {
       const $checkedIssues = $('.selected_issue:checked');
-      const $updateIssuesIds = $('#update_issues_ids');
+      const $updateIssuesIds = $('#update_issuable_ids');
       const $issuesOtherFilters = $('.issues-other-filters');
       const $issuesBulkUpdate = $('.issues_bulk_update');
 

app/assets/javascripts/issues-bulk-assignment.js

@@ -5,7 +5,7 @@
       if (opts == null) {
         opts = {};
       }
-      this.container = (ref = opts.container) != null ? ref : $('.content'), this.form = (ref1 = opts.form) != null ? ref1 : this.getElement('.bulk-update'), this.issues = (ref2 = opts.issues) != null ? ref2 : this.getElement('.issues-list .issue');
+      this.container = (ref = opts.container) != null ? ref : $('.content'), this.form = (ref1 = opts.form) != null ? ref1 : this.getElement('.bulk-update'), this.issues = (ref2 = opts.issues) != null ? ref2 : this.getElement('.issuable-list > li');
       this.form.data('bulkActions', this);
       this.willUpdateLabels = false;
       this.bindEvents();
@@ -106,7 +106,7 @@
           state_event: this.form.find('input[name="update[state_event]"]').val(),
           assignee_id: this.form.find('input[name="update[assignee_id]"]').val(),
           milestone_id: this.form.find('input[name="update[milestone_id]"]').val(),
-          issues_ids: this.form.find('input[name="update[issues_ids]"]').val(),
+          issuable_ids: this.form.find('input[name="update[issuable_ids]"]').val(),
           subscription_event: this.form.find('input[name="update[subscription_event]"]').val(),
           add_label_ids: [],
           remove_label_ids: []

app/assets/stylesheets/pages/issuable.scss

@@ -404,3 +404,18 @@
     margin-bottom: $gl-padding;
   }
 }
+
+.issuable-list {
+  li {
+    .issue-check {
+      float: left;
+      padding-right: $gl-padding;
+      margin-bottom: 10px;
+      min-width: 15px;
+
+      .selected_issue {
+        vertical-align: text-top;
+      }
+    }
+  }
+}

app/assets/stylesheets/pages/issues.scss

@@ -7,17 +7,6 @@
       margin-bottom: 2px;
     }
 
-    .issue-check {
-      float: left;
-      padding-right: 16px;
-      margin-bottom: 10px;
-      min-width: 15px;
-
-      .selected_issue {
-        vertical-align: text-top;
-      }
-    }
-
     .issue-labels {
       display: inline-block;
     }

app/controllers/concerns/issuable_actions.rb

@@ -3,6 +3,7 @@ module IssuableActions
 
   included do
     before_action :authorize_destroy_issuable!, only: :destroy
+    before_action :authorize_admin_issuable!, only: :bulk_update
   end
 
   def destroy
@@ -13,11 +14,41 @@ module IssuableActions
     redirect_to polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable.class])
   end
 
+  def bulk_update
+    result = Issuable::BulkUpdateService.new(project, current_user, bulk_update_params).execute(resource_name)
+    quantity = result[:count]
+
+    render json: { notice: "#{quantity} #{resource_name.pluralize(quantity)} updated" }
+  end
+
   private
 
   def authorize_destroy_issuable!
-    unless current_user.can?(:"destroy_#{issuable.to_ability_name}", issuable)
+    unless can?(current_user, :"destroy_#{issuable.to_ability_name}", issuable)
       return access_denied!
     end
   end
+
+  def authorize_admin_issuable!
+    unless can?(current_user, :"admin_#{resource_name}", @project)
+      return access_denied!
+    end
+  end
+
+  def bulk_update_params
+    params.require(:update).permit(
+      :issuable_ids,
+      :assignee_id,
+      :milestone_id,
+      :state_event,
+      :subscription_event,
+      label_ids: [],
+      add_label_ids: [],
+      remove_label_ids: []
+    )
+  end
+
+  def resource_name
+    @resource_name ||= controller_name.singularize
+  end
 end

app/controllers/projects/issues_controller.rb

@@ -20,9 +20,6 @@ class Projects::IssuesController < Projects::ApplicationController
   # Allow modify issue
   before_action :authorize_update_issue!, only: [:edit, :update]
 
-  # Allow issues bulk update
-  before_action :authorize_admin_issues!, only: [:bulk_update]
-
   respond_to :html
 
   def index
@@ -168,16 +165,6 @@ class Projects::IssuesController < Projects::ApplicationController
     end
   end
 
-  def bulk_update
-    result = Issues::BulkUpdateService.new(project, current_user, bulk_update_params).execute
-
-    respond_to do |format|
-      format.json do
-        render json: { notice: "#{result[:count]} issues updated" }
-      end
-    end
-  end
-
   protected
 
   def issue
@@ -237,17 +224,4 @@ class Projects::IssuesController < Projects::ApplicationController
       :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: []
     )
   end
-
-  def bulk_update_params
-    params.require(:update).permit(
-      :issues_ids,
-      :assignee_id,
-      :milestone_id,
-      :state_event,
-      :subscription_event,
-      label_ids: [],
-      add_label_ids: [],
-      remove_label_ids: []
-    )
-  end
 end

app/services/issuable/bulk_update_service.rb

+module Issuable
+  class BulkUpdateService < IssuableBaseService
+    def execute(type)
+      model_class = type.classify.constantize
+      update_class = type.classify.pluralize.constantize::UpdateService
+
+      ids = params.delete(:issuable_ids).split(",")
+      items = model_class.where(id: ids)
+
+      %i(state_event milestone_id assignee_id add_label_ids remove_label_ids subscription_event).each do |key|
+        params.delete(key) unless params[key].present?
+      end
+
+      items.each do |issuable|
+        next unless can?(current_user, :"update_#{type}", issuable)
+
+        update_class.new(issuable.project, current_user, params).execute(issuable)
+      end
+
+      {
+        count:    items.count,
+        success:  !items.count.zero?
+      }
+    end
+  end
+end

app/services/issues/bulk_update_service.rb

-module Issues
-  class BulkUpdateService < BaseService
-    def execute
-      issues_ids   = params.delete(:issues_ids).split(",")
-      issue_params = params
-
-      %i(state_event milestone_id assignee_id add_label_ids remove_label_ids subscription_event).each do |key|
-        issue_params.delete(key) unless issue_params[key].present?
-      end
-
-      issues = Issue.where(id: issues_ids)
-
-      issues.each do |issue|
-        next unless can?(current_user, :update_issue, issue)
-
-        Issues::UpdateService.new(issue.project, current_user, issue_params).execute(issue)
-      end
-
-      {
-        count:    issues.count,
-        success:  !issues.count.zero?
-      }
-    end
-  end
-end

app/views/projects/issues/_issue.html.haml

 %li{ id: dom_id(issue), class: issue_css_classes(issue), url: issue_path(issue), data: { labels: issue.label_ids, id: issue.id } }
-  - if controller.controller_name == 'issues' && can?(current_user, :admin_issue, @project)
+  - if @bulk_edit
     .issue-check
-      = check_box_tag dom_id(issue,"selected"), nil, false, 'data-id' => issue.id, class: "selected_issue"
+      = check_box_tag dom_id(issue, "selected"), nil, false, 'data-id' => issue.id, class: "selected_issue"
 
   .issue-title.title
     %span.issue-title-text

app/views/projects/issues/_issues.html.haml

-%ul.content-list.issues-list
+%ul.content-list.issues-list.issuable-list
   = render @issues
   - if @issues.blank?
     %li

app/views/projects/issues/index.html.haml

 - @no_container = true
+- @bulk_edit = can?(current_user, :admin_issue, @project)
+
 - page_title "Issues"
 - new_issue_email = @project.new_issue_address(current_user)
 = render "projects/issues/head"

app/views/projects/merge_requests/_merge_request.html.haml

 %li{ class: mr_css_classes(merge_request) }
+  - if @bulk_edit
+    .issue-check
+      = check_box_tag dom_id(merge_request, "selected"), nil, false, 'data-id' => merge_request.id, class: "selected_issue"
+
   .merge-request-title.title
     %span.merge-request-title-text
       = link_to merge_request.title, merge_request_path(merge_request)

app/views/projects/merge_requests/_merge_requests.html.haml

-%ul.content-list.mr-list
+%ul.content-list.mr-list.issuable-list
   = render @merge_requests
   - if @merge_requests.blank?
     %li

app/views/projects/merge_requests/index.html.haml

 - @no_container = true
+- @bulk_edit = can?(current_user, :admin_merge_request, @project)
+
 - page_title "Merge Requests"
 = render "projects/issues/head"
 = render 'projects/last_push'

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

+- boards_page = controller.controller_name == 'boards'
+
 .issues-filters
   .issues-details-filters.row-content-block.second-block
     = form_tag page_filter_path(without: [:assignee_id, :author_id, :milestone_title, :label_name, :issue_search]), method: :get, class: 'filter-form js-filter-form' do
       - if params[:issue_search].present?
         = hidden_field_tag :issue_search, params[:issue_search]
-      - if controller.controller_name == 'issues' && can?(current_user, :admin_issue, @project)
+      - if @bulk_edit
         .check-all-holder
           = check_box_tag "check_all_issues", nil, false,
             class: "check_all_issues left"
@@ -30,7 +32,7 @@
           %a{href: page_filter_path(without: [:assignee_id, :author_id, :milestone_title, :label_name, :issue_search])} Reset filters
 
         .pull-right
-          - if controller.controller_name == 'boards'
+          - if boards_page
             #js-boards-seach.issue-boards-search
               %input.pull-left.form-control{ type: "search", placeholder: "Filter by name...", "v-model" => "filters.search", "debounce" => "250" }
               - if can?(current_user, :admin_list, @project)
@@ -45,9 +47,9 @@
           - else
             = render 'shared/sort_dropdown'
 
-    - if controller.controller_name == 'issues'
+    - if @bulk_edit
       .issues_bulk_update.hide
-        = form_tag bulk_update_namespace_project_issues_path(@project.namespace, @project), method: :post, class: 'bulk-update'  do
+        = form_tag [:bulk_update, @project.namespace.becomes(Namespace), @project, type], method: :post, class: 'bulk-update'  do
           .filter-item.inline
             = dropdown_tag("Status", options: { toggle_class: "js-issue-status", title: "Change status", dropdown_class: "dropdown-menu-status dropdown-menu-selectable", data: { field_name: "update[state_event]" } } ) do
               %ul
@@ -70,10 +72,10 @@
                 %li
                   %a{href: "#", data: {id: "unsubscribe"}} Unsubscribe
 
-          = hidden_field_tag 'update[issues_ids]', []
+          = hidden_field_tag 'update[issuable_ids]', []
           = hidden_field_tag :state_event, params[:state_event]
           .filter-item.inline
-            = button_tag "Update issues", class: "btn update_selected_issues btn-save"
+            = button_tag "Update #{type.to_s.humanize(capitalize: false)}", class: "btn update_selected_issues btn-save"
 
   - if !@labels.nil?
     .row-content-block.second-block.filtered-labels{ class: ("hidden" if !@labels.any?) }

config/routes.rb

@@ -747,6 +747,7 @@ Rails.application.routes.draw do
             get :branch_to
             get :update_branches
             get :diff_for_path
+            post :bulk_update
           end
 
           resources :discussions, only: [], constraints: { id: /\h{40}/ } do

features/steps/project/merge_requests.rb

@@ -31,7 +31,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   end
 
   step 'I click link "Closed"' do
-    click_link "Closed"
+    page.within('.issues-state-filters') do
+      click_link "Closed"
+    end
   end
 
   step 'I should see merge request "Wiki Feature"' do

spec/features/merge_requests/update_merge_requests_spec.rb

+require 'rails_helper'
+
+feature 'Multiple merge requests updating from merge_requests#index', feature: true do
+  include WaitForAjax
+
+  let!(:user)    { create(:user)}
+  let!(:project) { create(:project) }
+  let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+  before do
+    project.team << [user, :master]
+    login_as(user)
+  end
+
+  context 'status', js: true do
+    describe 'close merge request' do
+      before do
+        visit namespace_project_merge_requests_path(project.namespace, project)
+      end
+
+      it 'closes merge request' do
+        change_status('Closed')
+
+        expect(page).to have_selector('.merge-request', count: 0)
+      end
+    end
+
+    describe 'reopen merge request' do
+      before do
+        merge_request.close
+        visit namespace_project_merge_requests_path(project.namespace, project, state: 'closed')
+      end
+
+      it 'reopens merge request' do
+        change_status('Open')
+
+        expect(page).to have_selector('.merge-request', count: 0)
+      end
+    end
+  end
+
+  context 'assignee', js: true do
+    describe 'set assignee' do
+      before do
+        visit namespace_project_merge_requests_path(project.namespace, project)
+      end
+
+      it "updates merge request with assignee" do
+        change_assignee(user.name)
+
+        page.within('.merge-request .controls') do
+          expect(find('.author_link')["title"]).to have_content(user.name)
+        end
+      end
+    end
+
+    describe 'remove assignee' do
+      before do
+        merge_request.assignee = user
+        merge_request.save
+        visit namespace_project_merge_requests_path(project.namespace, project)
+      end
+
+      it "removes assignee from the merge request" do
+        change_assignee('Unassigned')
+
+        expect(find('.merge-request .controls')).not_to have_css('.author_link')
+      end
+    end
+  end
+
+  context 'milestone', js: true do
+    let(:milestone)  { create(:milestone, project: project) }
+
+    describe 'set milestone' do
+      before do
+        visit namespace_project_merge_requests_path(project.namespace, project)
+      end
+
+      it "updates merge request with milestone" do
+        change_milestone(milestone.title)
+
+        expect(find('.merge-request')).to have_content milestone.title
+      end
+    end
+
+    describe 'unset milestone' do
+      before do
+        merge_request.milestone = milestone
+        merge_request.save
+        visit namespace_project_merge_requests_path(project.namespace, project)
+      end
+
+      it "removes milestone from the merge request" do
+        change_milestone("No Milestone")
+
+        expect(find('.merge-request')).not_to have_content milestone.title
+      end
+    end
+  end
+
+  def change_status(text)
+    find('#check_all_issues').click
+    find('.js-issue-status').click
+    find('.dropdown-menu-status a', text: text).click
+    click_update_merge_requests_button
+  end
+
+  def change_assignee(text)
+    find('#check_all_issues').click
+    find('.js-update-assignee').click
+    wait_for_ajax
+
+    page.within '.dropdown-menu-user' do
+      click_link text
+    end
+
+    click_update_merge_requests_button
+  end
+
+  def change_milestone(text)
+    find('#check_all_issues').click
+    find('.issues_bulk_update .js-milestone-select').click
+    find('.dropdown-menu-milestone a', text: text).click
+    click_update_merge_requests_button
+  end
+
+  def click_update_merge_requests_button
+    find('.update_selected_issues').click
+    wait_for_ajax
+  end
+end

spec/services/issues/bulk_update_service_spec.rb → spec/services/issuable/bulk_update_service_spec.rb

 require 'spec_helper'
 
-describe Issues::BulkUpdateService, services: true do
+describe Issuable::BulkUpdateService, services: true do
   let(:user)    { create(:user) }
   let(:project) { create(:empty_project, namespace: user.namespace) }
 
   def bulk_update(issues, extra_params = {})
     bulk_update_params = extra_params
-      .reverse_merge(issues_ids: Array(issues).map(&:id).join(','))
+      .reverse_merge(issuable_ids: Array(issues).map(&:id).join(','))
 
-    Issues::BulkUpdateService.new(project, user, bulk_update_params).execute
+    Issuable::BulkUpdateService.new(project, user, bulk_update_params).execute('issue')
   end
 
   describe 'close issues' do