Add support for updating labels with quick actions

Fixes the case when the update service is passed label_ids together
with add_label_ids and remove_label_ids from quick actions

This allows support for updating the labels via the dropdown and the
description quick actions in one request

app/controllers/concerns/issuable_actions.rb

@@ -16,19 +16,6 @@ module IssuableActions
     end
   end
 
-  def permitted_keys
-    [
-      :issuable_ids,
-      :assignee_id,
-      :milestone_id,
-      :state_event,
-      :subscription_event,
-      label_ids: [],
-      add_label_ids: [],
-      remove_label_ids: []
-    ]
-  end
-
   def show
     respond_to do |format|
       format.html do
@@ -221,10 +208,20 @@ module IssuableActions
   end
 
   def bulk_update_params
-    permitted_keys_array = permitted_keys.dup
-    permitted_keys_array << { assignee_ids: [] }
+    params.require(:update).permit(bulk_update_permitted_keys)
+  end
 
-    params.require(:update).permit(permitted_keys_array)
+  def bulk_update_permitted_keys
+    [
+      :issuable_ids,
+      :assignee_id,
+      :milestone_id,
+      :state_event,
+      :subscription_event,
+      assignee_ids: [],
+      add_label_ids: [],
+      remove_label_ids: []
+    ]
   end
 
   def resource_name

app/services/issuable/bulk_update_service.rb

@@ -17,9 +17,8 @@ module Issuable
       ids = params.delete(:issuable_ids).split(",")
       items = find_issuables(parent, model_class, ids)
 
-      permitted_attrs(type).each do |key|
-        params.delete(key) unless params[key].present?
-      end
+      params.slice!(*permitted_attrs(type))
+      params.delete_if { |k, v| v.blank? }
 
       if params[:assignee_ids] == [IssuableFinder::Params::NONE.to_s]
         params[:assignee_ids] = []

app/services/issuable_base_service.rb

@@ -129,15 +129,11 @@ class IssuableBaseService < BaseService
     add_label_ids = attributes.delete(:add_label_ids)
     remove_label_ids = attributes.delete(:remove_label_ids)
 
-    new_label_ids = existing_label_ids || label_ids || []
+    new_label_ids = label_ids || existing_label_ids || []
     new_label_ids |= extra_label_ids
 
-    if add_label_ids.blank? && remove_label_ids.blank?
-      new_label_ids = label_ids if label_ids
-    else
-      new_label_ids |= add_label_ids if add_label_ids
-      new_label_ids -= remove_label_ids if remove_label_ids
-    end
+    new_label_ids |= add_label_ids if add_label_ids
+    new_label_ids -= remove_label_ids if remove_label_ids
 
     new_label_ids.uniq
   end

ee/app/controllers/concerns/ee/issuable_actions.rb

@@ -9,8 +9,10 @@ module EE
       weight
     ].freeze
 
-    override :permitted_keys
-    def permitted_keys
+    private
+
+    override :bulk_update_permitted_keys
+    def bulk_update_permitted_keys
       @permitted_keys ||= (super + EE_PERMITTED_KEYS).freeze
     end
   end

spec/services/issuable/bulk_update_service_spec.rb

@@ -42,13 +42,11 @@ describe Issuable::BulkUpdateService do
     let(:issue_no_labels) { create(:issue, project: project) }
     let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests, issue_no_labels] }
 
-    let(:labels) { [] }
     let(:add_labels) { [] }
     let(:remove_labels) { [] }
 
     let(:bulk_update_params) do
       {
-        label_ids:        labels.map(&:id),
         add_label_ids:    add_labels.map(&:id),
         remove_label_ids: remove_labels.map(&:id)
       }
@@ -58,27 +56,6 @@ describe Issuable::BulkUpdateService do
       bulk_update(issues, bulk_update_params)
     end
 
-    context 'when label_ids are passed' do
-      let(:issues) { [issue_all_labels, issue_no_labels] }
-      let(:labels) { [bug, regression] }
-
-      it 'updates the labels of all issues passed to the labels passed' do
-        expect(issues.map(&:reload).map(&:label_ids)).to all(match_array(labels.map(&:id)))
-      end
-
-      it 'does not update issues not passed in' do
-        expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id)
-      end
-
-      context 'when those label IDs are empty' do
-        let(:labels) { [] }
-
-        it 'updates the issues passed to have no labels' do
-          expect(issues.map(&:reload).map(&:label_ids)).to all(be_empty)
-        end
-      end
-    end
-
     context 'when add_label_ids are passed' do
       let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] }
       let(:add_labels) { [bug, regression, merge_requests] }
@@ -122,69 +99,21 @@ describe Issuable::BulkUpdateService do
         expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id)
       end
     end
+  end
 
-    context 'when add_label_ids and label_ids are passed' do
-      let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests] }
-      let(:labels) { [merge_requests] }
-      let(:add_labels) { [regression] }
-
-      it 'adds the label IDs to all issues passed' do
-        expect(issues.map(&:reload).map(&:label_ids)).to all(include(regression.id))
-      end
-
-      it 'ignores the label IDs parameter' do
-        expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id))
-      end
-
-      it 'does not update issues not passed in' do
-        expect(issue_no_labels.label_ids).to be_empty
-      end
-    end
-
-    context 'when remove_label_ids and label_ids are passed' do
-      let(:issues) { [issue_no_labels, issue_bug_and_regression] }
-      let(:labels) { [merge_requests] }
-      let(:remove_labels) { [regression] }
-
-      it 'removes the label IDs from all issues passed' do
-        expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(regression.id)
-      end
-
-      it 'ignores the label IDs parameter' do
-        expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id)
-      end
-
-      it 'does not update issues not passed in' do
-        expect(issue_all_labels.label_ids).to contain_exactly(bug.id, regression.id, merge_requests.id)
-      end
-    end
-
-    context 'when add_label_ids, remove_label_ids, and label_ids are passed' do
-      let(:issues) { [issue_bug_and_merge_requests, issue_no_labels] }
-      let(:labels) { [regression] }
-      let(:add_labels) { [bug] }
-      let(:remove_labels) { [merge_requests] }
-
-      it 'adds the label IDs to all issues passed' do
-        expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id))
-      end
+  context 'with issuables at a project level' do
+    let(:parent) { project }
 
-      it 'removes the label IDs from all issues passed' do
-        expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(merge_requests.id)
-      end
+    context 'with unpermitted attributes' do
+      let(:issues) { create_list(:issue, 2, project: project) }
+      let(:label) { create(:label, project: project) }
 
-      it 'ignores the label IDs parameter' do
-        expect(issues.map(&:reload).flat_map(&:label_ids)).not_to include(regression.id)
-      end
+      it 'does not update the issues' do
+        bulk_update(issues, label_ids: [label.id])
 
-      it 'does not update issues not passed in' do
-        expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id)
+        expect(issues.map(&:reload).map(&:label_ids)).not_to include(label.id)
       end
     end
-  end
-
-  context 'with issuables at a project level' do
-    let(:parent) { project }
 
     describe 'close issues' do
       let(:issues) { create_list(:issue, 2, project: project) }

spec/services/issues/update_service_spec.rb

@@ -3,13 +3,13 @@
 require 'spec_helper'
 
 describe Issues::UpdateService, :mailer do
-  let(:user) { create(:user) }
-  let(:user2) { create(:user) }
-  let(:user3) { create(:user) }
-  let(:group) { create(:group, :public) }
-  let(:project) { create(:project, :repository, group: group) }
-  let(:label) { create(:label, project: project) }
-  let(:label2) { create(:label) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:user2) { create(:user) }
+  let_it_be(:user3) { create(:user) }
+  let_it_be(:group) { create(:group, :public) }
+  let_it_be(:project, reload: true) { create(:project, :repository, group: group) }
+  let_it_be(:label) { create(:label, project: project) }
+  let_it_be(:label2) { create(:label, project: project) }
 
   let(:issue) do
     create(:issue, title: 'Old title',
@@ -19,7 +19,7 @@ describe Issues::UpdateService, :mailer do
                    author: create(:user))
   end
 
-  before do
+  before_all do
     project.add_maintainer(user)
     project.add_developer(user2)
     project.add_developer(user3)
@@ -669,28 +669,24 @@ describe Issues::UpdateService, :mailer do
       context 'when add_label_ids and label_ids are passed' do
         let(:params) { { label_ids: [label.id], add_label_ids: [label3.id] } }
 
-        it 'ignores the label_ids parameter' do
-          expect(result.label_ids).not_to include(label.id)
+        before do
+          issue.update(labels: [label2])
         end
 
-        it 'adds the passed labels' do
-          expect(result.label_ids).to include(label3.id)
+        it 'replaces the labels with the ones in label_ids and adds those in add_label_ids' do
+          expect(result.label_ids).to contain_exactly(label.id, label3.id)
         end
       end
 
       context 'when remove_label_ids and label_ids are passed' do
-        let(:params) { { label_ids: [], remove_label_ids: [label.id] } }
+        let(:params) { { label_ids: [label.id, label2.id, label3.id], remove_label_ids: [label.id] } }
 
         before do
           issue.update(labels: [label, label3])
         end
 
-        it 'ignores the label_ids parameter' do
-          expect(result.label_ids).not_to be_empty
-        end
-
-        it 'removes the passed labels' do
-          expect(result.label_ids).not_to include(label.id)
+        it 'replaces the labels with the ones in label_ids and removes those in remove_label_ids' do
+          expect(result.label_ids).to contain_exactly(label2.id, label3.id)
         end
       end