Skip to content

book-club: Uwe #138 follow-up — next-meeting card, external heading, proposed-by, remove, PDF export#250

Merged
fabiodalez-dev merged 7 commits into
mainfrom
feat/book-club-uwe-138-followup
Jul 11, 2026
Merged

book-club: Uwe #138 follow-up — next-meeting card, external heading, proposed-by, remove, PDF export#250
fabiodalez-dev merged 7 commits into
mainfrom
feat/book-club-uwe-138-followup

Conversation

@fabiodalez-dev

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

Copy link
Copy Markdown
Owner

Implements the actionable items from Uwe's #138 feedback. (The question-mark help texts are intentionally out of scope for this PR.)

Changes

  • Next-meeting card now shows the book, end time, an online join link and the agenda — not just title/time/location, so a meeting's edited details are visible at the top of the sidebar.
  • External-proposal heading is now an action ("Propose a book not yet in the catalogue") instead of a question.
  • "Proposed by" dropdown (managers only) lets a manager attribute a proposal to another active member, on both the catalogue and external forms (validated against active membership).
  • "Remove" button deletes a book from the club's list (managers only, with a confirm). deleteClubBook now also cleans up an orphaned, never-acquired external-book row — which also fixes the existing reject-proposal path.
  • PDF export of the club's reading list grouped by workflow state (GET /book-club/{slug}/books.pdf), available to members and managers.

New strings across it/en/de/fr.

Tests

tests/book-club-uwe.spec.js test ⑥ 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): BookRepository guarded the scraped translator/illustrator against the wrong array (a dead condition — now guards the submitted manual value, honouring the "manual wins" intent); SettingsRepository dropped two redundant instanceof checks (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à

    • Nel Book Club: rimozione definitiva dei libri dal club e generazione/stampa della lista lettura in PDF.
    • Form “Proponi un libro” con selezione “Proposto da” (solo membri attivi) e gestione del prossimo incontro con campi aggiornati.
    • Etichette: stampa PDF anche per singola copia.
  • Correzioni / Miglioramenti

    • Migliorata la precedenza tra dati manuali ed estratti per traduttore/illustratore.
    • Migliorate le notifiche agli amministratori per eventi Book Club e nuovi incontri.
    • Sicurezza/qualità link “Partecipa online” e visualizzazione agenda.
  • Localizzazione e aggiornamenti

    • Export/statistiche traduzioni più complete e coerenti; preservati i cataloghi “custom” durante gli aggiornamenti.
    • Aggiornate le traduzioni (it/EN/DE/FR) per le nuove funzionalità Book Club ed etichette.

…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).
@coderabbitai

coderabbitai Bot commented Jul 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: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

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

Run ID: bc0c335d-9f7d-427e-9a07-af80ea7d7099

📥 Commits

Reviewing files that changed from the base of the PR and between a14263f and 3b6648d.

📒 Files selected for processing (6)
  • app/Controllers/LibriController.php
  • app/Controllers/SettingsController.php
  • app/Models/BookRepository.php
  • app/Support/NotificationService.php
  • storage/plugins/book-club/views/public/show.php
  • tests/bookrepository-scraped-precedence.spec.js
📝 Walkthrough

Walkthrough

Il 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.

Changes

Funzionalità Book Club

Layer / File(s) Summary
Notifiche e attribuzione
app/Support/NotificationService.php, storage/plugins/book-club/src/*
Richieste, proposte e incontri generano notifiche; le proposte possono essere attribuite a membri attivi.
Rimozione ed esportazione
storage/plugins/book-club/{BookClubPlugin.php,src/*}
Sono aggiunte route per rimuovere libri e generare PDF, con cancellazione transazionale.
Interfaccia e test
storage/plugins/book-club/views/*, tests/book-club-*
La vista integra proposta, rimozione, stampa e dati degli incontri; i test verificano i flussi.
Versione e traduzioni
storage/plugins/book-club/plugin.json, locale/*.json
Il plugin passa alla versione 1.4.1 e vengono aggiunte le stringhe necessarie.

Configurazione delle etichette PDF

Layer / File(s) Summary
Impostazioni e generazione
app/Controllers/{SettingsController,LibriController}.php, app/Views/settings/index.php, app/Views/libri/scheda_libro.php
Le etichette supportano dimensioni custom, contenuti configurabili e filtro per copia.
Verifica E2E
tests/admin-features.spec.js, tests/label-pdf-content.spec.js
I test verificano impostazioni, dimensioni PDF, contenuti e selezione della copia.

Traduzioni e manutenzione

Layer / File(s) Summary
Export e localizzazioni custom
app/Controllers/Admin/LanguagesController.php, app/Support/Updater.php, tests/*language*, tests/*updater*
Gli export usano chiavi canoniche e l’updater preserva i cataloghi custom esistenti.
Regressioni applicative
tests/discussion-238-followup.unit.php, tests/plugin-manager.unit.php
Sono aggiornati i controlli sui percorsi PDF, impostazioni, updater e messaggi di upgrade.

Correzioni repository applicativi

Layer / File(s) Summary
Fallback e gestione mysqli
app/Models/{BookRepository,SettingsRepository}.php, tests/bookrepository-scraped-precedence.spec.js
Il fallback scraped viene applicato quando il campo manuale è vuoto; getEmailTemplates chiude direttamente le risorse mysqli.

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
Loading

Possibly related PRs

🚥 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 riassume bene i principali cambi del PR: follow-up Book Club con card prossima riunione, proposta esterna, proposed-by, rimozione e PDF.
Docstring Coverage ✅ Passed Docstring coverage is 70.97% 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 unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/book-club-uwe-138-followup

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: 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

📥 Commits

Reviewing files that changed from the base of the PR and between ed9ea55 and 9a8424a.

📒 Files selected for processing (12)
  • app/Models/BookRepository.php
  • app/Models/SettingsRepository.php
  • locale/de_DE.json
  • locale/en_US.json
  • locale/fr_FR.json
  • locale/it_IT.json
  • storage/plugins/book-club/BookClubPlugin.php
  • storage/plugins/book-club/plugin.json
  • storage/plugins/book-club/src/PublicController.php
  • storage/plugins/book-club/src/Repo.php
  • storage/plugins/book-club/views/public/show.php
  • tests/book-club-uwe.spec.js

Comment thread storage/plugins/book-club/src/PublicController.php
Comment thread storage/plugins/book-club/src/Repo.php
Comment thread storage/plugins/book-club/views/public/show.php
Comment thread storage/plugins/book-club/views/public/show.php Outdated
Comment thread storage/plugins/book-club/views/public/show.php
Comment thread tests/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.

@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.

♻️ Duplicate comments (2)
storage/plugins/book-club/src/PublicController.php (1)

617-628: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Bug non risolto: i libri "pending" restano invisibili nel PDF per i manager.

STATE_PENDING non fa mai parte di workflowStates(), quindi filtrare solo per !$canManage (riga 619) lascia i libri pending in $booksByState['pending'], ma il foreach ($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é $books non è 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 win

Race condition non risolta: deleteClubBook() vs acquireExternalBook().

Nessun lock/transazione: se le due chiamate corrono in parallelo sullo stesso club_book, deleteClubBook() può cancellare la riga appena repuntata a libri da acquireExternalBook(), 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9a8424a and 20d5334.

📒 Files selected for processing (10)
  • app/Support/NotificationService.php
  • locale/de_DE.json
  • locale/en_US.json
  • locale/fr_FR.json
  • locale/it_IT.json
  • storage/plugins/book-club/src/BaseController.php
  • storage/plugins/book-club/src/MeetingController.php
  • storage/plugins/book-club/src/PublicController.php
  • storage/plugins/book-club/src/Repo.php
  • tests/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).

@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: 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

📥 Commits

Reviewing files that changed from the base of the PR and between 20d5334 and 1f88294.

📒 Files selected for processing (22)
  • app/Controllers/Admin/LanguagesController.php
  • app/Controllers/LibriController.php
  • app/Controllers/SettingsController.php
  • app/Support/Updater.php
  • app/Views/libri/scheda_libro.php
  • app/Views/settings/index.php
  • locale/de_DE.json
  • locale/en_US.json
  • locale/fr_FR.json
  • locale/it_IT.json
  • storage/plugins/book-club/src/PublicController.php
  • storage/plugins/book-club/src/Repo.php
  • storage/plugins/book-club/views/public/show.php
  • tests/admin-features.spec.js
  • tests/book-club-followup.unit.php
  • tests/book-club-uwe.spec.js
  • tests/bookrepository-scraped-precedence.spec.js
  • tests/discussion-238-followup.unit.php
  • tests/label-pdf-content.spec.js
  • tests/language-export-completeness.unit.php
  • tests/plugin-manager.unit.php
  • tests/updater-custom-locale.unit.php

Comment thread locale/fr_FR.json Outdated
Comment thread locale/it_IT.json Outdated
Comment thread tests/language-export-completeness.unit.php
Comment thread tests/updater-custom-locale.unit.php Outdated
@fabiodalez-dev

fabiodalez-dev commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Code review

Branch: feat/book-club-uwe-138-followupmain
PR: #250 (open)
Review ID: rev_01KX8KP03C50DVE0K98JYBGXDT
Sub-agent tokens: 2,580,680 across 23 invocations

Found 12 findings across all lanes:

  • Deep lane (correctness/security): 3 resolved

Deep lane — correctness & security

✓ Auto-fixable (3)

# Score Impact File Issue Status
F001 45 correctness app/Support/NotificationService.php:1367-1426 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. (human-confirmed) ✓ fixed and verified (3b6648d)
F003 45 correctness app/Models/BookRepository.php:1258-1262 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. (human-confirmed) ✓ fixed and verified (3b6648d)
F004 80 security storage/plugins/book-club/views/public/show.php:624-626 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
Details and fix proposals

F001 — 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: app/Support/NotificationService.php:1367-1426
Score: 45
Human-confirmed: @fabiodalezbackup@gmail.com at 2026-07-11T14:33:41Z — walkthrough decision — option A (move createNotification inside the try)
Promoted from disposition=uncertain / actionability=auto_fixable / score_phase4=45
Fix direction: In NotificationService::notifyAdmins() (app/Support/NotificationService.php ~line 1381-1438), move the $this->createNotification(...) call (currently the first statement, before the try) to be the first statement INSIDE the existing try { } catch (\Throwable) block, so both the in-app bell and the email path are covered by the same catch. Keep the log message distinguishable (e.g. prefix 'notifyAdmins: in-app notification failed' vs the existing email failure message). Note the isSmtpReachable guard's early return must still allow the in-app bell to have fired first — the bell call must come before the guard.

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: app/Models/BookRepository.php:1258-1262
Score: 45
Human-confirmed: @fabiodalezbackup@gmail.com at 2026-07-11T14:33:41Z — walkthrough decision — option A (strict manual-presence guard)
Promoted from disposition=uncertain / actionability=auto_fixable / score_phase4=45
Fix direction: In app/Models/BookRepository.php lines ~1258-1262, replace empty($data['traduttore']) / empty($data['illustratore']) with the same strict admission test the field loop uses at ~line 1204, negated: !(array_key_exists('traduttore', $data) && $data['traduttore'] !== '' && $data['traduttore'] !== null) — for BOTH traduttore and illustratore. Keep !empty($data['scraped_translator'])/scraped_illustrator on the scraped side. Must stay PHPStan level-5 clean (do NOT revert to the array_key_exists($cols) form). Extend tests/bookrepository-scraped-precedence.spec.js with a behavioral case: manual '0' + scraped present → '0' wins.

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: storage/plugins/book-club/views/public/show.php:624-626
Score: 80 (strong)

Evidence:

  • storage/plugins/book-club/views/public/show.php:471 — meeting list gates video_url: ($isMember || $canManage) ? sanitizePublicHttpUrl(...) : ''
  • storage/plugins/book-club/views/public/show.php:624 — sidebar card computes the sanitized video_url with NO membership gate
  • storage/plugins/book-club/views/public/show.php:626 — renders the ungated join link (Partecipa online) to any viewer
  • storage/plugins/book-club/src/BaseController.php:156 — canView() returns true for any visitor on privacy in [public, private], so non-members reach show()
  • storage/plugins/book-club/src/PublicController.php:95 — show() passes nextMeeting unconditionally (not member-gated)
  • storage/plugins/book-club/src/MobileApiController.php:186 — parallel surface gates it: video_url => isMember || canManage ? ... : ''
  • storage/plugins/book-club/src/MeetingController.php:244 — in-code invariant comment: video_url is members-only everywhere else; ICS feed only emits it with a valid member token (:247)
  • storage/plugins/book-club/src/ApiController.php:208 — Upcoming meetings: location yes, video_url NEVER
  • git 9a8424a — the sidebar video_url (+agenda) rows were ADDED by this PR (introduced_by_pr confirmed)
  • show.php:479-481 — agenda in the meeting list is shown to ALL viewers; only minutes and video_url are member-gated, so the sidebar agenda is NOT a leak (agenda half of the merged finding disproven)

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:

  • storage/plugins/book-club/views/public/show.php — Change line 624 to: $bcVideoUrl = ($isMember || $canManage) ? \App\Support\HtmlHelper::sanitizePublicHttpUrl((string) ($nextMeeting['video_url'] ?? '')) : ''; (why: Restores the members-only invariant for the join link, mirroring the in-repo precedent at line 471 and the identical gate in MobileApiController:186)

Verification:

  • View /book-club/{slug} (club with next meeting having video_url) as logged-out / non-member: Partecipa online must NOT render in the sidebar
  • View as active member and as manager: the link SHOULD render
  • Confirm the meeting-list link (line 471) still behaves identically (regression baseline)

Edge cases to preserve:

  • Managers/staff (canManage true even when not a member) must still see the link
  • Pinakes admin/staff bypass (isPinakesAdmin) flows through canManage — link visible
  • Do not alter agenda rendering (:628-630) — public by design across the plugin

Latest fix attempt (fixrun_01KX8QPKNPAZTZ4AACQ7Y7C9FW): fixed and verified

Fix runs

Run fixrun_01KX8QPKNPAZTZ4AACQ7Y7C9FW — 2026-07-11T14:22:07Z

  • Outcomes: 3 fixed and verified
  • Commits: fd09670
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.
@fabiodalez-dev

Copy link
Copy Markdown
Owner Author

Auto-recommendation acceptance

rev_01KX8KP03C50DVE0K98JYBGXDT · run_id=fixrun_01KX8QPKNPAZTZ4AACQ7Y7C9FW · choice=apply_all · threshold=50 · reviewer=auto-rec/fabiodalezbackup@gmail.com · ts=2026-07-11T14:11:33Z

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)

  • F006 — renderLandscapeLabel now receives the publisher already concatenated into $autoriStr (via buildAuthorString + label_include_publisher) and truncates the combined 'Author — Publisher' string to 30 chars, whereas previously the publisher had its own separate mb_substr(...,0,20) slot; on landscape labels the publisher is now frequently truncated away.
    • Hint: In renderLandscapeLabel (app/Controllers/LibriController.php:3037), the hard mb_substr($autoriStr, 0, 30) runs BEFORE the width-based mb_substr($infoText, 0, $maxInfoChars) at lines 3042-3043, so the publisher (concatenated at buildAuthorString) is clipped by the fixed 30-char cap regardless of label size. Remove or significantly raise the premature 30-char cap at line 3037 and let the existing availableWidth-based maxInfoChars truncation be the sole truncation point — mirroring how $maxTitleChars is derived from availableWidth for the title.
  • F013 — notifyAdmins() is called synchronously and unqueued from new user-facing request handlers (join-club, propose-book, create-meeting); it loops over every active admin/staff (plus club managers) and calls PHPMailer's blocking send() for each one before the controller can flash success and redirect, so the acting user's request hangs for the full duration of N sequential SMTP sends with no progress feedback.
    • Hint: notifyAdmins() (app/Support/NotificationService.php:1414-1422) loops recipients calling $this->emailService->sendEmail() directly, bypassing the circuit breaker sendWithRetry() already implements (Mailer::isSmtpReachable() guard from fc2a61ce). Add the same guard — call Mailer::isSmtpReachable() once before the foreach in notifyAdmins() and skip the email loop (log via SecureLogger) if unreachable — so a down SMTP host fails fast instead of blocking the join-club/propose-book/create-meeting request handlers with per-recipient blocking sends.

Auto-recommendation acceptance: append-only audit. Each /adamsreview:fix run posts a fresh entry. Promoted findings are now confirmed_mechanical with human_confirmation set; Phase 8 dispatches them next.

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.
@fabiodalez-dev

Copy link
Copy Markdown
Owner Author

Walkthrough decisions

rev_01KX8KP03C50DVE0K98JYBGXDT · threshold=45 · reviewer=fabiodalezbackup@gmail.com · ts=2026-07-11T14:28:29Z

Walked 3 qualifying finding(s) (the uncertain tier left open after the fix run). All 3 promoted with the recommended option; 0 skipped.

Promoted

  • F001 — notifyAdmins()'s in-app createNotification() call runs outside the try/catch, contradicting the docblock's "never thrown" promise.
    • Decision: move createNotification() inside the existing try (contract honored for future callers; distinguishable log message).
  • F003 — empty() treats a manually-entered translator/illustrator value of the literal string "0" as absent, letting the scraped value overwrite it.
    • Decision: replace empty() with the same strict admission test the field loop uses (array_key_exists && !== '' && !== null) for both fields, PHPStan-clean, plus a behavioral test for the "0" case.
  • F008 — label content checkboxes are persisted only inside the valid-dimensions branch, so an invalid custom size silently discards toggle changes.
    • Decision: hoist the show_* persistence loop out of the dimension-validation branch — checkbox persistence is independent of dimension validity.

Walkthrough decisions: append-only audit. Promoted findings are now confirmed_mechanical with human_confirmation set; the next /adamsreview:fix run dispatches them.

…, 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.
@fabiodalez-dev fabiodalez-dev merged commit e06490c into main Jul 11, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant