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..96cdd669cb0b 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,253 @@ describe('Sortable lists controller', () => { expect(body.body.get('prev_id')).toBe('2'); }); + describe('batch menu moves', () => { + function renderBatchMenuFixture({ + collectionMoveUrl = '/projects/demo/backlogs/work_packages/move', + }:{ collectionMoveUrl?:string|null } = {}) { + const elements = renderSelectableRoot({ + optimistic: true, + 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]); + + 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('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(); + 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.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 () => { + 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'); @@ -1208,7 +1455,7 @@ describe('Sortable lists controller', () => { expect(fetchMock).not.toHaveBeenCalled(); }); - it('reports per-direction move availability for gating', async () => { + 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; @@ -1219,16 +1466,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({ collectionMoveUrl: '/collection-move-url' }); + 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({ collectionMoveUrl: '/collection-move-url' }); + 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({ collectionMoveUrl: '/collection-move-url' }); + 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({ collectionMoveUrl: '/collection-move-url' }); + 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({ 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(); + 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(); }); }); @@ -1589,18 +1898,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 cc32d62b7dfc..fc2357b139c3 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -53,6 +53,8 @@ import { isOrderableItem, itemAcceptsDestination, reorderRows, + resolveBlockMove, + resolveBlockMoveAvailability, resolveDirectionalPreviousItemId, resolveItemId, resolveItemLabel, @@ -490,20 +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)) { - return { - top: false, up: false, down: false, bottom: false, - }; + const list = this.ownerListOf(itemElement); + if (!list || !isOrderableItem(itemElement)) { + return null; } - const list = this.ownerListOf(itemElement); + const scope = this.actionScopeFor(itemElement); + if (scope.kind === 'refused') { + return resolveMoveAvailability({ + itemElement, + rowsContainer: list.rowsContainer, + }); + } - return list ? resolveMoveAvailability({ itemElement, rowsContainer: list.rowsContainer }) : null; + if (!this.resolveCollectionMoveUrl()) { + return { top: false, up: false, down: false, bottom: false }; + } + + return resolveBlockMoveAvailability({ itemElements: scope.items, rowsContainer: list.rowsContainer }); } moveToDestination(itemElement:HTMLElement, target:DestinationIdentity):void { @@ -537,6 +548,62 @@ export default class SortableListsController extends Controller imp return; } + 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. + 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, + }); + if (!resolution.available) { + 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; @@ -558,8 +625,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/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 00d9a5e0814b..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 @@ -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 }; @@ -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); }); @@ -1909,8 +1937,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 +1970,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/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); 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..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 @@ -33,6 +33,8 @@ import { itemMobility, permittedDestinations, reorderRows, + resolveBlockMove, + resolveBlockMoveAvailability, sortableItemMobilityAttribute, resolveDirectionalPreviousItemId, resolveMoveAvailability, @@ -661,6 +663,175 @@ 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 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(); + + 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 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(); + + 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..f07b59cdc20c 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,125 @@ 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, index) => ( + row === null || resolveItemElement(row, rowsContainer) !== itemElements[index] + ))) { + 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. 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); 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..a5fa19eef2b8 --- /dev/null +++ b/modules/backlogs/spec/features/work_packages/batch_move_via_menu_spec.rb @@ -0,0 +1,208 @@ +# 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_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.visit! + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [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_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.visit! + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [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.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 + 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.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 + 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