Add endpoint to move multiple issues35757-move-issues-in-boards-pderichs

Add specs for new endpoint to move multiple issues.
Add changelog entry

Just check the first issue for the ability to move / update

Add specs for exceeding limits and malformed requests

Changed name of shared examples

Change title of changelog entry

Use %i instead of %w

Check permission to update issue on project instead of board

Use admin_issue permission to check for issue move ability

Changed variable name to avoid shadow issue_params method

Rename route to bulk_move

Change route definition

Check permissions for each issue

Combine methods for parameters permit check

Remove extra context

Change description of context

Check param for type Array

Add unit tests to MoveService

Use before_action for permission check

Use set instead of let!

Use let's instead of set

6 files changed, 354 insertions, 11 deletions

diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb
index 0dd7500623d..353a9806fd1 100644
--- a/app/controllers/boards/issues_controller.rb
+++ b/app/controllers/boards/issues_controller.rb
@@ -2,16 +2,24 @@
 
 module Boards
   class IssuesController < Boards::ApplicationController
+    # This is the maximum amount of issues which can be moved by one request to
+    # bulk_move for now. This is temporary and might be removed in future by
+    # introducing an alternative (async?) approach.
+    # (related: https://gitlab.com/groups/gitlab-org/-/epics/382)
+    MAX_MOVE_ISSUES_COUNT = 50
+
     include BoardsResponses
     include ControllerWithCrossProjectAccessCheck
 
     requires_cross_project_access if: -> { board&.group_board? }
 
-    before_action :whitelist_query_limiting, only: [:index, :update]
+    before_action :whitelist_query_limiting, only: [:index, :update, :bulk_move]
     before_action :authorize_read_issue, only: [:index]
     before_action :authorize_create_issue, only: [:create]
     before_action :authorize_update_issue, only: [:update]
     skip_before_action :authenticate_user!, only: [:index]
+    before_action :validate_id_list, only: [:bulk_move]
+    before_action :can_move_issues?, only: [:bulk_move]
 
     # rubocop: disable CodeReuse/ActiveRecord
     def index
@@ -46,6 +54,17 @@ module Boards
       end
     end
 
+    def bulk_move
+      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
+    end
+
     def update
       service = Boards::Issues::MoveService.new(board_parent, current_user, move_params)
 
@@ -58,6 +77,10 @@ module Boards
 
     private
 
+    def can_move_issues?
+      head(:forbidden) unless can?(current_user, :admin_issue, board)
+    end
+
     def render_issues(issues, metadata)
       data = { issues: serialize_as_json(issues) }
       data.merge!(metadata)
@@ -90,8 +113,9 @@ module Boards
                    end
     end
 
-    def move_params
-      params.permit(:board_id, :id, :from_list_id, :to_list_id, :move_before_id, :move_after_id)
+    def move_params(multiple = false)
+      id_param = multiple ? :ids : :id
+      params.permit(id_param, :board_id, :from_list_id, :to_list_id, :move_before_id, :move_after_id)
     end
 
     def issue_params
@@ -112,5 +136,10 @@ module Boards
       # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42439
       Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42428')
     end
+
+    def validate_id_list
+      head(:bad_request) unless params[:ids].is_a?(Array)
+      head(:unprocessable_entity) if params[:ids].size > MAX_MOVE_ISSUES_COUNT
+    end
   end
 end
diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb
index e27d34dbcab..755d723b9a0 100644
--- a/app/services/boards/issues/move_service.rb
+++ b/app/services/boards/issues/move_service.rb
@@ -4,14 +4,37 @@ module Boards
   module Issues
     class MoveService < Boards::BaseService
       def execute(issue)
-        return false unless can?(current_user, :update_issue, issue)
-        return false if issue_params(issue).empty?
+        issue_modification_params = issue_params(issue)
+        return false if issue_modification_params.empty?
 
-        update(issue)
+        move_single_issue(issue, issue_modification_params)
+      end
+
+      def execute_multiple(issues)
+        return false if issues.empty?
+
+        last_inserted_issue_id = nil
+        issues.map do |issue|
+          issue_modification_params = issue_params(issue)
+          next 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 })
+          end
+
+          last_inserted_issue_id = issue.id
+          move_single_issue(issue, issue_modification_params)
+        end.all?
       end
 
       private
 
+      def move_single_issue(issue, issue_modification_params)
+        return false unless can?(current_user, :update_issue, issue)
+
+        update(issue, issue_modification_params)
+      end
+
       def board
         @board ||= parent.boards.find(params[:board_id])
       end
@@ -33,8 +56,8 @@ module Boards
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      def update(issue)
-        ::Issues::UpdateService.new(issue.project, current_user, issue_params(issue)).execute(issue)
+      def update(issue, issue_modification_params)
+        ::Issues::UpdateService.new(issue.project, current_user, issue_modification_params).execute(issue)
       end
 
       def issue_params(issue)
@@ -48,6 +71,7 @@ module Boards
           )
         end
 
+        move_between_ids = move_between_ids(params)
         if move_between_ids
           attrs[:move_between_ids] = move_between_ids
           attrs[:board_group_id] = board.group&.id
@@ -78,8 +102,8 @@ module Boards
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      def move_between_ids
-        ids = [params[:move_after_id], params[:move_before_id]]
+      def move_between_ids(move_params)
+        ids = [move_params[:move_after_id], move_params[:move_before_id]]
                 .map(&:to_i)
                 .map { |m| m.positive? ? m : nil }
 
diff --git a/changelogs/unreleased/35757-move-issues-in-boards-pderichs.yml b/changelogs/unreleased/35757-move-issues-in-boards-pderichs.yml
new file mode 100644
index 00000000000..ae50350f70d
--- /dev/null
+++ b/changelogs/unreleased/35757-move-issues-in-boards-pderichs.yml
@@ -0,0 +1,5 @@
+---
+title: Add endpoint to move multiple issues in boards
+merge_request: 30216
+author:
+type: added
diff --git a/config/routes.rb b/config/routes.rb
index a42fc037227..ae14b421028 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -82,7 +82,11 @@ Rails.application.routes.draw do
         resources :issues, only: [:index, :create, :update]
       end
 
-      resources :issues, module: :boards, only: [:index, :update]
+      resources :issues, module: :boards, only: [:index, :update] do
+        collection do
+          put :bulk_move, format: :json
+        end
+      end
 
       Gitlab.ee do
         resources :users, module: :boards, only: [:index]
diff --git a/spec/controllers/boards/issues_controller_spec.rb b/spec/controllers/boards/issues_controller_spec.rb
index c84bb913cad..6cad060d888 100644
--- a/spec/controllers/boards/issues_controller_spec.rb
+++ b/spec/controllers/boards/issues_controller_spec.rb
@@ -164,6 +164,201 @@ describe Boards::IssuesController do
       end
     end
 
+    describe 'PUT move_multiple' 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 }
+      let(:guest) { create(:group_member, :guest, user: create(:user), group: group ).user }
+      let(:project) { create(:project, group: group) }
+      let(:group) { create(:group) }
+      let(:board) { create(:board, project: project) }
+      let(:list1) { create(:list, board: board, label: todo, position: 0) }
+      let(:list2) { create(:list, board: board, label: development, position: 1) }
+      let(:issue1) { create(:labeled_issue, project: project, labels: [todo], author: user, relative_position: 10) }
+      let(:issue2) { create(:labeled_issue, project: project, labels: [todo], author: user, relative_position: 20) }
+      let(:issue3) { create(:labeled_issue, project: project, labels: [todo], author: user, relative_position: 30) }
+      let(:issue4) { create(:labeled_issue, project: project, labels: [development], author: user, relative_position: 100) }
+
+      let(:move_params) do
+        {
+          board_id: board.id,
+          ids: [issue1.id, issue2.id, issue3.id],
+          from_list_id: list1.id,
+          to_list_id: list2.id,
+          move_before_id: issue4.id,
+          move_after_id: nil
+        }
+      end
+
+      before do
+        project.add_maintainer(user)
+        project.add_guest(guest)
+      end
+
+      shared_examples 'move issues endpoint provider' do
+        before do
+          sign_in(signed_in_user)
+        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_response['issues']
+          expect(responded_issues.length).to eq expected_issue_count
+
+          ids_in_order = responded_issues.pluck('id')
+          expect(ids_in_order).to eq(expected_issue_ids_in_order)
+        end
+      end
+
+      context 'when items are moved to another list' do
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { user }
+          let(:move_issues_params) { move_params }
+          let(:requesting_user) { user }
+          let(:expected_status) { 200 }
+          let(:expected_issue_count) { 4 }
+          let(:expected_issue_ids_in_order) { [issue4.id, issue1.id, issue2.id, issue3.id] }
+        end
+      end
+
+      context 'when moving just one issue' do
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { user }
+          let(:move_issues_params) do
+            move_params.dup.tap do |hash|
+              hash[:ids] = [issue2.id]
+            end
+          end
+          let(:requesting_user) { user }
+          let(:expected_status) { 200 }
+          let(:expected_issue_count) { 2 }
+          let(:expected_issue_ids_in_order) { [issue4.id, issue2.id] }
+        end
+      end
+
+      context 'when user is not allowed to move issue' do
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { guest }
+          let(:move_issues_params) do
+            move_params.dup.tap do |hash|
+              hash[:ids] = [issue2.id]
+            end
+          end
+          let(:requesting_user) { user }
+          let(:expected_status) { 403 }
+          let(:expected_issue_count) { 1 }
+          let(:expected_issue_ids_in_order) { [issue4.id] }
+        end
+      end
+
+      context 'when issues should be moved visually above existing issue in list' do
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { user }
+          let(:move_issues_params) do
+            move_params.dup.tap do |hash|
+              hash[:move_after_id] = issue4.id
+              hash[:move_before_id] = nil
+            end
+          end
+          let(:requesting_user) { user }
+          let(:expected_status) { 200 }
+          let(:expected_issue_count) { 4 }
+          let(:expected_issue_ids_in_order) { [issue1.id, issue2.id, issue3.id, issue4.id] }
+        end
+      end
+
+      context 'when destination list is empty' do
+        before do
+          # Remove issue from list
+          issue4.labels -= [development]
+          issue4.save!
+        end
+
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { user }
+          let(:move_issues_params) do
+            move_params.dup.tap do |hash|
+              hash[:move_before_id] = nil
+            end
+          end
+          let(:requesting_user) { user }
+          let(:expected_status) { 200 }
+          let(:expected_issue_count) { 3 }
+          let(:expected_issue_ids_in_order) { [issue1.id, issue2.id, issue3.id] }
+        end
+      end
+
+      context 'when no position arguments are given' do
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { user }
+          let(:move_issues_params) do
+            move_params.dup.tap do |hash|
+              hash[:move_before_id] = nil
+            end
+          end
+          let(:requesting_user) { user }
+          let(:expected_status) { 200 }
+          let(:expected_issue_count) { 4 }
+          let(:expected_issue_ids_in_order) { [issue1.id, issue2.id, issue3.id, issue4.id] }
+        end
+      end
+
+      context 'when move_before_id and move_after_id are given' do
+        let(:issue5) { create(:labeled_issue, project: project, labels: [development], author: user, relative_position: 90) }
+
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { user }
+          let(:move_issues_params) do
+            move_params.dup.tap do |hash|
+              hash[:move_before_id] = issue5.id
+              hash[:move_after_id] = issue4.id
+            end
+          end
+          let(:requesting_user) { user }
+          let(:expected_status) { 200 }
+          let(:expected_issue_count) { 5 }
+          let(:expected_issue_ids_in_order) { [issue5.id, issue1.id, issue2.id, issue3.id, issue4.id] }
+        end
+      end
+
+      context 'when request contains too many issues' do
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { user }
+          let(:move_issues_params) do
+            move_params.dup.tap do |hash|
+              hash[:ids] = (0..51).to_a
+            end
+          end
+          let(:requesting_user) { user }
+          let(:expected_status) { 422 }
+          let(:expected_issue_count) { 1 }
+          let(:expected_issue_ids_in_order) { [issue4.id] }
+        end
+      end
+
+      context 'when request is malformed' do
+        it_behaves_like 'move issues endpoint provider' do
+          let(:signed_in_user) { user }
+          let(:move_issues_params) do
+            move_params.dup.tap do |hash|
+              hash[:ids] = 'foobar'
+            end
+          end
+          let(:requesting_user) { user }
+          let(:expected_status) { 400 }
+          let(:expected_issue_count) { 1 }
+          let(:expected_issue_ids_in_order) { [issue4.id] }
+        end
+      end
+    end
+
     def list_issues(user:, board:, list: nil)
       sign_in(user)
 
diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb
index 16e2a2fba6b..1bfb5602df2 100644
--- a/spec/services/boards/issues/move_service_spec.rb
+++ b/spec/services/boards/issues/move_service_spec.rb
@@ -52,5 +52,91 @@ describe Boards::Issues::MoveService do
 
       it_behaves_like 'issues move service', true
     end
+
+    describe '#execute_multiple' do
+      set(:group)  { create(:group) }
+      set(:user)   { create(:user) }
+      set(:project) { create(:project, namespace: group) }
+      set(:board1) { create(:board, group: group) }
+      set(:development) { create(:group_label, group: group, name: 'Development') }
+      set(:testing) { create(:group_label, group: group, name: 'Testing') }
+      set(:list1) { create(:list, board: board1, label: development, position: 0) }
+      set(:list2) { create(:list, board: board1, label: testing, position: 1) }
+      let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list2.id } }
+
+      before 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)
+      end
+
+      context 'moving multiple issues' do
+        let(:issue1) { create(:labeled_issue, project: project, labels: [development]) }
+        let(:issue2) { create(:labeled_issue, project: project, labels: [development]) }
+
+        it 'moves multiple issues from one list to another' do
+          expect(described_class.new(group, user, params).execute_multiple([issue1, issue2])).to be_truthy
+
+          expect(issue1.labels).to eq([testing])
+          expect(issue2.labels).to eq([testing])
+        end
+      end
+
+      context 'moving a single issue' do
+        let(:issue1) { create(:labeled_issue, project: project, labels: [development]) }
+
+        it 'moves one issue' do
+          expect(described_class.new(group, user, params).execute_multiple([issue1])).to be_truthy
+
+          expect(issue1.labels).to eq([testing])
+        end
+      end
+
+      context 'moving issues visually after an existing issue' do
+        let(:existing_issue) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) }
+        let(:issue1) { create(:labeled_issue, project: project, labels: [development]) }
+        let(:issue2) { create(:labeled_issue, project: project, labels: [development]) }
+
+        let(:move_params) do
+          params.dup.tap do |hash|
+            hash[:move_before_id] = existing_issue.id
+          end
+        end
+
+        it 'moves one issue' do
+          expect(described_class.new(group, user, move_params).execute_multiple([issue1, issue2])).to be_truthy
+
+          expect(issue1.labels).to eq([testing])
+          expect(issue2.labels).to eq([testing])
+
+          expect(issue1.relative_position > existing_issue.relative_position).to eq(true)
+          expect(issue2.relative_position > issue1.relative_position).to eq(true)
+        end
+      end
+
+      context 'moving issues visually before an existing issue' do
+        let(:existing_issue) { create(:labeled_issue, project: project, labels: [testing], relative_position: 10) }
+        let(:issue1) { create(:labeled_issue, project: project, labels: [development]) }
+        let(:issue2) { create(:labeled_issue, project: project, labels: [development]) }
+
+        let(:move_params) do
+          params.dup.tap do |hash|
+            hash[:move_after_id] = existing_issue.id
+          end
+        end
+
+        it 'moves one issue' do
+          expect(described_class.new(group, user, move_params).execute_multiple([issue1, issue2])).to be_truthy
+
+          expect(issue1.labels).to eq([testing])
+          expect(issue2.labels).to eq([testing])
+
+          expect(issue2.relative_position < existing_issue.relative_position).to eq(true)
+          expect(issue1.relative_position < issue2.relative_position).to eq(true)
+        end
+      end
+    end
   end
 end