From 65f8a774d64c0c600c8c90741a4c33036b77b3ea Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Tue, 23 Jun 2026 19:30:24 +0200 Subject: [PATCH 1/2] fix(books): address CodeRabbit findings on the series autocomplete (#179) CodeRabbit's inline review (5 Major findings) landed after #187 was merged and the docs follow-up commit was rate-limited, so they went unaddressed. This applies them. - Security/convention: the four series autocomplete fields used HtmlHelper::e() in the view; views must use htmlspecialchars(..., ENT_QUOTES, 'UTF-8') (12 occurrences across option value/text + hidden input). - Correctness (#74 class): the series _onEnterKey only committed the typed value when NOTHING was highlighted. With a highlighted suggestion that differs from the typed text, Enter fell through to Choices and committed the wrong value. Apply the same highlighted-vs-typed guard the publisher field already uses; add a dedicated regression test. - Tests: the issue-179 skip guard now also requires E2E_ADMIN_PASS (not just the email), so a missing password skips instead of failing at login. - Tests: setSeriesAutocomplete (series-cycles) and the collana fill (full-test) now drive the real Choices widget (open -> type -> Enter) instead of poking the hidden input, so they catch widget regressions. The helper stays context-aware: pages without the widget (series-detail admin form) fall back to a plain fill. Validated: PHPStan L5, php -l, issue-179 6/6, series-cycles 15/15, full-test 135/135. --- app/Views/libri/partials/book_form.php | 28 ++++++++++---- tests/full-test.spec.js | 21 ++++++----- tests/issue-179-series-autocomplete.spec.js | 27 +++++++++++++- tests/series-cycles.spec.js | 41 ++++++++++++++++----- 4 files changed, 89 insertions(+), 28 deletions(-) diff --git a/app/Views/libri/partials/book_form.php b/app/Views/libri/partials/book_form.php index 7710ed21..7fa44086 100644 --- a/app/Views/libri/partials/book_form.php +++ b/app/Views/libri/partials/book_form.php @@ -508,9 +508,9 @@ - +

@@ -518,9 +518,9 @@ - +

@@ -537,9 +537,9 @@ - +

@@ -554,9 +554,9 @@ - +
@@ -2232,6 +2232,18 @@ classNames: { containerInner: 'choices__inner' } const typed = input ? input.value.trim() : ''; const dd = wrapper ? wrapper.querySelector('.choices__list--dropdown') : null; const hl = dd ? dd.querySelector('.choices__item--selectable.is-highlighted') : null; + // A suggestion is highlighted but the user typed something + // different: commit the TYPED value instead of letting Choices + // pick the highlight (same guard as the publisher field, #74). + if (typed && hl) { + const nameEl = hl.querySelector('.choices__item-text') || hl.childNodes[0]; + const highlightedText = (nameEl ? nameEl.textContent : hl.textContent).trim().toLowerCase(); + if (highlightedText !== typed.toLowerCase()) { + event.preventDefault(); + commitTyped(typed); + return; + } + } if (typed && !hl) { event.preventDefault(); commitTyped(typed); return; } return origEnter(event, hasActiveDropdown); }; diff --git a/tests/full-test.spec.js b/tests/full-test.spec.js index d99e5bbe..40cad364 100644 --- a/tests/full-test.spec.js +++ b/tests/full-test.spec.js @@ -687,16 +687,17 @@ test.describe.serial('Phase 3: Manual Book Creation', () => { await copies.fill('2'); } - // Series (collana) — now a Choices.js autocomplete (#179) whose submitted - // value lives in a hidden input set via the exposed setter. - await page.evaluate((v) => { - if (window.__seriesAutocomplete && window.__seriesAutocomplete.collana) { - window.__seriesAutocomplete.collana(v); - } else { - const h = document.getElementById('collana'); - if (h) h.value = v; - } - }, `TestSeries_${RUN_ID}`); + // Series (collana) — now a Choices.js autocomplete (#179). Drive the real + // widget (open → type → Enter): Enter goes through the Choices keypath and + // commits the typed value, exercising the widget instead of poking the + // hidden input directly. Works for the brand-new name created here. + const seriesValue = `TestSeries_${RUN_ID}`; + const collanaWrapper = page.locator('.choices', { has: page.locator('#collana_select') }); + await collanaWrapper.click(); + const collanaSearch = collanaWrapper.locator('input[type="search"], .choices__input--cloned').first(); + await collanaSearch.fill(seriesValue); + await collanaSearch.press('Enter'); + await expect(page.locator('#collana')).toHaveValue(seriesValue); // Notes const note = page.locator('#note_varie, textarea[name="note_varie"]'); diff --git a/tests/issue-179-series-autocomplete.spec.js b/tests/issue-179-series-autocomplete.spec.js index 91baf2a7..220c0af7 100644 --- a/tests/issue-179-series-autocomplete.spec.js +++ b/tests/issue-179-series-autocomplete.spec.js @@ -12,7 +12,7 @@ const ADMIN_PASS = process.env.E2E_ADMIN_PASS || ''; let page; test.beforeAll(async ({ browser }) => { - test.skip(!ADMIN_EMAIL, 'creds not configured (use /tmp/run-e2e.sh)'); + test.skip(!ADMIN_EMAIL || !ADMIN_PASS, 'E2E admin credentials not configured (set E2E_ADMIN_EMAIL and E2E_ADMIN_PASS, e.g. via /tmp/run-e2e.sh)'); page = await browser.newPage(); await page.goto(`${BASE}/accedi`); await page.fill('input[name="email"]', ADMIN_EMAIL); @@ -59,6 +59,31 @@ test('Typing a brand-new universe name commits it (create-new path)', async () = await expect(page.locator('#serie_padre')).toHaveValue(NEW); }); +test('Enter commits the TYPED value even when a different suggestion is highlighted (#74)', async () => { + await page.goto(`${BASE}/admin/books/create`); + await page.waitForSelector('#serie_padre_select', { state: 'attached', timeout: 10000 }); + + // "Seed: Fairy" is a prefix of the existing universe "Seed: Fairy Tail + // Universe": typing it lists BOTH the typed value and the existing one. We + // move the highlight onto the EXISTING suggestion (different from the typed + // text) and press Enter — the field MUST commit the typed text, not the + // highlighted suggestion. Without the _onEnterKey guard this regresses to the + // issue #74 bug (wrong value committed). + const PARTIAL = 'Seed: Fairy'; + await universeWrapper().click(); + const search = universeWrapper().locator('input[type="search"], .choices__input--cloned').first(); + await search.fill(PARTIAL); + + await expect( + page.locator('.choices__list--dropdown .choices__item--selectable', { hasText: 'Seed: Fairy Tail Universe' }).first() + ).toBeVisible({ timeout: 8000 }); + // Highlight moves off the prepended typed option onto the existing universe. + await search.press('ArrowDown'); + await search.press('Enter'); + + await expect(page.locator('#serie_padre')).toHaveValue(PARTIAL); +}); + // All four series fields share the same generic autocomplete: each suggests its // own existing values (from the right collane source) and writes the picked // value to its hidden input. diff --git a/tests/series-cycles.spec.js b/tests/series-cycles.spec.js index ff197372..96e88a48 100644 --- a/tests/series-cycles.spec.js +++ b/tests/series-cycles.spec.js @@ -151,17 +151,40 @@ async function submitBookForm(page, expectedId = null) { return Number(match?.[1]); } -// The universe/group/cycle/series fields are Choices.js autocompletes (#179); -// their submitted value lives in a hidden input set via the exposed setter. +// The universe/group/cycle/series fields are Choices.js autocompletes (#179). +// Drive the REAL widget flow (open → type → pick suggestion) so the test +// exercises fetch/dropdown/selection, not just the hidden input. The typed +// value is always offered as a create-option, so this works for new names too. async function setSeriesAutocomplete(page, field, value) { - await page.evaluate(([f, v]) => { - if (window.__seriesAutocomplete && window.__seriesAutocomplete[f]) { - window.__seriesAutocomplete[f](v); - } else { + const v = String(value || '').trim(); + const wrapper = page.locator('.choices', { has: page.locator(`#${field}_select`) }); + const hasWidget = (await wrapper.count()) > 0; + + // Pages without the Choices widget (e.g. the series-detail admin form) expose + // a plain with the field id — fill it directly. + if (!hasWidget) { + const plain = page.locator(`#${field}`); + if ((await plain.count()) > 0) await plain.fill(v); + return; + } + + if (!v) { + await page.evaluate((f) => { const h = document.getElementById(f); - if (h) h.value = v; - } - }, [field, value || '']); + if (h) h.value = ''; + }, field); + return; + } + + // Book form: drive the real widget. Pressing Enter goes through the Choices + // keypath (_onEnterKey) and commits the typed value for both new and existing + // names — the canonical free-text-autocomplete gesture, not a hidden-input + // poke. Exercises the #74 Enter-commit guard too. + await wrapper.click(); + const search = wrapper.locator('input[type="search"], .choices__input--cloned').first(); + await search.fill(v); + await search.press('Enter'); + await expect(page.locator(`#${field}`)).toHaveValue(v); } async function createBookWithSeries(page, data) { From 9d89f65323d722e8ac41ae968854c66b2aa1c268 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Tue, 23 Jun 2026 19:54:07 +0200 Subject: [PATCH 2/2] test(books): use the repo Choices.js pattern (fill+wait+click) for series fields (#179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit (#188): the setSeriesAutocomplete helper and the full-test collana fill committed via Enter; the repo convention (.coderabbit.yaml) is fill + waitForTimeout + click suggestion. Switch both to it. The earlier click attempt failed only because the dropdown locator was page-scoped and matched a hidden dropdown of one of the other Choices widgets on the page — scope it to the field's own .choices wrapper. The typed value is offered as a create-option so it works for new names too. Validated: series-cycles 15/15, full-test 135/135. --- tests/full-test.spec.js | 13 +++++++++---- tests/series-cycles.spec.js | 13 ++++++++----- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/tests/full-test.spec.js b/tests/full-test.spec.js index 40cad364..5af963dd 100644 --- a/tests/full-test.spec.js +++ b/tests/full-test.spec.js @@ -688,15 +688,20 @@ test.describe.serial('Phase 3: Manual Book Creation', () => { } // Series (collana) — now a Choices.js autocomplete (#179). Drive the real - // widget (open → type → Enter): Enter goes through the Choices keypath and - // commits the typed value, exercising the widget instead of poking the - // hidden input directly. Works for the brand-new name created here. + // widget per the repo convention (.coderabbit.yaml): fill + waitForTimeout + + // click the suggestion. The typed value is always offered as a create-option, + // so it works for the brand-new name created here. The dropdown locator is + // scoped to this field's wrapper (the page has several Choices widgets). const seriesValue = `TestSeries_${RUN_ID}`; const collanaWrapper = page.locator('.choices', { has: page.locator('#collana_select') }); await collanaWrapper.click(); const collanaSearch = collanaWrapper.locator('input[type="search"], .choices__input--cloned').first(); await collanaSearch.fill(seriesValue); - await collanaSearch.press('Enter'); + await page.waitForTimeout(350); + await collanaWrapper + .locator('.choices__list--dropdown .choices__item--selectable', { hasText: seriesValue }) + .first() + .click(); await expect(page.locator('#collana')).toHaveValue(seriesValue); // Notes diff --git a/tests/series-cycles.spec.js b/tests/series-cycles.spec.js index 96e88a48..c8476004 100644 --- a/tests/series-cycles.spec.js +++ b/tests/series-cycles.spec.js @@ -176,14 +176,17 @@ async function setSeriesAutocomplete(page, field, value) { return; } - // Book form: drive the real widget. Pressing Enter goes through the Choices - // keypath (_onEnterKey) and commits the typed value for both new and existing - // names — the canonical free-text-autocomplete gesture, not a hidden-input - // poke. Exercises the #74 Enter-commit guard too. + // Book form: drive the real widget per the repo convention + // (.coderabbit.yaml) — fill + waitForTimeout + click the suggestion. The typed + // value is always offered as a create-option, so this works for new names too. await wrapper.click(); const search = wrapper.locator('input[type="search"], .choices__input--cloned').first(); await search.fill(v); - await search.press('Enter'); + await page.waitForTimeout(350); + await wrapper + .locator('.choices__list--dropdown .choices__item--selectable', { hasText: v }) + .first() + .click(); await expect(page.locator(`#${field}`)).toHaveValue(v); }