diff --git a/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.spec.ts b/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.spec.ts new file mode 100644 index 000000000000..bdb14bfd63b1 --- /dev/null +++ b/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.spec.ts @@ -0,0 +1,246 @@ +//-- 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 { waitFor } from '@testing-library/dom'; +import { vi, type Mock } from 'vitest'; + +import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; +import type RefreshOnFormChangesControllerType from './refresh-on-form-changes.controller'; +import { serializeFormQuery } from './refresh-on-form-changes.controller'; + +describe('Refresh on form changes controller', () => { + let ctx:StimulusTestContext; + let RefreshOnFormChangesController:typeof RefreshOnFormChangesControllerType; + let fetchSpy:Mock; + let renderStreamMessage:Mock; + let originalTurbo:typeof window.Turbo; + + beforeAll(async () => { + ({ default: RefreshOnFormChangesController } = await import('./refresh-on-form-changes.controller')); + }); + + beforeEach(async () => { + fetchSpy = vi.fn().mockImplementation(() => Promise.resolve(turboStreamResponse())); + renderStreamMessage = vi.fn().mockResolvedValue(undefined); + + originalTurbo = window.Turbo; + window.Turbo = { + ...originalTurbo, + fetch: fetchSpy, + renderStreamMessage, + } as typeof window.Turbo; + vi.spyOn(window, 'fetch').mockImplementation(fetchSpy); + + ctx = await setupStimulusTest({ + controllers: { 'refresh-on-form-changes': RefreshOnFormChangesController }, + }); + }); + + afterEach(() => { + ctx.dispose(); + window.Turbo = originalTurbo; + vi.restoreAllMocks(); + }); + + function turboStreamResponse(html = '') { + return new Response(html, { + status: 200, + headers: { 'Content-Type': 'text/vnd.turbo-stream.html' }, + }); + } + + async function renderForm() { + await ctx.mount(` +
+ + +
+ `); + + return ctx.getController('refresh-on-form-changes'); + } + + it('requests a turbo stream refresh with the current form data as query params', async () => { + const controller = await renderForm(); + + controller.triggerTurboStream(); + + await waitFor(() => { + expect(fetchSpy).toHaveBeenCalled(); + }); + + const [url, init] = fetchSpy.mock.calls[0] as [string, RequestInit]; + const parsedUrl = new URL(url, window.location.origin); + + expect(parsedUrl.pathname).toBe('/refresh'); + expect(parsedUrl.searchParams.get('sprint[name]')).toBe('Created sprint'); + expect(parsedUrl.searchParams.get('sprint[goal][text]')).toBe('Deliver the first MVP scope.'); + expect(init).toEqual(expect.objectContaining({ + method: 'GET', + credentials: 'same-origin', + headers: expect.objectContaining({ + Accept: 'text/vnd.turbo-stream.html, text/html, application/xhtml+xml', + 'X-Requested-With': 'XMLHttpRequest', + }) as HeadersInit, + })); + await waitFor(() => { + expect(renderStreamMessage).toHaveBeenCalledWith( + '', + ); + }); + }); + + it('aborts an in-flight refresh before starting the next one', async () => { + const controller = await renderForm(); + let firstReject!:(error:DOMException) => void; + let firstSignal:AbortSignal|undefined; + + fetchSpy + .mockImplementationOnce((_url:string, init:RequestInit) => { + firstSignal = init.signal ?? undefined; + + return new Promise((_resolve, reject) => { + firstReject = reject; + }); + }) + .mockResolvedValueOnce(turboStreamResponse()); + + controller.triggerTurboStream(); + await waitFor(() => { + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + controller.triggerTurboStream(); + await waitFor(() => { + expect(fetchSpy).toHaveBeenCalledTimes(2); + }); + + expect(firstSignal?.aborted).toBe(true); + firstReject(new DOMException('The operation was aborted.', 'AbortError')); + + await waitFor(() => { + expect(renderStreamMessage).toHaveBeenCalledOnce(); + }); + }); + + it('aborts an in-flight refresh when disconnected', async () => { + const controller = await renderForm(); + let signal:AbortSignal|undefined; + + fetchSpy.mockImplementationOnce((_url:string, init:RequestInit) => { + signal = init.signal ?? undefined; + + return new Promise((resolve) => { + void resolve; + }); + }); + + controller.triggerTurboStream(); + await waitFor(() => { + expect(fetchSpy).toHaveBeenCalledOnce(); + }); + + ctx.container.querySelector('form')!.remove(); + await ctx.nextFrame(); + + expect(signal?.aborted).toBe(true); + }); + + it('coalesces rapid refresh requests into a single request with the latest form data', async () => { + const controller = await renderForm(); + const goalInput = ctx.container.querySelector('[name="sprint[goal][text]"]')!; + + controller.triggerTurboStream(); + goalInput.value = 'Updated goal'; + controller.triggerTurboStream(); + + await waitFor(() => { + expect(fetchSpy).toHaveBeenCalledOnce(); + }); + const [url] = fetchSpy.mock.calls[0] as [string, RequestInit]; + const parsedUrl = new URL(url, window.location.origin); + + expect(parsedUrl.searchParams.get('sprint[goal][text]')).toBe('Updated goal'); + }); + + describe('serializeFormQuery', () => { + function buildForm(innerHtml:string):HTMLFormElement { + const form = document.createElement('form'); + form.innerHTML = innerHtml; + return form; + } + + it('serializes string form fields into a query string', () => { + const form = buildForm(` + + + `); + + const params = new URLSearchParams(serializeFormQuery(form)); + + expect(params.get('sprint[name]')).toBe('Created sprint'); + expect(params.get('sprint[goal][text]')).toBe('Deliver the first MVP scope.'); + }); + + it('omits file inputs so File values never reach the query string', () => { + const form = buildForm(` + + + `); + const fileInput = form.querySelector('[name="sprint[attachment]"]')!; + const transfer = new DataTransfer(); + transfer.items.add(new File(['data'], 'report.pdf', { type: 'application/pdf' })); + fileInput.files = transfer.files; + + const params = new URLSearchParams(serializeFormQuery(form)); + + expect(params.has('sprint[attachment]')).toBe(false); + expect(params.get('sprint[name]')).toBe('Created sprint'); + }); + }); + + it('swallows abort errors but logs other request errors', async () => { + const controller = await renderForm(); + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + + fetchSpy.mockRejectedValueOnce(new DOMException('The operation was aborted.', 'AbortError')); + controller.triggerTurboStream(); + await waitFor(() => { + expect(fetchSpy).toHaveBeenCalledOnce(); + }); + expect(consoleError).not.toHaveBeenCalled(); + + fetchSpy.mockRejectedValueOnce(new Error('network down')); + controller.triggerTurboStream(); + await waitFor(() => { + expect(consoleError).toHaveBeenCalledWith(new Error('network down')); + }); + }); +}); diff --git a/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.ts b/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.ts index 8d4d068f55ce..6f5338236c2e 100644 --- a/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.ts +++ b/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.ts @@ -28,10 +28,29 @@ * ++ */ -import { ApplicationController } from 'stimulus-use'; -import { renderStreamMessage } from '@hotwired/turbo'; +import { ApplicationController, useDebounce } from 'stimulus-use'; +import { FetchRequest } from '@rails/request.js'; + +const TURBO_STREAM_REFRESH_DELAY = 50; + +// Serialize a form's fields into a query string. URLSearchParams does not accept +// a FormData object whose entries may hold File values, so copy the string +// entries across explicitly, dropping any file inputs. +export function serializeFormQuery(form:HTMLFormElement):string { + const params = new URLSearchParams(); + new FormData(form).forEach((value, key) => { + if (typeof value === 'string') { + params.append(key, value); + } + }); + return params.toString(); +} export default class RefreshOnFormChangesController extends ApplicationController { + static debounces = [ + { name: 'performTurboStreamRefresh', wait: TURBO_STREAM_REFRESH_DELAY }, + ]; + static targets = [ 'form', ]; @@ -46,23 +65,48 @@ export default class RefreshOnFormChangesController extends ApplicationControlle declare refreshUrlValue:string; declare turboStreamUrlValue:string; + private abortController:AbortController|null = null; + + connect():void { + useDebounce(this); + } + + disconnect():void { + this.abortController?.abort(); + this.abortController = null; + } + triggerReload():void { - window.location.href = `${this.refreshUrlValue}?${this.getSerializedFormData()}`; + window.location.replace(`${this.refreshUrlValue}?${serializeFormQuery(this.formTarget)}`); } - async triggerTurboStream():Promise { - await fetch(`${this.turboStreamUrlValue}?${this.getSerializedFormData()}`, { - headers: { - Accept: 'text/vnd.turbo-stream.html', - }, - }).then((r) => r.text()) - .then((html) => renderStreamMessage(html)); + triggerTurboStream():void { + void this.performTurboStreamRefresh(); } - private getSerializedFormData():string { - // without the cast to undefined, the URLSearchParams constructor will - // not accept the FormData object. - const formData = new FormData(this.formTarget) as unknown as undefined; - return new URLSearchParams(formData).toString(); + private async performTurboStreamRefresh():Promise { + // Cancel any refresh still in flight so a slower, earlier response cannot + // arrive last and overwrite the form with stale state (e.g. a half-entered + // date range clobbering the completed one). + this.abortController?.abort(); + const abortController = new AbortController(); + this.abortController = abortController; + + try { + const request = new FetchRequest('get', this.turboStreamUrlValue, { + query: new FormData(this.formTarget), + responseKind: 'turbo-stream', + signal: abortController.signal, + }); + await request.perform(); + } catch (error) { + if (!(error instanceof DOMException && error.name === 'AbortError')) { + console.error(error); + } + } finally { + if (this.abortController === abortController) { + this.abortController = null; + } + } } } diff --git a/modules/backlogs/app/components/backlogs/sprint_form_component.rb b/modules/backlogs/app/components/backlogs/sprint_form_component.rb index 2bed573eda4f..1345eec466d3 100644 --- a/modules/backlogs/app/components/backlogs/sprint_form_component.rb +++ b/modules/backlogs/app/components/backlogs/sprint_form_component.rb @@ -77,7 +77,7 @@ def banner_text end def goal - sprint.goals.find_or_initialize_by(project:) + sprint.goal_for(project) || sprint.goals.build(project:) end def goal_label diff --git a/modules/backlogs/app/models/sprint.rb b/modules/backlogs/app/models/sprint.rb index a06f17792446..c2926f939bac 100644 --- a/modules/backlogs/app/models/sprint.rb +++ b/modules/backlogs/app/models/sprint.rb @@ -113,11 +113,7 @@ def visible_to?(project) end def goal_for(project) - if goals.loaded? - goals.find { |goal| goal.project_id == project.id } - else - goals.find_by(project:) - end + goals.find { |goal| goal.project_id == project.id } end def goal_text_for(project) diff --git a/modules/backlogs/spec/features/backlogs/create_spec.rb b/modules/backlogs/spec/features/backlogs/create_spec.rb index 8fdadb30f878..b1b14b90465b 100644 --- a/modules/backlogs/spec/features/backlogs/create_spec.rb +++ b/modules/backlogs/spec/features/backlogs/create_spec.rb @@ -104,7 +104,11 @@ within_dialog "New sprint" do fill_in "Sprint name", with: "Created sprint" fill_in "Start date", with: start_date_fmt - fill_in "Finish date", with: finish_date_fmt + fill_in("Finish date", with: finish_date_fmt).send_keys(:tab) + # Wait for the date-triggered form refresh to settle before filling the + # goal, otherwise the in-flight refresh re-renders the form and discards + # the goal that was just typed. + expect(page).to have_field "Duration", with: "16 days", readonly: true fill_in "Sprint goal", with: "Deliver the first MVP scope." click_on "Create" @@ -114,6 +118,24 @@ planning_page.expect_sprint_heading_with_goal("Created sprint", "Deliver the first MVP scope.") end + it "keeps the sprint goal when entering the dates (regression #AGILE-301)" do + planning_page.visit! + + planning_page.open_create_sprint_dialog + + within_dialog "New sprint" do + fill_in "Sprint name", with: "Created sprint" + fill_in "Sprint goal", with: "Deliver the first MVP scope." + + fill_in "Start date", with: start_date_fmt + fill_in("Finish date", with: finish_date_fmt).send_keys(:tab) + + # Entering the dates triggers a form refresh; it must not wipe the goal. + expect(page).to have_field "Duration", with: "16 days", readonly: true + expect(page).to have_field "Sprint goal", with: "Deliver the first MVP scope." + end + end + it "previews the sprint duration when changing the dates" do planning_page.visit! @@ -123,7 +145,7 @@ expect(page).to have_field "Duration", with: "", readonly: true page.fill_in "Start date", with: start_date_fmt - page.fill_in "Finish date", with: finish_date_fmt + page.fill_in("Finish date", with: finish_date_fmt).send_keys(:tab) expect(page).to have_field "Duration", with: "16 days", readonly: true end @@ -155,7 +177,8 @@ within_dialog "New sprint" do page.fill_in "Start date", with: start_date_fmt - page.fill_in "Finish date", with: too_early_finish_date.strftime("%Y-%m-%d") + page.fill_in("Finish date", with: too_early_finish_date.strftime("%Y-%m-%d")) + .send_keys(:tab) # Shows duration as zero if finish date is before start date: expect(page).to have_field "Duration", with: "0 days", readonly: true diff --git a/modules/backlogs/spec/models/sprint_spec.rb b/modules/backlogs/spec/models/sprint_spec.rb index fe59d21ad020..8b5c3e2f86c4 100644 --- a/modules/backlogs/spec/models/sprint_spec.rb +++ b/modules/backlogs/spec/models/sprint_spec.rb @@ -443,6 +443,13 @@ other_project = create(:project) expect(sprint.goal_for(other_project)).to be_nil end + + it "returns an in-memory goal for an unsaved sprint" do + sprint = build(:sprint, project:) + sprint_goal = sprint.goals.build(project:, text: "Ship dashboard") + + expect(sprint.goal_for(project)).to eq(sprint_goal) + end end describe "#goal_text_for" do