diff --git a/app/components/open_project/common/border_box_list_component.sass b/app/components/open_project/common/border_box_list_component.sass index b9bef95c1eff..901cf782572a 100644 --- a/app/components/open_project/common/border_box_list_component.sass +++ b/app/components/open_project/common/border_box_list_component.sass @@ -64,7 +64,8 @@ &[data-batch-selected] border-top-color: var(--box-list-item-selected-border-color) - &:has(> .Box-card[aria-current="true"]) + &:has(> .Box-card[aria-current="true"]), + &:has(> .Box-card[data-activating]) border-top-color: var(--box-list-item-pressed-border-color) &.op-border-box-list_transparent @@ -208,7 +209,13 @@ // Current state: the card is open in the split screen. aria-current is // maintained by the backlogs--work-package controller once the URL reflects // the details pane; the strong border wins over a simultaneous batch selection. -.Box-row:has(> .Box-card[aria-current="true"]) +// +// data-activating is that controller's synchronous pressed state — the card +// was just clicked or Enter-activated and its visit has not landed yet. It +// is visual only (no ARIA) and reuses the pressed border so it hands over +// seamlessly to aria-current once the URL reflects the details pane. +.Box-row:has(> .Box-card[aria-current="true"]), +.Box-row:has(> .Box-card[data-activating]) @include op-box-list-item-edge-borders(var(--box-list-item-pressed-border-color)) // Drag-and-drop row states, scoped to the border-box list. The global @@ -306,6 +313,26 @@ .-browser-firefox & box-shadow: none + // The stack and the lift share the one box-shadow property, so both are + // declared here or one replaces the other. + // + // The depth the preview writes is the batch's, uncapped: past four cards the + // added depth stops reading, so this default holds every deeper batch at the + // maximum and the shallow depths below override it. + &[data-preview][data-stack-depth] + box-shadow: op-drag-stack-shadows(), var(--shadow-floating-medium) + + // Blur-free and inside the reserved overhang, so only the lift has to go. + .-browser-firefox & + box-shadow: op-drag-stack-shadows() + + @for $layers from 1 through $op-drag-stack-max-layers - 1 + &[data-preview][data-stack-depth="#{$layers}"] + box-shadow: op-drag-stack-shadows($layers: $layers), var(--shadow-floating-medium) + + .-browser-firefox & + box-shadow: op-drag-stack-shadows($layers: $layers) + .Box--condensed .Box-card padding: var(--stack-padding-condensed) var(--stack-padding-normal) diff --git a/config/locales/js-en.yml b/config/locales/js-en.yml index 7c54ca744684..5d500c186c3d 100644 --- a/config/locales/js-en.yml +++ b/config/locales/js-en.yml @@ -861,11 +861,23 @@ en: sortable_lists: announcements: + batch_too_large: "Cannot move %{count} items at once. Select no more than %{max}." fallback_item_label: "Item" fallback_list_name: "another list" move_failed_check_position: "Move failed. Check the item's current position." + # The batch keys are plural hashes so translators can add the plural + # categories their locale needs; the one: branch is unreachable (a + # one-item move announces through the singular keys). + move_failed_check_positions_batch: + other: "Move failed. Check the items' current positions." move_failed_rolled_back: "Move failed. %{label} returned to its previous position." + move_failed_rolled_back_batch: + other: "Move failed. %{count} items returned to their previous positions." moved: "%{label} moved to position %{position} of %{total}" + moved_batch: + other: "%{count} items moved to positions %{first} through %{last} of %{total}" + moved_batch_to_list: + other: "%{count} items moved to %{list}, positions %{first} through %{last} of %{total}" moved_to_list: "%{label} moved to %{list}, position %{position} of %{total}" selection: cleared: "Selection cleared." diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index e40327563ee6..da3c5cc73125 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -7,7 +7,7 @@ - `./src/common/` - Framework-agnostic modules (the `core-common` alias), importable from both Angular and Stimulus. Code belongs here when it depends on neither framework and both sides need it; a helper only Stimulus controllers use belongs in `./src/stimulus/helpers/` instead. - `./src/stimulus/` - Stimulus controllers - `./src/turbo/` - Turbo integration -- `sortable-lists` batch selection is opt-in: a root enables it with a `selectionEnabled` value, and no other consumer's behavior changes. A root also sets `announcementScope`, so the shared controller's announcements speak the consumer's vocabulary instead of "item", and `selectionDescriptionId`, pointing at one shared element every selected card references via `aria-describedby`. Items declare `mobility` — `fixed`, `confined` or `free` — which gates dragging, selection eligibility, and positional moves alike. A missing value means `free`, so a consumer that renders none keeps working; an unrecognised one falls closed to `fixed` rather than handing the user controls the server will refuse. The pure selection model lives in `./src/common/batch-selection.ts` (framework-agnostic, so Angular consumers can adopt it); the DOM-facing adapter is `sortable-lists/selection.ts`, and gesture interpretation sits behind `sortable-lists/selection-orchestrator.ts`, which takes a narrow host port and imports no Stimulus. Selection identity is `(type, id)`, never the id alone: ids are unique per source table, so a nested list of another type can hold a colliding one. A root must render exactly one instance of each `(type, id)`, and an item declaring no type is refused as a candidate. A batch holds one item type — that cohort rule is orchestrator policy, not a constraint of the model, since identity namespacing and batch compatibility are different concerns. Ranges and select-all (Ctrl/Cmd+A) are both confined to the focused card's list; selecting across lists is a deliberate gap, reserved for a separate mechanism. An item belongs to its nearest ancestor root, so an independently nested root is an ownership boundary. Batch movement is not implemented: a drag still moves one card and collapses any wider selection onto it — that's a later work package. +- `sortable-lists` batch selection is opt-in: a root enables it with a `selectionEnabled` value, and no other consumer's behavior changes. A root also sets `announcementScope`, so the shared controller's announcements speak the consumer's vocabulary instead of "item", and `selectionDescriptionId`, pointing at one shared element every selected card references via `aria-describedby`. Items declare `mobility` — `fixed`, `confined` or `free` — which gates dragging, selection eligibility, and positional moves alike. A missing value means `free`, so a consumer that renders none keeps working; an unrecognised one falls closed to `fixed` rather than handing the user controls the server will refuse. The pure selection model lives in `./src/common/batch-selection.ts` (framework-agnostic, so Angular consumers can adopt it); the DOM-facing adapter is `sortable-lists/selection.ts`, and gesture interpretation sits behind `sortable-lists/selection-orchestrator.ts`, which takes a narrow host port and imports no Stimulus. Selection identity is `(type, id)`, never the id alone: ids are unique per source table, so a nested list of another type can hold a colliding one. A root must render exactly one instance of each `(type, id)`, and an item declaring no type is refused as a candidate. A batch holds one item type — that cohort rule is orchestrator policy, not a constraint of the model, since identity namespacing and batch compatibility are different concerns. Ranges and select-all (Ctrl/Cmd+A) are both confined to the focused card's list; selecting across lists is a deliberate gap, reserved for a separate mechanism. An item belongs to its nearest ancestor root, so an independently nested root is an ownership boundary. Dragging a selected card moves the whole batch: the root freezes the drag's batch in the preview callback (`freezeDragBatch`) and marks its rows at drag start (`markDragBatch`), and a selection-enabled root with a `collectionMoveUrl` value submits ordered `ids[]` to the collection move action — for one dragged card or many. Dragging an unselected card selects it, collapsing any wider selection. A root's `moveAnnouncementScope` value keys the move announcements the same way `announcementScope` keys the selection ones. - `data-batch-selected` is written on the sortable item element — the row, in Backlogs — while `aria-current` is written on the card inside it. A stylesheet assuming both live on the same element will silently paint nothing while attribute assertions stay green. ## Configuration Files diff --git a/frontend/src/global_styles/content/drag_and_drop.sass b/frontend/src/global_styles/content/drag_and_drop.sass index 9a19490f5958..7b477c9b4437 100644 --- a/frontend/src/global_styles/content/drag_and_drop.sass +++ b/frontend/src/global_styles/content/drag_and_drop.sass @@ -25,6 +25,7 @@ // // See COPYRIGHT and LICENSE files for more details. //++ +@use "sass:list" // The intend for this file is to become the place where all drag&drop styles are placed. // As there will hopefully also be a shared style for resizing, and as those two will hopefully also share some styles @@ -45,3 +46,66 @@ &:active cursor: grabbing + +// Ghost layers behind a multi-card drag preview, one per further card in the +// batch up to the maximum. Composed into the card's own shadow by the +// component that renders it (border_box_list_component.sass). +$op-drag-stack-max-layers: 3 !default +$op-drag-stack-step: 6px !default +$op-drag-stack-ring: 1px !default +$op-drag-stack-shade-blur: 4px !default +$op-drag-stack-shade-color: rgba(37, 41, 46, 0.18) !default + +$op-drag-badge-overhang: 8px !default +$op-drag-stack-overhang: $op-drag-stack-step * $op-drag-stack-max-layers + $op-drag-stack-shade-blur + +// renderDragPreview reserves the overhangs as container padding, and can only +// do so inline (Pragmatic inline-resets the container first), so they are +// published as custom properties rather than duplicated as numbers there. +:root + --op-drag-badge-overhang: #{$op-drag-badge-overhang} + --op-drag-stack-overhang: #{$op-drag-stack-overhang} + +// Each depth emits shade, fill then ring: CSS paints a shadow list front to +// back, so a depth's shade lands above its own fill, in the gap under the card +// in front of it. The deepest shade reaches `step * layers + shade-blur` past +// the right and bottom edges; the blur is under every offset, so nothing +// reaches past the top or left. +@function op-drag-stack-shadows($layers: $op-drag-stack-max-layers, $step: $op-drag-stack-step, $ring: $op-drag-stack-ring, $blur: $op-drag-stack-shade-blur, $shade: $op-drag-stack-shade-color) + $shadows: () + + @for $depth from 1 through $layers + $offset: $step * $depth + $shadows: list.append($shadows, $offset $offset $blur 0 $shade, comma) + $shadows: list.append($shadows, $offset $offset 0 0 var(--bgColor-default), comma) + $shadows: list.append($shadows, $offset $offset 0 $ring var(--borderColor-default), comma) + + @return $shadows + +// Multi-card batch count badge on a drag preview, on Primer's Counter +// contract plus the positioning Counter does not own and the accent skin the +// drop indicator uses. +// +// Paint past the container's border box lands in Firefox's drag snapshot and +// shifts its origin off the grab offset, so the badge sits in the padding +// renderDragPreview writes. The right offset keeps it on the card's corner. +.op-sortable-lists-drag-preview-batch-badge + position: absolute + top: 0 + right: $op-drag-stack-overhang - $op-drag-badge-overhang + min-width: 20px + height: 20px + padding: 0 6px + border-radius: 999px + background-color: var(--bgColor-accent-emphasis) + color: var(--fgColor-onEmphasis) + font-size: 12px + font-weight: 600 + line-height: 20px + text-align: center + box-shadow: var(--shadow-floating-medium) + + // The blur past the container's border box would shift Firefox's snapshot + // origin like the overhang above. + .-browser-firefox & + box-shadow: none diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts index a33534a536f8..ff6d569caf6e 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts @@ -158,4 +158,39 @@ describe('Backlogs split-view-sync controller', () => { expect(refresh).not.toHaveBeenCalled(); }); + + it('refreshes every cached member of a batch event, in order', async () => { + await renderHost(); + + dispatchMoved({ work_package_ids: [11, 12, 13] }); + + await waitFor(() => { + expect(id).toHaveBeenCalledWith('11'); + expect(id).toHaveBeenCalledWith('12'); + expect(id).toHaveBeenCalledWith('13'); + expect(refresh).toHaveBeenCalledTimes(3); + }); + }); + + it('skips uncached members of a batch event', async () => { + hasValue.mockImplementation(() => state.mock.calls.length === 2); + await renderHost(); + + dispatchMoved({ work_package_ids: [11, 12] }); + + await waitFor(() => { + expect(state).toHaveBeenCalledWith('11'); + expect(state).toHaveBeenCalledWith('12'); + expect(refresh).toHaveBeenCalledTimes(1); + }); + }); + + it('ignores an event with neither id field', async () => { + await renderHost(); + + dispatchMoved({}); + await ctx.nextFrame(); + + expect(refresh).not.toHaveBeenCalled(); + }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts index 41805446b9a8..d2b47a99ed0e 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts @@ -53,17 +53,21 @@ export default class SplitViewSyncController extends Controller { // was ever opened, it will still be in the cache leading to a refresh request. The upside of this potentially // wasteful refresh is that when the work package is later on reopened in the split view, its information // is correct as well. - onWorkPackageMoved(event:CustomEvent<{ work_package_id?:number }>):void { - const workPackageId = event.detail?.work_package_id; + onWorkPackageMoved(event:CustomEvent<{ work_package_id?:number; work_package_ids?:number[] }>):void { + const detail = event.detail ?? {}; + const ids = detail.work_package_ids + ?? (detail.work_package_id !== undefined ? [detail.work_package_id] : []); // apiV3Service is wired asynchronously via useAngularServices, so it may be absent // if the event somehow fires before the services resolve. - if (workPackageId === undefined || !this.apiV3Service) { return; } + if (ids.length === 0 || !this.apiV3Service) { return; } - const id = workPackageId.toString(); const { work_packages: workPackages } = this.apiV3Service; - if (workPackages.cache.state(id).hasValue()) { - void workPackages.id(id).refresh(); - } + ids.forEach((rawId) => { + const id = rawId.toString(); + if (workPackages.cache.state(id).hasValue()) { + void workPackages.id(id).refresh(); + } + }); } } diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.spec.ts index dfed73434f28..75d873d30c1c 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.spec.ts @@ -177,6 +177,80 @@ describe('Backlogs work package controller', () => { } }); + // The pressed state is visual only — data-activating, never ARIA — so + // every user gets synchronous feedback regardless of batch selection + // being enabled for them. + describe('activation feedback', () => { + it('shows pressed feedback synchronously on click, before any navigation', async () => { + const workPackage = renderWorkPackage(); + + await nextFrame(); + workPackage.dispatchEvent(new MouseEvent('click', { bubbles: true })); + + expect(workPackage.hasAttribute('data-activating')).toBe(true); + expect(workPackage.hasAttribute('aria-current')).toBe(false); + expect(navigation.openSplitPane).not.toHaveBeenCalled(); + }); + + it('shows pressed feedback synchronously on Enter', async () => { + const workPackage = renderWorkPackage(); + + await nextFrame(); + keydown(workPackage, 'Enter'); + + expect(workPackage.hasAttribute('data-activating')).toBe(true); + expect(workPackage.hasAttribute('aria-current')).toBe(false); + }); + + it('clears pressed feedback when the visit lands on the card', async () => { + const workPackage = renderWorkPackage(); + + await nextFrame(); + workPackage.dispatchEvent(new MouseEvent('click', { bubbles: true })); + document.dispatchEvent(new CustomEvent('turbo:visit', { + detail: { url: '/projects/demo/backlogs/details/SP-42' }, + })); + + expect(workPackage.hasAttribute('data-activating')).toBe(false); + expect(workPackage.getAttribute('aria-current')).toBe('true'); + }); + + it('clears pressed feedback when the visit lands elsewhere', async () => { + const workPackage = renderWorkPackage(); + + await nextFrame(); + workPackage.dispatchEvent(new MouseEvent('click', { bubbles: true })); + document.dispatchEvent(new CustomEvent('turbo:visit', { + detail: { url: '/projects/demo/backlogs' }, + })); + + expect(workPackage.hasAttribute('data-activating')).toBe(false); + expect(workPackage.hasAttribute('aria-current')).toBe(false); + }); + + it('clears pressed feedback when a double-click cancels the pending click', async () => { + const workPackage = renderWorkPackage(); + + await nextFrame(); + workPackage.dispatchEvent(new MouseEvent('click', { bubbles: true })); + workPackage.dispatchEvent(new MouseEvent('dblclick', { bubbles: true })); + + expect(workPackage.hasAttribute('data-activating')).toBe(false); + expect(navigation.openFullPane).toHaveBeenCalledTimes(1); + }); + + it('clears pressed feedback when the card disconnects', async () => { + const workPackage = renderWorkPackage(); + + await nextFrame(); + workPackage.dispatchEvent(new MouseEvent('click', { bubbles: true })); + fixture.remove(); + await nextFrame(); + + expect(workPackage.hasAttribute('data-activating')).toBe(false); + }); + }); + it('marks the card as current when the URL points at it', async () => { const workPackage = renderWorkPackage(); diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts index 7821095d7000..454360176316 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts @@ -72,9 +72,16 @@ export default class WorkPackageController extends Controller imple clearTimeout(this.clickTimeout); this.clickTimeout = null; } + + // A card that reconnects must not come back pressed. + this.unmarkAsActivating(); } private syncCurrentFromUrl(locationUrl:string):void { + // However the visit resolved, the pressed state hands over to + // aria-current or to nothing. + this.unmarkAsActivating(); + const { pathname } = new URL(locationUrl, window.location.origin); const [, id] = DETAILS_URL_PATTERN.exec(pathname) ?? []; // Bookmarks and external links may still carry a numeric ID after the @@ -88,7 +95,10 @@ export default class WorkPackageController extends Controller imple // Not set optimistically: activation waits out the double-click delay below // and may resolve to the full view instead, so asserting a current work - // package here would announce a navigation that may never happen. + // package here would announce a navigation that may never happen. Feedback + // is visual only: data-activating goes on synchronously and carries no ARIA + // semantics, since the card is an article and role=button was rejected in + // AGILE-251. markAsCurrent():void { this.element.setAttribute('aria-current', 'true'); } @@ -97,6 +107,14 @@ export default class WorkPackageController extends Controller imple this.element.removeAttribute('aria-current'); } + markAsActivating():void { + this.element.setAttribute('data-activating', ''); + } + + unmarkAsActivating():void { + this.element.removeAttribute('data-activating'); + } + handleEvent(event:Event):void { switch (event.type) { case 'click': @@ -119,6 +137,7 @@ export default class WorkPackageController extends Controller imple if (this.clickTimeout !== null) return; + this.markAsActivating(); this.clickTimeout = window.setTimeout(() => { this.clickTimeout = null; this.openSplitPane(); @@ -134,6 +153,7 @@ export default class WorkPackageController extends Controller imple if (this.clickTimeout !== null) { clearTimeout(this.clickTimeout); this.clickTimeout = null; + this.unmarkAsActivating(); } this.openFullPane(); @@ -149,6 +169,7 @@ export default class WorkPackageController extends Controller imple event.preventDefault(); + this.markAsActivating(); if (event.shiftKey) { this.openFullPane(); } else { 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 814d026f2a71..5d47dc6dfc1c 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -54,6 +54,7 @@ vi.mock('@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-previe setCustomNativeDragPreview: vi.fn(), })); +import { attachClosestEdge } from '@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge'; import type { monitorForElements as monitorForElementsFn } from '@atlaskit/pragmatic-drag-and-drop/element/adapter'; import { waitFor } from '@testing-library/dom'; import { type Mock, type MockInstance } from 'vitest'; @@ -90,7 +91,7 @@ describe('Sortable lists controller', () => { ({ sortableItemData, sortableListData } = await import('./sortable-lists/drag-and-drop')); }); - function input() { + function input({ clientY = 10 }:{ clientY?:number } = {}) { return { altKey: false, button: 0, @@ -99,9 +100,26 @@ describe('Sortable lists controller', () => { metaKey: false, shiftKey: false, clientX: 10, - clientY: 10, + clientY, pageX: 10, - pageY: 10, + pageY: clientY, + }; + } + + // A fixed-size hit box for attachClosestEdge to resolve 'top' or 'bottom' + // against; paired with input({ clientY }) below (10 reads as 'top', 90 as + // 'bottom' against this box). + function rect():DOMRect { + return { + top: 0, + bottom: 100, + left: 0, + right: 100, + width: 100, + height: 100, + x: 0, + y: 0, + toJSON: () => ({}), }; } @@ -122,7 +140,13 @@ describe('Sortable lists controller', () => { moveUrlTemplate = '/move/{id}', optimistic = false, selectionEnabled = false, - }:{ moveUrlTemplate?:string|null; optimistic?:boolean; selectionEnabled?:boolean } = {}) { + collectionMoveUrl = null, + }:{ + moveUrlTemplate?:string|null; + optimistic?:boolean; + selectionEnabled?:boolean; + collectionMoveUrl?:string|null; + } = {}) { fixture.innerHTML = `
{ ${moveUrlTemplate ? `data-sortable-lists-move-url-template-value="${moveUrlTemplate}"` : ''} ${optimistic ? 'data-sortable-lists-optimistic-value="true"' : ''} ${selectionEnabled ? 'data-sortable-lists-selection-enabled-value="true"' : ''} + ${collectionMoveUrl ? `data-sortable-lists-collection-move-url-value="${collectionMoveUrl}"` : ''} data-sortable-lists-sortable-lists--list-outlet="#sortable-root [data-controller~='sortable-lists--list']" data-sortable-lists-sortable-lists--item-outlet="#sortable-root [data-controller~='sortable-lists--item']" data-sortable-lists-sortable-lists--scrollable-outlet="#sortable-root [data-controller~='sortable-lists--scrollable']" @@ -156,11 +181,12 @@ describe('Sortable lists controller', () => { async function dropCurrentItemOnList(sourceElement:HTMLElement, list:HTMLElement, type = 'work_package') { const monitorOptions = vi.mocked(monitorForElements).mock.lastCall?.[0]; + const rootElement = sourceElement.closest('[data-controller="sortable-lists"]'); monitorOptions?.onDrop?.({ source: sourcePayload( sourceElement, - itemData(sourceElement.getAttribute('data-sortable-lists--item-id-value')!, type), + itemData(sourceElement.getAttribute('data-sortable-lists--item-id-value')!, type, rootElement), ), location: { initial: { @@ -189,8 +215,8 @@ describe('Sortable lists controller', () => { await flushPromises(); } - function itemData(itemId = '1', type = 'work_package') { - return sortableItemData({ itemId, type }); + function itemData(itemId = '1', type = 'work_package', rootElement:HTMLElement|null = null) { + return sortableItemData({ itemId, type, rootElement }); } function sourcePayload(element:HTMLElement, data:Record = itemData()) { @@ -332,8 +358,16 @@ describe('Sortable lists controller', () => { // is itself focusable. function renderSelectableRoot({ moveUrlTemplate = '/move/{id}', - }:{ moveUrlTemplate?:string|null } = {}) { - const fixtureElements = renderFixture({ moveUrlTemplate, selectionEnabled: true }); + optimistic = false, + collectionMoveUrl = null, + }:{ + moveUrlTemplate?:string|null; + optimistic?:boolean; + collectionMoveUrl?:string|null; + } = {}) { + const fixtureElements = renderFixture({ + moveUrlTemplate, selectionEnabled: true, optimistic, collectionMoveUrl, + }); fixtureElements.items.forEach((item) => item.setAttribute('tabindex', '0')); return fixtureElements; @@ -376,13 +410,35 @@ describe('Sortable lists controller', () => { window.I18n.store({ en: { js: { + // Distinct wording, so a test asserting the consumer scope was + // consulted cannot pass against the default scope by accident. + backlogs: { + announcements: { + batch_too_large: '[backlogs_batch_too_large:%{count}:%{max}]', + fallback_item_label: 'Work package', + fallback_list_name: 'another list', + move_failed_check_position: 'Move failed. Check the work package\'s current position.', + move_failed_check_positions_batch: { other: 'Move failed. Check the work packages\' current positions.' }, + move_failed_rolled_back: 'Move failed. %{label} returned to its previous position.', + move_failed_rolled_back_batch: { other: 'Move failed. %{count} work packages returned to their previous positions.' }, + moved: '%{label} work package moved to position %{position} of %{total}', + moved_batch: { other: '%{count} work packages moved to positions %{first} through %{last} of %{total}' }, + moved_batch_to_list: { other: '%{count} work packages moved to %{list}, positions %{first} through %{last} of %{total}' }, + moved_to_list: '%{label} moved to %{list}, position %{position} of %{total}', + }, + }, sortable_lists: { announcements: { + batch_too_large: '[batch_too_large:%{count}:%{max}]', fallback_item_label: 'Item', fallback_list_name: 'another list', move_failed_check_position: 'Move failed. Check the item\'s current position.', + move_failed_check_positions_batch: { other: 'Move failed. Check the items\' current positions.' }, move_failed_rolled_back: 'Move failed. %{label} returned to its previous position.', + move_failed_rolled_back_batch: { other: 'Move failed. %{count} items returned to their previous positions.' }, moved: '%{label} moved to position %{position} of %{total}', + moved_batch: { other: '%{count} items moved to positions %{first} through %{last} of %{total}' }, + moved_batch_to_list: { other: '%{count} items moved to %{list}, positions %{first} through %{last} of %{total}' }, moved_to_list: '%{label} moved to %{list}, position %{position} of %{total}', }, selection: selectionTranslations, @@ -1184,15 +1240,6 @@ describe('Sortable lists controller', () => { expect(controller.moveAvailability(document.createElement('li'))).toBeNull(); }); - it('resolves the owning list element of an item for the drag payload', async () => { - const { root, sourceList, firstSourceItem } = renderFixture(); - await ctx.nextFrame(); - const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; - - expect(controller.ownerListElementOf(firstSourceItem)).toBe(sourceList); - expect(controller.ownerListElementOf(document.createElement('li'))).toBeNull(); - }); - describe('nested list topology', () => { it('resolves the source row of a nested item against its innermost list', async () => { const { fieldList, firstFieldItem } = renderNestedFixture(); @@ -1292,21 +1339,47 @@ describe('Sortable lists controller', () => { }); }); - // Collapsing a batch and selecting the dragged card are different things: - // with nothing selected, a drag must not manufacture a one-card batch. - it('leaves an empty selection empty when a drag starts with nothing selected', async () => { + // A drag selects the dragged card when nothing was selected, so a + // cancelled drag leaves the same state either way. + it('selects the dragged card when a drag starts with nothing selected', async () => { const { root, firstSourceItem } = renderSelectableRoot(); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; - controller.collapseSelectionForDrag(firstSourceItem); + controller.freezeDragBatch(firstSourceItem); + + expect(document.querySelectorAll('[data-batch-selected]')).toHaveLength(1); + expect(firstSourceItem.hasAttribute('data-batch-selected')).toBe(true); + }); + + // Unlike a drag: a failed menu move would otherwise leave behind a + // selection the user never made. + it('selects nothing for a menu move with nothing selected', async () => { + const { root, firstSourceItem } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + + controller.moveInDirection(firstSourceItem, 'down'); + + 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(document.querySelector('[data-batch-selected]')).toBeNull(); + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0]]); }); - // A drag that narrows a larger batch to one card is a count change a - // screen-reader user has to hear. - it('announces the new count when a drag collapses a multi-card batch', async () => { + // 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. + it('announces the new count when dragging a card outside the batch collapses it', async () => { const { root, items } = renderSelectableRoot(); await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; @@ -1315,9 +1388,9 @@ describe('Sortable lists controller', () => { items[2].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true })); announceSpy.mockClear(); - controller.collapseSelectionForDrag(items[0]); + controller.freezeDragBatch(items[3]); - expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0]]); + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[3]]); expect(announceSpy.mock.calls.map((call) => [call[0], call[1]])).toEqual([ ['[selected:1]', { politeness: 'polite' }], ]); @@ -2126,6 +2199,42 @@ describe('Sortable lists controller', () => { expect(items.some(isSelected)).toBe(false); }); + describe('batch cap', () => { + it('refuses a drag whose batch exceeds the cap and announces it', async () => { + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); + root.setAttribute('data-sortable-lists-max-batch-size-value', '2'); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + click(items[0]); click(items[1], { ctrlKey: true }); click(items[2], { ctrlKey: true }); + announceSpy.mockClear(); + + expect(controller.dragRefused(items[0])).toBe(true); + expect(announceSpy).toHaveBeenCalledWith('[batch_too_large:3:2]', { politeness: 'assertive' }); + expect(controller.dragRefused(items[4])).toBe(false); + }); + + it('never refuses without a cap', async () => { + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + click(items[0]); click(items[1], { ctrlKey: true }); click(items[2], { ctrlKey: true }); + + expect(controller.dragRefused(items[0])).toBe(false); + }); + + it('does not refuse a batch exactly at the cap', async () => { + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); + root.setAttribute('data-sortable-lists-max-batch-size-value', '3'); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + click(items[0]); click(items[1], { ctrlKey: true }); click(items[2], { ctrlKey: true }); + announceSpy.mockClear(); + + expect(controller.dragRefused(items[0])).toBe(false); + expect(announceSpy).not.toHaveBeenCalled(); + }); + }); + function morphRoot(root:HTMLElement) { root.dispatchEvent(new CustomEvent('turbo:morph-element', { bubbles: true })); } @@ -2231,7 +2340,7 @@ describe('Sortable lists controller', () => { expect(announceSpy).not.toHaveBeenCalled(); }); - // selectedIds() filters to elements still in the document, so it would + // selectedItems() filters to elements still in the document, so it would // pass even with the model unpruned. The anchor is the one place an // unpruned model is observable: a dangling one makes the Shift+click // below report an unavailable range instead of restarting the selection. @@ -2250,4 +2359,690 @@ describe('Sortable lists controller', () => { expect(isSelected(items[2])).toBe(true); }); }); + + describe('batch dragging', () => { + let root:HTMLElement; + let list1:HTMLElement; + let list2:HTMLElement; + let item1:HTMLElement; + let item2:HTMLElement; + let item3:HTMLElement; + let controller:SortableListsControllerType; + + beforeEach(async () => { + const fixtureElements = renderSelectableRoot({ + moveUrlTemplate: '/move/{id}', + optimistic: true, + collectionMoveUrl: '/collection-move-url', + }); + root = fixtureElements.root; + list1 = fixtureElements.sourceList; + list2 = fixtureElements.targetList; + item1 = list1.querySelector('[data-sortable-lists--item-id-value="1"]')!; + item2 = list1.querySelector('[data-sortable-lists--item-id-value="2"]')!; + item3 = list1.querySelector('[data-sortable-lists--item-id-value="3"]')!; + + await ctx.nextFrame(); + controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + }); + + function selectItems(...selected:HTMLElement[]) { + click(selected[0]); + selected.slice(1).forEach((item) => click(item, { ctrlKey: true })); + } + + function rowIdsIn(list:HTMLElement):string[] { + return itemIds(list); + } + + function selectedRowIds():string[] { + return Array.from(root.querySelectorAll('[data-batch-selected]')) + .map((element) => element.getAttribute('data-sortable-lists--item-id-value')!); + } + + // Mirrors item.controller.ts's onGenerateDragPreview and onDragStart: + // the root freezes the batch this drag represents, then marks its rows, + // before anything else can happen to it. + function beginDrag(source:HTMLElement) { + controller.freezeDragBatch(source); + controller.markDragBatch(); + } + + function batchDropTargets({ targetList, targetItem, edge }:{ + targetList:HTMLElement; + targetItem:HTMLElement|null; + edge:'top'|'bottom'|null; + }) { + const dropTargets:ReturnType[] = []; + + if (targetItem && edge) { + vi.spyOn(targetItem, 'getBoundingClientRect').mockReturnValue(rect()); + const targetItemId = targetItem.getAttribute('data-sortable-lists--item-id-value')!; + const data = attachClosestEdge(sortableItemData({ itemId: targetItemId, type: 'work_package' }), { + element: targetItem, + input: input({ clientY: edge === 'bottom' ? 90 : 10 }), + allowedEdges: ['top', 'bottom'], + }); + dropTargets.push(dropTargetRecord(targetItem, data)); + } + + dropTargets.push(dropTargetRecord(targetList, sortableListData({ + type: targetList.getAttribute('data-sortable-lists--list-type-value')!, + listId: targetList.getAttribute('data-sortable-lists--list-id-value'), + name: targetList.getAttribute('data-sortable-lists--list-name-value'), + }))); + + return dropTargets; + } + + // The second half of simulateDrop, split out so a test can mutate the + // DOM between drag start (beginDrag) and this. + async function completeDrop({ source, targetList, targetItem, edge }:{ + source:HTMLElement; + targetList:HTMLElement; + targetItem:HTMLElement|null; + edge:'top'|'bottom'|null; + }) { + const monitorOptions = vi.mocked(monitorForElements).mock.lastCall?.[0]; + const sourceId = source.getAttribute('data-sortable-lists--item-id-value')!; + + monitorOptions?.onDrop?.({ + source: sourcePayload(source, itemData(sourceId, 'work_package', root)), + location: { + initial: { dropTargets: [], input: input() }, + current: { dropTargets: batchDropTargets({ targetList, targetItem, edge }), input: input() }, + previous: { dropTargets: [] }, + }, + }); + + await flushPromises(); + } + + async function simulateDrop(args:{ + source:HTMLElement; + targetList:HTMLElement; + targetItem:HTMLElement|null; + edge:'top'|'bottom'|null; + }) { + beginDrag(args.source); + await completeDrop(args); + } + + // A drag released outside every registered drop target still fires + // onDrop, with no targets for resolveDropIntent to work from. + async function simulateCancelledDrop({ source }:{ source:HTMLElement }) { + beginDrag(source); + const monitorOptions = vi.mocked(monitorForElements).mock.lastCall?.[0]; + const sourceId = source.getAttribute('data-sortable-lists--item-id-value')!; + + monitorOptions?.onDrop?.({ + source: sourcePayload(source, itemData(sourceId, 'work_package', root)), + location: { + initial: { dropTargets: [], input: input() }, + current: { dropTargets: [], input: input() }, + previous: { dropTargets: [] }, + }, + }); + + await flushPromises(); + } + + // Item drop targets ask on every dragover; the owner is settled for the + // drag at its start and forgotten with the frozen batch. + describe('ownerDestinationOf', () => { + let list2Rows:HTMLElement; + + const destinationOf = (list:HTMLElement) => ({ + type: list.getAttribute('data-sortable-lists--list-type-value')!, + id: list.getAttribute('data-sortable-lists--list-id-value'), + }); + + beforeEach(() => { + list2Rows = list2.querySelector('[data-sortable-lists--item-id-value="4"]')!.parentElement!; + }); + + function cancelDrag(source:HTMLElement) { + vi.mocked(monitorForElements).mock.lastCall?.[0].onDrop?.({ + source: sourcePayload(source), + location: { + initial: { dropTargets: [], input: input() }, + current: { dropTargets: [], input: input() }, + previous: { dropTargets: [] }, + }, + }); + } + + it('remembers the owner for the drag', () => { + beginDrag(item1); + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list1)); + + list2Rows.append(item2); + + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list1)); + }); + + it('forgets the owner once the drag ends', () => { + beginDrag(item1); + controller.ownerDestinationOf(item2); + list2Rows.append(item2); + + cancelDrag(item1); + + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list2)); + }); + + it('answers live outside a drag', () => { + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list1)); + + list2Rows.append(item2); + + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list2)); + }); + }); + + // The destination policy is applied over the whole batch, and a batch may + // span lists: one confined member pins the block to the list it already + // sits in, wherever the dragged card itself is. + describe('confined batch-mates', () => { + let item4:HTMLElement; + + const destinationOf = (list:HTMLElement) => ({ + type: list.getAttribute('data-sortable-lists--list-type-value')!, + id: list.getAttribute('data-sortable-lists--list-id-value'), + }); + + beforeEach(() => { + item4 = list2.querySelector('[data-sortable-lists--item-id-value="4"]')!; + item3.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); + }); + + async function completeConfinedDrop({ source = item1, targetList, targetItem, edge }:{ + source?:HTMLElement; + targetList:HTMLElement; + targetItem:HTMLElement|null; + edge:'top'|'bottom'|null; + }) { + const monitorOptions = vi.mocked(monitorForElements).mock.lastCall?.[0]; + const sourceId = source.getAttribute('data-sortable-lists--item-id-value')!; + + monitorOptions?.onDrop?.({ + source: sourcePayload(source, sortableItemData({ + itemId: sourceId, + type: 'work_package', + rootElement: root, + permittedDestinations: controller.dragPermittedDestinations(source), + })), + location: { + initial: { dropTargets: [], input: input() }, + current: { dropTargets: batchDropTargets({ targetList, targetItem, edge }), input: input() }, + previous: { dropTargets: [] }, + }, + }); + + await flushPromises(); + } + + it('permits every list while no member is confined', () => { + selectItems(item1, item2); + + expect(controller.dragPermittedDestinations(item1)).toBeNull(); + }); + + it('pins the drag to the list a selected confined batch-mate sits in', () => { + selectItems(item1, item3); + + expect(controller.dragPermittedDestinations(item1)).toEqual([destinationOf(list1)]); + }); + + it('pins the drag to a confined batch-mate in another list', () => { + item4.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); + selectItems(item1, item4); + + expect(controller.dragPermittedDestinations(item1)).toEqual([destinationOf(list2)]); + }); + + it('permits nothing while confined members disagree on their list', () => { + item4.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); + selectItems(item3, item4); + + expect(controller.dragPermittedDestinations(item3)).toEqual([]); + }); + + it('does not pin the drag while the confined card is unselected', () => { + selectItems(item1, item2); + + expect(controller.dragPermittedDestinations(item1)).toBeNull(); + }); + + it('pins the confined card itself without any selection', () => { + expect(controller.dragPermittedDestinations(item3)).toEqual([destinationOf(list1)]); + }); + + // Unreachable through a drag today, since a fixed card registers no + // draggable and cannot join a selection, but the lists follow the same + // policy the menus read rather than a confinement test of their own. + it('permits nothing for a fixed card', () => { + item2.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + + expect(controller.dragPermittedDestinations(item2)).toEqual([]); + }); + + it('refuses a cross-list drop of a batch with a confined member', async () => { + const targetListIdsBefore = rowIdsIn(list2); + selectItems(item1, item3); + beginDrag(item1); + + await completeConfinedDrop({ targetList: list2, targetItem: null, edge: null }); + + expect(fetchMock).not.toHaveBeenCalled(); + expect(rowIdsIn(list1)).toEqual(['1', '2', '3']); + expect(rowIdsIn(list2)).toEqual(targetListIdsBefore); + }); + + it('still reorders a batch with a confined member within its list', async () => { + selectItems(item1, item3); + beginDrag(item1); + + await completeConfinedDrop({ targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(fetchMock).toHaveBeenCalled(); + expect(rowIdsIn(list1)).toEqual(['2', '1', '3']); + }); + + // The reorder the dragged card's own list would accept on its own: the + // batch-mate it carries cannot follow it there. + it('refuses a reorder in the dragged card\'s list while a mate is confined elsewhere', async () => { + item4.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); + selectItems(item1, item4); + beginDrag(item1); + + await completeConfinedDrop({ targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(fetchMock).not.toHaveBeenCalled(); + expect(rowIdsIn(list1)).toEqual(['1', '2', '3']); + }); + + // The mirror of the refusal above, and a drop the server accepts: only + // the free member changes list. The block lands at the start, since the + // row it was dropped against is itself a member and cannot anchor it. + it('accepts a drop in the list its confined mate already occupies', async () => { + item4.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); + selectItems(item1, item4); + beginDrag(item1); + + await completeConfinedDrop({ targetList: list2, targetItem: item4, edge: 'bottom' }); + + expect(fetchMock).toHaveBeenCalled(); + expect(rowIdsIn(list2)).toEqual(['1', '4', '5']); + }); + }); + + // getInitialDataForExternal reads this before the batch is frozen, so it + // has to answer from the live selection rather than the frozen snapshot. + describe('externalDragItems', () => { + it('returns just the card while nothing is selected', () => { + expect(controller.externalDragItems(item1)).toEqual([item1]); + }); + + it('returns every batch member when the card is part of a selection', () => { + selectItems(item1, item3); + + expect(controller.externalDragItems(item1)).toEqual([item1, item3]); + }); + + it('returns just the card when it is not part of the selection', () => { + selectItems(item3); + + expect(controller.externalDragItems(item1)).toEqual([item1]); + }); + + it('does not touch the selection', () => { + selectItems(item1, item3); + + controller.externalDragItems(item1); + + expect(selectedRowIds()).toEqual(['1', '3']); + }); + }); + + // Ids are unique per source table; a nested list of another type can + // hold a colliding one, and the batch must never claim it. + it('leaves a same-id row of another type unmarked by the drag batch', async () => { + const collidingRow = document.createElement('li'); + collidingRow.setAttribute('data-controller', 'sortable-lists--item'); + collidingRow.setAttribute('data-sortable-lists--item-id-value', '1'); + collidingRow.setAttribute('data-sortable-lists--item-type-value', 'section'); + list2.appendChild(collidingRow); + await ctx.nextFrame(); + + selectItems(item1, item3); + beginDrag(item1); + + expect(item1.hasAttribute('data-dragging')).toBe(true); + expect(item3.hasAttribute('data-dragging')).toBe(true); + expect(collidingRow.hasAttribute('data-dragging')).toBe(false); + }); + + it('moves every selected row and PUTs ordered ids to the collection URL', async () => { + selectItems(item1, item3); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + const url = fetchMock.mock.calls[0][0] as string; + const options = fetchMock.mock.calls[0][1] as { body:FormData }; + expect(url).toContain('/collection-move-url'); + expect(url).toContain('optimistic=true'); + const body = options.body; + expect(body.getAll('ids[]')).toEqual(['1', '3']); + expect(body.get('prev_id')).toBe('2'); + // both rows moved contiguously after item 2: + expect(rowIdsIn(list1)).toEqual(['2', '1', '3']); + }); + + it('drags an unselected card alone through the collection URL', async () => { + // select item 3, drag item 2: it is not part of the batch, so it + // collapses any selection onto itself and moves alone. + selectItems(item3); + + await simulateDrop({ source: item2, targetList: list1, targetItem: item1, edge: 'top' }); + + const body = (fetchMock.mock.calls[0][1].body) as FormData; + expect(body.getAll('ids[]')).toEqual(['2']); + }); + + // Batch = {1, 3}. Dropping "top" on item 2 asks for its predecessor, and + // the only candidate — item 1 — is a batch member, so the walk has to + // fall through past it to blank rather than return '1'. + it('excludes selected rows when resolving the predecessor', async () => { + selectItems(item1, item3); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'top' }); + + const body = (fetchMock.mock.calls[0][1].body) as FormData; + expect(body.get('prev_id')).toBe(''); + }); + + it('suppresses a block no-op without a request', async () => { + // select 1 and 2 (already contiguous at top), drop 1 at the top again. + selectItems(item1, item2); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'top' }); + + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('rolls the whole block back on failure and keeps the selection', async () => { + selectItems(item1, item3); + fetchMock.mockResolvedValueOnce(new Response('', { status: 500 })); + + await simulateDrop({ source: item1, targetList: list2, targetItem: null, edge: null }); + + expect(rowIdsIn(list1)).toEqual(['1', '2', '3']); + // selection preserved for retry: + expect(selectedRowIds()).toEqual(['1', '3']); + }); + + it('clears the frozen batch when a drag is cancelled', async () => { + // simulate a drop that resolves no intent (dropTargets: []), then a + // fresh singular drag of item 2 — the stale batch must not leak in. + await simulateCancelledDrop({ source: item1 }); + await simulateDrop({ source: item2, targetList: list1, targetItem: item3, edge: 'bottom' }); + + const body = (fetchMock.mock.calls[0][1].body) as FormData; + expect(body.getAll('ids[]')).toEqual(['2']); + }); + + it('clears the selection after a successful move', async () => { + // select 1 and 3, successful batch drop: + selectItems(item1, item3); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(selectedRowIds()).toEqual([]); + }); + + it('keeps the selection when the move fails', async () => { + selectItems(item1, item3); + fetchMock.mockResolvedValueOnce(new Response('', { status: 500 })); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(selectedRowIds()).toEqual(['1', '3']); + }); + + it('aborts when a frozen member row vanished mid-drag', async () => { + // select 1 and 3; begin the drag of item 1; remove item 3's row from + // the DOM (as a mid-drag morph would); then complete the drop. + selectItems(item1, item3); + beginDrag(item1); + item3.remove(); + + await completeDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + // No request, no partial DOM move: + expect(fetchMock).not.toHaveBeenCalled(); + expect(rowIdsIn(list1)).toEqual(['1', '2']); + + // The snapshot was still consumed — a following singular drag is clean: + await simulateDrop({ source: item2, targetList: list1, targetItem: item1, edge: 'top' }); + const body = (fetchMock.mock.calls[0][1].body) as FormData; + expect(body.getAll('ids[]')).toEqual(['2']); + }); + + it('announces when a frozen member row vanished mid-drag', async () => { + selectItems(item1, item3); + beginDrag(item1); + item3.remove(); + announceSpy.mockClear(); + + await completeDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(announceSpy).toHaveBeenCalledWith( + 'Move failed. Check the items\' current positions.', + { politeness: 'assertive' }, + ); + }); + + describe('drag presentation', () => { + function draggingIds():string[] { + return Array.from(root.querySelectorAll('[data-dragging]')) + .map((element) => element.getAttribute('data-sortable-lists--item-id-value')!); + } + + it('marks every selected row as the drag source when the batch begins', () => { + selectItems(item1, item3); + + beginDrag(item1); + + expect(draggingIds().sort()).toEqual(['1', '3']); + }); + + it('marks only the dragged row when it is not part of a selection', () => { + selectItems(item3); + + beginDrag(item2); + + expect(draggingIds()).toEqual(['2']); + }); + + it('returns the batch size from freezeDragBatch', () => { + selectItems(item1, item3); + + expect(controller.freezeDragBatch(item1)).toBe(2); + expect(controller.freezeDragBatch(item2)).toBe(1); + }); + + it('clears every dragging mark after a completed drop', async () => { + selectItems(item1, item3); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); + }); + + it('clears every dragging mark after a cancelled drop', async () => { + selectItems(item1, item3); + + await simulateCancelledDrop({ source: item1 }); + + expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); + }); + + it('sweeps dragging marks defensively on disconnect', () => { + selectItems(item1, item3); + beginDrag(item1); + + controller.disconnect(); + + expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); + // Frozen batch nulled, not just its marks cleared: markDragBatch + // has nothing to mark. + controller.markDragBatch(); + expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); + }); + + // A stray mark on an element the batch never touched stands in for a + // row Pragmatic's own onDrop cleanup never reached. + it('sweeps a leftover mark from a row outside the frozen batch on drop', async () => { + selectItems(item1, item3); + item2.setAttribute('data-dragging', 'source'); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); + }); + + // A morph can replace a batch-mate's row with a fresh element that + // never went through markDraggingRows, so it arrives unmarked while + // still part of the frozen batch. + it('re-marks a batch-mate row a mid-drag morph replaced', async () => { + selectItems(item1, item3); + beginDrag(item1); + + // A morph-replaced node arrives from server HTML without the + // in-memory mark, which the clone would otherwise inherit. + const replacement = item3.cloneNode(true) as HTMLElement; + replacement.removeAttribute('data-dragging'); + item3.replaceWith(replacement); + replacement.dispatchEvent(new CustomEvent('turbo:morph-element', { bubbles: true })); + await Promise.resolve(); + + expect(replacement.getAttribute('data-dragging')).toBe('source'); + + await completeDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); + }); + }); + + describe('batch announcements', () => { + it('announces one batch movement with the block position range', async () => { + // a fourth row so the block's position range (2 through 3) reads + // distinctly from the list's total (4). + list1.append(itemRow('9')); + selectItems(item1, item3); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + // list1 reads ['2', '1', '3', '9'] afterwards: the batch lands after + // item 2, at positions 2 and 3 of 4. + expect(announceSpy).toHaveBeenCalledWith( + expect.stringContaining('2 items moved to positions 2 through 3 of 4'), + expect.anything(), + ); + }); + + // Set post-connect: Stimulus Values read the attribute live, so a + // synchronous set-then-drop in one test is safe. + it('speaks the consumer scope when moveAnnouncementScope is set', async () => { + root.setAttribute('data-sortable-lists-move-announcement-scope-value', 'js.backlogs.announcements'); + + // Nothing selected, so the single dragged card moves alone through + // the collection URL: the singular wording path. + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(announceSpy).toHaveBeenCalledWith( + expect.stringContaining('work package moved to position'), + expect.anything(), + ); + }); + }); + + describe('failure announcements', () => { + it('announces the check-positions warning on a 422 whose rollback is unverified', async () => { + selectItems(item1, item3); + let resolveMove:(response:Response) => void; + fetchMock.mockImplementationOnce(() => new Promise((resolve) => { + resolveMove = resolve; + })); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + // Removing a batch row between the request being issued and + // resolving, as a concurrent morph would, leaves rowsRemainAt unable + // to confirm the block, so the rollback is skipped. + item3.remove(); + resolveMove!(new Response('', { status: 422 })); + await flushPromises(); + + expect(announceSpy).toHaveBeenCalledWith( + expect.stringContaining('Check the items'), + expect.anything(), + ); + }); + + it('stays silent on a 422 whose rollback verified', async () => { + selectItems(item1, item3); + fetchMock.mockResolvedValueOnce(new Response('', { status: 422 })); + + await simulateDrop({ source: item1, targetList: list1, targetItem: item2, edge: 'bottom' }); + + expect(announceSpy).not.toHaveBeenCalledWith( + expect.stringContaining('Move failed'), expect.anything(), + ); + }); + }); + }); + + // The collection URL alone picks the contract: a root that renders one + // sends a lone card through it even when nothing can be selected. + describe('drop route', () => { + it('moves a single card through the collection URL on a root without selection', async () => { + const { root, sourceList } = renderFixture({ collectionMoveUrl: '/collection-move-url' }); + const item1 = sourceList.querySelector('[data-sortable-lists--item-id-value="1"]')!; + const item2 = sourceList.querySelector('[data-sortable-lists--item-id-value="2"]')!; + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + controller.freezeDragBatch(item2); + controller.markDragBatch(); + + vi.spyOn(item1, 'getBoundingClientRect').mockReturnValue(rect()); + const targetData = attachClosestEdge(sortableItemData({ itemId: '1', type: 'work_package' }), { + element: item1, + input: input({ clientY: 10 }), + allowedEdges: ['top', 'bottom'], + }); + vi.mocked(monitorForElements).mock.lastCall?.[0].onDrop?.({ + source: sourcePayload(item2, itemData('2', 'work_package', root)), + location: { + initial: { dropTargets: [], input: input() }, + current: { + dropTargets: [ + dropTargetRecord(item1, targetData), + dropTargetRecord(sourceList, sortableListData({ type: 'backlog_bucket', listId: '1', name: 'Product backlog' })), + ], + input: input(), + }, + previous: { dropTargets: [] }, + }, + }); + await flushPromises(); + + const url = fetchMock.mock.calls[0][0] as string; + const body = fetchMock.mock.calls[0][1].body as FormData; + expect(url).toContain('/collection-move-url'); + expect(body.getAll('ids[]')).toEqual(['2']); + expect(itemIds(sourceList)).toEqual(['2', '1', '3']); + }); + }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index 6458d539bbe9..c4f96655881e 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -39,15 +39,18 @@ import { flipMove } from 'core-stimulus/helpers/flip-helper'; import { parseTemplate } from 'url-template'; import { buildMoveFormData, - isSortableItemData, + isItemFromRoot, resolveDropIntent, + singleItemBatch, type RootAwareChild, type SortableListData, type SortableListsRoot, } from './sortable-lists/drag-and-drop'; +import { selectionKey, type SelectionItem, type SelectionKey } from 'core-common/batch-selection'; import { captureRowPositions, isOrderableItem, + itemAcceptsDestination, reorderRows, resolveDirectionalPreviousItemId, resolveItemId, @@ -59,26 +62,39 @@ import { rowOf, rowsRemainAt, sortableListsBusyAttribute, + type DestinationIdentity, type MoveAvailability, type MoveDirection, } from './sortable-lists/list-dom'; import { SelectionOrchestrator, type SelectionHost } from './sortable-lists/selection-orchestrator'; +import { itemIdentity, orderedItemElements } from './sortable-lists/selection'; type CleanupFn = () => void; type ElementDropPayload = ElementEventPayloadMap['onDrop']; type MoveResult = { ok:true }|{ ok:false; showToast:boolean }; +// items is null on the single-item contract, the batch on the collection one. +interface DropRoute { url:string; items:SelectionItem[]|null } interface MoveAnnouncementContext { label:string|null; listName:string|null; crossList:boolean } +// Reduced to a same-origin relative URL: an absolute or foreign-origin +// template would otherwise be submitted as given. +function relativeUrl(url:URL):string { + return `${url.pathname}${url.search}${url.hash}`; +} + export default class SortableListsController extends Controller implements SortableListsRoot, SelectionHost { static outlets = ['sortable-lists--list', 'sortable-lists--item', 'sortable-lists--scrollable']; static values = { moveUrlTemplate: String, moveUrlTemplates: Object, + collectionMoveUrl: String, optimistic: { type: Boolean, default: false }, selectionEnabled: { type: Boolean, default: false }, announcementScope: { type: String, default: 'js.sortable_lists.selection' }, + moveAnnouncementScope: { type: String, default: 'js.sortable_lists.announcements' }, selectionDescriptionId: { type: String, default: '' }, + maxBatchSize: { type: Number, default: 0 }, }; declare readonly sortableListsListOutlets:import('./sortable-lists/list.controller').default[]; @@ -89,10 +105,14 @@ export default class SortableListsController extends Controller imp declare readonly hasMoveUrlTemplateValue:boolean; declare readonly moveUrlTemplatesValue:Record; declare readonly hasMoveUrlTemplatesValue:boolean; + declare readonly collectionMoveUrlValue:string; + declare readonly hasCollectionMoveUrlValue:boolean; declare readonly optimisticValue:boolean; declare readonly selectionEnabledValue:boolean; declare readonly announcementScopeValue:string; + declare readonly moveAnnouncementScopeValue:string; declare readonly selectionDescriptionIdValue:string; + declare readonly maxBatchSizeValue:number; private selection?:SelectionOrchestrator; @@ -103,8 +123,7 @@ export default class SortableListsController extends Controller imp connect():void { this.monitorCleanupFn = monitorForElements({ canMonitor: ({ source }) => !this.busy - && isSortableItemData(source.data) - && source.data.rootElement === this.element, + && isItemFromRoot(this.element, source.data), onDrop: (args) => { void this.handleDrop(args); }, @@ -124,6 +143,11 @@ export default class SortableListsController extends Controller imp this.teardownSelection(); this.monitorCleanupFn?.(); this.monitorCleanupFn = undefined; + // A drag in flight when the controller disconnects would otherwise leave + // its marks in the cached page and its frozen batch in this instance. + this.clearDraggingRows(); + this.activeDragBatch = null; + this.dragOwnerDestinations = null; } // A Turbo morph can toggle the permission-gated value on a live root @@ -210,13 +234,120 @@ export default class SortableListsController extends Controller imp } } - // Live ordered membership, for AGILE-278's batch move. - selectedIds():string[] { - return this.selection?.selectedIds() ?? []; + // Frozen at drag start and consumed exactly once per drop, cancelled ones + // included: neither Escape nor a mid-drag morph can change what is + // submitted, and no stale batch leaks into the next drag. + private activeDragBatch:SelectionItem[]|null = null; + + // Every item drop target asks for its owner on each dragover, and the + // answer holds for the whole drag, so it is remembered alongside the batch. + private dragOwnerDestinations:WeakMap|null = null; + + // Pragmatic dispatches onGenerateDragPreview before onDragStart; the + // preview needs the count, the drag start marks the rows. + freezeDragBatch(itemElement:HTMLElement):number { + const scope = this.selection?.selectForAction(itemElement); + this.activeDragBatch = scope?.kind === 'batch' + ? scope.items.map((item) => itemIdentity(item)).filter((item):item is SelectionItem => item !== null) + : null; + this.dragOwnerDestinations = new WeakMap(); + + return Math.max(1, this.activeDragBatch?.length ?? 0); + } + + markDragBatch():void { + if (this.activeDragBatch) { + this.markDraggingRows(this.activeDragBatch); + } + } + + // The destinations every member of the prospective batch accepts, null when + // the block reaches all of them. A batch may span lists, so a member that + // only accepts its own pins the block there, never to the dragged card's + // list. + dragPermittedDestinations(itemElement:HTMLElement):DestinationIdentity[]|null { + const scope = this.selection?.actionScopeFor(itemElement); + const members = scope?.kind === 'batch' ? scope.items : [itemElement]; + const ownerDestinationOf = (item:HTMLElement) => this.ownerDestinationOf(item); + + const lists = this.ownedListOutlets(); + const permitted = lists + .map((list) => this.destinationOf(list.listData)) + .filter((destination) => members.every((member) => itemAcceptsDestination(member, destination, ownerDestinationOf))); + + return permitted.length === lists.length ? null : permitted; + } + + // Asked in canDrag, the earliest point a drag can be stopped: an oversized + // batch is told so before any preview or drop feedback appears. + dragRefused(itemElement:HTMLElement):boolean { + if (this.maxBatchSizeValue <= 0) { + return false; + } + + const scope = this.selection?.actionScopeFor(itemElement); + const count = scope?.kind === 'batch' ? scope.items.length : 1; + if (count <= this.maxBatchSizeValue) { + return false; + } + + void announce( + I18n.t(`${this.moveAnnouncementScopeValue}.batch_too_large`, { count, max: this.maxBatchSizeValue }), + { politeness: 'assertive' }, + ); + return true; + } + + externalDragItems(itemElement:HTMLElement):HTMLElement[] { + const scope = this.selection?.actionScopeFor(itemElement); + return scope?.kind === 'batch' ? scope.items : [itemElement]; } - collapseSelectionForDrag(itemElement:HTMLElement):void { - this.selection?.collapseForDrag(itemElement); + private destinationOf(listData:SortableListData):DestinationIdentity { + return { type: listData.type, id: listData.listId == null ? null : String(listData.listId) }; + } + + // Outlets match document-wide; another root's lists are not ours. + private ownedListOutlets() { + return this.sortableListsListOutlets.filter((list) => this.element.contains(list.element)); + } + + // Marked on the item element itself, the same one the item controller's + // own onDragStart marks, so CSS keys off one convention regardless of + // which controller did the marking. + private markDraggingRows(items:SelectionItem[]):void { + const elements = this.itemElementsByKey(); + items.forEach((item) => { + elements.get(selectionKey(item))?.setAttribute('data-dragging', 'source'); + }); + } + + // Every mark under the root, not just the frozen batch's own rows: a + // cancelled drop, or the item controller's onDrop missing a row, would + // otherwise leave one behind. + private clearDraggingRows():void { + this.element.querySelectorAll('[data-dragging]').forEach((element) => element.removeAttribute('data-dragging')); + } + + // One document query per callback; never kept, so a morph cannot leave it + // stale. Keyed on type as well as id: ids collide across source tables. + private itemElementsByKey():Map { + const map = new Map(); + orderedItemElements(this.element).forEach((element) => { + const identity = itemIdentity(element); + if (identity) { + map.set(selectionKey(identity), element); + } + }); + return map; + } + + private takeActiveDragBatch():SelectionItem[]|null { + const batch = this.activeDragBatch; + this.clearDraggingRows(); + this.activeDragBatch = null; + this.dragOwnerDestinations = null; + return batch; } // A morph desyncs the children's drag-and-drop state in two ways. Stimulus @@ -264,6 +395,13 @@ export default class SortableListsController extends Controller imp // morph can strip or preserve the marker attribute independently of // the model. this.selection?.reconcile(); + + // A row a morph replaces mid-drag comes back as fresh server HTML that + // never went through markDragBatch, so it loses data-dragging with the + // element it replaced. + if (this.activeDragBatch) { + this.markDraggingRows(this.activeDragBatch); + } }); }; @@ -359,12 +497,11 @@ export default class SortableListsController extends Controller imp return; } - // Last, after every resolution above: several of them bail, and - // collapsing earlier would destroy the batch for a move that never runs. - this.selection?.collapseForMove(itemElement); + this.selection?.collapseForAction(itemElement); void this.performMove({ - sourceRow, + rows: [sourceRow], + items: null, rowsContainer: list.rowsContainer, listData: list.listData, previousItemId, @@ -372,12 +509,6 @@ export default class SortableListsController extends Controller imp }); } - // The list element an item currently belongs to, for the confinement field - // on the drag payload; null outside any registered list. - ownerListElementOf(itemElement:HTMLElement):HTMLElement|null { - return this.ownerListOf(itemElement)?.element ?? null; - } - // The owning list of an item is the innermost list outlet containing its // element: in nested topologies (a section item hosting a field list) the // item is contained by every ancestor list, and only the innermost one @@ -392,13 +523,29 @@ export default class SortableListsController extends Controller imp return this.ownerListOf(itemElement)?.rowsContainer ?? null; } + ownerDestinationOf(element:HTMLElement):DestinationIdentity|null { + const remembered = this.dragOwnerDestinations?.get(element); + if (remembered !== undefined) { + return remembered; + } + + const listData = this.ownerListOf(element)?.listData; + const destination = listData ? this.destinationOf(listData) : null; + this.dragOwnerDestinations?.set(element, destination); + return destination; + } + private async handleDrop({ location, source }:ElementDropPayload) { + // Before any bail-out below: a cancelled drop still consumes the frozen + // snapshot rather than leaking it into the next drag. + const frozenBatch = this.takeActiveDragBatch(); + if (this.busy) { debugLog('sortable-lists: ignoring drop, a move is already in progress'); return; } - if (!isSortableItemData(source.data) || !(source.element instanceof HTMLElement)) { + if (!isItemFromRoot(this.element, source.data) || !(source.element instanceof HTMLElement)) { debugLog('sortable-lists: ignoring drop, source is not a sortable item', source.data); return; } @@ -408,66 +555,132 @@ export default class SortableListsController extends Controller imp return; } - const moveUrl = this.resolveMoveUrl({ itemId: source.data.itemId, type: source.data.type }); - if (!moveUrl) { + const route = this.dropRouteFor(frozenBatch, source.data); + if (!route) { debugLog('sortable-lists: ignoring drop, no move URL for item', source.data.itemId); return; } + const batch = route.items; + // One item type per batch, so the exclusion set is that type plus ids. const intent = resolveDropIntent({ location, root: this.element, sourceData: source.data, + excludedItems: { + type: source.data.type, + ids: new Set((batch ?? singleItemBatch(source.data)).map((item) => item.id)), + }, }); if (!intent) { debugLog('sortable-lists: ignoring drop, it did not resolve to a move'); return; } - const sourceList = this.ownerListOf(source.element); - const sourceRow = sourceList ? rowOf(sourceList.rowsContainer, source.element) : null; - if (!sourceRow) { - debugLog('sortable-lists: ignoring drop, could not resolve the source row element'); + const rows = batch + ? this.rowsForItems(batch) + : this.singleSourceRow(source.element); + if (!rows) { + debugLog('sortable-lists: ignoring drop, could not resolve every batch row'); + this.announceMoveFailure({ label: null, listName: null, crossList: false }, false, batch?.length ?? 1); return; } await this.performMove({ - sourceRow, + rows, + items: batch, rowsContainer: intent.rowsContainer, listData: intent.listData, previousItemId: intent.previousItemId, - moveUrl, + moveUrl: route.url, }); } - // Optimistically reorder a single row, persist the move, and roll the row - // back (with a FLIP animation and an error toast) if the server rejects it. - // Shared by drag drops and programmatic menu moves. + // The collection URL is the capability signal: a root that renders one moves + // every drag through the collection contract, one card or many. Without it + // the dragged item's own move template applies. + private dropRouteFor(frozenBatch:SelectionItem[]|null, sourceData:{ type:string; itemId:string }):DropRoute|null { + const collectionUrl = this.resolveCollectionMoveUrl(); + if (collectionUrl) { + const items = frozenBatch && frozenBatch.length > 0 ? frozenBatch : singleItemBatch(sourceData); + return { url: collectionUrl, items }; + } + + const url = this.resolveMoveUrl(sourceData); + return url ? { url, items: null } : null; + } + + private get collectionMoveUrl():string|null { + return this.hasCollectionMoveUrlValue && this.collectionMoveUrlValue !== '' ? this.collectionMoveUrlValue : null; + } + + private resolveCollectionMoveUrl():string|null { + const collectionMoveUrl = this.collectionMoveUrl; + if (!collectionMoveUrl) { + return null; + } + + const url = new URL(collectionMoveUrl, window.location.href); + if (this.optimisticValue) { + url.searchParams.set('optimistic', 'true'); + } + + return relativeUrl(url); + } + + // Refused whole when a row is missing: a member that vanished mid-drag + // means a partial block would diverge from the ids the request claims. + private rowsForItems(items:SelectionItem[]):HTMLElement[]|null { + const elements = this.itemElementsByKey(); + const rows:HTMLElement[] = []; + + for (const item of items) { + const itemElement = elements.get(selectionKey(item)) ?? null; + const container = itemElement ? this.ownerRowsContainer(itemElement) : null; + const row = container && itemElement ? rowOf(container, itemElement) : null; + if (!row) { + return null; + } + rows.push(row); + } + + return rows; + } + + private singleSourceRow(sourceElement:HTMLElement):HTMLElement[]|null { + const sourceList = this.ownerListOf(sourceElement); + const sourceRow = sourceList ? rowOf(sourceList.rowsContainer, sourceElement) : null; + return sourceRow ? [sourceRow] : null; + } + + // Shared by drag drops, single or batch, and by the menu moves that pass + // no items. private async performMove({ - sourceRow, + rows, + items, rowsContainer, listData, previousItemId, moveUrl, }:{ - sourceRow:HTMLElement; + rows:HTMLElement[]; + items:SelectionItem[]|null; rowsContainer:HTMLElement; listData:SortableListData; previousItemId:string|null; moveUrl:string; }):Promise { - const rows = [sourceRow]; // Captured before the reorder: afterwards the row already belongs to the // target list, so source-relative facts would be lost. const announcementContext:MoveAnnouncementContext = { - label: resolveItemLabel(sourceRow), + label: resolveItemLabel(rows[0]), listName: listData.name, - crossList: sourceRow.parentElement !== rowsContainer, + crossList: rows.some((row) => row.parentElement !== rowsContainer), }; const rollback = captureRowPositions(rows); reorderRows({ rows, rowsContainer, previousItemId }); - // The reorder resolving back to the source's current DOM position means + // The reorder resolving back to the block's current DOM position means // the move is a no-op — nothing to persist, so no request. Comparing DOM // placement (not predecessor ids) keeps non-item rows such as truncation // markers out of the equation. @@ -476,39 +689,45 @@ export default class SortableListsController extends Controller imp return; } - this.announceMove(announcementContext, sourceRow, rowsContainer); + this.announceMove(announcementContext, rows, rowsContainer); const optimisticPlacement = captureRowPositions(rows); - const result = await this.moveItem({ listData, previousItemId, moveUrl }); - - if (!result.ok) { - let rolledBack = false; - try { - // A concurrent morph that removed or repositioned the rows carries - // fresher server state than the pre-move snapshot; roll back only - // while the rows still sit where the optimistic move put them. - if (rowsRemainAt(optimisticPlacement)) { - flipMove(rows, () => restoreRowPositions(rollback)); - // restoreRowPositions silently skips rows whose captured parent - // disconnected, so verify the postcondition instead of trusting - // the absence of an exception. - rolledBack = rowsRemainAt(rollback); - } - } catch (error) { - debugLog('Failed to roll back sortable list item move', error); - } + const result = await this.moveItem({ listData, previousItemId, moveUrl, items }); + + if (result.ok) { + // Movement clears selection and anchor; failure preserves both for a + // retry. performMove is the shared boundary for the menu path too. + this.selection?.clearSilently(); + return; + } - if (result.showToast) { - this.dispatchErrorToast(); - this.announceMoveFailure(announcementContext, rolledBack); + let rolledBack = false; + try { + // A concurrent morph carries fresher server state than the pre-move + // snapshot, so roll back only while the rows still sit where the + // optimistic move put them. + if (rowsRemainAt(optimisticPlacement)) { + flipMove(rows, () => restoreRowPositions(rollback)); + // restoreRowPositions silently skips rows whose captured parent + // disconnected, so the postcondition is verified rather than assumed. + rolledBack = rowsRemainAt(rollback); } + } catch (error) { + debugLog('Failed to roll back sortable list item move', error); + } + + if (result.showToast) { + this.dispatchErrorToast(); + } + // A 422 streams its own flash, which knows nothing about the client's + // rollback: without this, a rejection plus a concurrent morph would leave + // an unverified rollback unannounced. + if (result.showToast || !rolledBack) { + this.announceMoveFailure(announcementContext, rolledBack, rows.length); } } - // The template must expand to a same-origin relative URL: the expansion is - // reduced to path + search + hash, so an absolute template's origin would - // be dropped silently. private resolveMoveUrl({ itemId, type }:{ itemId:string; type:string|null }):string|null { const template = this.moveUrlTemplateFor(type); if (!template) { @@ -523,7 +742,7 @@ export default class SortableListsController extends Controller imp url.searchParams.set('optimistic', 'true'); } - return `${url.pathname}${url.search}${url.hash}`; + return relativeUrl(url); } // The dragged item's type keys the template: the move endpoint belongs to @@ -540,19 +759,24 @@ export default class SortableListsController extends Controller imp listData, previousItemId, moveUrl, + items, }:{ listData:SortableListData; previousItemId:string|null; moveUrl:string; + items:SelectionItem[]|null; }):Promise { const request = new FetchRequest( 'put', moveUrl, { + // The one boundary where the batch's (type, id) pairs are projected + // down to the bare ids the PUT takes. body: buildMoveFormData({ listId: listData.listId, previousItemId, type: listData.type, + itemIds: items?.map((item) => item.id) ?? null, }), responseKind: 'turbo-stream', }, @@ -598,39 +822,50 @@ export default class SortableListsController extends Controller imp // global live region, in sync with what sighted users see. Failure paths // append their own message below. A 422 stays silent here: its error flash // is streamed by the server and self-announces (matching the toast rule). - private announceMove(context:MoveAnnouncementContext, sourceRow:HTMLElement, rowsContainer:HTMLElement):void { - const placement = resolveItemPosition({ row: sourceRow, rowsContainer }); + // The consumer's vocabulary: Backlogs says "work package", not "item". + private announceMove(context:MoveAnnouncementContext, rows:HTMLElement[], rowsContainer:HTMLElement):void { + const placement = resolveItemPosition({ row: rows[0], rowsContainer }); if (!placement) { return; } + const scope = this.moveAnnouncementScopeValue; // Resolved outside the options object literal below: nested inside it, // the call's generic return type would be inferred from the object's // contextual `TranslateOptions` index signature (`any`) instead of its // own `string` default. - const label = context.label ?? I18n.t('js.sortable_lists.announcements.fallback_item_label'); - const listName = context.listName ?? I18n.t('js.sortable_lists.announcements.fallback_list_name'); - const message = context.crossList - ? I18n.t('js.sortable_lists.announcements.moved_to_list', { - label, - list: listName, - position: placement.position, - total: placement.total, - }) - : I18n.t('js.sortable_lists.announcements.moved', { - label, - position: placement.position, - total: placement.total, - }); + const label = context.label ?? I18n.t(`${scope}.fallback_item_label`); + const listName = context.listName ?? I18n.t(`${scope}.fallback_list_name`); + + let message:string; + if (rows.length > 1) { + const first = placement.position; + const last = placement.position + rows.length - 1; + message = context.crossList + ? I18n.t(`${scope}.moved_batch_to_list`, { count: rows.length, list: listName, first, last, total: placement.total }) + : I18n.t(`${scope}.moved_batch`, { count: rows.length, first, last, total: placement.total }); + } else { + message = context.crossList + ? I18n.t(`${scope}.moved_to_list`, { label, list: listName, position: placement.position, total: placement.total }) + : I18n.t(`${scope}.moved`, { label, position: placement.position, total: placement.total }); + } void announce(message, { politeness: 'polite' }); } - private announceMoveFailure(context:MoveAnnouncementContext, rolledBack:boolean):void { - const label = context.label ?? I18n.t('js.sortable_lists.announcements.fallback_item_label'); - const message = rolledBack - ? I18n.t('js.sortable_lists.announcements.move_failed_rolled_back', { label }) - : I18n.t('js.sortable_lists.announcements.move_failed_check_position'); + private announceMoveFailure(context:MoveAnnouncementContext, rolledBack:boolean, count:number):void { + const scope = this.moveAnnouncementScopeValue; + const label = context.label ?? I18n.t(`${scope}.fallback_item_label`); + let message:string; + if (rolledBack) { + message = count > 1 + ? I18n.t(`${scope}.move_failed_rolled_back_batch`, { count }) + : I18n.t(`${scope}.move_failed_rolled_back`, { label }); + } else { + message = count > 1 + ? I18n.t(`${scope}.move_failed_check_positions_batch`, { count }) + : I18n.t(`${scope}.move_failed_check_position`); + } void announce(message, { politeness: 'assertive' }); } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts index c5b0cc1e2669..9839b59dd9b4 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts @@ -116,6 +116,10 @@ describe('sortable lists drag and drop helpers', () => { it('rejects data with a blank item id', () => { expect(isSortableItemData(sortableItemData({ type: 'work_package', itemId: '' }))).toBe(false); }); + + it('rejects data with a blank type', () => { + expect(isSortableItemData(sortableItemData({ type: '', itemId: '1' }))).toBe(false); + }); }); describe('isSortableListData', () => { @@ -212,6 +216,23 @@ describe('sortable lists drag and drop helpers', () => { expect(data.get('list_id')).toEqual(''); expect(data.get('prev_id')).toEqual(''); }); + + it('appends ordered ids for a batch payload', () => { + const data = buildMoveFormData({ + listId: '7', previousItemId: '3', type: 'sprint', itemIds: ['12', '9', '15'], + }); + + expect(data.getAll('ids[]')).toEqual(['12', '9', '15']); + expect(data.get('list_type')).toBe('sprint'); + expect(data.get('list_id')).toBe('7'); + expect(data.get('prev_id')).toBe('3'); + }); + + it('omits ids for a singular payload', () => { + const data = buildMoveFormData({ listId: '7', previousItemId: null, type: 'sprint' }); + + expect(data.getAll('ids[]')).toEqual([]); + }); }); describe('resolvePreviousSortableItemId', () => { @@ -222,7 +243,7 @@ describe('sortable lists drag and drop helpers', () => { rowsContainer.append(targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '1', targetItem: target, closestEdge: 'bottom', rowsContainer })).toEqual('3'); + expect(resolvePreviousSortableItemId({ excludedItems: { type: 'work_package', ids: new Set(['1']) }, targetItem: target, closestEdge: 'bottom', rowsContainer })).toEqual('3'); }); it('uses the row item as previous item when the drop target is the row', () => { @@ -231,7 +252,7 @@ describe('sortable lists drag and drop helpers', () => { rowsContainer.append(targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '1', targetItem: targetRow, closestEdge: 'bottom', rowsContainer })).toEqual('3'); + expect(resolvePreviousSortableItemId({ excludedItems: { type: 'work_package', ids: new Set(['1']) }, targetItem: targetRow, closestEdge: 'bottom', rowsContainer })).toEqual('3'); }); it('uses the previous row item when dropping on the top edge', () => { @@ -242,7 +263,7 @@ describe('sortable lists drag and drop helpers', () => { rowsContainer.append(first, targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top', rowsContainer })).toEqual('1'); + expect(resolvePreviousSortableItemId({ excludedItems: { type: 'work_package', ids: new Set(['2']) }, targetItem: target, closestEdge: 'top', rowsContainer })).toEqual('1'); }); it('uses the previous row item when dropping on the top edge of a row target', () => { @@ -252,7 +273,7 @@ describe('sortable lists drag and drop helpers', () => { rowsContainer.append(first, targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: targetRow, closestEdge: 'top', rowsContainer })).toEqual('1'); + expect(resolvePreviousSortableItemId({ excludedItems: { type: 'work_package', ids: new Set(['2']) }, targetItem: targetRow, closestEdge: 'top', rowsContainer })).toEqual('1'); }); it('treats a missing closest edge as dropping before the target item', () => { @@ -263,7 +284,7 @@ describe('sortable lists drag and drop helpers', () => { rowsContainer.append(first, targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: null, rowsContainer })).toEqual('1'); + expect(resolvePreviousSortableItemId({ excludedItems: { type: 'work_package', ids: new Set(['2']) }, targetItem: target, closestEdge: null, rowsContainer })).toEqual('1'); }); it('uses a truncation marker when dropping before a tail item', () => { @@ -274,7 +295,7 @@ describe('sortable lists drag and drop helpers', () => { rowsContainer.append(first, showMoreRow('5'), targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top', rowsContainer })).toEqual('5'); + expect(resolvePreviousSortableItemId({ excludedItems: { type: 'work_package', ids: new Set(['2']) }, targetItem: target, closestEdge: 'top', rowsContainer })).toEqual('5'); }); it('skips the source item and uses a preceding truncation marker when resolving the previous item', () => { @@ -286,7 +307,7 @@ describe('sortable lists drag and drop helpers', () => { rowsContainer.append(first, showMoreRow(), sourceRow, targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top', rowsContainer })).toEqual('hidden-item'); + expect(resolvePreviousSortableItemId({ excludedItems: { type: 'work_package', ids: new Set(['2']) }, targetItem: target, closestEdge: 'top', rowsContainer })).toEqual('hidden-item'); }); it('returns null when dropping before the first item', () => { @@ -296,7 +317,89 @@ describe('sortable lists drag and drop helpers', () => { rowsContainer.append(targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top', rowsContainer })).toBeNull(); + expect(resolvePreviousSortableItemId({ excludedItems: { type: 'work_package', ids: new Set(['2']) }, targetItem: target, closestEdge: 'top', rowsContainer })).toBeNull(); + }); + + it('skips every excluded id when resolving the previous item', () => { + // rows: A, B, C, D — drop with top edge on D while A and C are excluded + // (selected): the closest preceding unexcluded item is B. + const rowsContainer = document.createElement('ul'); + const rowA = itemRow('A'); + const rowB = itemRow('B'); + const rowC = itemRow('C'); + const rowD = itemRow('D'); + + rowsContainer.append(rowA, rowB, rowC, rowD); + + const result = resolvePreviousSortableItemId({ + excludedItems: { type: 'work_package', ids: new Set(['A', 'C']) }, + targetItem: rowD, + closestEdge: 'top', + rowsContainer, + }); + + expect(result).toBe('B'); + }); + + it('refuses an excluded item as bottom-edge anchor', () => { + // bottom edge on C, but C is excluded: fall through to the sibling walk. + const rowsContainer = document.createElement('ul'); + const rowA = itemRow('A'); + const rowB = itemRow('B'); + const rowC = itemRow('C'); + const rowD = itemRow('D'); + + rowsContainer.append(rowA, rowB, rowC, rowD); + + const result = resolvePreviousSortableItemId({ + excludedItems: { type: 'work_package', ids: new Set(['A', 'C']) }, + targetItem: rowC, + closestEdge: 'bottom', + rowsContainer, + }); + + expect(result).toBe('B'); + }); + + // Ids are unique per source table, so a same-id row of another type is a + // legitimate anchor, not a batch member to skip. + it('does not exclude a same-id row of another type', () => { + const rowsContainer = document.createElement('ul'); + const collidingRow = itemRow('A'); + collidingRow.setAttribute('data-sortable-lists--item-type-value', 'section'); + const targetRow = itemRow('B'); + targetRow.setAttribute('data-sortable-lists--item-type-value', 'work_package'); + + rowsContainer.append(collidingRow, targetRow); + + const result = resolvePreviousSortableItemId({ + excludedItems: { type: 'work_package', ids: new Set(['A']) }, + targetItem: targetRow, + closestEdge: 'top', + rowsContainer, + }); + + expect(result).toBe('A'); + }); + + // A truncation marker resolves no type, so a bare-id collision there + // stays excluded. + it('keeps excluding a truncation marker whose previous item id collides', () => { + const rowsContainer = document.createElement('ul'); + const first = itemRow('1'); + const marker = showMoreRow('A'); + const targetRow = itemRow('B'); + + rowsContainer.append(first, marker, targetRow); + + const result = resolvePreviousSortableItemId({ + excludedItems: { type: 'work_package', ids: new Set(['A']) }, + targetItem: targetRow, + closestEdge: 'top', + rowsContainer, + }); + + expect(result).toBe('1'); }); }); @@ -425,8 +528,7 @@ describe('sortable lists drag and drop helpers', () => { sourceData: sortableItemData({ type: 'work_package', itemId: '1', - sourceListElement: sourceList, - confined: true, + permittedDestinations: [{ type: 'sprint', id: '9' }], }), }); @@ -447,8 +549,7 @@ describe('sortable lists drag and drop helpers', () => { sourceData: sortableItemData({ type: 'work_package', itemId: '1', - sourceListElement: list, - confined: true, + permittedDestinations: [{ type: 'sprint', id: '7' }], }), }); 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 812fdb8972e1..cee21892fb92 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 @@ -39,13 +39,19 @@ import { // resolution (lifecycle-manager) does. import { getElementFromPointWithoutHoneypot } from '@atlaskit/pragmatic-drag-and-drop/private/get-element-from-point-without-honey-pot'; import { type DragLocationHistory } from '@atlaskit/pragmatic-drag-and-drop/types'; +import { type SelectionItem } from 'core-common/batch-selection'; import { + isExcludedItem, resolveClosestItemElement, resolveItemElement, resolveItemId, + resolveItemType, resolveListAppendPreviousItemId, - resolvePreviousItemId, + resolvePreviousItem, rowOf, + sameDestination, + type DestinationIdentity, + type ExcludedItems, type MoveAvailability, type MoveDirection, } from './list-dom'; @@ -55,19 +61,23 @@ import { const sortableItemDataKey = Symbol('sortable-list-item'); const sortableListDataKey = Symbol('sortable-list'); -export interface SortableItemData extends Record { +// What a drop target exposes: the identity a drop resolves against, and +// nothing that would have to be recomputed on every dragover. +export interface SortableItemIdentity extends Record { [sortableItemDataKey]:true; type:string; itemId:string; +} + +// What the dragged source carries, resolved once at drag start. +export interface SortableItemData extends SortableItemIdentity { rootElement:HTMLElement|null; - // The list element the drag started in, resolved by the root at drag start - // (items hold no list reference themselves). Null when the item is not in a - // registered list. Mirrors the rootElement pattern: identity is carried on - // the payload so drop targets can decide without walking the DOM. - sourceListElement:HTMLElement|null; - // A confined item may only land in sourceListElement or one of its rows; - // every other container refuses it. See confinementAllowsDrop. - confined:boolean; + // The destinations this drag may land in, resolved across the whole batch + // at drag start; null when nothing restricts it, empty when nothing + // accepts it. Identities rather than list elements: a morph can replace a + // permitted list mid-drag, and elements frozen here would then name nodes + // that have left the document. + permittedDestinations:DestinationIdentity[]|null; } export type SortableListDropPosition = 'start'|'end'; @@ -95,15 +105,26 @@ export interface SortableListsRoot { moveInDirection(itemElement:HTMLElement, direction:MoveDirection):void; // A snapshot for menu gating; 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. - ownerListElementOf(itemElement:HTMLElement):HTMLElement|null; // The rows container of the item's innermost owning list, or null when the // item is not (yet) inside a list the root knows about. ownerRowsContainer(itemElement:HTMLElement):HTMLElement|null; - // A drag moves exactly one item until AGILE-278 lands, so it collapses any - // wider batch onto the dragged card. - collapseSelectionForDrag(itemElement:HTMLElement):void; + // Freezes the drag's batch and returns its size; the preview renders it. + freezeDragBatch(itemElement:HTMLElement):number; + // Marks the frozen batch's rows; a no-op before freezeDragBatch. + markDragBatch():void; + // Asked while the drag payload is built, which Pragmatic dispatches before + // freezeDragBatch freezes the batch, so the answer comes from the live + // selection in the same synchronous dragstart turn. + dragPermittedDestinations(itemElement:HTMLElement):DestinationIdentity[]|null; + // The destination of the element's innermost owning list, or null when no + // list the root knows about claims it. + ownerDestinationOf(element:HTMLElement):DestinationIdentity|null; + // Asked in canDrag: true when the item's prospective batch exceeds the + // server's cap, so the drag never starts. + dragRefused(itemElement:HTMLElement):boolean; + // The cards an external drop should receive: the prospective batch, read + // before the batch is frozen, without touching the selection. + externalDragItems(itemElement:HTMLElement):HTMLElement[]; } // Implemented by the list, item and scrollable controllers so the root can @@ -116,7 +137,16 @@ export interface RootAwareChild { reregister():void; } -export function isSortableItemData(data:Record):data is SortableItemData { +export function sortableItemIdentity({ type, itemId }:{ type:string; itemId:string }):SortableItemIdentity { + return { [sortableItemDataKey]: true, type, itemId }; +} + +export function singleItemBatch({ type, itemId }:{ type:string; itemId:string }):SelectionItem[] { + return [{ type, id: itemId }]; +} + +// The source-only fields are what isItemFromRoot narrows on beyond this. +export function isSortableItemData(data:Record):data is SortableItemIdentity { return data[sortableItemDataKey] === true && typeof data.type === 'string' && data.type.length > 0 @@ -135,22 +165,17 @@ export function sortableItemData({ type, itemId, rootElement = null, - sourceListElement = null, - confined = false, + permittedDestinations = null, }:{ type:string; itemId:string; rootElement?:HTMLElement|null; - sourceListElement?:HTMLElement|null; - confined?:boolean; + permittedDestinations?:DestinationIdentity[]|null; }):SortableItemData { return { - [sortableItemDataKey]: true, - type, - itemId, + ...sortableItemIdentity({ type, itemId }), rootElement, - sourceListElement, - confined, + permittedDestinations, }; } @@ -181,13 +206,16 @@ export function buildMoveFormData({ listId, previousItemId, type, + itemIds = null, }:{ listId:string|null; previousItemId:string|null; type:string; + itemIds?:string[]|null; }):FormData { const data = new FormData(); + itemIds?.forEach((id) => data.append('ids[]', id)); data.append('list_type', type); data.append('list_id', listId ?? ''); data.append('prev_id', previousItemId ?? ''); @@ -204,15 +232,15 @@ export function isItemFromRoot( ):data is SortableItemData { return rootElement != null && isSortableItemData(data) - && data.rootElement === rootElement; + && (data as SortableItemData).rootElement === rootElement; } -// Whether a drop on the given target may amount to a move under the source's -// confinement. contains() includes the element itself, so one predicate passes -// both the source list element and every row inside it while failing every -// foreign container. The source list passing is load-bearing: a drop resolves -// through the list target (resolveDropIntent returns null without one), so -// failing it would kill within-list reorder, not just cross-list moves. +// Whether a drop into the given destination may amount to a move for this +// batch. A destination the batch owns passing is load-bearing: a drop +// resolves through the list target (resolveDropIntent returns null without +// one), so failing it would kill within-list reorder, not just cross-list +// moves. A null destination is one no list claims, which nothing restricted +// accepts. // // Item drop targets consult this in canDrop and refuse outright; list drop // targets stay accepted regardless (an accepted target is what keeps the @@ -221,20 +249,25 @@ export function isItemFromRoot( // release over a container this fails for resolves to no move at all, and // the drop-indicator layers consult it too — rows never show a drop position // for it, and the list marks its container refused instead of active. -export function confinementAllowsDrop( +export function permittedDestinationsAllowDrop( data:SortableItemData, - targetElement:Element, + destination:DestinationIdentity|null, ):boolean { - return !data.confined || (data.sourceListElement?.contains(targetElement) ?? false); + return data.permittedDestinations === null + || data.permittedDestinations.some((permitted) => sameDestination(destination, permitted)); +} + +export function destinationOfList(listData:SortableListData):DestinationIdentity { + return { type: listData.type, id: listData.listId }; } export function resolvePreviousSortableItemId({ - sourceItemId, + excludedItems, targetItem, closestEdge, rowsContainer, }:{ - sourceItemId:string; + excludedItems:ExcludedItems; targetItem:HTMLElement; closestEdge:Edge|null; rowsContainer:Element; @@ -242,7 +275,8 @@ export function resolvePreviousSortableItemId({ const targetItemElement = resolveItemElement(targetItem, rowsContainer); const targetItemId = targetItemElement ? resolveItemId(targetItemElement) : null; - if (closestEdge === 'bottom' && targetItemId !== sourceItemId) { + if (closestEdge === 'bottom' && targetItemElement && targetItemId !== null + && !isExcludedItem(excludedItems, { id: targetItemId, type: resolveItemType(targetItemElement) })) { return targetItemId; } @@ -250,9 +284,9 @@ export function resolvePreviousSortableItemId({ let row = targetRow?.previousElementSibling ?? null; while (row) { - const itemId = resolvePreviousItemId(row, rowsContainer); - if (itemId && itemId !== sourceItemId) { - return itemId; + const item = resolvePreviousItem(row, rowsContainer); + if (item && !isExcludedItem(excludedItems, item)) { + return item.id; } row = row.previousElementSibling; @@ -265,11 +299,11 @@ export function resolvePreviousSortableItemId({ // the position the target list declares: 'start' inserts before the first row // (null previous item), 'end' appends after the last. function resolveListOnlyPreviousItemId({ - sourceItemId, + excludedItems, rowsContainer, dropPosition, }:{ - sourceItemId:string; + excludedItems:ExcludedItems; rowsContainer:HTMLElement; dropPosition:SortableListDropPosition; }):string|null { @@ -277,7 +311,7 @@ function resolveListOnlyPreviousItemId({ return null; } - return resolveListAppendPreviousItemId({ sourceItemId, rowsContainer }); + return resolveListAppendPreviousItemId({ excludedItems, rowsContainer }); } export interface DropIntent { @@ -297,27 +331,33 @@ export function resolveDropIntent({ location, root, sourceData, + excludedItems = { type: sourceData.type, ids: new Set(singleItemBatch(sourceData).map((item) => item.id)) }, }:{ location:DragLocationHistory; root:HTMLElement; sourceData:SortableItemData; + excludedItems?:ExcludedItems; }):DropIntent|null { - const targetItem = location.current.dropTargets.find( - (target):target is typeof target & { data:SortableItemData; element:HTMLElement } => ( - isSortableItemData(target.data) && target.element instanceof HTMLElement && root.contains(target.element) - && confinementAllowsDrop(sourceData, target.element) - ), - ); const targetList = location.current.dropTargets.find( (target):target is typeof target & { data:SortableListData; element:HTMLElement } => ( isSortableListData(target.data) && target.element instanceof HTMLElement && root.contains(target.element) - && confinementAllowsDrop(sourceData, target.element) + && permittedDestinationsAllowDrop(sourceData, destinationOfList(target.data)) ), ); if (!targetList) { return null; } + // Scoped to the accepted list rather than gated on its own account: an + // item drop target carries only its identity, and the list it sits in is + // the destination a drop into it would reach. + const targetItem = location.current.dropTargets.find( + (target):target is typeof target & { data:SortableItemIdentity; element:HTMLElement } => ( + isSortableItemData(target.data) && target.element instanceof HTMLElement + && targetList.element.contains(target.element) + ), + ); + const listElement = targetList.element; const listData = targetList.data; const rowsContainer = listData.rowsContainer ?? listElement; @@ -346,13 +386,13 @@ export function resolveDropIntent({ const previousItemId = targetItem ? resolvePreviousSortableItemId({ - sourceItemId: sourceData.itemId, + excludedItems, targetItem: targetItem.element, closestEdge: extractClosestEdge(targetItem.data), rowsContainer, }) : resolveListOnlyPreviousItemId({ - sourceItemId: sourceData.itemId, + excludedItems, rowsContainer, dropPosition: listData.dropPosition, }); 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 e75a2bf7f9a4..1fa208a97d2d 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 @@ -49,6 +49,7 @@ vi.mock('@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-previe setCustomNativeDragPreview: vi.fn(), })); +import { attachClosestEdge } from '@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge'; import type { draggable as draggableFn, dropTargetForElements as dropTargetForElementsFn } from '@atlaskit/pragmatic-drag-and-drop/element/adapter'; import type { setCustomNativeDragPreview as setCustomNativeDragPreviewFn } from '@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview'; import type { preventUnhandled as preventUnhandledType } from '@atlaskit/pragmatic-drag-and-drop/prevent-unhandled'; @@ -56,6 +57,7 @@ import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test- import type { ActionEvent } from '@hotwired/stimulus'; import type ItemControllerType from './item.controller'; import type { SortableListsRoot } from './drag-and-drop'; +import type { DestinationIdentity } from './list-dom'; describe('Sortable lists item controller', () => { let draggable:typeof draggableFn; @@ -64,6 +66,7 @@ describe('Sortable lists item controller', () => { let setCustomNativeDragPreview:typeof setCustomNativeDragPreviewFn; let ItemController:typeof ItemControllerType; let sortableItemData:typeof import('./drag-and-drop').sortableItemData; + let sortableItemIdentity:typeof import('./drag-and-drop').sortableItemIdentity; interface TestItemController { renderDropIndicator(edge:'top'|'bottom'|null):void; @@ -76,7 +79,7 @@ describe('Sortable lists item controller', () => { ({ preventUnhandled } = await import('@atlaskit/pragmatic-drag-and-drop/prevent-unhandled')); ({ setCustomNativeDragPreview } = await import('@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview')); ({ default: ItemController } = await import('./item.controller')); - ({ sortableItemData } = await import('./drag-and-drop')); + ({ sortableItemData, sortableItemIdentity } = await import('./drag-and-drop')); }); function controllerFor(element:HTMLElement) { @@ -91,9 +94,9 @@ describe('Sortable lists item controller', () => { function fakeRoot( element = document.createElement('div'), - { busy = false, ownerListElement = null, ownerRowsContainer = () => null }:{ + { busy = false, ownerDestination = null, ownerRowsContainer = () => null }:{ busy?:boolean; - ownerListElement?:HTMLElement|null; + ownerDestination?:DestinationIdentity|null; ownerRowsContainer?:(itemElement:HTMLElement) => HTMLElement|null; } = {}, ):SortableListsRoot { @@ -103,9 +106,19 @@ describe('Sortable lists item controller', () => { busy, moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), - ownerListElementOf: vi.fn(() => ownerListElement), ownerRowsContainer: vi.fn(ownerRowsContainer), - collapseSelectionForDrag: vi.fn(), + freezeDragBatch: vi.fn(() => 1), + markDragBatch: vi.fn(), + ownerDestinationOf: vi.fn(() => ownerDestination), + // Mirrors the real root's fallback for a batchless drag: the item's own + // mobility attribute is the whole answer. + dragPermittedDestinations: vi.fn((itemElement:HTMLElement) => ( + itemElement.getAttribute('data-sortable-lists--item-mobility-value') === 'confined' + ? [ownerDestination].filter((destination):destination is DestinationIdentity => destination !== null) + : null + )), + dragRefused: vi.fn(() => false), + externalDragItems: vi.fn((item:HTMLElement) => [item]), }; } @@ -134,12 +147,21 @@ describe('Sortable lists item controller', () => { Object.defineProperty(controller, 'hasTypeValue', { value: true }); Object.defineProperty(controller, 'externalUrlValue', { value: externalUrl ?? '' }); Object.defineProperty(controller, 'hasExternalUrlValue', { value: externalUrl !== null }); - Object.defineProperty(controller, 'labelValue', { value: label ?? '' }); - Object.defineProperty(controller, 'hasLabelValue', { value: label !== null }); // Written to the element, not stubbed as a controller property: the // controller reads mobility through list-dom's parser, which stubbing - // would bypass. + // would bypass. The same goes for id, type, external URL and label: the + // batch-aware external payload reads every member off the DOM, not off + // this card's own controller instance. + element.setAttribute('data-controller', 'sortable-lists--item'); + element.setAttribute('data-sortable-lists--item-id-value', '123'); + element.setAttribute('data-sortable-lists--item-type-value', 'item'); element.setAttribute('data-sortable-lists--item-mobility-value', mobility); + if (externalUrl !== null) { + element.setAttribute('data-sortable-lists--item-external-url-value', externalUrl); + } + if (label !== null) { + element.setAttribute('data-sortable-lists--item-label-value', label); + } Object.defineProperty(controller, 'hasHandleTarget', { value: handle !== null }); if (handle) { Object.defineProperty(controller, 'handleTarget', { value: handle }); @@ -283,6 +305,40 @@ describe('Sortable lists item controller', () => { expect(nextElement.dataset.dropPosition).toEqual('top'); }); + it('skips every consecutive dragged sibling when placing the indicator below a row', () => { + const list = document.createElement('ul'); + const [row, mateOne, mateTwo, after] = ['1', '2', '3', '4'].map((id) => { + const li = document.createElement('li'); + li.setAttribute('data-controller', 'sortable-lists--item'); + li.setAttribute('data-sortable-lists--item-id-value', id); + li.setAttribute('data-sortable-lists--item-type-value', 'item'); + return li; + }); + mateOne.setAttribute('data-dragging', 'source'); + mateTwo.setAttribute('data-dragging', 'source'); + list.append(row, mateOne, mateTwo, after); + connectedControllerFor(row, { root: fakeRoot() }); + + vi.spyOn(row, 'getBoundingClientRect').mockReturnValue({ + top: 0, bottom: 100, left: 0, right: 100, width: 100, height: 100, x: 0, y: 0, toJSON: () => ({}), + }); + + // Built the same way Pragmatic's own attachClosestEdge does, so the edge + // lives under its private symbol key rather than a plain property. + const data = attachClosestEdge(sortableItemIdentity({ itemId: '1', type: 'item' }), { + element: row, + input: { clientX: 10, clientY: 90 } as never, + allowedEdges: ['top', 'bottom'], + }); + + vi.mocked(dropTargetForElements).mock.lastCall?.[0].onDragEnter?.({ + self: { data }, + } as never); + + expect(after.dataset.dropPosition).toBe('top'); + expect(mateOne.dataset.dropPosition).toBeUndefined(); + }); + it('removes the drop position when leaving an item', () => { const element = document.createElement('li'); const nextElement = document.createElement('li'); @@ -422,6 +478,36 @@ describe('Sortable lists item controller', () => { }); }); + it('exposes only identity and edge as drop-target data', () => { + const element = document.createElement('article'); + connectedControllerFor(element, { root: fakeRoot() }); + + const data = vi.mocked(dropTargetForElements).mock.lastCall?.[0].getData?.({ + element, input: { clientX: 0, clientY: 0 } as never, source: {} as never, + }); + + expect(data).toEqual(expect.objectContaining({ itemId: '123', type: 'item' })); + expect(Object.keys(data ?? {})).toEqual(['type', 'itemId']); + expect(data).not.toHaveProperty('rootElement'); + expect(data).not.toHaveProperty('permittedDestinations'); + }); + + it('refuses a drop onto a row marked as part of the dragged batch', () => { + const root = document.createElement('div'); + const targetElement = document.createElement('article'); + targetElement.setAttribute('data-dragging', 'source'); + connectedControllerFor(targetElement, { root: fakeRoot(root) }); + + expect(vi.mocked(dropTargetForElements).mock.lastCall?.[0].canDrop?.({ + element: targetElement, + input: {} as never, + source: { + data: sortableItemData({ type: 'item', itemId: '456', rootElement: root }), + element: document.createElement('article'), + } as never, + })).toBe(false); + }); + it('does not accept itself as an item drop target', () => { const root = document.createElement('div'); const element = document.createElement('article'); @@ -487,11 +573,14 @@ describe('Sortable lists item controller', () => { })).toBe(true); }); - // A confined item may only land in its source list. Rows of that list keep - // accepting it (within-list reorder), rows of any other list refuse it, and - // with no payload list there is nothing it may land on. - describe('a confined drag source', () => { - function canDropOnto(targetElement:HTMLElement, root:HTMLElement, sourceListElement:HTMLElement|null) { + // A pinned drag may only land in the lists its payload permits. Rows of one + // keep accepting it (within-list reorder), rows of any other refuse it, and + // an empty set leaves nothing it may land on. + describe('a pinned drag source', () => { + const sprint7:DestinationIdentity = { type: 'sprint', id: '7' }; + const sprint9:DestinationIdentity = { type: 'sprint', id: '9' }; + + function canDropOnto(targetElement:HTMLElement, root:HTMLElement, permitted:DestinationIdentity[]) { return vi.mocked(dropTargetForElements).mock.lastCall?.[0].canDrop?.({ element: targetElement, input: {} as never, @@ -500,8 +589,7 @@ describe('Sortable lists item controller', () => { type: 'item', itemId: '456', rootElement: root, - sourceListElement, - confined: true, + permittedDestinations: permitted, }), element: document.createElement('article'), } as never, @@ -516,40 +604,47 @@ describe('Sortable lists item controller', () => { expect(draggable).toHaveBeenCalledWith(expect.objectContaining({ element })); }); - it('is accepted by a row inside its source list', () => { + it('is accepted by a row inside a permitted list', () => { + const root = document.createElement('div'); + const targetElement = document.createElement('article'); + + connectedControllerFor(targetElement, { root: fakeRoot(root, { ownerDestination: sprint7 }) }); + + expect(canDropOnto(targetElement, root, [sprint7])).toBe(true); + }); + + // The list a morph replaced keeps its identity, so the drop it would have + // refused on a frozen element still lands. + it('is accepted by a row whose list was replaced mid-drag', () => { const root = document.createElement('div'); - const sourceList = document.createElement('div'); const targetElement = document.createElement('article'); - sourceList.appendChild(targetElement); - connectedControllerFor(targetElement, { root: fakeRoot(root) }); + connectedControllerFor(targetElement, { root: fakeRoot(root, { ownerDestination: { ...sprint7 } }) }); - expect(canDropOnto(targetElement, root, sourceList)).toBe(true); + expect(canDropOnto(targetElement, root, [{ ...sprint7 }])).toBe(true); }); - it('is refused by a row of a foreign list', () => { + it('is refused by a row outside every permitted list', () => { const root = document.createElement('div'); - const sourceList = document.createElement('div'); const targetElement = document.createElement('article'); - connectedControllerFor(targetElement, { root: fakeRoot(root) }); + connectedControllerFor(targetElement, { root: fakeRoot(root, { ownerDestination: sprint9 }) }); - expect(canDropOnto(targetElement, root, sourceList)).toBe(false); + expect(canDropOnto(targetElement, root, [sprint7])).toBe(false); }); - it('is refused everywhere when its payload carries no source list', () => { + it('is refused everywhere when its payload permits no list', () => { const root = document.createElement('div'); const targetElement = document.createElement('article'); - connectedControllerFor(targetElement, { root: fakeRoot(root) }); + connectedControllerFor(targetElement, { root: fakeRoot(root, { ownerDestination: sprint7 }) }); - expect(canDropOnto(targetElement, root, null)).toBe(false); + expect(canDropOnto(targetElement, root, [])).toBe(false); }); }); - it('accepts an unconfined drop from a row of another list', () => { + it('accepts an unrestricted drop from a row of another list', () => { const root = document.createElement('div'); - const foreignList = document.createElement('div'); const targetElement = document.createElement('article'); connectedControllerFor(targetElement, { root: fakeRoot(root) }); @@ -562,7 +657,6 @@ describe('Sortable lists item controller', () => { type: 'item', itemId: '456', rootElement: root, - sourceListElement: foreignList, }), element: document.createElement('article'), } as never, @@ -633,6 +727,23 @@ describe('Sortable lists item controller', () => { ); }); + it('keeps a non-web URL out of the text/html flavour', () => { + const element = document.createElement('article'); + + connectedControllerFor(element, { + externalUrl: 'javascript:alert(1)', + label: 'Card', + }); + + const externalData = vi.mocked(draggable).mock.lastCall?.[0] + .getInitialDataForExternal?.(draggableArgs(element)); + + expect(externalData).toEqual({ + 'text/uri-list': 'javascript:alert(1)', + 'text/plain': 'javascript:alert(1)', + }); + }); + it('does not expose native external drag data without an external URL', () => { const element = document.createElement('article'); @@ -649,6 +760,31 @@ describe('Sortable lists item controller', () => { expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialDataForExternal).toBeUndefined(); }); + it('lists every batch member\'s URL for external consumers', () => { + const element = document.createElement('article'); + const mate = document.createElement('article'); + mate.setAttribute('data-controller', 'sortable-lists--item'); + mate.setAttribute('data-sortable-lists--item-id-value', '124'); + mate.setAttribute('data-sortable-lists--item-external-url-value', 'http://example.org/work_packages/124'); + mate.setAttribute('data-sortable-lists--item-label-value', 'Mate'); + element.setAttribute('data-sortable-lists--item-external-url-value', 'http://example.org/work_packages/123'); + element.setAttribute('data-sortable-lists--item-label-value', 'Card'); + + connectedControllerFor(element, { + externalUrl: 'http://example.org/work_packages/123', + label: 'Card', + root: { ...fakeRoot(), externalDragItems: vi.fn(() => [element, mate]) }, + }); + + const externalData = vi.mocked(draggable).mock.lastCall?.[0].getInitialDataForExternal?.(draggableArgs(element)); + + expect(externalData).toEqual({ + 'text/uri-list': 'http://example.org/work_packages/123\r\nhttp://example.org/work_packages/124', + 'text/plain': 'http://example.org/work_packages/123\nhttp://example.org/work_packages/124', + 'text/html': 'Card
Mate', + }); + }); + it('prevents unhandled browser drag feedback while dragging an item', () => { const element = document.createElement('article'); @@ -725,6 +861,21 @@ describe('Sortable lists item controller', () => { })).toBe(false); }); + it('refuses the drag when the root refuses it', () => { + const element = document.createElement('article'); + const text = document.createElement('span'); + element.appendChild(text); + vi.spyOn(document, 'elementFromPoint').mockReturnValue(text); + + const root = fakeRoot(); + root.dragRefused = vi.fn(() => true); + connectedControllerFor(element, { root }); + + expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ + element, dragHandle: null, input: { clientX: 10, clientY: 10 } as never, + })).toBe(false); + }); + it('refuses to drag before the root reference is connected', () => { const element = document.createElement('article'); const text = document.createElement('span'); @@ -747,25 +898,42 @@ describe('Sortable lists item controller', () => { .toEqual(expect.objectContaining({ itemId: '123', type: 'item', rootElement: root })); }); - it('includes the root-resolved source list and confinement in the drag payload', () => { + it('includes the root-resolved permitted destinations in the payload', () => { const root = document.createElement('div'); - const sourceList = document.createElement('div'); const element = document.createElement('article'); connectedControllerFor(element, { - root: fakeRoot(root, { ownerListElement: sourceList }), + root: fakeRoot(root, { ownerDestination: { type: 'sprint', id: '7' } }), mobility: 'confined', }); expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(element))) - .toEqual(expect.objectContaining({ sourceListElement: sourceList, confined: true })); + .toEqual(expect.objectContaining({ permittedDestinations: [{ type: 'sprint', id: '7' }] })); }); - it('defaults the payload to unconfined with no source list', () => { + // Rootless, so the item's own mobility is the whole answer: free accepts + // every list, and a confined one cannot name the list it sits in. + it('permits every list for a rootless free item', () => { const element = document.createElement('article'); connectedControllerFor(element); expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(element))) - .toEqual(expect.objectContaining({ sourceListElement: null, confined: false })); + .toEqual(expect.objectContaining({ permittedDestinations: null })); + }); + + // The permitted destinations are the root's batch-aware answer, not the + // item's own mobility: a free card dragging a confined batch-mate is pinned + // to the mate's list, which need not be its own. + it('carries the batch-aware permitted destinations of the root in the payload', () => { + const root = document.createElement('div'); + const element = document.createElement('article'); + const mateDestination = { type: 'sprint', id: '9' }; + connectedControllerFor(element, { + root: { ...fakeRoot(root), dragPermittedDestinations: vi.fn(() => [mateDestination]) }, + mobility: 'free', + }); + + expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(element))) + .toEqual(expect.objectContaining({ permittedDestinations: [mateDestination] })); }); describe('Stimulus application wiring', () => { @@ -930,6 +1098,68 @@ describe('Sortable lists item controller', () => { expect(preview.querySelector('[data-backlogs--work-package-target]')).toBeNull(); }); + it('renders no batch badge without a connected root', async () => { + const { article } = renderBacklogsRow(); + const previewContainer = document.createElement('div'); + + vi.spyOn(article, 'getBoundingClientRect').mockReturnValue({ + x: 0, y: 0, top: 0, left: 0, right: 320, bottom: 64, width: 320, height: 64, toJSON: vi.fn(), + }); + + await ctx.nextFrame(); + + vi.mocked(draggable).mock.lastCall?.[0].onGenerateDragPreview?.({ + ...dragEventPayload(article), + nativeSetDragImage: vi.fn(), + }); + + const previewOptions = vi.mocked(setCustomNativeDragPreview).mock.lastCall?.[0] as { + render:({ container }:{ container:HTMLElement }) => void; + }; + previewOptions.render({ container: previewContainer }); + + expect(previewContainer.querySelector('.op-sortable-lists-drag-preview-batch-badge')).toBeNull(); + }); + + it('adds a batch count badge to the preview matching the frozen batch size', async () => { + const { row, article } = renderBacklogsRow(); + const previewContainer = document.createElement('div'); + + vi.spyOn(article, 'getBoundingClientRect').mockReturnValue({ + x: 0, y: 0, top: 0, left: 0, right: 320, bottom: 64, width: 320, height: 64, toJSON: vi.fn(), + }); + + await ctx.nextFrame(); + + const controller = ctx.getController>('sortable-lists--item', row); + controller.connectRoot({ + element: row, + busy: false, + moveInDirection: vi.fn(), + moveAvailability: vi.fn(() => null), + ownerRowsContainer: vi.fn(() => null), + freezeDragBatch: vi.fn(() => 3), + markDragBatch: vi.fn(), + dragPermittedDestinations: vi.fn(() => null), + ownerDestinationOf: vi.fn(() => null), + dragRefused: vi.fn(() => false), + externalDragItems: vi.fn((element:HTMLElement) => [element]), + }); + + vi.mocked(draggable).mock.lastCall?.[0].onGenerateDragPreview?.({ + ...dragEventPayload(article), + nativeSetDragImage: vi.fn(), + }); + + const previewOptions = vi.mocked(setCustomNativeDragPreview).mock.lastCall?.[0] as { + render:({ container }:{ container:HTMLElement }) => void; + }; + previewOptions.render({ container: previewContainer }); + + const badge = previewContainer.querySelector('.op-sortable-lists-drag-preview-batch-badge'); + expect(badge?.textContent).toEqual('3'); + }); + it('offsets the preview so the pointer keeps its grab position on the card', async () => { const { article } = renderBacklogsRow(); @@ -973,6 +1203,80 @@ describe('Sortable lists item controller', () => { expect(previewOptions.getOffset({ container })).toEqual({ x: 40, y: 30 }); }); + // A batch preview pads the container's top for the badge overhang, + // shifting the card down by it, so the grab offset has to shift too. + // Rendered through the real preview, so the padding measured here is the + // one renderDragPreview writes. + it('extends the grab offset by the batch container padding', async () => { + const { row, article } = renderBacklogsRow(); + + vi.spyOn(article, 'getBoundingClientRect').mockReturnValue({ + x: 100, + y: 200, + top: 200, + left: 100, + right: 420, + bottom: 264, + width: 320, + height: 64, + toJSON: vi.fn(), + }); + + await ctx.nextFrame(); + + const controller = ctx.getController>('sortable-lists--item', row); + controller.connectRoot({ + element: row, + busy: false, + moveInDirection: vi.fn(), + moveAvailability: vi.fn(() => null), + ownerRowsContainer: vi.fn(() => null), + freezeDragBatch: vi.fn(() => 3), + markDragBatch: vi.fn(), + dragPermittedDestinations: vi.fn(() => null), + ownerDestinationOf: vi.fn(() => null), + dragRefused: vi.fn(() => false), + externalDragItems: vi.fn((element:HTMLElement) => [element]), + }); + + vi.mocked(draggable).mock.lastCall?.[0].onGenerateDragPreview?.({ + ...dragEventPayload(article), + location: { current: { input: { clientX: 140, clientY: 230 } } } as never, + nativeSetDragImage: vi.fn(), + }); + + const previewOptions = vi.mocked(setCustomNativeDragPreview).mock.lastCall?.[0] as { + render:({ container }:{ container:HTMLElement }) => void; + getOffset:(args:{ container:HTMLElement }) => { x:number; y:number }; + }; + const container = document.createElement('div'); + // getComputedStyle resolves empty on a detached element. + document.body.appendChild(container); + // The overhang the preview pads with comes from drag_and_drop.sass, + // which no spec loads, so the token is declared here to resolve. + container.style.setProperty('--op-drag-badge-overhang', '8px'); + + vi.spyOn(container, 'getBoundingClientRect').mockReturnValue({ + x: 0, + y: 0, + top: 0, + left: 0, + right: 336, + bottom: 88, + width: 336, + height: 88, + toJSON: vi.fn(), + }); + + try { + previewOptions.render({ container }); + + expect(previewOptions.getOffset({ container })).toEqual({ x: 40, y: 38 }); + } finally { + container.remove(); + } + }); + function generatePreview(article:HTMLElement):HTMLElement { const previewContainer = document.createElement('div'); @@ -1327,25 +1631,60 @@ describe('Sortable lists item controller', () => { expect(document.activeElement).toBe(item); }); - it('collapses the batch onto the dragged item when a drag starts', async () => { + it('marks the batch on drag start', async () => { const item = await renderItem({ mobility: 'free' }); const controller = controllerFor(item); - const collapseSelectionForDrag = vi.fn(); + const markDragBatch = vi.fn(); const root:SortableListsRoot = { element: item, busy: false, moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), - ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), - collapseSelectionForDrag, + freezeDragBatch: vi.fn(() => 1), + markDragBatch, + dragPermittedDestinations: vi.fn(() => null), + ownerDestinationOf: vi.fn(() => null), + dragRefused: vi.fn(() => false), + externalDragItems: vi.fn((element:HTMLElement) => [element]), }; controller.connectRoot(root); vi.mocked(draggable).mock.lastCall?.[0].onDragStart?.(dragEventPayload(item)); - expect(collapseSelectionForDrag).toHaveBeenCalledWith(item); + expect(markDragBatch).toHaveBeenCalled(); + }); + + // Pragmatic invokes onGenerateDragPreview before onDragStart, so the + // batch has to be frozen by preview time. Proven on an item with no + // preview target, which catches a call made past the preview guard. + it('freezes the batch at the top of onGenerateDragPreview, before the preview renders', async () => { + const item = await renderItem({ mobility: 'free' }); + const controller = controllerFor(item); + const freezeDragBatch = vi.fn(() => 1); + const root:SortableListsRoot = { + element: item, + busy: false, + moveInDirection: vi.fn(), + moveAvailability: vi.fn(() => null), + ownerRowsContainer: vi.fn(() => null), + freezeDragBatch, + markDragBatch: vi.fn(), + dragPermittedDestinations: vi.fn(() => null), + ownerDestinationOf: vi.fn(() => null), + dragRefused: vi.fn(() => false), + externalDragItems: vi.fn((element:HTMLElement) => [element]), + }; + + controller.connectRoot(root); + + vi.mocked(draggable).mock.lastCall?.[0].onGenerateDragPreview?.({ + ...dragEventPayload(item), + nativeSetDragImage: vi.fn(), + }); + + expect(freezeDragBatch).toHaveBeenCalledWith(item); }); }); }); 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 9ee39dd476b4..86d5edefe0dc 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -33,6 +33,7 @@ import { } from '@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge'; import { combine } from '@atlaskit/pragmatic-drag-and-drop/combine'; import { draggable, dropTargetForElements } from '@atlaskit/pragmatic-drag-and-drop/element/adapter'; +import { formatURLsForExternal } from '@atlaskit/pragmatic-drag-and-drop/element/format-urls-for-external'; import { preserveOffsetOnSource } from '@atlaskit/pragmatic-drag-and-drop/element/preserve-offset-on-source'; import { setCustomNativeDragPreview } from '@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview'; import { preventUnhandled } from '@atlaskit/pragmatic-drag-and-drop/prevent-unhandled'; @@ -41,14 +42,23 @@ import { Controller, type ActionEvent } from '@hotwired/stimulus'; import type { ActionMenuElement } from '@openproject/primer-view-components/app/components/primer/alpha/action_menu/action_menu_element'; import { closestDragBlockingElement } from 'core-stimulus/helpers/interactive-element-helper'; import { - confinementAllowsDrop, + permittedDestinationsAllowDrop, isItemFromRoot, sortableItemData, + sortableItemIdentity, type RootAwareChild, type SortableItemData, type SortableListsRoot, } from './drag-and-drop'; -import { isConfinedItem, isMoveDirection, isOrderableItem, sortableItemSelector } from './list-dom'; +import { + isMoveDirection, + isOrderableItem, + itemMobility, + resolveItemExternalUrl, + resolveItemLabel, + sortableItemSelector, + webLinkHref, +} from './list-dom'; import { renderDragPreview } from './preview'; type CleanupFn = () => void; @@ -62,10 +72,8 @@ export default class ItemController extends Controller implements R type: String, externalUrl: String, hideUnavailable: { type: Boolean, default: true }, - label: String, // See ItemMobility in list-dom. A `confined` item is still a full drag - // source; only its own list accepts it as a drop target, so a release - // anywhere else lands nowhere and the item stays put. + // source; only the lists the batch's permitted set names accept it. mobility: { type: String, default: 'free' }, }; @@ -76,8 +84,6 @@ export default class ItemController extends Controller implements R declare readonly externalUrlValue:string; declare readonly hasExternalUrlValue:boolean; declare readonly hideUnavailableValue:boolean; - declare readonly labelValue:string; - declare readonly hasLabelValue:boolean; declare readonly handleTarget:HTMLElement; declare readonly hasHandleTarget:boolean; @@ -221,16 +227,14 @@ export default class ItemController extends Controller implements R } : {}), canDrag: ({ input }) => { const { root } = this; - if (root == null || root.busy) { + if (root == null || root.busy || root.dragRefused(this.element)) { return false; } return this.canDragFromPoint(input.clientX, input.clientY); }, getInitialData: () => this.getItemData(), onDragStart: () => { - // One drag moves one item until AGILE-278 lands, so a wider batch - // collapses onto it rather than appearing to come along. - this.root?.collapseSelectionForDrag(this.element); + this.root?.markDragBatch(); // Cancels drops landing outside registered drop targets. This also // guards the external data channel: a misdropped card carrying // text/uri-list would otherwise navigate the current tab to that URL. @@ -243,20 +247,38 @@ export default class ItemController extends Controller implements R this.element.removeAttribute('data-dragging'); }, onGenerateDragPreview: ({ location, nativeSetDragImage }) => { + // Pragmatic dispatches this before onDragStart, so the batch has to + // be frozen by the time the preview renders. + const batchSize = this.root?.freezeDragBatch(this.element) ?? 1; + if (!this.hasPreviewTarget) { return; } setCustomNativeDragPreview({ nativeSetDragImage, - getOffset: preserveOffsetOnSource({ - element: this.previewTarget, - input: location.current.input, - }), + // preserveOffsetOnSource assumes the card sits at the container's + // origin, but a batch preview pads the container's top for the + // badge overhang and shifts the card down by it. Measured off the + // container, so the stylesheet stays the single source of the + // geometry; a single-card preview measures 0. + getOffset: (args) => { + const offset = preserveOffsetOnSource({ + element: this.previewTarget, + input: location.current.input, + })(args); + + return { + x: offset.x, + // A detached container's computed style resolves empty. + y: offset.y + (parseFloat(getComputedStyle(args.container).paddingTop) || 0), + }; + }, render: ({ container }) => renderDragPreview({ previewTarget: this.previewTarget, sourceElement: this.element, container, + batchSize, }), }); }, @@ -281,15 +303,15 @@ export default class ItemController extends Controller implements R return isItemFromRoot(root.element, source.data) && source.data.itemId !== this.idValue && source.data.type === this.typeValue - && confinementAllowsDrop(source.data, this.element); - }, - getData: ({ input }) => { - return attachClosestEdge(this.getItemData(), { - element: this.element, - input, - allowedEdges: ['top', 'bottom'], - }); + && !this.element.hasAttribute('data-dragging') + && permittedDestinationsAllowDrop(source.data, this.root?.ownerDestinationOf(this.element) ?? null); }, + // Only the identity a drop needs; the batch-aware fields are computed + // for the dragged source alone. + getData: ({ input }) => attachClosestEdge( + sortableItemIdentity({ itemId: this.idValue, type: this.typeValue }), + { element: this.element, input, allowedEdges: ['top', 'bottom'] }, + ), getIsSticky: ({ input }) => this.isWithinRowsSpan(input), onDragEnter: ({ self }) => { const closestEdge = extractClosestEdge(self.data); @@ -340,22 +362,31 @@ export default class ItemController extends Controller implements R && input.clientY <= lastRow.getBoundingClientRect().bottom; } - // The URL flavours carry the bare URL; text/html joins in only when the item - // has a label (the same one announcements use), as a link for rich-text - // targets (notes apps, editors). The anchor is built through a detached DOM - // element so the browser escapes the label and URL canonically. + // Every member of the prospective batch, so an external drop receives the + // whole block; text/html joins in as one link per labelled member. private externalDragData():Record { - const url = this.externalUrlValue; + const members = this.root?.externalDragItems(this.element) ?? [this.element]; + const entries = members + .map((member) => ({ url: resolveItemExternalUrl(member), label: resolveItemLabel(member) })) + .filter((entry):entry is { url:string; label:string|null } => entry.url !== null); + const urls = entries.map((entry) => entry.url); const data:Record = { - 'text/uri-list': url, - 'text/plain': url, + 'text/uri-list': formatURLsForExternal(urls), + 'text/plain': urls.join('\n'), }; - if (this.hasLabelValue && this.labelValue !== '') { + const links = entries.flatMap((entry) => { + const href = entry.label ? webLinkHref(entry.url) : null; + if (!href) { + return []; + } const anchor = this.element.ownerDocument.createElement('a'); - anchor.href = url; - anchor.textContent = this.labelValue; - data['text/html'] = anchor.outerHTML; + anchor.href = href; + anchor.textContent = entry.label; + return [anchor.outerHTML]; + }); + if (links.length > 0) { + data['text/html'] = links.join('
'); } return data; @@ -366,8 +397,12 @@ export default class ItemController extends Controller implements R itemId: this.idValue, type: this.typeValue, rootElement: this.root?.element ?? null, - sourceListElement: this.root?.ownerListElementOf(this.element) ?? null, - confined: isConfinedItem(this.element), + // A rootless item can carry no batch, so its own mobility is the + // whole answer, and it can name no list either: anything short of free + // movement leaves it accepting nothing. + permittedDestinations: this.root + ? this.root.dragPermittedDestinations(this.element) + : (itemMobility(this.element) === 'free' ? null : []), }); } @@ -397,14 +432,13 @@ export default class ItemController extends Controller implements R return { element: this.element, edge }; } - const nextItem = this.element.nextElementSibling; + let next = this.element.nextElementSibling; + while (next instanceof HTMLElement && next.matches(sortableItemSelector) && next.hasAttribute('data-dragging')) { + next = next.nextElementSibling; + } - if ( - nextItem instanceof HTMLElement && - nextItem.matches(sortableItemSelector) && - !nextItem.hasAttribute('data-dragging') - ) { - return { element: nextItem, edge: 'top' }; + if (next instanceof HTMLElement && next.matches(sortableItemSelector)) { + return { element: next, edge: 'top' }; } return { element: this.element, edge }; 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 da677c3b14f9..7fe83a4f7a2d 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 @@ -28,8 +28,8 @@ import { captureRowPositions, - isConfinedItem, isOrderableItem, + itemAcceptsDestination, itemMobility, reorderRows, sortableItemMobilityAttribute, @@ -89,7 +89,7 @@ describe('sortable lists DOM helpers', () => { list.append(itemRow('1'), showMoreRow(), itemRow('2'), itemRow('3')); - expect(resolveListAppendPreviousItemId({ sourceItemId: '3', rowsContainer: list })).toEqual('2'); + expect(resolveListAppendPreviousItemId({ excludedItems: { type: 'work_package', ids: new Set(['3']) }, rowsContainer: list })).toEqual('2'); }); it('returns null when the list has no other items', () => { @@ -97,7 +97,7 @@ describe('sortable lists DOM helpers', () => { list.append(itemRow('1')); - expect(resolveListAppendPreviousItemId({ sourceItemId: '1', rowsContainer: list })).toBeNull(); + expect(resolveListAppendPreviousItemId({ excludedItems: { type: 'work_package', ids: new Set(['1']) }, rowsContainer: list })).toBeNull(); }); it('returns null for an empty list nested inside an outer item, not the outer item\'s id', () => { @@ -113,7 +113,19 @@ describe('sortable lists DOM helpers', () => { list.append(placeholder); outerItem.append(list); - expect(resolveListAppendPreviousItemId({ sourceItemId: 'field-1', rowsContainer: list })).toBeNull(); + expect(resolveListAppendPreviousItemId({ excludedItems: { type: 'work_package', ids: new Set(['field-1']) }, rowsContainer: list })).toBeNull(); + }); + + it('appends after the last row not in the excluded set', () => { + // rows: A, B, C — excluded {B, C} → append lands after A. + const rowsContainer = listElement(); + + rowsContainer.append(itemRow('A'), itemRow('B'), itemRow('C')); + + expect(resolveListAppendPreviousItemId({ + excludedItems: { type: 'work_package', ids: new Set(['B', 'C']) }, + rowsContainer, + })).toBe('A'); }); }); @@ -416,14 +428,30 @@ describe('itemMobility', () => { expect(itemMobility(itemWith(''))).toBe('fixed'); }); - it('derives orderable and confined from the union', () => { + it('derives orderable from the union', () => { expect(isOrderableItem(itemWith('free'))).toBe(true); expect(isOrderableItem(itemWith('confined'))).toBe(true); expect(isOrderableItem(itemWith('fixed'))).toBe(false); + }); +}); + +describe('itemAcceptsDestination', () => { + const sprint1 = { type: 'sprint', id: '1' }; + const sprint2 = { type: 'sprint', id: '2' }; + + function item(mobility:'fixed'|'confined'|'free' = 'free'):HTMLElement { + const element = document.createElement('li'); + element.setAttribute(sortableItemMobilityAttribute, mobility); + return element; + } + + it('answers for one item which destinations it accepts', () => { + const ownerDestinationOf = () => sprint1; - expect(isConfinedItem(itemWith('confined'))).toBe(true); - expect(isConfinedItem(itemWith('free'))).toBe(false); - expect(isConfinedItem(itemWith('fixed'))).toBe(false); + expect(itemAcceptsDestination(item(), sprint2, ownerDestinationOf)).toBe(true); + expect(itemAcceptsDestination(item('fixed'), sprint1, ownerDestinationOf)).toBe(false); + expect(itemAcceptsDestination(item('confined'), sprint1, ownerDestinationOf)).toBe(true); + expect(itemAcceptsDestination(item('confined'), sprint2, ownerDestinationOf)).toBe(false); }); }); 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 1ef732f7e504..70898adda405 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -114,8 +114,32 @@ export function isOrderableItem(itemElement:Element):boolean { return itemMobility(itemElement) !== 'fixed'; } -export function isConfinedItem(itemElement:Element):boolean { - return itemMobility(itemElement) === 'confined'; +// A destination an item may be moved to: a list, identified by type and id +// (null for the type's unlisted bucket). +export interface DestinationIdentity { + type:string; + id:string|null; +} + +export function sameDestination(left:DestinationIdentity|null, right:DestinationIdentity):boolean { + return left?.type === right.type && left.id === right.id; +} + +// Whether the item may enter the destination: the one policy behind every +// surface offering a move. +export function itemAcceptsDestination( + item:HTMLElement, + target:DestinationIdentity, + ownerDestinationOf:(item:HTMLElement) => DestinationIdentity|null, +):boolean { + switch (itemMobility(item)) { + case 'fixed': + return false; + case 'confined': + return sameDestination(ownerDestinationOf(item), target); + default: + return true; + } } export function resolveItemType(element:Element):string|null { @@ -160,6 +184,33 @@ export function resolvePreviousItemId(element:Element, boundary:Element):string| return item ? resolveItemId(item) : element.getAttribute(sortablePreviousItemIdAttribute); } +// resolvePreviousItemId plus the type of the item the id belongs to. A +// truncation marker row resolves no item element, so its id carries no type. +export function resolvePreviousItem(element:Element, boundary:Element):{ id:string; type:string|null }|null { + const item = resolveItemElement(element, boundary); + if (item) { + const id = resolveItemId(item); + return id ? { id, type: resolveItemType(item) } : null; + } + + const markerId = element.getAttribute(sortablePreviousItemIdAttribute); + return markerId ? { id: markerId, type: null } : null; +} + +// The dragged batch a predecessor walk must skip. One item type per batch, +// so a type plus an id set represents it completely. +export interface ExcludedItems { + type:string; + ids:ReadonlySet; +} + +// Excluded only when id and type both match: ids collide across source +// tables, so a same-id row of another type is a legitimate anchor. A +// truncation marker resolves no type and stays excluded on its id alone. +export function isExcludedItem(excluded:ExcludedItems, { id, type }:{ id:string; type:string|null }):boolean { + return excluded.ids.has(id) && (type === null || type === excluded.type); +} + // The inverse of resolvePreviousItemId: the previous item id can point at a // hidden item collapsed behind a truncation marker row, which carries the id // on data-sortable-lists-prev-item-id rather than exposing an item element. @@ -176,18 +227,18 @@ function resolveAnchorRow(rowsContainer:HTMLElement, previousItemId:string):HTML } export function resolveListAppendPreviousItemId({ - sourceItemId, + excludedItems, rowsContainer, }:{ - sourceItemId:string; + excludedItems:ExcludedItems; rowsContainer:Element; }):string|null { const rows = listRows(rowsContainer).reverse(); for (const row of rows) { - const itemId = resolvePreviousItemId(row, rowsContainer); - if (itemId && itemId !== sourceItemId) { - return itemId; + const item = resolvePreviousItem(row, rowsContainer); + if (item && !isExcludedItem(excludedItems, item)) { + return item.id; } } @@ -341,6 +392,17 @@ export function resolveItemLabel(row:Element):string|null { : null; } +export function resolveItemExternalUrl(itemElement:Element):string|null { + const url = itemElement.getAttribute('data-sortable-lists--item-external-url-value'); + return url === '' ? null : url; +} + +// text/html reaches targets that follow the link, so only a web URL becomes +// one; any other scheme stays confined to the plain flavours. +export function webLinkHref(url:string):string|null { + return url.startsWith('http://') || url.startsWith('https://') ? url : null; +} + // A row a predecessor id can be read from: an item row, or a non-item row // annotated with the id of the last hidden item it stands in for (a // truncation marker). Unannotated non-item rows (a divider, a heading) give 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 e1eaa25599f0..027d6882f171 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 @@ -36,6 +36,7 @@ import type { dropTargetForElements as dropTargetForElementsFn } from '@atlaskit import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; import type ListControllerType from './list.controller'; import type { sortableItemData as sortableItemDataFn, SortableListsRoot } from './drag-and-drop'; +import type { DestinationIdentity } from './list-dom'; // The list controller is tested in ISOLATION: the root drives the outlet // hand-over in production (sortable-lists.controller.ts), so here we render only @@ -76,9 +77,13 @@ describe('Sortable lists list controller', () => { busy, moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), - ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), - collapseSelectionForDrag: vi.fn(), + freezeDragBatch: vi.fn(() => 1), + markDragBatch: vi.fn(), + dragPermittedDestinations: vi.fn(() => null), + ownerDestinationOf: vi.fn(() => null), + dragRefused: vi.fn(() => false), + externalDragItems: vi.fn((item:HTMLElement) => [item]), }; } @@ -120,10 +125,10 @@ describe('Sortable lists list controller', () => { function source( rootElement:HTMLElement|null, type = 'work_package', - { confined = false, sourceListElement = null }:{ confined?:boolean; sourceListElement?:HTMLElement|null } = {}, + { permittedDestinations = null }:{ permittedDestinations?:DestinationIdentity[]|null } = {}, ) { return { - data: sortableItemData({ itemId: '1', type, rootElement, confined, sourceListElement }), + data: sortableItemData({ itemId: '1', type, rootElement, permittedDestinations }), element: document.createElement('li'), } as never; } @@ -240,7 +245,7 @@ describe('Sortable lists list controller', () => { expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, - source: source(root, 'work_package', { confined: true, sourceListElement: document.createElement('ul') }), + source: source(root, 'work_package', { permittedDestinations: [{ type: 'sprint', id: '9' }] }), })).toBe(true); }); @@ -303,7 +308,7 @@ describe('Sortable lists list controller', () => { options?.onDragEnter?.({ location: locationOver(), - source: source(rootElement, 'work_package', { confined: true, sourceListElement: document.createElement('ul') }), + source: source(rootElement, 'work_package', { permittedDestinations: [{ type: 'sprint', id: '9' }] }), } as never); expect(list.dataset.dropContainer).toEqual('refused'); @@ -316,7 +321,7 @@ describe('Sortable lists list controller', () => { options?.onDragEnter?.({ location: locationOver(), - source: source(rootElement, 'work_package', { confined: true, sourceListElement: list }), + source: source(rootElement, 'work_package', { permittedDestinations: [{ type: 'sprint', id: '7' }] }), } as never); expect(list.dataset.dropContainer).toEqual('active'); @@ -329,7 +334,7 @@ describe('Sortable lists list controller', () => { options?.onDragEnter?.({ location: locationOver(), - source: source(rootElement, 'work_package', { confined: true, sourceListElement: document.createElement('ul') }), + source: source(rootElement, 'work_package', { permittedDestinations: [{ type: 'sprint', id: '9' }] }), } as never); expect(list.dataset.dropContainer).toEqual('refused'); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts index 7898d55d4ac1..c342eb1e6191 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts @@ -30,7 +30,8 @@ import { dropTargetForElements } from '@atlaskit/pragmatic-drag-and-drop/element import { type DragLocationHistory } from '@atlaskit/pragmatic-drag-and-drop/types'; import { Controller } from '@hotwired/stimulus'; import { - confinementAllowsDrop, + destinationOfList, + permittedDestinationsAllowDrop, isItemFromRoot, isSortableItemData, sortableListData, @@ -96,12 +97,13 @@ export default class ListController extends Controller implements R this.cleanupFn = dropTargetForElements({ element: this.element, - // A confined item from another list stays accepted here on purpose, - // even though releasing it resolves to nothing (see acceptsDrop): only - // an accepted target lets Pragmatic keep the standard 'move' drop - // effect on the dragover, and with it the standard cursor. Refused, the - // container falls through to the browser default, which Chrome renders - // as a copy cursor — promising an "add" that will never happen. + // A container the dragged block may not reach stays accepted here on + // purpose, even though releasing it resolves to nothing (see + // permittedDestinationsAllowDrop): only an accepted target lets Pragmatic keep + // the standard 'move' drop effect on the dragover, and with it the + // standard cursor. Refused, the container falls through to the browser + // default, which Chrome renders as a copy cursor — promising an "add" + // that will never happen. // Pragmatic's getDropEffect cannot express 'none', so acceptance is the // only supported way to control the cursor over these containers. canDrop: ({ source }) => this.canDrop(source.data), @@ -178,17 +180,17 @@ export default class ListController extends Controller implements R // The list is the item targets' parent drop target, so its onDrag keeps firing // while the pointer is over a row. Indicate only for a list-only drop (no item // target in play), so the row gap indicator owns the over-a-row case. Whether - // a release would amount to a move decides the indicator's state: a confined - // item's source list counts as a move (containment includes the list element - // itself, keeping within-list reorder alive), while a foreign container stays - // an accepted drop target (see canDrop above) whose release resolves to - // nothing — resolveDropIntent applies the same confinement filter — and is - // marked refused so it can signal that a drop will not land here. + // a release would amount to a move decides the indicator's state: a permitted + // destination counts as a move (this list's own destination, keeping + // within-list reorder alive), while an unpermitted container stays an accepted + // drop target (see canDrop above) whose release resolves to nothing — + // resolveDropIntent gates on the same permitted destinations — and is marked refused + // so it can signal that a drop will not land here. private syncDropIndicator(location:DragLocationHistory, sourceData:Record):void { if (!isItemFromRoot(this.root?.element ?? null, sourceData) || location.current.dropTargets.some(({ data }) => isSortableItemData(data))) { this.clearDropIndicator(); - } else if (confinementAllowsDrop(sourceData, this.element)) { + } else if (permittedDestinationsAllowDrop(sourceData, destinationOfList(this.listData))) { this.renderDropIndicator('active'); } else { this.renderDropIndicator('refused'); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts index 2595cc54dff1..357e637f8f1e 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts @@ -168,5 +168,181 @@ describe('sortable lists drag preview', () => { expect(container.classList.contains('Box--condensed')).toBe(false); expect(container.classList.contains('Box--spacious')).toBe(false); }); + + describe('batch count badge', () => { + const badgeSelector = '.op-sortable-lists-drag-preview-batch-badge'; + + it('adds no badge for a single-card drag (the default)', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ previewTarget: target, sourceElement: target, container }); + + expect(container.querySelector(badgeSelector)).toBeNull(); + expect(container.style.paddingTop).toEqual(''); + expect(container.style.paddingRight).toEqual(''); + expect(container.style.paddingBottom).toEqual(''); + }); + + it('adds no badge when batchSize is explicitly 1', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 1, + }); + + expect(container.querySelector(badgeSelector)).toBeNull(); + }); + + it('adds a badge with the batch count inside the container for a multi-card drag', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 3, + }); + + const badge = container.querySelector(badgeSelector); + + expect(badge).not.toBeNull(); + expect(badge?.textContent).toEqual('3'); + expect(container.contains(badge)).toBe(true); + }); + + it('carries the Primer Counter classes and the batch-badge class', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 3, + }); + + const badge = container.querySelector(badgeSelector); + + expect(badge?.classList.contains('Counter')).toBe(true); + expect(badge?.classList.contains('Counter--primary')).toBe(true); + expect(badge?.classList.contains('op-sortable-lists-drag-preview-batch-badge')).toBe(true); + }); + + it('anchors the badge to the container without disturbing the already-appended preview clone', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 3, + }); + + const preview = container.querySelector('[data-preview]'); + const badge = container.querySelector(badgeSelector); + + // The preview clone survives lit-html's render() alongside the badge: + // both are present in the container at once. + expect(preview).not.toBeNull(); + expect(badge).not.toBeNull(); + expect(container.contains(preview)).toBe(true); + expect(container.contains(badge)).toBe(true); + }); + + // The padding holds the badge's overhang inside the container's border + // box. It has to be written inline and after Pragmatic's own popover + // reset (padding: 0), which the pre-zeroed padding here reproduces. The + // widths stay in drag_and_drop.sass, so what is written is a reference + // to its tokens rather than a length. + it('pads the container inline past Pragmatic popover reset for a multi-card drag', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + container.style.padding = '0'; + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 3, + }); + + expect(container.style.position).toEqual('relative'); + expect(container.style.paddingTop).toEqual('var(--op-drag-badge-overhang)'); + expect(container.style.paddingRight).toEqual('var(--op-drag-stack-overhang)'); + expect(container.style.paddingBottom).toEqual('var(--op-drag-stack-overhang)'); + expect(container.style.paddingLeft).toEqual('0px'); + }); + }); + + describe('batch stack', () => { + const stackDepth = (container:HTMLElement) => container + .querySelector('[data-preview]') + ?.getAttribute('data-stack-depth'); + + it('leaves the clone unstacked for a single-card drag (the default)', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ previewTarget: target, sourceElement: target, container }); + + expect(stackDepth(container)).toBeNull(); + }); + + it('leaves the clone unstacked when batchSize is explicitly 1', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 1, + }); + + expect(stackDepth(container)).toBeNull(); + }); + + it.each([ + [2, '1'], + [3, '2'], + [4, '3'], + ])('gives a batch of %i a layer per card behind the front one', (batchSize, depth) => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize, + }); + + expect(stackDepth(container)).toEqual(depth); + }); + + // Capping how deep the stack draws belongs to the stylesheet, which + // holds every depth past its layers at the deepest one it has. + it('passes a large batch through uncapped', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 27, + }); + + expect(stackDepth(container)).toEqual('26'); + }); + + it('stacks the clone for a multi-card drag without disturbing its own classes', () => { + const target = withWidth(previewTarget(), 320); + target.classList.add('op-card'); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 3, + }); + + expect(stackDepth(container)).toEqual('2'); + expect(container.querySelector('[data-preview]')?.classList.contains('op-card')).toBe(true); + }); + + it('adds no element for the layers, so the container holds only the clone and the badge', () => { + const target = withWidth(previewTarget(), 320); + const container = document.createElement('div'); + + renderDragPreview({ + previewTarget: target, sourceElement: target, container, batchSize: 3, + }); + + expect(container.querySelectorAll('[data-preview]')).toHaveLength(1); + expect(container.children).toHaveLength(2); + }); + }); }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts index df42edadfa61..5431dd41930e 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts @@ -26,6 +26,8 @@ // See COPYRIGHT and LICENSE files for more details. //++ +import { html, render } from 'lit-html'; + // Builds the custom native drag preview for a sortable item: a sanitised clone // of the item's preview target, sized to match and carrying the originating // Box's density so its card styling survives being mounted outside the Box. @@ -54,14 +56,35 @@ const PREVIEW_STRIPPED_ATTRIBUTES = [ // `.Box--condensed .Box-card`) would not apply to it otherwise. const BOX_DENSITY_VARIANT_CLASSES = ['Box--condensed', 'Box--spacious'] as const; +// The count badge on a multi-card drag's preview, styled on Primer's Counter +// contract plus this class for the positioning Counter does not own. The +// native drag snapshot is taken synchronously at dragstart, before an Angular +// element would have painted, so it cannot be a custom element. +const BATCH_BADGE_CLASS = 'op-sortable-lists-drag-preview-batch-badge'; + +// Reserved as container padding so nothing paints past the border box: Firefox +// folds such overflow into the drag snapshot and shifts its origin off the grab +// offset. Written inline because Pragmatic inline-resets the container before +// render(), and only a later inline write outranks that; the item controller +// reads the top padding back to compensate the grab offset. Reserved for the +// deepest stack whatever the batch size, so the badge keeps one offset. The +// widths themselves stay in drag_and_drop.sass, which derives the stack one +// from the layer geometry. +const BATCH_BADGE_OVERHANG = 'var(--op-drag-badge-overhang)'; +const DRAG_STACK_OVERHANG = 'var(--op-drag-stack-overhang)'; + export function renderDragPreview({ previewTarget, sourceElement, container, + batchSize = 1, }:{ previewTarget:HTMLElement; sourceElement:HTMLElement; container:HTMLElement; + // A batch larger than one card adds a count badge, so a multi-card drag + // reads differently from a single one. + batchSize?:number; }):void { const previewWidth = previewTarget.getBoundingClientRect().width; const preview = previewTarget.cloneNode(true) as HTMLElement; @@ -86,6 +109,31 @@ export function renderDragPreview({ }); container.append(preview); + + if (batchSize > 1) { + // How many of the further cards the stack can show is the stylesheet's + // call, so the depth goes out unclamped and the deepest rule catches + // anything past the layers it draws. + preview.setAttribute('data-stack-depth', `${batchSize - 1}`); + + // Anchors the badge to the container rather than whatever ancestor + // Pragmatic mounts it under. Nothing paints past the card's left edge. + container.style.position = 'relative'; + container.style.paddingTop = BATCH_BADGE_OVERHANG; + container.style.paddingRight = DRAG_STACK_OVERHANG; + container.style.paddingBottom = DRAG_STACK_OVERHANG; + renderBatchBadge(container, batchSize); + } +} + +// Absolutely positioned over the card clone's top-right corner; the +// container's padding is the overhang it sits in (drag_and_drop.sass). +// +// render() is safe on a container that already holds the preview clone: it +// inserts a marker before its own end node and manages content from there +// on, rather than clearing pre-existing children. +function renderBatchBadge(container:HTMLElement, batchSize:number):void { + render(html`${batchSize}`, container); } export function sanitizePreview(element:HTMLElement):void { 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 8729e01054bc..7436950072f0 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 @@ -72,9 +72,13 @@ describe('Sortable lists scrollable controller', () => { busy: false, moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), - ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), - collapseSelectionForDrag: vi.fn(), + freezeDragBatch: vi.fn(() => 1), + markDragBatch: vi.fn(), + dragPermittedDestinations: vi.fn(() => null), + ownerDestinationOf: vi.fn(() => null), + dragRefused: vi.fn(() => false), + externalDragItems: vi.fn((item:HTMLElement) => [item]), }; } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.ts index 65e7c78221ef..7bee2f64c4dd 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.ts @@ -29,7 +29,7 @@ import { autoScrollForElements } from '@atlaskit/pragmatic-drag-and-drop-auto-scroll/element'; import { Controller } from '@hotwired/stimulus'; import { - isSortableItemData, + isItemFromRoot, type RootAwareChild, type SortableListsRoot, } from './drag-and-drop'; @@ -75,9 +75,7 @@ export default class ScrollableController extends Controller implem element: this.element, canScroll: ({ source }) => { const { root } = this; - return root != null - && isSortableItemData(source.data) - && source.data.rootElement === root.element; + return root != null && isItemFromRoot(root.element, source.data); }, getAllowedAxis: () => this.allowedAxis, getConfiguration: () => ({ maxScrollSpeed: this.maxScrollSpeed }), 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 59c664fc8bc0..d574bd5f1995 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 @@ -148,7 +148,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('1'))); expect(isSelected(item('1'))).toBe(true); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); }); it('extends a range from the anchor', () => { @@ -157,7 +157,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('1'))); orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); - expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2', '3']); }); it('routes focus through the host rather than touching the element', () => { @@ -176,7 +176,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('1'))); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); it('clears presentation without disturbing the model', () => { @@ -186,7 +186,7 @@ describe('SelectionOrchestrator', () => { orchestrator.clearPresentation(); expect(isSelected(item('1'))).toBe(false); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); }); // Falling through mid-move would open the details pane on a card the batch @@ -200,7 +200,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(event); expect(event.defaultPrevented).toBe(true); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); }); // Both roots listen at the document; the first to clear consumes the key, @@ -222,8 +222,8 @@ describe('SelectionOrchestrator', () => { secondRoot.remove(); } - expect(first.selectedIds()).toEqual([]); - expect(second.selectedIds()).toEqual([]); + expect(first.selectedItems().map((i) => i.id)).toEqual([]); + expect(second.selectedItems().map((i) => i.id)).toEqual([]); }); it('still leaves an Escape an overlay consumed alone', () => { @@ -235,7 +235,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleEscape(event); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); }); describe('announcements', () => { @@ -251,7 +251,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('1'), { shiftKey: true })); - expect(orchestrator.selectedIds()).toEqual(['1', '2']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2']); expect(spoken()).toEqual(['[selected:2]']); }); @@ -265,7 +265,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('2'), { shiftKey: true })); - expect(orchestrator.selectedIds()).toEqual(['2']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2']); expect(spoken()).toEqual([]); }); @@ -276,7 +276,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('1'))); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); expect(spoken()).toEqual([]); }); @@ -288,7 +288,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('2'))); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); expect(spoken()).toEqual([]); }); @@ -299,7 +299,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('2'))); - expect(orchestrator.selectedIds()).toEqual(['2']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2']); expect(spoken()).toEqual([]); }); @@ -339,7 +339,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(keydownOn(item('1'), 'ArrowDown', { shiftKey: true })); expect(focused).toBe(item('2')); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); }); // Consuming the key with nothing to select would block the browser's own @@ -353,7 +353,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(false); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); it('still consumes Ctrl/Cmd+A when there is something to select', () => { @@ -363,7 +363,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(true); - expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2', '3']); }); it('anchors select-all inside the list when the focused card is fixed', () => { @@ -372,11 +372,11 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(keydownOn(item('1'), 'a', { ctrlKey: true })); - expect(orchestrator.selectedIds()).toEqual(['2', '3']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2', '3']); // The fallback anchor is the first orderable card of the same list, so // a follow-up Shift ranges within it rather than from another list. orchestrator.handleKeydown(keydownOn(item('3'), ' ', { shiftKey: true })); - expect(orchestrator.selectedIds()).toEqual(['2', '3']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2', '3']); }); // Select all binds to the platform's one multi-select modifier: ⌘ on @@ -389,7 +389,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(true); - expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2', '3']); }); it('leaves Ctrl+A alone on Apple platforms', () => { @@ -400,7 +400,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(false); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); it('leaves Meta+A alone off Apple platforms', () => { @@ -410,7 +410,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(false); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); // Non-Latin layouts print another letter on the A key; AZERTY prints A @@ -422,7 +422,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(true); - expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2', '3']); }); it('selects all from the A key of an AZERTY layout', () => { @@ -431,7 +431,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); - expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2', '3']); }); it('leaves Ctrl+Q alone on an AZERTY layout although it sits on KeyA', () => { @@ -441,7 +441,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(false); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); // Non-Latin keys throughout, so that only the guard under test, never @@ -460,7 +460,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(false); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); // Stubbed rather than passed as `modifierAltGraph`: not every engine maps @@ -473,7 +473,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(false); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); // With nothing selectable in this list, the browser's own select-all @@ -487,7 +487,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); expect(event.defaultPrevented).toBe(false); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); // Holding Space would otherwise toggle the card over and over. @@ -497,7 +497,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(keydownOn(item('1'), ' ', { repeat: true })); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); }); it('refuses to extend a range onto a fixed card', () => { @@ -507,7 +507,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(keydownOn(item('1'), 'ArrowDown', { shiftKey: true })); expect(isSelected(item('2'))).toBe(false); - expect(orchestrator.selectedIds()).toEqual([]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual([]); }); it('refuses to extend a range over a fixed card', () => { @@ -518,7 +518,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); expect(isSelected(item('3'))).toBe(false); expect(announceSpy.mock.calls.map((call) => call[0])).toEqual(['[range_blocked]']); }); @@ -533,7 +533,7 @@ describe('SelectionOrchestrator', () => { const event = clickOn(item('2'), { ctrlKey: true }); orchestrator.handleClick(event); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); expect(isSelected(item('2'))).toBe(false); expect(event.defaultPrevented).toBe(false); }); @@ -545,7 +545,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('2'), { ctrlKey: true })); - expect(orchestrator.selectedIds()).toEqual(['1', '2']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2']); }); // Meta is not an alternate multi-select modifier off Apple platforms: a @@ -557,11 +557,11 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('2'), { metaKey: true })); - expect(orchestrator.selectedIds()).toEqual(['2']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2']); }); }); - // collapseForDrag stamps the anchor at drag start, so a cross-list drop + // The anchor's list key is stamped when it is set, so a cross-list drop // leaves it naming the source list. Without a rebind the next Shift in the // destination reads as cross-list and restarts instead of extending. it('extends a range after the anchored card moved to another list', () => { @@ -573,7 +573,7 @@ describe('SelectionOrchestrator', () => { orchestrator.reconcile(); orchestrator.handleClick(clickOn(item('4'), { shiftKey: true })); - expect(orchestrator.selectedIds()).toEqual(['1', '4']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '4']); }); describe('one batch, one item type', () => { @@ -595,7 +595,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(sectionItem('1'), { ctrlKey: true })); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); expect(isSelected(sectionItem('1'))).toBe(true); expect(isSelected(item('1'))).toBe(false); }); @@ -619,7 +619,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleKeydown(event); - expect(orchestrator.selectedIds()).toEqual(['1']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); expect(isSelected(sectionItem('1'))).toBe(true); expect(isSelected(item('2'))).toBe(false); }); @@ -635,7 +635,7 @@ describe('SelectionOrchestrator', () => { orchestrator.reconcile(); orchestrator.handleClick(clickOn(item('4'), { shiftKey: true })); - expect(orchestrator.selectedIds()).toEqual(['1', '4']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '4']); }); }); @@ -647,7 +647,194 @@ describe('SelectionOrchestrator', () => { orchestrator.reconcile(); - expect(orchestrator.selectedIds()).toEqual(['2']); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2']); expect(announceSpy).toHaveBeenCalled(); }); + + describe('action scopes', () => { + it('reports the selected batch without changing selection or its anchor', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('2'), { ctrlKey: true })); + + const scope = orchestrator.actionScopeFor(item('2')); + + expect(scope).toEqual({ kind: 'batch', items: [item('1'), item('2')] }); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2']); + orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2', '3']); + }); + + it('reports an unselected orderable card alone without selecting it', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + + expect(orchestrator.actionScopeFor(item('3'))).toEqual({ kind: 'batch', items: [item('3')] }); + expect(isSelected(item('3'))).toBe(false); + }); + + it('selects an unselected orderable card for an action, replacing the batch', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('2'), { ctrlKey: true })); + + const scope = orchestrator.selectForAction(item('3')); + + expect(scope).toEqual({ kind: 'batch', items: [item('3')] }); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['3']); + }); + + // A drag with nothing selected selects the dragged card: cancelling the + // drag then leaves the same state whether or not a batch existed before. + it('selects the card for an action when nothing was selected', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + + expect(orchestrator.selectForAction(item('2'))).toEqual({ kind: 'batch', items: [item('2')] }); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2']); + }); + + it('keeps the selected batch for an action on a member', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('3'), { ctrlKey: true })); + + expect(orchestrator.selectForAction(item('1')).items).toEqual([item('1'), item('3')]); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '3']); + }); + + 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.selectForAction(item('3'))).toEqual({ kind: 'refused', items: [] }); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1']); + }); + + it('stays silent when it selects a card with nothing selected', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + announceSpy.mockClear(); + + orchestrator.selectForAction(item('2')); + + expect(announceSpy).not.toHaveBeenCalled(); + expect(isSelected(item('2'))).toBe(true); + }); + + it('stays silent when it replaces a single selection', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + announceSpy.mockClear(); + + orchestrator.selectForAction(item('2')); + + expect(announceSpy).not.toHaveBeenCalled(); + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['2']); + }); + + it('announces the collapse of a wider batch', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('2'), { ctrlKey: true })); + orchestrator.handleClick(clickOn(item('3'), { ctrlKey: true })); + announceSpy.mockClear(); + + orchestrator.selectForAction(item('4')); + + expect(announceSpy.mock.calls.map((call) => call[0])).toEqual(['[selected:1]']); + }); + }); + + // 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)); + orchestrator.handleClick(clickOn(item('1'))); + announceSpy.mockClear(); + + orchestrator.clearSilently(); + + expect(orchestrator.selectedItems()).toEqual([]); + expect(isSelected(item('1'))).toBe(false); + expect(announceSpy).not.toHaveBeenCalled(); + }); + + // Ctrl-click deselecting the only member leaves the anchor behind; a + // later Shift-click must not range from a card that is no longer part + // of anything. + it('drops a dangling anchor left by a deselect', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('1'), { ctrlKey: true })); + + orchestrator.clearSilently(); + orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); + + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['3']); + }); + + it('is a no-op with nothing selected', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + + expect(() => orchestrator.clearSilently()).not.toThrow(); + }); + }); }); 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 b354f224d642..6c3499f1221c 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -26,7 +26,7 @@ // See COPYRIGHT and LICENSE files for more details. //++ -import { BatchSelection, type SelectionAnchor, type SelectionKey } from 'core-common/batch-selection'; +import { BatchSelection, type SelectionAnchor, type SelectionItem, type SelectionKey } from 'core-common/batch-selection'; import { announce } from '@primer/live-region-element'; import { resolveItemId, resolveItemType } from './list-dom'; import { @@ -36,6 +36,7 @@ import { liveOrderableListItems, neighbourItem, orderedItemElements, + orderedSelectedItemElements, orderedSelectedItems, resolveCandidate, resolveRangeItems, @@ -59,6 +60,14 @@ export interface SelectionHost { ownerRowsContainer(itemElement:HTMLElement):HTMLElement|null; } +export type ActionScope = + | { kind:'batch'; items:HTMLElement[] } + | { kind:'refused'; items:[] }; + +// What resolving an action scope does to the selection: nothing, add the +// card to it, or replace it with the card alone. +type ScopeMutation = 'none'|'join'|'collapse'; + /** * Batch selection: gestures in, model and presentation out. */ @@ -88,30 +97,70 @@ export class SelectionOrchestrator { constructor(private readonly host:SelectionHost) {} - // Live ordered membership, for AGILE-278's batch move. - selectedIds():string[] { - return orderedSelectedItems(this.host.rootElement, this.selection.keys).map((item) => item.id); + // Live ordered membership. Full (type, id) pairs: ids collide across + // source tables. + selectedItems():SelectionItem[] { + return orderedSelectedItems(this.host.rootElement, this.selection.keys); } - // A menu move relocates exactly one card, so it collapses like a drag. - collapseForMove(itemElement:HTMLElement):void { - this.collapseForDrag(itemElement); + get hasSelection():boolean { + return this.selection.size > 0; } - collapseForDrag(itemElement:HTMLElement):void { - // Nothing selected is nothing to collapse: a drag must not manufacture - // a one-card batch. - if (this.selection.size === 0) { - return; - } + // The cards an action invoked from this card applies to, without touching + // the selection: the batch when the card is a member, the card alone + // otherwise. Consulted while a drag payload is built, before the batch is + // frozen. + actionScopeFor(itemElement:HTMLElement):ActionScope { + return this.resolveActionScope(itemElement, 'none'); + } + // Same, but an unselected orderable card becomes the selection first, so + // a drag or a menu action on it leaves one consistent state behind. + selectForAction(itemElement:HTMLElement):ActionScope { + 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); if (!candidate?.orderable) { + return { kind: 'refused', items: [] }; + } + + const key = { type: candidate.type, id: candidate.id }; + if (mutation === 'collapse' || (mutation === 'join' && !this.selection.has(key))) { + this.selection.replace(key, candidate.listKey); + // Speaks only when a wider batch actually collapsed: selecting the + // card a gesture landed on is not a loss the user needs read back. + this.renderSelection('navigation'); + } + + const items = this.selection.has(key) + ? orderedSelectedItemElements(this.host.rootElement, this.selection.keys) + : [candidate.itemElement]; + + return { kind: 'batch', items }; + } + + // Silent: the move announcement is the feedback, and "Selection cleared." + // on top of it would be noise. Also drops an anchor a deselect left + // behind, which a later Shift gesture would otherwise range from. + clearSilently():void { + if (this.selection.size === 0 && this.selection.anchor === null) { return; } - this.selection.replace({ type: candidate.type, id: candidate.id }, candidate.listKey); - this.renderSelection('selection'); + this.selection.clear(); + this.syncSelectionPresentation(); + this.lastRenderedKeys = this.selection.keys; } // The platform's one multi-select modifier: ⌘ on Apple platforms, Ctrl diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts index bf9de05b3679..451765d43c94 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts @@ -35,6 +35,7 @@ import { liveOrderableListItems, neighbourItem, orderedItemElements, + orderedSelectedItemElements, orderedSelectedItems, resolveCandidate, resolveRangeItems, @@ -191,6 +192,12 @@ describe('sortable-lists selection adapter', () => { expect(orderedSelectedItems(root, keys).map((item) => item.id)).toEqual(['1', '3', '4']); }); + it('returns selected item elements in live document order', () => { + const keys = new Set(['4', '1', '3'].map((id) => selectionKey({ type: 'work_package', id }))); + + expect(orderedSelectedItemElements(root, keys)).toEqual([itemFor('1'), itemFor('3'), itemFor('4')]); + }); + it('lists only live orderable items', () => { expect(liveOrderableItems(root).map((item) => item.id)).toEqual(['1', '2', '3', '4']); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts index ee3ef3044692..b6a531f8a46c 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts @@ -141,13 +141,20 @@ export function orderedSelectedItems(root:HTMLElement, keys:ReadonlySet item !== null && keys.has(selectionKey(item))); } -function itemIdentity(itemElement:Element):SelectionItem|null { +export function itemIdentity(itemElement:Element):SelectionItem|null { const id = resolveItemId(itemElement); const type = resolveItemType(itemElement); return id && type ? { type, id } : null; } +export function orderedSelectedItemElements(root:HTMLElement, keys:ReadonlySet):HTMLElement[] { + return orderedItemElements(root).filter((item) => { + const identity = itemIdentity(item); + return identity !== null && keys.has(selectionKey(identity)); + }); +} + export function liveOrderableItems(root:HTMLElement):SelectionItem[] { return orderedItemElements(root) .filter(isOrderableItem) diff --git a/modules/backlogs/app/contracts/backlogs/work_packages/batch_move_params_contract.rb b/modules/backlogs/app/contracts/backlogs/work_packages/batch_move_params_contract.rb new file mode 100644 index 000000000000..5a714e049f6d --- /dev/null +++ b/modules/backlogs/app/contracts/backlogs/work_packages/batch_move_params_contract.rb @@ -0,0 +1,83 @@ +# 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. +#++ + +module Backlogs + module WorkPackages + class BatchMoveParamsContract < ::ParamsContract + validate :ids_distinct_and_present + validate :batch_within_cap + validate :target_resolvable + validate :predecessor_well_formed + + # BaseContract#errors returns model.errors whenever the model responds + # to it, and project does: without this override, validating the + # contract would clear and repopulate the live project's own error bag. + def errors + @errors ||= ActiveModel::Errors.new(self) + end + + private + + def ids + Array(params[:ids]).map(&:to_s) + end + + def ids_distinct_and_present + return unless ids.empty? || ids.any?(&:blank?) || ids.uniq.length != ids.length + + errors.add(:base, I18n.t("backlogs.work_packages.move_collection.invalid_ids")) + end + + def batch_within_cap + return if ids.length <= BatchUpdateService::MAX_BATCH_SIZE + + errors.add(:base, I18n.t("backlogs.work_packages.move_collection.too_many_work_packages", + max: BatchUpdateService::MAX_BATCH_SIZE)) + end + + def target_resolvable + return if Backlogs::Target.from_list(params[:list_type], params[:list_id]) + + errors.add(:base, I18n.t("backlogs.work_packages.update_service.invalid_target_type")) + end + + # A nonblank prev_id must be a pure integer id, or Active Record would + # integer-cast a digit-prefixed string; and a member cannot anchor its + # own batch. + def predecessor_well_formed + prev_id = params[:prev_id] + return if prev_id.nil? || prev_id.to_s.blank? + return if prev_id.to_s.match?(/\A\d+\z/) && ids.exclude?(prev_id.to_s) + + errors.add(:base, I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor")) + end + end + end +end diff --git a/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb b/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb index 72f343c20a42..385097cff9ac 100644 --- a/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb +++ b/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb @@ -121,8 +121,94 @@ def move render_update_turbo_streams(call) end + def move_collection # rubocop:disable Metrics/AbcSize + contract = Backlogs::WorkPackages::BatchMoveParamsContract.new(@project, current_user, params: move_collection_params) + return render_move_collection_error(contract.errors.full_messages.join(" ")) unless contract.valid? + + work_packages = find_collection_work_packages(move_collection_params[:ids]) + return render_move_collection_error(t(".work_packages_not_found")) unless work_packages + + # Snapshot before the call: move_after reloads mid-method and destroys + # dirty tracking, exactly as the member action's comment explains. + source_targets = work_packages.to_h { |wp| [wp.id, Backlogs::Target.for_work_package(wp)] } + + call = ::Backlogs::WorkPackages::BatchUpdateService + .new(user: current_user, work_packages:) + .call(**collection_move_service_params) + + if optimistic_same_list_batch_move?(call, source_targets) + dispatch_event_via_turbo_stream( + WORK_PACKAGE_MOVED_EVENT, + detail: { work_package_ids: call.result.map(&:id) } + ) + return respond_with_turbo_streams(status: call) + end + + render_update_collection_turbo_streams(call) + end + private + def render_update_collection_turbo_streams(call) + if call.success? + reload_frame_via_turbo_stream("backlogs_container") + dispatch_event_via_turbo_stream( + WORK_PACKAGE_MOVED_EVENT, + detail: { work_package_ids: call.result.map(&:id) } + ) + render_invisible_after_move_batch_flash(call.result) + else + render_error_flash_message_via_turbo_stream( + message: I18n.t(:notice_unsuccessful_update_with_reason, reason: batch_failure_reason(call)) + ) + end + + respond_with_turbo_streams(status: call) + end + + # A member failure is reported with the member: the batch's own message + # is empty then, and the flash would not say which work package refused. + def batch_failure_reason(call) + failed = call.dependent_results.find(&:failure?) + return call.message unless failed + + work_package = failed.result + return failed.message unless work_package.is_a?(WorkPackage) + + t("backlogs.work_packages.move_collection.member_failed", + work_package: work_package.to_fs(:caption), reason: failed.message) + end + + def optimistic_same_list_batch_move?(call, source_targets) + return false unless optimistic_move? && call.success? && call.result.any? + + destination = Backlogs::Target.for_work_package(call.result.first) + call.result.all? { |wp| source_targets[wp.id] == destination } && + requested_block_honored?(call.result) + end + + # The batch form of requested_anchor_honored?: the first member sits where + # the request anchored it and every further member directly below its + # predecessor, which is the client's optimistic block. + def requested_block_honored?(results) # rubocop:disable Metrics/AbcSize + return false unless move_collection_params.key?(:prev_id) + + # One anchor query; the rest of the block is checked in memory. + # BatchUpdateService reloads every moved member before returning and + # the members share one target scope, so adjacent positions prove + # adjacency. A gap from elsewhere fails this check falsely, degrading + # to the full frame reload rather than skipping a needed one. + prev_id = move_collection_params[:prev_id].presence + first = results.first + anchor_honored = prev_id ? first.higher_item&.id == prev_id.to_i : first.higher_item.nil? + + anchor_honored && results.each_cons(2).all? { |above, below| below.position == above.position + 1 } + end + + def collection_move_service_params + move_collection_params.to_h.symbolize_keys.except(:ids).compact + end + def render_update_turbo_streams(call) if call.success? reload_frame_via_turbo_stream("backlogs_container") @@ -149,10 +235,24 @@ def render_invisible_after_move_flash(work_package) return unless work_package_invisible_after_move?(work_package) render_flash_message_via_turbo_stream( - message: I18n.t(:notice_work_package_invisible_after_move, backlog: target_list_name(work_package)) + message: I18n.t(:notice_work_package_invisible_after_move, count: 1, backlog: target_list_name(work_package)) ) end + # A member's own type or status can hide it independently of its + # list-mates, so the first member cannot answer this for the batch. + def render_invisible_after_move_batch_flash(results) + invisible = results.select { |wp| work_package_invisible_after_move?(wp) } + return if invisible.empty? + + render_flash_message_via_turbo_stream(message: invisible_after_move_batch_message(invisible)) + end + + def invisible_after_move_batch_message(invisible) + I18n.t(:notice_work_package_invisible_after_move, + count: invisible.size, backlog: target_list_name(invisible.first)) + end + # A dialog move (never flagged optimistic) is announced by the server; the # optimistic drag and menu paths announce client-side, and the flash # covers moves whose result is no longer visible. Persisted no-ops @@ -205,6 +305,32 @@ def load_work_package @work_package = @work_packages.find(params.expect(:id)) end + # Every submitted id must resolve to a distinct, visible work package of + # this project, in the submitted order: silently dropping a member would + # break the client's optimistic block. Nil when any id does not resolve. + def find_collection_work_packages(ids) + found = WorkPackage.visible.where(project: @project, id: ids).index_by { |wp| wp.id.to_s } + ordered = ids.map { |id| found[id.to_s] } + + ordered.any?(&:nil?) ? nil : ordered + end + + def render_move_collection_error(reason) + render_error_flash_message_via_turbo_stream( + message: I18n.t(:notice_unsuccessful_update_with_reason, reason:) + ) + respond_with_turbo_streams(status: :unprocessable_entity) + end + + # params.expect guarantees a present, non-empty array of scalar ids; the + # optional placement and target fields go through permit instead. + def move_collection_params + @move_collection_params ||= begin + ids = params.expect(ids: []) + params.permit(:prev_id, :list_type, :list_id).merge(ids:) + end + end + def move_path move_project_backlogs_work_package_path(@project, @work_package, backlog_filter_params) end diff --git a/modules/backlogs/app/services/backlogs/work_packages/batch_update_service.rb b/modules/backlogs/app/services/backlogs/work_packages/batch_update_service.rb new file mode 100644 index 000000000000..f3de96c3787f --- /dev/null +++ b/modules/backlogs/app/services/backlogs/work_packages/batch_update_service.rb @@ -0,0 +1,324 @@ +# 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. +#++ + +# Moves an ordered batch of work packages into one Backlogs list as a single +# atomic operation. Each member is moved through the existing single-work- +# package UpdateService, always inserting after the previously moved member, +# so the batch lands as one contiguous block in input order. +# +# Call it outside a transaction. The advisory locks it takes are +# transaction-scoped (`pg_advisory_xact_lock`), and Postgres binds those to +# the top-level transaction, not to the savepoint below: a caller that wraps +# this in its own transaction would hold every container's lock until that +# outer transaction ends, serializing unrelated moves behind it. +class Backlogs::WorkPackages::BatchUpdateService + # Enforced by the controller before it loads the batch and by the service + # itself for every other caller. + MAX_BATCH_SIZE = 500 + + attr_reader :user, :work_packages + + def initialize(user:, work_packages:) + @user = user + @work_packages = work_packages + end + + # :explicit (nonblank prev_id), :top (blank prev_id, no anchor) or :append + # (absent prev_id → the last non-batch member of the target, read under the + # placement lock so it joins the lock set and can be revalidated); a nil + # anchor means an empty target. + Placement = Data.define(:mode, :anchor) do + def initial_prev_id = anchor ? anchor.id.to_s : "" + end + + def call(list_type: nil, list_id: nil, prev_id: nil) # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity + return empty_batch_failure if work_packages.empty? + + contract = Backlogs::WorkPackages::BatchMoveParamsContract.new( + work_packages.first.project, + user, + params: { ids: work_packages.map(&:id), list_type:, list_id:, prev_id: } + ) + return ServiceResult.failure(errors: contract.errors) unless contract.valid? + + target = Backlogs::Target.from_list(list_type, list_id) + return mixed_projects_failure unless work_packages.map(&:project_id).uniq.one? + + # Captured once: placement resolution, anchor revalidation and the cohort + # check must agree on one project, not re-derive it from a member a + # concurrent move could have relocated. + @batch_project_id = work_packages.first.project_id + @batch_project = work_packages.first.project + @batch_source_targets = work_packages.to_h do |work_package| + [work_package.id, Backlogs::Target.for_work_package(work_package)] + end + + call = nil + # Its own savepoint: joined into an enclosing transaction, the rollback + # below would be swallowed and half the batch would commit. + WorkPackage.transaction(requires_new: true) do + call = move_batch(target, prev_id, list_type:, list_id:) + raise ActiveRecord::Rollback if call.failure? + end + call + rescue StandardError => e + # An operational exception from a later member must not escape as a 500 + # once the rollback has already happened. The message is unlocalized + # adapter detail, so it is logged rather than shown in the flash. + Rails.logger.error { "Backlogs batch move failed: #{e.class}: #{e.message}" } + ServiceResult.failure(message: I18n.t("backlogs.work_packages.batch_update_service.unexpected_failure")) + end + + private + + def move_batch(target, prev_id, list_type:, list_id:) + destination = raw_destination(target) + acquire_ordered_locks(ordered_lifecycle_records(destination)) + acquire_placement_serialization_lock(target, prev_id) + placement = resolve_placement(target, prev_id) + return placement if placement.is_a?(ServiceResult) + + acquire_ordered_locks(lock_entries(placement.anchor)) + return stale_batch_failure unless cohort_intact? + + lock_destination_row!(destination) + return unavailable_target_failure unless target_available?(target) + + anchor_failure = revalidate_anchor(placement, target) + return anchor_failure if anchor_failure + + move_members(placement, list_type:, list_id:) + end + + def move_members(placement, list_type:, list_id:) # rubocop:disable Metrics/AbcSize + call = ServiceResult.success(result: []) + current_prev_id = placement.initial_prev_id + + work_packages.each do |work_package| + # An earlier member's move_after shifts other rows' positions through + # update_all without touching their loaded Ruby objects, and + # remove_from_list uses the in-memory position as the threshold it + # decrements from — a stale read corrupts the positions it writes. + work_package.reload + inner = Backlogs::WorkPackages::UpdateService + .new(user:, work_package:) + .call(list_type:, list_id:, prev_id: current_prev_id) + call.add_dependent!(inner) + return call if inner.failure? + + call.result << inner.result + current_prev_id = inner.result.id.to_s + end + + # WorkPackage#call_after_update_hook builds its context from `self`, so + # without this a hook consumer observes the interim position a later + # member's update_all left behind. Still inside the outer transaction, + # so the hooks fire against final rows. + call.result.each(&:reload) + call + end + + # Sprint lifecycle services serialize on the Sprint model mutex before + # enumerating and moving their work packages, so a batch has to join every + # source lifecycle as well as the target's: locking only the target lets a + # batch move a member out after FinishService has enumerated it. Sorted by + # class and id, so two batches with inverse source/target pairs cannot + # deadlock. + def ordered_lifecycle_records(destination) + source_records = @batch_source_targets.values.uniq.filter_map { |target| raw_destination(target) } + + (source_records + [destination]) + .compact + .uniq { |record| lifecycle_lock_identity(record) } + .sort_by { |record| lifecycle_lock_identity(record) } + end + + def lifecycle_lock_identity(record) + [record.class.name, record.id] + end + + # Unanchored placement depends on target-relative state no row can carry: + # in an empty Inbox two batches would otherwise both commit positions 1..N. + # Explicit placement needs none of this, being serialized by its anchor's + # own lock. + def acquire_placement_serialization_lock(target, prev_id) + return if prev_id.present? + + suffix = ["backlogs_batch_update_destination", target.list_type, target.list_id].compact.join("_") + # rubocop:disable Lint/EmptyBlock -- the lock outlives the block; see acquire_ordered_locks + OpenProject::Mutex.with_advisory_lock_transaction(batch_project, suffix) {} + # rubocop:enable Lint/EmptyBlock + end + + # Ascending id order, so two overlapping batches request the same lock + # sequence and neither waits on the other while holding one. + def lock_entries(anchor) + (work_packages + [anchor]).compact.uniq.sort_by(&:id) + end + + # Transaction-scoped locks outlive their block until the enclosing + # transaction ends, so each one is taken with an empty block in one flat + # sequence. The gem's per-thread lock stack forgets the lock at block exit, + # so the inner services re-request theirs; Postgres grants a lock the + # session already holds without waiting. + def acquire_ordered_locks(entries) + entries.each do |entry| + # rubocop:disable Lint/EmptyBlock -- the lock outlives the block; see the comment above + OpenProject::Mutex.with_advisory_lock_transaction(entry) {} + # rubocop:enable Lint/EmptyBlock + end + end + + # Unscoped by policy so completion, deletion and reassignment all resolve + # to the same advisory identity. + def raw_destination(target) + case target + in Backlogs::Target::SprintId + Sprint.find_by(id: target.list_id) + in Backlogs::Target::BucketId + BacklogBucket.find_by(id: target.list_id) + in Backlogs::Target::InboxId + nil + end + end + + # lock! reloads under FOR UPDATE, so a concurrent completion, deletion or + # reassignment commits before the availability query runs. Inbox has no + # destination row; its placement is serialized by the advisory lock alone. + def lock_destination_row!(destination) + destination&.lock! + rescue ActiveRecord::RecordNotFound + nil + end + + # The anchor is scoped to the batch project because the acts_as_list scope + # includes project_id: in a shared sprint another project's work package + # would pass a container-only comparison, yet be unresolvable for + # move_after, which then silently inserts at the top. + def resolve_placement(target, prev_id) + return Placement.new(mode: :append, anchor: last_non_batch_member(target)) if prev_id.nil? + return Placement.new(mode: :top, anchor: nil) if prev_id.to_s.blank? + + anchor = WorkPackage.visible(user).where(project_id: batch_project_id).find_by(id: prev_id) + anchor ? Placement.new(mode: :explicit, anchor:) : stale_predecessor_failure + end + + # A member could have been moved to another project, or out of its source + # container by a lifecycle service, or deleted, between the controller + # loading the batch and the locks being taken. A hopped member would move + # in a different acts_as_list scope, splitting the block, and the chained + # prev_id would then cross scopes into move_after's silent insert-at-top. + def cohort_intact? + current = WorkPackage + .visible(user) + .where(id: work_packages.map(&:id), project_id: batch_project_id) + .select(:id, :sprint_id, :backlog_bucket_id) + + current.size == work_packages.size && current.all? do |work_package| + Backlogs::Target.for_work_package(work_package) == batch_source_targets.fetch(work_package.id) + end + end + + # Under lock the anchor must still be what placement resolution saw: same + # project, same list, and for append still the last non-batch member. A + # concurrently moved anchor would otherwise fall through to move_after's + # silent insert-at-top. + def revalidate_anchor(placement, target) # rubocop:disable Metrics/AbcSize + anchor = placement.anchor + return if anchor.nil? + + anchor.reload + unless anchor.project_id == batch_project_id && Backlogs::Target.for_work_package(anchor) == target + return stale_predecessor_failure + end + + stale_predecessor_failure if placement.mode == :append && last_non_batch_member(target)&.id != anchor.id + rescue ActiveRecord::RecordNotFound + stale_predecessor_failure + end + + # The contract only revalidates a sprint or bucket target when the + # corresponding column changes, so a same-list reorder never triggers it + # and a sprint completed after the page loaded stays an accepted + # destination. Mirrors the contract's own assignable_sprints and + # backlog_bucket_belongs_to_project checks for every placement mode alike. + def target_available?(target) + case target + in Backlogs::Target::SprintId + Sprint.assignable(project: batch_project, user:).exists?(id: target.list_id) + in Backlogs::Target::BucketId + BacklogBucket.for_project(batch_project).exists?(id: target.list_id) + in Backlogs::Target::InboxId + true + end + end + + def last_non_batch_member(target) + WorkPackage + .visible(user) + .where(project_id: batch_project_id, **target.attributes) + .where.not(id: work_packages.map(&:id)) + .where.not(position: nil) + .order(:position) + .last + end + + def batch_project_id + @batch_project_id + end + + def batch_project + @batch_project + end + + def batch_source_targets + @batch_source_targets + end + + def empty_batch_failure + ServiceResult.failure(message: I18n.t("backlogs.work_packages.move_collection.invalid_ids")) + end + + def stale_predecessor_failure + ServiceResult.failure(message: I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor")) + end + + def stale_batch_failure + ServiceResult.failure(message: I18n.t("backlogs.work_packages.batch_update_service.stale_batch")) + end + + def unavailable_target_failure + ServiceResult.failure(message: I18n.t("backlogs.work_packages.batch_update_service.unavailable_target")) + end + + def mixed_projects_failure + ServiceResult.failure(message: I18n.t("backlogs.work_packages.batch_update_service.mixed_projects")) + end +end diff --git a/modules/backlogs/app/views/backlogs/backlog/show.html.erb b/modules/backlogs/app/views/backlogs/backlog/show.html.erb index 961b551c20d6..a39f9f54998c 100644 --- a/modules/backlogs/app/views/backlogs/backlog/show.html.erb +++ b/modules/backlogs/app/views/backlogs/backlog/show.html.erb @@ -53,6 +53,9 @@ See COPYRIGHT and LICENSE files for more details. sortable_lists_optimistic_value: true, action: "#{Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT}@document->backlogs--split-view-sync#onWorkPackageMoved", sortable_lists_move_url_template_value: backlogs_move_url_template(@project), + sortable_lists_collection_move_url_value: move_project_backlogs_work_packages_path(@project), + sortable_lists_max_batch_size_value: Backlogs::WorkPackages::BatchUpdateService::MAX_BATCH_SIZE, + sortable_lists_move_announcement_scope_value: "js.backlogs.announcements", sortable_lists_selection_enabled_value: batch_selection_allowed?(@project), sortable_lists_announcement_scope_value: "js.backlogs.selection", sortable_lists_selection_description_id_value: diff --git a/modules/backlogs/config/locales/en.yml b/modules/backlogs/config/locales/en.yml index 003a8ff89a5b..3dce5e68e083 100644 --- a/modules/backlogs/config/locales/en.yml +++ b/modules/backlogs/config/locales/en.yml @@ -287,8 +287,19 @@ en: add_existing_dialog: invalid_target: "The target you are trying to add to is invalid." target_not_found: "The sprint or backlog you are trying to add to was not found." + batch_update_service: + mixed_projects: "All work packages of a batch must belong to the same project." + stale_batch: "At least one work package changed while the move was being prepared. Please check the current positions and try again." + stale_predecessor: "The work package to insert after has been moved elsewhere. Please check the current positions and try again." + unavailable_target: "The destination list is no longer available. Please reload the page and try again." + unexpected_failure: "The work packages could not be moved. Please try again." move: moved_announcement: "%{label} moved to %{list}, position %{position} of %{total}" + move_collection: + invalid_ids: "The list of work packages to move is invalid." + member_failed: "%{work_package}: %{reason}" + too_many_work_packages: "No more than %{max} work packages can be moved at once." + work_packages_not_found: "At least one work package could not be found in this project." update_service: invalid_target_type: "list_type must be one of: backlog_bucket with a list_id, sprint with a list_id, or inbox without a list_id." missing_target: "list_type or list_id must be present." @@ -327,8 +338,11 @@ en: notice_unsuccessful_finish_with_reason: "The sprint could not be completed: %{reason}" notice_unsuccessful_start: "The sprint could not be started." notice_unsuccessful_start_with_reason: "The sprint could not be started: %{reason}" - notice_work_package_invisible_after_move: > - The work package was moved to %{backlog} but is not visible because its type or status is excluded from the backlog. + notice_work_package_invisible_after_move: + one: > + The work package was moved to %{backlog} but is not visible because its type or status is excluded from the backlog. + other: > + %{count} work packages were moved to %{backlog} but are not visible because their type or status is excluded from the backlog. permission_create_sprints: "Create sprints" permission_manage_sprint_items: "Manage sprint items" permission_select_backlog_types_and_statuses: "Select backlog types and statuses" diff --git a/modules/backlogs/config/locales/js-en.yml b/modules/backlogs/config/locales/js-en.yml index a07d340d8d2d..48973394f3b6 100644 --- a/modules/backlogs/config/locales/js-en.yml +++ b/modules/backlogs/config/locales/js-en.yml @@ -30,6 +30,25 @@ en: js: backlogs: + announcements: + batch_too_large: "Cannot move %{count} work packages at once. Select no more than %{max}." + fallback_item_label: "Work package" + fallback_list_name: "another list" + move_failed_check_position: "Move failed. Check the work package's current position." + # The batch keys are plural hashes so translators can add the plural + # categories their locale needs; the one: branch is unreachable (a + # one-card move announces through the singular keys). + move_failed_check_positions_batch: + other: "Move failed. Check the work packages' current positions." + move_failed_rolled_back: "Move failed. %{label} returned to its previous position." + move_failed_rolled_back_batch: + other: "Move failed. %{count} work packages returned to their previous positions." + moved: "%{label} moved to position %{position} of %{total}" + moved_batch: + other: "%{count} work packages moved to positions %{first} through %{last} of %{total}" + moved_batch_to_list: + other: "%{count} work packages moved to %{list}, positions %{first} through %{last} of %{total}" + moved_to_list: "%{label} moved to %{list}, position %{position} of %{total}" selection: card_state: "Selected" cleared: "Selection cleared." diff --git a/modules/backlogs/config/routes.rb b/modules/backlogs/config/routes.rb index 110453c37eff..47c094093502 100644 --- a/modules/backlogs/config/routes.rb +++ b/modules/backlogs/config/routes.rb @@ -99,6 +99,7 @@ collection do get :add_existing_dialog post :add_existing + put :move, action: :move_collection end member do diff --git a/modules/backlogs/lib/open_project/backlogs/engine.rb b/modules/backlogs/lib/open_project/backlogs/engine.rb index c9512df6e07b..439d8ebfecc2 100644 --- a/modules/backlogs/lib/open_project/backlogs/engine.rb +++ b/modules/backlogs/lib/open_project/backlogs/engine.rb @@ -85,8 +85,14 @@ def self.settings dependencies: %i[view_sprints manage_board_views manage_sprint_items] permission :manage_sprint_items, - { "backlogs/work_packages": %i[move move_to_sprint_dialog move_to_bucket_dialog add_existing_dialog - add_existing] }, + { "backlogs/work_packages": %i[ + move + move_collection + move_to_sprint_dialog + move_to_bucket_dialog + add_existing_dialog + add_existing + ] }, permissible_on: :project, require: :member, dependencies: %i[view_sprints edit_work_packages] diff --git a/modules/backlogs/spec/contracts/backlogs/work_packages/batch_move_params_contract_spec.rb b/modules/backlogs/spec/contracts/backlogs/work_packages/batch_move_params_contract_spec.rb new file mode 100644 index 000000000000..e801379d9487 --- /dev/null +++ b/modules/backlogs/spec/contracts/backlogs/work_packages/batch_move_params_contract_spec.rb @@ -0,0 +1,98 @@ +# 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" + +RSpec.describe Backlogs::WorkPackages::BatchMoveParamsContract do + shared_let(:project) { create(:project) } + shared_let(:user) { create(:user) } + + def contract(params) + described_class.new(project, user, params:) + end + + it "accepts distinct ids, a resolvable target and a numeric predecessor" do + expect(contract(ids: %w[1 2], list_type: "sprint", list_id: "3", prev_id: "4")).to be_valid + end + + it "accepts a blank and an absent predecessor" do + expect(contract(ids: %w[1], list_type: "inbox", prev_id: "")).to be_valid + expect(contract(ids: %w[1], list_type: "inbox")).to be_valid + end + + it "rejects blank or duplicate ids" do + expect(contract(ids: ["1", ""], list_type: "inbox")).not_to be_valid + + duplicate = contract(ids: %w[1 1], list_type: "inbox") + expect(duplicate).not_to be_valid + expect(duplicate.errors.full_messages) + .to include(I18n.t("backlogs.work_packages.move_collection.invalid_ids")) + end + + it "keeps its errors off the project" do + invalid = contract(ids: %w[1 1], list_type: "inbox") + expect(invalid).not_to be_valid + + expect(project.errors).to be_empty + expect(invalid.errors.full_messages).not_to be_empty + end + + it "rejects an empty id list" do + expect(contract(ids: [], list_type: "inbox")).not_to be_valid + end + + it "rejects more ids than the cap" do + ids = Array.new(Backlogs::WorkPackages::BatchUpdateService::MAX_BATCH_SIZE + 1) { |i| (i + 1).to_s } + + oversized = contract(ids:, list_type: "inbox") + expect(oversized).not_to be_valid + expect(oversized.errors.full_messages) + .to include(I18n.t("backlogs.work_packages.move_collection.too_many_work_packages", + max: Backlogs::WorkPackages::BatchUpdateService::MAX_BATCH_SIZE)) + end + + it "rejects an unresolvable target" do + unresolvable = contract(ids: %w[1], list_type: "sprint") + expect(unresolvable).not_to be_valid + expect(unresolvable.errors.full_messages) + .to include(I18n.t("backlogs.work_packages.update_service.invalid_target_type")) + end + + it "rejects a malformed predecessor instead of integer-casting it" do + malformed = contract(ids: %w[1], list_type: "inbox", prev_id: "12abc") + expect(malformed).not_to be_valid + expect(malformed.errors.full_messages) + .to include(I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor")) + end + + it "rejects a predecessor that is part of the batch" do + expect(contract(ids: %w[1 2], list_type: "inbox", prev_id: "2")).not_to be_valid + end +end diff --git a/modules/backlogs/spec/features/work_packages/batch_move_spec.rb b/modules/backlogs/spec/features/work_packages/batch_move_spec.rb new file mode 100644 index 000000000000..41ef8a8e25b4 --- /dev/null +++ b/modules/backlogs/spec/features/work_packages/batch_move_spec.rb @@ -0,0 +1,250 @@ +# 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" + +# Selenium, not Cuprite: batch selection is driven through modifier clicks, +# and the DnD engine needs real browser drag events. +RSpec.describe "Backlogs batch move", :js, :selenium, :settings_reset do + include RSpec::Wait + + 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!(:sprint) { create(:sprint, project:) } + let!(:sprint_wp1) { create(:work_package, sprint:, position: 1, type:, project:) } + let!(:sprint_wp2) { create(:work_package, sprint:, position: 2, type:, project:) } + let!(:sprint_wp3) { create(:work_package, sprint:, position: 3, type:, project:) } + let!(:bucket) { create(:backlog_bucket, project:, name: "Backlog bucket") } + let!(:bucket_wp1) { create(:work_package, backlog_bucket: bucket, position: 1, type:, project:) } + let!(:bucket_wp2) { create(:work_package, backlog_bucket: bucket, position: 2, type:, project:) } + + let(:backlogs_page) { Pages::Backlog.new(project) } + + current_user do + create(:user, member_with_roles: { project => manage_sprint_items_role }) + end + + before do + backlogs_page.visit! + end + + it "moves a sparse cross-list batch as one ordered block and clears the selection" do + backlogs_page.toggle_card(bucket_wp2) + backlogs_page.toggle_card(sprint_wp3) + + backlogs_page.drag_work_package(bucket_wp2, after: sprint_wp1) + + backlogs_page.expect_sprint_items_in_order(sprint, items: [sprint_wp1, bucket_wp2, sprint_wp3, sprint_wp2]) + expect(backlogs_page.selected_card_ids).to be_empty + # Poll persistence, never trust the DOM alone: + wait_for { sprint.work_packages_for(project).pluck(:id) } + .to eq [sprint_wp1.id, bucket_wp2.id, sprint_wp3.id, sprint_wp2.id] + end + + it "moves an unselected card alone, replacing the batch" do + backlogs_page.toggle_card(sprint_wp1) + backlogs_page.toggle_card(sprint_wp2) + + backlogs_page.drag_work_package(sprint_wp3, after: bucket_wp1) + + wait_for { WorkPackage.where(backlog_bucket: bucket).order(:position).pluck(:id) } + .to eq [bucket_wp1.id, sprint_wp3.id, bucket_wp2.id] + backlogs_page.expect_sprint_items_in_order(sprint, items: [sprint_wp1, sprint_wp2]) + # The drag collapsed the batch onto the moved card, and the successful + # move then cleared the selection — nothing stays selected. + expect(backlogs_page.selected_card_ids).to be_empty + end + + it "restores every row and keeps the selection when the server rejects the batch", + with_ee: %i[readonly_work_packages] do + backlogs_page.toggle_card(sprint_wp2) + backlogs_page.toggle_card(sprint_wp3) + + # Invalidate one member server-side after the page rendered it movable: + # a readonly status blocks the position write, so the batch 422s. + readonly_status = create(:status, is_readonly: true) + sprint_wp3.update_columns(status_id: readonly_status.id) + + # Not drag_work_package: it derives frame_reload: true from cross-list + # identity, and a rejected move never reloads the frame. + backlogs_page.drag_work_package_expecting_failure(sprint_wp2, after: bucket_wp1) + + # Rows restored, batch preserved for retry: + backlogs_page.expect_sprint_items_in_order(sprint, items: [sprint_wp1, sprint_wp2, sprint_wp3]) + expect(backlogs_page.selected_card_ids) + .to contain_exactly(sprint_wp2.id.to_s, sprint_wp3.id.to_s) + wait_for { sprint.work_packages_for(project).pluck(:id) } + .to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "keeps a same-list batch reorder without reloading the frame" do + backlogs_page.toggle_card(sprint_wp1) + backlogs_page.toggle_card(sprint_wp2) + + # The order assertion below cannot tell an optimistic client-side move + # from a reload that lands inside the wait window; the probe only flips + # if `#backlogs_container` actually reloads. + backlogs_page.install_backlogs_container_reload_probe + + backlogs_page.drag_work_package(sprint_wp1, after: sprint_wp3) + + backlogs_page.expect_sprint_items_in_order(sprint, items: [sprint_wp3, sprint_wp1, sprint_wp2]) + backlogs_page.expect_backlogs_container_not_reloaded + wait_for { sprint.work_packages_for(project).pluck(:id) } + .to eq [sprint_wp3.id, sprint_wp1.id, sprint_wp2.id] + end + + it "moves a batch into an empty list" do + empty_sprint = create(:sprint, project:, name: "Empty sprint") + backlogs_page.visit! + backlogs_page.toggle_card(sprint_wp1) + backlogs_page.toggle_card(sprint_wp3) + + backlogs_page.drag_work_package(sprint_wp1, into: empty_sprint) + + backlogs_page.expect_sprint_items_in_order(empty_sprint, items: [sprint_wp1, sprint_wp3]) + wait_for { empty_sprint.work_packages_for(project).pluck(:id) }.to eq [sprint_wp1.id, sprint_wp3.id] + end + + it "moves a batch to the top of a list" do + backlogs_page.toggle_card(bucket_wp1) + backlogs_page.toggle_card(bucket_wp2) + + backlogs_page.drag_work_package(bucket_wp1, before: sprint_wp1) + + backlogs_page.expect_sprint_items_in_order(sprint, items: [bucket_wp1, bucket_wp2, sprint_wp1, sprint_wp2, sprint_wp3]) + wait_for { sprint.work_packages_for(project).pluck(:id) } + .to eq [bucket_wp1.id, bucket_wp2.id, sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "moves every card of a list selected with Ctrl/Cmd+A" do + backlogs_page.send_work_package_card_keys(sprint_wp1, [backlogs_page.multi_select_modifier, "a"]) + expect(backlogs_page.selected_card_ids).to match_array([sprint_wp1, sprint_wp2, sprint_wp3].map { it.id.to_s }) + + backlogs_page.drag_work_package(sprint_wp2, after: bucket_wp1) + + backlogs_page.expect_bucket_items_in_order(bucket, items: [bucket_wp1, sprint_wp1, sprint_wp2, sprint_wp3, bucket_wp2]) + wait_for { WorkPackage.where(backlog_bucket: bucket).order(:position).pluck(:id) } + .to eq [bucket_wp1.id, sprint_wp1.id, sprint_wp2.id, sprint_wp3.id, bucket_wp2.id] + expect(backlogs_page.selected_card_ids).to be_empty + end + + # TRUNCATE_MIDDLE stubbed to 2 makes the tail 1 card, so only + # inbox_wps[0..1] and inbox_wps[4] render; inbox_wps[2..3] sit behind the + # fold. A drop before the first visible row past the marker (inbox_wps[4]) + # anchors on the last hidden card the marker names (inbox_wps[3]), landing + # right after it. + context "with a truncated inbox" do + let!(:inbox_wps) { create_list(:work_package, 5, project:, type:) } + + before do + stub_const("Backlogs::InboxComponent::TRUNCATE_MIDDLE", 2) + backlogs_page.visit! + end + + it "drops a batch behind the fold" do + backlogs_page.expect_inbox_show_more + backlogs_page.toggle_card(sprint_wp1) + backlogs_page.toggle_card(sprint_wp2) + + backlogs_page.drag_work_package(sprint_wp1, before: inbox_wps[4]) + + # The move lands mid-fold: the truncated view still shows only + # inbox_wps[0], inbox_wps[1] and inbox_wps[4], unchanged from before the + # drag. Expanding is the only way to observe the new order in the DOM. + backlogs_page.click_inbox_show_more + backlogs_page.expect_inbox_items_in_order(items: [inbox_wps[0], inbox_wps[1], inbox_wps[2], inbox_wps[3], sprint_wp1, + sprint_wp2, inbox_wps[4]]) + wait_for { WorkPackage.where(project:, sprint_id: nil, backlog_bucket_id: nil).order(:position).pluck(:id) } + .to eq [inbox_wps[0].id, inbox_wps[1].id, inbox_wps[2].id, inbox_wps[3].id, + sprint_wp1.id, sprint_wp2.id, inbox_wps[4].id] + end + end + + describe "with a batch-mate confined to another list", with_ee: %i[readonly_work_packages] do + let!(:readonly_status) { create(:status, :readonly) } + let!(:other_sprint) { create(:sprint, project:) } + let!(:confined_wp) do + create(:work_package, sprint: other_sprint, position: 1, type:, project:, status: readonly_status) + end + let!(:other_sprint_wp) { create(:work_package, sprint: other_sprint, position: 2, type:, project:) } + + # The outer visit renders the page before this group's own records exist. + before do + backlogs_page.visit! + end + + it "refuses a drop in a list the confined member cannot enter" do + backlogs_page.expect_work_package_confined(confined_wp) + backlogs_page.toggle_card(confined_wp) + backlogs_page.toggle_card(sprint_wp1) + + backlogs_page.drag_work_package_without_move(sprint_wp1, into: sprint) + + backlogs_page.expect_sprint_items_in_order(sprint, items: [sprint_wp1, sprint_wp2, sprint_wp3]) + expect(confined_wp.reload.sprint_id).to eq(other_sprint.id) + end + + # The mirror of the refusal above: the list the confined member already + # occupies is a destination the whole batch can reach, and the menus have + # always offered it. + it "moves the batch into the list its confined member occupies" do + backlogs_page.toggle_card(confined_wp) + backlogs_page.toggle_card(sprint_wp1) + + backlogs_page.drag_work_package(sprint_wp1, into: other_sprint) + + expect(backlogs_page.selected_card_ids).to be_empty + backlogs_page.expect_sprint_items_in_order(other_sprint, items: [sprint_wp1, confined_wp, other_sprint_wp]) + wait_for { other_sprint.work_packages_for(project).pluck(:id) } + .to eq [sprint_wp1.id, confined_wp.id, other_sprint_wp.id] + end + + it "refuses every drop while confined members sit in different lists" do + confined_in_sprint = create(:work_package, sprint:, position: 4, type:, project:, status: readonly_status) + backlogs_page.visit! + backlogs_page.toggle_card(confined_wp) + backlogs_page.toggle_card(confined_in_sprint) + + backlogs_page.drag_work_package_without_move(confined_in_sprint, into: sprint) + + expect(confined_in_sprint.reload.sprint_id).to eq(sprint.id) + expect(confined_wp.reload.sprint_id).to eq(other_sprint.id) + end + end +end diff --git a/modules/backlogs/spec/requests/backlogs/backlog_spec.rb b/modules/backlogs/spec/requests/backlogs/backlog_spec.rb index 3618bd98ca13..c25cb1be6cbe 100644 --- a/modules/backlogs/spec/requests/backlogs/backlog_spec.rb +++ b/modules/backlogs/spec/requests/backlogs/backlog_spec.rb @@ -78,6 +78,12 @@ expect(response.body).to include( %(data-sortable-lists-move-url-template-value="/projects/#{project.identifier}/backlogs/work_packages/{id}/move") ) + expect(response.body).to include( + %(data-sortable-lists-collection-move-url-value="/projects/#{project.identifier}/backlogs/work_packages/move") + ) + expect(response.body).to include( + 'data-sortable-lists-move-announcement-scope-value="js.backlogs.announcements"' + ) expect(response.body).to include( "data-sortable-lists-sortable-lists--list-outlet=" \ "\"#backlogs_container [data-controller~='sortable-lists--list']\"" diff --git a/modules/backlogs/spec/requests/work_packages/move_collection_spec.rb b/modules/backlogs/spec/requests/work_packages/move_collection_spec.rb new file mode 100644 index 000000000000..7e085fed013a --- /dev/null +++ b/modules/backlogs/spec/requests/work_packages/move_collection_spec.rb @@ -0,0 +1,289 @@ +# 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" + +RSpec.describe "Backlogs collection move", :skip_csrf, type: :rails_request do + shared_let(:type) { create(:type) } + shared_let(:project) do + create(:project, types: [type], enabled_module_names: %i[backlogs work_package_tracking]) + end + shared_let(:sprint) { create(:sprint, project:) } + shared_let(:bucket) { create(:backlog_bucket, project:) } + + let!(:bucket_wp1) { create(:work_package, backlog_bucket: bucket, position: 1, type:, project:) } + let!(:bucket_wp2) { create(:work_package, backlog_bucket: bucket, position: 2, type:, project:) } + let!(:sprint_wp1) { create(:work_package, sprint:, position: 1, type:, project:) } + + let(:permissions) { %i[view_work_packages edit_work_packages view_sprints manage_sprint_items] } + let(:user) { create(:user, member_with_permissions: { project => permissions }) } + + current_user { user } + + def move_collection(ids:, **params) + put move_project_backlogs_work_packages_path(project), + params: { ids:, **params }, + headers: { "Accept" => "text/vnd.turbo-stream.html" } + end + + context "without the manage_sprint_items permission" do + let(:permissions) { %i[view_work_packages edit_work_packages view_sprints] } + + it "forbids the request" do + move_collection(ids: [bucket_wp1.id], list_type: "sprint", list_id: sprint.id) + + expect(response).to have_http_status(:forbidden) + end + end + + shared_examples "rejects the whole request" do |status: :unprocessable_entity| + it "rejects without moving anything" do + expect { subject } + .not_to change { WorkPackage.order(:id).pluck(:id, :sprint_id, :backlog_bucket_id, :position) } + + expect(response).to have_http_status(status) + end + end + + describe "parameter validation" do + context "without an ids parameter" do + subject do + put move_project_backlogs_work_packages_path(project), + params: { list_type: "sprint", list_id: sprint.id }, + headers: { "Accept" => "text/vnd.turbo-stream.html" } + end + + it_behaves_like "rejects the whole request", status: :bad_request + end + + context "with a blank id" do + subject { move_collection(ids: [bucket_wp1.id, ""], list_type: "sprint", list_id: sprint.id) } + + it_behaves_like "rejects the whole request" + end + + context "with duplicate ids" do + subject { move_collection(ids: [bucket_wp1.id, bucket_wp1.id], list_type: "sprint", list_id: sprint.id) } + + it_behaves_like "rejects the whole request" + end + + context "with an id from another project" do + let!(:other_wp) { create(:work_package) } + + subject { move_collection(ids: [bucket_wp1.id, other_wp.id], list_type: "sprint", list_id: sprint.id) } + + it_behaves_like "rejects the whole request" + end + + context "with an id of a work package the user cannot see" do + let!(:invisible_wp) { create(:work_package, project: create(:project)) } + + subject { move_collection(ids: [invisible_wp.id], list_type: "sprint", list_id: sprint.id) } + + it_behaves_like "rejects the whole request" + end + + context "with more ids than the batch cap" do + # Synthetic ids: the cap must fire before any of them reach the + # database lookup. + subject do + move_collection(ids: Array.new(Backlogs::WorkPackages::BatchUpdateService::MAX_BATCH_SIZE + 1) { |i| (i + 1).to_s }, + list_type: "sprint", list_id: sprint.id) + end + + it_behaves_like "rejects the whole request" + + it "names the cap in the rejection" do + subject + + expect(response.body).to include( + ERB::Util.html_escape( + I18n.t("backlogs.work_packages.move_collection.too_many_work_packages", + max: Backlogs::WorkPackages::BatchUpdateService::MAX_BATCH_SIZE) + ) + ) + end + end + end + + describe "successful moves" do + context "with an optimistic same-list reorder whose block is honored" do + it "responds with the moved event only, no frame reload", :aggregate_failures do + move_collection(ids: [sprint_wp1.id], list_type: "sprint", list_id: sprint.id, + prev_id: "", optimistic: true) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("backlogs:work-package-moved") + expect(response.body).to include("work_package_ids") + expect(response.body).not_to include('target="backlogs_container"') + end + end + + context "with a cross-list batch" do + it "reloads the backlogs frame and emits the ordered batch event", :aggregate_failures do + move_collection(ids: [bucket_wp1.id, bucket_wp2.id], list_type: "sprint", list_id: sprint.id, + prev_id: sprint_wp1.id, optimistic: true) + + expect(response).to have_http_status(:ok) + expect(response.body).to include('target="backlogs_container"') + expect(response.body).to include("work_package_ids") + expect(sprint.work_packages_for(project).pluck(:id)) + .to eq [sprint_wp1.id, bucket_wp1.id, bucket_wp2.id] + end + end + + context "with an optimistic downward same-list block whose placement is honored" do + let!(:sprint_wp2) { create(:work_package, sprint:, position: 2, type:, project:) } + let!(:sprint_wp3) { create(:work_package, sprint:, position: 3, type:, project:) } + + it "responds with the moved event only, no frame reload", :aggregate_failures do + move_collection(ids: [sprint_wp1.id, sprint_wp2.id], list_type: "sprint", list_id: sprint.id, + prev_id: sprint_wp3.id, optimistic: true) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("work_package_ids") + expect(response.body).not_to include('target="backlogs_container"') + expect(sprint.work_packages_for(project).pluck(:id)).to eq [sprint_wp3.id, sprint_wp1.id, sprint_wp2.id] + expect(sprint.work_packages_for(project).pluck(:position)).to eq [1, 2, 3] + end + end + + context "when the persisted block diverges from the request" do + it "reloads instead of skipping" do + # An append has no prev_id for the anchor check to hold against, so + # the optimistic placement is unverifiable and must reconcile. + move_collection(ids: [sprint_wp1.id], list_type: "sprint", list_id: sprint.id, + optimistic: true) + + expect(response.body).to include('target="backlogs_container"') + end + end + end + + describe "failed moves" do + let!(:sprint_wp2) { create(:work_package, sprint:, position: 2, type:, project:) } + let!(:sprint_wp3) { create(:work_package, sprint:, position: 3, type:, project:) } + + it "streams an error flash and a 422 without moving anything" do + move_collection(ids: [sprint_wp2.id], list_type: "sprint", list_id: sprint.id, + prev_id: bucket_wp1.id, optimistic: true) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.body).to include( + ERB::Util.html_escape(I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor")) + ) + expect(sprint.work_packages_for(project).pluck(:id)) + .to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "streams an error flash and a 422 for a same-list reorder into a completed sprint" do + sprint.update!(status: "completed") + + move_collection(ids: [sprint_wp2.id], list_type: "sprint", list_id: sprint.id, + prev_id: sprint_wp3.id, optimistic: true) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.body).to include( + ERB::Util.html_escape(I18n.t("backlogs.work_packages.batch_update_service.unavailable_target")) + ) + expect(sprint.work_packages_for(project).pluck(:id)) + .to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "names the work package that refused the move", with_ee: %i[readonly_work_packages] do + readonly_status = create(:status, :readonly) + sprint_wp3.update_columns(status_id: readonly_status.id) + + move_collection(ids: [sprint_wp2.id, sprint_wp3.id], list_type: "backlog_bucket", list_id: bucket.id, + prev_id: "", optimistic: true) + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.body).to include(ERB::Util.html_escape(sprint_wp3.reload.to_fs(:caption))) + end + end + + describe "invisibility after move" do + # Moving into a sprint short-circuits the type/status exclusion check + # (work_package_invisible_after_move? only applies it to backlog + # destinations), so the target here is the bucket. + let(:excluded_type) { create(:type) } + + before do + project.project_types.create!(type: excluded_type) + project.backlog_excluded_types << excluded_type + end + + context "when only the second moved member becomes invisible" do + let!(:hidden_member) { create(:work_package, sprint:, position: 2, type: excluded_type, project:) } + + it "flashes the singular invisible-after-move notice" do + move_collection(ids: [sprint_wp1.id, hidden_member.id], list_type: "backlog_bucket", list_id: bucket.id, + prev_id: bucket_wp2.id) + + expect(response).to have_http_status(:ok) + expect(response.body).to include( + ERB::Util.html_escape(I18n.t(:notice_work_package_invisible_after_move, count: 1, backlog: bucket.name)) + ) + end + end + + context "when every moved member stays visible" do + let!(:visible_member) { create(:work_package, sprint:, position: 2, type:, project:) } + + it "does not flash" do + move_collection(ids: [sprint_wp1.id, visible_member.id], list_type: "backlog_bucket", list_id: bucket.id, + prev_id: bucket_wp2.id) + + expect(response).to have_http_status(:ok) + expect(response.body).not_to include( + ERB::Util.html_escape(I18n.t(:notice_work_package_invisible_after_move, count: 1, backlog: bucket.name)) + ) + end + end + + context "when more than one moved member becomes invisible" do + let!(:hidden_member1) { create(:work_package, sprint:, position: 2, type: excluded_type, project:) } + let!(:hidden_member2) { create(:work_package, sprint:, position: 3, type: excluded_type, project:) } + + it "flashes the plural invisible-after-move notice" do + move_collection(ids: [hidden_member1.id, hidden_member2.id], list_type: "backlog_bucket", list_id: bucket.id, + prev_id: bucket_wp2.id) + + expect(response).to have_http_status(:ok) + expect(response.body).to include( + ERB::Util.html_escape( + I18n.t(:notice_work_package_invisible_after_move, count: 2, backlog: bucket.name) + ) + ) + end + end + end +end diff --git a/modules/backlogs/spec/routing/backlogs/work_packages_routing_spec.rb b/modules/backlogs/spec/routing/backlogs/work_packages_routing_spec.rb index 149f22e437a7..044ae50908fc 100644 --- a/modules/backlogs/spec/routing/backlogs/work_packages_routing_spec.rb +++ b/modules/backlogs/spec/routing/backlogs/work_packages_routing_spec.rb @@ -83,5 +83,13 @@ project_id: "project_42" ) } + + it { + expect(put("/projects/project_42/backlogs/work_packages/move")).to route_to( + controller: "backlogs/work_packages", + action: "move_collection", + project_id: "project_42" + ) + } end end diff --git a/modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_concurrency_spec.rb b/modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_concurrency_spec.rb new file mode 100644 index 000000000000..a29a0352cee3 --- /dev/null +++ b/modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_concurrency_spec.rb @@ -0,0 +1,536 @@ +# 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" + +RSpec.describe Backlogs::WorkPackages::BatchUpdateService, + "concurrent destination updates", + type: :model, + use_transactional_fixtures: false do + self.use_transactional_tests = false + + before do + baseline_user_ids + baseline_role_ids + baseline_status_ids + baseline_priority_ids + fixture_connection_pool.unpin_connection! + end + + after do + side_user_ids = factory_side_user_ids + project.destroy! + user.destroy! + User.where(id: side_user_ids).destroy_all + TypeVariant.where(type_id: type.id).delete_all + Type.unscoped.where(id: type.id).delete_all + Role.where.not(id: baseline_role_ids).destroy_all + Status.where.not(id: baseline_status_ids).delete_all + IssuePriority.where.not(id: baseline_priority_ids).delete_all + ensure + fixture_connection_pool.pin_connection!(true) + end + + let(:fixture_connection_pool) { ActiveRecord::Base.connection_pool } + let(:baseline_user_ids) { User.not_builtin.ids } + let(:factory_side_user_ids) do + User.not_builtin.where.not(id: [*baseline_user_ids, user.id]).ids + end + let(:baseline_role_ids) { Role.pluck(:id) } + let(:baseline_status_ids) { Status.pluck(:id) } + let(:baseline_priority_ids) { IssuePriority.pluck(:id) } + let!(:type) { create(:type) } + let!(:project) do + create(:project, types: [type], enabled_module_names: %i[backlogs work_package_tracking]) + end + let!(:user) do + create(:user, member_with_permissions: { + project => %i[ + view_work_packages + edit_work_packages + view_sprints + manage_sprint_items + start_complete_sprint + ] + }) + end + let!(:source_sprint) do + create(:sprint, + project:, + status: :active, + start_date: Date.current, + finish_date: 1.week.from_now.to_date) + end + let!(:source_bucket) { create(:backlog_bucket, project:) } + let!(:empty_bucket) { create(:backlog_bucket, project:) } + let!(:sprint_work_packages) do + create_list(:work_package, 2, sprint: source_sprint, type:, project:) + end + let!(:bucket_work_packages) do + create_list(:work_package, 2, backlog_bucket: source_bucket, type:, project:) + end + + def cleanup_concurrency_threads(release_events:, threads:, join_timeout: 5) + original_exception = $! + release_events.compact.each(&:set) + threads = threads.compact + cleanup_errors = [] + + threads.each do |thread| + collect_cleanup_error(cleanup_errors) { thread.join(join_timeout) } + end + + lingering_threads = threads.select(&:alive?) + if lingering_threads.any? + cleanup_errors << RuntimeError.new( + "#{lingering_threads.size} thread(s) did not stop during concurrency cleanup" + ) + end + + lingering_threads.each do |thread| + collect_cleanup_error(cleanup_errors) { thread.kill } + collect_cleanup_error(cleanup_errors) { thread.join } + end + + return if original_exception || cleanup_errors.empty? + + raise cleanup_errors.first + end + + def collect_cleanup_error(cleanup_errors) + yield + rescue StandardError => e + cleanup_errors << e + end + + # rubocop:disable RSpec/ExampleLength + it "serializes disjoint batches before resolving append placement in an empty target", retry: 0 do + first_service = described_class.new(user:, work_packages: sprint_work_packages) + second_service = described_class.new(user:, work_packages: bucket_work_packages) + first_paused = Concurrent::Event.new + release_first = Concurrent::Event.new + second_progress = Queue.new + + allow(first_service).to receive(:target_available?).and_wrap_original do |method, *args| + first_paused.set + raise "timed out waiting to release the first append" unless release_first.wait(5) + + method.call(*args) + end + allow(second_service).to receive(:resolve_placement).and_wrap_original do |method, *args| + resolved = method.call(*args) + second_progress << :placement_resolved + resolved + end + allow(OpenProject::Mutex).to receive(:with_advisory_lock_transaction) + .and_wrap_original do |method, entry, suffix = nil, *args, &block| + if Thread.current[:batch_append] == :second && entry == empty_bucket && suffix.nil? + second_progress << :destination_lifecycle_lock_attempted + end + method.call(entry, suffix, *args, &block) + end + + first_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + Thread.current[:batch_append] = :first + first_service.call(list_type: "backlog_bucket", list_id: empty_bucket.id.to_s) + end + end + raise "first append did not reach the placement barrier" unless first_paused.wait(5) + + second_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + Thread.current[:batch_append] = :second + second_service.call(list_type: "backlog_bucket", list_id: empty_bucket.id.to_s) + end + end + + observed_progress = Timeout.timeout(5) { second_progress.pop } + release_first.set + first_result = first_thread.value + second_result = second_thread.value + + expect(observed_progress).to eq :destination_lifecycle_lock_attempted + expect([first_result, second_result]).to all(be_success) + expect(WorkPackage.where(backlog_bucket: empty_bucket).order(:position).pluck(:id)) + .to eq [*sprint_work_packages.map(&:id), *bucket_work_packages.map(&:id)] + ensure + cleanup_concurrency_threads( + release_events: [release_first], + threads: [first_thread, second_thread] + ) + end + # rubocop:enable RSpec/ExampleLength + + # rubocop:disable RSpec/ExampleLength + it "serializes whitespace-top moves before placing into an empty inbox", retry: 0 do + first_service = described_class.new(user:, work_packages: sprint_work_packages) + second_service = described_class.new(user:, work_packages: bucket_work_packages) + first_placed = Concurrent::Event.new + release_first = Concurrent::Event.new + release_second = Concurrent::Event.new + second_progress = Queue.new + + allow(first_service).to receive(:move_members).and_wrap_original do |method, *args, **kwargs, &block| + result = method.call(*args, **kwargs, &block) + first_placed.set + raise "timed out waiting to commit the first top move" unless release_first.wait(5) + + result + end + allow(second_service).to receive(:move_members).and_wrap_original do |method, *args, **kwargs, &block| + result = method.call(*args, **kwargs, &block) + second_progress << :placement_finished + raise "timed out waiting to commit the second top move" unless release_second.wait(5) + + result + end + allow(OpenProject::Mutex).to receive(:with_advisory_lock_transaction) + .and_wrap_original do |method, entry, suffix = nil, *args, &block| + if Thread.current[:batch_top] == :second && + entry == project && suffix == "backlogs_batch_update_destination_inbox" + second_progress << :target_lock_attempted + end + method.call(entry, suffix, *args, &block) + end + + first_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + Thread.current[:batch_top] = :first + first_service.call(list_type: "inbox", prev_id: " \t") + end + end + raise "first top move did not reach the commit barrier" unless first_placed.wait(5) + + second_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + Thread.current[:batch_top] = :second + second_service.call(list_type: "inbox", prev_id: " \t") + end + end + + observed_progress = Timeout.timeout(5) { second_progress.pop } + release_first.set + release_second.set + first_result = first_thread.value + second_result = second_thread.value + + expect([first_result, second_result]).to all(be_success) + inbox_work_packages = WorkPackage.where(project:, sprint_id: nil, backlog_bucket_id: nil) + expect(inbox_work_packages.order(:position).pluck(:position)).to eq [1, 2, 3, 4] + expect(inbox_work_packages.order(:position).pluck(:id)) + .to eq [*bucket_work_packages.map(&:id), *sprint_work_packages.map(&:id)] + expect(observed_progress).to eq :target_lock_attempted + ensure + cleanup_concurrency_threads( + release_events: [release_first, release_second], + threads: [first_thread, second_thread] + ) + end + # rubocop:enable RSpec/ExampleLength + + # rubocop:disable RSpec/ExampleLength + it "waits for a destination mutation before checking a same-list move", retry: 0 do + release_mutation = Concurrent::Event.new + mutation_ready = Concurrent::Event.new + mutation_pid = Queue.new + batch_pid = Queue.new + batch_result = Queue.new + batch_finished = Concurrent::Event.new + original_order = sprint_work_packages.map(&:id) + + mutation_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do |connection| + Sprint.transaction do + locked_sprint = Sprint.lock.find(source_sprint.id) + locked_sprint.update!(status: :completed) + mutation_pid << connection.select_value("SELECT pg_backend_pid()").to_i + mutation_ready.set + raise "timed out waiting to commit the sprint mutation" unless release_mutation.wait(5) + end + end + end + raise "sprint mutation did not acquire its row lock" unless mutation_ready.wait(5) + + batch_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do |connection| + batch_pid << connection.select_value("SELECT pg_backend_pid()").to_i + result = described_class + .new(user:, work_packages: [sprint_work_packages.last]) + .call(list_type: "sprint", list_id: source_sprint.id.to_s, prev_id: "") + batch_result << result + batch_finished.set + end + end + + mutator_backend_pid = Timeout.timeout(5) { mutation_pid.pop } + batch_backend_pid = Timeout.timeout(5) { batch_pid.pop } + observed_progress = Timeout.timeout(5) do + loop do + blocked = ActiveRecord::Base.connection.select_value( + "SELECT #{mutator_backend_pid} = ANY(pg_blocking_pids(#{batch_backend_pid}))" + ) + break :destination_lock_wait if blocked + break :batch_finished if batch_finished.set? + + batch_finished.wait(0.01) + end + end + + release_mutation.set + mutation_thread.value + batch_thread.value + result = Timeout.timeout(5) { batch_result.pop } + + expect(observed_progress).to eq :destination_lock_wait + expect(result).to be_failure + expect(result.message) + .to eq I18n.t("backlogs.work_packages.batch_update_service.unavailable_target") + expect(source_sprint.reload).to be_completed + expect(source_sprint.work_packages_for(project).pluck(:id)).to eq original_order + ensure + cleanup_concurrency_threads( + release_events: [release_mutation], + threads: [mutation_thread, batch_thread] + ) + end + # rubocop:enable RSpec/ExampleLength + + # rubocop:disable RSpec/ExampleLength + it "waits for sprint finish before resolving an append", retry: 0 do + finish_paused = Concurrent::Event.new + release_finish = Concurrent::Event.new + batch_progress = Queue.new + batch_result = Queue.new + append_work_package = bucket_work_packages.first + original_cohort = sprint_work_packages.map(&:id) + append_service = described_class.new(user:, work_packages: [append_work_package]) + + allow(WorkPackages::UpdateService).to receive(:new).and_wrap_original do |method, *args, **kwargs| + if Thread.current[:finish_race] && !Thread.current[:finish_paused] + Thread.current[:finish_paused] = true + finish_paused.set + raise "timed out waiting to continue sprint finish" unless release_finish.wait(5) + end + + method.call(*args, **kwargs) + end + allow(append_service).to receive(:resolve_placement).and_wrap_original do |method, *args| + resolved = method.call(*args) + batch_progress << :placement_resolved + resolved + end + allow(OpenProject::Mutex).to receive(:with_advisory_lock_transaction) + .and_wrap_original do |method, entry, *args, &block| + if Thread.current[:finish_race_batch] && entry.is_a?(Sprint) && entry.id == source_sprint.id + batch_progress << :sprint_lock_attempted + end + method.call(entry, *args, &block) + end + + finish_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + Thread.current[:finish_race] = true + Backlogs::Sprints::FinishService + .new(user:, model: source_sprint) + .call(unfinished_action: "move_to_top_of_backlog") + end + end + raise "sprint finish did not reach its first work-package update" unless finish_paused.wait(5) + + batch_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + Thread.current[:finish_race_batch] = true + result = append_service.call(list_type: "sprint", list_id: source_sprint.id.to_s) + batch_result << result + end + end + + observed_progress = Timeout.timeout(5) { batch_progress.pop } + + expect(observed_progress).to eq :sprint_lock_attempted + expect(batch_thread.join(0.1)).to be_nil + expect(batch_progress).to be_empty + + release_finish.set + finish_result = finish_thread.value + batch_thread.value + append_result = Timeout.timeout(5) { batch_result.pop } + + expect(finish_result).to be_success + expect(append_result).to be_failure + expect(append_result.message) + .to eq I18n.t("backlogs.work_packages.batch_update_service.unavailable_target") + expect(source_sprint.reload).to be_completed + expect(WorkPackage.where(id: original_cohort).pluck(:sprint_id)).to all(be_nil) + expect(source_sprint.work_packages_for(project)).to be_empty + expect(append_work_package.reload).to have_attributes(backlog_bucket: source_bucket, sprint_id: nil) + ensure + cleanup_concurrency_threads( + release_events: [release_finish], + threads: [finish_thread, batch_thread] + ) + end + # rubocop:enable RSpec/ExampleLength + + # rubocop:disable RSpec/ExampleLength + it "waits for sprint finish before moving its enumerated cohort out", retry: 0 do + finish_paused = Concurrent::Event.new + release_finish = Concurrent::Event.new + batch_progress = Queue.new + batch_result = Queue.new + moving_work_package = sprint_work_packages.last + original_cohort = sprint_work_packages.map(&:id) + move_service = described_class.new(user:, work_packages: [moving_work_package]) + + allow(WorkPackages::UpdateService).to receive(:new).and_wrap_original do |method, *args, **kwargs| + if Thread.current[:finish_move_out_race] && !Thread.current[:finish_paused] + Thread.current[:finish_paused] = true + finish_paused.set + raise "timed out waiting to continue sprint finish" unless release_finish.wait(5) + end + + method.call(*args, **kwargs) + end + allow(OpenProject::Mutex).to receive(:with_advisory_lock_transaction) + .and_wrap_original do |method, entry, *args, &block| + if Thread.current[:finish_move_out_batch] && entry.is_a?(Sprint) && entry.id == source_sprint.id + batch_progress << :source_lock_attempted + end + method.call(entry, *args, &block) + end + + finish_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + Thread.current[:finish_move_out_race] = true + Backlogs::Sprints::FinishService + .new(user:, model: source_sprint) + .call(unfinished_action: "move_to_top_of_backlog") + end + end + raise "sprint finish did not enumerate its cohort" unless finish_paused.wait(5) + + batch_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + Thread.current[:finish_move_out_batch] = true + result = move_service.call(list_type: "backlog_bucket", list_id: empty_bucket.id.to_s) + batch_result << result + batch_progress << :batch_finished + end + end + + observed_progress = Timeout.timeout(5) { batch_progress.pop } + release_finish.set + finish_result = finish_thread.value + batch_thread.value + move_result = Timeout.timeout(5) { batch_result.pop } + + expect(observed_progress).to eq :source_lock_attempted + expect(finish_result).to be_success + expect(move_result).to be_failure + expect(move_result.message) + .to eq I18n.t("backlogs.work_packages.batch_update_service.stale_batch") + expect(source_sprint.reload).to be_completed + expect(WorkPackage.where(id: original_cohort).pluck(:sprint_id)).to all(be_nil) + expect(WorkPackage.where(id: original_cohort).pluck(:backlog_bucket_id)).to all(be_nil) + expect(empty_bucket.work_packages).to be_empty + ensure + cleanup_concurrency_threads( + release_events: [release_finish], + threads: [finish_thread, batch_thread] + ) + end + # rubocop:enable RSpec/ExampleLength + + it "terminates a thread that misses the cleanup deadline and reports the cleanup failure" do + release = Concurrent::Event.new + blocker = Queue.new + thread = Thread.new do + release.wait + blocker.pop + end + thread.report_on_exception = false + + expect do + cleanup_concurrency_threads(release_events: [release], threads: [thread], join_timeout: 0.01) + end.to raise_error(RuntimeError, /did not stop during concurrency cleanup/) + expect(release).to be_set + expect(thread).not_to be_alive + ensure + thread&.kill + thread&.join + end + + it "stops all workers and reports a worker failure when no example failure is propagating" do + failed_thread = Thread.new { raise "worker failure" } + failed_thread.report_on_exception = false + blocker = Queue.new + lingering_thread = Thread.new { blocker.pop } + lingering_thread.report_on_exception = false + Timeout.timeout(5) { Thread.pass while failed_thread.alive? } + + expect do + cleanup_concurrency_threads( + release_events: [], + threads: [failed_thread, lingering_thread], + join_timeout: 0.01 + ) + end.to raise_error(RuntimeError, "worker failure") + expect(failed_thread).not_to be_alive + expect(lingering_thread).not_to be_alive + ensure + lingering_thread&.kill + lingering_thread&.join + end + + it "keeps an original failure authoritative while stopping failed and lingering workers" do + failed_thread = Thread.new { raise "worker failure" } + failed_thread.report_on_exception = false + blocker = Queue.new + lingering_thread = Thread.new { blocker.pop } + lingering_thread.report_on_exception = false + Timeout.timeout(5) { Thread.pass while failed_thread.alive? } + + expect do + raise "original failure" + ensure + cleanup_concurrency_threads( + release_events: [], + threads: [failed_thread, lingering_thread], + join_timeout: 0.01 + ) + end.to raise_error(RuntimeError, "original failure") + expect(failed_thread).not_to be_alive + expect(lingering_thread).not_to be_alive + ensure + lingering_thread&.kill + lingering_thread&.join + end +end diff --git a/modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_spec.rb b/modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_spec.rb new file mode 100644 index 000000000000..6a888797fecd --- /dev/null +++ b/modules/backlogs/spec/services/backlogs/work_packages/batch_update_service_spec.rb @@ -0,0 +1,545 @@ +# 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" + +RSpec.describe Backlogs::WorkPackages::BatchUpdateService, type: :model do + shared_let(:type) { create(:type) } + shared_let(:project) do + create(:project, types: [type], enabled_module_names: %i[backlogs work_package_tracking]) + end + shared_let(:user) do + create(:user, member_with_permissions: { + project => %i[view_work_packages edit_work_packages view_sprints manage_sprint_items] + }) + end + let!(:sprint) { create(:sprint, project:) } + let!(:bucket) { create(:backlog_bucket, project:) } + + let!(:sprint_wp1) { create(:work_package, sprint:, position: 1, type:, project:) } + let!(:sprint_wp2) { create(:work_package, sprint:, position: 2, type:, project:) } + let!(:sprint_wp3) { create(:work_package, sprint:, position: 3, type:, project:) } + let!(:bucket_wp1) { create(:work_package, backlog_bucket: bucket, position: 1, type:, project:) } + let!(:bucket_wp2) { create(:work_package, backlog_bucket: bucket, position: 2, type:, project:) } + + def service(work_packages) + described_class.new(user:, work_packages:) + end + + def sprint_order + sprint.work_packages_for(project).pluck(:id) + end + + it "moves a cross-list batch as one contiguous block after the predecessor" do + result = service([bucket_wp1, sprint_wp3]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: sprint_wp1.id.to_s) + + expect(result).to be_success + expect(result.result.map(&:id)).to eq [bucket_wp1.id, sprint_wp3.id] + expect(sprint_order).to eq [sprint_wp1.id, bucket_wp1.id, sprint_wp3.id, sprint_wp2.id] + expect(sprint.work_packages_for(project).pluck(:position)).to eq [1, 2, 3, 4] + expect(bucket_wp1.reload.backlog_bucket_id).to be_nil + end + + it "inserts at the top for a blank prev_id" do + result = service([sprint_wp2, sprint_wp3]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: "") + + expect(result).to be_success + expect(sprint_order).to eq [sprint_wp2.id, sprint_wp3.id, sprint_wp1.id] + expect(sprint.work_packages_for(project).pluck(:position)).to eq [1, 2, 3] + end + + it "treats a whitespace-only prev_id as top, like a blank one" do + result = service([sprint_wp3]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: " ") + + expect(result).to be_success + expect(sprint_order).to eq [sprint_wp3.id, sprint_wp1.id, sprint_wp2.id] + end + + it "appends after the last non-batch member for an absent prev_id" do + result = service([sprint_wp1, bucket_wp2]) + .call(list_type: "sprint", list_id: sprint.id.to_s) + + expect(result).to be_success + # sprint_wp1 was already in the sprint: append gathers it behind the last + # member that is NOT part of the batch (sprint_wp3), not behind itself. + expect(sprint_order).to eq [sprint_wp2.id, sprint_wp3.id, sprint_wp1.id, bucket_wp2.id] + expect(sprint.work_packages_for(project).pluck(:position)).to eq [1, 2, 3, 4] + end + + it "appends at the top of an otherwise empty target" do + empty_bucket = create(:backlog_bucket, project:) + + result = service([sprint_wp1, sprint_wp2]) + .call(list_type: "backlog_bucket", list_id: empty_bucket.id.to_s) + + expect(result).to be_success + expect(WorkPackage.where(backlog_bucket: empty_bucket).order(:position).pluck(:id)) + .to eq [sprint_wp1.id, sprint_wp2.id] + expect(WorkPackage.where(backlog_bucket: empty_bucket).order(:position).pluck(:position)) + .to eq [1, 2] + # The source sprint loses two of its three members: a gap left behind + # instead of renumbering the remaining member down to position 1 would + # corrupt future inserts there without ever failing an id-only check. + expect(sprint_order).to eq [sprint_wp3.id] + expect(sprint.work_packages_for(project).pluck(:position)).to eq [1] + end + + it "fails for an invalid target" do + result = service([sprint_wp1]).call(list_type: "unknown", list_id: "1") + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.update_service.invalid_target_type") + end + + describe "atomicity" do + it "rolls back every member when a later member fails", with_ee: %i[readonly_work_packages] do + # A readonly status blocks every attribute write through + # WorkPackage#modification_blocked, so the inner service fails for it. + readonly_status = create(:status, is_readonly: true) + blocked = create(:work_package, backlog_bucket: bucket, position: 3, type:, project:, + status: readonly_status) + + result = service([bucket_wp1, blocked]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: sprint_wp1.id.to_s) + + expect(result).to be_failure + expect(bucket_wp1.reload.backlog_bucket_id).to eq bucket.id + expect(bucket_wp1.position).to eq 1 + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "rolls back inside an enclosing transaction", with_ee: %i[readonly_work_packages] do + readonly_status = create(:status, is_readonly: true) + blocked = create(:work_package, backlog_bucket: bucket, position: 3, type:, project:, + status: readonly_status) + + result = nil + WorkPackage.transaction do + result = service([bucket_wp1, blocked]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: sprint_wp1.id.to_s) + end + + expect(result).to be_failure + expect(bucket_wp1.reload).to have_attributes(backlog_bucket_id: bucket.id, sprint_id: nil, position: 1) + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "returns a failed result and rolls back when a later member raises" do + # An operational exception from a later member must not escape as a + # 500: the design requires one failed batch result after rollback. + # The raw adapter message ("boom") is internal detail and unlocalized, + # so the user-facing result carries a generic i18n message while the + # exception itself is logged. + failing_inner = Backlogs::WorkPackages::UpdateService.new(user:, work_package: bucket_wp2) + allow(failing_inner).to receive(:call).and_raise(ActiveRecord::StatementInvalid, "boom") + allow(Backlogs::WorkPackages::UpdateService).to receive(:new).and_call_original + allow(Backlogs::WorkPackages::UpdateService) + .to receive(:new).with(user:, work_package: bucket_wp2) + .and_return(failing_inner) + logged_message = nil + allow(Rails.logger).to receive(:error) { |&blk| logged_message = blk.call } + + result = service([bucket_wp1, bucket_wp2]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: "") + + expect(result).to be_failure + expect(result.message) + .to eq I18n.t("backlogs.work_packages.batch_update_service.unexpected_failure") + expect(logged_message).to include("boom") + expect(bucket_wp1.reload).to have_attributes(backlog_bucket_id: bucket.id, position: 1) + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + # The established call_hook observation pattern — see + # update_service_persistence_spec.rb's "after-commit hooks" describe. + it "fires update hooks only after the whole batch commits, each observing the final order" do + observed_orders = [] + observed_states = [] + allow(OpenProject::Hook).to receive(:call_hook).and_call_original + allow(OpenProject::Hook).to receive(:call_hook).with(:work_package_after_update, anything) do |_hook, context| + observed_orders << sprint.work_packages_for(project).pluck(:id) + # The hook context carries the WorkPackage INSTANCE + # (WorkPackage#call_after_update_hook builds it from `self`), not a + # fresh DB read: read straight off the instance's own attributes, no + # reload/query here, to prove it already holds its final state. + hook_wp = context[:work_package] + observed_states << [hook_wp.id, hook_wp.sprint_id, hook_wp.backlog_bucket_id, hook_wp.position] + end + + # A same-list downward reorder: sprint_wp1 is processed first and lands + # above sprint_wp2's own original slot, so sprint_wp2's later + # remove_from_list (removing IT from that slot) decrements + # sprint_wp1's already-written row out from under its in-memory + # instance — exactly the shape that exposes a stale hook context. + service([sprint_wp1, sprint_wp2]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: sprint_wp3.id.to_s) + + final_order = [sprint_wp3.id, sprint_wp1.id, sprint_wp2.id] + expect(observed_orders.size).to eq 2 + # Every hook call already sees the complete committed batch — no hook + # may observe a partially moved intermediate state. + expect(observed_orders).to all(eq(final_order)) + expect(observed_states).to contain_exactly( + [sprint_wp1.id, sprint.id, nil, 2], + [sprint_wp2.id, sprint.id, nil, 3] + ) + end + + it "fires no update hook for a rolled-back batch", with_ee: %i[readonly_work_packages] do + readonly_status = create(:status, is_readonly: true) + blocked = create(:work_package, backlog_bucket: bucket, position: 3, type:, project:, + status: readonly_status) + allow(OpenProject::Hook).to receive(:call_hook).and_call_original + + service([bucket_wp1, blocked]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: "") + + expect(OpenProject::Hook) + .not_to have_received(:call_hook).with(:work_package_after_update, anything) + end + + it "names the failing member as a dependent result", with_ee: %i[readonly_work_packages] do + readonly_status = create(:status, :readonly) + sprint_wp3.update_columns(status_id: readonly_status.id) + + result = service([sprint_wp2, sprint_wp3]) + .call(list_type: "backlog_bucket", list_id: bucket.id.to_s, prev_id: "") + + expect(result).to be_failure + failed = result.dependent_results.find(&:failure?) + expect(failed.result).to eq sprint_wp3 + expect(failed.message).to be_present + expect(sprint.work_packages_for(project).pluck(:id)).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + end + + describe "batch shape" do + it "rejects an empty batch without touching the database" do + result = service([]).call(list_type: "inbox") + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.move_collection.invalid_ids") + end + + it "rejects members from two projects" do + foreign = create(:work_package, type:) + + result = service([sprint_wp1, foreign]).call(list_type: "backlog_bucket", list_id: bucket.id.to_s, prev_id: "") + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.mixed_projects") + end + + it "rejects a batch above the cap before touching the database" do + oversized = Array.new(described_class::MAX_BATCH_SIZE + 1) { sprint_wp1 } + + expect do + result = service(oversized).call(list_type: "backlog_bucket", list_id: bucket.id.to_s, prev_id: "") + expect(result.message).to include( + I18n.t("backlogs.work_packages.move_collection.too_many_work_packages", + max: described_class::MAX_BATCH_SIZE) + ) + end.not_to change { WorkPackage.order(:id).pluck(:id, :sprint_id, :backlog_bucket_id, :position) } + end + + it "ignores an invisible anchor" do + hidden = create(:work_package, sprint:, position: 4, type:, project:) + allow(WorkPackage).to receive(:visible).with(user).and_return(WorkPackage.where.not(id: hidden.id)) + + result = service([bucket_wp1]).call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: hidden.id.to_s) + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor") + end + + it "appends after the last positioned member when a row carries no position" do + create(:work_package, sprint:, type:, project:).update_columns(position: nil) + + result = service([bucket_wp1]).call(list_type: "sprint", list_id: sprint.id.to_s) + + expect(result).to be_success + expect(result.result.first.higher_item).to eq sprint_wp3 + end + end + + describe "batch project cohort" do + it "rejects a batch whose project changed after loading but before the lock" do + other_project = create(:project, types: [type]) + hopped = sprint_wp2 + + batch = service([sprint_wp1, hopped]) + # Simulate the race directly on the row, bypassing the loaded instance: + # a member hops to another project between controller load and lock + # acquisition. + WorkPackage.find(hopped.id).update_columns(project_id: other_project.id) + + result = batch.call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: "") + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.stale_batch") + # Nothing moved: the sprint order is exactly what it was before the + # call, and the hopped member is left exactly where the race put it — + # the rejection does not depend on the inner service failing. + expect(sprint.work_packages.order(:position).pluck(:id)).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + expect(sprint.work_packages.order(:position).pluck(:position)).to eq [1, 2, 3] + expect(hopped.reload.project_id).to eq other_project.id + expect(sprint_wp1.reload.project_id).to eq project.id + end + + it "rejects a batch with a member deleted after loading" do + batch = service([sprint_wp1, sprint_wp2]) + # Simulate the race directly on the row, bypassing the loaded instance: + # a member is deleted between controller load and lock acquisition. + WorkPackage.where(id: sprint_wp2.id).delete_all + + result = batch.call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: "") + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.stale_batch") + expect(sprint_wp1.reload.position).to eq 1 + end + end + + describe "advisory locks" do + def record_locks + locked = [] + allow(OpenProject::Mutex).to receive(:with_advisory_lock_transaction) + .and_wrap_original do |method, entry, suffix = nil, *args, &block| + locked << [entry, suffix] + method.call(entry, suffix, *args, &block) + end + locked + end + + def work_package_lock_ids(locked) + locked.filter_map { |entry, _suffix| entry.id if entry.is_a?(WorkPackage) } + end + + it "acquires the batch and predecessor locks in ascending id order" do + locked = record_locks + + # Deliberately out-of-order input, predecessor id between them. + service([sprint_wp3, sprint_wp1]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: sprint_wp2.id.to_s) + + expect(locked.first).to eq [sprint, nil] + expect(work_package_lock_ids(locked).first(3)).to eq [sprint_wp3.id, sprint_wp1.id, sprint_wp2.id].sort + end + + it "locks the implicit append anchor for an absent prev_id" do + locked = record_locks + + service([bucket_wp1]).call(list_type: "sprint", list_id: sprint.id.to_s) + + expect(locked.first(3)).to eq [ + [bucket, nil], + [sprint, nil], + [project, "backlogs_batch_update_destination_sprint_#{sprint.id}"] + ] + expect(work_package_lock_ids(locked).first(2)).to eq [bucket_wp1.id, sprint_wp3.id].sort + end + + it "takes the source and destination lifecycle locks before work-package locks for top placement" do + locked = record_locks + + service([sprint_wp2]).call(list_type: "backlog_bucket", list_id: bucket.id.to_s, prev_id: "") + + expect(locked.first(3)).to eq [ + [bucket, nil], + [sprint, nil], + [project, "backlogs_batch_update_destination_backlog_bucket_#{bucket.id}"] + ] + expect(locked.find { |entry, _suffix| entry.is_a?(WorkPackage) }).to eq [sprint_wp2, nil] + end + + it "takes the source lifecycle and inbox placement locks before work-package locks" do + locked = record_locks + + service([sprint_wp1]).call(list_type: "inbox") + + expect(locked.first(2)).to eq [ + [sprint, nil], + [project, "backlogs_batch_update_destination_inbox"] + ] + end + + it "orders lifecycle locks by the concrete mutex identity" do + stub_const("ArchivedSprint", Class.new(Sprint)) + concrete_sprint = sprint.becomes(ArchivedSprint) + batch = service([bucket_wp1]) + lock_names = [] + + allow(batch).to receive(:raw_destination).and_wrap_original do |method, target| + destination = method.call(target) + destination.is_a?(Sprint) && destination.id == sprint.id ? concrete_sprint : destination + end + allow(OpenProject::Mutex) + .to receive(:with_advisory_lock) + .and_wrap_original do |method, resource_class, lock_name, *args, &block| + lock_names << lock_name + method.call(resource_class, lock_name, *args, &block) + end + + result = batch.call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: "") + + expect(result).to be_success + expect(lock_names.grep(/mutex_on_(ArchivedSprint|BacklogBucket)_/).first(2)).to eq [ + "mutex_on_ArchivedSprint_#{sprint.id}", + "mutex_on_BacklogBucket_#{bucket.id}" + ] + end + + it "takes every lock in one flat sequence rather than nested blocks" do + depths = [] + depth = 0 + allow(OpenProject::Mutex).to receive(:with_advisory_lock_transaction) + .and_wrap_original do |method, *args, &block| + depths << depth + depth += 1 + begin + method.call(*args, &block) + ensure + depth -= 1 + end + end + + service([sprint_wp3, sprint_wp1, sprint_wp2]) + .call(list_type: "backlog_bucket", list_id: bucket.id.to_s, prev_id: bucket_wp1.id.to_s) + + expect(depths).to all(eq(0)) + end + end + + describe "target availability" do + it "rejects a same-list reorder inside a sprint that completed after load" do + sprint.update!(status: "completed") + + result = service([sprint_wp1]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: sprint_wp3.id.to_s) + + expect(result).to be_failure + expect(result.message) + .to eq I18n.t("backlogs.work_packages.batch_update_service.unavailable_target") + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + expect(sprint.work_packages_for(project).pluck(:position)).to eq [1, 2, 3] + end + + it "rejects a cross-list move into a sprint that completed after load" do + sprint.update!(status: "completed") + + result = service([bucket_wp1]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: sprint_wp1.id.to_s) + + expect(result).to be_failure + expect(result.message) + .to eq I18n.t("backlogs.work_packages.batch_update_service.unavailable_target") + expect(bucket_wp1.reload.backlog_bucket_id).to eq bucket.id + expect(bucket_wp1.position).to eq 1 + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "rejects a backlog bucket target from another project" do + other_project = create(:project, types: [type]) + foreign_bucket = create(:backlog_bucket, project: other_project) + + result = service([sprint_wp1]) + .call(list_type: "backlog_bucket", list_id: foreign_bucket.id.to_s) + + expect(result).to be_failure + expect(result.message) + .to eq I18n.t("backlogs.work_packages.batch_update_service.unavailable_target") + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "locks and reloads the authoritative backlog bucket row before policy" do + recorder = ActiveRecord::QueryRecorder.new do + service([sprint_wp1]).call(list_type: "backlog_bucket", list_id: bucket.id.to_s, prev_id: "") + end + + expect(recorder.log.grep(/FROM "backlog_buckets".*FOR UPDATE/).size).to eq 1 + end + end + + describe "stale predecessor" do + it "rejects a predecessor that is not in the target list" do + result = service([sprint_wp2]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: bucket_wp1.id.to_s) + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor") + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "rejects a predecessor contained in the batch" do + result = service([sprint_wp1, sprint_wp2]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: sprint_wp1.id.to_s) + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor") + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "rejects a missing predecessor" do + result = service([sprint_wp1]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: "999999") + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor") + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "rejects a shared-sprint predecessor from another project" do + # A shared sprint can contain another project's work packages, but the + # acts_as_list scope includes project_id: such an anchor would be + # unresolvable for move_after and silently fall back to the top. + other_project = create(:project, types: [type]) + foreign_wp = create(:work_package, sprint:, type:, project: other_project) + + result = service([sprint_wp2]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: foreign_wp.id.to_s) + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor") + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + + it "rejects a malformed prev_id instead of integer-casting it" do + result = service([sprint_wp2]) + .call(list_type: "sprint", list_id: sprint.id.to_s, prev_id: "#{sprint_wp1.id}abc") + + expect(result).to be_failure + expect(result.message).to eq I18n.t("backlogs.work_packages.batch_update_service.stale_predecessor") + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + end +end diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index 23d7327018d8..ecbce1b28d67 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -887,6 +887,29 @@ def drag_work_package(moved, before: nil, after: nil, into: nil) retry end + # Drags expecting a rejection: drag_work_package waits on the frame + # reload a successful cross-list move causes, while a rejected move only + # streams an error flash, so this settles on the stream render instead. + def drag_work_package_expecting_failure(moved, after:) + # See pick_up_and_release_work_package for the retry rationale. + retry_block( + args: { + tries: 3, + on: [ + Capybara::Cuprite::ObsoleteNode, + Selenium::WebDriver::Error::StaleElementReferenceError + ] + } + ) do + moved_element = find(work_package_selector(moved)) + target_element = find(work_package_selector(after)) + + wait_for_backlogs_turbo_stream(frame_reload: false) do + drag_backlogs_item(source: moved_element, target: target_element, edge: :bottom) + end + end + end + # Drags a confined card over another sprint's list body and releases it # there. The release must resolve to nothing: no drop indicator over the # target, no row of it accepting, no move request. The card's unchanged @@ -906,7 +929,7 @@ def drag_work_package_without_move(moved, into:) target_element = find(list_body_selector(sprint_selector(into))) install_backlogs_move_request_probe begin - drag_backlogs_item(source: moved_element, target: target_element) + drag_backlogs_item(source: moved_element, target: target_element, dwell: true) ensure stop_backlogs_move_request_probe end @@ -918,20 +941,25 @@ def drag_work_package_without_move(moved, into:) # The refusal must be observable, or the assertions above would also pass # for a drag that never engaged. The drop has to reach the controller — - # the foreign container stays an accepted drop target so the drag keeps + # the refused container stays an accepted drop target so the drag keeps # the standard cursor, so it may appear in the drop's target list, but no - # row of it may — and the final dragover, the one over the foreign - # container, must show no drop position and mark that container refused - # (the muted danger outline) rather than active. Earlier dragovers may - # legitimately show indicators while the pointer is still crossing the - # card's own list, which keeps accepting it for real. + # row of it may — and the last container feedback the drag painted must be + # a refusal (the muted danger outline) rather than an active outline. + # Container state is read across the whole event stream, not from the + # final dragover: the drop engine paints on an animation frame, so a + # refusal can land on a later dragenter than the last dragover. Earlier + # feedback may legitimately be active while the pointer is still crossing + # a list that accepts the drag for real. def expect_backlogs_drag_refused refusal = page.evaluate_script(<<~JS) (() => { const state = window.__opBacklogsDndProbeState; const call = state?.handleDropCalls?.at(-1); - const lastDragover = (state?.events ?? []) - .filter((event) => event.type === 'dragover') + const events = state?.events ?? []; + const lastDragover = events.filter((event) => event.type === 'dragover').at(-1); + const lastContainers = events + .map((event) => event.dropContainers) + .filter((containers) => containers.length > 0) .at(-1); return { @@ -939,7 +967,7 @@ def expect_backlogs_drag_refused dropTargetTypes: call?.dropTargets?.map((target) => target.data?.entries?.type) ?? [], observedDragover: Boolean(lastDragover), dropPositions: lastDragover?.dropPositions ?? null, - dropContainers: lastDragover?.dropContainers ?? null + dropContainers: lastContainers ?? null }; })() JS @@ -1149,8 +1177,8 @@ def readonly_lock_selector "[aria-label='#{Status.human_attribute_name(:is_readonly)}']" end - def drag_backlogs_item(source:, target:, edge: nil) - selenium_drag_backlogs_item(source:, target:, edge:) + def drag_backlogs_item(source:, target:, edge: nil, dwell: false) + selenium_drag_backlogs_item(source:, target:, edge:, dwell:) end def pick_up_and_release_backlogs_item(source) @@ -1208,11 +1236,11 @@ def scroll_backlogs_source_into_view(source) scroll_to_element(source, block: :nearest) end - def selenium_drag_backlogs_item(source:, target:, edge: nil) + def selenium_drag_backlogs_item(source:, target:, edge: nil, dwell: false) install_backlogs_dnd_probe(source:, target:, edge:) offset_x, offset_y = selenium_target_offset(target.native.rect, edge:) - perform_native_drag(source:, target:, offset_x:, offset_y:) + perform_native_drag(source:, target:, offset_x:, offset_y:, dwell:) # Assert Pragmatic DnD tore down its own honey-pot overlay before we force # a cleanup, so a regression that leaves the overlay stuck is caught here diff --git a/spec/support/shared/drag_and_drop_helper_spec.rb b/spec/support/shared/drag_and_drop_helper_spec.rb index 8c63f0bfd0de..928cefadfd9c 100644 --- a/spec/support/shared/drag_and_drop_helper_spec.rb +++ b/spec/support/shared/drag_and_drop_helper_spec.rb @@ -70,13 +70,18 @@ def drag_n_drop_element(from:, to:, offset_x: nil, offset_y: nil) # relative to the target element's center (callers pick the exact drop point # for edge targeting), so callers don't need to keep the target scrolled into # view before computing them. -def perform_native_drag(source:, target:, offset_x: 0, offset_y: 0) +# +# `dwell` adds a second pointer move over the target before releasing. One +# move produces a single dragover, and a drag engine that paints its drop +# feedback on an animation frame has not painted by then; a caller asserting +# that feedback needs the extra event. +def perform_native_drag(source:, target:, offset_x: 0, offset_y: 0, dwell: false) # Ensure both elements are on the page, note this works only if the screen # size can fit both. scroll_to_element(source, block: :nearest) scroll_to_element(target, block: :nearest) - page + action = page .driver .browser .action @@ -85,8 +90,10 @@ def perform_native_drag(source:, target:, offset_x: 0, offset_y: 0) .pause(duration: 0.1) .move_to(target.native, offset_x, offset_y) .pause(duration: 0.1) - .release - .perform + + action = action.move_by(0, 1).pause(duration: 0.1) if dwell + + action.release.perform end def drag_by_pixel(element:, by_x:, by_y:)