From 3a694f7ccb9633c8ab6a7501bfa25be9ac0f8237 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Tue, 16 Jul 2019 11:38:44 +0200 Subject: [PATCH] Add spec for delta builder --- frontend/karma.conf.js | 3 +- .../reorder-delta-builder.spec.ts | 244 ++++++++++++++++++ .../drag-and-drop/reorder-delta-builder.ts | 46 ++-- 3 files changed, 275 insertions(+), 18 deletions(-) create mode 100644 frontend/src/app/modules/common/drag-and-drop/reorder-delta-builder.spec.ts diff --git a/frontend/karma.conf.js b/frontend/karma.conf.js index 7555bf1067..32fe197906 100644 --- a/frontend/karma.conf.js +++ b/frontend/karma.conf.js @@ -16,7 +16,6 @@ module.exports = function (config) { plugins: [ require('karma-jasmine'), require('karma-chrome-launcher'), - require('karma-jasmine-html-reporter'), require('karma-coverage-istanbul-reporter'), require('@angular-devkit/build-angular/plugins/karma') ], @@ -30,7 +29,7 @@ module.exports = function (config) { angularCli: { environment: 'dev' }, - reporters: ['progress', 'kjhtml'], + reporters: ['progress'], port: 9876, colors: true, failOnEmptyTestSuite: true, diff --git a/frontend/src/app/modules/common/drag-and-drop/reorder-delta-builder.spec.ts b/frontend/src/app/modules/common/drag-and-drop/reorder-delta-builder.spec.ts new file mode 100644 index 0000000000..78e35a91fc --- /dev/null +++ b/frontend/src/app/modules/common/drag-and-drop/reorder-delta-builder.spec.ts @@ -0,0 +1,244 @@ +// -- copyright +// OpenProject is a project management system. +// Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License version 3. +// +// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +// Copyright (C) 2006-2013 Jean-Philippe Lang +// Copyright (C) 2010-2013 the ChiliProject Team +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License +// as published by the Free Software Foundation; either version 2 +// of the License, or (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program; if not, write to the Free Software +// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +// +// See doc/COPYRIGHT.rdoc for more details. +// ++ + +import {QueryOrder} from "core-app/modules/hal/dm-services/query-order-dm.service"; +import { + DEFAULT_ORDER, + MAX_ORDER, + ReorderDeltaBuilder +} from "core-app/modules/common/drag-and-drop/reorder-delta-builder"; + +describe('ReorderDeltaBuilder', () => { + const work_packages:string[] = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10']; + + function buildDelta( + wpId:string, + positions:QueryOrder, + wps:string[] = work_packages, + fromIndex:number|null = null) { + // As work_packages is already the list with moved element, simply compute the index + let index:number = work_packages.indexOf(wpId); + + if (index === -1) { + throw "Invalid wpId given for work_packages, must be contained."; + } + return new ReorderDeltaBuilder(wps, positions, wpId, index, fromIndex).buildDelta(); + } + + it('Empty, inserting at beginning sets the delta for wpId 1 to the default value', () => { + let delta = buildDelta('1', {}); + expect(Object.keys(delta)).toEqual(['1']); + expect(delta['1']).toEqual(DEFAULT_ORDER); + }); + + it('Empty, inserting at end sets the delta for all predecessors', () => { + let delta = buildDelta('10', {}); + expect(Object.keys(delta).length).toEqual(work_packages.length); + expect(delta).toEqual({ + '1': 0, + '2': 16384, + '3': 32768, + '4': 49152, + '5': 65536, + '6': 81920, + '7': 98304, + '8': 114688, + '9': 131072, + '10': 139264 // 131072 + ORDER_DISTANCE/2 + }); + }); + + it('Empty, inserting at end middle the delta for all predecessors', () => { + let delta = buildDelta('5', {}); + expect(Object.keys(delta).length).toEqual(5); + expect(delta).toEqual({ + '1': 0, + '2': 16384, + '3': 32768, + '4': 49152, + '5': 57344 // 49152 + 8192 + }); + }); + + it('has no problems inserting in the beginning old sort oder (1..n)', () => { + let positions = { + '5': 0, + '2': 1, + '3': 2, + '4': 3, + '1': 4, + '6': 5, + '7': 6, + '8': 7, + '9': 8, + '10': 9, + }; + + let delta = buildDelta('1', positions); + + expect(Object.keys(delta).length).toEqual(1); + // Expected to set to 1(position of 2) - 8192(half default distance) + expect(delta['1']).toEqual(-8191); + }); + + it('has no problems inserting in the middle of old sort oder (1..n)', () => { + let positions = { + '1': 0, + '2': 1, + '3': 2, + '4': 4, + '6': 5, + '7': 6, + '8': 7, + '9': 8, + '10': 9, + }; + + let delta = buildDelta('5', positions); + + // 0 does not have to be updated + expect(Object.keys(delta).length).toEqual(9); + expect(delta).toEqual({ + '2': 1, + '3': 2, + '4': 3, + '5': 4, + '6': 5, + '7': 6, + '8': 7, + '9': 8, + '10': 9 + }); + }); + + it('will reorder old sort if there is not enough data', () => { + let positions = { + '1': 0, + '3': 1, + '4': 2, + }; + + let delta = buildDelta('2', positions); + + expect(Object.keys(delta).length).toEqual(9); + expect(delta).toEqual({ + '2': 1, + '3': 2, + '4': 3, + '5': 4, + '6': 5, + '7': 6, + '8': 7, + '9': 8, + '10': 9, + }); + }); + + it('will shift min position when successor is max', () => { + let positions = { + '1': DEFAULT_ORDER, + '2': MAX_ORDER - 1, + '4': MAX_ORDER, + }; + + let delta = buildDelta('3', positions, ['1', '2', '3', '4']); + expect(Object.keys(delta).length).toEqual(3); + expect(delta).toEqual({ + // 1 remains at DEFAULT_ORDER + // rest is evenly spaced until MAX + '2': 715827882, + // 715827882 * 2 + '3': 1431655764, + // 715827882 * 3 + // gets reassigned due to integer floor() + '4': 2147483646 + }); + }); + + it('will shift first position back when predecessor is max', () => { + let positions = { + '1': MAX_ORDER - 1, + '2': MAX_ORDER, + }; + + let delta = buildDelta('3', positions, ['1', '2', '3']); + expect(Object.keys(delta).length).toEqual(3); + expect(delta).toEqual({ + '1': MAX_ORDER - 4, + '2': MAX_ORDER - 2, + '3': MAX_ORDER + }); + }); + + it('with successor position, sets the delta for wpId 1 to the default value', () => { + let positions = { '2': DEFAULT_ORDER }; + let delta = buildDelta('1', positions); + + expect(Object.keys(delta)).toEqual(['1']); + // expect position to be ORDER_DISTANCE/2 before successor position + expect(delta['1']).toEqual(-8192); + }); + + it('fills in all predecessors when inserting at index > 0', () => { + let positions = {}; + let delta = buildDelta('2', positions); + + expect(Object.keys(delta)).toEqual(['1', '2']); + // expect position to be ORDER_DISTANCE/2 before successor position + expect(delta['1']).toEqual(0); + expect(delta['2']).toEqual(8192); + }); + + it('just sets the default when order contains wpId only', () => { + let positions = {}; + let delta = buildDelta('1', positions, ['1']); + + expect(Object.keys(delta)).toEqual(['1']); + expect(delta['1']).toEqual(0); + }); + + it('just shifts two values when index <- 1 -> fromIndex', () => { + let positions = { '1': 8192, '2': 0, '3': 16384}; + // From index 1 to 0 + let delta = buildDelta('1', positions, ['1', '2', '3'], 1); + + expect(Object.keys(delta)).toEqual(['1', '2']); + expect(delta['1']).toEqual(0); + expect(delta['2']).toEqual(8192); + }); + + // Move to where successor has undefined position + it('adds to the predecessor if successor has no position', () => { + let positions = { '1': 0 }; + let delta = buildDelta('2', positions, ['1', '2', '3']); + + expect(Object.keys(delta)).toEqual(['2']); + expect(delta['2']).toEqual(8192); + }); + +}); diff --git a/frontend/src/app/modules/common/drag-and-drop/reorder-delta-builder.ts b/frontend/src/app/modules/common/drag-and-drop/reorder-delta-builder.ts index 6d0f808d09..2080fec162 100644 --- a/frontend/src/app/modules/common/drag-and-drop/reorder-delta-builder.ts +++ b/frontend/src/app/modules/common/drag-and-drop/reorder-delta-builder.ts @@ -1,5 +1,6 @@ import {QueryOrder} from "core-app/modules/hal/dm-services/query-order-dm.service"; import {debugLog, timeOutput} from "core-app/helpers/debug_output"; +import {debug} from "util"; // min allowed position export const MIN_ORDER = -2147483647; @@ -38,7 +39,7 @@ export class ReorderDeltaBuilder { } public buildDelta():QueryOrder { - timeOutput("Building delta", () => this.buildInsertPosition()); + timeOutput(`Building delta for ${this.wpId}@${this.index}`, () => this.buildInsertPosition()); debugLog("Order DELTA was built as %O", this.delta); @@ -66,16 +67,28 @@ export class ReorderDeltaBuilder { // Ensure previous positions exist so we can insert wpId @ index const predecessorPosition = this.buildUpPredecessorPosition(); + // Ensure we reorder when predecessor is at max already + if (predecessorPosition >= MAX_ORDER) { + debugLog(`Predecessor position is at max order, need to reorder`); + return this.reorderedInsert(); + } + // Get the actual successor position, it might vary wildly from the optimal position const successorPosition = this.positionFor(this.index + 1); if (successorPosition === undefined) { // Successor does not have a position yet (is NULL), any position will work - // so let's use the optimal one. + // so let's use the optimal one which is halfway to a potential successor this.delta[this.wpId] = predecessorPosition + (ORDER_DISTANCE / 2); return; } + // Ensure we reorder when successor is at max already + if (successorPosition >= MAX_ORDER) { + debugLog(`Successor position is at max order, need to reorder`); + return this.reorderedInsert(); + } + // successor exists and has a position // We will want to insert at the half way from predecessorPosition ... successorPosition const distance = Math.floor((successorPosition - predecessorPosition) / 2); @@ -177,19 +190,24 @@ export class ReorderDeltaBuilder { // Get the current distance between orders // Both must be set by now due to +buildUpPredecessorPosition+ having run. - let min = this.minPosition!; - let max = this.maxPosition!; + let [minWpId, min] = this.minPosition!; + let [maxWpId, max] = this.maxPosition!; + // We can keep min and max orders if distance/(items to distribute) >= 1 let space = Math.floor((max - min) / (itemsToDistribute - 1)); + debugLog(`Min=${min} Max=${max}, items=${itemsToDistribute}, space=${space}`) + // If no space is left, first try to add to the max item // Or subtract from the min item if (space < 1) { if ((max + itemsToDistribute) <= MAX_ORDER) { max += itemsToDistribute; + this.delta[maxWpId] = max; } else if ((min - itemsToDistribute) >= MIN_ORDER) { min -= itemsToDistribute; + this.delta[minWpId] = min; } else { // This should not happen in a 4-byte integer with our frontend throw "Elements cannot be moved further and no space is left. Too many elements"; @@ -199,16 +217,11 @@ export class ReorderDeltaBuilder { space = Math.floor((max - min) / (itemsToDistribute - 1)); } + debugLog(`Min=${min} Max=${max}, items=${itemsToDistribute}, space=${space}`) + // Assign positions for all values in between min/max for (let i = 1; i < itemsToDistribute; i++) { const wpId = this.order[i]; - - // If we reached a point where the position is undefined - // and larger than our point of insertion, we can keep them this way - if (i > this.index && this.livePosition(wpId) === undefined) { - return; - } - this.delta[wpId] = min + (i * space); } } @@ -216,22 +229,23 @@ export class ReorderDeltaBuilder { /** * Returns the minimal position assigned currently */ - private get minPosition():number|undefined { + private get minPosition():[string, number] { const wpId = this.order[0]!; - return this.livePosition(wpId); + return [wpId, this.livePosition(wpId)!]; } /** * Returns the maximum position assigned currently. * Note that a list can be unpositioned at the beginning, so this may return undefined */ - private get maxPosition():number|undefined { + private get maxPosition():[string, number]|undefined { for (let i = this.order.length - 1; i >= 0; i--) { - let position = this.livePosition(this.order[i]); + let wpId = this.order[i]; + let position = this.livePosition(wpId); // Return the first set position. if (position !== undefined) { - return position; + return [wpId, position]; } }