Skip to content

fix(loans): issues #333, #334, #336 + canonical status badge/label helpers - #337

Merged
fabiodalez-dev merged 10 commits into
mainfrom
claude/fix-issues-333-334-336-vjae53
Aug 13, 2026
Merged

fix(loans): issues #333, #334, #336 + canonical status badge/label helpers#337
fabiodalez-dev merged 10 commits into
mainfrom
claude/fix-issues-333-334-336-vjae53

Conversation

@fabiodalez-dev

@fabiodalez-dev fabiodalez-dev commented Aug 11, 2026

Copy link
Copy Markdown
Owner

Fixes the three loan-related issues and follows up with the centralization that prevents them from recurring.

Issue fixes

#333 — cancelled loan shown as "Unknown"
User cancellation sets stato='annullato', a valid enum value no admin view handled, so it fell through to "Sconosciuto/Unknown" and looked stuck. All surfaces now render it (loans list SSR + DataTables, loan details, user details, book-page history), with an Annullato filter button and CSV-export state. A loan closed without a return (annullato/scaduto) no longer claims "not yet returned".

#334 — notifications dropdown covered in /admin
The loans-overview and integrity-report page headers were sticky top-0 z-30 — same z-index as the layout header but later in the DOM, so they painted over the app header and its notifications dropdown while scrolling. Sticky removed.

#336 — loan editing bounced with no_copies_available
update() re-checked capacity over the whole new window, so any commitment already coexisting with the current period (e.g. a queued reservation) blocked every date edit — even shortening. It now checks only the newly-claimed date segments, the same extension-window convention renew() uses; bulk extension aligned too. Error messaging is now explicit: /admin/loans explains capacity conflicts, and the admin book page recognizes the extension_conflicts/renewal_failed keys renew() actually emits (it only knew the never-emitted renewal_conflict).

Centralization follow-up

The root cause of #333 was five per-view copies of the stato→badge switch. Now:

  • app/Views/partials/loan-status-badge.php — single source for color/icon/label of every prestiti.stato value; all admin views consume it (DataTables gets a PHP-generated JS map). Lives under app/Views because Tailwind's content globs only scan there.
  • loan_status_label_map() + existing translate_loan_status() in helpers.php feed every text-label consumer: stats chart, dashboard calendar, ICS feed, book-page occupancy calendar, user history views, loans-overview chips, PDF and CSV export.
  • This removed a real ambiguity: calendar/ICS translated in_ritardo as "Scaduto", which now collides with the real scaduto state; everything says "In Ritardo".
  • Deliberate exceptions are documented in place (copy-row phrasing, ICS event titles, return-form outcome select).

User-facing history

Cancelled (annullato) and pickup-expired (scaduto) loans now appear in the user's loan history (profile + account dashboard + counter) instead of vanishing, ordered by closing time, with a dedicated icon and no review button (the book never went out).

Mobile API (plugin 1.4.3)

/me/loans history includes the same closed states (additive — status is documented as the raw enum value), and every loan payload gains a server-localized status_label from the canonical helper, documented in the OpenAPI schema. Consumed by the companion PR fabiodalez-dev/Pinakes-Android#30 (independent — both sides tolerate the other being older).

i18n & tests

The two new user-facing strings are translated in all five bundled locales; every other label reuses already-translated keys (verified against all locale files). New regression-guard suite tests/issues-333-334-336.unit.php: 48 static checks, all green, including probes that fail if a local stato→label map reappears. php -l clean on every touched file.

Closes #333, closes #334, closes #336.

🤖 Generated with Claude Code

https://claude.ai/code/session_014KsK1ikTUGbUr5W3xrSJ6w

Summary by CodeRabbit

  • Nuove funzionalità

    • Lo storico dei prestiti include anche quelli annullati e scaduti, con ordinamento migliorato.
    • Aggiunti filtro ed esportazione CSV per i prestiti annullati.
    • Le app mobile ricevono etichette di stato localizzate e la data della richiesta.
    • Uniformata la visualizzazione di badge, stati e traduzioni.
  • Correzioni

    • Migliorati i controlli di disponibilità durante modifiche e rinnovi.
    • Corretta la visualizzazione delle date di restituzione.
    • Disabilitate le recensioni per prestiti annullati o scaduti.
    • Risolte sovrapposizioni degli header durante lo scorrimento.

claude added 4 commits August 11, 2026 10:22
#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
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@fabiodalez-dev, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1a1efb06-9cba-45b4-8a10-ade23e36721a

📥 Commits

Reviewing files that changed from the base of the PR and between 8cb7901 and 7b680b0.

📒 Files selected for processing (2)
  • locale/fr_FR.json
  • tests/issues-333-334-336.unit.php
📝 Walkthrough

Walkthrough

Il PR restringe la validazione delle date e dei periodi aggiunti. Centralizza badge ed etichette degli stati. Include prestiti annullati e scaduti nelle cronologie e nell’API mobile. Aggiorna errori, localizzazioni, viste e test di regressione.

Changes

Ciclo di vita dei prestiti

Layer / File(s) Summary
Finestre di capacità
app/Controllers/PrestitiController.php, tests/loan-bulk-extension-capacity.unit.php, locale/*
update() valida date ISO rigorose, usa dati riletti sotto lock e controlla solo i periodi aggiunti. L’estensione bulk verifica solo la nuova finestra.
Stati canonici e badge
app/helpers.php, app/Views/partials/*, app/Views/prestiti/*, app/Views/libri/*, app/Views/utenti/*, app/Views/admin/*, app/Views/dashboard/*, app/Views/profile/*, app/Views/user_dashboard/*, app/Support/*
Le viste usano helper condivisi per etichette, badge, icone e stati del calendario. I prestiti annullati e scaduti non consentono nuove recensioni.
Cronologia e API dei prestiti
app/Controllers/UserActionsController.php, app/Controllers/UserDashboardController.php, storage/plugins/mobile-api/..., tests/loan-reservation-consistency.unit.php
Le cronologie includono annullato e scaduto. L’ordinamento usa data_restituzione o updated_at. L’API espone status_label e requested_at.
Errori e intestazioni
app/Views/prestiti/index.php, app/Views/admin/*, locale/*
Le viste gestiscono i nuovi codici di errore. Gli header non sono più sticky. I cataloghi contengono i nuovi messaggi.
Test di regressione
tests/issues-333-334-336.unit.php
I test verificano stati, badge, cronologie, API, localizzazioni, intestazioni e finestre di capacità.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Utente
  participant PrestitiController
  participant CapacityChecks
  participant MobileAPI
  Utente->>PrestitiController: aggiorna o estende il prestito
  PrestitiController->>CapacityChecks: verifica i giorni aggiunti
  CapacityChecks-->>PrestitiController: disponibilità o conflitto
  Utente->>MobileAPI: richiede la cronologia
  MobileAPI-->>Utente: status_label e requested_at
Loading

Possibly related PRs

Suggested reviewers: fabiodalez

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Il titolo identifica correttamente la correzione degli issue principali e la centralizzazione degli helper per stati e badge dei prestiti.
Docstring Coverage ✅ Passed Docstring coverage is 66.67% which is sufficient. The required threshold is 60.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/fix-issues-333-334-336-vjae53

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 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 861-889: After acquiring the loan row lock, refresh data_prestito
and data_scadenza from the locking query instead of using the stale $current
values. Rebuild $newPrestito and $newScadenza from the locked record, then rerun
the date-range validation before the claimedWindows capacity checks in the
update flow.

In `@app/Controllers/UserActionsController.php`:
- Line 84: Preserve the full closure timestamp in the historical-loan ordering
by replacing the DATE() fallback with the raw updated_at value in the ORDER BY
expression at app/Controllers/UserActionsController.php#L84-L84,
app/Controllers/UserDashboardController.php#L195-L195, and
storage/plugins/mobile-api/src/Controllers/ActionsController.php#L112-L112. Keep
data_restituzione as the preferred timestamp and retain the existing descending
ordering.

In `@app/Views/prestiti/index.php`:
- Around line 89-111: Gestisci il codice renewal_failed nello switch dei
messaggi di errore della vista, aggiungendo il messaggio localizzato “Rinnovo
non riuscito. Riprova.” prima del caso generico di aggiornamento prestito.

In `@app/Views/user_dashboard/prenotazioni.php`:
- Around line 817-822: Nel pulsante di recensione, sostituisci `HtmlHelper::e()`
usato per l'attributo `data-book-title` con `htmlspecialchars(..., ENT_QUOTES,
'UTF-8')`, mantenendo il fallback su `$loan['titolo'] ?? ''` e il comportamento
di escaping HTML.

In `@tests/issues-333-334-336.unit.php`:
- Around line 29-41: Strengthen tests/issues-333-334-336.unit.php by adding a
file-reading helper that validates is_file() and exits nonzero when reads fail,
then use it for the source loads at lines 29-41, 111-113, 153-155, and 227. In
the source-section checks around lines 187-205, require $closeStart >
$updateStart and $renewStart > $bulkStart, and assert $updateSource and
$bulkSource are non-empty before dependent negative checks.
🪄 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: d4cbcd09-2d80-4a05-b32d-f694eb2b5682

📥 Commits

Reviewing files that changed from the base of the PR and between 69301d0 and b4f4fd8.

📒 Files selected for processing (25)
  • app/Controllers/PrestitiController.php
  • app/Controllers/UserActionsController.php
  • app/Controllers/UserDashboardController.php
  • app/Support/IcsGenerator.php
  • app/Views/admin/integrity_report.php
  • app/Views/admin/pending_loans.php
  • app/Views/admin/stats.php
  • app/Views/dashboard/index.php
  • app/Views/libri/scheda_libro.php
  • app/Views/partials/loan-status-badge.php
  • app/Views/prestiti/dettagli_prestito.php
  • app/Views/prestiti/index.php
  • app/Views/profile/reservations.php
  • app/Views/user_dashboard/prenotazioni.php
  • app/Views/utenti/dettagli_utente.php
  • app/helpers.php
  • locale/da_DK.json
  • locale/de_DE.json
  • locale/en_US.json
  • locale/fr_FR.json
  • locale/it_IT.json
  • storage/plugins/mobile-api/plugin.json
  • storage/plugins/mobile-api/src/Controllers/ActionsController.php
  • storage/plugins/mobile-api/src/Controllers/OpenApiController.php
  • tests/issues-333-334-336.unit.php

Comment thread app/Controllers/PrestitiController.php
Comment thread app/Controllers/UserActionsController.php Outdated
Comment thread app/Views/prestiti/index.php
Comment thread app/Views/user_dashboard/prenotazioni.php
Comment thread tests/issues-333-334-336.unit.php Outdated
- 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

Copy link
Copy Markdown
Owner Author

@coderabbitai review


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

@fabiodalez-dev Avvio la revisione della PR #337. Mi concentrerò su bug reali, problemi di sicurezza e violazioni delle regole del progetto.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/Controllers/PrestitiController.php (1)

878-907: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Allinea tutti i controlli alla sola porzione di periodo aggiunta.

I due percorsi includono ancora giorni già coperti dalla finestra precedente oppure usano intervalli diversi per capacità e copia. Questo può rifiutare aggiornamenti validi con un falso conflitto.

  • app/Controllers/PrestitiController.php#L878-L907: calcola la differenza esatta tra le finestre, senza includere gli estremi già presenti.
  • app/Controllers/PrestitiController.php#L1616-L1620: definisci un unico $extensionStart e usalo in hasFreeCapacity() e nel controllo $copyOverlap.
🤖 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 878 - 907, Allinea i
controlli alle sole porzioni realmente aggiunte: in
app/Controllers/PrestitiController.php:878-907 aggiorna il calcolo di
$claimedWindows per escludere gli estremi già inclusi nella vecchia finestra; in
app/Controllers/PrestitiController.php:1616-1620 definisci un unico
$extensionStart e riutilizzalo sia in hasFreeCapacity() sia nel controllo
$copyOverlap, mantenendo identico l’intervallo verificato da entrambi.
🤖 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 795-807: Normalize and strictly validate the effective dates in
the update flow before any comparisons: require exact Y-m-d input, reject
invalid calendar dates such as 2026-02-30, and derive canonical values once. Use
those normalized dates for the range checks, CapacityService calls, the logic
around the later notification comparison, and LoanRepository::update instead of
the original strings.

In `@storage/plugins/mobile-api/src/Controllers/OpenApiController.php`:
- Line 1265: Update the OpenAPI schema entry for requested_at in the loan
mapping definition to use a type allowing both string and null, and remove the
nullable flag; keep its date format and description unchanged.

---

Outside diff comments:
In `@app/Controllers/PrestitiController.php`:
- Around line 878-907: Allinea i controlli alle sole porzioni realmente
aggiunte: in app/Controllers/PrestitiController.php:878-907 aggiorna il calcolo
di $claimedWindows per escludere gli estremi già inclusi nella vecchia finestra;
in app/Controllers/PrestitiController.php:1616-1620 definisci un unico
$extensionStart e riutilizzalo sia in hasFreeCapacity() sia nel controllo
$copyOverlap, mantenendo identico l’intervallo verificato da entrambi.
🪄 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: 79707f86-2c73-42b0-962b-f17e7c85dd14

📥 Commits

Reviewing files that changed from the base of the PR and between b4f4fd8 and fc5cb4a.

📒 Files selected for processing (8)
  • app/Controllers/PrestitiController.php
  • app/Controllers/UserActionsController.php
  • app/Controllers/UserDashboardController.php
  • app/Views/prestiti/index.php
  • app/Views/user_dashboard/prenotazioni.php
  • storage/plugins/mobile-api/src/Controllers/ActionsController.php
  • storage/plugins/mobile-api/src/Controllers/OpenApiController.php
  • tests/issues-333-334-336.unit.php

Comment thread app/Controllers/PrestitiController.php
Comment thread storage/plugins/mobile-api/src/Controllers/OpenApiController.php Outdated
claude added 2 commits August 11, 2026 13:41
- 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

Copy link
Copy Markdown
Owner Author

@coderabbitai review


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

@fabiodalez-dev Avvio la revisione della PR #337. Verificherò bug reali, vulnerabilità di sicurezza e violazioni delle regole del progetto.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 762-763: Update isStrictIsoDate() to reject any non-ASCII date
input, including NUL bytes, before calling DateTime::createFromFormat(). Ensure
invalid values return false so both date-validation paths produce invalid_dates
instead of propagating ValueError.

In `@tests/issues-333-334-336.unit.php`:
- Around line 185-188: Rendi strutturali entrambe le regressioni in
tests/issues-333-334-336.unit.php: nelle righe 185-188 estrai il blocco dello
schema requested_at e verifica sia la forma OpenAPI 3.1 con type string/null sia
l’assenza di nullable; nelle righe 225-233 verifica che i risultati di
$dayBefore($oldPrestito) e $dayAfter($oldScadenza) siano effettivamente usati
nel calcolo di $claimedWindows, non solo presenti nel testo.
🪄 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: d199a5ef-e755-490d-933b-28daf3c6719f

📥 Commits

Reviewing files that changed from the base of the PR and between fc5cb4a and 78ac15b.

📒 Files selected for processing (4)
  • app/Controllers/PrestitiController.php
  • storage/plugins/mobile-api/src/Controllers/OpenApiController.php
  • tests/issues-333-334-336.unit.php
  • tests/loan-reservation-consistency.unit.php

Comment thread app/Controllers/PrestitiController.php
Comment thread tests/issues-333-334-336.unit.php
claude and others added 2 commits August 11, 2026 15:41
… 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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/issues-333-334-336.unit.php (1)

121-124: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Verifica la condizione che disabilita la recensione.

Le linee 121-124 verificano solo la presenza di $canReview. Il test passa anche se una view consente ancora recensioni per prestiti annullato o scaduto.

Verifica in entrambe le view che $canReview escluda esplicitamente questi due stati.

Correzione proposta
 $check(
-    str_contains($profileReservations, '$canReview') && str_contains($userDashboard, '$canReview'),
+    str_contains($profileReservations, "\$canReview = !in_array(\$loan['stato'], ['annullato', 'scaduto'], true)")
+        && str_contains($userDashboard, "\$canReview = !in_array(\$loan['stato'], ['annullato', 'scaduto'], true)"),
     'history hides the review button for loans that never went out (annullato/scaduto)'
 );
🤖 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/issues-333-334-336.unit.php` around lines 121 - 124, Strengthen the
assertion in the test around $profileReservations and $userDashboard so both
views verify that $canReview explicitly excludes the annullato and scaduto loan
states, rather than only checking that the variable exists. Keep the existing
check for both views and ensure the assertion fails if either state can still
permit reviews.
🤖 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/fr_FR.json`:
- Line 6770: Correggi la traduzione francese del messaggio “Modifica non
salvata...” sostituendo “réservé par un autre prêt” con una formulazione che
indichi chiaramente che l’esemplare è già assegnato a un altro prestito, senza
implicare che il prestito lo abbia riservato.

---

Outside diff comments:
In `@tests/issues-333-334-336.unit.php`:
- Around line 121-124: Strengthen the assertion in the test around
$profileReservations and $userDashboard so both views verify that $canReview
explicitly excludes the annullato and scaduto loan states, rather than only
checking that the variable exists. Keep the existing check for both views and
ensure the assertion fails if either state can still permit reviews.
🪄 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: bbcc8b1c-5776-4183-95fe-80cf445155b2

📥 Commits

Reviewing files that changed from the base of the PR and between 78ac15b and 8cb7901.

📒 Files selected for processing (9)
  • app/Controllers/PrestitiController.php
  • app/Views/prestiti/index.php
  • locale/da_DK.json
  • locale/de_DE.json
  • locale/en_US.json
  • locale/fr_FR.json
  • locale/it_IT.json
  • tests/issues-333-334-336.unit.php
  • tests/loan-bulk-extension-capacity.unit.php

Comment thread locale/fr_FR.json Outdated
…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
@fabiodalez-dev
fabiodalez-dev merged commit c5ebe62 into main Aug 13, 2026
6 checks passed
fabiodalez-dev added a commit that referenced this pull request Aug 13, 2026
Stable 0.7.59. Integrates the four feature/fix PRs plus the review hardening and security pass. Full required-check matrix green.
@fabiodalez-dev
fabiodalez-dev deleted the claude/fix-issues-333-334-336-vjae53 branch August 13, 2026 10:57
Himura2la pushed a commit to hackerembassy/Pinakes that referenced this pull request Aug 13, 2026
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 (fabiodalez-dev#335 settings, fabiodalez-dev#337 badge helpers,
fabiodalez-dev#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants