Use ServiceResponse in BulkUpdateService

- Refactor BulkUpdateService to include
error responses
- Update IssuableActions to render error message
- Update issues controller specs
- Update BulkUpdateService specs

app/controllers/concerns/issuable_actions.rb

@@ -110,9 +110,13 @@ module IssuableActions
 
   def bulk_update
     result = Issuable::BulkUpdateService.new(parent, current_user, bulk_update_params).execute(resource_name)
-    quantity = result[:count]
 
-    render json: { notice: "#{quantity} #{resource_name.pluralize(quantity)} updated" }
+    if result.success?
+      quantity = result.payload[:count]
+      render json: { notice: "#{quantity} #{resource_name.pluralize(quantity)} updated" }
+    elsif result.error?
+      render json: { errors: result.message }, status: result.http_status
+    end
   end
 
   # rubocop:disable CodeReuse/ActiveRecord

app/services/issuable/bulk_update_service.rb

@@ -11,34 +11,29 @@ module Issuable
     end
 
     def execute(type)
-      model_class = type.classify.constantize
-      update_class = type.classify.pluralize.constantize::UpdateService
-
       ids = params.delete(:issuable_ids).split(",")
-      items = find_issuables(parent, model_class, ids)
-      filter_update_params(type)
-
-      items.each do |issuable|
-        next unless can?(current_user, :"update_#{type}", issuable)
-
-        update_class.new(issuable.issuing_parent, current_user, params).execute(issuable)
-      end
+      set_update_params(type)
+      items = update_issuables(type, ids)
 
-      {
-        count:    items.count,
-        success:  !items.count.zero?
-      }
+      response_success(payload: { count: items.count })
+    rescue ArgumentError => e
+      response_error(e.message, 422)
     end
 
     private
 
-    def permitted_attrs(type)
-      attrs = %i(state_event milestone_id add_label_ids remove_label_ids subscription_event)
+    def set_update_params(type)
+      params.slice!(*permitted_attrs(type))
+      params.delete_if { |k, v| v.blank? }
 
-      issuable_specific_attrs(type, attrs)
+      if params[:assignee_ids] == [IssuableFinder::Params::NONE.to_s]
+        params[:assignee_ids] = []
+      end
     end
 
-    def issuable_specific_attrs(type, attrs)
+    def permitted_attrs(type)
+      attrs = %i(state_event milestone_id add_label_ids remove_label_ids subscription_event)
+
       if type == 'issue' || type == 'merge_request'
         attrs.push(:assignee_ids)
       else
@@ -46,13 +41,18 @@ module Issuable
       end
     end
 
-    def filter_update_params(type)
-      params.slice!(*permitted_attrs(type))
-      params.delete_if { |k, v| v.blank? }
+    def update_issuables(type, ids)
+      model_class = type.classify.constantize
+      update_class = type.classify.pluralize.constantize::UpdateService
+      items = find_issuables(parent, model_class, ids)
 
-      if params[:assignee_ids] == [IssuableFinder::Params::NONE.to_s]
-        params[:assignee_ids] = []
+      items.each do |issuable|
+        next unless can?(current_user, :"update_#{type}", issuable)
+
+        update_class.new(issuable.issuing_parent, current_user, params).execute(issuable)
       end
+
+      items
     end
 
     def find_issuables(parent, model_class, ids)
@@ -62,6 +62,14 @@ module Issuable
         model_class.id_in(ids).of_projects(parent.all_projects)
       end
     end
+
+    def response_success(message: nil, payload: nil)
+      ServiceResponse.success(message: message, payload: payload)
+    end
+
+    def response_error(message, http_status)
+      ServiceResponse.error(message: message, http_status: http_status)
+    end
   end
 end
 

ee/app/services/ee/issuable/bulk_update_service.rb

@@ -14,20 +14,18 @@ module EE
         super
       end
 
-      override :issuable_specific_attrs
-      def issuable_specific_attrs(type, attrs)
+      override :permitted_attrs
+      def permitted_attrs(type)
         return super unless type == 'issue'
 
-        super.push(:health_status, :epic)
+        super.push(:health_status, :epic_id)
       end
 
-      override :filter_update_params
-      def filter_update_params(type)
+      override :set_update_params
+      def set_update_params(type)
         super
 
         set_health_status
-
-        params
       end
 
       def set_health_status

ee/spec/features/issues/bulk_assignment_health_status_spec.rb

@@ -62,6 +62,10 @@ RSpec.describe 'Issues > Health status bulk assignment' do
     end
   end
 
+  before do
+    stub_feature_flags(vue_issuables_list: false)
+  end
+
   context 'as an allowed user', :js do
     before do
       allow(group).to receive(:feature_enabled?).and_return(true)

ee/spec/services/ee/issuable/bulk_update_service_spec.rb

@@ -12,8 +12,8 @@ RSpec.describe Issuable::BulkUpdateService do
 
   shared_examples 'updates issuables attribute' do |attribute|
     it 'succeeds and returns the correct number of issuables updated' do
-      expect(subject[:success]).to be_truthy
-      expect(subject[:count]).to eq(issuables.count)
+      expect(subject.success?).to be_truthy
+      expect(subject.payload[:count]).to eq(issuables.count)
       issuables.each do |issuable|
         expect(issuable.reload.send(attribute)).to eq(new_value)
       end
@@ -31,46 +31,41 @@ RSpec.describe Issuable::BulkUpdateService do
   context 'with issues' do
     let_it_be(:type) { 'issue' }
     let_it_be(:parent) { group }
-    let(:issue1) { create(:issue, project: project1, health_status: :at_risk, epic: epic) }
-    let(:issue2) { create(:issue, project: project2, health_status: :at_risk, epic: epic) }
-    let(:epic) { create(:epic, group: group) }
-    let(:epic2) { create(:epic, group: group) }
+    let(:issue1) { create(:issue, project: project1, health_status: :at_risk) }
+    let(:issue2) { create(:issue, project: project2, health_status: :at_risk) }
     let(:issuables) { [issue1, issue2] }
 
     before do
       group.add_reporter(user)
     end
 
-    context 'updating health status and epic' do
+    context 'updating health status' do
       let(:params) do
         {
           issuable_ids: issuables.map(&:id),
-          health_status: :on_track,
-          epic: epic2
+          health_status: :on_track
         }
       end
 
       context 'when features are enabled' do
         before do
-          stub_licensed_features(epics: true, issuable_health_status: true)
+          stub_licensed_features(issuable_health_status: true)
         end
 
         it 'succeeds and returns the correct number of issuables updated' do
-          expect(subject[:success]).to be_truthy
-          expect(subject[:count]).to eq(issuables.count)
+          expect(subject.success?).to be_truthy
+          expect(subject.payload[:count]).to eq(issuables.count)
           issuables.each do |issuable|
-            issuable.reload
-            expect(issuable.epic).to eq(epic2)
-            expect(issuable.health_status).to eq('on_track')
+            expect(issuable.reload.health_status).to eq('on_track')
           end
         end
 
-        context "when params values are '0'" do
+        context "when params value is '0'" do
           let(:params) { { issuable_ids: issuables.map(&:id), health_status: '0' } }
 
           it 'succeeds and remove values' do
-            expect(subject[:success]).to be_truthy
-            expect(subject[:count]).to eq(issuables.count)
+            expect(subject.success?).to be_truthy
+            expect(subject.payload[:count]).to eq(issuables.count)
             issuables.each do |issuable|
               issuable.reload
               expect(issuable.health_status).to be_nil
@@ -79,13 +74,12 @@ RSpec.describe Issuable::BulkUpdateService do
         end
       end
 
-      context 'when features are disabled' do
+      context 'when feature issuable_health_status is disabled' do
         before do
-          stub_licensed_features(epics: false, issuable_health_status: false)
+          stub_licensed_features(issuable_health_status: false)
         end
 
         it_behaves_like 'does not update issuables attribute', :health_status
-        it_behaves_like 'does not update issuables attribute', :epic
       end
 
       context 'when user can not update issues' do
@@ -94,14 +88,6 @@ RSpec.describe Issuable::BulkUpdateService do
         end
 
         it_behaves_like 'does not update issuables attribute', :health_status
-        it_behaves_like 'does not update issuables attribute', :epic
-      end
-
-      context 'when user can not admin epic' do
-        let(:epic3) { create(:epic, group: create(:group)) }
-        let(:params) { { issuable_ids: issuables.map(&:id), epic: epic3 } }
-
-        it_behaves_like 'does not update issuables attribute', :epic
       end
     end
   end
@@ -153,8 +139,8 @@ RSpec.describe Issuable::BulkUpdateService do
         let(:params) { { issuable_ids: [epic1.id, epic3.id, outer_epic.id], add_label_ids: [label3.id] } }
 
         it 'updates epics that belong to the parent group or descendants' do
-          expect(subject[:success]).to be_truthy
-          expect(subject[:count]).to eq(2)
+          expect(subject.success?).to be_truthy
+          expect(subject.payload[:count]).to eq(2)
 
           expect(epic1.reload.labels).to eq([label1, label3])
           expect(epic3.reload.labels).to eq([label1, label3])

spec/services/issuable/bulk_update_service_spec.rb

@@ -18,8 +18,8 @@ RSpec.describe Issuable::BulkUpdateService do
     it 'succeeds' do
       result = bulk_update(issuables, milestone_id: milestone.id)
 
-      expect(result[:success]).to be_truthy
-      expect(result[:count]).to eq(issuables.count)
+      expect(result.success?).to be_truthy
+      expect(result.payload[:count]).to eq(issuables.count)
     end
 
     it 'updates the issuables milestone' do
@@ -121,8 +121,8 @@ RSpec.describe Issuable::BulkUpdateService do
       it 'succeeds and returns the correct number of issues updated' do
         result = bulk_update(issues, state_event: 'close')
 
-        expect(result[:success]).to be_truthy
-        expect(result[:count]).to eq(issues.count)
+        expect(result.success?).to be_truthy
+        expect(result.payload[:count]).to eq(issues.count)
       end
 
       it 'closes all the issues passed' do
@@ -139,8 +139,8 @@ RSpec.describe Issuable::BulkUpdateService do
       it 'succeeds and returns the correct number of issues updated' do
         result = bulk_update(issues, state_event: 'reopen')
 
-        expect(result[:success]).to be_truthy
-        expect(result[:count]).to eq(issues.count)
+        expect(result.success?).to be_truthy
+        expect(result.payload[:count]).to eq(issues.count)
       end
 
       it 'reopens all the issues passed' do
@@ -161,8 +161,8 @@ RSpec.describe Issuable::BulkUpdateService do
 
           result = bulk_update(merge_request, assignee_ids: [user.id, new_assignee.id])
 
-          expect(result[:success]).to be_truthy
-          expect(result[:count]).to eq(1)
+          expect(result.success?).to be_truthy
+          expect(result.payload[:count]).to eq(1)
         end
 
         it 'updates the assignee to the user ID passed' do
@@ -199,8 +199,8 @@ RSpec.describe Issuable::BulkUpdateService do
 
           result = bulk_update(issue, assignee_ids: [new_assignee.id])
 
-          expect(result[:success]).to be_truthy
-          expect(result[:count]).to eq(1)
+          expect(result.success?).to be_truthy
+          expect(result.payload[:count]).to eq(1)
         end
 
         it 'updates the assignee to the user ID passed' do
@@ -273,8 +273,8 @@ RSpec.describe Issuable::BulkUpdateService do
         issue2 = create(:issue, project: create(:project))
         result = bulk_update([issue1, issue2], assignee_ids: [user.id])
 
-        expect(result[:success]).to be_truthy
-        expect(result[:count]).to eq(1)
+        expect(result.success?).to be_truthy
+        expect(result.payload[:count]).to eq(1)
 
         expect(issue1.reload.assignees).to eq([user])
         expect(issue2.reload.assignees).to be_empty
@@ -332,8 +332,8 @@ RSpec.describe Issuable::BulkUpdateService do
         milestone = create(:milestone, group: group)
         result = bulk_update([issue1, issue2, issue3], milestone_id: milestone.id)
 
-        expect(result[:success]).to be_truthy
-        expect(result[:count]).to eq(2)
+        expect(result.success?).to be_truthy
+        expect(result.payload[:count]).to eq(2)
 
         expect(issue1.reload.milestone).to eq(milestone)
         expect(issue2.reload.milestone).to be_nil