From f77c47a51c8fef379b2dc9473545e53a28ec3c7f Mon Sep 17 00:00:00 2001
From: Douglas Barbosa Alexandre <dbalexandre@gmail.com>
Date: Tue, 16 Aug 2016 01:07:25 -0300
Subject: [PATCH] Remove lookup inside services to keep them consistent with
other ones
---
.../projects/boards/issues_controller.rb | 8 +++-
.../projects/boards/lists_controller.rb | 11 +++--
app/services/boards/issues/move_service.rb | 8 +---
app/services/boards/lists/destroy_service.rb | 14 +++---
app/services/boards/lists/move_service.rb | 21 +++------
.../boards/issues/move_service_spec.rb | 36 +++++++--------
.../boards/lists/destroy_service_spec.rb | 14 +++---
.../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
--
GitLab