Merge branch '325794-positioning-iprovements' into 'master'

Add graphql mutation for repositioning epics on epic boards

See merge request gitlab-org/gitlab!59379

app/services/boards/base_item_move_service.rb

@@ -23,14 +23,15 @@ module Boards
       end
 
       reposition_ids = move_between_ids(params)
-      if reposition_ids
-        attrs[:move_between_ids] = reposition_ids
-        attrs.merge!(reposition_parent)
-      end
+      attrs.merge!(reposition_params(reposition_ids)) if reposition_ids
 
       attrs
     end
 
+    def reposition_params(reposition_ids)
+      reposition_parent.merge(move_between_ids: reposition_ids)
+    end
+
     def move_single_issuable(issuable, issuable_modification_params)
       ability_name = :"admin_#{issuable.to_ability_name}"
       return unless can?(current_user, ability_name, issuable)

doc/api/graphql/reference/index.md

@@ -2105,14 +2105,17 @@ Input type: `EpicMoveListInput`
 | <a id="mutationepicmovelistboardid"></a>`boardId` | [`BoardsEpicBoardID!`](#boardsepicboardid) | Global ID of the board that the epic is in. |
 | <a id="mutationepicmovelistclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
 | <a id="mutationepicmovelistepicid"></a>`epicId` | [`EpicID!`](#epicid) | ID of the epic to mutate. |
-| <a id="mutationepicmovelistfromlistid"></a>`fromListId` | [`BoardsEpicListID!`](#boardsepiclistid) | ID of the board list that the epic will be moved from. |
-| <a id="mutationepicmovelisttolistid"></a>`toListId` | [`BoardsEpicListID!`](#boardsepiclistid) | ID of the board list that the epic will be moved to. |
+| <a id="mutationepicmovelistfromlistid"></a>`fromListId` | [`BoardsEpicListID`](#boardsepiclistid) | ID of the board list that the epic will be moved from. Required if moving between lists. |
+| <a id="mutationepicmovelistmoveafterid"></a>`moveAfterId` | [`EpicID`](#epicid) | ID of epic that should be placed after the current epic. |
+| <a id="mutationepicmovelistmovebeforeid"></a>`moveBeforeId` | [`EpicID`](#epicid) | ID of epic that should be placed before the current epic. |
+| <a id="mutationepicmovelisttolistid"></a>`toListId` | [`BoardsEpicListID!`](#boardsepiclistid) | ID of the list the epic will be in after mutation. |
 
 #### Fields
 
 | Name | Type | Description |
 | ---- | ---- | ----------- |
 | <a id="mutationepicmovelistclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationepicmovelistepic"></a>`epic` | [`Epic`](#epic) | The epic after mutation. |
 | <a id="mutationepicmovelisterrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
 
 ### `Mutation.epicSetSubscription`

ee/app/graphql/mutations/boards/epic_boards/epic_move_list.rb

@@ -17,12 +17,34 @@ module Mutations
                   description: 'ID of the epic to mutate.'
 
         argument :from_list_id, ::Types::GlobalIDType[::Boards::EpicList],
-                  required: true,
-                  description: 'ID of the board list that the epic will be moved from.'
+                  required: false,
+                  description: 'ID of the board list that the epic will be moved from. Required if moving between lists.'
 
         argument :to_list_id, ::Types::GlobalIDType[::Boards::EpicList],
                   required: true,
-                  description: 'ID of the board list that the epic will be moved to.'
+                  description: 'ID of the list the epic will be in after mutation.'
+
+        argument :move_before_id, ::Types::GlobalIDType[::Epic],
+                 required: false,
+                 description: 'ID of epic that should be placed before the current epic.'
+
+        argument :move_after_id, ::Types::GlobalIDType[::Epic],
+                 required: false,
+                 description: 'ID of epic that should be placed after the current epic.'
+
+        field :epic,
+            Types::EpicType,
+            null: true,
+            description: 'The epic after mutation.'
+
+        def ready?(**args)
+          if args.slice(:from_list_id, :move_after_id, :move_before_id).empty?
+            raise Gitlab::Graphql::Errors::ArgumentError,
+                  'One of the parameters fromListId, afterId, beforeId is required together with the toListId parameter.'
+          end
+
+          super
+        end
 
         def resolve(**args)
           board = authorized_find!(id: args[:board_id])
@@ -54,8 +76,10 @@ module Mutations
 
         def move_list_arguments(args)
           {
-            from_list_id: args[:from_list_id].find&.id,
-            to_list_id: args[:to_list_id].find&.id
+            from_list_id: args[:from_list_id]&.model_id,
+            to_list_id: args[:to_list_id]&.model_id,
+            move_after_id: args[:move_after_id]&.model_id,
+            move_before_id: args[:move_before_id]&.model_id
           }
         end
       end

ee/app/models/boards/epic_board_position.rb

@@ -3,6 +3,7 @@
 module Boards
   class EpicBoardPosition < ApplicationRecord
     include RelativePositioning
+    include BulkInsertSafe
 
     belongs_to :epic_board, optional: false, inverse_of: :epic_board_positions
     belongs_to :epic, optional: false
@@ -14,12 +15,22 @@ module Boards
       reorder('relative_position ASC', 'id DESC')
     end
 
-    def self.relative_positioning_query_base(position)
-      where(epic_board_id: position.epic_board_id)
-    end
+    class << self
+      def relative_positioning_query_base(position)
+        where(epic_board_id: position.epic_board_id)
+      end
+
+      def relative_positioning_parent_column
+        :epic_board_id
+      end
+
+      def last_for_board_id(board_id)
+        where(epic_board_id: board_id).where.not(relative_position: nil).order(relative_position: :desc).first
+      end
 
-    def self.relative_positioning_parent_column
-      :epic_board_id
+      def bulk_upsert(positions)
+        bulk_upsert!(positions, unique_by: %i[epic_board_id epic_id], validate: false)
+      end
     end
   end
 end

ee/app/models/ee/epic.rb

@@ -74,6 +74,7 @@ module EE
       scope :in_issues, -> (issues) { joins(:epic_issues).where(epic_issues: { issue_id: issues }).distinct }
       scope :has_parent, -> { where.not(parent_id: nil) }
       scope :iid_starts_with, -> (query) { where("CAST(iid AS VARCHAR) LIKE ?", "#{sanitize_sql_like(query)}%") }
+      scope :from_id, -> (epic_id) { where('epics.id >= ?', epic_id) }
 
       scope :with_web_entity_associations, -> { preload(:author, group: [:ip_restrictions, :route]) }
 
@@ -117,12 +118,25 @@ module EE
         reorder('relative_position ASC', 'id DESC')
       end
 
+      scope :join_board_position, ->(board_id) do
+        epics = ::Epic.arel_table
+        positions = ::Boards::EpicBoardPosition.arel_table
+
+        epic_positions = epics.join(positions, Arel::Nodes::OuterJoin)
+          .on(epics[:id].eq(positions[:epic_id]).and(positions[:epic_board_id].eq(board_id)))
+
+        joins(epic_positions.join_sources)
+      end
+
       scope :order_relative_position_on_board, ->(board_id) do
-        left_joins(:epic_board_positions)
-          .where(boards_epic_board_positions: { epic_board_id: [nil, board_id] })
+        join_board_position(board_id)
           .reorder(::Gitlab::Database.nulls_last_order('boards_epic_board_positions.relative_position', 'ASC'), 'epics.id DESC')
       end
 
+      scope :without_board_position, ->(board_id) do
+        where(boards_epic_board_positions: { relative_position: nil })
+      end
+
       scope :with_api_entity_associations, -> { preload(:author, :labels, group: :route) }
       scope :start_date_inherited, -> { where(start_date_is_fixed: [nil, false]) }
       scope :due_date_inherited, -> { where(due_date_is_fixed: [nil, false]) }

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

@@ -9,6 +9,12 @@ module Boards
         EpicsFinder.new(current_user, filter_params.merge(group_id: parent.id))
       end
 
+      def filter(items)
+        return super unless params[:from_id].present?
+
+        super.from_id(params[:from_id])
+      end
+
       def board
         @board ||= parent.epic_boards.find(params[:board_id])
       end

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

@@ -20,6 +20,11 @@ module Boards
         ::Label.ids_on_epic_board(board.id)
       end
 
+      override :reposition_params
+      def reposition_params(reposition_ids)
+        super.merge(list_id: params[:to_list_id], board_group: parent)
+      end
+
       def reposition_parent
         { board_id: board.id }
       end

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

+# frozen_string_literal: true
+
+module Boards
+  module Epics
+    class PositionCreateService < Boards::BaseService
+      include Gitlab::Utils::StrongMemoize
+
+      LIMIT = 100
+
+      def execute
+        validate_params!
+        time = DateTime.current
+
+        positions = epics_on_board_list.map.with_index(1) do |list_epic, index|
+          Boards::EpicBoardPosition.new(
+            epic_id: list_epic.id,
+            epic_board_id: board_id,
+            relative_position: start_position * index,
+            created_at: time,
+            updated_at: time
+          )
+        end
+
+        return if positions.empty?
+
+        Boards::EpicBoardPosition.bulk_upsert(positions)
+      end
+
+      private
+
+      def validate_params!
+        raise ArgumentError, 'board_id param is missing' if params[:board_id].blank?
+        raise ArgumentError, 'list_id param is missing' if params[:list_id].blank?
+      end
+
+      def start_position
+        strong_memoize(:start_position) do
+          last_board_position = Boards::EpicBoardPosition.last_for_board_id(board_id)
+          base = last_board_position&.relative_position || Boards::EpicBoardPosition::START_POSITION
+          base + Boards::EpicBoardPosition::IDEAL_DISTANCE
+        end
+      end
+
+      def epics_on_board_list
+        # the positions will be created for all epics in the board list
+        # which don't have position set yet and which appear in the list
+        # before the epic being positioned. Epics w/o position are ordered
+        # by ID in descending order so we need to set position for epics with
+        # id >= from_id
+        list_params = { board_id: board_id, id: list_id, from_id: params[:from_id] }
+
+        Boards::Epics::ListService.new(parent, current_user, list_params).execute
+          .without_board_position(board_id)
+          .select(:id)
+          .limit(LIMIT)
+      end
+
+      def board_id
+        @board_id ||= params.delete(:board_id)
+      end
+
+      def list_id
+        @list_id ||= params.delete(:list_id)
+      end
+    end
+  end
+end

ee/app/services/epics/update_service.rb

@@ -105,19 +105,48 @@ module Epics
 
     def reposition_on_board(epic)
       return unless params[:move_between_ids]
-      return unless params[positioning_scope_key]
+      return unless epic_board_id
 
-      epic_board_position = issuable_for_positioning(epic.id, params[positioning_scope_key])
+      fill_missing_positions_before
 
+      epic_board_position = issuable_for_positioning(epic.id, epic_board_id, create_missing: true)
       handle_move_between_ids(epic_board_position)
 
-      epic_board_position.save
+      epic_board_position.save!
     end
 
-    def issuable_for_positioning(id, board_id)
+    # we want to create missing only for the epic being moved
+    # other records are handled by PositionCreateService
+    def issuable_for_positioning(id, board_id, create_missing: false)
       return unless id
 
-      Boards::EpicBoardPosition.find_by_epic_id_and_epic_board_id(id, board_id)
+      position = Boards::EpicBoardPosition.find_by_epic_id_and_epic_board_id(id, board_id)
+
+      return position if position
+
+      Boards::EpicBoardPosition.create!(epic_id: id, epic_board_id: board_id) if create_missing
+    end
+
+    def fill_missing_positions_before
+      before_id = params[:move_between_ids].compact.max
+      list_id = params.delete(:list_id)
+      board_group = params.delete(:board_group)
+
+      return unless before_id
+      # if position for the epic above exists we don't need to create positioning records
+      return if issuable_for_positioning(before_id, epic_board_id)
+
+      service_params = {
+        board_id: epic_board_id,
+        list_id: list_id, # we need to have positions only for the current list
+        from_id: before_id # we need to have positions only for the epics above
+      }
+
+      Boards::Epics::PositionCreateService.new(board_group, current_user, service_params).execute
+    end
+
+    def epic_board_id
+      params[positioning_scope_key]
     end
 
     def positioning_scope_key

ee/spec/graphql/mutations/boards/epic_boards/epic_move_list_spec.rb

@@ -14,57 +14,50 @@ RSpec.describe ::Mutations::Boards::EpicBoards::EpicMoveList do
   let_it_be(:backlog) { create(:epic_list, epic_board: board, list_type: :backlog) }
   let_it_be(:labeled_list) { create(:epic_list, epic_board: board, label: development) }
 
-  let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
+  let(:current_ctx) { { current_user: current_user } }
   let(:params) do
     {
       board_id: board.to_global_id,
-      epic_id: epic.to_global_id,
+      epic_id: epic.to_global_id
+    }
+  end
+
+  let(:move_params) do
+    {
       from_list_id: backlog.to_global_id,
       to_list_id: labeled_list.to_global_id
     }
   end
 
-  subject { mutation.resolve(**params) }
+  subject do
+    sync(resolve(described_class, args: params.merge(move_params), ctx: current_ctx))
+  end
 
   context 'arguments' do
     subject { described_class }
 
-    it { is_expected.to have_graphql_arguments(:boardId, :epicId, :fromListId, :toListId) }
+    it { is_expected.to have_graphql_arguments(:boardId, :epicId, :fromListId, :toListId, :moveBeforeId, :moveAfterId) }
   end
 
   describe '#resolve' do
-    context 'when epic_boards are disabled' do
-      before do
-        stub_licensed_features(epics: true)
-        stub_feature_flags(epic_boards: false)
-        group.add_developer(current_user)
-      end
+    before do
+      stub_licensed_features(epics: true)
+      stub_feature_flags(epic_boards: true)
+    end
 
-      it 'raises resource not available error' do
-        expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+    context 'when user does not have permissions' do
+      it 'does not allow the move' do
+        expect { subject }.to raise_error
       end
     end
 
-    context 'when epic_boards are enabled' do
+    context 'when everything is ok' do
       before do
-        stub_licensed_features(epics: true)
-        stub_feature_flags(epic_boards: true)
-      end
-
-      context 'when user does not have permissions to admin the board' do
-        it 'raises resource not available error' do
-          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
-        end
+        group.add_developer(current_user)
       end
 
-      context 'when user has permissions to admin the board' do
-        before do
-          group.add_developer(current_user)
-        end
-
-        it 'moves the epic to another list' do
-          expect { subject }.to change { epic.reload.labels }.from([]).to([development])
-        end
+      it 'moves the epic to another list' do
+        expect { subject }.to change { epic.reload.labels }.from([]).to([development])
       end
     end
   end

ee/spec/models/boards/epic_board_position_spec.rb

@@ -42,6 +42,17 @@ RSpec.describe Boards::EpicBoardPosition do
     end
   end
 
+  describe '.last_for_board_id' do
+    let_it_be(:position1) { create(:epic_board_position, relative_position: 1, epic_board: epic_board) }
+    let_it_be(:position2) { create(:epic_board_position, relative_position: 1900, epic_board: epic_board) }
+    let_it_be(:position3) { create(:epic_board_position, relative_position: 4000) }
+    let_it_be(:position4) { create(:epic_board_position, epic_board: epic_board, relative_position: nil) }
+
+    it 'returns highest not null position' do
+      expect(described_class.last_for_board_id(epic_board.id)).to eq(position2)
+    end
+  end
+
   context 'relative positioning' do
     let_it_be(:positioning_group) { create(:group) }
     let_it_be(:positioning_board) { create(:epic_board, group: positioning_group) }

ee/spec/models/epic_spec.rb

@@ -47,18 +47,43 @@ RSpec.describe Epic do
       end
     end
 
-    describe '.order_relative_position_on_board' do
+    describe 'relative position scopes' do
       let_it_be(:board) { create(:epic_board) }
+      let_it_be(:other_board) { create(:epic_board) }
       let_it_be(:epic1) { create(:epic) }
       let_it_be(:epic2) { create(:epic) }
       let_it_be(:epic3) { create(:epic) }
 
-      it 'returns epics ordered by position on the board, null last' do
-        create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 10)
-        create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 20)
-        create(:epic_board_position, epic: epic3, epic_board: board, relative_position: 20)
+      let_it_be(:position1) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 20) }
+      let_it_be(:position2) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 10) }
+      let_it_be(:position3) { create(:epic_board_position, epic: epic3, epic_board: board, relative_position: 20) }
+      # this position should be ignored because it's for other board:
+      let_it_be(:position5) { create(:epic_board_position, epic: confidential_epic, epic_board: other_board, relative_position: 5) }
 
-        expect(described_class.order_relative_position_on_board(board.id)).to eq([epic2, epic3, epic1, public_epic, confidential_epic])
+      describe '.order_relative_position_on_board' do
+        it 'returns epics ordered by position on the board, null last' do
+          epics = described_class.order_relative_position_on_board(board.id)
+
+          expect(epics).to eq([epic2, epic3, epic1, public_epic, confidential_epic])
+        end
+      end
+
+      describe 'without_board_position' do
+        it 'returns only epics which do not have position set for the board' do
+          epics = described_class.join_board_position(board.id).without_board_position(board.id)
+
+          expect(epics).to match_array([confidential_epic, public_epic])
+        end
+      end
+
+      describe '.join_board_position' do
+        it 'returns epics with joined position for the board' do
+          positions = described_class.join_board_position(board.id)
+            .select('boards_epic_board_positions.relative_position as pos').map(&:pos)
+
+          # confidential_epic and public_epic should have both nil position for the board
+          expect(positions).to match_array([20, 10, 20, nil, nil])
+        end
       end
     end
 
@@ -76,6 +101,17 @@ RSpec.describe Epic do
         expect(described_class.in_milestone(milestone.id)).to match_array([epic1, epic2])
       end
     end
+
+    describe 'from_id' do
+      let_it_be(:max_id) { Epic.maximum(:id) }
+      let_it_be(:epic1) { create(:epic, id: max_id + 1) }
+      let_it_be(:epic2) { create(:epic, id: max_id + 2) }
+      let_it_be(:epic3) { create(:epic, id: max_id + 3) }
+
+      it 'returns records with id bigger or equal to the provided param' do
+        expect(described_class.from_id(epic2.id)).to match_array([epic2, epic3])
+      end
+    end
   end
 
   describe 'validations' do

ee/spec/requests/api/graphql/mutations/boards/epic_boards/epic_move_list_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Reposition and move epic between board lists' do
+  include GraphqlHelpers
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:development) { create(:group_label, group: group, name: 'Development') }
+  let(:epic) { create(:epic, group: group) }
+
+  let_it_be(:board) { create(:epic_board, group: group) }
+  let_it_be(:backlog) { create(:epic_list, epic_board: board, list_type: :backlog) }
+  let_it_be(:labeled_list) { create(:epic_list, epic_board: board, label: development) }
+
+  let(:mutation_class) { Mutations::Boards::EpicBoards::EpicMoveList }
+  let(:mutation_name) { mutation_class.graphql_name }
+  let(:mutation_result_identifier) { mutation_name.camelize(:lower) }
+
+  let(:params) do
+    {
+      board_id: global_id_of(board),
+      epic_id: global_id_of(epic)
+    }
+  end
+
+  let(:move_params) do
+    {
+      from_list_id: global_id_of(backlog),
+      to_list_id: global_id_of(labeled_list)
+    }
+  end
+
+  subject { post_graphql_mutation(mutation(params), current_user: current_user) }
+
+  context 'when epic_boards are disabled' do
+    before do
+      stub_licensed_features(epics: true)
+      stub_feature_flags(epic_boards: false)
+      group.add_developer(current_user)
+    end
+
+    it 'raises feature not available error' do
+      subject
+
+      expect(graphql_errors).to include(a_hash_including('message' => 'epic_boards feature is disabled'))
+    end
+  end
+
+  context 'when epic_boards are enabled' do
+    before do
+      stub_licensed_features(epics: true)
+      stub_feature_flags(epic_boards: true)
+    end
+
+    context 'when user does not have permissions to admin the board' do
+      it 'raises resource not available error' do
+        subject
+
+        message = "The resource that you are attempting to access does not exist or you don't have permission to perform this action"
+        expect(graphql_errors).to include(a_hash_including('message' => message))
+      end
+    end
+
+    context 'when user has permissions to admin the board' do
+      before do
+        group.add_developer(current_user)
+      end
+
+      context 'when required move params are missing' do
+        let(:move_params) { { to_list_id: global_id_of(backlog) } }
+
+        it 'raises an error' do
+          subject
+
+          message = 'One of the parameters fromListId, afterId, beforeId is required together with the toListId parameter.'
+          expect(graphql_errors).to include(a_hash_including('message' => message))
+        end
+      end
+
+      context 'moving an epic to another list' do
+        # rubocop: disable CodeReuse/ActiveRecord
+        it 'moves the epic to another list' do
+          expect { subject }.to change { epic.reload.labels }.from([]).to([development])
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+      end
+
+      context 'repositioning an epic' do
+        let!(:epic1) { create(:epic, group: group) }
+        let!(:epic_board_position) { create(:epic_board_position, epic_board: board, epic: epic1) }
+        let!(:epic2) { create(:epic, group: group) }
+        let!(:epic3) { create(:epic, group: group) }
+
+        def position(epic)
+          epic.epic_board_positions.first.relative_position
+        end
+
+        context 'when both move_before_id and move_after_id params are present' do
+          let(:move_params) do
+            {
+              move_before_id: global_id_of(epic3),
+              move_after_id: global_id_of(epic2),
+              to_list_id: global_id_of(backlog)
+            }
+          end
+
+          it 'repositions the epic' do
+            subject
+
+            expect(position(epic)).to be > position(epic3)
+          end
+        end
+
+        context 'when only move_before_id param is present' do
+          let(:move_params) do
+            {
+              to_list_id: global_id_of(backlog),
+              move_before_id: global_id_of(epic3)
+            }
+          end
+
+          it 'repositions the epic' do
+            subject
+
+            expect(position(epic)).to be > position(epic3)
+          end
+        end
+
+        context 'when only move_after_id param is present' do
+          let(:move_params) do
+            {
+              to_list_id: global_id_of(backlog),
+              move_after_id: global_id_of(epic3)
+            }
+          end
+
+          it 'repositions the epic' do
+            subject
+
+            expect(position(epic)).to be < position(epic3)
+          end
+        end
+      end
+    end
+  end
+
+  def mutation(additional_params = {})
+    graphql_mutation(mutation_name, move_params.merge(additional_params),
+      <<-QL.strip_heredoc
+        clientMutationId
+        epic {
+          iid,
+          relativePosition
+          labels {
+            nodes {
+              title
+            }
+          }
+        }
+        errors
+    QL
+    )
+  end
+end

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

@@ -50,5 +50,13 @@ RSpec.describe Boards::Epics::ListService do
 
       expect(epics).to eq([list1_epic2, list1_epic1, list1_epic3])
     end
+
+    it 'calls the from_id scope' do
+      expect(Epic).to receive(:from_id).with(list1_epic2.id).and_call_original
+
+      described_class
+        .new(group, user, { board_id: board.id, id: list1.id, from_id: list1_epic2.id })
+        .execute
+    end
   end
 end

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

@@ -19,7 +19,7 @@ RSpec.describe Boards::Epics::MoveService do
     let_it_be(:closed) { create(:epic_list, epic_board: board, list_type: :closed, label: nil) }
     let_it_be(:other_board_list) { create(:epic_list, epic_board: other_board, list_type: :closed, label: nil) }
 
-    let(:epic) { create(:epic, group: group) }
+    let_it_be_with_reload(:epic) { create(:epic, group: group) }
 
     let(:params) { { board_id: board.id, from_list_id: from_list.id, to_list_id: to_list.id } }
     let(:from_list) { backlog }
@@ -42,36 +42,6 @@ RSpec.describe Boards::Epics::MoveService do
         group.add_maintainer(user)
       end
 
-      context 'when repositioning epics' do
-        let_it_be(:epic1) { create(:epic, group: group) }
-        let_it_be(: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(:params) { { board_id: board.id, move_before_id: epic1.id } }
-
-        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 'with invalid params' do
-          context 'with board from another group' do
-            let(:board) { create(:epic_board) }
-
-            it 'raises an error' do
-              expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
-            end
-          end
-        end
-      end
-
       context 'when moving an epic between lists' do
         context 'when moving the epic from backlog' do
           context 'to a labeled list' do
@@ -131,6 +101,117 @@ RSpec.describe Boards::Epics::MoveService do
           end
         end
       end
+
+      context 'when repositioning an epic' do
+        let_it_be(:epic1) { create(:epic, group: group) }
+        let_it_be(:epic2) { create(:epic, group: group) }
+        let_it_be(:epic3) { create(:epic, group: group) }
+
+        def create_positions
+          create(:epic_board_position, epic: epic3, epic_board: board, relative_position: 40)
+          create(:epic_board_position, epic: epic, epic_board: board, relative_position: 50)
+          create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 60)
+          create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 80)
+        end
+
+        let(:params) do
+          {
+            board_id: board.id,
+            to_list_id: backlog.id
+          }
+        end
+
+        def epic_relative_position(epic)
+          epic.epic_board_positions.find_by(epic_board_id: board.id)&.relative_position
+        end
+
+        context 'with invalid params' do
+          context 'with board from another group' do
+            let(:other_group) { create(:group) }
+            let(:board) { create(:epic_board, group: other_group) }
+
+            before do
+              other_group.add_maintainer(user)
+              params[:move_before_id] = epic2.id
+            end
+
+            it 'raises an error' do
+              expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+            end
+          end
+        end
+
+        shared_examples 'correct positioning' do
+          context 'when both move_before_id and move_after_id are present' do
+            before do
+              params[:move_before_id] = epic2.id
+              params[:move_after_id] = epic1.id
+            end
+
+            it 'moves the epic' do
+              subject
+
+              expect(epic_relative_position(epic)).to be > epic_relative_position(epic2)
+            end
+          end
+
+          context 'when only move_before_id is present' do
+            before do
+              params[:move_before_id] = epic1.id
+            end
+
+            it 'moves the epic' do
+              subject
+
+              expect(epic_relative_position(epic)).to be > epic_relative_position(epic1)
+            end
+          end
+
+          context 'when only move_after_id is present' do
+            before do
+              params[:move_after_id] = epic3.id
+            end
+
+            it 'moves the epic' do
+              subject
+
+              expect(epic_relative_position(epic)).to be < epic_relative_position(epic3)
+            end
+          end
+        end
+
+        context 'in current list' do
+          context 'when all epics have respective position records' do
+            before do
+              create_positions
+            end
+
+            it_behaves_like 'correct positioning'
+          end
+
+          context 'when epics do not have respective position records' do
+            it_behaves_like 'correct positioning'
+          end
+        end
+
+        context 'during a movement to another list' do
+          before do
+            epic.labels = [development]
+          end
+
+          context 'when all epics have respective position records' do
+            before do
+              create_positions
+            end
+
+            it_behaves_like 'correct positioning'
+          end
+
+          context 'when epics do not have respective position records' do
+            it_behaves_like 'correct positioning'
+          end
+        end
+      end
     end
   end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Boards::Epics::PositionCreateService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:board) { create(:epic_board, group: group) }
+  let_it_be(:list)  { create(:epic_list, epic_board: board, list_type: :backlog) }
+
+  let_it_be_with_reload(:epic1) { create(:epic, group: group) }
+  let_it_be_with_reload(:epic2) { create(:epic, group: group) }
+  let_it_be_with_reload(:epic3) { create(:epic, group: group) }
+  let_it_be_with_reload(:epic4) { create(:epic, group: group) }
+  let_it_be(:epic_other_group) { create(:epic, group: create(:group)) }
+
+  let(:params) { { board_id: board.id, list_id: list.id } }
+
+  describe '#execute' do
+    before do
+      stub_licensed_features(epics: true)
+      group.add_developer(user)
+    end
+
+    context 'with invalid params' do
+      it 'raises an error when board_id is missing' do
+        expect { described_class.new(group, user, { list_id: list.id }).execute }
+          .to raise_error(ArgumentError, 'board_id param is missing')
+      end
+
+      it 'raises an error when list_id is missing' do
+        expect { described_class.new(group, user, { board_id: board.id }).execute }
+          .to raise_error(ArgumentError, 'list_id param is missing')
+      end
+    end
+
+    context 'with correct params' do
+      subject { described_class.new(group, user, params).execute }
+
+      context 'without additional params' do
+        context 'when there are no positions' do
+          it 'creates the positions for all epics in the list' do
+            expect { subject }.to change { Boards::EpicBoardPosition.count }.by(4)
+          end
+
+          it 'sets the relative_position based on id' do
+            subject
+
+            expect(Boards::EpicBoardPosition.order(:relative_position).map(&:epic_id))
+              .to eq([epic4.id, epic3.id, epic2.id, epic1.id])
+          end
+        end
+
+        context 'when some positions exist' do
+          context 'when there is still space for new positions' do
+            let_it_be(:epic_position1) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 1000) }
+            let_it_be(:epic_position3) { create(:epic_board_position, epic: epic3, epic_board: board, relative_position: 10) }
+
+            it 'creates the positions for non existing epics in the list' do
+              expect { subject }.to change { Boards::EpicBoardPosition.count }.by(2)
+            end
+
+            it 'sets the relative_position based on id after the last existing position' do
+              subject
+
+              expect(Boards::EpicBoardPosition.order(:relative_position).map(&:epic_id))
+                .to eq([epic3.id, epic1.id, epic4.id, epic2.id])
+            end
+
+            it 'does not update the existing epic positions' do
+              subject
+
+              expect(epic_position1.reload.relative_position).to eq(1000)
+              expect(epic_position3.reload.relative_position).to eq(10)
+            end
+          end
+
+          context 'when there is no space for new positions' do
+            let_it_be(:epic_position1) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: Boards::EpicBoardPosition::MAX_POSITION) }
+
+            it 'raises an error' do
+              expect { subject }.to raise_error(ActiveModel::RangeError)
+            end
+          end
+        end
+
+        context 'when all positions exist' do
+          let_it_be(:epic_position1) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 1000) }
+          let_it_be(:epic_position2) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 10) }
+          let_it_be(:epic_position3) { create(:epic_board_position, epic: epic3, epic_board: board, relative_position: 10) }
+          let_it_be(:epic_position4) { create(:epic_board_position, epic: epic4, epic_board: board, relative_position: 10) }
+
+          it 'does not create new positions' do
+            expect { subject }.not_to change { Boards::EpicBoardPosition.count }
+          end
+        end
+      end
+
+      context 'with additional params' do
+        context 'for backlog list' do
+          let(:params) { { board_id: board.id, list_id: list.id, from_id: epic2.id } }
+
+          it 'creates the positions for all epics until the last param' do
+            expect { subject }.to change { Boards::EpicBoardPosition.count }.by(3)
+          end
+
+          it 'sets the relative_position based on id' do
+            subject
+
+            expect(Boards::EpicBoardPosition.order(:relative_position).map(&:epic_id))
+              .to eq([epic4.id, epic3.id, epic2.id])
+          end
+        end
+
+        context 'for labeled list' do
+          let_it_be(:label) { create(:group_label, group: group, name: 'Development') }
+          let_it_be(:labeled_list) { create(:epic_list, epic_board: board, label: label) }
+
+          let(:params) { { board_id: board.id, list_id: labeled_list.id, from_id: epic2.id } }
+
+          before do
+            epic1.labels = [label]
+            epic2.labels = [label]
+            epic4.labels = [label]
+          end
+
+          it 'creates the positions for all epics until the last param' do
+            expect { subject }.to change { Boards::EpicBoardPosition.count }.by(2)
+          end
+
+          it 'sets the relative_position based on id only for the epis with list label' do
+            subject
+
+            expect(Boards::EpicBoardPosition.order(:relative_position).map(&:epic_id))
+              .to eq([epic4.id, epic2.id])
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/services/epics/update_service_spec.rb

@@ -97,41 +97,125 @@ RSpec.describe Epics::UpdateService do
     end
 
     context 'when repositioning an epic on a board' do
-      let(:epic1) { create(:epic, group: group) }
-      let(:epic2) { create(:epic, group: group) }
+      let_it_be(:group) { create(:group) }
+      let_it_be(:epic) { create(:epic, group: group) }
+      let_it_be(:epic1) { create(:epic, group: group) }
+      let_it_be(:epic2) { create(:epic, group: group) }
+      let_it_be(:epic3) { 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_it_be(:board) { create(:epic_board, group: group) }
+      let_it_be(:list) { create(:epic_list, epic_board: board, list_type: :backlog) }
 
-      let(:board) { create(:epic_board, group: group) }
+      def position(epic)
+        epic.epic_board_positions.first&.relative_position
+      end
+
+      before do
+        group.add_maintainer(user)
+      end
+
+      shared_examples 'board repositioning' do
+        context 'when moving between 2 epics on the board' do
+          subject { update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id, board_group: group) }
+
+          it 'moves the epic correctly' do
+            subject
+
+            expect(position(epic)).to be > position(epic2)
+
+            # we don't create the position for epic below if it does not exist before the positioning
+            expect(position(epic)).to be < position(epic1) if position(epic1)
+          end
+        end
 
-      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)
+        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, list_id: list.id, board_group: group)
 
-          expect(epic_position.reload.relative_position)
-            .to be_between(epic1_position.relative_position, epic2_position.relative_position)
+            expect(position(epic)).to be > position(epic2)
+          end
         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)
+      context 'when board position records exist for all epics' do
+        let_it_be_with_reload(:epic_position) { create(:epic_board_position, epic: epic, epic_board: board, relative_position: 1) }
+        let_it_be_with_reload(:epic1_position) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 30) }
+        let_it_be_with_reload(:epic2_position) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 20) }
+        let_it_be_with_reload(:epic3_position) { create(:epic_board_position, epic: epic3, epic_board: board, relative_position: 10) }
+
+        it_behaves_like 'board repositioning'
+
+        context 'when moving beetween 2 epics on the board' do
+          it 'keeps epic3 on top of the board' do
+            update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id, board_group: group)
+
+            expect(position(epic3)).to be < position(epic2)
+            expect(position(epic3)).to be < position(epic1)
+          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: [epic3.id, nil], board_id: board.id, list_id: list.id, board_group: group)
 
-          expect(epic_position.reload.relative_position).to be > epic2_position.relative_position
+            expect(epic_position.reload.relative_position).to be < epic3_position.relative_position
+          end
+        end
+
+        context 'when moving the epic to the end' do
+          it 'keeps epic3 on top of the board' do
+            update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id, board_group: group)
+
+            expect(position(epic3)).to be < position(epic2)
+            expect(position(epic3)).to be < position(epic1)
+          end
         end
       end
 
-      context 'when moving the epic to the beginning' do
-        before do
-          epic_position.update_column(:relative_position, 25)
+      context 'when board position records are missing' do
+        context 'when the position does not exist for any record' do
+          it_behaves_like 'board repositioning'
+        end
+
+        context 'when the epic is in a subgroup' do
+          let(:subgroup) { create(:group, parent: group) }
+          let(:epic) { create(:epic, group: subgroup) }
+
+          it_behaves_like 'board repositioning'
+        end
+
+        context 'when the position does not exist for the record being moved' do
+          let_it_be_with_reload(:epic1_position) { create(:epic_board_position, epic: epic1, epic_board: board, relative_position: 30) }
+          let_it_be_with_reload(:epic2_position) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 20) }
+
+          it_behaves_like 'board repositioning'
+        end
+
+        context 'when the position exists for the above and moving records but not for higher ids' do
+          let_it_be_with_reload(:epic2_position) { create(:epic_board_position, epic: epic2, epic_board: board, relative_position: 30) }
+          let_it_be_with_reload(:epic_position) { create(:epic_board_position, epic: epic, epic_board: board, relative_position: 10) }
+
+          subject { update_epic(move_between_ids: [epic1.id, epic2.id], board_id: board.id, list_id: list.id, board_group: group) }
+
+          it 'moves the epic correctly' do
+            subject
+
+            expect(position(epic)).to be > position(epic2)
+          end
+
+          it 'does not create new position records' do
+            expect { subject }.not_to change { Boards::EpicBoardPosition.count }
+          end
         end
 
-        it 'moves the epic correctly' do
-          update_epic(move_between_ids: [epic1.id, nil], board_id: board.id)
+        context 'when the position does not exist for the records around the one being moved' do
+          let_it_be_with_reload(:epic_position) { create(:epic_board_position, epic: epic, epic_board: board, relative_position: 10) }
 
-          expect(epic_position.reload.relative_position).to be < epic1_position.relative_position
+          it_behaves_like 'board repositioning'
         end
       end
     end