Merge branch '323194-epic-board-reposition' into 'master'

Reposition epics on the board using graphQL

See merge request gitlab-org/gitlab!56236

app/services/boards/base_item_move_service.rb

@@ -22,6 +22,12 @@ module Boards
         )
       end
 
+      reposition_ids = move_between_ids(params)
+      if reposition_ids
+        attrs[:move_between_ids] = reposition_ids
+        attrs.merge!(reposition_parent)
+      end
+
       attrs
     end
 
@@ -68,5 +74,13 @@ module Boards
 
       Array(label_ids).compact
     end
+
+    def move_between_ids(move_params)
+      ids = [move_params[:move_after_id], move_params[:move_before_id]]
+              .map(&:to_i)
+              .map { |m| m > 0 ? m : nil }
+
+      ids.any? ? ids : nil
+    end
   end
 end

app/services/boards/issues/move_service.rb

@@ -3,8 +3,6 @@
 module Boards
   module Issues
     class MoveService < Boards::BaseItemMoveService
-      extend ::Gitlab::Utils::Override
-
       def execute_multiple(issues)
         return execute_multiple_empty_result if issues.empty?
 
@@ -57,25 +55,8 @@ module Boards
         ::Issues::UpdateService.new(issue.project, current_user, issue_modification_params).execute(issue)
       end
 
-      override :issuable_params
-      def issuable_params(issuable)
-        attrs = super
-
-        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
-        end
-
-        attrs
-      end
-
-      def move_between_ids(move_params)
-        ids = [move_params[:move_after_id], move_params[:move_before_id]]
-                .map(&:to_i)
-                .map { |m| m > 0 ? m : nil }
-
-        ids.any? ? ids : nil
+      def reposition_parent
+        { board_group_id: board.group&.id }
       end
     end
   end

app/services/issuable_base_service.rb

@@ -424,6 +424,20 @@ class IssuableBaseService < BaseService
     associations
   end
 
+  def handle_move_between_ids(issuable_position)
+    return unless params[:move_between_ids]
+
+    after_id, before_id = params.delete(:move_between_ids)
+    positioning_scope_id = params.delete(positioning_scope_key)
+
+    issuable_before = issuable_for_positioning(before_id, positioning_scope_id)
+    issuable_after = issuable_for_positioning(after_id, positioning_scope_id)
+
+    raise ActiveRecord::RecordNotFound unless issuable_before || issuable_after
+
+    issuable_position.move_between(issuable_before, issuable_after)
+  end
+
   def has_changes?(issuable, old_labels: [], old_assignees: [], old_reviewers: [])
     valid_attrs = [:title, :description, :assignee_ids, :reviewer_ids, :milestone_id, :target_branch]
 

app/services/issues/update_service.rb

@@ -96,19 +96,15 @@ module Issues
     end
 
     def handle_move_between_ids(issue)
-      return unless params[:move_between_ids]
-
-      after_id, before_id = params.delete(:move_between_ids)
-      board_group_id = params.delete(:board_group_id)
-
-      issue_before = get_issue_if_allowed(before_id, board_group_id)
-      issue_after = get_issue_if_allowed(after_id, board_group_id)
-      raise ActiveRecord::RecordNotFound unless issue_before || issue_after
+      super
 
-      issue.move_between(issue_before, issue_after)
       rebalance_if_needed(issue)
     end
 
+    def positioning_scope_key
+      :board_group_id
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
     def change_issue_duplicate(issue)
       canonical_issue_id = params.delete(:canonical_issue_id)
@@ -185,7 +181,7 @@ module Issues
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def get_issue_if_allowed(id, board_group_id = nil)
+    def issuable_for_positioning(id, board_group_id = nil)
       return unless id
 
       issue =

ee/app/services/boards/epics/move_service.rb

@@ -12,6 +12,10 @@ module Boards
       def board
         @board ||= parent.epic_boards.find(params[:board_id])
       end
+
+      def reposition_parent
+        { board_id: board.id }
+      end
     end
   end
 end

ee/app/services/epics/update_service.rb

@@ -10,6 +10,8 @@ module Epics
     ].freeze
 
     def execute(epic)
+      reposition_on_board(epic)
+
       # start_date and end_date columns are no longer writable by users because those
       # are composite fields managed by the system.
       params.extract!(:start_date, :end_date)
@@ -64,6 +66,27 @@ module Epics
       end
     end
 
+    def reposition_on_board(epic)
+      return unless params[:move_between_ids]
+      return unless params[positioning_scope_key]
+
+      epic_board_position = issuable_for_positioning(epic.id, params[positioning_scope_key])
+
+      handle_move_between_ids(epic_board_position)
+
+      epic_board_position.save
+    end
+
+    def issuable_for_positioning(id, board_id)
+      return unless id
+
+      Boards::EpicBoardPosition.find_by_epic_id_and_epic_board_id(id, board_id)
+    end
+
+    def positioning_scope_key
+      :board_id
+    end
+
     def saved_change_to_epic_dates?(epic)
       (epic.saved_changes.keys.map(&:to_sym) & EPIC_DATE_FIELDS).present?
     end

ee/spec/services/boards/epics/move_service_spec.rb

@@ -41,52 +41,84 @@ RSpec.describe Boards::Epics::MoveService do
         group.add_maintainer(user)
       end
 
-      context 'when moving the epic from backlog' do
-        context 'to a labeled list' do
-          let(:to_list) { list1 }
+      context 'when repositioning epics' do
+        let_it_be(:epic1) { create(:epic, group: group) }
+        let_it_be(:epic2) { create(:epic, group: group) }
 
-          it 'keeps the epic opened and adds the labels' do
-            expect { subject }.not_to change { epic.state }
+        let!(:epic_position) { create(:epic_board_position, epic: epic, epic_board: board, relative_position: 10) }
+        let!(:epic1_position) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 20) }
+        let!(:epic2_position) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 30) }
 
-            expect(epic.labels).to eq([development])
-          end
-        end
+        let(:params) { { board_id: board.id, move_before_id: epic1.id } }
 
-        context 'to the closed list' do
-          it 'closes the epic' do
-            expect { subject }.to change { epic.state }.from('opened').to('closed')
+        context 'with valid params' do
+          it 'moves the epic' do
+            subject
+
+            expect(epic_position.reload.relative_position).to be > epic1_position.relative_position
+            expect(epic_position.relative_position).to be < epic2_position.relative_position
           end
         end
 
-        context 'to the closed list in another board' do
-          let(:to_list) { other_board_list }
+        context 'with invalid params' do
+          context 'with board from another group' do
+            let(:board) { create(:epic_board) }
 
-          it 'does not close the epic' do
-            expect { subject }.not_to change { epic.state }
+            it 'raises an error' do
+              expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+            end
           end
         end
       end
 
-      context 'when moving the epic from a labeled list' do
-        before do
-          epic.labels = [development]
-        end
+      context 'when moving an epic between lists' do
+        context 'when moving the epic from backlog' do
+          context 'to a labeled list' do
+            let(:to_list) { list1 }
+
+            it 'keeps the epic opened and adds the labels' do
+              expect { subject }.not_to change { epic.state }
+
+              expect(epic.labels).to eq([development])
+            end
+          end
 
-        let(:from_list) { list1 }
+          context 'to the closed list' do
+            it 'closes the epic' do
+              expect { subject }.to change { epic.state }.from('opened').to('closed')
+            end
+          end
 
-        context 'to another labeled list' do
-          let(:to_list) { list2 }
+          context 'to the closed list in another board' do
+            let(:to_list) { other_board_list }
 
-          it 'changes the labels' do
-            expect { subject }.to change { epic.reload.labels }.from([development]).to([testing])
+            it 'does not close the epic' do
+              expect { subject }.not_to change { epic.state }
+            end
           end
         end
 
-        context 'to the closed list' do
-          let(:to_list) { closed }
+        context 'when moving the epic from a labeled list' do
+          before do
+            epic.labels = [development]
+          end
+
+          let(:from_list) { list1 }
+
+          context 'to another labeled list' do
+            let(:to_list) { list2 }
+
+            it 'changes the labels' do
+              expect { subject }.to change { epic.reload.labels }.from([development]).to([testing])
+            end
+          end
+
+          context 'to the closed list' do
+            let(:to_list) { closed }
 
-          it 'closes the epic' do
-            expect { subject }.to change { epic.state }.from('opened').to('closed')
+            it 'closes the epic' do
+              expect { subject }.to change { epic.state }.from('opened').to('closed')
+            end
           end
         end
       end

ee/spec/services/epics/update_service_spec.rb

@@ -84,6 +84,46 @@ RSpec.describe Epics::UpdateService do
       end
     end
 
+    context 'when repositioning an epic on a board' do
+      let(:epic1) { create(:epic, group: group) }
+      let(:epic2) { create(:epic, group: group) }
+
+      let!(:epic_position) { create(:epic_board_position, epic: epic, epic_board: board, relative_position: 10) }
+      let!(:epic1_position) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 20) }
+      let!(:epic2_position) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 30) }
+
+      let(:board) { create(:epic_board, group: group) }
+
+      context 'when moving beetween 2 epics on the board' do
+        it 'moves the epic correctly' do
+          update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id)
+
+          expect(epic_position.reload.relative_position)
+            .to be_between(epic1_position.relative_position, epic2_position.relative_position)
+        end
+      end
+
+      context 'when moving the epic to the end' do
+        it 'moves the epic correctly' do
+          update_epic(move_between_ids: [nil, epic2.id], board_id: board.id)
+
+          expect(epic_position.reload.relative_position).to be > epic2_position.relative_position
+        end
+      end
+
+      context 'when moving the epic to the beginning' do
+        before do
+          epic_position.update_column(:relative_position, 25)
+        end
+
+        it 'moves the epic correctly' do
+          update_epic(move_between_ids: [epic1.id, nil], board_id: board.id)
+
+          expect(epic_position.reload.relative_position).to be < epic1_position.relative_position
+        end
+      end
+    end
+
     context 'after_save callback to store_mentions' do
       let(:user2) { create(:user) }
       let(:epic) { create(:epic, group: group, description: "simple description") }