diff --git a/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts new file mode 100644 index 000000000000..f612cc165252 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts @@ -0,0 +1,489 @@ +//-- 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 * as Turbo from '@hotwired/turbo'; +import type { MockInstance } from 'vitest'; +import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; +import type FiltersFormControllerType from './filters-form.controller'; + +interface VisitingSession { + visit:(location:string, options:object) => void; +} + +interface StatusRow { + hidden:boolean; + disabled:boolean; + value:string; +} + +const UNKNOWN_TO_SERVER:StatusRow = { hidden: true, disabled: false, value: '' }; +const APPLIED_ON_SERVER:StatusRow = { hidden: false, disabled: true, value: '2' }; + +describe('Filters form pending rows', () => { + let ctx:StimulusTestContext; + let Controller:typeof FiltersFormControllerType; + let originalUrl:string; + let visit:MockInstance; + + beforeAll(async () => { + ({ default: Controller } = await import('./filters-form.controller')); + }); + + beforeEach(() => { + originalUrl = window.location.href; + visit = vi.spyOn(Turbo.session as unknown as VisitingSession, 'visit').mockImplementation(() => undefined); + }); + + afterEach(() => { + vi.restoreAllMocks(); + ctx.dispose(); + window.history.replaceState(window.history.state, '', originalUrl); + }); + + function form(statusRow:StatusRow) { + return `
+ +
+ +
+ +
+
+ +
`; + } + + async function mount(html = form(UNKNOWN_TO_SERVER)) { + ctx = await setupStimulusTest({ controllers: { 'filter--filters-form': Controller } }); + await ctx.mount(html); + return ctx.getController('filter--filters-form'); + } + + function host():HTMLElement { + return ctx.container.querySelector('[data-controller]')!; + } + + // getByRole returns HTMLElement; the generic narrows it where `.value` is read or written. + const addFilterSelect = () => ctx.screen.getByRole('combobox', { name: 'Add filter' }); + const statusOperator = () => ctx.screen.getByRole('combobox', { name: 'Status operator', hidden: true }); + const subjectOperator = () => ctx.screen.getByRole('combobox', { name: 'Subject operator', hidden: true }); + const subjectRow = () => subjectOperator().closest('[data-filter--filters-form-target="filter"]')!; + const statusOption = () => ctx.screen.getByRole('option', { name: 'Status', hidden: true }); + const subjectOption = () => ctx.screen.getByRole('option', { name: 'Subject', hidden: true }); + const statusValue = () => ctx.container.querySelector('[data-filter-name="status_id"] input[name="value"]')!; + const subjectValue = () => ctx.screen.getByRole('textbox', { name: 'Subject value', hidden: true }); + + function addStatusFilter(controller:FiltersFormControllerType) { + addFilterSelect().value = 'status_id'; + controller.addFilterByName('status_id'); + } + + function selectStatus(id:string) { + statusValue().value = id; + } + + // Turbo's snapshot clone carries live control values, not just markup. `element` is not + // part of the published typings. + async function restoreCachedPage() { + const { element: cached } = Turbo.PageSnapshot.fromElement(host()).clone() as unknown as { element:HTMLElement }; + ctx.dispose(); + ctx = await setupStimulusTest({ controllers: { 'filter--filters-form': Controller } }); + ctx.container.append(cached); + await ctx.nextFrame(); + return ctx.getController('filter--filters-form'); + } + + function rerenderFromServer(statusRow:StatusRow) { + const template = document.createElement('template'); + template.innerHTML = form(statusRow); + Turbo.morphElements(host(), template.content.firstElementChild!); + } + + it('keeps an added row visible through a re-render that does not know it yet', async () => { + const controller = await mount(); + + addStatusFilter(controller); + expect(statusOperator()).toBeVisible(); + expect(statusOption()).toBeDisabled(); + + // Two morphs: Idiomorph syncs the incoming attributes (hidden) before it removes the ones + // only the current element has (the marker), so a single morph passes even when the + // marker itself is not protected. + rerenderFromServer(UNKNOWN_TO_SERVER); + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(statusOperator()).toBeVisible(); + expect(statusOption()).toBeDisabled(); + }); + + it("keeps a pending row's operator and draft through a re-render that does not know it", async () => { + const controller = await mount(); + addFilterSelect().value = 'subject'; + controller.addFilterByName('subject'); + subjectOperator().value = '!~'; + subjectValue().value = 'draft'; + + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(subjectOperator()).toBeVisible(); + expect(subjectOperator()).toHaveValue('!~'); + expect(subjectValue()).toHaveValue('draft'); + }); + + it('hands the row to the server once it has been submitted with a value', async () => { + const controller = await mount(); + addStatusFilter(controller); + + selectStatus('2'); + controller.autocompleteSendForm(); + rerenderFromServer(APPLIED_ON_SERVER); + + expect(statusOperator()).toBeVisible(); + expect(statusOption()).toBeDisabled(); + expect(statusValue()).toHaveValue('2'); + }); + + it('no longer protects a submitted row from a response that drops it', async () => { + const controller = await mount(); + addStatusFilter(controller); + + selectStatus('2'); + controller.autocompleteSendForm(); + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(statusOperator()).not.toBeVisible(); + expect(statusOption()).toBeEnabled(); + }); + + it('lets the server own a row that was removed again', async () => { + const controller = await mount(); + addStatusFilter(controller); + + controller.removeFilter({ params: { filterName: 'status_id' } }); + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(statusOperator()).not.toBeVisible(); + expect(statusOption()).toBeEnabled(); + }); + + it('does not keep other rows from being hidden by the server', async () => { + const controller = await mount(); + addStatusFilter(controller); + + // The operator select carries data-filter-name too, so the row is reached via its target attribute. + subjectRow().hidden = false; + expect(subjectOperator()).toBeVisible(); + + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(subjectOperator()).not.toBeVisible(); + expect(statusOperator()).toBeVisible(); + }); + + it('takes the add-filter option without the caller selecting it first', async () => { + const controller = await mount(); + + controller.addFilterByName('status_id'); + + expect(statusOption()).toBeDisabled(); + expect(addFilterSelect()).toHaveValue(''); + expect(statusOperator()).toBeVisible(); + }); + + it('drops an unsent row when a cached page is restored, keeping applied rows', async () => { + // The server applied "subject ~ x": row visible, option taken, value present. + const controller = await mount(form(UNKNOWN_TO_SERVER) + .replace('data-filter-name="subject" data-filter-type="string" hidden', 'data-filter-name="subject" data-filter-type="string"') + .replace('', '') + .replace(' { + const controller = await mount(); + addFilterSelect().value = 'subject'; + controller.addFilterByName('subject'); + subjectValue().value = 'unsent draft'; + + const restored = await restoreCachedPage(); + + expect(subjectOperator()).not.toBeVisible(); + expect(subjectValue()).toHaveValue(''); + + addFilterSelect().value = 'subject'; + restored.addFilterByName('subject'); + + expect(subjectOperator()).toBeVisible(); + expect(visit).not.toHaveBeenCalled(); + }); + + describe('with a date range row', () => { + function dateRow(rangeValue:string) { + return `
+ + +
`; + } + + const rangeOperator = () => ctx.screen.getByRole('combobox', { name: 'Dates interval operator', hidden: true }); + const rangeInput = () => ctx.screen.getByRole('textbox', { name: 'Dates interval range', hidden: true }); + + function rerenderDateRow() { + const template = document.createElement('template'); + template.innerHTML = dateRow('-'); + Turbo.morphElements(host(), template.content.firstElementChild!); + } + + async function addDatesInterval(rangeValue:string) { + const controller = await mount(dateRow(rangeValue)); + addFilterSelect().value = 'dates_interval'; + controller.addFilterByName('dates_interval'); + return controller; + } + + // The picker renders an empty range as "-"; a cleared picker leaves "". + it.each(['-', ''])('keeps a range row pending while its input holds %j', async (rangeValue) => { + const controller = await addDatesInterval(rangeValue); + + expect(controller.serializedFiltersWith()).toBe(''); + + rerenderDateRow(); + + expect(rangeOperator()).toBeVisible(); + expect(controller.serializedFiltersWith()).toBe(''); + }); + + it.each([ + ['2026-09-01 - ', '["2026-09-01",""]'], + [' - 2026-09-30', '["","2026-09-30"]'], + ])('still submits the one-sided range %j', async (rangeValue, serialized) => { + const controller = await addDatesInterval('-'); + + rangeInput().value = rangeValue; + controller.sendForm(); + + expect(controller.serializedFiltersWith()).toBe(`dates_interval <>d ${serialized}`); + }); + + it('releases the range row once both dates are set', async () => { + const controller = await addDatesInterval('-'); + + rangeInput().value = '2026-09-01 - 2026-09-30'; + controller.sendForm(); + expect(controller.serializedFiltersWith()).toBe('dates_interval <>d ["2026-09-01","2026-09-30"]'); + + rerenderDateRow(); + + expect(rangeOperator()).not.toBeVisible(); + expect(controller.serializedFiltersWith()).toBe(''); + }); + }); + + describe('with a date row that swaps its picker per operator', () => { + const FILTER_NAME = 'created_at'; + + beforeAll(async () => { + await import('@primer/view-components/app/lib/primer/forms/primer_multi_input'); + }); + + // Mirrors Filters::Inputs::DateForm: one picker per operator inside a primer-multi-input, + // with the inactive one hidden and disabled by activateField(). + function dateRow() { + return `
+ + +
`; + } + + const dateOperator = () => ctx.screen.getByRole('combobox', { name: 'Created on operator', hidden: true }); + // An input carrying `hidden` drops out of the role tree, so the pickers are found by name. + const dayInput = () => ctx.container.querySelector('[data-name="singleDay"]')!; + const rangeInput = () => ctx.container.querySelector('[data-name="dateRange"]')!; + + function addCreatedOn(controller:FiltersFormControllerType) { + addFilterSelect().value = FILTER_NAME; + controller.addFilterByName(FILTER_NAME); + } + + function switchOperatorTo(controller:FiltersFormControllerType, operator:string) { + dateOperator().value = operator; + controller.setValueVisibility({ target: dateOperator(), params: { filterName: FILTER_NAME } }); + } + + function rerenderCreatedOn() { + const template = document.createElement('template'); + template.innerHTML = dateRow(); + Turbo.morphElements(host(), template.content.firstElementChild!); + } + + it('keeps the picker of a chosen operator through a re-render that does not know the row', async () => { + const controller = await mount(dateRow()); + addCreatedOn(controller); + switchOperatorTo(controller, '<>d'); + + rerenderCreatedOn(); + + expect(dateOperator()).toHaveValue('<>d'); + expect(rangeInput()).toBeVisible(); + expect(dayInput()).not.toBeVisible(); + }); + + it('shows the picker of the default operator again after a cached page is restored', async () => { + const controller = await mount(dateRow()); + addCreatedOn(controller); + switchOperatorTo(controller, '<>d'); + + expect(rangeInput()).toBeVisible(); + expect(dayInput()).not.toBeVisible(); + + const restored = await restoreCachedPage(); + addCreatedOn(restored); + + expect(dateOperator()).toHaveValue('=d'); + expect(dayInput()).toBeVisible(); + expect(dayInput()).toBeEnabled(); + expect(rangeInput()).not.toBeVisible(); + }); + }); + + describe('on the Meetings transport (JSON filters, stream morph of the subheader)', () => { + function subheader(statusRow:StatusRow) { + return form(statusRow) + .replace('data-filter--filters-form-turbo-frame-request-value="backlogs_container"', + 'data-filter--filters-form-turbo-stream-request-value="true" data-filter--filters-form-output-format-value="json"'); + } + + function streamResponse(statusRow:StatusRow) { + return ` + + `; + } + + let fetchSpy:ReturnType; + + beforeEach(() => { + document.body.insertAdjacentHTML('beforeend', ''); + fetchSpy = vi.fn((_url:string) => Promise.resolve(new Response(streamResponse(UNKNOWN_TO_SERVER)))); + vi.stubGlobal('fetch', fetchSpy); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + document.getElementById('global-loading-indicator')?.remove(); + }); + + it('keeps the added row through the morphed response and never sends it', async () => { + ctx = await setupStimulusTest({ controllers: { 'filter--filters-form': Controller } }); + await ctx.mount(`
${subheader(UNKNOWN_TO_SERVER)}
`); + const controller = ctx.getController('filter--filters-form'); + window.history.replaceState(window.history.state, '', `${window.location.pathname}?filters=[]`); + + addStatusFilter(controller); + // Adding the empty row alone changes nothing the server would receive, so sendForm() + // returns early; an unrelated (unowned) filter change forces the request. + controller.currentFiltersValue = [{ title: { operator: '~', values: ['x'] } }]; + controller.sendForm(); + + await vi.waitFor(() => expect(ctx.container.querySelector('[data-rendered]')).not.toBeNull()); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + const filtersParam = new URL(String(fetchSpy.mock.calls[0][0]), window.location.origin).searchParams.get('filters'); + expect(filtersParam).toBe('[{"title":{"operator":"~","values":["x"]}}]'); + expect(filtersParam).not.toContain('status_id'); + expect(statusOperator()).toBeVisible(); + expect(statusOption()).toBeDisabled(); + }); + }); +}); diff --git a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts index 81aaf1511bd4..1d99e8e4cc9c 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts @@ -27,7 +27,12 @@ //++ import { Controller } from '@hotwired/stimulus'; -import { renderStreamMessage, visit } from '@hotwired/turbo'; +import { + renderStreamMessage, + visit, + type TurboBeforeMorphAttributeEvent, + type TurboBeforeMorphElementEvent, +} from '@hotwired/turbo'; import { debounce } from 'lodash-es'; import { hideElement, @@ -50,6 +55,24 @@ type SerializedFilter = Record; type FilterFunc = (_value:T) => boolean; +// Marks a row the user added that holds no value yet. It is not part of any request, so a +// re-render of the form would hide it again; the morph guard keeps marked rows as they are. +const PENDING_ATTRIBUTE = 'data-filter-pending'; + +// Turbo's page snapshot keeps live control values, so a hidden row would otherwise still +// carry the draft the user typed and submit it the moment the filter is added again. +function resetControls(row:HTMLElement) { + row.querySelectorAll('input, select, textarea').forEach((control) => { + if (control instanceof HTMLSelectElement) { + Array.from(control.options).forEach((option) => { option.selected = option.defaultSelected; }); + } else if (control instanceof HTMLInputElement && (control.type === 'checkbox' || control.type === 'radio')) { + control.checked = control.defaultChecked; + } else { + control.value = control.defaultValue; + } + }); +} + export default class FiltersFormController extends Controller { static targets = [ 'filterFormToggle', @@ -114,24 +137,30 @@ export default class FiltersFormController extends Controller { }); private boundListener:() => void; - private boundClearListener:(event:MouseEvent) => void; + private abortController?:AbortController; private sentFilters:string|null = null; initialize() { // Initialize runs anytime an element with a controller connected to the DOM for the first time this.boundListener = debounce(this.sendFormLive.bind(this), 300); - - this.boundClearListener = (event:MouseEvent) => this.clearInputWithButton(event); } connect() { + this.abortController = new AbortController(); + const { signal } = this.abortController; + const clearButton = document.getElementById(this.clearButtonIdValue); - clearButton?.addEventListener('click', this.boundClearListener); + clearButton?.addEventListener('click', (event) => this.clearInputWithButton(event), { signal }); + this.element.addEventListener('turbo:before-morph-element', this.keepPendingRows, { signal }); + this.element.addEventListener('turbo:before-morph-attribute', this.keepPendingOptionsTaken, { signal }); + + // A restored page brings its markup back but not this controller's model, so a marker + // already in the DOM here belongs to whoever was cached. + this.pendingRows().forEach((row) => this.releasePendingRow(row, { hide: true })); } disconnect() { - const clearButton = document.getElementById(this.clearButtonIdValue); - clearButton?.removeEventListener('click', this.boundClearListener); + this.abortController?.abort(); } addFilterSelectTargetConnected() { @@ -298,8 +327,9 @@ export default class FiltersFormController extends Controller { const selectedFilter = this.findTargetByName(filterName, this.filterTargets); if (selectedFilter) { selectedFilter.removeAttribute('hidden'); + selectedFilter.setAttribute(PENDING_ATTRIBUTE, ''); } - this.addFilterSelectTarget.selectedOptions[0].disabled = true; + this.setFilterOptionTaken(filterName, true); this.addFilterSelectTarget.selectedIndex = 0; this.focusFilterValueIfPossible(selectedFilter); @@ -342,15 +372,76 @@ export default class FiltersFormController extends Controller { removeFilter({ params: { filterName } }:{ params:{ filterName:string } }) { const filterToRemove = this.findTargetByName(filterName, this.filterTargets); - filterToRemove?.setAttribute('hidden', ''); - - const selectOptions = Array.from(this.addFilterSelectTarget.options); - const removedFilterOption = selectOptions.find((option) => option.value === filterName); - removedFilterOption?.removeAttribute('disabled'); + if (filterToRemove) { + filterToRemove.setAttribute('hidden', ''); + filterToRemove.removeAttribute(PENDING_ATTRIBUTE); + } + this.setFilterOptionTaken(filterName, false); this.sendFormLive(); } + private setFilterOptionTaken(filterName:string, taken:boolean) { + const option = Array.from(this.addFilterSelectTarget.options).find((candidate) => candidate.value === filterName); + if (option) { + option.disabled = taken; + } + } + + private pendingRows():HTMLElement[] { + return this.filterTargets.filter((row) => row.hasAttribute(PENDING_ATTRIBUTE)); + } + + private releaseSubmittedPendingRows() { + const submitted = new Set(this.parseFilters().map((filter) => filter.name)); + this.pendingRows() + .filter((row) => submitted.has(row.dataset.filterName!)) + .forEach((row) => this.releasePendingRow(row, { hide: false })); + } + + private releasePendingRow(row:HTMLElement, { hide }:{ hide:boolean }) { + row.removeAttribute(PENDING_ATTRIBUTE); + if (hide) { + row.setAttribute('hidden', ''); + this.setFilterOptionTaken(row.dataset.filterName!, false); + resetControls(row); + this.showValueForOperator(row.dataset.filterName!); + } + } + + // Which picker is shown, and whether the value container is shown at all, follows the + // operator through attributes that a reset of control values leaves untouched. + private showValueForOperator(filterName:string) { + const operator = this.findTargetByName(filterName, this.operatorTargets); + if (operator) { + this.setValueVisibility({ target: operator, params: { filterName } }); + } + } + + // The server does not know a pending row, so its response would hide the row and put its + // operator, draft value and active picker back to defaults. The row is left out of the morph. + private readonly keepPendingRows = (event:TurboBeforeMorphElementEvent) => { + const target = event.target as HTMLElement; + + if (this.filterTargets.includes(target) && target.hasAttribute(PENDING_ATTRIBUTE)) { + event.preventDefault(); + } + }; + + private readonly keepPendingOptionsTaken = (event:TurboBeforeMorphAttributeEvent) => { + const { attributeName } = event.detail; + const target = event.target as HTMLElement; + + const keepsOptionTaken = attributeName === 'disabled' + && target instanceof HTMLOptionElement + && target.closest('select') === this.addFilterSelectTarget + && this.pendingRows().some((row) => row.dataset.filterName === target.value); + + if (keepsOptionTaken) { + event.preventDefault(); + } + }; + clearInputWithButton(event:MouseEvent) { // Primer does not trigger an input event when clearing the value of the input field unless // it is focused. This handler will find the sibling input of the clear button inside the @@ -451,6 +542,8 @@ export default class FiltersFormController extends Controller { } sendForm() { + this.releaseSubmittedPendingRows(); + // When we want the filter content to be written to a hidden input, do this. // When we do not also want the turbo requests, we can exit early here. Otherwise the automatic redirect // would also be triggered. We do not want this in the case where we use the filter input in another form @@ -644,10 +737,11 @@ export default class FiltersFormController extends Controller { value = [dateValue].filter((v) => v !== ''); } else if (operator === this.betweenDatesOperator) { - const rangeValue = this.findTargetById(filterName, this.dateRangeTargets)?.value; - const [fromValue, toValue] = rangeValue?.split(' - ') ?? []; + // The range picker renders an empty range as "-" (see Filters::Inputs::DateForm#between_dates_div). + const rangeValue = this.findTargetById(filterName, this.dateRangeTargets)?.value ?? ''; + const [fromValue = '', toValue = ''] = rangeValue === '-' ? [] : rangeValue.split(' - '); - value = [fromValue, toValue]; + value = fromValue === '' && toValue === '' ? [] : [fromValue, toValue]; } if (value && value.length > 0) { return value; diff --git a/spec/features/projects/lists/filters_spec.rb b/spec/features/projects/lists/filters_spec.rb index 8325603f6469..fb05b292b0f3 100644 --- a/spec/features/projects/lists/filters_spec.rb +++ b/spec/features/projects/lists/filters_spec.rb @@ -444,10 +444,22 @@ def load_and_open_filters(user) project_created_on_this_week, project_created_on_fixed_date) + # a range row without dates yet is not a filter: the list and the URL stay as they are. + # The operator change submits behind a 300 ms debounce, so the quiet period must outlast it + # for the assertions below to see a request that should not have happened. projects_page.set_filter("created_at", "Created on", - "between", - ["2017-11-10", "2017-11-12"]) + "between") + wait_for_network_idle(duration: 0.5) + + projects_page.expect_projects_listed(project_created_on_today, + project_created_on_this_week, + project_created_on_fixed_date) + expect(page).to have_no_current_path(/created_at/) + + projects_page.within_filter("created_at") do + projects_page.set_datetime_filter("created_at", "between", ["2017-11-10", "2017-11-12"]) + end projects_page.expect_projects_not_listed(project_created_on_today) projects_page.expect_projects_listed(project_created_on_fixed_date)