Commit 94258b72 authored by Jarka Košanová's avatar Jarka Košanová

Improve errors reporting for board lists update

- replace the hash with ServiceResponse
- make sure the errors are returned if
nothing is updated
- updated the callers
parent 9b26541f
......@@ -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
......
......@@ -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
......
......@@ -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)
......
---
title: Improve errors reporting for board lists update
merge_request: 59549
author:
type: added
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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) }
......
......@@ -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) }
......
......@@ -2,14 +2,30 @@
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
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
service.execute(list)
end
service.execute(list)
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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment