fix: catalogue "on loan" count includes books with no copies - #303
Merged
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: 59 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 (23)
📝 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
The loan_pickup_ready / loan_pickup_expired notifications only exposed the
pickup deadline under {{scadenza_ritiro}}. A customised template using
{{pickup_deadline}} — the natural name a user copies from the DB column —
left the placeholder unresolved. I now pass the value under both names and
document {{pickup_deadline}} in each template's placeholder list.
…eholders
The admin email-template editor listed each {{token}} as a bare technical name
with no explanation, and nothing translated per locale. I added
SettingsMailTemplates::placeholderDescriptions() — a localised, human-readable
description for all 41 placeholder tokens — and the settings view now shows it
as a tooltip on each placeholder chip. The tokens stay untranslated (they are
substitution keys); only the descriptions are localised, across it/en/de/da/fr.
…ning
Four review findings on this branch:
1. The language *edit* page rendered stored (stale) total_keys/translated_keys
while the *list* page rendered source-derived stats — the same locale showed
two different denominators (en_US 4653 vs 6654). Added
Language::getByCodeWithDerivedStats() (shared helper with
getAllWithDerivedStats) and routed edit() through it.
2. Dropped the hardcoded "6613" from a controller comment (already stale at 6654);
the denominator is derived at runtime and must not be cited as a literal.
3. Catalogue card badge: "In prestito" now shows ONLY for stato='prestato'; every
other not-available case (non_disponibile, or a stale stato on a zero-copy
book) falls to "Non disponibile" instead of being mislabelled as on loan.
4. Language::translatedKeyCount validates the locale code shape
(^[a-z]{2}_[A-Z]{2}$) before interpolating it into a filesystem path —
defence-in-depth against traversal, since the code comes from the DB.
Verified: PHPStan clean, language-derived-stats 17/17, catalog-reserved-category
13/13 (badges unchanged for the asserted cases).
…t gaps Finding 5 from the review — the derived-stats helpers hardcoded the locale/ directory via __DIR__, which forced the unit test to write fixtures into the real repository tree (orphaned on a hard kill) and left the malformed-JSON branch untested. Root-cause fix: - Language::__construct now takes an optional $localeDir (defaults to the shipped locale/); the source-key cache moved from a static to a per-instance property so an injected dir isn't shadowed by a prior instance's cache. - Dropped the redundant min($translated, $sourceTotal) cap: array_intersect_key against the source key set already bounds the count to <= source, so the cap was unreachable dead code — documented the invariant instead. - The unit test now points a dedicated Language instance at a throwaway dir under sys_get_temp_dir(): fixtures never touch repo locale/, and it adds the previously-missing malformed-JSON case. 17 → 20 assertions. Backward compatible (the second constructor arg is optional; no other caller passes it). PHPStan clean; migration/danish/export i18n tests still green.
Owner
Author
|
Ran a multi-lens self-review of this branch before cutting a release. Everything surfaced was P2 (no P0/P1); all fixed in a33f2b1 + 3bfeede:
Full-test suite and PHPStan green. Reviewed alongside #302 (both touch the catalogue views) — the two are region-disjoint, no semantic conflict. |
fabiodalez-dev
added a commit
that referenced
this pull request
Jul 29, 2026
Release candidate bundling the reviewed catalogue + scraper + i18n work: - #303 catalogue availability facets, source-derived language stats, Open Library 1.0.2 native Goodreads cover scraper, #304 email fixes, review fixes. - #298 catalogue card row alignment (+ font-swap/thrashing/mobile review fixes). - UI wording: "series number" → "number in series" (README + en_US); the CSV import header mapping is intentionally left unchanged for back-compatibility. No database migration — availability is derived from circulation state and language stats from the shipped locale files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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