Book club (#138): reconcile external proposals on acquire + 'never chosen' archive#280
Book club (#138): reconcile external proposals on acquire + 'never chosen' archive#280fabiodalez-dev wants to merge 10 commits into
Conversation
…chosen archive Three items from Uwe's round-3 feedback on discussion #138: 1. BUG — "Add to catalogue" returned "Acquisition failed, please try again." for an external proposal the manager had already bought and added to the catalogue manually. acquireExternalBook() always INSERTed a fresh libri row; with the same ISBN that hit libri's UNIQUE isbn13/isbn10 key, threw, rolled back the whole transaction, and left the proposal stuck as external. Now it reconciles: if a non-deleted catalogue book already has the same isbn13/ isbn10, it links the club entry to that row (no duplicate) — which is exactly the "status changes once it's in the catalogue" behaviour Uwe expected, one click. The `deleted_at IS NULL` guard keeps it from linking to a soft-deleted book. 2. FEATURE — "proposed but never chosen" archive. New Repo::neverChosenProposals(): books that were on the ballot in a CLOSED poll but were never the winner of any closed poll, grouped per book with how many closed polls they appeared in and when last. Shown as a "Proposte non selezionate" section on the polls page (a club with years of history finally gets its long list of rejected proposals). A book that lost early but won later is correctly excluded; open polls don't count. 3. (Closed polls are already viewable — the polls list shows closed polls with a "Chiusa" badge and each links to its options/scores. The archive above also serves the "see what books were part of a poll" ask.) Tests: tests/bookclub-acquire-reconcile-poll-history.unit.php (18 behavioural assertions against the live DB, seeded + cleaned up) — reconcile on isbn13/ isbn10, no-ISBN creates new, soft-deleted-book guard; never-chosen inclusion/ exclusion (winner, late-winner, open-poll, two-time loser dedup, empty club). New strings across all four locales. PHPStan level 5 clean.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughIl plugin Book Club aggiunge la riconciliazione ISBN delle proposte esterne, rimuove la chiusura lazy dai flussi di lettura e chiude i poll scaduti durante manutenzione o voto. Le viste distinguono poll aperti e chiusi, con nuove traduzioni e test aggiornati. ChangesBook Club: riconciliazione catalogo
Ciclo di vita dei poll
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Catalogo
participant BookClubPlugin
participant Repo
participant Database
Catalogo->>BookClubPlugin: book.save.after
BookClubPlugin->>Repo: reconcileExternalBooksForCatalogueBook
Repo->>Database: cerca e blocca ISBN attivo
Database-->>Repo: record catalogo compatibile
Repo->>Database: aggiorna collegamento proposta
sequenceDiagram
participant Lettore
participant MobileApiController
participant PollController
participant Poll
Lettore->>MobileApiController: POST voto
MobileApiController->>PollController: chiude poll scaduto per id
PollController->>Poll: aggiorna stato a closed
Poll-->>Lettore: poll_closed (409)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
…d-polls accordion
Builds on the acquire-time reconcile with the behaviour Uwe actually asked for
("status should change automatically once it's in the catalogue"):
- reconcileExternalBooksWithCatalogue(clubId): batch-links every external
proposal whose exact normalized ISBN-10/13 now matches an ACTIVE catalogue
book. Never creates catalogue data, so it's safe to run when a manager opens
the club page — giving immediate consistency without a cron. Idempotent.
- Extracted shared helpers from acquireExternalBook: activeCatalogueBookIdByIsbn
(now FOR UPDATE — closes a soft-delete race), normalizedIsbnParts (uppercases
the ISBN-10 X), and linkExternalBookToCatalogue, which REFUSES to merge two
club-books onto the same libro_id (poll options / votes / reading history may
reference either id, so silently picking one would lose data).
- Closed polls surfaced in a <details> accordion on the club page and the polls
page (open polls stay at the top; the closed list is one click away).
Side-effects on GET are intentional here (reconcile is $canManage-gated,
closeExpiredForClub runs for every authorized visit) for deterministic,
cron-free consistency; a future refactor can move reconciliation to a
post-catalogue-write hook + maintenance backfill and keep GET read-only.
Tests expanded to 33 unit assertions (automatic pass: single-match link,
external stamp, no catalogue rows, idempotency, ambiguous same-club duplicate
refused, soft-delete guard, formatted-ISBN normalization; never-chosen excludes
the voting state) + an E2E covering auto-reconcile without the Acquire button.
All four locales; PHPStan level 5 clean.
… maintenance Follow-up to the review discussion: GET handlers (club, polls, web/mobile dashboards) no longer reconcile proposals or close polls. Consistency is now event- and job-driven, deterministic without hidden writes on read paths: - book.save.after hook → onCatalogueBookSaved → reconcileExternalBooksFor CatalogueBook(): the moment a book's exact ISBN enters the catalogue through the normal create/update flow, matching external proposals across clubs are linked. Targeted (only clubs with a matching-ISBN proposal), reusing the guarded linkExternalBookToCatalogue. - cron/full-maintenance.php: backfill (reconcileAllExternalBooksWithCatalogue, per-club transactions) for imports/integrations that bypass the hook, and closes expired polls. - Admin login stays the automatic fallback (60-minute cooldown). - First expired vote POST closes the poll as an immediate fallback (web + mobile MobileApiController), before the ballot is rejected — GET routes stay read-only. - Deadline comparisons use the MySQL clock (closes_at <= NOW()) so PHP/DB timezone skew can't mis-close or mis-open a poll. - plugin.json 1.4.2 → 1.4.3 so already-active installs re-run onActivate and register the new book.save.after hook. - code-quality.spec.js: fixed a false positive on grouped Slim routes. Verified: PHPStan level 5 clean; bookclub unit test 38/38; code-quality 15/15; book-club-uwe + mobile-api-bookclub-fixes E2E 11/11. GETs confirmed read-only.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@locale/en_US.json`:
- Line 6586: Update the “Non in catalogo” translation in the en_US locale from
the British spelling “catalogue” to the American spelling “catalog”, preserving
the existing translation key.
In `@tests/book-club-uwe.spec.js`:
- Around line 390-411: Aggiorna il seed di scadenza nel test attorno alla query
UPDATE di bookclub_polls: usa il timestamp UTC e un margine sufficientemente
ampio, come nel test mobile-api-bookclub-fixes, invece di NOW() locale con un
solo minuto. Mantieni invariati status, pollId e le asserzioni successive,
assicurando che pollDeadlinePassed() riconosca sempre il poll come scaduto.
In `@tests/code-quality.spec.js`:
- Around line 340-345: Aggiorna il controllo nel blocco `if (!found &&
search.startsWith('/api/v1/'))` affinché la ricerca di `->get(...)` sia limitata
alla callback associata allo stesso `->group('/api/v1')`, invece di verificare
separatamente l’intero contenuto di ogni `phpFiles` source. Mantieni il matching
del percorso figlio tramite `escapedChild` e considera valida la route solo
quando il GET appartiene effettivamente a quel gruppo.
🪄 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
Run ID: c35582cb-1181-4535-80e6-cfc52d911c85
📒 Files selected for processing (17)
locale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonstorage/plugins/book-club/BookClubPlugin.phpstorage/plugins/book-club/README.mdstorage/plugins/book-club/plugin.jsonstorage/plugins/book-club/src/MobileApiController.phpstorage/plugins/book-club/src/PollController.phpstorage/plugins/book-club/src/PublicController.phpstorage/plugins/book-club/src/Repo.phpstorage/plugins/book-club/views/public/polls.phpstorage/plugins/book-club/views/public/show.phptests/book-club-uwe.spec.jstests/bookclub-acquire-reconcile-poll-history.unit.phptests/code-quality.spec.jstests/mobile-api-bookclub-fixes.spec.js
💤 Files with no reviewable changes (1)
- storage/plugins/book-club/src/PublicController.php
- locale/en_US.json: "Not in catalogue" -> "Not in catalog" (this string is new
in this PR; en_US is a US-English locale).
- tests/book-club-uwe.spec.js: seed the expired-poll deadline with
DATE_SUB(UTC_TIMESTAMP(), INTERVAL 26 HOUR) instead of NOW()-1min. The mysql
CLI seeds in the host time zone while the app connection runs at +00:00, so a
NOW()-based "past" instant could still read as future to pollDeadlinePassed()
on a non-UTC host, flaking the close assertion. Mirrors the already-in-suite
pattern in mobile-api-bookclub-fixes.spec.js. Verified: test ① passes.
Skipped the third finding (code-quality.spec.js /api/v1 group scoping): not
materializable — the repo has exactly two `->group('/api/v1')` blocks, one per
file, and no `/api/v2`, so every `->get()` in those files necessarily belongs
to the v1 group. The proposed fix (isolating the group callback with balanced
regex) would add fragile, ReDoS-prone parsing for a purely theoretical case.
Self-review — multi-lens passRan a structured review over the diff. The refactor is well-built and I verified the load-bearing invariants hold: the The findings below are the residual costs of the deliberate GET→read-only move. Worth addressing1. 🟠 Expired-but-open polls display as votable until maintenance/a vote closes them. (5 lenses converged here.) 2. 🟡 Books entering via import/scraping bypass Minor / low
Dismissed (checked, not issues)
|
Follow-up to the multi-lens review of this PR (plugin 1.4.3 -> 1.4.4).
Expired-poll display (the strongest finding — 5 lenses):
- An expired-but-still-open poll (deadline passed, maintenance/vote hasn't
closed it yet) was rendered as a votable "Aperta" poll, so a member could
cast a ballot only to have it silently discarded. poll()/clubPolls() now
expose a MySQL-clock `deadline_passed` flag (read-only, no write), and the
web poll page + list show an honest "Votazione terminata" badge and a
"scaduta il" past-tense deadline. The ballot stays reachable on purpose so
the first vote POST is still the cron-free close fallback; that POST now
answers with a clear "deadline passed, poll closed, vote not recorded"
message instead of the generic "La votazione è chiusa." The mobile GET keeps
reporting persisted state ('open') by design — its vote endpoint returns the
409 poll_closed the app already handles.
Import reconciliation gap:
- CSV and LibraryThing imports inserted into `libri` without firing
book.save.after, so an imported book matching an outstanding external club
proposal was only reconciled on the next maintenance run. Both importers now
dispatch the hook after the per-row commit (outside the transaction, because
the reconcile listener opens its own). reconcileExternalBooksWithCatalogue()
gained a self-owned nesting guard (MySQL has no @@in_transaction), and
reconcileExternalBooksForCatalogueBook() early-exits when no club has an
unresolved external proposal, so per-book import stays cheap.
Smaller findings:
- acquireExternalBook() reports a distinct reason when the club already holds
another proposal linked to the same catalogue book, so the manager sees a
specific message instead of a generic "Acquisizione non riuscita".
- activeCatalogueBookIdByIsbn() adds ORDER BY id for a deterministic pick when
the catalogue legitimately holds homonym ISBN editions.
New i18n keys ("Votazione terminata" + the deadline-passed vote message) added
to all four locales. Verified: book-club-uwe (4/4, incl. a new assertion that
the expired poll shows the ended badge), mobile-api-bookclub-fixes (7/7), all
bookclub-*.unit.php, PHPStan clean.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
storage/plugins/book-club/src/Repo.php (2)
918-979: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEccezione ingoiata quando la transazione è annidata (non posseduta).
Nel blocco
catch, se$ownsTxèfalse(chiamata annidata che riusa la transazione del chiamante), l'eccezione viene solo loggata e la funzione ritorna0, senza rollback né rilancio. Il chiamante esterno che possiede la transazione non viene informato dell'errore e potrebbe farecommit()di uno stato parziale/inconsistente. Attualmente nessun chiamante annida davvero la chiamata (lo dice il commento in testa alla classe), ma è proprio lo scopo di$ownsReconcileTxsupportare questo scenario in futuro.🛡️ Fix proposto
} catch (\Throwable $e) { - if ($ownsTx) { - $this->db->rollback(); - $this->ownsReconcileTx = false; - } - SecureLogger::error('[BookClub] automatic external reconciliation rolled back: ' . $e->getMessage()); - return 0; + if ($ownsTx) { + $this->db->rollback(); + $this->ownsReconcileTx = false; + SecureLogger::error('[BookClub] automatic external reconciliation rolled back: ' . $e->getMessage()); + return 0; + } + // Chiamata annidata: propaga per lasciare il rollback al proprietario della tx. + throw $e; }🤖 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 `@storage/plugins/book-club/src/Repo.php` around lines 918 - 979, Update the catch block in reconcileExternalBooksWithCatalogue so that when ownsTx is false, the caught Throwable is rethrown after logging instead of returning 0. Preserve rollback and state reset for transactions owned by this method, while ensuring the outer transaction owner is notified and can prevent committing partial state.
1077-1166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplessità eccessiva segnalata da PHPMD.
acquireExternalBook()ha CC=16 (soglia 10) e NPath=3565 (soglia 200). Estrarre il blocco di creazione dellibri(righe ~1111-1147) in un metodo privato dedicato ridurrebbe la complessità e semplificherebbe i test.🤖 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 `@storage/plugins/book-club/src/Repo.php` around lines 1077 - 1166, Riduci la complessità di acquireExternalBook estraendo il blocco di creazione del record libri, inclusi editore, autori, publisher, indice di ricerca e copia fisica, in un metodo privato dedicato. Il nuovo metodo deve ricevere i dati necessari, restituire il nuovo libroId o segnalare il fallimento tramite l’eccezione già gestita dal chiamante; mantieni invariati il riuso tramite activeCatalogueBookIdByIsbn e il flusso transazionale esistente.Source: Linters/SAST tools
🤖 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 5663: Update the French translation for the expired-voting message in
locale/fr_FR.json, changing “votre voix n'a pas été enregistrée” to “votre vote
n'a pas été enregistré” while preserving the rest of the message.
---
Outside diff comments:
In `@storage/plugins/book-club/src/Repo.php`:
- Around line 918-979: Update the catch block in
reconcileExternalBooksWithCatalogue so that when ownsTx is false, the caught
Throwable is rethrown after logging instead of returning 0. Preserve rollback
and state reset for transactions owned by this method, while ensuring the outer
transaction owner is notified and can prevent committing partial state.
- Around line 1077-1166: Riduci la complessità di acquireExternalBook estraendo
il blocco di creazione del record libri, inclusi editore, autori, publisher,
indice di ricerca e copia fisica, in un metodo privato dedicato. Il nuovo metodo
deve ricevere i dati necessari, restituire il nuovo libroId o segnalare il
fallimento tramite l’eccezione già gestita dal chiamante; mantieni invariati il
riuso tramite activeCatalogueBookIdByIsbn e il flusso transazionale 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
Run ID: 30dc3bf2-41f7-478d-aa8f-2142932bb481
📒 Files selected for processing (14)
app/Controllers/CsvImportController.phpapp/Controllers/LibraryThingImportController.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonstorage/plugins/book-club/plugin.jsonstorage/plugins/book-club/src/MobileApiController.phpstorage/plugins/book-club/src/PollController.phpstorage/plugins/book-club/src/PublicController.phpstorage/plugins/book-club/src/Repo.phpstorage/plugins/book-club/views/public/poll.phpstorage/plugins/book-club/views/public/polls.phptests/book-club-uwe.spec.js
…ion, complexity - locale/fr_FR.json: "votre voix n'a pas été enregistrée" -> "votre vote n'a pas été enregistré" — in a voting context the noun is "vote" (masculine), not "voix" (voice). - Repo::reconcileExternalBooksWithCatalogue(): the catch now rethrows when the method did NOT own the transaction (nested call reusing a caller's tx), instead of logging and returning 0. Swallowing there would let the outer transaction owner commit partial state unaware of the failure. Owned-tx path still rolls back + returns 0 as before. No caller nests today, but that is exactly what the $ownsReconcileTx guard exists to support. - Repo::acquireExternalBook(): extracted the new-libri creation block (publisher + INSERT + authors + search index + physical copy) into a private createCatalogueBookFromExternal() to cut the method's cyclomatic/NPath complexity. Pure move — it runs inside the same transaction and throws on failure exactly as before. Verified: bookclub-acquire-reconcile-poll-history.unit.php 38/38, PHPStan clean, fr_FR JSON valid (en/de parity untouched).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/book-club/src/Repo.php (1)
885-916: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAggiungi un vincolo unico su
(club_id, libro_id)perbookclub_books.
linkExternalBookToCatalogue()blocca sololibro_idquando esiste già una riga collegata; in assenza diUNIQUE(club_id, libro_id), due acquisizioni concorrenti allo stesso ISBN possono entrambe passare il check e inserire/rivendere due club-book distinti sullo stessolibro_id. Aggiungi il constraint in schema/plugin; se serve mantenere le righe duplicate già esistenti, fallisce la migration perché il check da solo non è sufficiente.🤖 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 `@storage/plugins/book-club/src/Repo.php` around lines 885 - 916, Update the storage plugin schema to add a unique constraint on the bookclub_books (club_id, libro_id) pair, ensuring concurrent acquisitions cannot create duplicate club-book links. Account for existing duplicate rows during migration: either reconcile them before applying the constraint or allow the migration to fail rather than relying solely on linkExternalBookToCatalogue()’s pre-check.
🤖 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.
Outside diff comments:
In `@storage/plugins/book-club/src/Repo.php`:
- Around line 885-916: Update the storage plugin schema to add a unique
constraint on the bookclub_books (club_id, libro_id) pair, ensuring concurrent
acquisitions cannot create duplicate club-book links. Account for existing
duplicate rows during migration: either reconcile them before applying the
constraint or allow the migration to fail rather than relying solely on
linkExternalBookToCatalogue()’s pre-check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6837dcdf-0b7f-4e38-bc20-0afa50e5cc23
📒 Files selected for processing (2)
locale/fr_FR.jsonstorage/plugins/book-club/src/Repo.php
…r hook isolation)
Follow-up review of this PR surfaced a few real gaps:
- views/public/show.php: the club home page's open-polls list still rendered an
expired-but-still-open poll as "Aperta" / "scade il", while poll.php and
polls.php were updated to show "Votazione terminata" / "scaduta il". The
deadline_passed flag is already on each row (clubPolls computes it), so this
was just the third view missing the same read-only treatment. Now consistent.
Covered by a new assertion in book-club-uwe.spec.js (the expired poll shows
the ended badge on the club page too).
- Csv/LibraryThing importers: book.save.after was dispatched INSIDE the per-row
try{} after the commit. The book-club listener swallows throwables, but ANY
other listener that throws would reach the per-row catch — which rolls back
(a no-op on the already-committed row) AND records the row as a failed import
even though it was committed and counted as success, double-counting it. The
dispatch is now wrapped in its own try/catch so a listener throw can never
reach the per-row transaction's error handling.
- views/public/polls.php: the new "Votazioni chiuse" <details> used the
bc-summary class, which was defined only in show.php's <style> block, so it
rendered unstyled. Added the class definition to polls.php.
- Repo::acquireExternalBook() docblock updated to describe the ISBN-reuse branch
and the already_linked / $reason refuse-to-merge outcome added in this PR
(it still claimed unconditional creation).
Verified: php -l + PHPStan clean, bookclub-acquire-reconcile-poll-history.unit.php
38/38. The new show.php badge assertion runs in the full E2E suite.
Self-review follow-up — findings + fixes (commit fb72eb8)Re-reviewed this PR with a multi-lens pass. The core is solid (nested-tx guard + rethrow correct, Correctness
UX / consistency
Docs
Dismissed (checked, not defects): Verified: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/book-club/src/Repo.php (1)
867-877: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
normalizedIsbnPartsnon valida il checksum ISBN.A differenza di
CsvImportController::normalizeIsbn()(che usaIsbnFormatter::isValid()), qui si controlla solo la lunghezza (10/13). Un ISBN esterno malformato ma di lunghezza corretta potrebbe teoricamente combaciare per uguaglianza stringa con un ISBN reale in catalogo, causando un merge indesiderato. Rischio basso ma valuta di allineare la validazione per coerenza.🤖 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 `@storage/plugins/book-club/src/Repo.php` around lines 867 - 877, Allinea normalizedIsbnParts alla validazione ISBN già usata da CsvImportController::normalizeIsbn(), riutilizzando IsbnFormatter::isValid() prima di restituire le parti normalizzate. Mantieni il filtraggio dei caratteri e il controllo delle lunghezze, ma restituisci null per i valori con checksum non valido.
🤖 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/it_IT.json`:
- Around line 6597-6600: Replace the invalid %n placeholder with the standard %d
placeholder in the affected translation keys and values across it_IT.json,
en_US.json, de_DE.json, and fr_FR.json, preserving the existing translated
wording and placeholder order.
---
Outside diff comments:
In `@storage/plugins/book-club/src/Repo.php`:
- Around line 867-877: Allinea normalizedIsbnParts alla validazione ISBN già
usata da CsvImportController::normalizeIsbn(), riutilizzando
IsbnFormatter::isValid() prima di restituire le parti normalizzate. Mantieni il
filtraggio dei caratteri e il controllo delle lunghezze, ma restituisci null per
i valori con checksum non valido.
🪄 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: ffc89220-5595-4ef9-8de9-39511cc79106
📒 Files selected for processing (11)
app/Controllers/CsvImportController.phpapp/Controllers/LibraryThingImportController.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonstorage/plugins/book-club/src/Repo.phpstorage/plugins/book-club/views/public/polls.phpstorage/plugins/book-club/views/public/show.phptests/book-club-uwe.spec.jstests/code-quality.spec.js
| "%n prestiti verranno estesi di %d giorni.": "%n prestiti verranno estesi di %d giorni.", | ||
| "Prestiti estesi": "Prestiti estesi", | ||
| "Nessun prestito esteso": "Nessun prestito esteso", | ||
| "%n prestiti estesi di %d giorni.": "%n prestiti estesi di %d giorni.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -nP '%n|sprintf\s*\(|vsprintf\s*\(' app storage tests localeRepository: fabiodalez-dev/Pinakes
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== I18n locations =="
fd -a 'I18n\.php$|Translate\.php$|loc\.php$|Localization' . | sed "s#$(pwd)/##" | head -50
echo
echo "== sprintf/%n occurrences (precise files) =="
rg -n --glob '!vendor/**' --glob '!node_modules/**' --glob '!build/**' --glob '!dist/**' '%n|function sprintf|class I18n|function tr|function __|sprintf\(' app storage tests locale | head -200
echo
echo "== I18n.php relevant sections =="
if [ -f app/Support/I18n.php ]; then
nl -ba app/Support/I18n.php | sed -n '220,390p'
fiRepository: fabiodalez-dev/Pinakes
Length of output: 25640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/Support/I18n.php relevant sections =="
awk 'NR>=220 && NR<=390 {printf "%5d %s\n", NR, $0}' app/Support/I18n.php
echo
echo "== app/helpers.php relevant sections =="
awk 'NR>=1 && NR<=60 {printf "%5d %s\n", NR, $0}' app/helpers.php
echo
echo "== Call sites for the new string keys =="
rg -n '"Prenotazioni estese"|Prestito|estesa|estesi|prestiti verranno estesi|prestiti estesi' app storage tests locales 2>/dev/null | head -200
echo
echo "== All %n keys in locale JSON files =="
python3 - <<'PY'
import json, pathlib
for p in pathlib.Path("locale").glob("*.json"):
data=json.load(open(p, encoding="utf-8"))
hits=[k for k in data if "%n" in k]
if hits:
print(p)
for k in hits:
print(" KEY:", k.encode("utf-8"))
print(" VALUE:", data[k].encode("utf-8"))
PYRepository: fabiodalez-dev/Pinakes
Length of output: 32929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
bash -c 'php -r "try { echo sprintf(\"%n %d\", 1, 2), \"\\n\"; } catch (\\Throwable \\$e) { echo get_class(\\$e), \": \", \\$e->getMessage(), \"\\n\"; }"'Repository: fabiodalez-dev/Pinakes
Length of output: 263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
cat > "$tmp" <<'PHP'
<?php
try {
echo sprintf("%n %d", 1, 2), "\n";
} catch (Throwable $e) {
echo get_class($e), ": ", $e->getMessage(), "\n";
exit(99);
}
PHP
php "$tmp"Repository: fabiodalez-dev/Pinakes
Length of output: 203
Sostituisci %n con un placeholder standard nelle traduzioni.
Il traduttore passa le stringhe a PHP sprintf; %n non è un specificatore valido e innesca ValueError: Unknown format specifier "n". Usa %d nelle chiavi e nei valori di it_IT.json, en_US.json, de_DE.json e fr_FR.json, mantenendo anche la traduzione coerente.
🤖 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 `@locale/it_IT.json` around lines 6597 - 6600, Replace the invalid %n
placeholder with the standard %d placeholder in the affected translation keys
and values across it_IT.json, en_US.json, de_DE.json, and fr_FR.json, preserving
the existing translated wording and placeholder order.
Source: Path instructions
…ion strings CodeRabbit flagged %n (a dangerous C sprintf specifier PHP rejects) in the bulk loan-extension count strings. They are only ever expanded via JS String.replace, so no crash occurs today, but %n is a footgun if ever passed to PHP sprintf/vsprintf. Switched to the standard %s (count) + %d (days), which sprintf accepts and the placeholder-parity check recognizes. Keeps all shared locales byte-identical across the open PRs.
CodeRabbit findings — dispositionVerified each against the current head:
No product code changed for the stale items. |
…ks self-heal Two review findings on the book-club plugin: - Repo::normalizedIsbnParts() only checked the digit length (13/10), so an arbitrary run of digits became a reconcile match key and two unrelated books sharing a malformed code could be linked. It now validates the ISBN checksum (IsbnFormatter::isValidIsbn13 / isValidIsbn10) — checksum-only, so genuine 978/979 ISBNs and correctly-checksummed codes still match, garbage does not. - ensureBookclubBooksExternalSupport() now self-heals the uq_bcbooks (club_id, libro_id) unique key on the upgrade path (it only lived in CREATE TABLE). It REFUSES to add the key when duplicate rows exist and logs instead: dropping a bookclub_books row cascades into its polls, votes, discussions and reading history — the same reason linkExternalBookToCatalogue never merges two club-books. On any real install the key has always been present, so this is defensive self-healing, never a destructive dedupe. Tests: new bookclub-uq-bcbooks-and-isbn-checksum.unit.php runs the real private migration guard against a sandbox (drop key -> re-add when clean; refuse + keep both rows when duplicated; key always restored) and pins the checksum gate (20/20). Reconcile fixtures updated to checksum-valid ISBNs — the old synthetic ISBNs relied on the very malformed-code matching this fix removes (38/38).
1 participant
Addresses Uwe's round-3 feedback on discussion #138.
🐛 Bug — "Add to catalogue" → "Acquisition failed, please try again."
An external proposal the manager had already bought and added to the catalogue manually could not be acquired.
acquireExternalBook()always inserted a freshlibrirow; with the same ISBN that hitlibri's UNIQUE isbn13/isbn10 key, threw, rolled back the whole transaction, and left the proposal stuck as "External proposal".Now it reconciles: if a non-deleted catalogue book already has the same
isbn13/isbn10, the club entry is linked to that existing row instead of inserting a duplicate — exactly the "status changes once it's in the catalogue" behaviour Uwe expected, in one click. Adeleted_at IS NULLguard keeps it from linking to a soft-deleted book.✨ Feature — "proposed but never chosen" archive
New
Repo::neverChosenProposals(): books that were on the ballot in a closed poll but were never the winner of any closed poll, grouped per book with how many closed polls they appeared in and when last. Rendered as a "Proposte non selezionate" section on the polls page — a club with years of history finally gets its long list of rejected proposals. A book that lost early but won later is correctly excluded; open polls don't count.ℹ️ Closed polls
Already viewable — the polls list shows closed polls with a "Chiusa" badge, each linking to its options/scores. The archive above also serves the "see what books were part of a poll" ask.
Tests
tests/bookclub-acquire-reconcile-poll-history.unit.php— 18 behavioural assertions against the live DB (seeded + cleaned up):times_in_poll=2), one-time loser (=1), empty club → empty archive.New strings across all four locales. PHPStan level 5 clean.
Summary by CodeRabbit
Nuove funzionalità
Correzioni
Localizzazione
Documentazione / Test