book-club: Uwe #138 follow-up — next-meeting card, external heading, proposed-by, remove, PDF export#250
Conversation
…er external heading, proposed-by, remove, PDF export Implements the actionable items from Uwe's #138 feedback (the question-mark help texts are intentionally out of scope for this change): - Next-meeting card now shows the book, end time, an online "join" link and the agenda — not just title/time/location, so edited details are visible at the top of the sidebar. - The external-proposal heading is now an action ("Propose a book not yet in the catalogue") instead of a question. - Managers can attribute a proposal to another active member via a "Proposed by" dropdown (both the catalogue and external forms); validated against active membership. - A dedicated "Remove" button deletes a book from the club's list (managers only, with confirm); deleteClubBook now also cleans up an orphaned, never-acquired external-book row (also fixes the existing reject path). - PDF export of the club's reading list grouped by workflow state (GET /book-club/{slug}/books.pdf), for members and managers. New strings across it/en/de/fr. Covered end-to-end by tests/book-club-uwe.spec.js (test ⑥, all five behaviours in the browser). Also fixes 4 pre-existing PHPStan level-5 errors surfaced while touching these files (full tree is now clean): BookRepository guarded the scraped translator/illustrator against the wrong array (dead condition — now guards the submitted manual value, honouring the "manual wins" intent); SettingsRepository dropped two redundant instanceof checks (mysqli runs in exception mode).
|
Warning Review limit reached
Next review available in: 8 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 Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIl PR estende il Book Club con notifiche, attribuzione delle proposte, rimozione dei libri ed esportazione PDF. Aggiunge configurazione e test per etichette PDF, completa gli export linguistici, preserva le localizzazioni custom e corregge comportamenti nei repository applicativi. ChangesFunzionalità Book Club
Configurazione delle etichette PDF
Traduzioni e manutenzione
Correzioni repository applicativi
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Gestore
participant PublicController
participant Repo
participant NotificationService
Gestore->>PublicController: invia proposta con proposed_by
PublicController->>Repo: verifica membro attivo e salva proposta
Repo-->>PublicController: proposta creata
PublicController->>NotificationService: notifica i manager
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@storage/plugins/book-club/src/PublicController.php`:
- Around line 593-604: Filtra sempre i libri con stato
BookClubPlugin::STATE_PENDING prima di costruire $booksByState, non solo quando
!$canManage, poiché workflowStates() non include quello stato. Mantieni il
controllo successivo su $books così un club composto esclusivamente da proposte
pending viene trattato come vuoto e mostra il messaggio previsto invece di
generare un PDF senza contenuti.
In `@storage/plugins/book-club/src/Repo.php`:
- Around line 946-964: Update deleteClubBook() to use the same transaction and
row-level locking strategy as acquireExternalBook(), selecting the target
bookclub_books row with FOR UPDATE before deleting it. Keep the external_book_id
capture and cleanup within that transaction, and commit or roll back
consistently so deleteClubBook() and acquireExternalBook() cannot modify the
same record concurrently.
In `@storage/plugins/book-club/views/public/show.php`:
- Around line 285-298: Estrai il markup del dropdown “Proposto da”, attualmente
duplicato nei form catalogo ed esterno, in una funzione o partial
riutilizzabile. Aggiorna entrambi i punti, inclusi i riferimenti a `$canManage`,
`$members`, l’opzione “Me stesso” e il filtro sui membri attivi, mantenendo
invariati nomi, valori e escaping.
- Around line 245-249: Replace the inline translated confirmation string in the
book-removal form’s onsubmit handler with a safely encoded data attribute, then
have the handler read that value and pass it to confirm without allowing
translated apostrophes to break JavaScript. Preserve the existing confirmation
behavior and prevent submission when the user cancels.
- Around line 611-625: Update the video-link block around
nextMeeting['video_url'] to sanitize the URL before deciding whether to render
it. Render the “Partecipa online” link only when sanitizePublicHttpUrl() returns
a non-empty valid URL, and reuse that sanitized value for href instead of
sanitizing the raw value inline.
In `@tests/book-club-uwe.spec.js`:
- Line 320: Aggiungi un’asserzione su clubId, subito dopo il suo recupero nella
configurazione del test, verificando che sia maggiore di 0 come già avviene per
memberUserId. Mantieni invariata la UPDATE successiva e interrompi il test
quando il club non viene trovato.
🪄 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: 6ed902fa-2ca0-4bab-97a3-ae81818b5a1b
📒 Files selected for processing (12)
app/Models/BookRepository.phpapp/Models/SettingsRepository.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonstorage/plugins/book-club/BookClubPlugin.phpstorage/plugins/book-club/plugin.jsonstorage/plugins/book-club/src/PublicController.phpstorage/plugins/book-club/src/Repo.phpstorage/plugins/book-club/views/public/show.phptests/book-club-uwe.spec.js
…gs via the existing NotificationService Book-club events that need attention were only firing unused do_action hooks, so admins were never told about a join request, a new proposal, or a new meeting. Rather than hand-rolling recipient selection + mail in the plugin, hook into the existing systems: a new generic NotificationService::notifyAdmins($type, $title, $message, $link, $relatedId) does the two things every other Pinakes admin notification does — createNotification() for the in-app admin bell PLUS an email to every active admin/staff (same recipient query as the private sendToAdmins), with the body wrapped in EmailService's branded base template like the rest of the app. The book-club controllers just call a thin BaseController delegate; no custom email loop or recipient query in the plugin. Wired on: a pending join request (new_user), a new catalogue/external proposal, and a newly scheduled meeting. New strings across it/en/de/fr. Covered by tests/book-club-uwe.spec.js (asserts a proposal creates the admin-bell row).
…nakes admins A club owner who isn't a Pinakes admin never saw the admin bell, so they'd miss join requests/proposals on their own club. notifyAdmins() now takes an optional list of extra recipients and emails the union of admins + those recipients, de-duplicated case-insensitively by email (someone who is both an admin and the club owner is emailed once). The book-club events pass the club's active owner/moderators, so they're always in the loop while the in-app bell still serves the Pinakes admins. No new strings.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
storage/plugins/book-club/src/PublicController.php (1)
617-628: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBug non risolto: i libri "pending" restano invisibili nel PDF per i manager.
STATE_PENDINGnon fa mai parte diworkflowStates(), quindi filtrare solo per!$canManage(riga 619) lascia i libri pending in$booksByState['pending'], ma ilforeach ($states as $state)non itera mai su quella chiave: per un manager con solo proposte pending il PDF esce con intestazione ma senza contenuto, e senza il messaggio "Nessun libro nel club" (perché$booksnon è vuoto).🐛 Fix proposto
$states = $this->repo->workflowStates($club); - $books = $this->repo->clubBooks((int) $club['id']); - if (!$canManage) { - $books = array_values(array_filter( - $books, - static fn(array $b): bool => $b['state'] !== BookClubPlugin::STATE_PENDING - )); - } + // 'pending' non fa mai parte di $states: va escluso a monte per allineare + // il conteggio di $books al rendering per stato e al fallback "vuoto". + $books = array_values(array_filter( + $this->repo->clubBooks((int) $club['id']), + static fn(array $b): bool => $b['state'] !== BookClubPlugin::STATE_PENDING + ));🤖 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/PublicController.php` around lines 617 - 628, Update the book grouping flow around PublicController’s workflowStates(), $books filtering, and the subsequent foreach over $states so manager-visible pending books are represented in the PDF. Ensure STATE_PENDING is included in the rendered state iteration or handled explicitly, preserving the existing non-manager filtering and displaying pending-only clubs with their books instead of an empty section.storage/plugins/book-club/src/Repo.php (1)
946-964: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRace condition non risolta:
deleteClubBook()vsacquireExternalBook().Nessun lock/transazione: se le due chiamate corrono in parallelo sullo stesso
club_book,deleteClubBook()può cancellare la riga appena repuntata alibridaacquireExternalBook(), perdendo l'associazione al club senza errori.🔒 Fix proposto
public function deleteClubBook(int $clubBookId): bool { - // Capture the external link BEFORE deleting the club-book row... - $row = $this->row('SELECT external_book_id FROM bookclub_books WHERE id = ?', 'i', [$clubBookId]); - $ok = $this->exec('DELETE FROM bookclub_books WHERE id = ?', 'i', [$clubBookId]); - $extId = $row !== null ? (int) ($row['external_book_id'] ?? 0) : 0; - if ($ok && $extId > 0) { - $this->exec( - 'DELETE FROM bookclub_external_books WHERE id = ? AND acquired_libro_id IS NULL', - 'i', - [$extId] - ); - } - return $ok; + $this->db->begin_transaction(); + try { + $row = $this->row('SELECT external_book_id FROM bookclub_books WHERE id = ? FOR UPDATE', 'i', [$clubBookId]); + $ok = $this->exec('DELETE FROM bookclub_books WHERE id = ?', 'i', [$clubBookId]); + $extId = $row !== null ? (int) ($row['external_book_id'] ?? 0) : 0; + if ($ok && $extId > 0) { + $this->exec('DELETE FROM bookclub_external_books WHERE id = ? AND acquired_libro_id IS NULL', 'i', [$extId]); + } + $this->db->commit(); + return $ok; + } catch (\Throwable $e) { + $this->db->rollback(); + SecureLogger::error('[BookClub] deleteClubBook rolled back: ' . $e->getMessage()); + return false; + } }🤖 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 946 - 964, Coordinate deleteClubBook() with acquireExternalBook() using a transaction and row-level lock on the shared club-book record. Ensure acquisition and deletion cannot interleave, revalidate the record while holding the lock, and commit or roll back the complete operation so a successful acquisition cannot have its club association removed.
🤖 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.
Duplicate comments:
In `@storage/plugins/book-club/src/PublicController.php`:
- Around line 617-628: Update the book grouping flow around PublicController’s
workflowStates(), $books filtering, and the subsequent foreach over $states so
manager-visible pending books are represented in the PDF. Ensure STATE_PENDING
is included in the rendered state iteration or handled explicitly, preserving
the existing non-manager filtering and displaying pending-only clubs with their
books instead of an empty section.
In `@storage/plugins/book-club/src/Repo.php`:
- Around line 946-964: Coordinate deleteClubBook() with acquireExternalBook()
using a transaction and row-level lock on the shared club-book record. Ensure
acquisition and deletion cannot interleave, revalidate the record while holding
the lock, and commit or roll back the complete operation so a successful
acquisition cannot have its club association removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43695359-954c-47d9-8874-06f28ad01123
📒 Files selected for processing (10)
app/Support/NotificationService.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonstorage/plugins/book-club/src/BaseController.phpstorage/plugins/book-club/src/MeetingController.phpstorage/plugins/book-club/src/PublicController.phpstorage/plugins/book-club/src/Repo.phptests/book-club-uwe.spec.js
…ening ## Labels (#238) - Print a SINGLE copy's label: the copy-labels PDF endpoint accepts ?copy_id= and the physical-copies table gets a per-row print action (reuses the existing label endpoint/renderers — no parallel PDF path). - Custom label size (width×height) alongside the presets, and per-field content checkboxes (app name, title, subtitle, author+publisher, Dewey) so a label prints only the wanted fields. ## Language management (#238) - Custom locales (e.g. nb_NO) are no longer deleted on update: the updater classifies non-bundled locale catalogs (isCustomLocalePath) and skips them in orphan cleanup and both copy passes. - "Download JSON" now exports EVERY current application key, with an empty string for anything untranslated, and the stats count against the current key set, so new missing keys are identifiable after each update. ## Book-club review fixes (PR #250 / CodeRabbit) - deleteClubBook() runs in a transaction with SELECT ... FOR UPDATE so it can't race acquireExternalBook() on the same row. - The reading-list PDF renders a manager's pending proposals as their own state instead of silently dropping them. - Book-removal confirm no longer breaks on a translated apostrophe (SweetAlert2 data attribute instead of an inline confirm() string). - Next-meeting card and meetings list only render the video link when sanitizePublicHttpUrl() returns a valid http(s) URL (no empty-href dead link). - "Proposed by" dropdown extracted into a single shared closure (was duplicated). ## Also - BookRepository: manual translator/illustrator win over the scraped fallback (guard was dead); SettingsRepository redundant instanceof removed — full-tree PHPStan clean. - Real tests replacing str_contains checks: updater-custom-locale.unit.php (19/19, custom-locale preservation logic), language-export-completeness.unit.php (6/6, real download() emits all keys), label-pdf-content.spec.js (E2E that generates the PDF and asserts page size + content via pdfinfo/pdftotext). New strings across it/en/de/fr (parity maintained).
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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`:
- Around line 6522-6525: Remove the duplicate “Nome applicazione” and “Stampa
etichetta” entries from the end of locale/fr_FR.json, preserving their existing
earlier definitions and leaving the unique “Sottotitolo libro” and “Autore ed
editore” entries unchanged.
In `@locale/it_IT.json`:
- Around line 6522-6525: Rimuovi le definizioni duplicate delle chiavi "Nome
applicazione" e "Stampa etichetta" dal catalogo, mantenendo le occorrenze già
presenti nelle rispettive definizioni originali. Conserva invece le chiavi
uniche "Sottotitolo libro" e "Autore ed editore".
In `@tests/language-export-completeness.unit.php`:
- Around line 55-69: Update the database setup and cleanup around the nb_NO
language mutation to preserve the complete pre-test row and restore it in a
finally block, including the case where no row originally existed.
Alternatively, run the test against an isolated database; ensure the existing
nb_NO record is never permanently deleted after the test.
In `@tests/updater-custom-locale.unit.php`:
- Around line 52-54: Rendi deterministica l’asserzione nel test che usa
$isCustom per il percorso Windows, sostituendo la condizione alternativa false
|| true con una verifica diretta che locale\\nb_NO.json venga classificato come
catalogo custom.
🪄 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: 02212697-891b-400f-baa6-1bfcb198692a
📒 Files selected for processing (22)
app/Controllers/Admin/LanguagesController.phpapp/Controllers/LibriController.phpapp/Controllers/SettingsController.phpapp/Support/Updater.phpapp/Views/libri/scheda_libro.phpapp/Views/settings/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonstorage/plugins/book-club/src/PublicController.phpstorage/plugins/book-club/src/Repo.phpstorage/plugins/book-club/views/public/show.phptests/admin-features.spec.jstests/book-club-followup.unit.phptests/book-club-uwe.spec.jstests/bookrepository-scraped-precedence.spec.jstests/discussion-238-followup.unit.phptests/label-pdf-content.spec.jstests/language-export-completeness.unit.phptests/plugin-manager.unit.phptests/updater-custom-locale.unit.php
Code reviewBranch: Found 12 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (3)
Details and fix proposalsF001 — notifyAdmins()'s docblock promises "Best-effort: failures are logged, never thrown", but the in-app createNotification() call runs outside the try/catch that guards the rest of the function, so any DB failure there (mysqli exception mode) propagates as an uncaught exception to the caller instead of being logged.File: Latest fix attempt (fixrun_01KX8SWH4PK4ZPS5KN2HSXSMZA): fixed and verified F003 — Switching the scraped-fallback guard from array_key_exists($cols) to empty($data[...]) makes PHP's loose empty() treat a manually-entered translator/illustrator value of the literal string "0" as absent, silently overwriting it with the scraped value even though the librarian explicitly typed something.File: Latest fix attempt (fixrun_01KX8SWH4PK4ZPS5KN2HSXSMZA): fixed and verified F004 — The new 'Prossimo incontro' sidebar card renders the meeting video/join URL to any club viewer, whereas the meeting list at line 471 deliberately gates the same video_url behind ($isMember || $canManage); since canView() returns true for non-members on public/private clubs, this leaks the online-meeting join link to non-members.File: Evidence:
Approach: Gate the sidebar video_url exactly like the meeting-list precedent at line 471: only compute the sanitized URL when ($isMember || $canManage), otherwise empty string so the if-block is skipped. Do NOT gate agenda — it is public by design across the plugin. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_01KX8QPKNPAZTZ4AACQ7Y7C9FW): fixed and verified Fix runsRun
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F004 | FG-1 | ✓ fixed and verified | |
| F006 | FG-2 | ✓ fixed and verified | |
| F013 | FG-3 | ✓ fixed and verified |
Run fixrun_01KX8SWH4PK4ZPS5KN2HSXSMZA — 2026-07-11T14:46:40Z
- Outcomes: 3 fixed and verified
- Commits:
3b6648d
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F001 | FG-1 | ✓ fixed and verified | |
| F003 | FG-2 | ✓ fixed and verified | |
| F008 | FG-3 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
…ty, deterministic assertion - locale/*.json: remove the physically duplicated "Nome applicazione" and "Stampa etichetta" keys (present twice in ALL four catalogs, not just it/fr — the tail re-adds shadowed the originals at parse time). Kept the original definitions; normalized the two stray typographic apostrophes in fr_FR values to the file's dominant ASCII style (1080:4). - tests/language-export-completeness.unit.php: snapshot the FULL pre-test languages row for nb_NO and restore it column-by-column in cleanup, so a user's real nb_NO registration is never lost by running the test (proven: a seeded pre-existing row survives byte-identical). - tests/updater-custom-locale.unit.php: the backslash-path assertion was an always-satisfiable OR; now asserts directly that a Windows-separator path classifies as a custom catalog. Still 19/19.
Auto-recommendation acceptance
2 auto-rec finding(s) eligible at threshold ≥ 50. Of those, 2 auto-promoted via batch, 0 promoted with an edited or alternative hint, 0 skipped per-finding. Promoted (batch)
Auto-recommendation acceptance: append-only audit. Each |
Fix groups (committed): - [FG-1] F004 — storage/plugins/book-club/views/public/show.php: verified Gate the next-meeting sidebar video/join URL behind (isMember || canManage), mirroring the meeting-list gate — the join link was leaking to non-members. - [FG-2] F006 — app/Controllers/LibriController.php: verified Remove the premature fixed 30-char cap on the author+publisher string in renderLandscapeLabel; the width-based truncation is the sole cut point, so the publisher is no longer clipped away on landscape labels. - [FG-3] F013 — app/Support/NotificationService.php: verified Add the Mailer::isSmtpReachable() circuit-breaker before notifyAdmins()'s per-recipient email loop (same guard sendWithRetry uses) so a down SMTP host fails fast instead of hanging user-facing request handlers; the in-app admin bell still fires. Post-fix review: 3/3 groups verified complete; 0 group(s) partial; 0 group(s) reverted.
Walkthrough decisions
Walked 3 qualifying finding(s) (the uncertain tier left open after the fix run). All 3 promoted with the recommended option; 0 skipped. Promoted
Walkthrough decisions: append-only audit. Promoted findings are now |
…, 0 reverted) Fix groups (committed): - [FG-1] F001 — app/Support/NotificationService.php: verified Move the in-app createNotification() call inside notifyAdmins()'s try/catch so the docblock's "never thrown" contract holds for both channels; a phase marker keeps in-app vs email failures distinguishable in the log. - [FG-2] F003 — app/Models/BookRepository.php, tests/bookrepository-scraped-precedence.spec.js: verified Guard the scraped translator/illustrator fallback with the same strict admission test the field loop uses (array_key_exists && !== '' && !== null) so a manually-typed literal "0" is preserved; behavioral "0"-wins test added. - [FG-3] F008 — app/Controllers/SettingsController.php: verified Persist the label content-visibility checkboxes on every submit, independent of the width/height validation branch, so an invalid custom size no longer silently discards toggle changes. Post-fix review: 3/3 groups verified complete; 0 group(s) partial; 0 group(s) reverted.
Implements the actionable items from Uwe's #138 feedback. (The question-mark help texts are intentionally out of scope for this PR.)
Changes
deleteClubBooknow also cleans up an orphaned, never-acquired external-book row — which also fixes the existing reject-proposal path.GET /book-club/{slug}/books.pdf), available to members and managers.New strings across it/en/de/fr.
Tests
tests/book-club-uwe.spec.jstest ⑥ exercises all five behaviours end-to-end in the browser (heading text, proposed-by → member attribution, PDF 200/application-pdf, remove + orphan cleanup, next-meeting card agenda + online link). 4/4 pass.Also
Fixes 4 pre-existing PHPStan level-5 errors surfaced while touching these files (full tree is now clean):
BookRepositoryguarded the scraped translator/illustrator against the wrong array (a dead condition — now guards the submitted manual value, honouring the "manual wins" intent);SettingsRepositorydropped two redundantinstanceofchecks (mysqli runs in exception mode).No schema migration; plugin bumped to 1.4.1 (its boot self-heal re-runs ensureSchema when needed).
Summary by CodeRabbit
Nuove funzionalità
Correzioni / Miglioramenti
Localizzazione e aggiornamenti