feat: ISSN visibility, series links, multi-volume works (#75)#95
Open
fabiodalez-dev wants to merge 26 commits intomainfrom
Open
feat: ISSN visibility, series links, multi-volume works (#75)#95fabiodalez-dev wants to merge 26 commits intomainfrom
fabiodalez-dev wants to merge 26 commits intomainfrom
Conversation
ISSN:
- Move ISSN field from LibraryThing section to main form (next to EAN)
- Add ISSN format validation (XXXX-XXXX pattern) in JS
- Show ISSN in admin book detail and frontend book detail
Series ("Nella stessa collana"):
- Query books with same collana value in FrontendController
- Show pill-style links to other volumes in the series on frontend
book detail, ordered by numero_serie
- Add idx_collana index for efficient series queries
Multi-volume works:
- Create volumi table linking parent works to individual volume books
- Admin book detail shows "Volumi di quest'opera" table when book
is a parent work (opera)
- Admin book detail shows "Volume X of WORK" badge when book is a
child volume
- Each volume is an independent book with own metadata/loans
- Migration 0.5.1: CREATE TABLE volumi + ADD INDEX idx_collana
Version bumped to 0.5.1. Translations added for EN and DE.
Refs #75
Add Priority 0 (same collana) before same-author in getRelatedBooks(). Books in the same series are shown first, ordered by numero_serie. Remaining slots are filled with same-author, same-genre, then recent books — ensuring the 3 related slots always have content.
Tests cover: 1. ISSN field visible in main form (not hidden in LT section) 2. ISSN value persists in admin detail 3. ISSN displayed on frontend book detail 4. ISSN HTML pattern validation (XXXX-XXXX format) 5. idx_collana index exists in database 6. Frontend "same series" section shows sibling books 7. volumi table structure (columns, unique key, foreign keys) 8. Admin detail shows volumes table for parent works 9. Admin detail shows parent work badge for child volumes 10. Related books prioritize same-series (verified via DB + EXPLAIN) Also removes duplicate ISSN input from LibraryThing section (moved to main form in previous commit).
Critical fixes: - Guard $stmt->bind_param() with if($stmt) in getRelatedBooks Priority 1 to prevent fatal crash on prepare failure - Add idx_collana index to schema.sql for fresh install parity - Remove INFORMATION_SCHEMA.TABLES check on every admin page load; prepare() returning false already handles missing table - Add LIMIT 100 to admin volumes query Performance fixes: - Eliminate duplicate collana query: fetch seriesBooks once, pass to getRelatedBooks() which reuses them for Priority 0 - Replace correlated subquery (ORDER BY CASE) with direct LEFT JOIN on ruolo='principale' in series and volumes queries Security fix: - Add server-side ISSN validation (format XXXX-XXXX) in both store() and update(), stripping spaces and discarding invalid formats Also add error logging on series query prepare failure.
ISSN was in LIBRARYTHING_FIELDS_DEFAULTS, so user-submitted ISSN data was silently discarded when LibraryThing plugin was not installed, even though the form field was visible. Move issn to the base $fields array in both store() and update() so it works independently.
Test 11 navigates to a book detail page on the frontend and verifies: - "Nella stessa collana" section renders with the series name - Sibling books (Book 2 and 3) appear as clickable links - At least 2 series links are present as <a> tags Also fix test seeding: add author to series books so book_path() generates valid URLs (without author, slug becomes 'autore' and the route doesn't match).
Owner
Author
|
@coderabbitai full review |
|
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:
📝 WalkthroughWalkthroughAdds series (collane) and multi-volume (volumi) support: schema + migration, admin UI and API for collane, multi-volume management in Libri, ISSN normalization/validation, frontend series display and related-book prioritization, installer updates, autoload entries, routes, and extensive E2E tests; bumps version to 0.5.1. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin UI
participant CollaneCtrl as CollaneController
participant LibriCtrl as LibriController
participant DB as Database
Admin->>CollaneCtrl: POST /admin/collane/crea {nome}
CollaneCtrl->>DB: INSERT IGNORE collane
DB-->>CollaneCtrl: OK
CollaneCtrl-->>Admin: Redirect to collana detail
Admin->>CollaneCtrl: POST /admin/collane/bulk-assign {book_ids, collana}
CollaneCtrl->>DB: UPDATE libri SET collana=?
DB-->>CollaneCtrl: affected rows
CollaneCtrl-->>Admin: JSON {success:true}
Admin->>LibriCtrl: POST /admin/libri/volumi/add {opera_id, volume_id}
LibriCtrl->>DB: INSERT INTO volumi ... ON DUPLICATE KEY UPDATE
DB-->>LibriCtrl: OK
LibriCtrl-->>Admin: JSON {success:true}
sequenceDiagram
participant User as Frontend User
participant FrontendCtrl as FrontendController
participant DB as Database
participant View as Book Detail View
User->>FrontendCtrl: GET /book/{id}
FrontendCtrl->>DB: SELECT book details (includes collana)
DB-->>FrontendCtrl: book record
alt book.collana present
FrontendCtrl->>DB: SELECT books WHERE collana=? (seriesBooks)
DB-->>FrontendCtrl: seriesBooks
FrontendCtrl->>FrontendCtrl: getRelatedBooks(..., seriesBooks) (Priority 0 uses seriesBooks)
else
FrontendCtrl->>FrontendCtrl: getRelatedBooks(...) (author/genre/recent priorities)
end
FrontendCtrl-->>View: Render page with seriesBooks + relatedBooks
View-->>User: HTML
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
version.json (1)
3-3: Consider a minor version bump for new features.The version was incremented from 0.5.0 to 0.5.1 (patch bump). According to semantic versioning conventions, patch versions are typically reserved for backwards-compatible bug fixes, while minor versions indicate new backwards-compatible functionality.
This PR introduces new features (ISSN field, series links, multi-volume support) and schema changes (new
volumitable,idx_collanaindex), which would typically warrant a minor version bump (0.5.0 → 0.6.0) rather than a patch bump.If the project follows its own versioning convention or intentionally uses patch increments for feature releases, this is fine. Otherwise, consider bumping to 0.6.0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@version.json` at line 3, The package version was bumped only to "0.5.1" (patch) but this PR adds new features and schema changes; update the "version" field from "0.5.1" to "0.6.0" in the version JSON (the "version" key/value) to reflect a minor version bump, and ensure any accompanying changelog/release notes or references to the old version are updated accordingly (search for the "version" key or "0.5.1" literal to locate all occurrences).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/FrontendController.php`:
- Around line 678-679: Replace the raw error_log call in FrontendController (the
"series query prepare failed" branch) with a SecureLogger error call;
specifically, call SecureLogger::error with a short descriptive message
("FrontendController: series query prepare failed") and pass the raw DB error in
metadata (e.g. ['db_error' => $db->error]) so SecureLogger can sanitize/handle
it instead of using error_log().
In `@app/Controllers/LibriController.php`:
- Around line 741-748: The ISSN sanitizer currently discards malformed values
and accepts unhyphenated input; instead, change the logic in the LibriController
code that handles $fields['issn'] so that invalid ISSNs are rejected (do not
silently set to null or erase an existing DB value) and valid ISSNs are
normalized to the canonical hyphenated format (XXXX-XXXX, final character may be
digit or X). Specifically, replace the current preg_replace/preg_match branch
around $fields['issn'] so it: 1) trims spaces, 2) validates strictly against the
ISSN pattern allowing an optional existing hyphen but failing on any other
format, 3) if valid, reformats to the hyphenated form before assigning back to
$fields['issn'], and 4) if invalid, returns/records a validation error (or
throws the controller’s standard validation exception) instead of setting the
field to null; update the same logic in the other occurrence referenced (the
block that mirrors this behavior).
In `@app/Views/libri/partials/book_form.php`:
- Around line 208-212: The current ISSN input (id="issn", name="issn") allows
the hyphen to be optional; change the HTML pattern to require the hyphen (strict
"XXXX-XXXX") and ensure the placeholder stays "1234-5678"; also update the
client-side ISSN validation regex used elsewhere in this file (the JS ISSN
validation) to require the hyphen as well so both HTML and JS enforce the same
strict format.
In `@app/Views/libri/scheda_libro.php`:
- Line 982: Replace the hardcoded url('/admin/libri/' . (int)$parentWork['id'])
occurrences with the route helper (e.g., route_path('admin.libri.show', ['id' =>
(int)$parentWork['id']]) or RouteTranslator::route('admin.libri.show', ['id' =>
(int)$parentWork['id']])) in scheda_libro.php; update both locations where
url('/admin/libri/' . (int)$parentWork['id']) appears (near usage with
$parentWork['id'] at the two reported spots) so the link generation uses the
named route key instead of a hardcoded path. Ensure the parameter name matches
the route definition (e.g., 'id' or 'libro') and keep htmlspecialchars wrapping
as before.
In `@installer/database/migrations/migrate_0.5.1.sql`:
- Around line 24-28: The migration's composite unique key uk_opera_volume
(opera_id, volume_id) still allows the same volume_id to be associated with
multiple opera_id values; change the schema so volume_id is unique (e.g.,
replace or add a UNIQUE constraint on volume_id) so a volume can only have one
parent; update the constraint definition near uk_opera_volume and ensure foreign
keys fk_volumi_opera and fk_volumi_volume remain unchanged; this aligns the DB
with LibriController::show() which expects a single parent row for v.volume_id.
In `@installer/database/schema.sql`:
- Around line 891-896: The current volumi table allows a volume_id to be linked
to multiple opera_id values and permits self-linking; update the schema so each
volume has exactly one parent and cannot be its own parent by replacing the
composite UNIQUE KEY `uk_opera_volume` with a uniqueness constraint on
`volume_id` alone (ensure `volume_id` is UNIQUE) and add a CHECK constraint that
`opera_id <> volume_id`; keep the existing FOREIGN KEY constraints
(`fk_volumi_opera`, `fk_volumi_volume`) in place to preserve referential
integrity.
In `@locale/de_DE.json`:
- Line 3461: Update the German translation string for the ISSN validation
message by replacing the incorrect Italian "l'ultima" with correct German "die
letzte" in the value for the key "ISSN deve essere nel formato XXXX-XXXX (8
cifre, l'ultima può essere X).": change the translated sentence to "ISSN muss im
Format XXXX-XXXX sein (8 Ziffern, die letzte kann X sein)." so grammar is
correct; locate the exact JSON entry matching that key and modify only the
value.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 141-157: Test "3. ISSN displayed on frontend book detail page"
currently skips loading the real public detail page and instead queries the DB;
change the test to derive the public book URL (use the admin response for path
or the sitemap lookup instead of constructing BASE/), then perform request.get
against that public detail URL (use the same request object and variables like
seriesBookIds and BASE) and assert the response HTML contains ISSN_VALUE (not
just check dbQuery). Remove or fail the silent DB-only fallback: if the public
page URL resolution or fetch fails the test should fail rather than falling back
to dbQuery; update similar patterns in the other failing tests (lines referenced
in comment) to resolve the real href and assert on rendered HTML.
- Around line 48-88: The E2E suite uses shared DB setup in test.beforeAll and
relies on test.describe.serial but the repo's test invocation (package.json test
script and playwright.config.js) doesn't enforce single-worker execution or load
/tmp/run-e2e.sh with env creds as required by CLAUDE.md; fix by ensuring CI/dev
runs use the wrapper script with --workers=1 (create /tmp/run-e2e.sh if not
provided) and update the npm test script to invoke that script (so Playwright is
launched as: /tmp/run-e2e.sh --workers=1 ...), and confirm playwright.config.js
doesn't override workers; refer to test.describe.serial, test.beforeAll,
package.json test script and /tmp/run-e2e.sh when making the change.
---
Nitpick comments:
In `@version.json`:
- Line 3: The package version was bumped only to "0.5.1" (patch) but this PR
adds new features and schema changes; update the "version" field from "0.5.1" to
"0.6.0" in the version JSON (the "version" key/value) to reflect a minor version
bump, and ensure any accompanying changelog/release notes or references to the
old version are updated accordingly (search for the "version" key or "0.5.1"
literal to locate all occurrences).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af7c23e5-bafc-4af0-86ac-d86025d82eff
📒 Files selected for processing (11)
app/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Views/frontend/book-detail.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/issue-75-issn-series-volumes.spec.jsversion.json
app/Views/libri/scheda_libro.php
Outdated
| <i class="fas fa-layer-group text-indigo-500 mr-1"></i> | ||
| <?= __("Questo libro è il volume") ?> <strong><?= (int) $parentWork['numero_volume'] ?></strong> | ||
| <?= __("dell'opera") ?> | ||
| <a href="<?= htmlspecialchars(url('/admin/libri/' . (int)$parentWork['id']), ENT_QUOTES, 'UTF-8') ?>" class="text-indigo-600 hover:underline font-semibold"> |
There was a problem hiding this comment.
Use route helpers instead of hardcoded admin paths.
Line 982 and Line 1017 still hardcode /admin/libri/.... Please switch these to route_path('...') (or RouteTranslator::route('...')) for consistency and route safety.
As per coding guidelines: "Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login."
Also applies to: 1017-1017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/libri/scheda_libro.php` at line 982, Replace the hardcoded
url('/admin/libri/' . (int)$parentWork['id']) occurrences with the route helper
(e.g., route_path('admin.libri.show', ['id' => (int)$parentWork['id']]) or
RouteTranslator::route('admin.libri.show', ['id' => (int)$parentWork['id']])) in
scheda_libro.php; update both locations where url('/admin/libri/' .
(int)$parentWork['id']) appears (near usage with $parentWork['id'] at the two
reported spots) so the link generation uses the named route key instead of a
hardcoded path. Ensure the parameter name matches the route definition (e.g.,
'id' or 'libro') and keep htmlspecialchars wrapping as before.
Comment on lines
+48
to
+88
| test.describe.serial('Issue #75: ISSN, Series & Multi-Volume', () => { | ||
| /** @type {import('@playwright/test').BrowserContext} */ | ||
| let context; | ||
| /** @type {import('@playwright/test').Page} */ | ||
| let page; | ||
|
|
||
| let seriesBookIds = []; | ||
| let parentWorkId = 0; | ||
| let volumeIds = []; | ||
|
|
||
| test.beforeAll(async ({ browser }) => { | ||
| context = await browser.newContext(); | ||
| page = await context.newPage(); | ||
| await loginAsAdmin(page); | ||
|
|
||
| // Create a test author for the series books | ||
| dbExec(`INSERT IGNORE INTO autori (id, nome) VALUES (9991, 'Series TestAuthor')`); | ||
|
|
||
| // Create 3 books in the same series (collana) | ||
| for (let i = 1; i <= 3; i++) { | ||
| dbExec(`INSERT INTO libri (titolo, collana, numero_serie, issn, copie_totali, created_at, updated_at) | ||
| VALUES ('E2E Series Book ${i}', '${SERIES_NAME}', '${i}', ${i === 1 ? `'${ISSN_VALUE}'` : 'NULL'}, 1, NOW(), NOW())`); | ||
| const id = dbQuery(`SELECT MAX(id) FROM libri WHERE titolo='E2E Series Book ${i}' AND deleted_at IS NULL`); | ||
| seriesBookIds.push(parseInt(id)); | ||
| dbExec(`INSERT IGNORE INTO libri_autori (libro_id, autore_id, ruolo) VALUES (${id}, 9991, 'principale')`); | ||
| } | ||
|
|
||
| // Create parent work + 2 volumes for multi-volume test | ||
| dbExec(`INSERT INTO libri (titolo, copie_totali, created_at, updated_at) | ||
| VALUES ('E2E Complete Works', 1, NOW(), NOW())`); | ||
| parentWorkId = parseInt(dbQuery(`SELECT MAX(id) FROM libri WHERE titolo='E2E Complete Works' AND deleted_at IS NULL`)); | ||
|
|
||
| for (let i = 1; i <= 2; i++) { | ||
| dbExec(`INSERT INTO libri (titolo, copie_totali, created_at, updated_at) | ||
| VALUES ('E2E Volume ${i}: Part ${i}', 1, NOW(), NOW())`); | ||
| const vid = parseInt(dbQuery(`SELECT MAX(id) FROM libri WHERE titolo='E2E Volume ${i}: Part ${i}' AND deleted_at IS NULL`)); | ||
| volumeIds.push(vid); | ||
| dbExec(`INSERT INTO volumi (opera_id, volume_id, numero_volume, titolo_volume) | ||
| VALUES (${parentWorkId}, ${vid}, ${i}, 'Part ${i}')`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -S \
-g 'package.json' \
-g '.github/workflows/*.yml' \
-g '.github/workflows/*.yaml' \
-g 'playwright.config.*' \
-g 'README*' \
-g 'scripts/**' \
'playwright test|/tmp/run-e2e\.sh|--workers(?:=| )1'Repository: fabiodalez-dev/Pinakes
Length of output: 147
🏁 Script executed:
cat tests/playwright.config.jsRepository: fabiodalez-dev/Pinakes
Length of output: 365
🏁 Script executed:
find .github/workflows -type f \( -name "*.yml" -o -name "*.yaml" \) -exec cat {} \;Repository: fabiodalez-dev/Pinakes
Length of output: 10925
🏁 Script executed:
rg -n '/tmp/run-e2e' --type-list | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 455
🏁 Script executed:
rg 'run-e2e' --type shell --type bashRepository: fabiodalez-dev/Pinakes
Length of output: 95
🏁 Script executed:
ls -la .github/workflows/Repository: fabiodalez-dev/Pinakes
Length of output: 307
🏁 Script executed:
find . -name '*run-e2e*' -o -name '*e2e*' 2>/dev/null | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
find ./scripts -type f 2>/dev/null | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 272
🏁 Script executed:
rg '/tmp/run-e2e' --type txt --type md --type js --type jsonRepository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
find . -name 'README*' -type f | xargs cat 2>/dev/null | head -100Repository: fabiodalez-dev/Pinakes
Length of output: 6127
🏁 Script executed:
find . -path './tests' -name '*.spec.js' | head -5 | xargs ls -lRepository: fabiodalez-dev/Pinakes
Length of output: 1639
🏁 Script executed:
cat CLAUDE.mdRepository: fabiodalez-dev/Pinakes
Length of output: 8116
🏁 Script executed:
head -100 tests/issue-75-issn-series-volumes.spec.jsRepository: fabiodalez-dev/Pinakes
Length of output: 4479
Current E2E test setup does not comply with coding guidelines — must use /tmp/run-e2e.sh with --workers=1.
The repository's package.json test command (playwright test --config=tests/playwright.config.js) and playwright.config.js do not invoke /tmp/run-e2e.sh or specify --workers=1. According to CLAUDE.md, all E2E tests must use this pattern to set DB/admin credentials as env vars and serialize execution.
This test file uses test.describe.serial() only at the suite level and creates a shared DB fixture in test.beforeAll(). Parallel Playwright workers can still race on these fixed records unless run in serial mode with proper credential setup.
Create /tmp/run-e2e.sh (or document how it's externally provided) and update the npm test script to use it with --workers=1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/issue-75-issn-series-volumes.spec.js` around lines 48 - 88, The E2E
suite uses shared DB setup in test.beforeAll and relies on test.describe.serial
but the repo's test invocation (package.json test script and
playwright.config.js) doesn't enforce single-worker execution or load
/tmp/run-e2e.sh with env creds as required by CLAUDE.md; fix by ensuring CI/dev
runs use the wrapper script with --workers=1 (create /tmp/run-e2e.sh if not
provided) and update the npm test script to invoke that script (so Playwright is
launched as: /tmp/run-e2e.sh --workers=1 ...), and confirm playwright.config.js
doesn't override workers; refer to test.describe.serial, test.beforeAll,
package.json test script and /tmp/run-e2e.sh when making the change.
- Make volume_id UNIQUE in volumi table (prevents a volume belonging to multiple parent works — aligns with LIMIT 1 in controller) - Add CHECK constraint opera_id <> volume_id (prevents self-linking) - Replace error_log with SecureLogger::warning for series query failure - Fix German grammar: "die letzte" instead of "letzte" - Apply same constraints to both schema.sql and migrate_0.5.1.sql
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
locale/de_DE.json (1)
3461-3461:⚠️ Potential issue | 🟡 MinorFix German grammar in ISSN validation message (still unresolved).
Line 3461 should use “die letzte” for correct German grammar.
✏️ Suggested fix
- "ISSN deve essere nel formato XXXX-XXXX (8 cifre, l'ultima può essere X).": "ISSN muss im Format XXXX-XXXX sein (8 Ziffern, letzte kann X sein).", + "ISSN deve essere nel formato XXXX-XXXX (8 cifre, l'ultima può essere X).": "ISSN muss im Format XXXX-XXXX sein (8 Ziffern, die letzte kann X sein).",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/de_DE.json` at line 3461, Update the German translation value for the ISSN validation message: change "letzte kann X sein" to "die letzte kann X sein" in the JSON entry whose English/Italian key is "ISSN deve essere nel formato XXXX-XXXX (8 cifre, l'ultima può essere X)." so the German value reads "ISSN muss im Format XXXX-XXXX sein (8 Ziffern, die letzte kann X sein)." Ensure you only modify the right-hand string value and keep the surrounding punctuation and JSON formatting intact.installer/database/migrations/migrate_0.5.1.sql (1)
24-28:⚠️ Potential issue | 🟠 MajorAlign migration constraints with single-parent volume model.
Line 24 only enforces unique
(opera_id, volume_id)pairs, so avolume_idcan still be attached to multiple parent works.🔧 Suggested fix
- UNIQUE KEY uk_opera_volume (opera_id, volume_id), + UNIQUE KEY uk_volume_id (volume_id), KEY idx_opera (opera_id), - KEY idx_volume (volume_id),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.5.1.sql` around lines 24 - 28, The UNIQUE constraint currently named uk_opera_volume on (opera_id, volume_id) still allows the same volume_id to be linked to multiple opera_id; update the migration for the volumi table to enforce that each volume_id appears only once by adding or replacing with a unique key on volume_id (e.g., add UNIQUE KEY uk_volume (volume_id)), keeping the foreign key CONSTRAINT fk_volumi_volume and CONSTRAINT fk_volumi_opera intact so the FK behavior remains unchanged.installer/database/schema.sql (1)
891-896:⚠️ Potential issue | 🟠 MajorEnforce one parent per volume at schema level.
Line 891 uses
(opera_id, volume_id)uniqueness, which still allows the samevolume_idto be linked to multiple parent works.🔧 Suggested fix
- UNIQUE KEY `uk_opera_volume` (`opera_id`,`volume_id`), + UNIQUE KEY `uk_volume_id` (`volume_id`), KEY `idx_opera` (`opera_id`), - KEY `idx_volume` (`volume_id`),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` around lines 891 - 896, The current UNIQUE KEY `uk_opera_volume` (`opera_id`,`volume_id`) allows a single `volume_id` to appear with multiple `opera_id`s; modify the schema to enforce one parent per volume by replacing or adding a unique constraint on `volume_id` (e.g., drop/alter `uk_opera_volume` and create UNIQUE KEY `uk_volume` (`volume_id`)) while preserving the existing foreign keys `fk_volumi_opera` and `fk_volumi_volume` and the table definition so that each volume can only be linked to a single parent work.app/Views/libri/partials/book_form.php (1)
208-212:⚠️ Potential issue | 🟠 MajorRequire strict ISSN hyphen format in both validators.
Line 210 and Line 2896 still allow missing hyphen (
-?), so12345678is accepted even though the required format isXXXX-XXXX.🔧 Suggested fix
- <input id="issn" name="issn" type="text" class="form-input" placeholder="<?= __('es. 1234-5678') ?>" value="<?php echo HtmlHelper::e($book['issn'] ?? ''); ?>" pattern="\d{4}-?\d{3}[\dXx]" /> + <input id="issn" name="issn" type="text" class="form-input" placeholder="<?= __('es. 1234-5678') ?>" value="<?php echo HtmlHelper::e($book['issn'] ?? ''); ?>" pattern="\d{4}-\d{3}[\dXx]" />- const issn = form.querySelector('input[name="issn"]').value.replace(/\s/g, ''); - if (issn && !/^\d{4}-?\d{3}[\dXx]$/.test(issn)) { + const issn = form.querySelector('input[name="issn"]').value.replace(/\s/g, '').toUpperCase(); + if (issn && !/^\d{4}-\d{3}[\dX]$/.test(issn)) {Also applies to: 2895-2903
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/partials/book_form.php` around lines 208 - 212, The ISSN pattern currently allows an optional hyphen (e.g., pattern="\d{4}-?\d{3}[\dXx]"), so strings like "12345678" are accepted; update the client-side input with id "issn" to require the hyphen (use "\d{4}-\d{3}[\dXx]") and make the matching change in the corresponding server-side validator (the ISSN validation entry in the validators/validation array or function used for this form) so both client and server enforce the strict "XXXX-XXXX" format.app/Controllers/FrontendController.php (1)
679-679:⚠️ Potential issue | 🟠 MajorUse
SecureLogger::error()for this prepare failure path.At Line 679, logging raw
$db->errorthrougherror_log()can leak SQL/schema details and bypass centralized sanitization.🔧 Proposed fix
- error_log('FrontendController: series query prepare failed: ' . $db->error); + \App\Support\SecureLogger::error('FrontendController: series query prepare failed', [ + 'book_id' => $book_id, + 'db_error' => $db->error, + ]);As per coding guidelines "Use
SecureLogger::error()noterror_log()for sensitive contexts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` at line 679, Replace the direct call to error_log in FrontendController's series query prepare failure path with a call to SecureLogger::error() so the message goes through centralized sanitization; use the same context string ("FrontendController: series query prepare failed") and include the $db->error details passed as data to SecureLogger::error (not concatenated into a raw string) so sensitive SQL/schema info is handled by the logger.app/Controllers/LibriController.php (1)
741-748:⚠️ Potential issue | 🟠 MajorReject invalid ISSN instead of silently nulling it.
At Line 745 (and mirrored at Line 1236), invalid ISSN input is converted to empty and then persisted as
NULL(Line 747 / Line 1238). This can unintentionally erase an existing ISSN on update, and it still accepts non-hyphenated input despite the requiredXXXX-XXXXformat.🔧 Proposed fix
- // Sanitize ISSN: strip spaces, validate format (XXXX-XXXX) + // Normalize + validate ISSN (canonical format: XXXX-XXXX) if (isset($fields['issn'])) { - $issn = preg_replace('/\s+/', '', (string) $fields['issn']); - if ($issn !== '' && !preg_match('/^\d{4}-?\d{3}[\dXx]$/', $issn)) { - $issn = ''; // Invalid format — discard - } - $fields['issn'] = $issn !== '' ? $issn : null; + $issnRaw = trim((string) $fields['issn']); + if ($issnRaw === '') { + $fields['issn'] = null; + } else { + $issnCompact = strtoupper(str_replace('-', '', preg_replace('/\s+/', '', $issnRaw))); + if (!preg_match('/^\d{7}[\dX]$/', $issnCompact)) { + // return/throw validation error instead of silently clearing value + throw new \InvalidArgumentException(__('ISSN non valido. Usa il formato XXXX-XXXX.')); + } + $fields['issn'] = substr($issnCompact, 0, 4) . '-' . substr($issnCompact, 4, 4); + } }Also applies to: 1232-1239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 748, The ISSN sanitization currently turns invalid values into null and overwrites existing data; change it to reject invalid ISSNs instead: in LibriController where $fields['issn'] is processed, strip whitespace but validate against a strict hyphenated pattern /^\d{4}-\d{3}[\dXx]$/ (no optional hyphen) and if the value fails validation return/throw a validation error (do not set $fields['issn'] = null or overwrite an existing ISSN on update); only assign $fields['issn'] when the input passes the hyphenated regex so invalid inputs are rejected rather than persisted as NULL.
🧹 Nitpick comments (3)
tests/issue-75-issn-series-volumes.spec.js (3)
90-102: Cleanup logic should handle partial failures more gracefully.The
test.afterAllcleanup usesSET FOREIGN_KEY_CHECKS=0which is appropriate, but if any cleanup statement fails, subsequent cleanup may not run. Consider wrapping in try-catch or using a single multi-statement cleanup.💡 Suggested improvement for more robust cleanup
test.afterAll(async () => { // Clean up test data const allIds = [...seriesBookIds, parentWorkId, ...volumeIds].filter(id => id > 0); if (allIds.length > 0) { - dbExec(`SET FOREIGN_KEY_CHECKS=0`); - dbExec(`DELETE FROM libri_autori WHERE libro_id IN (${allIds.join(',')})`); - dbExec(`DELETE FROM volumi WHERE opera_id IN (${allIds.join(',')}) OR volume_id IN (${allIds.join(',')})`); - dbExec(`DELETE FROM libri WHERE id IN (${allIds.join(',')})`); - dbExec(`DELETE FROM autori WHERE id=9991`); - dbExec(`SET FOREIGN_KEY_CHECKS=1`); + try { + dbExec(`SET FOREIGN_KEY_CHECKS=0`); + dbExec(`DELETE FROM libri_autori WHERE libro_id IN (${allIds.join(',')})`); + dbExec(`DELETE FROM volumi WHERE opera_id IN (${allIds.join(',')}) OR volume_id IN (${allIds.join(',')})`); + dbExec(`DELETE FROM libri WHERE id IN (${allIds.join(',')})`); + dbExec(`DELETE FROM autori WHERE id=9991`); + } finally { + dbExec(`SET FOREIGN_KEY_CHECKS=1`); + } } await context?.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 90 - 102, The cleanup in test.afterAll (using seriesBookIds, parentWorkId, volumeIds and dbExec) can short-circuit if a statement fails and leave FOREIGN_KEY_CHECKS disabled; wrap the sequence of dbExec calls in a try/catch/finally: perform all delete statements in the try (prefer awaiting dbExec if it returns a promise), catch and log any error, and in finally always reset FOREIGN_KEY_CHECKS=1 and await context?.close() so cleanup always resets DB state and closes context even on partial failures.
19-29: SQL values in test utilities are not parameterized, but risk is contained to test environment.The
dbQueryanddbExecfunctions execute raw SQL strings without parameterization. While this would be a critical issue in production code, here the risk is mitigated because:
- This is test code running against a test database
- All input values are test constants defined in the file (e.g.,
SERIES_NAME,ISSN_VALUE)However, ensure these utilities are never used with user-controlled input in future test expansions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 19 - 29, dbQuery and dbExec currently execute raw SQL strings which could be misused if later fed user-controlled input; update these test utilities to either (a) enforce a safeguard that rejects SQL containing suspicious characters/patterns (e.g. semicolons, comment markers, or concatenation tokens) or (b) replace them with a small parameterized helper that uses the mysql client library bind/escape functions so callers pass values separately; locate and change the dbQuery and dbExec functions to implement the chosen safeguard and add a brief comment warning maintainers to never pass untrusted input into these helpers.
314-318: Consider usingEXPLAIN FORMAT=JSONor adding index hints for more robust verification.The test expects
EXPLAINoutput to containidx_collana, but MySQL's EXPLAIN format and index selection vary across versions and depend on table statistics. With only 3 rows of test data, MySQL's query optimizer may choose a full table scan instead of the index, causing the assertion to fail unpredictably.Either use
EXPLAIN FORMAT=JSONwith a programmatic JSON check (more stable across versions), addUSE INDEX (idx_collana)to force index usage, or insert sufficient test data to make the index selection cost-effective.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 314 - 318, The test currently asserts the plain EXPLAIN output contains "idx_collana", which is flaky; change the check to use EXPLAIN FORMAT=JSON (via dbQuery) and parse the returned JSON to assert the optimizer used idx_collana (check fields like "key" / "used_key" in the JSON), or alternatively modify the SELECT to include a USE INDEX (idx_collana) hint to force index usage for the query that references SERIES_NAME on the libri table; update the assertion that currently checks explain to look at the JSON field (or verify the hint is present) instead of string-matching the plain EXPLAIN output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/FrontendController.php`:
- Around line 1753-1757: The loop that appends items from $seriesBooks to
$related_books pushes raw rows that lack the view-expected autori field; update
the loop (the array_slice($seriesBooks, 0, $limit) foreach) to transform each
$sb by setting $sb['autori'] = $sb['autore_principale'] (or formatting it if
needed) before pushing into $related_books so same-series cards render the
correct author(s) instead of fallback text.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Line 679: Replace the direct call to error_log in FrontendController's series
query prepare failure path with a call to SecureLogger::error() so the message
goes through centralized sanitization; use the same context string
("FrontendController: series query prepare failed") and include the $db->error
details passed as data to SecureLogger::error (not concatenated into a raw
string) so sensitive SQL/schema info is handled by the logger.
In `@app/Controllers/LibriController.php`:
- Around line 741-748: The ISSN sanitization currently turns invalid values into
null and overwrites existing data; change it to reject invalid ISSNs instead: in
LibriController where $fields['issn'] is processed, strip whitespace but
validate against a strict hyphenated pattern /^\d{4}-\d{3}[\dXx]$/ (no optional
hyphen) and if the value fails validation return/throw a validation error (do
not set $fields['issn'] = null or overwrite an existing ISSN on update); only
assign $fields['issn'] when the input passes the hyphenated regex so invalid
inputs are rejected rather than persisted as NULL.
In `@app/Views/libri/partials/book_form.php`:
- Around line 208-212: The ISSN pattern currently allows an optional hyphen
(e.g., pattern="\d{4}-?\d{3}[\dXx]"), so strings like "12345678" are accepted;
update the client-side input with id "issn" to require the hyphen (use
"\d{4}-\d{3}[\dXx]") and make the matching change in the corresponding
server-side validator (the ISSN validation entry in the validators/validation
array or function used for this form) so both client and server enforce the
strict "XXXX-XXXX" format.
In `@installer/database/migrations/migrate_0.5.1.sql`:
- Around line 24-28: The UNIQUE constraint currently named uk_opera_volume on
(opera_id, volume_id) still allows the same volume_id to be linked to multiple
opera_id; update the migration for the volumi table to enforce that each
volume_id appears only once by adding or replacing with a unique key on
volume_id (e.g., add UNIQUE KEY uk_volume (volume_id)), keeping the foreign key
CONSTRAINT fk_volumi_volume and CONSTRAINT fk_volumi_opera intact so the FK
behavior remains unchanged.
In `@installer/database/schema.sql`:
- Around line 891-896: The current UNIQUE KEY `uk_opera_volume`
(`opera_id`,`volume_id`) allows a single `volume_id` to appear with multiple
`opera_id`s; modify the schema to enforce one parent per volume by replacing or
adding a unique constraint on `volume_id` (e.g., drop/alter `uk_opera_volume`
and create UNIQUE KEY `uk_volume` (`volume_id`)) while preserving the existing
foreign keys `fk_volumi_opera` and `fk_volumi_volume` and the table definition
so that each volume can only be linked to a single parent work.
In `@locale/de_DE.json`:
- Line 3461: Update the German translation value for the ISSN validation
message: change "letzte kann X sein" to "die letzte kann X sein" in the JSON
entry whose English/Italian key is "ISSN deve essere nel formato XXXX-XXXX (8
cifre, l'ultima può essere X)." so the German value reads "ISSN muss im Format
XXXX-XXXX sein (8 Ziffern, die letzte kann X sein)." Ensure you only modify the
right-hand string value and keep the surrounding punctuation and JSON formatting
intact.
---
Nitpick comments:
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 90-102: The cleanup in test.afterAll (using seriesBookIds,
parentWorkId, volumeIds and dbExec) can short-circuit if a statement fails and
leave FOREIGN_KEY_CHECKS disabled; wrap the sequence of dbExec calls in a
try/catch/finally: perform all delete statements in the try (prefer awaiting
dbExec if it returns a promise), catch and log any error, and in finally always
reset FOREIGN_KEY_CHECKS=1 and await context?.close() so cleanup always resets
DB state and closes context even on partial failures.
- Around line 19-29: dbQuery and dbExec currently execute raw SQL strings which
could be misused if later fed user-controlled input; update these test utilities
to either (a) enforce a safeguard that rejects SQL containing suspicious
characters/patterns (e.g. semicolons, comment markers, or concatenation tokens)
or (b) replace them with a small parameterized helper that uses the mysql client
library bind/escape functions so callers pass values separately; locate and
change the dbQuery and dbExec functions to implement the chosen safeguard and
add a brief comment warning maintainers to never pass untrusted input into these
helpers.
- Around line 314-318: The test currently asserts the plain EXPLAIN output
contains "idx_collana", which is flaky; change the check to use EXPLAIN
FORMAT=JSON (via dbQuery) and parse the returned JSON to assert the optimizer
used idx_collana (check fields like "key" / "used_key" in the JSON), or
alternatively modify the SELECT to include a USE INDEX (idx_collana) hint to
force index usage for the query that references SERIES_NAME on the libri table;
update the assertion that currently checks explain to look at the JSON field (or
verify the hint is present) instead of string-matching the plain EXPLAIN output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ac0ba14-53ca-4ae0-bdb7-5528a1b9ca9e
📒 Files selected for processing (11)
app/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Views/frontend/book-detail.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/issue-75-issn-series-volumes.spec.jsversion.json
- Require strict ISSN hyphen format (XXXX-XXXX) in HTML pattern and JS - Normalize ISSN server-side to canonical format (strip spaces/hyphens, validate 8 digits, reformat as XXXX-XXXX); invalid values no longer overwrite existing data (unset instead of null) - Map autore_principale to autori field when pushing seriesBooks into related books, so same-series cards render correct author names - Update test 4 to expect hyphen-required pattern
The update path was still using the old validation logic (accepted unhyphenated input, stored lowercase x). Now both create and update use the same normalization: strip whitespace/hyphens, validate 8 digits, reformat to canonical XXXX-XXXX, uppercase X.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/Controllers/LibriController.php (1)
1238-1245:⚠️ Potential issue | 🟠 MajorISSN handling in
update()is inconsistent withstore()and may silently erase valid data.Two issues:
- No normalization:
store()normalizes toXXXX-XXXXformat, butupdate()does not, leading to inconsistent data.- Silent erasure: Invalid input sets
$fields['issn'] = null, which will overwrite an existing valid ISSN. In contrast,store()usesunset()to preserve existing values.🔧 Proposed fix to align with store() logic
// Sanitize ISSN on update as well if (isset($fields['issn'])) { - $issn = preg_replace('/\s+/', '', (string) $fields['issn']); - if ($issn !== '' && !preg_match('/^\d{4}-?\d{3}[\dXx]$/', $issn)) { - $issn = ''; + $issnRaw = trim((string) $fields['issn']); + if ($issnRaw === '') { + $fields['issn'] = null; + } else { + $issnCompact = strtoupper(str_replace('-', '', preg_replace('/\s+/', '', $issnRaw))); + if (preg_match('/^\d{7}[\dX]$/', $issnCompact)) { + $fields['issn'] = substr($issnCompact, 0, 4) . '-' . substr($issnCompact, 4, 4); + } else { + unset($fields['issn']); // Invalid — don't overwrite existing value + } } - $fields['issn'] = $issn !== '' ? $issn : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 1238 - 1245, The update() ISSN handling currently strips spaces and may set $fields['issn'] = null on invalid input, which differs from store() and can erase a valid existing ISSN; change update() to mirror store(): trim and normalize the ISSN into the canonical "XXXX-XXXX" form (same normalization logic used in store()), validate against the same regex, and if the input is invalid or empty use unset($fields['issn']) instead of assigning null so the existing database value is preserved; locate the ISSN block in update() (the if (isset($fields['issn'])) {...} section) and apply the same normalization/validation/unset behavior as store().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/FrontendController.php`:
- Around line 661-668: The query preparing $stmtSeries currently limits the
series to 20 (the SQL LIMIT 20) which truncates $seriesBooks used by
getRelatedBooks() and the “Nella stessa collana” pills; remove the LIMIT 20 from
the $stmtSeries SQL (and the duplicate truncated query at the other occurrence
noted around 683-684) so the full sibling list is fetched, then enforce any UI
cap by slicing inside getRelatedBooks() (or implement an explicit “show more”
affordance) rather than truncating at the DB query.
---
Duplicate comments:
In `@app/Controllers/LibriController.php`:
- Around line 1238-1245: The update() ISSN handling currently strips spaces and
may set $fields['issn'] = null on invalid input, which differs from store() and
can erase a valid existing ISSN; change update() to mirror store(): trim and
normalize the ISSN into the canonical "XXXX-XXXX" form (same normalization logic
used in store()), validate against the same regex, and if the input is invalid
or empty use unset($fields['issn']) instead of assigning null so the existing
database value is preserved; locate the ISSN block in update() (the if
(isset($fields['issn'])) {...} section) and apply the same
normalization/validation/unset behavior as store().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8f01b6d-495f-43fc-b4b7-c819e526f88b
📒 Files selected for processing (7)
app/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Views/libri/partials/book_form.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsontests/issue-75-issn-series-volumes.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/issue-75-issn-series-volumes.spec.js
- app/Views/libri/partials/book_form.php
- Add if($stmt) guard + $stmt->close() to Priority 2 (genre) and Priority 3 (recent) in getRelatedBooks to prevent crash on prepare failure (same pattern as Priority 1 fix) - Add @var annotations for $seriesBooks and $collana in book-detail.php - Remove redundant ?? fallback on $collana (always defined by controller) - PHPStan level 5: 0 errors
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
app/Controllers/LibriController.php (1)
741-754:⚠️ Potential issue | 🟠 MajorReject invalid ISSN input instead of silently dropping it.
When ISSN is malformed, both paths
unset($fields['issn'])and continue. This makes invalid input succeed without user feedback, so server-side validation is not actually enforced for provided ISSN values.💡 Suggested fix
--- a/app/Controllers/LibriController.php +++ b/app/Controllers/LibriController.php @@ - } else { - unset($fields['issn']); // Invalid — don't overwrite existing value - } + } else { + $response->getBody()->write(json_encode([ + 'error' => true, + 'message' => __('ISSN non valido. Usa il formato XXXX-XXXX.'), + ], JSON_UNESCAPED_UNICODE)); + return $response->withStatus(422)->withHeader('Content-Type', 'application/json'); + }--- a/app/Controllers/LibriController.php +++ b/app/Controllers/LibriController.php @@ - } else { - unset($fields['issn']); // Invalid — don't overwrite existing value - } + } else { + $_SESSION['error_message'] = __('ISSN non valido. Usa il formato XXXX-XXXX.'); + return $response->withHeader('Location', url('/admin/libri/modifica/' . $id))->withStatus(302); + }Also applies to: 1238-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 754, The code currently silences malformed ISSN values by unsetting $fields['issn'] (using the preg_match/unset branch), which allows invalid input to pass; instead, when the ISSN fails the /^\d{7}[\dX]$/ check you must treat it as a validation error: replace the unset($fields['issn']) behavior with explicit rejection (for example add a validation error for 'issn' via your validator or throw/return a 400/ValidationException) and stop further processing so the request fails; apply the same change to the duplicate block (the other occurrence around lines 1238-1251) so invalid ISSNs are reported rather than silently dropped.app/Controllers/FrontendController.php (2)
661-669:⚠️ Potential issue | 🟡 MinorRemove query-level truncation for same-series source data.
Line 668 still caps siblings with
LIMIT 20, which can hide valid books in long series before UI-level slicing. Keep the source set complete and cap only where needed for display.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 661 - 669, The SQL in the $stmtSeries prepared statement includes a LIMIT 20 which truncates the source sibling set; remove the "LIMIT 20" clause from the query inside the prepare(...) call so the full series result set is returned by $stmtSeries, and apply any display-limit later where results are sliced for UI (e.g., after fetching from $stmtSeries in the controller/view logic).
679-679:⚠️ Potential issue | 🟡 MinorUse
SecureLogger::error()for this DB prepare failure path.Line 679 logs a prepare failure with DB error context; this should be emitted via
errorlevel.🔧 Proposed change
- \App\Support\SecureLogger::warning('FrontendController: series query prepare failed', ['db_error' => $db->error]); + \App\Support\SecureLogger::error('FrontendController: series query prepare failed', ['db_error' => $db->error]);As per coding guidelines "Use
SecureLogger::error()noterror_log()for sensitive contexts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` at line 679, Replace the warning-level log with an error-level log for the DB prepare failure in FrontendController: change the call to SecureLogger::warning('FrontendController: series query prepare failed', ['db_error' => $db->error]) to use SecureLogger::error(...) so the prepare failure (using $db->error as context) is emitted at error severity; update the single invocation where the series query prepare failure is handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Around line 661-669: The SQL in the $stmtSeries prepared statement includes a
LIMIT 20 which truncates the source sibling set; remove the "LIMIT 20" clause
from the query inside the prepare(...) call so the full series result set is
returned by $stmtSeries, and apply any display-limit later where results are
sliced for UI (e.g., after fetching from $stmtSeries in the controller/view
logic).
- Line 679: Replace the warning-level log with an error-level log for the DB
prepare failure in FrontendController: change the call to
SecureLogger::warning('FrontendController: series query prepare failed',
['db_error' => $db->error]) to use SecureLogger::error(...) so the prepare
failure (using $db->error as context) is emitted at error severity; update the
single invocation where the series query prepare failure is handled.
In `@app/Controllers/LibriController.php`:
- Around line 741-754: The code currently silences malformed ISSN values by
unsetting $fields['issn'] (using the preg_match/unset branch), which allows
invalid input to pass; instead, when the ISSN fails the /^\d{7}[\dX]$/ check you
must treat it as a validation error: replace the unset($fields['issn']) behavior
with explicit rejection (for example add a validation error for 'issn' via your
validator or throw/return a 400/ValidationException) and stop further processing
so the request fails; apply the same change to the duplicate block (the other
occurrence around lines 1238-1251) so invalid ISSNs are reported rather than
silently dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42e6aaca-dfd8-4450-a8c9-43f620871826
📒 Files selected for processing (3)
app/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Views/frontend/book-detail.php
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
app/Views/libri/scheda_libro.php (1)
982-982:⚠️ Potential issue | 🟠 MajorReplace hardcoded admin book routes with named routes.
Line 982 and Line 1017 still build
/admin/libri/...directly. Please switch both to route helpers.As per coding guidelines: "Never use hardcoded routes. Use `route_path('key')` or `RouteTranslator::route('key')` instead of hardcoded paths like `/accedi` or `/login`."🔧 Suggested change
- <a href="<?= htmlspecialchars(url('/admin/libri/' . (int)$parentWork['id']), ENT_QUOTES, 'UTF-8') ?>" class="text-indigo-600 hover:underline font-semibold"> + <a href="<?= htmlspecialchars(route_path('admin.libri.show', ['id' => (int)$parentWork['id']]), ENT_QUOTES, 'UTF-8') ?>" class="text-indigo-600 hover:underline font-semibold"> - <a href="<?= htmlspecialchars(url('/admin/libri/' . (int)$vol['id']), ENT_QUOTES, 'UTF-8') ?>" class="text-primary hover:underline"> + <a href="<?= htmlspecialchars(route_path('admin.libri.show', ['id' => (int)$vol['id']]), ENT_QUOTES, 'UTF-8') ?>" class="text-primary hover:underline">Also applies to: 1017-1017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` at line 982, Replace the hardcoded '/admin/libri/…' URL construction in the anchor hrefs with the app's named route helper (use route_path('...') or RouteTranslator::route('...') per project conventions) and pass the id parameter instead of string-concatenating (replace the href that uses (int)$parentWork['id'] and the other href that builds with the book id at line ~1017). Locate the anchors that build "/admin/libri/" and change them to call the appropriate named route (e.g. the admin libri show route) with ['id' => (int)$parentWork['id']] (and the corresponding ['id' => (int)$book['id']] for the other occurrence).tests/issue-75-issn-series-volumes.spec.js (1)
141-157:⚠️ Potential issue | 🟠 MajorThese checks can pass while the public detail page is broken.
Test 3 never resolves the real public book URL, and Tests 6/11 fall back to DB assertions when the guessed route misses. That means routing/template regressions won't fail the suite. Resolve the actual public href first, then make the test fail if the public page cannot be fetched and rendered.
Also applies to: 196-224, 324-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 141 - 157, The test "3. ISSN displayed on frontend book detail page" is using a guessed frontend URL and falling back to DB assertions, so update the test to resolve the real public book href before asserting: use the admin API response for `${BASE}/admin/libri/${bookId}` (resp) to extract the actual public path/href (from resp body or headers) instead of constructing `${BASE}/` or guessing routes, then perform request.get on that exact public URL and expect a 200 and rendered content; if the public page request fails or returns non-200, make the test fail immediately rather than asserting against dbQuery results (remove or keep DB checks only as secondary assertions), and apply the same change pattern to the other affected tests noted (around the blocks using seriesBookIds, bookId, request.get, and dbQuery at the other ranges).app/Controllers/LibriController.php (1)
741-754:⚠️ Potential issue | 🟠 MajorReject invalid ISSNs instead of unsetting them.
These branches still accept compact input like
12345678, and on malformed input they silentlyunset($fields['issn'])instead of failing the request. That means the backend no longer enforces the advertisedXXXX-XXXXrule, and the later optional-field update that merges raw request data with$fieldscan re-expose the invalid payload. Return a validation error here and only keep a canonical hyphenated ISSN when the submitted value matches the server rule.Also applies to: 1238-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 754, The ISSN normalization block in LibriController.php currently unsets invalid values (using unset($fields['issn'])) instead of rejecting them, allowing malformed inputs to slip through; change this so that when $fields['issn'] is present and non-empty you only accept it if it matches the canonical rule (four digits + hyphen + four digits) or can be canonicalized from an 8-char compact form (preg_match('/^\d{7}[\dX]$/', $issnCompact') then set $fields['issn'] = substr(...).'-'.substr(...); otherwise do not unset but add a validation failure/throw a ValidationException (or return a 4xx validation error) so the request is rejected rather than silently dropping the field; keep the empty-string behavior that sets null.
🧹 Nitpick comments (4)
locale/en_US.json (1)
2778-2779: Prefer a single placeholder-based translation instead of split sentence fragments.At Line 2778 and Line 2779, splitting one sentence into two keys makes localization brittle for other languages. Use one key with a placeholder for the volume number.
🌍 Suggested i18n-safe key shape
- "Questo libro è il volume": "This book is volume", - "dell'opera": "of the work", + "Questo libro è il volume %s dell'opera": "This book is volume %s of the work",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/en_US.json` around lines 2778 - 2779, Replace the two fragmented keys "Questo libro è il volume" and "dell'opera" with a single placeholder-based key (e.g. "Questo libro è il volume {volume} dell'opera") and corresponding English translation ("This book is volume {volume} of the work"); then update any usage sites that concatenated the two keys to use the new key and pass the volume value as the {volume} interpolation parameter (look for uses referencing "Questo libro è il volume" or "dell'opera" to change to the new key and supply the volume variable).tests/issue-75-issn-series-volumes.spec.js (1)
162-180: This only exercises the HTML pattern.The PR adds server-side ISSN validation too, but this spec never submits an invalid ISSN and checks that the save is rejected and the stored value stays unchanged. Add one negative submit-path assertion so controller-side regressions are caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 162 - 180, Add a negative server-side submit assertion: after verifying the HTML pattern with issnInput, set an invalid value (e.g., '12345678') into issnInput, submit the form (e.g., click the form submit button), wait for the save to be rejected (assert presence of server error indicator or that navigation/state didn't change), then reload the edit page for the same seriesBookIds[1] and assert the issnInput value remains the original/unchanged value; use the existing page.goto(`${BASE}/admin/libri/modifica/${seriesBookIds[1]}`), issnInput locator, and form submit action to implement this negative submit-path check so controller-side validation regressions are caught.installer/database/schema.sql (1)
424-424: Consider a composite index for series queries on soft-deleted datasets.Current lookups filter by
collanaplusdeleted_at; in larger catalogs,(collana, deleted_at)can reduce scanned rows versus indexingcollanaalone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` at line 424, The existing single-column index KEY `idx_collana` (`collana`) should be replaced or supplemented with a composite index that includes `deleted_at` to optimize queries that filter by both `collana` and `deleted_at`; update the index definition referencing the current index name `idx_collana` (or create a new `idx_collana_deleted_at`) to cover (`collana`,`deleted_at`) so series lookups on soft-deleted datasets use the composite index.app/Controllers/FrontendController.php (1)
1785-1796: Add logging for silentprepare()failures in related-books fallbacks.The new guards prevent crashes, but failures in these branches are currently silent and hard to diagnose in production.
As per coding guidelines "Use `SecureLogger::error()` not `error_log()` for sensitive contexts".Proposed pattern
$stmt = $db->prepare($query); if ($stmt) { $types = str_repeat('i', count($author_ids)) . str_repeat('i', count($exclude_ids)) . 'i'; $params = array_merge($author_ids, $exclude_ids, [$remaining]); $stmt->bind_param($types, ...$params); $stmt->execute(); $result = $stmt->get_result(); while ($row = $result->fetch_assoc()) { $related_books[] = $row; } $stmt->close(); + } else { + \App\Support\SecureLogger::warning('FrontendController: related-books author query prepare failed', [ + 'book_id' => $book_id, + 'db_error' => $db->error, + ]); }Also applies to: 1819-1830, 1853-1864
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 1785 - 1796, The prepare() call can fail silently; update the related-books fallback blocks around the $stmt = $db->prepare($query) checks (the branches that currently only proceed when $stmt is truthy) to log failures via SecureLogger::error() when $stmt is falsy, including brief context (e.g. "related-books: prepare failed"), the $db->error and an identifier for the query (not full sensitive payload) so issues are diagnosable; ensure you use SecureLogger::error() instead of error_log(), and apply the same change to the other similar blocks noted (the other prepare/stmt checks around the related-books logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@installer/database/migrations/migrate_0.5.1.sql`:
- Around line 4-5: The migration declares compatibility "MySQL 5.7+, MariaDB
10.0+" but relies on the CHECK constraint chk_volumi_not_self which those
versions parse but do not enforce; either update the compatibility header to the
enforcing minimums ("MySQL 8.0.16+, MariaDB 10.2.1+") or implement enforcement
logic (e.g., a BEFORE INSERT/BEFORE UPDATE trigger named to mirror
chk_volumi_not_self that validates the same condition and raises an error, or
add equivalent application-level validation) so the chk_volumi_not_self rule is
actually enforced across all supported deployments.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 64-72: The test seeds use a hard-coded author id (9991) and static
SERIES_NAME/ISSN_VALUE which can collide with existing rows; change the setup to
generate a per-run unique suffix (e.g., append a timestamp/UUID) and create the
author dynamically instead of assuming id 9991—capture the created author id
(via INSERT + SELECT LAST_INSERT_ID() or equivalent) and push any created libro
ids into seriesBookIds as you already do; use the unique suffix when
constructing titolo and series names so COUNT assertions are isolated, and
update the afterAll teardown to delete only the tracked author id and tracked
libro ids (from seriesBookIds) rather than deleting the fixed id 9991 or relying
on static names.
- Around line 31-39: The loginAsAdmin helper currently waits for a generic URL
pattern that also matches the login page; change the post-submit check in
loginAsAdmin to wait for a URL or DOM marker that only appears after a
successful admin login (e.g. the exact /admin/dashboard URL or an admin-only
element such as an "Admin Dashboard" heading or a logout link). Update the await
page.waitForURL(...) call in loginAsAdmin to target that post-login-only URL or
replace it with await page.waitForSelector(...) for a reliable admin-only DOM
selector so a failed submit cannot be mistaken as a successful login.
---
Duplicate comments:
In `@app/Controllers/LibriController.php`:
- Around line 741-754: The ISSN normalization block in LibriController.php
currently unsets invalid values (using unset($fields['issn'])) instead of
rejecting them, allowing malformed inputs to slip through; change this so that
when $fields['issn'] is present and non-empty you only accept it if it matches
the canonical rule (four digits + hyphen + four digits) or can be canonicalized
from an 8-char compact form (preg_match('/^\d{7}[\dX]$/', $issnCompact') then
set $fields['issn'] = substr(...).'-'.substr(...); otherwise do not unset but
add a validation failure/throw a ValidationException (or return a 4xx validation
error) so the request is rejected rather than silently dropping the field; keep
the empty-string behavior that sets null.
In `@app/Views/libri/scheda_libro.php`:
- Line 982: Replace the hardcoded '/admin/libri/…' URL construction in the
anchor hrefs with the app's named route helper (use route_path('...') or
RouteTranslator::route('...') per project conventions) and pass the id parameter
instead of string-concatenating (replace the href that uses
(int)$parentWork['id'] and the other href that builds with the book id at line
~1017). Locate the anchors that build "/admin/libri/" and change them to call
the appropriate named route (e.g. the admin libri show route) with ['id' =>
(int)$parentWork['id']] (and the corresponding ['id' => (int)$book['id']] for
the other occurrence).
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 141-157: The test "3. ISSN displayed on frontend book detail page"
is using a guessed frontend URL and falling back to DB assertions, so update the
test to resolve the real public book href before asserting: use the admin API
response for `${BASE}/admin/libri/${bookId}` (resp) to extract the actual public
path/href (from resp body or headers) instead of constructing `${BASE}/` or
guessing routes, then perform request.get on that exact public URL and expect a
200 and rendered content; if the public page request fails or returns non-200,
make the test fail immediately rather than asserting against dbQuery results
(remove or keep DB checks only as secondary assertions), and apply the same
change pattern to the other affected tests noted (around the blocks using
seriesBookIds, bookId, request.get, and dbQuery at the other ranges).
---
Nitpick comments:
In `@app/Controllers/FrontendController.php`:
- Around line 1785-1796: The prepare() call can fail silently; update the
related-books fallback blocks around the $stmt = $db->prepare($query) checks
(the branches that currently only proceed when $stmt is truthy) to log failures
via SecureLogger::error() when $stmt is falsy, including brief context (e.g.
"related-books: prepare failed"), the $db->error and an identifier for the query
(not full sensitive payload) so issues are diagnosable; ensure you use
SecureLogger::error() instead of error_log(), and apply the same change to the
other similar blocks noted (the other prepare/stmt checks around the
related-books logic).
In `@installer/database/schema.sql`:
- Line 424: The existing single-column index KEY `idx_collana` (`collana`)
should be replaced or supplemented with a composite index that includes
`deleted_at` to optimize queries that filter by both `collana` and `deleted_at`;
update the index definition referencing the current index name `idx_collana` (or
create a new `idx_collana_deleted_at`) to cover (`collana`,`deleted_at`) so
series lookups on soft-deleted datasets use the composite index.
In `@locale/en_US.json`:
- Around line 2778-2779: Replace the two fragmented keys "Questo libro è il
volume" and "dell'opera" with a single placeholder-based key (e.g. "Questo libro
è il volume {volume} dell'opera") and corresponding English translation ("This
book is volume {volume} of the work"); then update any usage sites that
concatenated the two keys to use the new key and pass the volume value as the
{volume} interpolation parameter (look for uses referencing "Questo libro è il
volume" or "dell'opera" to change to the new key and supply the volume
variable).
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 162-180: Add a negative server-side submit assertion: after
verifying the HTML pattern with issnInput, set an invalid value (e.g.,
'12345678') into issnInput, submit the form (e.g., click the form submit
button), wait for the save to be rejected (assert presence of server error
indicator or that navigation/state didn't change), then reload the edit page for
the same seriesBookIds[1] and assert the issnInput value remains the
original/unchanged value; use the existing
page.goto(`${BASE}/admin/libri/modifica/${seriesBookIds[1]}`), issnInput
locator, and form submit action to implement this negative submit-path check so
controller-side validation regressions are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5901ed06-caa8-4c91-8883-1c353d38e946
📒 Files selected for processing (11)
app/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Views/frontend/book-detail.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/issue-75-issn-series-volumes.spec.jsversion.json
Comment on lines
+31
to
+39
| async function loginAsAdmin(page) { | ||
| await page.goto(`${BASE}/admin/dashboard`); | ||
| const emailField = page.locator('input[name="email"]'); | ||
| if (await emailField.isVisible({ timeout: 3000 }).catch(() => false)) { | ||
| await emailField.fill(ADMIN_EMAIL); | ||
| await page.fill('input[name="password"]', ADMIN_PASS); | ||
| await page.click('button[type="submit"]'); | ||
| await page.waitForURL(/.*(?:dashboard|admin).*/, { timeout: 15000 }); | ||
| } |
There was a problem hiding this comment.
Wait for a post-login-only URL here.
/admin/login already matches /.*(?:dashboard|admin).*/, so a failed submit can satisfy this helper and leave the rest of the suite unauthenticated. Wait for a URL or page marker that only exists after a successful admin login.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/issue-75-issn-series-volumes.spec.js` around lines 31 - 39, The
loginAsAdmin helper currently waits for a generic URL pattern that also matches
the login page; change the post-submit check in loginAsAdmin to wait for a URL
or DOM marker that only appears after a successful admin login (e.g. the exact
/admin/dashboard URL or an admin-only element such as an "Admin Dashboard"
heading or a logout link). Update the await page.waitForURL(...) call in
loginAsAdmin to target that post-login-only URL or replace it with await
page.waitForSelector(...) for a reliable admin-only DOM selector so a failed
submit cannot be mistaken as a successful login.
Comment on lines
+64
to
+72
| dbExec(`INSERT IGNORE INTO autori (id, nome) VALUES (9991, 'Series TestAuthor')`); | ||
|
|
||
| // Create 3 books in the same series (collana) | ||
| for (let i = 1; i <= 3; i++) { | ||
| dbExec(`INSERT INTO libri (titolo, collana, numero_serie, issn, copie_totali, created_at, updated_at) | ||
| VALUES ('E2E Series Book ${i}', '${SERIES_NAME}', '${i}', ${i === 1 ? `'${ISSN_VALUE}'` : 'NULL'}, 1, NOW(), NOW())`); | ||
| const id = dbQuery(`SELECT MAX(id) FROM libri WHERE titolo='E2E Series Book ${i}' AND deleted_at IS NULL`); | ||
| seriesBookIds.push(parseInt(id)); | ||
| dbExec(`INSERT IGNORE INTO libri_autori (libro_id, autore_id, ruolo) VALUES (${id}, 9991, 'principale')`); |
There was a problem hiding this comment.
Give the seeded fixtures a unique namespace per run.
This suite hard-codes author id 9991 plus static series/work titles, then deletes that author by the fixed id in afterAll. On a reused DB, a pre-existing row can be linked or deleted, and stale books from an earlier failed run can skew the COUNT(*) assertions. Generate a per-run suffix and track the created ids from that unique seed instead.
Also applies to: 76-86, 90-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/issue-75-issn-series-volumes.spec.js` around lines 64 - 72, The test
seeds use a hard-coded author id (9991) and static SERIES_NAME/ISSN_VALUE which
can collide with existing rows; change the setup to generate a per-run unique
suffix (e.g., append a timestamp/UUID) and create the author dynamically instead
of assuming id 9991—capture the created author id (via INSERT + SELECT
LAST_INSERT_ID() or equivalent) and push any created libro ids into
seriesBookIds as you already do; use the unique suffix when constructing titolo
and series names so COUNT assertions are isolated, and update the afterAll
teardown to delete only the tracked author id and tracked libro ids (from
seriesBookIds) rather than deleting the fixed id 9991 or relying on static
names.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
app/Views/libri/scheda_libro.php (1)
982-982:⚠️ Potential issue | 🟠 MajorReplace hardcoded admin book URLs with named routes.
Line 982 and Line 1017 still hardcode
/admin/libri/{id}. Switch toroute_path(...)(orRouteTranslator::route(...)) for route safety and consistency.🔧 Suggested change
- <a href="<?= htmlspecialchars(url('/admin/libri/' . (int)$parentWork['id']), ENT_QUOTES, 'UTF-8') ?>" class="text-indigo-600 hover:underline font-semibold"> + <a href="<?= htmlspecialchars(route_path('admin.libri.show', ['id' => (int)$parentWork['id']]), ENT_QUOTES, 'UTF-8') ?>" class="text-indigo-600 hover:underline font-semibold"> <?= App\Support\HtmlHelper::e($parentWork['titolo']) ?> </a>- <a href="<?= htmlspecialchars(url('/admin/libri/' . (int)$vol['id']), ENT_QUOTES, 'UTF-8') ?>" class="text-primary hover:underline"> + <a href="<?= htmlspecialchars(route_path('admin.libri.show', ['id' => (int)$vol['id']]), ENT_QUOTES, 'UTF-8') ?>" class="text-primary hover:underline"> <?= App\Support\HtmlHelper::e($vol['titolo_volume'] ?: $vol['titolo']) ?> </a>As per coding guidelines: "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login."Also applies to: 1017-1017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` at line 982, The anchor uses a hardcoded path url('/admin/libri/' . (int)$parentWork['id']) — replace it with the named-route helper (e.g. route_path(...) or RouteTranslator::route(...)) so route generation is consistent and safe; update the href to call route_path('admin.libri.show', ['id' => (int)$parentWork['id']]) (or RouteTranslator::route('admin.libri.show', ['id' => (int)$parentWork['id']])) and do the same replacement for the second occurrence around line 1017.tests/issue-75-issn-series-volumes.spec.js (3)
31-39:⚠️ Potential issue | 🟠 MajorWait for a post-login-only state here.
/admin/loginalready matches/.*(?:dashboard|admin).*/, so a failed submit can satisfy this wait immediately and leave the rest of the suite unauthenticated. Wait for the exact dashboard URL or an admin-only DOM marker instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 31 - 39, In loginAsAdmin, the post-submit wait is too permissive and can be satisfied by the login page itself; change the wait to target a post-login-only state by awaiting either page.waitForURL(`${BASE}/admin/dashboard`) for the exact dashboard path or waiting for an admin-only DOM marker (e.g., await page.waitForSelector('selector-for-admin-only', { timeout: 15000 })) after the submit to ensure the session is actually authenticated before proceeding.
64-72:⚠️ Potential issue | 🟠 MajorUse per-run fixtures instead of fixed ids and titles.
The hard-coded author id
9991, static titles/series name, andSELECT MAX(id)lookups make this suite collide with stale or concurrent data. In a reused DB that can link the books to the wrong author, skew the counts, and letafterAll()delete a non-test row.Also applies to: 76-86, 90-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 64 - 72, Replace hard-coded fixtures with per-run created records: create the test author via dbExec and capture its inserted id (use LAST_INSERT_ID() or dbQuery that returns the insert id) instead of using 9991; generate unique SERIES_NAME and book titles (e.g., append a random/timestamp suffix) instead of static strings; insert each libro and capture each libro id directly from the insert result (do not use SELECT MAX(id)) and push those ids into seriesBookIds; when inserting into libri_autori use the captured author id; keep using dbExec/dbQuery and the existing SERIES_NAME, ISSN_VALUE, seriesBookIds variables as the reference points to locate where to change.
141-157:⚠️ Potential issue | 🟠 MajorFail if the real public detail URL cannot be resolved.
These tests guess frontend paths and then fall back to DB assertions, so routing/template regressions still go green. Resolve the actual public href first, fetch that page, and assert on its HTML; if the URL cannot be derived or the page is not reachable, the test should fail instead of dropping to SQL checks.
Also applies to: 196-224, 324-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 141 - 157, The test currently fetches an admin API and then falls back to DB assertions (dbQuery, ISSN_VALUE) instead of failing when the public detail URL cannot be resolved; update the test (the '3. ISSN displayed on frontend book detail page' block using seriesBookIds, BASE, request.get, resp, title, frontResp) to extract the actual public href (e.g., parse resp.body/resp.text or the sitemap to get the canonical/public path for the book), then perform request.get on that derived URL and assert frontResp.status() === 200 and that frontResp.text() contains the expected ISSN; if you cannot derive the public URL or the fetch returns non-200, explicitly fail the test (do not revert to dbQuery/ISSN_VALUE fallback) so routing/template regressions cause test failures.installer/database/migrations/migrate_0.5.1.sql (1)
4-5:⚠️ Potential issue | 🟠 MajorThe declared DB compatibility does not match the
CHECKbehavior.This migration advertises MySQL 5.7 / MariaDB 10.0 compatibility, but on those versions
CHECK (opera_id <> volume_id)is parsed and not enforced. That means this migration alone does not guarantee the invariant the rest of the feature assumes. Either raise the documented minimum versions or add trigger/application validation for older deployments.Do MySQL 5.7 and MariaDB 10.0 enforce CHECK constraints? From which MySQL and MariaDB versions are CHECK constraints actually enforced?Also applies to: 17-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.5.1.sql` around lines 4 - 5, The migration's header comment claims "MySQL 5.7+, MariaDB 10.0+" but the CHECK constraint "CHECK (opera_id <> volume_id)" is only parsed (not enforced) on those versions; update the migration to either raise the documented minimum to versions that enforce CHECK (e.g., MySQL 8.0.16+/MariaDB 10.2.1+ as appropriate) by editing the compatibility comment, or keep the stated compatibility and add an enforcement fallback (implement a BEFORE INSERT/UPDATE trigger that rejects rows when opera_id == volume_id or add application-level validation run during migration) so the invariant is guaranteed on older servers; locate the CHECK clause and the top-of-file compatibility comment in migrate_0.5.1.sql and apply one of these fixes consistently.app/Controllers/LibriController.php (1)
741-754:⚠️ Potential issue | 🟠 MajorReject malformed ISSNs instead of unsetting the field.
12345678still becomes a valid ISSN here because the hyphen is stripped before validation, so the backend is not enforcing the sameXXXX-XXXXcontract as the form. The invalid branch is also unsafe on update:BookRepository::updateBasic()bindsissnas$data['issn'] ?? null, so unsetting the key here can still clear the stored value while the request succeeds. Return a validation error for malformed input and reuse one strict normalizer in bothstore()andupdate().Also applies to: 1238-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 754, In LibriController, stop unsetting malformed ISSN values and instead validate/normalize strictly and return a validation error on bad input; extract the normalization/validation logic into a single helper (e.g., normalizeIssn/validateIssn) that both store() and update() call to convert inputs to the canonical XXXX-XXXX form, and in update() ensure you do not remove the issn key when input is invalid—return a validation failure (HTTP 4xx) so BookRepository::updateBasic() never receives an absent key that would clear the stored ISSN; use a strict check (digits + optional final X, exactly 8 chars when compacted) and a single canonicalizer used everywhere.app/Controllers/FrontendController.php (2)
679-679:⚠️ Potential issue | 🟡 MinorUse
SecureLogger::error()for this prepare failure path.This is an error condition (query preparation failed) and should use the error channel consistently.
As per coding guidelines "Use
SecureLogger::error()noterror_log()for sensitive contexts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` at line 679, The log call in FrontendController's series query prepare failure currently uses \App\Support\SecureLogger::warning; change it to \App\Support\SecureLogger::error so the prepare failure is recorded as an error, keeping the same context array (e.g. ['db_error' => $db->error]) and message ("FrontendController: series query prepare failed") to maintain consistency with guidelines requiring SecureLogger::error for error conditions.
661-668:⚠️ Potential issue | 🟡 MinorDon’t truncate same-series results at query source.
LIMIT 20can hide valid siblings from both “Nella stessa collana” and related-books priority. Fetch full siblings here, then cap at rendering/selection time.Suggested change
- ORDER BY CAST(l.numero_serie AS UNSIGNED), l.titolo - LIMIT 20 + ORDER BY CAST(l.numero_serie AS UNSIGNED), l.titolo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 661 - 668, The SQL that prepares $stmtSeries in FrontendController currently truncates sibling books with "LIMIT 20"; remove that LIMIT so the query returns all siblings (keep the WHERE, ORDER BY and selected columns intact), then enforce any 20-item cap later at rendering or in the selection logic where $stmtSeries results are consumed; ensure you only change the SQL string (referencing $stmtSeries and the prepared statement on $db) and not the downstream filtering so the UI can decide which and how many siblings to display.
🧹 Nitpick comments (2)
locale/en_US.json (1)
2782-2782: Polish the ISSN error message wording for native readability.Line 2782 is clear, but “in the format” reads more naturally in English UI copy.
✏️ Optional copy tweak
- "ISSN deve essere nel formato XXXX-XXXX (8 cifre, l'ultima può essere X).": "ISSN must be in format XXXX-XXXX (8 digits, last one can be X).", + "ISSN deve essere nel formato XXXX-XXXX (8 cifre, l'ultima può essere X).": "ISSN must be in the format XXXX-XXXX (8 digits; the last one can be X).",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/en_US.json` at line 2782, Update the English translation for the ISSN error message: replace the current value "ISSN must be in format XXXX-XXXX (8 digits, last one can be X)." with a more natural phrasing such as "ISSN must be in the format XXXX-XXXX (8 digits, last digit may be X)." Locate the string entry in locale/en_US.json (the key "ISSN deve essere nel formato XXXX-XXXX (8 cifre, l'ultima può essere X).") and only modify its translated value to the new wording.installer/database/schema.sql (1)
891-892:uk_opera_volumeis redundant withuk_volume_id.Since
volume_idis already unique, the composite unique index does not add extra protection and can be removed to reduce index overhead.Optional simplification
- UNIQUE KEY `uk_opera_volume` (`opera_id`,`volume_id`), UNIQUE KEY `uk_volume_id` (`volume_id`),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` around lines 891 - 892, The composite unique index `uk_opera_volume` is redundant because `volume_id` is already constrained unique by `uk_volume_id`; remove the `UNIQUE KEY uk_opera_volume (opera_id, volume_id)` declaration to avoid duplicate indexing overhead and leave `UNIQUE KEY uk_volume_id (volume_id)` as the sole uniqueness constraint on `volume_id`. Ensure any references or queries that assumed the composite index still function with the single-column unique index on `volume_id`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 296-319: The test only queries the DB but never calls the actual
related-books logic; update the test to invoke the function that builds the
"Potrebbero interessarti" list (e.g., getRelatedBooks or the controller method
used in the app) for the target book id (use seriesBookIds[0]) and assert that
the returned array begins with the same-series siblings (seriesBookIds[1] and
seriesBookIds[2]) in the expected order; keep the existing DB assertions if
useful but replace or augment them with assertions on the actual getRelatedBooks
result to verify prioritization.
---
Duplicate comments:
In `@app/Controllers/FrontendController.php`:
- Line 679: The log call in FrontendController's series query prepare failure
currently uses \App\Support\SecureLogger::warning; change it to
\App\Support\SecureLogger::error so the prepare failure is recorded as an error,
keeping the same context array (e.g. ['db_error' => $db->error]) and message
("FrontendController: series query prepare failed") to maintain consistency with
guidelines requiring SecureLogger::error for error conditions.
- Around line 661-668: The SQL that prepares $stmtSeries in FrontendController
currently truncates sibling books with "LIMIT 20"; remove that LIMIT so the
query returns all siblings (keep the WHERE, ORDER BY and selected columns
intact), then enforce any 20-item cap later at rendering or in the selection
logic where $stmtSeries results are consumed; ensure you only change the SQL
string (referencing $stmtSeries and the prepared statement on $db) and not the
downstream filtering so the UI can decide which and how many siblings to
display.
In `@app/Controllers/LibriController.php`:
- Around line 741-754: In LibriController, stop unsetting malformed ISSN values
and instead validate/normalize strictly and return a validation error on bad
input; extract the normalization/validation logic into a single helper (e.g.,
normalizeIssn/validateIssn) that both store() and update() call to convert
inputs to the canonical XXXX-XXXX form, and in update() ensure you do not remove
the issn key when input is invalid—return a validation failure (HTTP 4xx) so
BookRepository::updateBasic() never receives an absent key that would clear the
stored ISSN; use a strict check (digits + optional final X, exactly 8 chars when
compacted) and a single canonicalizer used everywhere.
In `@app/Views/libri/scheda_libro.php`:
- Line 982: The anchor uses a hardcoded path url('/admin/libri/' .
(int)$parentWork['id']) — replace it with the named-route helper (e.g.
route_path(...) or RouteTranslator::route(...)) so route generation is
consistent and safe; update the href to call route_path('admin.libri.show',
['id' => (int)$parentWork['id']]) (or RouteTranslator::route('admin.libri.show',
['id' => (int)$parentWork['id']])) and do the same replacement for the second
occurrence around line 1017.
In `@installer/database/migrations/migrate_0.5.1.sql`:
- Around line 4-5: The migration's header comment claims "MySQL 5.7+, MariaDB
10.0+" but the CHECK constraint "CHECK (opera_id <> volume_id)" is only parsed
(not enforced) on those versions; update the migration to either raise the
documented minimum to versions that enforce CHECK (e.g., MySQL 8.0.16+/MariaDB
10.2.1+ as appropriate) by editing the compatibility comment, or keep the stated
compatibility and add an enforcement fallback (implement a BEFORE INSERT/UPDATE
trigger that rejects rows when opera_id == volume_id or add application-level
validation run during migration) so the invariant is guaranteed on older
servers; locate the CHECK clause and the top-of-file compatibility comment in
migrate_0.5.1.sql and apply one of these fixes consistently.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 31-39: In loginAsAdmin, the post-submit wait is too permissive and
can be satisfied by the login page itself; change the wait to target a
post-login-only state by awaiting either
page.waitForURL(`${BASE}/admin/dashboard`) for the exact dashboard path or
waiting for an admin-only DOM marker (e.g., await
page.waitForSelector('selector-for-admin-only', { timeout: 15000 })) after the
submit to ensure the session is actually authenticated before proceeding.
- Around line 64-72: Replace hard-coded fixtures with per-run created records:
create the test author via dbExec and capture its inserted id (use
LAST_INSERT_ID() or dbQuery that returns the insert id) instead of using 9991;
generate unique SERIES_NAME and book titles (e.g., append a random/timestamp
suffix) instead of static strings; insert each libro and capture each libro id
directly from the insert result (do not use SELECT MAX(id)) and push those ids
into seriesBookIds; when inserting into libri_autori use the captured author id;
keep using dbExec/dbQuery and the existing SERIES_NAME, ISSN_VALUE,
seriesBookIds variables as the reference points to locate where to change.
- Around line 141-157: The test currently fetches an admin API and then falls
back to DB assertions (dbQuery, ISSN_VALUE) instead of failing when the public
detail URL cannot be resolved; update the test (the '3. ISSN displayed on
frontend book detail page' block using seriesBookIds, BASE, request.get, resp,
title, frontResp) to extract the actual public href (e.g., parse
resp.body/resp.text or the sitemap to get the canonical/public path for the
book), then perform request.get on that derived URL and assert
frontResp.status() === 200 and that frontResp.text() contains the expected ISSN;
if you cannot derive the public URL or the fetch returns non-200, explicitly
fail the test (do not revert to dbQuery/ISSN_VALUE fallback) so routing/template
regressions cause test failures.
---
Nitpick comments:
In `@installer/database/schema.sql`:
- Around line 891-892: The composite unique index `uk_opera_volume` is redundant
because `volume_id` is already constrained unique by `uk_volume_id`; remove the
`UNIQUE KEY uk_opera_volume (opera_id, volume_id)` declaration to avoid
duplicate indexing overhead and leave `UNIQUE KEY uk_volume_id (volume_id)` as
the sole uniqueness constraint on `volume_id`. Ensure any references or queries
that assumed the composite index still function with the single-column unique
index on `volume_id`.
In `@locale/en_US.json`:
- Line 2782: Update the English translation for the ISSN error message: replace
the current value "ISSN must be in format XXXX-XXXX (8 digits, last one can be
X)." with a more natural phrasing such as "ISSN must be in the format XXXX-XXXX
(8 digits, last digit may be X)." Locate the string entry in locale/en_US.json
(the key "ISSN deve essere nel formato XXXX-XXXX (8 cifre, l'ultima può essere
X).") and only modify its translated value to the new wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1c2c66d-4592-4acf-bffd-02036d0b5435
📒 Files selected for processing (11)
app/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Views/frontend/book-detail.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/issue-75-issn-series-volumes.spec.jsversion.json
- Update migration compatibility comment to MySQL 8.0.16+/MariaDB 10.2.1+ (CHECK constraints not enforced on older versions) - Remove redundant uk_opera_volume composite index (uk_volume_id already ensures uniqueness on volume_id) - Polish EN translation: "in format" → "in the format"
There was a problem hiding this comment.
🧹 Nitpick comments (1)
installer/database/schema.sql (1)
887-893: Consider a composite index for ordered volume reads.Given the new UI behavior, volume queries will likely filter by
opera_idand sort bynumero_volume. Adding a composite index can avoid extra sorting work.💡 Proposed schema tweak
CREATE TABLE `volumi` ( `id` int NOT NULL AUTO_INCREMENT, `opera_id` int NOT NULL COMMENT 'Parent work (the complete multi-volume work)', `volume_id` int NOT NULL COMMENT 'Child book (individual volume)', `numero_volume` smallint unsigned DEFAULT '1' COMMENT 'Volume number within the work', `titolo_volume` varchar(255) COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'Override title for this volume', `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY (`id`), UNIQUE KEY `uk_volume_id` (`volume_id`), KEY `idx_opera` (`opera_id`), + KEY `idx_opera_numero_volume` (`opera_id`,`numero_volume`), CONSTRAINT `fk_volumi_opera` FOREIGN KEY (`opera_id`) REFERENCES `libri` (`id`) ON DELETE CASCADE, CONSTRAINT `fk_volumi_volume` FOREIGN KEY (`volume_id`) REFERENCES `libri` (`id`) ON DELETE CASCADE, CONSTRAINT `chk_volumi_not_self` CHECK (`opera_id` <> `volume_id`) ) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` around lines 887 - 893, Add a composite index to speed queries that filter by `opera_id` and order by `numero_volume`: next to the existing KEY `idx_opera` add a new index on the columns (`opera_id`, `numero_volume`) (name it e.g. `idx_opera_numero_volume`) so queries using WHERE opera_id = ? ORDER BY numero_volume can use the index and avoid an extra sort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@installer/database/schema.sql`:
- Around line 887-893: Add a composite index to speed queries that filter by
`opera_id` and order by `numero_volume`: next to the existing KEY `idx_opera`
add a new index on the columns (`opera_id`, `numero_volume`) (name it e.g.
`idx_opera_numero_volume`) so queries using WHERE opera_id = ? ORDER BY
numero_volume can use the index and avoid an extra sort.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9969e4d-97c6-4244-8334-9654753517de
📒 Files selected for processing (3)
installer/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/en_US.json
🚧 Files skipped from review as they are similar to previous changes (2)
- locale/en_US.json
- installer/database/migrations/migrate_0.5.1.sql
The series pills section should show all sibling books in the collana, not an arbitrary cap. The flex-wrap CSS handles layout naturally.
Test 7 now checks for uk_volume_id instead of the removed uk_opera_volume composite index.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/Controllers/LibriController.php (1)
741-754:⚠️ Potential issue | 🟠 MajorInvalid ISSN input is still silently accepted instead of being rejected.
On Line 751 and Line 1248, invalid values are dropped via
unset($fields['issn']), so users get no validation error. Also, Line 747 and Line 1244 compact input before validation, so non-hyphenated values are accepted even though server-side validation is supposed to enforceXXXX-XXXX. Please fail validation explicitly (422/form error) for malformed ISSN and keep normalization only for valid inputs.Also applies to: 1238-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 754, The ISSN normalization currently compacts and then silently unsets invalid values ($fields['issn']), which accepts non-hyphenated input and hides errors; change the logic so you only normalize when the input is already valid and otherwise fail validation explicitly with a 422/form error. Concretely: validate the raw trimmed input ($fields['issn'] or $issnRaw) against the canonical pattern (four digits, hyphen, four digits, allowing final X: /^\d{4}-\d{3}[\dX]$/), do NOT accept compacted forms for validation, and if the input is non-empty but does not match the canonical pattern add a validation error (or throw the controller’s standard validation exception) instead of unset($fields['issn']); only when the raw input is valid produce the normalized value (uppercase, ensure canonical hyphen format) and assign it back to $fields['issn'].app/Views/libri/scheda_libro.php (1)
974-1031:⚠️ Potential issue | 🟠 MajorReplace hardcoded admin book links with route helpers (Line 982 and Line 1017).
The new links still hardcode
/admin/libri/...viaurl(...). Please use named route helpers instead.🔧 Proposed change
- <a href="<?= htmlspecialchars(url('/admin/libri/' . (int)$parentWork['id']), ENT_QUOTES, 'UTF-8') ?>" class="text-indigo-600 hover:underline font-semibold"> + <a href="<?= htmlspecialchars(route_path('admin.libri.show', ['id' => (int)$parentWork['id']]), ENT_QUOTES, 'UTF-8') ?>" class="text-indigo-600 hover:underline font-semibold"> <?= App\Support\HtmlHelper::e($parentWork['titolo']) ?> </a> ... - <a href="<?= htmlspecialchars(url('/admin/libri/' . (int)$vol['id']), ENT_QUOTES, 'UTF-8') ?>" class="text-primary hover:underline"> + <a href="<?= htmlspecialchars(route_path('admin.libri.show', ['id' => (int)$vol['id']]), ENT_QUOTES, 'UTF-8') ?>" class="text-primary hover:underline"> <?= App\Support\HtmlHelper::e($vol['titolo_volume'] ?: $vol['titolo']) ?> </a>As per coding guidelines: "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` around lines 974 - 1031, Replace the hardcoded admin links that call htmlspecialchars(url('/admin/libri/' . (int)$parentWork['id']), ...) and htmlspecialchars(url('/admin/libri/' . (int)$vol['id']), ...) with the named route helper (e.g. route_path or RouteTranslator::route); specifically build the URL with route_path('admin.libri.show', ['id' => (int)$parentWork['id']]) and route_path('admin.libri.show', ['id' => (int)$vol['id']]) (or RouteTranslator::route('admin.libri.show', …) if your app uses that), keep the htmlspecialchars(…, ENT_QUOTES, 'UTF-8') wrapping and existing casts/HTML-escaping elsewhere (App\Support\HtmlHelper::e($parentWork['titolo']) and App\Support\HtmlHelper::e($vol['titolo_volume'] ?: $vol['titolo'])), and update both occurrences in the template.
🧹 Nitpick comments (1)
tests/issue-75-issn-series-volumes.spec.js (1)
19-29: Consider handling SQL errors gracefully.The
dbQueryanddbExechelpers useexecFileSyncwithout try/catch. A failing query will throw with the full MySQL error (potentially including table structure details). For test code this is acceptable, but wrapping in try/catch would give cleaner error messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 19 - 29, dbQuery and dbExec call execFileSync directly so MySQL failures surface raw, noisy errors; wrap the execFileSync calls in try/catch inside both functions (dbQuery and dbExec) and rethrow a cleaner, concise Error that includes which helper failed, the SQL being executed (or a short summary), and the original error message (err.message or err.stderr) rather than the full MySQL dump to keep test output readable while preserving useful diagnostics. Ensure the catch preserves the original stack or message (e.g., include err.message) so debugging info isn't lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/FrontendController.php`:
- Around line 661-668: The SELECT in the prepared statement assigned to
$stmtSeries references a non-existent column l.immagine_copertina; update the
SQL in the $stmtSeries query (the SELECT that includes l.id, l.titolo,
l.numero_serie, l.copertina_url, l.immagine_copertina ...) to remove
l.immagine_copertina or replace it with the correct column defined in the schema
(for example use l.copertina_url or the actual cover column name from
installer/database/schema.sql) so the query only selects valid columns and the
series/series-priority features are not broken.
In `@locale/en_US.json`:
- Around line 2778-2779: Consolidate the two fragmented locale keys ("Questo
libro è il volume" and "dell'opera") into a single templated key like "Questo
libro è il volume {volume} dell'opera" in locale/en_US.json, remove the two old
keys, and update the template value to the English equivalent (e.g. "This book
is volume {volume} of the work"); then modify the view that uses them
(app/Views/libri/scheda_libro.php around the current usage near lines 980–981)
to call the new key and pass the volume number as the {volume} parameter instead
of concatenating two separate translations.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 209-212: The request URL passed to request.get uses
`${BASE}//e2e-series-book-1/${bookId}` which produces a double slash and omits
the author segment; update the call that constructs detailResp so the path
includes the missing author slug (e.g., 'unknown' or derive the actual author
slug from the test fixture/DB) and remove the extra slash so it becomes
`${BASE}/{authorSlug}/e2e-series-book-1/${bookId}`; adjust any test setup that
provides authorSlug accordingly to ensure the route matches the frontend pattern
expected by the test.
---
Duplicate comments:
In `@app/Controllers/LibriController.php`:
- Around line 741-754: The ISSN normalization currently compacts and then
silently unsets invalid values ($fields['issn']), which accepts non-hyphenated
input and hides errors; change the logic so you only normalize when the input is
already valid and otherwise fail validation explicitly with a 422/form error.
Concretely: validate the raw trimmed input ($fields['issn'] or $issnRaw) against
the canonical pattern (four digits, hyphen, four digits, allowing final X:
/^\d{4}-\d{3}[\dX]$/), do NOT accept compacted forms for validation, and if the
input is non-empty but does not match the canonical pattern add a validation
error (or throw the controller’s standard validation exception) instead of
unset($fields['issn']); only when the raw input is valid produce the normalized
value (uppercase, ensure canonical hyphen format) and assign it back to
$fields['issn'].
In `@app/Views/libri/scheda_libro.php`:
- Around line 974-1031: Replace the hardcoded admin links that call
htmlspecialchars(url('/admin/libri/' . (int)$parentWork['id']), ...) and
htmlspecialchars(url('/admin/libri/' . (int)$vol['id']), ...) with the named
route helper (e.g. route_path or RouteTranslator::route); specifically build the
URL with route_path('admin.libri.show', ['id' => (int)$parentWork['id']]) and
route_path('admin.libri.show', ['id' => (int)$vol['id']]) (or
RouteTranslator::route('admin.libri.show', …) if your app uses that), keep the
htmlspecialchars(…, ENT_QUOTES, 'UTF-8') wrapping and existing
casts/HTML-escaping elsewhere (App\Support\HtmlHelper::e($parentWork['titolo'])
and App\Support\HtmlHelper::e($vol['titolo_volume'] ?: $vol['titolo'])), and
update both occurrences in the template.
---
Nitpick comments:
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 19-29: dbQuery and dbExec call execFileSync directly so MySQL
failures surface raw, noisy errors; wrap the execFileSync calls in try/catch
inside both functions (dbQuery and dbExec) and rethrow a cleaner, concise Error
that includes which helper failed, the SQL being executed (or a short summary),
and the original error message (err.message or err.stderr) rather than the full
MySQL dump to keep test output readable while preserving useful diagnostics.
Ensure the catch preserves the original stack or message (e.g., include
err.message) so debugging info isn't lost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac429664-e253-4861-8932-d8891d3c90b8
📒 Files selected for processing (11)
app/Controllers/FrontendController.phpapp/Controllers/LibriController.phpapp/Views/frontend/book-detail.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/issue-75-issn-series-volumes.spec.jsversion.json
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
app/Views/libri/scheda_libro.php (1)
981-981:⚠️ Potential issue | 🟠 MajorUse the named admin-book route here.
Line 981 and Line 1016 still hardcode
/admin/libri/.... That bypasses route translation/base-path handling and reintroduces the same routing drift in the new parent/volume links. Please switch both hrefs to the named admin book-detail route and keep the current escaping wrapper.As per coding guidelines: "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login."Also applies to: 1016-1016
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` at line 981, The anchor currently hardcodes url('/admin/libri/' . (int)$parentWork['id']) — replace this with the named route helper (e.g. route_path('admin-book', ['id' => (int)$parentWork['id']]) or RouteTranslator::route('admin-book', ['id' => (int)$parentWork['id']])) and keep the existing htmlspecialchars(..., ENT_QUOTES, 'UTF-8') wrapper; do the same for the other hardcoded admin/libri link (the one using the volume id) so both links use the named admin-book route and preserve escaping.tests/issue-75-issn-series-volumes.spec.js (5)
64-73:⚠️ Potential issue | 🟠 MajorFixtures are not isolated per run.
Hardcoded identifiers (
autori.id=9991, static titles/series names) andSELECT MAX(id)make this suite vulnerable to stale-data collisions and flaky assertions in reused databases.Also applies to: 76-87, 90-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 64 - 73, The test uses hardcoded identifiers and SELECT MAX(id) which causes flaky, non-isolated fixtures; instead, create the author via dbExec and capture its inserted id (use INSERT ... RETURNING or dbQuery to get LAST_INSERT_ID()), generate unique titles/series names using a random/timestamp suffix (replace SERIES_NAME/ISSN_VALUE usages with test-local unique values), capture each book's inserted id directly (INSERT ... RETURNING or LAST_INSERT_ID()) rather than SELECT MAX(id), push those ids into seriesBookIds, and avoid assuming id=9991 by using the newly created author id when inserting into libri_autori; update all occurrences in this block and the similar blocks noted (lines 76-87, 90-99) to follow the same pattern.
295-318:⚠️ Potential issue | 🟠 MajorTest 10 validates preconditions, not the related-books prioritization behavior.
Current assertions only prove sibling rows/index existence; they never assert the actual related-books output/order for the target book.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 295 - 318, The test "10. Related books section prioritizes same-series books" currently only checks DB state via dbQuery and EXPLAIN; change it to call the actual function under test (getRelatedBooks) for the target book id (seriesBookIds[0]) and assert that the returned related-books array contains the same-series siblings (seriesBookIds[1] and seriesBookIds[2]) and that those sibling ids appear before any non-series ids (i.e., check ordering/indices in the returned list). Keep the existing precondition checks if desired, but add explicit assertions on the getRelatedBooks result (and its order) instead of relying solely on raw DB queries.
141-157:⚠️ Potential issue | 🟠 MajorDon’t downgrade frontend assertions to DB-only fallbacks.
These tests can pass even if frontend routes/templates are broken, because they fall back to direct DB checks when page resolution fails.
Also applies to: 196-224, 323-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 141 - 157, The test "3. ISSN displayed on frontend book detail page" currently falls back to DB queries (dbQuery(...)) and hits the root URL (${BASE}/) which masks broken frontend routes/templates; update the test to resolve the book's frontend path (use the admin API response from request.get(`${BASE}/admin/libri/${bookId}`) or sitemap lookup) then perform request.get(...) against that specific frontend book URL (not `${BASE}/`) and assert the response body contains the expected ISSN_VALUE; keep the dbQuery(`SELECT issn ...`) only as an auxiliary check if desired but do not replace the frontend assertion, and ensure variables used are seriesBookIds, bookId, BASE, request.get and ISSN_VALUE so the change is easy to locate.
48-61:⚠️ Potential issue | 🟠 MajorEncode the required serial E2E execution contract for this suite.
This suite shares DB fixtures in
beforeAll, so running outside/tmp/run-e2e.sh --workers=1risks cross-worker collisions and false failures.As per coding guidelines "E2E tests with Playwright require
/tmp/run-e2e.shto set DB/admin credentials as env vars. Always use--workers=1for serial execution".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 48 - 61, This suite uses shared DB fixtures and already uses test.describe.serial; add a runtime guard in the test.beforeAll block that asserts a dedicated env var (e.g. process.env.E2E_SINGLE_WORKER or RUN_E2E_SERIAL) is present/true and throw a clear error instructing to run the suite with /tmp/run-e2e.sh --workers=1 if the check fails; update the beforeAll in tests/issue-75-issn-series-volumes.spec.js (the existing test.beforeAll function) to perform this check before creating the browser context so runs outside the single-worker execution abort immediately.
31-39:⚠️ Potential issue | 🟠 MajorUse a post-login-only success check.
Line 38 can pass on
/admin/login, so failed sign-in may be treated as authenticated. Wait for a dashboard-only URL or admin-only DOM marker.🔧 Minimal fix
- await page.waitForURL(/.*(?:dashboard|admin).*/, { timeout: 15000 }); + await page.waitForURL('**/admin/dashboard', { timeout: 15000 }); + await expect(page.locator('a[href*="/logout"], button:has-text("Logout")')).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 31 - 39, The loginAsAdmin helper uses a permissive wait (page.waitForURL(/.*(?:dashboard|admin).*/) ) that can match /admin/login on failure; change loginAsAdmin to wait for a post-login-only condition instead — e.g. replace the regex waitForURL with an explicit dashboard-only wait (page.waitForURL('/admin/dashboard', { timeout: 15000 })) or wait for a known admin-only DOM marker (await page.waitForSelector('selector-for-admin-dashboard'/* e.g. "text=Admin Dashboard" or "#admin-dashboard" */, { timeout: 15000 })) after submitting credentials so failed sign-ins are not treated as authenticated.app/Controllers/LibriController.php (1)
741-752:⚠️ Potential issue | 🟠 MajorReject invalid ISSN explicitly instead of silently skipping it.
At Line 751 and Line 1248, malformed ISSN input is dropped via
unset($fields['issn']). That allows invalid submissions to pass without validation feedback, so format enforcement is incomplete.💡 Suggested direction
- } else { - unset($fields['issn']); // Invalid — don't overwrite existing value - } + } else { + $response->getBody()->write(json_encode([ + 'error' => true, + 'message' => __('ISSN non valido. Usa il formato XXXX-XXXX.'), + ], JSON_UNESCAPED_UNICODE)); + return $response->withStatus(422)->withHeader('Content-Type', 'application/json'); + }Apply equivalent invalid-input handling in
update()(using the same response pattern used by that flow).Also applies to: 1238-1249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 752, The code currently silently removes malformed ISSN by calling unset($fields['issn']) which lets invalid submissions proceed; instead, change this behavior to explicitly reject the input using the same validation/response pattern used elsewhere in LibriController (e.g., the update() flow). Replace the unset($fields['issn']) branch with logic that records a validation error for 'issn' and returns the same error response (HTTP 422 or the controller's validation response) used by update()/store() so the client receives a clear validation failure for $fields['issn'] rather than silently dropping it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/LibriApiController.php`:
- Line 415: The query in LibriApiController.php uses GROUP_CONCAT(DISTINCT
a.nome SEPARATOR ', ') AS autori which de-duplicates and only concatenates first
names (a.nome), causing distinct authors with same given name to be collapsed
and surnames to be lost; update the aggregation to use a full author label
(e.g., CONCAT_WS(' ', a.nome, a.cognome) or the existing author display column)
and remove DISTINCT on just a.nome (or apply DISTINCT to the full label) so each
unique full name is preserved, ensuring GROUP_CONCAT(DISTINCT <full_name_expr>
SEPARATOR ', ') is used and the alias autori remains.
In `@app/Controllers/PublicApiController.php`:
- Around line 342-344: The public API changed review field names and removed
legacy keys causing breaking clients; update the response mapping in
PublicApiController so the SELECT/response includes backward-compatible aliases
(e.g., keep r.stelle AS voto and r.descrizione AS testo or add fields named voto
and testo alongside titolo_recensione and descrizione) for the review projection
used in the methods referencing r.stelle / r.titolo AS titolo_recensione /
r.descrizione (also apply the same aliasing for the other mapping around the r.*
lines at 368-370) so both the new names and legacy keys are returned for one
deprecation window.
- Line 240: The SELECT in getBookAuthors() returns the column named
`nazionalità` which doesn't match the response mapping key `nazionalita`, so the
field can be dropped; update the SELECT to alias the column (e.g., SELECT ...
a.`nazionalità` AS nazionalita) so the returned row keys match the mapping used
by getBookAuthors() and the API includes the nationality field.
In `@app/Views/libri/scheda_libro.php`:
- Around line 1008-1021: The ISBN column currently renders only $vol['isbn13'],
so volumes that only have an ISBN-10 show blank; update the rendering in
scheda_libro.php (the foreach over $volumes, cell that references
$vol['isbn13']) to fall back to $vol['isbn10'] when isbn13 is empty (e.g.,
choose $vol['isbn13'] ?? $vol['isbn10'] or use a conditional that outputs isbn10
if isbn13 is falsy) and ensure the value is passed through
App\Support\HtmlHelper::e() and htmlspecialchars/url logic remains unchanged.
In `@installer/database/migrations/migrate_0.5.1.sql`:
- Around line 1-5: Don't modify migrate_0.5.1.sql in-place because the migration
runner uses the recorded version in the migrations table (see
scripts/manual-upgrade.php logic that skips applied versions); instead create a
new corrective migration (e.g., migrate_0.5.2.sql) that applies the volumi and
idx_collana DDL changes idempotently (use IF NOT EXISTS / ALTER .. ADD IF NOT
EXISTS patterns) so environments that already have 0.5.1 recorded will receive
the fixes, or alternatively confirm no shared environment has already applied
0.5.1 before shipping this change.
---
Duplicate comments:
In `@app/Controllers/LibriController.php`:
- Around line 741-752: The code currently silently removes malformed ISSN by
calling unset($fields['issn']) which lets invalid submissions proceed; instead,
change this behavior to explicitly reject the input using the same
validation/response pattern used elsewhere in LibriController (e.g., the
update() flow). Replace the unset($fields['issn']) branch with logic that
records a validation error for 'issn' and returns the same error response (HTTP
422 or the controller's validation response) used by update()/store() so the
client receives a clear validation failure for $fields['issn'] rather than
silently dropping it.
In `@app/Views/libri/scheda_libro.php`:
- Line 981: The anchor currently hardcodes url('/admin/libri/' .
(int)$parentWork['id']) — replace this with the named route helper (e.g.
route_path('admin-book', ['id' => (int)$parentWork['id']]) or
RouteTranslator::route('admin-book', ['id' => (int)$parentWork['id']])) and keep
the existing htmlspecialchars(..., ENT_QUOTES, 'UTF-8') wrapper; do the same for
the other hardcoded admin/libri link (the one using the volume id) so both links
use the named admin-book route and preserve escaping.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 64-73: The test uses hardcoded identifiers and SELECT MAX(id)
which causes flaky, non-isolated fixtures; instead, create the author via dbExec
and capture its inserted id (use INSERT ... RETURNING or dbQuery to get
LAST_INSERT_ID()), generate unique titles/series names using a random/timestamp
suffix (replace SERIES_NAME/ISSN_VALUE usages with test-local unique values),
capture each book's inserted id directly (INSERT ... RETURNING or
LAST_INSERT_ID()) rather than SELECT MAX(id), push those ids into seriesBookIds,
and avoid assuming id=9991 by using the newly created author id when inserting
into libri_autori; update all occurrences in this block and the similar blocks
noted (lines 76-87, 90-99) to follow the same pattern.
- Around line 295-318: The test "10. Related books section prioritizes
same-series books" currently only checks DB state via dbQuery and EXPLAIN;
change it to call the actual function under test (getRelatedBooks) for the
target book id (seriesBookIds[0]) and assert that the returned related-books
array contains the same-series siblings (seriesBookIds[1] and seriesBookIds[2])
and that those sibling ids appear before any non-series ids (i.e., check
ordering/indices in the returned list). Keep the existing precondition checks if
desired, but add explicit assertions on the getRelatedBooks result (and its
order) instead of relying solely on raw DB queries.
- Around line 141-157: The test "3. ISSN displayed on frontend book detail page"
currently falls back to DB queries (dbQuery(...)) and hits the root URL
(${BASE}/) which masks broken frontend routes/templates; update the test to
resolve the book's frontend path (use the admin API response from
request.get(`${BASE}/admin/libri/${bookId}`) or sitemap lookup) then perform
request.get(...) against that specific frontend book URL (not `${BASE}/`) and
assert the response body contains the expected ISSN_VALUE; keep the
dbQuery(`SELECT issn ...`) only as an auxiliary check if desired but do not
replace the frontend assertion, and ensure variables used are seriesBookIds,
bookId, BASE, request.get and ISSN_VALUE so the change is easy to locate.
- Around line 48-61: This suite uses shared DB fixtures and already uses
test.describe.serial; add a runtime guard in the test.beforeAll block that
asserts a dedicated env var (e.g. process.env.E2E_SINGLE_WORKER or
RUN_E2E_SERIAL) is present/true and throw a clear error instructing to run the
suite with /tmp/run-e2e.sh --workers=1 if the check fails; update the beforeAll
in tests/issue-75-issn-series-volumes.spec.js (the existing test.beforeAll
function) to perform this check before creating the browser context so runs
outside the single-worker execution abort immediately.
- Around line 31-39: The loginAsAdmin helper uses a permissive wait
(page.waitForURL(/.*(?:dashboard|admin).*/) ) that can match /admin/login on
failure; change loginAsAdmin to wait for a post-login-only condition instead —
e.g. replace the regex waitForURL with an explicit dashboard-only wait
(page.waitForURL('/admin/dashboard', { timeout: 15000 })) or wait for a known
admin-only DOM marker (await
page.waitForSelector('selector-for-admin-dashboard'/* e.g. "text=Admin
Dashboard" or "#admin-dashboard" */, { timeout: 15000 })) after submitting
credentials so failed sign-ins are not treated as authenticated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 416eac9e-50ca-4248-b196-81adf45dea44
📒 Files selected for processing (13)
app/Controllers/FrontendController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PublicApiController.phpapp/Views/frontend/book-detail.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/issue-75-issn-series-volumes.spec.jsversion.json
- LibriApiController::byGenre(): use hasTableColumn() for cognome check (same pattern as list() method) instead of hardcoding a.nome - PublicApiController::getBookAuthors(): alias nazionalità AS nazionalita so response mapping keys match - PublicApiController::getBookReviews(): add backward-compatible voto/testo aliases alongside stelle/descrizione - Volumes admin table: fallback to isbn10 when isbn13 is empty - Add isbn10 to volumes query SELECT
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
app/Views/libri/scheda_libro.php (1)
981-982:⚠️ Potential issue | 🟡 MinorUse route helpers for the new book-detail links.
Lines 981 and 1016 introduce fresh hardcoded
/admin/libri/{id}paths. Please switch both to the named admin-book route helper so locale or route changes do not leave these relationship links behind.As per coding guidelines: "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login"Also applies to: 1016-1017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` around lines 981 - 982, Replace the hardcoded admin book URLs with the named route helper: where the template builds url('/admin/libri/' . (int)$parentWork['id']) (and the similar occurrence for the related work around the second occurrence), call the configured route helper (e.g. route_path('admin-book', ['id' => (int)$parentWork['id']]) or RouteTranslator::route('admin-book', ['id' => (int)$parentWork['id']])) so the link generation uses the named route; update both places that render the anchor hrefs (the lines using url('/admin/libri/...') and the related anchor near the second occurrence) to use the route helper while keeping the existing escaping via App\Support\HtmlHelper::e and htmlspecialchars for the displayed title.tests/issue-75-issn-series-volumes.spec.js (5)
64-73:⚠️ Potential issue | 🟠 MajorFixture seeding is collision-prone across runs.
Static author ID/title values plus
SELECT MAX(id)(Line 70/Line 83) can bind stale rows and delete unrelated data (Line 98) on reused databases.💡 Suggested stabilization pattern
-const SERIES_NAME = 'E2E Test Trilogy'; +const RUN_ID = Date.now().toString(36); +const SERIES_NAME = `E2E Test Trilogy ${RUN_ID}`; +let authorId = 0; - dbExec(`INSERT IGNORE INTO autori (id, nome) VALUES (9991, 'Series TestAuthor')`); + dbExec(`INSERT INTO autori (nome) VALUES ('Series TestAuthor ${RUN_ID}')`); + authorId = Number(dbQuery('SELECT LAST_INSERT_ID()')); - const id = dbQuery(`SELECT MAX(id) FROM libri WHERE titolo='E2E Series Book ${i}' AND deleted_at IS NULL`); + const id = Number(dbQuery('SELECT LAST_INSERT_ID()')); - dbExec(`INSERT IGNORE INTO libri_autori (libro_id, autore_id, ruolo) VALUES (${id}, 9991, 'principale')`); + dbExec(`INSERT INTO libri_autori (libro_id, autore_id, ruolo) VALUES (${id}, ${authorId}, 'principale')`); - dbExec(`DELETE FROM autori WHERE id=9991`); + if (authorId > 0) dbExec(`DELETE FROM autori WHERE id=${authorId}`);Also applies to: 76-87, 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 64 - 73, The test seeding uses a hardcoded author id (9991), static titles and SELECT MAX(id) which causes collisions across runs; change the setup to insert the author and books using unique values (e.g. append a timestamp or random suffix to SERIES_NAME/ titles or use a generated UUID) and capture their real IDs from the DB insert result instead of SELECT MAX(id), then push those returned ids into seriesBookIds and use them for libri_autori inserts and later cleanup; update all uses of dbExec/dbQuery in this block (and similar blocks at lines referenced) to return/consume the inserted ID and avoid blind DELETEs that rely on MAX(id).
295-318:⚠️ Potential issue | 🟠 MajorRelated-books prioritization is not being validated at output level.
This checks fixture presence and index usage, but not the actual
"Potrebbero interessarti"ordering for the target book.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 295 - 318, The test currently only verifies DB fixtures and index usage but doesn't assert the actual related-books output; call the function that produces the related-books for the target book (e.g., getRelatedBooks or the equivalent API used elsewhere) with seriesBookIds[0], capture its returned list or rendered fragment, and add assertions that the same-series siblings (seriesBookIds[1], seriesBookIds[2]) appear first and in the expected order; also assert the presence of the "Potrebbero interessarti" label in the rendered output if applicable so the test validates both ordering and UI output rather than only DB state (refer to the test name "10. Related books section prioritizes same-series books", SERIES_NAME, seriesBookIds, and dbQuery for locating the relevant test).
31-39:⚠️ Potential issue | 🟠 MajorUse a post-login-only success condition in
loginAsAdmin().Line 38’s regex also matches
/admin/login, so a failed submit can be treated as authenticated.💡 Suggested fix
async function loginAsAdmin(page) { await page.goto(`${BASE}/admin/dashboard`); const emailField = page.locator('input[name="email"]'); if (await emailField.isVisible({ timeout: 3000 }).catch(() => false)) { await emailField.fill(ADMIN_EMAIL); await page.fill('input[name="password"]', ADMIN_PASS); await page.click('button[type="submit"]'); - await page.waitForURL(/.*(?:dashboard|admin).*/, { timeout: 15000 }); + await page.waitForURL(/\/admin\/dashboard(?:\/)?$/, { timeout: 15000 }); + await expect(page.locator('a[href*="logout"], form[action*="logout"]')).toBeVisible(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 31 - 39, The current loginAsAdmin() uses page.waitForURL(/.*(?:dashboard|admin).*/) which can match /admin/login; change the post-submit success check to a condition that only appears after a successful login (e.g., waitForURL for a specific post-login path like /admin/dashboard or waitForSelector for a dashboard-specific element or logout button). Update the waitForURL/ waitForSelector call inside loginAsAdmin to use that post-login-only indicator so a failed submit cannot be mistaken for success.
141-157:⚠️ Potential issue | 🟠 MajorFrontend validations are currently optional, so regressions can pass.
Line 151 only fetches homepage in Test 3, and Lines 217-223 / Lines 333-339 downgrade failures to DB checks. These tests should fail if the public detail page cannot be resolved/rendered.
Also applies to: 196-224, 323-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 141 - 157, The test currently only fetches the homepage and falls back to DB assertions (using request.get(`${BASE}/`) and dbQuery(...)), so frontend regressions slip; update the '3. ISSN displayed on frontend book detail page' test to resolve the actual public detail path for the book (use the admin API response or construct the public path from seriesBookIds/bookId), perform a GET against that detail URL with request.get(detailUrl) and assert resp.status() === 200 and that the response body contains the ISSN string (compare to ISSN_VALUE), and remove or stop downgrading failures to DB-only checks (dbQuery(...) should be used only as supplemental verification, not as a replacement for asserting the frontend page renders). Ensure the same change is applied to the other affected tests that use the same pattern (the tests that call request.get(`${BASE}/`) and then only assert DB values).
48-48:⚠️ Potential issue | 🟠 MajorEnforce the required E2E invocation contract (
/tmp/run-e2e.sh --workers=1).
test.describe.serial()here does not guarantee single-worker execution across files/runs, while this suite mutates shared DB state.#!/bin/bash # Verify repo-level Playwright invocation respects wrapper + single worker rg -n \ -g 'package.json' \ -g 'tests/playwright.config.js' \ -g '.github/workflows/*.yml' \ -g '.github/workflows/*.yaml' \ '/tmp/run-e2e\.sh|playwright test|--workers(=| )1'As per coding guidelines:
tests/**/*.spec.js: “E2E tests with Playwright require/tmp/run-e2e.shto set DB/admin credentials as env vars. Always use--workers=1for serial execution”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` at line 48, The test uses test.describe.serial(...) but doesn't enforce the repo-level E2E invocation contract (/tmp/run-e2e.sh --workers=1); add a runtime guard at the top of this spec (tests/issue-75-issn-series-volumes.spec.js) that verifies the wrapper/worker contract and fails fast if not met — for example check process.env or process.execArgv to ensure the wrapper was used or that workers == 1 (e.g. process.env.PLAYWRIGHT_WORKERS === '1' or presence of an env set by /tmp/run-e2e.sh), and throw a clear error before test.describe.serial is invoked so the suite never runs without the required single-worker wrapper.app/Controllers/LibriController.php (1)
741-754:⚠️ Potential issue | 🟠 MajorReject malformed ISSNs instead of ignoring them.
Lines 747-752 and 1244-1249 still strip separators before validation, so values like
1234567Xor1234-56-78pass server-side even though the form advertisesXXXX-XXXX. Invalid values are then dropped withunset($fields['issn']), which makes create/update succeed without an explicit validation error.Also applies to: 1238-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 754, The ISSN handling in LibriController currently strips separators then accepts malformed inputs and uses unset($fields['issn']) to silently drop invalid values; instead validate the raw trimmed input against the canonical ISSN format (pattern /^\d{4}-\d{3}[\dX]$/ with uppercase X), do not remove hyphens before checking, and on invalid input add a validation error (or return a validation failure) rather than unsetting $fields['issn']; update the block that reads and normalizes $fields['issn'] to accept '' -> null, accept only matches to the canonical regex, and on success normalize case/format, otherwise record a validation error so create/update fails explicitly.
🧹 Nitpick comments (1)
installer/database/schema.sql (1)
891-892: Indexvolumifor the new sort path.The new read in
app/Controllers/LibriController.php(Lines 528-536) filters byopera_idand orders bynumero_volume, soidx_operaonly helps the filter and still leaves a sort on every detail page. A composite index on(opera_id, numero_volume)fits that query shape and can replace the single-column key.📈 Suggested index change
- KEY `idx_opera` (`opera_id`), + KEY `idx_opera_numero_volume` (`opera_id`,`numero_volume`),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` around lines 891 - 892, The single-column index KEY `idx_opera` isn't covering the query in LibriController (filter by opera_id and ORDER BY numero_volume); replace it with a composite index on (opera_id, numero_volume) so the filter and sort are covered. In schema.sql drop or remove KEY `idx_opera` and add a composite index (e.g. `idx_opera_numero_volume`) on the columns `opera_id, numero_volume` alongside the existing UNIQUE KEY `uk_volume_id`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/LibriController.php`:
- Around line 528-536: The query prepared into $volStmt in LibriController.php
silently caps returned parent volumes with "LIMIT 100" (the SELECT from volumi v
JOIN libri l ... ORDER BY v.numero_volume LIMIT 100); remove the hard cap or
replace it with a proper pagination mechanism (e.g. add LIMIT/OFFSET driven by
request params and a UI "show more"/page controls) or explicitly expose the cap
in the admin/detail view so users know results are truncated—update the SQL and
corresponding controller method that builds $volStmt and adjust the view to
handle pagination or display a truncated-results notice.
In `@app/Views/libri/partials/book_form.php`:
- Around line 2895-2903: Update LibriController::store() and
LibriController::update() to validate ISSN server-side using the same strict
pattern used in the client (four digits, hyphen, three digits and a digit or X/x
— /^\d{4}-\d{3}[\dXx]$/) after trimming whitespace; if the incoming issn is
present but does not match that regex, add a validation error for the issn field
and return the form with errors (do not silently unset the field), ensuring the
controller uses the same normalized input logic as the client before validating
so compact or separator-removed submissions are rejected with a proper error
message.
- Around line 208-210: The placeholder string for the ISSN input is output raw
(placeholder="<?= __('es. 1234-5678') ?>") and must be escaped to prevent
injected quotes from breaking attributes; update the placeholder to use the view
escaping helper (for consistency use HtmlHelper::e or htmlspecialchars with
ENT_QUOTES and UTF-8) — locate the input with id="issn" in the book form and
replace the raw __('es. 1234-5678') with the escaped version (e.g.,
HtmlHelper::e(__('es. 1234-5678')) or htmlspecialchars(__('es. 1234-5678'),
ENT_QUOTES, 'UTF-8')) so the attribute is safely escaped.
---
Duplicate comments:
In `@app/Controllers/LibriController.php`:
- Around line 741-754: The ISSN handling in LibriController currently strips
separators then accepts malformed inputs and uses unset($fields['issn']) to
silently drop invalid values; instead validate the raw trimmed input against the
canonical ISSN format (pattern /^\d{4}-\d{3}[\dX]$/ with uppercase X), do not
remove hyphens before checking, and on invalid input add a validation error (or
return a validation failure) rather than unsetting $fields['issn']; update the
block that reads and normalizes $fields['issn'] to accept '' -> null, accept
only matches to the canonical regex, and on success normalize case/format,
otherwise record a validation error so create/update fails explicitly.
In `@app/Views/libri/scheda_libro.php`:
- Around line 981-982: Replace the hardcoded admin book URLs with the named
route helper: where the template builds url('/admin/libri/' .
(int)$parentWork['id']) (and the similar occurrence for the related work around
the second occurrence), call the configured route helper (e.g.
route_path('admin-book', ['id' => (int)$parentWork['id']]) or
RouteTranslator::route('admin-book', ['id' => (int)$parentWork['id']])) so the
link generation uses the named route; update both places that render the anchor
hrefs (the lines using url('/admin/libri/...') and the related anchor near the
second occurrence) to use the route helper while keeping the existing escaping
via App\Support\HtmlHelper::e and htmlspecialchars for the displayed title.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 64-73: The test seeding uses a hardcoded author id (9991), static
titles and SELECT MAX(id) which causes collisions across runs; change the setup
to insert the author and books using unique values (e.g. append a timestamp or
random suffix to SERIES_NAME/ titles or use a generated UUID) and capture their
real IDs from the DB insert result instead of SELECT MAX(id), then push those
returned ids into seriesBookIds and use them for libri_autori inserts and later
cleanup; update all uses of dbExec/dbQuery in this block (and similar blocks at
lines referenced) to return/consume the inserted ID and avoid blind DELETEs that
rely on MAX(id).
- Around line 295-318: The test currently only verifies DB fixtures and index
usage but doesn't assert the actual related-books output; call the function that
produces the related-books for the target book (e.g., getRelatedBooks or the
equivalent API used elsewhere) with seriesBookIds[0], capture its returned list
or rendered fragment, and add assertions that the same-series siblings
(seriesBookIds[1], seriesBookIds[2]) appear first and in the expected order;
also assert the presence of the "Potrebbero interessarti" label in the rendered
output if applicable so the test validates both ordering and UI output rather
than only DB state (refer to the test name "10. Related books section
prioritizes same-series books", SERIES_NAME, seriesBookIds, and dbQuery for
locating the relevant test).
- Around line 31-39: The current loginAsAdmin() uses
page.waitForURL(/.*(?:dashboard|admin).*/) which can match /admin/login; change
the post-submit success check to a condition that only appears after a
successful login (e.g., waitForURL for a specific post-login path like
/admin/dashboard or waitForSelector for a dashboard-specific element or logout
button). Update the waitForURL/ waitForSelector call inside loginAsAdmin to use
that post-login-only indicator so a failed submit cannot be mistaken for
success.
- Around line 141-157: The test currently only fetches the homepage and falls
back to DB assertions (using request.get(`${BASE}/`) and dbQuery(...)), so
frontend regressions slip; update the '3. ISSN displayed on frontend book detail
page' test to resolve the actual public detail path for the book (use the admin
API response or construct the public path from seriesBookIds/bookId), perform a
GET against that detail URL with request.get(detailUrl) and assert resp.status()
=== 200 and that the response body contains the ISSN string (compare to
ISSN_VALUE), and remove or stop downgrading failures to DB-only checks
(dbQuery(...) should be used only as supplemental verification, not as a
replacement for asserting the frontend page renders). Ensure the same change is
applied to the other affected tests that use the same pattern (the tests that
call request.get(`${BASE}/`) and then only assert DB values).
- Line 48: The test uses test.describe.serial(...) but doesn't enforce the
repo-level E2E invocation contract (/tmp/run-e2e.sh --workers=1); add a runtime
guard at the top of this spec (tests/issue-75-issn-series-volumes.spec.js) that
verifies the wrapper/worker contract and fails fast if not met — for example
check process.env or process.execArgv to ensure the wrapper was used or that
workers == 1 (e.g. process.env.PLAYWRIGHT_WORKERS === '1' or presence of an env
set by /tmp/run-e2e.sh), and throw a clear error before test.describe.serial is
invoked so the suite never runs without the required single-worker wrapper.
---
Nitpick comments:
In `@installer/database/schema.sql`:
- Around line 891-892: The single-column index KEY `idx_opera` isn't covering
the query in LibriController (filter by opera_id and ORDER BY numero_volume);
replace it with a composite index on (opera_id, numero_volume) so the filter and
sort are covered. In schema.sql drop or remove KEY `idx_opera` and add a
composite index (e.g. `idx_opera_numero_volume`) on the columns `opera_id,
numero_volume` alongside the existing UNIQUE KEY `uk_volume_id`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6d165f5-49bd-4fa6-a89a-7752ee72b6e3
📒 Files selected for processing (13)
app/Controllers/FrontendController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PublicApiController.phpapp/Views/frontend/book-detail.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/issue-75-issn-series-volumes.spec.jsversion.json
Comment on lines
+2895
to
+2903
| const issn = form.querySelector('input[name="issn"]').value.replace(/\s/g, ''); | ||
| if (issn && !/^\d{4}-\d{3}[\dXx]$/.test(issn)) { | ||
| Swal.fire({ | ||
| icon: 'error', | ||
| title: __('ISSN Non Valido'), | ||
| text: __('ISSN deve essere nel formato XXXX-XXXX (8 cifre, l\'ultima può essere X).') | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Server-side ISSN validation is still weaker than this client rule.
These lines enforce strict XXXX-XXXX, but the current LibriController::store() / update() flow still accepts compact ISSNs after stripping separators and silently drops invalid values. Non-JS or direct submits can therefore bypass the UI contract or lose the edit without feedback. Please make the controller enforce the same format and return a validation error instead of unsetting the field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/libri/partials/book_form.php` around lines 2895 - 2903, Update
LibriController::store() and LibriController::update() to validate ISSN
server-side using the same strict pattern used in the client (four digits,
hyphen, three digits and a digit or X/x — /^\d{4}-\d{3}[\dXx]$/) after trimming
whitespace; if the incoming issn is present but does not match that regex, add a
validation error for the issn field and return the form with errors (do not
silently unset the field), ensuring the controller uses the same normalized
input logic as the client before validating so compact or separator-removed
submissions are rejected with a proper error message.
Admin book detail now supports creating and managing volume relationships: - "Aggiungi volume" button opens SweetAlert modal with book search (autocomplete via /api/search/unified) and volume number input - "Rimuovi" button on each volume row with confirmation dialog - "Configura come opera multi-volume" link shown on books without existing volume relationships - Two new API endpoints: POST /admin/libri/volumi/add and POST /admin/libri/volumi/remove (CSRF + admin auth protected) - DOM-safe rendering (textContent, no innerHTML with user data) - Translations added for EN and DE (13 new keys) Staff can now fully manage multi-volume relationships from the UI without needing direct database access.
New admin page at /admin/collane with: - List all series with book counts and volume ranges - Detail page for each series showing all books ordered by volume number - Rename series (updates all books with that collana value) - Merge two series (moves all books from source to target) - Create multi-volume parent work from a series (generates parent book + links all series books as volumes in the volumi table) Sidebar link added between Generi and Prestiti. Translations: 25 new keys for IT, EN, DE. Uses the same UI patterns as genre management (cards, forms with CSRF tokens, session-based success/error messages).
The installer's table count verification (expected == actual) breaks whenever a new table is added. The missing-table check is sufficient to verify schema integrity. Plugins and migrations can legitimately add extra tables.
…digo - Rewrite collane/index.php and collane/dettaglio.php using app's existing style (gray-800 buttons, rounded-xl cards, breadcrumbs) - Remove ALL indigo/violet colors from scheda_libro.php volume section - Add "Assegna collana" bulk action button in book list (SweetAlert input modal → POST /admin/collane/bulk-assign) - Create collane table (id, nome, descrizione) in migration and schema for series metadata and descriptions - Add collane to Installer::EXPECTED_TABLES - Add CollaneController::bulkAssign() endpoint - Add translations for bulk assign (IT/EN/DE)
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (3)
app/Views/libri/scheda_libro.php (1)
1479-1603: Silent failure in search error handling.Line 1530 has an empty
catchblock that silently swallows errors during the volume search. This makes debugging difficult if the search API fails.Also, the fetch calls on lines 1548 and 1586 use hardcoded paths (
/admin/libri/volumi/add,/admin/libri/volumi/remove). While JavaScript doesn't have access to PHP route helpers, consider passing these URLs from PHP via a data attribute or global config for maintainability.💡 Suggested fix for silent catch
- } catch { resultsDiv.textContent = ''; } + } catch (err) { + console.error('Volume search failed:', err); + resultsDiv.textContent = ''; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` around lines 1479 - 1603, The search catch inside addVolumeModal currently swallows errors; update the catch to surface the error (e.g., log to console and show a small inline message in `#swal-volume-results` or use Swal.showValidationMessage) so failures are visible and debuggable, and ensure the handler refers to the right elements (swal-volume-results, swal-volume-search); additionally, avoid hardcoded admin endpoints in addVolumeModal and removeVolume by reading the add/remove URLs from a PHP-injected value (e.g., window.ROUTES.addVolume / window.ROUTES.removeVolume or data-route attributes on a container element) and use those variables instead of the literal '/admin/libri/volumi/add' and '/admin/libri/volumi/remove'.app/Controllers/LibriController.php (1)
2033-2038: Consider checking and logging delete operation result.The
removeVolumemethod doesn't verify if the delete operation succeeded or if there was an error. While returning success for idempotent deletes is acceptable, database errors should still be logged.♻️ Suggested improvement
$stmt = $db->prepare("DELETE FROM volumi WHERE opera_id = ? AND volume_id = ?"); if ($stmt) { $stmt->bind_param('ii', $operaId, $volumeId); - $stmt->execute(); + if (!$stmt->execute()) { + SecureLogger::warning('Failed to remove volume relationship', [ + 'opera_id' => $operaId, + 'volume_id' => $volumeId, + 'db_error' => $db->error, + ]); + } $stmt->close(); + } else { + SecureLogger::error('Failed to prepare DELETE statement for volumi', [ + 'opera_id' => $operaId, + 'volume_id' => $volumeId, + ]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 2033 - 2038, In removeVolume, after preparing and executing the DELETE ($stmt on $db with $operaId and $volumeId), check the execution result and log any database error or unexpected affected rows: verify $stmt !== false, then after $stmt->execute() inspect $stmt->errno/$stmt->error and $stmt->affected_rows and log errors (or use the controller logger) if errno is non-zero or if an error string exists; also log a debug/info message when affected_rows === 0 if you want idempotent-delete visibility; ensure you still close $stmt afterwards.installer/database/schema.sql (1)
424-424: Consider a composite index for the common series query shape.
idx_collanahelps, but same-series queries typically include soft-delete filtering too. Adding(collana, deleted_at)can further reduce scanned rows.Suggested index refinement
- KEY `idx_collana` (`collana`), + KEY `idx_collana` (`collana`), + KEY `idx_collana_deleted_at` (`collana`, `deleted_at`),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` at line 424, The existing single-column index KEY `idx_collana` on `collana` should be replaced or augmented with a composite index on (collana, deleted_at) because common queries filter by both collana and soft-delete state; update the schema to add or alter the index named `idx_collana` (or create a new index such as `idx_collana_deleted_at`) covering the columns `collana` and `deleted_at` so query planners can use the composite index for WHERE collana = ? AND deleted_at IS NULL/<> ? patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/CollaneController.php`:
- Around line 166-174: The current handler returns success JSON even when the DB
update fails; update the logic in the CollaneController methods that perform
updates (the inline update block around the prepare/execute and the bulkAssign()
method) to check for errors: verify prepare() returned a valid $stmt and if not
return a JSON error (success:false) with an appropriate HTTP status; after
execute() check its boolean result and/or $stmt->affected_rows to ensure a row
was changed, and on failure return an error JSON and HTTP 4xx/5xx; always close
the statement ($stmt->close()) and include a short error message or code in the
response to surface DB issues instead of unconditionally returning success.
- Around line 249-265: The loop uses INSERT IGNORE and blindly increments
$volNum even when inserts are skipped or $stmtInsert is falsy, so detect
insertion failures and abort the whole operation: wrap the work in a
transaction, replace INSERT IGNORE behavior by checking $stmtInsert->execute()
return and $stmtInsert->affected_rows (or throw on SQL duplication) inside the
while loop, do not increment $volNum or continue on false/0 affected_rows, and
on any failure roll back the transaction, close $stmtInsert/$stmtBooks, and set
an error message instead of the success message
($_SESSION['success_message')—use the existing $stmtInsert, $stmtBooks,
$parentId, $parentTitle and $volNum variables to locate and implement these
checks.
- Line 52: Replace hardcoded redirects to "/admin/collane" in CollaneController
with the named-route helper and include the series name so detail view opens:
find return statements that call withHeader('Location',
url('/admin/collane'))->withStatus(302) (and similar redirects in the
rename/merge flows and at the other noted locations) and change them to use
route_path('admin.collane') or RouteTranslator::route('admin.collane') instead
of url('/admin/collane'); append the nome as the dettaglio query param (e.g.
?dettaglio=<nome>) when redirecting so the list opens the correct detail; ensure
you pull the correct $nome variable from the current scope (rename/merge
handlers and other methods where those redirects occur) and use the same
withStatus(302) chaining.
- Around line 227-235: The INSERT for the synthetic parent in CollaneController
creates copie_totali = 0 but leaves copie_disponibili at its default (1); update
the INSERT used with $db/$stmt so it sets copie_disponibili = 0 as well (e.g.
add the copie_disponibili column and a 0 value: VALUES (?, ?, 0, 0, NOW(),
NOW())), keeping the same bind_param call for $parentTitle and $collana so
$parentId is correct after $stmt->execute() and $stmt->close().
In `@app/Controllers/LibriController.php`:
- Around line 2010-2012: Replace the direct exposure of the database error in
the JSON response (where the code currently writes json_encode(['error' => true,
'message' => $db->error])) with a generic client-facing message (e.g., 'Internal
server error' or 'Database error') and return the 500 response as before;
simultaneously, log the actual database error using SecureLogger::error()
including contextual info (e.g., function or query and $db->error) so the real
error is recorded server-side; locate this change in LibriController where
$response, $success, and $db are used to handle the failed DB operation.
In `@app/Views/collane/dettaglio.php`:
- Line 91: The translation output from __("Sei sicuro? Tutti i libri verranno
spostati nella collana di destinazione.") is being injected directly into the
onsubmit JavaScript string (confirm(...)) causing an XSS risk; update the
onsubmit so the argument to confirm() is produced with PHP's json_encode and
JSON_HEX_TAG to properly escape quotes and JS-sensitive characters (i.e.,
replace the raw __("...") inside the onsubmit confirm call with a json_encoded
version) while leaving the form action/html escaping unchanged.
In `@app/Views/collane/index.php`:
- Line 11: Replace hardcoded admin URL usage in the href that currently calls
url('/admin/dashboard') with the routing helper route_path('key') (or
RouteTranslator::route('key')) while preserving the existing
htmlspecialchars(...) escaping and ENT_QUOTES, 'UTF-8' parameters; locate
occurrences of url('/admin/...') in this view (including the same pattern used
at the other mentioned places) and swap them to route_path('your.route.key') (or
RouteTranslator::route('your.route.key')) inside the htmlspecialchars call so
the attribute escaping remains unchanged.
- Around line 72-73: The template uses truthy checks for $c['min_num'] and
$c['max_num'], so a value of 0 is treated as false and the range is hidden;
change the condition to explicit null checks (e.g. check $c['min_num'] !== null
and $c['max_num'] !== null) before rendering the "min – max" output so zero is
displayed correctly, and keep the rendering of <?= (int) $c['min_num'] ?> – <?=
(int) $c['max_num'] ?> unchanged.
In `@app/Views/libri/index.php`:
- Around line 1174-1179: The current handler only checks data.error and may
treat non-2xx HTTP responses as success; update the logic that processes resp
and data (the block using const data = await resp.json(), resp, Swal.fire and
window.location.reload()) to first check resp.ok (or resp.status) and treat
non-2xx as a failure path — show the error Swal.fire with data.message or a
fallback error message when resp.ok is false (or when parsing fails), and only
show the success Swal.fire and call window.location.reload() when resp.ok is
true and data.error is not set.
- Line 1168: Replace the hardcoded '/admin/collane/bulk-assign' in the fetch
call that assigns resp (the line creating const resp = await fetch(...)) with a
routed path helper; use the project's route helper (e.g.,
route_path('admin.collane.bulk-assign') or
RouteTranslator::route('admin.collane.bulk-assign')) so the URL is generated
from routes instead of being hardcoded, and keep the existing window.BASE_PATH
handling if needed when concatenating the generated path.
In `@installer/database/migrations/migrate_0.5.1.sql`:
- Around line 43-50: The collane.nome column length (VARCHAR(255)) is
inconsistent with libri.collana (VARCHAR(100)), so update the schema to use the
same max length and correct the comment; either change collane.nome to
VARCHAR(100) so it matches libri.collana or expand libri.collana to VARCHAR(255)
and update its definition and any related constraints—locate the CREATE TABLE
collane block (collane, nome) and the libri table definition (libri, collana)
and make both VARCHAR lengths identical and adjust the comment text accordingly.
In `@locale/de_DE.json`:
- Around line 3469-3492: This JSON contains four duplicate translation keys
("Unisci", "Rimuovi", "ID libro non valido", "Parametri non validi") which cause
lint failures; remove the later duplicate occurrence of each key (keep the
first/earliest definition for each of Unisci, Rimuovi, ID libro non valido,
Parametri non validi), ensure the remaining entries keep the intended
translations (e.g., the earlier German values), then validate the JSON and
re-run the linter to confirm duplicates are resolved.
In `@locale/en_US.json`:
- Around line 2790-2815: This block contains duplicate locale keys that override
earlier entries — remove the duplicate keys "Unisci", "Parametri non validi",
"Rimuovi", and "ID libro non valido" from this JSON fragment so each translation
key appears only once (keep the original canonical entries defined elsewhere),
or merge any differing translations into the canonical definitions; ensure the
resulting JSON object in this section only contains unique keys to avoid
order-dependent overrides.
In `@vendor/composer/installed.json`:
- Line 1874: Update the "dev" flag in the vendor/composer/installed.json entry
currently set as "dev": true to "dev": false so the installed metadata reflects
release mode; locate the "dev" key in the same package object (the surrounding
JSON for the package entry at the shown diff) and change its boolean value, and
also quickly scan other package objects in installed.json to ensure no other
entries incorrectly have "dev": true.
---
Nitpick comments:
In `@app/Controllers/LibriController.php`:
- Around line 2033-2038: In removeVolume, after preparing and executing the
DELETE ($stmt on $db with $operaId and $volumeId), check the execution result
and log any database error or unexpected affected rows: verify $stmt !== false,
then after $stmt->execute() inspect $stmt->errno/$stmt->error and
$stmt->affected_rows and log errors (or use the controller logger) if errno is
non-zero or if an error string exists; also log a debug/info message when
affected_rows === 0 if you want idempotent-delete visibility; ensure you still
close $stmt afterwards.
In `@app/Views/libri/scheda_libro.php`:
- Around line 1479-1603: The search catch inside addVolumeModal currently
swallows errors; update the catch to surface the error (e.g., log to console and
show a small inline message in `#swal-volume-results` or use
Swal.showValidationMessage) so failures are visible and debuggable, and ensure
the handler refers to the right elements (swal-volume-results,
swal-volume-search); additionally, avoid hardcoded admin endpoints in
addVolumeModal and removeVolume by reading the add/remove URLs from a
PHP-injected value (e.g., window.ROUTES.addVolume / window.ROUTES.removeVolume
or data-route attributes on a container element) and use those variables instead
of the literal '/admin/libri/volumi/add' and '/admin/libri/volumi/remove'.
In `@installer/database/schema.sql`:
- Line 424: The existing single-column index KEY `idx_collana` on `collana`
should be replaced or augmented with a composite index on (collana, deleted_at)
because common queries filter by both collana and soft-delete state; update the
schema to add or alter the index named `idx_collana` (or create a new index such
as `idx_collana_deleted_at`) covering the columns `collana` and `deleted_at` so
query planners can use the composite index for WHERE collana = ? AND deleted_at
IS NULL/<> ? patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 368fd9d0-fe51-468d-99bd-18f575679ab1
📒 Files selected for processing (24)
app/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PublicApiController.phpapp/Routes/web.phpapp/Views/collane/dettaglio.phpapp/Views/collane/index.phpapp/Views/frontend/book-detail.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/classes/Installer.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/issue-75-issn-series-volumes.spec.jsvendor/composer/autoload_classmap.phpvendor/composer/autoload_static.phpvendor/composer/installed.jsonvendor/composer/installed.phpversion.json
💤 Files with no reviewable changes (2)
- vendor/composer/autoload_classmap.php
- vendor/composer/autoload_static.php
| { | ||
| $collana = trim((string) ($request->getQueryParams()['nome'] ?? '')); | ||
| if ($collana === '') { | ||
| return $response->withHeader('Location', url('/admin/collane'))->withStatus(302); |
There was a problem hiding this comment.
Use named routes for these redirects, and pass nome to the detail view.
Line 50 reads nome, but Lines 123 and 149 redirect with ?dettaglio= on a hardcoded /admin/collane URL, so rename/merge can send the user back to the list instead of the series detail. The other redirects have the same hardcoded-route problem.
As per coding guidelines "Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login".
Also applies to: 111-123, 137-149, 223-230, 239-239, 266-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/CollaneController.php` at line 52, Replace hardcoded
redirects to "/admin/collane" in CollaneController with the named-route helper
and include the series name so detail view opens: find return statements that
call withHeader('Location', url('/admin/collane'))->withStatus(302) (and similar
redirects in the rename/merge flows and at the other noted locations) and change
them to use route_path('admin.collane') or
RouteTranslator::route('admin.collane') instead of url('/admin/collane'); append
the nome as the dettaglio query param (e.g. ?dettaglio=<nome>) when redirecting
so the list opens the correct detail; ensure you pull the correct $nome variable
from the current scope (rename/merge handlers and other methods where those
redirects occur) and use the same withStatus(302) chaining.
Comment on lines
+166
to
+174
| $stmt = $db->prepare("UPDATE libri SET numero_serie = ?, updated_at = NOW() WHERE id = ? AND deleted_at IS NULL"); | ||
| if ($stmt) { | ||
| $stmt->bind_param('si', $numero, $bookId); | ||
| $stmt->execute(); | ||
| $stmt->close(); | ||
| } | ||
|
|
||
| $response->getBody()->write(json_encode(['success' => true], JSON_UNESCAPED_UNICODE)); | ||
| return $response->withHeader('Content-Type', 'application/json'); |
There was a problem hiding this comment.
Don't emit success JSON unless the UPDATE actually succeeds.
Line 173 always returns success when prepare() fails, and Lines 201-205 do the same if execute() fails in bulkAssign(). That hides DB errors and leaves the admin UI believing the change was saved when nothing changed.
🛠️ Suggested guard pattern
- if ($stmt) {
- $stmt->bind_param('si', $numero, $bookId);
- $stmt->execute();
- $stmt->close();
- }
-
- $response->getBody()->write(json_encode(['success' => true], JSON_UNESCAPED_UNICODE));
- return $response->withHeader('Content-Type', 'application/json');
+ if (!$stmt) {
+ $response->getBody()->write(json_encode(['error' => true, 'message' => __('Errore database')], JSON_UNESCAPED_UNICODE));
+ return $response->withStatus(500)->withHeader('Content-Type', 'application/json');
+ }
+
+ $stmt->bind_param('si', $numero, $bookId);
+ if (!$stmt->execute()) {
+ $stmt->close();
+ $response->getBody()->write(json_encode(['error' => true, 'message' => __('Errore database')], JSON_UNESCAPED_UNICODE));
+ return $response->withStatus(500)->withHeader('Content-Type', 'application/json');
+ }
+ $stmt->close();
+
+ $response->getBody()->write(json_encode(['success' => true], JSON_UNESCAPED_UNICODE));
+ return $response->withHeader('Content-Type', 'application/json');Also applies to: 193-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/CollaneController.php` around lines 166 - 174, The current
handler returns success JSON even when the DB update fails; update the logic in
the CollaneController methods that perform updates (the inline update block
around the prepare/execute and the bulkAssign() method) to check for errors:
verify prepare() returned a valid $stmt and if not return a JSON error
(success:false) with an appropriate HTTP status; after execute() check its
boolean result and/or $stmt->affected_rows to ensure a row was changed, and on
failure return an error JSON and HTTP 4xx/5xx; always close the statement
($stmt->close()) and include a short error message or code in the response to
surface DB issues instead of unconditionally returning success.
Comment on lines
+249
to
+265
| $stmtInsert = $db->prepare("INSERT IGNORE INTO volumi (opera_id, volume_id, numero_volume) VALUES (?, ?, ?)"); | ||
| while ($row = $result->fetch_assoc()) { | ||
| $bookId = (int) $row['id']; | ||
| $num = (int) ($row['numero_serie'] ?: $volNum); | ||
| if ($stmtInsert) { | ||
| $stmtInsert->bind_param('iii', $parentId, $bookId, $num); | ||
| $stmtInsert->execute(); | ||
| } | ||
| $volNum++; | ||
| } | ||
| if ($stmtInsert) { | ||
| $stmtInsert->close(); | ||
| } | ||
| $stmtBooks->close(); | ||
| } | ||
|
|
||
| $_SESSION['success_message'] = sprintf(__('Opera "%s" creata con %d volumi'), $parentTitle, $volNum - 1); |
There was a problem hiding this comment.
Fail the whole operation when a volume link is skipped.
volumi.volume_id is unique, but this loop uses INSERT IGNORE and increments $volNum even when the row is ignored—or when $stmtInsert is false. A duplicate submit or any pre-existing link can therefore create a new parent work with zero linked volumes while Line 265 still reports full success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/CollaneController.php` around lines 249 - 265, The loop uses
INSERT IGNORE and blindly increments $volNum even when inserts are skipped or
$stmtInsert is falsy, so detect insertion failures and abort the whole
operation: wrap the work in a transaction, replace INSERT IGNORE behavior by
checking $stmtInsert->execute() return and $stmtInsert->affected_rows (or throw
on SQL duplication) inside the while loop, do not increment $volNum or continue
on false/0 affected_rows, and on any failure roll back the transaction, close
$stmtInsert/$stmtBooks, and set an error message instead of the success message
($_SESSION['success_message')—use the existing $stmtInsert, $stmtBooks,
$parentId, $parentTitle and $volNum variables to locate and implement these
checks.
- "Nuova Collana" button on /admin/collane with SweetAlert input - Description field on /admin/collane/dettaglio (saved to collane table) - Autocomplete in bulk assign modal (searches collane + libri.collana) - API endpoint GET /api/collane/search for autocomplete - Routes: POST /admin/collane/crea, POST /admin/collane/descrizione - Translations: 10 new keys for IT/EN/DE
- Add "Elimina collana" button on detail page (red danger style, removes collana from all books + deletes from collane table) - 6 new E2E tests: page load, create collana, save description, verify description, bulk assign via API, delete collana - Route POST /admin/collane/elimina with CSRF + admin auth - Translations for delete (IT/EN/DE)
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (19)
app/Views/libri/index.php (2)
1202-1207:⚠️ Potential issue | 🟠 MajorTreat non-2xx bulk-assign responses as failure.
Current logic branches only on
data.error; non-2xx replies without that field can hit the success path.🔧 Suggested fix
- const data = await resp.json(); - if (data.error) { + const data = await resp.json().catch(() => ({})); + if (!resp.ok || data.success === false || data.error) { await Swal.fire({ icon: 'error', title: __('Errore'), text: data.message }); } else { await Swal.fire({ icon: 'success', title: __('Collana assegnata'), text: data.message, timer: 2000, showConfirmButton: false }); window.location.reload(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` around lines 1202 - 1207, The success path currently only checks data.error and can treat non-2xx HTTP responses without that field as success; update the logic to treat non-2xx responses as failures by checking the Response object (resp.ok or resp.status) before showing the success Swal. Specifically, after const data = await resp.json(); change the branch to if (!resp.ok || data.error) { await Swal.fire({ icon: 'error', title: __('Errore'), text: data.message || __('Errore sconosciuto') }); } else { await Swal.fire({ icon: 'success', title: __('Collana assegnata'), text: data.message, timer: 2000, showConfirmButton: false }); window.location.reload(); } and ensure you still use resp and data.message consistently.
1173-1173:⚠️ Potential issue | 🟠 MajorReplace hardcoded endpoints with routed paths.
The new fetch URLs are hardcoded and can drift from route definitions/base-path behavior.
🔧 Suggested fix
+ const collanaSearchUrl = <?= json_encode(url(route_path('api.collane.search')), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>; + const bulkAssignCollanaUrl = <?= json_encode(url(route_path('admin.collane.bulk-assign')), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>; @@ - const r = await fetch((window.BASE_PATH || '') + '/api/collane/search?q=' + encodeURIComponent(q)); + const r = await fetch(`${collanaSearchUrl}?q=${encodeURIComponent(q)}`); @@ - const resp = await fetch((window.BASE_PATH || '') + '/admin/collane/bulk-assign', { + const resp = await fetch(bulkAssignCollanaUrl, {As per coding guidelines "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login."Also applies to: 1196-1196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/index.php` at line 1173, The fetch call that builds the URL using (window.BASE_PATH || '') + '/api/collane/search?q=' + encodeURIComponent(q) is a hardcoded route; replace it with the routed path helper (e.g. route_path('api_collane_search') or RouteTranslator::route('api_collane_search')) so the URL is derived from routes, e.g. fetch(route_path('api_collane_search') + '?q=' + encodeURIComponent(q)); update the other identical occurrence (the fetch using same pattern at the later location) as well and keep the encodeURIComponent(q) and variable r/fetch unchanged.locale/de_DE.json (1)
3469-3469:⚠️ Potential issue | 🟠 MajorRemove duplicate JSON keys introduced in this block.
Line 3469 (
"Unisci"), Line 3481 ("ID libro non valido"), Line 3492 ("Rimuovi"), Line 3493 ("Parametri non validi"), and Line 3497 ("Descrizione") re-declare keys that already exist earlier in the same object. This can break linting and makes key resolution ambiguous.✂️ Proposed fix
"Nome collana di destinazione": "Name der Zielreihe", - "Unisci": "Zusammenführen", "Sei sicuro? Tutti i libri verranno spostati nella collana di destinazione.": "Sind Sie sicher? Alle Bücher werden in die Zielreihe verschoben.", @@ "Parametri non validi per l'unione": "Ungültige Parameter für Zusammenführung", "Opera \"%s\" creata con %d volumi": "Werk \"%s\" erstellt mit %d Bänden", "Errore nella creazione dell'opera": "Fehler beim Erstellen des Werks", - "ID libro non valido": "Ungültige Buch-ID", "Aggiungi volume": "Band hinzufügen", @@ "Volume rimosso": "Band entfernt", "Configura come opera multi-volume": "Als mehrbändiges Werk konfigurieren", - "Rimuovi": "Entfernen", - "Parametri non validi": "Ungültige Parameter", "Errore database": "Datenbankfehler", "Nuova Collana": "Neue Reihe", "Collana \"%s\" creata": "Reihe \"%s\" erstellt", - "Descrizione": "Beschreibung", "Descrizione della collana...": "Beschreibung der Reihe...",Also applies to: 3481-3481, 3492-3493, 3497-3497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/de_DE.json` at line 3469, Remove the duplicate JSON keys in the de_DE locale object by deleting or merging the later re-declarations for the keys "Unisci", "ID libro non valido", "Rimuovi", "Parametri non validi", and "Descrizione" so each key appears only once; locate the duplicate entries (the second occurrences around the block containing "Unisci": "Zusammenführen") and either remove those duplicate lines or consolidate their values with the original entries to preserve the correct translation.app/Controllers/LibriController.php (3)
2010-2012:⚠️ Potential issue | 🟠 MajorDo not expose raw database errors in API responses.
Line 2011 returns
$db->errordirectly to clients, leaking internal details. Log server-side and return a generic message.As per coding guidelines: "Use `SecureLogger::error()` not `error_log()` for sensitive contexts".🛡️ Suggested fix
if (!$success) { - $response->getBody()->write(json_encode(['error' => true, 'message' => $db->error], JSON_UNESCAPED_UNICODE)); + SecureLogger::error('Failed to add volume relationship', [ + 'opera_id' => $operaId, + 'volume_id' => $volumeId, + 'db_error' => $db->error, + ]); + $response->getBody()->write(json_encode(['error' => true, 'message' => __('Errore durante il salvataggio')], JSON_UNESCAPED_UNICODE)); return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 2010 - 2012, Replace the direct exposure of $db->error in the response: instead of returning $db->error to clients in the branch that checks !$success, call SecureLogger::error(...) to log the detailed database error server-side (include context like query or request id if available) and send a generic JSON error message to the client (e.g. { "error": true, "message": "Internal server error" }) with a 500 status and Content-Type application/json; update the block around the existing $success check and $response handling so only the generic message is written to the response body while SecureLogger records the raw $db->error.
741-754:⚠️ Potential issue | 🟠 MajorInvalid ISSN input is still silently ignored instead of rejected.
Current flow unsets
issnon invalid input and continues successfully. That does not enforce validation and can hide user mistakes. Return a validation error (e.g., 422) when ISSN format is invalid.Also applies to: 1238-1251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 754, The ISSN normalization block that checks $fields['issn'] currently unsets invalid inputs (using unset($fields['issn'])) which silently accepts bad data; instead, when the preg_match('/^\d{7}[\dX]$/', $issnCompact') check fails you must return a validation error (HTTP 422) and stop processing rather than clearing the field. Update the handling in the existing ISSN block (the code that reads $issnRaw, computes $issnCompact and uses preg_match) to produce and return a 422 validation response (or throw the controller's ValidationException) with a clear message about invalid ISSN format, and apply the same change to the duplicate ISSN handling block around lines 1238-1251 so both locations enforce rejection rather than unsetting.
528-536:⚠️ Potential issue | 🟡 Minor
LIMIT 100still silently truncates parent volumes.This can hide valid entries with no UI indication. Either paginate or make truncation explicit in the view.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 528 - 536, The SQL prepared in $volStmt currently includes a hard-coded "LIMIT 100" which silently truncates parent volumes; remove the fixed limit and implement proper pagination (accept page/limit inputs, add OFFSET and LIMIT placeholders to the query, and bind parameters) or, if you intentionally cap results, keep the limit but surface an explicit truncation indicator in the view rendering path (where the controller passes the volumes to the template) so users know results are truncated; update the code referencing $volStmt and the controller's render logic accordingly.installer/database/schema.sql (1)
885-885:⚠️ Potential issue | 🟠 MajorKeep
collane.nomealigned withlibri.collanalength.Fresh installs will still get
collane.nome VARCHAR(255)vslibri.collana VARCHAR(100), which makes the “must match” contract inconsistent.🛠️ Suggested fix
- `nome` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'Series name (must match libri.collana values)', + `nome` varchar(100) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'Series name (must match libri.collana values)',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` at line 885, collane.nome is defined as varchar(255) but must match libri.collana (varchar(100)); update the schema so collane.nome uses varchar(100) (and adjust its COMMENT if needed) to keep the length contract consistent with libri.collana; locate the column definition for collane.nome in installer/database/schema.sql and change its type from varchar(255) to varchar(100).installer/database/migrations/migrate_0.5.1.sql (1)
45-45:⚠️ Potential issue | 🟠 Major
collane.nomelength is inconsistent withlibri.collana.
collane.nomeisVARCHAR(255), butlibri.collanaisVARCHAR(100). This breaks the “must match” expectation and can cause truncation/validation mismatches.🧩 Suggested fix
- nome VARCHAR(255) NOT NULL COMMENT ''Series name (must match libri.collana values)'', + nome VARCHAR(100) NOT NULL COMMENT ''Series name (must match libri.collana values)'',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.5.1.sql` at line 45, The column size mismatch: collane.nome is VARCHAR(255) while libri.collana is VARCHAR(100); update the migration so both columns use the same length (pick one standard length and apply it consistently) — e.g., change collane.nome to VARCHAR(100) or expand libri.collana to VARCHAR(255) and update the ALTER/CREATE statement in the migration that defines collane.nome and the migration/DDL that defines libri.collana (ensure both definitions use the identical VARCHAR(n) and adjust any related constraints/comments).app/Views/collane/dettaglio.php (1)
104-104:⚠️ Potential issue | 🟠 MajorEscape the confirm strings for JavaScript context.
Dropping
__()directly intoconfirm('...')breaks as soon as a translation contains an apostrophe and leaves an XSS sink in both forms. Emit those messages withjson_encode(...)inside the attribute instead.Based on learnings, "Applies to **/*.php : Use `json_encode(..., JSON_HEX_TAG)` for any PHP→JS output in views."Suggested fix
- <form method="post" action="<?= htmlspecialchars(url('/admin/collane/unisci'), ENT_QUOTES, 'UTF-8') ?>" onsubmit="return confirm('<?= __("Sei sicuro? Tutti i libri verranno spostati nella collana di destinazione.") ?>')"> + <form method="post" action="<?= htmlspecialchars(url('/admin/collane/unisci'), ENT_QUOTES, 'UTF-8') ?>" onsubmit='return confirm(<?= json_encode(__("Sei sicuro? Tutti i libri verranno spostati nella collana di destinazione."), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>)'> - <form method="post" action="<?= htmlspecialchars(url('/admin/collane/elimina'), ENT_QUOTES, 'UTF-8') ?>" onsubmit="return confirm('<?= __("Sei sicuro? La collana verrà rimossa da tutti i libri.") ?>')"> + <form method="post" action="<?= htmlspecialchars(url('/admin/collane/elimina'), ENT_QUOTES, 'UTF-8') ?>" onsubmit='return confirm(<?= json_encode(__("Sei sicuro? La collana verrà rimossa da tutti i libri."), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>)'>Also applies to: 136-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/collane/dettaglio.php` at line 104, The inline JavaScript confirm() message currently inserts the translation via <?= __("...") ?> which breaks on apostrophes and leaves an XSS sink; change the onsubmit to emit a JSON-encoded, JS-safe string instead (use json_encode(__("..."), JSON_HEX_TAG) or equivalent) so the confirm(...) argument is properly escaped for JS; update the form tag that contains onsubmit with the confirm call (and the other similar occurrence around line 136) to use the json_encoded translation value rather than raw __() output.app/Views/collane/index.php (2)
11-11:⚠️ Potential issue | 🟠 MajorReplace the hardcoded collane paths with route keys.
This view still bakes
/admin/...into the breadcrumb, the createfetch(), and the detail links. That bypasses translated/custom routes; for the JS call, emit the route from PHP instead of concatenating a literal admin path.Based on learnings, "Applies to **/*.php : Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login."Also applies to: 45-49, 97-97, 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/collane/index.php` at line 11, The breadcrumb and other links in this view are using hardcoded admin paths (e.g., url('/admin/dashboard')) which bypass translated/custom routes; replace these with the app's route helper (e.g., route_path('admin.dashboard') or RouteTranslator::route('admin.dashboard')) for the breadcrumb, and emit the correct route string from PHP into the JS fetch/create/detail links instead of concatenating '/admin/...'; update the occurrences referenced (the url(...) in the breadcrumb and the similar hardcoded paths at the other noted spots around lines 45-49, 97, and 114) so all links and emitted routes use the route key helpers (matching the route names used by your router).
107-108:⚠️ Potential issue | 🟡 MinorUse explicit null checks for the volume range.
0is falsy here, so a valid lower or upper bound of0renders as—. Compare againstnullexplicitly instead.Suggested fix
- <?php if ($c['min_num'] && $c['max_num']): ?> + <?php if ($c['min_num'] !== null && $c['max_num'] !== null): ?>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/collane/index.php` around lines 107 - 108, The template currently uses a truthy check for the volume range which treats 0 as falsy; change the conditional to explicitly check for null (e.g. $c['min_num'] !== null and $c['max_num'] !== null) so valid zero bounds render correctly, and keep the existing output of (int) $c['min_num'] and (int) $c['max_num'] unchanged; locate the conditional around $c['min_num'] and $c['max_num'] in the collane index view and replace the truthy check with explicit null comparisons.tests/issue-75-issn-series-volumes.spec.js (4)
141-157:⚠️ Potential issue | 🟠 MajorThese frontend checks can go green while the public pages are broken.
Test 3 never loads the book detail page, and Tests 6/11 rely on guessed slugs with a DB fallback. That means routing/template regressions still pass. Resolve the real public href from the app and assert on the rendered HTML; if the page cannot be loaded, the test should fail instead of falling back to SQL.
Also applies to: 196-223, 323-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 141 - 157, The test currently skips loading the real public book-detail page by querying the DB (dbQuery) and directly requesting BASE/; update the test ('3. ISSN displayed on frontend book detail page') to resolve the public href from the application (e.g., fetch the public URL from the sitemap or the admin book API response), fetch the actual rendered HTML via request.get(theResolvedPublicPath) instead of falling back to SQL, assert the ISSN string is present in the returned HTML, and make the test fail if the public page request does not return 200; remove any DB-based ISSN assertions and any fallback logic that masks routing/template failures.
45-46:⚠️ Potential issue | 🟠 MajorGive these fixtures a unique namespace and stop hard-coding author id
9991.Using static titles/series names plus a fixed author id makes the suite nondeterministic on a reused DB, and the teardown can delete pre-existing rows. Generate a per-run suffix, create the author dynamically, and clean up only the ids captured from that unique seed.
Also applies to: 64-72, 75-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 45 - 46, The test uses static fixtures (SERIES_NAME, ISSN_VALUE) and a hard-coded author id 9991 which makes runs nondeterministic; update the spec to append a per-run unique suffix (e.g., timestamp or UUID) to SERIES_NAME and any titles/series constants, generate the author dynamically instead of using literal 9991 (create the author via the test helper/API and capture its returned id), and store all created resource ids (authorId, seriesId, volumeId, etc.) in a local array so the teardown only deletes those captured ids from this run; update references to SERIES_NAME and ISSN_VALUE and replace any literal 9991 usages with the captured authorId.
31-39:⚠️ Potential issue | 🟠 MajorWait for a post-login-only URL here.
/.*(?:dashboard|admin).*/still matches admin/login URLs, so a failed submit can satisfy the helper and invalidate the rest of the suite. Wait for the actual dashboard URL or an admin-only DOM marker instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 31 - 39, The helper loginAsAdmin currently waits for a loose URL regex (/.*(?:dashboard|admin).*/) that can match the login page; update loginAsAdmin to wait for a post-login-only condition such as the exact dashboard path or an admin-only DOM marker: after submitting (in loginAsAdmin) replace the waitForURL call with a waitForURL that matches the exact dashboard URL (e.g. '/admin/dashboard' or the full resolved path) or use await page.waitForSelector('<admin-only-selector>') where '<admin-only-selector>' is a stable element visible only after successful admin login; ensure you remove the ambiguous regex and target either page.waitForURL('/admin/dashboard') or page.waitForSelector for the admin-only DOM marker so failed submits cannot satisfy the helper.
295-318:⚠️ Potential issue | 🟠 MajorThis never verifies the actual related-books ordering.
The assertions only prove same-series siblings exist and that
idx_collanais present. If"Potrebbero interessarti"stops prepending same-series books, this test still passes. Assert on the rendered related-books section (or the controller output) and check that the first related cards are the sibling volumes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 295 - 318, The test currently only checks DB results (seriesCount, siblingIds, explain) but does not assert the actual related-books ordering; update the test (the '10. Related books section prioritizes same-series books' case) to fetch the rendered related-books output for the target book (use the app controller/route that renders the book detail or call getRelatedBooks if available), parse the related-books cards from the HTML/response, and assert that the first related card IDs match the expected sibling order (use seriesBookIds[0] as the source and compare the first related IDs to siblingIds or explicit seriesBookIds[1], seriesBookIds[2]); keep the existing DB checks but add this rendered-output assertion so the test fails if same-series siblings are not prepended.app/Controllers/CollaneController.php (3)
52-53:⚠️ Potential issue | 🟠 MajorReplace hardcoded redirect URLs and unify detail query key.
Redirects are hardcoded (
url('/admin/...')), andrename()/merge()usedettagliowhileshow()readsnome(Line 50), which can break detail navigation consistency.As per coding guidelines "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login".Also applies to: 122-123, 133-134, 146-147, 167-168, 181-182, 192-193, 206-207, 218-219, 232-233, 244-245, 351-352, 358-359, 367-368, 394-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 52 - 53, The controller uses hardcoded redirect URLs and inconsistent detail query keys: replace all url('/admin/collane') (and other hardcoded '/admin/...' paths listed) with the route helper (e.g., route_path('admin.collane.index') or RouteTranslator::route('admin.collane.index')) in methods that perform redirects (e.g., in the method containing the return withHeader(...)->withStatus(302) and the other locations noted), and unify the detail query key by changing calls that set/query 'dettaglio' (in rename(), merge()) or 'nome' (in show()) to use a single key (pick one, e.g., 'dettaglio') across CollaneController so link generation and lookups are consistent; update any places that read or write the old key to the chosen key and adjust route generation parameters accordingly.
125-130:⚠️ Potential issue | 🟠 MajorDon’t return success when DB writes fail.
Multiple mutating paths still report success after silent
prepare()/execute()failures, which can desync UI and data state.🛠️ Guard pattern (apply to each write endpoint)
$stmt = $db->prepare("UPDATE libri SET numero_serie = ?, updated_at = NOW() WHERE id = ? AND deleted_at IS NULL"); -if ($stmt) { - $stmt->bind_param('si', $numero, $bookId); - $stmt->execute(); - $stmt->close(); -} - -$response->getBody()->write(json_encode(['success' => true], JSON_UNESCAPED_UNICODE)); -return $response->withHeader('Content-Type', 'application/json'); +if (!$stmt) { + $response->getBody()->write(json_encode(['error' => true, 'message' => __('Errore database')], JSON_UNESCAPED_UNICODE)); + return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); +} +$stmt->bind_param('si', $numero, $bookId); +if (!$stmt->execute()) { + $stmt->close(); + $response->getBody()->write(json_encode(['error' => true, 'message' => __('Errore database')], JSON_UNESCAPED_UNICODE)); + return $response->withStatus(500)->withHeader('Content-Type', 'application/json'); +} +$stmt->close(); +$response->getBody()->write(json_encode(['success' => true], JSON_UNESCAPED_UNICODE)); +return $response->withHeader('Content-Type', 'application/json');Also applies to: 184-189, 209-216, 235-242, 261-269, 286-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 125 - 130, The insert logic in CollaneController currently ignores failures from $db->prepare and $stmt->execute and still returns success; update the write endpoints (the INSERT block using $db->prepare for collane and the similar blocks at the other mentioned ranges) to check the return values and errors: if $db->prepare returns false, log/return an error and do not report success; after bind_param/execute, verify execute() returned true and on failure close the stmt and return an error response (including DB error info if available) instead of proceeding; apply this guard pattern to the prepare/execute sequences in the collane insert and the other mutating methods referenced so UI only sees success on actual DB success.
355-395:⚠️ Potential issue | 🟠 MajorParent-work creation can produce invalid/partial graph state.
Two issues here: parent insert omits
copie_disponibili = 0, and volume linking usesINSERT IGNOREwithout failure checks/transaction, so you can report success with missing links.🛠️ Safer parent+volume creation
-$stmt = $db->prepare("INSERT INTO libri (titolo, collana, copie_totali, created_at, updated_at) VALUES (?, ?, 0, NOW(), NOW())"); +$stmt = $db->prepare("INSERT INTO libri (titolo, collana, copie_totali, copie_disponibili, created_at, updated_at) VALUES (?, ?, 0, 0, NOW(), NOW())");+$db->begin_transaction(); +try { // create parent ... - $stmtInsert = $db->prepare("INSERT IGNORE INTO volumi (opera_id, volume_id, numero_volume) VALUES (?, ?, ?)"); + $stmtInsert = $db->prepare("INSERT INTO volumi (opera_id, volume_id, numero_volume) VALUES (?, ?, ?)"); while ($row = $result->fetch_assoc()) { // ... - $stmtInsert->execute(); + if (!$stmtInsert->execute() || $stmtInsert->affected_rows !== 1) { + throw new \RuntimeException('volume link failed'); + } } + $db->commit(); +} catch (\Throwable $e) { + $db->rollback(); + $_SESSION['error_message'] = __('Errore nella creazione dell\'opera'); + return $response->withHeader('Location', route_path('admin.collane'))->withStatus(302); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 355 - 395, The parent insert and volume linking must be atomic and set copie_disponibili explicitly: change the "INSERT INTO libri (titolo, collana, copie_totali, created_at, updated_at) VALUES ..." to include copie_disponibili = 0 (or add the column in VALUES) when preparing the statement in the code that calls $db->prepare for the libri insert, and wrap the whole sequence (parent INSERT and the volumi INSERTs) in a DB transaction using $db->begin_transaction(), $db->commit(), and $db->rollback() on error; after each prepare/execute (both the parent prepare and the $stmtInsert for "INSERT IGNORE INTO volumi (...)") check return values and use $stmtInsert->affected_rows (or execute return) to detect failures instead of silently ignoring; on any failure rollback, set the session error message and redirect, otherwise commit and continue to set the success message and redirect to the new parent via $db->commit().locale/en_US.json (1)
2790-2818:⚠️ Potential issue | 🟠 MajorRemove duplicate locale keys introduced in this block.
Line [2790] (
"Unisci"), Line [2802] ("ID libro non valido"), Line [2813] ("Rimuovi"), Line [2814] ("Parametri non validi"), and Line [2818] ("Descrizione") are already declared earlier in the same JSON object. This makes translation resolution order-dependent and triggers duplicate-key lint errors.♻️ Proposed fix
- "Unisci": "Merge", - "ID libro non valido": "Invalid book ID", - "Rimuovi": "Remove", - "Parametri non validi": "Invalid parameters", - "Descrizione": "Description",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/en_US.json` around lines 2790 - 2818, This JSON block introduces duplicate translation keys ("Unisci", "ID libro non valido", "Rimuovi", "Parametri non validi", "Descrizione"); remove these duplicate entries from this block (keep the original earlier definitions) so each key appears only once, then run the locale linter to verify no duplicate-key errors remain.
🧹 Nitpick comments (2)
app/Views/libri/scheda_libro.php (1)
1547-1560: Consider using route helpers for API endpoint paths.The JavaScript volume management functions use hardcoded paths like
/admin/libri/volumi/addand/admin/libri/volumi/remove. While JavaScript doesn't have direct access to PHP route helpers, consider generating these paths server-side and embedding them viajson_encode(..., JSON_HEX_TAG)in a data attribute or JS variable for consistency.Example approach
Add a script block at the top of the view to expose routes:
<script> window.PINAKES_ROUTES = { volumiAdd: <?= json_encode(url('/admin/libri/volumi/add'), JSON_HEX_TAG) ?>, volumiRemove: <?= json_encode(url('/admin/libri/volumi/remove'), JSON_HEX_TAG) ?> }; </script>Then use in JS:
const resp = await fetch(window.PINAKES_ROUTES.volumiAdd, { ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` around lines 1547 - 1560, Replace hardcoded API paths used in the fetch calls (e.g., '/admin/libri/volumi/add' and '/admin/libri/volumi/remove') by exposing server-generated route URLs to JS and using those variables; add a small script in the view that sets window.PINAKES_ROUTES = { volumiAdd: <?= json_encode(url('/admin/libri/volumi/add'), JSON_HEX_TAG) ?>, volumiRemove: <?= json_encode(url('/admin/libri/volumi/remove'), JSON_HEX_TAG) ?> } and then update the fetch in the relevant functions (the code block that calls fetch((window.BASE_PATH || '') + '/admin/libri/volumi/add', ...) and the counterpart remove call) to use window.PINAKES_ROUTES.volumiAdd and window.PINAKES_ROUTES.volumiRemove respectively so routes remain consistent and safe.app/Controllers/FrontendController.php (1)
1785-1795: Consider logging prepare failures in getRelatedBooks.When
$db->prepare()returnsfalse, the code silently skips the query without logging. While this gracefully degrades functionality, it may hide underlying database issues. Consider adding warning logs for observability, similar to line 678.Example for the same-author query block
$stmt = $db->prepare($query); if ($stmt) { // ... existing code ... + } else { + \App\Support\SecureLogger::warning('getRelatedBooks: same-author query prepare failed', ['db_error' => $db->error]); }Also applies to: 1819-1829, 1853-1863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 1785 - 1795, In getRelatedBooks, add warning logs when $db->prepare(...) returns false so prepare failures aren't silent: after each prepare check where currently "if ($stmt) { ... }" add an else branch that logs a warning including the query context and $db->error (or the DB connection's error property) so you capture the failure reason; apply the same pattern to the other two prepare blocks referenced in the method (the same-author and the two other prepares around the method) so all prepare failures are logged with identifying context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/CollaneController.php`:
- Around line 209-245: The rename() and merge() methods only update
libri.collana, leaving collane rows stale; update the collane metadata too: in
rename() after updating libri, run an UPDATE on collane to set nome = ? (and
slug if your app uses one, e.g., via your slug generation routine) and
updated_at where nome = ? so the series description stays in sync; in merge()
after moving libri, either move/merge the collane row(s) (UPDATE collane SET
nome = ?, slug = ?, updated_at = NOW() WHERE nome = ?) or DELETE the emptied
source collana and ensure the target collana exists (create it if needed) so
show() (which reads collane.nome/description) won’t be orphaned or stale.
- Around line 149-167: Wrap the two dependent DB writes in an explicit
transaction: call $db->begin_transaction() before preparing the UPDATE/DELETE,
perform the UPDATE (prepared statement $stmt) and DELETE (prepared statement
$stmt2), check execution results and set $affected from $stmt->affected_rows,
then call $db->commit() on success; on any prepare/execute failure call
$db->rollback(), close any opened statements, and set an appropriate error
message in $_SESSION['error_message'] before redirecting. Ensure you still close
$stmt and $stmt2 in both success and failure paths and reference the existing
symbols $db, $stmt, $stmt2, $nome, and $affected in the transaction flow.
In `@app/Controllers/LibriController.php`:
- Around line 2033-2040: The removeVolume() handler currently returns
{"success":true} even when $db->prepare(...) fails; modify the code to detect a
false $stmt from $db->prepare in removeVolume(), log the DB error via
SecureLogger::error(...) (include $db->error or exception details), set response
status to 500 and return a JSON error payload (e.g.
{"success":false,"error":"db_prepare_failed"}) instead of the success response;
only proceed with bind_param/execute/close and success response when $stmt is
valid.
- Around line 1994-1998: The request validation allows numero_volume to be
coerced to 0 or negative; update the validation around $numero (set from
$data['numero_volume']) to enforce $numero >= 1 before performing the insert:
ensure the controller method in LibriController (where $numero is assigned)
checks that $numero is an integer >= 1 and returns a 400 JSON error (same format
used for other parameter errors) when it is not, or include the condition in the
existing parameter check that currently tests $operaId/$volumeId so invalid
numero values also trigger the error response.
In `@app/Views/collane/dettaglio.php`:
- Around line 15-17: The breadcrumb and other links in dettaglio.php are using
hardcoded url('/admin/...') paths; replace these with route helpers such as
route_path('admin.dashboard') or RouteTranslator::route('admin.dashboard') (and
the appropriate route keys for collane, book show, and each form action
referenced at lines ~47,74,91,104,119,136) while preserving the existing
htmlspecialchars(..., ENT_QUOTES, 'UTF-8') escaping and CSS/class attributes;
ensure you update href attributes and form action attributes to use the route
helper functions (route_path(...) or RouteTranslator::route(...)) instead of
url('/admin/...') so remapped/translatable routes resolve correctly.
In `@app/Views/layout.php`:
- Around line 265-266: The anchor currently hardcodes "/admin/collane"; replace
the hardcoded path with the localized route helper (e.g. call
route_path('collane') or RouteTranslator::route('collane')) inside the existing
htmlspecialchars(...) call so the href uses the route key 'collane' and still
passes ENT_QUOTES and 'UTF-8'; keep the rest of the anchor attributes and
classes unchanged.
In `@tests/collane-management.spec.js`:
- Around line 44-46: The tests use fixed constants TEST_COLLANA and TEST_DESC
and rely on SELECT MAX(id) or deleting by name which breaks on reused DBs;
change TEST_COLLANA and TEST_DESC to include a unique per-run suffix (e.g.,
timestamp or UUID) at creation time, capture and store the actual inserted row
ids returned by the insert operations (use those ids for subsequent SELECTs and
assertions instead of SELECT MAX(id)), and modify cleanup to delete only rows
whose ids are in the tracked-id list rather than deleting by title/name; update
all places that reference TEST_COLLANA/TEST_DESC or use MAX(id) (the constants
and the insert/select/delete logic) to use the per-run suffixed values and
tracked ids for assertions and teardown.
- Around line 27-35: The login helper function loginAsAdmin uses a too-broad
waitForURL(/admin/) which can match the login page and allow tests to proceed
unauthenticated; change the waitForURL call in loginAsAdmin to wait for the
exact dashboard path (e.g., '/admin/dashboard' or a strict regex matching that
full path) so the helper only returns after successful navigation to the
authenticated admin dashboard.
In `@vendor/composer/installed.php`:
- Line 10: The committed vendor/composer/installed.php contains 'dev' => true
indicating dev dependencies were installed; update the vendor state to reflect
production dependencies by regenerating the vendor folder with composer install
--no-dev (or remove/exclude vendor/composer/installed.php from VCS) and then
commit the resulting change so 'dev' => false (or the file is not tracked);
ensure the repo matches the release workflow that uses --no-dev.
---
Duplicate comments:
In `@app/Controllers/CollaneController.php`:
- Around line 52-53: The controller uses hardcoded redirect URLs and
inconsistent detail query keys: replace all url('/admin/collane') (and other
hardcoded '/admin/...' paths listed) with the route helper (e.g.,
route_path('admin.collane.index') or
RouteTranslator::route('admin.collane.index')) in methods that perform redirects
(e.g., in the method containing the return withHeader(...)->withStatus(302) and
the other locations noted), and unify the detail query key by changing calls
that set/query 'dettaglio' (in rename(), merge()) or 'nome' (in show()) to use a
single key (pick one, e.g., 'dettaglio') across CollaneController so link
generation and lookups are consistent; update any places that read or write the
old key to the chosen key and adjust route generation parameters accordingly.
- Around line 125-130: The insert logic in CollaneController currently ignores
failures from $db->prepare and $stmt->execute and still returns success; update
the write endpoints (the INSERT block using $db->prepare for collane and the
similar blocks at the other mentioned ranges) to check the return values and
errors: if $db->prepare returns false, log/return an error and do not report
success; after bind_param/execute, verify execute() returned true and on failure
close the stmt and return an error response (including DB error info if
available) instead of proceeding; apply this guard pattern to the
prepare/execute sequences in the collane insert and the other mutating methods
referenced so UI only sees success on actual DB success.
- Around line 355-395: The parent insert and volume linking must be atomic and
set copie_disponibili explicitly: change the "INSERT INTO libri (titolo,
collana, copie_totali, created_at, updated_at) VALUES ..." to include
copie_disponibili = 0 (or add the column in VALUES) when preparing the statement
in the code that calls $db->prepare for the libri insert, and wrap the whole
sequence (parent INSERT and the volumi INSERTs) in a DB transaction using
$db->begin_transaction(), $db->commit(), and $db->rollback() on error; after
each prepare/execute (both the parent prepare and the $stmtInsert for "INSERT
IGNORE INTO volumi (...)") check return values and use
$stmtInsert->affected_rows (or execute return) to detect failures instead of
silently ignoring; on any failure rollback, set the session error message and
redirect, otherwise commit and continue to set the success message and redirect
to the new parent via $db->commit().
In `@app/Controllers/LibriController.php`:
- Around line 2010-2012: Replace the direct exposure of $db->error in the
response: instead of returning $db->error to clients in the branch that checks
!$success, call SecureLogger::error(...) to log the detailed database error
server-side (include context like query or request id if available) and send a
generic JSON error message to the client (e.g. { "error": true, "message":
"Internal server error" }) with a 500 status and Content-Type application/json;
update the block around the existing $success check and $response handling so
only the generic message is written to the response body while SecureLogger
records the raw $db->error.
- Around line 741-754: The ISSN normalization block that checks $fields['issn']
currently unsets invalid inputs (using unset($fields['issn'])) which silently
accepts bad data; instead, when the preg_match('/^\d{7}[\dX]$/', $issnCompact')
check fails you must return a validation error (HTTP 422) and stop processing
rather than clearing the field. Update the handling in the existing ISSN block
(the code that reads $issnRaw, computes $issnCompact and uses preg_match) to
produce and return a 422 validation response (or throw the controller's
ValidationException) with a clear message about invalid ISSN format, and apply
the same change to the duplicate ISSN handling block around lines 1238-1251 so
both locations enforce rejection rather than unsetting.
- Around line 528-536: The SQL prepared in $volStmt currently includes a
hard-coded "LIMIT 100" which silently truncates parent volumes; remove the fixed
limit and implement proper pagination (accept page/limit inputs, add OFFSET and
LIMIT placeholders to the query, and bind parameters) or, if you intentionally
cap results, keep the limit but surface an explicit truncation indicator in the
view rendering path (where the controller passes the volumes to the template) so
users know results are truncated; update the code referencing $volStmt and the
controller's render logic accordingly.
In `@app/Views/collane/dettaglio.php`:
- Line 104: The inline JavaScript confirm() message currently inserts the
translation via <?= __("...") ?> which breaks on apostrophes and leaves an XSS
sink; change the onsubmit to emit a JSON-encoded, JS-safe string instead (use
json_encode(__("..."), JSON_HEX_TAG) or equivalent) so the confirm(...) argument
is properly escaped for JS; update the form tag that contains onsubmit with the
confirm call (and the other similar occurrence around line 136) to use the
json_encoded translation value rather than raw __() output.
In `@app/Views/collane/index.php`:
- Line 11: The breadcrumb and other links in this view are using hardcoded admin
paths (e.g., url('/admin/dashboard')) which bypass translated/custom routes;
replace these with the app's route helper (e.g., route_path('admin.dashboard')
or RouteTranslator::route('admin.dashboard')) for the breadcrumb, and emit the
correct route string from PHP into the JS fetch/create/detail links instead of
concatenating '/admin/...'; update the occurrences referenced (the url(...) in
the breadcrumb and the similar hardcoded paths at the other noted spots around
lines 45-49, 97, and 114) so all links and emitted routes use the route key
helpers (matching the route names used by your router).
- Around line 107-108: The template currently uses a truthy check for the volume
range which treats 0 as falsy; change the conditional to explicitly check for
null (e.g. $c['min_num'] !== null and $c['max_num'] !== null) so valid zero
bounds render correctly, and keep the existing output of (int) $c['min_num'] and
(int) $c['max_num'] unchanged; locate the conditional around $c['min_num'] and
$c['max_num'] in the collane index view and replace the truthy check with
explicit null comparisons.
In `@app/Views/libri/index.php`:
- Around line 1202-1207: The success path currently only checks data.error and
can treat non-2xx HTTP responses without that field as success; update the logic
to treat non-2xx responses as failures by checking the Response object (resp.ok
or resp.status) before showing the success Swal. Specifically, after const data
= await resp.json(); change the branch to if (!resp.ok || data.error) { await
Swal.fire({ icon: 'error', title: __('Errore'), text: data.message || __('Errore
sconosciuto') }); } else { await Swal.fire({ icon: 'success', title: __('Collana
assegnata'), text: data.message, timer: 2000, showConfirmButton: false });
window.location.reload(); } and ensure you still use resp and data.message
consistently.
- Line 1173: The fetch call that builds the URL using (window.BASE_PATH || '') +
'/api/collane/search?q=' + encodeURIComponent(q) is a hardcoded route; replace
it with the routed path helper (e.g. route_path('api_collane_search') or
RouteTranslator::route('api_collane_search')) so the URL is derived from routes,
e.g. fetch(route_path('api_collane_search') + '?q=' + encodeURIComponent(q));
update the other identical occurrence (the fetch using same pattern at the later
location) as well and keep the encodeURIComponent(q) and variable r/fetch
unchanged.
In `@installer/database/migrations/migrate_0.5.1.sql`:
- Line 45: The column size mismatch: collane.nome is VARCHAR(255) while
libri.collana is VARCHAR(100); update the migration so both columns use the same
length (pick one standard length and apply it consistently) — e.g., change
collane.nome to VARCHAR(100) or expand libri.collana to VARCHAR(255) and update
the ALTER/CREATE statement in the migration that defines collane.nome and the
migration/DDL that defines libri.collana (ensure both definitions use the
identical VARCHAR(n) and adjust any related constraints/comments).
In `@installer/database/schema.sql`:
- Line 885: collane.nome is defined as varchar(255) but must match libri.collana
(varchar(100)); update the schema so collane.nome uses varchar(100) (and adjust
its COMMENT if needed) to keep the length contract consistent with
libri.collana; locate the column definition for collane.nome in
installer/database/schema.sql and change its type from varchar(255) to
varchar(100).
In `@locale/de_DE.json`:
- Line 3469: Remove the duplicate JSON keys in the de_DE locale object by
deleting or merging the later re-declarations for the keys "Unisci", "ID libro
non valido", "Rimuovi", "Parametri non validi", and "Descrizione" so each key
appears only once; locate the duplicate entries (the second occurrences around
the block containing "Unisci": "Zusammenführen") and either remove those
duplicate lines or consolidate their values with the original entries to
preserve the correct translation.
In `@locale/en_US.json`:
- Around line 2790-2818: This JSON block introduces duplicate translation keys
("Unisci", "ID libro non valido", "Rimuovi", "Parametri non validi",
"Descrizione"); remove these duplicate entries from this block (keep the
original earlier definitions) so each key appears only once, then run the locale
linter to verify no duplicate-key errors remain.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 141-157: The test currently skips loading the real public
book-detail page by querying the DB (dbQuery) and directly requesting BASE/;
update the test ('3. ISSN displayed on frontend book detail page') to resolve
the public href from the application (e.g., fetch the public URL from the
sitemap or the admin book API response), fetch the actual rendered HTML via
request.get(theResolvedPublicPath) instead of falling back to SQL, assert the
ISSN string is present in the returned HTML, and make the test fail if the
public page request does not return 200; remove any DB-based ISSN assertions and
any fallback logic that masks routing/template failures.
- Around line 45-46: The test uses static fixtures (SERIES_NAME, ISSN_VALUE) and
a hard-coded author id 9991 which makes runs nondeterministic; update the spec
to append a per-run unique suffix (e.g., timestamp or UUID) to SERIES_NAME and
any titles/series constants, generate the author dynamically instead of using
literal 9991 (create the author via the test helper/API and capture its returned
id), and store all created resource ids (authorId, seriesId, volumeId, etc.) in
a local array so the teardown only deletes those captured ids from this run;
update references to SERIES_NAME and ISSN_VALUE and replace any literal 9991
usages with the captured authorId.
- Around line 31-39: The helper loginAsAdmin currently waits for a loose URL
regex (/.*(?:dashboard|admin).*/) that can match the login page; update
loginAsAdmin to wait for a post-login-only condition such as the exact dashboard
path or an admin-only DOM marker: after submitting (in loginAsAdmin) replace the
waitForURL call with a waitForURL that matches the exact dashboard URL (e.g.
'/admin/dashboard' or the full resolved path) or use await
page.waitForSelector('<admin-only-selector>') where '<admin-only-selector>' is a
stable element visible only after successful admin login; ensure you remove the
ambiguous regex and target either page.waitForURL('/admin/dashboard') or
page.waitForSelector for the admin-only DOM marker so failed submits cannot
satisfy the helper.
- Around line 295-318: The test currently only checks DB results (seriesCount,
siblingIds, explain) but does not assert the actual related-books ordering;
update the test (the '10. Related books section prioritizes same-series books'
case) to fetch the rendered related-books output for the target book (use the
app controller/route that renders the book detail or call getRelatedBooks if
available), parse the related-books cards from the HTML/response, and assert
that the first related card IDs match the expected sibling order (use
seriesBookIds[0] as the source and compare the first related IDs to siblingIds
or explicit seriesBookIds[1], seriesBookIds[2]); keep the existing DB checks but
add this rendered-output assertion so the test fails if same-series siblings are
not prepended.
---
Nitpick comments:
In `@app/Controllers/FrontendController.php`:
- Around line 1785-1795: In getRelatedBooks, add warning logs when
$db->prepare(...) returns false so prepare failures aren't silent: after each
prepare check where currently "if ($stmt) { ... }" add an else branch that logs
a warning including the query context and $db->error (or the DB connection's
error property) so you capture the failure reason; apply the same pattern to the
other two prepare blocks referenced in the method (the same-author and the two
other prepares around the method) so all prepare failures are logged with
identifying context.
In `@app/Views/libri/scheda_libro.php`:
- Around line 1547-1560: Replace hardcoded API paths used in the fetch calls
(e.g., '/admin/libri/volumi/add' and '/admin/libri/volumi/remove') by exposing
server-generated route URLs to JS and using those variables; add a small script
in the view that sets window.PINAKES_ROUTES = { volumiAdd: <?=
json_encode(url('/admin/libri/volumi/add'), JSON_HEX_TAG) ?>, volumiRemove: <?=
json_encode(url('/admin/libri/volumi/remove'), JSON_HEX_TAG) ?> } and then
update the fetch in the relevant functions (the code block that calls
fetch((window.BASE_PATH || '') + '/admin/libri/volumi/add', ...) and the
counterpart remove call) to use window.PINAKES_ROUTES.volumiAdd and
window.PINAKES_ROUTES.volumiRemove respectively so routes remain consistent and
safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e60c1a90-ba72-48b9-bee4-fa39c9f74512
📒 Files selected for processing (25)
app/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PublicApiController.phpapp/Routes/web.phpapp/Views/collane/dettaglio.phpapp/Views/collane/index.phpapp/Views/frontend/book-detail.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/classes/Installer.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/collane-management.spec.jstests/issue-75-issn-series-volumes.spec.jsvendor/composer/autoload_classmap.phpvendor/composer/autoload_static.phpvendor/composer/installed.jsonvendor/composer/installed.phpversion.json
💤 Files with no reviewable changes (2)
- vendor/composer/autoload_classmap.php
- vendor/composer/autoload_static.php
Comment on lines
+15
to
+17
| <li><a href="<?= htmlspecialchars(url('/admin/dashboard'), ENT_QUOTES, 'UTF-8') ?>" class="text-gray-500 hover:text-gray-700 transition-colors"><i class="fas fa-home mr-1"></i>Home</a></li> | ||
| <li><i class="fas fa-chevron-right text-gray-400 text-xs"></i></li> | ||
| <li><a href="<?= htmlspecialchars(url('/admin/collane'), ENT_QUOTES, 'UTF-8') ?>" class="text-gray-500 hover:text-gray-700 transition-colors"><?= __("Collane") ?></a></li> |
There was a problem hiding this comment.
Switch these collane admin URLs to route keys.
The breadcrumb, the book link, and every form action here hardcode /admin/..., so any translated or remapped route leaves this page with stale links/actions. Use route_path(...) or RouteTranslator::route(...) and keep the current attribute escaping.
As per coding guidelines, "Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login."
Also applies to: 47-47, 74-74, 91-91, 104-104, 119-119, 136-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/collane/dettaglio.php` around lines 15 - 17, The breadcrumb and
other links in dettaglio.php are using hardcoded url('/admin/...') paths;
replace these with route helpers such as route_path('admin.dashboard') or
RouteTranslator::route('admin.dashboard') (and the appropriate route keys for
collane, book show, and each form action referenced at lines
~47,74,91,104,119,136) while preserving the existing htmlspecialchars(...,
ENT_QUOTES, 'UTF-8') escaping and CSS/class attributes; ensure you update href
attributes and form action attributes to use the route helper functions
(route_path(...) or RouteTranslator::route(...)) instead of url('/admin/...') so
remapped/translatable routes resolve correctly.
Comment on lines
+265
to
+266
| <a class="nav-link group flex items-center px-4 py-3 rounded-lg transition-all duration-200 hover:bg-gray-100 text-gray-700 hover:text-gray-900" | ||
| href="<?= htmlspecialchars(url('/admin/collane'), ENT_QUOTES, 'UTF-8') ?>"> |
There was a problem hiding this comment.
Use the collane route key here.
This new nav item hardcodes /admin/collane, so localized/custom route mappings can drift out of sync with the rest of the app. Generate the href with route_path(...) or RouteTranslator::route(...) and keep the existing htmlspecialchars(..., ENT_QUOTES, 'UTF-8').
As per coding guidelines, "Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths like /accedi or /login."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/layout.php` around lines 265 - 266, The anchor currently hardcodes
"/admin/collane"; replace the hardcoded path with the localized route helper
(e.g. call route_path('collane') or RouteTranslator::route('collane')) inside
the existing htmlspecialchars(...) call so the href uses the route key 'collane'
and still passes ENT_QUOTES and 'UTF-8'; keep the rest of the anchor attributes
and classes unchanged.
Comment on lines
+27
to
+35
| async function loginAsAdmin(page) { | ||
| await page.goto(`${BASE}/admin/dashboard`); | ||
| const emailField = page.locator('input[name="email"]'); | ||
| if (await emailField.isVisible({ timeout: 3000 }).catch(() => false)) { | ||
| await emailField.fill(ADMIN_EMAIL); | ||
| await page.fill('input[name="password"]', ADMIN_PASS); | ||
| await page.click('button[type="submit"]'); | ||
| await page.waitForURL(/admin/, { timeout: 15000 }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files tests/ | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 561
🏁 Script executed:
cat -n tests/collane-management.spec.js | head -80Repository: fabiodalez-dev/Pinakes
Length of output: 3551
🏁 Script executed:
# Search for BASE URL definition and login patterns
rg "BASE|login|admin" tests/collane-management.spec.js -nRepository: fabiodalez-dev/Pinakes
Length of output: 905
🏁 Script executed:
# Check if there's a run-e2e.sh or test setup file
find . -name "run-e2e.sh" -o -name "*setup*" -o -name "*.config.*" | grep -E "(test|e2e|setup|config)" | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 237
Fix overly broad URL matcher in login helper.
The regex /admin/ can match login pages (e.g., /admin/login, /admin/) if authentication fails silently, leaving subsequent tests unauthenticated while tests continue. Wait specifically for /admin/dashboard to ensure actual authenticated access.
Suggested fix
- await page.waitForURL(/admin/, { timeout: 15000 });
+ await page.waitForURL(/\/admin\/dashboard(?:\?.*)?$/, { timeout: 15000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/collane-management.spec.js` around lines 27 - 35, The login helper
function loginAsAdmin uses a too-broad waitForURL(/admin/) which can match the
login page and allow tests to proceed unauthenticated; change the waitForURL
call in loginAsAdmin to wait for the exact dashboard path (e.g.,
'/admin/dashboard' or a strict regex matching that full path) so the helper only
returns after successful navigation to the authenticated admin dashboard.
Comment on lines
+44
to
+46
| const TEST_COLLANA = 'E2E Test Series'; | ||
| const TEST_DESC = 'A test series for E2E testing'; | ||
|
|
There was a problem hiding this comment.
Namespace the seeded rows per run.
This suite uses fixed titles/series names and then finds rows with SELECT MAX(id) or deletes by name. On a reused DB, stale rows or another run can make later assertions target the wrong records and can wipe unrelated data during cleanup. Generate a per-run suffix and clean up by tracked ids only.
Also applies to: 53-58, 89-90, 121-123, 148-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/collane-management.spec.js` around lines 44 - 46, The tests use fixed
constants TEST_COLLANA and TEST_DESC and rely on SELECT MAX(id) or deleting by
name which breaks on reused DBs; change TEST_COLLANA and TEST_DESC to include a
unique per-run suffix (e.g., timestamp or UUID) at creation time, capture and
store the actual inserted row ids returned by the insert operations (use those
ids for subsequent SELECTs and assertions instead of SELECT MAX(id)), and modify
cleanup to delete only rows whose ids are in the tracked-id list rather than
deleting by title/name; update all places that reference TEST_COLLANA/TEST_DESC
or use MAX(id) (the constants and the insert/select/delete logic) to use the
per-run suffixed values and tracked ids for assertions and teardown.
- collane.nome VARCHAR(100) to match libri.collana (was 255) - removeVolume: return 500 on prepare failure (was silent success) - addVolume: already uses SecureLogger (confirmed) - confirm() in dettaglio.php: use json_encode for JS safety - Bulk assign: check resp.ok before showing success - rename(): sync collane table nome on rename - merge(): delete source collane entry after merge - Remove 5 duplicate JSON keys in en_US.json and de_DE.json - vendor/composer: regenerated with --no-dev (dev=>false)
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/PublicApiController.php (1)
339-353:⚠️ Potential issue | 🟠 MajorMissing approval status filter leaks unapproved reviews.
The public API returns all reviews regardless of moderation state. The
RecensioniRepository::getApprovedReviewsForBookfilters bystato = 'approvata', but this endpoint does not. Pending or rejected reviews will be exposed publicly.🔒 Proposed fix: add status filter
FROM recensioni r JOIN utenti u ON r.utente_id = u.id WHERE r.libro_id = ? + AND r.stato = 'approvata' ORDER BY r.created_at DESC🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/PublicApiController.php` around lines 339 - 353, The SQL in PublicApiController that selects reviews (the $sql string) currently returns all recensioni for a libro_id and thus leaks unapproved reviews; update the query used in the PublicApiController method that returns book reviews to include the approval filter (e.g. add "AND r.stato = 'approvata'" to the WHERE clause or equivalent parameterized status check) so it matches RecensioniRepository::getApprovedReviewsForBook, and ensure the status value is passed/bound securely if using a parameter.
♻️ Duplicate comments (5)
app/Views/layout.php (1)
265-266:⚠️ Potential issue | 🟠 MajorUse the route helper for the Collane nav item.
Hardcoding
/admin/collanebypasses localized/custom route mappings. Keep the existing escaping, but generate this href from the collane route key instead, e.g.htmlspecialchars(route_path('collane'), ENT_QUOTES, 'UTF-8').As per coding guidelines "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login" and "Usehtmlspecialchars()with ENT_QUOTES and UTF-8 encoding forroute_path()calls in HTML attributes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/layout.php` around lines 265 - 266, Replace the hardcoded href "/admin/collane" with a call to the route helper so localized/custom routes are respected: use htmlspecialchars(route_path('collane'), ENT_QUOTES, 'UTF-8') (or RouteTranslator::route('collane') if your app uses that translator) in the anchor's href while keeping the existing class and escaping; update the href in the anchor element that currently generates the Collane nav item to call route_path('collane') wrapped with htmlspecialchars as specified.installer/database/migrations/migrate_0.5.1.sql (1)
13-33:⚠️ Potential issue | 🟠 MajorCreate-if-missing will not repair earlier
0.5.1drafts.Once
volumiorcollanealready exists, these blocks short-circuit toSELECT 1, so any shared database that created an earlier revision of this same migration will never receive the corrected key/constraint definitions. Please move the corrective DDL into explicitALTER TABLEsteps in a follow-up migration, or confirm0.5.1has not been applied anywhere persistent.Also applies to: 39-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/migrations/migrate_0.5.1.sql` around lines 13 - 33, The migration currently uses CREATE TABLE-if-missing logic for volumi (and similarly for collane) which skips when the table already exists and therefore never repairs earlier 0.5.1 drafts; update the fix by creating a new follow-up migration that runs explicit ALTER TABLE statements to add/modify the UNIQUE KEY uk_volume_id, the INDEX idx_opera, the foreign key constraints fk_volumi_opera and fk_volumi_volume, and the CHECK chk_volumi_not_self (and the analogous constraints for collane) so existing tables get corrected even if they were created by an earlier draft; ensure the new migration targets the same table names (volumi, collane) and uses ALTER TABLE ... ADD/ADD CONSTRAINT/ADD UNIQUE/ADD INDEX or MODIFY as appropriate rather than CREATE TABLE wrapped in an IF-exists SELECT 1.app/Views/collane/index.php (2)
11-11:⚠️ Potential issue | 🟠 MajorReplace hardcoded admin URLs with route keys.
Line 11, Line 45, Line 97, and Line 114 still hardcode
/admin/...paths. Switch these toroute_path('...')(orRouteTranslator::route('...')) and keep escaping in HTML attributes.♻️ Suggested update pattern
- <li><a href="<?= htmlspecialchars(url('/admin/dashboard'), ENT_QUOTES, 'UTF-8') ?>" class="text-gray-500 hover:text-gray-700 transition-colors"><i class="fas fa-home mr-1"></i>Home</a></li> + <li><a href="<?= htmlspecialchars(route_path('admin.dashboard'), ENT_QUOTES, 'UTF-8') ?>" class="text-gray-500 hover:text-gray-700 transition-colors"><i class="fas fa-home mr-1"></i>Home</a></li> - const resp = await fetch((window.BASE_PATH || '') + '/admin/collane/crea', { + const resp = await fetch(<?= json_encode(route_path('admin.collane.create'), JSON_HEX_TAG) ?>, { - <a href="<?= htmlspecialchars(url('/admin/collane/dettaglio?nome=' . urlencode($c['collana'])), ENT_QUOTES, 'UTF-8') ?>" class="text-gray-900 hover:text-gray-700 font-medium"> + <a href="<?= htmlspecialchars(route_path('admin.collane.show') . '?nome=' . urlencode($c['collana']), ENT_QUOTES, 'UTF-8') ?>" class="text-gray-900 hover:text-gray-700 font-medium"> - <a href="<?= htmlspecialchars(url('/admin/collane/dettaglio?nome=' . urlencode($c['collana'])), ENT_QUOTES, 'UTF-8') ?>" class="text-sm text-gray-500 hover:text-gray-700"> + <a href="<?= htmlspecialchars(route_path('admin.collane.show') . '?nome=' . urlencode($c['collana']), ENT_QUOTES, 'UTF-8') ?>" class="text-sm text-gray-500 hover:text-gray-700">As per coding guidelines, "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login," and usehtmlspecialchars(..., ENT_QUOTES, 'UTF-8')forroute_path()in HTML attributes.Also applies to: 45-45, 97-97, 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/collane/index.php` at line 11, Replace the hardcoded admin URLs (e.g., "/admin/dashboard" and the other three occurrences) with the route helper (route_path('...') or RouteTranslator::route('...')) and keep the existing HTML escaping via htmlspecialchars(..., ENT_QUOTES, 'UTF-8'); locate the anchor lines that currently use the literal "/admin/..." and substitute the literal string with a call to route_path using the appropriate route key (for example "admin.dashboard") while preserving the surrounding attributes and class names.
107-108:⚠️ Potential issue | 🟡 MinorUse explicit null checks for volume bounds.
Line 107 uses truthy checks, so
0values are treated as absent and the range is hidden.✅ Suggested fix
- <?php if ($c['min_num'] && $c['max_num']): ?> + <?php if ($c['min_num'] !== null && $c['max_num'] !== null): ?> <?= (int) $c['min_num'] ?> – <?= (int) $c['max_num'] ?> <?php else: ?>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/collane/index.php` around lines 107 - 108, The conditional currently uses truthy checks so 0 is treated as absent; update the if that references $c['min_num'] and $c['max_num'] to use explicit existence/null checks (for example replace if ($c['min_num'] && $c['max_num']) with if ($c['min_num'] !== null && $c['max_num'] !== null) or use isset($c['min_num']) && isset($c['max_num'])), leaving the displayed casts (int) $c['min_num'] and (int) $c['max_num'] unchanged.app/Controllers/CollaneController.php (1)
184-192:⚠️ Potential issue | 🟡 MinorSuccess message shown even if database operation failed.
If
$db->prepare()returnsfalse(e.g., connection issue), the code skips the execution block but still sets a success message at Line 191, misleading the user.🛡️ Suggested guard
$stmt = $db->prepare("INSERT INTO collane (nome, descrizione) VALUES (?, ?) ON DUPLICATE KEY UPDATE descrizione = VALUES(descrizione)"); - if ($stmt) { + if (!$stmt) { + $_SESSION['error_message'] = __('Errore database'); + return $response->withHeader('Location', url('/admin/collane/dettaglio?nome=' . urlencode($nome)))->withStatus(302); + } $stmt->bind_param('ss', $nome, $descrizione); $stmt->execute(); $stmt->close(); - } $_SESSION['success_message'] = __('Descrizione salvata');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 184 - 192, The success message is set even when the DB operation fails because $db->prepare() may return false; update the CollaneController logic to check the outcome of prepare() and execute() before setting $_SESSION['success_message' ]: if $stmt is false or $stmt->execute() returns false, capture $db->error or $stmt->error, set an error message (or error flash) and avoid the success message, ensure you still close $stmt when valid, and only perform the redirect with the success flash after a confirmed successful execute; use the existing $db->prepare, $stmt->execute, $stmt->close, and $_SESSION['success_message']/error variables to locate and modify the code.
🧹 Nitpick comments (2)
tests/issue-75-issn-series-volumes.spec.js (1)
217-223: Fallback to DB checks masks frontend routing/template regressions.Tests 6 and 11 fall back to database assertions when the constructed URL returns a non-200 status. This means the test passes even when the public book detail page is broken or the URL pattern is wrong. Consider failing the test explicitly when the expected frontend page is unreachable, then fixing the URL construction.
♻️ Suggested approach
- if (detailResp.status() === 200) { - const html = await detailResp.text(); - expect(html).toContain(SERIES_NAME); - expect(html).toContain('E2E Series Book 2'); - expect(html).toContain('E2E Series Book 3'); - } else { - // Alternative: check DB directly that the series query would return results - const siblings = dbQuery( - `SELECT COUNT(*) FROM libri WHERE collana='${SERIES_NAME}' AND id != ${bookId} AND deleted_at IS NULL` - ); - expect(parseInt(siblings)).toBe(2); - } + expect(detailResp.status()).toBe(200); + const html = await detailResp.text(); + expect(html).toContain(SERIES_NAME); + expect(html).toContain('E2E Series Book 2'); + expect(html).toContain('E2E Series Book 3');If the URL pattern is uncertain, resolve it dynamically (e.g., from the admin page or sitemap) rather than silently accepting failures.
Also applies to: 332-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 217 - 223, The test currently falls back to a DB assertion (dbQuery with SERIES_NAME and bookId and expect(parseInt(siblings)).toBe(2)) when the constructed URL returns a non-200, which masks frontend routing/template regressions; change the test so it first asserts the HTTP response status is 200 and fails the test if not (do not silently run the DB check), and if the URL pattern is uncertain implement a dynamic resolution step (e.g., fetch the correct book URL from the admin/sitemap) before asserting the page contents; only use a DB-based assertion as an explicit, documented fallback that still marks the test as failed when the frontend is unreachable.app/Controllers/CollaneController.php (1)
125-133:INSERT IGNOREsilently ignores duplicates but still reports success.If a collana with this name already exists,
INSERT IGNOREdoes nothing, yet Line 132 shows a success message claiming the series was "created". Consider checking$stmt->affected_rowsto provide accurate feedback.♻️ Suggested improvement
$stmt = $db->prepare("INSERT IGNORE INTO collane (nome) VALUES (?)"); if ($stmt) { $stmt->bind_param('s', $nome); $stmt->execute(); + $created = $stmt->affected_rows > 0; $stmt->close(); } - $_SESSION['success_message'] = sprintf(__('Collana "%s" creata'), $nome); + if ($created ?? false) { + $_SESSION['success_message'] = sprintf(__('Collana "%s" creata'), $nome); + } else { + $_SESSION['info_message'] = sprintf(__('Collana "%s" già esistente'), $nome); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 125 - 133, The current INSERT IGNORE into the collane table can succeed without inserting when a duplicate exists, but the controller unconditionally sets $_SESSION['success_message'] and redirects as if a new collana was created; update the logic after $stmt->execute() to check $stmt->affected_rows (or $db->affected_rows) and only set $_SESSION['success_message'] and redirect to collane/dettaglio when affected_rows > 0, otherwise set an appropriate error or info message (e.g. "Collana already exists") and handle the redirect/response accordingly; ensure you reference the existing $stmt prepared via prepare("INSERT IGNORE INTO collane (nome) VALUES (?)"), the $nome variable, and close $stmt as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/FrontendController.php`:
- Around line 661-668: The ORDER BY in the $stmtSeries query treats
blank/non-numeric libri.numero_serie as 0 via CAST(... AS UNSIGNED), misordering
series; update the SQL prepared in FrontendController.php (the $stmtSeries
declaration) to explicitly detect numeric series and sort numeric values first
then non-numeric — e.g. use a numeric-check expression (like numero_serie REGEXP
'^[0-9]+' or TRIM(numero_serie) REGEXP ...) to order numeric rows before others,
then CAST(numero_serie AS UNSIGNED) for the numeric ordering and finally
l.titolo for tie-breaks so empty/blank/'Speciale' values don’t appear before
volume 1.
In `@app/Views/libri/index.php`:
- Line 1173: Replace the hardcoded fetch URL in the fetch call (the line that
constructs '/api/collane/search?q=...') with a routed URL emitted from PHP using
route_path('...') (or RouteTranslator::route('...')); e.g., generate a JS
variable at the top of the script block like const COLLANA_SEARCH_URL = '<?=
route_path("collane.search") ?>' and then use COLLANA_SEARCH_URL + '?q=' +
encodeURIComponent(q) in the fetch call so the code uses the
route_path-generated path instead of the hardcoded '/api/collane/search'.
In `@installer/database/schema.sql`:
- Line 58: The schema change shrank columns like `nome` to VARCHAR(100) without
a migration or input validation; either revert these columns back to 255 or add
an ALTER TABLE step to migrate existing DBs in migrate_0.5.1.sql (and any other
relevant migration files) and add server-side validation in the create/update
flows for authors, publishers, shelves, and branches to reject >100 char names.
Update the migration(s) to ALTER TABLE to change the column definitions and
update the API/model validation logic (the author/publisher/shelf/branch
create/update handlers) to enforce and return a clear validation error when
inputs exceed the new limit.
In `@locale/en_US.json`:
- Line 2803: Update the English translation value for the key "Cerca libro" in
locale/en_US.json to use more natural phrasing; replace the current value
"Search book" with "Search for a book" so the action label reads correctly in
the UI.
---
Outside diff comments:
In `@app/Controllers/PublicApiController.php`:
- Around line 339-353: The SQL in PublicApiController that selects reviews (the
$sql string) currently returns all recensioni for a libro_id and thus leaks
unapproved reviews; update the query used in the PublicApiController method that
returns book reviews to include the approval filter (e.g. add "AND r.stato =
'approvata'" to the WHERE clause or equivalent parameterized status check) so it
matches RecensioniRepository::getApprovedReviewsForBook, and ensure the status
value is passed/bound securely if using a parameter.
---
Duplicate comments:
In `@app/Controllers/CollaneController.php`:
- Around line 184-192: The success message is set even when the DB operation
fails because $db->prepare() may return false; update the CollaneController
logic to check the outcome of prepare() and execute() before setting
$_SESSION['success_message' ]: if $stmt is false or $stmt->execute() returns
false, capture $db->error or $stmt->error, set an error message (or error flash)
and avoid the success message, ensure you still close $stmt when valid, and only
perform the redirect with the success flash after a confirmed successful
execute; use the existing $db->prepare, $stmt->execute, $stmt->close, and
$_SESSION['success_message']/error variables to locate and modify the code.
In `@app/Views/collane/index.php`:
- Line 11: Replace the hardcoded admin URLs (e.g., "/admin/dashboard" and the
other three occurrences) with the route helper (route_path('...') or
RouteTranslator::route('...')) and keep the existing HTML escaping via
htmlspecialchars(..., ENT_QUOTES, 'UTF-8'); locate the anchor lines that
currently use the literal "/admin/..." and substitute the literal string with a
call to route_path using the appropriate route key (for example
"admin.dashboard") while preserving the surrounding attributes and class names.
- Around line 107-108: The conditional currently uses truthy checks so 0 is
treated as absent; update the if that references $c['min_num'] and $c['max_num']
to use explicit existence/null checks (for example replace if ($c['min_num'] &&
$c['max_num']) with if ($c['min_num'] !== null && $c['max_num'] !== null) or use
isset($c['min_num']) && isset($c['max_num'])), leaving the displayed casts (int)
$c['min_num'] and (int) $c['max_num'] unchanged.
In `@app/Views/layout.php`:
- Around line 265-266: Replace the hardcoded href "/admin/collane" with a call
to the route helper so localized/custom routes are respected: use
htmlspecialchars(route_path('collane'), ENT_QUOTES, 'UTF-8') (or
RouteTranslator::route('collane') if your app uses that translator) in the
anchor's href while keeping the existing class and escaping; update the href in
the anchor element that currently generates the Collane nav item to call
route_path('collane') wrapped with htmlspecialchars as specified.
In `@installer/database/migrations/migrate_0.5.1.sql`:
- Around line 13-33: The migration currently uses CREATE TABLE-if-missing logic
for volumi (and similarly for collane) which skips when the table already exists
and therefore never repairs earlier 0.5.1 drafts; update the fix by creating a
new follow-up migration that runs explicit ALTER TABLE statements to add/modify
the UNIQUE KEY uk_volume_id, the INDEX idx_opera, the foreign key constraints
fk_volumi_opera and fk_volumi_volume, and the CHECK chk_volumi_not_self (and the
analogous constraints for collane) so existing tables get corrected even if they
were created by an earlier draft; ensure the new migration targets the same
table names (volumi, collane) and uses ALTER TABLE ... ADD/ADD CONSTRAINT/ADD
UNIQUE/ADD INDEX or MODIFY as appropriate rather than CREATE TABLE wrapped in an
IF-exists SELECT 1.
---
Nitpick comments:
In `@app/Controllers/CollaneController.php`:
- Around line 125-133: The current INSERT IGNORE into the collane table can
succeed without inserting when a duplicate exists, but the controller
unconditionally sets $_SESSION['success_message'] and redirects as if a new
collana was created; update the logic after $stmt->execute() to check
$stmt->affected_rows (or $db->affected_rows) and only set
$_SESSION['success_message'] and redirect to collane/dettaglio when
affected_rows > 0, otherwise set an appropriate error or info message (e.g.
"Collana already exists") and handle the redirect/response accordingly; ensure
you reference the existing $stmt prepared via prepare("INSERT IGNORE INTO
collane (nome) VALUES (?)"), the $nome variable, and close $stmt as currently
done.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 217-223: The test currently falls back to a DB assertion (dbQuery
with SERIES_NAME and bookId and expect(parseInt(siblings)).toBe(2)) when the
constructed URL returns a non-200, which masks frontend routing/template
regressions; change the test so it first asserts the HTTP response status is 200
and fails the test if not (do not silently run the DB check), and if the URL
pattern is uncertain implement a dynamic resolution step (e.g., fetch the
correct book URL from the admin/sitemap) before asserting the page contents;
only use a DB-based assertion as an explicit, documented fallback that still
marks the test as failed when the frontend is unreachable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 737aaa2f-030b-4e81-b6e2-d03e9b7e9192
📒 Files selected for processing (24)
app/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PublicApiController.phpapp/Routes/web.phpapp/Views/collane/dettaglio.phpapp/Views/collane/index.phpapp/Views/frontend/book-detail.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/classes/Installer.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/collane-management.spec.jstests/issue-75-issn-series-volumes.spec.jsvendor/composer/autoload_classmap.phpvendor/composer/autoload_static.phpvendor/composer/installed.phpversion.json
| const q = inp.value.trim(); | ||
| if (q.length < 1) { res.textContent = ''; return; } | ||
| try { | ||
| const r = await fetch((window.BASE_PATH || '') + '/api/collane/search?q=' + encodeURIComponent(q)); |
There was a problem hiding this comment.
Replace hardcoded search endpoint with a routed path.
Line 1173 uses a hardcoded URL /api/collane/search. Per project guidelines, use route_path() to generate URLs so they stay in sync with route definitions.
🔧 Suggested change
Emit the URL from PHP at the top of the script block, then reference it in JS:
+ const collaneSearchUrl = <?= json_encode(url(route_path('api.collane.search')), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT) ?>;
// Bulk assign collana
...
- const r = await fetch((window.BASE_PATH || '') + '/api/collane/search?q=' + encodeURIComponent(q));
+ const r = await fetch(collaneSearchUrl + '?q=' + encodeURIComponent(q));As per coding guidelines: "Never use hardcoded routes. Use route_path('key') or RouteTranslator::route('key') instead of hardcoded paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Views/libri/index.php` at line 1173, Replace the hardcoded fetch URL in
the fetch call (the line that constructs '/api/collane/search?q=...') with a
routed URL emitted from PHP using route_path('...') (or
RouteTranslator::route('...')); e.g., generate a JS variable at the top of the
script block like const COLLANA_SEARCH_URL = '<?= route_path("collane.search")
?>' and then use COLLANA_SEARCH_URL + '?q=' + encodeURIComponent(q) in the fetch
call so the code uses the route_path-generated path instead of the hardcoded
'/api/collane/search'.
installer/database/schema.sql
Outdated
| `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| `updated_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, | ||
| `nome` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| `nome` varchar(100) COLLATE utf8mb4_unicode_ci NOT NULL, |
There was a problem hiding this comment.
These VARCHAR(100) shrinks need a matching upgrade path and validation.
Fresh installs now cap author, publisher, shelf, and branch names at 100 chars, but migrate_0.5.1.sql does not alter existing databases and the current create/update paths shown for authors, publishers, and shelves still pass longer values through unchanged. That makes saves installation-dependent and can turn previously valid input into truncation/errors on fresh installs. Either keep these columns at 255, or ship the matching ALTER TABLE steps and request validation for the new limit.
Also applies to: 151-151, 739-739, 754-754
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@installer/database/schema.sql` at line 58, The schema change shrank columns
like `nome` to VARCHAR(100) without a migration or input validation; either
revert these columns back to 255 or add an ALTER TABLE step to migrate existing
DBs in migrate_0.5.1.sql (and any other relevant migration files) and add
server-side validation in the create/update flows for authors, publishers,
shelves, and branches to reject >100 char names. Update the migration(s) to
ALTER TABLE to change the column definitions and update the API/model validation
logic (the author/publisher/shelf/branch create/update handlers) to enforce and
return a clear validation error when inputs exceed the new limit.
- Set copie_disponibili=0 when creating parent work from collana (was inheriting default 1, making parent appear "available") - Add AND r.stato='approvata' to public reviews API (was leaking unapproved reviews) - Revert accidental VARCHAR(255→100) shrink on autori.nome, editori.nome, scaffali.nome, sedi.nome caused by overly broad sed
…tions - ISSN: return explicit validation error instead of silently discarding invalid values (both store and update paths) - Collane index: UNION with collane table so empty series remain visible - Add 8 missing translation keys to EN and DE locale files - Add translations for cycle prevention and ISSN error messages
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
installer/database/schema.sql (1)
393-424:⚠️ Potential issue | 🟠 MajorAdd DB-level integrity between
libri.collanaandcollane.nome.Right now
collane.nomelooks like the canonical series key, butlibri.collanais still unconstrained free text. That already lets write paths like merge move books to a target name that has nocollanerow, leaving orphaned series values and detached metadata. Either add a foreign key (ON UPDATE CASCADE/ON DELETE SET NULL) or make every write path create/validate thecollanerow transactionally.Also applies to: 883-891
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@installer/database/schema.sql` around lines 393 - 424, libri.collana currently stores free text while collane.nome is the canonical series name, causing orphaned series values; add a DB-level foreign key from libri.collana to collane.nome (or collane.nome's primary/unique key) with ON UPDATE CASCADE and ON DELETE SET NULL to maintain integrity, or alternatively update the write paths (functions that insert/update libri rows) to transactionally create/validate collane rows before saving (ensure referential consistency in code paths that touch libri.collana). Refer to the libri table definition (columns `collana`, `id`, and existing KEY `idx_collana`) and the collane table's `nome` unique key when implementing the FK or the transactional creation/validation.
♻️ Duplicate comments (17)
locale/en_US.json (1)
2803-2803:⚠️ Potential issue | 🟡 MinorPolish the English action label at Line 2803.
"Search book"is awkward in English UI copy; use"Search for a book".✏️ Suggested text fix
- "Cerca libro": "Search book", + "Cerca libro": "Search for a book",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locale/en_US.json` at line 2803, Update the English translation value for the JSON key "Cerca libro": replace the current value "Search book" with the improved copy "Search for a book" in locale/en_US.json so the UI reads naturally; locate the entry by the key "Cerca libro" and change only the string value.app/Views/collane/dettaglio.php (1)
15-17:⚠️ Potential issue | 🟠 MajorReplace the hardcoded admin URLs with route keys.
Every navigation/form target here bakes in
/admin/..., so any translated or remapped route will leave this page with stale links and actions. Please switch these to the appropriateroute_path(...)orRouteTranslator::route(...)helper and keep the current attribute escaping.As per coding guidelines: "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login".Also applies to: 47-47, 74-74, 91-91, 104-104, 119-119, 136-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/collane/dettaglio.php` around lines 15 - 17, The template currently uses hardcoded admin URLs via url('/admin/dashboard') and url('/admin/collane') inside the htmlspecialchars calls; replace these with the framework route helpers (e.g., route_path('admin.dashboard') or RouteTranslator::route('admin.dashboard')) while preserving the existing htmlspecialchars(..., ENT_QUOTES, 'UTF-8') escaping and the surrounding link attributes and classes; apply the same replacement for all other hardcoded '/admin/...' instances noted (lines near the url(...) calls and the locations you listed) so links use route keys instead of literal paths.tests/collane-management.spec.js (2)
44-58:⚠️ Potential issue | 🟠 MajorNamespace seeded rows and track inserted IDs per run.
This suite reuses fixed series/book names, looks rows up with
SELECT MAX(id), and cleans up by name/title. On a reused DB or a second run, stale rows can satisfy the assertions and teardown can delete someone else’s fixtures.Also applies to: 89-90, 121-123, 148-149, 171-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collane-management.spec.js` around lines 44 - 58, The tests use fixed names (TEST_COLLANA/TEST_DESC) and SELECT MAX(id) which can pick up preexisting rows; change test fixtures to generate unique names per run (e.g., append timestamp or UUID to TEST_COLLANA/TEST_DESC), create rows with INSERT ... RETURNING id (or read the inserted id via the API used) and store those IDs in variables, update any lookup code that currently uses SELECT MAX(id) to use the stored IDs instead, and update teardown to delete/update rows by those stored IDs (not by name) via dbExec so cleanup only removes the fixtures created by this run; apply same pattern for the other occurrences referenced (lines ~89-90, 121-123, 148-149, 171-172).
27-35:⚠️ Potential issue | 🟠 MajorWait for the authenticated dashboard, not any
/adminURL.
/admin/also matches login and intermediate admin pages, so this helper can return while the session is still unauthenticated and let the rest of the suite fail later for the wrong reason.🔧 Minimal fix
- await page.waitForURL(/admin/, { timeout: 15000 }); + await page.waitForURL(/\/admin\/dashboard(?:\?.*)?$/, { timeout: 15000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collane-management.spec.js` around lines 27 - 35, The helper loginAsAdmin currently waits for any /admin URL which can match login/intermediate pages; update the post-login wait to target the authenticated dashboard only by changing the waitForURL call in loginAsAdmin to wait for the specific dashboard route (e.g., "/admin/dashboard" or a stricter regex like /^\/admin\/dashboard/) with the same timeout so the function only returns once the real dashboard is loaded.app/Views/libri/partials/book_form.php (1)
208-210:⚠️ Potential issue | 🟠 MajorKeep the server-side ISSN rule identical to this UI contract.
These lines now require strict
XXXX-XXXX, butLibriController::store()andLibriController::update()still strip separators and accept compact ISSNs before reformatting them. Non-JS or direct submits can therefore bypass the rule this form advertises.Also applies to: 2895-2903
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/partials/book_form.php` around lines 208 - 210, The form enforces an ISSN format with a hyphen (XXXX-XXXX) but LibriController::store() and LibriController::update() currently strip separators and accept compact ISSNs; update the server-side validation to match the UI contract by rejecting compact ISSNs and requiring the dashed format instead. Specifically, in LibriController::store() and LibriController::update() replace the logic that strips separators/accepts compact values with a regex check (e.g. ^\d{4}-\d{3}[0-9Xx]$) and return a validation error when the ISSN does not match, and ensure the same validation is applied wherever ISSN handling is duplicated (the other referenced block).tests/issue-75-issn-series-volumes.spec.js (4)
31-39:⚠️ Potential issue | 🟠 MajorWait for a post-login-only location here.
/admin/loginstill matches the current regex, so a failed submit can satisfy the wait and leave the rest of the suite unauthenticated. Wait for/admin/dashboardor an admin-only marker instead.🔧 Suggested fix
- await page.waitForURL(/.*(?:dashboard|admin).*/, { timeout: 15000 }); + await page.waitForURL(/\/admin\/dashboard\/?$/, { timeout: 15000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 31 - 39, The post-login wait in loginAsAdmin is too permissive—login failure can still satisfy the regex; update the wait to assert an admin-only destination or marker after submit: in the loginAsAdmin function replace the page.waitForURL(/.*(?:dashboard|admin).*/, { timeout: 15000 }) with a strict check such as waiting for the exact /admin/dashboard URL or waiting for a known admin-only DOM marker (e.g., a selector visible only on the admin dashboard) to ensure the test is actually authenticated.
295-318:⚠️ Potential issue | 🟠 MajorThis still doesn’t verify the related-books priority.
The SQL checks only prove the siblings exist and that
idx_collanais available. They never assert thatPotrebbero interessartiactually starts with the same-series books, so the new priority-0 controller path can regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 295 - 318, The test currently only checks DB state and index usage but never asserts the actual ordering returned by the related-books logic; update the test to call the real function or endpoint that produces the "Potrebbero interessarti" list (e.g., invoke getRelatedBooks or the controller route that uses the priority-0 path) for the first series book and assert that the returned list begins with the same-series sibling IDs (use seriesBookIds and/or seriesBookIds[1], seriesBookIds[2] to validate the first N entries), ensuring the controller's priority-0 path still yields same-series books first.
141-156:⚠️ Potential issue | 🟠 MajorDon’t downgrade frontend assertions to DB-only checks.
These tests still pass when the public detail page is broken: Test 3 never loads it, and Tests 6/11 fall back to SQL assertions when the guessed URL fails. Resolve the real public href from the app itself—e.g. the admin “Visualizza” link—and assert the rendered HTML/DOM instead of silently switching to database checks.
Also applies to: 196-223, 323-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 141 - 156, The test "3. ISSN displayed on frontend book detail page" currently uses dbQuery fallbacks instead of loading the public detail page; update it to parse the admin response (resp from request.get(`${BASE}/admin/libri/${bookId}`)) to extract the real "Visualizza" href for the public book page (instead of constructing or skipping to DB checks), then perform a request.get to that extracted URL and assert the rendered HTML/DOM contains the expected ISSN (ISSN_VALUE); remove the silent DB-only assertions (issnDb/dbQuery for this check) and apply the same pattern wherever similar fallbacks appear (tests referencing seriesBookIds, request.get, dbQuery, and BASE) so tests always verify the live frontend output.
63-99:⚠️ Potential issue | 🟠 MajorThese fixtures are not isolated between runs.
The fixed author id
9991plus static titles/series names can collide with stale or real rows, and theFOREIGN_KEY_CHECKStoggles do nothing here because eachdbExec()call starts a newmysqlprocess. On a reused DB, this can attach the books to the wrong author, delete unrelated data, or leave leftovers behind for later runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/issue-75-issn-series-volumes.spec.js` around lines 63 - 99, The fixtures use a hardcoded author id (9991), static titles/series names and separate dbExec calls which breaks isolation—replace the fixed id by inserting the author via dbExec and capturing the real id with dbQuery/SELECT LAST_INSERT_ID() (or use a single connection/transaction), append a unique suffix to SERIES_NAME/titles (timestamp or random) so seriesBookIds/parentWorkId/volumeIds are unique, ensure all related INSERTs use the captured author id and push the actual returned libro ids into seriesBookIds/volumeIds, and in test.afterAll delete only those captured ids (without globally toggling FOREIGN_KEY_CHECKS across separate mysql invocations) or perform cleanup inside the same DB connection/transaction so FK checks take effect; update references to dbExec, dbQuery, seriesBookIds, parentWorkId, volumeIds accordingly.app/Views/libri/scheda_libro.php (1)
981-981:⚠️ Potential issue | 🟠 MajorReplace the new hardcoded multi-volume routes with named routes.
The parent-work links and the new fetch calls still inline
/admin/libri/...and/api/search/unified. That makes this feature drift from the router and will break as soon as those route definitions move or get translated. Please emit these URLs viaroute_path(...)/RouteTranslator::route(...), and pass the JS ones into the script as PHP-rendered constants instead of concatenating path strings here.As per coding guidelines: "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths like/accedior/login."Also applies to: 1017-1017, 1505-1505, 1548-1548, 1586-1586
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/libri/scheda_libro.php` at line 981, Replace hardcoded paths like url('/admin/libri/' . (int)$parentWork['id']) and any inline '/api/search/unified' fetch URLs by using the framework route helpers (route_path('route_key', ['id' => (int)$parentWork['id']]) or RouteTranslator::route('route_key')) for the anchor hrefs and server-side rendering of API endpoints; inject those API route strings into the page as PHP-rendered JS constants (e.g., a JS variable set from route_path) and update the client-side fetches to use those constants. Locate occurrences around the parent-work link generation (the anchor using url(...)), the client fetches that reference '/api/search/unified', and the other flagged spots, and replace string concatenation with the route helper calls so routes remain stable and translatable.app/Controllers/FrontendController.php (1)
661-667:⚠️ Potential issue | 🟡 MinorSort non-numeric
numero_serievalues after real volume numbers.
CAST(l.numero_serie AS UNSIGNED)turns''or labels likeSpecialeinto0, so those rows can jump ahead of volume 1. The new same-series strip and the priority-0 related-books list both inherit that misordering.🔧 Suggested fix
- ORDER BY CAST(l.numero_serie AS UNSIGNED), l.titolo + ORDER BY + CASE + WHEN TRIM(l.numero_serie) REGEXP '^[0-9]+$' THEN 0 + ELSE 1 + END, + CASE + WHEN TRIM(l.numero_serie) REGEXP '^[0-9]+$' THEN CAST(l.numero_serie AS UNSIGNED) + ELSE NULL + END, + l.numero_serie, + l.titolo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/FrontendController.php` around lines 661 - 667, The ORDER BY using CAST(l.numero_serie AS UNSIGNED) causes non-numeric values like '' or 'Speciale' to become 0 and sort before real volumes; update the SQL used where $stmtSeries is prepared in FrontendController.php to prefer numeric series first and then non-numeric labels by changing the ORDER BY to something like: first sort by whether numero_serie matches a numeric regex (e.g. numero_serie REGEXP '^[0-9]+') DESC, then by CAST(numero_serie AS UNSIGNED) ASC (for the numeric ones), and finally by l.titolo ASC to deterministically order non-numeric entries — adjust the query string assigned to $stmtSeries accordingly.app/Controllers/LibriController.php (2)
1996-2000:⚠️ Potential issue | 🟡 MinorValidate
numero_volumelower bound before insert.Line 1996 allows
0/negative values to pass request validation; reject them as400before DB write.Suggested fix
$numero = (int) ($data['numero_volume'] ?? 1); - if ($operaId <= 0 || $volumeId <= 0 || $operaId === $volumeId) { + if ($operaId <= 0 || $volumeId <= 0 || $operaId === $volumeId || $numero < 1) { $response->getBody()->write(json_encode(['error' => true, 'message' => __('Parametri non validi')], JSON_UNESCAPED_UNICODE)); return $response->withStatus(400)->withHeader('Content-Type', 'application/json'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 1996 - 2000, The code casts numero_volume to $numero but doesn't reject zero/negative values before DB write; update the validation (near the existing check that uses $operaId and $volumeId) to also reject $numero when $numero <= 0 by returning a 400 JSON response (same format as the other validation error). Locate the $numero assignment and the surrounding conditional (references: $numero, $data['numero_volume'], $operaId, $volumeId, $response) and add the $numero lower-bound check so invalid values are rejected before any insert.
528-536:⚠️ Potential issue | 🟡 MinorAvoid silently truncating volume lists at 100 rows.
Line 536 still hard-caps results with
LIMIT 100, so larger opere lose rows with no user signal.Suggested fix
$volStmt = $db->prepare(" SELECT v.numero_volume, v.titolo_volume, l.id, l.titolo, l.isbn13, l.isbn10, (SELECT a.nome FROM libri_autori la JOIN autori a ON la.autore_id = a.id WHERE la.libro_id = l.id AND la.ruolo = 'principale' LIMIT 1) AS autore FROM volumi v JOIN libri l ON v.volume_id = l.id AND l.deleted_at IS NULL WHERE v.opera_id = ? ORDER BY v.numero_volume - LIMIT 100 ");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 528 - 536, The query prepared in $volStmt contains a hard-coded "LIMIT 100" which silently truncates results for large opere; remove the fixed LIMIT or replace it with a paginated parameter so callers can request full result sets or specific pages. Update the SQL in $volStmt to either drop "LIMIT 100" entirely or change it to use bound parameters (e.g., "LIMIT ? OFFSET ?"), then modify the execute call that binds v.opera_id to also bind the new limit/offset variables (or expose a method parameter) so pagination or configurable limits are applied instead of the silent 100-row cap.app/Controllers/CollaneController.php (4)
400-416:⚠️ Potential issue | 🟠 Major
INSERT IGNOREsilently skips volumes that are already linked elsewhere.The
volumi.volume_idcolumn has a UNIQUE constraint (per schema). UsingINSERT IGNOREmeans if a book is already a volume of another parent work, the insert silently fails but$volNumstill increments. The success message will report an incorrect count.Consider checking
$stmtInsert->affected_rowsafter each insert, or use a regular INSERT and handle the duplicate key error to provide accurate feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 400 - 416, The INSERT IGNORE causes silent skips so $volNum increments even when no row is inserted; after calling $stmtInsert->execute() in the loop (where $stmtInsert, $parentId, $bookId, $num are used) check $stmtInsert->affected_rows and only increment $volNum (and count successes) when affected_rows === 1, or switch to a regular INSERT and catch duplicate-key errors (SQLSTATE 23000) to treat them as non-success without incrementing the success count; ensure the final success message uses the actual count of inserted rows rather than the loop iteration counter.
154-172:⚠️ Potential issue | 🟠 MajorWrap dependent writes in a transaction to ensure atomicity.
The
delete()method performs two dependent writes (UPDATElibri+ DELETEcollane) without transaction boundaries. If the DELETE fails after UPDATE succeeds, you'll have inconsistent state where books lost their collana reference but the collane row still exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 154 - 172, In the delete() method of CollaneController, wrap the two dependent DB writes (the UPDATE via $stmt and the DELETE via $stmt2 using $db and $nome) in a transaction: call $db->begin_transaction() before preparing/executing, only $db->commit() after both statements succeed, and call $db->rollback() if either prepare/execute fails; ensure $stmt and $stmt2 are properly closed on both success and failure, move setting $_SESSION['success_message'] to after commit (and set an error message before redirect on rollback), and return the redirect response in both cases.
284-292:⚠️ Potential issue | 🟠 MajorReturn error when database operation fails.
The method returns success JSON even when
prepare()fails because the success response at Line 291 is always reached. This hides database errors from the admin UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 284 - 292, The code always returns a success JSON even if the DB prepare/execute fails; update the CollaneController flow around the $stmt = $db->prepare(...) block to detect failures: if prepare() returns false, respond with a JSON error and non-200 status (use $db->error); after bind_param/execute check the boolean result of $stmt->execute() and if false return a JSON error using $stmt->error and an appropriate HTTP status via $response->withStatus(...); only return the success JSON when prepare and execute both succeed and ensure $stmt->close() still runs when applicable.
57-57:⚠️ Potential issue | 🟠 MajorUse named routes instead of hardcoded paths.
Multiple redirects throughout this controller use hardcoded URLs like
url('/admin/collane'). As per coding guidelines, useroute_path('key')orRouteTranslator::route('key')instead of hardcoded paths.This applies to Lines 57, 127, 138, 151, 172, 186, 197, 211, 232, 246, 267, 374, 381, 390, 417.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` at line 57, In CollaneController replace hardcoded redirect URLs like url('/admin/collane') with the named-route helper (e.g., route_path('admin.collane.index') or RouteTranslator::route('admin.collane.index')) in all controller methods that perform redirects (the redirect return statements in the class); find occurrences in methods that handle create/update/delete/list actions and change the response->withHeader('Location', ...) calls to use the named route helper so they generate URLs via route names instead of literal paths.
🧹 Nitpick comments (5)
app/Views/collane/index.php (1)
45-45: Consider using route helpers for admin paths.Multiple locations use hardcoded admin paths:
- Line 45:
'/admin/collane/crea'in JavaScript fetch- Lines 97 & 114:
url('/admin/collane/dettaglio?...')For consistency with the codebase guidelines, consider outputting route paths from PHP into JavaScript variables, and using route helpers for the PHP URLs.
Example approach for JavaScript route
<script> + const COLLANE_CREATE_URL = <?= json_encode(url('/admin/collane/crea'), JSON_HEX_TAG) ?>; async function createCollana() { // ... - const resp = await fetch((window.BASE_PATH || '') + '/admin/collane/crea', { + const resp = await fetch(COLLANE_CREATE_URL, {As per coding guidelines: "Never use hardcoded routes. Use
route_path('key')orRouteTranslator::route('key')instead of hardcoded paths."Also applies to: 97-97, 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Views/collane/index.php` at line 45, Replace hardcoded admin URLs with framework route helpers and expose JS routes from PHP: generate the create path with route_path (or RouteTranslator::route) and inject it into the page (e.g. a JS variable like COLLANA_CREa_PATH) then use that variable in the fetch call instead of '/admin/collane/crea'; similarly replace the PHP url('/admin/collane/dettaglio?...') usages by calling route_path('admin.collane.dettaglio', [...params...]) (or RouteTranslator::route('admin.collane.dettaglio')) to build the two dettaglio links; update index.php to output these helper-generated paths and reference them in the JS and hrefs (look for the fetch call and the two url(...) occurrences).app/Controllers/LibriController.php (1)
741-755: Extract ISSN normalization/validation into one shared method.The same ISSN logic is duplicated in
store()andupdate(). Centralizing it reduces drift risk and keeps behavior identical across create/edit paths.Refactor sketch
- // Normalize + validate ISSN (canonical format: XXXX-XXXX) - if (isset($fields['issn'])) { - ... - } + $fields['issn'] = $this->normalizeIssnOrRedirect( + $fields['issn'] ?? null, + $response, + '/admin/libri/crea' + ); - // Normalize + validate ISSN on update (same logic as create) - if (isset($fields['issn'])) { - ... - } + $fields['issn'] = $this->normalizeIssnOrRedirect( + $fields['issn'] ?? null, + $response, + '/admin/libri/modifica/' . $id + );private function normalizeIssn(string|null $raw): ?string { $issnRaw = trim((string) $raw); if ($issnRaw === '') { return null; } $issnCompact = strtoupper(str_replace('-', '', preg_replace('/\s+/', '', $issnRaw))); if (!preg_match('/^\d{7}[\dX]$/', $issnCompact)) { throw new \InvalidArgumentException('invalid_issn'); } return substr($issnCompact, 0, 4) . '-' . substr($issnCompact, 4, 4); }Also applies to: 1239-1253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/LibriController.php` around lines 741 - 755, Duplicate ISSN normalization/validation logic in store() and update() should be extracted into a single private helper (e.g., normalizeIssn) that accepts the raw value, returns the canonical ISSN string or null, and throws an \InvalidArgumentException on invalid input; replace the inline blocks in store() and update() with calls to normalizeIssn($fields['issn']) and handle the exception by setting $_SESSION['error_message'] and returning the same redirect response used today. Ensure the helper uses the same transformations and regex (/^\d{7}[\dX]$/) and canonicalizes to "XXXX-XXXX" so both create/edit paths behave identically.app/Controllers/CollaneController.php (1)
214-230: Consider wrapping rename operations in a transaction.The rename operation updates both
libri.collanaandcollane.nomein two separate statements. While less critical than delete (since partial failure is recoverable), a transaction would ensure atomicity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Controllers/CollaneController.php` around lines 214 - 230, Wrap the two update operations in an explicit transaction: call $db->begin_transaction() before preparing the statements in the CollaneController rename flow, execute both prepared statements (the UPDATE on libri and the UPDATE on collane), check their execution/affected_rows and only call $db->commit() if both succeed; on any failure call $db->rollback(), close prepared statements, and set the $_SESSION['success_message'] only after commit (or set an error message after rollback). Ensure you still bind_param and close both $stmt and $stmtSync, and handle exceptions/errors returned by $db or the statement execute calls so the transaction is correctly rolled back on failure.app/Routes/web.php (2)
1391-1396: Consider adding rate limiting to the collane list route for consistency.Other admin list routes (e.g.,
/admin/generiat line 1472,/admin/utentiat line 1022) include rate limiting middleware. The new collane routes lack this protection.♻️ Suggested fix
$app->get('/admin/collane', function ($request, $response) use ($app) { $controller = new \App\Controllers\CollaneController(); $db = $app->getContainer()->get('db'); return $controller->index($request, $response, $db); -})->add(new AdminAuthMiddleware()); +})->add(new \App\Middleware\RateLimitMiddleware(30, 60))->add(new AdminAuthMiddleware());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Routes/web.php` around lines 1391 - 1396, The /admin/collane route is missing the rate-limiting middleware used by other admin list routes; update the route definition that creates the CollaneController and adds AdminAuthMiddleware so it also adds the same RateLimitMiddleware instance (the same middleware class used on /admin/generi and /admin/utenti) to the middleware chain for that route, ensuring the route closure (where new \App\Controllers\CollaneController() is instantiated and ->add(new AdminAuthMiddleware()) is called) includes ->add(new RateLimitMiddleware(...)) or the equivalent factory call to match the existing routes' configuration.
1446-1450: Consider adding rate limiting to the search API endpoint.The
/api/collane/searchendpoint lacks rate limiting, unlike similar autocomplete endpoints (e.g.,/api/collocazione/suggerisciat line 1540 has rate limiting). This could allow excessive requests.♻️ Suggested fix
$app->get('/api/collane/search', function ($request, $response) use ($app) { $controller = new \App\Controllers\CollaneController(); $db = $app->getContainer()->get('db'); return $controller->searchApi($request, $response, $db); -})->add(new AdminAuthMiddleware()); +})->add(new \App\Middleware\RateLimitMiddleware(30, 60))->add(new AdminAuthMiddleware());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Routes/web.php` around lines 1446 - 1450, The /api/collane/search route currently lacks rate limiting; update the route definition that constructs CollaneController::searchApi to include the same rate-limiting middleware used by the similar endpoint (/api/collocazione/suggerisci). Locate the route where AdminAuthMiddleware is added and append or prepend the existing RateLimitMiddleware (or the concrete class used for the suggerisci route) with the same configuration (requests/period) so the search endpoint is subject to the same throttling policy; no changes to CollaneController::searchApi are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Controllers/CollaneController.php`:
- Around line 249-265: The merge() method performs two dependent writes ($stmt
updating libri and $stmtDel deleting from collane) so wrap these in a DB
transaction: begin a transaction on $db before preparing/executing $stmt, check
each prepare/execute result and on any failure rollback and set an error session
message, otherwise commit after both succeed; ensure you call
$db->begin_transaction(), $db->commit(), and $db->rollback() as appropriate and
close statements in both success and error paths so the UPDATE ($stmt) and
DELETE ($stmtDel) are atomic.
- Around line 130-138: The current INSERT IGNORE into collane silently succeeds
when a duplicate nome exists but still sets $_SESSION['success_message'] as if a
new collana was created; after executing the prepared statement
($stmt->execute()), check $stmt->affected_rows (or $db->affected_rows) to
determine if a row was inserted (affected_rows > 0) and set
$_SESSION['success_message'] to a "created" message only in that case, otherwise
set a different message like "Collana already exists" and still redirect to the
same dettaglio URL; also handle the case where prepare() returns false by
setting an error message or logging before redirecting.
In `@app/Controllers/LibriController.php`:
- Around line 2055-2060: The code always returns success after calling
$stmt->execute() in LibriController (the block using $stmt->bind_param('ii',
$operaId, $volumeId); $stmt->execute(); $stmt->close();) so failures are
reported as success; change the flow to check the boolean result of
$stmt->execute() (or $stmt->error/errno) before closing and only write/return
the JSON success when execute() returned true, otherwise return an error JSON
and appropriate HTTP status and include the statement error for
logging/response; ensure you reference the existing $stmt variable and its
execute(), error, and close() methods when implementing the check.
- Around line 2003-2014: The current cycle prevention in LibriController only
checks a direct reverse link using $cycleCheck with $volumeId and $operaId,
which misses longer cycles (e.g., A→B→C→A); modify the check to detect any
existing ancestor relationship by walking the parent chain from opera_id up to
root (or using a recursive CTE if MySQL 8+). Concretely: replace the single
SELECT COUNT(*) check with either (a) a recursive CTE query starting at opera_id
that traverses volumi.volume_id→opera_id edges and tests whether volumeId
appears in the result set, or (b) an iterative loop that queries the parent
(opera_id) of the current node until null/0 and compares each parent to
volumeId; keep using $volumeId/$operaId and ensure you abort and return the same
400 JSON response if an ancestor match is found.
In `@app/Views/libri/scheda_libro.php`:
- Around line 1505-1528: The search handler treats the unified-search response
as if it returns data.books/data.results of book objects and uses
b.title/titolo, but SearchController::unifiedSearch() returns a flat results
array of mixed items with keys label and type; update the code that builds books
(currently the const books = (data.books || data.results || []).filter(...)) to
instead read from data.results (or data.results || []) and filter items where
item.type === 'book' and item.id !== operaId, then render the book title from
item.label (falling back to item.titolo or item.title if present); keep existing
DOM logic (resultsDiv, row, selectVolume) but reference the filtered book items
and label for display.
In `@locale/de_DE.json`:
- Line 3472: Update the German translations to use imperative instructional
tone: replace "Erstellt ein übergeordnetes Buch, das alle Bände dieser Reihe
sammelt." (currently for "Crea un libro padre...") with an imperative like
"Erstelle ein übergeordnetes Buch, das alle Bände dieser Reihe sammelt." and
replace "Entfernt ..." (line with the key matching the original Italian
"Rimuove...") with the imperative form "Entferne ..." so both strings are
formal/imperative and consistent with admin helper text; locate and update the
exact string values in locale/de_DE.json for the entries whose current German
texts start with "Erstellt" and "Entfernt".
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 129-136: The test currently only reads seeded data; update the
spec so it actually exercises the save/update path by filling the ISSN input and
submitting the admin book form: after navigating to
`${BASE}/admin/libri/${seriesBookIds[0]}` and waiting for networkidle, use
page.fill('input[name="issn"]', NEW_ISSN_VALUE) (replace or derive
NEW_ISSN_VALUE from ISSN_VALUE), trigger the form submission (e.g., click the
save/submit button or call page.$eval to submit), wait for navigation or
networkidle, then reload or grab page.content() and assert the persisted value
is present (expect pageContent toContain the new ISSN and the 'ISSN' label) so
the store()/update() path is exercised.
---
Outside diff comments:
In `@installer/database/schema.sql`:
- Around line 393-424: libri.collana currently stores free text while
collane.nome is the canonical series name, causing orphaned series values; add a
DB-level foreign key from libri.collana to collane.nome (or collane.nome's
primary/unique key) with ON UPDATE CASCADE and ON DELETE SET NULL to maintain
integrity, or alternatively update the write paths (functions that insert/update
libri rows) to transactionally create/validate collane rows before saving
(ensure referential consistency in code paths that touch libri.collana). Refer
to the libri table definition (columns `collana`, `id`, and existing KEY
`idx_collana`) and the collane table's `nome` unique key when implementing the
FK or the transactional creation/validation.
---
Duplicate comments:
In `@app/Controllers/CollaneController.php`:
- Around line 400-416: The INSERT IGNORE causes silent skips so $volNum
increments even when no row is inserted; after calling $stmtInsert->execute() in
the loop (where $stmtInsert, $parentId, $bookId, $num are used) check
$stmtInsert->affected_rows and only increment $volNum (and count successes) when
affected_rows === 1, or switch to a regular INSERT and catch duplicate-key
errors (SQLSTATE 23000) to treat them as non-success without incrementing the
success count; ensure the final success message uses the actual count of
inserted rows rather than the loop iteration counter.
- Around line 154-172: In the delete() method of CollaneController, wrap the two
dependent DB writes (the UPDATE via $stmt and the DELETE via $stmt2 using $db
and $nome) in a transaction: call $db->begin_transaction() before
preparing/executing, only $db->commit() after both statements succeed, and call
$db->rollback() if either prepare/execute fails; ensure $stmt and $stmt2 are
properly closed on both success and failure, move setting
$_SESSION['success_message'] to after commit (and set an error message before
redirect on rollback), and return the redirect response in both cases.
- Around line 284-292: The code always returns a success JSON even if the DB
prepare/execute fails; update the CollaneController flow around the $stmt =
$db->prepare(...) block to detect failures: if prepare() returns false, respond
with a JSON error and non-200 status (use $db->error); after bind_param/execute
check the boolean result of $stmt->execute() and if false return a JSON error
using $stmt->error and an appropriate HTTP status via
$response->withStatus(...); only return the success JSON when prepare and
execute both succeed and ensure $stmt->close() still runs when applicable.
- Line 57: In CollaneController replace hardcoded redirect URLs like
url('/admin/collane') with the named-route helper (e.g.,
route_path('admin.collane.index') or
RouteTranslator::route('admin.collane.index')) in all controller methods that
perform redirects (the redirect return statements in the class); find
occurrences in methods that handle create/update/delete/list actions and change
the response->withHeader('Location', ...) calls to use the named route helper so
they generate URLs via route names instead of literal paths.
In `@app/Controllers/FrontendController.php`:
- Around line 661-667: The ORDER BY using CAST(l.numero_serie AS UNSIGNED)
causes non-numeric values like '' or 'Speciale' to become 0 and sort before real
volumes; update the SQL used where $stmtSeries is prepared in
FrontendController.php to prefer numeric series first and then non-numeric
labels by changing the ORDER BY to something like: first sort by whether
numero_serie matches a numeric regex (e.g. numero_serie REGEXP '^[0-9]+') DESC,
then by CAST(numero_serie AS UNSIGNED) ASC (for the numeric ones), and finally
by l.titolo ASC to deterministically order non-numeric entries — adjust the
query string assigned to $stmtSeries accordingly.
In `@app/Controllers/LibriController.php`:
- Around line 1996-2000: The code casts numero_volume to $numero but doesn't
reject zero/negative values before DB write; update the validation (near the
existing check that uses $operaId and $volumeId) to also reject $numero when
$numero <= 0 by returning a 400 JSON response (same format as the other
validation error). Locate the $numero assignment and the surrounding conditional
(references: $numero, $data['numero_volume'], $operaId, $volumeId, $response)
and add the $numero lower-bound check so invalid values are rejected before any
insert.
- Around line 528-536: The query prepared in $volStmt contains a hard-coded
"LIMIT 100" which silently truncates results for large opere; remove the fixed
LIMIT or replace it with a paginated parameter so callers can request full
result sets or specific pages. Update the SQL in $volStmt to either drop "LIMIT
100" entirely or change it to use bound parameters (e.g., "LIMIT ? OFFSET ?"),
then modify the execute call that binds v.opera_id to also bind the new
limit/offset variables (or expose a method parameter) so pagination or
configurable limits are applied instead of the silent 100-row cap.
In `@app/Views/collane/dettaglio.php`:
- Around line 15-17: The template currently uses hardcoded admin URLs via
url('/admin/dashboard') and url('/admin/collane') inside the htmlspecialchars
calls; replace these with the framework route helpers (e.g.,
route_path('admin.dashboard') or RouteTranslator::route('admin.dashboard'))
while preserving the existing htmlspecialchars(..., ENT_QUOTES, 'UTF-8')
escaping and the surrounding link attributes and classes; apply the same
replacement for all other hardcoded '/admin/...' instances noted (lines near the
url(...) calls and the locations you listed) so links use route keys instead of
literal paths.
In `@app/Views/libri/partials/book_form.php`:
- Around line 208-210: The form enforces an ISSN format with a hyphen
(XXXX-XXXX) but LibriController::store() and LibriController::update() currently
strip separators and accept compact ISSNs; update the server-side validation to
match the UI contract by rejecting compact ISSNs and requiring the dashed format
instead. Specifically, in LibriController::store() and LibriController::update()
replace the logic that strips separators/accepts compact values with a regex
check (e.g. ^\d{4}-\d{3}[0-9Xx]$) and return a validation error when the ISSN
does not match, and ensure the same validation is applied wherever ISSN handling
is duplicated (the other referenced block).
In `@app/Views/libri/scheda_libro.php`:
- Line 981: Replace hardcoded paths like url('/admin/libri/' .
(int)$parentWork['id']) and any inline '/api/search/unified' fetch URLs by using
the framework route helpers (route_path('route_key', ['id' =>
(int)$parentWork['id']]) or RouteTranslator::route('route_key')) for the anchor
hrefs and server-side rendering of API endpoints; inject those API route strings
into the page as PHP-rendered JS constants (e.g., a JS variable set from
route_path) and update the client-side fetches to use those constants. Locate
occurrences around the parent-work link generation (the anchor using url(...)),
the client fetches that reference '/api/search/unified', and the other flagged
spots, and replace string concatenation with the route helper calls so routes
remain stable and translatable.
In `@locale/en_US.json`:
- Line 2803: Update the English translation value for the JSON key "Cerca
libro": replace the current value "Search book" with the improved copy "Search
for a book" in locale/en_US.json so the UI reads naturally; locate the entry by
the key "Cerca libro" and change only the string value.
In `@tests/collane-management.spec.js`:
- Around line 44-58: The tests use fixed names (TEST_COLLANA/TEST_DESC) and
SELECT MAX(id) which can pick up preexisting rows; change test fixtures to
generate unique names per run (e.g., append timestamp or UUID to
TEST_COLLANA/TEST_DESC), create rows with INSERT ... RETURNING id (or read the
inserted id via the API used) and store those IDs in variables, update any
lookup code that currently uses SELECT MAX(id) to use the stored IDs instead,
and update teardown to delete/update rows by those stored IDs (not by name) via
dbExec so cleanup only removes the fixtures created by this run; apply same
pattern for the other occurrences referenced (lines ~89-90, 121-123, 148-149,
171-172).
- Around line 27-35: The helper loginAsAdmin currently waits for any /admin URL
which can match login/intermediate pages; update the post-login wait to target
the authenticated dashboard only by changing the waitForURL call in loginAsAdmin
to wait for the specific dashboard route (e.g., "/admin/dashboard" or a stricter
regex like /^\/admin\/dashboard/) with the same timeout so the function only
returns once the real dashboard is loaded.
In `@tests/issue-75-issn-series-volumes.spec.js`:
- Around line 31-39: The post-login wait in loginAsAdmin is too permissive—login
failure can still satisfy the regex; update the wait to assert an admin-only
destination or marker after submit: in the loginAsAdmin function replace the
page.waitForURL(/.*(?:dashboard|admin).*/, { timeout: 15000 }) with a strict
check such as waiting for the exact /admin/dashboard URL or waiting for a known
admin-only DOM marker (e.g., a selector visible only on the admin dashboard) to
ensure the test is actually authenticated.
- Around line 295-318: The test currently only checks DB state and index usage
but never asserts the actual ordering returned by the related-books logic;
update the test to call the real function or endpoint that produces the
"Potrebbero interessarti" list (e.g., invoke getRelatedBooks or the controller
route that uses the priority-0 path) for the first series book and assert that
the returned list begins with the same-series sibling IDs (use seriesBookIds
and/or seriesBookIds[1], seriesBookIds[2] to validate the first N entries),
ensuring the controller's priority-0 path still yields same-series books first.
- Around line 141-156: The test "3. ISSN displayed on frontend book detail page"
currently uses dbQuery fallbacks instead of loading the public detail page;
update it to parse the admin response (resp from
request.get(`${BASE}/admin/libri/${bookId}`)) to extract the real "Visualizza"
href for the public book page (instead of constructing or skipping to DB
checks), then perform a request.get to that extracted URL and assert the
rendered HTML/DOM contains the expected ISSN (ISSN_VALUE); remove the silent
DB-only assertions (issnDb/dbQuery for this check) and apply the same pattern
wherever similar fallbacks appear (tests referencing seriesBookIds, request.get,
dbQuery, and BASE) so tests always verify the live frontend output.
- Around line 63-99: The fixtures use a hardcoded author id (9991), static
titles/series names and separate dbExec calls which breaks isolation—replace the
fixed id by inserting the author via dbExec and capturing the real id with
dbQuery/SELECT LAST_INSERT_ID() (or use a single connection/transaction), append
a unique suffix to SERIES_NAME/titles (timestamp or random) so
seriesBookIds/parentWorkId/volumeIds are unique, ensure all related INSERTs use
the captured author id and push the actual returned libro ids into
seriesBookIds/volumeIds, and in test.afterAll delete only those captured ids
(without globally toggling FOREIGN_KEY_CHECKS across separate mysql invocations)
or perform cleanup inside the same DB connection/transaction so FK checks take
effect; update references to dbExec, dbQuery, seriesBookIds, parentWorkId,
volumeIds accordingly.
---
Nitpick comments:
In `@app/Controllers/CollaneController.php`:
- Around line 214-230: Wrap the two update operations in an explicit
transaction: call $db->begin_transaction() before preparing the statements in
the CollaneController rename flow, execute both prepared statements (the UPDATE
on libri and the UPDATE on collane), check their execution/affected_rows and
only call $db->commit() if both succeed; on any failure call $db->rollback(),
close prepared statements, and set the $_SESSION['success_message'] only after
commit (or set an error message after rollback). Ensure you still bind_param and
close both $stmt and $stmtSync, and handle exceptions/errors returned by $db or
the statement execute calls so the transaction is correctly rolled back on
failure.
In `@app/Controllers/LibriController.php`:
- Around line 741-755: Duplicate ISSN normalization/validation logic in store()
and update() should be extracted into a single private helper (e.g.,
normalizeIssn) that accepts the raw value, returns the canonical ISSN string or
null, and throws an \InvalidArgumentException on invalid input; replace the
inline blocks in store() and update() with calls to
normalizeIssn($fields['issn']) and handle the exception by setting
$_SESSION['error_message'] and returning the same redirect response used today.
Ensure the helper uses the same transformations and regex (/^\d{7}[\dX]$/) and
canonicalizes to "XXXX-XXXX" so both create/edit paths behave identically.
In `@app/Routes/web.php`:
- Around line 1391-1396: The /admin/collane route is missing the rate-limiting
middleware used by other admin list routes; update the route definition that
creates the CollaneController and adds AdminAuthMiddleware so it also adds the
same RateLimitMiddleware instance (the same middleware class used on
/admin/generi and /admin/utenti) to the middleware chain for that route,
ensuring the route closure (where new \App\Controllers\CollaneController() is
instantiated and ->add(new AdminAuthMiddleware()) is called) includes ->add(new
RateLimitMiddleware(...)) or the equivalent factory call to match the existing
routes' configuration.
- Around line 1446-1450: The /api/collane/search route currently lacks rate
limiting; update the route definition that constructs
CollaneController::searchApi to include the same rate-limiting middleware used
by the similar endpoint (/api/collocazione/suggerisci). Locate the route where
AdminAuthMiddleware is added and append or prepend the existing
RateLimitMiddleware (or the concrete class used for the suggerisci route) with
the same configuration (requests/period) so the search endpoint is subject to
the same throttling policy; no changes to CollaneController::searchApi are
required.
In `@app/Views/collane/index.php`:
- Line 45: Replace hardcoded admin URLs with framework route helpers and expose
JS routes from PHP: generate the create path with route_path (or
RouteTranslator::route) and inject it into the page (e.g. a JS variable like
COLLANA_CREa_PATH) then use that variable in the fetch call instead of
'/admin/collane/crea'; similarly replace the PHP
url('/admin/collane/dettaglio?...') usages by calling
route_path('admin.collane.dettaglio', [...params...]) (or
RouteTranslator::route('admin.collane.dettaglio')) to build the two dettaglio
links; update index.php to output these helper-generated paths and reference
them in the JS and hrefs (look for the fetch call and the two url(...)
occurrences).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86dea940-f462-478a-bb85-5e1140106f8f
📒 Files selected for processing (24)
app/Controllers/CollaneController.phpapp/Controllers/FrontendController.phpapp/Controllers/LibriApiController.phpapp/Controllers/LibriController.phpapp/Controllers/PublicApiController.phpapp/Routes/web.phpapp/Views/collane/dettaglio.phpapp/Views/collane/index.phpapp/Views/frontend/book-detail.phpapp/Views/layout.phpapp/Views/libri/index.phpapp/Views/libri/partials/book_form.phpapp/Views/libri/scheda_libro.phpinstaller/classes/Installer.phpinstaller/database/migrations/migrate_0.5.1.sqlinstaller/database/schema.sqllocale/de_DE.jsonlocale/en_US.jsontests/collane-management.spec.jstests/issue-75-issn-series-volumes.spec.jsvendor/composer/autoload_classmap.phpvendor/composer/autoload_static.phpvendor/composer/installed.phpversion.json
Comment on lines
+130
to
+138
| $stmt = $db->prepare("INSERT IGNORE INTO collane (nome) VALUES (?)"); | ||
| if ($stmt) { | ||
| $stmt->bind_param('s', $nome); | ||
| $stmt->execute(); | ||
| $stmt->close(); | ||
| } | ||
|
|
||
| $_SESSION['success_message'] = sprintf(__('Collana "%s" creata'), $nome); | ||
| return $response->withHeader('Location', url('/admin/collane/dettaglio?nome=' . urlencode($nome)))->withStatus(302); |
There was a problem hiding this comment.
INSERT IGNORE succeeds silently if collana already exists.
Using INSERT IGNORE means if the collana name already exists, the insert is silently ignored but the success message still says "Collana created". This is minor since the user ends up on the correct detail page either way.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Controllers/CollaneController.php` around lines 130 - 138, The current
INSERT IGNORE into collane silently succeeds when a duplicate nome exists but
still sets $_SESSION['success_message'] as if a new collana was created; after
executing the prepared statement ($stmt->execute()), check $stmt->affected_rows
(or $db->affected_rows) to determine if a row was inserted (affected_rows > 0)
and set $_SESSION['success_message'] to a "created" message only in that case,
otherwise set a different message like "Collana already exists" and still
redirect to the same dettaglio URL; also handle the case where prepare() returns
false by setting an error message or logging before redirecting.
Comment on lines
+129
to
+136
| test('2. ISSN value is saved and visible in admin book detail', async () => { | ||
| await page.goto(`${BASE}/admin/libri/${seriesBookIds[0]}`); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| const pageContent = await page.content(); | ||
| expect(pageContent).toContain(ISSN_VALUE); | ||
| expect(pageContent).toContain('ISSN'); | ||
| }); |
There was a problem hiding this comment.
This never exercises the ISSN save path.
The test only renders seeded data on the admin detail page; it never edits the form or submits it. A regression in store()/update() would still pass here. Please change the ISSN in input[name="issn"], submit the form, and then assert the persisted value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/issue-75-issn-series-volumes.spec.js` around lines 129 - 136, The test
currently only reads seeded data; update the spec so it actually exercises the
save/update path by filling the ISSN input and submitting the admin book form:
after navigating to `${BASE}/admin/libri/${seriesBookIds[0]}` and waiting for
networkidle, use page.fill('input[name="issn"]', NEW_ISSN_VALUE) (replace or
derive NEW_ISSN_VALUE from ISSN_VALUE), trigger the form submission (e.g., click
the save/submit button or call page.$eval to submit), wait for navigation or
networkidle, then reload or grab page.content() and assert the persisted value
is present (expect pageContent toContain the new ISSN and the 'ISSN' label) so
the store()/update() path is exercised.
- CRITICAL: fix unified search response parsing in add-volume modal
(was checking data.books, API returns flat array with type/label)
- Wrap delete/rename/merge in transactions for atomicity
- Add error handling to removeVolume and updateOrder
- Walk full ancestor chain for cycle prevention (not just direct A↔B)
- Validate numero_volume >= 1 before insert
- Remove LIMIT 100 on volumes query
- Track actual affected_rows in INSERT IGNORE volume count
- Extract normalizeIssn() shared helper (DRY store/update)
- Fix non-numeric numero_serie sorting ('' and 'Speciale' after real numbers)
- Polish EN translation "Search for a book", DE imperative tone
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
1 participant
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
volumitable — links parent works to individual volume books. Admin detail shows volumes table for parent works and parent reference badge for child volumesvolumi+ ADD INDEXidx_collana$stmt->bind_param()against false prepare (crash prevention)Test plan
Refs #75
🤖 Generated with Claude Code
Summary by CodeRabbit