Remove lookup inside services to keep them consistent with other ones

parent affed370
No related merge requests found
......@@ -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
......
......@@ -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)
......
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
......
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
......
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
......
......@@ -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)
......
......@@ -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
......@@ -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
......
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