diff options
-rw-r--r-- | app/controllers/projects/boards/issues_controller.rb | 8 | ||||
-rw-r--r-- | app/controllers/projects/boards/lists_controller.rb | 11 | ||||
-rw-r--r-- | app/services/boards/issues/move_service.rb | 8 | ||||
-rw-r--r-- | app/services/boards/lists/destroy_service.rb | 14 | ||||
-rw-r--r-- | app/services/boards/lists/move_service.rb | 21 | ||||
-rw-r--r-- | spec/services/boards/issues/move_service_spec.rb | 36 | ||||
-rw-r--r-- | spec/services/boards/lists/destroy_service_spec.rb | 14 | ||||
-rw-r--r-- | spec/services/boards/lists/move_service_spec.rb | 44 |
8 files changed, 75 insertions, 81 deletions
diff --git a/app/controllers/projects/boards/issues_controller.rb b/app/controllers/projects/boards/issues_controller.rb index a038639d0ac..7bc3aacb9bf 100644 --- a/app/controllers/projects/boards/issues_controller.rb +++ b/app/controllers/projects/boards/issues_controller.rb @@ -19,7 +19,7 @@ module Projects def update service = ::Boards::Issues::MoveService.new(project, current_user, move_params) - if service.execute + if service.execute(issue) head :ok else head :unprocessable_entity @@ -28,12 +28,16 @@ module Projects private + def issue + @issue ||= project.issues.visible_to_user(current_user).find_by!(iid: params[:id]) + end + def authorize_read_issue! return render_403 unless can?(current_user, :read_issue, project) end def authorize_update_issue! - return render_403 unless can?(current_user, :update_issue, project) + return render_403 unless can?(current_user, :update_issue, issue) end def filter_params diff --git a/app/controllers/projects/boards/lists_controller.rb b/app/controllers/projects/boards/lists_controller.rb index 2790642382a..7834e12f62d 100644 --- a/app/controllers/projects/boards/lists_controller.rb +++ b/app/controllers/projects/boards/lists_controller.rb @@ -3,6 +3,7 @@ module Projects class ListsController < Boards::ApplicationController before_action :authorize_admin_list!, only: [:create, :update, :destroy, :generate] before_action :authorize_read_list!, only: [:index] + before_action :load_list, only: [:update, :destroy] def index render json: serialize_as_json(project.board.lists) @@ -21,7 +22,7 @@ module Projects def update service = ::Boards::Lists::MoveService.new(project, current_user, move_params) - if service.execute + if service.execute(@list) head :ok else head :unprocessable_entity @@ -31,7 +32,7 @@ module Projects def destroy service = ::Boards::Lists::DestroyService.new(project, current_user, params) - if service.execute + if service.execute(@list) head :ok else head :unprocessable_entity @@ -58,12 +59,16 @@ module Projects return render_403 unless can?(current_user, :read_list, project) end + def load_list + @list = project.board.lists.find(params[:id]) + end + def list_params params.require(:list).permit(:label_id) end def move_params - params.require(:list).permit(:position).merge(id: params[:id]) + params.require(:list).permit(:position) end def serialize_as_json(resource) diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb index 97fe099ccb5..6c6f60e1496 100644 --- a/app/services/boards/issues/move_service.rb +++ b/app/services/boards/issues/move_service.rb @@ -1,8 +1,8 @@ module Boards module Issues class MoveService < Boards::BaseService - def execute - return false unless issue.present? + def execute(issue) + return false unless user.can?(:update_issue, issue) return false unless valid_move? update_service.execute(issue) @@ -14,10 +14,6 @@ module Boards moving_from_list.present? && moving_to_list.present? end - def issue - @issue ||= project.issues.visible_to_user(user).find_by!(iid: params[:id]) - end - def moving_from_list @moving_from_list ||= board.lists.find_by(id: params[:from_list_id]) end diff --git a/app/services/boards/lists/destroy_service.rb b/app/services/boards/lists/destroy_service.rb index 8d244fa3966..9edb596bf46 100644 --- a/app/services/boards/lists/destroy_service.rb +++ b/app/services/boards/lists/destroy_service.rb @@ -1,27 +1,23 @@ module Boards module Lists class DestroyService < Boards::BaseService - def execute + def execute(list) return false unless list.label? list.with_lock do - decrement_higher_lists - remove_list + decrement_higher_lists(list) + remove_list(list) end end private - def list - @list ||= board.lists.find(params[:id]) - end - - def decrement_higher_lists + def decrement_higher_lists(list) board.lists.label.where('position > ?', list.position) .update_all('position = position - 1') end - def remove_list + def remove_list(list) list.destroy end end diff --git a/app/services/boards/lists/move_service.rb b/app/services/boards/lists/move_service.rb index 1c91fed0ff4..2b2b5c3943a 100644 --- a/app/services/boards/lists/move_service.rb +++ b/app/services/boards/lists/move_service.rb @@ -1,35 +1,28 @@ module Boards module Lists class MoveService < Boards::BaseService - def execute + def execute(list) + @old_position = list.position + @new_position = params[:position] + return false unless list.label? return false unless valid_move? list.with_lock do reorder_intermediate_lists - update_list_position + update_list_position(list) end end private - def list - @list ||= board.lists.find(params[:id]) - end + attr_reader :old_position, :new_position def valid_move? new_position.present? && new_position != old_position && new_position >= 0 && new_position < board.lists.label.size end - def old_position - @old_position ||= list.position - end - - def new_position - @new_position ||= params[:position] - end - def reorder_intermediate_lists if old_position < new_position decrement_intermediate_lists @@ -50,7 +43,7 @@ module Boards .update_all('position = position + 1') end - def update_list_position + def update_list_position(list) list.update_attribute(:position, new_position) end end diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb index a60f5a9c202..65f2247918f 100644 --- a/spec/services/boards/issues/move_service_spec.rb +++ b/spec/services/boards/issues/move_service_spec.rb @@ -22,9 +22,9 @@ describe Boards::Issues::MoveService, services: true do context 'when moving from backlog' do it 'adds the label of the list it goes to' do issue = create(:labeled_issue, project: project, labels: [bug]) - params = { id: issue.iid, from_list_id: backlog.id, to_list_id: list1.id } + params = { from_list_id: backlog.id, to_list_id: list1.id } - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) expect(issue.reload.labels).to contain_exactly(bug, development) end @@ -33,9 +33,9 @@ describe Boards::Issues::MoveService, services: true do context 'when moving to backlog' do it 'removes all list-labels' do issue = create(:labeled_issue, project: project, labels: [bug, development, testing]) - params = { id: issue.iid, from_list_id: list1.id, to_list_id: backlog.id } + params = { from_list_id: list1.id, to_list_id: backlog.id } - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) expect(issue.reload.labels).to contain_exactly(bug) end @@ -44,9 +44,9 @@ describe Boards::Issues::MoveService, services: true do context 'when moving from backlog to done' do it 'closes the issue' do issue = create(:labeled_issue, project: project, labels: [bug]) - params = { id: issue.iid, from_list_id: backlog.id, to_list_id: done.id } + params = { from_list_id: backlog.id, to_list_id: done.id } - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) issue.reload expect(issue.labels).to contain_exactly(bug) @@ -56,16 +56,16 @@ describe Boards::Issues::MoveService, services: true do context 'when moving an issue between lists' do let(:issue) { create(:labeled_issue, project: project, labels: [bug, development]) } - let(:params) { { id: issue.iid, from_list_id: list1.id, to_list_id: list2.id } } + let(:params) { { from_list_id: list1.id, to_list_id: list2.id } } it 'delegates the label changes to Issues::UpdateService' do expect_any_instance_of(Issues::UpdateService).to receive(:execute).with(issue).once - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) end - it 'removess the label from the list it came from and adds the label of the list it goes to' do - described_class.new(project, user, params).execute + it 'removes the label from the list it came from and adds the label of the list it goes to' do + described_class.new(project, user, params).execute(issue) expect(issue.reload.labels).to contain_exactly(bug, testing) end @@ -73,16 +73,16 @@ describe Boards::Issues::MoveService, services: true do context 'when moving to done' do let(:issue) { create(:labeled_issue, project: project, labels: [bug, development, testing]) } - let(:params) { { id: issue.iid, from_list_id: list2.id, to_list_id: done.id } } + let(:params) { { from_list_id: list2.id, to_list_id: done.id } } it 'delegates the close proceedings to Issues::CloseService' do expect_any_instance_of(Issues::CloseService).to receive(:execute).with(issue).once - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) end it 'removes all list-labels and close the issue' do - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) issue.reload expect(issue.labels).to contain_exactly(bug) @@ -92,16 +92,16 @@ describe Boards::Issues::MoveService, services: true do context 'when moving from done' do let(:issue) { create(:labeled_issue, :closed, project: project, labels: [bug]) } - let(:params) { { id: issue.iid, from_list_id: done.id, to_list_id: list2.id } } + let(:params) { { from_list_id: done.id, to_list_id: list2.id } } it 'delegates the re-open proceedings to Issues::ReopenService' do expect_any_instance_of(Issues::ReopenService).to receive(:execute).with(issue).once - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) end it 'adds the label of the list it goes to and reopen the issue' do - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) issue.reload expect(issue.labels).to contain_exactly(bug, testing) @@ -112,9 +112,9 @@ describe Boards::Issues::MoveService, services: true do context 'when moving from done to backlog' do it 'reopens the issue' do issue = create(:labeled_issue, :closed, project: project, labels: [bug]) - params = { id: issue.iid, from_list_id: done.id, to_list_id: backlog.id } + params = { from_list_id: done.id, to_list_id: backlog.id } - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(issue) issue.reload expect(issue.labels).to contain_exactly(bug) diff --git a/spec/services/boards/lists/destroy_service_spec.rb b/spec/services/boards/lists/destroy_service_spec.rb index 3b887a55a97..6eff445feee 100644 --- a/spec/services/boards/lists/destroy_service_spec.rb +++ b/spec/services/boards/lists/destroy_service_spec.rb @@ -9,9 +9,9 @@ describe Boards::Lists::DestroyService, services: true do context 'when list type is label' do it 'removes list from board' do list = create(:list, board: board) - service = described_class.new(project, user, id: list.id) + service = described_class.new(project, user) - expect { service.execute }.to change(board.lists, :count).by(-1) + expect { service.execute(list) }.to change(board.lists, :count).by(-1) end it 'decrements position of higher lists' do @@ -21,7 +21,7 @@ describe Boards::Lists::DestroyService, services: true do staging = create(:list, board: board, position: 2) done = create(:done_list, board: board) - described_class.new(project, user, id: development.id).execute + described_class.new(project, user).execute(development) expect(backlog.reload.position).to be_nil expect(review.reload.position).to eq 0 @@ -32,16 +32,16 @@ describe Boards::Lists::DestroyService, services: true do it 'does not remove list from board when list type is backlog' do list = create(:backlog_list, board: board) - service = described_class.new(project, user, id: list.id) + service = described_class.new(project, user) - expect { service.execute }.not_to change(board.lists, :count) + expect { service.execute(list) }.not_to change(board.lists, :count) end it 'does not remove list from board when list type is done' do list = create(:done_list, board: board) - service = described_class.new(project, user, id: list.id) + service = described_class.new(project, user) - expect { service.execute }.not_to change(board.lists, :count) + expect { service.execute(list) }.not_to change(board.lists, :count) end end end diff --git a/spec/services/boards/lists/move_service_spec.rb b/spec/services/boards/lists/move_service_spec.rb index 11de74a96a1..3e9b7d07fc6 100644 --- a/spec/services/boards/lists/move_service_spec.rb +++ b/spec/services/boards/lists/move_service_spec.rb @@ -15,90 +15,90 @@ describe Boards::Lists::MoveService, services: true do context 'when list type is set to label' do it 'keeps position of lists when new position is nil' do - service = described_class.new(project, user, id: planning.id, position: nil) + service = described_class.new(project, user, position: nil) - service.execute + service.execute(planning) expect(current_list_positions).to eq [0, 1, 2, 3] end it 'keeps position of lists when new positon is equal to old position' do - service = described_class.new(project, user, id: planning.id, position: planning.position) + service = described_class.new(project, user, position: planning.position) - service.execute + service.execute(planning) expect(current_list_positions).to eq [0, 1, 2, 3] end it 'keeps position of lists when new positon is negative' do - service = described_class.new(project, user, id: planning.id, position: -1) + service = described_class.new(project, user, position: -1) - service.execute + service.execute(planning) expect(current_list_positions).to eq [0, 1, 2, 3] end it 'keeps position of lists when new positon is equal to number of labels lists' do - service = described_class.new(project, user, id: planning.id, position: board.lists.label.size) + service = described_class.new(project, user, position: board.lists.label.size) - service.execute + service.execute(planning) expect(current_list_positions).to eq [0, 1, 2, 3] end it 'keeps position of lists when new positon is greater than number of labels lists' do - service = described_class.new(project, user, id: planning.id, position: board.lists.label.size + 1) + service = described_class.new(project, user, position: board.lists.label.size + 1) - service.execute + service.execute(planning) expect(current_list_positions).to eq [0, 1, 2, 3] end it 'increments position of intermediate lists when new positon is equal to first position' do - service = described_class.new(project, user, id: staging.id, position: 0) + service = described_class.new(project, user, position: 0) - service.execute + service.execute(staging) expect(current_list_positions).to eq [1, 2, 3, 0] end it 'decrements position of intermediate lists when new positon is equal to last position' do - service = described_class.new(project, user, id: planning.id, position: board.lists.label.last.position) + service = described_class.new(project, user, position: board.lists.label.last.position) - service.execute + service.execute(planning) expect(current_list_positions).to eq [3, 0, 1, 2] end it 'decrements position of intermediate lists when new position is greater than old position' do - service = described_class.new(project, user, id: planning.id, position: 2) + service = described_class.new(project, user, position: 2) - service.execute + service.execute(planning) expect(current_list_positions).to eq [2, 0, 1, 3] end it 'increments position of intermediate lists when new position is lower than old position' do - service = described_class.new(project, user, id: staging.id, position: 1) + service = described_class.new(project, user, position: 1) - service.execute + service.execute(staging) expect(current_list_positions).to eq [0, 2, 3, 1] end end it 'keeps position of lists when list type is backlog' do - service = described_class.new(project, user, id: backlog.id, position: 2) + service = described_class.new(project, user, position: 2) - service.execute + service.execute(backlog) expect(current_list_positions).to eq [0, 1, 2, 3] end it 'keeps position of lists when list type is done' do - service = described_class.new(project, user, id: done.id, position: 2) + service = described_class.new(project, user, position: 2) - service.execute + service.execute(done) expect(current_list_positions).to eq [0, 1, 2, 3] end |