From 249a09b68aa38e8aa0b21e30bdf26ec809c5f685 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 04:05:31 +0100 Subject: [PATCH 1/8] [AGILE-363] Resolve batch positions Establishes one DOM resolution for availability and execution. Validates contiguous blocks and explicit unavailable reasons while preserving singular placement and sparse-list boundaries. https://community.openproject.org/wp/AGILE-363 --- .../sortable-lists/item.controller.spec.ts | 26 ++-- .../dynamic/sortable-lists/list-dom.spec.ts | 140 ++++++++++++++++++ .../dynamic/sortable-lists/list-dom.ts | 117 +++++++++++++++ 3 files changed, 270 insertions(+), 13 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index 00d9a5e0814b..feae5e035a8d 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts @@ -105,8 +105,8 @@ describe('Sortable lists item controller', () => { return { element, busy, - actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), - selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + actionScopeFor: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), availableDestinations: vi.fn(() => []), moveToDestination: vi.fn(), moveInDirection: vi.fn(), @@ -1124,8 +1124,8 @@ describe('Sortable lists item controller', () => { controller.connectRoot({ element: row, busy: false, - actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), - selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + actionScopeFor: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), availableDestinations: vi.fn(() => []), moveToDestination: vi.fn(), moveInDirection: vi.fn(), @@ -1222,8 +1222,8 @@ describe('Sortable lists item controller', () => { controller.connectRoot({ element: row, busy: false, - actionScopeFor: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), - selectForAction: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + actionScopeFor: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), availableDestinations: vi.fn(() => []), moveToDestination: vi.fn(), moveInDirection: vi.fn(), @@ -1415,8 +1415,8 @@ describe('Sortable lists item controller', () => { ) => ({ element: el, busy: false, - actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), - selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + actionScopeFor: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), availableDestinations: vi.fn(() => []), moveToDestination: vi.fn(), moveAvailability: () => availability, @@ -1424,7 +1424,7 @@ describe('Sortable lists item controller', () => { } as unknown as SortableListsRoot); function stubMenuRoot(el:HTMLElement, position:{ isFirst:boolean; isLast:boolean }) { - const actionScopeFor = vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })); + const actionScopeFor = vi.fn(():ActionScope => ({ kind: 'refused', items: [] })); const availableDestinations = vi.fn((_scope:ActionScope, _candidates:DestinationIdentity[]):DestinationIdentity[] => []); const root = { ...stubRoot(el, position), actionScopeFor, availableDestinations }; @@ -1909,8 +1909,8 @@ describe('Sortable lists item controller', () => { const root:SortableListsRoot = { element: item, busy: false, - actionScopeFor: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), - selectForAction: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + actionScopeFor: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), availableDestinations: vi.fn(() => []), moveToDestination: vi.fn(), moveInDirection: vi.fn(), @@ -1942,8 +1942,8 @@ describe('Sortable lists item controller', () => { const root:SortableListsRoot = { element: item, busy: false, - actionScopeFor: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), - selectForAction: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + actionScopeFor: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), availableDestinations: vi.fn(() => []), moveToDestination: vi.fn(), moveInDirection: vi.fn(), diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts index 84a6331051d9..574ead8732bd 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts @@ -33,6 +33,8 @@ import { itemMobility, permittedDestinations, reorderRows, + resolveBlockMove, + resolveBlockMoveAvailability, sortableItemMobilityAttribute, resolveDirectionalPreviousItemId, resolveMoveAvailability, @@ -661,6 +663,144 @@ describe('directional move helpers', () => { }); }); +describe('block move helpers', () => { + function fixture():{ + rowsContainer:HTMLUListElement; + items:HTMLLIElement[]; + marker:HTMLLIElement; + divider:HTMLLIElement; + } { + const rowsContainer = document.createElement('ul'); + const items = ['1', '2', '3', '4'].map((id) => { + const row = document.createElement('li'); + row.setAttribute('data-sortable-lists--item-id-value', id); + return row; + }); + const marker = document.createElement('li'); + marker.setAttribute('data-sortable-lists-prev-item-id', 'last-hidden'); + marker.setAttribute('data-sortable-lists-omitted-count', '3'); + const divider = document.createElement('li'); + divider.classList.add('divider'); + + rowsContainer.append(...items, marker, divider); + + return { rowsContainer, items, marker, divider }; + } + + it('resolves one card in all four directions', () => { + const { rowsContainer, items: [, second] } = fixture(); + + expect(resolveBlockMove({ itemElements: [second], direction: 'top', rowsContainer })) + .toEqual({ available: true, rows: [second], previousItemId: null }); + expect(resolveBlockMove({ itemElements: [second], direction: 'up', rowsContainer })) + .toEqual({ available: true, rows: [second], previousItemId: null }); + expect(resolveBlockMove({ itemElements: [second], direction: 'down', rowsContainer })) + .toEqual({ available: true, rows: [second], previousItemId: '3' }); + expect(resolveBlockMove({ itemElements: [second], direction: 'bottom', rowsContainer })) + .toEqual({ available: true, rows: [second], previousItemId: '4' }); + }); + + it('resolves an adjacent block in all four directions and excludes selected predecessors', () => { + const { rowsContainer, items: [, second, third] } = fixture(); + + expect(resolveBlockMove({ itemElements: [second, third], direction: 'top', rowsContainer })) + .toEqual({ available: true, rows: [second, third], previousItemId: null }); + expect(resolveBlockMove({ itemElements: [second, third], direction: 'up', rowsContainer })) + .toEqual({ available: true, rows: [second, third], previousItemId: null }); + expect(resolveBlockMove({ itemElements: [second, third], direction: 'down', rowsContainer })) + .toEqual({ available: true, rows: [second, third], previousItemId: '4' }); + expect(resolveBlockMove({ itemElements: [second, third], direction: 'bottom', rowsContainer })) + .toEqual({ available: true, rows: [second, third], previousItemId: '4' }); + }); + + it('rejects an empty or fixed selection as not orderable', () => { + const { rowsContainer, items: [first, second] } = fixture(); + second.setAttribute(sortableItemMobilityAttribute, 'fixed'); + + expect(resolveBlockMove({ itemElements: [], direction: 'top', rowsContainer })) + .toEqual({ available: false, reason: 'not-orderable' }); + expect(resolveBlockMove({ itemElements: [first, second], direction: 'top', rowsContainer })) + .toEqual({ available: false, reason: 'not-orderable' }); + }); + + it('rejects selected elements from different containers', () => { + const { rowsContainer, items: [first] } = fixture(); + const { items: [foreign] } = fixture(); + + expect(resolveBlockMove({ itemElements: [first, foreign], direction: 'top', rowsContainer })) + .toEqual({ available: false, reason: 'cross-list' }); + }); + + it('rejects sparse, reverse-order, and duplicate input', () => { + const { rowsContainer, items: [first, second, third] } = fixture(); + + expect(resolveBlockMove({ itemElements: [first, third], direction: 'top', rowsContainer })) + .toEqual({ available: false, reason: 'non-contiguous' }); + expect(resolveBlockMove({ itemElements: [third, second], direction: 'top', rowsContainer })) + .toEqual({ available: false, reason: 'non-contiguous' }); + expect(resolveBlockMove({ itemElements: [second, second], direction: 'top', rowsContainer })) + .toEqual({ available: false, reason: 'non-contiguous' }); + }); + + it('rejects up and down across an adjacent truncation marker', () => { + const { rowsContainer, items: [, second, third], marker } = fixture(); + + second.after(marker); + expect(resolveBlockMove({ itemElements: [second], direction: 'down', rowsContainer })) + .toEqual({ available: false, reason: 'truncation-boundary' }); + expect(resolveBlockMove({ itemElements: [third], direction: 'up', rowsContainer })) + .toEqual({ available: false, reason: 'truncation-boundary' }); + }); + + it('rejects up and down across an unaddressable gap', () => { + const { rowsContainer, items: [, second, third], divider } = fixture(); + + second.after(divider); + expect(resolveBlockMove({ itemElements: [second], direction: 'down', rowsContainer })) + .toEqual({ available: false, reason: 'unaddressable-gap' }); + expect(resolveBlockMove({ itemElements: [third], direction: 'up', rowsContainer })) + .toEqual({ available: false, reason: 'unaddressable-gap' }); + }); + + it('keeps top and bottom available across loaded sparse-list boundaries', () => { + const { rowsContainer, items: [, second, third, fourth], marker } = fixture(); + + second.after(marker); + expect(resolveBlockMove({ itemElements: [third], direction: 'top', rowsContainer })) + .toEqual({ available: true, rows: [third], previousItemId: null }); + expect(resolveBlockMove({ itemElements: [second], direction: 'bottom', rowsContainer })) + .toEqual({ available: true, rows: [second], previousItemId: '4' }); + expect(resolveBlockMove({ itemElements: [third], direction: 'bottom', rowsContainer })) + .toEqual({ available: true, rows: [third], previousItemId: '4' }); + expect(resolveBlockMove({ itemElements: [fourth], direction: 'top', rowsContainer })) + .toEqual({ available: true, rows: [fourth], previousItemId: null }); + }); + + it('rejects every move that returns the block to its effective placement', () => { + const { rowsContainer, items: [first, second, third, fourth] } = fixture(); + + expect(resolveBlockMove({ itemElements: [first], direction: 'top', rowsContainer })) + .toEqual({ available: false, reason: 'no-op' }); + expect(resolveBlockMove({ itemElements: [first, second], direction: 'up', rowsContainer })) + .toEqual({ available: false, reason: 'no-op' }); + expect(resolveBlockMove({ itemElements: [fourth], direction: 'down', rowsContainer })) + .toEqual({ available: false, reason: 'no-op' }); + expect(resolveBlockMove({ itemElements: [third, fourth], direction: 'bottom', rowsContainer })) + .toEqual({ available: false, reason: 'no-op' }); + }); + + it('derives availability by resolving all four directions', () => { + const { rowsContainer, items: [first, second, third, fourth] } = fixture(); + + expect(resolveBlockMoveAvailability({ itemElements: [first, second], rowsContainer })) + .toEqual({ top: false, up: false, down: true, bottom: true }); + expect(resolveBlockMoveAvailability({ itemElements: [second, third], rowsContainer })) + .toEqual({ top: true, up: true, down: true, bottom: true }); + expect(resolveBlockMoveAvailability({ itemElements: [third, fourth], rowsContainer })) + .toEqual({ top: true, up: true, down: false, bottom: false }); + }); +}); + describe('resolveItemPosition', () => { function item(id:string, label?:string):HTMLLIElement { const row = document.createElement('li'); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts index 45ec856aade2..a5f32a732d46 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -450,6 +450,123 @@ export function resolveDirectionalPreviousItemId({ export type MoveAvailability = Record; +export interface BlockMoveResolution { + available:true; + rows:HTMLElement[]; + previousItemId:string|null; +} + +export type BlockMoveUnavailableReason = + | 'not-orderable' + | 'cross-list' + | 'non-contiguous' + | 'truncation-boundary' + | 'unaddressable-gap' + | 'no-op'; + +export interface BlockMoveUnavailable { + available:false; + reason:BlockMoveUnavailableReason; +} + +export type BlockMoveResult = BlockMoveResolution|BlockMoveUnavailable; + +export function resolveBlockMove({ + itemElements, + direction, + rowsContainer, +}:{ + itemElements:HTMLElement[]; + direction:MoveDirection; + rowsContainer:HTMLElement; +}):BlockMoveResult { + const unavailable = (reason:BlockMoveUnavailableReason):BlockMoveUnavailable => ({ available: false, reason }); + if (itemElements.length === 0 || itemElements.some((item) => !isOrderableItem(item))) { + return unavailable('not-orderable'); + } + + const rows = listRows(rowsContainer); + const selectedRows = itemElements.map((item) => rowOf(rowsContainer, item)); + if (selectedRows.some((row) => row === null)) { + return unavailable('cross-list'); + } + + const block = selectedRows as HTMLElement[]; + const indexes = block.map((row) => rows.indexOf(row)); + if ( + new Set(block).size !== block.length || + indexes.some((index, offset) => offset > 0 && index !== indexes[offset - 1] + 1) + ) { + return unavailable('non-contiguous'); + } + + const itemRows = rows.filter((row) => isItemRow(row, rowsContainer)); + const itemIndexes = block.map((row) => itemRows.indexOf(row)); + if (itemIndexes.some((index, offset) => index < 0 || (offset > 0 && index !== itemIndexes[offset - 1] + 1))) { + return unavailable('non-contiguous'); + } + + const firstRowIndex = indexes[0]; + const lastRowIndex = indexes[indexes.length - 1]; + const firstItemIndex = itemIndexes[0]; + const lastItemIndex = itemIndexes[itemIndexes.length - 1]; + + let previousItemId:string|null|undefined; + switch (direction) { + case 'top': + previousItemId = firstItemIndex === 0 ? undefined : null; + break; + case 'bottom': { + if (lastItemIndex === itemRows.length - 1) return unavailable('no-op'); + const selected = new Set(block); + const anchor = [...itemRows].reverse().find((row) => !selected.has(row as HTMLElement)); + const anchorId = anchor ? resolvePreviousItemId(anchor, rowsContainer) : null; + previousItemId = anchorId ?? undefined; + break; + } + case 'up': { + if (firstItemIndex === 0) return unavailable('no-op'); + const adjacent = rows[firstRowIndex - 1]; + if (!isItemRow(adjacent, rowsContainer)) { + return unavailable(adjacent && rowOmittedCount(adjacent) > 0 ? 'truncation-boundary' : 'unaddressable-gap'); + } + const anchor = rows[firstRowIndex - 2]; + if (anchor && !isAddressableRow(anchor, rowsContainer)) return unavailable('unaddressable-gap'); + previousItemId = anchor ? resolvePreviousItemId(anchor, rowsContainer) : null; + break; + } + case 'down': { + if (lastItemIndex === itemRows.length - 1) return unavailable('no-op'); + const adjacent = rows[lastRowIndex + 1]; + if (!isItemRow(adjacent, rowsContainer)) { + return unavailable(adjacent && rowOmittedCount(adjacent) > 0 ? 'truncation-boundary' : 'unaddressable-gap'); + } + const adjacentId = resolvePreviousItemId(adjacent, rowsContainer); + previousItemId = adjacentId ?? undefined; + break; + } + } + + return previousItemId === undefined + ? unavailable(firstItemIndex === 0 || lastItemIndex === itemRows.length - 1 ? 'no-op' : 'unaddressable-gap') + : { available: true, rows: block, previousItemId }; +} + +export function resolveBlockMoveAvailability({ + itemElements, + rowsContainer, +}:{ + itemElements:HTMLElement[]; + rowsContainer:HTMLElement; +}):MoveAvailability { + return { + top: resolveBlockMove({ itemElements, rowsContainer, direction: 'top' }).available, + up: resolveBlockMove({ itemElements, rowsContainer, direction: 'up' }).available, + down: resolveBlockMove({ itemElements, rowsContainer, direction: 'down' }).available, + bottom: resolveBlockMove({ itemElements, rowsContainer, direction: 'bottom' }).available, + }; +} + // Availability of all four directional moves for the item, or null when it is // not (yet) a row of the container. The row scan happens once; the four // per-direction resolutions only index into it. From b9007dfa2e533fc0a8f0b817ebcda3353a0a4567 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 04:11:49 +0100 Subject: [PATCH 2/8] [AGILE-363] Validate block row ownership Verifies that every resolved row owns its supplied item and prevents nested lists from substituting an outer host row. https://community.openproject.org/wp/AGILE-363 --- .../dynamic/sortable-lists/list-dom.spec.ts | 31 +++++++++++++++++++ .../dynamic/sortable-lists/list-dom.ts | 4 ++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts index 574ead8732bd..122a52d99364 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts @@ -731,6 +731,26 @@ describe('block move helpers', () => { .toEqual({ available: false, reason: 'cross-list' }); }); + it('rejects an inner-list item that resolves to its outer host row', () => { + const rowsContainer = document.createElement('ul'); + const outerHost = document.createElement('li'); + outerHost.setAttribute('data-sortable-lists--item-id-value', 'outer-1'); + const nestedRows = document.createElement('ul'); + const inner = document.createElement('li'); + inner.setAttribute('data-sortable-lists--item-id-value', 'inner-1'); + nestedRows.append(inner); + outerHost.append(nestedRows); + + const outerSecond = document.createElement('li'); + outerSecond.setAttribute('data-sortable-lists--item-id-value', 'outer-2'); + const outerThird = document.createElement('li'); + outerThird.setAttribute('data-sortable-lists--item-id-value', 'outer-3'); + rowsContainer.append(outerHost, outerSecond, outerThird); + + expect(resolveBlockMove({ itemElements: [inner, outerSecond], direction: 'bottom', rowsContainer })) + .toEqual({ available: false, reason: 'cross-list' }); + }); + it('rejects sparse, reverse-order, and duplicate input', () => { const { rowsContainer, items: [first, second, third] } = fixture(); @@ -762,6 +782,17 @@ describe('block move helpers', () => { .toEqual({ available: false, reason: 'unaddressable-gap' }); }); + it('keeps fixed rows addressable as up and down neighbours', () => { + const { rowsContainer, items: [first, second, third] } = fixture(); + first.setAttribute(sortableItemMobilityAttribute, 'fixed'); + third.setAttribute(sortableItemMobilityAttribute, 'fixed'); + + expect(resolveBlockMove({ itemElements: [second], direction: 'up', rowsContainer })) + .toEqual({ available: true, rows: [second], previousItemId: null }); + expect(resolveBlockMove({ itemElements: [second], direction: 'down', rowsContainer })) + .toEqual({ available: true, rows: [second], previousItemId: '3' }); + }); + it('keeps top and bottom available across loaded sparse-list boundaries', () => { const { rowsContainer, items: [, second, third, fourth], marker } = fixture(); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts index a5f32a732d46..f07b59cdc20c 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -487,7 +487,9 @@ export function resolveBlockMove({ const rows = listRows(rowsContainer); const selectedRows = itemElements.map((item) => rowOf(rowsContainer, item)); - if (selectedRows.some((row) => row === null)) { + if (selectedRows.some((row, index) => ( + row === null || resolveItemElement(row, rowsContainer) !== itemElements[index] + ))) { return unavailable('cross-list'); } From 2d59f8112e9ee9b6a4efa25d580f0a34783c83db Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 04:16:52 +0100 Subject: [PATCH 3/8] [AGILE-363] Gate batch position actions Resolves availability over the invoker's prospective scope and exposes only moves the live block resolver can execute. Preserves destinations for contiguous same-list selections. https://community.openproject.org/wp/AGILE-363 --- .../dynamic/sortable-lists.controller.spec.ts | 82 ++++++++++++++++--- .../dynamic/sortable-lists.controller.ts | 15 ++-- .../dynamic/sortable-lists/drag-and-drop.ts | 3 +- .../sortable-lists/item.controller.spec.ts | 32 +++++++- .../dynamic/sortable-lists/item.controller.ts | 11 +-- 5 files changed, 116 insertions(+), 27 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index a56ae2f1942a..4fa9a51ef7ad 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -1209,7 +1209,7 @@ describe('Sortable lists controller', () => { }); it('reports per-direction move availability for gating', async () => { - const { root, firstSourceItem } = renderFixture(); + const { root, firstSourceItem } = renderSelectableRoot(); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; @@ -1219,16 +1219,78 @@ describe('Sortable lists controller', () => { }); }); - // A card that takes no part in ordering must not be offered a move the - // click path would then refuse. - it('reports no available direction for a non-movable item', async () => { - const { root, firstSourceItem } = renderFixture(); - firstSourceItem.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); - await ctx.nextFrame(); - const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + describe('batch move availability', () => { + const unavailable = { top: false, up: false, down: false, bottom: false }; + const select = (element:HTMLElement, init:MouseEventInit = {}) => { + element.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ...init })); + }; - expect(controller.moveAvailability(firstSourceItem)).toEqual({ - top: false, up: false, down: false, bottom: false, + it('reports the available directions for a selected contiguous block', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + select(items[1]); + select(items[2], { ctrlKey: true }); + + expect(controller.moveAvailability(items[1])).toEqual({ + top: true, up: true, down: false, bottom: false, + }); + }); + + it('reports every direction unavailable for a sparse selection', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + select(items[0]); + select(items[2], { ctrlKey: true }); + + expect(controller.moveAvailability(items[0])).toEqual(unavailable); + }); + + it('reports every direction unavailable for a cross-list selection', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + select(items[2]); + select(items[3], { ctrlKey: true }); + + expect(controller.moveAvailability(items[2])).toEqual(unavailable); + }); + + it('reports one-card availability for an unselected invoker without changing the selection', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + select(items[0]); + select(items[1], { ctrlKey: true }); + + expect(controller.moveAvailability(items[3])).toEqual({ + top: false, up: false, down: true, bottom: true, + }); + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0], items[1]]); + }); + + it('reports normal within-list availability for a confined contiguous block', async () => { + const { root, items } = renderSelectableRoot(); + items[0].setAttribute('data-sortable-lists--item-mobility-value', 'confined'); + items[1].setAttribute('data-sortable-lists--item-mobility-value', 'confined'); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + select(items[0]); + select(items[1], { ctrlKey: true }); + + expect(controller.moveAvailability(items[0])).toEqual({ + top: false, up: false, down: true, bottom: true, + }); + }); + + it('reports null for a fixed invoker', async () => { + const { root, items } = renderSelectableRoot(); + items[0].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + + expect(controller.moveAvailability(items[0])).toBeNull(); }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index cc32d62b7dfc..c85ed527e41d 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -54,11 +54,11 @@ import { itemAcceptsDestination, reorderRows, resolveDirectionalPreviousItemId, + resolveBlockMoveAvailability, resolveItemId, resolveItemLabel, resolveItemPosition, resolveItemType, - resolveMoveAvailability, restoreRowPositions, rowOf, rowsRemainAt, @@ -496,14 +496,19 @@ export default class SortableListsController extends Controller imp // gating; the click path re-resolves the live DOM. moveAvailability(itemElement:HTMLElement):MoveAvailability|null { if (!isOrderableItem(itemElement)) { - return { - top: false, up: false, down: false, bottom: false, - }; + return null; + } + + const scope = this.actionScopeFor(itemElement); + if (scope.kind === 'refused') { + return null; } const list = this.ownerListOf(itemElement); - return list ? resolveMoveAvailability({ itemElement, rowsContainer: list.rowsContainer }) : null; + return list + ? resolveBlockMoveAvailability({ itemElements: scope.items, rowsContainer: list.rowsContainer }) + : null; } moveToDestination(itemElement:HTMLElement, target:DestinationIdentity):void { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts index ac55430ddaa6..be4abb0184e2 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts @@ -108,7 +108,8 @@ export interface SortableListsRoot { availableDestinations(scope:ActionScope, candidates:DestinationIdentity[]):DestinationIdentity[]; moveToDestination(itemElement:HTMLElement, target:DestinationIdentity):void; moveInDirection(itemElement:HTMLElement, direction:MoveDirection):void; - // A snapshot for menu gating; the click path re-resolves against the live DOM. + // A snapshot for menu gating over the invoker's prospective action scope; + // the click path re-resolves against the live DOM. moveAvailability(itemElement:HTMLElement):MoveAvailability|null; // The element of the list an item currently belongs to; null outside any // registered list. Items carry no list reference, so the root resolves it. diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index feae5e035a8d..8b74bad3156a 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts @@ -1546,7 +1546,7 @@ describe('Sortable lists item controller', () => { expect(menu.showItem).toHaveBeenCalledWith(moveToInbox); }); - it('hides destination actions and the position submenu for a true multi-card scope', async () => { + it('shows available batch position directions when every destination action is hidden', async () => { const { el, menu } = renderItemWithMenu(1, true); document.body.appendChild(el); const controller = await mountItemController(el); @@ -1556,6 +1556,7 @@ describe('Sortable lists item controller', () => { const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); actionScopeFor.mockReturnValue(scope); availableDestinations.mockReturnValue([]); + root.moveAvailability = () => ({ top: false, up: true, down: true, bottom: false }); controller.connectRoot(root); const moveToSprint = destinationFor(el, [{ type: 'sprint', id: '1' }]); @@ -1566,8 +1567,35 @@ describe('Sortable lists item controller', () => { const divider = el.querySelector('li[data-sortable-lists--item-target="moveDivider"]')!; expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); expect(menu.hideItem).toHaveBeenCalledWith(moveToInbox); + expect(menu.showItem).toHaveBeenCalledWith(moveMenu); + expect(menu.hideItem).toHaveBeenCalledWith(liFor(el, 'top')); + expect(menu.showItem).toHaveBeenCalledWith(liFor(el, 'up')); + expect(menu.showItem).toHaveBeenCalledWith(liFor(el, 'down')); + expect(menu.hideItem).toHaveBeenCalledWith(liFor(el, 'bottom')); + expect(divider.hasAttribute('hidden')).toBe(false); + }); + + it('hides an all-unavailable batch position submenu and its directions', async () => { + const { el, menu } = renderItemWithMenu(1, true); + document.body.appendChild(el); + const controller = await mountItemController(el); + const scope:ActionScope = { kind: 'batch', items: [el] }; + const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); + actionScopeFor.mockReturnValue(scope); + availableDestinations.mockReturnValue([]); + root.moveAvailability = () => ({ top: false, up: false, down: false, bottom: false }); + controller.connectRoot(root); + + const moveToSprint = destinationFor(el, [{ type: 'sprint', id: '1' }]); + await menuCtx!.nextFrame(); + + const moveMenu = el.querySelector('li[data-sortable-lists--item-target="moveMenu"]')!; + const divider = el.querySelector('li[data-sortable-lists--item-target="moveDivider"]')!; + expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); expect(menu.hideItem).toHaveBeenCalledWith(moveMenu); - expect(menu.hideItem).not.toHaveBeenCalledWith(liFor(el, 'top')); + for (const direction of ['top', 'up', 'down', 'bottom']) { + expect(menu.hideItem).toHaveBeenCalledWith(liFor(el, direction)); + } expect(divider.hasAttribute('hidden')).toBe(true); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index 5663d551926b..ed9524bba5fc 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -525,7 +525,7 @@ export default class ItemController extends Controller implements R const scope = root.actionScopeFor(this.element); this.refreshDestinationAvailability(root, scope); - this.refreshMoveMenuAvailability(root, scope); + this.refreshMoveMenuAvailability(root); this.refreshMoveDivider(); } @@ -550,14 +550,7 @@ export default class ItemController extends Controller implements R } } - private refreshMoveMenuAvailability(root:SortableListsRoot, scope:ActionScope):void { - if (scope.kind === 'batch' && scope.items.length > 1) { - if (this.hasMoveMenuTarget) { - this.setAvailability(this.moveMenuTarget, false); - } - return; - } - + private refreshMoveMenuAvailability(root:SortableListsRoot):void { // Null availability means the item is not in a list yet; leave the menu // alone until the outlet wiring settles. const availability = root.moveAvailability(this.element); From 265be29f4ec9a736d8c250561ef7558fead0de09 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 04:24:00 +0100 Subject: [PATCH 4/8] [AGILE-363] Move selected blocks by menu Routes selection roots through the shared block resolver and moves the ordered block through the optimistic collection path. Keeps roots without selection on their singular member endpoint. https://community.openproject.org/wp/AGILE-363 --- .../dynamic/sortable-lists.controller.spec.ts | 195 ++++++++++++++++++ .../dynamic/sortable-lists.controller.ts | 55 ++++- 2 files changed, 249 insertions(+), 1 deletion(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 4fa9a51ef7ad..d35a2919a761 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -1195,6 +1195,201 @@ describe('Sortable lists controller', () => { expect(body.body.get('prev_id')).toBe('2'); }); + describe('batch menu moves', () => { + function renderBatchMenuFixture() { + const elements = renderSelectableRoot({ + optimistic: true, + collectionMoveUrl: '/projects/demo/backlogs/work_packages/move', + }); + elements.sourceList.setAttribute('data-sortable-lists--list-type-value', 'sprint'); + elements.sourceList.setAttribute('data-sortable-lists--list-id-value', '8'); + elements.sourceList.append(elements.items[3]); + + return elements; + } + + function selectItems(...items:HTMLElement[]):void { + items.forEach((item, index) => { + item.dispatchEvent(new MouseEvent('click', { + bubbles: true, + cancelable: true, + ctrlKey: index > 0, + })); + }); + } + + function selectedIds(root:HTMLElement):string[] { + return Array.from(root.querySelectorAll('[data-batch-selected]')) + .map((item) => item.getAttribute('data-sortable-lists--item-id-value')!); + } + + function controllerFor(root:HTMLElement):SortableListsControllerType { + return ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + } + + it('moves the ordered selection immediately and excludes it from the predecessor', async () => { + let resolveMove!:(response:Response) => void; + fetchMock.mockImplementationOnce(() => new Promise((resolve) => { + resolveMove = resolve; + })); + const { root, sourceList, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[1], items[2]); + + controllerFor(root).moveInDirection(items[1], 'down'); + + expect(itemIds(sourceList)).toEqual(['1', '4', '2', '3']); + const [url, options] = fetchMock.mock.lastCall as [string, { body:FormData }]; + expect(url).toBe('/projects/demo/backlogs/work_packages/move?optimistic=true'); + expect([...options.body.entries()]).toEqual([ + ['ids[]', '2'], + ['ids[]', '3'], + ['list_type', 'sprint'], + ['list_id', '8'], + ['prev_id', '4'], + ]); + + resolveMove(new Response('', { status: 200 })); + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + }); + + it('clears the selection after a successful collection move', async () => { + const { root, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[1], items[2]); + + controllerFor(root).moveInDirection(items[1], 'down'); + + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + expect(selectedIds(root)).toEqual([]); + }); + + it('rolls a rejected collection move back and preserves the selection', async () => { + fetchMock.mockResolvedValueOnce(new Response('', { status: 422 })); + const { root, sourceList, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[1], items[2]); + + controllerFor(root).moveInDirection(items[1], 'down'); + + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + expect(itemIds(sourceList)).toEqual(['1', '2', '3', '4']); + expect(selectedIds(root)).toEqual(['2', '3']); + }); + + it('warns when a rejected collection move cannot be rolled back safely', async () => { + let resolveMove!:(response:Response) => void; + fetchMock.mockImplementationOnce(() => new Promise((resolve) => { + resolveMove = resolve; + })); + const { root, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[1], items[2]); + + controllerFor(root).moveInDirection(items[1], 'down'); + items[2].remove(); + resolveMove(new Response('', { status: 422 })); + + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + expect(announceSpy).toHaveBeenCalledWith( + expect.stringContaining('Check the items'), + expect.objectContaining({ politeness: 'assertive' }), + ); + }); + + it('does not request a move when the invoker has no owned list', async () => { + const { root } = renderBatchMenuFixture(); + await ctx.nextFrame(); + + controllerFor(root).moveInDirection(itemRow('99'), 'down'); + await flushPromises(); + + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('does not request a move when the live selection is unavailable', async () => { + const { root, sourceList, targetList, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[1], items[4]); + + controllerFor(root).moveInDirection(items[1], 'down'); + await flushPromises(); + + expect(itemIds(sourceList)).toEqual(['1', '2', '3', '4']); + expect(itemIds(targetList)).toEqual(['5']); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + // A menu opened on an unselected card can go stale: by the time the item + // is activated its direction may be a no-op. The refusal must leave the + // batch the user built alone rather than replacing it with the invoker. + it('leaves the selection alone when an unselected invoker cannot move', async () => { + const { root, sourceList, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[1], items[2]); + + controllerFor(root).moveInDirection(items[0], 'top'); + await flushPromises(); + + expect(fetchMock).not.toHaveBeenCalled(); + expect(itemIds(sourceList)).toEqual(['1', '2', '3', '4']); + expect(selectedIds(root)).toEqual(['2', '3']); + }); + + it('does not request a no-op block move', async () => { + const { root, sourceList, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[0], items[1]); + + controllerFor(root).moveInDirection(items[0], 'top'); + await flushPromises(); + + expect(itemIds(sourceList)).toEqual(['1', '2', '3', '4']); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('uses the collection contract for an unselected one-card invoker', async () => { + const { root, sourceList, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + + controllerFor(root).moveInDirection(items[0], 'down'); + await flushPromises(); + + expect(itemIds(sourceList)).toEqual(['2', '1', '3', '4']); + const [url, options] = fetchMock.mock.lastCall as [string, { body:FormData }]; + expect(url).toBe('/projects/demo/backlogs/work_packages/move?optimistic=true'); + expect(options.body.getAll('ids[]')).toEqual(['1']); + }); + + it('preserves the selection and refuses the action while busy', async () => { + const { root, sourceList, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[1], items[2]); + root.setAttribute('data-sortable-lists-busy', 'true'); + + controllerFor(root).moveInDirection(items[0], 'down'); + await flushPromises(); + + expect(itemIds(sourceList)).toEqual(['1', '2', '3', '4']); + expect(selectedIds(root)).toEqual(['2', '3']); + expect(fetchMock).not.toHaveBeenCalled(); + }); + }); + + it('retains the singular member contract without collection capability', async () => { + const { root, sourceList, firstSourceItem } = renderFixture({ optimistic: true }); + await ctx.nextFrame(); + + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + controller.moveInDirection(firstSourceItem, 'down'); + await flushPromises(); + + expect(itemIds(sourceList)).toEqual(['2', '1', '3']); + const [url, options] = fetchMock.mock.lastCall as [string, { body:FormData }]; + expect(url).toBe('/move/1?optimistic=true'); + expect(options.body.getAll('ids[]')).toEqual([]); + }); + it('refuses a directional move for a non-movable item', async () => { const { root, sourceList, firstSourceItem } = renderFixture(); firstSourceItem.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index c85ed527e41d..cebfb428fd2c 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -53,8 +53,9 @@ import { isOrderableItem, itemAcceptsDestination, reorderRows, - resolveDirectionalPreviousItemId, + resolveBlockMove, resolveBlockMoveAvailability, + resolveDirectionalPreviousItemId, resolveItemId, resolveItemLabel, resolveItemPosition, @@ -542,6 +543,58 @@ export default class SortableListsController extends Controller imp return; } + if (this.selection && this.hasCollectionMoveUrlValue) { + // Resolved without mutating: every check below can still refuse the + // move, and a stale menu must not replace the user's batch with the + // invoker for a move that then never runs. + const scope = this.actionScopeFor(itemElement); + if (scope.kind === 'refused') { + return; + } + + const list = this.ownerListOf(itemElement); + if (!list) { + return; + } + + const resolution = resolveBlockMove({ + itemElements: scope.items, + direction, + rowsContainer: list.rowsContainer, + }); + const moveUrl = this.resolveCollectionMoveUrl(); + if (!resolution.available || !moveUrl) { + return; + } + + // Scope members are resolved candidates, so both identity attributes + // exist; refusing on a mismatch keeps the moved rows and the submitted + // ids from ever diverging. + const items = scope.items.flatMap((element):SelectionItem[] => { + const type = resolveItemType(element); + const id = resolveItemId(element); + return type && id ? [{ type, id }] : []; + }); + if (items.length !== scope.items.length) { + return; + } + + // Committed only now the move is known executable: invoking a position + // action on an unselected card selects it, and a failed request keeps + // that selection for the retry. + this.selectForAction(itemElement); + + void this.performMove({ + rows: resolution.rows, + items, + rowsContainer: list.rowsContainer, + listData: list.listData, + previousItemId: resolution.previousItemId, + moveUrl, + }); + return; + } + const list = this.ownerListOf(itemElement); if (!list) { return; From 7b17f7bd73204cafee3bf9c87364d6291daa3b20 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 04:38:46 +0100 Subject: [PATCH 5/8] [AGILE-363] Cover batch position actions Exercises every direction and gating boundary through Selenium. Covers optimistic order, feedback, cleanup, and 422 rollback while protecting one-card, sparse, cross-list, and confined scopes. https://community.openproject.org/wp/AGILE-363 --- .../work_package_card_menu_component_spec.rb | 4 +- .../batch_destination_menu_spec.rb | 5 +- .../work_packages/batch_move_via_menu_spec.rb | 202 ++++++++++++++++++ .../backlogs/spec/support/pages/backlog.rb | 34 +++ 4 files changed, 242 insertions(+), 3 deletions(-) create mode 100644 modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb diff --git a/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb b/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb index c3c3766a8e8b..9836ab41cbc8 100644 --- a/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb @@ -245,9 +245,11 @@ def render_component(work_package: self.work_package, "[data-action='click->sortable-lists--item#move']" ) end - expect(page).to have_css("li[data-sortable-lists--item-target='moveMenu']") + expect(page).to have_css("li[data-sortable-lists--item-target='moveMenu']", count: 1) expect(page).to have_no_field("direction", type: :hidden) expect(page).to have_no_field("prev_id", type: :hidden) + expect(page).to have_no_field("ids[]", type: :hidden) + expect(page).to have_no_css("form[action$='/backlogs/work_packages/move']") end context "when the work package is the only item in its list" do diff --git a/modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb b/modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb index de0a4ae303c8..5ad133730a49 100644 --- a/modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb +++ b/modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb @@ -190,16 +190,17 @@ ) end - it "omits Move to position from a multi-card action scope" do + it "offers Move to position for a contiguous multi-card action scope" do sprint = create(:sprint, project:, name: "Sprint") create(:backlog_bucket, project:, name: "Destination bucket") first_story = create(:work_package, project:, type:, sprint:, position: 1) second_story = create(:work_package, project:, type:, sprint:, position: 2) + create(:work_package, project:, type:, sprint:, position: 3) backlogs_page.visit! backlogs_page.select_cards(first_story, second_story) - backlogs_page.expect_no_work_package_action(first_story, "Move to position") + backlogs_page.expect_move_to_position_available(first_story, action: "Move down") backlogs_page.expect_work_package_action(first_story, "Move to backlog bucket") end diff --git a/modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb b/modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb new file mode 100644 index 000000000000..5b8af0be3fb8 --- /dev/null +++ b/modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb @@ -0,0 +1,202 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# 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 COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" +require_relative "../../support/pages/backlog" + +RSpec.describe "Move selected backlog cards to a position via their menu", + :js, + :selenium, + :settings_reset, + with_ee: %i[readonly_work_packages] do + let!(:project) do + create(:project, types: [type], enabled_module_names: %w(work_package_tracking backlogs)) + end + let(:type) { create(:type) } + let(:manage_sprint_items_role) do + create(:project_role, + permissions: %i[view_sprints manage_sprint_items view_work_packages edit_work_packages]) + end + let(:backlogs_page) { Pages::Backlog.new(project) } + + current_user do + create(:user, member_with_roles: { project => manage_sprint_items_role }) + end + + def create_sprint_stories(sprint, count: 6, status: nil) + Array.new(count) do |index| + create(:work_package, **{ project:, type:, sprint:, status:, position: index + 1 }.compact) + end + end + + it "moves a contiguous block to the top and then down while preserving its order" do + sprint = create(:sprint, project:, name: "Sprint") + stories = create_sprint_stories(sprint) + backlogs_page.visit! + + backlogs_page.select_contiguous_cards(stories[2], stories[3]) + backlogs_page.expect_move_to_position_available(stories[2], action: "Move to top") + backlogs_page.move_selected_cards(invoker: stories[2], action: "Move to top") + + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [stories[2], stories[3], stories[0], stories[1], stories[4], stories[5]] + ) + backlogs_page.expect_polite_announcement("2 work packages moved to positions 1 through 2 of 6") + backlogs_page.expect_persisted_sprint_order( + sprint, + stories[2], stories[3], stories[0], stories[1], stories[4], stories[5] + ) + backlogs_page.expect_no_selected_cards + + backlogs_page.select_contiguous_cards(stories[2], stories[3]) + backlogs_page.move_selected_cards(invoker: stories[3], action: "Move down") + + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [stories[0], stories[2], stories[3], stories[1], stories[4], stories[5]] + ) + backlogs_page.expect_persisted_sprint_order( + sprint, + stories[0], stories[2], stories[3], stories[1], stories[4], stories[5] + ) + end + + it "moves a contiguous block to the bottom and then up while preserving its order" do + sprint = create(:sprint, project:, name: "Sprint") + stories = create_sprint_stories(sprint) + backlogs_page.visit! + + backlogs_page.select_contiguous_cards(stories[2], stories[3]) + backlogs_page.move_selected_cards(invoker: stories[3], action: "Move to bottom") + + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [stories[0], stories[1], stories[4], stories[5], stories[2], stories[3]] + ) + backlogs_page.expect_persisted_sprint_order( + sprint, + stories[0], stories[1], stories[4], stories[5], stories[2], stories[3] + ) + backlogs_page.expect_no_selected_cards + + backlogs_page.select_contiguous_cards(stories[2], stories[3]) + backlogs_page.move_selected_cards(invoker: stories[2], action: "Move up") + + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [stories[0], stories[1], stories[4], stories[2], stories[3], stories[5]] + ) + backlogs_page.expect_persisted_sprint_order( + sprint, + stories[0], stories[1], stories[4], stories[2], stories[3], stories[5] + ) + end + + it "keeps the singular menu contract for an unselected card" do + sprint = create(:sprint, project:, name: "Sprint") + stories = create_sprint_stories(sprint, count: 3) + backlogs_page.visit! + + backlogs_page.expect_move_to_position_available(stories[1], action: "Move up") + backlogs_page.move_selected_cards(invoker: stories[1], action: "Move up") + + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [stories[1], stories[0], stories[2]] + ) + backlogs_page.expect_polite_announcement("#{stories[1].to_fs(:caption)} moved to position 1 of 3") + backlogs_page.expect_persisted_sprint_order(sprint, stories[1], stories[0], stories[2]) + end + + it "omits positional actions for sparse and cross-list selections" do + sprint = create(:sprint, project:, name: "Sprint") + other_sprint = create(:sprint, project:, name: "Other sprint") + stories = create_sprint_stories(sprint, count: 4) + other_story = create(:work_package, project:, type:, sprint: other_sprint) + backlogs_page.visit! + + backlogs_page.select_cards(stories[0], stories[2]) + backlogs_page.expect_move_to_position_unavailable(stories[0]) + backlogs_page.clear_card_selection(stories[0]) + + backlogs_page.select_cards(stories[0], other_story) + backlogs_page.expect_move_to_position_unavailable(stories[0]) + end + + it "moves a contiguous confined block within its list" do + sprint = create(:sprint, project:, name: "Sprint") + readonly_status = create(:status, is_readonly: true) + stories = create_sprint_stories(sprint, count: 4, status: readonly_status) + backlogs_page.visit! + + backlogs_page.select_contiguous_cards(stories[1], stories[2]) + backlogs_page.expect_move_to_position_available(stories[1], action: "Move down") + backlogs_page.move_selected_cards(invoker: stories[1], action: "Move down") + + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [stories[0], stories[3], stories[1], stories[2]] + ) + backlogs_page.expect_persisted_sprint_order(sprint, stories[0], stories[3], stories[1], stories[2]) + end + + it "omits one-step actions at truncation boundaries and block no-ops" do + stub_const("Backlogs::InboxComponent::TRUNCATE_MIDDLE", 2) + inbox_stories = Array.new(5) do |index| + create(:work_package, project:, type:, position: index + 1) + end + backlogs_page.visit! + + backlogs_page.select_contiguous_cards(inbox_stories[0], inbox_stories[1]) + backlogs_page.expect_move_to_position_unavailable(inbox_stories[0], action: "Move down") + backlogs_page.expect_move_to_position_unavailable(inbox_stories[0], action: "Move to top") + backlogs_page.clear_card_selection(inbox_stories[0]) + + backlogs_page.select_cards(inbox_stories.last) + backlogs_page.expect_move_to_position_unavailable(inbox_stories.last, action: "Move to bottom") + end + + it "rolls a rejected move back and preserves the selected block" do + sprint = create(:sprint, project:, name: "Sprint") + stories = create_sprint_stories(sprint, count: 4) + backlogs_page.visit! + + backlogs_page.select_contiguous_cards(stories[1], stories[2]) + stories[2].destroy! + backlogs_page.move_selected_cards(invoker: stories[1], action: "Move down") + + backlogs_page.expect_move_error( + I18n.t("backlogs.work_packages.move_collection.work_packages_not_found") + ) + backlogs_page.expect_work_packages_in_sprint_in_order(sprint, work_packages: stories) + backlogs_page.expect_selected_cards_in_order(stories[1], stories[2]) + end +end diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index 8555b5bd08c9..df94ad4ada12 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -745,16 +745,50 @@ def selected_card_ids def select_cards(*work_packages) work_packages.each { |work_package| toggle_card(work_package) } + expect(page).to have_css("[data-batch-selected]", count: work_packages.size) + end + + # Settles on the range's own endpoints rather than a card count, which + # only the caller knows for a span wider than a pair. + def select_contiguous_cards(first, last) + toggle_card(first) + extend_selection_to(last) + expect(page).to have_css("#{work_package_selector(first)}[data-batch-selected]") + expect(page).to have_css("#{work_package_selector(last)}[data-batch-selected]") end def expect_selected_cards_in_order(*work_packages) + expect(page).to have_css("[data-batch-selected]", count: work_packages.size) expect(selected_card_ids).to eq(work_packages.map { |work_package| work_package.id.to_s }) end def expect_no_selected_cards + expect(page).to have_no_css("[data-batch-selected]") expect(selected_card_ids).to be_empty end + def move_selected_cards(invoker:, action:, wait: false) + click_in_work_package_move_submenu(invoker, action, wait:) + end + + def expect_move_to_position_available(work_package, action: nil) + within_work_package_move_submenu(work_package) do |submenu| + next unless action + + expect(submenu).to have_selector(:menuitem, text: action, exact_text: true) + end + end + + def expect_move_to_position_unavailable(work_package, action: nil) + if action + within_work_package_move_submenu(work_package) do |submenu| + expect(submenu).to have_no_selector(:menuitem, text: action, exact_text: true) + end + else + expect_no_work_package_action(work_package, "Move to position") + end + end + def clear_card_selection(work_package) work_package_card(work_package).send_keys(:escape) expect_no_selected_cards From 9d7d53525bece9f35c05917e82bb224b178f4f28 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 04:54:14 +0100 Subject: [PATCH 6/8] [AGILE-363] Guard batch position capability Resolves collection capability before availability or mutation. Makes incomplete selection roots fail closed while keeping roots without selection on their singular member behavior. https://community.openproject.org/wp/AGILE-363 --- .../dynamic/sortable-lists.controller.spec.ts | 62 +++++++++++++------ .../dynamic/sortable-lists.controller.ts | 16 +++-- .../sortable-lists/list.controller.spec.ts | 4 +- .../scrollable.controller.spec.ts | 4 +- .../selection-orchestrator.spec.ts | 59 ------------------ .../sortable-lists/selection-orchestrator.ts | 7 --- 6 files changed, 57 insertions(+), 95 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index d35a2919a761..6dcdaddfa161 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -1196,11 +1196,16 @@ describe('Sortable lists controller', () => { }); describe('batch menu moves', () => { - function renderBatchMenuFixture() { + function renderBatchMenuFixture({ + collectionMoveUrl = '/projects/demo/backlogs/work_packages/move', + }:{ collectionMoveUrl?:string|null } = {}) { const elements = renderSelectableRoot({ optimistic: true, - collectionMoveUrl: '/projects/demo/backlogs/work_packages/move', + collectionMoveUrl, }); + if (collectionMoveUrl === '') { + elements.root.setAttribute('data-sortable-lists-collection-move-url-value', ''); + } elements.sourceList.setAttribute('data-sortable-lists--list-type-value', 'sprint'); elements.sourceList.setAttribute('data-sortable-lists--list-id-value', '8'); elements.sourceList.append(elements.items[3]); @@ -1374,6 +1379,35 @@ describe('Sortable lists controller', () => { expect(selectedIds(root)).toEqual(['2', '3']); expect(fetchMock).not.toHaveBeenCalled(); }); + + it.each([ + ['missing', null], + ['blank', ''], + ])('reports every direction unavailable when the collection URL is %s', async (_state, collectionMoveUrl) => { + const { root, items } = renderBatchMenuFixture({ collectionMoveUrl }); + await ctx.nextFrame(); + selectItems(items[0], items[1]); + + expect(controllerFor(root).moveAvailability(items[0])).toEqual({ + top: false, up: false, down: false, bottom: false, + }); + }); + + it.each([ + ['missing', null], + ['blank', ''], + ])('preserves a selected block without requesting when the collection URL is %s', async (_state, collectionMoveUrl) => { + const { root, sourceList, items } = renderBatchMenuFixture({ collectionMoveUrl }); + await ctx.nextFrame(); + selectItems(items[0], items[1]); + + controllerFor(root).moveInDirection(items[0], 'down'); + await flushPromises(); + + expect(itemIds(sourceList)).toEqual(['1', '2', '3', '4']); + expect(selectedIds(root)).toEqual(['1', '2']); + expect(fetchMock).not.toHaveBeenCalled(); + }); }); it('retains the singular member contract without collection capability', async () => { @@ -1404,7 +1438,7 @@ describe('Sortable lists controller', () => { }); it('reports per-direction move availability for gating', async () => { - const { root, firstSourceItem } = renderSelectableRoot(); + const { root, firstSourceItem } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; @@ -1421,7 +1455,7 @@ describe('Sortable lists controller', () => { }; it('reports the available directions for a selected contiguous block', async () => { - const { root, items } = renderSelectableRoot(); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; select(items[1]); @@ -1433,7 +1467,7 @@ describe('Sortable lists controller', () => { }); it('reports every direction unavailable for a sparse selection', async () => { - const { root, items } = renderSelectableRoot(); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; select(items[0]); @@ -1443,7 +1477,7 @@ describe('Sortable lists controller', () => { }); it('reports every direction unavailable for a cross-list selection', async () => { - const { root, items } = renderSelectableRoot(); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; select(items[2]); @@ -1453,7 +1487,7 @@ describe('Sortable lists controller', () => { }); it('reports one-card availability for an unselected invoker without changing the selection', async () => { - const { root, items } = renderSelectableRoot(); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; select(items[0]); @@ -1466,7 +1500,7 @@ describe('Sortable lists controller', () => { }); it('reports normal within-list availability for a confined contiguous block', async () => { - const { root, items } = renderSelectableRoot(); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); items[0].setAttribute('data-sortable-lists--item-mobility-value', 'confined'); items[1].setAttribute('data-sortable-lists--item-mobility-value', 'confined'); await ctx.nextFrame(); @@ -1846,18 +1880,6 @@ describe('Sortable lists controller', () => { expect(document.querySelectorAll('[data-batch-selected]')).toHaveLength(0); }); - it('collapses a wider batch onto the card a menu move names', async () => { - const { root, items } = renderSelectableRoot(); - await ctx.nextFrame(); - const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; - items[1].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true })); - items[2].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true })); - - controller.moveInDirection(items[0], 'down'); - - expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0]]); - }); - // A card that is not part of the batch collapses it onto itself, which is // a count change a screen-reader user has to hear. Dragging a member // instead carries the whole batch — see the "batch dragging" block below. diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index cebfb428fd2c..5418d7dd6db8 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -505,6 +505,10 @@ export default class SortableListsController extends Controller imp return null; } + if (!this.resolveCollectionMoveUrl()) { + return { top: false, up: false, down: false, bottom: false }; + } + const list = this.ownerListOf(itemElement); return list @@ -543,7 +547,12 @@ export default class SortableListsController extends Controller imp return; } - if (this.selection && this.hasCollectionMoveUrlValue) { + if (this.selection) { + const moveUrl = this.resolveCollectionMoveUrl(); + if (!moveUrl) { + return; + } + // Resolved without mutating: every check below can still refuse the // move, and a stale menu must not replace the user's batch with the // invoker for a move that then never runs. @@ -562,8 +571,7 @@ export default class SortableListsController extends Controller imp direction, rowsContainer: list.rowsContainer, }); - const moveUrl = this.resolveCollectionMoveUrl(); - if (!resolution.available || !moveUrl) { + if (!resolution.available) { return; } @@ -616,8 +624,6 @@ export default class SortableListsController extends Controller imp return; } - this.selection?.collapseForAction(itemElement); - void this.performMove({ rows: [sourceRow], items: null, diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts index ca03ea4eda9f..cf1f8d8854e0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts @@ -76,8 +76,8 @@ describe('Sortable lists list controller', () => { return { element, busy, - actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), - selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + actionScopeFor: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), availableDestinations: vi.fn(() => []), moveToDestination: vi.fn(), moveInDirection: vi.fn(), diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts index f551249f0d65..f04a262a5cd8 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts @@ -71,8 +71,8 @@ describe('Sortable lists scrollable controller', () => { return { element, busy: false, - actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), - selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + actionScopeFor: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn(():ActionScope => ({ kind: 'refused', items: [] })), availableDestinations: vi.fn(() => []), moveToDestination: vi.fn(), moveInDirection: vi.fn(), diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index d574bd5f1995..e575d2243f5d 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -745,65 +745,6 @@ describe('SelectionOrchestrator', () => { }); }); - // A menu action relocates one card, so it collapses the batch onto that - // card rather than leaving a wider selection to outlive the move. - describe('collapseForAction', () => { - it('collapses a wider batch onto a member', () => { - const orchestrator = new SelectionOrchestrator(hostFor(root)); - orchestrator.handleClick(clickOn(item('1'))); - orchestrator.handleClick(clickOn(item('3'), { ctrlKey: true })); - - expect(orchestrator.collapseForAction(item('1'))).toEqual({ kind: 'batch', items: [item('1')] }); - expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); - }); - - it('collapses a wider batch onto a card outside it', () => { - const orchestrator = new SelectionOrchestrator(hostFor(root)); - orchestrator.handleClick(clickOn(item('1'))); - orchestrator.handleClick(clickOn(item('2'), { ctrlKey: true })); - - expect(orchestrator.collapseForAction(item('3')).items).toEqual([item('3')]); - expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['3']); - }); - - it('leaves an empty selection empty', () => { - const orchestrator = new SelectionOrchestrator(hostFor(root)); - - expect(orchestrator.collapseForAction(item('2')).items).toEqual([item('2')]); - expect(orchestrator.selectedItems()).toEqual([]); - }); - - it('refuses a fixed card without disturbing the batch', () => { - item('3').setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); - const orchestrator = new SelectionOrchestrator(hostFor(root)); - orchestrator.handleClick(clickOn(item('1'))); - - expect(orchestrator.collapseForAction(item('3'))).toEqual({ kind: 'refused', items: [] }); - expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); - }); - - it('announces the collapse of a wider batch', () => { - const orchestrator = new SelectionOrchestrator(hostFor(root)); - orchestrator.handleClick(clickOn(item('1'))); - orchestrator.handleClick(clickOn(item('3'), { ctrlKey: true })); - announceSpy.mockClear(); - - orchestrator.collapseForAction(item('1')); - - expect(announceSpy.mock.calls.map((call) => call[0])).toEqual(['[selected:1]']); - }); - - it('stays silent collapsing a single selection onto itself', () => { - const orchestrator = new SelectionOrchestrator(hostFor(root)); - orchestrator.handleClick(clickOn(item('1'))); - announceSpy.mockClear(); - - orchestrator.collapseForAction(item('1')); - - expect(announceSpy).not.toHaveBeenCalled(); - }); - }); - describe('clearSilently', () => { it('clears model, anchor and presentation without an announcement', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts index baf23d266c53..6041f3783cf2 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -130,13 +130,6 @@ export class SelectionOrchestrator { return this.resolveActionScope(itemElement, 'join'); } - // A menu action relocates exactly this card, so a wider batch collapses - // onto it rather than outliving the move. It must not manufacture a - // selection where the user made none: a failed move would otherwise leave - // that card selected. - collapseForAction(itemElement:HTMLElement):ActionScope { - return this.resolveActionScope(itemElement, this.hasSelection ? 'collapse' : 'none'); - } private resolveActionScope(itemElement:HTMLElement, mutation:ScopeMutation):ActionScope { const candidate = resolveCandidate(this.host.rootElement, itemElement); From 74e5d6d1f0405ae0862762a938abda186cd9630d Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 23:54:41 +0100 Subject: [PATCH 7/8] [AGILE-363] Restore singular position gating Restores per-direction availability for plain sortable roots while keeping selection roots on block and capability gating. Protects blank predecessors and removes the obsolete collapse shim. https://community.openproject.org/wp/AGILE-363 --- .../dynamic/sortable-lists.controller.spec.ts | 22 ++++++++++++++++-- .../dynamic/sortable-lists.controller.ts | 23 ++++++++++--------- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 6dcdaddfa161..96cdd669cb0b 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -1258,6 +1258,24 @@ describe('Sortable lists controller', () => { await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); }); + it('serializes a top block move with an explicit blank predecessor', async () => { + const { root, sourceList, items } = renderBatchMenuFixture(); + await ctx.nextFrame(); + selectItems(items[1], items[2]); + + controllerFor(root).moveInDirection(items[1], 'top'); + + expect(itemIds(sourceList)).toEqual(['2', '3', '1', '4']); + const options = fetchMock.mock.lastCall?.[1] as { body:FormData }; + expect([...options.body.entries()]).toEqual([ + ['ids[]', '2'], + ['ids[]', '3'], + ['list_type', 'sprint'], + ['list_id', '8'], + ['prev_id', ''], + ]); + }); + it('clears the selection after a successful collection move', async () => { const { root, items } = renderBatchMenuFixture(); await ctx.nextFrame(); @@ -1437,8 +1455,8 @@ describe('Sortable lists controller', () => { expect(fetchMock).not.toHaveBeenCalled(); }); - it('reports per-direction move availability for gating', async () => { - const { root, firstSourceItem } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); + it('reports per-direction availability for a plain sortable root', async () => { + const { root, firstSourceItem } = renderFixture(); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index 5418d7dd6db8..fc2357b139c3 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -60,6 +60,7 @@ import { resolveItemLabel, resolveItemPosition, resolveItemType, + resolveMoveAvailability, restoreRowPositions, rowOf, rowsRemainAt, @@ -491,29 +492,29 @@ export default class SortableListsController extends Controller imp return this.element.hasAttribute(sortableListsBusyAttribute); } - // A direction is offered exactly when the move resolver can produce a - // target for it, and never for an item moveInDirection would refuse below. - // Null means the item is not in an owned list yet. A snapshot for menu - // gating; the click path re-resolves the live DOM. + // A direction is offered exactly when the move resolver can produce a target + // for it, which keeps the menu honest about truncated lists. Null means the + // item is not in an owned list yet, or takes no part in ordering. A snapshot + // for menu gating; the click path re-resolves the live DOM. moveAvailability(itemElement:HTMLElement):MoveAvailability|null { - if (!isOrderableItem(itemElement)) { + const list = this.ownerListOf(itemElement); + if (!list || !isOrderableItem(itemElement)) { return null; } const scope = this.actionScopeFor(itemElement); if (scope.kind === 'refused') { - return null; + return resolveMoveAvailability({ + itemElement, + rowsContainer: list.rowsContainer, + }); } if (!this.resolveCollectionMoveUrl()) { return { top: false, up: false, down: false, bottom: false }; } - const list = this.ownerListOf(itemElement); - - return list - ? resolveBlockMoveAvailability({ itemElements: scope.items, rowsContainer: list.rowsContainer }) - : null; + return resolveBlockMoveAvailability({ itemElements: scope.items, rowsContainer: list.rowsContainer }); } moveToDestination(itemElement:HTMLElement, target:DestinationIdentity):void { From 9b06e658e0723d695e691c5f45b7cbdf61e26e5b Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 19 Aug 2026 19:57:21 +0100 Subject: [PATCH 8/8] Assert position outcomes through the page Replaces the persisted-order database checks with a reload and a rendered-order assertion, so the spec proves persistence by what the user sees surviving the reload. Payload and ordering semantics remain covered by the controller unit specs and the request specs. --- .../work_packages/batch_move_via_menu_spec.rb | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb b/modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb index 5b8af0be3fb8..a5fa19eef2b8 100644 --- a/modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb +++ b/modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb @@ -70,10 +70,6 @@ def create_sprint_stories(sprint, count: 6, status: nil) work_packages: [stories[2], stories[3], stories[0], stories[1], stories[4], stories[5]] ) backlogs_page.expect_polite_announcement("2 work packages moved to positions 1 through 2 of 6") - backlogs_page.expect_persisted_sprint_order( - sprint, - stories[2], stories[3], stories[0], stories[1], stories[4], stories[5] - ) backlogs_page.expect_no_selected_cards backlogs_page.select_contiguous_cards(stories[2], stories[3]) @@ -83,9 +79,11 @@ def create_sprint_stories(sprint, count: 6, status: nil) sprint, work_packages: [stories[0], stories[2], stories[3], stories[1], stories[4], stories[5]] ) - backlogs_page.expect_persisted_sprint_order( + + backlogs_page.visit! + backlogs_page.expect_work_packages_in_sprint_in_order( sprint, - stories[0], stories[2], stories[3], stories[1], stories[4], stories[5] + work_packages: [stories[0], stories[2], stories[3], stories[1], stories[4], stories[5]] ) end @@ -101,10 +99,6 @@ def create_sprint_stories(sprint, count: 6, status: nil) sprint, work_packages: [stories[0], stories[1], stories[4], stories[5], stories[2], stories[3]] ) - backlogs_page.expect_persisted_sprint_order( - sprint, - stories[0], stories[1], stories[4], stories[5], stories[2], stories[3] - ) backlogs_page.expect_no_selected_cards backlogs_page.select_contiguous_cards(stories[2], stories[3]) @@ -114,9 +108,11 @@ def create_sprint_stories(sprint, count: 6, status: nil) sprint, work_packages: [stories[0], stories[1], stories[4], stories[2], stories[3], stories[5]] ) - backlogs_page.expect_persisted_sprint_order( + + backlogs_page.visit! + backlogs_page.expect_work_packages_in_sprint_in_order( sprint, - stories[0], stories[1], stories[4], stories[2], stories[3], stories[5] + work_packages: [stories[0], stories[1], stories[4], stories[2], stories[3], stories[5]] ) end @@ -133,7 +129,12 @@ def create_sprint_stories(sprint, count: 6, status: nil) work_packages: [stories[1], stories[0], stories[2]] ) backlogs_page.expect_polite_announcement("#{stories[1].to_fs(:caption)} moved to position 1 of 3") - backlogs_page.expect_persisted_sprint_order(sprint, stories[1], stories[0], stories[2]) + + backlogs_page.visit! + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [stories[1], stories[0], stories[2]] + ) end it "omits positional actions for sparse and cross-list selections" do @@ -165,7 +166,12 @@ def create_sprint_stories(sprint, count: 6, status: nil) sprint, work_packages: [stories[0], stories[3], stories[1], stories[2]] ) - backlogs_page.expect_persisted_sprint_order(sprint, stories[0], stories[3], stories[1], stories[2]) + + backlogs_page.visit! + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [stories[0], stories[3], stories[1], stories[2]] + ) end it "omits one-step actions at truncation boundaries and block no-ops" do