Merge branch '13633-order-epic-issues-everything' into 'master'

Add support for relative ordering between classes for epics and epic_issues

Closes #13633

See merge request gitlab-org/gitlab!17800

db/post_migrate/20190926180443_schedule_epic_issues_after_epics_move.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ScheduleEpicIssuesAfterEpicsMove < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INTERVAL = 5.minutes.to_i
+  BATCH_SIZE = 100
+  MIGRATION = 'MoveEpicIssuesAfterEpics'
+
+  disable_ddl_transaction!
+
+  class Epic < ActiveRecord::Base
+    self.table_name = 'epics'
+
+    include ::EachBatch
+  end
+
+  def up
+    return unless ::Gitlab.ee?
+
+    Epic.each_batch(of: BATCH_SIZE) do |batch, index|
+      range = batch.pluck('MIN(id)', 'MAX(id)').first
+      delay = index * interval
+      BackgroundMigrationWorker.perform_in(delay, MIGRATION, *range)
+    end
+  end
+
+  def down
+    # no need
+  end
+end

ee/app/assets/javascripts/related_issues/components/related_issuable_input.vue

@@ -122,7 +122,10 @@ export default {
       // Avoid tokenizing partial input when clicking an autocomplete item
       if (!this.isAutoCompleteOpen) {
         const { value } = this.$refs.input;
-        this.$emit('addIssuableFormBlur', value);
+        // Avoid event emission when only pathIdSeparator has been typed
+        if (value !== this.pathIdSeparator) {
+          this.$emit('addIssuableFormBlur', value);
+        }
       }
     },
     onFocus() {

ee/app/assets/javascripts/related_items_tree/mixins/tree_dd_mixin.js

 import Draggable from 'vuedraggable';
 
 import defaultSortableConfig from '~/sortable/sortable_config';
-import { ChildType, idProp, relativePositions } from '../constants';
+import { idProp, relativePositions } from '../constants';
 
 export default {
   computed: {
@@ -17,59 +17,63 @@ export default {
         'ghost-class': 'tree-item-drag-active',
         'data-parent-reference': this.parentItem.reference,
         value: this.children,
-        move: this.handleDragOnMove,
       };
 
       return this.userSignedIn ? options : {};
     },
   },
   methods: {
+    /**
+     * This method returns ID property's value for a given
+     * item by accessing it using correct property name.
+     *
+     * Here's the name of id property for item types;
+     *    Type `Epic` ->  `id`
+     *    Type `Issue` ->  `epicIssueId`
+     *
+     * @param {object} item
+     */
+    getItemId(item) {
+      return item[idProp[item.type]];
+    },
     /**
      * This method returns an object containing
+     *
      *  - `id` Global ID of target item.
      *  - `adjacentReferenceId` Global ID of adjacent item that's
      *                          either above or below new position of target item.
-     *  - `relativePosition` String representation of adjacent item which can be
-     *                       either `above` or `below`.
-     *
-     * Note: Current implementation of this method handles Epics and Issues separately
-     *       But once we support interspersed reordering, we won't need to treat
-     *       them separately.
+     *  - `relativePosition` String representation of adjacent item with respect to
+     *                       target item, which can be either `before` or `after`.
      *
      * @param {number} object.newIndex new position of target item
      * @param {object} object.targetItem target item object
      */
     getTreeReorderMutation({ newIndex, targetItem }) {
-      const currentItemEpicsBeginAtIndex = 0;
-      const { currentItemIssuesBeginAtIndex, children } = this;
-      const isEpic = targetItem.type === ChildType.Epic;
-      const idPropVal = idProp[targetItem.type];
-      let adjacentReferenceId;
       let relativePosition;
 
-      // This condition does either of the two checks as follows;
-      // 1. If target item is of type *Epic* and newIndex is *NOT* on top of Epics list.
-      // 2. If target item is of type *Issue* and newIndex is *NOT* on top of Issues list.
-      if (
-        (isEpic && newIndex > currentItemEpicsBeginAtIndex) ||
-        (!isEpic && newIndex > currentItemIssuesBeginAtIndex)
-      ) {
-        // We set `adjacentReferenceId` to the item ID that's _above_ the target items new position.
-        // And since adjacent item is above, we set `relativePosition` to `Before`.
-        adjacentReferenceId = children[newIndex - 1][idPropVal];
+      // adjacentReference is always the item that's at the position
+      // where target was moved.
+      const adjacentReferenceId = this.getItemId(this.children[newIndex]);
+
+      if (newIndex === 0) {
+        // If newIndex is `0`, item was moved to the top.
+        // Adjacent reference will be the one which is currently at the top,
+        // and it's relative position with respect to target's new position is `after`.
+        relativePosition = relativePositions.After;
+      } else if (newIndex === this.children.length - 1) {
+        // If newIndex is last position in list, item was moved to the bottom.
+        // Adjacent reference will be the one which is currently at the bottom,
+        // and it's relative position with respect to target's new position is `before`.
         relativePosition = relativePositions.Before;
       } else {
-        // We set `adjacentReferenceId` to the item ID that's on top of the list (either Epics or Issues)
-        // And since adjacent item is below, we set `relativePosition` to `After`.
-        adjacentReferenceId =
-          children[isEpic ? currentItemEpicsBeginAtIndex : currentItemIssuesBeginAtIndex][
-            idPropVal
-          ];
+        // If newIndex is neither top nor bottom, it was moved somewhere in the middle.
+        // Adjacent reference will be the one which currently at that position,
+        // and it's relative postion with respect to target's new position is `after`.
         relativePosition = relativePositions.After;
       }
 
       return {
-        id: targetItem[idPropVal],
+        id: this.getItemId(targetItem),
         adjacentReferenceId,
         relativePosition,
       };
@@ -82,33 +86,6 @@ export default {
     handleDragOnStart() {
       document.body.classList.add('is-dragging');
     },
-    /**
-     * This event handler is constantly fired as user is dragging
-     * the item around the UI.
-     *
-     * This method returns boolean value based on following
-     * condition checks, thus preventing interspersed ordering;
-     * 1. If item being dragged is Epic,
-     *    and it is moved on top of Issues; return `false`
-     * 2. If item being dragged is Issue,
-     *    and it is moved on top of Epics; return `false`.
-     * 3. If above two conditions are not met; return `true`.
-     *
-     * @param {object} event Object representing drag move event.
-     */
-    handleDragOnMove({ dragged, related }) {
-      let isAllowed = false;
-
-      if (dragged.classList.contains('js-item-type-epic')) {
-        isAllowed = related.classList.contains('js-item-type-epic');
-      } else {
-        isAllowed = related.classList.contains('js-item-type-issue');
-      }
-
-      document.body.classList.toggle('no-drop', !isAllowed);
-
-      return isAllowed;
-    },
     /**
      * This event handler is fired when user releases the dragging
      * item.

ee/app/assets/javascripts/related_items_tree/store/actions.js

@@ -259,13 +259,31 @@ export const setItemInputValue = ({ commit }, data) => commit(types.SET_ITEM_INP
 
 export const requestAddItem = ({ commit }) => commit(types.REQUEST_ADD_ITEM);
 export const receiveAddItemSuccess = ({ dispatch, commit, getters }, { rawItems }) => {
-  const items = rawItems.map(item =>
-    formatChildItem({
-      ...convertObjectPropsToCamelCase(item, { deep: !getters.isEpic }),
+  const items = rawItems.map(item => {
+    // This is needed since Rails API to add Epic/Issue
+    // doesn't return global ID string.
+    // We can remove this change once add epic/issue
+    // action is moved to GraphQL.
+    // See https://gitlab.com/gitlab-org/gitlab/issues/34529
+    const globalItemId = {};
+    if (getters.isEpic) {
+      globalItemId.id = !`${item.id}`.includes('gid://') ? `gid://gitlab/Epic/${item.id}` : item.id;
+    } else {
+      globalItemId.epicIssueId = !`${item.epic_issue_id}`.includes('gid://')
+        ? `gid://gitlab/EpicIssue/${item.epic_issue_id}`
+        : item.epic_issue_id;
+    }
+
+    return formatChildItem({
+      ...convertObjectPropsToCamelCase(item, {
+        deep: !getters.isEpic,
+        dropKeys: ['id', 'epic_issue_id'],
+      }),
+      ...globalItemId,
       type: getters.isEpic ? ChildType.Epic : ChildType.Issue,
       userPermissions: getters.isEpic ? { adminEpic: item.can_admin } : {},
-    }),
-  );
+    });
+  });
 
   commit(types.RECEIVE_ADD_ITEM_SUCCESS, {
     insertAt: getters.isEpic ? 0 : getters.issuesBeginAtIndex,

ee/app/assets/javascripts/related_items_tree/utils/epic_utils.js

@@ -34,15 +34,13 @@ export const formatChildItem = item =>
  * @param {Array} children
  */
 export const extractChildEpics = children =>
-  children.edges
-    .map(({ node, epicNode = node }) =>
-      formatChildItem({
-        ...epicNode,
-        fullPath: epicNode.group.fullPath,
-        type: ChildType.Epic,
-      }),
-    )
-    .sort(sortChildren);
+  children.edges.map(({ node, epicNode = node }) =>
+    formatChildItem({
+      ...epicNode,
+      fullPath: epicNode.group.fullPath,
+      type: ChildType.Epic,
+    }),
+  );
 
 /**
  * Returns formatted array of Assignees that doesn't contain
@@ -62,20 +60,20 @@ export const extractIssueAssignees = assignees =>
  * @param {Array} issues
  */
 export const extractChildIssues = issues =>
-  issues.edges
-    .map(({ node, issueNode = node }) =>
-      formatChildItem({
-        ...issueNode,
-        type: ChildType.Issue,
-        assignees: extractIssueAssignees(issueNode.assignees),
-      }),
-    )
-    .sort(sortChildren);
+  issues.edges.map(({ node, issueNode = node }) =>
+    formatChildItem({
+      ...issueNode,
+      type: ChildType.Issue,
+      assignees: extractIssueAssignees(issueNode.assignees),
+    }),
+  );
 
 /**
  * Parses Graph query response and updates
  * children array to include issues within it
+ * and then sorts everything based on `relativePosition`
+ *
  * @param {Object} responseRoot
  */
 export const processQueryResponse = ({ epic }) =>
-  [].concat(extractChildEpics(epic.children), extractChildIssues(epic.issues));
+  [].concat(extractChildEpics(epic.children), extractChildIssues(epic.issues)).sort(sortChildren);

ee/app/models/concerns/epic_tree_sorting.rb

+# frozen_string_literal: true
+
+module EpicTreeSorting
+  extend ActiveSupport::Concern
+  include FromUnion
+  include RelativePositioning
+
+  class_methods do
+    def relative_positioning_query_base(object)
+      from_union([
+        EpicIssue.select("id, relative_position, epic_id, 'epic_issue' as object_type").in_epic(object.parent_ids),
+        Epic.select("id, relative_position, parent_id as epic_id, 'epic' as object_type").where(parent_id: object.parent_ids)
+      ])
+    end
+
+    def relative_positioning_parent_column
+      :epic_id
+    end
+  end
+
+  included do
+    def move_sequence(start_pos, end_pos, delta)
+      items_to_update = scoped_items
+        .select(:id, :object_type)
+        .where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
+        .where.not('object_type = ? AND id = ?', self.class.table_name.singularize, self.id)
+
+      items_to_update.group_by { |item| item.object_type }.each do |type, group_items|
+        ids = group_items.map(&:id)
+        items = type.camelcase.constantize.where(id: ids).select(:id)
+        items.update_all("relative_position = relative_position + #{delta}")
+      end
+    end
+  end
+end

ee/app/models/ee/epic.rb

@@ -12,9 +12,9 @@ module EE
       include Referable
       include Awardable
       include LabelEventable
-      include RelativePositioning
       include UsageStatistics
       include FromUnion
+      include EpicTreeSorting
 
       enum state_id: {
         opened: ::Epic.available_states[:opened],
@@ -177,14 +177,6 @@ module EE
         ::Group
       end
 
-      def relative_positioning_query_base(epic)
-        in_parents(epic.parent_ids)
-      end
-
-      def relative_positioning_parent_column
-        :parent_id
-      end
-
       # Return the deepest relation level for an epic.
       # Example 1:
       # epic1 - parent: nil

ee/app/models/epic_issue.rb

 # frozen_string_literal: true
 
 class EpicIssue < ApplicationRecord
-  include RelativePositioning
+  include EpicTreeSorting
 
   validates :epic, :issue, presence: true
   validates :issue, uniqueness: true
@@ -10,14 +10,7 @@ class EpicIssue < ApplicationRecord
   belongs_to :issue
 
   alias_attribute :parent_ids, :epic_id
+  alias_attribute :parent, :epic
 
   scope :in_epic, ->(epic_id) { where(epic_id: epic_id) }
-
-  def self.relative_positioning_query_base(epic_issue)
-    in_epic(epic_issue.parent_ids)
-  end
-
-  def self.relative_positioning_parent_column
-    :epic_id
-  end
 end

ee/app/services/epics/tree_reorder_service.rb

@@ -11,40 +11,54 @@ module Epics
     end
 
     def execute
-      klass = case moving_object
-              when EpicIssue
-                EpicIssues::UpdateService
-              when Epic
-                EpicLinks::UpdateService
-              end
-
-      return error('Only epics and epic_issues are supported.') unless klass
-
       error_message = validate_objects
 
       return error(error_message) if error_message.present?
 
-      klass.new(moving_object, current_user, moving_params).execute
+      move!
+      success
     end
 
     private
 
-    def moving_params
-      key = case params[:relative_position].to_sym
-            when :after
-              :move_after_id
-            when :before
-              :move_before_id
-            end
+    def move!
+      moving_object.move_between(before_object, after_object)
+      moving_object.save!(touch: false)
+    end
+
+    def before_object
+      return unless params[:relative_position] == 'before'
 
-      {}.tap { |p| p[key] = adjacent_reference.id }
+      adjacent_reference
+    end
+
+    def after_object
+      return unless params[:relative_position] == 'after'
+
+      adjacent_reference
     end
 
-    # for now we support only ordering within the same type
-    # Follow-up issue: https://gitlab.com/gitlab-org/gitlab/issues/13633
     def validate_objects
+      return 'Relative position is not valid.' unless valid_relative_position?
+
+      unless supported_type?(moving_object) && supported_type?(adjacent_reference)
+        return 'Only epics and epic_issues are supported.'
+      end
+
       return 'You don\'t have permissions to move the objects.' unless authorized?
-      return 'Provided objects are not the same type.' if moving_object.class != adjacent_reference.class
+      return 'Both objects have to belong to the same parent epic.' unless same_parent?
+    end
+
+    def valid_relative_position?
+      %w(before after).include?(params[:relative_position])
+    end
+
+    def same_parent?
+      moving_object.parent == adjacent_reference.parent
+    end
+
+    def supported_type?(object)
+      object.is_a?(EpicIssue) || object.is_a?(Epic)
     end
 
     def authorized?

ee/lib/gitlab/background_migration/move_epic_issues_after_epics.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    class MoveEpicIssuesAfterEpics
+      class EpicIssue < ActiveRecord::Base
+        self.table_name = 'epic_issues'
+      end
+
+      class Epic < ActiveRecord::Base
+        self.table_name = 'epics'
+      end
+
+      def perform(start_id, stop_id)
+        maximum_epic_position = Epic.maximum(:relative_position)
+
+        return unless maximum_epic_position
+
+        max_position = Gitlab::Database::MAX_INT_VALUE
+        delta = ((maximum_epic_position - max_position) / 2.0).abs.ceil
+
+        EpicIssue.where(epic_id: start_id..stop_id).where('relative_position < ?', max_position - delta)
+          .update_all("relative_position = relative_position + #{delta}")
+      end
+    end
+  end
+end

ee/spec/javascripts/related_items_tree/components/tree_root_spec.js

@@ -116,7 +116,6 @@ describe('RelatedItemsTree', () => {
                   'ghost-class': 'tree-item-drag-active',
                   'data-parent-reference': mockParentItem.reference,
                   value: wrapper.vm.children,
-                  move: wrapper.vm.handleDragOnMove,
                 }),
               );
             });
@@ -133,78 +132,115 @@ describe('RelatedItemsTree', () => {
         });
 
         describe('methods', () => {
-          describe('getTreeReorderMutation', () => {
-            it('returns an object containing `id`, `adjacentReferenceId` & `relativePosition` when newIndex param is 0 and targetItem is Epic', () => {
-              const targetItem = wrapper.vm.children[1]; // 2nd Epic position
-              const newIndex = 0; // We're moving targetItem to top of Epics list & Epics begin at 0
+          describe('getItemId', () => {
+            it('returns value of `id` prop when item is an Epic', () => {
+              expect(wrapper.vm.getItemId(wrapper.vm.children[0])).toBe(mockEpic1.id);
+            });
 
-              const treeReorderMutation = wrapper.vm.getTreeReorderMutation({
-                targetItem,
-                newIndex,
-              });
+            it('returns value of `epicIssueId` prop when item is an Issue', () => {
+              expect(wrapper.vm.getItemId(wrapper.vm.children[2])).toBe(mockIssue1.epicIssueId);
+            });
+          });
 
-              expect(treeReorderMutation).toEqual(
+          describe('getTreeReorderMutation', () => {
+            it('returns an object containing ID of targetItem', () => {
+              const targetItemEpic = wrapper.vm.children[0];
+              const targetItemIssue = wrapper.vm.children[2];
+              const newIndex = 0;
+
+              expect(
+                wrapper.vm.getTreeReorderMutation({
+                  targetItem: targetItemEpic,
+                  newIndex,
+                }),
+              ).toEqual(
                 jasmine.objectContaining({
-                  id: targetItem.id,
-                  adjacentReferenceId: mockEpic1.id,
-                  relativePosition: 'after',
+                  id: mockEpic1.id,
+                }),
+              );
+
+              expect(
+                wrapper.vm.getTreeReorderMutation({
+                  targetItem: targetItemIssue,
+                  newIndex,
+                }),
+              ).toEqual(
+                jasmine.objectContaining({
+                  id: mockIssue1.epicIssueId,
                 }),
               );
             });
 
-            it('returns an object containing `id`, `adjacentReferenceId` & `relativePosition` when newIndex param is 1 and targetItem is Epic', () => {
+            it('returns an object containing `adjacentReferenceId` of children item at provided `newIndex`', () => {
               const targetItem = wrapper.vm.children[0];
-              const newIndex = 1;
-
-              const treeReorderMutation = wrapper.vm.getTreeReorderMutation({
-                targetItem,
-                newIndex,
-              });
 
-              expect(treeReorderMutation).toEqual(
+              expect(
+                wrapper.vm.getTreeReorderMutation({
+                  targetItem,
+                  newIndex: 0,
+                }),
+              ).toEqual(
                 jasmine.objectContaining({
-                  id: targetItem.id,
                   adjacentReferenceId: mockEpic1.id,
-                  relativePosition: 'before',
                 }),
               );
-            });
 
-            it('returns an object containing `id`, `adjacentReferenceId` & `relativePosition` when newIndex param is 0 and targetItem is Issue', () => {
-              const targetItem = wrapper.vm.children[3]; // 2nd Issue position
-              const newIndex = 2; // We're moving targetItem to top of Issues list & Issues begin at 2
+              expect(
+                wrapper.vm.getTreeReorderMutation({
+                  targetItem,
+                  newIndex: 2,
+                }),
+              ).toEqual(
+                jasmine.objectContaining({
+                  adjacentReferenceId: mockIssue1.epicIssueId,
+                }),
+              );
+            });
 
-              const treeReorderMutation = wrapper.vm.getTreeReorderMutation({
-                targetItem,
-                newIndex,
-              });
+            it('returns object containing `relativePosition` containing `after` when `newIndex` param is 0', () => {
+              const targetItem = wrapper.vm.children[0];
 
-              expect(treeReorderMutation).toEqual(
+              expect(
+                wrapper.vm.getTreeReorderMutation({
+                  targetItem,
+                  newIndex: 0,
+                }),
+              ).toEqual(
                 jasmine.objectContaining({
-                  id: targetItem.epicIssueId,
-                  adjacentReferenceId: mockIssue1.epicIssueId,
                   relativePosition: 'after',
                 }),
               );
             });
 
-            it('returns an object containing `id`, `adjacentReferenceId` & `relativePosition` when newIndex param is 1 and targetItem is Issue', () => {
-              const targetItem = wrapper.vm.children[2];
-              const newIndex = 3; // Here 3 is first issue of the list, hence spec descripton says `newIndex` as 1.
-
-              const treeReorderMutation = wrapper.vm.getTreeReorderMutation({
-                targetItem,
-                newIndex,
-              });
+            it('returns object containing `relativePosition` containing `before` when `newIndex` param is last item index', () => {
+              const targetItem = wrapper.vm.children[0];
 
-              expect(treeReorderMutation).toEqual(
+              expect(
+                wrapper.vm.getTreeReorderMutation({
+                  targetItem,
+                  newIndex: wrapper.vm.children.length - 1,
+                }),
+              ).toEqual(
                 jasmine.objectContaining({
-                  id: targetItem.epicIssueId,
-                  adjacentReferenceId: mockIssue1.epicIssueId,
                   relativePosition: 'before',
                 }),
               );
             });
+
+            it('returns object containing `relativePosition` containing `after` when `newIndex` param neither `0` nor last item index', () => {
+              const targetItem = wrapper.vm.children[0];
+
+              expect(
+                wrapper.vm.getTreeReorderMutation({
+                  targetItem,
+                  newIndex: 2,
+                }),
+              ).toEqual(
+                jasmine.objectContaining({
+                  relativePosition: 'after',
+                }),
+              );
+            });
           });
 
           describe('handleDragOnStart', () => {
@@ -217,58 +253,6 @@ describe('RelatedItemsTree', () => {
             });
           });
 
-          describe('handleDragOnMove', () => {
-            let dragged;
-            let related;
-            let mockEvent;
-
-            beforeEach(() => {
-              dragged = document.createElement('li');
-              related = document.createElement('li');
-              mockEvent = {
-                dragged,
-                related,
-              };
-            });
-
-            it('returns `true` when an epic is reordered within epics list', () => {
-              dragged.classList.add('js-item-type-epic');
-              related.classList.add('js-item-type-epic');
-
-              expect(wrapper.vm.handleDragOnMove(mockEvent)).toBe(true);
-            });
-
-            it('returns `true` when an issue is reordered within issues list', () => {
-              dragged.classList.add('js-item-type-issue');
-              related.classList.add('js-item-type-issue');
-
-              expect(wrapper.vm.handleDragOnMove(mockEvent)).toBe(true);
-            });
-
-            it('returns `false` when an issue is reordered within epics list', () => {
-              dragged.classList.add('js-item-type-issue');
-              related.classList.add('js-item-type-epic');
-
-              expect(wrapper.vm.handleDragOnMove(mockEvent)).toBe(false);
-            });
-
-            it('returns `false` when an epic is reordered within issues list', () => {
-              dragged.classList.add('js-item-type-epic');
-              related.classList.add('js-item-type-issue');
-
-              expect(wrapper.vm.handleDragOnMove(mockEvent)).toBe(false);
-            });
-
-            it('adds class `no-drop` to body element when reordering is not allowed', () => {
-              dragged.classList.add('js-item-type-epic');
-              related.classList.add('js-item-type-issue');
-
-              wrapper.vm.handleDragOnMove(mockEvent);
-
-              expect(document.body.classList.contains('no-drop')).toBe(true);
-            });
-          });
-
           describe('handleDragOnEnd', () => {
             it('removes class `is-dragging` from document body', () => {
               spyOn(wrapper.vm, 'reorderItem').and.stub();

ee/spec/lib/gitlab/background_migration/move_epic_issues_after_epics_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::MoveEpicIssuesAfterEpics, :migration, schema: 20190926180443 do
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:issues) { table(:issues) }
+  let(:epics) { table(:epics) }
+  let(:epic_issues) { table(:epic_issues) }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    let(:epic_params) do
+      {
+        title: 'Epic',
+        title_html: 'Epic',
+        group_id: group.id,
+        author_id: user.id
+      }
+    end
+    let(:issue_params) do
+      {
+        title: 'Issue',
+        title_html: 'Issue',
+        project_id: project.id,
+        author_id: user.id
+      }
+    end
+
+    let(:user) { users.create(name: 'test', email: 'test@example.com', projects_limit: 5) }
+    let(:group) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+
+    context 'when there are epic_issues present' do
+      let(:project) { projects.create(namespace_id: group.id, name: 'foo') }
+      let(:base_epic) { epics.create(epic_params.merge(iid: 3, relative_position: 500)) }
+      let(:issue_1) { issues.create(issue_params.merge(iid: 1)) }
+      let(:issue_2) { issues.create(issue_params.merge(iid: 2)) }
+      let(:issue_3) { issues.create(issue_params.merge(iid: 3)) }
+
+      let!(:epic_1) { epics.create(epic_params.merge(iid: 1, relative_position: 100)) }
+      let!(:epic_2) { epics.create(epic_params.merge(iid: 2, relative_position: 5000)) }
+      let!(:epic_issue_1) { epic_issues.create(issue_id: issue_1.id, epic_id: base_epic.id, relative_position: 400) }
+      let!(:epic_issue_2) { epic_issues.create(issue_id: issue_2.id, epic_id: base_epic.id, relative_position: 5010) }
+      let!(:epic_issue_3) { epic_issues.create(issue_id: issue_3.id, epic_id: base_epic.id, relative_position: Gitlab::Database::MAX_INT_VALUE - 10) }
+
+      before do
+        subject.perform(epics.first.id, epics.last.id)
+      end
+
+      it 'does not change relative_position of epics' do
+        expect(base_epic.relative_position).to eq(500)
+        expect(epic_1.relative_position).to eq(100)
+        expect(epic_2.relative_position).to eq(5000)
+      end
+
+      it 'moves epic_issues after epics' do
+        expect(epic_issue_1.reload.relative_position).to be > 5000
+        expect(epic_issue_2.reload.relative_position).to be > 5000
+      end
+
+      it 'keeps epic_issues order' do
+        expect(epic_issue_1.reload.relative_position).to be < epic_issue_2.reload.relative_position
+      end
+
+      it 'does not change the relative_position of epic_issue getting to the max value' do
+        expect(epic_issue_3.reload.relative_position).to eq(Gitlab::Database::MAX_INT_VALUE - 10)
+      end
+    end
+
+    context 'when there are no epics' do
+      it 'runs correctly' do
+        expect(subject.perform(1, 10)).to be_nil
+      end
+    end
+
+    context 'when there are no epic_issues' do
+      it 'runs correctly' do
+        epics.create(epic_params.merge(iid: 3, relative_position: 500))
+
+        expect(subject.perform(1, 10)).to be_zero
+      end
+    end
+  end
+end

ee/spec/models/concerns/epic_tree_sorting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EpicTreeSorting do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:base_epic) { create(:epic, group: group) }
+  let!(:epic_issue1) { create(:epic_issue, epic: base_epic, relative_position: 10) }
+  let!(:epic_issue2) { create(:epic_issue, epic: base_epic, relative_position: 500) }
+  let!(:epic_issue3) { create(:epic_issue, epic: base_epic, relative_position: 1002) }
+  let!(:epic1) { create(:epic, parent: base_epic, group: group, relative_position: 100) }
+  let!(:epic2) { create(:epic, parent: base_epic, group: group, relative_position: 1000) }
+  let!(:epic3) { create(:epic, parent: base_epic, group: group, relative_position: 1001) }
+
+  context '#move_after' do
+    it 'moves an epic' do
+      epic1.move_after(epic_issue2)
+
+      expect(epic1.relative_position).to be_between(epic_issue2.reload.relative_position, epic2.reload.relative_position).exclusive
+    end
+
+    it 'moves an epic_issue' do
+      epic_issue2.move_after(epic2)
+
+      expect(epic_issue2.relative_position).to be_between(epic2.reload.relative_position, epic3.reload.relative_position).exclusive
+      expect(epic_issue3.reload.relative_position).to be > epic3.reload.relative_position
+    end
+  end
+
+  context '#move_before' do
+    it 'moves an epic' do
+      epic2.move_before(epic_issue2)
+
+      expect(epic2.relative_position).to be_between(epic_issue1.reload.relative_position, epic_issue2.reload.relative_position).exclusive
+    end
+
+    it 'moves an epic_issue' do
+      epic_issue3.move_before(epic2)
+
+      expect(epic_issue3.relative_position).to be_between(epic_issue2.reload.relative_position, epic2.reload.relative_position).exclusive
+    end
+  end
+
+  context '#move_between' do
+    it 'moves an epic' do
+      epic1.move_between(epic_issue1, epic_issue2)
+
+      expect(epic1.relative_position).to be_between(epic_issue1.reload.relative_position, epic_issue2.reload.relative_position).exclusive
+    end
+
+    it 'moves an epic_issue' do
+      epic_issue3.move_between(epic1, epic_issue2)
+
+      expect(epic_issue3.relative_position).to be_between(epic1.reload.relative_position, epic_issue2.reload.relative_position).exclusive
+    end
+  end
+
+  context '#move_sequence' do
+    let!(:epic_issue1) { create(:epic_issue, epic: base_epic, relative_position: 1000) }
+    let!(:epic_issue2) { create(:epic_issue, epic: base_epic, relative_position: 1001) }
+    let!(:epic_issue3) { create(:epic_issue, epic: base_epic, relative_position: 1004) }
+    let!(:epic1) { create(:epic, parent: base_epic, group: group, relative_position: 1002) }
+    let!(:epic2) { create(:epic, parent: base_epic, group: group, relative_position: 1003) }
+    let!(:epic3) { create(:epic, parent: base_epic, group: group, relative_position: 1005) }
+
+    context 'when self is an epic' do
+      it 'moves all objects correctly' do
+        epic1.move_sequence(1003, 1005, 500)
+
+        expect(epic_issue1.reload.relative_position).to eq(1000)
+        expect(epic_issue2.reload.relative_position).to eq(1001)
+        expect(epic_issue3.reload.relative_position).to eq(1504)
+
+        expect(epic1.reload.relative_position).to eq(1002)
+        expect(epic2.reload.relative_position).to eq(1503)
+        expect(epic3.reload.relative_position).to eq(1505)
+      end
+    end
+
+    context 'when self is an epic_issue' do
+      it 'moves all objects correctly' do
+        epic_issue1.move_sequence(1001, 1005, 500)
+
+        expect(epic_issue1.reload.relative_position).to eq(1000)
+        expect(epic_issue2.reload.relative_position).to eq(1501)
+        expect(epic_issue3.reload.relative_position).to eq(1504)
+
+        expect(epic1.reload.relative_position).to eq(1502)
+        expect(epic2.reload.relative_position).to eq(1503)
+        expect(epic3.reload.relative_position).to eq(1505)
+      end
+    end
+  end
+end

ee/spec/requests/api/graphql/mutations/epic_tree/reorder_spec.rb

@@ -106,7 +106,7 @@ describe 'Updating an epic tree' do
           end
         end
 
-        context 'when moving an epic fails' do
+        context 'when moving an epic fails due to another parent' do
           let(:epic2) { create(:epic, relative_position: 20) }
 
           it_behaves_like 'a mutation that does not update the tree'
@@ -114,7 +114,7 @@ describe 'Updating an epic tree' do
           it 'returns the error message' do
             post_graphql_mutation(mutation, current_user: current_user)
 
-            expect(mutation_response['errors']).to eq(['Epic not found for given params'])
+            expect(mutation_response['errors']).to eq(['Both objects have to belong to the same parent epic.'])
           end
         end
       end
@@ -138,7 +138,7 @@ describe 'Updating an epic tree' do
         end
       end
 
-      context 'when moving an issue fails' do
+      context 'when moving an issue fails due to another parent' do
         let(:epic_issue2) { create(:epic_issue, relative_position: 20) }
 
         before do
@@ -151,7 +151,7 @@ describe 'Updating an epic tree' do
         it 'returns the error message' do
           post_graphql_mutation(mutation, current_user: current_user)
 
-          expect(mutation_response['errors']).to eq(['Epic issue not found for given params'])
+          expect(mutation_response['errors']).to eq(['Both objects have to belong to the same parent epic.'])
         end
       end
     end

ee/spec/services/epics/tree_reorder_service_spec.rb

@@ -12,10 +12,10 @@ describe Epics::TreeReorderService do
     let(:issue2) { create(:issue, project: project) }
     let(:epic1) { create(:epic, group: group, parent: epic, relative_position: 10) }
     let(:epic2) { create(:epic, group: group, parent: epic, relative_position: 20) }
-    let(:epic_issue1) { create(:epic_issue, epic: epic, issue: issue1, relative_position: 10) }
-    let(:epic_issue2) { create(:epic_issue, epic: epic, issue: issue2, relative_position: 20) }
+    let(:epic_issue1) { create(:epic_issue, epic: epic, issue: issue1, relative_position: 30) }
+    let(:epic_issue2) { create(:epic_issue, epic: epic, issue: issue2, relative_position: 40) }
 
-    let(:relative_position) { :after }
+    let(:relative_position) { 'after' }
     let!(:tree_object_1) { epic1 }
     let!(:tree_object_2) { epic2 }
     let(:adjacent_reference_id) { GitlabSchema.id_from_object(tree_object_1) }
@@ -32,10 +32,8 @@ describe Epics::TreeReorderService do
 
     shared_examples 'error for the tree update' do |expected_error|
       it 'does not change relative_positions' do
-        subject
-
-        expect(tree_object_1.relative_position).to eq(10)
-        expect(tree_object_2.relative_position).to eq(20)
+        expect { subject }.not_to change { tree_object_1.relative_position }
+        expect { subject }.not_to change { tree_object_2.relative_position }
       end
 
       it 'returns error status' do
@@ -47,7 +45,7 @@ describe Epics::TreeReorderService do
       end
     end
 
-    context 'when epics feature is enabled' do
+    context 'when epics feature is not enabled' do
       it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
     end
 
@@ -65,16 +63,26 @@ describe Epics::TreeReorderService do
           group.add_developer(user)
         end
 
+        context 'when relative_position is not valid' do
+          let(:relative_position) { 'whatever' }
+
+          it_behaves_like 'error for the tree update', 'Relative position is not valid.'
+        end
+
         context 'when moving EpicIssue' do
           let!(:tree_object_1) { epic_issue1 }
           let!(:tree_object_2) { epic_issue2 }
 
-          # for now we support only ordering within the same type
-          # Follow-up issue: https://gitlab.com/gitlab-org/gitlab/issues/13633
           context 'when object being moved is not the same type as the switched object' do
+            let!(:tree_object_3) { epic1 }
+            let!(:tree_object_4) { epic2 }
             let(:adjacent_reference_id) { GitlabSchema.id_from_object(epic2) }
 
-            it_behaves_like 'error for the tree update', 'Provided objects are not the same type.'
+            it 'reorders the objects' do
+              subject
+
+              expect(epic2.reload.relative_position).to be > tree_object_2.reload.relative_position
+            end
           end
 
           context 'when no object to switch is provided' do
@@ -85,8 +93,22 @@ describe Epics::TreeReorderService do
             end
           end
 
+          context 'when object being moved is from of another epic' do
+            before do
+              other_epic = create(:epic, group: group)
+              epic_issue2.update(epic: other_epic)
+            end
+
+            it_behaves_like 'error for the tree update', 'Both objects have to belong to the same parent epic.'
+          end
+
           context 'when object being moved is not supported type' do
             let(:moving_object_id) { GitlabSchema.id_from_object(issue1) }
+
+            it_behaves_like 'error for the tree update', 'Only epics and epic_issues are supported.'
+          end
+
+          context 'when adjacent object is not supported type' do
             let(:adjacent_reference_id) { GitlabSchema.id_from_object(issue2) }
 
             it_behaves_like 'error for the tree update', 'Only epics and epic_issues are supported.'
@@ -117,14 +139,6 @@ describe Epics::TreeReorderService do
           let!(:tree_object_1) { epic1 }
           let!(:tree_object_2) { epic2 }
 
-          # for now we support only ordering within the same type
-          # Follow-up issue: https://gitlab.com/gitlab-org/gitlab/issues/13633
-          context 'when object being moved is not the same type as the switched object' do
-            let(:adjacent_reference_id) { GitlabSchema.id_from_object(epic_issue2) }
-
-            it_behaves_like 'error for the tree update', 'Provided objects are not the same type.'
-          end
-
           context 'when the reordered epics are not subepics of the base epic' do
             let(:another_group) { create(:group) }
             let(:another_epic) { create(:epic, group: another_group) }
@@ -137,6 +151,15 @@ describe Epics::TreeReorderService do
             it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
           end
 
+          context 'when object being moved is froms another epic' do
+            before do
+              other_epic = create(:epic, group: group)
+              epic2.update(parent: other_epic)
+            end
+
+            it_behaves_like 'error for the tree update', 'Both objects have to belong to the same parent epic.'
+          end
+
           context 'when moving is successful' do
             it 'updates the links relative positions' do
               subject