diff options
21 files changed, 237 insertions, 84 deletions
diff --git a/CHANGELOG b/CHANGELOG index bafd1e867f7..f709758abf6 100644 --- a/CHANGELOG +++ b/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) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 38cdc7b9fba..179d3bc38a5 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/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; diff --git a/app/assets/javascripts/issuable.js.es6 b/app/assets/javascripts/issuable.js.es6 index 4006ac740b2..82c14ef0157 100644 --- a/app/assets/javascripts/issuable.js.es6 +++ b/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'); diff --git a/app/assets/javascripts/issues-bulk-assignment.js b/app/assets/javascripts/issues-bulk-assignment.js index 98d3358ba92..8ca90490426 100644 --- a/app/assets/javascripts/issues-bulk-assignment.js +++ b/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: [] diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 46c4a11aa2e..63845e6b9ba 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/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; + } + } + } +} diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index d14224ed00f..7a26b7ad497 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/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; } diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index f40b62446e5..77b4efffd7f 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/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 diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 72d2d361878..de02e28e384 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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 diff --git a/app/services/issuable/bulk_update_service.rb b/app/services/issuable/bulk_update_service.rb new file mode 100644 index 00000000000..60891cbb255 --- /dev/null +++ b/app/services/issuable/bulk_update_service.rb @@ -0,0 +1,26 @@ +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 diff --git a/app/services/issues/bulk_update_service.rb b/app/services/issues/bulk_update_service.rb deleted file mode 100644 index 7e19a73f71a..00000000000 --- a/app/services/issues/bulk_update_service.rb +++ /dev/null @@ -1,25 +0,0 @@ -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 diff --git a/app/views/projects/issues/_issue.html.haml b/app/views/projects/issues/_issue.html.haml index 79b14819865..851d4c06990 100644 --- a/app/views/projects/issues/_issue.html.haml +++ b/app/views/projects/issues/_issue.html.haml @@ -1,7 +1,7 @@ %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 diff --git a/app/views/projects/issues/_issues.html.haml b/app/views/projects/issues/_issues.html.haml index f34f3c05737..a2c31c0b4c5 100644 --- a/app/views/projects/issues/_issues.html.haml +++ b/app/views/projects/issues/_issues.html.haml @@ -1,4 +1,4 @@ -%ul.content-list.issues-list +%ul.content-list.issues-list.issuable-list = render @issues - if @issues.blank? %li diff --git a/app/views/projects/issues/index.html.haml b/app/views/projects/issues/index.html.haml index 1a87045aa60..8da9f2100e9 100644 --- a/app/views/projects/issues/index.html.haml +++ b/app/views/projects/issues/index.html.haml @@ -1,4 +1,6 @@ - @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" diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index 5029b365f93..31f8d0aeb5b 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -1,4 +1,8 @@ %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) diff --git a/app/views/projects/merge_requests/_merge_requests.html.haml b/app/views/projects/merge_requests/_merge_requests.html.haml index 446887774a4..fe82f751f53 100644 --- a/app/views/projects/merge_requests/_merge_requests.html.haml +++ b/app/views/projects/merge_requests/_merge_requests.html.haml @@ -1,4 +1,4 @@ -%ul.content-list.mr-list +%ul.content-list.mr-list.issuable-list = render @merge_requests - if @merge_requests.blank? %li diff --git a/app/views/projects/merge_requests/index.html.haml b/app/views/projects/merge_requests/index.html.haml index ace275c689b..144b3a9c8c8 100644 --- a/app/views/projects/merge_requests/index.html.haml +++ b/app/views/projects/merge_requests/index.html.haml @@ -1,4 +1,6 @@ - @no_container = true +- @bulk_edit = can?(current_user, :admin_merge_request, @project) + - page_title "Merge Requests" = render "projects/issues/head" = render 'projects/last_push' diff --git a/app/views/shared/issuable/_filter.html.haml b/app/views/shared/issuable/_filter.html.haml index fabf6d74392..93c4d5c3d30 100644 --- a/app/views/shared/issuable/_filter.html.haml +++ b/app/views/shared/issuable/_filter.html.haml @@ -1,9 +1,11 @@ +- 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?) } diff --git a/config/routes.rb b/config/routes.rb index 262a174437a..068c92d1400 100644 --- a/config/routes.rb +++ b/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 diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 56b28949585..df17b5626c6 100644 --- a/features/steps/project/merge_requests.rb +++ b/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 diff --git a/spec/features/merge_requests/update_merge_requests_spec.rb b/spec/features/merge_requests/update_merge_requests_spec.rb new file mode 100644 index 00000000000..b56fdfe5611 --- /dev/null +++ b/spec/features/merge_requests/update_merge_requests_spec.rb @@ -0,0 +1,132 @@ +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 diff --git a/spec/services/issues/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index ac08aa53b0b..6f7ce8ca992 100644 --- a/spec/services/issues/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -1,14 +1,14 @@ 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 |