Merge branch '35757-move-issues-in-boards-pderichs' into 'master'

Add endpoint to move issues in boards

See merge request gitlab-org/gitlab-ce!30216

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

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 }
 

changelogs/unreleased/35757-move-issues-in-boards-pderichs.yml

+---
+title: Add endpoint to move multiple issues in boards
+merge_request: 30216
+author:
+type: added

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]

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)
 

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