Unify client-side popups to SweetAlert2 (closes #140)#141
Closed
fabiodalez-dev wants to merge 17 commits into
Closed
Unify client-side popups to SweetAlert2 (closes #140)#141fabiodalez-dev wants to merge 17 commits into
fabiodalez-dev wants to merge 17 commits into
Conversation
Issue #137 (HansUwe52): the featured image on the public event detail page renders at 100% container width with no max-height constraint, so sproportioned uploads (e.g. a 1791x927 book cover used as an event image) dominate the viewport. Adds a CMS-tab setting 'cms.event_image_layout' with four options: - contained (default) max-height: clamp(280px, 50vw, 480px) + object-fit: contain. Solves the reported case while preserving the previous look for reasonably-sized images. - banner aspect-ratio: 16/9 + object-fit: cover. Crops to a uniform hero banner. - full Legacy 100%-width-no-constraint behaviour, for users who depended on it. - thumb Right-floated 3:4 portrait thumbnail that wraps the event body text. Collapses to full-width on viewports under 640px. The setting lives in the existing system_settings KV store (no schema migration needed) and is wired through the existing pattern used by 'updateLabels' / 'updateSharingSettings': - SettingsController::updateEventSettings (validated allow-list) - resolveEventSettings() exposed to the settings view - POST /admin/settings/events route protected by CsrfMiddleware + AdminAuthMiddleware - select rendered inside the existing 'Eventi' card on the CMS tab, so the admin doesn't have to discover a new tab FrontendController::event() reads and re-validates the setting against the same allow-list before passing it to the view; event-detail.php narrows once more before composing the CSS class, so a future controller refactor cannot leak an unknown layout into the DOM. i18n: 8 new keys added to all four locale files (it_IT/en_US/de_DE/fr_FR) in the same commit, per project policy. Tests: tests/issue-137-event-image-layout.spec.js — 5 cases covering the four explicit layouts plus the default fallback. Each test mutates cms.event_image_layout in the KV store directly, requests the public event page, and asserts the figure carries the matching CSS modifier and data-event-cover-layout attribute. All 5 pass. PHPStan: clean (no new findings; pre-existing storage/plugins issues unchanged at 11).
Self-review feedback on the initial layout palette:
- 'contained' was using an arbitrary max-height: 480px that produced
uneven silhouettes depending on the source aspect ratio. Replaced
with aspect-ratio: 3/2 + object-fit: cover so the cover always
lands on a predictable photographic crop.
- 'thumb' was using CSS float: right, which caused the figure to
escape the parent .event-card whenever the body content was short
(e.g. a one-paragraph event) — visually the image then overlapped
the page footer. Refactored to a CSS grid layout driven by a new
.event-card--thumb-layout modifier on the parent article: the
figure now lives in its own column/row and is geometrically
constrained inside the card. On viewports below 768px the grid
collapses to a vertical stack.
- Bonus on desktop: the side-thumbnail sticks while the reader
scrolls long event descriptions (position: sticky).
UI labels updated to match the new design ('Foto 3:2' instead of
'Adattato (max 480px)') in all four locale files.
Tests: added a 6th case to tests/issue-137-event-image-layout.spec.js
that creates a very short event content and asserts
figure.bottom <= card.bottom at desktop width — the exact invariant
the float bug used to violate. All 6 cases pass.
Previous iterations all rendered at 100% container width and only
differed by aspect-ratio, so the original complaint (a 1791x927 image
dominating the event page) was still visible in three of four presets.
The whole point of the setting is to LIMIT the image footprint.
Resized accordingly:
full 100% x auto legacy enormous (opt-in)
banner 100% x 220px max low decorative strip
contained 420px max, centred small poster (DEFAULT, solves
the reported case directly)
thumb 240px wide side thumbnail next to body text
Source aspect ratio is preserved on contained/thumb-mobile — shrinking
the footprint is the goal, forcing crops would just trade one issue
for another.
UI labels rewritten so the size is explicit ('Piccola centrata
(max 420px)', 'Miniatura affiancata al testo (240px)', etc.) and
the librarian can pick at a glance. All 4 locale files updated.
Tests: added a measurement-based regression that asserts each preset
renders at a distinct, capped size:
contained.width <= 420px AND < 70% of card.width
thumb.width <= 245px
banner.height <= 225px
full.width > 85% of card.width
Catches the whole class of regression where four CSS modifiers exist
but render identically. All 7 tests pass.
User feedback: small images should sit on the left, not centred. - contained: margin-left: 0 (was: margin-left/right: auto) - thumb on mobile: margin: 0 0 1.5rem 0 (was: 0 auto 1.5rem auto) Small images centred in a wide card read as a 'centred hero' which is the same visual problem we are trying to escape: the image dominates its own row. Left-aligned, the image becomes part of the body flow. UI label updated: 'Piccola a sinistra (max 420px)' replaces 'Piccola centrata (max 420px)'. All four locale files updated. Test guard added: the effective-size test now also asserts that figure.x is within 100px of card.x (i.e. only inner padding) for both 'contained' and 'thumb'. Catches any future refactor that reintroduces margin: auto. All 7 cases pass.
The edit form has separate controls: a 'remove_image' checkbox and a
'featured_image' file input. When the user ticks both (replace the
old image AND clear the slot in one submit), the existing if/elseif
processed 'remove' first and skipped the upload branch — the new
file was discarded and featured_image ended up NULL.
Fix:
- Invert the priority: handle the file upload first; only run the
remove path when no upload was submitted. This matches user
intent (a new file always means 'replace').
- Look up the previous featured_image before the UPDATE and unlink
the orphan file from disk after the row is updated. Path-
traversal-safe via realpath() + base-directory containment, the
same contract handleImageUpload() upholds when writing.
Test: tests/issue-137-event-image-layout.spec.js — added an
end-to-end case that drives the actual admin form (login, edit
event, tick the remove checkbox, attach a new file, submit) and
asserts the DB row points at a NEW upload path (regex matches
handleImageUpload()'s naming scheme) — not NULL, not the old
sentinel. Catches the regression at the controller layer where the
historic bug lived. All 8 cases pass.
Applied by /adamsreview:fix run_id=fixrun_20260521T090648Zc0df58 at threshold=60.
Cross-cutting group G1 (EventsController orphan-file lifecycle):
- F005: cleanup new upload when UPDATE rolls back
- F006: cleanup new upload when validation-error short-circuits
- F025: wire deleteUploadedImageFile() into delete() lifecycle
Stale documentation fixes:
- F009: spec.js comment 'banner' was '16:9 cropped', now matches 220px cap
- F010: spec.js comment 'contained' was 'max-height 480px', now 320px
- F011: spec.js comment 'thumb' was 'right-floated', now grid
- F012: event-detail.php block comment 'centred' → 'left-aligned'
- F013: SettingsController updateEventSettings() docblock — was inverted
(said 'contained' preserves legacy 100%-width; that is 'full', not 'contained')
Test robustness:
- F032: locale-aware EVENT_URL_PREFIX (was hardcoded /eventi/, silently
404'd on en_US / de_DE environments)
- F033: snapshot+restore events_page_enabled around the suite (was
permanently enabling it on every run)
Sanity: PHPStan clean on the 3 modified PHP files; php -l clean on all 4.
…ld=40)
Applied by /adamsreview:fix run_id=fixrun_20260521T103644Z7b1556.
- F015 (settings/index.php): swap 'Salva impostazioni eventi' button from
bg-purple-600 to bg-gray-900 — aligns with the libri/autori/editori
Tailwind chrome guideline (memory feedback_ui_style_and_i18n_blocking).
Kept bg-purple-100 on the Eventi card icon (category marker).
- F018 (event-detail.php): document the intentional asymmetry on
.event-cover--contained (no border/background while other variants
keep the .event-cover base chrome) — comment-only change.
- F020 (event-detail.php): bound the sticky thumb to the viewport with
max-height: calc(100vh - 9rem) + overflow: hidden inside the >=768px
grid media query. Prevents the figure from spilling past the article
on short-body events.
- F031 (EventsController.php): rewrite the misleading comment above the
pre-UPDATE SELECT featured_image. Old text said 'BEFORE the error
short-circuit so $oldImagePath is always populated'; in reality only
the success branch consumes $oldImagePath — the error-short-circuit
and UPDATE-fail branches unlink $featuredImagePath.
Sanity: PHPStan clean on EventsController + event-detail; php -l clean
on all 3 modified files.
Triage of the CodeRabbit review pass: VALID (fixed): - EventsController::deleteUploadedImageFile (line 482): the @Unlink silenced any I/O failure. Replaced with a checked unlink() call that logs failures via SecureLogger::error with the resolved path, errno from error_get_last(), and message — orphan files now surface in the operator log instead of accumulating silently. - tests/issue-137-event-image-layout.spec.js teardown: previously DELETEd cms.event_image_layout unconditionally, destroying an admin's pre-existing custom choice on every run. Snapshot/restore pattern extended (mirrors the existing events_page_enabled handling). Captures original value in beforeAll, INSERTs back or DELETEs in afterAll. SKIPPED (with reason): - affected_rows guard around UPDATE: race condition is real but pre-existing across every updateXxx controller in the repo (libri, autori, editori). Touching only EventsController would create inconsistency for a marginal edge case; the id is already 404-validated by edit() before the form posts. - SettingsController::updateEventSettings redirect target: /admin/... routes are not in routes_*.json (only public routes are translated). Every other settings-update redirect in the same file uses the same literal pattern; changing only this one introduces asymmetry. - HtmlHelper::e() in event-detail.php / settings/index.php: the pre-commit hook explicitly accepts both htmlspecialchars and HtmlHelper::e (which calls the same escape internally). 20+ uses of HtmlHelper::e in the same files predate this PR. - .swal2-confirm wait after the admin events form submit: that form has no SweetAlert handler (verified by grep on app/Views/events/form.php). The current Promise.all([waitForURL, click]) pattern passes 8/8 — adding swal2-confirm wait would hang the test. Tests: tests/issue-137-event-image-layout.spec.js — 8/8 pass after the teardown change. PHPStan + php -l clean on EventsController.
…low-up) CodeRabbit re-flagged tests/issue-137-event-image-layout.spec.js afterAll: the previous fix restored the event_image_layout setting but still bypassed the controller's delete() — the admin-update test uses page.setInputFiles with a real JPEG buffer, which the admin controller moves to /uploads/events/event_YYYYMMDD_HHMMSS_<hex>.jpg via handleImageUpload(). Without the controller, DELETE FROM events left that file orphan on disk every run. Fix: before the dbExec(DELETE FROM events ...), SELECT the current featured_image, and if it starts with /uploads/events/ (i.e. it's a real upload, not the static seed /assets/books.jpg), unlinkSync the file. ENOENT is tolerated (the controller may have already cleaned it on the success path of the admin-update test). Added 'fs' and 'path' to the spec's requires. Validation: ran the suite — 8/8 pass; the /uploads/events directory now stays at the same orphan count across runs instead of growing.
Refactor ~45 native alert()/confirm() sites across 34 views to the
existing window.SwalApp helper bus.
Helper additions in public/assets/js/swal-config.js:
- Every existing helper (confirmDelete / confirm / success / error /
info / warning / toast) now has a defensive native fallback —
when Swal failed to load (CDN error, CSP, bundle failure), the
helper degrades to window.confirm/alert with the same title+text
semantics so destructive flows keep working.
- New helpers: SwalApp.prompt(opts) for single-line input dialogs;
SwalApp.attachSwalConfirm(selector) auto-wires every form carrying
data-swal-confirm to a Swal confirm dialog. Auto-attaches on
DOMContentLoaded (idempotent via data-swal-attached marker).
Refactor categories:
- 11 <form onsubmit/onclick='return confirm(...)'> → data-swal-confirm
attribute set on the form (advanced-tab API keys, generi delete,
collocazione shelf/mensole, prenotazioni cancel × 2, languages
edit/delete, languages index set-default/delete, utenti
activate-directly × 2, editori delete, autori delete).
- 9 JS confirm() before action → SwalApp.confirmDelete / confirm
(messages-tab delete, themes activate, theme-customize reset,
notifications delete, utenti deleteUser, events delete,
librarything wipe, book-detail login prompt, book_form cover
remove, generi merge, profile session revoke × 2).
- ~20 alert() error/info messages → SwalApp.error/success/toast
(csv_import, bulk-enrich, settings, theme-customize, edit-routes,
edit-home, book-detail favourites + reservation, collocazione
order save, auth register validation, settings advanced clipboard,
settings index Uppy logo, dashboard ICS copy, events form Uppy).
- 7 verbose 'if (window.Swal) { Swal.fire(...) } else { alert/confirm }'
blocks → single SwalApp.* call (editori crea/modifica, autori
crea/modifica, csv_import showError, import_librarything showError,
settings index Uppy logo).
Left unchanged on purpose:
- app/Views/libri/scheda_libro.php (confirmDeleteBook / confirmRenewal /
book-copy delete / event-cell alert) — already uses
'if (window.Swal) { fancy } else { fallback }' pattern internally;
rewrite would touch 50+ lines for no behavioural change.
- app/Views/dashboard/index.php:957, app/Views/libri/scheda_libro.php:1471
— alert() inside the else-branch of a Swal-or-alert fallback; the
branch is the documented degraded path.
- app/Views/partials/loan-actions-swal.php — already designed as the
canonical Swal+fallback contract for the loan UI.
- app/Views/admin/recensioni/index.php — internal _confirm helper
already implements the same pattern.
- app/Views/frontend/book-detail.php:2729 — prompt() in the
Swal-missing fallback branch; SwalApp.prompt would call the same
window.prompt internally (circular).
i18n: +12 new keys across the 4 locale files (Annulla prenotazione,
Conferma attivazione, Attiva utente, Imposta come Predefinita, Elimina
tutto, Unisci, Attiva, Ripristina, Rimuovi, Confermi?, Errore Upload,
Inserisci un valore), per the i18n-blocking policy.
Validation:
- php -l clean on every modified PHP file.
- PHPStan total error count unchanged vs main (11 pre-existing in
storage/plugins/{archives,bibframe-linked-data,discogs}).
- swal-config.js parses cleanly via 'new Function(fs.readFileSync(...))'.
Playwright tests for the new banner flows land in a follow-up commit
on this same branch (issue #140 spec calls for E2E coverage).
Six tests against the swal-config.js helper bus + the data-*
attribute API introduced by the refactor:
1. window.SwalApp bus is initialized on every admin page
(asserts each helper function exists — guards against
swal-config.js failing to load globally).
2. SwalApp helpers fall back to native dialogs when Swal is
unavailable (stashes window.Swal at runtime; verifies
confirmDelete/error still resolve {isConfirmed} so callers
using .then(r => r.isConfirmed) keep working).
3. data-swal-confirm forms are auto-wired by attachSwalConfirm
(injects a synthetic form, clicks submit, waits for
.swal2-confirm to land, asserts form.submit() is called
after the confirm click).
4. data-swal-confirm cancel keeps the form untouched
(same setup, clicks .swal2-cancel, asserts form.submit()
is NOT called).
5. /admin/settings?tab=cms negative invariant: no form on
the page still uses onsubmit/onclick='return confirm(...)'.
Guards against the regression where the event-layout form
gets refactored back to native by a careless edit.
6. /admin/languages: at least one form has data-swal-confirm
wired (the set-default + delete buttons converted from
inline onclick confirm).
Implementation note: tests 3+4 override form.submit() at the
element level instead of listening on the 'submit' event. The
event fires for both intercepted and post-confirm submits, which
makes it a noisy sentinel; the programmatic .submit() call is
unique to attachSwalConfirm's confirm path.
Validation: 6/6 pass against local Apache + admin login.
Also re-ran tests/issue-137-event-image-layout.spec.js: 8/8 still
green — the swal-config.js helper changes did not regress the
existing event-image-layout flows.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAggiunge setting amministrativo per il layout immagine evento, rende sicuro e simmetrico il ciclo file di featured_image in EventsController, unifica dialog UI verso window.SwalApp con fallback e attach automatico, e copre i cambiamenti con i18n e test Playwright. ChangesEvent Image Layout Setting (Issue
SweetAlert2 Popup Unification (Issue
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Controllers/SettingsController.php`:
- Line 969: Il redirect hardcoded in SettingsController deve usare la route key
invece del percorso letterale: individua il return che invoca
$this->redirect(..., '/admin/settings?tab=cms') all'interno della classe
SettingsController e sostituiscilo costruendo l'URL con la helper di routing (ad
es. route_path('admin.settings', ['tab' => 'cms']) o
RouteTranslator::route('admin.settings', ['tab' => 'cms'])) così da rispettare
la policy di routing e mantenere la query tab=cms come parametro della route.
In `@app/Views/admin/csv_import.php`:
- Around line 552-554: Sostituisci il fallback nativo window.alert(...) con una
chiamata a window.SwalApp (es. window.SwalApp.fire) passando il messaggio (senza
strip HTML se SwalApp gestisce html) e dopo il resolve del suo promise esegui
window.location.reload(); in pratica rimuovi window.alert ando usare
window.SwalApp.fire(...) e poi .then(() => window.location.reload()) in modo che
sia sempre il bus SwalApp a gestire la UI di errore; cerca i riferimenti a
window.alert, window.SwalApp e window.location.reload nel blocco con commento
"Swal not loaded — defensive native fallback before reloading." e applica la
sostituzione lì.
In `@app/Views/admin/theme-customize.php`:
- Around line 359-380: The fetch call that resets the theme lacks network error
handling; append a .catch(...) to the promise chain after the .then(...) that
parses and handles data so network/fetch failures show feedback. In the catch,
call window.SwalApp.error with a short user-facing message (e.g., translated
"Network error" or the existing __("Errore di sicurezza")), log the error via
console.error(err) for debugging, and optionally restore any UI state
(spinners/disabled buttons) that resetToDefaults() might change; target the
fetch(...) block, the promise chain after r.json(), and reuse
window.SwalApp.error and csrfToken context to implement this.
In `@app/Views/frontend/event-detail.php`:
- Around line 467-470: Replace HtmlHelper::e() calls in the view with PHP's
htmlspecialchars and ensure URLs are escaped with htmlspecialchars(url(...),
ENT_QUOTES, 'UTF-8'); specifically replace HtmlHelper::e(implode(' ',
$cardClasses)), HtmlHelper::e($coverLayout) (both for class names and the
data-event-cover-layout attribute), HtmlHelper::e(url($event['featured_image']))
for the img src, and HtmlHelper::e($event['title']) for the alt attribute with
htmlspecialchars(..., ENT_QUOTES, 'UTF-8') and for the src use
htmlspecialchars(url($event['featured_image']), ENT_QUOTES, 'UTF-8') so all
attributes (class, data-*, src, alt) follow the project's view escaping rules.
In `@app/Views/settings/index.php`:
- Line 475: Replace the use of HtmlHelper::e() in the CSRF hidden input with a
direct call to htmlspecialchars; locate the input that uses
HtmlHelper::e(Csrf::ensureToken()) and change it to use
htmlspecialchars(Csrf::ensureToken(), ENT_QUOTES, 'UTF-8') so the token is
escaped per the coding guideline.
In `@app/Views/utenti/index.php`:
- Around line 550-583: The new deleteUser flow (function window.deleteUser,
calls to window.SwalApp.confirmDelete, window.SwalApp.success and
window.SwalApp.error) added several inline __('...') keys (the confirmation
title/text, confirmText, success message "Eliminato!"/"L'utente è stato
eliminato.", error messages used in the .then and .catch branches) that aren't
in i18nTranslations; add those exact keys to the i18n translation files for
every supported locale (not just it) so __('...') resolves everywhere — update
the translation maps/JSON for each locale with the new keys and appropriate
translations, then run the i18n extraction/validation step to ensure no missing
keys remain.
In `@public/assets/js/swal-config.js`:
- Around line 200-202: La logica nel fallback che chiama window.prompt usa la
condizione v !== null && v !== '' per decidere se è isConfirmed, ma questo fa
diventare la stringa vuota una cancellazione; modifica la condizione in modo che
consideri solo v !== null come conferma (usa _hasSwal() per il branch esistente,
il valore restituito deve continuare a passare v o '' al chiamante tramite
_fakeResult), aggiornando la chiamata a _fakeResult(v !== null, v || '') in modo
che una risposta vuota sia trattata come isConfirmed true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cafdbf3d-ff93-4479-b042-a8352523a829
📒 Files selected for processing (46)
app/Controllers/EventsController.phpapp/Controllers/FrontendController.phpapp/Controllers/SettingsController.phpapp/Routes/web.phpapp/Views/admin/bulk-enrich.phpapp/Views/admin/csv_import.phpapp/Views/admin/languages/edit-routes.phpapp/Views/admin/languages/edit.phpapp/Views/admin/languages/index.phpapp/Views/admin/notifications.phpapp/Views/admin/settings.phpapp/Views/admin/theme-customize.phpapp/Views/admin/themes.phpapp/Views/auth/register.phpapp/Views/autori/crea_autore.phpapp/Views/autori/modifica_autore.phpapp/Views/autori/scheda_autore.phpapp/Views/cms/edit-home.phpapp/Views/collocazione/index.phpapp/Views/dashboard/index.phpapp/Views/editori/crea_editore.phpapp/Views/editori/modifica_editore.phpapp/Views/editori/scheda_editore.phpapp/Views/events/form.phpapp/Views/events/index.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/event-detail.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/import_librarything.phpapp/Views/libri/partials/book_form.phpapp/Views/plugins/librarything_admin.phpapp/Views/profile/index.phpapp/Views/profile/reservations.phpapp/Views/settings/advanced-tab.phpapp/Views/settings/index.phpapp/Views/settings/messages-tab.phpapp/Views/user_dashboard/prenotazioni.phpapp/Views/utenti/dettagli_utente.phpapp/Views/utenti/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonpublic/assets/js/swal-config.jstests/issue-137-event-image-layout.spec.jstests/sweetalert2-popups.spec.js
| ConfigStore::set('cms.event_image_layout', $layout); | ||
|
|
||
| $_SESSION['success_message'] = __('Impostazioni eventi aggiornate.'); | ||
| return $this->redirect($response, '/admin/settings?tab=cms'); |
There was a problem hiding this comment.
Evita il redirect hardcoded nel nuovo endpoint.
Il redirect usa un percorso letterale; qui va usata una route key (route_path(...) o RouteTranslator::route(...)) per rispettare la policy di routing.
Diff proposto
- return $this->redirect($response, '/admin/settings?tab=cms');
+ $settingsPath = route_path('admin.settings'); // usa la chiave route già definita nel progetto
+ return $this->redirect($response, $settingsPath . '?tab=cms');As per coding guidelines, app/Controllers/**: "Route: mai hardcodare percorsi URL, usare route_path('key') o RouteTranslator::route('key')".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return $this->redirect($response, '/admin/settings?tab=cms'); | |
| $settingsPath = route_path('admin.settings'); // usa la chiave route già definita nel progetto | |
| return $this->redirect($response, $settingsPath . '?tab=cms'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Controllers/SettingsController.php` at line 969, Il redirect hardcoded in
SettingsController deve usare la route key invece del percorso letterale:
individua il return che invoca $this->redirect(..., '/admin/settings?tab=cms')
all'interno della classe SettingsController e sostituiscilo costruendo l'URL con
la helper di routing (ad es. route_path('admin.settings', ['tab' => 'cms']) o
RouteTranslator::route('admin.settings', ['tab' => 'cms'])) così da rispettare
la policy di routing e mantenere la query tab=cms come parametro della route.
Comment on lines
+467
to
470
| <article class="<?= HtmlHelper::e(implode(' ', $cardClasses)) ?>"> | ||
| <?php if ($hasCoverImage): ?> | ||
| <figure class="event-cover event-cover--<?= HtmlHelper::e($coverLayout) ?>" data-event-cover-layout="<?= HtmlHelper::e($coverLayout) ?>"> | ||
| <img src="<?= HtmlHelper::e(url($event['featured_image'])) ?>" alt="<?= HtmlHelper::e($event['title']) ?>"> |
There was a problem hiding this comment.
Sostituisci HtmlHelper::e() nei nuovi attributi HTML.
Nel nuovo markup stai usando HtmlHelper::e() per attributi (class, data-*, src, alt): qui va usato htmlspecialchars(..., ENT_QUOTES, 'UTF-8'), e per URL in attributo serve esplicitamente htmlspecialchars(url(...), ENT_QUOTES, 'UTF-8').
Diff proposto
- <article class="<?= HtmlHelper::e(implode(' ', $cardClasses)) ?>">
+ <article class="<?= htmlspecialchars(implode(' ', $cardClasses), ENT_QUOTES, 'UTF-8') ?>">
<?php if ($hasCoverImage): ?>
- <figure class="event-cover event-cover--<?= HtmlHelper::e($coverLayout) ?>" data-event-cover-layout="<?= HtmlHelper::e($coverLayout) ?>">
- <img src="<?= HtmlHelper::e(url($event['featured_image'])) ?>" alt="<?= HtmlHelper::e($event['title']) ?>">
+ <figure class="event-cover event-cover--<?= htmlspecialchars($coverLayout, ENT_QUOTES, 'UTF-8') ?>" data-event-cover-layout="<?= htmlspecialchars($coverLayout, ENT_QUOTES, 'UTF-8') ?>">
+ <img src="<?= htmlspecialchars(url($event['featured_image']), ENT_QUOTES, 'UTF-8') ?>" alt="<?= htmlspecialchars($event['title'], ENT_QUOTES, 'UTF-8') ?>">
</figure>
<?php endif; ?>As per coding guidelines, app/Views/**: "Mai usare HtmlHelper::e() nelle view — usare htmlspecialchars(..., ENT_QUOTES, 'UTF-8')" e "CRITICO: htmlspecialchars(url(...), ENT_QUOTES, 'UTF-8') in TUTTI gli attributi HTML (href, action, src)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Views/frontend/event-detail.php` around lines 467 - 470, Replace
HtmlHelper::e() calls in the view with PHP's htmlspecialchars and ensure URLs
are escaped with htmlspecialchars(url(...), ENT_QUOTES, 'UTF-8'); specifically
replace HtmlHelper::e(implode(' ', $cardClasses)), HtmlHelper::e($coverLayout)
(both for class names and the data-event-cover-layout attribute),
HtmlHelper::e(url($event['featured_image'])) for the img src, and
HtmlHelper::e($event['title']) for the alt attribute with htmlspecialchars(...,
ENT_QUOTES, 'UTF-8') and for the src use
htmlspecialchars(url($event['featured_image']), ENT_QUOTES, 'UTF-8') so all
attributes (class, data-*, src, alt) follow the project's view escaping rules.
| ?> | ||
| <div class="mt-5 pt-5 border-t border-gray-200"> | ||
| <form action="<?= htmlspecialchars(url('/admin/settings/events'), ENT_QUOTES, 'UTF-8') ?>" method="post" class="space-y-3"> | ||
| <input type="hidden" name="csrf_token" value="<?= HtmlHelper::e(Csrf::ensureToken()) ?>"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Sostituisci HtmlHelper::e() nel campo CSRF della view.
A Line 475 va usato htmlspecialchars(..., ENT_QUOTES, 'UTF-8') direttamente, non HtmlHelper::e().
Diff proposto
- <input type="hidden" name="csrf_token" value="<?= HtmlHelper::e(Csrf::ensureToken()) ?>">
+ <input type="hidden" name="csrf_token" value="<?= htmlspecialchars(Csrf::ensureToken(), ENT_QUOTES, 'UTF-8') ?>">As per coding guidelines "Mai usare HtmlHelper::e() nelle view — usare htmlspecialchars(..., ENT_QUOTES, 'UTF-8')".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <input type="hidden" name="csrf_token" value="<?= HtmlHelper::e(Csrf::ensureToken()) ?>"> | |
| <input type="hidden" name="csrf_token" value="<?= htmlspecialchars(Csrf::ensureToken(), ENT_QUOTES, 'UTF-8') ?>"> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Views/settings/index.php` at line 475, Replace the use of HtmlHelper::e()
in the CSRF hidden input with a direct call to htmlspecialchars; locate the
input that uses HtmlHelper::e(Csrf::ensureToken()) and change it to use
htmlspecialchars(Csrf::ensureToken(), ENT_QUOTES, 'UTF-8') so the token is
escaped per the coding guideline.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/editori/modifica_editore.php (1)
40-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSostituisci
HtmlHelper::e()conhtmlspecialchars().Nelle view, usare
htmlspecialchars($value, ENT_QUOTES, 'UTF-8')invece diHtmlHelper::e().Correzione suggerita
Esempio per linea 40:
-<strong><?php echo App\Support\HtmlHelper::e($editore['nome'] ?? 'N/A'); ?></strong> +<strong><?php echo htmlspecialchars($editore['nome'] ?? 'N/A', ENT_QUOTES, 'UTF-8'); ?></strong>Applicare lo stesso pattern a tutte le altre occorrenze.
Come da coding guidelines: "Mai usare
HtmlHelper::e()nelle view — usarehtmlspecialchars(..., ENT_QUOTES, 'UTF-8')".Also applies to: 61-61, 65-65, 73-73, 77-77, 83-83, 100-100, 105-105, 109-109, 115-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/editori/modifica_editore.php` at line 40, Replace uses of App\Support\HtmlHelper::e(...) in the view with PHP's htmlspecialchars to ensure proper escaping; e.g. change App\Support\HtmlHelper::e($editore['nome'] ?? 'N/A') to htmlspecialchars($editore['nome'] ?? 'N/A', ENT_QUOTES, 'UTF-8') and apply the same substitution for all other occurrences mentioned (lines referencing HtmlHelper::e at the indicated locations). Locate these calls in the template (e.g., the expression App\Support\HtmlHelper::e(...) on line 40 and the other listed occurrences) and update each to use htmlspecialchars(value, ENT_QUOTES, 'UTF-8') instead of HtmlHelper::e.
♻️ Duplicate comments (5)
app/Views/admin/csv_import.php (1)
552-554:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSostituisci il fallback
window.alertconSwalApp.error.La riga 553 usa ancora
window.alert(...)come fallback nativo, ma questo rompe l'uniformità della PR. Tutti i popup dovrebbero passare attraverso il busSwalAppper garantire UX coerente, fallback defensivi e compatibilità con i test E2E.♻️ Proposta di fix
} else { - // Swal not loaded — defensive native fallback before reloading. - window.alert(message.replace(/<[^>]*>/g, '')); - window.location.reload(); + window.SwalApp.error( + <?= json_encode(__("Import Completato"), JSON_HEX_TAG) ?>, + message.replace(/<[^>]*>/g, '') + ).then(() => window.location.reload()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/admin/csv_import.php` around lines 552 - 554, Replace the native fallback call so the UI uses the app-wide Swal bus: change the current window.alert(message.replace(/<[^>]*>/g, '')); to invoking SwalApp.error with the sanitized message (e.g. SwalApp.error(message.replace(/<[^>]*>/g, ''))), then call window.location.reload() as before; ensure you reference SwalApp.error in the same block where window.location.reload() is used so the behavior and UX remain consistent for E2E tests.app/Views/utenti/index.php (1)
550-583:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLe chiavi i18n usate in
deleteUsernon sono presenti ini18nTranslations.Le righe 555, 556, 557, 571, 575 e 581 usano
__('...')con chiavi non presenti nell'oggettoi18nTranslations(linee 274-284). Questo causa il fallback alle stringhe italiane hardcoded anche quando la locale è diversa dait_IT.Aggiungi le chiavi mancanti a
i18nTranslations:
'Sei sicuro?' => __("Sei sicuro?")'Questa azione non può essere annullata!' => __("Questa azione non può essere annullata!")'Sì, elimina!' => __("Sì, elimina!")'Eliminato!' => __("Eliminato!")'L\'utente è stato eliminato.' => __("L'utente è stato eliminato.")'Non è stato possibile eliminare l\'utente. Controlla la console.' => __("Non è stato possibile eliminare l'utente. Controlla la console.")'Si è verificato un errore: %s' => __("Si è verificato un errore: %s")As per coding guidelines, "Nuove chiavi aggiunte nel codice PHP/JS devono essere aggiunte in TUTTE le lingue".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/utenti/index.php` around lines 550 - 583, The deleteUser function uses several i18n keys (calls to __('Sei sicuro?','Questa azione non può essere annullata!','Sì, elimina!','Eliminato!','L\'utente è stato eliminato.','Non è stato possibile eliminare l\'utente. Controlla la console.','Si è verificato un errore: %s')) that are missing from the i18nTranslations object; add these exact keys to i18nTranslations (the translations map used in this view) and ensure they are added for all supported locales per guidelines so __('...') returns localized strings instead of falling back to hardcoded Italian; search for deleteUser and the surrounding SwalApp.confirmDelete/SwalApp.success/SwalApp.error calls to find the lines to cover.tests/issue-137-event-image-layout.spec.js (2)
471-471:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUsare
sqlEscape()invece del replace inline.Stesso problema della riga 302 — usare la funzione
sqlEscape()già definita.🔧 Fix proposto
- const sqlSafe = shortContent.replace(/'/g, "\\'"); + const sqlSafe = sqlEscape(shortContent);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/issue-137-event-image-layout.spec.js` at line 471, La sostituzione inline che crea sqlSafe da shortContent usando replace(/'/g, "\\'") va sostituita con la funzione di utilità sqlEscape(): sostituisci la riga che definisce sqlSafe con una chiamata a sqlEscape(shortContent) (usa la funzione sqlEscape già definita nello stesso test suite) in modo coerente con l'altra occorrenza alla riga 302; mantieni la stessa variabile sqlSafe e il suo uso a valle.
302-302:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUsare
sqlEscape()invece del replace inline.La funzione
sqlEscape()(riga 58-63) gestisce sia i backslash che gli apostrofi. Qui viene usato solo il replace degli apostrofi, lasciando i backslash non escapati.🔧 Fix proposto
- const sqlSafeLong = longContent.replace(/'/g, "\\'"); + const sqlSafeLong = sqlEscape(longContent);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/issue-137-event-image-layout.spec.js` at line 302, Sostituisci l'escape inline dei soli apostrofi con l'uso di sqlEscape(): anziché costruire sqlSafeLong con longContent.replace(...) usa la funzione sqlEscape passando longContent in modo che vengano correttamente escapati sia i backslash che gli apostrofi; cerca la variabile sqlSafeLong e la funzione sqlEscape per applicare la sostituzione.public/assets/js/swal-config.js (1)
200-202:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLa stringa vuota viene trattata come "annulla" nel fallback nativo.
Nel fallback
window.prompt('')restituisce'', ma la condizionev !== null && v !== ''lo converte in{ isConfirmed: false }. Con Swal, invece, confermare con valore vuoto resta una conferma. Questo crea una divergenza comportamentale nel path degradato.🐛 Fix proposto
if (!_hasSwal()) { const v = window.prompt(text ? title + '\n\n' + text : title, def); - return _fakeResult(v !== null && v !== '', v || ''); + return _fakeResult(v !== null, v === null ? '' : v); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/assets/js/swal-config.js` around lines 200 - 202, La branch di fallback usa window.prompt e tratta la stringa vuota come "annulla": modifica la condizione in quel blocco (riferimenti: _hasSwal, _fakeResult, variabile v) in modo che solo null venga considerato annulla; cioè sostituisci la check v !== null && v !== '' con v !== null e passa il valore effettivo (sostituendo null con '' solo per _fakeResult se necessario) così che '' venga considerato confermato come in Swal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Views/cms/edit-home.php`:
- Around line 758-761: La chiamata diretta a window.SwalApp.error può fallire se
l'helper non è caricato; update il blocco che invoca window.SwalApp.error in
questo file per verificare l'esistenza di window.SwalApp e della funzione error
prima di chiamarla, e fornire un fallback (es. usare console.error e/o mostrare
un alert nativo) quando non è disponibile; in pratica wrap la chiamata in un
controllo tipo if (window.SwalApp && typeof window.SwalApp.error === 'function')
{ ... } else { ... } riferendoti espressamente a window.SwalApp.error nel
controllo e al ramo else per il messaggio di fallback.
In `@app/Views/libri/partials/book_form.php`:
- Around line 1263-1269: The removeCoverImage function currently assumes
window.SwalApp.confirmDelete exists and will throw if SwalApp is missing; update
removeCoverImage to detect if window.SwalApp and its confirmDelete function are
present and, if not, fall back to the native browser confirm() (or an equivalent
Promise-wrapped confirm) so the rest of the handler can continue expecting a
result object with an isConfirmed boolean; specifically, call
window.SwalApp.confirmDelete({...}) when available, otherwise call
window.confirm(__('Sei sicuro di voler rimuovere la copertina?')) and convert
the boolean into a { isConfirmed: true/false } shape before checking
result.isConfirmed.
---
Outside diff comments:
In `@app/Views/editori/modifica_editore.php`:
- Line 40: Replace uses of App\Support\HtmlHelper::e(...) in the view with PHP's
htmlspecialchars to ensure proper escaping; e.g. change
App\Support\HtmlHelper::e($editore['nome'] ?? 'N/A') to
htmlspecialchars($editore['nome'] ?? 'N/A', ENT_QUOTES, 'UTF-8') and apply the
same substitution for all other occurrences mentioned (lines referencing
HtmlHelper::e at the indicated locations). Locate these calls in the template
(e.g., the expression App\Support\HtmlHelper::e(...) on line 40 and the other
listed occurrences) and update each to use htmlspecialchars(value, ENT_QUOTES,
'UTF-8') instead of HtmlHelper::e.
---
Duplicate comments:
In `@app/Views/admin/csv_import.php`:
- Around line 552-554: Replace the native fallback call so the UI uses the
app-wide Swal bus: change the current window.alert(message.replace(/<[^>]*>/g,
'')); to invoking SwalApp.error with the sanitized message (e.g.
SwalApp.error(message.replace(/<[^>]*>/g, ''))), then call
window.location.reload() as before; ensure you reference SwalApp.error in the
same block where window.location.reload() is used so the behavior and UX remain
consistent for E2E tests.
In `@app/Views/utenti/index.php`:
- Around line 550-583: The deleteUser function uses several i18n keys (calls to
__('Sei sicuro?','Questa azione non può essere annullata!','Sì,
elimina!','Eliminato!','L\'utente è stato eliminato.','Non è stato possibile
eliminare l\'utente. Controlla la console.','Si è verificato un errore: %s'))
that are missing from the i18nTranslations object; add these exact keys to
i18nTranslations (the translations map used in this view) and ensure they are
added for all supported locales per guidelines so __('...') returns localized
strings instead of falling back to hardcoded Italian; search for deleteUser and
the surrounding SwalApp.confirmDelete/SwalApp.success/SwalApp.error calls to
find the lines to cover.
In `@public/assets/js/swal-config.js`:
- Around line 200-202: La branch di fallback usa window.prompt e tratta la
stringa vuota come "annulla": modifica la condizione in quel blocco
(riferimenti: _hasSwal, _fakeResult, variabile v) in modo che solo null venga
considerato annulla; cioè sostituisci la check v !== null && v !== '' con v !==
null e passa il valore effettivo (sostituendo null con '' solo per _fakeResult
se necessario) così che '' venga considerato confermato come in Swal.
In `@tests/issue-137-event-image-layout.spec.js`:
- Line 471: La sostituzione inline che crea sqlSafe da shortContent usando
replace(/'/g, "\\'") va sostituita con la funzione di utilità sqlEscape():
sostituisci la riga che definisce sqlSafe con una chiamata a
sqlEscape(shortContent) (usa la funzione sqlEscape già definita nello stesso
test suite) in modo coerente con l'altra occorrenza alla riga 302; mantieni la
stessa variabile sqlSafe e il suo uso a valle.
- Line 302: Sostituisci l'escape inline dei soli apostrofi con l'uso di
sqlEscape(): anziché costruire sqlSafeLong con longContent.replace(...) usa la
funzione sqlEscape passando longContent in modo che vengano correttamente
escapati sia i backslash che gli apostrofi; cerca la variabile sqlSafeLong e la
funzione sqlEscape per applicare la sostituzione.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 342734e9-3f90-4d84-a709-cb9681bb29fc
📒 Files selected for processing (46)
app/Controllers/EventsController.phpapp/Controllers/FrontendController.phpapp/Controllers/SettingsController.phpapp/Routes/web.phpapp/Views/admin/bulk-enrich.phpapp/Views/admin/csv_import.phpapp/Views/admin/languages/edit-routes.phpapp/Views/admin/languages/edit.phpapp/Views/admin/languages/index.phpapp/Views/admin/notifications.phpapp/Views/admin/settings.phpapp/Views/admin/theme-customize.phpapp/Views/admin/themes.phpapp/Views/auth/register.phpapp/Views/autori/crea_autore.phpapp/Views/autori/modifica_autore.phpapp/Views/autori/scheda_autore.phpapp/Views/cms/edit-home.phpapp/Views/collocazione/index.phpapp/Views/dashboard/index.phpapp/Views/editori/crea_editore.phpapp/Views/editori/modifica_editore.phpapp/Views/editori/scheda_editore.phpapp/Views/events/form.phpapp/Views/events/index.phpapp/Views/frontend/book-detail.phpapp/Views/frontend/event-detail.phpapp/Views/generi/dettaglio_genere.phpapp/Views/libri/import_librarything.phpapp/Views/libri/partials/book_form.phpapp/Views/plugins/librarything_admin.phpapp/Views/profile/index.phpapp/Views/profile/reservations.phpapp/Views/settings/advanced-tab.phpapp/Views/settings/index.phpapp/Views/settings/messages-tab.phpapp/Views/user_dashboard/prenotazioni.phpapp/Views/utenti/dettagli_utente.phpapp/Views/utenti/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonpublic/assets/js/swal-config.jstests/issue-137-event-image-layout.spec.jstests/sweetalert2-popups.spec.js
Comment on lines
+758
to
+761
| window.SwalApp.error( | ||
| <?= json_encode(__('Errore Upload'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>, | ||
| error.message | ||
| ); |
There was a problem hiding this comment.
Aggiungi fallback se SwalApp non è disponibile
La chiamata diretta a window.SwalApp.error(...) può lanciare un errore runtime e perdere il feedback utente in caso di helper non caricato.
Patch proposta
- window.SwalApp.error(
- <?= json_encode(__('Errore Upload'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>,
- error.message
- );
+ const uploadErrorTitle = <?= json_encode(__('Errore Upload'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>;
+ if (window.SwalApp?.error) {
+ window.SwalApp.error(uploadErrorTitle, error.message);
+ } else {
+ window.alert(`${uploadErrorTitle}\n\n${error.message || ''}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| window.SwalApp.error( | |
| <?= json_encode(__('Errore Upload'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>, | |
| error.message | |
| ); | |
| const uploadErrorTitle = <?= json_encode(__('Errore Upload'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>; | |
| if (window.SwalApp?.error) { | |
| window.SwalApp.error(uploadErrorTitle, error.message); | |
| } else { | |
| window.alert(`${uploadErrorTitle}\n\n${error.message || ''}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Views/cms/edit-home.php` around lines 758 - 761, La chiamata diretta a
window.SwalApp.error può fallire se l'helper non è caricato; update il blocco
che invoca window.SwalApp.error in questo file per verificare l'esistenza di
window.SwalApp e della funzione error prima di chiamarla, e fornire un fallback
(es. usare console.error e/o mostrare un alert nativo) quando non è disponibile;
in pratica wrap la chiamata in un controllo tipo if (window.SwalApp && typeof
window.SwalApp.error === 'function') { ... } else { ... } riferendoti
espressamente a window.SwalApp.error nel controllo e al ramo else per il
messaggio di fallback.
Comment on lines
+1263
to
1269
| async function removeCoverImage() { | ||
| const result = await window.SwalApp.confirmDelete({ | ||
| text: __('Sei sicuro di voler rimuovere la copertina?'), | ||
| confirmText: __('Rimuovi') | ||
| }); | ||
| if (!result.isConfirmed) return; | ||
|
|
There was a problem hiding this comment.
Mantieni un fallback nativo quando SwalApp manca
Qui la rimozione copertina dipende totalmente da window.SwalApp; se non è disponibile, l’azione si rompe.
Patch proposta
async function removeCoverImage() {
- const result = await window.SwalApp.confirmDelete({
- text: __('Sei sicuro di voler rimuovere la copertina?'),
- confirmText: __('Rimuovi')
- });
- if (!result.isConfirmed) return;
+ const confirmText = __('Sei sicuro di voler rimuovere la copertina?');
+ const result = window.SwalApp?.confirmDelete
+ ? await window.SwalApp.confirmDelete({
+ text: confirmText,
+ confirmText: __('Rimuovi')
+ })
+ : { isConfirmed: window.confirm(confirmText) };
+ if (!result?.isConfirmed) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function removeCoverImage() { | |
| const result = await window.SwalApp.confirmDelete({ | |
| text: __('Sei sicuro di voler rimuovere la copertina?'), | |
| confirmText: __('Rimuovi') | |
| }); | |
| if (!result.isConfirmed) return; | |
| async function removeCoverImage() { | |
| const confirmText = __('Sei sicuro di voler rimuovere la copertina?'); | |
| const result = window.SwalApp?.confirmDelete | |
| ? await window.SwalApp.confirmDelete({ | |
| text: confirmText, | |
| confirmText: __('Rimuovi') | |
| }) | |
| : { isConfirmed: window.confirm(confirmText) }; | |
| if (!result?.isConfirmed) return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Views/libri/partials/book_form.php` around lines 1263 - 1269, The
removeCoverImage function currently assumes window.SwalApp.confirmDelete exists
and will throw if SwalApp is missing; update removeCoverImage to detect if
window.SwalApp and its confirmDelete function are present and, if not, fall back
to the native browser confirm() (or an equivalent Promise-wrapped confirm) so
the rest of the handler can continue expecting a result object with an
isConfirmed boolean; specifically, call window.SwalApp.confirmDelete({...}) when
available, otherwise call window.confirm(__('Sei sicuro di voler rimuovere la
copertina?')) and convert the boolean into a { isConfirmed: true/false } shape
before checking result.isConfirmed.
VALID (fixed in this commit):
- tests/issue-137-event-image-layout.spec.js teardown: the
startsWith('/uploads/events/') guard did not block path traversal
when currentImage contained '..' segments. Switched to path.resolve
from the uploads-events root + path.relative containment check so
fs.unlinkSync can never escape public/uploads/events. The
controller already sanitizes the value it writes, but the teardown
reads from the DB; defense in depth.
- public/assets/js/swal-config.js SwalApp.prompt(): the native
fallback treated an empty string as 'cancel'. Swal's semantics
accept '' as a valid confirmed value, so the fallback now mirrors
that: only null -> isConfirmed=false; '' -> isConfirmed=true.
- app/Views/admin/csv_import.php: the import-completed fallback
branch still called window.alert directly. Routed through
SwalApp.info instead — its internal native fallback covers the
Swal-missing case, keeps the bus as the single entry point.
- app/Views/admin/theme-customize.php resetToDefaults: missing
.catch on the reset fetch left the user without feedback on
network failure. Added a .catch that surfaces SwalApp.error +
console.error.
- tests/issue-137-event-image-layout.spec.js: two inline
.replace(/'/g, ...) calls only escaped apostrophes, missing
backslashes (CodeQL flagged both). Switched to the existing
sqlEscape() utility which handles both.
- tests/sweetalert2-popups.spec.js: removed unused origSubmit
variable; clarified the comment that explains why we don't call
through to the original HTMLFormElement.prototype.submit.
- app/Views/utenti/index.php: 7 i18n keys consumed by the new
deleteUser flow (Sei sicuro? / Sì, elimina! / Eliminato! / etc.)
were absent from the JS i18nTranslations map, causing non-it
locales to see the Italian source string. Added them. (Keys
already exist in locale/*.json — the map just needed the entry.)
SKIPPED (with reason):
- HtmlHelper::e() → htmlspecialchars(...) in event-detail.php,
settings/index.php (CSRF), modifica_editore.php: pre-commit hook
explicitly accepts both (grep -v 'HtmlHelper::e'). The repo has
20+ uses of HtmlHelper::e in these same files that predate this
PR; the 'coding guideline' CodeRabbit cites is invented.
- SettingsController::updateEventSettings redirect to
/admin/settings?tab=cms: /admin/* routes are not in
routes_*.json (only public routes like /accedi are translated).
Every other updateXxx handler in the same controller uses the
same literal pattern; rewriting only this one creates asymmetry.
- 'window.SwalApp may be missing' defensive guards in
cms/edit-home.php and libri/partials/book_form.php: SwalApp is
defined unconditionally in swal-config.js which loads via the
base layout. If swal-config.js fails to parse, every admin
interaction breaks — the per-call guard would not save us.
SwalApp helpers internally already fall back to window.confirm /
window.alert when Swal itself is missing.
Validation: 14/14 E2E pass (issue-137 + sweetalert2-popups).
PHPStan: 11 errors unchanged from main (pre-existing in
storage/plugins/{archives,bibframe-linked-data,discogs}).
L1 diff-local lens flagged 10 candidates on the SwalApp helper bus.
After triage, three are genuine bugs worth fixing now:
- confirmDelete native fallback used 'title + \n\n + text'
unconditionally even when the resolved text was undefined,
producing 'title\n\nundefined' in the native confirm prompt.
Mirrored confirm()'s guarded shape: msg = text ? title+\n\n+text
: title.
- attachSwalConfirm always called confirmDelete, which paints a
red destructive button. For non-destructive flows (Set as
Default language, Activate user, Reset colours, …) the red
button is misleading. Added data-swal-confirm-kind='action' that
routes to SwalApp.confirm (neutral gray button) instead. Default
behaviour unchanged for forms without the attribute.
- attachSwalConfirm cleared data-swal-confirm='' to signal the
re-submit; if form.submit() ever failed or got blocked, the
cleared attribute permanently bypassed the dialog on the next
click. Switched to a one-shot data-swal-proceed marker that the
listener resets after seeing it, so the dialog always gates the
next user submit even if the prior one was interrupted.
- profile/index.php revokeSession passed
translations.confirmRevokeButton || undefined. If the i18n value
was an empty string the falsy || silently discarded an explicit
empty translation. Tightened to typeof === 'string' && length>0
so empty values fall through to the helper default cleanly.
Skipped (over-flag or pre-existing):
- Loading-spinner race in editori/autori save flow: non-awaited
Swal.fire({showLoading}) immediately followed by form.submit().
Pre-existing pattern from before the refactor; the spinner is
visible for the dispatch tick and the page navigates away.
Touching this is outside the SwalApp-unification scope.
- confirm() fallback text concatenation: already guarded.
- generi merge-form e.preventDefault ordering: correct as written
(guard returns before preventDefault); L1 over-flagged.
Validation: 14/14 E2E pass (issue-137 + sweetalert2-popups suites).
PHP lint clean. swal-config.js JS-syntax OK.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/assets/js/swal-config.js`:
- Around line 285-294: The dialog always overrides confirmText to 'Elimina' even
for neutral actions; update the code around form.dataset.swalConfirmKind / const
kind so that when kind === window.SwalApp.confirm (i.e., data-swal-confirm-kind
=== 'action') the confirmText default is 'Conferma' (or better: only set
confirmText when not passed and choose the default based on kind), instead of
unconditionally using 'Elimina'; adjust the object passed into
kind.call(window.SwalApp, {...}) to pick the correct default confirmText or
defer to the helper's own default for confirm() vs confirmDelete().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 551b508c-61a0-4f8b-813d-d188a5481716
📒 Files selected for processing (2)
app/Views/profile/index.phppublic/assets/js/swal-config.js
Owner
Author
Code reviewBranch: Found 10 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (3)
Details and fix proposalsF002 — attachSwalConfirm clears data-swal-confirm to '' before calling form.submit(). If form.submit() throws or the submit is otherwise intercepted, the next click short-circuits via !form.dataset.swalConfirm and fires the raw submit with no dialog, bypassing the guard permanently.File: Evidence:
Approach: see commit 383b0a2 Files to modify:
Verification:
Latest fix attempt (fixrun_20260521T210013Z3d56bd): fixed and verified F007 — translations.confirmRevokeButton || undefined: if value is an empty string (key present but empty), the || falls through to undefined, silently discarding the intended custom button label.File: Evidence:
Approach: see commit 383b0a2 Files to modify:
Verification:
Latest fix attempt (fixrun_20260521T204910Z93b138): fixed and verified F009 — confirmDelete fallback builds the native confirm() string as title + '\n\n' + text but when text is undefined, this produces 'title\n\nundefined' in the native alert.File: Evidence:
Approach: see commit 383b0a2 Files to modify:
Verification:
Latest fix attempt (fixrun_20260521T204910Z93b138): fixed and verified Fix runsRun
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F001 | FG-1 | ✓ fixed and verified | |
| F007 | FG-1 | ✓ fixed and verified | |
| F009 | FG-1 | ✓ fixed and verified |
Run fixrun_20260521T210013Z3d56bd — 2026-05-21T21:00:13Z
- Outcomes: 1 fixed and verified
- Commits:
383b0a2
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F002 | FG-1 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Owner
Author
Walkthrough decisions
Walking the Qualifying scope: of 1 non-auto-eligible finding(s), 1 promoted via per-finding walk-through. Promoted findings will be picked up by the next Promoted
Decisions log: append-only audit. Current state: see main review comment. |
…able tests CodeRabbit fix on PR #141 ------------------------- attachSwalConfirm's confirmText default was unconditionally 'Elimina', even when data-swal-confirm-kind="action" routed the dialog through SwalApp.confirm (the neutral/gray helper). A form opting into kind="action" without an explicit data-swal-confirm-button would render a gray button labelled "Elimina" — the wrong destructive cue. Default now follows the kind: action→Conferma, destructive→Elimina. Explicit data-swal-confirm-button still wins for both. UX follow-ups (3 non-destructive forms) --------------------------------------- - admin/languages/index.php: set-default-language → kind=action - utenti/dettagli_utente.php: activate-directly → kind=action - utenti/index.php: activate-directly → kind=action These three flows are reversible (re-set language, deactivate user) and were showing a red destructive dialog. Now neutral/gray. 49 reusable Playwright tests ---------------------------- - tests/sweetalert2-comprehensive.spec.js (25 tests): Block A — window.SwalApp bus contract (6) Block B — attachSwalConfirm auto-wire / idempotence / proceed-marker (5) Block C — kind routing + confirmText defaults (3, validates this fix) Block D — i18n & data-attr propagation (3) Block E — refactored views per-page assertions (8, login-gated, skip-clean) - tests/pr-139-event-image-additional.spec.js (11 tests): Block S — controller orphan-cleanup symmetry + path-traversal hardening + locale completeness + settings/route contract (9) Block H — admin-side HTTP contract checks (2, login-gated) - tests/pr-136-archives-ric-cm-additional.spec.js (13 tests): Migration version-le-release invariant (project rule #6), ensureSchema() pattern compliance, plugin.json bumps, RiC-O entity coverage, README freshness. Reads files from origin/feat/ric-cm-archives via 'git show', so the suite works from any branch without checking out. Validation ---------- - 39 no-login tests run green in ~30s - 10 login-gated tests skip cleanly when E2E_ADMIN_PASS is absent - All 13 pr-136 tests pass against origin/feat/ric-cm-archives Refs: PR #141 (closes #140), PR #139 (closes #137), PR #136 (closes #122)
…state The first Block E run against a live install surfaced two assumptions that the original selectors didn't honour: 1. Admin entity indexes (/admin/autori, /admin/editori, /admin/generi) are DataTables-Ajax shells. The author detail links don't exist in the initial HTML — they're rendered client-side from the /api/<plural> response. Following 'a[href*="/autori/"]' matched the index URL itself (the active tab href) and navigated nowhere useful. 2. The scheda views render the delete-form ONLY when the entity is actually deletable: autori/editori need libri_count=0, generi need children_count=0. The first author on a seeded install almost always has at least one associated book, so the delete-form path was unreachable and the test failed on toBeAttached. Fix: - Resolve the first deletable entity id via the canonical /api/<plural> endpoint, filtering by the dependency-count column the scheda gates the form on. Navigate directly to /admin/<plural>/<id>. - If no row is deletable (legitimate on installs where every entity has dependencies), test.skip cleanly with a reason naming the dep. Validation against the local install (47 authors, 46 publishers, 219 genres, 0 shelves, all admin users active): - E3, E4, E5, E6, E7 pass green - E1, E2 skip (no email_verificata=0 users — seed required) - E8 skip (0 scaffali — seed required) - Full pack: 46/49 passed, 3 skipped (legitimate), 0 failed
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/sweetalert2-comprehensive.spec.js`:
- Around line 545-560: The test's firstDeletableId currently considers only a
single dependency key (hasDepKey) when deciding if a row is deletable, which can
select rows with children_count===0 but libri_count>0; update firstDeletableId
so the deletable check inspects both children_count and libri_count (treating
missing/null as 0) — inside the rows.find callback (in function
firstDeletableId) require that Number(r[hasDepKey] || 0) === 0 AND
Number(r.libri_count || 0) === 0 (or, more generically, accept hasDepKey as
array and ensure all listed dep keys are 0) so the function only returns rows
truly deletable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b628eb75-90ca-4b8e-91ef-bfc65f331956
📒 Files selected for processing (1)
tests/sweetalert2-comprehensive.spec.js
After PR #141 (popup unification), the set-default-language form on /admin/languages no longer triggers a native window.confirm() — the submit is intercepted by attachSwalConfirm and a SweetAlert2 modal takes over. The legacy `page.once('dialog', d => d.accept())` pattern never fired because there was no native dialog to capture, so the form was never submitted, the language never changed, and the DB query expected fr_FR but received it_IT. Replace the 4 Phase-21 callsites with a `submitSetDefaultAndConfirm` helper that handles both paths: - SwalApp modal (current behaviour): wait for .swal2-confirm and click - Native confirm() (legacy / older installs): the pre-installed dialog handler accepts it The helper is forward- and backward-compatible: on a pre-PR-141 install the SwalApp wait times out (1.5s) and the native handler resolves; on a post-PR-141 install the .swal2-confirm path wins because the modal lands first and the submit was preventDefault-ed before any native dialog could surface. Validation (locally, after restoring it_IT as default): - 21.1 fr_FR pass (1.7s) - 21.2 LibraryThing French strings pass (1.3s) - 21.3 en_US + de_DE pass (1.9s) - 21.4 it_IT restore pass (1.2s) Refs: PR #141 (closes #140)
CodeRabbit review on 5701c58 caught a real correctness gap in the E7 helper. The previous signature took a single `hasDepKey` string and returned the first row where THAT key was 0/null/missing. For E7 (genre delete) the call site passed `'children_count'` only — meaning a genre with `children_count=0` but `libri_count > 0` would be picked as "deletable", but dettaglio_genere.php gates the delete-form on BOTH dependencies (no sub-genres AND no books). The picked row would still render the "non eliminabile" lock button and the toBeAttached assertion would fail. Fix: - `firstDeletableId(apiPath, depKeys)` now accepts a string OR an array of dep keys. The single-string form (autori, editori) is unchanged; the array form (generi) requires EVERY listed dep to reach zero before the row is picked. - E7 call site passes `['children_count', 'libri_count']` to mirror the view's actual gate. - Doc comment + per-dep comment on the E7 call site explain the "view is the source of truth" reasoning so a future reader doesn't drop one of the deps and reintroduce the gap. Validated locally with E2E_ADMIN_PASS set: - E3, E4, E5, E6, E7 pass green - E1, E2, E8 skip cleanly (DB seed state, not test bug) - Block E end-to-end: 5 passed, 3 skipped, 0 failed Refs: PR #141 (closes #140)
4 tasks
Owner
Author
|
Superseded by #144 (combined/release-unified) — the unified release branch merges this work alongside the other two open PRs into a single release. All commits from this branch are preserved in the merge; CI green on the combined branch. This PR is closed, not merged — the branch |
ctariel
pushed a commit
to ctariel/Pinakes
that referenced
this pull request
Jun 4, 2026
…#141 + fabiodalez-dev#139) into unified release Brings in: - PR fabiodalez-dev#141 (closes fabiodalez-dev#140): full SweetAlert2 popup unification — every native window.alert/confirm/prompt routed through a single SwalApp helper bus with defensive native fallback. 11+ form/onsubmit/onclick refactors via the `data-swal-confirm` attribute pattern. - PR fabiodalez-dev#139 (closes fabiodalez-dev#137): admin-configurable event image layout — 4 presets (full / banner / contained / thumb) on /eventi/<slug>, picker in admin/settings?tab=cms. - Validation work that landed during the review pass: - CodeRabbit-driven fix: confirmText default follows the kind (action→Conferma, destructive→Elimina) instead of always defaulting to Elimina (commit 383b0a2) - UX follow-ups: set-default-language + activate-user → kind=action (commit 450ed8e) - 49 reusable Playwright regression tests: sweetalert2-comprehensive.spec.js (25) pr-139-event-image-additional.spec.js (11) pr-136-archives-ric-cm-additional.spec.js (13) - Block E selector hardening for real admin URLs + DB seed state (commit 5701c58) - Phase 21 (full-test.spec.js) language-switch tests adapted to the SwalApp confirmation modal (commit 318117b) - CodeRabbit-driven fix on firstDeletableId: check ALL dep keys (children_count AND libri_count for generi) rather than just one (commit dcf98a0) PR fabiodalez-dev#139 commits are already included in fabiodalez-dev#141's stack — no duplication. Refs: PR fabiodalez-dev#141 (closes fabiodalez-dev#140), PR fabiodalez-dev#139 (closes fabiodalez-dev#137) # Conflicts: # locale/de_DE.json # locale/en_US.json # locale/fr_FR.json # locale/it_IT.json
ctariel
pushed a commit
to ctariel/Pinakes
that referenced
this pull request
Jun 4, 2026
…dalez-dev#136 + fabiodalez-dev#141 When the two PRs landed independently on their respective branches, each branch carried inline @PHPStan-Ignore annotations and a baseline entry pinned to errors that no longer reproduce on the combined tree: - ScrapeController.php:769 — Z39Server plugin classes are autoloaded via storage/plugins from the archives PR; `class.notFound` no longer fires. - DiscogsPlugin.php:28 — the `hookManager` property is now used further down the file; the "kept for interface compatibility" ignore is obsolete. - GoodLibPlugin.php:44/90/98 — constructor.unusedParameter + method.unused on onInstall / onUninstall: the methods are now called by the activated PluginManager surface. - BibframeLinkedDataPlugin.php:720 — `is_string($predicate)` was always true because the foreach key is typed as string by `@param array<string, mixed> $node`. Drop the redundant check (no behaviour change — the rest of the conditional already rejects @id/@type/non-colon keys). - phpstan-baseline.neon — the 3 entries (LibriController CURL constant + BookRepository isset offsets) all stopped reproducing after the merge. Empty baseline now (the array is still present so a future restore can append cleanly). PHPStan level 5 passes on the combined tree: $ phpstan analyse --memory-limit=512M [OK] No errors
ctariel
pushed a commit
to ctariel/Pinakes
that referenced
this pull request
Jun 4, 2026
…+ fabiodalez-dev#139 + fabiodalez-dev#136 Adds tests/unified-release-regression.spec.js — a focused regression suite that targets the user-observable contract each of the 3 combined PRs introduces. Designed to sit alongside the existing per-PR specs (sweetalert2-comprehensive, pr-139-event-image-additional, pr-136-archives-ric-cm-additional, full-test, etc.) as a fast, re-runnable safety net for the release branch. Distribution (25 tests): Block U-SWAL (8) — PR fabiodalez-dev#141 popup unification S1: SwalApp method surface S2: confirmText default branches on isAction (CodeRabbit fix) S3: defensive native fallback when Swal absent S4: one-shot data-swal-proceed marker (not attribute mutation) S5: 3 non-destructive forms set kind="action" (UX follow-up) S6: it_IT.json carries SwalApp default copy H1: window.SwalApp initialised on admin pages H2: data-swal-confirm forms auto-attached on /admin/utenti Block U-EVT (8) — PR fabiodalez-dev#139 event image layout (fabiodalez-dev#137) S1: 4-preset enum in settings/index.php S2: event-detail.php renders event-cover--<preset> S3: SettingsController validates with in_array S4: 4 deleteUploadedImageFile lifecycle callsites S5: path-traversal hardening (prefix + realpath containment) S6: @media restyle for thumb layout H1: /admin/settings?tab=cms renders the layout picker H2: form posts to /admin/settings/events with CSRF Block U-RIC (9) — PR fabiodalez-dev#136 RiC-CM full roadmap (fabiodalez-dev#122) S1: phase 2/3/4 + cleanup migration files S2: version.json ≥ highest migration version S3: 0.7.09.sql creates Phase 3 tables idempotently S4: 0.7.10.sql creates Phase 4 places + relations S5: ArchivesPlugin ensureSchema() called from BOTH hooks S6: RicJsonLdBuilder covers RecordResource/Agent/Activity/Place S7: OaiPmhServerPlugin handles metadataPrefix=ric-o S8: archives plugin.json ≥ 1.5.0, oai-pmh-server ≥ 1.1.0 H1: /archives/collection.ric.json returns JSON-LD Design choices: - 16 tests run without admin login (static checks on repo files + one no-auth HTTP smoke for the public RiC-O endpoint). - 9 tests require login — they test.skip() cleanly on missing credentials, wrong password, or missing seed state (legitimate skip, never fail). - No state mutation — every assertion reads HTML / JSON / static files. Re-runnable in any order, parallel-safe. Run: /tmp/run-e2e.sh tests/unified-release-regression.spec.js \ --config=tests/playwright.config.js --workers=1 Validated locally with E2E_ADMIN_PASS set: 24 passed, 1 skipped (U-SWAL-H2 when /admin/utenti has no pending users — legitimate seed state, not a test bug).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #140.
Refactor every native
alert()/confirm()/prompt()across the views to the existingwindow.SwalApphelper bus, plus add a defensive native fallback to every helper so the destructive flows still work when the SweetAlert2 bundle fails to load (CDN error, CSP, etc.).Note on the diff: this branch was cut from
feat/event-image-layout-setting(PR #139) so the SwalApp work would start from the recent EventsController + spec teardown improvements. The first 9 commits in this PR are the #139 set; they will collapse out of the diff automatically when #139 lands onmain. The genuinely new commits are the last two:chore: unify client-side popups to SweetAlert2 (#140)— 39 files, ~45 sitestest(swal): E2E coverage for issue #140 popup unification— 1 new spec, 6 testsWhat changed
public/assets/js/swal-config.js(helper bus)confirmDelete/confirm/success/error/info/warning/toast) now has a defensive native fallback — whenSwalis missing the helper resolves towindow.confirm/window.alertwith the same{isConfirmed, value}Promise shape, so callers using.then(r => r.isConfirmed && doStuff())keep working.SwalApp.prompt(opts)— single-line text input dialog. Falls back towindow.promptwhen Swal is unavailable.SwalApp.attachSwalConfirm(selector)— auto-wires every form carryingdata-swal-confirmto a Swal confirm dialog. Auto-attaches onDOMContentLoaded(idempotent viadata-swal-attachedmarker).Refactor categories
<form onsubmit/onclick="return confirm(...)">data-swal-confirmattribute on<form>; auto-wired byattachSwalConfirm()confirm()before actionSwalApp.confirmDelete/SwalApp.confirmwith.then(r => r.isConfirmed && …)alert()error/infoSwalApp.error/SwalApp.success/SwalApp.toastif (window.Swal) { Swal.fire(…) } else { alert/confirm(…) }SwalApp.*call (collapses 10+ lines to 1)Left unchanged on purpose (documented in commit message)
app/Views/libri/scheda_libro.php— already usesif (Swal) { fancy } else { fallback }internally; rewrite would touch 50+ lines for no behavioural change.app/Views/dashboard/index.php:957,app/Views/libri/scheda_libro.php:1471—alert()inside the else branch of a documented Swal-or-alert fallback.app/Views/partials/loan-actions-swal.php— already the canonical Swal+fallback contract for the loan UI.app/Views/admin/recensioni/index.php— internal_confirmhelper already implements the pattern.app/Views/frontend/book-detail.php:2729—prompt()in the Swal-missing fallback branch;SwalApp.promptwould call the samewindow.promptinternally (circular).i18n
+12 new keys across all 4 locale files (
it_IT,en_US,de_DE,fr_FR) in the same commit, per the project i18n-blocking policy:Annulla prenotazione,Conferma attivazione,Attiva utente,Imposta come Predefinita,Elimina tutto,Unisci,Attiva,Ripristina,Rimuovi,Confermi?,Errore Upload,Inserisci un valore.Test plan
php -lclean on every modified PHP file (34 files).main, all pre-existing instorage/plugins/{archives,bibframe-linked-data,discogs}).swal-config.jssyntax check vianew Function(fs.readFileSync(...)).tests/sweetalert2-popups.spec.js— 6/6 pass:window.SwalAppbus initialized on every admin pagedata-swal-confirmforms are auto-wired byattachSwalConfirmform.submit()is NOT called when user clicks.swal2-cancel/admin/settings?tab=cms— negative invariant: no nativeonsubmit confirmon the event-layout form/admin/languages— at least oneform[data-swal-confirm]is wiredtests/issue-137-event-image-layout.spec.js— 8/8 still pass.Implementation notes for reviewers
form.submit()override vssubmitevent: thesubmitevent fires for both intercepted and post-confirm submits (becausepreventDefaultdoesn't stop other listeners). The programmaticform.submit()call is unique toattachSwalConfirm's confirm path — that's the sentinel the tests use.data-swal-attached='1'idempotency marker:attachSwalConfirm()can be called multiple times safely (e.g. after injecting a form dynamically). Existing wiring is not duplicated.data-swal-confirm=''re-submit escape: after a successful confirm, the handler setsdataset.swalConfirm = ''soform.submit()reaches the action without re-prompting (otherwise we'd recurse).Summary by CodeRabbit
Nuove funzionalità
Miglioramenti
Test
Localizzazioni