feat: reservation system improvements and availability fixes #25
Conversation
- Fix table sorting in all admin pages (autori, editori, prestiti, utenti) - Parse DataTables order[0][column] and order[0][dir] parameters - Dynamic ORDER BY instead of hardcoded values - Add styled session-expired.php error page for CSRF failures - CsrfMiddleware detects AJAX vs form requests - AJAX: returns JSON error - Form: shows styled HTML error page
- Fix table sorting in all admin pages (autori, editori, prestiti, utenti) - Parse DataTables order[0][column] and order[0][dir] parameters - Dynamic ORDER BY instead of hardcoded values - Add styled session-expired.php error page for CSRF failures - CsrfMiddleware detects AJAX vs form requests - AJAX: returns JSON error - Form: shows styled HTML error page
## Reservation System - Add ReservationReassignmentService for automatic copy reassignment - Add scripts/check-expired-reservations.php cron job - Fix availability calendar to correctly handle lost/damaged copies - Add 'annullato' and 'scaduto' states to prestiti enum (migrate_0.4.3.sql) ## Updater Improvements - Implement streaming database backup to prevent memory exhaustion - Add shutdown handler to cleanup maintenance mode on fatal errors - Add checkStaleMaintenanceMode() to auto-recover stuck updates - Add memory limit increase for large database updates - Add concurrent update prevention with file locking ## Bug Fixes - Fix JavaScript regex escaping in catalog.php (HEREDOC double-escape) - Fix getBookTotalCopies() fallback logic for lost copies - Fix SQL column references in ReservationReassignmentService ## Other Changes - Add English translations for new Updater log messages - Update MaintenanceService with admin login hook - Add copy status change logging in CopyController
WalkthroughQuesta pull request implementa il riassegnamento automatico delle prenotazioni quando le copie diventano disponibili o non disponibili, aggiunge il supporto per soft delete dei libri, estende gli stati delle prenotazioni/prestiti, introduce un nuovo controllo di manutenzione per le prenotazioni scadute, e sostituisce il log di errori generico con SecureLogger in tutta l'applicazione. Changes
Sequence DiagramsequenceDiagram
actor User
participant Ctrl as Controller<br/>(Copy/Libri/Prestiti)
participant RRS as ReservationReassignment<br/>Service
participant DB as Database
participant CopyRepo as CopyRepository
participant NotifServ as NotificationService
participant Logger as SecureLogger
Note over Ctrl,Logger: Scenario 1: Copy diventa non-disponibile (es. danneggiato)
User->>Ctrl: Aggiorna stato copia
Ctrl->>DB: BEGIN TRANSACTION
Ctrl->>DB: UPDATE copie SET stato=?
Ctrl->>RRS: reassignOnCopyLost(copiaId)
RRS->>DB: SELECT ... FOR UPDATE prenotazione attiva
alt Prenotazione attiva trovata
RRS->>CopyRepo: findAvailableCopy(libroId)
alt Copia disponibile esiste
RRS->>DB: UPDATE prestiti SET copia_id=?
RRS->>DB: UPDATE copie SET stato='prenotato'
RRS->>Logger: Log reassignment success
RRS->>NotifServ: deferNotification() (se transazione esterna)
else No available copy
RRS->>DB: UPDATE prestiti SET copia_id=NULL
RRS->>Logger: Log no copy available
RRS->>NotifServ: notifyUserCopyUnavailable()
end
end
Ctrl->>DB: COMMIT
RRS->>NotifServ: flushDeferredNotifications()
RRS-->>User: Notifica via email (copia assegnata/non disponibile)
Note over Ctrl,Logger: Scenario 2: Nuova copia creata (store libri)
User->>Ctrl: Crea nuovo libro con copia
Ctrl->>DB: BEGIN TRANSACTION
Ctrl->>DB: INSERT INTO libri
Ctrl->>DB: INSERT INTO copie
Ctrl->>RRS: reassignOnNewCopy(libroId, newCopiaId)
RRS->>DB: SELECT ... FOR UPDATE prenotazione pendente più vecchia
alt Prenotazione trovata
RRS->>DB: UPDATE prestiti SET copia_id=newCopiaId
RRS->>DB: UPDATE copie SET stato='prenotato'
RRS->>NotifServ: deferNotification()
end
Ctrl->>DB: COMMIT
RRS->>NotifServ: flushDeferredNotifications()
RRS-->>User: Notifica: copia assegnata alla prenotazione
Note over Ctrl,Logger: Scenario 3: Copia restituita (return prestito)
User->>Ctrl: Restituisci prestito
Ctrl->>DB: BEGIN TRANSACTION
Ctrl->>DB: UPDATE prestiti SET stato='restituito'
Ctrl->>RRS: reassignOnReturn(copiaId)
RRS->>DB: SELECT libro da copiaId
RRS->>RRS: reassignOnNewCopy(libroId, copiaId)
RRS->>DB: SELECT ... FOR UPDATE prossima prenotazione
alt Prenotazione in attesa
RRS->>DB: UPDATE prestiti, copie
RRS->>NotifServ: deferNotification()
else Nessuna prenotazione
RRS->>DB: UPDATE copie SET stato='disponibile'
end
Ctrl->>DB: COMMIT
RRS->>NotifServ: flushDeferredNotifications()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Aree che richiedono attenzione particolare:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
♻️ Duplicate comments (1)
app/Controllers/PrestitiController.php (1)
613-625: Uso diReservationReassignmentServicedentro transazione di restituzione (vedi nota sul servizio)La chiamata a
$reassignmentService->reassignOnReturn($copia_id)avviene all’interno di una transazione già aperta inprocessReturn(). Poiché i metodi del servizio aprono e chiudono a loro volta transazioni, si applicano qui le stesse considerazioni su transazioni nidificate e visibilità dei dati già commentate inApp\Services\ReservationReassignmentService(rischio di commit parziali o di non vedere lo stato aggiornato della copia).Suggerisco di rivedere l’ownership delle transazioni come indicato nel commento sul servizio.
Also applies to: 637-645
🧹 Nitpick comments (17)
app/Models/BookRepository.php (2)
626-667:syncAuthorsefficiente, ma fallback diprocessAuthorIdmigliorabileL’early return in
syncAuthorsquando!$authorIdsevita una INSERT loop inutile dopo la DELETE: bene. InprocessAuthorId, invece, il fallback finalereturn (int) $authorData;potrebbe trasformare valori inaspettati (es. array/oggetti) in interi con warning di conversione; visto che i soli formati validi sembrano essere “ID numerico” e stringhenew_*, conviene rendere il fallback esplicitamente sicuro.Suggerimento:
private function processAuthorId($authorData): int { // Handle both old format (just ID) and new format (could be temp ID with label) if (is_numeric($authorData)) { - return (int) $authorData; + return (int) $authorData; } // Handle new author format from Choices.js (new_timestamp) if (is_string($authorData) && strpos($authorData, 'new_') === 0) { // This is a new author that needs to be created // For now, just skip it - this would need additional handling // based on your form submission logic return 0; } - - return (int) $authorData; + + // Fallback: formati non numerici/inaspettati non vengono usati + return 0; }
724-769:updateOptionalspiù sicuro, con piccolo nit di leggibilità SQLLa costruzione dinamica di
$colscon early return quando è vuoto evita UPDATE inutili, e la distinzione di tipo (ipernumero_pagine/anno_pubblicazione,sper il resto) è corretta. Micro‑nit: per leggibilità della query, potresti aggiungere uno spazio dopo la virgola inimplode:- $sql = 'UPDATE libri SET ' . implode(',', $set) . ', updated_at=NOW() WHERE id=?'; + $sql = 'UPDATE libri SET ' . implode(', ', $set) . ', updated_at=NOW() WHERE id=?';app/Views/errors/session-expired.php (1)
9-11: Permettere l’override di$pageTitlee$loginUrldalla chiamanteQui le variabili vengono sempre sovrascritte, impedendo a chi include la view di passare un titolo personalizzato o un URL di login diverso (
/loginhardcoded):- $pageTitle = __('Sessione Scaduta'); - $loginUrl = '/login'; + $pageTitle = $pageTitle ?? __('Sessione Scaduta'); + $loginUrl = $loginUrl ?? '/login';Così mantieni i default attuali ma consenti configurazioni diverse quando necessario.
.gitignore (1)
319-321: IgnorareCHANGELOG.mdnel repositoryAggiungere
CHANGELOG.mdalla sezione “Verbose documentation” rende chiaro che il changelog principale non viene tracciato in git (oltre a essere escluso dai pacchetti). Se è davvero gestito fuori repo o generato automaticamente, la scelta è coerente; altrimenti valuterei se mantenerlo versionato.installer/database/schema.sql (1)
342-342: Suggerimento: considera un indice sudeleted_atper le query di soft delete.Per ottimizzare le query che filtrano i libri non cancellati (es.
WHERE deleted_at IS NULL), potrebbe essere utile aggiungere un indice parziale o un indice standard su questa colonna.KEY `idx_libri_deleted_at` (`deleted_at`)scripts/check-expired-reservations.php (1)
40-109: Implementazione corretta dello script per prenotazioni scadute.Lo script gestisce correttamente le prenotazioni scadute:
Query e selezione (linee 40-49):
- Seleziona correttamente le prenotazioni con stato='prenotato', attivo=1 e data_scadenza < oggi
Elaborazione transazionale (linee 62-76):
- Usa transazioni per garantire atomicità
- Marca la prenotazione come 'scaduto' e attivo=0
- Aggiunge una nota di sistema con la data
Gestione copia (linee 78-97):
- Usa FOR UPDATE per il locking corretto (linea 80)
- Verifica lo stato della copia prima di modificarla (linea 86)
- Rilascia la copia (disponibile) e attiva il riassegnamento (linea 95)
Nota: Le notifiche utente sono commentate (linee 103-104) con un chiaro promemoria che il metodo deve essere aggiunto. Considera di implementare questa funzionalità per migliorare l'esperienza utente.
Vuoi che generi il metodo
notifyUserReservationExpired()per il NotificationService?locale/en_US.json (3)
1690-1703: Chiavi MaintenanceService ben strutturateLe chiavi di log per
MaintenanceServicecoprono i vari casi d’errore e di esecuzione e sono denominate in modo chiaro; le traduzioni inglesi sono aderenti all’italiano. Verifica solo che le stesse chiavi esistano anche negli altri file di localizzazione, se supportati.
2474-2474: Label “Scaduta il”L’etichetta “Scaduta il” → “Expired on” è corretta grammaticalmente e adatta a prenotazioni/risorse al femminile. Assicurati solo di usare la stessa forma ovunque per coerenza con lo stato
scaduto/scaduta.
3594-3623: Messaggi per riassegnazione copie e prenotazioni completano bene il dominioLe nuove stringhe per errori di riassegnazione, notifiche di copia non disponibile e messaggi di messa “in attesa” della prenotazione sono coerenti con il comportamento descritto nel PR e i placeholder
%srisultano corretti. L’unica nota è “DataIntegrity warning (store loan)”, lasciata in forma ibrida: se è un puro tag tecnico di log va bene così, altrimenti potresti localizzarla per allinearla al resto.app/Controllers/UserActionsController.php (6)
185-188: Log errore incancelLoanIl passaggio da
error_logaSecureLogger::errorcon messaggio localizzato è positivo. Valuta di includere nel contesto ancheloan_id,user_id(e magaricopia_id) per rendere i log più utili in fase di debug.
259-262: Uso di SecureLogger incancelReservationAnche qui il passaggio a
SecureLogger::errorè corretto e coerente con il resto del codice. Come sopra, aggiungerereservation_ideuser_idnel contesto renderebbe il log più diagnosticabile.
359-362: Log strutturato perchangeReservationDateLa gestione degli errori con
SecureLogger::error(__('Errore modifica data prenotazione'), [...])è adeguata. Potresti includere anchereservation_id,user_id,libro_ide la nuova data richiesta per facilitare l’analisi di eventuali problemi in produzione.
446-447: Warning su notifica richiesta prestito non inviataL’uso di
SecureLogger::warningnel catch della notifica (notifyLoanRequest) è appropriato e mantiene l’operazione principale di creazione del prestito indipendente da eventuali problemi di notifica. Anche qui, aggiungereloan_id,libro_ideuser_idnel contesto sarebbe utile.
451-454: Gestione errori nel flussoloan()La sostituzione di
error_logconSecureLogger::error(__('Errore richiesta prestito'), ...)nel catch esterno è corretta. Suggerirei di includere nel contesto almenolibro_ideutente_id, così da poter correlare rapidamente gli errori ai record interessati.
562-565: Logging errori inreserve()Il nuovo log tramite
SecureLogger::error(__('Errore prenotazione'), ...)nel catch del metodoreserveè coerente con gli altri flussi di prenotazione/prestito. Anche qui vale la stessa raccomandazione: aggiungerelibro_id,utente_ide magari l’intervallo di date richiesto rende il log molto più utile.app/Support/MaintenanceService.php (1)
61-82: Aggiornare la firma di ritorno dirunAll()per includereexpired_reservations
runAll()ora popola$results['expired_reservations'], ma la phpdoc del metodo non include questa chiave nella shape dell’array restituito. Per evitare sorprese a chi fa static analysis o IDE completion, converrebbe aggiungereexpired_reservations: intalla annotation di ritorno.Also applies to: 98-105
app/Controllers/PrestitiController.php (1)
824-835: CSV prestiti: includere i nuovi statiannullatoescadutoNel CSV:
\$validStatesnon include i nuovi stati enumannullatoescaduto, quindi non è possibile filtrarli via?stati=anche se esistono in DB.\$statusLabelsnon ha etichette localizzate per questi stati, quindi nel CSV apparirebbero eventualmente non tradotti.Per mantenere coerenza con l’enum di
prestiti, suggerisco di:- $validStates = ['pendente', 'prenotato', 'in_corso', 'in_ritardo', 'restituito', 'perso', 'danneggiato']; + $validStates = ['pendente', 'prenotato', 'in_corso', 'in_ritardo', 'restituito', 'perso', 'danneggiato', 'annullato', 'scaduto'];e di estendere le label:
$statusLabels = [ 'pendente' => __('Pendente'), 'prenotato' => __('Prenotato'), 'in_corso' => __('In Corso'), 'in_ritardo' => __('In Ritardo'), 'restituito' => __('Restituito'), 'perso' => __('Perso'), 'danneggiato' => __('Danneggiato'), + 'annullato' => __('Annullato'), + 'scaduto' => __('Scaduto'), ];Also applies to: 909-917
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.gitignore(1 hunks).rsync-filter(1 hunks)app/Controllers/AutoriApiController.php(1 hunks)app/Controllers/CopyController.php(2 hunks)app/Controllers/EditoriApiController.php(2 hunks)app/Controllers/LibriController.php(5 hunks)app/Controllers/PrestitiApiController.php(2 hunks)app/Controllers/PrestitiController.php(10 hunks)app/Controllers/ReservationsController.php(1 hunks)app/Controllers/UserActionsController.php(7 hunks)app/Controllers/UtentiApiController.php(1 hunks)app/Middleware/CsrfMiddleware.php(1 hunks)app/Models/BookRepository.php(18 hunks)app/Services/ReservationReassignmentService.php(1 hunks)app/Support/MaintenanceService.php(7 hunks)app/Support/PluginManager.php(2 hunks)app/Support/Updater.php(6 hunks)app/Views/errors/session-expired.php(1 hunks)app/Views/frontend/catalog.php(5 hunks)app/Views/prestiti/crea_prestito.php(1 hunks)installer/database/migrations/migrate_0.4.2.sql(1 hunks)installer/database/migrations/migrate_0.4.3.sql(1 hunks)installer/database/schema.sql(3 hunks)locale/en_US.json(5 hunks)scripts/check-expired-reservations.php(1 hunks)storage/plugins/api-book-scraper/wrapper.php(1 hunks)updater.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
app/Views/prestiti/crea_prestito.php (1)
app/helpers.php (1)
__(25-28)
app/Middleware/CsrfMiddleware.php (1)
app/helpers.php (1)
__(25-28)
app/Models/BookRepository.php (4)
app/Support/MaintenanceService.php (1)
__construct(31-34)app/Models/PublisherRepository.php (1)
__construct(10-10)app/Models/AuthorRepository.php (1)
__construct(10-10)app/Models/DashboardStats.php (1)
__construct(10-10)
storage/plugins/api-book-scraper/wrapper.php (1)
storage/plugins/api-book-scraper/ApiBookScraperPlugin.php (1)
ApiBookScraperPlugin(14-609)
app/Controllers/UserActionsController.php (2)
app/Services/ReservationReassignmentService.php (1)
ReservationReassignmentService(17-379)app/Support/DataIntegrity.php (2)
DataIntegrity(9-1222)recalculateBookAvailability(234-348)
app/Controllers/CopyController.php (1)
app/Services/ReservationReassignmentService.php (3)
ReservationReassignmentService(17-379)reassignOnCopyLost(113-180)reassignOnReturn(186-203)
app/Support/Updater.php (1)
app/Controllers/UpdateController.php (1)
createBackup(122-152)
🪛 LanguageTool
updater.md
[uncategorized] ~11-~11: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...Quando aggiungi un file a .gitignore, **DEVI aggiungerlo ANCHE a .rsync-filter...
(GR_09_001)
[uncategorized] ~105-~105: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...ipt per creare pacchetti di release | | Filter Rules | .rsync-filter | **⭐ R...
(GR_09_001)
[uncategorized] ~105-~105: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... | Filter Rules | .rsync-filter | **⭐ Regole include/exclude per rsync (PRIM...
(GR_09_001)
[uncategorized] ~296-~296: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... ## File version.json Questo file è la fonte di verità per la versione dell'a...
(GR_09_001)
[uncategorized] ~512-~512: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...# Scopo Questo file contiene lo schema COMPLETO del database per le fresh ins...
(GR_09_001)
[uncategorized] ~702-~702: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... .rsync-filter | rsync filter rules | Primario (raccomandato) | | `.distigno...
(GR_09_001)
[uncategorized] ~707-~707: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...i gitignore con negazioni (!path) che NON FUNZIONANO con `rsync --exclude-fr...
(GR_09_001)
[uncategorized] ~738-~738: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... pattern` | | Ordine | Non importante | CRITICO - primo match vince | | Plugin...
(GR_09_001)
[uncategorized] ~764-~764: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... nel Pacchetto Il pacchetto di release INCLUDE tutto il necessario per funzio...
(GR_09_001)
[uncategorized] ~775-~775: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "*".
Context: ... node_modules? 1. Gli utenti possono personalizzare CSS/JS senza dover conf...
(GR_09_001)
[uncategorized] ~776-~776: In questo caso l'uso della 'd' eufonica è ammesso: "ed eseguire".
Context: ...ta modificare i sorgenti in frontend/ e eseguire npm run build 3. Senza node_modules, ...
(ST_03_001)
[uncategorized] ~778-~778: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... su hosting condivisi 4. Il pacchetto è autosufficiente - funziona subito, per...
(GR_09_001)
[uncategorized] ~836-~836: In questo caso l'uso della 'd' eufonica è ammesso: "ed estrae".
Context: ... | downloadUpdate($version) | Scarica e estrae il pacchetto | | createBackup() | Cre...
(ST_03_001)
[uncategorized] ~986-~986: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... Versione ### Prima del Release - [ ] version.json aggiornato con nuova vers...
(GR_09_001)
[uncategorized] ~987-~987: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "*".
Context: ...n aggiornato con nuova versione - [ ] Migrazione SQL creata (se modifiche DB...
(GR_09_001)
[uncategorized] ~988-~988: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "*".
Context: ...ne SQL creata (se modifiche DB) - [ ] schema.sql sincronizzato con migrazion...
(GR_09_001)
[uncategorized] ~989-~989: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "*".
Context: ...ql sincronizzato con migrazione - [ ] data_it_IT.sql aggiornato (se nuovi se...
(GR_09_001)
[uncategorized] ~990-~990: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "*".
Context: ....sql aggiornato (se nuovi seed) - [ ] data_en_US.sql aggiornato (se nuovi se...
(GR_09_001)
[uncategorized] ~991-~991: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "*".
Context: ....sql aggiornato (se nuovi seed) - [ ] locale/en_US.json aggiornato (se nuove...
(GR_09_001)
[uncategorized] ~992-~992: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "*".
Context: ... aggiornato (se nuove stringhe) - [ ] CHANGELOG.md aggiornato (opzionale) - ...
(GR_09_001)
[uncategorized] ~1148-~1148: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... Sotto "Workflow permissions" seleziona "Read and write permissions" 3. Spunta...
(GR_09_001)
[uncategorized] ~1168-~1168: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...i CI) Prima di creare un tag, verifica SEMPRE: ```bash # 1. package-lock.jso...
(GR_09_001)
[uncategorized] ~1317-~1317: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...e esclusioni, ma rsync --exclude-from NON supporta questa sintassi. ```giti...
(GR_09_001)
[uncategorized] ~1427-~1427: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...e permissions" 2. Oppure (consigliato): Fai la build in locale e usa `gh relea...
(GR_09_001)
[uncategorized] ~1479-~1479: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...alla distribuzione deve essere aggiunto ESPLICITAMENTE a .rsync-filter: ```...
(GR_09_001)
[uncategorized] ~1517-~1517: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "**".
Context: ...: Il plugin scraping-pro è un plugin PREMIUM venduto separatamente. **NON ...
(GR_09_001)
[uncategorized] ~1609-~1609: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...ione idempotente (CREATE IF NOT EXISTS, NO AFTER) - [ ] schema.sql sincroniz...
(GR_09_001)
[uncategorized] ~1682-~1682: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ...ile di Migrazione I file di migrazione NON DEVONO MAI essere eliminati, anche...
(GR_09_001)
[uncategorized] ~1686-~1686: L’asterisco si usa per un’omissione nel numero di tre consecutivi: "***".
Context: ... nel Pacchetto Il pacchetto di release INCLUDE la cartella vendor/ producti...
(GR_09_001)
🪛 markdownlint-cli2 (0.18.1)
updater.md
80-80: Link fragments should be valid
(MD051, link-fragments)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
339-339: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
506-506: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
568-568: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
649-649: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
863-863: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
897-897: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1697-1697: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (32)
app/Models/BookRepository.php (5)
8-12: Costruttore con property promotion e type hint mysqliBuona scelta usare
public function __construct(private mysqli $db)in linea con gli altri repository (AuthorRepository,PublisherRepository, ecc.): meno boilerplate e dipendenza tipizzata chiaramente.
46-126: GestionenullingetByIde caricamento autoriL’early return
if (!$row) return null;è coerente con la firma?arraye rende esplicito il caso “libro non trovato”. Il loop sugli autori con l’arrayautoriinizializzato esplicitamente migliora la leggibilità senza cambiare il comportamento.
176-391: Normalizzazione input e cast espliciti increateBasic/updateBasicI cast espliciti per ID (scaffale/mensola/posizione/posizione_id), codici (
isbn*,ean), importi (peso,prezzo) e copie (copie_totali,copie_disponibili), insieme alla conversione di stringhe vuote innull, rendono più robusta l’INSERT/UPDATE contro input “sporchi” da form. Anche il logging con cast(int)perinsert_idecodemantiene i payload coerenti.Also applies to: 393-599
601-624: Gestione enum e cache di metadati colonneLa normalizzazione degli enum tramite
trim((string) $value)e confronto case‑insensitive contro le opzioni digetEnumOptions()è sensata e tollera maiuscole/minuscole dall’UI. La cache per colonne ($columnCache,$tableColumnCache) e per enum ($enumCachecon chiave$table . '.' . $column) riduce round‑trip aINFORMATION_SCHEMA/SHOW COLUMNSsenza complicare troppo il codice. La whitelist di tabelle inhasColumnInTable()è una buona difesa contro SQL injection sul nome tabella, con l’avvertenza che future tabelle dovranno essere aggiunte lì.Also applies to: 771-785, 787-823, 828-849
669-679: GestionenullingetScaffaleLetter/getMensolaLevelIl controllo sul risultato di
fetch()e il cast esplicito di$levelsolo quando non ènullrendono i metodi più sicuri rispetto a valori mancanti in DB; il contratto che restituiscenullin caso di nessun risultato è chiaro e facile da consumare dabuildCollocazioneString.Also applies to: 681-694
.rsync-filter (1)
175-179: Esclusione esplicita di CHANGELOG.md dal pacchettoL’aggiunta di
CHANGELOG.mdagli EXCLUDES rende esplicito che il changelog non deve finire nel pacchetto di distribuzione, in linea con la gestione della documentazione altrove. Nessuna criticità.storage/plugins/api-book-scraper/wrapper.php (1)
11-20: Rinomina a$_pluginInstancee registry globaleL’uso di
$_pluginInstanceal posto di$pluginevita conflitti con la variabile$plugindel chiamante e mantiene coerente la registrazione in$GLOBALS['plugins']['api-book-scraper']. Comportamento invariato, cambio pulito.app/Controllers/UtentiApiController.php (1)
87-115: Ordinamento DataTables lato server implementato in modo sicuroLa gestione dell’ordinamento tramite
columnMape normalizzazione didiradASC/DESCgarantisce unORDER BYsicuro e sotto controllo applicativo. Il fallback suu.cognome+u.nomemantiene un ordinamento deterministico. Sembra tutto a posto.Also applies to: 119-120
app/Controllers/EditoriApiController.php (1)
99-124: Ordinamento dinamico editori coerente con filtri e HAVINGLa mappa
columnMape la normalizzazione didirpermettono di costruireORDER BYsicuri, inclusa la colonna derivatalibri_count. L’uso di$subOrderColumnnella subquery quando è presenteHAVINGmantiene lo stesso comportamento di ordinamento anche sui risultati filtrati per numero di libri. Implementazione solida.Also applies to: 174-191
app/Controllers/AutoriApiController.php (1)
145-169: Ordinamento dinamico autori ben integrato con colonne opzionaliLa gestione di
columnMapcon la colonna nazionalità risolta via$colNaze l’aliaslibri_countrende l’ordinamento flessibile ma sicuro. L’ORDER BY $orderColumn $orderDirusa solo colonne note e direzioni normalizzate, mantenendo intatti i filtri esistenti. Nessuna anomalia evidente.Also applies to: 171-175
app/Support/PluginManager.php (1)
1139-1177: Isolamento delle variabili del plugin inloadPluginL’introduzione delle variabili prefissate (
$_pluginId,$_pluginName,$_pluginPath,$_mainFile) prima delrequire_onceelimina il rischio che il file del plugin sovrascriva$plugine rompa la logica successiva. L’uso coerente di$_pluginIdpersetPluginId()eregisterPluginHooks()e il logging suSecureLoggermigliorano robustezza e tracciabilità del caricamento dei plugin.installer/database/migrations/migrate_0.4.3.sql (1)
1-5: LGTM - Migrazione corretta per estendere l'enum stato.La migrazione aggiunge correttamente i nuovi valori
annullatoescadutoall'enumprestiti.stato, mantenendo i valori esistenti e il default. La modifica è allineata conschema.sql(linea 596).app/Views/prestiti/crea_prestito.php (1)
75-77: LGTM - Nuovo caso di errore ben integrato.Il nuovo caso
duplicate_reservationsegue correttamente il pattern esistente degli altri casi di errore e utilizza la funzione__()per l'internazionalizzazione.installer/database/schema.sql (2)
596-596: LGTM - Estensione enumprestiti.statocorretta.I nuovi valori
annullatoescadutosono allineati con la migrazionemigrate_0.4.3.sqle supportano la gestione delle prenotazioni scadute descritta negli obiettivi del PR.
113-115:sede_idfield design is intentional.The
sede_idfield is added without a foreign key constraint because theseditable does not exist in the schema. This is appropriate as a preparatory field for future functionality. Once theseditable is created, consider adding a foreign key constraint tosede_idat that time.app/Controllers/CopyController.php (2)
8-8: LGTM - Import corretto.L'import di
SecureLoggerè necessario per il logging degli errori nella nuova logica di reassignment.
155-160: LGTM - Ordine corretto delle operazioni.Il ricalcolo della disponibilità del libro (
recalculateBookAvailability) avviene correttamente dopo la logica di reassignment, garantendo che i contatori siano aggiornati dopo tutte le modifiche.installer/database/migrations/migrate_0.4.2.sql (1)
1-15: Remove migration files or implement migration execution system.The migration files (migrate_0.4.2.sql, migrate_0.4.3.sql) exist but are never executed by the installer. The Installer.php only processes schema.sql, data_*.sql, and triggers.sql. These migration files have no execution path in the codebase and will remain unused. Either remove them or implement a migration runner that loads and executes migrations in version order during installation.
Likely an incorrect or invalid review comment.
app/Controllers/PrestitiApiController.php (1)
107-141: Implementazione corretta dell'ordinamento con whitelist.L'approccio all'ordinamento dinamico è sicuro:
- L'indice della colonna è convertito a int (linea 123)
- La direzione è validata (ASC o DESC, linea 124)
- Solo le colonne nella whitelist
$columnMappossono essere usate (linee 126-129)L'interpolazione diretta nella clausola ORDER BY (linea 141) è necessaria perché ORDER BY non può essere parametrizzato nei prepared statement. La whitelist fornisce protezione adeguata contro SQL injection.
Nota: La colonna calcolata 'utente' (linea 115) viene ordinata usando l'alias dalla SELECT. Questo funziona correttamente in MySQL, ma verifica che l'ordinamento per nome utente produca i risultati attesi.
app/Middleware/CsrfMiddleware.php (1)
63-126: Implementazione corretta della gestione CSRF con supporto AJAX.L'implementazione distingue correttamente tra richieste AJAX e form tradizionali:
Richieste AJAX (linee 66-83):
- Restituisce JSON con codici di errore appropriati (SESSION_EXPIRED o CSRF_INVALID)
- Include un URL di redirect quando la sessione è scaduta
- Usa le traduzioni per i messaggi di errore
Richieste form (linee 84-94):
- Mostra una pagina HTML stilizzata per una migliore UX
- Usa il pattern ob_start/ob_get_clean per il rendering della view
Metodo isAjaxRequest (linee 105-126):
- Controlla X-Requested-With header (jQuery e framework comuni)
- Controlla Accept header per preferenza JSON
- Controlla Content-Type per richieste JSON
- L'approccio euristico è solido e copre i casi d'uso comuni
Nota: L'uso di
require(linea 89) invece diincludecauserà un fatal error se la view non esiste, ma questo è accettabile dato che indica un'installazione danneggiata.updater.md (1)
1-1698: Documentazione completa e ben strutturata del sistema di aggiornamento.Il documento fornisce una guida esauriente per il sistema di aggiornamento Pinakes:
Punti di forza:
- Sezione critica iniziale (linee 5-62): Evidenzia chiaramente la regola fondamentale che .gitignore e .rsync-filter sono sistemi separati
- Struttura completa: Copre tutti gli aspetti (versioning, migrazioni, build, troubleshooting)
- Lezioni apprese (linee 1309-1546): Documentazione preziosa dei problemi incontrati e delle soluzioni
- Checklist pratiche: Fornisce checklist verificabili per evitare errori comuni
- Flow consigliato (linee 1213-1280): Guida passo-passo per creare release
Nota: I problemi di formattazione markdown segnalati dall'analisi statica (mancanza di identificatori lingua nei code block, uso di emphasis invece di heading) sono minori e non impattano la leggibilità.
Suggerimento: Considera di aggiungere un indice cliccabile all'inizio per facilitare la navigazione in un documento così lungo.
app/Controllers/ReservationsController.php (1)
377-417: Comportamento del fallback per i dati legacy è intenzionale e coerente.La logica è corretta: il metodo restituisce accuratamente 0 quando tutti i dati sono tracciati nella tabella
copiee tutti risultano non disponibili (persi/danneggiati/in manutenzione).Il fallback per i dati legacy (linea 409) usa
GREATEST(IFNULL(copie_totali, 1), 1)che restituisce sempre almeno 1. Questo è un design intenzionale confermato da:
- Commento esplicito nel codice: "This handles legacy data where copies weren't tracked individually"
- Implementazione identica in
ReservationManager.phpcon cross-reference: "This ensures consistency with ReservationsController::getBookTotalCopies()"- Sistema a due livelli: tracciamento moderno (tabella
copie, può tornare 0) vs. dati legacy (colonnacopie_totali, minimo 1)La validazione di sistema impedisce comunque che
copie_totalisia 0 (enforced inLibriController), quindi questa assunzione di minimo 1 per dati senza tracciamento individuale è ragionevole.locale/en_US.json (2)
110-129: Messaggi di avanzamento/errore aggiornamento coerentiLe nuove stringhe per stato avanzamento, backup e errori di aggiornamento sono coerenti con il resto del file e i placeholder risultano corretti. Nessun problema da segnalare.
2271-2271: Nuovo messaggio per doppio prestito/prenotazioneLa stringa “Questo utente ha già un prestito o una prenotazione attiva per questo libro.” riflette correttamente la logica di prevenzione dei duplicati ed è tradotta in modo chiaro.
app/Controllers/UserActionsController.php (2)
8-8: Import di SecureLogger correttoL’import di
App\Support\SecureLoggerè coerente con i nuovi utilizzi nel file e non introduce problemi di namespace.
217-221: Commento esplicativo incancelReservationIl commento aggiunto sul possibile uso improprio di
reservation_idper oggettiprestitichiarisce bene un edge case lato frontend. Nessun problema funzionale.app/Controllers/LibriController.php (2)
671-741: Allineamento gestionescraped_author_bioin store/updateL’uso di
$scrapedAuthorBio/$scrapedAuthorBioUpdateè ora consistente trastore()eupdate(), e l’aggiornamento condizionale delle biografie solo se vuote per il primo autore selezionato è coerente con il commento. Nessun problema evidente da segnalare.Also applies to: 1198-1265
1340-1375: Riassegnazione e gestione lista d’attesa su nuove copieLa sequenza “crea copia →
ReservationReassignmentService::reassignOnNewCopy()→ReservationManager::processBookAvailability()su ogni nuova copia è coerente con il modello a due livelli (prestiti prenotati + coda prenotazioni) e non introduce problemi di concorrenza evidenti, dato che il servizio gestisce i lock sulla copia. L’eventuale costo di istanziare i servizi dentro il loop è marginale rispetto al lavoro DB.app/Controllers/PrestitiController.php (2)
114-127: Guard contro prestiti/prenotazioni duplicate per stesso utente/libroLa query di controllo su
prestiticonattivo = 1estato IN ('in_corso','prenotato','pendente','in_ritardo')blocca correttamente la creazione di un nuovo prestito quando l’utente ha già un prestito/prenotazione attiva per lo stesso libro, come descritto nel commento “Case 8”. Comportamento chiaro e coerente con la regola di business.
760-773: Logging del rinnovo con SecureLoggerL’uso di
SecureLogger::warningper la validazione fallita eSecureLogger::errornel catch del rinnovo fornisce contesto strutturato (loan_id + messaggio) senza alterare il flusso di controllo. Nessuna anomalia evidente.app/Support/Updater.php (2)
490-594: Backup DB in streaming: soluzione robusta per tabelle grandiLa nuova implementazione di
backupDatabase()che usa:
- handle di file aperto in scrittura,
SHOW TABLES+SHOW CREATE TABLEper la struttura,real_query()+use_result()per leggere i dati riga per riga,INSERTscritti direttamente viafwrite(),riduce sensibilmente il rischio di esaurire la memoria su database voluminosi. La chiusura esplicita dell’handle e l’eliminazione del file parziale in caso di eccezione sono gestite correttamente.
Nessun problema funzionale evidente.
1458-1607: Lock di aggiornamento, gestione memoria e recovery modalità manutenzioneLe aggiunte in
performUpdate()e metodi correlati sono nel complesso ben strutturate:
- Il lock file
storage/cache/update.lockconflock(LOCK_EX|LOCK_NB)previene aggiornamenti concorrenti e fornisce un PID per il debug.- Il
register_shutdown_functionche, in caso di errore fatale, logga il problema e rimuove sia.maintenanceche il lock riduce il rischio di rimanere bloccati in modalità manutenzione.- L’aumento automatico del
memory_limitad almeno 256M (tramiteparseMemoryLimit()) aiuta i backup/aggiornamenti su installazioni con limiti troppo stretti.checkStaleMaintenanceMode()fornisce un recovery automatico se un precedente update ha lasciato.maintenancevecchio di oltre 30 minuti.La logica di cleanup finale (
cleanup()+ rilascio lock +unlink($lockFile)) copre correttamente sia il caso di successo che di errore.Also applies to: 1612-1631, 1637-1664
| /** | ||
| * Check and expire reservations past their due date (Case 4) | ||
| * | ||
| * Finds prestiti with stato='prenotato' where data_scadenza < today, | ||
| * marks them as 'scaduto', frees assigned copies, and triggers | ||
| * reassignment to next user in queue. | ||
| * | ||
| * @return int Number of reservations expired | ||
| * @throws \RuntimeException If query preparation fails | ||
| */ | ||
| public function checkExpiredReservations(): int | ||
| { | ||
| $today = date('Y-m-d'); | ||
|
|
||
| // Find expired reservations | ||
| $stmt = $this->db->prepare(" | ||
| SELECT id, libro_id, copia_id, utente_id | ||
| FROM prestiti | ||
| WHERE stato = 'prenotato' | ||
| AND attivo = 1 | ||
| AND data_scadenza < ? | ||
| "); | ||
|
|
||
| if (!$stmt) { | ||
| throw new \RuntimeException('Failed to prepare expired reservations query'); | ||
| } | ||
|
|
||
| $stmt->bind_param('s', $today); | ||
| $stmt->execute(); | ||
| $result = $stmt->get_result(); | ||
| $expiredReservations = $result ? $result->fetch_all(MYSQLI_ASSOC) : []; | ||
| $stmt->close(); | ||
|
|
||
| $expiredCount = 0; | ||
| $reassignmentService = new \App\Services\ReservationReassignmentService($this->db); | ||
| $integrity = new DataIntegrity($this->db); | ||
|
|
||
| foreach ($expiredReservations as $reservation) { | ||
| $this->db->begin_transaction(); | ||
|
|
||
| try { | ||
| $id = (int) $reservation['id']; | ||
| $copiaId = $reservation['copia_id'] ? (int) $reservation['copia_id'] : null; | ||
| $libroId = (int) $reservation['libro_id']; | ||
|
|
||
| // Mark as expired | ||
| $updateStmt = $this->db->prepare(" | ||
| UPDATE prestiti | ||
| SET stato = 'scaduto', | ||
| attivo = 0, | ||
| updated_at = NOW(), | ||
| note = CONCAT(COALESCE(note, ''), '\n[System] " . __('Scaduta il') . " " . date('d/m/Y') . "') | ||
| WHERE id = ? | ||
| "); | ||
| $updateStmt->bind_param('i', $id); | ||
| $updateStmt->execute(); | ||
| $updateStmt->close(); | ||
|
|
||
| // If a copy was assigned, make it available (if currently 'prenotato') | ||
| if ($copiaId) { | ||
| $checkCopy = $this->db->prepare("SELECT stato FROM copie WHERE id = ? FOR UPDATE"); | ||
| $checkCopy->bind_param('i', $copiaId); | ||
| $checkCopy->execute(); | ||
| $copyResult = $checkCopy->get_result(); | ||
| $copyState = $copyResult ? $copyResult->fetch_assoc() : null; | ||
| $checkCopy->close(); | ||
|
|
||
| if ($copyState && $copyState['stato'] === 'prenotato') { | ||
| // Update copy to available | ||
| $updateCopy = $this->db->prepare("UPDATE copie SET stato = 'disponibile' WHERE id = ?"); | ||
| $updateCopy->bind_param('i', $copiaId); | ||
| $updateCopy->execute(); | ||
| $updateCopy->close(); | ||
|
|
||
| // Trigger reassignment logic for this copy | ||
| $reassignmentService->reassignOnReturn($copiaId); | ||
| } | ||
| } | ||
|
|
||
| // Recalculate book availability | ||
| $integrity->recalculateBookAvailability($libroId); | ||
|
|
||
| $this->db->commit(); | ||
| $expiredCount++; | ||
|
|
||
| SecureLogger::info(__('MaintenanceService prenotazione scaduta'), [ | ||
| 'prestito_id' => $id, | ||
| 'libro_id' => $libroId, | ||
| 'copia_id' => $copiaId | ||
| ]); | ||
|
|
||
| } catch (\Throwable $e) { | ||
| $this->db->rollback(); | ||
| SecureLogger::error(__('MaintenanceService errore scadenza prenotazione'), [ | ||
| 'prestito_id' => $reservation['id'], | ||
| 'error' => $e->getMessage() | ||
| ]); | ||
| } | ||
| } | ||
|
|
||
| return $expiredCount; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*ReservationReassignmentService*"Repository: fabiodalez-dev/Pinakes
Length of output: 115
🏁 Script executed:
cat -n app/Services/ReservationReassignmentService.php | head -100Repository: fabiodalez-dev/Pinakes
Length of output: 4339
🏁 Script executed:
cat -n app/Services/ReservationReassignmentService.php | tail -n +100Repository: fabiodalez-dev/Pinakes
Length of output: 12128
🏁 Script executed:
sed -n '348,449p' app/Support/MaintenanceService.phpRepository: fabiodalez-dev/Pinakes
Length of output: 4055
🏁 Script executed:
rg -n "CONCAT.*__\(" app/ --type php -A 2 -B 2Repository: fabiodalez-dev/Pinakes
Length of output: 512
🏁 Script executed:
rg -n "bind_param.*note" app/ --type php -A 3 -B 3Repository: fabiodalez-dev/Pinakes
Length of output: 2204
Nested transactions in checkExpiredReservations() and SQL string concatenation for note field require fixes
Two critical issues:
-
Nested transactions with
ReservationReassignmentServicereassignOnReturn()callsreassignOnNewCopy(), which opensbegin_transaction()inside the outer transaction ofcheckExpiredReservations()- MySQL doesn't support true nested transactions; the inner
commit()/rollback()can interfere with the outer transaction, causing state visibility issues and potential partial commits - Move
$reassignmentService->reassignOnReturn($copiaId)outside the transaction (aftercommit()) so reassignment is attempted only after the reservation is persisted, or remove transaction management from the service to let the caller control the transaction scope
-
Localized string concatenated directly in SQL query
- The note update concatenates
__('Scaduta il')anddate('d/m/Y')directly into the SQL string without parameterization - If the translated string contains quotes or special characters, it breaks SQL syntax or enables injection
- Pass the note suffix as a bound parameter instead:
$noteSuffix = "\n[System] " . __('Scaduta il') . ' ' . date('d/m/Y'); $updateStmt = $this->db->prepare(" UPDATE prestiti SET stato = 'scaduto', attivo = 0, updated_at = NOW(), note = CONCAT(COALESCE(note, ''), ?) WHERE id = ? "); $updateStmt->bind_param('si', $noteSuffix, $id);
- The note update concatenates
🤖 Prompt for AI Agents
In app/Support/MaintenanceService.php around lines 348 to 449, fix two issues:
(1) avoid nested transactions by removing the call to
$reassignmentService->reassignOnReturn($copiaId) from inside the transaction —
commit the outer transaction first and then call reassignOnReturn() (or refactor
ReservationReassignmentService to not start/commit/rollback transactions so
callers control the transaction scope); (2) stop concatenating localized strings
into the SQL: build the note suffix in PHP (e.g. "\n[System] " . __('Scaduta
il') . ' ' . date('d/m/Y')) and pass it as a bound parameter to the UPDATE
statement using CONCAT(COALESCE(note, ''), ?) with appropriate bind_param('si',
$noteSuffix, $id).
| if (genreDisplay.level > 0) { | ||
| const backValue = genreDisplay.level === 1 ? '' : (genreDisplay.parent?.nome || ''); | ||
| const backEscaped = backValue.replace(/\\/g, '\\\\').replace(/'/g, "\\'"); | ||
| const backEscaped = backValue.replace(/\\\\/g, '\\\\\\\\').replace(/'/g, "\\\\'"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the actual file to see the context around the flagged lines
head -c 500000 app/Views/frontend/catalog.php | wc -lRepository: fabiodalez-dev/Pinakes
Length of output: 70
🏁 Script executed:
# Get line count of the file
wc -l app/Views/frontend/catalog.phpRepository: fabiodalez-dev/Pinakes
Length of output: 101
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '1760,1780p' app/Views/frontend/catalog.phpRepository: fabiodalez-dev/Pinakes
Length of output: 1254
🏁 Script executed:
# Also check the other lines flagged: 1777, 1804, 1815, 1838
sed -n '1800,1820p' app/Views/frontend/catalog.php
sed -n '1835,1845p' app/Views/frontend/catalog.phpRepository: fabiodalez-dev/Pinakes
Length of output: 2377
🏁 Script executed:
# Test the regex patterns to verify the reviewer's claim about what they match
node <<'EOF'
// Test the old pattern vs new pattern
const testStrings = [
'test', // no backslash
'test\\value', // single backslash (in JavaScript string: \\)
'test\\\\value', // double backslash (in JavaScript string: \\\\)
'test\\\\\\value', // triple backslash
];
console.log("=== OLD PATTERN: /\\\\/g (matches single backslash) ===");
testStrings.forEach(str => {
const result = str.replace(/\\/g, '\\\\');
console.log(`Input: "${str}" -> Output: "${result}"`);
});
console.log("\n=== NEW PATTERN: /\\\\\\\\/g (matches double backslash) ===");
testStrings.forEach(str => {
const result = str.replace(/\\\\/g, '\\\\\\\\');
console.log(`Input: "${str}" -> Output: "${result}"`);
});
console.log("\n=== Actual regex breakdown ===");
console.log("Old /\\\\/g : matches single backslash (\\)");
console.log("New /\\\\\\\\/g : matches two backslashes (\\\\)");
EOFRepository: fabiodalez-dev/Pinakes
Length of output: 642
🏁 Script executed:
# Check git history for this file
git log --oneline -20 app/Views/frontend/catalog.phpRepository: fabiodalez-dev/Pinakes
Length of output: 134
🏁 Script executed:
# Look at the actual diff to understand what changed
git log -p --max-count=5 app/Views/frontend/catalog.php | grep -A 10 -B 10 "replace.*\\\\\\\\" | head -100Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Search for references to this pattern in commits
git log --all --grep="escaping\|backslash\|HEREDOC" --oneline | head -10Repository: fabiodalez-dev/Pinakes
Length of output: 134
🏁 Script executed:
# Search for database migrations or seeding files
fd -e php -e sql | grep -i "migrat\|seed\|database" | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 2562
🏁 Script executed:
# Look for generi and editori table references
rg -n "generi|editori" --type=php -i | grep -i "CREATE\|INSERT\|TABLE" | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 2492
🏁 Script executed:
# Check for any data fixtures or seeders
find . -type f \( -name "*seed*" -o -name "*fixture*" -o -name "*migration*" \) | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 105
🏁 Script executed:
# Search for where generi.nome and editori.nome are populated or referenced
rg -n "generi.*nome|editori.*nome" --type=sql --type=php | head -30Repository: fabiodalez-dev/Pinakes
Length of output: 3130
🏁 Script executed:
# Look for HEREDOC syntax around the flagged lines
sed -n '1700,1850p' app/Views/frontend/catalog.php | head -200Repository: fabiodalez-dev/Pinakes
Length of output: 7251
🏁 Script executed:
# Search for any HEREDOC declarations in the file
rg -n "<<<" app/Views/frontend/catalog.php | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 93
🏁 Script executed:
# Check if there's documentation or comments about the escaping issue
rg -n -B 5 "replace.*\\\\\\\\" app/Views/frontend/catalog.php | head -80Repository: fabiodalez-dev/Pinakes
Length of output: 2046
🏁 Script executed:
# Look at how the escaped value is used - specifically the onclick handler
sed -n '1763,1770p' app/Views/frontend/catalog.php
sed -n '1777,1784p' app/Views/frontend/catalog.php
sed -n '1804,1811p' app/Views/frontend/catalog.phpRepository: fabiodalez-dev/Pinakes
Length of output: 1612
🏁 Script executed:
# Create a test script to understand PHP HEREDOC escaping
php <<'EOF'
<?php
// Test how backslashes work in HEREDOC
$test_heredoc = <<<JS
const pattern1 = /\\/g;
const pattern2 = /\\\\/g;
const replacement1 = '\\\\';
const replacement2 = '\\\\\\\\';
JS;
echo "=== PHP HEREDOC Raw Content ===\n";
echo $test_heredoc;
echo "\n\n=== Interpreted by JavaScript ===\n";
echo json_encode($test_heredoc);
EOFRepository: fabiodalez-dev/Pinakes
Length of output: 356
🏁 Script executed:
# Now test what the actual regex patterns do
php <<'EOF'
<?php
// Simulate what the PHP code actually does
$test_values = [
'normal_genre',
"genre'with'quote",
'genre\\with\\backslash',
'genre\\\\with\\\\double'
];
$heredoc = <<<JS
// This is what's in the actual code:
function testEscaping(value) {
return value.replace(/\\\\/g, '\\\\\\\\').replace(/'/g, "\\\\'");
}
JS;
echo "PHP HEREDOC source code:\n";
echo $heredoc;
echo "\n\n=== What JavaScript will receive ===\n";
echo json_encode($heredoc, JSON_PRETTY_PRINT);
EOFRepository: fabiodalez-dev/Pinakes
Length of output: 415
🏁 Script executed:
# Test the actual escaping with Node.js to see what JavaScript receives from PHP
node <<'EOF'
// This represents what PHP sends to the browser (after HEREDOC interpretation)
const phpHeredocOutput = `function testEscaping(value) {
return value.replace(/\\\\/g, '\\\\\\\\').replace(/'/g, "\\\\'");
}`;
console.log("=== What PHP HEREDOC sends to browser ===");
console.log(phpHeredocOutput);
console.log("\n=== Testing the regex pattern ===");
const testCases = [
'normal_genre',
"genre'with'quote",
'genre\\with\\backslash',
'genre\\\\with\\\\double'
];
testCases.forEach(testVal => {
const result = testVal.replace(/\\\\/g, '\\\\\\\\').replace(/'/g, "\\\\'");
console.log(`Input: ${JSON.stringify(testVal)} -> Output: ${JSON.stringify(result)}`);
});
EOFRepository: fabiodalez-dev/Pinakes
Length of output: 495
🏁 Script executed:
# Test with character-by-character inspection
node <<'EOF'
function inspect(str) {
let chars = '';
for (let i = 0; i < str.length; i++) {
chars += `[${str.charCodeAt(i)}:${str[i]}]`;
}
return chars;
}
// Test what the actual regex pattern does
const testCases = [
{ desc: 'single backslash', input: 'a\\b' },
{ desc: 'double backslash', input: 'a\\\\b' },
{ desc: 'triple backslash', input: 'a\\\\\\b' },
{ desc: 'quad backslash', input: 'a\\\\\\\\b' }
];
console.log("=== Testing /\\\\/g pattern (from PHP HEREDOC) ===");
testCases.forEach(tc => {
const result = tc.input.replace(/\\\\/g, '\\\\\\\\');
console.log(`${tc.desc}:`);
console.log(` Input chars: ${inspect(tc.input)}`);
console.log(` Input visual: "${tc.input}"`);
console.log(` Output chars: ${inspect(result)}`);
console.log(` Output visual: "${result}"`);
console.log();
});
console.log("\n=== What /\\\\/g actually matches ===");
const testStr = 'a\\b\\\\c\\\\\\d';
console.log(`Test string: "${testStr}"`);
console.log(`Character breakdown: ${inspect(testStr)}`);
console.log(`Matches for /\\\\\\\\/g:`);
const matches = testStr.match(/\\\\/g);
console.log(`Found ${matches ? matches.length : 0} matches`);
EOFRepository: fabiodalez-dev/Pinakes
Length of output: 967
🏁 Script executed:
# Extract the exact pattern as it appears in the source code
sed -n '1763p' app/Views/frontend/catalog.php | cat -ARepository: fabiodalez-dev/Pinakes
Length of output: 165
🏁 Script executed:
# Use hexdump to see the exact bytes
sed -n '1763p' app/Views/frontend/catalog.php | od -cRepository: fabiodalez-dev/Pinakes
Length of output: 108
🏁 Script executed:
# Search for any recent changes to this pattern using git
git log -p --all -- app/Views/frontend/catalog.php | grep -B 5 -A 5 "replace.*backslash\|replace.*\\\\\\\\\|updateFilter.*genere" | head -150Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Check if there are any comments or issues mentioning this escaping change
rg -n "escap\|HEREDOC\|regex.*issue" app/Views/frontend/catalog.php -A 2 -B 2 | head -50Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Look for the PR or commit message that mentions "JavaScript regex issue"
git log --all --oneline | grep -i "escap\|regex\|javascript\|HEREDOC" | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Check if there's a way to see what the pattern was before
git show HEAD:app/Views/frontend/catalog.php | sed -n '1763p' 2>/dev/null || echo "Pattern not found in HEAD"Repository: fabiodalez-dev/Pinakes
Length of output: 164
🏁 Script executed:
# Test if the current escaping prevents JavaScript errors in onclick handlers
node <<'EOF'
// Simulate the escaping and onclick handler execution
function escapeForOnclick(value) {
return value.replace(/\\\\/g, '\\\\\\\\').replace(/'/g, "\\\\'");
}
// Test cases that could appear in genre/publisher names
const testCases = [
'Science Fiction',
"O'Reilly",
'C++ Programming',
'Linux\\Unix (hypothetical)',
'Back\\\\slash (hypothetical)'
];
console.log("=== Testing escaping for onclick safety ===\n");
testCases.forEach(testVal => {
const escaped = escapeForOnclick(testVal);
const onclickCode = `onclick="updateFilter('genere', '${escaped}'); return false;"`;
console.log(`Original: "${testVal}"`);
console.log(`Escaped: "${escaped}"`);
console.log(`Onclick: ${onclickCode}`);
// Try to parse the onclick as JavaScript to check for syntax errors
try {
// Simulate what the browser would see: the onclick attribute value
const jsCode = `updateFilter('genere', '${escaped}'); return false;`;
// Just check if it's valid JavaScript syntax
new Function(jsCode);
console.log(`✓ Valid JavaScript syntax`);
} catch (e) {
console.log(`✗ SYNTAX ERROR: ${e.message}`);
}
console.log();
});
EOFRepository: fabiodalez-dev/Pinakes
Length of output: 970
🏁 Script executed:
# Search for any README or documentation about escaping
find . -name "README*" -o -name "*.md" | grep -v node_modules | xargs grep -l "escap\|HEREDOC" 2>/dev/null | head -5Repository: fabiodalez-dev/Pinakes
Length of output: 141
🏁 Script executed:
# Check the catalog.php file for any context comments
sed -n '1450,1470p' app/Views/frontend/catalog.phpRepository: fabiodalez-dev/Pinakes
Length of output: 811
🏁 Script executed:
# Look for updateFilter function definition to understand parameter handling
rg -n "function updateFilter\|const updateFilter" app/Views/frontend/catalog.php -A 10 | head -40Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Search for any CSV files or data imports that might contain backslashes
find . -name "*.csv" -o -name "*import*" -o -name "*seed*" | grep -v vendor | grep -v node_modulesRepository: fabiodalez-dev/Pinakes
Length of output: 140
🏁 Script executed:
# Look for sample data or documentation about genre names
find . -type f \( -name "*.sql" -o -name "*.json" \) | xargs grep -l "generi\|genre" 2>/dev/null | grep -v vendor | head -5Repository: fabiodalez-dev/Pinakes
Length of output: 267
🏁 Script executed:
# Check if there's any validation or sanitization of genre/publisher names
rg -n "nome.*sanitize\|nome.*filter\|nome.*trim\|nome.*strip" app/Controllers/ --type=php | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Look for database schema definition
find . -name "*.sql" -type f | grep -v vendor | xargs grep -n "CREATE TABLE.*generi\|CREATE TABLE.*editori" 2>/dev/nullRepository: fabiodalez-dev/Pinakes
Length of output: 186
🏁 Script executed:
# Check migrations folder
find . -type d -name "*migration*" | grep -v vendorRepository: fabiodalez-dev/Pinakes
Length of output: 97
🏁 Script executed:
# Check the schema for generi and editori tables
sed -n '198,220p' installer/database/schema.sql
sed -n '147,170p' installer/database/schema.sqlRepository: fabiodalez-dev/Pinakes
Length of output: 3101
🏁 Script executed:
# Check the sample data to see if any backslashes exist
head -50 installer/database/data_it_IT.sql | grep -i "generi\|editori"Repository: fabiodalez-dev/Pinakes
Length of output: 5448
🏁 Script executed:
# Search for backslashes in the SQL data files
grep -n "\\\\" installer/database/data_*.sql | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 19608
Escaping pattern appears correct for actual use case, but verify data requirements
The pattern /\\\\/g (in source code) translates to /\\/g in JavaScript and correctly matches single backslashes in runtime data. Testing confirms it properly escapes the apostrophe case (O'Reilly → O\'Reilly), which is essential to prevent JavaScript syntax errors in the onclick handler.
Sample genre and publisher data contains no backslashes—all special characters are apostrophes in Italian contractions (e.g., "d'avventura", "dell'orrore"). The escaping chain works correctly for this realistic data.
However, clarify the PR context: no git history shows a prior pattern change from /\\/g to /\\\\/g. If this was recently changed, verify the old pattern also worked correctly or document what specific issue this fix resolves. If backslashes may appear in imported data (CSV uploads), add explicit validation to remove or reject them rather than relying on escaping.
| `sede_id` int DEFAULT NULL, | ||
| `numero_inventario` varchar(100) COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| `stato` enum('disponibile','prestato','manutenzione','perso','danneggiato') COLLATE utf8mb4_unicode_ci DEFAULT 'disponibile', | ||
| `stato` enum('disponibile','prestato','prenotato','manutenzione','in_restauro','perso','danneggiato','in_trasferimento') COLLATE utf8mb4_unicode_ci DEFAULT 'disponibile', |
There was a problem hiding this comment.
Inconsistenza: validazione stati in CopyController non aggiornata.
L'enum copie.stato ora include prenotato, in_restauro, e in_trasferimento, ma la validazione in CopyController.php (linea 60) accetta solo ['disponibile', 'prestato', 'manutenzione', 'danneggiato', 'perso']. Inoltre, la logica di reassignment (linea 140) usa in_restauro che non è nella lista di validazione.
Questo causerà errori se si tenta di impostare questi nuovi stati tramite l'interfaccia admin.
È necessario aggiornare CopyController.php:
- $statiValidi = ['disponibile', 'prestato', 'manutenzione', 'danneggiato', 'perso'];
+ $statiValidi = ['disponibile', 'prestato', 'prenotato', 'manutenzione', 'in_restauro', 'danneggiato', 'perso', 'in_trasferimento'];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In installer/database/schema.sql line 115 and CopyController.php around lines 60
and 140, the DB enum for copie.stato now includes 'prenotato', 'in_restauro',
and 'in_trasferimento' but the controller's validation list (line ~60) and
reassignment logic (line ~140) are out of sync; update the validation array to
include 'prenotato', 'in_restauro', and 'in_trasferimento' and ensure the
reassignment logic uses the same canonical state names (either include
'in_restauro' in the allowed list or remove its special-case use) so the
controller accepts and handles the new DB states consistently.
- Fix nested transactions in ReservationReassignmentService
- Added isInTransaction() to detect active transactions via @@in_transaction
- Added beginTransactionIfNeeded(), commitIfOwned(), rollbackIfOwned() helpers
- Updated reassignOnNewCopy() and reassignOnCopyLost() to use transaction-aware methods
- Fixed race condition in reassignOnCopyLost() - now verifies copy state after lock
- Add missing states to CopyController validation
- Added prenotato, in_restauro, in_trasferimento to match DB enum
- Add soft delete filtering to BookRepository queries
- Added WHERE l.deleted_at IS NULL to listWithAuthors(), getById(),
getByAuthorId(), getByPublisherId()
- Add prenotazioni table check in book deletion guard
- Now blocks deletion if active waitlist entries exist (stato='attiva')
- Add annullato/scaduto states to CSV export
- Updated validStates and statusLabels arrays
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/Services/ReservationReassignmentService.php (1)
368-380: Logging dell’email in chiaro: PII non necessaria nei logNel ramo di successo/errore:
SecureLogger::info(__('Notifica prenotazione disponibile inviata'), [ 'prestito_id' => $prestitoId, 'email' => $data['email'] ]); ... SecureLogger::warning(__('Invio notifica prenotazione fallito'), [ 'prestito_id' => $prestitoId, 'email' => $data['email'] ]);stai registrando l’indirizzo email completo. Per minimizzare PII nei log, valuterei di:
- rimuovere il campo
- mascherarlo (es. solo dominio, oppure hash irreversibile) se serve per correlazioni.
Esempio:
SecureLogger::info(__('Notifica prenotazione disponibile inviata'), [ 'prestito_id' => $prestitoId, // 'email' omessa o mascherata per privacy ]);
🧹 Nitpick comments (1)
app/Services/ReservationReassignmentService.php (1)
95-167: Finestra di race sulla prenotazione selezionata (stato può cambiare prima dell’UPDATE)
reassignOnNewCopy()seleziona la prenotazione “bloccata” fuori dalla transazione, poi dentro la transazione aggiorna solocopia_id:$reservation = ... ... UPDATE prestiti SET copia_id = ? WHERE id = ?Nel frattempo, un altro processo può aver marcato quella prenotazione come
annullato/scaduto/attivo=0. In questo caso assegneresti comunque una copia a una prenotazione ormai non più valida.Per rendere il flusso più robusto:
- aggiungi una condizione all’UPDATE (
WHERE id = ? AND stato = 'prenotato' AND attivo = 1),- e controlla
affected_rowsper capire se l’aggiornamento è andato a buon fine (in caso contrario esci/rollback).Questo evita di “rianimare” prenotazioni che sono state chiuse in parallelo.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Controllers/CopyController.php(3 hunks)app/Controllers/LibriController.php(5 hunks)app/Controllers/PrestitiController.php(12 hunks)app/Models/BookRepository.php(22 hunks)app/Services/ReservationReassignmentService.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/Controllers/PrestitiController.php (3)
app/Models/LoanRepository.php (1)
close(101-175)app/helpers.php (1)
__(25-28)app/Services/ReservationReassignmentService.php (2)
ReservationReassignmentService(17-459)reassignOnReturn(266-283)
app/Services/ReservationReassignmentService.php (2)
app/Support/NotificationService.php (1)
NotificationService(11-1362)app/Models/CopyRepository.php (2)
CopyRepository(8-245)updateStatus(92-100)
app/Models/BookRepository.php (3)
app/Models/AuthorRepository.php (1)
__construct(10-10)app/Models/PublisherRepository.php (1)
__construct(10-10)app/Models/DashboardStats.php (1)
__construct(10-10)
app/Controllers/CopyController.php (1)
app/Services/ReservationReassignmentService.php (3)
ReservationReassignmentService(17-459)reassignOnCopyLost(173-245)reassignOnReturn(266-283)
🔇 Additional comments (22)
app/Controllers/CopyController.php (2)
59-61: Allineamento stati validi con i casi gestiti a valleL’estensione di
$statiValidiper includereprenotato,in_restauroein_trasferimentorende coerente la validazione con i casi di business gestiti più sotto (es. branch suin_restauro). Buon allineamento enum ↔ logica applicativa.
135-153: Integrazione conReservationReassignmentServiceben incapsulataL’invocazione del servizio di riassegnazione dopo l’
UPDATE copie:
- gestisce il caso “copia non più utilizzabile” (
perso,danneggiato,manutenzione,in_restauro) tramitereassignOnCopyLost,- e il caso “copia torna disponibile” tramite
reassignOnReturn,- incapsulata in un
try/catchche logga conSecureLoggerma non blocca il flusso utente.Struttura solida e coerente con il nuovo servizio; non vedo problemi di correttezza qui.
app/Models/BookRepository.php (11)
23-46: Filtro soft delete correttamente applicato alle listeL’aggiunta di
WHERE l.deleted_at IS NULLinlistWithAuthors()e nelle altre query principali nasconde i libri soft-deleted senza cambiare la firma pubblica dei metodi. Questo allinea il repository alla semantica del nuovodelete()che aggiornadeleted_at.
48-123:getById()coerente con soft delete e con gerarchia generiL’aggiunta del filtro
l.deleted_at IS NULLingetById()e la riorganizzazione dei nomi di genere/radice in base ai livelli (nonno_idecc.) sono coerenti con il modello dati esistente; il ciclo su$authorsResè semplice e sicuro. Nessuna issue evidente.
181-396: Normalizzazione input increateBasicrobusta (ISBN/EAN, collocazione, copie, tipi numerici)La nuova logica:
- converte scaffale/mensola/posizione in interi con sentinel
nullper valori ≤ 0,- normalizza ISBN/EAN eliminando whitespace e convertendo stringhe vuote in
null,- forza
peso/prezzoafloatsolo quando presenti, e- inizializza
copie_totali/copie_disponibilicon cast espliciti a int.Questo riduce parecchi edge case su vincoli DB e tipi; il binding dinamico via
$addFieldresta coerente.
398-604:updateBasicallineato alla logica di creazione (normalizzazioni + logging mirato)La simmetria con
createBasic(cast per scaffale/mensola/posizione, normalizzazione ISBN/EAN/peso/prezzo, gestione campi opzionali tramite$addSet) migliora la consistenza tra insert e update. Anche il logging debug condizionato adAPP_ENV=developmentè prudente lato sicurezza.
631-654:syncAuthors: early-return su lista vuota rende più chiaro il flussoL’early-return quando
$authorIdsè vuoto evita l’esecuzione inutile dell’INSERT, mantenendo comunque la cancellazione precedente dei legami autore–libro. Flusso pulito e prevedibile.
656-672:processAuthorId: gestione backward‑compat per formati diversi di autoreLa funzione gestisce sia:
- ID numerici puri, sia
- formati stringa tipo
new_...(Choices.js) che vengono ignorati (ritorno 0), sia- altri valori che vengono castati a int.
È una soluzione pragmatica per supportare formati legacy e nuovi senza rompere il codice chiamante.
720-727: Implementazione soft delete sudelete()Sostituire il
DELETEfisico conUPDATE libri SET deleted_at = NOW()preserva storia e integrità referenziale con il minimo impatto. Dato che le principali query ora filtranodeleted_at IS NULL, il comportamento esterno è coerente con “cancellazione” logica.
729-774:updateOptionals: aggiornamento dinamico dei campi opzionali ben tipizzatoLa costruzione di
$colse poi di$set/$types/$vals:
- converte correttamente
numero_pagine/anno_pubblicazionea interi e il resto a stringa,- onora la presenza di colonne tramite
hasColumn,- evita UPDATE se
$colsè vuoto.Buon compromesso tra flessibilità e sicurezza tipica.
776-789: Cache colonne tabella per ridurre round‑trip al DB
hasColumn()con cache in$columnCacheevitaSHOW COLUMNSripetuti. Logica semplice e thread-safe per singola richiesta; nessuna issue.
791-807: Whitelist tabelle inhasColumnInTable: mitigazione SQL injection su INFORMATION_SCHEMALa whitelist esplicita di tabelle (
libri,autori,libri_autori, ecc.) prima di interrogareINFORMATION_SCHEMA.COLUMNSè un buon hardening: impedisce che nomi tabella controllati dall’utente entrino nella query.
833-854:getEnumOptionscon cache per chiavitable.columnLa cache
$enumCachecon chiave$table . '.' . $columnevita query ripetute suINFORMATION_SCHEMAe semplifica l’uso dinormalizeEnumValue. Implementazione corretta e leggibile.app/Controllers/LibriController.php (3)
9-10: Uso coerente diSecureLoggernel controller libriL’import di
App\Support\SecureLoggerallinea questo controller agli altri (prestiti, copie) per logging strutturato, mantenendoerror_logsolo dove strettamente necessario. Buona convergenza di stile.
1351-1374: Aggancio della riassegnazione e della waitlist durante l’aumento delle copieNel ramo
update()dove aumenticopie_totali:
- crei nuove copie via
CopyRepository::create,- per ogni nuova copia invochi
ReservationReassignmentService->reassignOnNewCopy($id, $newCopyId)intry/catch,- poi
ReservationManager->processBookAvailability($id)per trasformare prenotazioni in prestiti quando c’è capacità.Questo collega bene il lifecycle delle copie con la coda di prenotazione, e l’uso di
SecureLoggernei catch evita che un errore di riassegnazione blocchi l’update del libro.
1661-1695: Guard di cancellazione libro ora considera sia prestiti sia prenotazioni attiveIl nuovo blocco in
delete():
- conta prestiti attivi con stati
'in_corso','prenotato','pendente','in_ritardo',- conta prenotazioni da
prenotazioniconstato='attiva',- blocca la cancellazione se uno dei due conteggi è > 0, con messaggio chiaro.
Questo chiude il buco segnalato in passato (prenotazioni attive non considerate) e previene inconsistenze lato utente.
app/Controllers/PrestitiController.php (5)
114-127: Blocco di doppi prestiti/prenotazioni attive per stesso utente/libroIl controllo:
SELECT id FROM prestiti WHERE libro_id = ? AND utente_id = ? AND attivo = 1 AND stato IN ('in_corso', 'prenotato', 'pendente', 'in_ritardo')prima dell’inserimento impedisce che lo stesso utente abbia più prestiti/prenotazioni “attive” sullo stesso libro nella tabella
prestiti. È una buona salvaguardia lato UX; tieni solo presente che non considera la tabellaprenotazioni, quindi eventuali code ancora lì verranno gestite dal nuovo sistema e non da questo guard (scelta di design, non un bug).
588-626: Riassegnazione della copia restituita + gestione lista d’attesa ben incapsulateNel
processReturn():
- dopo aver reso la copia
disponibile, chiamiReservationReassignmentService->reassignOnReturn($copia_id)per assegnarla alla prenotazione bloccata più vecchia (nuovo sistema basato suprestiti),- poi
ReservationManager->processBookAvailability($libro_id)per gestire prenotazioni future/legacy,- il tutto dentro
try/catchcon logging tramiteSecureLogger.Questo collega correttamente il flusso di restituzione con la coda di prenotazioni, e il log d’errore (
Riassegnazione copia fallita) fornisce contesto utile senza interrompere la restituzione.
609-611: Log di validazione prestito non bloccante ma visibileIn
processReturn()sevalidateAndUpdateLoan()fallisce:SecureLogger::warning(__('Validazione prestito fallita'), ['loan_id' => $id, 'message' => $validationResult['message']]);è una buona scelta: l’utente non viene bloccato dalla restituzione, ma l’anomalia resta tracciata per diagnosi.
759-773: Rinnovo: logging strutturato su fallimenti di validazione/DBNel
renew():
- se
validateAndUpdateLoan()fallisce, logghi un warning conloan_ide messaggio,- in caso di eccezione, rollback e
SecureLogger::error('Rinnovo prestito fallito', ...).Questo rende il percorso di rinnovo molto più osservabile rispetto a
error_log, senza cambiare il comportamento per l’utente finale.
821-919: Export CSV: supporto per nuovi stati e logging errori queryIn
exportCsv():
validStatese$statusLabelsincludono oraannullatoescaduto, allineati al nuovo enum,- se la query senza parametri fallisce, logghi l’errore DB con
SecureLogger::error,- il resto della generazione CSV (sanitizzazione formula-injection, header i18n) resta invariato.
L’aggiornamento è coerente con le nuove transizioni di stato.
app/Services/ReservationReassignmentService.php (1)
313-381: Method exists and is correctly implementedThe method
sendReservationBookAvailable()does exist inNotificationServiceat lines 990–991. The call is correct and will not cause a runtime error. The suggested alternative method namenotifyReservationBookAvailable()does not exist in the codebase.Likely an incorrect or invalid review comment.
- Fix PII logging: replace email with utente_id in ReservationReassignmentService - Fix nested transactions: use setExternalTransaction(true) in MaintenanceService - Fix SQL injection: use bound parameter for note suffix in checkExpiredReservations - Fix idx_ruolo index: rename to idx_tipo_utente (ruolo column doesn't exist in utenti) - Add idx_tipo_utente to installer schema.sql for fresh installations - Add DataIntegrity check #11: verify annullato/scaduto loans have attivo=0 - Add DataIntegrity fix #3b: correct annullato/scaduto loans with attivo=1 - Update version to 0.4.2
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
installer/database/schema.sql (1)
115-115: LGTM! L'enum copie.stato è stato esteso correttamente.I nuovi stati (
prenotato,in_restauro,in_trasferimento) sono coerenti con le funzionalità di riassegnazione prenotazioni introdotte in questo PR.Nota: esiste già un commento precedente che segnala la necessità di aggiornare
CopyController.phpper accettare questi nuovi stati nella validazione.app/Services/ReservationReassignmentService.php (1)
207-216: Branch di fallback incompleto: la secondafindAvailableCopynon riassegna la prenotazione.Quando la prima copia candidata non è più disponibile (race condition), il codice cerca una nuova copia con
findAvailableCopy()ma se la trova, ritorna senza tentare la riassegnazione. La prenotazione rimane senza copia assegnata.Soluzione: se
$nextCopyIdè trovato, dovrebbe ritentare l'assegnazione (possibilmente con un loop limitato per evitare cicli infiniti).if (!$copyStatus || $copyStatus['stato'] !== 'disponibile') { $this->rollbackIfOwned($ownTransaction); // Prova a cercare un'altra copia $nextCopyId = $this->findAvailableCopy((int) $reservation['libro_id'], $copiaId); if (!$nextCopyId) { // Nessuna copia disponibile $this->handleNoCopyAvailable((int) $reservation['id']); + } else { + // Riprova con la nuova copia trovata (ricorsione limitata) + $this->reassignOnCopyLost($copiaId); } return; }
🧹 Nitpick comments (2)
app/Support/MaintenanceService.php (1)
68-82: Allinea le PHPDoc con il nuovo campoexpired_reservations
runAll()erunIfNeeded()ora popolanoexpired_reservationsnel risultato, ma le PHPDoc degli array-shape non lo dichiarano. Questo può confondere gli strumenti statici e gli IDE.Suggerisco di aggiornare le annotazioni:
- In
runIfNeeded(): aggiungereexpired_reservations?: intal tipo di ritorno.- In
runAll(): aggiungereexpired_reservations: intal tipo di ritorno.Also applies to: 45-59, 551-557
app/Services/ReservationReassignmentService.php (1)
250-260:handleNoCopyAvailableesegue UPDATE fuori dalla transazione.Il metodo
handleNoCopyAvailableè chiamato doporollbackIfOwned()(linea 209) o nel branchelse(linea 243), ma esegue un UPDATE senza gestione transazionale propria. Se chiamato dopo un rollback, l'UPDATE viene eseguito senza protezione atomica.Considera di avvolgere l'UPDATE in una transazione o di documentare che è intenzionalmente non-transazionale.
private function handleNoCopyAvailable(int $reservationId): void { + $ownTransaction = $this->beginTransactionIfNeeded(); + try { $stmt = $this->db->prepare("UPDATE prestiti SET copia_id = NULL WHERE id = ?"); $stmt->bind_param('i', $reservationId); $stmt->execute(); $stmt->close(); + $this->commitIfOwned($ownTransaction); + } catch (Exception $e) { + $this->rollbackIfOwned($ownTransaction); + SecureLogger::error(__('Errore gestione prenotazione senza copia'), [ + 'reservation_id' => $reservationId, + 'error' => $e->getMessage() + ]); + } $this->notifyUserCopyUnavailable($reservationId, 'lost_copy'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/Services/ReservationReassignmentService.php(1 hunks)app/Support/DataIntegrity.php(3 hunks)app/Support/MaintenanceService.php(7 hunks)installer/database/indexes_optimization.sql(1 hunks)installer/database/indexes_optimization_mysql.sql(1 hunks)installer/database/schema.sql(4 hunks)locale/en_US.json(6 hunks)version.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- version.json
🔇 Additional comments (14)
locale/en_US.json (1)
1689-1703: Nuove chiavi di traduzione per manutenzione/prenotazioni coerentiLe stringhe aggiunte per
MaintenanceService, integrità dati e flussi di prenotazione/prestito sono consistenti con l’uso nel codice (__()usa esattamente queste chiavi) e i placeholder (%d,%s) risultano corretti. Nulla da segnalare lato funzionale.Also applies to: 3448-3624
installer/database/indexes_optimization_mysql.sql (1)
79-83: Indiceidx_tipo_utentesuutenticoerente con gli accessi attesiLa sostituzione/aggiunta dell’indice su
utenti.tipo_utenteviaadd_index_if_not_existsè allineata alle query tipiche (filtri per ruolo utente) e sicura grazie al controllo suINFORMATION_SCHEMA.STATISTICS. Nulla da correggere.installer/database/indexes_optimization.sql (1)
86-91: Allineamento indiceidx_tipo_utenteanche nello script genericoL’aggiunta di
idx_tipo_utente (tipo_utente)nello script MariaDB mantiene consistente la strategia d’indicizzazione tra ambienti e supporta i filtri per tipo utente. Modifica corretta.app/Support/MaintenanceService.php (1)
223-251: La struttura transazionale è corretta:ReservationReassignmentServicerispetta il contrattosetExternalTransaction(true)La verifica conferma che il servizio implementa correttamente il pattern per evitare transazioni annidate:
setExternalTransaction()è presente (riga 35-39 del service) e imposta il flag$externalTransaction- Quando il flag è true,
isInTransaction()ritorna true (riga 46-48), causandobeginTransactionIfNeeded()a non eseguirebegin_transaction()- Solo
commit()erollback()vengono eseguiti se il servizio ha iniziato la transazione ($ownTransactionè true)- In
MaintenanceService, il flag è correttamente impostato a riga 384 prima di usare il servizio nel loopreassignOnReturn()è chiamato dentro la stessa transazione (riga 408) e rispetta il vincoloIl coordinamento transazionale è solido e previene i problemi di transazioni annidate in MySQL.
installer/database/schema.sql (4)
342-342: LGTM! Soft delete implementato correttamente.La colonna
deleted_atnullable è il pattern standard per il soft delete. Assicurati che le query di lettura escludano i record condeleted_at IS NOT NULLdove appropriato.
596-596: LGTM! Nuovi stati prestiti aggiunti.Gli stati
annullatoescadutosono coerenti con le migrazionimigrate_0.4.3.sqle con i nuovi controlli inDataIntegrity.php.
749-750: LGTM! Indici aggiuntivi per ottimizzazione query.Gli indici su
data_ultimo_accessoetipo_utentemiglioreranno le performance delle query di reporting e filtraggio utenti.
113-113: No action needed. Theseditable does not exist in the schema, so the concern about missing foreign key constraints onsede_idis not applicable to this database schema. Ifsediis intended to be a reference table, it should be defined in the schema; otherwise, thesede_idcolumn may be managed through application logic rather than database-level constraints.Likely an incorrect or invalid review comment.
app/Support/DataIntegrity.php (3)
568-586: LGTM! Verifica coerente per prestiti terminati.Il controllo per prestiti con stato
annullato/scadutoche hanno ancoraattivo = 1è corretto e la severitàerrorè appropriata dato che rappresenta un'incoerenza nei dati.
706-714: LGTM! Correzione automatica coerente.La correzione che imposta
attivo = 0per i prestiti terminati è l'azione corretta per risolvere l'incoerenza rilevata dal check #11.
1076-1076: LGTM! Nome indice allineato allo schema.Il cambio da
idx_ruoloaidx_tipo_utenteè coerente con la definizione dello schema inschema.sql(linea 750).app/Services/ReservationReassignmentService.php (3)
440-458: LGTM! Gestione URL base con fallback robusto.La logica gestisce correttamente:
- Variabile d'ambiente
APP_CANONICAL_URL- Fallback a
$_SERVERcon validazione del formato host- Default sicuro a
localhost
1-29: LGTM! Struttura del servizio ben organizzata.La classe segue buone pratiche:
- Strict types abilitati
- Dependency injection del database connection
- Servizi correlati inizializzati nel costruttore
- Flag per gestione transazioni esterne
95-167: Gestione transazioni migliorata rispetto alla versione precedente.La logica di
reassignOnNewCopyora utilizza correttamentebeginTransactionIfNeeded()ecommitIfOwned()per supportare sia chiamate standalone che chiamate all'interno di transazioni esterne. Il lockFOR UPDATEe la verifica dello stato della copia proteggono dalle race condition.
- Replace MariaDB-specific @@in_transaction with MySQL-compatible @@autocommit check - Fix wrong enum value 'autore' -> 'principale' in author query - Add retry loop with max attempts in reassignOnCopyLost for race conditions - Wrap handleNoCopyAvailable UPDATE in proper transaction handling - Add new findAvailableCopyExcluding method for efficient copy exclusion - Add English translations for new log messages
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/Services/ReservationReassignmentService.php (4)
97-169: Rischio di race sulla scelta della prenotazione inreassignOnNewCopyLa logica è chiara e l’uso di
SELECT ... FOR UPDATEsulla copia + notifica post‑commit è ottimo. Rimane però una piccola finestra di race:
- La prenotazione “bloccata” viene letta fuori transazione (righe 101‑116).
- Due worker che chiamano
reassignOnNewCopy()quasi in parallelo (es. per due nuove copie dello stesso libro) potrebbero selezionare la stessa prenotazione e poi aggiornarla entrambi, con l’ultimo commit che vince e una delle nuove copie marcataprenotatoma senza prestito associato.Per ridurre questo rischio ti proporrei, quando/ se la concorrenza diventa un problema:
- spostare la SELECT della prenotazione dentro la transazione e usare
FOR UPDATEsuprestiti, oppure- usare una clausola aggiuntiva nell’UPDATE tipo
... WHERE id = ? AND stato = 'prenotato'e controllareaffected_rowsper capire se qualcuno l’ha già presa nel frattempo, per poi ritentare.Non è un blocker per un contesto a bassa concorrenza, ma vale la pena tenerlo a mente.
267-290:handleNoCopyAvailable: semantica corretta, ma attenzione all’uso in transazioni esterneLa scelta di azzerare
copia_ide lasciare la prenotazione attiva (in coda) è sensata, e la notifica viene giustamente inviata dopo il commit. Solo una nota di utilizzo:
- Anche qui usi
beginTransactionIfNeeded()/commitIfOwned(). Se in futuro questo metodo venisse chiamato dentro una transazione aperta altrove senzasetExternalTransaction(true), si torna al problema di possibile nested‑transaction/implicit commit discusso sopra.Valuterei di documentare chiaramente che i metodi pubblici del servizio (compreso il percorso che passa da
handleNoCopyAvailable) vanno usati fuori da transazioni esplicite, a meno di impostare esplicitamente l’“external transaction mode”.
315-341: Metodo privato non usatofindAvailableCopyPHPMD segnala (e il file conferma) che
findAvailableCopy()non è referenziato: tutta la logica reale passa perfindAvailableCopyExcluding().Per evitare dead code e futuri fraintendimenti, ti suggerirei di:
- rimuovere il metodo se non è realmente previsto un utilizzo, oppure
- promuoverlo a helper comune e riusarlo (ad es. facendolo chiamare da
findAvailableCopyExcluding()quandoexcludeCopiaIdsè vuoto).
452-501:notifyUserCopyUnavailable: coerenza con NotificationService, ma il docblock è un po’ fuorvianteLa funzione usa
NotificationService::createNotification('general', ...)per creare una notifica admin e logga soloprestito_id/utente_id/reason, senza PII sensibili: bene.L’unica cosa leggermente fuorviante è il docblock:
- il commento dice “Notifica l’utente che la copia prenotata non è più disponibile”, ma in pratica non viene inviata nessuna email all’utente, solo una notifica admin.
Se l’intenzione è effettivamente solo quella di avvisare gli admin, potresti aggiornare la descrizione (o aggiungere in futuro un invio email qui, analogo a
notifyUserCopyAvailable).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Services/ReservationReassignmentService.php(1 hunks)locale/en_US.json(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Services/ReservationReassignmentService.php (2)
app/Support/NotificationService.php (3)
NotificationService(11-1362)sendReservationBookAvailable(990-992)createNotification(1067-1096)app/Models/CopyRepository.php (2)
CopyRepository(8-245)updateStatus(92-100)
🪛 PHPMD (2.15.0)
app/Services/ReservationReassignmentService.php
315-341: Avoid unused private methods such as 'findAvailableCopy'. (undefined)
(UnusedPrivateMethod)
🔇 Additional comments (8)
locale/en_US.json (4)
110-129: ✓ Aggiornamento e backup - Traduzioni coerenti e completeLe nuove voci relative al flusso di aggiornamento (linee 110-129) seguono pattern coerenti con il resto del file. Esempi ben tradotti:
- "Aggiornamento completato con successo" → "Update completed successfully" ✓
- "Avvio backup database" → "Starting database backup" ✓
- "Memory limit aumentato" → "Memory limit increased" ✓
Nessun problema di traduzione rilevato.
1689-1703: ✓ MaintenanceService - Messaggi coerenti e ben localizzatiLe voci MaintenanceService (linee 1689-1703) mantengono un pattern coerente con prefisso "MaintenanceService " seguono da descrizione dell'errore/stato. Le traduzioni sono tecnicamente accurate:
- "MaintenanceService connessione database fallita" → "MaintenanceService database connection failed" ✓
- "MaintenanceService errore generazione ICS" → "MaintenanceService ICS generation error" ✓
- "MaintenanceService prenotazione convertita in prestito" → "MaintenanceService reservation converted to loan" ✓
Tutte le voci seguono lo stesso pattern strutturale. Nessun problema rilevato.
2271-2271: ✓ Messaggi utente - Traduzioni chiare e contestualmente appropriateLinea 2271: "Questo utente ha già un prestito o una prenotazione attiva per questo libro." → "This user already has an active loan or reservation for this book." - Traduzione accurata che mantiene il significato del vincolo di business logic. ✓
Linea 2474: "Scaduta il" → "Expired on" - Introduzione appropriata di nuovo stato per prenotazioni scadute (allineato con "scaduto" su linea 2473). ✓
Also applies to: 2474-2474
3448-3448: ✓ Messaggio di integrità dati - Terminology coerenteLinea 3448: "Prestito ID %d con stato '%s' ha ancora attivo = 1" → "Loan ID %d with status '%s' still has active = 1"
Traduzione corretta che mantiene il riferimento tecnico al campo database "attivo". Il formato dell'ID prestito e stato è coerente con altri messaggi diagnostici nel file.
app/Services/ReservationReassignmentService.php (4)
175-262: Buona gestione della copia persa con tentativi multipli e fallbackLa nuova implementazione di
reassignOnCopyLost()è decisamente più robusta:
- loop con
maxRetriese lista$excludedCopiesper evitare loop infiniti;- lock della copia candidata con
SELECT ... FOR UPDATEe verifica dello stato prima di aggiornareprestitie lo stato della copia;- in caso di errori/race, rollback condizionato e log strutturato con
attempt;- alla fine, log di warning e
handleNoCopyAvailable()se non si trova una copia valida.Questa struttura copre bene il race condition evidenziato nelle review precedenti ed evita di lasciare la prenotazione appesa alla copia persa.
296-313:reassignOnReturn: integrazione semplice e leggibileLa funzione che risale al
libro_iddalla copia e delega areassignOnNewCopy()rende il comportamento del “ritorno copia” consistente con l’aggiunta di una nuova copia. Logica lineare, niente da segnalare.
382-447: Notifica “copia disponibile”: query e payload coerenti, fix enum autore correttoQuesta parte è ben strutturata:
- query con join su
utentielibri+ controllo robusto su$data['email'];- recupero dell’autore principale con
ORDER BY la.ruolo = 'principale' DESCche corregge il precedente uso del valore enum inesistente'autore';- costruzione delle variabili per il template email con fallback sensati e link basati sulla
getBaseUrl().Nel complesso il flusso è chiaro e aderente al resto del sistema di notifiche.
506-524:getBaseUrl: gestione robusta di URL canonica e fallback hostLa costruzione della base URL è difensiva:
- preferisce
APP_CANONICAL_URLvalidata confilter_var;- in assenza, usa protocollo da
HTTPS+HTTP_HOSTma lo valida con una regex restrittiva, con fallback sicuro sulocalhost.Implementazione pulita e consistente con un contesto web/CLI misto.
locale/en_US.json
Outdated
| "Eccezione creazione tabella %s:": "Exception creating table %s:", | ||
| "Errore riassegnazione copia": "Copy reassignment error", | ||
| "Errore riassegnazione copia persa": "Lost copy reassignment error", | ||
| "Impossibile notificare utente: dati mancanti": "Cannot notify user: missing data", | ||
| "Notifica prenotazione disponibile inviata": "Reservation available notification sent", | ||
| "Invio notifica prenotazione fallito": "Reservation notification failed", | ||
| "Impossibile notificare utente copia non disponibile": "Cannot notify user copy unavailable", | ||
| "La copia assegnata è stata segnalata come persa o danneggiata": "The assigned copy has been reported as lost or damaged", | ||
| "La prenotazione è scaduta": "The reservation has expired", | ||
| "La copia non è più disponibile": "The copy is no longer available", | ||
| "Prenotazione: copia non disponibile": "Reservation: copy unavailable", | ||
| "Prenotazione per \"%s\" (utente: %s) messa in attesa. %s.": "Reservation for \"%s\" (user: %s) put on hold. %s.", | ||
| "Notifica copia non disponibile creata": "Copy unavailable notification created", | ||
| "Errore gestione cambio stato copia": "Copy status change handling error", | ||
| "DataIntegrity warning (store loan)": "DataIntegrity warning (store loan)", | ||
| "Notifica prestito fallita": "Loan notification failed", | ||
| "Validazione prestito fallita": "Loan validation failed", | ||
| "Riassegnazione copia fallita": "Copy reassignment failed", | ||
| "Errore elaborazione restituzione": "Return processing error", | ||
| "Rinnovo prestito fallito": "Loan renewal failed", | ||
| "Errore export CSV": "CSV export error", | ||
| "Errore annullamento prestito": "Loan cancellation error", | ||
| "Errore annullamento prenotazione": "Reservation cancellation error", | ||
| "Errore modifica data prenotazione": "Reservation date change error", | ||
| "Notifica richiesta prestito fallita": "Loan request notification failed", | ||
| "Errore richiesta prestito": "Loan request error", | ||
| "Errore prenotazione": "Reservation error", | ||
| "Riassegnazione prenotazione nuova copia fallita": "New copy reservation reassignment failed", | ||
| "Elaborazione lista attesa fallita": "Waitlist processing failed", | ||
| "La tua prenotazione per \"%s\" è stata messa in attesa. %s. Verrai notificato quando sarà disponibile una nuova copia.": "Your reservation for \"%s\" has been put on hold. %s. You will be notified when a new copy becomes available.", | ||
| "Esauriti tentativi riassegnazione copia": "Exhausted copy reassignment attempts", | ||
| "Errore gestione copia non disponibile": "Copy unavailable handling error" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file and line numbers exist
head -c 500 locale/en_US.json | tail -c 200
wc -l locale/en_US.json
sed -n '3595,3626p' locale/en_US.json | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 1823
🏁 Script executed:
# Search for usage of these translation keys in PHP code
rg "Errore riassegnazione copia|Impossibile notificare utente|Notifica prenotazione disponibile" --type=php -n | head -30Repository: fabiodalez-dev/Pinakes
Length of output: 738
🏁 Script executed:
# Check for ReservationReassignmentService and related classes
rg "ReservationReassignmentService|MaintenanceService" --type=php -n -A 3 | head -50Repository: fabiodalez-dev/Pinakes
Length of output: 3093
🏁 Script executed:
# Verify Italian translations exist in locale/it_IT.json
ls -la locale/it_IT.json 2>/dev/null || echo "File not found"
wc -l locale/it_IT.json 2>/dev/null || echo "Cannot check file"Repository: fabiodalez-dev/Pinakes
Length of output: 153
🏁 Script executed:
# Search for the English keys in PHP to verify usage
rg "Copy reassignment error|Cannot notify user|Loan notification failed" --type=php -n | head -30Repository: fabiodalez-dev/Pinakes
Length of output: 48
🏁 Script executed:
# Read the Italian locale file completely
cat locale/it_IT.jsonRepository: fabiodalez-dev/Pinakes
Length of output: 3428
🏁 Script executed:
# Search for more of the 32 translation keys in PHP code
rg "Invio notifica prenotazione fallito|La prenotazione è scaduta|La copia non è più disponibile|DataIntegrity warning|Notifica prestito fallita|Validazione prestito fallita|Riassegnazione copia fallita|Errore elaborazione restituzione|Rinnovo prestito fallito" --type=php -nRepository: fabiodalez-dev/Pinakes
Length of output: 1710
🏁 Script executed:
# Check how many of the new keys are actually referenced in PHP files
echo "=== Checking all 32 new keys usage ==="
rg "Eccezione creazione tabella|Errore riassegnazione copia|Errore riassegnazione copia persa|Impossibile notificare utente|Notifica prenotazione disponibile inviata|Invio notifica prenotazione fallito|Impossibile notificare utente copia non disponibile|La copia assegnata è stata segnalata come persa|La prenotazione è scaduta|La copia non è più disponibile|Prenotazione: copia non disponibile|Prenotazione per|Notifica copia non disponibile creata|Errore gestione cambio stato copia|DataIntegrity warning|Notifica prestito fallita|Validazione prestito fallita|Riassegnazione copia fallita|Errore elaborazione restituzione|Rinnovo prestito fallito|Errore export CSV|Errore annullamento prestito|Errore annullamento prenotazione|Errore modifica data prenotazione|Notifica richiesta prestito fallita|Errore richiesta prestito|Errore prenotazione|Riassegnazione prenotazione nuova copia fallita|Elaborazione lista attesa fallita|La tua prenotazione per|Esauriti tentativi riassegnazione copia|Errore gestione copia non disponibile" --type=php | wc -lRepository: fabiodalez-dev/Pinakes
Length of output: 107
🏁 Script executed:
# List all files that use __() translation function
rg "__('" --type=php -l | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 123
🏁 Script executed:
# Search for specific usage of each key group to count exactly which ones are used
echo "=== Checking key usage in detail ==="
rg "Errore riassegnazione copia|Errore riassegnazione copia persa|Impossibile notificare utente|Notifica prenotazione disponibile inviata|Invio notifica prenotazione fallito" --type=php -c | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 152
🏁 Script executed:
# Check if Italian keys are missing by searching en_US.json structure
head -20 locale/en_US.json
tail -20 locale/en_US.jsonRepository: fabiodalez-dev/Pinakes
Length of output: 2322
🏁 Script executed:
# Check all locale files in the repository
find locale -type f -name "*.json" | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 151
🏁 Script executed:
# Try to find orphaned keys - those in en_US.json but not used in PHP
echo "=== Keys that might be orphaned ==="
rg "Errore export CSV|Errore annullamento prestito|Errore annullamento prenotazione|Errore modifica data prenotazione|Notifica richiesta prestito fallita|Errore richiesta prestito|Errore prenotazione|Riassegnazione prenotazione nuova copia fallita|Elaborazione lista attesa fallita|Esauriti tentativi riassegnazione copia|Errore gestione copia non disponibile" --type=phpRepository: fabiodalez-dev/Pinakes
Length of output: 1603
🏁 Script executed:
# Final verification: Count total keys in en_US.json vs it_IT.json
echo "=== Locale file sizes ==="
echo "en_US.json lines: $(wc -l < locale/en_US.json)"
echo "it_IT.json lines: $(wc -l < locale/it_IT.json)"
# Check if it_IT.json has similar structure (should have corresponding keys)
echo -e "\n=== Checking if it_IT.json mirrors en_US.json structure ==="
head -5 locale/it_IT.json | grep -o ":" | wc -l
grep -c "\".*\":" locale/en_US.json || echo "Keys in en_US.json"Repository: fabiodalez-dev/Pinakes
Length of output: 203
🏁 Script executed:
# Verify one more key that was supposedly "orphaned" to confirm all are used
rg "Errore export CSV|Errore annullamento prestito" --type=php -B 2 -A 2 | head -20Repository: fabiodalez-dev/Pinakes
Length of output: 959
All 32 new translation keys are actively used; Italian locale file severely incomplete
Verification confirms all 32 translation keys introduced in lines 3595-3626 are actively used throughout the codebase:
- ReservationReassignmentService.php uses 9 keys
- PrestitiController.php uses 8+ keys
- UserActionsController.php uses 7 keys
- LibriController.php uses 2 keys
- LoanRepository.php uses 1 key
No orphaned translations exist. However, a critical localization gap has been identified: locale/it_IT.json contains only 42 lines with approximately 4 key-value pairs, while locale/en_US.json contains 3,627 lines with ~3,625 translations. The Italian locale file is missing not just these 32 keys, but the entire set of library management, loan, and reservation translations—leaving Italian users unable to view any error messages in their language.
Action required: Populate locale/it_IT.json with Italian translations for all entries in locale/en_US.json, or determine if a different localization strategy is in place.
- DataTable: use server default ordering (p.id DESC) to show most recent loans/reservations first instead of ordering by book title column - MaintenanceService: add expired_reservations to PHPDoc array-shape for runAll() and runIfNeeded() return types
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/prestiti/index.php (1)
5-26: Allineare la vista ai nuovi statiannullatoescadutodell’enumprestiti.statusIn questa vista i nuovi stati
annullatoescadutonon sono mai gestiti (badge, filtri rapidi, colonna stato della DataTable, export CSV). I prestiti con questi stati finiranno come “Sconosciuto” e non saranno filtrabili/esportabili per stato, disallineando la UI rispetto al modello dati introdotto nel PR.Suggerisco di aggiungere esplicitamente i due stati in tutti i punti interessati:
- Badge di stato (
getStatusBadge)- case 'danneggiato': - return "<span class='$baseClasses bg-red-100 text-red-800'><i class='fas fa-times-circle mr-2'></i>" . __("Danneggiato") . "</span>"; + case 'danneggiato': + return "<span class='$baseClasses bg-red-100 text-red-800'><i class='fas fa-times-circle mr-2'></i>" . __("Danneggiato") . "</span>"; + case 'annullato': + return "<span class='$baseClasses bg-gray-100 text-gray-800'><i class='fas fa-ban mr-2'></i>" . __("Annullato") . "</span>"; + case 'scaduto': + return "<span class='$baseClasses bg-red-100 text-red-800'><i class='fas fa-calendar-times mr-2'></i>" . __("Scaduto") . "</span>";
- Pulsanti filtro rapidi sopra la tabella
<button data-status="in_ritardo" class="status-filter-btn btn-secondary px-3 py-1.5"><?= __("In Ritardo") ?></button> + <button data-status="scaduto" class="status-filter-btn btn-secondary px-3 py-1.5"><?= __("Scaduto") ?></button> + <button data-status="annullato" class="status-filter-btn btn-secondary px-3 py-1.5"><?= __("Annullato") ?></button> <button data-status="restituito" class="status-filter-btn btn-secondary px-3 py-1.5"><?= __("Restituito") ?></button>
- Render dello stato nella colonna DataTables
case 'danneggiato': return `<span class='${baseClasses} bg-red-100 text-red-800'><i class='fas fa-times-circle mr-2'></i><?= __("Danneggiato") ?></span>`; + case 'annullato': + return `<span class='${baseClasses} bg-gray-100 text-gray-800'><i class='fas fa-ban mr-2'></i><?= __("Annullato") ?></span>`; + case 'scaduto': + return `<span class='${baseClasses} bg-red-100 text-red-800'><i class='fas fa-calendar-times mr-2'></i><?= __("Scaduto") ?></span>`; default:
- Stati selezionabili nell’export CSV
<label class="flex items-center gap-2 cursor-pointer p-2 hover:bg-gray-50 rounded"> <input type="checkbox" name="export-status" value="in_ritardo" class="export-status-cb w-4 h-4 text-yellow-600 rounded" checked> <span class="inline-flex items-center px-2 py-0.5 rounded-full text-xs font-medium bg-yellow-100 text-yellow-800"><i class="fas fa-exclamation-triangle mr-1"></i>${__('In Ritardo')}</span> </label> + <label class="flex items-center gap-2 cursor-pointer p-2 hover:bg-gray-50 rounded"> + <input type="checkbox" name="export-status" value="scaduto" class="export-status-cb w-4 h-4 text-red-600 rounded" checked> + <span class="inline-flex items-center px-2 py-0.5 rounded-full text-xs font-medium bg-red-100 text-red-800"><i class="fas fa-calendar-times mr-1"></i>${__('Scaduto')}</span> + </label> + <label class="flex items-center gap-2 cursor-pointer p-2 hover:bg-gray-50 rounded"> + <input type="checkbox" name="export-status" value="annullato" class="export-status-cb w-4 h-4 text-gray-600 rounded" checked> + <span class="inline-flex items-center px-2 py-0.5 rounded-full text-xs font-medium bg-gray-100 text-gray-800"><i class="fas fa-ban mr-1"></i>${__('Annullato')}</span> + </label> <label class="flex items-center gap-2 cursor-pointer p-2 hover:bg-gray-50 rounded"> <input type="checkbox" name="export-status" value="restituito" class="export-status-cb w-4 h-4 text-green-600 rounded" checked> <span class="inline-flex items-center px-2 py-0.5 rounded-full text-xs font-medium bg-green-100 text-green-800"><i class="fas fa-check-circle mr-1"></i>${__('Restituito')}</span> </label>Così la UI resta coerente con le nuove regole di dominio introdotte dalle migration.
Also applies to: 188-193, 325-345, 494-520
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Support/MaintenanceService.php(8 hunks)app/Views/prestiti/index.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Support/MaintenanceService.php (5)
app/Controllers/UserActionsController.php (1)
loan(380-456)app/Controllers/PrestitiController.php (1)
close(496-505)app/Services/ReservationReassignmentService.php (3)
ReservationReassignmentService(17-525)setExternalTransaction(35-39)reassignOnReturn(296-313)app/Support/DataIntegrity.php (2)
DataIntegrity(9-1252)recalculateBookAvailability(234-348)app/Controllers/CopyController.php (1)
updateCopy(51-161)
🔇 Additional comments (6)
app/Support/MaintenanceService.php (5)
43-43: LGTM: Annotazioni di tipo aggiornate correttamente.Le annotazioni di tipo per
expired_reservationssono state aggiunte in modo coerente sia arunIfNeeded()che arunAll(), e il contatore è correttamente inizializzato nell'array dei risultati.Also applies to: 68-68, 75-75
88-88: LGTM: Migrazione a SecureLogger completata correttamente.La migrazione da
error_logaSecureLoggerè stata implementata in modo coerente in tutto il file, con contesto strutturato appropriato (ID, messaggi di errore, stato). Questo migliora l'osservabilità e la sicurezza del logging.Also applies to: 95-95, 103-103, 110-110, 121-121, 129-129, 133-133, 247-250, 327-330, 338-341, 438-442, 446-449, 533-535, 551-557, 565-567
96-104: LGTM: Integrazione di checkExpiredReservations pulita e coerente.L'integrazione del nuovo task di manutenzione segue lo stesso pattern degli altri task esistenti, con gestione degli errori appropriata e logging strutturato.
382-384: LGTM: Transazioni nidificate risolte correttamente.L'uso di
setExternalTransaction(true)risolve la preoccupazione precedente sulle transazioni nidificate, delegando il controllo della transazione al chiamante (MaintenanceService) invece che al servizio di riassegnazione.Based on learnings, this addresses the previous major concern about nested transaction handling.
395-409: LGTM: Concatenazione SQL corretta con parametri vincolati.Il suffisso della nota è ora costruito in PHP e passato come parametro vincolato tramite
bind_param('si', $noteSuffix, $id), risolvendo il rischio di SQL injection precedentemente segnalato.Based on learnings, this addresses the previous major concern about SQL string concatenation.
app/Views/prestiti/index.php (1)
288-372:order: []correctly delegates ordering to server with proper defaultThe implementation is sound. With
serverSide: trueandorder: [], the client sends no initial order parameters. The backend correctly appliesORDER BY p.id DESCas the default (lines 108–109 of PrestitiApiController), and respects any order parameters sent by DataTables on subsequent interactions (lines 122–130). DataTables 2.3.4 fully supports this pattern.
- Add deferred notifications queue to ReservationReassignmentService - Notifications are queued when externalTransaction=true - MaintenanceService calls flushDeferredNotifications() after commit - Prevents race condition where notifications sent before data persisted Also fixes session-expired.php: - Allow $pageTitle and $loginUrl override via null coalescing - Replace CDN Font Awesome with local /assets/vendor.css
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
locale/en_US.json (1)
3595-3627: Nuove stringhe errori/manutenzione per riassegnazione e integrità dati: placeholder OK, copertura completaLe ~32 nuove chiavi (eccezioni tabelle, errori riassegnazione copia/prenotazione, export CSV, validazione prestiti, lista d’attesa, notifica differita) hanno:
- conteggio placeholder
%scoerente tra IT/EN- messaggi ben distinti per i vari rami di errore (log + notifiche)
Unico follow‑up: assicurarsi che le stesse chiavi esistano e siano tradotte anche nelle altre locale (in particolare
locale/it_IT.json), per evitare fallback misti in produzione.
🧹 Nitpick comments (4)
app/Services/ReservationReassignmentService.php (2)
80-130: Rilevamento della transazione tramite@@autocommitè fragile; chiarire il contratto d’usoL’uso di
SELECT @@autocommitinisInTransaction()presuppone che tutte le transazioni esplicite vengano aperte conmysqli::begin_transaction()(che disattiva l’autocommit). Se in futuro qualche chiamante usasseSTART TRANSACTIONoSET autocommit = 0in modo diverso, il servizio potrebbe:
- non riconoscere una transazione già aperta e chiamare
begin_transaction()(rischio di commit impliciti/non attesi), oppure- considerare “in transazione” una connessione solo perché l’autocommit è 0 di default.
Suggerirei di rendere il contratto esplicito in docblock (es. “se usato dentro transazioni esterne è obbligatorio
setExternalTransaction(true)oppurebegin_transaction()di mysqli”) o, meglio, basareisInTransaction()solo suexternalTransactione rimuovere l’euristica su@@autocommit.
508-557:notifyUserCopyUnavailable: solo notifica admin (no email utente) — comportamento intenzionale?Qui viene:
- caricata prenotazione + utente + libro
- scelta del messaggio in base a
$reason(lost_copy,expired, default)- creata una notifica in‑app con
createNotification, senza invio email all’utenteSe l’intenzione è informare solo l’amministratore (e non l’utente finale) sul fatto che la prenotazione è tornata “in attesa”, il comportamento è ok; in caso contrario valuterei l’aggiunta di una mail utente analoga a quella di disponibilità.
app/Support/MaintenanceService.php (2)
427-430: Considera di isolare la riassegnazione in un try-catch separato per garantire che la scadenza venga sempre completata.Attualmente, se
reassignOnReturn()alla riga 428 lancia un'eccezione, il blocco catch alla riga 447 esegue il rollback dell'intera transazione, impedendo che la prenotazione scaduta venga marcata come 'scaduto'. Dal punto di vista della logica di business, la marcatura di una prenotazione scaduta dovrebbe avere successo anche se la riassegnazione fallisce, poiché la scadenza è un requisito obbligatorio mentre la riassegnazione è un'ottimizzazione.Anche se
ReservationReassignmentServiceprobabilmente gestisce gli errori internamente, errori imprevisti del database durante la riassegnazione potrebbero comunque causare il rollback.Applica questa modifica per garantire che la scadenza venga sempre completata:
} } + // Tentativo di riassegnazione (non bloccare la scadenza se fallisce) + if ($copiaId) { + try { + $reassignmentService->reassignOnReturn($copiaId); + } catch (\Throwable $reassignError) { + // Log ma non bloccare la scadenza + SecureLogger::warning(__('Errore durante riassegnazione post-scadenza'), [ + 'prestito_id' => $id, + 'copia_id' => $copiaId, + 'error' => $reassignError->getMessage() + ]); + } + } + // Recalculate book availability $integrity->recalculateBookAvailability($libroId);E rimuovi la chiamata originale alle righe 427-429.
Also applies to: 447-452
348-457: Implementazione solida dicheckExpiredReservations()con buona gestione delle transazioni e del locking.Punti di forza dell'implementazione:
- Documentazione chiara che spiega lo scopo del metodo (Case 4 del piano di prenotazione)
- Query preparata con binding corretto dei parametri (righe 363-379)
- Locking pessimistico con FOR UPDATE sulla copia (riga 413) per prevenire race conditions
- Controllo difensivo dello stato della copia (riga 420) per evitare di liberare copie già prestate o danneggiate
- Logging strutturato con IDs per la tracciabilità (righe 441-445, 449-452)
- Isolamento degli errori tramite transazioni separate per ogni prenotazione, così i fallimenti non bloccano l'intero processo
Nota prestazionale (opzionale): Il loop processa ogni prenotazione in una transazione separata, il che garantisce l'isolamento ma potrebbe essere lento con molte prenotazioni scadute simultaneamente. Per la natura di un task di manutenzione periodico, questo approccio è accettabile. Se in futuro si osservassero timeout o rallentamenti, si potrebbe considerare il batching o la limitazione del numero di prenotazioni processate per esecuzione.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Services/ReservationReassignmentService.php(1 hunks)app/Support/MaintenanceService.php(8 hunks)app/Views/errors/session-expired.php(1 hunks)locale/en_US.json(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Views/errors/session-expired.php
🧰 Additional context used
🧬 Code graph analysis (2)
app/Services/ReservationReassignmentService.php (2)
app/Support/NotificationService.php (2)
NotificationService(11-1362)sendReservationBookAvailable(990-992)app/Models/CopyRepository.php (2)
CopyRepository(8-245)updateStatus(92-100)
app/Support/MaintenanceService.php (2)
app/Services/ReservationReassignmentService.php (4)
ReservationReassignmentService(17-581)setExternalTransaction(42-46)reassignOnReturn(352-369)flushDeferredNotifications(52-70)app/Support/DataIntegrity.php (2)
DataIntegrity(9-1252)recalculateBookAvailability(234-348)
🪛 PHPMD (2.15.0)
app/Services/ReservationReassignmentService.php
371-397: Avoid unused private methods such as 'findAvailableCopy'. (undefined)
(UnusedPrivateMethod)
🔇 Additional comments (13)
locale/en_US.json (4)
110-129: Stringhe updater/backup coerenti con il nuovo flusso di aggiornamentoLe nuove chiavi per avanzamento aggiornamento/backup sembrano consistenti con la logica dell’updater (nessun placeholder mancante o disallineato). Nulla da segnalare dal punto di vista tecnico.
1690-1703: Messaggi MaintenanceService allineati al codice di manutenzioneLe stringhe di log per
MaintenanceServicecoprono in modo granulare i vari rami (prestiti, prenotazioni, ICS, notifiche). I testi sono chiari e coerenti; verificate solo che i nomi corrispondano esattamente ai messaggi usati nel codice per evitare chiavi “quasi duplicate”.
2271-2271: Messaggio blocco doppio prestito/prenotazione semanticamente corretto
"Questo utente ha già un prestito o una prenotazione attiva per questo libro."descrive bene il vincolo applicato lato dominio; nulla da eccepire sulle stringhe.
2474-2474: Label “Scaduta il” in linea con i nuovi stati di prestito/prenotazioneLa nuova chiave per data di scadenza passata è coerente con l’introduzione dello stato
scaduto; naming chiaro e allineato alle altre etichette di stato.app/Services/ReservationReassignmentService.php (5)
136-216: FlussoreassignOnNewCopy: logica e concorrenza gestite in modo sensato
- Selezione FIFO della prenotazione bloccata (
stato = 'prenotato',attivo = 1, copia nulla o non disponibile) è coerente.- Lock della nuova copia con
SELECT ... FOR UPDATEe ricontrollostato = 'disponibile'riduce bene le race condition.- Aggiornamento di
prestiti.copia_id+copie.stato = 'prenotato'dentro transazione, con notifica post‑commit o differita, è corretto.Nulla di critico da segnalare qui.
222-309:reassignOnCopyLost: buon handling con retry ed esclusione copie già provateLa logica:
- trova la prenotazione attiva sulla copia persa
- tenta fino a
maxRetriesdi assegnare copie disponibili diverse tramitefindAvailableCopyExcluding- per ogni tentativo blocca la copia con
FOR UPDATEe ricontrollastato = 'disponibile'- in caso di saturazione o errori ripetuti, logga warning e passa a
handleNoCopyAvailableÈ un buon compromesso tra robustezza e semplicità; l’early‑return finale evita che la prenotazione rimanga agganciata alla copia persa.
314-346:handleNoCopyAvailable: comportamento chiaro per prenotazioni senza copia disponibileImpostare
copia_id = NULLe spostare la prenotazione in “coda senza copia” è semanticamente pulito; la distinzione tra notifica immediata e differita quandoexternalTransactionè true è coerente col resto del servizio. Log di errore adeguato in caso di eccezioni.
438-503:notifyUserCopyAvailable: recupero dati e variabili template ben strutturati
- La query unisce
prestiti,utenti,libricon le colonne necessarie.- Recupero autore principale ordinando per
la.ruolo = 'principale'è coerente con l’enum dello schema.- Scelta ISBN (
isbn13→isbn10→ vuoto) e costruzione dibook_url/profile_urlcongetBaseUrl()risultano sensate.- Logging usa solo
prestito_ideutente_id, evitando PII superflue.Nessun problema evidente.
562-580:getBaseUrl: gestione robusta tra APP_CANONICAL_URL e fallback HTTP_HOST
- Usa
APP_CANONICAL_URLse presente e valido, ripulito e senza slash finale.- In fallback costruisce l’URL da protocollo +
HTTP_HOSTcon validazione regex, ripiegando sulocalhostse l’host non è sicuro.La funzione è allineata alle nuove stringhe di diagnostica su
APP_CANONICAL_URL; niente di anomalo.app/Support/MaintenanceService.php (4)
382-384: Ottimo lavoro: i problemi segnalati nelle review precedenti sono stati risolti correttamente.Le modifiche implementate risolvono efficacemente tutti e tre i problemi critici identificati in precedenza:
Transazioni annidate risolte: L'uso di
setExternalTransaction(true)alla riga 384 impedisce alReservationReassignmentServicedi avviare transazioni annidate, lasciando il controllo transazionale al chiamante.Concatenazione SQL risolta: Il suffisso della nota è ora costruito in PHP (riga 396) e passato come parametro vincolato (riga 407), eliminando i rischi di SQL injection da stringhe tradotte con caratteri speciali.
Notifiche premature risolte: La chiamata a
flushDeferredNotifications()alla riga 439 avviene DOPO il commit della transazione (riga 435), garantendo che le notifiche vengano inviate solo se le modifiche al database persistono con successo.L'implementazione dimostra una buona comprensione dei pattern di gestione transazionale e della sicurezza SQL.
Also applies to: 396-409, 428-428, 435-435, 439-439
88-88: Eccellente migrazione al logging centralizzato.La sostituzione di
error_logconSecureLoggerè implementata in modo consistente in tutto il file, con logging strutturato che include contesto rilevante (ID delle entità, messaggi di errore, ecc.). Questo migliora notevolmente l'osservabilità e facilita il debugging in produzione.Also applies to: 95-95, 103-103, 110-110, 121-121, 129-129, 133-133, 247-250, 327-330, 338-341
536-538: Aggiornamenti al hook di login admin ben implementati.Le modifiche includono:
- Logging strutturato degli errori di connessione al database (righe 536-538)
- Inclusione di
expired_reservationsnelle metriche registrate dopo l'esecuzione delle manutenzioni (riga 558)- Gestione coerente degli errori con SecureLogger (righe 568-570)
La gestione delle risorse è corretta, con chiusura appropriata della connessione DB anche in caso di errore.
Also applies to: 554-560, 568-570
43-43: Integrazione pulita del taskexpired_reservationsnel workflow di manutenzione.Le modifiche sono ben strutturate:
- Documentazione PHPDoc aggiornata per riflettere il nuovo campo nei risultati (righe 43, 68)
- Inizializzazione corretta nel array
$results(riga 75)- Gestione errori coerente con gli altri task di manutenzione (righe 99-104)
L'integrazione segue gli stessi pattern degli altri task esistenti, mantenendo coerenza nel codice.
Also applies to: 68-68, 75-75, 99-104
| private function findAvailableCopy(int $libroId, ?int $excludeCopiaId = null): ?int | ||
| { | ||
| $sql = " | ||
| SELECT id | ||
| FROM copie | ||
| WHERE libro_id = ? | ||
| AND stato = 'disponibile' | ||
| "; | ||
| $params = [$libroId]; | ||
| $types = "i"; | ||
|
|
||
| if ($excludeCopiaId) { | ||
| $sql .= " AND id != ?"; | ||
| $params[] = $excludeCopiaId; | ||
| $types .= "i"; | ||
| } | ||
|
|
||
| $sql .= " LIMIT 1"; | ||
|
|
||
| $stmt = $this->db->prepare($sql); | ||
| $stmt->bind_param($types, ...$params); | ||
| $stmt->execute(); | ||
| $res = $stmt->get_result()->fetch_assoc(); | ||
| $stmt->close(); | ||
|
|
||
| return $res ? (int) $res['id'] : null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Search for all usages of findAvailableCopy in the codebase
rg -n "findAvailableCopy" --type=php -C2Repository: fabiodalez-dev/Pinakes
Length of output: 1416
Rimuovere il metodo privato findAvailableCopy: è codice morto non utilizzato
La ricerca nel codebase conferma che findAvailableCopy() non viene mai invocato. Il metodo è stato probabilmente creato durante un refactoring ma mai integrato nella logica effettiva. Esiste già findAvailableCopyExcluding() (riga 404) che viene utilizzato correttamente (riga 249) e gestisce l'esclusione di più copie.
Eliminare il metodo inutilizzato per evitare confusione nella manutenzione e ridurre la complessità del servizio.
🧰 Tools
🪛 PHPMD (2.15.0)
371-397: Avoid unused private methods such as 'findAvailableCopy'. (undefined)
(UnusedPrivateMethod)
🤖 Prompt for AI Agents
In app/Services/ReservationReassignmentService.php around lines 371 to 397, the
private method findAvailableCopy is dead code (never called) and duplicates
functionality provided by findAvailableCopyExcluding; remove the entire private
method declaration and any related unused variables/imports, then run tests/grep
to confirm there are no remaining references and update any docblocks or
comments that referenced it.
- Increase memory limit to 256MB for large updates - Add comprehensive logging at every step (GitHub API, download, ZIP, migrations) - Add connection diagnostics (DNS, SSL, cURL test) on failure - Helps troubleshoot update issues on different server configurations
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Support/Updater.php (1)
1383-1386: Splitting SQL per punto e virgola è fragile.La divisione delle istruzioni SQL usando
explode(';', $sql)non gestisce correttamente i punti e virgola all'interno di stringhe, procedure, o trigger. Questo potrebbe causare errori di migrazione con SQL complesso.Per migrazioni semplici va bene, ma se in futuro si aggiungono procedure o trigger, considerare l'uso di un parser SQL più robusto o eseguire i file di migrazione direttamente con
mysqli_multi_query.
🧹 Nitpick comments (4)
app/Support/Updater.php (4)
268-272: Limitare il logging della risposta HTTP.La preview della risposta (primi 500 caratteri) potrebbe contenere dati sensibili in caso di errori API che includono token o informazioni di autenticazione nei messaggi.
$this->debugLog('DEBUG', 'Risposta HTTP ricevuta', [ 'response_length' => $response !== false ? strlen($response) : 0, 'response_headers' => $responseHeaders, - 'response_preview' => $response !== false ? substr($response, 0, 500) : 'FALSE' + 'response_preview' => $response !== false ? '[REDACTED - ' . strlen($response) . ' bytes]' : 'FALSE' ]);
538-557: Caricamento completo in memoria per download di grandi dimensioni.Il file viene scaricato interamente in memoria con
file_get_contentsprima di essere scritto su disco. Per pacchetti di aggiornamento molto grandi, questo potrebbe causare esaurimento della memoria nonostante l'aumento del limite a 256MB.Considerare l'uso di streaming per il download:
// Alternativa con streaming $sourceStream = @fopen($downloadUrl, 'r', false, $context); $destStream = @fopen($zipPath, 'w'); if ($sourceStream && $destStream) { stream_copy_to_stream($sourceStream, $destStream); fclose($sourceStream); fclose($destStream); }
608-613: Lettura inefficiente per diagnostic hex dump.
file_get_contentslegge l'intero file ZIP solo per ottenere i primi 20 byte. Per file grandi questo spreca memoria.- 'file_first_bytes' => bin2hex(substr(file_get_contents($zipPath), 0, 20)) + 'file_first_bytes' => bin2hex(file_get_contents($zipPath, false, null, 0, 20))Oppure usare
fread:$handle = fopen($zipPath, 'rb'); $firstBytes = $handle ? bin2hex(fread($handle, 20)) : 'N/A'; if ($handle) fclose($handle);
948-968: Backup database senza lock potrebbe essere inconsistente.L'implementazione streaming è corretta, ma il backup non utilizza
LOCK TABLESoSTART TRANSACTION WITH CONSISTENT SNAPSHOT. Se i dati vengono modificati durante il backup, il risultato potrebbe essere inconsistente.Considerare l'aggiunta di un lock per garantire consistenza:
+ // Lock all tables for consistent backup + $lockQuery = "LOCK TABLES " . implode(" READ, ", array_map(fn($t) => "`{$t}`", $tables)) . " READ"; + $this->db->query($lockQuery); + foreach ($tables as $table) { // ... backup logic } + + $this->db->query("UNLOCK TABLES");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Support/Updater.php(32 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Support/Updater.php (1)
app/Controllers/UpdateController.php (1)
createBackup(122-152)
🔇 Additional comments (7)
app/Support/Updater.php (7)
59-89: LGTM!L'inizializzazione è corretta. L'uso di
uniqid('', true)per il path temporaneo fornisce sufficiente entropia, e la creazione della directory di backup con controllo di errore è appropriata.
339-410: LGTM!La diagnostica della connessione è ben implementata. Verifica DNS, SSL, e fallback cURL con corretta chiusura delle risorse.
1903-1917: LGTM! Gestione robusta degli errori fatali.Lo shutdown handler per la pulizia del maintenance mode in caso di errori fatali è una buona aggiunta per migliorare l'affidabilità. Il file locking previene correttamente aggiornamenti concorrenti.
2058-2089: LGTM!L'implementazione di
checkStaleMaintenanceMode()è solida. Il timeout di 30 minuti è ragionevole per gestire aggiornamenti interrotti, e la verifica del timestamp nel file JSON previene rimozioni accidentali.
1274-1288: LGTM! Buona protezione contro path traversal.Le verifiche di sicurezza sono appropriate: controllo per
..e null bytes, verifica conrealpathche i file restino nella directory di destinazione, e skip dei symlink per prevenire attacchi.
836-862: LGTM!La validazione del path per l'eliminazione del backup è corretta. Il controllo regex per separatori di path e
.., combinato con la verificarealpath, previene directory traversal.
2-9: Documentazione chiara e utile.Il docblock con le istruzioni per filtrare i log (
grep -i "updater") è un'aggiunta utile per il troubleshooting.
| private function debugLog(string $level, string $message, array $context = []): void | ||
| { | ||
| $fullMessage = "[Updater DEBUG] [{$level}] {$message}"; | ||
|
|
||
| // Always log to error_log for immediate visibility | ||
| error_log($fullMessage . ' ' . json_encode($context, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES)); | ||
|
|
||
| // Also log to SecureLogger if available | ||
| if (class_exists(SecureLogger::class)) { | ||
| $method = strtolower($level); | ||
| if (method_exists(SecureLogger::class, $method)) { | ||
| SecureLogger::$method($fullMessage, $context); | ||
| } else { | ||
| SecureLogger::info($fullMessage, $context); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Attenzione ai dati sensibili nei log.
Il metodo debugLog registra il contesto completo in error_log. In varie parti del codice vengono loggati contenuti SQL, percorsi di file, e preview di risposte HTTP. Verificare che non vengano mai inclusi dati sensibili (credenziali, token, PII) nel contesto passato a questo metodo.
private function debugLog(string $level, string $message, array $context = []): void
{
$fullMessage = "[Updater DEBUG] [{$level}] {$message}";
+ // Rimuovi eventuali dati sensibili dal contesto prima del logging
+ unset($context['password'], $context['token'], $context['secret']);
+
// Always log to error_log for immediate visibility
error_log($fullMessage . ' ' . json_encode($context, JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES));| private function parseMemoryLimit(string $limit): int | ||
| { | ||
| $limit = trim($limit); | ||
| $last = strtolower($limit[strlen($limit) - 1]); | ||
| $value = (int) $limit; | ||
|
|
||
| switch ($last) { | ||
| case 'g': | ||
| $value *= 1024 * 1024 * 1024; | ||
| break; | ||
| case 'm': | ||
| $value *= 1024 * 1024; | ||
| break; | ||
| case 'k': | ||
| $value *= 1024; | ||
| break; | ||
| } | ||
|
|
||
| return $value; | ||
| } |
There was a problem hiding this comment.
Edge case: stringa vuota causerebbe errore.
Se $limit è una stringa vuota dopo il trim(), la riga $last = strtolower($limit[strlen($limit) - 1]) accederà a un indice negativo.
private function parseMemoryLimit(string $limit): int
{
$limit = trim($limit);
+ if ($limit === '' || $limit === '-1') {
+ return PHP_INT_MAX;
+ }
$last = strtolower($limit[strlen($limit) - 1]);
$value = (int) $limit;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function parseMemoryLimit(string $limit): int | |
| { | |
| $limit = trim($limit); | |
| $last = strtolower($limit[strlen($limit) - 1]); | |
| $value = (int) $limit; | |
| switch ($last) { | |
| case 'g': | |
| $value *= 1024 * 1024 * 1024; | |
| break; | |
| case 'm': | |
| $value *= 1024 * 1024; | |
| break; | |
| case 'k': | |
| $value *= 1024; | |
| break; | |
| } | |
| return $value; | |
| } | |
| private function parseMemoryLimit(string $limit): int | |
| { | |
| $limit = trim($limit); | |
| if ($limit === '' || $limit === '-1') { | |
| return PHP_INT_MAX; | |
| } | |
| $last = strtolower($limit[strlen($limit) - 1]); | |
| $value = (int) $limit; | |
| switch ($last) { | |
| case 'g': | |
| $value *= 1024 * 1024 * 1024; | |
| break; | |
| case 'm': | |
| $value *= 1024 * 1024; | |
| break; | |
| case 'k': | |
| $value *= 1024; | |
| break; | |
| } | |
| return $value; | |
| } |
🤖 Prompt for AI Agents
In app/Support/Updater.php around lines 2037-2056, the parseMemoryLimit()
function will error when $limit is an empty string because
$limit[strlen($limit)-1] is invalid; add a guard after trim() to handle empty
input (e.g. if $limit === '' return 0 or an appropriate default/unlimited
indicator) and only compute $last and cast $value when strlen($limit) > 0;
ensure the function still correctly handles numeric-only values and units after
this check.
Summary
This PR introduces significant improvements to the reservation system, updater reliability, and fixes several critical bugs.
Reservation System Improvements
ReservationReassignmentService: Handles automatic reassignment of reservations when:scripts/check-expired-reservations.phpscript to automatically handle expired reservationsannullato(cancelled) andscaduto(expired) to the prestiti status enumUpdater Reliability Improvements
checkStaleMaintenanceMode()auto-removes maintenance files older than 30 minutesBug Fixes
/\\/gbecame/\/g)getBookTotalCopies()now correctly returns 0 when all copies are lost/damaged (instead of falling back tocopie_totali)nome_completo→nome,data_inizio/data_fine→data_prestito/data_scadenza)Migrations
migrate_0.4.2.sql: Index optimization for prestiti tablemigrate_0.4.3.sql: Add 'annullato' and 'scaduto' to prestiti status enumTest Plan
Summary by CodeRabbit
Release Notes v0.4.2
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.