Merge branch 'improve-lists-update-errors' into 'master'

Improve errors reporting for board lists update

See merge request gitlab-org/gitlab!59549

app/controllers/boards/lists_controller.rb

@@ -33,10 +33,10 @@ module Boards
       service = Boards::Lists::UpdateService.new(board_parent, current_user, update_list_params)
       result = service.execute(list)
 
-      if result[:status] == :success
+      if result.success?
         head :ok
       else
-        head result[:http_status]
+        head result.http_status
       end
     end
 

app/graphql/mutations/boards/lists/base_update.rb

@@ -20,8 +20,8 @@ module Mutations
           update_result = update_list(list, args)
 
           {
-            list: update_result[:list],
-            errors: list.errors.full_messages
+            list: update_result.payload[:list],
+            errors: update_result.errors
           }
         end
 

app/services/boards/lists/base_update_service.rb

@@ -3,16 +3,30 @@
 module Boards
   module Lists
     class BaseUpdateService < Boards::BaseService
+      extend ::Gitlab::Utils::Override
+
       def execute(list)
         if execute_by_params(list)
           success(list: list)
         else
-          error(list.errors.messages, 422)
+          message = list.errors.empty? ? 'The update was not successful.' : list.errors.messages
+
+          error(message, { list: list })
         end
       end
 
       private
 
+      override :error
+      def error(message, pass_back = {})
+        ServiceResponse.error(message: message, http_status: :unprocessable_entity, payload: pass_back)
+      end
+
+      override :success
+      def success(pass_back = {})
+        ServiceResponse.success(payload: pass_back)
+      end
+
       def execute_by_params(list)
         update_preferences_result = update_preferences(list) if can_read?(list)
         update_position_result = update_position(list) if can_admin?(list)

changelogs/unreleased/improve-lists-update-errors.yml

+---
+title: Improve errors reporting for board lists update
+merge_request: 59549
+author:
+type: added

ee/app/graphql/mutations/boards/lists/update_limit_metrics.rb

@@ -46,8 +46,8 @@ module Mutations
           update_result = update_list(args)
 
           {
-            list: update_result[:list],
-            errors: list.errors.full_messages
+            list: update_result.payload.fetch(:list),
+            errors: update_result.errors
           }
         end
 

ee/spec/graphql/mutations/boards/lists/update_limit_metrics_spec.rb

@@ -38,6 +38,10 @@ RSpec.describe Mutations::Boards::Lists::UpdateLimitMetrics do
         expect(reloaded_list.max_issue_count).to eq(10)
         expect(reloaded_list.max_issue_weight).to eq(50)
       end
+
+      it 'returns the correct response' do
+        expect(subject.keys).to match_array([:list, :errors])
+      end
     end
 
     context 'without admin rights' do

ee/spec/services/boards/epic_lists/update_service_spec.rb

@@ -6,7 +6,8 @@ RSpec.describe Boards::EpicLists::UpdateService do
   let_it_be(:user) { create(:user) }
   let_it_be(:group) { create(:group, :private) }
   let_it_be(:board) { create(:epic_board, group: group) }
-  let_it_be(:list) { create(:epic_list, epic_board: board, position: 0) }
+  let_it_be_with_reload(:list) { create(:epic_list, epic_board: board, position: 0) }
+  let_it_be_with_reload(:list2) { create(:epic_list, epic_board: board, position: 1) }
 
   before do
     stub_licensed_features(epics: true)

ee/spec/services/boards/lists/update_service_spec.rb

@@ -36,8 +36,8 @@ RSpec.describe 'EE::Boards::Lists::UpdateService' do
           service = Boards::Lists::UpdateService.new(board, user, { limit_metric: 'foo' })
           result = service.execute(list)
 
-          expect(result[:http_status]).to eq(422)
-          expect(result[:status]).to eq(:error)
+          expect(result.http_status).to eq(:unprocessable_entity)
+          expect(result.status).to eq(:error)
 
           reloaded_list = list.reload
           expect(reloaded_list.limit_metric).to be_nil
@@ -200,7 +200,7 @@ RSpec.describe 'EE::Boards::Lists::UpdateService' do
       service = Boards::Lists::UpdateService.new(board, user, initialization_params)
       result = service.execute(list)
 
-      expect(result[:status]).to eq(expected_service_result)
+      expect(result.status).to eq(expected_service_result)
 
       reloaded_list = list.reload
 

spec/requests/api/graphql/mutations/boards/lists/update_spec.rb

@@ -11,6 +11,7 @@ RSpec.describe 'Update of an existing board list' do
   let_it_be(:list) { create(:list, board: board, position: 0) }
   let_it_be(:list2) { create(:list, board: board) }
   let_it_be(:input) { { list_id: list.to_global_id.to_s, position: 1, collapsed: true } }
+
   let(:mutation) { graphql_mutation(:update_board_list, input) }
   let(:mutation_response) { graphql_mutation_response(:update_board_list) }
 

spec/services/boards/lists/update_service_spec.rb

@@ -3,8 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Boards::Lists::UpdateService do
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+
   let!(:list) { create(:list, board: board, position: 0) }
+  let!(:list2) { create(:list, board: board, position: 1) }
 
   describe '#execute' do
     let(:service) { described_class.new(board.resource_parent, user, params) }

spec/support/shared_examples/boards/lists/update_service_shared_examples.rb

@@ -2,15 +2,31 @@
 
 RSpec.shared_examples 'moving list' do
   context 'when user can admin list' do
-    it 'calls Lists::MoveService to update list position' do
+    before do
       board.resource_parent.add_developer(user)
+    end
 
+    context 'when the new position is valid' do
+      it 'calls Lists::MoveService to update list position' do
         expect_next_instance_of(Boards::Lists::MoveService, board.resource_parent, user, params) do |move_service|
           expect(move_service).to receive(:execute).with(list).and_call_original
         end
 
         service.execute(list)
       end
+
+      it 'returns a success response' do
+        expect(service.execute(list)).to be_success
+      end
+    end
+
+    context 'when the new position is invalid' do
+      let(:params) { { position: 10 } }
+
+      it 'returns error response' do
+        expect(service.execute(list)).to be_error
+      end
+    end
   end
 
   context 'when user cannot admin list' do
@@ -19,6 +35,10 @@ RSpec.shared_examples 'moving list' do
 
       service.execute(list)
     end
+
+    it 'returns an error response' do
+      expect(service.execute(list)).to be_error
+    end
   end
 end