From badfc6c3d73b13f21a55370c665f91b9d84eb139 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Mon, 22 Jun 2026 22:42:37 +0200 Subject: [PATCH 1/3] [AGILE-301] Preserve sprint goal on refresh Reuses `Sprint#goal_for` when rebuilding the sprint form so a date refresh no longer clears an unsaved goal. Covers the refresh path that previously wiped the field. https://community.openproject.org/wp/AGILE-301 --- .../backlogs/sprint_form_component.rb | 2 +- modules/backlogs/app/models/sprint.rb | 6 +--- .../spec/features/backlogs/create_spec.rb | 29 +++++++++++++++++-- modules/backlogs/spec/models/sprint_spec.rb | 7 +++++ 4 files changed, 35 insertions(+), 9 deletions(-) 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 From 04e21cb19032ee45a48799d53b0c7188093a759c Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Mon, 22 Jun 2026 22:42:38 +0200 Subject: [PATCH 2/3] [AGILE-301] Cancel stale form refreshes Debounces the Stimulus refresh and switches to `@rails/request.js`, aborting superseded requests so a slow earlier response cannot land last and overwrite the form with stale state. https://community.openproject.org/wp/AGILE-301 --- ...refresh-on-form-changes.controller.spec.ts | 209 ++++++++++++++++++ .../refresh-on-form-changes.controller.ts | 56 ++++- 2 files changed, 256 insertions(+), 9 deletions(-) create mode 100644 frontend/src/stimulus/controllers/refresh-on-form-changes.controller.spec.ts 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..b2607f03b8ff --- /dev/null +++ b/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.spec.ts @@ -0,0 +1,209 @@ +//-- 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'; + +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'); + }); + + 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..812f880a0755 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,16 @@ * ++ */ -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; export default class RefreshOnFormChangesController extends ApplicationController { + static debounces = [ + { name: 'performTurboStreamRefresh', wait: TURBO_STREAM_REFRESH_DELAY }, + ]; + static targets = [ 'form', ]; @@ -46,17 +52,49 @@ 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()}`; } - 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 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; + } + } } private getSerializedFormData():string { From 50bb2b0226a2f803fa96c462088566b44ac46a19 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Mon, 22 Jun 2026 23:11:46 +0200 Subject: [PATCH 3/3] Serialize refresh form data without File entries Extract the query serialization into serializeFormQuery, which copies only string entries so file inputs cannot leak into the URL and the FormData cast to undefined is gone. Use location.replace so a refresh does not push a history entry. --- ...refresh-on-form-changes.controller.spec.ts | 37 +++++++++++++++++++ .../refresh-on-form-changes.controller.ts | 22 +++++++---- 2 files changed, 51 insertions(+), 8 deletions(-) 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 index b2607f03b8ff..bdb14bfd63b1 100644 --- a/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.spec.ts +++ b/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.spec.ts @@ -31,6 +31,7 @@ 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; @@ -189,6 +190,42 @@ describe('Refresh on form changes controller', () => { 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); 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 812f880a0755..6f5338236c2e 100644 --- a/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.ts +++ b/frontend/src/stimulus/controllers/refresh-on-form-changes.controller.ts @@ -33,6 +33,19 @@ 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 }, @@ -64,7 +77,7 @@ export default class RefreshOnFormChangesController extends ApplicationControlle } triggerReload():void { - window.location.href = `${this.refreshUrlValue}?${this.getSerializedFormData()}`; + window.location.replace(`${this.refreshUrlValue}?${serializeFormQuery(this.formTarget)}`); } triggerTurboStream():void { @@ -96,11 +109,4 @@ export default class RefreshOnFormChangesController extends ApplicationControlle } } } - - 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(); - } }