Merge pull request #7993 from opf/fix/32114-Parent-changes-by-drag-and-drop-are-not-saved-correctly

[32114] Parent changes by drag and drop are not saved correctly

[ci skip]

frontend/src/app/components/wp-fast-table/helpers/wp-table-row-helpers.ts

@@ -6,6 +6,10 @@ export function rowId(workPackageId:string):string {
   return `wp-row-${workPackageId}-table`;
 }
 
+export function relationRowClass():string {
+  return `wp-table--relations-aditional-row`;
+}
+
 export function locateTableRow(workPackageId:string):JQuery {
   return jQuery('.' + rowId(workPackageId));
 }

frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts

@@ -6,11 +6,14 @@ import {
   hierarchyGroupClass,
   hierarchyRootClass
 } from "core-components/wp-fast-table/helpers/wp-table-hierarchy-helpers";
+import {WorkPackageCacheService} from "core-components/work-packages/work-package-cache.service";
+import {relationRowClass} from "core-components/wp-fast-table/helpers/wp-table-row-helpers";
 
 export class HierarchyDragActionService extends TableDragActionService {
 
   private wpTableHierarchies = this.injector.get(WorkPackageViewHierarchiesService);
   private relationHierarchyService = this.injector.get(WorkPackageRelationsHierarchyService);
+  private wpCacheService = this.injector.get(WorkPackageCacheService);
 
   public get applies() {
     return this.wpTableHierarchies.isEnabled;
@@ -24,36 +27,73 @@ export class HierarchyDragActionService extends TableDragActionService {
   }
 
   public handleDrop(workPackage:WorkPackageResource, el:HTMLElement):Promise<unknown> {
-    const parentObject = this.determineParent(el);
-    const newParent = parentObject ? parentObject.id : null;
-    return this.relationHierarchyService.changeParent(workPackage, newParent);
+    return this.determineParent(el).then((parentId:string|null) => {
+      return this.relationHierarchyService.changeParent(workPackage, parentId);
+    });
   }
 
   /**
    * Find an applicable parent element from the hierarchy information in the table.
    * @param el
    */
-  private determineParent(el:HTMLElement):{el:Element, id:string}|null {
+  private determineParent(el:HTMLElement):Promise<string|null> {
     let previous = el.previousElementSibling;
+    var parent = null;
 
-    while (previous !== null) {
-      // When there is no hierarchy group at all, we're at a flat list
-      const inGroup = previous.className.indexOf(hierarchyGroupClass('')) >= 0;
-      const isRoot = previous.className.indexOf(hierarchyRootClass('')) >= 0;
-      if (!(inGroup || isRoot)) {
-        return null;
+    if (previous !== null && !this.isFlatList(previous)) {
+      // If the previous element is a relation row,
+      // skip it until we find the real previous sibling
+      const isRelationRow = previous.className.indexOf(relationRowClass()) >= 0;
+      if (isRelationRow) {
+        let relationRoot = this.findRelationRowRoot(previous);
+        if (relationRoot == null) {
+          return Promise.resolve(null);
+        }
+        previous = relationRoot;
       }
 
-      // If the sibling is a hierarchy root, return this one
-      let wpId = (previous as HTMLElement).dataset.workPackageId!;
-      if (previous.classList.contains(hierarchyRootClass(wpId))) {
-        return {el: previous, id: wpId};
+      let previousWpId = (previous as HTMLElement).dataset.workPackageId!;
+      if (this.isHiearchyRoot(previous, previousWpId)) {
+        // If the sibling is a hierarchy root, return that sibling as new parent.
+        parent = previousWpId;
+      }
+      else {
+        // If the sibling is no hierarchy root, return it's parent.
+        // Thus, the dropped element will get the same hierarchy level as the sibling
+        parent = this.loadParentOfWP(previousWpId);
       }
+    }
 
+    return Promise.resolve(parent);
+  }
+
+  private findRelationRowRoot(el:Element):Element|null {
+    let previous = el.previousElementSibling;
+    while (previous !== null) {
+      if (previous.className.indexOf(relationRowClass()) < 0) {
+        return previous;
+      }
       previous = previous.previousElementSibling;
     }
 
     return null;
   }
 
+  private isFlatList(previous:Element):boolean {
+    const inGroup = previous.className.indexOf(hierarchyGroupClass('')) >= 0;
+    const isRoot = previous.className.indexOf(hierarchyRootClass('')) >= 0;
+
+    return !(inGroup || isRoot);
+  }
+
+  private isHiearchyRoot(previous:Element, previousWpId:string):boolean {
+    return previous.classList.contains(hierarchyRootClass(previousWpId));
+  }
+
+  private loadParentOfWP(wpId:string):Promise<string|null> {
+    return this.wpCacheService.require(wpId)
+      .then((wp:WorkPackageResource) => {
+        return Promise.resolve(wp.parent.id);
+      });
+  }
 }

spec/features/work_packages/sorting/manual_sorting_spec.rb

@@ -40,13 +40,28 @@ describe 'Manual sorting of WP table', type: :feature, js: true do
     FactoryBot.create(:work_package, subject: 'WP1', project: project, type: type_task, created_at: Time.now)
   end
   let(:work_package_2) do
-    FactoryBot.create(:work_package, subject: 'WP2', project: project, parent: work_package_1, type: type_task, created_at: Time.now - 1.minutes)
+    FactoryBot.create(:work_package,
+                      subject: 'WP2',
+                      project: project,
+                      parent: work_package_1,
+                      type: type_task,
+                      created_at: Time.now - 1.minutes)
   end
   let(:work_package_3) do
-    FactoryBot.create(:work_package, subject: 'WP3', project: project, parent: work_package_2, type: type_bug, created_at: Time.now - 2.minutes)
+    FactoryBot.create(:work_package,
+                      subject: 'WP3',
+                      project: project,
+                      parent: work_package_2,
+                      type: type_bug,
+                      created_at: Time.now - 2.minutes)
   end
   let(:work_package_4) do
-    FactoryBot.create(:work_package, subject: 'WP4', project: project, parent: work_package_3, type: type_bug, created_at: Time.now - 3.minutes)
+    FactoryBot.create(:work_package,
+                      subject: 'WP4',
+                      project: project,
+                      parent: work_package_3,
+                      type: type_bug,
+                      created_at: Time.now - 3.minutes)
   end
 
   let(:sort_by) { ::Components::WorkPackages::SortBy.new }
@@ -122,7 +137,7 @@ describe 'Manual sorting of WP table', type: :feature, js: true do
       hierarchies.expect_leaf_at(work_package_3, work_package_4)
     end
 
-    it 'can drag an element out of the hierarchy' do
+    it 'can drag an element completely out of the hierarchy' do
       # Move up the hierarchy
       wp_table.drag_and_drop_work_package from: 3, to: 0
       loading_indicator_saveguard
@@ -140,6 +155,47 @@ describe 'Manual sorting of WP table', type: :feature, js: true do
       hierarchies.expect_leaf_at(work_package_3, work_package_4)
       wp_page.expect_no_parent
     end
+
+    context 'drag an element partly out of the hierarchy' do
+      let(:work_package_5) do
+        FactoryBot.create(:work_package, subject: 'WP5', project: project, parent: work_package_1)
+      end
+      let(:work_package_6) do
+        FactoryBot.create(:work_package, subject: 'WP6', project: project, parent: work_package_1)
+      end
+
+      before do
+        work_package_5
+        work_package_6
+        work_package_4.parent = work_package_2
+        work_package_4.save!
+        wp_table.visit!
+
+        # Hierarchy enabled
+        wp_table.expect_work_package_order(work_package_1,
+                                           work_package_2,
+                                           work_package_3,
+                                           work_package_4,
+                                           work_package_5,
+                                           work_package_6)
+        hierarchies.expect_hierarchy_at(work_package_1, work_package_2)
+        hierarchies.expect_leaf_at(work_package_3, work_package_4, work_package_5, work_package_6)
+      end
+
+      it 'move below a sibling of my parent' do
+        wp_table.drag_and_drop_work_package from: 3, to: 5
+
+        loading_indicator_saveguard
+        wp_table.expect_work_package_order(work_package_1,
+                                           work_package_2,
+                                           work_package_3,
+                                           work_package_5,
+                                           work_package_4,
+                                           work_package_6)
+        hierarchies.expect_hierarchy_at(work_package_1, work_package_2)
+        hierarchies.expect_leaf_at(work_package_3, work_package_4, work_package_5, work_package_6)
+      end
+    end
   end
 
   describe 'group mode' do

spec/support/pages/work_packages/work_packages_table.rb

@@ -194,7 +194,7 @@ module Pages
     end
 
     def drag_and_drop_work_package(from:, to:)
-      # Wait a bit because drag & drop in selenium is easily offendedA
+      # Wait a bit because drag & drop in selenium is easily offended
       sleep 1
 
       rows = page.all('.wp-table--row')
@@ -238,7 +238,7 @@ module Pages
         .release
         .perform
 
-      # Wait a bit because drag & drop in selenium is easily offendedA
+      # Wait a bit because drag & drop in selenium is easily offended
       sleep 1
     end