fix: catalogue "on loan" count includes books with no copies - #303
fix: catalogue "on loan" count includes books with no copies#303fabiodalez-dev wants to merge 4 commits into
Conversation
The availability filter derived "On loan" from copie_disponibili <= 0. A book with copie_totali = 0 (a record with no copies registered) also has copie_disponibili = 0, so it was counted — and filtered — as "on loan" even though nothing is borrowed. On one library this showed 7 "on loan" when only 2 books were actually lent (the other 5 had zero copies). - "On loan" (count + filter condition) now requires copie_totali > 0: every copy is out, as opposed to there being no copies. - "All books" is now a real COUNT(DISTINCT l.id) instead of available + borrowed — a no-copies book is neither available nor on loan, so the sum under-counted the catalogue by exactly those records. tests/catalog-borrowed-count-empty-copies.spec.js seeds one available, one truly on-loan and two no-copies books and checks the badges by delta: "On loan" rises by 1 (not 3), "All books" by 4.
|
Warning Review limit reached
Next review available in: 47 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 (5)
📝 WalkthroughWalkthroughIl catalogo separa prenotati, prestati e libri non disponibili nei filtri, nei conteggi e nei badge. Il modello delle lingue ricalcola le statistiche dai file JSON usando la locale italiana come denominatore comune, con test dedicati. ChangesDisponibilità e stati del catalogo
Statistiche derivate delle lingue
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/FrontendController.php`:
- Around line 1185-1200: Update the availability badge text in the catalog view
to clarify that its total includes all catalogue records, including books
without copies, rather than describing it only as “Disponibili e in prestito.”
Keep the availability_stats calculation in the FrontendController unchanged.
In `@tests/catalog-borrowed-count-empty-copies.spec.js`:
- Around line 8-9: Correggi il commento descrittivo in
tests/catalog-borrowed-count-empty-copies.spec.js sostituendo l’incremento di
“All” da 3 a 4, mantenendo invariati il test e l’aspettativa esistente.
🪄 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: 5640b6af-0a72-4e96-9358-8c867efde398
📒 Files selected for processing (2)
app/Controllers/FrontendController.phptests/catalog-borrowed-count-empty-copies.spec.js
…4, not 3) CodeRabbit: four records are seeded (available + on-loan + two no-copies), so "All books" rises by 4 — the assertion was already correct, only the comment said 3.
…from source The availability filter derived "on loan" from copie_disponibili <= 0, which also matched reserved books (held by a pending request or slot reservation) and records with no copies. The filter and counts now mirror the recomputed libri.stato so the filter, the card badge and the book page agree by construction: - Available → copie_disponibili > 0 - Reserved → l.stato = 'prenotato' (new facet + count + card badge) - On loan → l.stato = 'prestato' (a copy actually checked out) The catalogue card gains a distinct "Not available" badge for books whose copies are all out of circulation (l.stato = 'non_disponibile'), instead of mislabelling them "on loan". i18n completion stats are now derived live from the Italian source (Language::getAllWithDerivedStats): every __() key IS the Italian string, so the source key count is the canonical denominator for every locale. total_keys can no longer drift and needs no migration when keys are added — the historical da_DK migration tests now assert their shipped literal instead of the live file count. Also clarifies the "All books" facet description (was "Available and on loan"). Tests: 12 E2E (catalog-reserved-category) + 17 unit (language-derived-stats).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/FrontendController.php (1)
1166-1217: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winQuattro query COUNT separate sulla stessa base — considera un'unica query con aggregazione condizionale.
queryAvailable,queryBorrowed,queryReservedequeryTotaleseguono 4 scan indipendenti (5 LEFT JOIN ciascuno) sulla stessa$availabilityBaseQuery, ad ogni caricamento della pagina catalogo/filtri. Puoi ottenere gli stessi 4 valori con un'unica query usandoSUM(CASE WHEN ... THEN 1 ELSE 0 END)eCOUNT(DISTINCT l.id), riducendo i round-trip DB da 4 a 1 su un percorso ad alto traffico.♻️ Esempio di consolidamento
- $queryAvailable = "SELECT COUNT(DISTINCT l.id) as cnt " . $availabilityBaseQuery . " AND l.copie_disponibili > 0"; - $stmt = $db->prepare($queryAvailable); - ... - $queryBorrowed = "SELECT COUNT(DISTINCT l.id) as cnt " . $availabilityBaseQuery . " AND l.stato = 'prestato'"; - ... - $queryReserved = "SELECT COUNT(DISTINCT l.id) as cnt " . $availabilityBaseQuery . " AND l.stato = 'prenotato'"; - ... - $queryTotal = "SELECT COUNT(DISTINCT l.id) as cnt " . $availabilityBaseQuery; - ... + $queryStats = "SELECT + COUNT(DISTINCT l.id) AS total_cnt, + COUNT(DISTINCT CASE WHEN l.copie_disponibili > 0 THEN l.id END) AS available_cnt, + COUNT(DISTINCT CASE WHEN l.stato = 'prestato' THEN l.id END) AS borrowed_cnt, + COUNT(DISTINCT CASE WHEN l.stato = 'prenotato' THEN l.id END) AS reserved_cnt + " . $availabilityBaseQuery; + $stmt = $db->prepare($queryStats); + if (!empty($paramsAvail)) { + $stmt->bind_param($typesAvail, ...$paramsAvail); + } + $stmt->execute(); + $row = $stmt->get_result()->fetch_assoc(); + $availableCount = $row['available_cnt'] ?? 0; + $borrowedCount = $row['borrowed_cnt'] ?? 0; + $reservedCount = $row['reserved_cnt'] ?? 0; + $totalCount = $row['total_cnt'] ?? 0;🤖 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/FrontendController.php` around lines 1166 - 1217, Consolida i conteggi attualmente eseguiti da $queryAvailable, $queryBorrowed, $queryReserved e $queryTotal in un’unica query aggregata basata su $availabilityBaseQuery, usando COUNT(DISTINCT l.id) e aggregazioni condizionali per available, reserved e borrowed. Mantieni invariati i filtri, i parametri bindati e i valori restituiti in $options['availability_stats'], sostituendo i quattro prepare/execute con un solo round-trip.
🤖 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/Models/Language.php`:
- Around line 73-74: In app/Models/Language.php lines 73-74, update the
translated-count logic in the relevant Language method to filter decoded locale
entries to canonical source keys before counting non-empty strings, while
preserving the sourceTotal cap. In tests/language-derived-stats.unit.php lines
120-124, expect zero when only orphan keys exist and add a separate assertion
covering translated canonical keys.
In `@tests/language-derived-stats.unit.php`:
- Around line 71-73: Prepare the required language data before calling
getAllWithDerivedStats(): create an isolated fixture containing at least five
locales, including it_IT, and ensure total_keys is linked to Italian for each
locale. Clean up the fixture or roll back the transaction before asserting the
returned statistics.
- Around line 120-124: Update the test around $big so orphan-only keys are
expected to produce a translated count of 0 rather than $srcCount. Add a
separate fixture using canonical keys from it_IT.json with translated values,
and assert that those keys are counted while preserving the source-total cap
behavior.
---
Outside diff comments:
In `@app/Controllers/FrontendController.php`:
- Around line 1166-1217: Consolida i conteggi attualmente eseguiti da
$queryAvailable, $queryBorrowed, $queryReserved e $queryTotal in un’unica query
aggregata basata su $availabilityBaseQuery, usando COUNT(DISTINCT l.id) e
aggregazioni condizionali per available, reserved e borrowed. Mantieni invariati
i filtri, i parametri bindati e i valori restituiti in
$options['availability_stats'], sostituendo i quattro prepare/execute con un
solo round-trip.
🪄 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: 772765a6-2329-4fec-9876-230d75a27571
📒 Files selected for processing (15)
app/Controllers/Admin/LanguagesController.phpapp/Controllers/FrontendController.phpapp/Models/Language.phpapp/Views/frontend/catalog-grid.phpapp/Views/frontend/catalog.phplocale/da_DK.jsonlocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsontests/catalog-borrowed-count-empty-copies.spec.jstests/catalog-reserved-category.spec.jstests/language-derived-stats.unit.phptests/migration-0.7.41.unit.phptests/migration-0.7.42.unit.php
1 participant
Problem
On the catalogue availability filter, "On loan" showed 7 on one library while only 2 books were actually lent. The filter derives "On loan" from
copie_disponibili <= 0— but a book withcopie_totali = 0(a record with no copies registered) also hascopie_disponibili = 0, so those empty records were counted and filtered as "on loan" despite nothing being borrowed. In that catalogue 5 of the 7 had zero copies.Fix
copie_disponibili <= 0 AND copie_totali > 0— every copy is out, as opposed to there being no copies.COUNT(DISTINCT l.id)instead ofavailable + borrowed. A no-copies book is neither available nor on loan, so the sum silently under-counted the catalogue by exactly those records; counting directly keeps the "All" badge correct."Available" (
copie_disponibili > 0) is unchanged. Empty-record books now appear only under "All books", not under either sub-filter.Test
tests/catalog-borrowed-count-empty-copies.spec.jsseeds one available, one truly on-loan (copie_totali=1, copie_disponibili=0) and two no-copies (copie_totali=0) books, then checks the filter badges by delta: "On loan" rises by 1 (not 3) and "All books" by 4.Full-tree PHPStan level 5 clean.
Summary by CodeRabbit
Nuove funzionalità
Correzioni
Test