fix(loans): harden reservation/loan lifecycle + copy-number generation (#238)#252
Merged
Conversation
Independent review of the loan/reservation overhaul plus a user-reported copy bug (#238). Every fix is covered by a behavioral test. Correctness (HIGH): - DataIntegrity: the overbooked-period auditor missed any overbooking that involved overdue loans. The open-ended sentinel produced an endExclusive of '10000-01-01' which ksort ordered BEFORE every real date, zeroing overdue occupancy in the daily-peak scan. Sentinel is now 9999-12-30 (+1 day stays a 4-digit key) with a defensive clamp; new regression test asserts detection. - check-expired-reservations cron locked prestiti -> copie -> libri, inverting the canonical libri -> prestiti -> copie order and opening a deadlock window vs the maintenance path. It now locks the book row first. Copy-number generation (#238): - Inventory codes are now a uniform '{base}-C{N}' on every copy (never a bare base), reductions trim from the END (newest available copies first, never a loaned/reserved one), and additions gap-fill the lowest free index with a global uniqueness check — fixing the 'Duplicate entry ...-C2' crash on the 1->2->1->2 sequence. Existing codes are left untouched (printed labels). New CopyRepository::allocateInventoryCodes / getRemovableCopiesNewestFirst, covered by copy-count-inventory.unit.php. Consistency / robustness: - CapacityService now counts legacy reservations with a NULL data_inizio_richiesta (COALESCE start), and getBookAvailabilityData got the same attivo-qualification as getBookAvailability (app + web agree again). - 'Restituito' action aligned across the loans table, loan details and edit views to the states the handler accepts (in_corso/in_ritardo) — no more 404 dead-ends. - CopyController no longer wipes a loan's note when returning via the copy form; LoanRepository::close does a final recalc so the wishlist gate can't fire on a stale counter; rejectLoan's deliberate soft-delete exception is documented. - MaintenanceController no longer runs the whole-table availability recalc twice per click (also fixed a double-count); IcsGenerator writes atomically (tmp + rename, LOCK_EX); cron scripts use Dotenv safeLoad so container/CLI runs without a .env boot. Verification: PHPStan level 5 clean; loan-edge-cases 60/60, copy-count 8/8, loan-reservation 21/21, loan-reservation-complete 26/26, loan-overlap 39/39, email-notifications 14/14, full-test 136/136 — all real-browser E2E on Apache.
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLa PR aggiorna prestiti, prenotazioni, copie e manutenzione con controlli di capacità e locking più coerenti, introduce assegnazione collision-free dei codici inventario, modifica calendari e PDF delle etichette, restringe i filtri di stato e amplia i test. ChangesGestione circolazione e copie
Configurazione e test infrastrutturali
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CopyController
participant PrestitiController
participant ReservationReassignmentService
participant DataIntegrity
CopyController->>PrestitiController: processReturn(payload con note)
PrestitiController->>ReservationReassignmentService: riassegna copia o prenotazione
ReservationReassignmentService->>DataIntegrity: recalculateBookAvailability
DataIntegrity-->>CopyController: disponibilità aggiornata
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/ReservationsAdminController.php (1)
341-392: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStesso problema di range invertito in
store()perdataPrenotazione/dataScadenza.Quando entrambe le date sono fornite esplicitamente dal form (
data_prenotazioneedata_scadenza), non c'è alcun clamp che garantiscadataScadenza >= dataPrenotazione, a differenza didataInizioRichiesta/dataFineRichiesta(righe 405-408).🛡️ Proposta di fix
} else { $dataScadenzaDate = $dataScadenza; // Keep date only for loan period + if ($dataScadenzaDate < $dataPrenotazioneDate) { + $dataScadenzaDate = $dataPrenotazioneDate; + } $dataScadenza = $dataScadenza . ' 23:59:59'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/ReservationsAdminController.php` around lines 341 - 392, Aggiungi in store(), dopo la normalizzazione di dataPrenotazioneDate e dataScadenzaDate e prima della costruzione dei valori con orario, una verifica che garantisca dataScadenzaDate >= dataPrenotazioneDate quando entrambe le date sono presenti. Se la scadenza è precedente all’inizio, riportala alla data di dataPrenotazioneDate, mantenendo invariata la logica esistente per i valori validi e per le date predefinite.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Controllers/LibriController.php`:
- Around line 1762-1774: Rendi atomico il flusso intorno a
getRemovableCopiesNewestFirst: avvia una transazione, acquisisci prima il lock
del libro, poi seleziona o rivalida le copie disponibili senza impegni e
rimuovile nella stessa transazione; ogni query su libri deve includere AND
deleted_at IS NULL. Usa una cancellazione condizionale che fallisca se la copia
è stata impegnata e incrementa $removed solo quando delete() restituisce true,
quindi esegui commit o rollback correttamente.
- Around line 1089-1095: In update(), validate and sanitize copie_totali before
calling allocateInventoryCodes(), enforcing the same 1–9999 bounds used by
store(). Also constrain the base inventory value to the maximum length that
keeps generated -C{N} codes within the numero_inventario VARCHAR(100) limit,
then pass only the validated values to allocateInventoryCodes() and preserve the
existing copy-note flow.
In `@app/Controllers/PrestitiController.php`:
- Around line 810-819: Aggiorna la query preparata in `$bookStmt` aggiungendo il
filtro `AND l.deleted_at IS NULL` alla clausola `WHERE`. Mantieni invariati
join, parametri e gestione del risultato, così i libri soft-eliminati non
superano il gate della notifica wishlist.
In `@app/Controllers/ReservationsAdminController.php`:
- Around line 128-131: Normalize the manually supplied $start and $end dates in
the reservation handling flow before persistence: when both are present and $end
precedes $start, clamp or adjust $end to the same valid boundary used for
dataInizioRichiesta/dataFineRichiesta. Update the logic surrounding the existing
$end defaulting block and the corresponding persistence fields so endDt is never
earlier than startDt, while preserving the current loan-duration default when
$end is empty.
- Around line 124-131: Extract the repeated loan-duration lookup and validation
from update(), store(), and createForm() into a private helper on
ReservationsAdminController. Have the helper use SettingsRepository with the
existing loans/loan_duration_days setting and preserve the current fallback to
30 for missing or non-positive values, then replace all three duplicated
calculation blocks with calls to that helper.
In `@app/Models/CopyRepository.php`:
- Around line 168-179: Make inventory-code allocation atomic in
allocateInventoryCodes and the subsequent LibriController inserts: serialize the
check-and-insert sequence with a lock scoped to the code base, or replace it
with an atomic insert-and-retry flow. Ensure both insertion paths acquire the
same protection before updating book metadata, and retry conflicting candidates
without returning unreserved codes.
In `@app/Support/IcsGenerator.php`:
- Around line 85-98: Aggiorna il flusso di scrittura atomica nel metodo che usa
$tmp e rename() per applicare chmod(0644) al file temporaneo dopo
file_put_contents() e prima di rename(). Mantieni invariata la gestione degli
errori e assicurati che il file finale erediti permessi leggibili dal web
server.
In `@app/Support/NotificationService.php`:
- Around line 411-414: Il claim atomico deve rivalidare anche data_scadenza per
evitare di usare dati obsoleti. Aggiorna il blocco di claim vicino a $updateStmt
affinché il predicato includa data_scadenza = ? e il bind includa la scadenza
acquisita in $loan; applica la stessa rivalidazione al claim overdue,
preservando i controlli esistenti su attivo, stato e warning_sent.
In `@config/settings.php`:
- Line 27: Update the DB_PORT handling in the settings configuration so an
explicitly empty environment value is treated as unset and falls back to port
3306 before integer conversion. Preserve the existing conversion for valid
non-empty port values.
- Around line 34-36: Remove the redundant conditional override for
$settings['displayErrorDetails'] after its initial definition, leaving the
existing initialization logic as the single source of truth.
In `@tests/copy-count-inventory.unit.php`:
- Around line 46-57: Require an explicitly configured dedicated test database
before connecting, rather than falling back to the application database from
.env; generate a unique random per-run token and use it in test titles, then
parameterize cleanup to delete only rows matching that exact token in the
copy-count inventory flow.
In `@tests/loan-edge-cases.unit.php`:
- Line 138: Allinea il conteggio dei test da 59 a 60 in tutti i riferimenti del
file: header iniziali, commento di sezione e formato printf del contatore
associato a TESTNO. Mantieni coerenti anche l’intervallo della sezione finale
come 53-60 e l’output conclusivo già impostato su 60.
---
Outside diff comments:
In `@app/Controllers/ReservationsAdminController.php`:
- Around line 341-392: Aggiungi in store(), dopo la normalizzazione di
dataPrenotazioneDate e dataScadenzaDate e prima della costruzione dei valori con
orario, una verifica che garantisca dataScadenzaDate >= dataPrenotazioneDate
quando entrambe le date sono presenti. Se la scadenza è precedente all’inizio,
riportala alla data di dataPrenotazioneDate, mantenendo invariata la logica
esistente per i valori validi e per le date predefinite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1ed3b51-019b-432c-9df3-eb5e357d3df7
📒 Files selected for processing (36)
app/Controllers/CopyController.phpapp/Controllers/LibriController.phpapp/Controllers/LoanApprovalController.phpapp/Controllers/MaintenanceController.phpapp/Controllers/PrestitiApiController.phpapp/Controllers/PrestitiController.phpapp/Controllers/ReservationManager.phpapp/Controllers/ReservationsAdminController.phpapp/Controllers/ReservationsController.phpapp/Controllers/UserActionsController.phpapp/Controllers/UserDashboardController.phpapp/Models/CopyRepository.phpapp/Models/DashboardStats.phpapp/Models/LoanRepository.phpapp/Services/CapacityService.phpapp/Services/ReservationReassignmentService.phpapp/Support/DataIntegrity.phpapp/Support/IcsGenerator.phpapp/Support/MaintenanceService.phpapp/Support/NotificationService.phpapp/Views/libri/scheda_libro.phpapp/Views/prestiti/dettagli_prestito.phpapp/Views/prestiti/index.phpapp/Views/prestiti/modifica_prestito.phpconfig/settings.phpcron/automatic-notifications.phpcron/full-maintenance.phpscripts/bulk-enrich-cron.phpscripts/check-expired-reservations.phpstorage/plugins/mobile-api/src/Controllers/ActionsController.phptests/copy-count-inventory.unit.phptests/email-notifications.spec.jstests/full-test.spec.jstests/loan-edge-cases.unit.phptests/loan-reservation-complete.spec.jstests/loan-reservation-consistency.unit.php
Comment on lines
+1762
to
+1774
| // Rimuovi copie in eccesso DALLA CODA (le ultime aggiunte), solo quelle | ||
| // disponibili e senza impegni (#238: prima si rimuoveva la PRIMA della | ||
| // lista ASC, lasciando codici col suffisso più alto → collisioni dopo). | ||
| $removable = $copyRepo->getRemovableCopiesNewestFirst($id); | ||
| $toRemove = $currentCopieCount - $newCopieCount; | ||
| $removed = 0; | ||
|
|
||
| foreach ($copie as $copia) { | ||
| if ($removed >= $toRemove) | ||
| foreach ($removable as $copia) { | ||
| if ($removed >= $toRemove) { | ||
| break; | ||
|
|
||
| // Rimuovi solo copie disponibili senza prestiti attivi | ||
| if ($copia['stato'] === 'disponibile' && empty($copia['prestito_id'])) { | ||
| $copyRepo->delete($copia['id']); | ||
| $removed++; | ||
| } | ||
| $copyRepo->delete($copia['id']); | ||
| $removed++; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Rendi atomica la selezione e la rimozione delle copie.
Tra la query a Line 1765 e il DELETE a Line 1773, un prestito può impegnare la copia: qui non viene acquisito il lock del libro né viene rieseguita la verifica. Inoltre $removed aumenta anche se delete() restituisce false. Esegui lock libro → selezione/rivalidazione → delete condizionale nella stessa transazione, incrementando il contatore solo dopo una cancellazione riuscita.
As per path instructions, “CRITICO: ogni query sulla tabella libri DEVE avere AND deleted_at IS NULL”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Controllers/LibriController.php` around lines 1762 - 1774, Rendi atomico
il flusso intorno a getRemovableCopiesNewestFirst: avvia una transazione,
acquisisci prima il lock del libro, poi seleziona o rivalida le copie
disponibili senza impegni e rimuovile nella stessa transazione; ogni query su
libri deve includere AND deleted_at IS NULL. Usa una cancellazione condizionale
che fallisca se la copia è stata impegnata e incrementa $removed solo quando
delete() restituisce true, quindi esegui commit o rollback correttamente.
Source: Path instructions
- LibriController: clamp copie_totali to 1..9999 in update() (as store());
reduction now uses a conditional delete (deleteIfRemovable) and counts only
real deletions; base length is capped so generated -C{N} codes fit VARCHAR(100).
- CopyRepository: allocateInventoryCodes caps the base at 90 chars; avoids the
short-var/count-in-loop nits; new deleteIfRemovable() closes the select→delete
race (FK RESTRICT is the hard backstop).
- PrestitiController: wishlist gate query on libri now filters deleted_at IS NULL.
- NotificationService: the warning and overdue atomic claims re-assert
data_scadenza so a concurrent renew() can't fire a stale-date notification.
- ReservationsAdminController: normalize inverted start/end in update() and store();
extracted loanDurationDays() helper (was triplicated).
- IcsGenerator: chmod 0644 on the temp file before rename (umask-proof) and a
random_bytes temp suffix.
- config/settings: empty env string treated as unset (DB_PORT='' → 3306); removed
the redundant displayErrorDetails override.
- tests: copy-count uses a per-run token so cleanup only touches its own rows;
loan-edge-cases counter aligned to 60.
PHPStan L5 clean; copy-count 8/8, loan-edge-cases 60/60, consistency green.
…rable padding (#238) Nikola reported that label content didn't scale with the label dimensions — only the surrounding whitespace grew, leaving text and barcode a fixed small size and wasting the label area, especially on larger formats. Root cause: fonts were derived only from width with low clamps (min(7..10)), the barcode was capped (min 44mm wide / 10mm tall), and cell heights were hardcoded in mm, so the layout never filled a bigger label. - Replace the two fixed-size renderers with one proportional renderer: content is a stack of weighted blocks (barcode dominates) that together fill the whole label area, for both orientations. Each text line auto-fits — the font starts from its allocated block height, then shrinks only if needed to fit the width (never overflows). The barcode fills the width and its allocated height. - Add a configurable 'Inner padding (mm)' label setting (label.padding), applied on every side on top of the automatic margin, clamped against the label size. - The content toggles (app name / title / subtitle / author-publisher / dewey) are honoured unchanged. Verified visually at 25×38, 70×36, 89×41 and 60×90 (content now fills the label), and by a new regression test that measures the title glyph height via pdftotext -bbox: it must grow ≥1.8× from a small to a large label, and padding must shrink it. Existing label E2E (page size, per-copy, toggles) still passes. PHPStan L5 clean; 3 new strings added to all four locales.
Phase 22.3 flaked with 'Navigation to /admin/archives/new is interrupted by another navigation to /admin/plugins' — a prior phase's async page reload racing the goto (same class as the plugin-activation steps). Settle + retry once, matching the existing guards.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Views/settings/index.php`:
- Line 646: Replace HtmlHelper::e() in the label_padding input within the
settings view with htmlspecialchars(..., ENT_QUOTES, 'UTF-8'), preserving the
existing fallback value and string conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c57f293-ee4e-422d-bfda-6f20af4a1098
📒 Files selected for processing (16)
app/Controllers/LibriController.phpapp/Controllers/PrestitiController.phpapp/Controllers/ReservationsAdminController.phpapp/Controllers/SettingsController.phpapp/Models/CopyRepository.phpapp/Support/IcsGenerator.phpapp/Support/NotificationService.phpapp/Views/settings/index.phpconfig/settings.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsontests/copy-count-inventory.unit.phptests/label-pdf-content.spec.jstests/loan-edge-cases.unit.php
Project rule: views escape with htmlspecialchars(..., ENT_QUOTES, 'UTF-8'), not HtmlHelper::e(). Aligns the line I added (CodeRabbit).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Independent review of the loan/reservation overhaul plus the user-reported copy-number bug (#238). Every fix is backed by a behavioral test that fails on the old code.
Correctness (HIGH)
DataIntegrity): the open-ended sentinel produced anendExclusiveof10000-01-01, whichksortordered before every real date, zeroing overdue occupancy in the daily-peak scan. Sentinel is now9999-12-30(its+1day stays a 4-digit key) plus a defensive clamp. New regression test asserts detection (fails on the old code).prestiti → copie → libri, opening a deadlock window against the maintenance path (which lockslibrifirst). Now takes the book-row lock first — canonicallibri → prestiti → copie.Copy-number generation — #238
Reproduced Nikola's exact
1 → 2 → 1 → 2sequence: old code crashed withDuplicate entry '…-C2'. Fixed:{base}-C{N}on every copy (never a bare base).CopyRepository::allocateInventoryCodes/getRemovableCopiesNewestFirst, covered bycopy-count-inventory.unit.php.Consistency / robustness (MEDIUM/LOW)
CapacityServicecounts legacy reservations with NULLdata_inizio_richiesta;getBookAvailabilityDatagot the sameattivoqualification asgetBookAvailability(app + web agree again).in_corso/in_ritardo) — no 404 dead-ends.CopyControllerno longer wipes a loan's note on copy-form return;LoanRepository::closedoes a final recalc so the wishlist gate can't fire on a stale counter;rejectLoan's deliberate soft-delete exception is documented.MaintenanceControllerno longer runs the whole-table recalc twice per click (also a double-count fix);IcsGeneratorwrites atomically (tmp + rename,LOCK_EX); cron scripts use DotenvsafeLoadso container/CLI runs boot without a.env.Verification
PHPStan L5 clean; loan-edge-cases 60/60, copy-count 8/8, loan-reservation 21/21, loan-reservation-complete 26/26, loan-overlap 39/39, email-notifications 14/14, full-test 136/136 — all real-browser E2E on Apache. No Android/API-contract change (only backend accuracy;
getBookAvailabilityDataschema unchanged).Summary by CodeRabbit