Remove lookup inside services to keep them consistent with other ones

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