From 732f4d036bac10c0002d4eca5a7342bab0c8785d Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Tue, 15 Sep 2026 13:33:01 +0200 Subject: [PATCH 1/3] fix(cms): stop the homepage save from switching sections back on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from a live library: the features section was switched off, and it came back on the next save. A section's visibility is on that page twice. The toggle in the ordering list writes to the database as soon as it is clicked; the "Visibile" checkbox inside the section's own card is written when the form is submitted. Nothing kept the two in step, so the natural sequence — switch a section off at the top of the page, then press Save — sent the card's value from when the page was loaded and turned the section straight back on. The toggle had worked; the save undid it. The two controls now follow each other, in both directions and back again when a toggle request is refused, so the page has a single answer to "is this visible" whichever control is used. Only the five sections that carry both controls are affected: hero and the four feature cards are saved without touching is_active, which is why they never showed this. Three things found while going through the rest of the CMS: An invalid field discarded every other edit in silence. Each section is written only `if (... && empty($errors))`, so one bad URL anywhere on the page throws away the whole submission — and the message named the offending field and stopped there, which reads as "that field was ignored" while a visibility someone had just changed quietly reverted. It now says that nothing was saved, and lists what to fix. The errors were also pre-escaped and joined with
before a view that escapes what it is given, so two problems rendered with the tag visible between them; they travel as plain text now. /admin/cms answered 404. The three entry points existed only as buttons inside the settings page, so the address they all shorten to led nowhere, and a page that settings does not link — the privacy policy, anything a locale adds — could be reached only by typing its slug. There is now an index listing the homepage, the events and every content page in the active language, read from the database rather than from a fixed menu. On a content page the heading did not line up with its own text. It sat directly in the page container while the text sat in a narrower centred column, which nobody notices while the theme centres the heading — but the editorial and command layouts align it left, and there it started about a hundred pixels further left than its first line. The heading now lives in the same column as the text: measured at 0px difference on both left-aligned layouts, and still centred on the text in the three that centre it. tests/cms-admin.spec.js covers the CMS as an administrator uses it: the index and that every page in the database is linked from it, the visibility of a section through both controls and its effect on the public homepage, the all-or-nothing save and its message, reordering, saving a content page, the heading alignment, and creating, editing and deleting an event. I checked the visibility test fails without the fix. --- app/Controllers/CmsController.php | 58 +++++++- app/Routes/web.php | 8 ++ app/Views/cms/edit-home.php | 45 ++++++- app/Views/cms/index.php | 125 +++++++++++++++++ app/Views/frontend/cms-page.php | 17 ++- locale/da_DK.json | 6 + locale/de_DE.json | 6 + locale/en_US.json | 6 + locale/fr_FR.json | 6 + locale/it_IT.json | 6 + tests/cms-admin.spec.js | 217 ++++++++++++++++++++++++++++++ 11 files changed, 491 insertions(+), 9 deletions(-) create mode 100644 app/Views/cms/index.php create mode 100644 tests/cms-admin.spec.js diff --git a/app/Controllers/CmsController.php b/app/Controllers/CmsController.php index 0fddee3d9..b0333361d 100644 --- a/app/Controllers/CmsController.php +++ b/app/Controllers/CmsController.php @@ -87,6 +87,50 @@ public function showPage(Request $request, Response $response, \mysqli $db, arra return $response; } + /** + * Index of everything the CMS manages (issue: /admin/cms answered 404). + * + * The three CMS entry points lived only as separate buttons inside the + * settings page, so /admin/cms — the address anyone shortens the others to — + * was a dead end, and a page that settings does not link (the privacy + * policy, any page a locale added) could be reached only by typing its slug. + * The list is read from the database rather than from a fixed menu, so a + * page that exists is a page an administrator can find. + */ + public function index(Request $request, Response $response, \mysqli $db, array $args = []): Response + { + $db->set_charset('utf8mb4'); + $currentLocale = \App\Support\I18n::getLocale(); + + $pages = []; + $stmt = $db->prepare( + 'SELECT slug, title, is_active, updated_at FROM cms_pages WHERE locale = ? ORDER BY title' + ); + if ($stmt !== false) { + $stmt->bind_param('s', $currentLocale); + if ($stmt->execute()) { + $result = $stmt->get_result(); + if ($result instanceof \mysqli_result) { + $pages = $result->fetch_all(MYSQLI_ASSOC); + } + } + $stmt->close(); + } + + $title = __('Contenuti del sito'); + + ob_start(); + include __DIR__ . '/../Views/cms/index.php'; + $content = ob_get_clean(); + + ob_start(); + include __DIR__ . '/../Views/layout.php'; + $html = ob_get_clean(); + + $response->getBody()->write($html); + return $response; + } + public function editHome(Request $request, Response $response, \mysqli $db, array $args): Response { // CRITICAL: Set UTF-8 charset to prevent corruption of Greek/Unicode characters @@ -555,7 +599,19 @@ public function updateHome(Request $request, Response $response, \mysqli $db, ar } if (!empty($errors)) { - $_SESSION['error_message'] = implode('
', array_map(fn($e) => htmlspecialchars($e, ENT_QUOTES, 'UTF-8'), $errors)); + // Every section above is written only `if (... && empty($errors))`, + // so one invalid field discards the whole submission — including + // edits to sections that have nothing to do with it. The message + // used to name the offending field and stop there, which reads as + // "that one field was ignored" while the visibility someone had just + // switched off quietly came back. Say what actually happened. + // + // Plain text, escaped by the view: the previous version pre-escaped + // each error and joined them with
, and the view escapes what it + // is given, so a submission with two problems rendered them as + // "first
second" with the tag visible in the middle. + $_SESSION['error_message'] = __('Nessuna modifica è stata salvata: correggi quanto segue e salva di nuovo.') + . ' ' . implode(' · ', $errors); } else { \App\Support\ContentCache::homeContentChanged(); diff --git a/app/Routes/web.php b/app/Routes/web.php index a40330e70..bec03c0cc 100644 --- a/app/Routes/web.php +++ b/app/Routes/web.php @@ -1008,6 +1008,14 @@ return $controller->delete($request, $response, $args); })->add(new CsrfMiddleware())->add(new AdminAuthMiddleware()); + // Admin CMS index. Declared before /admin/cms/{slug} so the bare address + // lists the content instead of being read as a page slug. + $app->get('/admin/cms[/]', function ($request, $response, $args) use ($app) { + $db = $app->getContainer()->get('db'); + $controller = new \App\Controllers\CmsController(); + return $controller->index($request, $response, $db, $args); + })->add(new AdminAuthMiddleware()); + // Admin CMS routes - Homepage $app->get('/admin/cms/home', function ($request, $response, $args) use ($app) { $db = $app->getContainer()->get('db'); diff --git a/app/Views/cms/edit-home.php b/app/Views/cms/edit-home.php index 3d9af8aa6..f82699f0c 100644 --- a/app/Views/cms/edit-home.php +++ b/app/Views/cms/edit-home.php @@ -1102,11 +1102,44 @@ function saveSectionOrder() { }); } + // Five sections carry their visibility TWICE on this page: the toggle in + // this list, which writes to the database immediately, and the "Visibile" + // checkbox inside the section's own card, which is written when the form is + // submitted. They used to be independent, so the obvious sequence — switch a + // section off up here, then press Save — sent the card's stale "on" value + // and turned the section straight back on. Keeping them in step means the + // page has one answer to "is this visible", whichever control is used, and + // that the answer follows a failed request back to where it was. + // + // The card's field is addressed by the section key it posts under, so no + // toggle-to-field map has to be maintained by hand. Sections whose + // visibility lives only in this list (hero, the four feature cards) have no + // field and are left alone — their save path does not touch is_active. + const visibilityFieldFor = (toggle) => { + const key = toggle.closest('.section-item')?.dataset.sectionKey; + return key ? document.querySelector('input[name="' + key + '[is_active]"]') : null; + }; + const syncVisibilityField = (toggle) => { + const field = visibilityFieldFor(toggle); + if (field) { + field.checked = toggle.checked; + } + }; + // Toggle visibility document.querySelectorAll('.toggle-visibility').forEach(toggle => { + const field = visibilityFieldFor(toggle); + if (field) { + // The other direction never hits the network: the card's checkbox is + // saved with the form, and this list shows what is about to be saved. + field.addEventListener('change', () => { + toggle.checked = field.checked; + }); + } toggle.addEventListener('change', function() { const sectionId = parseInt(this.dataset.sectionId); const isActive = this.checked ? 1 : 0; + syncVisibilityField(this); fetch(window.BASE_PATH + '/admin/cms/home/toggle-visibility', { method: 'POST', @@ -1126,8 +1159,10 @@ function saveSectionOrder() { if (data.error || data.code) { statusEl.textContent = '\u2717 ' + (data.error || ); statusEl.className = 'mt-4 text-sm text-red-600'; - // Revert checkbox + // Revert checkbox — and the card's field with it, so a refused + // write never leaves the two controls disagreeing. toggle.checked = !toggle.checked; + syncVisibilityField(toggle); // Handle session expiration - reload page to get new CSRF token if (data.code === 'SESSION_EXPIRED' || data.code === 'CSRF_INVALID') { @@ -1147,19 +1182,23 @@ function saveSectionOrder() { } else { statusEl.textContent = '\u2717 ' + (data.message || ); statusEl.className = 'mt-4 text-sm text-red-600'; - // Revert checkbox + // Revert checkbox — and the card's field with it, so a refused + // write never leaves the two controls disagreeing. toggle.checked = !toggle.checked; + syncVisibilityField(toggle); } }) .catch(err => { console.error(err); statusEl.textContent = '\u2717 ' + ; statusEl.className = 'mt-4 text-sm text-red-600'; - // Revert checkbox + // Revert checkbox — and the card's field with it. this.checked = !this.checked; + syncVisibilityField(this); }); }); }); } + }); diff --git a/app/Views/cms/index.php b/app/Views/cms/index.php new file mode 100644 index 000000000..b973dc737 --- /dev/null +++ b/app/Views/cms/index.php @@ -0,0 +1,125 @@ + $pages + * @var string $currentLocale + */ + +use App\Support\HtmlHelper; + +// The two fixed destinations. Everything else on this page comes from the +// database, so a page nobody thought to add to a menu is still reachable. +// Full class strings, never interpolated: Tailwind is purged against the +// sources, so a class assembled at runtime has no rule in the stylesheet. +$fixedCards = [ + [ + 'icon' => 'fas fa-home text-blue-600', + 'iconWrap' => 'w-10 h-10 rounded-xl bg-blue-100 flex items-center justify-center', + 'heading' => __('Homepage'), + 'description' => __('Modifica i contenuti della homepage: hero, features, CTA e immagine di sfondo'), + 'admin' => url('/admin/cms/home'), + 'live' => url('/'), + 'action' => __('Modifica Homepage'), + ], + [ + 'icon' => 'fas fa-calendar-alt text-purple-600', + 'iconWrap' => 'w-10 h-10 rounded-xl bg-purple-100 flex items-center justify-center', + 'heading' => __('Eventi'), + 'description' => __('Gestisci gli eventi della biblioteca: crea, modifica ed elimina eventi con immagini e descrizioni'), + 'admin' => url('/admin/cms/events'), + 'live' => route_path('events'), + 'action' => __('Gestisci Eventi'), + ], +]; +?> + +
+
+
+
+

+ + +

+

+ +

+
+ + + + +
+
+ +
+ +
+
+
+
+
+ +
+

+
+

+
+ + +
+
+
+
+ + + + +
+
+ +
+ +
+
+

+ + +

+

+ +

+
+
+ +

+ + + +
+
+
diff --git a/app/Views/frontend/cms-page.php b/app/Views/frontend/cms-page.php index 40342cc19..218a70b08 100644 --- a/app/Views/frontend/cms-page.php +++ b/app/Views/frontend/cms-page.php @@ -145,13 +145,20 @@
-
-

-
-
- +
+
+

+
+
+ <?= htmlspecialchars($title, ENT_QUOTES, 'UTF-8') ?> !u.pathname.includes('accedi') && !u.pathname.includes('login')); + } +} + +const listToggle = (key) => `[data-section-key="${key}"] input[type=checkbox]`; +const sectionActive = (key) => db(`SELECT is_active FROM home_content WHERE section_key='${key}'`); + +// Everything this suite changes is put back, so it can run on a working +// installation without leaving the homepage rearranged. +let originalState = {}; + +test.describe.serial('CMS admin', () => { + test.beforeAll(() => { + if (!process.env.E2E_ADMIN_EMAIL || !process.env.E2E_DB_USER) throw new Error('Run with /tmp/run-e2e.sh'); + const rows = db('SELECT section_key, is_active, display_order FROM home_content').split('\n').filter(Boolean); + originalState = Object.fromEntries(rows.map(r => { + const [key, active, order] = r.split('\t'); + return [key, { active, order }]; + })); + }); + + test.afterAll(() => { + try { + for (const [key, v] of Object.entries(originalState)) { + db(`UPDATE home_content SET is_active=${Number(v.active)}, display_order=${Number(v.order)} WHERE section_key='${key}'`); + } + db(`DELETE FROM events WHERE title LIKE '${marker}%'`); + } catch (e) { console.error('Scoped cleanup failed:', e.message); } + }); + + test('the CMS index lists everything that can be edited', async ({ page }) => { + await login(page); + const response = await page.goto(BASE + '/admin/cms'); + // Used to be a 404: the three entry points existed only as buttons inside + // the settings page, so the address they all shorten to led nowhere. + expect(response.status()).toBe(200); + + const links = await page.locator('a[href*="/admin/cms/"]').evaluateAll(as => as.map(a => a.getAttribute('href'))); + expect(links.some(h => h.endsWith('/admin/cms/home'))).toBe(true); + expect(links.some(h => h.endsWith('/admin/cms/events'))).toBe(true); + + // Every page in the database is reachable from here, including the ones no + // menu links to. + const slugs = db("SELECT slug FROM cms_pages WHERE locale='it_IT'").split('\n').filter(Boolean); + expect(slugs.length).toBeGreaterThan(0); + for (const slug of slugs) { + expect(links.some(h => h.endsWith('/admin/cms/' + slug)), `${slug} is linked`).toBe(true); + } + }); + + test('a section switched off in the list stays off when the page is saved', async ({ page }) => { + await login(page); + await page.goto(BASE + '/admin/cms/home'); + await page.locator(listToggle('features_title')).check(); + await page.waitForTimeout(800); + await page.reload(); + + // Switch it off in the ordering list… + await page.locator(listToggle('features_title')).uncheck(); + await page.waitForTimeout(800); + expect(sectionActive('features_title'), 'the toggle writes immediately').toBe('0'); + // …the card's own checkbox must follow, or the form will re-send "on". + await expect(page.locator('#features_visible')).not.toBeChecked(); + + await page.locator('button[type=submit]').first().click(); + await page.waitForLoadState('networkidle'); + expect(sectionActive('features_title'), 'saving must not resurrect the section').toBe('0'); + await expect(page.locator('#features_visible')).not.toBeChecked(); + }); + + test('and disappears from the public homepage', async ({ page, browser }) => { + await login(page); + const anonymous = await browser.newContext(); + const reader = await anonymous.newPage(); + + await reader.goto(BASE + '/'); + await expect(reader.locator('[data-section="features_title"]')).toHaveCount(0); + + // Back on: the section returns, so the switch works in both directions. + await page.goto(BASE + '/admin/cms/home'); + await page.locator('#features_visible').check(); + await expect(page.locator(listToggle('features_title'))).toBeChecked(); + await page.locator('button[type=submit]').first().click(); + await page.waitForLoadState('networkidle'); + expect(sectionActive('features_title')).toBe('1'); + + await reader.goto(BASE + '/'); + await expect(reader.locator('[data-section="features_title"]')).toHaveCount(1); + await anonymous.close(); + }); + + test('an invalid field saves nothing and says so', async ({ page }) => { + await login(page); + await page.goto(BASE + '/admin/cms/home'); + const before = sectionActive('features_title'); + + await page.locator('input[name="hero[button_link]"]').fill('non un url valido'); + await page.locator('#features_visible').uncheck(); + await page.locator('button[type=submit]').first().click(); + await page.waitForLoadState('networkidle'); + + // Every section is written only when there are no errors, so one bad field + // discards the whole submission. The message has to admit that much. + const body = await page.locator('body').innerText(); + expect(body).toContain('Nessuna modifica è stata salvata'); + expect(body).toContain('link del pulsante non è valido'); + expect(body).not.toContain('
'); + expect(sectionActive('features_title'), 'nothing was written').toBe(before); + }); + + test('section order can be rearranged and survives a reload', async ({ page }) => { + await login(page); + await page.goto(BASE + '/admin/cms/home'); + const csrf = await page.locator('meta[name="csrf-token"]').getAttribute('content'); + const id = db("SELECT id FROM home_content WHERE section_key='cta'"); + const currentOrder = Number(db("SELECT display_order FROM home_content WHERE section_key='cta'")); + + const resp = await page.request.post(BASE + '/admin/cms/home/reorder', { + headers: { 'Content-Type': 'application/json', 'X-CSRF-Token': csrf }, + data: { order: [{ id: Number(id), display_order: currentOrder + 1 }] }, + }); + expect(resp.status()).toBeLessThan(400); + expect(Number(db("SELECT display_order FROM home_content WHERE section_key='cta'"))).toBe(currentOrder + 1); + }); + + test('a content page saves, and its title lines up with its text', async ({ page, browser }) => { + await login(page); + await page.goto(BASE + '/admin/cms/chi-siamo'); + const titleInput = page.locator('input[name=title]').first(); + const original = await titleInput.inputValue(); + await titleInput.fill(`${original} ${marker}`); + await page.locator('button[type=submit]').first().click(); + await page.waitForLoadState('networkidle'); + await expect(page.locator('input[name=title]').first()).toHaveValue(new RegExp(marker)); + + // The heading used to sit in the page container while the text sat in a + // narrower column, so on a left-aligned theme it started about a hundred + // pixels further left than its own first line. + const anonymous = await browser.newContext(); + const reader = await anonymous.newPage(); + await reader.setViewportSize({ width: 1280, height: 900 }); + await reader.goto(BASE + '/chi-siamo'); + const heading = await reader.locator('h1.cms-title').boundingBox(); + const content = await reader.locator('.cms-content').first().boundingBox(); + const centred = await reader.locator('.cms-header').first().evaluate(el => getComputedStyle(el).textAlign === 'center'); + if (centred) { + const headingCentre = heading.x + heading.width / 2; + const contentCentre = content.x + content.width / 2; + expect(Math.abs(headingCentre - contentCentre)).toBeLessThan(2); + } else { + expect(Math.abs(heading.x - content.x)).toBeLessThan(2); + } + await anonymous.close(); + + // Put the title back. + await page.goto(BASE + '/admin/cms/chi-siamo'); + await page.locator('input[name=title]').first().fill(original); + await page.locator('button[type=submit]').first().click(); + await page.waitForLoadState('networkidle'); + }); + + test('events can be created, edited and deleted', async ({ page }) => { + await login(page); + await page.goto(BASE + '/admin/cms/events/create'); + await page.locator('input[name=title]').fill(`${marker} Evento`); + // The date field is driven by Flatpickr, so its own input is hidden: set it + // the way the main suite does rather than typing into a picker. + await page.locator('input[name=event_date]').evaluate((el, d) => { + el.value = d; + el.dispatchEvent(new Event('change', { bubbles: true })); + }, '2027-03-15'); + await page.locator('button[type=submit]').first().click(); + await page.waitForLoadState('networkidle'); + const id = db(`SELECT id FROM events WHERE title='${marker} Evento'`); + expect(id, 'the event was created').not.toBe(''); + + await page.goto(BASE + `/admin/cms/events/edit/${id}`); + await page.locator('input[name=title]').fill(`${marker} Evento modificato`); + await page.locator('button[type=submit]').first().click(); + await page.waitForLoadState('networkidle'); + expect(db(`SELECT title FROM events WHERE id=${id}`)).toBe(`${marker} Evento modificato`); + + const csrf = await page.locator('meta[name="csrf-token"]').getAttribute('content'); + const del = await page.request.post(BASE + `/admin/cms/events/delete/${id}`, { + form: { csrf_token: csrf }, + }); + expect(del.status()).toBeLessThan(400); + expect(db(`SELECT COUNT(*) FROM events WHERE id=${id}`)).toBe('0'); + }); +}); From ffc878d9f4010eb12e377765b3f354921797a80f Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Tue, 15 Sep 2026 14:25:42 +0200 Subject: [PATCH 2/3] fix(cms): stop drawing feature cards that were switched off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found on the live library this started from. Its homepage was publishing four cards reading "Feature 1", "Feature 2", "Feature 3" and "Feature 4" — a star icon each, no text — under the product's own default heading. The administrator had switched the four cards off, which is what anyone does when they do not want them, and the section drew them anyway. $homeContent only holds the sections that are switched on, so a card that was turned off is simply absent from it; the template looped over the four fixed indexes and filled each gap from its own defaults, which exist for a fresh install and read as placeholder text on a real site. It now renders the cards that are actually there, and when none are left it does not draw the empty grid at all — the heading and subtitle stay, because leaving them on is a separate choice the administrator makes with its own switch. Covered in tests/cms-admin.spec.js: four cards on the public homepage, one switched off leaves three and no "Feature 1" anywhere in the section, all four off leaves the section without a grid, and switching them back on brings the real cards back. The test fails without this change. --- .../frontend/home-sections/features_title.php | 23 ++++++-- tests/cms-admin.spec.js | 53 +++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/app/Views/frontend/home-sections/features_title.php b/app/Views/frontend/home-sections/features_title.php index b4412d8ef..4b8c5c8a8 100644 --- a/app/Views/frontend/home-sections/features_title.php +++ b/app/Views/frontend/home-sections/features_title.php @@ -13,11 +13,25 @@

+ +
-
@@ -31,7 +45,8 @@

- +
+
diff --git a/tests/cms-admin.spec.js b/tests/cms-admin.spec.js index 5b5dfe875..083942b8b 100644 --- a/tests/cms-admin.spec.js +++ b/tests/cms-admin.spec.js @@ -116,6 +116,59 @@ test.describe.serial('CMS admin', () => { await anonymous.close(); }); + test('a feature card switched off leaves no placeholder behind', async ({ page, browser }) => { + await login(page); + await page.goto(BASE + '/admin/cms/home'); + for (const key of ['feature_1', 'feature_2', 'feature_3', 'feature_4']) { + const toggle = page.locator(listToggle(key)); + if (!(await toggle.isChecked())) { + await toggle.check(); + await page.waitForTimeout(400); + } + } + await page.locator('#features_visible').check(); + await page.locator('button[type=submit]').first().click(); + await page.waitForLoadState('networkidle'); + + const anonymous = await browser.newContext(); + const reader = await anonymous.newPage(); + await reader.goto(BASE + '/?cb=' + Date.now()); + const before = await reader.locator('[data-section="features_title"] .feature-card').count(); + expect(before).toBe(4); + + // Switching a card off is what an administrator does when they do not want + // it. The section used to draw it anyway, from the template's own defaults: + // a live library published four cards reading "Feature 1" to "Feature 4", + // a star icon each and no text. + await page.goto(BASE + '/admin/cms/home'); + await page.locator(listToggle('feature_1')).uncheck(); + await page.waitForTimeout(900); + + await reader.goto(BASE + '/?cb=' + Date.now()); + await expect(reader.locator('[data-section="features_title"] .feature-card')).toHaveCount(3); + await expect(reader.locator('[data-section="features_title"]')).not.toContainText('Feature 1'); + + // With every card off, the empty grid is not drawn at all. + await page.goto(BASE + '/admin/cms/home'); + for (const key of ['feature_2', 'feature_3', 'feature_4']) { + await page.locator(listToggle(key)).uncheck(); + await page.waitForTimeout(400); + } + await reader.goto(BASE + '/?cb=' + Date.now()); + await expect(reader.locator('[data-section="features_title"]')).toHaveCount(1); + await expect(reader.locator('[data-section="features_title"] .feature-grid')).toHaveCount(0); + + // Put the cards back. + await page.goto(BASE + '/admin/cms/home'); + for (const key of ['feature_1', 'feature_2', 'feature_3', 'feature_4']) { + await page.locator(listToggle(key)).check(); + await page.waitForTimeout(400); + } + await reader.goto(BASE + '/?cb=' + Date.now()); + await expect(reader.locator('[data-section="features_title"] .feature-card')).toHaveCount(4); + await anonymous.close(); + }); + test('an invalid field saves nothing and says so', async ({ page }) => { await login(page); await page.goto(BASE + '/admin/cms/home'); From 8ad23ae25f261a01b3e34caa241e32d490848080 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Tue, 15 Sep 2026 15:02:08 +0200 Subject: [PATCH 3/3] fix(cms): address the review on the visibility fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restore the toggle to what it was before the request, not to the negation of whatever it holds when the answer arrives: the operator can click again while the write is in flight, and since the card's field now follows the toggle, a wrong restore would travel on to the form. Escape with htmlspecialchars() in the new view. HtmlHelper::e() decodes entities before escaping them again, so a title stored as an entity renders as the character instead of the literal text — the project rule for app/Views is the direct call, and the file had eight of them. In the suite: check every environment variable it needs in one place, so a missing password fails at the guard instead of timing out several steps later; and capture the page title in suite state so afterAll() puts it back even when an assertion fails before the test can. Not changed: the reviewer also asked for a .swal2-confirm click after each submit, per the path rule. These two forms redirect and render a flash message instead — measured zero .swal2-popup after saving both the homepage and a content page — so there is no dialog to confirm and nothing blocks the next step. --- app/Views/cms/edit-home.php | 21 +++++++++++++-------- app/Views/cms/index.php | 17 ++++++++--------- tests/cms-admin.spec.js | 13 ++++++++++++- 3 files changed, 33 insertions(+), 18 deletions(-) diff --git a/app/Views/cms/edit-home.php b/app/Views/cms/edit-home.php index f82699f0c..c9d02cce0 100644 --- a/app/Views/cms/edit-home.php +++ b/app/Views/cms/edit-home.php @@ -1139,6 +1139,11 @@ function saveSectionOrder() { toggle.addEventListener('change', function() { const sectionId = parseInt(this.dataset.sectionId); const isActive = this.checked ? 1 : 0; + // What to go back to if the write is refused. Flipping `checked` again + // would undo whatever the operator clicked in the meantime rather than + // restore the value this request set out from — and now that the card's + // field follows the toggle, a wrong restore would travel to the form. + const previous = !this.checked; syncVisibilityField(this); fetch(window.BASE_PATH + '/admin/cms/home/toggle-visibility', { @@ -1159,9 +1164,9 @@ function saveSectionOrder() { if (data.error || data.code) { statusEl.textContent = '\u2717 ' + (data.error || ); statusEl.className = 'mt-4 text-sm text-red-600'; - // Revert checkbox — and the card's field with it, so a refused - // write never leaves the two controls disagreeing. - toggle.checked = !toggle.checked; + // Restore what it was before this request — and the card's field + // with it, so a refused write never leaves the two disagreeing. + toggle.checked = previous; syncVisibilityField(toggle); // Handle session expiration - reload page to get new CSRF token @@ -1182,9 +1187,9 @@ function saveSectionOrder() { } else { statusEl.textContent = '\u2717 ' + (data.message || ); statusEl.className = 'mt-4 text-sm text-red-600'; - // Revert checkbox — and the card's field with it, so a refused - // write never leaves the two controls disagreeing. - toggle.checked = !toggle.checked; + // Restore what it was before this request — and the card's field + // with it, so a refused write never leaves the two disagreeing. + toggle.checked = previous; syncVisibilityField(toggle); } }) @@ -1192,8 +1197,8 @@ function saveSectionOrder() { console.error(err); statusEl.textContent = '\u2717 ' + ; statusEl.className = 'mt-4 text-sm text-red-600'; - // Revert checkbox — and the card's field with it. - this.checked = !this.checked; + // Restore what it was before this request — and the card's field too. + this.checked = previous; syncVisibilityField(this); }); }); diff --git a/app/Views/cms/index.php b/app/Views/cms/index.php index b973dc737..1cc7ae97b 100644 --- a/app/Views/cms/index.php +++ b/app/Views/cms/index.php @@ -6,7 +6,6 @@ * @var string $currentLocale */ -use App\Support\HtmlHelper; // The two fixed destinations. Everything else on this page comes from the // database, so a page nobody thought to add to a menu is still reachable. @@ -59,12 +58,12 @@
-
- +
+
-

+

-

+

@@ -74,7 +73,7 @@
- +
@@ -88,7 +87,7 @@

- +

@@ -100,8 +99,8 @@
  • - - / + + / diff --git a/tests/cms-admin.spec.js b/tests/cms-admin.spec.js index 083942b8b..f0c453508 100644 --- a/tests/cms-admin.spec.js +++ b/tests/cms-admin.spec.js @@ -35,10 +35,17 @@ const sectionActive = (key) => db(`SELECT is_active FROM home_content WHERE sect // Everything this suite changes is put back, so it can run on a working // installation without leaving the homepage rearranged. let originalState = {}; +// Captured inside the test that edits it, restored in afterAll: a failing +// assertion must not leave the marker in the page title for the next run. +let originalPageTitle = null; test.describe.serial('CMS admin', () => { test.beforeAll(() => { - if (!process.env.E2E_ADMIN_EMAIL || !process.env.E2E_DB_USER) throw new Error('Run with /tmp/run-e2e.sh'); + // Every variable this suite needs, checked together: a missing password + // used to get past the guard and fail several steps later as a timeout. + const required = ['E2E_ADMIN_EMAIL', 'E2E_ADMIN_PASS', 'E2E_DB_USER', 'E2E_DB_NAME', 'E2E_DB_PASS']; + const missing = required.filter(name => !process.env[name]); + if (missing.length) throw new Error(`Run with /tmp/run-e2e.sh — missing: ${missing.join(', ')}`); const rows = db('SELECT section_key, is_active, display_order FROM home_content').split('\n').filter(Boolean); originalState = Object.fromEntries(rows.map(r => { const [key, active, order] = r.split('\t'); @@ -52,6 +59,9 @@ test.describe.serial('CMS admin', () => { db(`UPDATE home_content SET is_active=${Number(v.active)}, display_order=${Number(v.order)} WHERE section_key='${key}'`); } db(`DELETE FROM events WHERE title LIKE '${marker}%'`); + if (originalPageTitle !== null) { + db(`UPDATE cms_pages SET title='${originalPageTitle.replace(/'/g, "''")}' WHERE slug='chi-siamo' AND locale='it_IT'`); + } } catch (e) { console.error('Scoped cleanup failed:', e.message); } }); @@ -208,6 +218,7 @@ test.describe.serial('CMS admin', () => { await page.goto(BASE + '/admin/cms/chi-siamo'); const titleInput = page.locator('input[name=title]').first(); const original = await titleInput.inputValue(); + originalPageTitle = original; await titleInput.fill(`${original} ${marker}`); await page.locator('button[type=submit]').first().click(); await page.waitForLoadState('networkidle');