From 1ebef4aae55adb5e17a76b92aea74467e49130bf Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Fri, 5 Jul 2019 12:50:20 +0200 Subject: Add result to MoveService#execute_multiple It adds a hash response which includes the count, success state and the moved issues itself so the caller has additional information about the result of the process. --- app/controllers/boards/issues_controller.rb | 7 ++-- app/services/boards/issues/move_service.rb | 39 +++++++++++++++++++---- spec/controllers/boards/issues_controller_spec.rb | 13 ++++++-- spec/services/boards/issues/move_service_spec.rb | 4 +-- 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb index 353a9806fd1..90528f75ffd 100644 --- a/app/controllers/boards/issues_controller.rb +++ b/app/controllers/boards/issues_controller.rb @@ -58,11 +58,8 @@ module Boards service = Boards::Issues::MoveService.new(board_parent, current_user, move_params(true)) issues = Issue.find(params[:ids]) - if service.execute_multiple(issues) - head :ok - else - head :unprocessable_entity - end + + render json: service.execute_multiple(issues) end def update diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb index 755d723b9a0..00ce27db7c8 100644 --- a/app/services/boards/issues/move_service.rb +++ b/app/services/boards/issues/move_service.rb @@ -11,26 +11,51 @@ module Boards end def execute_multiple(issues) - return false if issues.empty? + return execute_multiple_empty_result if issues.empty? + handled_issues = [] last_inserted_issue_id = nil - issues.map do |issue| + count = issues.each.inject(0) do |moved_count, issue| issue_modification_params = issue_params(issue) - next if issue_modification_params.empty? + next moved_count if issue_modification_params.empty? if last_inserted_issue_id - issue_modification_params[:move_between_ids] = move_between_ids({ move_after_id: nil, move_before_id: last_inserted_issue_id }) + issue_modification_params[:move_between_ids] = move_below(last_inserted_issue_id) end last_inserted_issue_id = issue.id - move_single_issue(issue, issue_modification_params) - end.all? + handled_issue = move_single_issue(issue, issue_modification_params) + handled_issues << present_issue_entity(handled_issue) if handled_issue + handled_issue && handled_issue.valid? ? moved_count + 1 : moved_count + end + + { + count: count, + success: count == issues.size, + issues: handled_issues + } end private + def present_issue_entity(issue) + ::API::Entities::Issue.represent(issue) + end + + def execute_multiple_empty_result + @execute_multiple_empty_result ||= { + count: 0, + success: false, + issues: [] + } + end + + def move_below(id) + move_between_ids({ move_after_id: nil, move_before_id: id }) + end + def move_single_issue(issue, issue_modification_params) - return false unless can?(current_user, :update_issue, issue) + return unless can?(current_user, :update_issue, issue) update(issue, issue_modification_params) end diff --git a/spec/controllers/boards/issues_controller_spec.rb b/spec/controllers/boards/issues_controller_spec.rb index 246d6f9e0f9..f8a25d814ed 100644 --- a/spec/controllers/boards/issues_controller_spec.rb +++ b/spec/controllers/boards/issues_controller_spec.rb @@ -160,7 +160,7 @@ describe Boards::IssuesController do end end - describe 'PUT move_multiple' do + describe 'PUT bulk_move' do let(:todo) { create(:group_label, group: group, name: 'Todo') } let(:development) { create(:group_label, group: group, name: 'Development') } let(:user) { create(:group_member, :maintainer, user: create(:user), group: group ).user } @@ -200,12 +200,21 @@ describe Boards::IssuesController do put :bulk_move, params: move_issues_params expect(response).to have_gitlab_http_status(expected_status) + if expected_status == 200 + expect(json_response).to include( + 'count' => move_issues_params[:ids].size, + 'success' => true + ) + + expect(json_response['issues'].pluck('id')).to include(*move_issues_params[:ids]) + end + list_issues user: requesting_user, board: board, list: list2 expect(response).to have_gitlab_http_status(200) expect(response).to match_response_schema('entities/issue_boards') - responded_issues = json_response['issues'] + responded_issues = JSON.parse(response.body)['issues'] expect(responded_issues.length).to eq expected_issue_count ids_in_order = responded_issues.pluck('id') diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb index 1bfb5602df2..cf84ec8fd4c 100644 --- a/spec/services/boards/issues/move_service_spec.rb +++ b/spec/services/boards/issues/move_service_spec.rb @@ -68,8 +68,8 @@ describe Boards::Issues::MoveService do project.add_developer(user) end - it 'returns false if list of issues is empty' do - expect(described_class.new(group, user, params).execute_multiple([])).to eq(false) + it 'returns the expected result if list of issues is empty' do + expect(described_class.new(group, user, params).execute_multiple([])).to eq({ count: 0, success: false, issues: [] }) end context 'moving multiple issues' do -- cgit v1.2.1 From 52bdcdff20127b09b5f088138b899c9ad6a1048b Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Tue, 16 Jul 2019 14:55:59 +0200 Subject: Use match_array instead of include --- spec/controllers/boards/issues_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/boards/issues_controller_spec.rb b/spec/controllers/boards/issues_controller_spec.rb index f8a25d814ed..2f0a8f93bd5 100644 --- a/spec/controllers/boards/issues_controller_spec.rb +++ b/spec/controllers/boards/issues_controller_spec.rb @@ -206,7 +206,7 @@ describe Boards::IssuesController do 'success' => true ) - expect(json_response['issues'].pluck('id')).to include(*move_issues_params[:ids]) + expect(json_response['issues'].pluck('id')).to match_array(move_issues_params[:ids]) end list_issues user: requesting_user, board: board, list: list2 -- cgit v1.2.1 From ca089d91f6313b32a6381aef36583e83869f4017 Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Tue, 16 Jul 2019 16:17:37 +0200 Subject: Modified spec so a second call to json_response is avoided --- spec/controllers/boards/issues_controller_spec.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/spec/controllers/boards/issues_controller_spec.rb b/spec/controllers/boards/issues_controller_spec.rb index 2f0a8f93bd5..0db58fbefc1 100644 --- a/spec/controllers/boards/issues_controller_spec.rb +++ b/spec/controllers/boards/issues_controller_spec.rb @@ -196,7 +196,7 @@ describe Boards::IssuesController do sign_in(signed_in_user) end - it 'moves issues as expected' do + it 'responds as expected' do put :bulk_move, params: move_issues_params expect(response).to have_gitlab_http_status(expected_status) @@ -208,13 +208,18 @@ describe Boards::IssuesController do expect(json_response['issues'].pluck('id')).to match_array(move_issues_params[:ids]) end + end + + it 'moves issues as expected' do + put :bulk_move, params: move_issues_params + expect(response).to have_gitlab_http_status(expected_status) list_issues user: requesting_user, board: board, list: list2 expect(response).to have_gitlab_http_status(200) expect(response).to match_response_schema('entities/issue_boards') - responded_issues = JSON.parse(response.body)['issues'] + responded_issues = json_response['issues'] expect(responded_issues.length).to eq expected_issue_count ids_in_order = responded_issues.pluck('id') -- cgit v1.2.1