diff --git a/frontend/src/stimulus/controllers/async-dialog.controller.spec.ts b/frontend/src/stimulus/controllers/async-dialog.controller.spec.ts new file mode 100644 index 000000000000..6581308bc323 --- /dev/null +++ b/frontend/src/stimulus/controllers/async-dialog.controller.spec.ts @@ -0,0 +1,266 @@ +//-- 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. +//++ + +import type { FetchResponse } from '@rails/request.js'; +import { waitFor } from '@testing-library/dom'; +import { vi, type Mock } from 'vitest'; + +import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; +import { TurboHelpers } from 'core-turbo/helpers'; +import type AsyncDialogControllerType from './async-dialog.controller'; + +const perform = vi.fn(); +const FetchRequest = vi.fn(function FetchRequestMock( + _method:string, + _url:string|URL, + _options:{ body?:FormData; responseKind:'turbo-stream' }, +) { + return { perform }; +}); + +vi.doMock('@rails/request.js', () => ({ FetchRequest })); + +describe('Async dialog controller', () => { + let ctx:StimulusTestContext; + let AsyncDialogController:typeof AsyncDialogControllerType; + let showProgressBar:Mock; + let hideProgressBar:Mock; + + beforeAll(async () => { + ({ default: AsyncDialogController } = await import('./async-dialog.controller')); + }); + + beforeEach(async () => { + FetchRequest.mockClear(); + perform.mockReset(); + showProgressBar = vi.spyOn(TurboHelpers, 'showProgressBar').mockImplementation(() => undefined); + hideProgressBar = vi.spyOn(TurboHelpers, 'hideProgressBar').mockImplementation(() => undefined); + + ctx = await setupStimulusTest({ + controllers: { 'async-dialog': AsyncDialogController }, + }); + }); + + afterEach(() => { + ctx.dispose(); + vi.restoreAllMocks(); + }); + + function turboStreamResponse({ + ok = true, + unprocessableEntity = false, + }:{ ok?:boolean; unprocessableEntity?:boolean } = {}) { + const renderTurboStream = vi.fn().mockResolvedValue(undefined); + const response = { + ok, + unprocessableEntity, + isTurboStream: true, + renderTurboStream, + } as unknown as FetchResponse; + return { response, renderTurboStream }; + } + + function nonTurboResponse() { + const renderTurboStream = vi.fn().mockResolvedValue(undefined); + const response = { + ok: true, + unprocessableEntity: false, + isTurboStream: false, + renderTurboStream, + } as unknown as FetchResponse; + return { response, renderTurboStream }; + } + + function resolveLikeFetchRequest(response:FetchResponse) { + perform.mockImplementation(async () => { + if ((response.ok || response.unprocessableEntity) && response.isTurboStream) { + await response.renderTurboStream(); + } + + return response; + }); + } + + async function mountPostButton() { + await ctx.mount(` +
+ + +
+ `); + + const form = ctx.container.querySelector('#dialog-form')!; + const button = ctx.container.querySelector('button')!; + Object.defineProperty(form, 'action', { configurable: true, value: '/dialog' }); + return { button, form }; + } + + function trigger(controller:AsyncDialogControllerType, url?:string) { + return (controller as unknown as { triggerTurboStream(url?:string):Promise }).triggerTurboStream(url); + } + + it('dispatches beforeLoad and submits form data captured after synchronous listeners run', async () => { + const { response } = turboStreamResponse(); + resolveLikeFetchRequest(response); + const { button, form } = await mountPostButton(); + let beforeLoad!:CustomEvent<{ form:HTMLFormElement|null }>; + button.addEventListener('async-dialog:beforeLoad', (event) => { + beforeLoad = event as CustomEvent<{ form:HTMLFormElement|null }>; + const input = document.createElement('input'); + input.name = 'ids[]'; + input.value = '7'; + form.appendChild(input); + }); + + button.click(); + + await waitFor(() => expect(FetchRequest).toHaveBeenCalledOnce()); + expect(beforeLoad.defaultPrevented).toBe(false); + expect(beforeLoad.detail.form).toBe(form); + const [, , options] = FetchRequest.mock.calls[0]; + const submitted = options.body; + expect(submitted).toBeInstanceOf(FormData); + expect(FetchRequest).toHaveBeenCalledWith('post', '/dialog', { + body: submitted, + responseKind: 'turbo-stream', + }); + if (!(submitted instanceof FormData)) throw new Error('Expected form data'); + expect([...submitted.entries()]).toEqual([['authenticity_token', 'token'], ['ids[]', '7']]); + }); + + it('makes no request when beforeLoad is canceled', async () => { + const { button } = await mountPostButton(); + button.addEventListener('async-dialog:beforeLoad', (event) => event.preventDefault()); + + button.click(); + await ctx.nextFrame(); + + expect(FetchRequest).not.toHaveBeenCalled(); + expect(showProgressBar).not.toHaveBeenCalled(); + }); + + it('keeps existing anchors on GET with their href', async () => { + const { response } = turboStreamResponse(); + resolveLikeFetchRequest(response); + await ctx.mount('Open'); + const anchor = ctx.container.querySelector('a')!; + + anchor.click(); + + await waitFor(() => expect(FetchRequest).toHaveBeenCalledOnce()); + expect(FetchRequest).toHaveBeenCalledWith('GET', anchor.href, { + body: undefined, + responseKind: 'turbo-stream', + }); + }); + + it('ignores repeated activation while a request is loading', async () => { + let resolveRequest!:(response:FetchResponse) => void; + perform.mockReturnValue(new Promise((resolve) => { resolveRequest = resolve; })); + const { button } = await mountPostButton(); + + button.click(); + button.click(); + + expect(FetchRequest).toHaveBeenCalledOnce(); + resolveRequest(turboStreamResponse().response); + await waitFor(() => expect(button).not.toHaveAttribute('aria-disabled')); + }); + + it.each([ + ['a successful response', turboStreamResponse().response], + ['an unprocessable response', turboStreamResponse({ ok: false, unprocessableEntity: true }).response], + ])('clears loading state after %s', async (_label, response) => { + resolveLikeFetchRequest(response); + const { button } = await mountPostButton(); + + button.click(); + expect(button).toHaveAttribute('aria-disabled', 'true'); + + await waitFor(() => expect(button).not.toHaveAttribute('aria-disabled')); + expect(showProgressBar).toHaveBeenCalledOnce(); + expect(hideProgressBar).toHaveBeenCalledOnce(); + }); + + it('renders a 500 Turbo Stream exactly once and clears loading state', async () => { + const { response, renderTurboStream } = turboStreamResponse({ ok: false }); + resolveLikeFetchRequest(response); + const { button } = await mountPostButton(); + + button.click(); + + await waitFor(() => expect(renderTurboStream).toHaveBeenCalledOnce()); + expect(button).not.toHaveAttribute('aria-disabled'); + expect(hideProgressBar).toHaveBeenCalledOnce(); + }); + + it('rejects a 200 non-Turbo response without rendering and clears loading state', async () => { + const { response, renderTurboStream } = nonTurboResponse(); + resolveLikeFetchRequest(response); + const { button } = await mountPostButton(); + const controller = ctx.getController('async-dialog', button); + + await expect(trigger(controller)).rejects.toThrow('Response is not a Turbo Stream'); + + expect(renderTurboStream).not.toHaveBeenCalled(); + expect(button).not.toHaveAttribute('aria-disabled'); + expect(hideProgressBar).toHaveBeenCalledOnce(); + }); + + it('clears loading state when the request rejects', async () => { + const error = new Error('network failed'); + perform.mockRejectedValue(error); + const { button } = await mountPostButton(); + const controller = ctx.getController('async-dialog', button); + + await expect(trigger(controller)).rejects.toBe(error); + + expect(button).not.toHaveAttribute('aria-disabled'); + expect(hideProgressBar).toHaveBeenCalledOnce(); + }); + + it('uses a custom event URL without including the associated form', async () => { + const { response } = turboStreamResponse(); + resolveLikeFetchRequest(response); + const { button } = await mountPostButton(); + const controller = ctx.getController('async-dialog', button); + let associatedForm:HTMLFormElement|null|undefined; + button.addEventListener('async-dialog:beforeLoad', (event) => { + associatedForm = (event as CustomEvent<{ form:HTMLFormElement|null }>).detail.form; + }); + + controller.handleOpenDialog(new CustomEvent('open', { detail: { url: '/override' } })); + + await waitFor(() => expect(FetchRequest).toHaveBeenCalledOnce()); + expect(associatedForm).toBeNull(); + expect(FetchRequest).toHaveBeenCalledWith('GET', '/override', { + body: undefined, + responseKind: 'turbo-stream', + }); + }); +}); diff --git a/frontend/src/stimulus/controllers/async-dialog.controller.ts b/frontend/src/stimulus/controllers/async-dialog.controller.ts index 1896be76f699..e116f6a5260c 100644 --- a/frontend/src/stimulus/controllers/async-dialog.controller.ts +++ b/frontend/src/stimulus/controllers/async-dialog.controller.ts @@ -26,82 +26,92 @@ // See COPYRIGHT and LICENSE files for more details. //++ -import { ApplicationController } from 'stimulus-use'; -import { renderStreamMessage } from '@hotwired/turbo'; +import { Controller } from '@hotwired/stimulus'; +import { FetchRequest } from '@rails/request.js'; +import { performTurboStreamRequest } from 'core-stimulus/helpers/request-helpers'; import { TurboHelpers } from 'core-turbo/helpers'; -export default class AsyncDialogController extends ApplicationController { +export default class AsyncDialogController extends Controller { static values = { disableDuringLoad: { type: Boolean, default: true } }; declare disableDuringLoadValue:boolean; private loading = false; - connect() { - // Only bind events if we have an href to work with - if (this.href) { - this.bindEventListeners(); - } - } + private readonly handleClick = (event:Event):void => { + event.preventDefault(); + void this.triggerTurboStream(); + }; - private bindEventListeners() { - this.element.addEventListener('click', (event:MouseEvent) => { + private readonly handleKeydown = (event:Event):void => { + const keyboardEvent = event as KeyboardEvent; + if (keyboardEvent.key === 'Enter' || keyboardEvent.key === ' ') { event.preventDefault(); - this.triggerTurboStream(this.href); - }); + void this.triggerTurboStream(); + } + }; - this.element.addEventListener('keydown', (event:KeyboardEvent) => { - if (event.key === 'Enter' || event.key === ' ') { - event.preventDefault(); - this.triggerTurboStream(this.href); - } - }); + connect():void { + if (this.href || (this.element instanceof HTMLButtonElement && this.element.form)) { + this.element.addEventListener('click', this.handleClick); + this.element.addEventListener('keydown', this.handleKeydown); + } } - private triggerTurboStream(url:string):void { + disconnect():void { + this.element.removeEventListener('click', this.handleClick); + this.element.removeEventListener('keydown', this.handleKeydown); + } + + private async triggerTurboStream(urlOverride?:string):Promise { if (this.disableDuringLoadValue && this.loading) return; - if (this.disableDuringLoadValue) { - this.loading = true; - (this.element as HTMLElement).setAttribute('aria-disabled', 'true'); - } - TurboHelpers.showProgressBar(); + const form = urlOverride + ? null + : this.element instanceof HTMLButtonElement ? this.element.form : null; + const event = this.dispatch('beforeLoad', { cancelable: true, detail: { form } }); + if (event.defaultPrevented) return; - void fetch(url, { - method: this.method, - headers: { - Accept: 'text/vnd.turbo-stream.html', - }, - }).then((response) => { - const contentType = response.headers.get('Content-Type') ?? ''; - const isTurboStream = contentType.includes('text/vnd.turbo-stream.html'); - - if (!isTurboStream) { - return Promise.reject(new Error('Response is not a Turbo Stream')); - } + const method = form?.method ?? this.method; + const url = urlOverride ?? form?.action ?? this.href; + const body = form ? new FormData(form) : undefined; - return response.text(); - }).then((html) => { - renderStreamMessage(html); - }).finally(() => { - if (this.disableDuringLoadValue) { - this.loading = false; - (this.element as HTMLElement).removeAttribute('aria-disabled'); - } + this.setLoading(true); + TurboHelpers.showProgressBar(); + + try { + await performTurboStreamRequest(new FetchRequest(method, url, { + body, + responseKind: 'turbo-stream', + })); + } finally { + this.setLoading(false); TurboHelpers.hideProgressBar(); - }); + } } handleOpenDialog(event:CustomEvent<{ url:string }>):void { // Trigger the dialog with custom URL - this.triggerTurboStream(event.detail.url); + void this.triggerTurboStream(event.detail.url); } - get href() { + get href():string { return (this.element as HTMLLinkElement).href; } - get method() { + get method():string { return (this.element as HTMLLinkElement).dataset.turboMethod ?? 'GET'; } + + private setLoading(loading:boolean):void { + this.loading = loading; + + if (this.disableDuringLoadValue) { + if (loading) { + this.element.setAttribute('aria-disabled', 'true'); + } else { + this.element.removeAttribute('aria-disabled'); + } + } + } } 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 3a6b55d1b8ed..a56ae2f1942a 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -1240,6 +1240,231 @@ 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(); + }); + + it('exposes batch action scopes and removes destinations occupied by every member', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + + items[0].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true })); + items[1].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true })); + const scope = controller.actionScopeFor(items[1]); + + expect(scope).toMatchObject({ kind: 'batch', items: [items[0], items[1]]}); + expect(controller.availableDestinations(scope, [ + { type: 'backlog_bucket', id: '1' }, + { type: 'sprint', id: '1' }, + ])).toEqual([{ type: 'sprint', id: '1' }]); + }); + + describe('direct destination moves', () => { + const destination = { type: 'inbox', id: null }; + + function destinationController(root:HTMLElement) { + return ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + } + + function selectItem(element:HTMLElement, init:MouseEventInit = {}):void { + element.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ...init })); + } + + function destinationSelected(element:HTMLElement):boolean { + return element.hasAttribute('data-batch-selected'); + } + + it('submits the selected scope in document order without optimistic or positional fields', async () => { + fetchMock.mockResolvedValueOnce(new Response('', { + headers: { 'Content-Type': 'text/vnd.turbo-stream.html' }, + status: 422, + })); + const { root, sourceList, items } = renderSelectableRoot({ + optimistic: true, + collectionMoveUrl: '/projects/demo/backlogs/work_packages/move', + }); + sourceList.insertBefore(items[1], items[0]); + await ctx.nextFrame(); + selectItem(items[0]); + selectItem(items[1], { ctrlKey: true }); + + destinationController(root).moveToDestination(items[0], destination); + await flushPromises(); + + const [requestUrl, requestOptions] = fetchMock.mock.lastCall as [string, { body:FormData }]; + expect(requestUrl).toBe('/projects/demo/backlogs/work_packages/move'); + expect([...requestOptions.body.entries()]).toEqual([ + ['ids[]', '2'], + ['ids[]', '1'], + ['list_type', 'inbox'], + ['list_id', ''], + ]); + expect(requestOptions.body.has('prev_id')).toBe(false); + expect(requestOptions.body.has('optimistic')).toBe(false); + expect(items.filter(destinationSelected)).toEqual([items[0], items[1]]); + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + expect(renderStreamMessageMock).toHaveBeenCalledOnce(); + }); + + it('replaces an unrelated selection when an unselected card invokes the action', async () => { + fetchMock.mockResolvedValueOnce(new Response('', { status: 422 })); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/batch/move' }); + await ctx.nextFrame(); + selectItem(items[1]); + selectItem(items[2], { ctrlKey: true }); + + destinationController(root).moveToDestination(items[0], destination); + await flushPromises(); + + const requestOptions = fetchMock.mock.lastCall?.[1] as { body:FormData }; + expect(requestOptions.body.getAll('ids[]')).toEqual(['1']); + expect(items.filter(destinationSelected)).toEqual([items[0]]); + }); + + it('does not reorder or clear selection before a successful frame stream reconciles the root', async () => { + fetchMock.mockResolvedValueOnce(new Response('', { + headers: { 'Content-Type': 'text/vnd.turbo-stream.html' }, + status: 200, + })); + const { root, sourceList, items } = renderSelectableRoot({ collectionMoveUrl: '/batch/move' }); + await ctx.nextFrame(); + selectItem(items[0]); + selectItem(items[1], { ctrlKey: true }); + + destinationController(root).moveToDestination(items[0], destination); + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + + expect(itemIds(sourceList)).toEqual(['1', '2', '3']); + expect(items.filter(destinationSelected)).toEqual([items[0], items[1]]); + expect(renderStreamMessageMock).toHaveBeenCalledOnce(); + }); + + it('does not mutate selection or submit while another move is busy', async () => { + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/batch/move' }); + await ctx.nextFrame(); + selectItem(items[1]); + root.setAttribute('data-sortable-lists-busy', 'true'); + const controller = destinationController(root); + + const prospectiveScope = controller.selectForAction(items[0]); + controller.moveToDestination(items[0], destination); + await flushPromises(); + + expect(prospectiveScope).toMatchObject({ kind: 'batch'}); + expect(items.filter(destinationSelected)).toEqual([items[1]]); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('clears a detached request busy marker when the cached root reconnects', async () => { + let resolveRequest!:(response:Response) => void; + fetchMock.mockImplementationOnce(() => new Promise((resolve) => { + resolveRequest = resolve; + })); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/batch/move' }); + await ctx.nextFrame(); + + destinationController(root).moveToDestination(items[0], destination); + expect(root.hasAttribute('data-sortable-lists-busy')).toBe(true); + + root.remove(); + await ctx.nextFrame(); + resolveRequest(new Response('', { status: 200 })); + await flushPromises(); + + // The settled request deliberately leaves a detached element alone. + expect(root.hasAttribute('data-sortable-lists-busy')).toBe(true); + + fixture.append(root); + await ctx.nextFrame(); + await ctx.nextFrame(); + + expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false); + + fetchMock.mockResolvedValueOnce(new Response('', { + headers: { 'Content-Type': 'text/vnd.turbo-stream.html' }, + status: 422, + })); + destinationController(root).moveToDestination(items[0], destination); + + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + }); + + // A destination move whose request never produces a stream (a network + // failure, or an error page) has no server flash to speak for it, so the + // client has to say something rather than just clearing the busy state. + it('dispatches an error toast when the destination request never streams', async () => { + const toastEvents:CustomEvent[] = []; + const onToast = (event:Event) => toastEvents.push(event as CustomEvent); + window.addEventListener('op:toasters:add', onToast); + fetchMock.mockRejectedValueOnce(new Error('network down')); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/batch/move' }); + await ctx.nextFrame(); + + destinationController(root).moveToDestination(items[0], destination); + await flushPromises(); + + expect(toastEvents).toHaveLength(1); + expect(toastEvents[0].detail.type).toBe('error'); + expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false); + + window.removeEventListener('op:toasters:add', onToast); + }); + + it('keeps a reconnected root busy until its detached request settles', async () => { + let resolveRequest!:(response:Response) => void; + fetchMock.mockImplementationOnce(() => new Promise((resolve) => { + resolveRequest = resolve; + })); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/batch/move' }); + await ctx.nextFrame(); + + destinationController(root).moveToDestination(items[0], destination); + expect(root.hasAttribute('data-sortable-lists-busy')).toBe(true); + + root.remove(); + await ctx.nextFrame(); + fixture.append(root); + await ctx.nextFrame(); + await ctx.nextFrame(); + + expect(root.hasAttribute('data-sortable-lists-busy')).toBe(true); + + destinationController(root).moveToDestination(items[1], destination); + await flushPromises(); + expect(fetchMock).toHaveBeenCalledOnce(); + + resolveRequest(new Response('', { status: 200 })); + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + }); + + it.each([ + ['a 500 Turbo Stream response', () => Promise.resolve(new Response('', { + headers: { 'Content-Type': 'text/vnd.turbo-stream.html' }, + status: 500, + })), 1], + ['a successful non-stream response', () => Promise.resolve(new Response('', { status: 200 })), 0], + ['a rejected request', () => Promise.reject(new Error('Network failure')), 0], + ])('clears busy state after %s', async (_description, request, streamRenderCount) => { + fetchMock.mockImplementationOnce(request); + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/batch/move' }); + await ctx.nextFrame(); + + destinationController(root).moveToDestination(items[0], destination); + expect(root.hasAttribute('data-sortable-lists-busy')).toBe(true); + await flushPromises(); + + await waitFor(() => expect(root.hasAttribute('data-sortable-lists-busy')).toBe(false)); + expect(renderStreamMessageMock).toHaveBeenCalledTimes(streamRenderCount); + }); + }); + describe('nested list topology', () => { it('resolves the source row of a nested item against its innermost list', async () => { const { fieldList, firstFieldItem } = renderNestedFixture(); @@ -1437,6 +1662,21 @@ describe('Sortable lists controller', () => { expect(row.hasAttribute('data-batch-selected')).toBe(false); }); + it('clears the live selection when a consumer reports a completed move', async () => { + const { root, items } = renderSelectableRoot(); + root.setAttribute( + 'data-action', + 'sortable-lists:test-move-completed@document->sortable-lists#clearSelectionAfterMove', + ); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { ctrlKey: true }); + + document.dispatchEvent(new CustomEvent('sortable-lists:test-move-completed')); + + expect(items.some(isSelected)).toBe(false); + }); + it('selects only the clicked card on a plain click', async () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); @@ -1987,17 +2227,17 @@ describe('Sortable lists controller', () => { expect(items.some(isSelected)).toBe(false); }); - // Escape clears local state only, so the busy gate must not swallow it. - it('clears the batch on Escape even during a busy move', async () => { + it('preserves the batch and consumes Escape during a busy move', async () => { const { root, items } = renderSelectableRoot(); await ctx.nextFrame(); click(items[0]); items[0].focus(); root.setAttribute('data-sortable-lists-busy', 'true'); - keydown(items[0], 'Escape'); + const event = keydown(items[0], 'Escape'); - expect(items.some(isSelected)).toBe(false); + expect(items.filter(isSelected)).toEqual([items[0]]); + expect(event.defaultPrevented).toBe(true); }); // An unconsumed Escape still reaches dialogs and menus. diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index 39ae97014804..cc32d62b7dfc 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -36,6 +36,7 @@ import { announce } from '@primer/live-region-element'; import { debugLog } from 'core-app/shared/helpers/debug_output'; import { OPToastEvent } from 'core-app/shared/components/toaster/toast-event'; import { flipMove } from 'core-stimulus/helpers/flip-helper'; +import { performTurboStreamRequest } from 'core-stimulus/helpers/request-helpers'; import { parseTemplate } from 'url-template'; import { buildMoveFormData, @@ -61,12 +62,14 @@ import { restoreRowPositions, rowOf, rowsRemainAt, + permittedDestinations, + sameDestination, sortableListsBusyAttribute, type DestinationIdentity, type MoveAvailability, type MoveDirection, } from './sortable-lists/list-dom'; -import { SelectionOrchestrator, type SelectionHost } from './sortable-lists/selection-orchestrator'; +import { SelectionOrchestrator, scopeIds, type ActionScope, type SelectionHost } from './sortable-lists/selection-orchestrator'; import { itemIdentity, orderedItemElements } from './sortable-lists/selection'; type CleanupFn = () => void; @@ -117,8 +120,13 @@ export default class SortableListsController extends Controller imp private monitorCleanupFn?:CleanupFn; private healScheduled = false; private reconcileScheduled = false; + private inFlightMoveRequests = 0; connect():void { + // Busy belongs to in-flight controller work, not to cached DOM markup. + // Reconnecting before settlement keeps the root blocked; reconnecting a + // stale cached root after settlement clears the marker. + this.syncBusyState(); this.monitorCleanupFn = monitorForElements({ canMonitor: ({ source }) => !this.busy && isItemFromRoot(this.element, source.data), @@ -231,6 +239,45 @@ export default class SortableListsController extends Controller imp } } + // Live ordered membership, for AGILE-278's batch move. + selectedItems():SelectionItem[] { + return this.selection?.selectedItems() ?? []; + } + + actionScopeFor(itemElement:HTMLElement):ActionScope { + return this.selection?.actionScopeFor(itemElement) ?? { kind: 'refused', items: [] }; + } + + selectForAction(itemElement:HTMLElement):ActionScope { + if (this.busy) { + return this.actionScopeFor(itemElement); + } + + return this.selection?.selectForAction(itemElement) ?? { kind: 'refused', items: [] }; + } + + // Consumer-owned non-optimistic forms do not call performMove, so their + // successful move event is the shared boundary at which the live batch is + // cleared. Failed requests emit no completion event and keep the selection. + clearSelectionAfterMove():void { + this.selection?.clearSilently(); + } + + // Where the batch may move: the candidates every member accepts, minus the + // one they all already occupy, which would be a move to nowhere. + availableDestinations(scope:ActionScope, candidates:DestinationIdentity[]):DestinationIdentity[] { + if (scope.kind === 'refused') { + return []; + } + + const ownerDestinationOf = (item:HTMLElement) => this.ownerDestinationOf(item); + const permitted = permittedDestinations({ items: scope.items, candidates, ownerDestinationOf }); + + return permitted.filter((target) => ( + !scope.items.every((item) => sameDestination(ownerDestinationOf(item), target)) + )); + } + // 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. @@ -459,6 +506,29 @@ export default class SortableListsController extends Controller imp return list ? resolveMoveAvailability({ itemElement, rowsContainer: list.rowsContainer }) : null; } + moveToDestination(itemElement:HTMLElement, target:DestinationIdentity):void { + if (this.busy) { + return; + } + + const moveUrl = this.resolveCollectionMoveUrl(false); + if (!moveUrl) { + return; + } + + const scope = this.selectForAction(itemElement); + if (scope.kind === 'refused') { + return; + } + + const body = new FormData(); + scopeIds(scope).forEach((id) => body.append('ids[]', id)); + body.append('list_type', target.type); + body.append('list_id', target.id ?? ''); + + void this.submitDestinationMove(moveUrl, body); + } + moveInDirection(itemElement:HTMLElement, direction:MoveDirection):void { // The menu is rendered server-side from a permission check that does not // know about per-work-package movability, so a stale or over-permissive @@ -510,6 +580,10 @@ export default class SortableListsController extends Controller imp return containing.find((list) => !containing.some((other) => other !== list && list.element.contains(other.element))) ?? null; } + ownerListElementOf(itemElement:HTMLElement):HTMLElement|null { + return this.ownerListOf(itemElement)?.element ?? null; + } + ownerRowsContainer(itemElement:HTMLElement):HTMLElement|null { return this.ownerListOf(itemElement)?.rowsContainer ?? null; } @@ -598,20 +672,46 @@ export default class SortableListsController extends Controller imp return this.hasCollectionMoveUrlValue && this.collectionMoveUrlValue !== '' ? this.collectionMoveUrlValue : null; } - private resolveCollectionMoveUrl():string|null { + private resolveCollectionMoveUrl(optimistic = this.optimisticValue):string|null { const collectionMoveUrl = this.collectionMoveUrl; if (!collectionMoveUrl) { return null; } const url = new URL(collectionMoveUrl, window.location.href); - if (this.optimisticValue) { + if (optimistic) { url.searchParams.set('optimistic', 'true'); + } else { + url.searchParams.delete('optimistic'); } return relativeUrl(url); } + private async submitDestinationMove(moveUrl:string, body:FormData):Promise { + const request = new FetchRequest( + 'put', + moveUrl, + { + body, + responseKind: 'turbo-stream', + }, + ); + + this.startMoveRequest(); + try { + await performTurboStreamRequest(request); + } catch (error) { + // Only a request that never produced a stream lands here — a rejection + // streams its own flash. Without the toast the busy state would simply + // clear and the batch would look moved. + debugLog('Failed to move sortable list items to destination', error); + this.dispatchErrorToast(); + } finally { + this.finishMoveRequest(); + } + } + // 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 { @@ -766,7 +866,7 @@ export default class SortableListsController extends Controller imp }, ); - this.setBusy(true); + this.startMoveRequest(); try { const response = await request.perform(); @@ -781,7 +881,26 @@ export default class SortableListsController extends Controller imp debugLog('Failed to move sortable list item due to request error', error); return { ok: false, showToast: true }; } finally { - this.setBusy(false); + this.finishMoveRequest(); + } + } + + private startMoveRequest():void { + this.inFlightMoveRequests += 1; + this.syncBusyState(); + } + + private finishMoveRequest():void { + this.inFlightMoveRequests = Math.max(0, this.inFlightMoveRequests - 1); + this.syncBusyState(); + } + + private syncBusyState():void { + // A successful frame stream may already have replaced this root. Avoid + // mutating detached cached DOM; connect() will project the current count + // if this element is restored later. + if (this.element.isConnected) { + this.setBusy(this.inFlightMoveRequests > 0); } } 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 cee21892fb92..ac55430ddaa6 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 @@ -55,6 +55,7 @@ import { type MoveAvailability, type MoveDirection, } from './list-dom'; +import type { ActionScope } from './selection-orchestrator'; // The Pragmatic DnD payloads exchanged between the sortable-lists root and // item controllers, built on top of the DOM contract in list-dom.ts. @@ -102,9 +103,16 @@ export interface SortableListData extends Record { export interface SortableListsRoot { readonly element:HTMLElement; readonly busy:boolean; + actionScopeFor(itemElement:HTMLElement):ActionScope; + selectForAction(itemElement:HTMLElement):ActionScope; + availableDestinations(scope:ActionScope, candidates:DestinationIdentity[]):DestinationIdentity[]; + moveToDestination(itemElement:HTMLElement, target:DestinationIdentity):void; moveInDirection(itemElement:HTMLElement, direction:MoveDirection):void; // A snapshot for menu gating; the click path re-resolves against the live DOM. 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; 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 991bc87c4160..00d9a5e0814b 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 @@ -58,6 +58,7 @@ 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'; +import type { ActionScope } from './selection-orchestrator'; describe('Sortable lists item controller', () => { let draggable:typeof draggableFn; @@ -104,8 +105,13 @@ describe('Sortable lists item controller', () => { return { element, busy, + actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + availableDestinations: vi.fn(() => []), + moveToDestination: vi.fn(), moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), + ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(ownerRowsContainer), freezeDragBatch: vi.fn(() => 1), markDragBatch: vi.fn(), @@ -1118,8 +1124,13 @@ describe('Sortable lists item controller', () => { controller.connectRoot({ element: row, busy: false, + actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + availableDestinations: vi.fn(() => []), + moveToDestination: vi.fn(), moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), + ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), freezeDragBatch: vi.fn(() => 3), markDragBatch: vi.fn(), @@ -1211,8 +1222,13 @@ describe('Sortable lists item controller', () => { controller.connectRoot({ element: row, busy: false, + actionScopeFor: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + availableDestinations: vi.fn(() => []), + moveToDestination: vi.fn(), moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), + ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), freezeDragBatch: vi.fn(() => 3), markDragBatch: vi.fn(), @@ -1346,13 +1362,13 @@ describe('Sortable lists item controller', () => { const menuElement = document.createElement('action-menu'); // The divider opens the move group, so everything below it is what // decides whether it still separates anything. - menuElement.innerHTML = (withDivider ? '
  • ' : '') - + ['top', 'up', 'down', 'bottom'].map((direction) => ( - `
  • ' - )).join(''); + menuElement.innerHTML = withDivider ? '
  • ' : ''; const parent = document.createElement('li'); parent.setAttribute('data-sortable-lists--item-target', 'moveMenu'); + parent.innerHTML = ['top', 'up', 'down', 'bottom'].map((direction) => ( + `
  • ' + )).join(''); menuElement.appendChild(parent); el.appendChild(menuElement); @@ -1372,6 +1388,16 @@ describe('Sortable lists item controller', () => { } const liFor = (el:HTMLElement, direction:string) => el.querySelector(`li[data-sortable-lists--item-direction-param="${direction}"]`)!; + const destinationWithMetadata = (el:HTMLElement, metadata:string) => { + const item = document.createElement('li'); + item.setAttribute('data-sortable-lists--item-target', 'destinationItem'); + item.dataset.sortableListsDestinations = metadata; + el.querySelector('action-menu')!.append(item); + return item; + }; + const destinationFor = (el:HTMLElement, candidates:{ type:string; id:string|null }[]) => ( + destinationWithMetadata(el, JSON.stringify(candidates)) + ); // Availability defaults to the first/last extremes so the position-driven // specs read naturally; individual tests can override the map to exercise // the marker-aware (truncated list) wiring. @@ -1387,9 +1413,24 @@ describe('Sortable lists item controller', () => { moveInDirection = vi.fn(), availability = availabilityFromPosition(position), ) => ({ - element: el, busy: false, moveAvailability: () => availability, moveInDirection, + element: el, + busy: false, + actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + availableDestinations: vi.fn(() => []), + moveToDestination: vi.fn(), + moveAvailability: () => availability, + moveInDirection, } as unknown as SortableListsRoot); + function stubMenuRoot(el:HTMLElement, position:{ isFirst:boolean; isLast:boolean }) { + const actionScopeFor = vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })); + const availableDestinations = vi.fn((_scope:ActionScope, _candidates:DestinationIdentity[]):DestinationIdentity[] => []); + const root = { ...stubRoot(el, position), actionScopeFor, availableDestinations }; + + return { root, actionScopeFor, availableDestinations }; + } + it('hides up/top for a first item and shows the rest', async () => { const { el, menu } = renderItemWithMenu(1); document.body.appendChild(el); @@ -1444,6 +1485,146 @@ describe('Sortable lists item controller', () => { expect(menu.hideItem).toHaveBeenCalledWith(parent); }); + it('projects deferred destination items for the selected invoker', async () => { + const { el, menu } = renderItemWithMenu(1, true); + document.body.appendChild(el); + const controller = await mountItemController(el); + const scope:ActionScope = { kind: 'batch', items: [el]}; + const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); + actionScopeFor.mockReturnValue(scope); + availableDestinations.mockImplementation((_scope, candidates) => ( + candidates.filter((candidate) => candidate.type === 'inbox') + )); + controller.connectRoot(root); + + const moveToSprint = destinationFor(el, [{ type: 'sprint', id: '1' }]); + const moveToInbox = destinationFor(el, [{ type: 'inbox', id: null }]); + await menuCtx!.nextFrame(); + + expect(actionScopeFor).toHaveBeenCalledWith(el); + expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); + expect(menu.showItem).toHaveBeenCalledWith(moveToInbox); + }); + + it('recomputes selected multi-card and prospective one-card scopes whenever the menu opens', async () => { + const { el, menu } = renderItemWithMenu(1); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); + const selectedPeer = document.createElement('div'); + selectedPeer.setAttribute('data-sortable-lists--item-id-value', '2'); + selectedPeer.setAttribute('data-sortable-lists--item-mobility-value', 'free'); + const selectedScope:ActionScope = { kind: 'batch', items: [el, selectedPeer]}; + const prospectiveScope:ActionScope = { kind: 'batch', items: [el]}; + let activeScope = selectedScope; + actionScopeFor.mockImplementation(() => activeScope); + availableDestinations.mockImplementation((scope, candidates) => ( + candidates.filter((candidate) => candidate.type === (scope === selectedScope ? 'sprint' : 'inbox')) + )); + controller.connectRoot(root); + + const moveToSprint = destinationFor(el, [{ type: 'sprint', id: '1' }]); + const moveToInbox = destinationFor(el, [{ type: 'inbox', id: null }]); + await menuCtx!.nextFrame(); + + expect(availableDestinations).toHaveBeenCalledWith(selectedScope, [{ type: 'sprint', id: '1' }]); + expect(availableDestinations).toHaveBeenCalledWith(selectedScope, [{ type: 'inbox', id: null }]); + expect(menu.showItem).toHaveBeenCalledWith(moveToSprint); + expect(menu.hideItem).toHaveBeenCalledWith(moveToInbox); + menu.hideItem.mockClear(); + menu.showItem.mockClear(); + activeScope = prospectiveScope; + + const menuElement = el.querySelector('action-menu')!; + const toggle = new ToggleEvent('toggle', { newState: 'open', oldState: 'closed' }); + menuElement.dispatchEvent(toggle); + + expect(actionScopeFor).toHaveBeenLastCalledWith(el); + expect(availableDestinations).toHaveBeenCalledWith(prospectiveScope, [{ type: 'sprint', id: '1' }]); + expect(availableDestinations).toHaveBeenCalledWith(prospectiveScope, [{ type: 'inbox', id: null }]); + expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); + expect(menu.showItem).toHaveBeenCalledWith(moveToInbox); + }); + + it('hides destination actions and the position submenu for a true multi-card scope', async () => { + const { el, menu } = renderItemWithMenu(1, true); + document.body.appendChild(el); + const controller = await mountItemController(el); + // Batch size is the member count: two elements, not one element with + // two ids. + const scope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); + actionScopeFor.mockReturnValue(scope); + availableDestinations.mockReturnValue([]); + controller.connectRoot(root); + + const moveToSprint = destinationFor(el, [{ type: 'sprint', id: '1' }]); + const moveToInbox = destinationFor(el, [{ type: 'inbox', id: null }]); + await menuCtx!.nextFrame(); + + const moveMenu = el.querySelector('li[data-sortable-lists--item-target="moveMenu"]')!; + const divider = el.querySelector('li[data-sortable-lists--item-target="moveDivider"]')!; + expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); + expect(menu.hideItem).toHaveBeenCalledWith(moveToInbox); + expect(menu.hideItem).toHaveBeenCalledWith(moveMenu); + expect(menu.hideItem).not.toHaveBeenCalledWith(liFor(el, 'top')); + expect(divider.hasAttribute('hidden')).toBe(true); + }); + + it('keeps only the current owner destination for a confined batch scope', async () => { + const { el, menu } = renderItemWithMenu(1); + el.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); + const scope:ActionScope = { kind: 'batch', items: [el]}; + actionScopeFor.mockReturnValue(scope); + availableDestinations.mockImplementation((_scope, candidates) => candidates.filter((candidate) => candidate.id === '12')); + controller.connectRoot(root); + + const moveToSprint = destinationFor(el, [{ type: 'sprint', id: '12' }, { type: 'sprint', id: '13' }]); + await menuCtx!.nextFrame(); + + expect(availableDestinations).toHaveBeenCalledWith(scope, [{ type: 'sprint', id: '12' }, { type: 'sprint', id: '13' }]); + expect(menu.showItem).toHaveBeenCalledWith(moveToSprint); + }); + + it('hides batch destinations for a fixed synthetic singular scope', async () => { + const { el, menu } = renderItemWithMenu(1); + el.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); + const scope:ActionScope = { kind: 'refused', items: [] }; + actionScopeFor.mockReturnValue(scope); + availableDestinations.mockReturnValue([]); + controller.connectRoot(root); + + const moveToSprint = destinationFor(el, [{ type: 'sprint', id: '12' }]); + await menuCtx!.nextFrame(); + + expect(availableDestinations).toHaveBeenCalledWith(scope, [{ type: 'sprint', id: '12' }]); + expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); + }); + + it.each([ + ['invalid JSON', '{invalid'], + ['non-array JSON', '{"type":"sprint","id":"12"}'], + ['a malformed candidate member', '[{"type":"sprint","id":"12"},{"type":"sprint"}]'], + ])('fails closed for %s destination metadata', async (_description, metadata) => { + const { el, menu } = renderItemWithMenu(1); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); + controller.connectRoot(root); + + const destination = destinationWithMetadata(el, metadata); + await menuCtx!.nextFrame(); + + expect(menu.hideItem).toHaveBeenCalledWith(destination); + expect(availableDestinations).not.toHaveBeenCalled(); + }); + // Regression: the divider is rendered server-side from a permission check // alone, so an item with nowhere to move used to be left with a separator // and nothing below it. @@ -1515,7 +1696,7 @@ describe('Sortable lists item controller', () => { expect(() => { controller.connectRoot(stubRoot(el, { isFirst: true, isLast: true }, moveInDirection)); // @ts-expect-error exercising the guard directly - controller.refreshMoveMenuAvailability?.(); + controller.refreshActionAvailability?.(); }).not.toThrow(); // move() must also no-op rather than throw: there is no menu to read @@ -1528,6 +1709,113 @@ describe('Sortable lists item controller', () => { }); }); + describe('destination activation', () => { + it('exposes direct destination movement as a required root capability', () => { + const moveToDestination = vi.fn(); + const root:SortableListsRoot = { ...fakeRoot(), moveToDestination }; + const item = document.createElement('div'); + + root.moveToDestination(item, { type: 'inbox', id: null }); + + expect(moveToDestination).toHaveBeenCalledWith(item, { type: 'inbox', id: null }); + }); + + it('replaces stale generated inputs with the current action scope in document order', () => { + const element = document.createElement('div'); + const [first, second] = ['2', '1'].map((id) => { + const member = document.createElement('div'); + member.setAttribute('data-sortable-lists--item-id-value', id); + return member; + }); + const selectForAction = vi.fn(():ActionScope => ({ kind: 'batch', items: [first, second] })); + const controller = connectedControllerFor(element, { + root: { ...fakeRoot(), selectForAction }, + }); + const form = document.createElement('form'); + form.innerHTML = ''; + const event = new CustomEvent('async-dialog:beforeLoad', { + cancelable: true, + detail: { form }, + }); + + controller.prepareDialog(event); + + expect(selectForAction).toHaveBeenCalledWith(element); + expect(event.defaultPrevented).toBe(false); + expect(Array.from(form.elements).map((input) => [(input as HTMLInputElement).name, (input as HTMLInputElement).value])).toEqual([ + ['kept', 'yes'], + ['ids[]', '2'], + ['ids[]', '1'], + ]); + expect(form.querySelectorAll('[data-sortable-lists-generated-id]')).toHaveLength(2); + }); + + it('cancels dialog loading without resolving or mutating the action scope while the root is busy', () => { + const element = document.createElement('div'); + const selectForAction = vi.fn(():ActionScope => ({ kind: 'batch', items: [element] })); + const controller = connectedControllerFor(element, { + root: { ...fakeRoot(), busy: true, selectForAction }, + }); + const form = document.createElement('form'); + form.innerHTML = ''; + const staleInput = form.querySelector('[data-sortable-lists-generated-id]'); + const event = new CustomEvent<{ form:HTMLFormElement|null }>('async-dialog:beforeLoad', { + cancelable: true, + detail: { form }, + }); + + controller.prepareDialog(event); + + expect(event.defaultPrevented).toBe(true); + expect(selectForAction).not.toHaveBeenCalled(); + expect(form.querySelector('[data-sortable-lists-generated-id]')).toBe(staleInput); + expect(Array.from(form.elements).map((input) => [(input as HTMLInputElement).name, (input as HTMLInputElement).value])).toEqual([ + ['ids[]', 'stale'], + ['kept', 'yes'], + ]); + }); + + it('cancels dialog loading for a fixed invoker with no batch action scope', () => { + const element = document.createElement('div'); + element.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const selectForAction = vi.fn(():ActionScope => ({ kind: 'refused', items: [] })); + const controller = connectedControllerFor(element, { + root: { ...fakeRoot(), selectForAction }, + }); + const form = document.createElement('form'); + const event = new CustomEvent('async-dialog:beforeLoad', { + cancelable: true, + detail: { form }, + }); + + controller.prepareDialog(event); + + expect(selectForAction).toHaveBeenCalledWith(element); + expect(event.defaultPrevented).toBe(true); + expect(form.querySelector('[name="ids[]"]')).toBeNull(); + }); + + it('delegates direct destination metadata to the root', () => { + const element = document.createElement('div'); + const moveToDestination = vi.fn(); + const controller = connectedControllerFor(element, { + root: { ...fakeRoot(), moveToDestination }, + }); + Object.defineProperty(controller, 'hasMenuElement', { value: true }); + Object.defineProperty(controller, 'menuElement', { + value: { isItemDisabled: vi.fn(() => false), isItemHidden: vi.fn(() => false) }, + }); + const item = document.createElement('li'); + item.dataset.sortableListsDestinations = '[{"type":"inbox","id":null}]'; + const event = new Event('click') as ActionEvent; + Object.defineProperty(event, 'currentTarget', { value: item }); + + controller.moveToDestination(event); + + expect(moveToDestination).toHaveBeenCalledWith(element, { type: 'inbox', id: null }); + }); + }); + describe('movability and focus', () => { let ctx:StimulusTestContext; @@ -1621,8 +1909,13 @@ describe('Sortable lists item controller', () => { const root:SortableListsRoot = { element: item, busy: false, + actionScopeFor: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + availableDestinations: vi.fn(() => []), + moveToDestination: vi.fn(), moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), + ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), freezeDragBatch: vi.fn(() => 1), markDragBatch, @@ -1649,8 +1942,13 @@ describe('Sortable lists item controller', () => { const root:SortableListsRoot = { element: item, busy: false, + actionScopeFor: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn((actionItem:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + availableDestinations: vi.fn(() => []), + moveToDestination: vi.fn(), moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), + ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), freezeDragBatch, markDragBatch: vi.fn(), diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index 1c83da470b66..5663d551926b 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -57,13 +57,26 @@ import { resolveItemExternalUrl, resolveItemLabel, sortableItemSelector, + type DestinationIdentity, } from './list-dom'; import { renderDragPreview } from './preview'; +import { scopeIds, type ActionScope } from './selection-orchestrator'; type CleanupFn = () => void; +function isDestinationIdentity(candidate:unknown):candidate is DestinationIdentity { + if (typeof candidate !== 'object' || candidate === null) { + return false; + } + + return 'type' in candidate + && 'id' in candidate + && typeof candidate.type === 'string' + && (typeof candidate.id === 'string' || candidate.id === null); +} + export default class ItemController extends Controller implements RootAwareChild { - static targets = ['handle', 'preview', 'moveItem', 'moveMenu', 'moveDivider', 'focus']; + static targets = ['handle', 'preview', 'destinationItem', 'moveItem', 'moveMenu', 'moveDivider', 'focus']; static elements = { menu: 'action-menu' }; static values = { @@ -88,6 +101,7 @@ export default class ItemController extends Controller implements R declare readonly hasHandleTarget:boolean; declare readonly previewTarget:HTMLElement; declare readonly hasPreviewTarget:boolean; + declare readonly destinationItemTargets:HTMLElement[]; declare readonly moveItemTargets:HTMLElement[]; declare readonly moveMenuTarget:HTMLElement; declare readonly hasMoveMenuTarget:boolean; @@ -113,7 +127,7 @@ export default class ItemController extends Controller implements R // `instanceof ToggleEvent` so a browser without the ToggleEvent global // cannot throw. if ((event as ToggleEvent).newState === 'open') { - this.refreshMoveMenuAvailability(); + this.refreshActionAvailability(); } }; @@ -134,15 +148,19 @@ export default class ItemController extends Controller implements R this.disconnectRoot(); } - // A move item entering the DOM (inline or via a deferred fragment) triggers - // an availability refresh here, but `this.root` is usually still unset at + // An action item entering the DOM (inline or via a deferred fragment) + // triggers an availability refresh here, but `this.root` is usually unset at // this point (the outlet's connectRoot callback runs later), so this call // typically no-ops. The menu-open toggle handler is what actually // establishes correct availability, refreshing on every open once the root // is connected and after any reorder has shifted siblings. No // include-fragment knowledge, so both hooks work for any menu. moveItemTargetConnected():void { - this.refreshMoveMenuAvailability(); + this.refreshActionAvailability(); + } + + destinationItemTargetConnected():void { + this.refreshActionAvailability(); } move(event:ActionEvent):void { @@ -161,6 +179,47 @@ export default class ItemController extends Controller implements R } } + prepareDialog(event:CustomEvent<{ form:HTMLFormElement|null }>):void { + const root = this.root; + if (!root || root.busy) { + event.preventDefault(); + return; + } + + const scope = root.selectForAction(this.element); + const form = event.detail.form; + if (!form || !scope || scope.kind === 'refused') { + event.preventDefault(); + return; + } + + form.querySelectorAll('[data-sortable-lists-generated-id]').forEach((input) => input.remove()); + scopeIds(scope).forEach((id) => { + const input = form.ownerDocument.createElement('input'); + input.type = 'hidden'; + input.name = 'ids[]'; + input.value = id; + input.dataset.sortableListsGeneratedId = ''; + form.append(input); + }); + } + + moveToDestination(event:ActionEvent):void { + const item = event.currentTarget; + if (!this.hasMenuElement || !(item instanceof HTMLElement)) { + return; + } + + if (this.menuElement.isItemDisabled(item) || this.menuElement.isItemHidden(item)) { + return; + } + + const candidates = this.destinationCandidates(item); + if (candidates.length === 1) { + this.root?.moveToDestination(this.element, candidates[0]); + } + } + // The focus host is the consumer's business: Backlogs puts the tab stop on // the card inside the row, another consumer may focus the row itself. focusItem():void { @@ -458,12 +517,47 @@ export default class ItemController extends Controller implements R this.dropIndicatorElement = undefined; } - private refreshMoveMenuAvailability():void { + private refreshActionAvailability():void { const root = this.root; if (!root || !this.hasMenuElement) { return; } + const scope = root.actionScopeFor(this.element); + this.refreshDestinationAvailability(root, scope); + this.refreshMoveMenuAvailability(root, scope); + this.refreshMoveDivider(); + } + + private refreshDestinationAvailability(root:SortableListsRoot, scope:ActionScope):void { + for (const item of this.destinationItemTargets) { + const candidates = this.destinationCandidates(item); + this.setAvailability(item, candidates.length > 0 && root.availableDestinations(scope, candidates).length > 0); + } + } + + private destinationCandidates(item:HTMLElement):DestinationIdentity[] { + try { + const candidates:unknown = JSON.parse(item.dataset.sortableListsDestinations ?? ''); + if (!Array.isArray(candidates)) { + return []; + } + + const destinations = candidates.filter(isDestinationIdentity); + return destinations.length === candidates.length ? destinations : []; + } catch { + return []; + } + } + + private refreshMoveMenuAvailability(root:SortableListsRoot, scope:ActionScope):void { + if (scope.kind === 'batch' && scope.items.length > 1) { + if (this.hasMoveMenuTarget) { + this.setAvailability(this.moveMenuTarget, false); + } + return; + } + // Null availability means the item is not in a list yet; leave the menu // alone until the outlet wiring settles. const availability = root.moveAvailability(this.element); @@ -486,8 +580,6 @@ export default class ItemController extends Controller implements R if (this.hasMoveMenuTarget) { this.setAvailability(this.moveMenuTarget, available > 0); } - - this.refreshMoveDivider(); } // The divider that opens the move group is rendered server-side from a 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 7fe83a4f7a2d..84a6331051d9 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 @@ -31,6 +31,7 @@ import { isOrderableItem, itemAcceptsDestination, itemMobility, + permittedDestinations, reorderRows, sortableItemMobilityAttribute, resolveDirectionalPreviousItemId, @@ -43,6 +44,7 @@ import { restoreRowPositions, rowOf, rowsRemainAt, + sameDestination, } from './list-dom'; describe('sortable lists DOM helpers', () => { @@ -455,6 +457,72 @@ describe('itemAcceptsDestination', () => { }); }); +describe('destination intersection', () => { + const inbox = { type: 'inbox', id: null }; + const sprint1 = { type: 'sprint', id: '1' }; + const sprint2 = { type: 'sprint', id: '2' }; + const bucket1 = { type: 'bucket', id: '1' }; + + function item(mobility:'fixed'|'confined'|'free' = 'free'):HTMLElement { + const element = document.createElement('li'); + element.setAttribute(sortableItemMobilityAttribute, mobility); + return element; + } + + it('permits every candidate for free-only members', () => { + const freeOne = item(); + const freeTwo = item(); + + expect(permittedDestinations({ + items: [freeOne, freeTwo], + candidates: [inbox, sprint1, sprint2, bucket1], + ownerDestinationOf: () => null, + })).toEqual([inbox, sprint1, sprint2, bucket1]); + }); + + it('intersects a confined member with free members', () => { + const confinedInSprint = item('confined'); + const freeInBucket = item(); + const ownerDestinationOf = (member:HTMLElement) => member === confinedInSprint ? sprint1 : bucket1; + + expect(permittedDestinations({ + items: [confinedInSprint, freeInBucket], + candidates: [inbox, sprint1, sprint2, bucket1], + ownerDestinationOf, + })).toEqual([sprint1]); + }); + + it('rejects confined members belonging to different destinations', () => { + const confinedInSprint = item('confined'); + const confinedInBucket = item('confined'); + const ownerDestinationOf = (member:HTMLElement) => member === confinedInSprint ? sprint1 : bucket1; + + expect(permittedDestinations({ + items: [confinedInSprint, confinedInBucket], + candidates: [inbox, sprint1, bucket1], + ownerDestinationOf, + })).toEqual([]); + }); + + it('rejects every destination when a member is fixed', () => { + expect(permittedDestinations({ + items: [item('fixed'), item()], + candidates: [inbox, sprint1], + ownerDestinationOf: () => null, + })).toEqual([]); + }); + + it('identifies targets occupied by every member independently of permission', () => { + const freeInSprint = item(); + const anotherFreeInSprint = item(); + const items = [freeInSprint, anotherFreeInSprint]; + const ownerDestinationOf = (_member:HTMLElement) => sprint1; + const permitted = permittedDestinations({ items, candidates: [sprint1, sprint2], ownerDestinationOf }); + + expect(permitted.filter((target) => !items.every((member) => sameDestination(ownerDestinationOf(member), target)))).toEqual([sprint2]); + }); +}); + describe('directional move helpers', () => { function container(ids:string[]):HTMLElement { const ul = document.createElement('ul'); 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 954090e46449..45ec856aade2 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -142,6 +142,24 @@ export function itemAcceptsDestination( } } +export function permittedDestinations({ + items, + candidates, + ownerDestinationOf, +}:{ + items:HTMLElement[]; + candidates:DestinationIdentity[]; + ownerDestinationOf:(item:HTMLElement) => DestinationIdentity|null; +}):DestinationIdentity[] { + if (items.length === 0 || items.some((item) => itemMobility(item) === 'fixed')) { + return []; + } + + return candidates.filter((target) => ( + items.every((item) => itemMobility(item) === 'free' || sameDestination(ownerDestinationOf(item), target)) + )); +} + export function resolveItemType(element:Element):string|null { const type = element.getAttribute(sortableItemTypeAttribute); 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 027d6882f171..ca03ea4eda9f 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 @@ -37,6 +37,7 @@ import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test- import type ListControllerType from './list.controller'; import type { sortableItemData as sortableItemDataFn, SortableListsRoot } from './drag-and-drop'; import type { DestinationIdentity } from './list-dom'; +import type { ActionScope } from './selection-orchestrator'; // 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 @@ -75,8 +76,13 @@ describe('Sortable lists list controller', () => { return { element, busy, + actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + availableDestinations: vi.fn(() => []), + moveToDestination: vi.fn(), moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), + ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), freezeDragBatch: vi.fn(() => 1), markDragBatch: vi.fn(), diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts index 7436950072f0..f551249f0d65 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 @@ -30,6 +30,7 @@ import { type autoScrollForElements as autoScrollForElementsFn } from '@atlaskit import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; import type ScrollableControllerType from './scrollable.controller'; import type { sortableItemData as sortableItemDataFn, SortableListsRoot } from './drag-and-drop'; +import type { ActionScope } from './selection-orchestrator'; vi.mock('@atlaskit/pragmatic-drag-and-drop-auto-scroll/element', () => ({ autoScrollForElements: vi.fn(() => vi.fn()), @@ -70,8 +71,13 @@ describe('Sortable lists scrollable controller', () => { return { element, busy: false, + actionScopeFor: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + selectForAction: vi.fn((item:HTMLElement):ActionScope => ({ kind: 'refused', items: [] })), + availableDestinations: vi.fn(() => []), + moveToDestination: vi.fn(), moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), + ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), freezeDragBatch: vi.fn(() => 1), markDragBatch: vi.fn(), 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 6c3499f1221c..baf23d266c53 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -64,6 +64,15 @@ export type ActionScope = | { kind:'batch'; items:HTMLElement[] } | { kind:'refused'; items:[] }; +// The ids a scope's members carry, in the scope's own order. Derived on +// demand rather than frozen into the scope: the elements are its identity, +// and a stale id list would outlive a morph that replaced a row. +export function scopeIds(scope:ActionScope):string[] { + return scope.items + .map((item) => resolveItemId(item)) + .filter((id):id is string => id !== null); +} + // 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'; @@ -405,6 +414,11 @@ export class SelectionOrchestrator { return; } + if (this.host.busy) { + event.preventDefault(); + return; + } + event.preventDefault(); escapesClearedBySelection.add(event); this.selection.clear(); diff --git a/frontend/src/stimulus/helpers/request-helpers.spec.ts b/frontend/src/stimulus/helpers/request-helpers.spec.ts index 0fd3d0427112..c9daf0cb851a 100644 --- a/frontend/src/stimulus/helpers/request-helpers.spec.ts +++ b/frontend/src/stimulus/helpers/request-helpers.spec.ts @@ -26,10 +26,60 @@ // See COPYRIGHT and LICENSE files for more details. //++ -import type { FetchResponse } from '@rails/request.js'; +import type { FetchRequest, FetchResponse } from '@rails/request.js'; import { TurboHelpers } from 'core-turbo/helpers'; -import { withLoadingIndicator, withProgressBar } from './request-helpers'; +import { performTurboStreamRequest, withLoadingIndicator, withProgressBar } from './request-helpers'; + +describe('performTurboStreamRequest', () => { + function requestFor(response:Partial):FetchRequest { + return { + perform: vi.fn().mockResolvedValue(response), + } as unknown as FetchRequest; + } + + it.each([ + ['successful', { ok: true, unprocessableEntity: false }], + ['unprocessable', { ok: false, unprocessableEntity: true }], + ])('leaves request.js to render a %s stream', async (_name, status) => { + const renderTurboStream = vi.fn(); + const response = { + ...status, + isTurboStream: true, + renderTurboStream, + } as unknown as FetchResponse; + + await expect(performTurboStreamRequest(requestFor(response))).resolves.toBe(response); + expect(renderTurboStream).not.toHaveBeenCalled(); + }); + + it('renders another unsuccessful Turbo Stream exactly once', async () => { + const renderTurboStream = vi.fn().mockResolvedValue(undefined); + const response = { + ok: false, + unprocessableEntity: false, + isTurboStream: true, + renderTurboStream, + } as unknown as FetchResponse; + + await performTurboStreamRequest(requestFor(response)); + expect(renderTurboStream).toHaveBeenCalledOnce(); + }); + + it('rejects a non-Turbo response without rendering it', async () => { + const renderTurboStream = vi.fn(); + const response = { + ok: true, + unprocessableEntity: false, + isTurboStream: false, + renderTurboStream, + } as unknown as FetchResponse; + + await expect(performTurboStreamRequest(requestFor(response))) + .rejects.toThrow('Response is not a Turbo Stream'); + expect(renderTurboStream).not.toHaveBeenCalled(); + }); +}); describe('withLoadingIndicator', () => { let indicator:HTMLElement|null; diff --git a/frontend/src/stimulus/helpers/request-helpers.ts b/frontend/src/stimulus/helpers/request-helpers.ts index a63f64b1aae8..a07f311e827b 100644 --- a/frontend/src/stimulus/helpers/request-helpers.ts +++ b/frontend/src/stimulus/helpers/request-helpers.ts @@ -36,6 +36,20 @@ export function post(url:string|URL, options?:Options) { return withLoadingIndicator(request.perform()); } +export async function performTurboStreamRequest(request:FetchRequest):Promise { + const response = await request.perform(); + + if (!response.isTurboStream) { + throw new Error('Response is not a Turbo Stream'); + } + + if (!response.ok && !response.unprocessableEntity) { + await response.renderTurboStream(); + } + + return response; +} + export function withLoadingIndicator(request:Promise) { const loadingIndicator = document.querySelector('#global-loading-indicator'); invariant(loadingIndicator, 'Expected an Element with id global-loading-indicator to be present'); diff --git a/modules/backlogs/app/components/backlogs/move_to_bucket_dialog_component.html.erb b/modules/backlogs/app/components/backlogs/move_to_bucket_dialog_component.html.erb index 366c690a9e72..2afb6182e796 100644 --- a/modules/backlogs/app/components/backlogs/move_to_bucket_dialog_component.html.erb +++ b/modules/backlogs/app/components/backlogs/move_to_bucket_dialog_component.html.erb @@ -27,26 +27,31 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -<%= - render(Primer::Alpha::Dialog.new(title: t(".title"), id: DIALOG_ID, size: :medium)) do |d| - d.with_header(variant: :large) - - d.with_body do - available_buckets = buckets - bucket_list_type = list_type +<%= render(Primer::Alpha::Dialog.new(title: t(".title"), id: DIALOG_ID, size: :medium_portrait)) do |d| %> + <% d.with_header(variant: :large) %> + <% d.with_body do %> + <%= primer_form_with( url: move_action, method: :put, id: FORM_ID, data: { turbo_stream: true } ) do |f| + selected_work_packages = work_packages + available_buckets = buckets + bucket_list_type = destination_list_type + render_inline_form(f) do |form| + selected_work_packages.each do |work_package| + form.hidden(name: "ids[]", value: work_package.id) + end form.hidden(name: "list_type", value: bucket_list_type) form.select_list( name: "list_id", label: BacklogBucket.human_attribute_name(:name), - visually_hide_label: true + visually_hide_label: true, + aria: { describedby: SELECTION_LABEL_ID } ) do |select| available_buckets.each do |bucket| select.option(label: bucket.name, value: bucket.id) @@ -54,9 +59,21 @@ See COPYRIGHT and LICENSE files for more details. end end end - end + %> + + <%= + render( + Backlogs::SelectedWorkPackagesComponent.new( + work_packages:, + description_id: SELECTION_LABEL_ID, + mt: 3 + ) + ) + %> + <% end %> - d.with_footer(mt: 2) do + <% d.with_footer(show_divider: true) do %> + <%= component_collection do |buttons| buttons.with_component(Primer::Beta::Button.new(data: { close_dialog_id: DIALOG_ID })) do t(:button_cancel) @@ -68,6 +85,6 @@ See COPYRIGHT and LICENSE files for more details. t(:button_move) end end - end - end -%> + %> + <% end %> +<% end %> diff --git a/modules/backlogs/app/components/backlogs/move_to_bucket_dialog_component.rb b/modules/backlogs/app/components/backlogs/move_to_bucket_dialog_component.rb index 14046c5bccbf..28f824cb0ff7 100644 --- a/modules/backlogs/app/components/backlogs/move_to_bucket_dialog_component.rb +++ b/modules/backlogs/app/components/backlogs/move_to_bucket_dialog_component.rb @@ -35,22 +35,21 @@ class MoveToBucketDialogComponent < ApplicationComponent DIALOG_ID = "move-to-backlog-bucket-dialog" FORM_ID = "move-to-backlog-bucket-dialog-form" + SELECTION_LABEL_ID = "move-to-backlog-bucket-dialog-selection" - attr_reader :work_package, :project, :buckets, :move_action + attr_reader :work_packages, :buckets, :move_action - def initialize(work_package:, project:, move_action:) + def initialize(work_packages:, buckets:, move_action:) super() - @work_package = work_package - @project = project + @work_packages = work_packages + @buckets = buckets @move_action = move_action - @buckets = BacklogBucket.where(project:).order_alphabetically - @buckets = @buckets.where.not(id: work_package.backlog_bucket_id) if work_package.backlog_bucket_id end private - def list_type + def destination_list_type Backlogs::Target::BucketId.new(nil).list_type end end diff --git a/modules/backlogs/app/components/backlogs/move_to_sprint_dialog_component.html.erb b/modules/backlogs/app/components/backlogs/move_to_sprint_dialog_component.html.erb index 29698a64638d..b2e61f548360 100644 --- a/modules/backlogs/app/components/backlogs/move_to_sprint_dialog_component.html.erb +++ b/modules/backlogs/app/components/backlogs/move_to_sprint_dialog_component.html.erb @@ -27,32 +27,53 @@ See COPYRIGHT and LICENSE files for more details. ++#%> -<%= - render(Primer::Alpha::Dialog.new(title: t(".title"), id: DIALOG_ID, size: :medium)) do |d| - d.with_header(variant: :large) - - d.with_body do - available_sprints = sprints - sprint_list_type = list_type +<%= render(Primer::Alpha::Dialog.new(title: t(".title"), id: DIALOG_ID, size: :medium_portrait)) do |d| %> + <% d.with_header(variant: :large) %> + <% d.with_body do %> + <%= primer_form_with( url: move_action, method: :put, id: FORM_ID, data: { turbo_stream: true } ) do |f| + selected_work_packages = work_packages + available_sprints = sprints + sprint_list_type = destination_list_type + render_inline_form(f) do |form| + selected_work_packages.each do |work_package| + form.hidden(name: "ids[]", value: work_package.id) + end form.hidden(name: "list_type", value: sprint_list_type) - form.select_list(name: "list_id", label: Sprint.human_model_name, visually_hide_label: true) do |select| + form.select_list( + name: "list_id", + label: Sprint.human_model_name, + visually_hide_label: true, + aria: { describedby: SELECTION_LABEL_ID } + ) do |select| available_sprints.each do |sprint| select.option(label: sprint.name, value: sprint.id) end end end end - end + %> + + <%= + render( + Backlogs::SelectedWorkPackagesComponent.new( + work_packages:, + description_id: SELECTION_LABEL_ID, + mt: 3 + ) + ) + %> + <% end %> - d.with_footer(mt: 2) do + <% d.with_footer(show_divider: true) do %> + <%= component_collection do |buttons| buttons.with_component(Primer::Beta::Button.new(data: { close_dialog_id: DIALOG_ID })) do t(:button_cancel) @@ -64,6 +85,6 @@ See COPYRIGHT and LICENSE files for more details. t(:button_move) end end - end - end -%> + %> + <% end %> +<% end %> diff --git a/modules/backlogs/app/components/backlogs/move_to_sprint_dialog_component.rb b/modules/backlogs/app/components/backlogs/move_to_sprint_dialog_component.rb index bf0efcf387f2..3e7ebf6751aa 100644 --- a/modules/backlogs/app/components/backlogs/move_to_sprint_dialog_component.rb +++ b/modules/backlogs/app/components/backlogs/move_to_sprint_dialog_component.rb @@ -35,22 +35,21 @@ class MoveToSprintDialogComponent < ApplicationComponent DIALOG_ID = "move-to-sprint-dialog" FORM_ID = "move-to-sprint-dialog-form" + SELECTION_LABEL_ID = "move-to-sprint-dialog-selection" - attr_reader :work_package, :project, :sprints, :move_action + attr_reader :work_packages, :sprints, :move_action - def initialize(work_package:, project:, move_action:) + def initialize(work_packages:, sprints:, move_action:) super() - @work_package = work_package - @project = project + @work_packages = work_packages + @sprints = sprints @move_action = move_action - @sprints = Sprint.for_project(@project).visible.not_completed.order_by_date - @sprints = @sprints.where.not(id: work_package.sprint_id) if work_package.sprint_id end private - def list_type + def destination_list_type Backlogs::Target::SprintId.new(nil).list_type end end diff --git a/modules/backlogs/app/components/backlogs/selected_work_packages_component.html.erb b/modules/backlogs/app/components/backlogs/selected_work_packages_component.html.erb new file mode 100644 index 000000000000..871f1b2344cf --- /dev/null +++ b/modules/backlogs/app/components/backlogs/selected_work_packages_component.html.erb @@ -0,0 +1,55 @@ +<%#-- 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. + +++#%> + +<%= render(OpPrimer::InsetBoxComponent.new(**@system_arguments)) do %> + <%= + render( + Primer::Beta::Heading.new( + tag: :h2, + id: description_id, + font_size: :small, + font_weight: :bold, + mb: 2 + ) + ) { t(".label", count: work_packages.size) } + %> + + <% work_packages.each do |work_package| %> + <%= + render( + WorkPackages::InfoLineComponent.new( + work_package:, + show_subject: true, + show_status: false, + my: 1 + ) + ) + %> + <% end %> +<% end %> diff --git a/modules/backlogs/app/components/backlogs/selected_work_packages_component.rb b/modules/backlogs/app/components/backlogs/selected_work_packages_component.rb new file mode 100644 index 000000000000..4552d03a4067 --- /dev/null +++ b/modules/backlogs/app/components/backlogs/selected_work_packages_component.rb @@ -0,0 +1,49 @@ +# 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 + # Lists the work packages a batch action is about to affect. + # + # The heading carries `description_id`; point the acting control's + # `aria-describedby` at that id so the count reaches assistive technology. + class SelectedWorkPackagesComponent < ApplicationComponent + include OpPrimer::ComponentHelpers + + attr_reader :work_packages, :description_id + + def initialize(work_packages:, description_id:, **system_arguments) + super() + + @work_packages = work_packages + @description_id = description_id + @system_arguments = system_arguments + end + end +end diff --git a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb index 8136bb615e92..b339a5285b40 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb +++ b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb @@ -75,8 +75,9 @@ See COPYRIGHT and LICENSE files for more details. id: dom_target(work_package, :menu, :move_to_inbox), label: t(".action_menu.move_to_inbox"), tag: :button, - href: move_project_backlogs_work_package_path(project, work_package), - form_arguments: { method: :put, inputs: [{ name: "list_type", value: inbox_list_type }] } + data: destination_data(inbox_list_type, []).merge( + action: "click->sortable-lists--item#moveToDestination:prevent" + ) ) do |item| item.with_leading_visual_icon(icon: :inbox) end @@ -86,8 +87,16 @@ See COPYRIGHT and LICENSE files for more details. list.with_item( id: dom_target(work_package, :menu, :move_to_backlog_bucket), label: t(".action_menu.move_to_backlog_bucket"), - href: move_to_bucket_dialog_project_backlogs_work_package_path(project, work_package), - content_arguments: { data: { controller: "async-dialog" } } + tag: :button, + href: move_to_bucket_dialog_project_backlogs_work_packages_path(project), + form_arguments: { method: :post }, + content_arguments: { + data: { + controller: "async-dialog", + action: "async-dialog:beforeLoad->sortable-lists--item#prepareDialog" + } + }, + data: destination_data(bucket_list_type, bucket_ids) ) do |item| item.with_leading_visual_icon(icon: :package) end @@ -97,8 +106,16 @@ See COPYRIGHT and LICENSE files for more details. list.with_item( id: dom_target(work_package, :menu, :move_to_sprint), label: t(".action_menu.move_to_sprint"), - href: move_to_sprint_dialog_project_backlogs_work_package_path(project, work_package), - content_arguments: { data: { controller: "async-dialog" } } + tag: :button, + href: move_to_sprint_dialog_project_backlogs_work_packages_path(project), + form_arguments: { method: :post }, + content_arguments: { + data: { + controller: "async-dialog", + action: "async-dialog:beforeLoad->sortable-lists--item#prepareDialog" + } + }, + data: destination_data(sprint_list_type, sprint_ids) ) do |item| item.with_leading_visual_icon(icon: :zap) end diff --git a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb index c93b030d452c..2294b8b79f60 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb +++ b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb @@ -36,15 +36,15 @@ class WorkPackageCardMenuComponent < ApplicationComponent include CommonHelper include Concerns::WorkPackageMovability - attr_reader :work_package, :project, :open_sprints_exist, :other_buckets_exist, :current_user + attr_reader :work_package, :project, :sprint_ids, :bucket_ids, :current_user - def initialize(work_package:, project:, open_sprints_exist:, other_buckets_exist:, current_user: User.current) + def initialize(work_package:, project:, sprint_ids:, bucket_ids:, current_user: User.current) super() @work_package = work_package @project = project - @open_sprints_exist = open_sprints_exist - @other_buckets_exist = other_buckets_exist + @sprint_ids = sprint_ids + @bucket_ids = bucket_ids @current_user = current_user end @@ -62,15 +62,15 @@ def show_move_items? end def show_move_to_inbox? - movable? && (work_package.sprint_id? || work_package.backlog_bucket_id?) + sortable? end def show_move_to_backlog_bucket? - movable? && other_buckets_exist + sortable? && bucket_ids.any? end def show_move_to_sprint? - movable? && open_sprints_exist + sortable? && sprint_ids.any? end def show_move_submenu? @@ -106,5 +106,23 @@ def build_move_item(menu, label:, icon:, direction:) def inbox_list_type Backlogs::Target::InboxId.list_type end + + def sprint_list_type + Backlogs::Target::SprintId[nil].list_type + end + + def bucket_list_type + Backlogs::Target::BucketId[nil].list_type + end + + def destination_data(type, ids) + candidates = ids.map { |id| { type:, id: id.to_s } } + candidates = [{ type:, id: nil }] if type == Backlogs::Target::InboxId.list_type + + { + sortable_lists__item_target: "destinationItem", + sortable_lists_destinations: candidates.to_json + } + 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 385097cff9ac..0e7888b60ce5 100644 --- a/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb +++ b/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb @@ -36,15 +36,15 @@ class WorkPackagesController < BaseController # split view open on the moved work package (see split-view-sync.controller.ts). WORK_PACKAGE_MOVED_EVENT = "#{OpTurbo::ComponentStream::DISPATCHED_EVENT_PREFIX}backlogs:work-package-moved".freeze - before_action :load_work_package, only: %i[menu move_to_sprint_dialog move_to_bucket_dialog move] + before_action :load_work_package, only: %i[menu move] # Deferred ActionMenu items (Primer include-fragment). def menu render(Backlogs::WorkPackageCardMenuComponent.new( project: @project, work_package: @work_package, - open_sprints_exist: target_open_sprints.exists?, - other_buckets_exist: target_buckets.exists?, + sprint_ids: Sprint.assignable(project: @project, user: current_user).order_by_date.ids, + bucket_ids: BacklogBucket.for_project(@project).order_alphabetically.ids, current_user: ), layout: false) @@ -69,18 +69,30 @@ def add_existing_dialog end def move_to_sprint_dialog - respond_with_dialog Backlogs::MoveToSprintDialogComponent.new( - work_package: @work_package, - project: @project, - move_action: move_path + work_packages = load_collection_work_packages + return if performed? + + sprints = destination_availability(work_packages).sprints + return render_move_collection_error(t(".no_available_destinations")) if sprints.empty? + + respond_with_dialog build_move_to_sprint_dialog( + work_packages:, + sprints:, + move_action: move_collection_path ) end def move_to_bucket_dialog - respond_with_dialog Backlogs::MoveToBucketDialogComponent.new( - work_package: @work_package, - project: @project, - move_action: move_path + work_packages = load_collection_work_packages + return if performed? + + buckets = destination_availability(work_packages).buckets + return render_move_collection_error(t(".no_available_destinations")) if buckets.empty? + + respond_with_dialog build_move_to_bucket_dialog( + work_packages:, + buckets:, + move_action: move_collection_path ) end @@ -156,7 +168,8 @@ def render_update_collection_turbo_streams(call) WORK_PACKAGE_MOVED_EVENT, detail: { work_package_ids: call.result.map(&:id) } ) - render_invisible_after_move_batch_flash(call.result) + invisible_feedback_rendered = render_invisible_after_move_batch_flash(call.result) + render_collection_move_announcement(call.result) unless optimistic_move? || invisible_feedback_rendered else render_error_flash_message_via_turbo_stream( message: I18n.t(:notice_unsuccessful_update_with_reason, reason: batch_failure_reason(call)) @@ -241,11 +254,12 @@ def render_invisible_after_move_flash(work_package) # 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) + def render_invisible_after_move_batch_flash(results) # rubocop:disable Naming/PredicateMethod invisible = results.select { |wp| work_package_invisible_after_move?(wp) } - return if invisible.empty? + return false if invisible.empty? render_flash_message_via_turbo_stream(message: invisible_after_move_batch_message(invisible)) + true end def invisible_after_move_batch_message(invisible) @@ -272,7 +286,7 @@ def render_move_announcement(work_package) render_live_region_update_message( message: t( - ".moved_announcement", + "backlogs.work_packages.move.moved_announcement", label: work_package.to_fs(:caption), list: target_list_name(work_package), position: index + 1, @@ -281,6 +295,29 @@ def render_move_announcement(work_package) ) end + def render_collection_move_announcement(work_packages) + return render_move_announcement(work_packages.first) if work_packages.one? + + scope_ids = announcement_list_scope(work_packages.first).pluck(:id) + first = scope_ids.index(work_packages.first.id) + return unless first + + render_live_region_update_message( + message: collection_move_announcement_message(work_packages, first:, total: scope_ids.size) + ) + end + + def collection_move_announcement_message(work_packages, first:, total:) + t( + ".moved_announcement", + count: work_packages.size, + list: target_list_name(work_packages.first), + first: first + 1, + last: first + work_packages.size, + total: + ) + end + # The scopes the page renders, so the announced position matches what a # sighted user sees: sprints are scoped to the project (shared sprints # render only the project's items), buckets and the inbox go through the @@ -305,6 +342,29 @@ def load_work_package @work_package = @work_packages.find(params.expect(:id)) end + # The dialogs validate the id list alone: they open before a destination + # is chosen, so BatchMoveParamsContract, which also requires a resolvable + # target, cannot speak for them. Renders its own error and returns nil, + # so callers test `performed?`. + def load_collection_work_packages + ids = move_collection_params[:ids] + + # Before the lookup: an oversized id list must not reach the database. + if ids.length > Backlogs::WorkPackages::BatchUpdateService::MAX_BATCH_SIZE + return render_move_collection_error( + t("backlogs.work_packages.move_collection.too_many_work_packages", + max: Backlogs::WorkPackages::BatchUpdateService::MAX_BATCH_SIZE) + ) + end + + if ids.any?(&:blank?) || ids.uniq.length != ids.length + return render_move_collection_error(t("backlogs.work_packages.move_collection.invalid_ids")) + end + + find_collection_work_packages(ids) || + render_move_collection_error(t("backlogs.work_packages.move_collection.work_packages_not_found")) + 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. @@ -322,6 +382,22 @@ def render_move_collection_error(reason) respond_with_turbo_streams(status: :unprocessable_entity) end + def destination_availability(work_packages) + Backlogs::WorkPackages::DestinationAvailability.new( + project: @project, + user: current_user, + work_packages: + ) + end + + def build_move_to_sprint_dialog(**args) + Backlogs::MoveToSprintDialogComponent.new(**args) + end + + def build_move_to_bucket_dialog(**args) + Backlogs::MoveToBucketDialogComponent.new(**args) + 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 @@ -331,19 +407,8 @@ def move_collection_params end end - def move_path - move_project_backlogs_work_package_path(@project, @work_package, backlog_filter_params) - end - - def target_open_sprints - Sprint.for_project(@project) - .visible.not_completed - .where.not(id: @work_package.sprint_id) - end - - def target_buckets - BacklogBucket.where(project: @project) - .where.not(id: @work_package.backlog_bucket_id) + def move_collection_path + move_project_backlogs_work_packages_path(@project, backlog_filter_params) end # After a move the work package might no longer be visible: the page's active 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 index f3de96c3787f..fe342434815b 100644 --- a/modules/backlogs/app/services/backlogs/work_packages/batch_update_service.rb +++ b/modules/backlogs/app/services/backlogs/work_packages/batch_update_service.rb @@ -109,7 +109,8 @@ def move_batch(target, prev_id, list_type:, list_id:) return stale_batch_failure unless cohort_intact? lock_destination_row!(destination) - return unavailable_target_failure unless target_available?(target) + destination_failure = revalidate_destination(target) + return destination_failure if destination_failure anchor_failure = revalidate_anchor(placement, target) return anchor_failure if anchor_failure @@ -246,6 +247,16 @@ def cohort_intact? end end + def cohort_intact? + current = WorkPackage + .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 @@ -267,19 +278,55 @@ def revalidate_anchor(placement, target) # rubocop:disable Metrics/AbcSize # 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. + # destination. Judged on freshly loaded rows rather than the batch's + # loaded instances: a member's status, and so its mobility, may have + # changed since the controller loaded it, which is exactly what this check + # under the lock exists to catch. + def revalidate_destination(target) + return unavailable_target_failure unless target_available?(target) + + refused = destination_availability.refusing(target) + refused_members_failure(refused) if refused.any? + end + def target_available?(target) + destination_availability.candidate?(target) + end + + # Freshly loaded rows, not the batch's loaded instances: a member's status, + # and so its mobility, may have changed since the controller loaded it, + # which is exactly what this check under the lock exists to catch. + def destination_availability + @destination_availability ||= Backlogs::WorkPackages::DestinationAvailability.new( + project: batch_project, + user:, + work_packages: WorkPackage.where(id: work_packages.map(&:id)).to_a + ) + end + + # Unscoped by policy so completion, deletion and reassignment all resolve + # to the same advisory identity. DestinationAvailability stays the + # authority once the locks have refreshed state. + def raw_destination(target) case target in Backlogs::Target::SprintId - Sprint.assignable(project: batch_project, user:).exists?(id: target.list_id) + Sprint.find_by(id: target.list_id) in Backlogs::Target::BucketId - BacklogBucket.for_project(batch_project).exists?(id: target.list_id) + BacklogBucket.find_by(id: target.list_id) in Backlogs::Target::InboxId - true + nil end end + # lock! reloads under FOR UPDATE, so a concurrent completion, deletion or + # reassignment commits before the candidate query above runs. Inbox has no + # destination row; its append is serialized by the advisory lock alone. + def lock_destination_row!(destination) + destination&.lock! + rescue ActiveRecord::RecordNotFound + nil + end + def last_non_batch_member(target) WorkPackage .visible(user) @@ -318,6 +365,21 @@ def unavailable_target_failure ServiceResult.failure(message: I18n.t("backlogs.work_packages.batch_update_service.unavailable_target")) end + # Refused whole, before any member moves, but naming the members that + # refused rather than leaving the caller to guess. + def refused_members_failure(members) + failure = unavailable_target_failure + members.each do |member| + failure.add_dependent!( + ServiceResult.failure( + result: member, + message: I18n.t("backlogs.work_packages.batch_update_service.unavailable_target") + ) + ) + end + failure + end + def mixed_projects_failure ServiceResult.failure(message: I18n.t("backlogs.work_packages.batch_update_service.mixed_projects")) end diff --git a/modules/backlogs/app/services/backlogs/work_packages/destination_availability.rb b/modules/backlogs/app/services/backlogs/work_packages/destination_availability.rb new file mode 100644 index 000000000000..5dddc5b24bc7 --- /dev/null +++ b/modules/backlogs/app/services/backlogs/work_packages/destination_availability.rb @@ -0,0 +1,109 @@ +# 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. +#++ + +class Backlogs::WorkPackages::DestinationAvailability + attr_reader :project, :user, :work_packages + + def initialize(project:, user:, work_packages:) + @project = project + @user = user + @work_packages = work_packages + end + + def permitted?(target) + user.allowed_in_project?(:manage_sprint_items, project) && + candidate?(target) && + work_packages.all? { |work_package| free?(work_package) || current_target(work_package) == target } + end + + def sprints + sprint_candidates.select { |sprint| offered?(Backlogs::Target.for(sprint)) } + end + + def buckets + bucket_candidates.select { |bucket| offered?(Backlogs::Target.for(bucket)) } + end + + def inbox? + offered?(Backlogs::Target::InboxId) + end + + # The members that cannot enter the target: a work package frozen by its + # status stays where it is, so only its own destination accepts it. + def refusing(target) + work_packages.reject { |work_package| free?(work_package) || current_target(work_package) == target } + end + + # Target candidacy alone, without the per-member arm of permitted?: the + # batch move reports which members refused rather than collapsing the whole + # batch into one anonymous failure. + def candidate?(target) + case target + in Backlogs::Target::SprintId + sprint_candidates.any? { |sprint| sprint.id == target.list_id } + in Backlogs::Target::BucketId + bucket_candidates.any? { |bucket| bucket.id == target.list_id } + in Backlogs::Target::InboxId + true + else + false + end + end + + private + + def offered?(target) + permitted?(target) && work_packages.any? { |work_package| current_target(work_package) != target } + end + + def sprint_candidates + @sprint_candidates ||= Sprint.assignable(project:, user:).order_by_date.to_a + end + + def bucket_candidates + @bucket_candidates ||= BacklogBucket.for_project(project).order_alphabetically.to_a + end + + def free?(work_package) + readonly_status_ids.exclude?(work_package.status_id) + end + + def readonly_status_ids + @readonly_status_ids ||= if Status.can_readonly? + Status.where(id: work_packages.map(&:status_id), is_readonly: true).ids + else + [] + end + end + + def current_target(work_package) + Backlogs::Target.for_work_package(work_package) + 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 a39f9f54998c..53ec6aa67c02 100644 --- a/modules/backlogs/app/views/backlogs/backlog/show.html.erb +++ b/modules/backlogs/app/views/backlogs/backlog/show.html.erb @@ -51,7 +51,10 @@ See COPYRIGHT and LICENSE files for more details. data: { controller: "backlogs--list-refresh backlogs--split-view-sync sortable-lists", sortable_lists_optimistic_value: true, - action: "#{Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT}@document->backlogs--split-view-sync#onWorkPackageMoved", + action: [ + "#{Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT}@document->backlogs--split-view-sync#onWorkPackageMoved", + "#{Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT}@document->sortable-lists#clearSelectionAfterMove" + ].join(" "), 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, diff --git a/modules/backlogs/config/locales/en.yml b/modules/backlogs/config/locales/en.yml index 3dce5e68e083..a23915ee5d6a 100644 --- a/modules/backlogs/config/locales/en.yml +++ b/modules/backlogs/config/locales/en.yml @@ -208,6 +208,12 @@ en: rebuild: "Rebuild" rebuild_positions: "Rebuild positions" remaining_hours: "Remaining work" + + selected_work_packages_component: + label: + one: "1 selected work package" + other: "%{count} selected work packages" + sharing: "Sprint sharing" show_burndown_chart: "Burndown chart" @@ -295,9 +301,16 @@ en: unexpected_failure: "The work packages could not be moved. Please try again." move: moved_announcement: "%{label} moved to %{list}, position %{position} of %{total}" + move_to_bucket_dialog: + no_available_destinations: "There are no available backlog buckets for these work packages." + move_to_sprint_dialog: + no_available_destinations: "There are no available sprints for these work packages." move_collection: invalid_ids: "The list of work packages to move is invalid." member_failed: "%{work_package}: %{reason}" + moved_announcement: + one: "%{count} work package moved to %{list}, position %{first} of %{total}" + other: "%{count} work packages moved to %{list}, positions %{first} through %{last} of %{total}" 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: diff --git a/modules/backlogs/config/routes.rb b/modules/backlogs/config/routes.rb index 47c094093502..9c221379021a 100644 --- a/modules/backlogs/config/routes.rb +++ b/modules/backlogs/config/routes.rb @@ -99,14 +99,14 @@ collection do get :add_existing_dialog post :add_existing + post :move_to_sprint_dialog + post :move_to_bucket_dialog put :move, action: :move_collection end member do get :menu put :move - get :move_to_sprint_dialog - get :move_to_bucket_dialog end end diff --git a/modules/backlogs/spec/components/backlogs/move_to_bucket_dialog_component_spec.rb b/modules/backlogs/spec/components/backlogs/move_to_bucket_dialog_component_spec.rb index 6af17413abea..6d85db18749c 100644 --- a/modules/backlogs/spec/components/backlogs/move_to_bucket_dialog_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/move_to_bucket_dialog_component_spec.rb @@ -35,11 +35,14 @@ current_user { admin } let(:project) { create(:project) } - let(:work_package) { create(:work_package, project:) } - let(:move_path) { Rails.application.routes.url_helpers.move_project_backlogs_work_package_path(project, work_package) } + let(:first) { create(:work_package, project:) } + let(:second) { create(:work_package, project:) } + let(:work_packages) { [second, first] } + let(:move_path) { Rails.application.routes.url_helpers.move_project_backlogs_work_packages_path(project) } + let(:buckets) { [create(:backlog_bucket, project:, name: "Passed Bucket")] } def render_component - render_inline(described_class.new(work_package:, project:, move_action: move_path)) + render_inline(described_class.new(work_packages:, buckets:, move_action: move_path)) end it "renders the dialog with the correct title" do @@ -48,23 +51,30 @@ def render_component expect(page).to have_text(I18n.t(:"backlogs.move_to_bucket_dialog_component.title")) end - it "renders a form targeting the move path via PUT" do + it "renders an ordered collection form targeting the move path via PUT", :aggregate_failures do render_component + expect(page).to have_text(second.subject) + expect(page).to have_text(first.subject) + expect(page.all("input[name='ids[]']", visible: :all).map(&:value)) + .to eq([second.id.to_s, first.id.to_s]) expect(page).to have_element(:form, action: move_path, method: "post") - expect(page).to have_element(:input, name: "_method", value: "put", visible: :all) + expect(page).to have_css("form[action='#{move_path}'] input[name='_method'][value='put']", visible: :all) + expect(page).to have_css( + "input[name='list_type'][value='#{Backlogs::Target::BucketId.new(nil).list_type}']", + visible: :all + ) end - context "when params[:all] is true" do - let(:move_path) do - Rails.application.routes.url_helpers.move_project_backlogs_work_package_path(project, work_package, all: "true") - end - - it "submits the move form with the all query preserved" do - render_component + it "describes the bucket select by the selection heading" do + render_component - expect(page).to have_element(:form, action: /all=true/) - end + label = page.find_css("##{described_class::SELECTION_LABEL_ID}").first + expect(label.text).to include(I18n.t("backlogs.selected_work_packages_component.label", count: 2)) + expect(page).to have_css( + "select[name='list_id'][aria-describedby~='#{described_class::SELECTION_LABEL_ID}']", + visible: :all + ) end it "renders Cancel and Move buttons" do @@ -74,42 +84,14 @@ def render_component expect(page).to have_button(I18n.t(:button_move)) end - context "when buckets exist" do - let!(:bucket_a) { create(:backlog_bucket, project:, name: "Alpha") } - let!(:bucket_b) { create(:backlog_bucket, project:, name: "Beta") } - - it "submits backlog_bucket list data" do - render_component - - expect(page).to have_css( - "input[name='list_type'][value='#{Backlogs::Target::BucketId.new(nil).list_type}']", - visible: :all - ) - expect(page).to have_element(:option, value: bucket_a.id, text: "Alpha") - expect(page).to have_element(:option, value: bucket_b.id, text: "Beta") - end - end - - context "when a bucket belongs to a different project" do - let!(:other_bucket) { create(:backlog_bucket, project: create(:project), name: "Other") } - - it "does not list buckets from other projects" do - render_component - - expect(page).to have_no_css(:option, text: "Other") - end - end - - context "when the work package is already in a bucket" do - let!(:current_bucket) { create(:backlog_bucket, project:, name: "Current") } - let!(:target_bucket) { create(:backlog_bucket, project:, name: "Target") } - let(:work_package) { create(:work_package, project:, backlog_bucket: current_bucket) } + context "when other bucket records exist" do + let!(:omitted_bucket) { create(:backlog_bucket, project:, name: "Omitted Bucket") } - it "excludes the current bucket from the options" do + it "renders only the destinations supplied by the controller" do render_component - expect(page).to have_no_css(:option, text: "Current") - expect(page).to have_element(:option, value: target_bucket.id, text: "Target") + expect(page).to have_css("option[value='#{buckets.first.id}']", text: "Passed Bucket") + expect(page).to have_no_css("option[value='#{omitted_bucket.id}']") end end end diff --git a/modules/backlogs/spec/components/backlogs/move_to_sprint_dialog_component_spec.rb b/modules/backlogs/spec/components/backlogs/move_to_sprint_dialog_component_spec.rb index 7a4703ba1d53..b66f02787b82 100644 --- a/modules/backlogs/spec/components/backlogs/move_to_sprint_dialog_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/move_to_sprint_dialog_component_spec.rb @@ -35,11 +35,14 @@ current_user { admin } let(:project) { create(:project) } - let(:work_package) { create(:work_package, project:) } - let(:move_path) { Rails.application.routes.url_helpers.move_project_backlogs_work_package_path(project, work_package) } + let(:first) { create(:work_package, project:) } + let(:second) { create(:work_package, project:) } + let(:work_packages) { [second, first] } + let(:move_path) { Rails.application.routes.url_helpers.move_project_backlogs_work_packages_path(project) } + let(:sprints) { [create(:sprint, project:, name: "Passed Sprint")] } def render_component - render_inline(described_class.new(work_package:, project:, move_action: move_path)) + render_inline(described_class.new(work_packages:, sprints:, move_action: move_path)) end it "renders the dialog with the correct title" do @@ -48,23 +51,30 @@ def render_component expect(page).to have_text(I18n.t(:"backlogs.move_to_sprint_dialog_component.title")) end - it "renders a form targeting the move path via PUT" do + it "renders an ordered collection form targeting the move path via PUT", :aggregate_failures do render_component + expect(page).to have_text(second.subject) + expect(page).to have_text(first.subject) + expect(page.all("input[name='ids[]']", visible: :all).map(&:value)) + .to eq([second.id.to_s, first.id.to_s]) expect(page).to have_element(:form, action: move_path, method: "post") expect(page).to have_css("form[action='#{move_path}'] input[name='_method'][value='put']", visible: :all) + expect(page).to have_css( + "input[name='list_type'][value='#{Backlogs::Target::SprintId.new(nil).list_type}']", + visible: :all + ) end - context "when params[:all] is true" do - let(:move_path) do - Rails.application.routes.url_helpers.move_project_backlogs_work_package_path(project, work_package, all: "true") - end - - it "submits the move form with the all query preserved" do - render_component + it "describes the sprint select by the selection heading" do + render_component - expect(page).to have_css("form[action*='all=true']", visible: :all) - end + label = page.find_css("##{described_class::SELECTION_LABEL_ID}").first + expect(label.text).to include(I18n.t("backlogs.selected_work_packages_component.label", count: 2)) + expect(page).to have_css( + "select[name='list_id'][aria-describedby~='#{described_class::SELECTION_LABEL_ID}']", + visible: :all + ) end it "renders Cancel and Save buttons" do @@ -74,65 +84,14 @@ def render_component expect(page).to have_button(I18n.t(:button_move)) end - context "when in_planning and active sprints exist" do - let!(:planning_sprint) { create(:sprint, project:, name: "Planning Sprint", status: "in_planning") } - let!(:active_sprint) { create(:sprint, project:, name: "Active Sprint", status: "active") } - - it "submits sprint list data" do - render_component - - expect(page).to have_css( - "input[name='list_type'][value='#{Backlogs::Target::SprintId.new(nil).list_type}']", - visible: :all - ) - expect(page).to have_css("option[value='#{planning_sprint.id}']", text: "Planning Sprint") - expect(page).to have_css("option[value='#{active_sprint.id}']", text: "Active Sprint") - end - end - - context "when a completed sprint exists" do - let!(:completed_sprint) { create(:sprint, project:, name: "Old Sprint", status: "completed") } - - it "does not list the completed sprint" do - render_component - - expect(page).to have_no_css("option", text: "Old Sprint") - end - end - - context "when a sprint belongs to a different project" do - let!(:other_sprint) { create(:sprint, project: create(:project), name: "Other Sprint") } - - it "does not list sprints from other projects" do - render_component - - expect(page).to have_no_css("option", text: "Other Sprint") - end - end - - context "when the work package is already in a sprint" do - let!(:current_sprint) { create(:sprint, project:, name: "Current Sprint") } - let!(:target_sprint) { create(:sprint, project:, name: "Target Sprint") } - let(:work_package) { create(:work_package, project:, sprint: current_sprint) } - - it "excludes that sprint from the options" do - render_component - - expect(page).to have_no_css("option", text: "Current Sprint") - expect(page).to have_css("option[value='#{target_sprint.id}']", text: "Target Sprint") - end - end - - context "when the current user cannot view sprints in the project" do - let(:other_user) { create(:user) } - let!(:hidden_sprint) { create(:sprint, project:, name: "Hidden Sprint") } - - current_user { other_user } + context "when other sprint records exist" do + let!(:omitted_sprint) { create(:sprint, project:, name: "Omitted Sprint") } - it "does not list sprints the user is not permitted to see" do + it "renders only the destinations supplied by the controller" do render_component - expect(page).to have_no_css("option", text: "Hidden Sprint") + expect(page).to have_css("option[value='#{sprints.first.id}']", text: "Passed Sprint") + expect(page).to have_no_css("option[value='#{omitted_sprint.id}']") end end end diff --git a/modules/backlogs/spec/components/backlogs/selected_work_packages_component_spec.rb b/modules/backlogs/spec/components/backlogs/selected_work_packages_component_spec.rb new file mode 100644 index 000000000000..32c3635ff3c9 --- /dev/null +++ b/modules/backlogs/spec/components/backlogs/selected_work_packages_component_spec.rb @@ -0,0 +1,79 @@ +# 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 "rails_helper" + +RSpec.describe Backlogs::SelectedWorkPackagesComponent, type: :component do + shared_let(:admin) { create(:admin) } + current_user { admin } + + shared_let(:project) { create(:project) } + shared_let(:epic_type) { create(:type, name: "Epic") } + shared_let(:feature_type) { create(:type, name: "Feature") } + shared_let(:epic) { create(:work_package, project:, type: epic_type, subject: "Contamination model") } + shared_let(:feature) { create(:work_package, project:, type: feature_type, subject: "Warning system") } + + let(:description_id) { "move-dialog-selection" } + + def render_component(work_packages: [feature, epic]) + render_inline(described_class.new(work_packages:, description_id:)) + end + + it "labels the box and names every work package in the given order", :aggregate_failures do + render_component + + expect(page).to have_text(I18n.t("backlogs.selected_work_packages_component.label", count: 2)) + expect(page.text).to match( + /Feature.*#{feature.formatted_id}.*Warning system.*Epic.*#{epic.formatted_id}.*Contamination model/mi + ) + end + + it "links each work package to its full view" do + render_component + + expect(page).to have_link(feature.formatted_id, href: "/work_packages/#{feature.id}") + expect(page).to have_link(epic.formatted_id, href: "/work_packages/#{epic.id}") + end + + it "heads the box with the selected count on the description element" do + render_component + + expect(page).to have_css( + "h2##{description_id}", + text: I18n.t("backlogs.selected_work_packages_component.label", count: 2) + ) + end + + it "heads a single selection in the singular" do + render_component(work_packages: [feature]) + + expect(page).to have_css("h2##{description_id}", text: "1 selected work package") + end +end diff --git a/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb b/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb index 68995222ff20..c3c3766a8e8b 100644 --- a/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb @@ -72,12 +72,14 @@ def create_story(position:, subject: "Test Story", story_points: 5) backlog_bucket: bucket) end - def render_component(work_package: self.work_package, open_sprints_exist: true, other_buckets_exist: true) + def render_component(work_package: self.work_package, + sprint_ids: [sprint].compact.map(&:id), + bucket_ids: [bucket].compact.map(&:id)) render_inline(described_class.new( work_package:, project:, - open_sprints_exist:, - other_buckets_exist:, + sprint_ids:, + bucket_ids:, current_user: user )) end @@ -188,6 +190,22 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, end describe "move menu items" do + it "renders project-level destination metadata including the invoking card's current target" do + other_sprint = create(:sprint, project:, name: "Sprint 2", start_date: Date.yesterday, finish_date: Date.tomorrow) + bucket = create(:backlog_bucket, project:) + + render_component(sprint_ids: [sprint.id, other_sprint.id], bucket_ids: [bucket.id]) + + expect(page).to have_css( + "li[data-sortable-lists--item-target~='destinationItem']" \ + "[data-sortable-lists-destinations*='\"type\":\"sprint\"']" + ) + expect(page).to have_css("li[data-sortable-lists-destinations*='\"id\":\"#{sprint.id}\"']") + expect(page).to have_css( + "li[data-sortable-lists-destinations*='\"type\":\"#{Backlogs::Target::InboxId.list_type}\"']" + ) + end + it "shows Move to top item with move-to-top icon" do render_component @@ -217,7 +235,7 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, end it "always renders the four move items as client targets", :aggregate_failures do - render_inline(described_class.new(work_package:, project:, open_sprints_exist: false, other_buckets_exist: false)) + render_inline(described_class.new(work_package:, project:, sprint_ids: [], bucket_ids: [])) # The target, direction, and action live on the item
  • , not the content button. %w[top up down bottom].each do |direction| @@ -264,7 +282,7 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, end it "still shows the Move to position submenu (permission-gated only)" do - render_component(open_sprints_exist: false, other_buckets_exist: false) + render_component(sprint_ids: [], bucket_ids: []) expect(page).to have_selector(:menuitem, text: "Move to position") end @@ -273,11 +291,25 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, end describe "Move to sprint item" do + it "loads the collection dialog with a POST form after freezing the action scope" do + render_component(sprint_ids: [sprint.id]) + + button = page.find("button#work_package_#{work_package.id}_menu_move_to_sprint") + form = button.find(:xpath, "ancestor::form") + + expect(button["data-controller"]).to eq("async-dialog") + expect(button["data-action"]).to eq("async-dialog:beforeLoad->sortable-lists--item#prepareDialog") + expect(form[:method]).to eq("post") + expect(form[:action]).to end_with( + "/projects/#{project.identifier}/backlogs/work_packages/move_to_sprint_dialog" + ) + end + context "when work package is in a sprint" do it "is shown with zap icon" do - render_component(open_sprints_exist: true) + render_component(sprint_ids: [sprint.id]) - expect(page).to have_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) expect(page).to have_octicon(:zap) expect(page).to have_text(I18n.t(:"backlogs.work_package_card_menu_component.action_menu.move_to_sprint")) end @@ -288,9 +320,9 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, let(:bucket) { create(:backlog_bucket, project:) } it "is shown" do - render_component(open_sprints_exist: true) + render_component(sprint_ids: [create(:sprint, project:).id]) - expect(page).to have_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) end end @@ -298,20 +330,34 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, let(:sprint) { nil } it "is shown" do - render_component(open_sprints_exist: true) + render_component(sprint_ids: [create(:sprint, project:).id]) - expect(page).to have_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) end end - it "is hidden when open_sprints_exist is false" do - render_component(open_sprints_exist: false) + it "is hidden when no sprint candidates exist" do + render_component(sprint_ids: []) - expect(page).to have_no_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) + expect(page).to have_no_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) end end describe "Move to backlog inbox item" do + it "binds a direct collection move without rendering a singular member form" do + render_component + + item = page.find( + "li[data-sortable-lists--item-target~='destinationItem']" \ + "[data-sortable-lists-destinations='[{\"type\":\"#{Backlogs::Target::InboxId.list_type}\",\"id\":null}]']" + ) + button = item.find("button#work_package_#{work_package.id}_menu_move_to_inbox") + + expect(item["data-action"]).to eq("click->sortable-lists--item#moveToDestination:prevent") + expect(button[:form]).to be_nil + expect(page).to have_no_field("list_type", type: :hidden) + end + context "when work package is in a sprint" do it "is shown with inbox icon" do render_component @@ -319,7 +365,7 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_inbox\z/) expect(page).to have_octicon(:inbox) expect(page).to have_text(I18n.t(:"backlogs.work_package_card_menu_component.action_menu.move_to_inbox")) - expect(page).to have_field("list_type", type: :hidden, with: Backlogs::Target::InboxId.list_type) + expect(page).to have_no_field("list_type", type: :hidden) end end @@ -337,20 +383,35 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, context "when work package is already in the inbox (no sprint, no bucket)" do let(:sprint) { nil } - it "is hidden" do + it "renders the inbox candidate for client-side projection" do render_component - expect(page).to have_no_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_inbox\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_inbox\z/) end end end describe "Move to backlog bucket item" do + it "loads the collection dialog with a POST form after freezing the action scope" do + bucket = create(:backlog_bucket, project:) + render_component(bucket_ids: [bucket.id]) + + button = page.find("button#work_package_#{work_package.id}_menu_move_to_backlog_bucket") + form = button.find(:xpath, "ancestor::form") + + expect(button["data-controller"]).to eq("async-dialog") + expect(button["data-action"]).to eq("async-dialog:beforeLoad->sortable-lists--item#prepareDialog") + expect(form[:method]).to eq("post") + expect(form[:action]).to end_with( + "/projects/#{project.identifier}/backlogs/work_packages/move_to_bucket_dialog" + ) + end + context "when work package is in a sprint" do it "is shown with package icon" do - render_component(other_buckets_exist: true) + render_component(bucket_ids: [create(:backlog_bucket, project:).id]) - expect(page).to have_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) expect(page).to have_octicon(:package) expect(page).to have_text(I18n.t(:"backlogs.work_package_card_menu_component.action_menu.move_to_backlog_bucket")) end @@ -361,9 +422,9 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, let(:bucket) { create(:backlog_bucket, project:) } it "is shown" do - render_component(other_buckets_exist: true) + render_component(bucket_ids: [bucket.id]) - expect(page).to have_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) end end @@ -371,16 +432,16 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, let(:sprint) { nil } it "is shown" do - render_component(other_buckets_exist: true) + render_component(bucket_ids: [create(:backlog_bucket, project:).id]) - expect(page).to have_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) end end - it "is hidden when other_buckets_exist is false" do - render_component(other_buckets_exist: false) + it "is hidden when no bucket candidates exist" do + render_component(bucket_ids: []) - expect(page).to have_no_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) + expect(page).to have_no_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) end end @@ -392,12 +453,11 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, let(:work_package) { readonly_story } context "with an Enterprise token", with_ee: %i[readonly_work_packages] do - it "offers no move to another container", :aggregate_failures do + it "renders destination candidates for client-side confined-scope projection", :aggregate_failures do render_component - expect(page).to have_no_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) - expect(page).to have_no_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_backlog_bucket\z/) - expect(page).to have_no_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_inbox\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_inbox\z/) end it "keeps the positional moves within its own list", :aggregate_failures do @@ -435,7 +495,7 @@ def render_component(work_package: self.work_package, open_sprints_exist: true, render_component expect(page).to have_selector(:menuitem, text: "Move to position") - expect(page).to have_element(:a, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) + expect(page).to have_element(:button, id: /\Awork_package_#{work_package.id}_menu_move_to_sprint\z/) end end end diff --git a/modules/backlogs/spec/controllers/backlogs/work_packages_controller_spec.rb b/modules/backlogs/spec/controllers/backlogs/work_packages_controller_spec.rb index 308ef477d023..a2086201aaf4 100644 --- a/modules/backlogs/spec/controllers/backlogs/work_packages_controller_spec.rb +++ b/modules/backlogs/spec/controllers/backlogs/work_packages_controller_spec.rb @@ -680,55 +680,31 @@ def turbo_stream_template(action:) expect(body).to include(I18n.t(:"js.button_open_details")) end - context "when another open sprint exists" do + context "when assignable sprints exist" do let!(:other_open_sprint) { create(:sprint, name: "Sprint 2", project:) } before { allow(Backlogs::WorkPackageCardMenuComponent).to receive(:new).and_call_original } - it "passes open_sprints_exist: true to the menu component" do + it "passes ordered project sprint candidates to the menu component" do response expect(Backlogs::WorkPackageCardMenuComponent) .to have_received(:new) - .with(hash_including(open_sprints_exist: true)) + .with(hash_including(sprint_ids: Sprint.assignable(project:, user:).order_by_date.ids)) end end - context "when no other open sprints exist" do - before { allow(Backlogs::WorkPackageCardMenuComponent).to receive(:new).and_call_original } - - it "passes open_sprints_exist: false to the menu component" do - response - - expect(Backlogs::WorkPackageCardMenuComponent) - .to have_received(:new) - .with(hash_including(open_sprints_exist: false)) - end - end - - context "when other backlog buckets exist" do + context "when backlog buckets exist" do let!(:buckets) { create_list(:backlog_bucket, 2, project:) } before { allow(Backlogs::WorkPackageCardMenuComponent).to receive(:new).and_call_original } - it "passes other_buckets_exist: true to the menu component" do + it "passes alphabetically ordered project bucket candidates to the menu component" do response expect(Backlogs::WorkPackageCardMenuComponent) .to have_received(:new) - .with(hash_including(other_buckets_exist: true)) - end - end - - context "when no backlog buckets exist" do - before { allow(Backlogs::WorkPackageCardMenuComponent).to receive(:new).and_call_original } - - it "passes other_buckets_exist: false to the menu component" do - response - - expect(Backlogs::WorkPackageCardMenuComponent) - .to have_received(:new) - .with(hash_including(other_buckets_exist: false)) + .with(hash_including(bucket_ids: buckets.sort_by(&:name).map(&:id))) end end @@ -1167,191 +1143,133 @@ def turbo_stream_template(action:) end end - describe "GET #move_to_sprint_dialog" do - let!(:other_sprint) { create(:sprint) } - let!(:displayed_sprints) { create_list(:sprint, 2, project:) } - - let(:params) { { project_id: project.id, id: work_package.id } } - - subject(:response) { get :move_to_sprint_dialog, params:, format: :turbo_stream } - - context "with a Sprint source" do - it "responds with a dialog turbo stream", :aggregate_failures do - expect(response).to be_successful - expect(response).to have_turbo_stream action: "dialog" - end + describe "POST #move_to_sprint_dialog" do + let!(:available_sprint) { create(:sprint, project:) } + let!(:first) { create(:work_package, status:, project:) } + let!(:second) { create(:work_package, status:, project:) } + let(:ids) { [second.id, first.id] } + let(:params) { { project_id: project.id, ids: } } - it "includes the existing sprints as list_id options" do - subject - - displayed_sprints.each do |sprint| - expect(body).to include(%(value="#{sprint.id}")) - end - end + subject(:response) { post :move_to_sprint_dialog, params:, format: :turbo_stream } - it "does not include the current sprint as a list_id option" do - subject - - expect(body).not_to include(%(value="#{sprint.id}")) - end - - it "does not include the other sprint" do - subject - - expect(body).not_to include(%(value="#{other_sprint.id}")) - end + before do + allow(controller).to receive(:build_move_to_sprint_dialog).and_return(Object.new) + allow(controller).to receive(:respond_with_dialog) { controller.head :ok } end - context "with inbox source (no sprint_id)" do - let(:inbox_work_package) { create(:work_package, status:, project:) } - let(:params) { { project_id: project.id, id: inbox_work_package.id } } - - it "responds with a dialog turbo stream", :aggregate_failures do - expect(response).to be_successful - expect(response).to have_turbo_stream action: "dialog" - end - - it "embeds the no-sprint work_packages path in the dialog form action URL" do - expect(body).to include("backlogs/work_packages/#{inbox_work_package.id}/move") - expect(body).not_to include("sprints") - end - - it "includes the existing sprints as list_id options" do - subject + it "passes the exact ordered collection and authoritative sprints to the dialog" do + response - displayed_sprints.each do |sprint| - expect(body).to include(%(value="#{sprint.id}")) - end - end - - it "does not include the other sprint" do - subject - - expect(body).not_to include(%(value="#{other_sprint.id}")) - end + expect(controller) + .to have_received(:build_move_to_sprint_dialog) + .with( + work_packages: [second, first], + sprints: [available_sprint], + move_action: move_project_backlogs_work_packages_path(project) + ) end - context "with a Backlog bucket source" do - let(:bucket) { create(:backlog_bucket, project:) } - let(:bucket_work_package) { create(:work_package, status:, project:, backlog_bucket: bucket) } - let(:params) { { project_id: project.id, id: bucket_work_package.id } } - - it "responds with a dialog turbo stream", :aggregate_failures do - expect(response).to be_successful - expect(response).to have_turbo_stream action: "dialog" - end - - it "includes the available sprints in the dialog" do - displayed_sprints.each do |sprint| - expect(body).to include(%(value="#{sprint.id}")) - end - end - - it "does not include sprints from other projects" do - subject - - expect(body).not_to include(%(value="#{other_sprint.id}")) + shared_examples "rejects invalid dialog ids" do + it "returns a 422 Turbo Stream without opening a dialog", :aggregate_failures do + expect(response).to have_http_status :unprocessable_entity + expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" + expect(response).not_to have_turbo_stream action: "dialog" + expect(controller).not_to have_received(:build_move_to_sprint_dialog) end end - context "when all=true is in params" do - let(:params) { { project_id: project.id, id: work_package.id, all: "true" } } + context "with a blank id" do + let(:ids) { [second.id, ""] } - it "embeds the all query in the dialog form action URL" do - expect(body).to include("all=true") - end + it_behaves_like "rejects invalid dialog ids" end - context "with a user lacking manage_sprint_items permission" do - let(:user) { create(:user, member_with_permissions: { project => %i[view_sprints view_work_packages] }) } + context "with duplicate ids" do + let(:ids) { [second.id, second.id] } - it "responds with 403" do - expect(response).to have_http_status :forbidden - end + it_behaves_like "rejects invalid dialog ids" end - context "with a user lacking project permission" do - let(:user) { create(:user) } - - it "responds with 404" do - expect(response).to have_http_status :not_found + context "with an id from another project" do + let(:foreign_project) do + create(:project, name: "Foreign dialog project", identifier: "foreign-dialog-project") end - end - end - - describe "GET #move_to_bucket_dialog" do - let!(:displayed_buckets) { create_list(:backlog_bucket, 2, project:) } - let!(:other_bucket) { create(:backlog_bucket, project: create(:project)) } - - let(:params) { { project_id: project.id, id: work_package.id } } - - subject(:response) { get :move_to_bucket_dialog, params:, format: :turbo_stream } - - context "with a Sprint source" do - it "responds with a dialog turbo stream", :aggregate_failures do - expect(response).to be_successful - expect(response).to have_turbo_stream action: "dialog" + let(:foreign_work_package) do + create(:work_package, project: foreign_project, author: user, status:, type: type_feature) end + let(:ids) { [second.id, foreign_work_package.id] } - it "includes the project buckets as list_id options" do - subject + it_behaves_like "rejects invalid dialog ids" + end - displayed_buckets.each do |bucket| - expect(body).to include(%(value="#{bucket.id}")) - end + context "with an id the user cannot see" do + let(:user) do + create(:user, member_with_permissions: { + project => %i[view_work_packages view_sprints manage_sprint_items] + }) end - - it "does not include buckets from other projects" do - subject - - expect(body).not_to include(%(value="#{other_bucket.id}")) + let(:invisible_project) do + create(:project, public: false, name: "Invisible dialog project", identifier: "invisible-dialog-project") end - end - - context "when the work package is in a bucket" do - let(:current_bucket) { create(:backlog_bucket, project:) } - let(:current_bucket_wp) { create(:work_package, status:, project:, backlog_bucket: current_bucket) } - let(:params) { { project_id: project.id, id: current_bucket_wp.id } } - - it "responds with a dialog turbo stream" do - expect(response).to be_successful - expect(response).to have_turbo_stream action: "dialog" + let(:invisible_work_package) do + create(:work_package, project: invisible_project, author: user, status:, type: type_feature) end + let(:ids) { [second.id, invisible_work_package.id] } - it "excludes the current bucket from the options" do - subject + it_behaves_like "rejects invalid dialog ids" + end - expect(body).not_to include(%(value="#{current_bucket.id}")) - end + context "when every sprint destination is omitted" do + let!(:available_sprint) { sprint } + let!(:first) { create(:work_package, status:, project:, sprint:) } + let!(:second) { create(:work_package, status:, project:, sprint:) } - it "includes the other project buckets" do - displayed_buckets.each do |bucket| - expect(body).to include(%(value="#{bucket.id}")) - end + it "returns a 422 Turbo Stream without opening an empty dialog", :aggregate_failures do + expect(response).to have_http_status :unprocessable_entity + expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" + expect(body).to include(I18n.t("backlogs.work_packages.move_to_sprint_dialog.no_available_destinations")) + expect(response).not_to have_turbo_stream action: "dialog" + expect(controller).not_to have_received(:build_move_to_sprint_dialog) end end + end - context "when all=true is in params" do - let(:params) { { project_id: project.id, id: work_package.id, all: "true" } } + describe "POST #move_to_bucket_dialog" do + let!(:available_bucket) { create(:backlog_bucket, project:) } + let!(:first) { create(:work_package, status:, project:) } + let!(:second) { create(:work_package, status:, project:) } + let(:params) { { project_id: project.id, ids: [second.id, first.id] } } - it "embeds the all query in the dialog form action URL" do - expect(body).to include("all=true") - end + subject(:response) { post :move_to_bucket_dialog, params:, format: :turbo_stream } + + before do + allow(controller).to receive(:build_move_to_bucket_dialog).and_return(Object.new) + allow(controller).to receive(:respond_with_dialog) { controller.head :ok } end - context "with a user lacking manage_sprint_items permission" do - let(:user) { create(:user, member_with_permissions: { project => %i[view_sprints view_work_packages] }) } + it "passes the exact ordered collection and authoritative buckets to the dialog" do + response - it "responds with 403" do - expect(response).to have_http_status :forbidden - end + expect(controller) + .to have_received(:build_move_to_bucket_dialog) + .with( + work_packages: [second, first], + buckets: [available_bucket], + move_action: move_project_backlogs_work_packages_path(project) + ) end - context "with a user lacking project permission" do - let(:user) { create(:user) } + context "when every bucket destination is omitted" do + let!(:available_bucket) { create(:backlog_bucket, project:) } + let!(:first) { create(:work_package, status:, project:, backlog_bucket: available_bucket) } + let!(:second) { create(:work_package, status:, project:, backlog_bucket: available_bucket) } - it "responds with 404" do - expect(response).to have_http_status :not_found + it "returns a 422 Turbo Stream without opening an empty dialog", :aggregate_failures do + expect(response).to have_http_status :unprocessable_entity + expect(response).to have_turbo_stream action: "flash", target: "op-primer-flash-component" + expect(body).to include(I18n.t("backlogs.work_packages.move_to_bucket_dialog.no_available_destinations")) + expect(response).not_to have_turbo_stream action: "dialog" + expect(controller).not_to have_received(:build_move_to_bucket_dialog) end end end diff --git a/modules/backlogs/spec/features/inbox_column_spec.rb b/modules/backlogs/spec/features/inbox_column_spec.rb index 67989c756311..101aa098ff25 100644 --- a/modules/backlogs/spec/features/inbox_column_spec.rb +++ b/modules/backlogs/spec/features/inbox_column_spec.rb @@ -292,10 +292,8 @@ click_button "Move" end - planning_page - .expect_and_dismiss_error( - "Update failed: Sprint is not assignable since it is either not shared with the project or already finished." - ) + expected_error = I18n.t("backlogs.work_packages.batch_update_service.unavailable_target") + planning_page.expect_and_dismiss_error("Update failed: #{expected_error}") # Item was *not* moved: planning_page.expect_inbox_items(items: inbox_wp1) diff --git a/modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb b/modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb new file mode 100644 index 000000000000..de0a4ae303c8 --- /dev/null +++ b/modules/backlogs/spec/features/work_packages/batch_destination_menu_spec.rb @@ -0,0 +1,253 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" +require_relative "../../support/pages/backlog" + +RSpec.describe "Backlogs batch destination menus", + :js, + :selenium, + :settings_reset, + with_ee: %i[readonly_work_packages] do + let!(:project) do + create(:project, types: [type], enabled_module_names: %w(work_package_tracking backlogs)) + end + let(:type) { create(:type) } + let(:manage_sprint_items_role) do + create(:project_role, + permissions: %i[view_sprints manage_sprint_items view_work_packages edit_work_packages]) + end + let(:backlogs_page) { Pages::Backlog.new(project) } + + current_user do + create(:user, member_with_roles: { project => manage_sprint_items_role }) + end + + it "moves a selected cross-list batch in live document order and announces its appended range" do + source_sprint = create(:sprint, project:, name: "Source sprint") + destination_sprint = create(:sprint, project:, name: "Destination sprint") + bucket = create(:backlog_bucket, project:, name: "Source bucket") + destination_story = create(:work_package, project:, type:, sprint: destination_sprint, position: 1) + bucket_story = create(:work_package, project:, type:, backlog_bucket: bucket, position: 1) + sprint_story = create(:work_package, project:, type:, sprint: source_sprint, position: 1) + backlogs_page.visit! + + backlogs_page.select_cards(sprint_story, bucket_story) + backlogs_page.expect_selected_cards_in_order(bucket_story, sprint_story) + + backlogs_page.open_destination_dialog(sprint_story, "Move to sprint") + backlogs_page.expect_destination_dialog( + "Move to sprint", + work_packages: [bucket_story, sprint_story] + ) + backlogs_page.submit_destination_dialog( + "Move to sprint", + field_label: Sprint.human_model_name, + option: destination_sprint.name + ) + + backlogs_page.expect_work_packages_in_sprint_in_order( + destination_sprint, + work_packages: [destination_story, bucket_story, sprint_story] + ) + backlogs_page.expect_polite_announcement( + I18n.t( + "backlogs.work_packages.move_collection.moved_announcement", + count: 2, + list: destination_sprint.name, + first: 2, + last: 3, + total: 3 + ) + ) + backlogs_page.expect_no_selected_cards + + backlogs_page.visit! + backlogs_page.expect_work_packages_in_sprint_in_order( + destination_sprint, + work_packages: [destination_story, bucket_story, sprint_story] + ) + end + + it "replaces the old selection when an unselected card invokes a destination action" do + first_sprint = create(:sprint, project:, name: "First sprint") + second_sprint = create(:sprint, project:, name: "Second sprint") + bucket = create(:backlog_bucket, project:, name: "Source bucket") + selected_bucket_story = create(:work_package, project:, type:, backlog_bucket: bucket, position: 1) + selected_sprint_story = create(:work_package, project:, type:, sprint: first_sprint, position: 1) + invoker = create(:work_package, project:, type:, sprint: second_sprint, position: 1) + backlogs_page.visit! + + backlogs_page.select_cards(selected_sprint_story, selected_bucket_story) + backlogs_page.expect_selected_cards_in_order(selected_bucket_story, selected_sprint_story) + + backlogs_page.move_to_backlog_inbox(invoker) + + backlogs_page.expect_work_packages_in_inbox_in_order(work_packages: [invoker]) + backlogs_page.expect_work_packages_in_backlog_bucket_in_order(bucket, work_packages: [selected_bucket_story]) + backlogs_page.expect_work_packages_in_sprint_in_order(first_sprint, work_packages: [selected_sprint_story]) + backlogs_page.expect_no_selected_cards + + backlogs_page.visit! + backlogs_page.expect_work_packages_in_inbox_in_order(work_packages: [invoker]) + backlogs_page.expect_work_packages_in_backlog_bucket_in_order(bucket, work_packages: [selected_bucket_story]) + backlogs_page.expect_work_packages_in_sprint_in_order(first_sprint, work_packages: [selected_sprint_story]) + end + + it "offers a partly occupied destination and gathers the whole batch at its end" do + destination_sprint = create(:sprint, project:, name: "Destination sprint") + bucket = create(:backlog_bucket, project:, name: "Source bucket") + destination_story = create(:work_package, project:, type:, sprint: destination_sprint, position: 1) + selected_destination_story = create(:work_package, project:, type:, sprint: destination_sprint, position: 2) + bucket_story = create(:work_package, project:, type:, backlog_bucket: bucket, position: 1) + backlogs_page.visit! + + backlogs_page.select_cards(selected_destination_story, bucket_story) + backlogs_page.expect_selected_cards_in_order(bucket_story, selected_destination_story) + backlogs_page.expect_work_package_action(selected_destination_story, "Move to sprint") + + backlogs_page.open_destination_dialog(selected_destination_story, "Move to sprint") + backlogs_page.expect_destination_dialog_options( + "Move to sprint", + field_label: Sprint.human_model_name, + options: [destination_sprint.name] + ) + backlogs_page.submit_destination_dialog( + "Move to sprint", + field_label: Sprint.human_model_name, + option: destination_sprint.name + ) + + backlogs_page.expect_work_packages_in_sprint_in_order( + destination_sprint, + work_packages: [destination_story, bucket_story, selected_destination_story] + ) + + backlogs_page.visit! + backlogs_page.expect_work_packages_in_sprint_in_order( + destination_sprint, + work_packages: [destination_story, bucket_story, selected_destination_story] + ) + end + + it "intersects destinations for confined selections and offers none when their lists conflict" do + first_sprint = create(:sprint, project:, name: "First sprint") + second_sprint = create(:sprint, project:, name: "Second sprint") + bucket = create(:backlog_bucket, project:, name: "Source bucket") + readonly_status = create(:status, is_readonly: true) + confined_first = create(:work_package, project:, type:, sprint: first_sprint, status: readonly_status) + confined_second = create(:work_package, project:, type:, sprint: second_sprint, status: readonly_status) + free_story = create(:work_package, project:, type:, backlog_bucket: bucket) + backlogs_page.visit! + + backlogs_page.select_cards(confined_first, free_story) + backlogs_page.expect_destination_actions( + confined_first, + present: ["Move to sprint"], + absent: ["Move to backlog bucket", "Move to backlog inbox"] + ) + backlogs_page.open_destination_dialog(confined_first, "Move to sprint") + backlogs_page.expect_destination_dialog_options( + "Move to sprint", + field_label: Sprint.human_model_name, + options: [first_sprint.name] + ) + backlogs_page.cancel_destination_dialog("Move to sprint") + backlogs_page.clear_card_selection(confined_first) + + backlogs_page.select_cards(confined_first, confined_second) + backlogs_page.expect_destination_actions( + confined_first, + present: [], + absent: ["Move to sprint", "Move to backlog bucket", "Move to backlog inbox"] + ) + end + + it "omits Move to position from a multi-card action scope" do + sprint = create(:sprint, project:, name: "Sprint") + create(:backlog_bucket, project:, name: "Destination bucket") + first_story = create(:work_package, project:, type:, sprint:, position: 1) + second_story = create(:work_package, project:, type:, sprint:, position: 2) + backlogs_page.visit! + + backlogs_page.select_cards(first_story, second_story) + + backlogs_page.expect_no_work_package_action(first_story, "Move to position") + backlogs_page.expect_work_package_action(first_story, "Move to backlog bucket") + end + + it "shows feedback without an empty modal when the last destination disappears before loading" do + sprint = create(:sprint, project:, name: "Only sprint") + bucket = create(:backlog_bucket, project:, name: "Source bucket") + first_story = create(:work_package, project:, type:, backlog_bucket: bucket, position: 1) + second_story = create(:work_package, project:, type:, backlog_bucket: bucket, position: 2) + backlogs_page.visit! + + backlogs_page.select_cards(first_story, second_story) + backlogs_page.invoke_destination_action_after_menu_load(first_story, "Move to sprint") do + sprint.update_columns(status: "completed") + end + + backlogs_page.expect_move_error( + I18n.t("backlogs.work_packages.move_to_sprint_dialog.no_available_destinations") + ) + backlogs_page.expect_no_destination_dialog + backlogs_page.expect_selected_cards_in_order(first_story, second_story) + backlogs_page.expect_work_packages_in_backlog_bucket_in_order(bucket, work_packages: [first_story, second_story]) + end + + it "rejects the complete batch atomically and retains its selection" do + sprint = create(:sprint, project:, name: "Destination sprint") + bucket = create(:backlog_bucket, project:, name: "Source bucket") + first_story = create(:work_package, project:, type:, backlog_bucket: bucket, position: 1) + second_story = create(:work_package, project:, type:, backlog_bucket: bucket, position: 2) + backlogs_page.visit! + + backlogs_page.select_cards(first_story, second_story) + backlogs_page.open_destination_dialog(first_story, "Move to sprint") + + second_story.update_columns(status_id: create(:status, is_readonly: true).id) + backlogs_page.submit_destination_dialog( + "Move to sprint", + field_label: Sprint.human_model_name, + option: sprint.name, + frame_reload: false + ) + + backlogs_page.expect_move_error( + I18n.t("backlogs.work_packages.batch_update_service.unavailable_target") + ) + backlogs_page.expect_work_packages_in_backlog_bucket_in_order(bucket, work_packages: [first_story, second_story]) + backlogs_page.expect_selected_cards_in_order(first_story, second_story) + + backlogs_page.visit! + backlogs_page.expect_work_packages_in_backlog_bucket_in_order(bucket, work_packages: [first_story, second_story]) + end +end diff --git a/modules/backlogs/spec/requests/work_packages/move_collection_spec.rb b/modules/backlogs/spec/requests/work_packages/move_collection_spec.rb index 7e085fed013a..a972c8543272 100644 --- a/modules/backlogs/spec/requests/work_packages/move_collection_spec.rb +++ b/modules/backlogs/spec/requests/work_packages/move_collection_spec.rb @@ -53,6 +53,22 @@ def move_collection(ids:, **params) headers: { "Accept" => "text/vnd.turbo-stream.html" } end + def load_move_to_sprint_dialog(ids:) + post move_to_sprint_dialog_project_backlogs_work_packages_path(project), + params: { ids: }, + headers: { "Accept" => "text/vnd.turbo-stream.html" } + end + + def expect_moved_event(ids) + expect(response.body).to have_turbo_stream(action: "dispatchEvent") do |streams| + expect(streams.size).to eq(1) + expect(streams.first["event-name"]) + .to eq(Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT) + expect(JSON.parse(streams.first["detail"])) + .to eq("work_package_ids" => ids) + end + end + context "without the manage_sprint_items permission" do let(:permissions) { %i[view_work_packages edit_work_packages view_sprints] } @@ -135,6 +151,65 @@ def move_collection(ids:, **params) end describe "successful moves" do + context "with one visible member" do + it "reuses the singular move announcement", :aggregate_failures do + move_collection(ids: [bucket_wp1.id], list_type: "sprint", list_id: sprint.id) + + expect(response).to have_http_status(:ok) + expect(response.body).to include( + ERB::Util.html_escape( + I18n.t("backlogs.work_packages.move.moved_announcement", + label: bucket_wp1.to_fs(:caption), list: sprint.name, position: 2, total: 2) + ) + ) + expect_moved_event([bucket_wp1.id]) + end + end + + context "with two visible members" do + it "announces their count, destination, contiguous range, and total", :aggregate_failures do + moved_ids = [bucket_wp2.id, bucket_wp1.id] + + move_collection(ids: moved_ids, list_type: "sprint", list_id: sprint.id) + + expect(response).to have_http_status(:ok) + expect(response.body).to include( + ERB::Util.html_escape( + I18n.t("backlogs.work_packages.move_collection.moved_announcement", + count: 2, list: sprint.name, first: 2, last: 3, total: 3) + ) + ) + expect(sprint.work_packages_for(project).pluck(:id)).to eq [sprint_wp1.id, *moved_ids] + expect_moved_event(moved_ids) + end + end + + context "with members already persisted at the requested append placement" do + it "treats the stale-state submission as success and announces the ordered batch", :aggregate_failures do + frozen_ids = [bucket_wp2.id, bucket_wp1.id] + load_move_to_sprint_dialog(ids: frozen_ids) + expect(response).to have_http_status(:ok) + + concurrently_moved = Backlogs::WorkPackages::BatchUpdateService + .new(user:, work_packages: frozen_ids.map { |id| WorkPackage.find(id) }) + .call(list_type: "sprint", list_id: sprint.id) + expect(concurrently_moved).to be_success + expect(sprint.work_packages_for(project).pluck(:id)).to eq [sprint_wp1.id, *frozen_ids] + + move_collection(ids: frozen_ids, list_type: "sprint", list_id: sprint.id) + + expect(response).to have_http_status(:ok) + expect(response.body).to include( + ERB::Util.html_escape( + I18n.t("backlogs.work_packages.move_collection.moved_announcement", + count: 2, list: sprint.name, first: 2, last: 3, total: 3) + ) + ) + expect(sprint.work_packages_for(project).pluck(:id)).to eq [sprint_wp1.id, *frozen_ids] + expect_moved_event(frozen_ids) + end + end + 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, @@ -228,6 +303,25 @@ def move_collection(ids:, **params) expect(response).to have_http_status(:unprocessable_entity) expect(response.body).to include(ERB::Util.html_escape(sprint_wp3.reload.to_fs(:caption))) end + + it "rejects a destination that becomes unavailable after the dialog loads", :aggregate_failures do + frozen_ids = [bucket_wp2.id, bucket_wp1.id] + load_move_to_sprint_dialog(ids: frozen_ids) + expect(response).to have_http_status(:ok) + + positions_before = WorkPackage.order(:id).pluck(:id, :sprint_id, :backlog_bucket_id, :position) + sprint.update!(status: "completed") + + move_collection(ids: frozen_ids, list_type: "sprint", list_id: sprint.id) + + 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(WorkPackage.order(:id).pluck(:id, :sprint_id, :backlog_bucket_id, :position)) + .to eq(positions_before) + expect(response.body).not_to have_turbo_stream(action: "dispatchEvent") + end end describe "invisibility after move" do @@ -252,6 +346,7 @@ def move_collection(ids:, **params) expect(response.body).to include( ERB::Util.html_escape(I18n.t(:notice_work_package_invisible_after_move, count: 1, backlog: bucket.name)) ) + expect(response.body).not_to have_turbo_stream(action: "liveRegion") end end @@ -283,6 +378,7 @@ def move_collection(ids:, **params) I18n.t(:notice_work_package_invisible_after_move, count: 2, backlog: bucket.name) ) ) + expect(response.body).not_to have_turbo_stream(action: "liveRegion") 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 044ae50908fc..83982e293ccc 100644 --- a/modules/backlogs/spec/routing/backlogs/work_packages_routing_spec.rb +++ b/modules/backlogs/spec/routing/backlogs/work_packages_routing_spec.rb @@ -51,20 +51,18 @@ } it { - expect(get("/projects/project_42/backlogs/work_packages/85/move_to_sprint_dialog")).to route_to( + expect(post("/projects/project_42/backlogs/work_packages/move_to_sprint_dialog")).to route_to( controller: "backlogs/work_packages", action: "move_to_sprint_dialog", - project_id: "project_42", - id: "85" + project_id: "project_42" ) } it { - expect(get("/projects/project_42/backlogs/work_packages/85/move_to_bucket_dialog")).to route_to( + expect(post("/projects/project_42/backlogs/work_packages/move_to_bucket_dialog")).to route_to( controller: "backlogs/work_packages", action: "move_to_bucket_dialog", - project_id: "project_42", - id: "85" + project_id: "project_42" ) } 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 index 6a888797fecd..c3966f995c5b 100644 --- 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 @@ -441,6 +441,33 @@ def work_package_lock_ids(locked) end describe "target availability" do + it "revalidates a member that became confined after destination projection", with_ee: %i[readonly_work_packages] do + projected = Backlogs::WorkPackages::DestinationAvailability.new( + project:, + user:, + work_packages: [bucket_wp1, bucket_wp2] + ) + expect(projected.sprints).to include(sprint) + + readonly_status = create(:status, :readonly) + WorkPackage.where(id: bucket_wp2.id).update_all(status_id: readonly_status.id) + allow(Backlogs::WorkPackages::UpdateService).to receive(:new).and_call_original + + 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.unavailable_target") + expect(Backlogs::WorkPackages::UpdateService).not_to have_received(:new) + expect([bucket_wp1.reload, bucket_wp2.reload]) + .to match [ + have_attributes(backlog_bucket_id: bucket.id, sprint_id: nil, position: 1), + have_attributes(backlog_bucket_id: bucket.id, sprint_id: nil, position: 2) + ] + expect(sprint_order).to eq [sprint_wp1.id, sprint_wp2.id, sprint_wp3.id] + end + it "rejects a same-list reorder inside a sprint that completed after load" do sprint.update!(status: "completed") diff --git a/modules/backlogs/spec/services/backlogs/work_packages/destination_availability_spec.rb b/modules/backlogs/spec/services/backlogs/work_packages/destination_availability_spec.rb new file mode 100644 index 000000000000..989e31740e51 --- /dev/null +++ b/modules/backlogs/spec/services/backlogs/work_packages/destination_availability_spec.rb @@ -0,0 +1,157 @@ +# 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::DestinationAvailability, 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 view_sprints manage_sprint_items] + }) + end + shared_let(:readonly_status) { create(:status, :readonly) } + + let!(:later_sprint) do + create(:sprint, project:, name: "Later", start_date: 2.weeks.from_now, finish_date: 3.weeks.from_now) + end + let!(:earlier_sprint) do + create(:sprint, project:, name: "Earlier", start_date: 1.week.from_now, finish_date: 2.weeks.from_now) + end + let!(:completed_sprint) { create(:sprint, project:, status: :completed) } + let!(:zulu_bucket) { create(:backlog_bucket, project:, name: "Zulu") } + let!(:alpha_bucket) { create(:backlog_bucket, project:, name: "Alpha") } + let!(:foreign_bucket) { create(:backlog_bucket, project: create(:project), name: "Foreign") } + + def availability(work_packages, for_user: user) + described_class.new(project:, user: for_user, work_packages:) + end + + describe "destination intersection" do + it "offers every available project destination to free members in record order" do + first = create(:work_package, project:, type:) + second = create(:work_package, project:, type:, sprint: earlier_sprint) + + result = availability([first, second]) + + expect(result.sprints).to eq [earlier_sprint, later_sprint] + expect(result.buckets).to eq [alpha_bucket, zulu_bucket] + expect(result.inbox?).to be true + end + + it "intersects a confined member's current list with a free member", with_ee: %i[readonly_work_packages] do + confined = create(:work_package, project:, type:, status: readonly_status, sprint: earlier_sprint) + free = create(:work_package, project:, type:, backlog_bucket: alpha_bucket) + + result = availability([confined, free]) + + expect(result.sprints).to eq [earlier_sprint] + expect(result.buckets).to be_empty + expect(result.inbox?).to be false + end + + it "offers nothing when confined members are in different lists", with_ee: %i[readonly_work_packages] do + first = create(:work_package, project:, type:, status: readonly_status, sprint: earlier_sprint) + second = create(:work_package, project:, type:, status: readonly_status, backlog_bucket: alpha_bucket) + + result = availability([first, second]) + + expect(result.sprints).to be_empty + expect(result.buckets).to be_empty + expect(result.inbox?).to be false + end + + it "omits an all-members-current option without revoking positional reuse" do + work_packages = create_list(:work_package, 2, project:, type:, sprint: earlier_sprint) + current_target = Backlogs::Target.for(earlier_sprint) + + result = availability(work_packages) + + expect(result.sprints).to eq [later_sprint] + expect(result.permitted?(current_target)).to be true + end + + it "loads read-only status identity in one query for the whole batch", + with_ee: %i[readonly_work_packages] do + work_packages = create_list(:work_package, 5, project:, type:, status: readonly_status) + work_packages.each(&:reload) + + recorder = ActiveRecord::QueryRecorder.new do + availability(work_packages).permitted?(Backlogs::Target::InboxId) + end + + status_queries = recorder.log.grep(/FROM "statuses"/) + expect(status_queries.size).to eq 1 + end + + it "ignores persisted read-only flags when the feature is unavailable" do + persisted_readonly_status = create(:status) + persisted_readonly_status.update_column(:is_readonly, true) + work_package = create(:work_package, project:, type:, status: persisted_readonly_status) + + expect(availability([work_package]).permitted?(Backlogs::Target.for(alpha_bucket))).to be true + end + end + + describe "authoritative candidates" do + let(:work_package) { create(:work_package, project:, type:) } + + it "rejects completed or otherwise unassignable sprints" do + result = availability([work_package]) + + expect(result.sprints).not_to include(completed_sprint) + expect(result.permitted?(Backlogs::Target.for(completed_sprint))).to be false + end + + it "rejects buckets outside the batch project" do + result = availability([work_package]) + + expect(result.buckets).not_to include(foreign_bucket) + expect(result.permitted?(Backlogs::Target.for(foreign_bucket))).to be false + end + + it "rejects every destination without manage_sprint_items permission" do + unauthorized = create(:user, member_with_permissions: { + project => %i[view_work_packages view_sprints] + }) + result = availability([work_package], for_user: unauthorized) + + expect(result.sprints).to be_empty + expect(result.buckets).to be_empty + expect(result.inbox?).to be false + expect(result.permitted?(Backlogs::Target.for(earlier_sprint))).to be false + expect(result.permitted?(Backlogs::Target.for(alpha_bucket))).to be false + expect(result.permitted?(Backlogs::Target::InboxId)).to be false + end + end +end diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index ecbce1b28d67..8555b5bd08c9 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -743,6 +743,131 @@ def selected_card_ids all("[data-batch-selected]").pluck("data-sortable-lists--item-id-value") end + def select_cards(*work_packages) + work_packages.each { |work_package| toggle_card(work_package) } + end + + def expect_selected_cards_in_order(*work_packages) + expect(selected_card_ids).to eq(work_packages.map { |work_package| work_package.id.to_s }) + end + + def expect_no_selected_cards + expect(selected_card_ids).to be_empty + end + + def clear_card_selection(work_package) + work_package_card(work_package).send_keys(:escape) + expect_no_selected_cards + end + + def expect_work_package_action(work_package, action_label) + within_work_package_menu(work_package) do |menu| + expect(menu).to have_selector(:menuitem, text: action_label, exact_text: true) + end + end + + def expect_no_work_package_action(work_package, action_label) + within_work_package_menu(work_package) do |menu| + expect(menu).to have_no_selector(:menuitem, text: action_label, exact_text: true) + end + end + + def expect_destination_actions(work_package, present:, absent:) + within_work_package_menu(work_package) do |menu| + present.each do |label| + expect(menu).to have_selector(:menuitem, text: label, exact_text: true) + end + absent.each do |label| + expect(menu).to have_no_selector(:menuitem, text: label, exact_text: true) + end + end + end + + def open_destination_dialog(work_package, action_label, dialog_title: action_label) + click_in_work_package_menu(work_package, action_label, wait: false) + expect(page).to have_selector(:modal, text: dialog_title) + end + + def invoke_destination_action_after_menu_load(work_package, action_label, &before_click) + within_work_package_menu(work_package) do |menu| + item = menu.find(:menuitem, text: action_label, exact_text: true) + before_click&.call + wait_for_backlogs_turbo_stream do + item.click + end + end + end + + def move_to_backlog_inbox(work_package) + within_work_package_menu(work_package) do |menu| + wait_for_backlogs_turbo_stream(frame_reload: true) do + menu.find(:menuitem, text: "Move to backlog inbox", exact_text: true).click + end + end + end + + def expect_destination_dialog(dialog_title, work_packages:) + within_modal dialog_title do + work_packages.each { |work_package| expect(page).to have_text(work_package.subject) } + expect(all("input[name='ids[]']", visible: :all).map(&:value)) + .to eq(work_packages.map { |work_package| work_package.id.to_s }) + end + end + + def expect_destination_dialog_options(dialog_title, field_label:, options:) + within_modal dialog_title do + select = find(:select, field_label) + expect(select.all(:option).map(&:text)).to eq(options) + end + end + + def submit_destination_dialog(dialog_title, field_label:, option:, frame_reload: true) + within_modal dialog_title do + select option, from: field_label + wait_for_backlogs_turbo_stream(frame_reload:) { click_button I18n.t(:button_move) } + end + end + + def cancel_destination_dialog(dialog_title) + within_modal dialog_title do + click_button I18n.t(:button_cancel) + end + expect(page).to have_no_selector(:modal, text: dialog_title) + end + + def expect_no_destination_dialog + expect(page).to have_no_selector(:modal) + end + + def expect_move_error(reason) + expect_flash( + type: :error, + message: I18n.t(:notice_unsuccessful_update_with_reason, reason:) + ) + end + + def expect_polite_announcement(message) + wait_for do + page.evaluate_script("document.querySelector('live-region')?.getMessage('polite')") + end.to eq(message) + end + + def expect_persisted_sprint_order(sprint, *work_packages) + wait_for { sprint.work_packages_for(project).pluck(:id) } + .to eq(work_packages.map(&:id)) + end + + def expect_persisted_bucket_order(bucket, *work_packages) + wait_for { WorkPackage.where(backlog_bucket: bucket).order_by_position.pluck(:id) } + .to eq(work_packages.map(&:id)) + end + + def expect_persisted_inbox_order(*work_packages) + wait_for do + WorkPackage.where(project:, sprint_id: nil, backlog_bucket_id: nil).order_by_position.pluck(:id) + end.to eq(work_packages.map(&:id)) + end + # The shared description every selected card's `aria-describedby` points # at. Rendered once, permanently `hidden` — screen readers still reach it # through the reference despite that — so `visible: :all` is required.