Loan extension: overdue-status recompute, localized date picker, bulk extend (#281)#284
Merged
Conversation
…e picker, bulk extend Addresses HansUwe52's loan-management report (#281) in three parts. 1. Overdue status not cleared after extending the due date. prestiti.stato is a stored enum, transitioned in_corso -> in_ritardo one-way by the maintenance/integrity jobs and never reverted, while the Edit-Loan save (LoanRepository::update) deliberately never touches lifecycle columns. So extending an overdue loan's due date left stato='in_ritardo' and it kept showing "Overdue". PrestitiController::update() now recomputes stato against the new due date (in_ritardo if past, else in_corso), scoped to the physically-out states so prenotato/da_ritirare are untouched, using the app timezone (DateHelper::today, same clock as MaintenanceService). Returning to in_corso also resets the reminder flags so the new window notifies afresh. 2. Date picker always Italian for non-English UI languages. flatpickr-init.js detectAppLocale() only recognized it/en and fell through to Italian, so German/French/Danish installs got an Italian calendar. vendor.js now bundles the de/fr/da flatpickr l10n and registers them; detectAppLocale() maps every shipped UI language and defaults to neutral English; date display is day-first for the European languages. The create-loan inline picker got the same fix. vendor.bundle.js rebuilt. 3. Bulk loan extension. New PrestitiController::bulkExtend() + POST /admin/loans/bulk-extend: extend the due date of several selected active out-loans by N days at once, recomputing stato per (1). Deliberately mirrors the manual Edit-Loan semantics (not renew(), which refuses overdue loans) since the point is extending overdue loans. The loans list gains a per-row checkbox (only for extendable loans), select-all, and a bulk bar; selection persists across DataTables page draws. New i18n keys added to all four locales. Verified: loan-extension-281.unit.php (12/12 — single + bulk recompute, scoping, reminder reset), a real browser bulk-extend against the running app (loan extended +7 days, status recomputed, success flash), PHPStan clean.
|
Warning Review limit reached
Next review available in: 30 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 Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesL’estensione bulk consente di selezionare prestiti attivi, verificare capacità e conflitti, aggiornare scadenza e stato atomically e mostrare l’esito nell’interfaccia. Flatpickr supporta italiano, inglese, tedesco, francese e danese. Gestione prestiti
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant LoanTable
participant PrestitiController
participant Database
Admin->>LoanTable: seleziona prestiti estendibili
LoanTable->>PrestitiController: invia ids[] e giorni
PrestitiController->>Database: blocca libri e prestiti
Database-->>PrestitiController: restituisce capacità e conflitti
PrestitiController->>Database: aggiorna scadenze e stati
PrestitiController-->>LoanTable: redirect con risultato
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
…ion strings CodeRabbit flagged %n (a dangerous C sprintf specifier PHP rejects) in the bulk loan-extension count strings. They are only ever expanded via JS String.replace, so no crash occurs today, but %n is a footgun if ever passed to PHP sprintf/vsprintf. Switched to the standard %s (count) + %d (days), which sprintf accepts and the placeholder-parity check recognizes. Keeps all shared locales byte-identical across the open PRs.
bulkExtend() added N days to each loan's CURRENT due date, so extending a deeply-overdue loan by a smaller N left it in the past and it stayed "Overdue" (e.g. 40 days overdue + 14 = still 26 days overdue). It now extends from the LATER of today and the due date: an overdue loan extends from today (lands in the future, status recomputes to in_corso), while a loan not yet due keeps extending from its own due date exactly as before. Mirrors the intent of the single Edit-Loan path, which already clears overdue via an absolute date. loan-bulk-extension-capacity.unit.php drives the real controller: a 40-days- overdue loan extended by 14 now lands at today+14 and returns to in_corso; a not-yet-due loan still extends from its due date (today+2 -> today+16). 11/11. PHPStan level 5 clean.
…icker Two real-browser specs complementing the controller unit tests: - loan-bulk-extend-ui-281.spec.js: drives the actual admin UI — selects a deeply-overdue loan, extends it by 14 days via the bulk action bar, and asserts it persists today+14 and flips back to in_corso. Also checks the Edit Loan flatpickr registers de/fr/da and renders the app locale. - loan-picker-locale-de-281.spec.js: the real cross-language proof. It switches the install to de_DE (languages default + system setting + the admin's own locale, since a logged-in user's locale drives the session), opens Edit Loan, and asserts the flatpickr calendar renders GERMAN weekday names (Mi/Do, not Mer/Gio) — then restores it_IT and removes every fixture. This is what the bug was about: the calendar must follow the configured locale, not a hardcoded Italian default. All three pass against the local Apache :8081 install.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/PrestitiController.php`:
- Around line 1283-1443: Riduci la complessità e la lunghezza di bulkExtend
estraendo la logica per singolo prestito in un metodo privato dedicato: calcolo
di newDueDate, verifica di CapacityService e copyOverlap, aggiornamento della
riga e conteggio dell’estensione. Mantieni in bulkExtend la validazione, la
gestione della transazione/rollback e il ciclo sui prestiti, passando al nuovo
metodo tutte le dipendenze e i dati necessari e preservando gli stessi
comportamenti in caso di conflitto o errore.
- Around line 1291-1300: Imponi un limite massimo a count($ids) nella
validazione iniziale del bulk-extend, insieme ai controlli esistenti su $ids e
$days, usando una costante o configurazione già disponibile per il limite batch.
Se il numero di prestiti supera il limite, restituisci lo stesso errore
bulk_extend_invalid senza avviare la transazione.
In `@tests/code-quality.spec.js`:
- Around line 322-323: Extract the repeated placeholder-normalization replace
chain into a shared helper function in tests/code-quality.spec.js, then reuse
that helper at the current normalization sites around normalizedPhpSources,
lines 374-376, and lines 406-408. Preserve the existing conversion behavior for
parameterized placeholders and plain placeholders while removing the duplicated
inline replacements.
🪄 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 Plus
Run ID: 050a85d3-299f-4677-a35f-7450d0316d70
📒 Files selected for processing (19)
app/Controllers/PrestitiController.phpapp/Routes/web.phpapp/Views/prestiti/crea_prestito.phpapp/Views/prestiti/index.phpfrontend/js/flatpickr-init.jsfrontend/js/vendor.jslocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonpublic/assets/661.bundle.jspublic/assets/flatpickr-init.jspublic/assets/main.csspublic/assets/vendor.bundle.jstests/code-quality.spec.jstests/loan-bulk-extend-ui-281.spec.jstests/loan-bulk-extension-capacity.unit.phptests/loan-extension-281.unit.phptests/loan-picker-locale-de-281.spec.js
Address CodeRabbit/PHPMD on bulkExtend(): - Cap the batch at BULK_EXTEND_MAX_LOANS (500). count($ids) is now validated alongside $days, so a "select all" can't hold FOR UPDATE on an unbounded set of libri/prestiti rows for the whole transaction and block concurrent loans/returns/reservations. Over the cap returns bulk_extend_invalid before any transaction starts. - Extract the per-loan work (new due date from max(today,due), capacity + copy overlap checks, the update) into applyBulkLoanExtension(), which returns the affected-row count or null on a conflict. bulkExtend() keeps validation, transaction/rollback and the loop; the all-or-nothing conflict contract is unchanged. Cuts the method's cyclomatic/NPath complexity and length. Behaviour-preserving: loan-bulk-extension-capacity 11/11, loan-extension-281 18/18, PHPStan level 5 clean.
… cap
- code-quality.spec.js: extract the repeated Slim placeholder-normalization
replace chain ({id:\d+} -> {}) into a single stripRoutePlaceholders() helper
and reuse it at the three sites that inlined it, so the copies can't drift.
15/15 unchanged.
- loan-bulk-extension-capacity.unit.php: pin the new batch-size cap — a 501-id
request (> BULK_EXTEND_MAX_LOANS) is rejected as bulk_extend_invalid before
any lock/transaction. 12/12.
Owner
Author
Code reviewBranch: Found 10 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (2)
Details and fix proposalsF001 — The stato/reminder-flag recompute after update() runs unconditionally on every loan edit, not only when data_scadenza changed, so editing an unrelated field on a non-overdue in_corso loan resets warning_sent/overdue_notification_sent to 0, re-arming duplicate 'due soon'/overdue emails for an unchanged schedule.File: Evidence:
Approach: Gate the stato/reminder-flag recompute on data_scadenza actually changing, mirroring the line-749 precedent but using a data_scadenza-specific guard (a data_prestito-only edit must not reset flags either). Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260724T114558Ze04cd2): fixed and verified F003 — Inserting the bulk-select checkbox as DataTables column 0 shifts every data column by one, but PrestitiApiController::list's columnMap still maps index 0->l.titolo, 1->utente, 2->data_prestito, 3->data_scadenza, 4->stato, so column-header sorting is off-by-one (clicking 'Libro' sorts by Utente, etc).File: Evidence:
Approach: Shift the server columnMap keys +1 to match the post-PR client indices; update the stale comment. Optional hardening: name-based ordering (future). Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260724T114558Ze04cd2): fixed and verified Fix runsRun
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F001 | FG-1 | ✓ fixed and verified | |
| F003 | FG-2 | ✓ fixed and verified | |
| F010 | FG-3 | ✓ fixed and verified |
Run fixrun_20260724T171241Zc97ea6 — 2026-07-24T17:12:41Z
- Outcomes: 1 fixed and verified
- Commits:
1af2efa
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F007 | FG-1 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Fix groups (committed): - [FG-1] F001 — PrestitiController.php, loan-extension-281.unit.php: verified. Gate the #281 stato/reminder-flag recompute on data_scadenza actually changing, so editing an unrelated loan field no longer resets warning_sent/overdue_notification_sent (duplicate reminder emails). Unit test extended with the unchanged-due-date regression case (20/20). - [FG-2] F003 — PrestitiApiController.php: verified. Shift the DataTables columnMap keys +1 for the new checkbox column 0, fixing off-by-one header sorting (clicking 'Libro' sorted by Utente). - [FG-3] F010 — prestiti/index.php + 4 locales: verified. Singular/plural branching on the loan count for the bulk-extend confirm and toast ('1 prestito' vs 'N prestiti'), singular keys added to it/en/de/fr in the same commit. Post-fix review: 3/3 groups verified complete; 0 partial; 0 reverted.
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)
tests/code-quality.spec.js (1)
380-386: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftEscludere commenti e stringhe dai match delle route.
Il parser ignora commenti e stringhe solo durante il conteggio delle parentesi, ma poi restituisce il testo grezzo. Di conseguenza,
getRe.test(normalized)edirectGetRe.test(normalized)possono riconoscere una route commentata o presente in una stringa, facendo passare il test anche quando l’endpoint non è registrato. Applicare il match solo a token PHP effettivi e aggiungere una fixture per commenti/stringhe.Also applies to: 409-418
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/code-quality.spec.js` around lines 380 - 386, Update groupedGetExists and the corresponding directGetRe matching path to strip or tokenize PHP comments and string literals before testing route expressions, so only actual PHP tokens can match. Preserve route placeholder normalization and add fixtures covering commented-out and string-contained routes to ensure unregistered endpoints do not pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/code-quality.spec.js`:
- Around line 34-37: Update stripRoutePlaceholders so normalization targets only
route string literals rather than arbitrary PHP blocks; the current broad
replacement can collapse code containing braces and remove valid routes from
normalizedRouteSources. Extract route literals before applying placeholder
normalization, or use PHP tokenization, while preserving replacement of route
placeholders within each literal.
---
Outside diff comments:
In `@tests/code-quality.spec.js`:
- Around line 380-386: Update groupedGetExists and the corresponding directGetRe
matching path to strip or tokenize PHP comments and string literals before
testing route expressions, so only actual PHP tokens can match. Preserve route
placeholder normalization and add fixtures covering commented-out and
string-contained routes to ensure unregistered endpoints do not pass.
🪄 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 Plus
Run ID: a6a3841b-affa-4d98-89b6-5a1733b5289c
📒 Files selected for processing (10)
app/Controllers/PrestitiApiController.phpapp/Controllers/PrestitiController.phpapp/Views/prestiti/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsontests/code-quality.spec.jstests/loan-bulk-extension-capacity.unit.phptests/loan-extension-281.unit.php
…d comments
Two CodeRabbit findings on the route checks in code-quality.spec.js:
- stripRoutePlaceholders' broad `{[^}:]+}` pattern could swallow a whole PHP
block containing no colon (`if ($x) { $app->get('/api/v1/foo', $h); }` ->
`if ($x) {}`), deleting real routes from the normalized sources. Both
patterns now require an identifier-like placeholder name, which matches
every Slim placeholder in the repo and can never match a code block. The
README-side inline normalization now reuses the same helper.
- Route regexes matched raw source, so a commented-out route registration
could still satisfy the checks. Added a string-aware stripPhpComments()
(same state machine as the group-body walker; string literals preserved so
'https://...' is never truncated) applied to every PHP source before any
route matching.
Self-test fixtures added inline (block survives normalization; constrained +
plain placeholders collapse; commented-out routes removed while real code and
string URLs survive). 15/15.
…F007) Clicking "Estendi prestiti selezionati" with a typed out-of-range days value (or a cleared field, which parses to 0) silently did nothing — the click handler returned with no feedback. The spinner arrows respect min/max, but direct typing bypasses them and type="button" never triggers native constraint validation. Now an out-of-range value surfaces a SweetAlert error naming the valid range (1-365) and refocuses the input (plain alert() fallback without Swal). The empty-selection early return stays silent by design — the action bar is hidden without a selection. New string added to all four locales in the same commit per the i18n rule. E2E: new regression test drives the real UI — types 0, clicks, asserts the error dialog appears with the range text and that no submit/navigation happened. 3/3 green.
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 `@locale/en_US.json`:
- Line 6601: Update the English translation value for “Nessun prestito esteso”
in the locale dictionary to use the plural zero-results wording “No loans
extended,” while leaving the translation key unchanged.
🪄 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 Plus
Run ID: 37bc6ef2-aafa-43d1-8511-2bb36b16ff3b
📒 Files selected for processing (7)
app/Views/prestiti/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsontests/code-quality.spec.jstests/loan-bulk-extend-ui-281.spec.js
CodeRabbit: the string shows when n === 0, so 'No loan extended' is
grammatically wrong — 'No loans extended'. Same idiomatic zero-plural for
German ('Keine Ausleihen verlängert'). Italian and French zero forms
(nessun/aucun + singular) are already correct.
# Conflicts: # locale/de_DE.json # locale/en_US.json # locale/fr_FR.json # locale/it_IT.json # tests/code-quality.spec.js
fabiodalez-dev
added a commit
that referenced
this pull request
Jul 24, 2026
… columnMap test indices Two merge artifacts surfaced by the full unit battery on main: - locale/da_DK.json lacked the three strings added after #283 branched (bulk-extend singular forms + the invalid-days message); added in Danish ('lån' is invariable, so singular matches the plural phrasing). - loan-due-visibility-email.unit.php asserted the pre-#281 DataTables columnMap indices (3=>data_scadenza, 4=>stato); the bulk-select checkbox at column 0 shifted them to 4/5 (the fix verified in #284). Test updated. danish-locale-279 18/18, loan-due-visibility-email 9/9.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #281 (HansUwe52's loan-management report). Three parts.
1. 🐛 Overdue status stays "Overdue" after extending the due date
prestiti.statois a stored enum. The maintenance/integrity jobs move a loanin_corso → in_ritardoone-way (nothing ever reverts it), and the Edit-Loan save (LoanRepository::update) deliberately never touches lifecycle columns — so extending an overdue loan's due date updated the date but leftstato='in_ritardo', and every view renders that stored value verbatim. Hence the loan kept showing Overdue.PrestitiController::update()now recomputesstatoagainst the new due date (in_ritardoif past, elsein_corso), scoped to the physically-out states soprenotato/da_ritirareare untouched, usingDateHelper::today()(app timezone, same clock asMaintenanceService). When a loan returns toin_corsothe reminder flags reset so the new window notifies afresh.2. 🐛 Date picker always Italian for non-English UI languages
Uwe: "If a language other than English is used, the calendar is displayed in Italian."
flatpickr-init.jsdetectAppLocale()only recognizedit/enand fell through to Italian for everything else. Nowvendor.jsbundles thede/fr/daflatpickr l10n and registers them,detectAppLocale()maps every shipped UI language and defaults to the neutral English, and the date display is day-first for the European languages. The create-loan inline picker got the same fix.vendor.bundle.jsrebuilt.3. ✨ Bulk loan extension
Uwe: "select multiple loans or all loans and extend the loan period." New
PrestitiController::bulkExtend()+POST /admin/loans/bulk-extend: extend the due date of several selected active out-loans by N days at once, recomputingstatoper part 1. It deliberately mirrors the manual Edit-Loan semantics — notrenew(), which refuses overdue loans and enforces the renewal limit — because the whole point is extending loans that are already overdue. The loans list gains a per-row selection checkbox (only for extendable loans), select-all, and a bulk action bar; selection persists across DataTables page draws.Verified
tests/loan-extension-281.unit.php— 12/12 (single + bulk recompute, state scoping, reminder reset, symmetric past/future transitions).data_scadenzaadvanced,statorecomputed, success flash shown.Note: the Danish (
da_DK) calendar localization here also depends on #283 shipping theda_DKlanguage; on an it/en/de/fr install parts 1–3 are fully self-contained.Summary by CodeRabbit