From e09ea6b9315e65f012ffae7a86462a7831de910a Mon Sep 17 00:00:00 2001 From: Henriette Dinger Date: Fri, 31 Jan 2020 10:48:04 +0100 Subject: [PATCH 1/3] Change determineParent algorithm. If the previous element is an open root, it gets the new parent. Otherwise, the parent of the previous element becomes the parent of the new parent of the dropped element. --- .../helpers/wp-table-row-helpers.ts | 4 ++ .../actions/hierarchy-drag-action.service.ts | 62 ++++++++++++++----- 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/frontend/src/app/components/wp-fast-table/helpers/wp-table-row-helpers.ts b/frontend/src/app/components/wp-fast-table/helpers/wp-table-row-helpers.ts index f63c014c7b..80cf136148 100644 --- a/frontend/src/app/components/wp-fast-table/helpers/wp-table-row-helpers.ts +++ b/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)); } diff --git a/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts b/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts index 2c8840a1c0..c5527914cd 100644 --- a/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts +++ b/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,63 @@ export class HierarchyDragActionService extends TableDragActionService { } public handleDrop(workPackage:WorkPackageResource, el:HTMLElement):Promise { - 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 { let previous = el.previousElementSibling; - 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) { + return Promise.resolve(null); + } - // 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}; + // 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; + } + + // 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 Promise.resolve(null); + } + + // If the sibling is a hierarchy root, return this one as new parent + let previousWpId = (previous as HTMLElement).dataset.workPackageId!; + if (previous.classList.contains(hierarchyRootClass(previousWpId))) { + return Promise.resolve(previousWpId); + } + + // If the sibling is no hierarchy root, return it's parent. + // Thus, the dropped element will get the same hierarchy level as the sibling + return this.wpCacheService.require(previousWpId) + .then((wp:WorkPackageResource) => { + return Promise.resolve(wp.parent.id); + }); + } + 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; } - } From 64872e5cdb1f2756a3ad108595b309b3ba513619 Mon Sep 17 00:00:00 2001 From: Henriette Dinger Date: Fri, 31 Jan 2020 15:21:39 +0100 Subject: [PATCH 2/3] Add test for new ancestor algorithm during drag'n'drop in hierarchy mode --- .../actions/hierarchy-drag-action.service.ts | 12 ++-- .../sorting/manual_sorting_spec.rb | 64 +++++++++++++++++-- .../work_packages/work_packages_table.rb | 4 +- 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts b/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts index c5527914cd..86ba727114 100644 --- a/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts +++ b/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts @@ -69,10 +69,7 @@ export class HierarchyDragActionService extends TableDragActionService { // If the sibling is no hierarchy root, return it's parent. // Thus, the dropped element will get the same hierarchy level as the sibling - return this.wpCacheService.require(previousWpId) - .then((wp:WorkPackageResource) => { - return Promise.resolve(wp.parent.id); - }); + return this.loadParentOfWP(previousWpId); } private findRelationRowRoot(el:Element):Element|null { @@ -86,4 +83,11 @@ export class HierarchyDragActionService extends TableDragActionService { return null; } + + private loadParentOfWP(wpId:string):Promise { + return this.wpCacheService.require(wpId) + .then((wp:WorkPackageResource) => { + return Promise.resolve(wp.parent.id); + }); + } } diff --git a/spec/features/work_packages/sorting/manual_sorting_spec.rb b/spec/features/work_packages/sorting/manual_sorting_spec.rb index eee4e1b3af..51107792b7 100644 --- a/spec/features/work_packages/sorting/manual_sorting_spec.rb +++ b/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 diff --git a/spec/support/pages/work_packages/work_packages_table.rb b/spec/support/pages/work_packages/work_packages_table.rb index 1d1f2d67b6..acc0201b21 100644 --- a/spec/support/pages/work_packages/work_packages_table.rb +++ b/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 From deb38cf5009ea33331abb452f8d307c31d171b60 Mon Sep 17 00:00:00 2001 From: Henriette Dinger Date: Tue, 4 Feb 2020 16:44:19 +0100 Subject: [PATCH 3/3] Refactor determineParent method to make it easier readable and satisfy rubocop --- .../actions/hierarchy-drag-action.service.ts | 60 ++++++++++--------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts b/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts index 86ba727114..0bcef4cb88 100644 --- a/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts +++ b/frontend/src/app/components/wp-table/drag-and-drop/actions/hierarchy-drag-action.service.ts @@ -38,38 +38,33 @@ export class HierarchyDragActionService extends TableDragActionService { */ private determineParent(el:HTMLElement):Promise { let previous = el.previousElementSibling; + var parent = null; - if (previous === null) { - return Promise.resolve(null); - } - - // 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); + 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; } - previous = relationRoot; - } - - // 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 Promise.resolve(null); - } - // If the sibling is a hierarchy root, return this one as new parent - let previousWpId = (previous as HTMLElement).dataset.workPackageId!; - if (previous.classList.contains(hierarchyRootClass(previousWpId))) { - return Promise.resolve(previousWpId); + 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); + } } - // If the sibling is no hierarchy root, return it's parent. - // Thus, the dropped element will get the same hierarchy level as the sibling - return this.loadParentOfWP(previousWpId); + return Promise.resolve(parent); } private findRelationRowRoot(el:Element):Element|null { @@ -84,6 +79,17 @@ export class HierarchyDragActionService extends TableDragActionService { 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 { return this.wpCacheService.require(wpId) .then((wp:WorkPackageResource) => {