Add support for interspersed reordering

Adds support for interspersed reordering between
epics and issues.

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/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();