Release 0.7.59-rc.3: integrated fixes and hardened release gates - #341
Merged
Conversation
…lete
Full-domain coherence review of the loan/reservation system. Eleven fixes,
each verified first-hand before changing anything:
Soft-delete & data exposure
- getBookAvailabilityData() now guards `deleted_at IS NULL` and returns null
for missing/soft-deleted books; every caller (calendar route, localized
availability route, disponibilita endpoint, mobile API) 404s cleanly. The
localized public route used to serve real per-day occupancy for
soft-deleted books to anonymous clients.
One clock everywhere (app timezone via DateHelper)
- calculateAvailability() default start was `new DateTime()` (process TZ):
the mobile calendar began on "yesterday" between midnight and 2am app time.
- DashboardStats mixed three clocks (SQL server date, process date(),
DateHelper) for the same `data_prestito <= today` predicate.
- DataIntegrity expired prenotazioni on server NOW() while
ReservationManager used the app clock for the same rows.
- User dashboard and pending-loans views computed overdue with time()
(process TZ), flagging loans overdue on the afternoon of the due day —
the cron only does so from the following day.
- crea_prestito prefills now use the app today + the configured
loan_duration_days (was process today + '+1 month').
Availability payload coherence
- next_due on /api/books/{id}/availability filtered only `attivo = 1`: it
could return the past due date of an overdue loan or the far-future date
of a scheduled one. Now holding-states only and never in the past.
- occupied_ranges on /api/libri/{id}/disponibilita omitted the
pendente-with-copy arm and the active reservation queue, contradicting
first_available computed per-day in the same response.
- Web calendar routes now exclude the requesting user's own reservations
(query param the mobile app and the server write gate already honoured),
so the picker no longer paints red days the server would accept.
Guards & lifecycle symmetry
- PrestitiController::store() strict-validates both dates as ISO like
update() does: the old strtotime guard was defeated by int<=false
coercion on unparseable input, and '12/03/2026' parsed American-style.
- LoanRepository::update() fallback honoured a hardcoded +14 days — half
the seeded loan_duration_days default; now reads the setting.
- rejectLoan now promotes the waitlist after freeing capacity (it was the
only release path that left a freed copy idle until the next maintenance
run) and flushes deferred notifications after commit.
New regression guard: tests/loan-coherence-audit.unit.php (10 checks).
Verified: PHPStan level 5 clean; 99 standalone unit tests pass; E2E
loan-reservation-complete 26/26, loan-state-model + loan-overlap 43/43,
full-test 137 passed / 8 install-skipped; soft-delete guard exercised live
(200 → soft-delete 404 → restore 200).
…ng, reject audit, renewal email Follow-ups agreed after the coherence audit, plus the second report on #301. Every fix was verified behaviourally and the whole branch diff went through an adversarial multi-agent review whose three confirmed findings are fixed here too. Auto-approval on the real entry point (#301) - The book-detail modal posts to ReservationsController::createReservation, which never consulted `auto_approve_requests` — the option only worked on the other (form) entry point, so real users' requests always landed in the approval queue. The modal path now promotes through the same race-safe canonical approval pipeline; with the option on, an available-copy request lands directly in "waiting for pickup" (pickup confirmation deliberately stays). The modal SweetAlert now branches on auto_approved instead of telling the user to wait for an approval that already happened. - New behavioural test drives the real endpoint with the option off and on (10 assertions: off → pending preserved; on → approved, copy assigned, pickup deadline set). app.timezone becomes a real setting - ConfigStore default + per-locale installer seeds (it→Rome, de→Berlin, fr→Paris, da→Copenhagen, en_US→UTC) + a validated select in the loans settings tab (DateTimeZone::listIdentifiers whitelist; invalid input is ignored). The adversarial review caught that loadDatabaseSettings()'s app-category allowlist never mapped the row back — without that mapping the whole feature was inert. Verified end-to-end over HTTP: save Copenhagen → the select reads back Copenhagen; existing installs without the row keep the previous Europe/Rome behaviour. Reject with audit instead of DELETE - rejectLoan was the only terminal transition that destroyed its row. It now marks the request annullato with processed_by + reason note, preserving history and statistics; the affected_rows race guard and the duplicate-check semantics (user can re-request) are unchanged. Renewal confirmation email - New loan_renewed template (base + the 4 locale overrides) and sendLoanRenewedNotification, fired post-commit by renew(): the borrower now learns the new due date even when the librarian renews at the desk. Verified live via Mailpit (renewal moved the due date and delivered the email with the new date and remaining renewals). Availability badge copy - "Non Disponibile" → "Non disponibile oggi" on both the hero badge and the sidebar state (the review caught the sidebar leftover): the aggregate badge is a today snapshot and no longer reads as contradicting the calendar's free future days. i18n: 8 new keys in all 5 locales (parity 6774 everywhere); the mail-template count check now derives from the base catalogue instead of hardcoding 22. Verified: PHPStan level 5 clean; 100 standalone unit tests pass (incl. the new reservation-path #301 test and 21 coherence guards); E2E loan suites 69/69 and full-test 137 passed / 8 install-skipped; timezone save/read-back and renewal email exercised live over HTTP.
#333 — a loan cancelled by the user (stato='annullato') showed as "Unknown" in every admin view and looked stuck: no badge/label switch handled the enum value. Render it everywhere (loans list SSR + DataTables, details page, user details, book page history), add an Annullato filter button and CSV-export state, and stop showing "not yet returned" for loans closed without a return (annullato/scaduto). #334 — the loans-overview and integrity-report page headers were sticky at top-0 z-30, the same z-index as the layout header but later in the DOM, so they painted over the app header and its notifications dropdown. Drop the sticky positioning. #336 — editing a loan's dates re-checked capacity over the WHOLE new window, so a commitment already coexisting with the current period (e.g. a queued reservation) bounced ANY date edit — even shortening — with no_copies_available. update() now checks only the newly-claimed date segments (renew()'s extension-window convention) and bulk extension checks the extension window only. Also surface clear error banners: /admin/loans explains capacity conflicts and renew() failures, and the admin book page now recognizes the 'extension_conflicts' / 'renewal_failed' keys renew() actually emits (it only knew the never emitted 'renewal_conflict'). New strings translated in all bundled locales. Closes #333, closes #334, closes #336. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
… in user history Follow-up to #333. Instead of five per-view copies of the stato → badge switch (the reason 'annullato' was missed in the first place), app/Views/partials/loan-status-badge.php is now the single source of truth for color/icon/label of every prestiti.stato value. All admin surfaces consume it: the loans list (both the SSR rows and the DataTables column, via a PHP-generated JS map), the loan details page, the user details page and the book page loan history. Labels come from the existing translate_loan_status() helper, already used by the PDF and CSV export, so text and badges cannot diverge. The partial lives under app/Views (not app/helpers.php) because Tailwind's content globs only scan app/Views/** — moving the class names out of the views would drop them from the compiled CSS. User-facing change: cancelled (annullato) and pickup-expired (scaduto) loans now appear in the user's loan history (profile + account dashboard + history counter) instead of vanishing. They sort by their closing time (COALESCE with updated_at) so a NULL return date doesn't sink them to the bottom, show a dedicated icon/label, and hide the "leave a review" button since the book never went out (the server would reject the review anyway). Regression guards extended accordingly (36 checks). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
Completes the #333 centralization sweep. New loan_status_label_map() in app/helpers.php (enum → label via translate_loan_status) feeds the JS lookups that previously kept hand-maintained copies: the stats 'loans by status' chart and the admin dashboard calendar popup. The ICS feed (IcsGenerator::translateStatus), the book page occupancy calendar, the user history views and the loans-overview state chips now delegate to translate_loan_status() as well; the user dashboard active-loan badges keep their frontend styling but take their label text from the helper. Side effect worth noting: the calendar/ICS alias "Scaduto" for in_ritardo is gone — since 'scaduto' (pickup expired) is a real enum state, that alias had become genuinely ambiguous; everything now says "In Ritardo". Two deliberate exceptions are documented in place: the per-copy row hint on the book page ("In prestito" describes the physical copy, not the loan state) and the ICS event titles (event naming with emoji, not status labels). The return form's outcome select also stays local: it mixes loan outcomes with copy destinations ("Restituito — copia in manutenzione"), which are not stato labels. Regression guards extended to 44 checks, including 'no local stato → label literal map left' probes on the swept views. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
…abel (1.4.3) The mobile history had the same gap just fixed on the web: cancelled (annullato) and pickup-expired (scaduto) loans vanished from /me/loans history. The endpoint now returns them, ordered by closing time (COALESCE with updated_at) so rows without a return date don't sink. 'status' is documented as the raw prestiti.stato value, so the extra states are an additive, spec-compatible change. Every loan payload also gains 'status_label': the server-localized label from the canonical translate_loan_status() helper (#333), so clients no longer need their own stato→label map and future enum values degrade gracefully. Documented in the OpenAPI schema; plugin version bumped to 1.4.3. The public API needs no change: it only ever exposes a book's active loan (attivo=1), which cancelled loans never are. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
- update(): re-read data_prestito/data_scadenza (and utente_id) in the FOR UPDATE query and rebuild the effective values + range validation from the LOCKED row. The claimed-windows capacity math previously used the pre-transaction read, so a concurrent update between the initial read and the lock could get its dates silently overwritten and the capacity check computed against a stale old window. The pre-transaction validation stays as a fast-fail. - History ordering: drop DATE() from the COALESCE fallback in the three history queries (profile, user dashboard, mobile API) so two loans closed the same day sort by their real closing moment. - /admin/loans banner: handle 'renewal_failed' explicitly. - user dashboard review button: htmlspecialchars(..., ENT_QUOTES) instead of HtmlHelper::e() in the data-book-title attribute (path rule). - Regression test: noisy source reader (missing/empty file → exit 1), ordered-position guards + non-empty assertions on the extracted update()/applyBulkLoanExtension() sections, so the negative checks can no longer pass vacuously. - mobile-api: additive 'requested_at' (DATE of created_at) on every /me/loans payload, documented in OpenAPI — gives clients an honest date for cancelled/expired loans, which never went out (pairs with the Android PR #30 review); status_label description softened to match the fallback-for-unknown-states semantics. Guards now 51 checks, all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
- update(): strict date validation replaces strtotime(). New
isStrictIsoDate() requires exact Y-m-d AND a real calendar date
(round-trip via createFromFormat), so ambiguous inputs ('2026-2-5')
and impossible dates ('2026-02-30', which strtotime silently
normalizes) are rejected instead of flowing — as non-canonical
strings — into the lexicographic window math, the due-date-changed
comparison and LoanRepository::update(). Applied to both the
pre-transaction fast-fail and the under-lock re-validation.
- update(): claimed windows are now the EXACT set difference
new∖old — the boundary days (old start / old due) are already held
by this loan, so a commitment sitting only on a boundary day no
longer produces a false conflict.
- applyBulkLoanExtension(): one interval for one decision — capacity
and the same-copy overlap check both gate on the added days only
(day after the current due date → new due date); the copy check
previously scanned the whole loan window while capacity scanned the
extension window.
- mobile-api OpenAPI: requested_at declares nullability the 3.1 way
(type ['string','null'], no 'nullable' flag), matching mapLoan()'s
explicit null.
Regression guards extended to 54 checks (exact-diff boundaries, no
strtotime in update(), unified bulk interval, 3.1 nullability), all
green; isStrictIsoDate verified against ambiguous/impossible/leap-year
inputs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
…emantics CI's loan-reservation-consistency guard pins the exact history predicate shared by the three history consumers (user dashboard, profile, mobile API). PR #337 deliberately extended that predicate with the closed no-return states (annullato, scaduto) so cancelled/expired loans stop vanishing from history (#333) — update the pinned literal accordingly. The guard still enforces that all three consumers share one predicate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
… structural test guards
- isStrictIsoDate(): shape-check via /^\d{4}-\d{2}-\d{2}$/D BEFORE touching
DateTime — createFromFormat() throws ValueError on input containing NUL
bytes (data_prestito=2026-01-01%00), which outside the try block surfaced
as an HTTP 500 instead of invalid_dates. The anchor also rejects trailing
newlines and non-ASCII digits. Verified against NUL/newline/Arabic-digit
inputs: all return false, nothing throws.
- Regression guards made structural: the requested_at check now isolates
the schema line and asserts the ABSENCE of the legacy 'nullable' flag
alongside the 3.1 type array (the mixed form no longer passes), and the
exact-diff check asserts the boundary helpers actually feed the
claimedWindows bounds instead of merely appearing in the source.
54 guards green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
…al review-guard - locale/fr_FR.json: 'réservé par un autre prêt' implied the loan reserves the copy; 'affecté à un autre prêt' matches the source meaning (the copy is already assigned to another loan). - Regression guard: assert the exact $canReview exclusion expression in both user history views (each with its own loop variable, $p / $loan) instead of the variable's mere presence. 55 guards green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w
Self-review of the loan-coherence PR surfaced three real regressions the
PR itself introduced (plus one pre-existing bug it touched), all verified
by an independent post-fix reviewer.
[FG-1] F007 + F009 - auto-approve flow (cross-cutting, both mirrored helpers)
- F007: autoApproveLoanRequest() read the auto_approve setting BEFORE its
own try{, so a post-commit DB hiccup escaped to the outer transaction
catch and returned HTTP 500 for a request that was already committed -
and the duplicate-request guard then blocked the user's retry. Moved the
SettingsRepository construction + read inside the try in BOTH
ReservationsController and UserActionsController; a throw now degrades to
the pending_approval path as the contract intends.
- F009: with a future start_date the loan is 'prenotato' (scheduled), but
the response said "approvato - in attesa di ritiro" and the modal
promised pickup instructions. The helper collapsed approveLoan's outcome
to a bool. Now approveLoan exposes loan_state/is_future_loan, the helper
returns nullable-string, and ReservationsController + book-detail.php
branch the message/status/footnote on scheduled vs pickup. auto_approved
stays a real boolean. New scheduled-loan i18n key in all 5 locales.
[FG-2] F033 - dashboard soft-delete (pre-existing, project soft-delete rule)
- The five prestiti sub-counts in DashboardStats::counts() had no libri
join, so a loan on a soft-deleted book inflated the badge above what its
destination list (which joins deleted_at IS NULL) can show - a clickable
"1" linking to an empty page. Added the soft-delete join to every
sub-count, preserving pickup_pronti's parenthesised OR group and the
DateHelper clock.
Tests: extended loan-auto-approval-301-reservation-path.unit.php with a
future-date section and a post-commit fault-injection case; added a
soft-delete guard to loan-coherence-audit.unit.php.
Post-fix review: 2/2 groups verified complete; 0 partial; 0 reverted.
…date errors)
The four manual/policy findings from the adamsreview run, fixed concern-aware
(each addressing the verify-pass concern the naive hint would have missed) and
verified by an independent reviewer.
F043 - admin loan form hardcoded the Italian route literal /api/libro; the
endpoint is registered per-locale via RouteTranslator, so it 404s if it_IT is
deactivated or renamed. Now resolves route_path('api_book') into a
JSON_HEX_TAG-escaped JS constant (base path already included - window.BASE_PATH
dropped from that fetch), mirroring the book-detail.php precedent.
F012 - the two availability routes wired the SESSION user as the exclusion
subject, but the admin loan form fetches them with the operator's session while
booking for a different borrower, so it painted days green the write gate
rejects. The subject is now explicit: an admin/staff caller may pass
?for_user=<borrower> (privilege-gated, numeric-checked); everyone else keeps the
session-id default, so the public self-service picker is unchanged. The admin
form threads the selected borrower and re-fetches on borrower change.
F040 - a user already holding an active reservation got an all-green picker
(their own reservation is excluded) then a hard rejection from the date-less
duplicate guard on submit - a dead end. calculateAvailability now returns
has_active_reservation (same predicate as the guard), surfaced in the payload,
and book-detail.php short-circuits with an explanation instead of the doomed
picker. Deliberately did NOT also exclude the user's own holding loans: that
would falsely free an overdue (in_ritardo) copy still physically out.
F028 - a malformed date and an inverted date pair shared one error message.
store() now splits invalid_date_format (naming YYYY-MM-DD) from invalid_dates,
and update()'s twin check was ported from the ambiguous strtotime parse (the
'12/03/2026' American-read bug) to the same strict isISODateFormat validation.
New user-facing strings added to all 5 locales. Verified: PHPStan level 5 clean;
100 unit tests; E2E loan-reservation/state-model/overlap 69/69; independent
post-fix review verified 4/4.
… email settings Address two review findings on the auto-approve reservation-path test: - The helper posted application/x-www-form-urlencoded via withParsedBody(), so it exercised createReservation's getParsedBody() branch — not the json_decode branch the book-detail modal actually hits (getParsedBody does not parse JSON). Send a JSON body behind Content-Type: application/json so the test drives the real production path. - Setup overwrote the email driver_mode/type rows but the cleanup restored only auto_approve_requests, leaking 'mail' onto every later test in the same runner. Capture both originals before the UPDATE and restore them in cleanup.
…blet widths The related-books grid clips every book past the first row (grid-auto-rows:0 + overflow:hidden) with no hint that more exist. Below 768px a horizontal snap-scroll carousel already shows the next card peeking as the cue; between 768 and 1023.98px the grid silently hid the extras, so on a narrow desktop window a user could not tell there were more related books (mobile was fine). Raise the carousel breakpoint from 767px to 1023.98px so the narrow/responsive range gets the same reachable snap-scroll strip, and for the 768-1023.98px sub-range show two whole books with a third clearly half-visible (flex-basis 38%) as an unmistakable scroll-for-more cue instead of one dominant cover. Desktop (>=1024px) grid is unchanged; phone one-book carousel is unchanged. The a11y inert/aria sync already keyed off computed overflow-x, so it handles the wider scroll range with no JS change.
…1 integration Conflict resolution: - app/Controllers/PrestitiController.php update() date validation: both #335 (F028) and #337 hardened the pre-transaction date check away from strtotime. Kept #337's isStrictIsoDate validator (its test asserts >=4 occurrences and no strtotime, and the under-lock re-check already uses it) AND #335's separate error codes (invalid_date_format vs invalid_dates) for actionable admin feedback. store() keeps DateHelper::isISODateFormat per #335's test; the two strict-ISO validators are behaviourally equivalent (both reject 2026-02-30 and NUL bytes). - locale/*.json: union of both sides' added keys (11 from #335 + 3 from #337), parity restored at 6784 keys across all 5 locales. Both suites pass: issues-333-334-336 (55/0), loan-coherence-audit, and the #301 reservation-path test (23/23).
Rename migrate_0.7.58.sql -> migrate_0.7.59-rc.1.sql (and its behavioural test). 0.7.58 is already released, so a migration named after it is version-skipped for installs already on 0.7.58 (updater runs migrationVersion > fromVersion). Naming it after the RC that ships it makes it run on the RC upgrade and on the eventual stable 0.7.59. Bump version.json to 0.7.59-rc.1. Audited: none of the other bundled changes (#335 settings, #337 badge helpers, #339 copy ordering) require a migration — every new DB read targets pre-existing columns/ENUM values or passes a safe default, and app.timezone self-heals on save.
…uest A hook row registered by an OLD plugin version and dropped in a NEW one lingers in plugin_hooks because upgrades replace plugin files but never re-run a plugin's onActivate() (which resyncs hooks via deleteHooks + reinsert). The hook loader then logged "[PluginManager] Method not found: <method> for hook <hook>" on EVERY request — e.g. a live install showed Z39ServerPlugin's stale admin.menu.items -> addAdminMenuItem 73x/day. When the callback method genuinely does not exist on the (already-instantiated) plugin class and there is no __call magic, delete that dead hook row. The install self-heals on the first request after the upgrade; a legitimate future re-activation reinserts the correct hook set anyway. pruneOrphanHook() reports whether a row was actually deleted so the removal is logged exactly once (a cached hook list can re-present the orphan for up to the QueryCache TTL; the DELETE then matches zero rows silently). Applies to both hook-load paths (prefetched batch and per-plugin fallback). Test (tests/plugin-orphan-hook-prune.unit.php, 8 checks): deletes the orphan + reports true; idempotent second call reports false; and — critically — a legitimate sibling hook, a non-matching method, and a different plugin_id are all left untouched (precise match, no over-delete).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLa versione 0.7.59-rc.2 aggiorna prestiti, prenotazioni, disponibilità, fuso orario, serie, stati, notifiche, API, plugin e CI. Aggiunge controlli per date, soft-delete, capacità, localizzazioni e fixture E2E. ChangesGestione prestiti e catalogo
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Prestatario
participant ReservationsController
participant LoanApprovalController
participant NotificationService
Prestatario->>ReservationsController: invia richiesta con date
ReservationsController->>LoanApprovalController: tenta approvazione automatica
LoanApprovalController-->>ReservationsController: restituisce loan_state persistito
ReservationsController->>NotificationService: invia notifica se la richiesta resta pendente
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/PrestitiController.php (1)
231-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSpostare la validazione ISO prima del calcolo della scadenza.
Quando
data_prestitonon è ISO edata_scadenzaè vuota,strtotime()restituiscefalseedate()genera unTypeErrornon gestito prima del redirectinvalid_date_format.🤖 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/PrestitiController.php` around lines 231 - 256, Move the ISO validation in the loan creation flow before the default `data_scadenza` calculation, after applying only the `data_prestito` default. Ensure invalid `data_prestito` values redirect to `invalid_date_format` before `strtotime()` is called, while preserving the existing default loan-duration calculation for valid ISO dates.
🤖 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/prestiti/crea_prestito.php`:
- Around line 576-581: Proteggi il flusso fetch della disponibilità da risposte
obsolete quando cambia rapidamente il prestatario. Nel codice che costruisce
availabilityUrl e aggiorna availabilityByDate e bookAvailability, assegna un
identificatore crescente alla richiesta (oppure usa AbortController) e applica
la risposta solo se corrisponde ancora alla richiesta più recente.
In `@app/Views/prestiti/modifica_prestito.php`:
- Line 80: Update the fallback in the data_prestito value expression to use the
application timezone via DateHelper::today() instead of date('Y-m-d'). Preserve
the existing prestito value, HTML escaping, and default behavior when
data_prestito is present.
In `@locale/de_DE.json`:
- Line 6774: Update the translation for “Predefinito: Europe/Rome” so the
displayed label is localized while the IANA timezone identifier remains exactly
“Europe/Rome”, not “Europe/Berlin”.
In `@locale/en_US.json`:
- Line 6774: Update the locale entry for “Predefinito: Europe/Rome” so its
English translation states “Default: Europe/Rome” and matches the actual
ConfigStore timezone default.
In `@storage/plugins/mobile-api/src/Controllers/ActionsController.php`:
- Around line 107-113: Update the history SQL query in ActionsController to
include pr.data_scadenza in its SELECT list, so mapLoan() can populate due_at
for historical loans, including scaduto entries. Leave the existing filters,
ordering, and limit unchanged.
In `@tests/loan-coherence-audit.unit.php`:
- Around line 37-46: Update the file-loading setup in the loan coherence audit
test to verify each path exists and each read succeeds before casting or using
its contents. Centralize this behavior in a reusable helper, then use it for the
variables loaded from ReservationsController, web routes, model/controller
files, DataIntegrity, and dashboard views so missing files fail clearly with
their path instead of becoming empty strings.
- Around line 61-65: Replace the fixed-length source slices in the audit checks
with bodies delimited by complete method signatures and explicit end-marker
fallbacks, including the checks around $nextDuePos, approval, and rejection
logic. For rejectLoan, locate the exact rejectLoan signature rather than a
prefix match, compute whether the closing boundary was found, and add
$rejectBodyDelimited to the negative assertion so it cannot inspect unrelated
methods or the rest of the file.
In `@tests/plugin-orphan-hook-prune.unit.php`:
- Around line 53-58: Rendi il fixture del plugin nel test indipendente dai seed:
sostituisci la query MIN(id) e l’uscita tramite exit(1) con la creazione di un
plugin dedicato, usa il relativo ID per esercitare pruneOrphanHook e rimuovi il
plugin fixture al termine del test.
In `@tests/settings-orphans-hardening.unit.php`:
- Around line 24-26: Update the locale validation loop around
SettingsMailTemplates::all so it compares each locale’s sorted template keys
against the sorted keys from it_IT, rather than only comparing counts. Preserve
the existing per-locale check structure and ensure the reference key set is
derived from the it_IT templates.
---
Outside diff comments:
In `@app/Controllers/PrestitiController.php`:
- Around line 231-256: Move the ISO validation in the loan creation flow before
the default `data_scadenza` calculation, after applying only the `data_prestito`
default. Ensure invalid `data_prestito` values redirect to `invalid_date_format`
before `strtotime()` is called, while preserving the existing default
loan-duration calculation for valid ISO dates.
🪄 Autofix
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: 9874b0cc-dbdd-497d-8451-08aa6e6a561d
📒 Files selected for processing (73)
app/Controllers/CollaneController.phpapp/Controllers/LibriController.phpapp/Controllers/LoanApprovalController.phpapp/Controllers/PrestitiController.phpapp/Controllers/ReservationsController.phpapp/Controllers/SettingsController.phpapp/Controllers/UserActionsController.phpapp/Controllers/UserDashboardController.phpapp/Models/CopyRepository.phpapp/Models/DashboardStats.phpapp/Models/LoanRepository.phpapp/Models/SeriesRepository.phpapp/Models/SettingsRepository.phpapp/Routes/web.phpapp/Support/ConfigStore.phpapp/Support/DataIntegrity.phpapp/Support/IcsGenerator.phpapp/Support/NotificationService.phpapp/Support/PluginManager.phpapp/Support/SettingsMailTemplates.phpapp/Support/mail_templates/da_DK.phpapp/Support/mail_templates/de_DE.phpapp/Support/mail_templates/en_US.phpapp/Support/mail_templates/fr_FR.phpapp/Views/admin/integrity_report.phpapp/Views/admin/pending_loans.phpapp/Views/admin/stats.phpapp/Views/collane/dettaglio.phpapp/Views/collane/index.phpapp/Views/dashboard/index.phpapp/Views/frontend/book-detail.phpapp/Views/libri/scheda_libro.phpapp/Views/partials/loan-status-badge.phpapp/Views/prestiti/crea_prestito.phpapp/Views/prestiti/dettagli_prestito.phpapp/Views/prestiti/index.phpapp/Views/prestiti/modifica_prestito.phpapp/Views/profile/reservations.phpapp/Views/settings/loans-tab.phpapp/Views/user_dashboard/index.phpapp/Views/user_dashboard/prenotazioni.phpapp/Views/utenti/dettagli_utente.phpapp/helpers.phpfrontend/js/copy-scanner.jsinstaller/database/data_da_DK.sqlinstaller/database/data_de_DE.sqlinstaller/database/data_en_US.sqlinstaller/database/data_fr_FR.sqlinstaller/database/data_it_IT.sqlinstaller/database/migrations/migrate_0.7.59-rc.1.sqlinstaller/database/schema.sqllocale/da_DK.jsonlocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonpublic/assets/copy-scanner.bundle.jsstorage/plugins/mobile-api/plugin.jsonstorage/plugins/mobile-api/src/Controllers/ActionsController.phpstorage/plugins/mobile-api/src/Controllers/CatalogController.phpstorage/plugins/mobile-api/src/Controllers/OpenApiController.phptests/discussion-238-followup.unit.phptests/full-test.spec.jstests/issues-333-334-336.unit.phptests/label-pdf-content.spec.jstests/loan-auto-approval-301-reservation-path.unit.phptests/loan-bulk-extension-capacity.unit.phptests/loan-coherence-audit.unit.phptests/loan-reservation-consistency.unit.phptests/migration-0.7.59-rc.1.unit.phptests/plugin-orphan-hook-prune.unit.phptests/settings-orphans-hardening.unit.phpversion.json
fabiodalez-dev
commented
Aug 12, 2026
fabiodalez-dev
left a comment
fabiodalez-dev
left a comment
Owner
Author
There was a problem hiding this comment.
Verdict
needs changes
Primary Goal
Assemble a 0.7.59-rc.1 integration branch that bundles PRs #335/#337/#339/#340, corrects the migration versioning, and adds a PluginManager orphan-hook self-heal.
Overview
TL;DR
Solid, well-documented integration branch with genuinely good reasoning behind the conflict resolution and the migration rename — but three things should be settled before this becomes the RC tag: the rejectLoan() note-length guard, the RC-versioned migration's behaviour on the 0.7.58 → 0.7.59 (stable, no RC) upgrade path, and the fact that the PluginManager self-heal silently DELETEs rows.
What this PR does
This is an explicit integration branch for 0.7.59-rc.1, keeping main clean until the RC validates. It merges four independent PRs and adds two out-of-band fixes found while verifying the live install:
| Area | Source | Change |
|---|---|---|
| Series completeness | #340 | collane.is_completa column, capability check threaded through CollaneController::index()/show(), admin toggle |
| Loan/reservation coherence | #335 | DateHelper::today() prefill, timezone setting, soft-delete on rejection, #301 auto-approve on the reservation modal, availability fixes |
| Status helpers + issues #333/#334/#336 | #337 | Canonical status badge/label helpers, strict ISO date validator |
| Copy ordering + scanner focus | #339 | CopyRepository::sortByInventoryNumber() applied to the label PDF, loan-form scanner focus |
| Migration versioning | net-new | migrate_0.7.58.sql → migrate_0.7.59-rc.1.sql, version.json bump |
| Plugin hooks | net-new | PluginManager prunes orphan plugin_hooks rows |
What's good
- The conflict resolution is the right one. Merging #335's separate
invalid_date_format/invalid_dateserror codes with #337'sisStrictIsoDatevalidator gives you both precise error reporting and a real guard — instead of thestrtotime($a) <= strtotime($b)pattern where a parse failure returnsfalse, gets coerced to0, and silently passes the comparison. Validating first, then comparing lexicographically onY-m-d, is correct and cheap. - Exposing
loan_state/is_future_loanin theapproveLoan()payload removes date re-derivation fromautoApproveLoanRequest(). One authority for state, consumed rather than recomputed — exactly right. rejectLoan()going from hardDELETEto anannullatotransition is a clear win for auditability and for statistics that would otherwise silently lose rows.- The migration reasoning is sound. Shipping a migration named after an already-released version is a real version-skip bug, and catching it during RC verification is the whole point of an RC branch.
Concerns
1. rejectLoan() — the length guard doesn't bound what it claims to.
The 500-char truncation is applied to the user-supplied reason, but the value written is CONCAT(existing_note, "\n[Admin] …: ", reason). The cap therefore bounds the appended fragment, not the resulting column value. An already-long note still overflows in strict mode — the exact failure the guard is described as preventing. Either check the column type (TEXT makes this a non-issue and the comment should say so) or bound the result.
2. RC-named migration on the stable upgrade path.
The description says the RC name makes it run "on the RC upgrade AND on the eventual stable 0.7.59". That holds only if the updater's version comparison treats 0.7.59-rc.1 the way semver does (0.7.59-rc.1 < 0.7.59) and applies every migration strictly greater than the installed version. Two paths deserve an explicit test, not just the 0.7.58 → 0.7.59-rc.1 one that's already green:
0.7.58 → 0.7.59(stable, RC never installed) — must apply the migration.0.7.59-rc.1 → 0.7.59(stable, RC already installed) — must not re-apply, or must be idempotent (ADD COLUMN IF NOT EXISTS/ guarded).
Also confirm schema.sql carries is_completa so fresh installs don't depend on the migration at all.
3. The plugin-hook self-heal deletes rows.
Deleting on first sight is the most aggressive of the available repairs. A hook row that looks orphaned because a plugin file failed to load — a transient autoload failure, a partially-deployed upgrade, a permissions blip — is indistinguishable at that moment from a hook the plugin genuinely dropped, and the DELETE is unrecoverable without re-running onActivate(). Marking the row disabled, or requiring the plugin to have loaded successfully before pruning its hooks, gets the same "stop logging on every request" outcome without destroying state. Worth also confirming the "logs once" mechanism is safe under concurrent requests.
4. Transaction semantics in rejectLoan().
The new ReservationManager waitlist promotion runs under setExternalTransaction(true), after the rejection has already been persisted. Worth being explicit about what happens if promotion throws mid-transaction — does the caller roll back the rejection too, and is that the behaviour you want?
5. Committed build artifact.
public/assets/copy-scanner.bundle.js changes alongside frontend/js/copy-scanner.js. That's presumably the established pattern here, but for an RC it's worth confirming the bundle was rebuilt from the branch's final source rather than an intermediate state — a stale bundle is invisible to PHPStan and to the unit suite.
6. Behaviour tightening worth a release note.
Strict ISO validation is a tightening: any caller that previously posted a loosely-parseable date and got away with it now redirects with invalid_date_format. The admin UI is covered by the tests, but confirm the mobile-api plugin controllers and any import path always send Y-m-d.
Validation
The validation story is unusually complete for an integration branch — PHPStan level 5, 100+ unit tests, migration guard, i18n parity at 6784 keys, and a real admin-UI upgrade test. The gaps above are the ones that specifically fall outside what those checks cover.
Scope Assessment
Breadth is by design here — this is a declared integration branch and every area maps to a named upstream PR, so normal drift rules don't apply to the merged content. Two items are genuinely net-new to this branch rather than merged: the migration rename (defensible and arguably release-blocking — it's exactly the class of bug an RC exists to catch) and the PluginManager orphan-hook self-heal. The latter is behavioural, deletes database rows, and has nothing to do with any of the four bundled PRs; it would be easier to review, revert, and reason about on its own. Not blocking, but flagged.
Risk Assessment
Highest risk is the migration versioning itself, ironically — the fix is correct in intent but the stable 0.7.59 upgrade path (RC never installed) and the rc.1 → stable path are the ones that will hit real users and are not covered by the green 0.7.58 → 0.7.59-rc.1 test. If the updater's comparison doesn't treat the RC suffix as semver-lower, is_completa is silently missing on stable upgrades. Second, rejectLoan(): it converts a terminal delete into a state update plus waitlist promotion inside an externally-managed transaction, and its length guard bounds the appended fragment rather than the final note value. Third, the PluginManager DELETE is unrecoverable and fires on a condition (hook method not found) that can be produced by transient load failures, not only by genuine plugin drift. Lower risk: strict ISO date validation is a tightening that will now reject inputs previously accepted — verify the mobile-api plugin and any import path.
Reuse Notes
DateHelper::today()and the newisStrictIsoDatevalidator are the canonical date entry points now — any remainingstrtotime()-based date comparison in the loan/reservation path should route through them rather than re-implementing the check.- The canonical status badge/label helpers from #337 (
app/Views/partials/loan-status-badge.phpand the helper functions) should be the single source for loan status rendering; worth a sweep to confirm no view still hardcodes badge markup or status strings. CopyRepository::sortByInventoryNumber()now exists as the natural-ordering primitive — any other copy listing (admin lists, exports, the mobile-api catalog controller) that still sorts by raw string should reuse it rather than duplicating the comparator.ReservationManager::setExternalTransaction()is the established pattern for nesting reservation work inside a caller-owned transaction — keep new callers on it instead of opening their own.
Action Items
- Fix or re-document the
rejectLoan()length guard: bound the resultingnotevalue, or confirm the column isTEXTand adjust the comment so it doesn't claim protection it isn't providing. - Add migration tests for the two uncovered upgrade paths:
0.7.58 → 0.7.59(stable, RC never installed) and0.7.59-rc.1 → 0.7.59(already applied). Make the migration idempotent if it isn't already. - Confirm
installer/database/schema.sqlincludescollane.is_completaso fresh installs don't depend on the migration. - Soften the
PluginManagerself-heal: mark the orphan row disabled rather thanDELETEing it, or gate the prune on the plugin having loaded successfully — and verify the once-only logging is safe under concurrent requests. - Document the failure semantics of the waitlist promotion inside
rejectLoan()'s external transaction: does a throw roll back the rejection, and is that intended? - Verify
$supportsCompleteFlagis actually consumed by thecollane/indexview — otherwise it's a dead local. - Confirm
public/assets/copy-scanner.bundle.jswas rebuilt from the branch's finalfrontend/js/copy-scanner.js, not an intermediate state. - Add a release note for the strict ISO date tightening, and check the
mobile-apiplugin controllers always sendY-m-d.
Separate PR Suggestions
PluginManagerorphan-hook self-heal — this is a net-new behavioral change (it DELETEs rows fromplugin_hookson first sight of a dead hook) discovered during RC verification, not part of any of the four integrated PRs. As its own PR it would get a focused review of the deletion predicate and the once-only logging, and would be independently revertable if it turns out to delete a hook that was merely temporarily unregistered.- Unifying
PrestitiController::isStrictIsoDate()withDateHelper::isISODateFormat()— a small, mechanical consolidation that is easier to review on its own than inside a four-PR integration branch, and which would let the RC ship with the hybrid as-is. - Extract the repeated loan-status HOLDING predicate (
stato IN ('in_corso','in_ritardo','da_ritirare','prenotato'), plus thependentevariant) into a single shared constant or helper, and updateweb.php,NotificationService, andReservationManagerto consume it. This is a cross-cutting cleanup that would be hard to review safely inside an already-large RC integration branch. - Introduce (or route through) a proper auth accessor for role checks inside route closures in
app/Routes/web.php, replacing direct$_SESSION['user']['tipo_utente']reads. This touches many handlers and deserves its own reviewable diff. - Extract the
PluginManagerorphan-hook self-heal (pruneOrphanHook()+ the two call-site changes) into its own PR. It is not part of #335/#337/#339/#340, it introduces a destructive DB write on the request read path, and bundling it into a release-candidate integration branch means it ships without a review cycle of its own. It also deserves its own test: a plugin whose class fails to load should NOT lose its hook rows. - Il passaggio da
HtmlHelper::e()ahtmlspecialchars()inline inapp/Views/prestiti/modifica_prestito.php(e la rimozione del relativouse): se è una scelta deliberata di stile, va fatta in modo uniforme su tutte le viste in una PR dedicata, non su un singolo file dentro una release candidate. - Il cambio del default della scadenza da 14 a 30 giorni: è una modifica di comportamento funzionale, merita una PR propria con il razionale e, idealmente, l'aggancio all'impostazione di durata prestito già presente in
settings/loans-tab.php. - Self-hosting the
zxing-wasm.wasmassets instead of relying on the upstreamlocateFiledefault that fetches fromfastly.jsdelivr.net. This is a build/deploy concern unrelated to the RC integration and would be easier to validate on its own branch. - Extract the
$checkreporter and the fail-loud$src()source loader into a sharedtests/_bootstrap.phpand migrate the existingtests/*.unit.phpguard files onto it, so no future suite re-introduces the silent-pass-on-empty-file behaviour. - Estrarre un
tests/bootstrap-db.phpcondiviso contest_db_connect(): mysqli(parsing.env, fallbackE2E_DB_*→DB_*, gestione socket vs TCP,set_charset) e un helperwithSetting()per il pattern salva-imposta-ripristina dellesystem_settings. Il blocco di connessione è oggi duplicato quasi identico in più script ditests/, e ogni copia può divergere sul fallbackDB_PASS/DB_PASSWORD. Rifattorizzazione a rischio zero sul comportamento, ma tocca molti file: meglio fuori da una PR di release. - Estrarre il bootstrap DB condiviso (parsing
.env+ connessionemysqlicon fallbackE2E_DB_*) in un helper unico per i test, e migrarci i due nuovi test più eventuali altri già esistenti che duplicano lo stesso blocco. Piccolo, meccanico, e fuori dal percorso critico della RC. - Promuovere a test comportamentali i check di
loan-coherence-audit.unit.phpche lo consentono (#1 soft-delete → 404 sull'endpoint availability, #5 rispetto diloan_duration_daysnel fallback diLoanRepository::update(), #8 validazione ISO strict inPrestitiController::store()), lasciando come source-invariant solo quelli che lo sono per natura (#4 clock unico). L'infrastruttura DB per farlo esiste già, come dimostra il test del prune in questa stessa PR. PluginManagerorphan-hook self-heal — unrelated to any of the four bundled PRs, changes runtime behaviour, and deletes database rows. As its own PR it can be reviewed on its merits (delete vs. disable, transient-failure handling, concurrency) and reverted independently if the RC needs to ship without it.- If the
storage/plugins/mobile-api/*controller changes are not strictly required by #335/#337/#339/#340, split them out — plugin-side API changes have a separate compatibility surface (mobile clients) from the core app and benefit from their own review and release note.
Addresses the RC review: the orphan-hook self-heal now DISABLES the dead row (is_active = 0) instead of removing it. The "method missing" signal is also what a partially-deployed upgrade or a transient class-load glitch would produce, and an irreversible removal would be unrecoverable. Disabling keeps the row for audit and is restored by a legitimate re-activation, while still stopping the per-request "Method not found" log: both hook-load SELECTs now filter is_active = 1 (which also makes the existing setHooksActive() deactivate path actually take effect in the loader). Concurrency-safe: the WHERE is_active = 1 clause is re-evaluated under the row lock, so at most one request logs the flip. Test renamed to plugin-orphan-hook-selfheal.unit.php (9 checks): the orphan is disabled not removed, the loader would no longer load it, the flip is idempotent, and a legitimate sibling / non-matching method / different plugin_id are never touched. Also hardens the series migration test (19 checks) with the updater's migration-selection contract for every upgrade path: migrate_0.7.59-rc.1 fires on 0.7.58 -> 0.7.59-rc.1 AND on 0.7.58 -> 0.7.59 stable (RC never installed), and does NOT re-fire on 0.7.59-rc.1 -> 0.7.59 (already applied; idempotent backstop). Reviewed and confirmed already-correct (no change needed): schema ships is_completa for fresh installs; the rejection-note fragment is bounded and its column is TEXT; the mobile-api reservation endpoint already validates Y-m-d via DateHelper::isISODateFormat; and copy-scanner.bundle.js rebuilds byte-identical from its source with the wasm assets self-hosted (no CDN fetch).
…nullato transition #335 changed loan rejection from a DELETE to a state transition (stato='annullato', attivo=0) — the row is kept for audit and to let the user re-request — but three E2E specs still asserted the old "rejection deletes the row" behavior. They slipped through because the RC upgrade regression runs full-test.spec.js only, not these. - loan-reservation.spec.js 5.2: assert the row survives as 'annullato' (attivo=0) instead of count===0. - swal-loans-reservations.spec.js: the index-widget and details-page reject tests poll for stato='annullato' instead of the row disappearing; titles updated. - loan-overlap.spec.js F.34: reject via UPDATE stato='annullato' (not a raw DELETE) and assert the copy is freed — this now verifies the capacity sweep treats a cancelled loan as not holding its copy, which is the real post-#335 behavior. All green: loan-reservation 21, swal-loans-reservations 14, loan-overlap 39, loan-state-model 4, plus full-test 137 and the plugin-activation preflight.
fabiodalez-dev
changed the title
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.
Integration branch for the 0.7.59-rc.1 release candidate. Bundles four open PRs on a dedicated branch (main untouched until the RC validates) and fixes the migration versioning + a plugin-hook self-heal found while verifying the live install.
What's integrated
collane.is_completacolumn + admin toggle.Migration versioning fix
Only #340 ships a migration. It was named
migrate_0.7.58.sql, but 0.7.58 is already released, so the updater would version-skip it for installs already on 0.7.58. Renamed tomigrate_0.7.59-rc.1.sql(RC-named so it runs on the RC upgrade AND on the eventual stable 0.7.59);version.jsonbumped to 0.7.59-rc.1. Audited the other three PRs: none needs a migration (pre-existing columns/ENUM, settings with safe defaults + self-heal).Conflict resolution
PrestitiController::update()date validation overlapped between #335 (separate error codes, F028) and #337 (strict-ISO validator). Resolved as a hybrid that satisfies both test suites:isStrictIsoDatevalidator + separateinvalid_date_format/invalid_datescodes. Locale files reconciled to 6784-key parity.Extra fix
PluginManagernow self-heals orphan hook rows (a hook a plugin registered in an old version and dropped in a new one lingered inplugin_hooksand logged "Method not found" on every request, because upgrades don't re-runonActivate()). Deletes the dead row on first sight; logs once.Validation
PHPStan level 5 clean, 100+ unit tests, migration guard, i18n parity, and the full
reinstall-test(Test A fresh install + Test B real admin-UI upgrade 0.7.58 → 0.7.59-rc.1) green. RC published as a prerelease and verified on the live install.Summary by CodeRabbit
Nuove funzionalità
Miglioramenti
Correzioni