From 226f6dab43a7ae37794234e14aab66a8d66ceca5 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 22 Jul 2026 00:22:08 +0200 Subject: [PATCH 01/10] fix(book-club #138): reconcile external proposals on acquire + never-chosen archive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- locale/de_DE.json | 8 +- locale/en_US.json | 8 +- locale/fr_FR.json | 8 +- locale/it_IT.json | 8 +- .../plugins/book-club/src/PollController.php | 3 + storage/plugins/book-club/src/Repo.php | 139 ++++++++--- .../plugins/book-club/views/public/polls.php | 29 +++ ...ub-acquire-reconcile-poll-history.unit.php | 216 ++++++++++++++++++ 8 files changed, 382 insertions(+), 37 deletions(-) create mode 100644 tests/bookclub-acquire-reconcile-poll-history.unit.php diff --git a/locale/de_DE.json b/locale/de_DE.json index 5e5bd7d5f..27061c2b0 100644 --- a/locale/de_DE.json +++ b/locale/de_DE.json @@ -6577,5 +6577,11 @@ "Genere e sottogeneri eliminati con successo!": "Genre und Untergenres erfolgreich gelöscht!", "Questa azione elimina il genere e tutti i suoi sottogeneri in modo permanente. I libri associati verranno scollegati da questi generi.": "Diese Aktion löscht das Genre und alle seine Untergenres dauerhaft. Zugeordnete Bücher werden von diesen Genres getrennt.", "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.": "Sind Sie sicher, dass Sie dieses Genre und alle seine Untergenres löschen möchten? Zugeordnete Bücher werden von diesen Genres getrennt.", - "Elimina genere e sottogeneri": "Genre und Untergenres löschen" + "Elimina genere e sottogeneri": "Genre und Untergenres löschen", + "Proposte non selezionate": "Nicht ausgewählte Vorschläge", + "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.": "Bücher, die in einer geschlossenen Abstimmung zur Wahl standen, aber nie ausgewählt wurden.", + "votazione": "Abstimmung", + "votazioni": "Abstimmungen", + "ultima": "zuletzt", + "Non in catalogo": "Nicht im Katalog" } diff --git a/locale/en_US.json b/locale/en_US.json index 0bd39013f..82075a92d 100644 --- a/locale/en_US.json +++ b/locale/en_US.json @@ -6577,5 +6577,11 @@ "Genere e sottogeneri eliminati con successo!": "Genre and subgenres deleted successfully!", "Questa azione elimina il genere e tutti i suoi sottogeneri in modo permanente. I libri associati verranno scollegati da questi generi.": "This action permanently deletes the genre and all of its subgenres. Associated books will be unlinked from these genres.", "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.": "Are you sure you want to delete this genre and all of its subgenres? Associated books will be unlinked from these genres.", - "Elimina genere e sottogeneri": "Delete genre and subgenres" + "Elimina genere e sottogeneri": "Delete genre and subgenres", + "Proposte non selezionate": "Proposals not selected", + "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.": "Books that were on the ballot in a closed poll but were never chosen.", + "votazione": "poll", + "votazioni": "polls", + "ultima": "last", + "Non in catalogo": "Not in catalogue" } diff --git a/locale/fr_FR.json b/locale/fr_FR.json index 2ea7a0674..a979eaab1 100644 --- a/locale/fr_FR.json +++ b/locale/fr_FR.json @@ -6577,5 +6577,11 @@ "Genere e sottogeneri eliminati con successo!": "Genre et sous-genres supprimés avec succès !", "Questa azione elimina il genere e tutti i suoi sottogeneri in modo permanente. I libri associati verranno scollegati da questi generi.": "Cette action supprime définitivement le genre et tous ses sous-genres. Les livres associés seront dissociés de ces genres.", "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.": "Voulez-vous vraiment supprimer ce genre et tous ses sous-genres ? Les livres associés seront dissociés de ces genres.", - "Elimina genere e sottogeneri": "Supprimer le genre et les sous-genres" + "Elimina genere e sottogeneri": "Supprimer le genre et les sous-genres", + "Proposte non selezionate": "Propositions non retenues", + "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.": "Livres qui ont été candidats dans un vote clos mais qui n'ont jamais été retenus.", + "votazione": "vote", + "votazioni": "votes", + "ultima": "dernière", + "Non in catalogo": "Pas au catalogue" } diff --git a/locale/it_IT.json b/locale/it_IT.json index 00a96034b..f9b714977 100644 --- a/locale/it_IT.json +++ b/locale/it_IT.json @@ -6577,5 +6577,11 @@ "Genere e sottogeneri eliminati con successo!": "Genere e sottogeneri eliminati con successo!", "Questa azione elimina il genere e tutti i suoi sottogeneri in modo permanente. I libri associati verranno scollegati da questi generi.": "Questa azione elimina il genere e tutti i suoi sottogeneri in modo permanente. I libri associati verranno scollegati da questi generi.", "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.": "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.", - "Elimina genere e sottogeneri": "Elimina genere e sottogeneri" + "Elimina genere e sottogeneri": "Elimina genere e sottogeneri", + "Proposte non selezionate": "Proposte non selezionate", + "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.": "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.", + "votazione": "votazione", + "votazioni": "votazioni", + "ultima": "ultima", + "Non in catalogo": "Non in catalogo" } diff --git a/storage/plugins/book-club/src/PollController.php b/storage/plugins/book-club/src/PollController.php index 7c7458cf3..bead2a748 100644 --- a/storage/plugins/book-club/src/PollController.php +++ b/storage/plugins/book-club/src/PollController.php @@ -140,6 +140,9 @@ public function pollsPage(ServerRequestInterface $request, ResponseInterface $re 'club' => $club, 'polls' => $this->repo->clubPolls((int) $club['id']), 'eligible' => $eligible, + // Books that lost every poll they were in (#138) — the "proposed but + // never chosen" archive members asked to browse. + 'neverChosen' => $this->repo->neverChosenProposals((int) $club['id']), 'isMember' => $this->isActiveMember($club), 'canManage' => $this->canManage($club), 'canCreate' => $this->can($club, 'polls.create'), diff --git a/storage/plugins/book-club/src/Repo.php b/storage/plugins/book-club/src/Repo.php index a39682ea3..940969b3e 100644 --- a/storage/plugins/book-club/src/Repo.php +++ b/storage/plugins/book-club/src/Repo.php @@ -855,41 +855,70 @@ public function acquireExternalBook(int $clubBookId): ?int $isbn13 = strlen((string) $digits) === 13 ? (string) $digits : null; $isbn10 = strlen((string) $digits) === 10 ? (string) $digits : null; $cover = ($row['copertina_url'] !== null && $row['copertina_url'] !== '') ? substr((string) $row['copertina_url'], 0, 255) : null; - $publisherId = $this->findOrCreatePublisher(isset($row['editore']) ? (string) $row['editore'] : null); - - // Only `titolo` is mandatory in `libri`; everything else is optional. - $inserted = $publisherId !== null - ? $this->exec( - 'INSERT INTO libri (titolo, anno_pubblicazione, isbn13, isbn10, copertina_url, editore_id) VALUES (?, ?, ?, ?, ?, ?)', - 'sisssi', - [$titolo, $anno, $isbn13, $isbn10, $cover, $publisherId] - ) - : $this->exec( - 'INSERT INTO libri (titolo, anno_pubblicazione, isbn13, isbn10, copertina_url) VALUES (?, ?, ?, ?, ?)', - 'sisss', - [$titolo, $anno, $isbn13, $isbn10, $cover] + // Reconcile-on-acquire (#138). If this book is ALREADY in the + // catalogue — the manager bought the proposed book and added it + // manually with the same ISBN — link the club entry to that existing + // row instead of inserting a duplicate. Without this, the INSERT + // below hits the UNIQUE isbn13/isbn10 key, the whole transaction + // rolls back, and the user just sees "Acquisition failed, please try + // again." with the proposal stuck as external forever. Matching an + // existing row IS the "status changes once it's in the catalogue" + // behaviour the user expects — one click links it. + $existingId = null; + if ($isbn13 !== null || $isbn10 !== null) { + $existing = $this->row( + 'SELECT id FROM libri + WHERE deleted_at IS NULL + AND ( (isbn13 IS NOT NULL AND isbn13 = ?) + OR (isbn10 IS NOT NULL AND isbn10 = ?) ) + LIMIT 1', + 'ss', + [$isbn13, $isbn10] ); - if (!$inserted) { - throw new \RuntimeException('catalog insert failed'); + $existingId = $existing !== null ? (int) $existing['id'] : null; } - $libroId = (int) $this->db->insert_id; - $this->attachExternalAuthors($libroId, isset($row['autori']) ? (string) $row['autori'] : null); - $this->attachPrimaryPublisher($libroId, $publisherId); - SearchIndexBuilder::rebuild($this->db, $libroId); - - // Give the acquired book one physical copy, matching how the normal - // catalogue-creation flow (LibriController) seeds copies. `libri` - // defaults copie_totali/copie_disponibili to 1, so WITHOUT a matching - // `copie` row the book would claim one available copy but have none - // to lend — and the per-copy features (labels, loan/return by code) - // would find nothing. Inventory number LIB-{id}, same as a single - // manually-created copy; the whole thing is inside this transaction. - if (!$this->exec( - "INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (?, ?, 'disponibile')", - 'is', - [$libroId, 'LIB-' . $libroId] - )) { - throw new \RuntimeException('catalog copy creation failed'); + + if ($existingId !== null) { + // Link to the existing catalogue row: it already has its copies, + // authors and publisher, so create nothing new. + $libroId = $existingId; + } else { + $publisherId = $this->findOrCreatePublisher(isset($row['editore']) ? (string) $row['editore'] : null); + + // Only `titolo` is mandatory in `libri`; everything else is optional. + $inserted = $publisherId !== null + ? $this->exec( + 'INSERT INTO libri (titolo, anno_pubblicazione, isbn13, isbn10, copertina_url, editore_id) VALUES (?, ?, ?, ?, ?, ?)', + 'sisssi', + [$titolo, $anno, $isbn13, $isbn10, $cover, $publisherId] + ) + : $this->exec( + 'INSERT INTO libri (titolo, anno_pubblicazione, isbn13, isbn10, copertina_url) VALUES (?, ?, ?, ?, ?)', + 'sisss', + [$titolo, $anno, $isbn13, $isbn10, $cover] + ); + if (!$inserted) { + throw new \RuntimeException('catalog insert failed'); + } + $libroId = (int) $this->db->insert_id; + $this->attachExternalAuthors($libroId, isset($row['autori']) ? (string) $row['autori'] : null); + $this->attachPrimaryPublisher($libroId, $publisherId); + SearchIndexBuilder::rebuild($this->db, $libroId); + + // Give the acquired book one physical copy, matching how the normal + // catalogue-creation flow (LibriController) seeds copies. `libri` + // defaults copie_totali/copie_disponibili to 1, so WITHOUT a matching + // `copie` row the book would claim one available copy but have none + // to lend — and the per-copy features (labels, loan/return by code) + // would find nothing. Inventory number LIB-{id}, same as a single + // manually-created copy; the whole thing is inside this transaction. + if (!$this->exec( + "INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (?, ?, 'disponibile')", + 'is', + [$libroId, 'LIB-' . $libroId] + )) { + throw new \RuntimeException('catalog copy creation failed'); + } } $bookRows = $this->execAffected( @@ -1072,6 +1101,50 @@ public function clubPolls(int $clubId): array ); } + /** + * Books that were on the ballot in a CLOSED poll but were never the winner + * of any closed poll (#138, Uwe's "proposed but not chosen" archive — a + * club with years of history accumulates a long list). Grouped per book + * with how many closed polls it appeared in and when it last did. A book + * that lost early polls but eventually won is excluded (it was chosen). + * + * @return list> + */ + public function neverChosenProposals(int $clubId): array + { + return $this->rows( + "SELECT cb.id AS club_book_id, + cb.state, + COALESCE(l.titolo, ext.titolo) AS titolo, + COALESCE(l.copertina_url, ext.copertina_url) AS copertina_url, + (cb.external_book_id IS NOT NULL) AS is_external, + cb.libro_id, + COALESCE( + (SELECT GROUP_CONCAT(" . \App\Support\AuthorName::DISPLAY_SQL_A . " SEPARATOR ', ') + FROM libri_autori la JOIN autori a ON a.id = la.autore_id + WHERE la.libro_id = l.id + AND la.ruolo IN ('principale', 'co-autore')), + ext.autori + ) AS autori, + COUNT(DISTINCT p.id) AS times_in_poll, + MAX(COALESCE(p.closed_at, p.closes_at, p.created_at)) AS last_poll_at + FROM bookclub_poll_options o + JOIN bookclub_polls p ON p.id = o.poll_id AND p.club_id = ? AND p.status = 'closed' + JOIN bookclub_books cb ON cb.id = o.club_book_id + LEFT JOIN libri l ON l.id = cb.libro_id AND l.deleted_at IS NULL + LEFT JOIN bookclub_external_books ext ON ext.id = cb.external_book_id + WHERE cb.id NOT IN ( + SELECT winner_club_book_id + FROM bookclub_polls + WHERE club_id = ? AND status = 'closed' AND winner_club_book_id IS NOT NULL + ) + GROUP BY cb.id, cb.state, titolo, copertina_url, is_external, cb.libro_id, autori + ORDER BY last_poll_at DESC", + 'ii', + [$clubId, $clubId] + ); + } + /** * Options with book info and aggregated score. * diff --git a/storage/plugins/book-club/views/public/polls.php b/storage/plugins/book-club/views/public/polls.php index 25515745f..42ec48f17 100644 --- a/storage/plugins/book-club/views/public/polls.php +++ b/storage/plugins/book-club/views/public/polls.php @@ -107,6 +107,35 @@ + +
+
+ +

+
+

+ +
+
+ + + · + +
+ + + · + + + · + +
+
+
+ +
+ +
diff --git a/tests/bookclub-acquire-reconcile-poll-history.unit.php b/tests/bookclub-acquire-reconcile-poll-history.unit.php new file mode 100644 index 000000000..e7e4be3aa --- /dev/null +++ b/tests/bookclub-acquire-reconcile-poll-history.unit.php @@ -0,0 +1,216 @@ += 2 && ($v[0] === '"' || $v[0] === "'") && $v[-1] === $v[0]) { + $v = substr($v, 1, -1); + } + $env[$k] = $v; +} +$socket = getenv('E2E_DB_SOCKET') ?: ($env['DB_SOCKET'] ?? '/opt/homebrew/var/mysql/mysql.sock'); +try { + $db = ($socket !== '' && file_exists($socket)) + ? new mysqli(null, $env['DB_USER'] ?? '', $env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? ''), $env['DB_NAME'] ?? '', 0, $socket) + : new mysqli($env['DB_HOST'] ?? '127.0.0.1', $env['DB_USER'] ?? '', $env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? ''), $env['DB_NAME'] ?? '', (int) ($env['DB_PORT'] ?? 3306)); +} catch (\Throwable $e) { + echo "SKIP: database not reachable (" . $e->getMessage() . ")\n"; + exit(0); +} +$db->set_charset('utf8mb4'); + +$TOKEN = 'zzbc138_' . bin2hex(random_bytes(4)); +$repo = new Repo($db); + +$pass = 0; $fail = 0; +$check = static function (bool $ok, string $label) use (&$pass, &$fail): void { + if ($ok) { $pass++; echo " OK {$label}\n"; } + else { $fail++; echo " FAIL {$label}\n"; } +}; + +$q = static fn (string $sql): mysqli_result|bool => $db->query($sql); +$scalar = static function (string $sql) use ($db) { + $r = $db->query($sql); + $row = $r instanceof mysqli_result ? $r->fetch_row() : null; + return $row[0] ?? null; +}; + +// Track created libri ids so we can clean up whatever the acquire create-path made. +$createdLibri = []; + +try { + // ── Seed a sandbox club ────────────────────────────────────────────────── + $q("INSERT INTO bookclub_clubs (ics_token, name, slug) VALUES ('" . bin2hex(random_bytes(16)) . "', '{$TOKEN} Club', '{$TOKEN}-club')"); + $clubId = (int) $db->insert_id; + + // Existing catalogue books the external proposals will reconcile against. + $isbn13 = '9780000' . random_int(100000, 999999); // 13 digits + $isbn10 = '0000' . random_int(100000, 999999); // 10 digits + $delIsbn13 = '9781111' . random_int(100000, 999999); + + $q("INSERT INTO libri (titolo, isbn13) VALUES ('{$TOKEN} Cat13', '{$isbn13}')"); + $catLibro13 = (int) $db->insert_id; $createdLibri[] = $catLibro13; + $q("INSERT INTO libri (titolo, isbn10) VALUES ('{$TOKEN} Cat10', '{$isbn10}')"); + $catLibro10 = (int) $db->insert_id; $createdLibri[] = $catLibro10; + // A soft-deleted catalogue book whose isbn13 is nulled on delete (per the + // soft-delete rule) — seed it deleted WITH the isbn kept to prove we still + // don't reconcile against deleted_at != NULL rows. + $q("INSERT INTO libri (titolo, isbn13, deleted_at) VALUES ('{$TOKEN} Deleted', '{$delIsbn13}', NOW())"); + $delLibro = (int) $db->insert_id; $createdLibri[] = $delLibro; + + /** Seed an external proposal + its bookclub_books row. Returns [clubBookId, extId]. */ + $seedExternal = static function (string $title, ?string $isbn) use ($db, $clubId): array { + $isbnSql = $isbn === null ? 'NULL' : "'" . $db->real_escape_string($isbn) . "'"; + $db->query("INSERT INTO bookclub_external_books (club_id, titolo, isbn) VALUES ({$clubId}, '" . $db->real_escape_string($title) . "', {$isbnSql})"); + $extId = (int) $db->insert_id; + $db->query("INSERT INTO bookclub_books (club_id, external_book_id, state) VALUES ({$clubId}, {$extId}, 'proposed')"); + return [(int) $db->insert_id, $extId]; + }; + + echo "A. acquireExternalBook — reconcile-on-acquire (#138 bug)\n"; + + // A1: isbn13 matches an existing catalogue book → link, no new libri row. + [$cb1, $ext1] = $seedExternal("{$TOKEN} Prop A1", $isbn13); + $libriBefore = (int) $scalar('SELECT COUNT(*) FROM libri'); + $res1 = $repo->acquireExternalBook($cb1); + $libriAfter = (int) $scalar('SELECT COUNT(*) FROM libri'); + $check($res1 === $catLibro13, 'A1 isbn13 match returns the EXISTING catalogue libro id (no duplicate)'); + $check($libriAfter === $libriBefore, 'A1 no new libri row inserted (reconciled, not duplicated)'); + $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cb1}") === $catLibro13, 'A1 club book repointed to the existing libro'); + $check($scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cb1}") === null, 'A1 club book external_book_id cleared'); + $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$ext1}") === $catLibro13, 'A1 external record acquired_libro_id set to existing'); + + // A2: isbn10 match → link. + [$cb2] = $seedExternal("{$TOKEN} Prop A2", $isbn10); + $res2 = $repo->acquireExternalBook($cb2); + $check($res2 === $catLibro10, 'A2 isbn10 match returns the existing catalogue libro id'); + + // A3: no ISBN → creates a NEW libri row. + [$cb3] = $seedExternal("{$TOKEN} Prop A3 NoIsbn", null); + $res3 = $repo->acquireExternalBook($cb3); + $check(is_int($res3) && $res3 > 0 && !in_array($res3, [$catLibro13, $catLibro10, $delLibro], true), 'A3 no-ISBN proposal creates a NEW libro'); + if (is_int($res3)) { $createdLibri[] = $res3; } + $check((int) $scalar("SELECT COUNT(*) FROM copie WHERE libro_id={$res3}") === 1, 'A3 created libro gets one physical copy'); + + // A4 (edge): the reconcile query's `deleted_at IS NULL` guard. The + // soft-deleted book keeps its isbn13 here so the query WOULD match it + // without the guard — proving the guard is what keeps acquire from linking + // to a deleted row (a live book with that isbn is the only valid target). + [$cb4] = $seedExternal("{$TOKEN} Prop A4 Deleted", $delIsbn13); + $res4 = $repo->acquireExternalBook($cb4); + $check($res4 !== $delLibro, 'A4 does NOT reconcile to a soft-deleted book (deleted_at IS NULL guard)'); + if (is_int($res4)) { $createdLibri[] = $res4; } + + // A5 (edge): ISBN matches nothing → creates new. + [$cb5] = $seedExternal("{$TOKEN} Prop A5", '9782222' . random_int(100000, 999999)); + $res5 = $repo->acquireExternalBook($cb5); + $check(is_int($res5) && !in_array($res5, [$catLibro13, $catLibro10, $delLibro], true), 'A5 non-matching ISBN creates a new libro'); + if (is_int($res5)) { $createdLibri[] = $res5; } + + echo "B. neverChosenProposals — proposed-but-not-chosen archive (#138 feature)\n"; + + // Seed club books (catalogue-side, simplest) to be poll options. + $mkClubBook = static function (string $title) use ($db, $clubId, &$createdLibri): int { + $db->query("INSERT INTO libri (titolo) VALUES ('" . $db->real_escape_string($title) . "')"); + $libroId = (int) $db->insert_id; $createdLibri[] = $libroId; + $db->query("INSERT INTO bookclub_books (club_id, libro_id, state) VALUES ({$clubId}, {$libroId}, 'proposed')"); + return (int) $db->insert_id; + }; + $bLoser = $mkClubBook("{$TOKEN} Loser"); // in closed poll, never wins + $bWinner = $mkClubBook("{$TOKEN} Winner"); // wins a closed poll + $bLateWin = $mkClubBook("{$TOKEN} LateWinner"); // loses one, wins a later one + $bOpen = $mkClubBook("{$TOKEN} OpenOnly"); // only in an OPEN poll + $bTwice = $mkClubBook("{$TOKEN} TwiceLoser"); // loses two closed polls + + $mkPoll = static function (string $status, ?int $winner) use ($db, $clubId): int { + $w = $winner === null ? 'NULL' : (string) $winner; + $db->query("INSERT INTO bookclub_polls (club_id, title, mode, status, winner_club_book_id, created_at) VALUES ({$clubId}, 'P', 'simple', '{$status}', {$w}, NOW())"); + return (int) $db->insert_id; + }; + $opt = static function (int $pollId, int $clubBookId) use ($db): void { + $db->query("INSERT INTO bookclub_poll_options (poll_id, club_book_id) VALUES ({$pollId}, {$clubBookId})"); + }; + + // Closed poll 1: winner=bWinner; options: bWinner, bLoser, bLateWin, bTwice. + $p1 = $mkPoll('closed', $bWinner); + $opt($p1, $bWinner); $opt($p1, $bLoser); $opt($p1, $bLateWin); $opt($p1, $bTwice); + // Closed poll 2: winner=bLateWin; options: bLateWin, bTwice. + $p2 = $mkPoll('closed', $bLateWin); + $opt($p2, $bLateWin); $opt($p2, $bTwice); + // Open poll: options bOpen (must NOT count). + $p3 = $mkPoll('open', null); + $opt($p3, $bOpen); $opt($p3, $bLoser); + + $never = $repo->neverChosenProposals($clubId); + $byId = []; + foreach ($never as $row) { $byId[(int) $row['club_book_id']] = $row; } + + $check(isset($byId[$bLoser]), 'B1 a book that lost a closed poll and never won appears'); + $check(!isset($byId[$bWinner]), 'B2 the winner is excluded'); + $check(!isset($byId[$bLateWin]), 'B3 a book that lost early but won later is excluded'); + $check(!isset($byId[$bOpen]), 'B4 a book only in an OPEN poll is excluded'); + $check(isset($byId[$bTwice]), 'B5 a two-time loser appears'); + $check(isset($byId[$bTwice]) && (int) $byId[$bTwice]['times_in_poll'] === 2, 'B6 two-time loser counts times_in_poll=2 (deduped per book)'); + $check(isset($byId[$bLoser]) && (int) $byId[$bLoser]['times_in_poll'] === 1, 'B7 one-time loser counts times_in_poll=1 (open poll not counted)'); + + // B8 (edge): a club with no closed polls yields nothing. + $q("INSERT INTO bookclub_clubs (ics_token, name, slug) VALUES ('" . bin2hex(random_bytes(16)) . "', '{$TOKEN} Empty', '{$TOKEN}-empty')"); + $emptyClub = (int) $db->insert_id; + $check($repo->neverChosenProposals($emptyClub) === [], 'B8 a club with no closed polls returns an empty archive'); +} finally { + // ── Cleanup (FK-safe) ──────────────────────────────────────────────────── + $db->query("DELETE FROM bookclub_poll_options WHERE poll_id IN (SELECT id FROM bookclub_polls WHERE club_id IN (SELECT id FROM bookclub_clubs WHERE slug LIKE '{$TOKEN}-%'))"); + $db->query("DELETE FROM bookclub_polls WHERE club_id IN (SELECT id FROM bookclub_clubs WHERE slug LIKE '{$TOKEN}-%')"); + $db->query("DELETE FROM bookclub_books WHERE club_id IN (SELECT id FROM bookclub_clubs WHERE slug LIKE '{$TOKEN}-%')"); + $db->query("DELETE FROM bookclub_external_books WHERE club_id IN (SELECT id FROM bookclub_clubs WHERE slug LIKE '{$TOKEN}-%')"); + $db->query("DELETE FROM bookclub_clubs WHERE slug LIKE '{$TOKEN}-%'"); + if ($createdLibri !== []) { + $ids = implode(',', array_map('intval', array_unique($createdLibri))); + $db->query("DELETE FROM copie WHERE libro_id IN ({$ids})"); + $db->query("DELETE FROM libri_autori WHERE libro_id IN ({$ids})"); + $db->query("DELETE FROM libri_editori WHERE libro_id IN ({$ids})"); + $db->query("DELETE FROM libri WHERE id IN ({$ids})"); + } + // Orphan authors/publishers seeded by the create path (name-tokened). + $db->query("DELETE FROM autori WHERE nome LIKE '{$TOKEN}%'"); + $db->query("DELETE FROM editori WHERE nome LIKE '{$TOKEN}%'"); + $db->close(); +} + +echo "\nPassed: {$pass} Failed: {$fail}\n"; +exit($fail > 0 ? 1 : 0); From a1c47233f42780de77cb55b19c994d26b3fb7e72 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 22 Jul 2026 07:55:43 +0200 Subject: [PATCH 02/10] feat(book-club #138): automatic reconciliation on club render + closed-polls accordion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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
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. --- locale/de_DE.json | 6 +- locale/en_US.json | 6 +- locale/fr_FR.json | 6 +- locale/it_IT.json | 6 +- .../plugins/book-club/src/PollController.php | 7 +- .../book-club/src/PublicController.php | 11 + storage/plugins/book-club/src/Repo.php | 203 +++++++++++++----- .../plugins/book-club/views/public/polls.php | 56 ++++- .../plugins/book-club/views/public/show.php | 44 +++- tests/book-club-uwe.spec.js | 41 +++- ...ub-acquire-reconcile-poll-history.unit.php | 132 ++++++++---- 11 files changed, 395 insertions(+), 123 deletions(-) diff --git a/locale/de_DE.json b/locale/de_DE.json index 27061c2b0..be4a14ed0 100644 --- a/locale/de_DE.json +++ b/locale/de_DE.json @@ -6579,9 +6579,11 @@ "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.": "Sind Sie sicher, dass Sie dieses Genre und alle seine Untergenres löschen möchten? Zugeordnete Bücher werden von diesen Genres getrennt.", "Elimina genere e sottogeneri": "Genre und Untergenres löschen", "Proposte non selezionate": "Nicht ausgewählte Vorschläge", - "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.": "Bücher, die in einer geschlossenen Abstimmung zur Wahl standen, aber nie ausgewählt wurden.", + "Proposte ancora in attesa di essere scelte. La lista include i libri mai votati e quelli che non hanno mai vinto una votazione chiusa.": "Vorschläge, die noch auf eine Auswahl warten. Dazu gehören Bücher, über die noch nie abgestimmt wurde, und Bücher, die noch nie eine abgeschlossene Abstimmung gewonnen haben.", "votazione": "Abstimmung", "votazioni": "Abstimmungen", "ultima": "zuletzt", - "Non in catalogo": "Nicht im Katalog" + "Non in catalogo": "Nicht im Katalog", + "Votazioni chiuse": "Geschlossene Abstimmungen", + "mai votata": "noch nicht abgestimmt" } diff --git a/locale/en_US.json b/locale/en_US.json index 82075a92d..128160fc4 100644 --- a/locale/en_US.json +++ b/locale/en_US.json @@ -6579,9 +6579,11 @@ "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.": "Are you sure you want to delete this genre and all of its subgenres? Associated books will be unlinked from these genres.", "Elimina genere e sottogeneri": "Delete genre and subgenres", "Proposte non selezionate": "Proposals not selected", - "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.": "Books that were on the ballot in a closed poll but were never chosen.", + "Proposte ancora in attesa di essere scelte. La lista include i libri mai votati e quelli che non hanno mai vinto una votazione chiusa.": "Proposals still awaiting selection. The list includes books not yet voted on and books that have never won a closed poll.", "votazione": "poll", "votazioni": "polls", "ultima": "last", - "Non in catalogo": "Not in catalogue" + "Non in catalogo": "Not in catalogue", + "Votazioni chiuse": "Closed polls", + "mai votata": "not yet voted on" } diff --git a/locale/fr_FR.json b/locale/fr_FR.json index a979eaab1..0a3ee2524 100644 --- a/locale/fr_FR.json +++ b/locale/fr_FR.json @@ -6579,9 +6579,11 @@ "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.": "Voulez-vous vraiment supprimer ce genre et tous ses sous-genres ? Les livres associés seront dissociés de ces genres.", "Elimina genere e sottogeneri": "Supprimer le genre et les sous-genres", "Proposte non selezionate": "Propositions non retenues", - "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.": "Livres qui ont été candidats dans un vote clos mais qui n'ont jamais été retenus.", + "Proposte ancora in attesa di essere scelte. La lista include i libri mai votati e quelli che non hanno mai vinto una votazione chiusa.": "Propositions encore en attente de sélection. La liste comprend les livres jamais soumis au vote et ceux qui n'ont jamais remporté un vote clos.", "votazione": "vote", "votazioni": "votes", "ultima": "dernière", - "Non in catalogo": "Pas au catalogue" + "Non in catalogo": "Pas au catalogue", + "Votazioni chiuse": "Votes clos", + "mai votata": "pas encore soumise au vote" } diff --git a/locale/it_IT.json b/locale/it_IT.json index f9b714977..57ed32c56 100644 --- a/locale/it_IT.json +++ b/locale/it_IT.json @@ -6579,9 +6579,11 @@ "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.": "Sei sicuro di voler eliminare questo genere e tutti i suoi sottogeneri? I libri associati verranno scollegati da questi generi.", "Elimina genere e sottogeneri": "Elimina genere e sottogeneri", "Proposte non selezionate": "Proposte non selezionate", - "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.": "Libri che sono stati candidati in una votazione chiusa ma non sono mai stati scelti.", + "Proposte ancora in attesa di essere scelte. La lista include i libri mai votati e quelli che non hanno mai vinto una votazione chiusa.": "Proposte ancora in attesa di essere scelte. La lista include i libri mai votati e quelli che non hanno mai vinto una votazione chiusa.", "votazione": "votazione", "votazioni": "votazioni", "ultima": "ultima", - "Non in catalogo": "Non in catalogo" + "Non in catalogo": "Non in catalogo", + "Votazioni chiuse": "Votazioni chiuse", + "mai votata": "mai votata" } diff --git a/storage/plugins/book-club/src/PollController.php b/storage/plugins/book-club/src/PollController.php index bead2a748..1728db099 100644 --- a/storage/plugins/book-club/src/PollController.php +++ b/storage/plugins/book-club/src/PollController.php @@ -136,13 +136,14 @@ public function pollsPage(ServerRequestInterface $request, ResponseInterface $re $this->closeExpiredForClub((int) $club['id']); $books = $this->repo->clubBooks((int) $club['id']); $eligible = $this->repo->pollEligibleBooks($club, $books); + $entryState = Repo::entryStateKey($this->repo->workflowStates($club)); return $this->renderPublic($response, 'public/polls', [ 'club' => $club, 'polls' => $this->repo->clubPolls((int) $club['id']), 'eligible' => $eligible, - // Books that lost every poll they were in (#138) — the "proposed but - // never chosen" archive members asked to browse. - 'neverChosen' => $this->repo->neverChosenProposals((int) $club['id']), + // Proposals still awaiting selection, including pre-Pinakes history + // entered without fabricating old polls (#138). + 'neverChosen' => $this->repo->neverChosenProposals((int) $club['id'], $entryState), 'isMember' => $this->isActiveMember($club), 'canManage' => $this->canManage($club), 'canCreate' => $this->can($club, 'polls.create'), diff --git a/storage/plugins/book-club/src/PublicController.php b/storage/plugins/book-club/src/PublicController.php index 4fa4c1398..816bd7838 100644 --- a/storage/plugins/book-club/src/PublicController.php +++ b/storage/plugins/book-club/src/PublicController.php @@ -40,6 +40,17 @@ public function show(ServerRequestInterface $request, ResponseInterface $respons $canManage = $this->canManage($club); $isMember = $membership !== null && $membership['status'] === 'active'; + // Keep the public list consistent with the ballot endpoint and the + // mobile API even when the maintenance cron has not run yet. + (new PollController($this->db, $this->repo))->closeExpiredForClub((int) $club['id']); + + // A manager may have entered an externally proposed book through the + // normal catalogue flow since the previous visit. Link exact ISBN + // matches before rendering; this is idempotent and never creates books. + if ($canManage) { + $this->repo->reconcileExternalBooksWithCatalogue((int) $club['id']); + } + $books = $this->repo->clubBooks((int) $club['id']); // Proposals awaiting moderation are manager-only. if (!$canManage) { diff --git a/storage/plugins/book-club/src/Repo.php b/storage/plugins/book-club/src/Repo.php index 940969b3e..f67544492 100644 --- a/storage/plugins/book-club/src/Repo.php +++ b/storage/plugins/book-club/src/Repo.php @@ -825,6 +825,130 @@ public function proposeExternalBook(int $clubId, array $data, string $state, ?in return (int) $this->db->insert_id; } + /** + * Resolve an active catalogue row by its normalized ISBN. The row lock is + * important: once acquisition decides to reuse a book, a concurrent soft + * delete must not turn that link into an immediately hidden club entry. + */ + private function activeCatalogueBookIdByIsbn(?string $isbn13, ?string $isbn10): ?int + { + if ($isbn13 === null && $isbn10 === null) { + return null; + } + $row = $this->row( + 'SELECT id FROM libri + WHERE deleted_at IS NULL + AND ((isbn13 IS NOT NULL AND isbn13 = ?) + OR (isbn10 IS NOT NULL AND isbn10 = ?)) + LIMIT 1 + FOR UPDATE', + 'ss', + [$isbn13, $isbn10] + ); + return $row !== null ? (int) $row['id'] : null; + } + + /** @return array{0:?string,1:?string} */ + private function normalizedIsbnParts(?string $isbn): array + { + $digits = $isbn !== null + ? strtoupper((string) preg_replace('/[^0-9Xx]/', '', $isbn)) + : ''; + return [ + strlen($digits) === 13 ? $digits : null, + strlen($digits) === 10 ? $digits : null, + ]; + } + + /** + * Repoint one external club-book to an existing catalogue book. Callers + * hold a transaction and the external rows' locks. Refuse to merge two + * club-book records: poll options, votes and reading history may already + * refer to either id, so silently choosing one would lose information. + */ + private function linkExternalBookToCatalogue(int $clubId, int $clubBookId, int $externalId, int $libroId): bool + { + $duplicate = $this->row( + 'SELECT id FROM bookclub_books + WHERE club_id = ? AND libro_id = ? AND id <> ? + LIMIT 1 + FOR UPDATE', + 'iii', + [$clubId, $libroId, $clubBookId] + ); + if ($duplicate !== null) { + return false; + } + + $bookRows = $this->execAffected( + 'UPDATE bookclub_books SET libro_id = ?, external_book_id = NULL WHERE id = ? AND external_book_id = ?', + 'iii', + [$libroId, $clubBookId, $externalId] + ); + if ($bookRows !== 1) { + throw new \RuntimeException('catalog acquisition club-book repoint failed'); + } + $externalRows = $this->execAffected( + 'UPDATE bookclub_external_books SET acquired_libro_id = ? WHERE id = ? AND acquired_libro_id IS NULL', + 'ii', + [$libroId, $externalId] + ); + if ($externalRows !== 1) { + throw new \RuntimeException('catalog acquisition external stamp failed'); + } + return true; + } + + /** + * Reconcile external proposals after their book has been entered manually + * in the catalogue. This never creates catalogue data: it only links an + * exact normalized ISBN-10/ISBN-13 match, and is therefore safe to run when + * a manager opens the club page. Returns the number of proposals linked. + */ + public function reconcileExternalBooksWithCatalogue(int $clubId): int + { + $this->db->begin_transaction(); + try { + $candidates = $this->rows( + "SELECT cb.id AS club_book_id, cb.external_book_id, ext.isbn + FROM bookclub_books cb + JOIN bookclub_external_books ext ON ext.id = cb.external_book_id + WHERE cb.club_id = ? + AND cb.external_book_id IS NOT NULL + AND ext.acquired_libro_id IS NULL + AND ext.isbn IS NOT NULL + AND TRIM(ext.isbn) <> '' + FOR UPDATE", + 'i', + [$clubId] + ); + + $reconciled = 0; + foreach ($candidates as $candidate) { + [$isbn13, $isbn10] = $this->normalizedIsbnParts((string) $candidate['isbn']); + $libroId = $this->activeCatalogueBookIdByIsbn($isbn13, $isbn10); + if ($libroId === null) { + continue; + } + if ($this->linkExternalBookToCatalogue( + $clubId, + (int) $candidate['club_book_id'], + (int) $candidate['external_book_id'], + $libroId + )) { + $reconciled++; + } + } + + $this->db->commit(); + return $reconciled; + } catch (\Throwable $e) { + $this->db->rollback(); + SecureLogger::error('[BookClub] automatic external reconciliation rolled back: ' . $e->getMessage()); + return 0; + } + } + /** * Acquire an external proposal into the catalogue: create the real `libri` * row from its metadata (this is the ONLY moment the book enters the @@ -836,7 +960,7 @@ public function acquireExternalBook(int $clubBookId): ?int $this->db->begin_transaction(); try { $row = $this->row( - 'SELECT cb.external_book_id, ext.titolo, ext.autori, ext.isbn, ext.anno, ext.editore, ext.copertina_url + 'SELECT cb.club_id, cb.external_book_id, ext.titolo, ext.autori, ext.isbn, ext.anno, ext.editore, ext.copertina_url FROM bookclub_books cb JOIN bookclub_external_books ext ON ext.id = cb.external_book_id WHERE cb.id = ? AND cb.external_book_id IS NOT NULL @@ -851,32 +975,13 @@ public function acquireExternalBook(int $clubBookId): ?int $extId = (int) $row['external_book_id']; $titolo = (string) $row['titolo']; $anno = ($row['anno'] !== null && $row['anno'] !== '') ? (int) $row['anno'] : null; - $digits = $row['isbn'] !== null ? preg_replace('/[^0-9Xx]/', '', (string) $row['isbn']) : ''; - $isbn13 = strlen((string) $digits) === 13 ? (string) $digits : null; - $isbn10 = strlen((string) $digits) === 10 ? (string) $digits : null; + [$isbn13, $isbn10] = $this->normalizedIsbnParts( + $row['isbn'] !== null ? (string) $row['isbn'] : null + ); $cover = ($row['copertina_url'] !== null && $row['copertina_url'] !== '') ? substr((string) $row['copertina_url'], 0, 255) : null; - // Reconcile-on-acquire (#138). If this book is ALREADY in the - // catalogue — the manager bought the proposed book and added it - // manually with the same ISBN — link the club entry to that existing - // row instead of inserting a duplicate. Without this, the INSERT - // below hits the UNIQUE isbn13/isbn10 key, the whole transaction - // rolls back, and the user just sees "Acquisition failed, please try - // again." with the proposal stuck as external forever. Matching an - // existing row IS the "status changes once it's in the catalogue" - // behaviour the user expects — one click links it. - $existingId = null; - if ($isbn13 !== null || $isbn10 !== null) { - $existing = $this->row( - 'SELECT id FROM libri - WHERE deleted_at IS NULL - AND ( (isbn13 IS NOT NULL AND isbn13 = ?) - OR (isbn10 IS NOT NULL AND isbn10 = ?) ) - LIMIT 1', - 'ss', - [$isbn13, $isbn10] - ); - $existingId = $existing !== null ? (int) $existing['id'] : null; - } + // If the manager already entered the purchase in Pinakes, reuse the + // active ISBN match instead of violating libri's unique ISBN keys. + $existingId = $this->activeCatalogueBookIdByIsbn($isbn13, $isbn10); if ($existingId !== null) { // Link to the existing catalogue row: it already has its copies, @@ -921,17 +1026,7 @@ public function acquireExternalBook(int $clubBookId): ?int } } - $bookRows = $this->execAffected( - 'UPDATE bookclub_books SET libro_id = ?, external_book_id = NULL WHERE id = ? AND external_book_id = ?', - 'iii', - [$libroId, $clubBookId, $extId] - ); - $externalRows = $this->execAffected( - 'UPDATE bookclub_external_books SET acquired_libro_id = ? WHERE id = ? AND acquired_libro_id IS NULL', - 'ii', - [$libroId, $extId] - ); - if ($bookRows !== 1 || $externalRows !== 1) { + if (!$this->linkExternalBookToCatalogue((int) $row['club_id'], $clubBookId, $extId, $libroId)) { throw new \RuntimeException('catalog acquisition repoint failed'); } @@ -1102,19 +1197,19 @@ public function clubPolls(int $clubId): array } /** - * Books that were on the ballot in a CLOSED poll but were never the winner - * of any closed poll (#138, Uwe's "proposed but not chosen" archive — a - * club with years of history accumulates a long list). Grouped per book - * with how many closed polls it appeared in and when it last did. A book - * that lost early polls but eventually won is excluded (it was chosen). + * Proposals still in the workflow entry state that never won a closed poll. + * This includes both recorded poll losers and older/pre-Pinakes proposals + * entered without a synthetic poll. Open-poll books are normally in the + * voting state and therefore stay out until their poll closes. * * @return list> */ - public function neverChosenProposals(int $clubId): array + public function neverChosenProposals(int $clubId, string $entryState): array { return $this->rows( "SELECT cb.id AS club_book_id, cb.state, + cb.created_at AS proposed_at, COALESCE(l.titolo, ext.titolo) AS titolo, COALESCE(l.copertina_url, ext.copertina_url) AS copertina_url, (cb.external_book_id IS NOT NULL) AS is_external, @@ -1128,20 +1223,26 @@ public function neverChosenProposals(int $clubId): array ) AS autori, COUNT(DISTINCT p.id) AS times_in_poll, MAX(COALESCE(p.closed_at, p.closes_at, p.created_at)) AS last_poll_at - FROM bookclub_poll_options o - JOIN bookclub_polls p ON p.id = o.poll_id AND p.club_id = ? AND p.status = 'closed' - JOIN bookclub_books cb ON cb.id = o.club_book_id + FROM bookclub_books cb + LEFT JOIN bookclub_poll_options o ON o.club_book_id = cb.id + LEFT JOIN bookclub_polls p ON p.id = o.poll_id AND p.club_id = ? AND p.status = 'closed' LEFT JOIN libri l ON l.id = cb.libro_id AND l.deleted_at IS NULL LEFT JOIN bookclub_external_books ext ON ext.id = cb.external_book_id - WHERE cb.id NOT IN ( + WHERE cb.club_id = ? + AND cb.state = ? + AND (l.id IS NOT NULL OR cb.external_book_id IS NOT NULL) + AND cb.id NOT IN ( SELECT winner_club_book_id FROM bookclub_polls WHERE club_id = ? AND status = 'closed' AND winner_club_book_id IS NOT NULL ) - GROUP BY cb.id, cb.state, titolo, copertina_url, is_external, cb.libro_id, autori - ORDER BY last_poll_at DESC", - 'ii', - [$clubId, $clubId] + GROUP BY cb.id, cb.state, cb.created_at, titolo, copertina_url, is_external, cb.libro_id, autori + ORDER BY COALESCE( + MAX(COALESCE(p.closed_at, p.closes_at, p.created_at)), + cb.created_at + ) DESC", + 'iisi', + [$clubId, $clubId, $entryState, $clubId] ); } diff --git a/storage/plugins/book-club/views/public/polls.php b/storage/plugins/book-club/views/public/polls.php index 42ec48f17..9e0b5b659 100644 --- a/storage/plugins/book-club/views/public/polls.php +++ b/storage/plugins/book-club/views/public/polls.php @@ -8,6 +8,7 @@ * @var array $club * @var list> $polls * @var list> $eligible proposals usable as options + * @var list> $neverChosen * @var bool $isMember * @var bool $canManage club managers (kept for non-creation UI) * @var bool|null $canCreate granular polls.create permission → creation form @@ -19,6 +20,14 @@ $slug = (string) $club['slug']; $csrf = \App\Support\Csrf::ensureToken(); $canCreate = $canCreate ?? $canManage; +$openPolls = array_values(array_filter( + $polls, + static fn(array $poll): bool => ($poll['status'] ?? '') === 'open' +)); +$closedPolls = array_values(array_filter( + $polls, + static fn(array $poll): bool => ($poll['status'] ?? '') === 'closed' +)); $modeLabels = [ 'simple' => __('Voto singolo'), 'multi' => __('Preferenza multipla'), @@ -79,10 +88,10 @@

- -

+ +

- +
@@ -96,15 +105,38 @@ · - · + ·
- - - +
+ + +
+ () +
+ +
+
+ +
+ + · + + · + + · + +
+
+ +
+ +
+
+ @@ -113,7 +145,7 @@

-

+

@@ -122,12 +154,16 @@ ·
- + + + + + · - · + ·
diff --git a/storage/plugins/book-club/views/public/show.php b/storage/plugins/book-club/views/public/show.php index a9dc7293c..1a0c934de 100644 --- a/storage/plugins/book-club/views/public/show.php +++ b/storage/plugins/book-club/views/public/show.php @@ -31,6 +31,14 @@ $booksByState[(string) $book['state']][] = $book; } $pendingProposals = $booksByState[\App\Plugins\BookClub\BookClubPlugin::STATE_PENDING] ?? []; +$openPolls = array_values(array_filter( + $polls, + static fn(array $poll): bool => ($poll['status'] ?? '') === 'open' +)); +$closedPolls = array_values(array_filter( + $polls, + static fn(array $poll): bool => ($poll['status'] ?? '') === 'closed' +)); // Members always get the tokenized feed URL: the token proves membership // and unlocks the members-only fields (e.g. video-conference links) that // the anonymous public-club feed omits. @@ -369,26 +377,48 @@ class="form-control mb-3">

- -

+ +

- +
- · + ·
- - - +
+ +
+ () +
+ +
+
+ +
+ + + · + + · + +
+
+ +
+ +
+
+ + { expect(dbQuery(`SELECT COUNT(*) FROM libri WHERE titolo=${sqlStr(EXT_TITLE)}`)).toBe('0'); expect(dbQuery(`SELECT COUNT(*) FROM libri WHERE titolo=${sqlStr(EXT_TITLE_2)}`)).toBe('0'); - async function proposeExternal(title, author, publisher = '') { + async function proposeExternal(title, author, publisher = '', isbn = '') { await page.goto(`${BASE}/book-club/${slug}`); await page.waitForLoadState('domcontentloaded'); const ext = page.locator('details.bc-external-propose'); @@ -238,6 +239,9 @@ test.describe.serial('Book Club — Uwe feedback', () => { if (publisher !== '') { await ext.locator('input[name="ext_editore"]').fill(publisher); } + if (isbn !== '') { + await ext.locator('input[name="ext_isbn"]').fill(isbn); + } await ext.locator('button:has-text("Proponi libro esterno")').click(); await page.waitForLoadState('networkidle'); @@ -258,7 +262,7 @@ test.describe.serial('Book Club — Uwe feedback', () => { // They land in the plugin tables, NOT in libri (the whole point of ①). const first = await proposeExternal(EXT_TITLE, `${EXT_AUTHOR_1}; ${EXT_AUTHOR_2}`, EXT_PUBLISHER); - const second = await proposeExternal(EXT_TITLE_2, 'John External'); + const second = await proposeExternal(EXT_TITLE_2, 'John External', '', AUTO_ISBN); // It renders with the "Proposta esterna" badge. await expect(page.locator('.bc-badge', { hasText: 'esterna' }).first()).toBeVisible(); @@ -280,9 +284,24 @@ test.describe.serial('Book Club — Uwe feedback', () => { expect(dbQuery(`SELECT COUNT(*) FROM libri WHERE titolo IN (${sqlStr(EXT_TITLE)}, ${sqlStr(EXT_TITLE_2)})`), 'opening a poll with external books must not create catalog rows').toBe('0'); - // Acquire it into the catalogue (manager action) — the one moment it enters libri. + // Reproduce Uwe's failure exactly: the manager later records the purchase + // through the normal catalogue side, then revisits the club. The GET must + // link the proposal by exact ISBN without requiring the Acquire button and + // without duplicating the catalogue row. + dbQuery(`INSERT INTO libri (titolo, isbn13) VALUES (${sqlStr(EXT_TITLE_2)}, ${sqlStr(AUTO_ISBN)})`); + const manuallyAddedLibroId = Number(dbQuery(`SELECT id FROM libri WHERE isbn13=${sqlStr(AUTO_ISBN)} LIMIT 1`) || '0'); + expect(manuallyAddedLibroId).toBeGreaterThan(0); + dbQuery(`INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (${manuallyAddedLibroId}, ${sqlStr(`LIB-${manuallyAddedLibroId}`)}, 'disponibile')`); await page.goto(`${BASE}/book-club/${slug}`); - await page.waitForLoadState('domcontentloaded'); + await page.waitForLoadState('networkidle'); + await expect.poll(() => dbQuery( + `SELECT CONCAT(IFNULL(libro_id,'NULL'),'|',IFNULL(external_book_id,'NULL')) FROM bookclub_books WHERE id=${second.clubBookId}` + )).toBe(`${manuallyAddedLibroId}|NULL`); + expect(dbQuery(`SELECT acquired_libro_id FROM bookclub_external_books WHERE id=${second.extBookId}`)).toBe(String(manuallyAddedLibroId)); + expect(dbQuery(`SELECT COUNT(*) FROM libri WHERE isbn13=${sqlStr(AUTO_ISBN)}`), + 'automatic reconciliation must not duplicate the manually added catalogue row').toBe('1'); + + // Acquire it into the catalogue (manager action) — the one moment it enters libri. await page.locator(`form[action*="/books/${first.clubBookId}/acquire"] button`).click(); await page.waitForLoadState('networkidle'); @@ -314,7 +333,19 @@ test.describe.serial('Book Club — Uwe feedback', () => { expect(searchIndex).toContain(EXT_AUTHOR_1); expect(searchIndex).toContain(EXT_PUBLISHER); expect(dbQuery(`SELECT COUNT(*) FROM libri WHERE titolo=${sqlStr(EXT_TITLE_2)}`), - 'the other external poll option is still not a catalog book').toBe('0'); + 'the automatically reconciled poll option has exactly one catalogue row').toBe('1'); + + // The main club page must also lazily close expired polls (cron is not a + // correctness dependency) and expose them in its explicit history. + const pollId = Number(dbQuery(`SELECT id FROM bookclub_polls WHERE club_id=${clubId} AND title=${sqlStr(pollTitle)} LIMIT 1`) || '0'); + expect(pollId).toBeGreaterThan(0); + dbQuery(`UPDATE bookclub_polls SET closes_at=DATE_SUB(NOW(), INTERVAL 1 MINUTE), status='open' WHERE id=${pollId}`); + await page.goto(`${BASE}/book-club/${slug}`); + await page.waitForLoadState('networkidle'); + expect(dbQuery(`SELECT status FROM bookclub_polls WHERE id=${pollId}`)).toBe('closed'); + const closedHistory = page.locator('details:has-text("Votazioni chiuse")'); + await closedHistory.locator('summary').click(); + await expect(closedHistory.locator(`a[href$="/polls/${pollId}"]`)).toBeVisible(); }); test('⑥ Uwe #138 follow-up: heading, proposed-by, remove, PDF, and next-meeting card fields', async () => { diff --git a/tests/bookclub-acquire-reconcile-poll-history.unit.php b/tests/bookclub-acquire-reconcile-poll-history.unit.php index e7e4be3aa..8aa0f056d 100644 --- a/tests/bookclub-acquire-reconcile-poll-history.unit.php +++ b/tests/bookclub-acquire-reconcile-poll-history.unit.php @@ -4,18 +4,17 @@ /** * Behavioural coverage for the #138 (Uwe round 3) book-club fixes: * - * A. Repo::acquireExternalBook() reconcile-on-acquire — when a proposed - * external book is already in the catalogue (same ISBN, because the manager - * bought it and added it manually), "Add to catalogue" must LINK to the - * existing row instead of inserting a duplicate that violates libri's UNIQUE - * isbn13/isbn10 key (the "Acquisition failed" bug). Edge cases: isbn13 match, - * isbn10 match, no-ISBN → creates new, ISBN matching only a SOFT-DELETED row - * → creates new (never reuse a deleted book). + * A. External-book reconciliation — when a proposed external book is already + * in the catalogue (same ISBN, because the manager bought it and added it + * manually), the manager's next club visit links it automatically. The + * explicit "Add to catalogue" fallback also reuses an existing match. + * Edge cases: isbn13/isbn10, idempotency, no match, a duplicate club entry, + * no-ISBN create, and an ISBN still owned by a SOFT-DELETED row. * * B. Repo::neverChosenProposals() — the "proposed but never chosen" archive. - * Edge cases: excluded once it wins a later closed poll; open polls don't - * count; a book in two closed polls appears once with times_in_poll=2; the - * winner is excluded; a club with no closed polls yields nothing. + * Edge cases: excluded once it wins a later closed poll; books currently in + * voting stay out; an entry-state proposal with no recorded poll is included + * for pre-Pinakes history; repeated losses are counted once per poll. * * Runs against the LIVE local MySQL, seeds rows under a unique token, and * cleans everything up (FK-safe) at the end. @@ -26,9 +25,11 @@ $root = dirname(__DIR__); mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT); require $root . '/vendor/autoload.php'; -require_once $root . '/storage/plugins/book-club/src/Repo.php'; +require_once $root . '/storage/plugins/book-club/BookClubPlugin.php'; +use App\Plugins\BookClub\BookClubPlugin; use App\Plugins\BookClub\Repo; +use App\Support\HookManager; $env = []; foreach (preg_split('/\r?\n/', (string) @file_get_contents($root . '/.env')) as $line) { @@ -44,16 +45,31 @@ $env[$k] = $v; } $socket = getenv('E2E_DB_SOCKET') ?: ($env['DB_SOCKET'] ?? '/opt/homebrew/var/mysql/mysql.sock'); +$host = getenv('E2E_DB_HOST') ?: ($env['DB_HOST'] ?? '127.0.0.1'); +$port = (int) (getenv('E2E_DB_PORT') ?: ($env['DB_PORT'] ?? 3306)); +$user = getenv('E2E_DB_USER') ?: ($env['DB_USER'] ?? ''); +$password = getenv('E2E_DB_PASS') ?: ($env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? '')); +$database = getenv('E2E_DB_NAME') ?: ($env['DB_NAME'] ?? ''); try { $db = ($socket !== '' && file_exists($socket)) - ? new mysqli(null, $env['DB_USER'] ?? '', $env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? ''), $env['DB_NAME'] ?? '', 0, $socket) - : new mysqli($env['DB_HOST'] ?? '127.0.0.1', $env['DB_USER'] ?? '', $env['DB_PASS'] ?? ($env['DB_PASSWORD'] ?? ''), $env['DB_NAME'] ?? '', (int) ($env['DB_PORT'] ?? 3306)); + ? new mysqli(null, $user, $password, $database, 0, $socket) + : new mysqli($host, $user, $password, $database, $port); } catch (\Throwable $e) { echo "SKIP: database not reachable (" . $e->getMessage() . ")\n"; exit(0); } $db->set_charset('utf8mb4'); +// ci-quality imports only the core installer schema. Make this test hermetic: +// install/upgrade the bundled plugin schema before seeding Book Club rows, +// exactly as the real plugin lifecycle does. The operation is idempotent, so +// local databases where the plugin is already active are left intact. +$schema = (new BookClubPlugin($db, new HookManager($db)))->ensureSchema(); +if ($schema['failed'] !== []) { + fwrite(STDERR, 'FAIL: Book Club test schema could not be prepared: ' . implode(', ', $schema['failed']) . "\n"); + exit(1); +} + $TOKEN = 'zzbc138_' . bin2hex(random_bytes(4)); $repo = new Repo($db); @@ -80,13 +96,17 @@ // Existing catalogue books the external proposals will reconcile against. $isbn13 = '9780000' . random_int(100000, 999999); // 13 digits - $isbn10 = '0000' . random_int(100000, 999999); // 10 digits + $isbn10 = (string) random_int(100000000, 999999999) . 'X'; // 9 digits + X + $externalIsbn10 = substr($isbn10, 0, 3) . '-' . substr($isbn10, 3, 3) . '-' . strtolower(substr($isbn10, 6)); + $autoIsbn13 = '9783333' . random_int(100000, 999999); $delIsbn13 = '9781111' . random_int(100000, 999999); $q("INSERT INTO libri (titolo, isbn13) VALUES ('{$TOKEN} Cat13', '{$isbn13}')"); $catLibro13 = (int) $db->insert_id; $createdLibri[] = $catLibro13; $q("INSERT INTO libri (titolo, isbn10) VALUES ('{$TOKEN} Cat10', '{$isbn10}')"); $catLibro10 = (int) $db->insert_id; $createdLibri[] = $catLibro10; + $q("INSERT INTO libri (titolo, isbn13) VALUES ('{$TOKEN} CatAuto', '{$autoIsbn13}')"); + $catLibroAuto = (int) $db->insert_id; $createdLibri[] = $catLibroAuto; // A soft-deleted catalogue book whose isbn13 is nulled on delete (per the // soft-delete rule) — seed it deleted WITH the isbn kept to prove we still // don't reconcile against deleted_at != NULL rows. @@ -102,60 +122,83 @@ return [(int) $db->insert_id, $extId]; }; - echo "A. acquireExternalBook — reconcile-on-acquire (#138 bug)\n"; + echo "A. automatic and explicit external reconciliation (#138 bug)\n"; + + // A1: the manager-view reconciliation path links an existing ISBN and is + // idempotent on subsequent page loads. + [$cbAuto, $extAuto] = $seedExternal("{$TOKEN} Prop Auto", $autoIsbn13); + $libriBeforeAuto = (int) $scalar('SELECT COUNT(*) FROM libri'); + $autoCount = $repo->reconcileExternalBooksWithCatalogue($clubId); + $check($autoCount === 1, 'A1 automatic pass reconciles exactly one matching external proposal'); + $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cbAuto}") === $catLibroAuto, 'A2 automatic pass points the proposal to the existing catalogue book'); + $check($scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cbAuto}") === null, 'A3 automatic pass clears external_book_id'); + $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$extAuto}") === $catLibroAuto, 'A4 automatic pass stamps the external record'); + $check((int) $scalar('SELECT COUNT(*) FROM libri') === $libriBeforeAuto, 'A5 automatic pass creates no catalogue rows'); + $check($repo->reconcileExternalBooksWithCatalogue($clubId) === 0, 'A6 automatic pass is idempotent'); - // A1: isbn13 matches an existing catalogue book → link, no new libri row. + // Explicit fallback: isbn13 matches an existing catalogue book → link, no new libri row. [$cb1, $ext1] = $seedExternal("{$TOKEN} Prop A1", $isbn13); $libriBefore = (int) $scalar('SELECT COUNT(*) FROM libri'); $res1 = $repo->acquireExternalBook($cb1); $libriAfter = (int) $scalar('SELECT COUNT(*) FROM libri'); - $check($res1 === $catLibro13, 'A1 isbn13 match returns the EXISTING catalogue libro id (no duplicate)'); - $check($libriAfter === $libriBefore, 'A1 no new libri row inserted (reconciled, not duplicated)'); - $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cb1}") === $catLibro13, 'A1 club book repointed to the existing libro'); - $check($scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cb1}") === null, 'A1 club book external_book_id cleared'); - $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$ext1}") === $catLibro13, 'A1 external record acquired_libro_id set to existing'); + $check($res1 === $catLibro13, 'A7 explicit isbn13 match returns the EXISTING catalogue libro id (no duplicate)'); + $check($libriAfter === $libriBefore, 'A8 explicit isbn13 match inserts no new libri row'); + $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cb1}") === $catLibro13, 'A9 explicit acquisition repoints the club book'); + $check($scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cb1}") === null, 'A10 explicit acquisition clears external_book_id'); + $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$ext1}") === $catLibro13, 'A11 explicit acquisition stamps the external record'); // A2: isbn10 match → link. - [$cb2] = $seedExternal("{$TOKEN} Prop A2", $isbn10); + [$cb2] = $seedExternal("{$TOKEN} Prop A2", $externalIsbn10); $res2 = $repo->acquireExternalBook($cb2); - $check($res2 === $catLibro10, 'A2 isbn10 match returns the existing catalogue libro id'); + $check($res2 === $catLibro10, 'A12 formatted isbn10 with lowercase x matches the normalized catalogue ISBN'); // A3: no ISBN → creates a NEW libri row. [$cb3] = $seedExternal("{$TOKEN} Prop A3 NoIsbn", null); $res3 = $repo->acquireExternalBook($cb3); - $check(is_int($res3) && $res3 > 0 && !in_array($res3, [$catLibro13, $catLibro10, $delLibro], true), 'A3 no-ISBN proposal creates a NEW libro'); + $check(is_int($res3) && $res3 > 0 && !in_array($res3, [$catLibro13, $catLibro10, $catLibroAuto, $delLibro], true), 'A13 no-ISBN proposal creates a NEW libro'); if (is_int($res3)) { $createdLibri[] = $res3; } - $check((int) $scalar("SELECT COUNT(*) FROM copie WHERE libro_id={$res3}") === 1, 'A3 created libro gets one physical copy'); + $check((int) $scalar("SELECT COUNT(*) FROM copie WHERE libro_id={$res3}") === 1, 'A14 created libro gets one physical copy'); - // A4 (edge): the reconcile query's `deleted_at IS NULL` guard. The - // soft-deleted book keeps its isbn13 here so the query WOULD match it - // without the guard — proving the guard is what keeps acquire from linking - // to a deleted row (a live book with that isbn is the only valid target). + // A4 (edge): the reconcile query's `deleted_at IS NULL` guard. This + // intentionally inconsistent fixture keeps its ISBN after soft deletion; + // production deletion clears unique identifiers, but old/manual data may + // not. Acquisition must fail safely rather than resurrect/link the deleted + // row. The external proposal must remain untouched for a manager to repair. [$cb4] = $seedExternal("{$TOKEN} Prop A4 Deleted", $delIsbn13); $res4 = $repo->acquireExternalBook($cb4); - $check($res4 !== $delLibro, 'A4 does NOT reconcile to a soft-deleted book (deleted_at IS NULL guard)'); - if (is_int($res4)) { $createdLibri[] = $res4; } + $check($res4 === null, 'A15 refuses an ISBN still owned by a soft-deleted book'); + $check((int) $scalar("SELECT external_book_id IS NOT NULL FROM bookclub_books WHERE id={$cb4}") === 1, 'A16 failed acquisition leaves the external proposal intact'); // A5 (edge): ISBN matches nothing → creates new. [$cb5] = $seedExternal("{$TOKEN} Prop A5", '9782222' . random_int(100000, 999999)); $res5 = $repo->acquireExternalBook($cb5); - $check(is_int($res5) && !in_array($res5, [$catLibro13, $catLibro10, $delLibro], true), 'A5 non-matching ISBN creates a new libro'); + $check(is_int($res5) && !in_array($res5, [$catLibro13, $catLibro10, $catLibroAuto, $delLibro], true), 'A17 non-matching ISBN creates a new libro'); if (is_int($res5)) { $createdLibri[] = $res5; } + // If the same catalogue book is already a distinct row in this club, do + // not merge ids: either may own polls, votes or reading history. + [$cbDuplicate, $extDuplicate] = $seedExternal("{$TOKEN} Prop Duplicate", $isbn10); + $check($repo->reconcileExternalBooksWithCatalogue($clubId) === 0, 'A18 automatic pass refuses an ambiguous same-club duplicate'); + $check((int) $scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cbDuplicate}") === $extDuplicate, 'A19 ambiguous proposal remains external with its history intact'); + echo "B. neverChosenProposals — proposed-but-not-chosen archive (#138 feature)\n"; // Seed club books (catalogue-side, simplest) to be poll options. - $mkClubBook = static function (string $title) use ($db, $clubId, &$createdLibri): int { + $mkClubBook = static function (string $title, string $state = 'proposed') use ($db, $clubId, &$createdLibri): int { $db->query("INSERT INTO libri (titolo) VALUES ('" . $db->real_escape_string($title) . "')"); $libroId = (int) $db->insert_id; $createdLibri[] = $libroId; - $db->query("INSERT INTO bookclub_books (club_id, libro_id, state) VALUES ({$clubId}, {$libroId}, 'proposed')"); + $db->query("INSERT INTO bookclub_books (club_id, libro_id, state) VALUES ({$clubId}, {$libroId}, '" . $db->real_escape_string($state) . "')"); return (int) $db->insert_id; }; $bLoser = $mkClubBook("{$TOKEN} Loser"); // in closed poll, never wins $bWinner = $mkClubBook("{$TOKEN} Winner"); // wins a closed poll $bLateWin = $mkClubBook("{$TOKEN} LateWinner"); // loses one, wins a later one - $bOpen = $mkClubBook("{$TOKEN} OpenOnly"); // only in an OPEN poll + $bOpen = $mkClubBook("{$TOKEN} OpenOnly", 'voting'); // currently in an OPEN poll $bTwice = $mkClubBook("{$TOKEN} TwiceLoser"); // loses two closed polls + $bLegacy = $mkClubBook("{$TOKEN} Legacy"); // pre-Pinakes proposal, no poll rows + $bDeleted = $mkClubBook("{$TOKEN} DeletedClubBook"); + $bDeletedLibro = (int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$bDeleted}"); + $q("UPDATE libri SET deleted_at=NOW() WHERE id={$bDeletedLibro}"); $mkPoll = static function (string $status, ?int $winner) use ($db, $clubId): int { $w = $winner === null ? 'NULL' : (string) $winner; @@ -176,22 +219,33 @@ $p3 = $mkPoll('open', null); $opt($p3, $bOpen); $opt($p3, $bLoser); - $never = $repo->neverChosenProposals($clubId); + $never = $repo->neverChosenProposals($clubId, 'proposed'); $byId = []; foreach ($never as $row) { $byId[(int) $row['club_book_id']] = $row; } $check(isset($byId[$bLoser]), 'B1 a book that lost a closed poll and never won appears'); $check(!isset($byId[$bWinner]), 'B2 the winner is excluded'); $check(!isset($byId[$bLateWin]), 'B3 a book that lost early but won later is excluded'); - $check(!isset($byId[$bOpen]), 'B4 a book only in an OPEN poll is excluded'); + $check(!isset($byId[$bOpen]), 'B4 a book currently in the voting state is excluded'); $check(isset($byId[$bTwice]), 'B5 a two-time loser appears'); $check(isset($byId[$bTwice]) && (int) $byId[$bTwice]['times_in_poll'] === 2, 'B6 two-time loser counts times_in_poll=2 (deduped per book)'); $check(isset($byId[$bLoser]) && (int) $byId[$bLoser]['times_in_poll'] === 1, 'B7 one-time loser counts times_in_poll=1 (open poll not counted)'); + $check(isset($byId[$bLegacy]) && (int) $byId[$bLegacy]['times_in_poll'] === 0, 'B8 an entry-state proposal with no poll history is included for legacy backfill'); + $check(!isset($byId[$bDeleted]), 'B9 a soft-deleted catalogue book is omitted instead of rendering a blank archive row'); - // B8 (edge): a club with no closed polls yields nothing. + // B10 (edge): a club with no proposals yields nothing. $q("INSERT INTO bookclub_clubs (ics_token, name, slug) VALUES ('" . bin2hex(random_bytes(16)) . "', '{$TOKEN} Empty', '{$TOKEN}-empty')"); $emptyClub = (int) $db->insert_id; - $check($repo->neverChosenProposals($emptyClub) === [], 'B8 a club with no closed polls returns an empty archive'); + $check($repo->neverChosenProposals($emptyClub, 'proposed') === [], 'B10 a club with no proposals returns an empty archive'); + + echo "C. closed-poll history — discoverability (#138 feature)\n"; + $clubView = (string) file_get_contents($root . '/storage/plugins/book-club/views/public/show.php'); + $pollsView = (string) file_get_contents($root . '/storage/plugins/book-club/views/public/polls.php'); + $check(str_contains($clubView, "__('Votazioni chiuse')") && str_contains($clubView, '$closedPolls'), 'C1 club page exposes an explicit closed-poll history'); + $check(str_contains($pollsView, "__('Votazioni chiuse')") && str_contains($pollsView, '$closedPolls'), 'C2 dedicated polls page exposes closed polls separately from active polls'); + $publicController = (string) file_get_contents($root . '/storage/plugins/book-club/src/PublicController.php'); + $check(str_contains($publicController, 'if ($canManage)') && str_contains($publicController, 'reconcileExternalBooksWithCatalogue'), 'C3 manager club view runs automatic exact-ISBN reconciliation before rendering'); + $check(str_contains($publicController, 'closeExpiredForClub'), 'C4 club view closes expired polls before rendering its open/closed sections'); } finally { // ── Cleanup (FK-safe) ──────────────────────────────────────────────────── $db->query("DELETE FROM bookclub_poll_options WHERE poll_id IN (SELECT id FROM bookclub_polls WHERE club_id IN (SELECT id FROM bookclub_clubs WHERE slug LIKE '{$TOKEN}-%'))"); From 17e11fa414366be8f1600127857f52b11e03bf80 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 22 Jul 2026 08:29:55 +0200 Subject: [PATCH 03/10] refactor(book-club #138): move reconciliation/close off GET to hook + maintenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- storage/plugins/book-club/BookClubPlugin.php | 49 ++++++-- storage/plugins/book-club/README.md | 7 +- storage/plugins/book-club/plugin.json | 4 +- .../book-club/src/MobileApiController.php | 13 +-- .../plugins/book-club/src/PollController.php | 47 ++++---- .../book-club/src/PublicController.php | 15 --- storage/plugins/book-club/src/Repo.php | 106 +++++++++++++++--- tests/book-club-uwe.spec.js | 87 ++++++++++++-- ...ub-acquire-reconcile-poll-history.unit.php | 76 ++++++++----- tests/code-quality.spec.js | 20 +++- tests/mobile-api-bookclub-fixes.spec.js | 51 ++++++--- 11 files changed, 345 insertions(+), 130 deletions(-) diff --git a/storage/plugins/book-club/BookClubPlugin.php b/storage/plugins/book-club/BookClubPlugin.php index 5f668a8c1..23bc5f9de 100644 --- a/storage/plugins/book-club/BookClubPlugin.php +++ b/storage/plugins/book-club/BookClubPlugin.php @@ -101,6 +101,9 @@ public function onActivate(): void // reminders — drives poll auto-close + meeting reminders even on // installs whose only "cron" is the on-admin-login fallback. $this->registerHookInDb('maintenance.after_run', 'onMaintenanceTick', 10); + // Reconcile an external proposal as soon as its exact ISBN is + // entered through the normal catalogue create/update flow. + $this->registerHookInDb('book.save.after', 'onCatalogueBookSaved', 10); // Public quotes of the quotes module on the core book detail page. $this->registerHookInDb('book.frontend.details', 'renderBookQuotes', 10); // Documents the /api/v1/bookclub bridge inside mobile-api's @@ -827,27 +830,57 @@ public function renderBookQuotes($book = null, $bookId = null): void } // ------------------------------------------------------------------ - // Automation (hook: maintenance.after_run) + // Automation (hooks: book.save.after, maintenance.after_run) // ------------------------------------------------------------------ + /** + * Event-driven primary path: a core catalogue save can satisfy an external + * club proposal immediately, without waiting for a page render or cron. + * Failures are isolated so an optional plugin can never abort the core + * catalogue save that triggered the hook. + * + * @param array $_fields + */ + public function onCatalogueBookSaved(int $bookId, array $_fields = []): void + { + if ($bookId <= 0) { + return; + } + try { + (new Repo($this->db))->reconcileExternalBooksForCatalogueBook($bookId); + } catch (\Throwable $e) { + SecureLogger::error('[BookClub] catalogue-save reconciliation failed: ' . $e->getMessage()); + } + } + /** * Scheduled sweep, invoked by MaintenanceService::runAll() (system cron * or the on-admin-login fallback): - * 1. close every open poll whose deadline has passed (winner advances + * 1. reconcile external proposals created by import paths that bypassed + * the normal book-save hook; + * 2. close every open poll whose deadline has passed (winner advances * in the workflow, losers return to the entry state); - * 2. email a reminder for meetings starting within the next 24 hours. - * Both steps are idempotent (status flip / reminder_sent_at stamp). + * 3. email a reminder for meetings starting within the next 24 hours. + * Every step is idempotent (link/state flip/reminder_sent_at stamp). */ public function onMaintenanceTick(): void { + $repo = new Repo($this->db); + try { + $repo->reconcileAllExternalBooksWithCatalogue(); + } catch (\Throwable $e) { + SecureLogger::error('[BookClub] maintenance reconciliation failed: ' . $e->getMessage()); + } + try { + (new PollController($this->db, $repo))->closeExpiredPolls(); + } catch (\Throwable $e) { + SecureLogger::error('[BookClub] maintenance poll close failed: ' . $e->getMessage()); + } try { - $repo = new Repo($this->db); - $polls = new PollController($this->db, $repo); - $polls->closeExpiredPolls(); (new MeetingController($this->db, $repo))->sendDueReminders(); } catch (\Throwable $e) { // Never let the plugin break the shared maintenance pass. - SecureLogger::error('[BookClub] onMaintenanceTick failed: ' . $e->getMessage()); + SecureLogger::error('[BookClub] maintenance meeting reminders failed: ' . $e->getMessage()); } foreach (Modules\Registry::all($this->db) as $module) { try { diff --git a/storage/plugins/book-club/README.md b/storage/plugins/book-club/README.md index f3645b47b..f8ec020ed 100644 --- a/storage/plugins/book-club/README.md +++ b/storage/plugins/book-club/README.md @@ -37,7 +37,8 @@ Opzionali (opt-in per club): **seasons** (stagioni con archivio storico), moderazione opzionale e limite di proposte aperte per membro. - **Votazioni**: voto singolo o **preferenza multipla (N voti a testa)** — il caso d'uso originale della #138 —, voto pubblico o segreto, scadenza con - **chiusura automatica** (cron o lazy alla visualizzazione), spareggio + **chiusura automatica** (manutenzione pianificata, fallback al login admin + e chiusura idempotente al primo tentativo di voto scaduto), spareggio deterministico a favore della proposta più antica, transizione automatica del vincitore nel workflow e ritorno dei perdenti allo stato iniziale. - **Incontri** in presenza/online/ibridi con ordine del giorno, verbale, posti @@ -65,8 +66,10 @@ views/admin/*, views/public/* ``` Hook core consumati: `app.routes.register`, `admin.menu.render`, +`book.save.after` (riconcilia subito le proposte esterne per ISBN), `maintenance.after_run` (fired da `MaintenanceService::runAll()` — cron di -sistema o fallback al login admin). +sistema o fallback al login admin; chiude le votazioni scadute e recupera +eventuali import che hanno bypassato l'hook di salvataggio). Hook esposti: `bookclub.club.created`, `bookclub.member.joined`, `bookclub.member.left`, `bookclub.book.proposed`, diff --git a/storage/plugins/book-club/plugin.json b/storage/plugins/book-club/plugin.json index 0a7a9c068..9bb1486b5 100644 --- a/storage/plugins/book-club/plugin.json +++ b/storage/plugins/book-club/plugin.json @@ -2,8 +2,8 @@ "name": "book-club", "display_name": "Book Club", "description": "Gestione della lettura collaborativa: club di lettura multipli con membri e ruoli, workflow del libro configurabile (Proposto → In votazione → Scelto → In lettura → Discussione → Concluso → Archivio), proposte dal catalogo, votazioni semplici o a preferenza multipla con scadenza e chiusura automatica, incontri con RSVP e feed iCal, dashboard personale multi-club e archivio storico. Progettato per la Discussion #138: dal piccolo circolo comunale alla rete bibliotecaria.", - "version": "1.4.2", - "changelog": "v1.4.1: feedback Uwe #138 — la card 'Prossimo incontro' mostra libro/agenda/link video/orario fine; heading proposta esterna più chiaro ('Proponi un libro non ancora in catalogo'); campo 'Proposto da' (un responsabile può attribuire una proposta a un membro attivo); pulsante 'Rimuovi' per togliere un libro dalla lista del club (pulisce anche l'external non acquisito); stampa PDF della lista libri per stato. v1.3.1: bookclub_review_meta non ha più la FK hard verso la tabella core recensioni — un plugin non può assumere presenza/tipo di una tabella core tra install diverse (recensioni è solo in schema.sql, mai in migration → su install vecchi può mancare (1824) o avere id int unsigned (3780), rompendo l attivazione). recensione_id resta colonna indicizzata (UNIQUE). Il bump ri-esegue onActivate/ensureSchema. v1.2.0: ponte API mobile (/api/v1/bookclub con token dell'app, discovery /health con app_access_enabled, timestamp ISO-8601 UTC), modulo prestito tra membri, permessi granulari nei controller, pesi voto configurabili, questionari con bozze modificabili e apertura programmata, citazioni pubbliche sulla scheda libro, restyle completo al design system del frontend. Il bump di versione fa rieseguire onActivate/ensureSchema sulle installazioni già attive (colonne closed_reason/weight_owner/weight_moderator/season_id, tabelle lending, hook book.frontend.details). v1.1.0: Fasi 2-4 del piano — architettura a 16 moduli attivabili per club (auto-discovery). Fase 2: lettura condivisa con sezioni e tracker, discussioni con spoiler protetti/reazioni/menzioni, integrazione biblioteca (copie, prestiti membri, lista d'attesa) e ponte recensioni, votazioni avanzate (stelle, classifica Borda, eliminazione a round, ponderato, quorum, spareggi), statistiche con rollup ed export JSON/CSV, stagioni, ruoli personalizzati con matrice permessi e automazioni. Fase 3: gamification (XP/livelli/badge/classifica), questionari con builder, citazioni e annotazioni con export, reading challenge, affinità tra lettori (opt-in) e suggerimenti, API REST read-only con OpenAPI. Fase 4: reading sprint, buddy reading, assistente IA opt-in (domande di discussione e riassunti verbali). v1.0.0: Fase 1 (MVP) — club, membri e ruoli, inviti email, workflow configurabile, proposte dal catalogo, votazioni simple/multi con chiusura automatica, incontri con RSVP, feed iCal, dashboard multi-club.", + "version": "1.4.3", + "changelog": "v1.4.3: le pagine club, votazioni e dashboard non riconciliano più libri né chiudono votazioni durante una GET; la riconciliazione delle proposte esterne parte da book.save.after con backfill nella manutenzione completa, mentre le votazioni scadute sono chiuse dalla manutenzione (cron o login admin) e, come fallback immediato, dal primo tentativo di voto POST. v1.4.1: feedback Uwe #138 — la card 'Prossimo incontro' mostra libro/agenda/link video/orario fine; heading proposta esterna più chiaro ('Proponi un libro non ancora in catalogo'); campo 'Proposto da' (un responsabile può attribuire una proposta a un membro attivo); pulsante 'Rimuovi' per togliere un libro dalla lista del club (pulisce anche l'external non acquisito); stampa PDF della lista libri per stato. v1.3.1: bookclub_review_meta non ha più la FK hard verso la tabella core recensioni — un plugin non può assumere presenza/tipo di una tabella core tra install diverse (recensioni è solo in schema.sql, mai in migration → su install vecchi può mancare (1824) o avere id int unsigned (3780), rompendo l attivazione). recensione_id resta colonna indicizzata (UNIQUE). Il bump ri-esegue onActivate/ensureSchema. v1.2.0: ponte API mobile (/api/v1/bookclub con token dell'app, discovery /health con app_access_enabled, timestamp ISO-8601 UTC), modulo prestito tra membri, permessi granulari nei controller, pesi voto configurabili, questionari con bozze modificabili e apertura programmata, citazioni pubbliche sulla scheda libro, restyle completo al design system del frontend. Il bump di versione fa rieseguire onActivate/ensureSchema sulle installazioni già attive (colonne closed_reason/weight_owner/weight_moderator/season_id, tabelle lending, hook book.frontend.details). v1.1.0: Fasi 2-4 del piano — architettura a 16 moduli attivabili per club (auto-discovery). Fase 2: lettura condivisa con sezioni e tracker, discussioni con spoiler protetti/reazioni/menzioni, integrazione biblioteca (copie, prestiti membri, lista d'attesa) e ponte recensioni, votazioni avanzate (stelle, classifica Borda, eliminazione a round, ponderato, quorum, spareggi), statistiche con rollup ed export JSON/CSV, stagioni, ruoli personalizzati con matrice permessi e automazioni. Fase 3: gamification (XP/livelli/badge/classifica), questionari con builder, citazioni e annotazioni con export, reading challenge, affinità tra lettori (opt-in) e suggerimenti, API REST read-only con OpenAPI. Fase 4: reading sprint, buddy reading, assistente IA opt-in (domande di discussione e riassunti verbali). v1.0.0: Fase 1 (MVP) — club, membri e ruoli, inviti email, workflow configurabile, proposte dal catalogo, votazioni simple/multi con chiusura automatica, incontri con RSVP, feed iCal, dashboard multi-club.", "author": "Fabiodalez", "author_url": "", "plugin_url": "https://github.com/fabiodalez-dev/Pinakes/discussions/138", diff --git a/storage/plugins/book-club/src/MobileApiController.php b/storage/plugins/book-club/src/MobileApiController.php index 3afac20a6..bd1d1db05 100644 --- a/storage/plugins/book-club/src/MobileApiController.php +++ b/storage/plugins/book-club/src/MobileApiController.php @@ -114,9 +114,6 @@ public function clubDetail(ServerRequestInterface $request, ResponseInterface $r if ($club === null) { return $error !== null ? $error($response) : $this->fail($response, 'not_found', __('Club non trovato.'), 404); } - // Lazy-close expired polls before reading, mirroring the web pages: - // the app must never see status='open' on a poll vote() would 409. - (new PollController($this->db, $this->repo))->closeExpiredForClub((int) $club['id']); $userId = (int) $this->userId(); $membership = $this->membership($club); $isMember = $membership !== null && $membership['status'] === 'active'; @@ -220,16 +217,12 @@ public function clubDetail(ServerRequestInterface $request, ResponseInterface $r public function dashboard(ServerRequestInterface $request, ResponseInterface $response): ResponseInterface { $userId = (int) $this->userId(); - $pollController = new PollController($this->db, $this->repo); $cards = []; foreach ($this->repo->listClubsForUser($userId) as $clubRow) { $club = $this->repo->clubById((int) $clubRow['id']); if ($club === null || !Registry::clubEnabled($club, $this->module)) { continue; } - // open_polls must not include deadline-passed polls (lazy close, - // one cheap SELECT per club — same guarantee as the club pages). - $pollController->closeExpiredForClub((int) $club['id']); $snapshot = $this->repo->clubSnapshot($club); $cards[] = [ 'club' => [ @@ -368,7 +361,11 @@ public function vote(ServerRequestInterface $request, ResponseInterface $respons if ($poll === null || (int) $poll['club_id'] !== (int) $club['id']) { return $this->fail($response, 'not_found', __('Club non trovato.'), 404); } - if ($poll['status'] !== 'open' || ($poll['closes_at'] !== null && strtotime((string) $poll['closes_at']) <= time())) { + if ($poll['status'] === 'open' && $this->repo->pollDeadlinePassed($pollId)) { + (new PollController($this->db, $this->repo))->closeExpiredPollById($pollId); + return $this->fail($response, 'poll_closed', __('La votazione è chiusa.'), 409); + } + if ($poll['status'] !== 'open') { return $this->fail($response, 'poll_closed', __('La votazione è chiusa.'), 409); } $mode = (string) ($poll['mode'] ?? 'simple'); diff --git a/storage/plugins/book-club/src/PollController.php b/storage/plugins/book-club/src/PollController.php index 1728db099..792ab5c7c 100644 --- a/storage/plugins/book-club/src/PollController.php +++ b/storage/plugins/book-club/src/PollController.php @@ -72,13 +72,6 @@ public function show(ServerRequestInterface $request, ResponseInterface $respons if ($poll === null || (int) $poll['club_id'] !== (int) $club['id']) { return $this->notFound($response); } - // Lazy close: correctness does not depend on the cron. Elimination - // polls resolve their rounds repeatedly until a winner emerges. - if ($poll['status'] === 'open' && $poll['closes_at'] !== null && strtotime((string) $poll['closes_at']) <= time()) { - $this->resolvePoll($poll, true); - $poll = $this->repo->poll($pollId) ?? $poll; - } - $userId = $this->userId(); $mode = (string) ($poll['mode'] ?? 'simple'); $round = $this->displayRound($poll); @@ -131,9 +124,6 @@ public function pollsPage(ServerRequestInterface $request, ResponseInterface $re if ($club === null || !$this->canView($club) || !$this->advancedVotingEnabled($club)) { return $this->notFound($response); } - // Same lazy-close guarantee as show(): the list must never present - // a deadline-passed poll as open while vote() rejects its ballots. - $this->closeExpiredForClub((int) $club['id']); $books = $this->repo->clubBooks((int) $club['id']); $eligible = $this->repo->pollEligibleBooks($club, $books); $entryState = Repo::entryStateKey($this->repo->workflowStates($club)); @@ -309,7 +299,15 @@ public function vote(ServerRequestInterface $request, ResponseInterface $respons if ($poll === null || (int) $poll['club_id'] !== (int) $club['id']) { return $this->notFound($response); } - if ($poll['status'] !== 'open' || ($poll['closes_at'] !== null && strtotime((string) $poll['closes_at']) <= time())) { + // POST fallback: if the scheduled maintenance has not run yet, the + // first attempted ballot performs the same idempotent close before it + // is rejected. GET routes remain read-only. + if ($poll['status'] === 'open' && $this->repo->pollDeadlinePassed($pollId)) { + $this->closeExpiredPollById($pollId); + $this->flash('error', __('La votazione è chiusa.')); + return $this->redirect($response, '/book-club/' . $slug . '/polls/' . $pollId); + } + if ($poll['status'] !== 'open') { $this->flash('error', __('La votazione è chiusa.')); return $this->redirect($response, '/book-club/' . $slug . '/polls/' . $pollId); } @@ -481,21 +479,22 @@ public function closeExpiredPolls(): void } /** - * Per-club lazy close, the same guarantee show() gives for a single - * poll ("correctness does not depend on the cron") extended to the - * list/read paths: club page poll list, mobile club detail and the - * dashboards. Only touches polls with status='open' AND closes_at in - * the past, so the close side effects (tally, winner transition, - * bookclub.poll.closed hook) run exactly once. + * Close one due poll from a mutating fallback path (web/mobile vote POST). + * The persisted status and the database clock are checked again so a + * concurrent maintenance pass remains harmless. */ - public function closeExpiredForClub(int $clubId): void + public function closeExpiredPollById(int $pollId): void { - foreach ($this->repo->expiredOpenPollsForClub($clubId) as $poll) { - try { - $this->resolvePoll($poll, true); - } catch (\Throwable $e) { - SecureLogger::error('[BookClub] lazy close failed for poll ' . $poll['id'] . ': ' . $e->getMessage()); - } + $poll = $this->repo->poll($pollId); + if ($poll === null + || $poll['status'] !== 'open' + || !$this->repo->pollDeadlinePassed($pollId)) { + return; + } + try { + $this->resolvePoll($poll, true); + } catch (\Throwable $e) { + SecureLogger::error('[BookClub] on-demand close failed for poll ' . $pollId . ': ' . $e->getMessage()); } } diff --git a/storage/plugins/book-club/src/PublicController.php b/storage/plugins/book-club/src/PublicController.php index 816bd7838..ada5c4fb0 100644 --- a/storage/plugins/book-club/src/PublicController.php +++ b/storage/plugins/book-club/src/PublicController.php @@ -40,17 +40,6 @@ public function show(ServerRequestInterface $request, ResponseInterface $respons $canManage = $this->canManage($club); $isMember = $membership !== null && $membership['status'] === 'active'; - // Keep the public list consistent with the ballot endpoint and the - // mobile API even when the maintenance cron has not run yet. - (new PollController($this->db, $this->repo))->closeExpiredForClub((int) $club['id']); - - // A manager may have entered an externally proposed book through the - // normal catalogue flow since the previous visit. Link exact ISBN - // matches before rendering; this is idempotent and never creates books. - if ($canManage) { - $this->repo->reconcileExternalBooksWithCatalogue((int) $club['id']); - } - $books = $this->repo->clubBooks((int) $club['id']); // Proposals awaiting moderation are manager-only. if (!$canManage) { @@ -707,12 +696,8 @@ public function dashboard(ServerRequestInterface $request, ResponseInterface $re { $userId = (int) $this->userId(); $clubs = $this->repo->listClubsForUser($userId); - $pollController = new PollController($this->db, $this->repo); $cards = []; foreach ($clubs as $club) { - // Lazy-close expired polls so the "votazioni aperte" column never - // lists a poll whose ballots would be rejected. - $pollController->closeExpiredForClub((int) $club['id']); $cards[] = [ 'club' => $club, 'snapshot' => $this->repo->clubSnapshot($club), diff --git a/storage/plugins/book-club/src/Repo.php b/storage/plugins/book-club/src/Repo.php index f67544492..a2f02160c 100644 --- a/storage/plugins/book-club/src/Repo.php +++ b/storage/plugins/book-club/src/Repo.php @@ -902,8 +902,9 @@ private function linkExternalBookToCatalogue(int $clubId, int $clubBookId, int $ /** * Reconcile external proposals after their book has been entered manually * in the catalogue. This never creates catalogue data: it only links an - * exact normalized ISBN-10/ISBN-13 match, and is therefore safe to run when - * a manager opens the club page. Returns the number of proposals linked. + * exact normalized ISBN-10/ISBN-13 match. The transaction and row locks + * make it safe to call from hooks, maintenance, and explicit POST actions. + * Returns the number of proposals linked. */ public function reconcileExternalBooksWithCatalogue(int $clubId): int { @@ -949,6 +950,85 @@ public function reconcileExternalBooksWithCatalogue(int $clubId): int } } + /** + * Targeted reconciliation for the core `book.save.after` hook. Only clubs + * with an unresolved external proposal matching the saved book's ISBN are + * swept; the regular per-club transaction rechecks the catalogue row and + * protects every mutation with locks. + */ + public function reconcileExternalBooksForCatalogueBook(int $libroId): int + { + if ($libroId <= 0) { + return 0; + } + + $book = $this->row( + 'SELECT isbn13, isbn10 FROM libri WHERE id = ? AND deleted_at IS NULL LIMIT 1', + 'i', + [$libroId] + ); + if ($book === null) { + return 0; + } + + [$bookIsbn13] = $this->normalizedIsbnParts( + isset($book['isbn13']) ? (string) $book['isbn13'] : null + ); + [, $bookIsbn10] = $this->normalizedIsbnParts( + isset($book['isbn10']) ? (string) $book['isbn10'] : null + ); + if ($bookIsbn13 === null && $bookIsbn10 === null) { + return 0; + } + + $clubIds = []; + foreach ($this->rows( + "SELECT DISTINCT cb.club_id, ext.isbn + FROM bookclub_books cb + JOIN bookclub_external_books ext ON ext.id = cb.external_book_id + WHERE cb.external_book_id IS NOT NULL + AND ext.acquired_libro_id IS NULL + AND ext.isbn IS NOT NULL + AND TRIM(ext.isbn) <> ''" + ) as $candidate) { + [$candidateIsbn13, $candidateIsbn10] = $this->normalizedIsbnParts((string) $candidate['isbn']); + if (($bookIsbn13 !== null && $candidateIsbn13 === $bookIsbn13) + || ($bookIsbn10 !== null && $candidateIsbn10 === $bookIsbn10)) { + $clubIds[(int) $candidate['club_id']] = true; + } + } + + $reconciled = 0; + foreach (array_keys($clubIds) as $clubId) { + $reconciled += $this->reconcileExternalBooksWithCatalogue((int) $clubId); + } + return $reconciled; + } + + /** + * Maintenance backfill for catalogue imports and integrations that bypass + * the normal book-save hook. Clubs are processed in separate transactions + * so one conflicting proposal cannot roll back the whole scheduled sweep. + */ + public function reconcileAllExternalBooksWithCatalogue(): int + { + $clubIds = $this->rows( + "SELECT DISTINCT cb.club_id + FROM bookclub_books cb + JOIN bookclub_external_books ext ON ext.id = cb.external_book_id + WHERE cb.external_book_id IS NOT NULL + AND ext.acquired_libro_id IS NULL + AND ext.isbn IS NOT NULL + AND TRIM(ext.isbn) <> ''" + ); + + $reconciled = 0; + foreach ($clubIds as $row) { + $reconciled += $this->reconcileExternalBooksWithCatalogue((int) $row['club_id']); + } + return $reconciled; + } + /** * Acquire an external proposal into the catalogue: create the real `libri` * row from its metadata (this is the ONLY moment the book enters the @@ -1381,21 +1461,19 @@ public function expiredOpenPolls(): array } /** - * Expired-but-still-open polls of ONE club — the per-club lazy-close - * sweep the read paths (club page, mobile detail, dashboards) run so - * correctness never depends on the maintenance cron. - * - * @return list> + * Use the database clock for deadline checks. PHP and MySQL may run in + * different timezones on shared hosting, while closes_at is persisted and + * compared consistently by MySQL throughout the maintenance path. */ - public function expiredOpenPollsForClub(int $clubId): array + public function pollDeadlinePassed(int $pollId): bool { - return $this->rows( - "SELECT * FROM bookclub_polls - WHERE club_id = ? AND status = 'open' - AND closes_at IS NOT NULL AND closes_at <= NOW()", + return $this->row( + "SELECT id FROM bookclub_polls + WHERE id = ? AND closes_at IS NOT NULL AND closes_at <= NOW() + LIMIT 1", 'i', - [$clubId] - ); + [$pollId] + ) !== null; } // ------------------------------------------------------------------ diff --git a/tests/book-club-uwe.spec.js b/tests/book-club-uwe.spec.js index 9238dc8ba..cd85f7896 100644 --- a/tests/book-club-uwe.spec.js +++ b/tests/book-club-uwe.spec.js @@ -31,7 +31,7 @@ const EXT_TITLE_2 = `External Book Alt ${RUN}`; const EXT_AUTHOR_1 = `Jane External ${RUN}`; const EXT_AUTHOR_2 = `Janet External ${RUN}`; const EXT_PUBLISHER = `External Press ${RUN}`; -const AUTO_ISBN = `978${String(Date.now()).slice(-10)}`; +const AUTO_ISBN = makeIsbn13(`978${String(Date.now()).slice(-9)}`); const MEMBER_EMAIL = `e2e-bc-uwe-${RUN}@example.test`; test.skip(!ADMIN_EMAIL || !ADMIN_PASS || !DB_USER || !DB_PASS || !DB_NAME || (!DB_HOST && !DB_SOCKET), 'E2E credentials not configured'); @@ -59,6 +59,16 @@ function dbQuery(sql) { } function sqlStr(s) { return "'" + String(s).replace(/'/g, "''") + "'"; } +function makeIsbn13(firstTwelveDigits) { + const digits = String(firstTwelveDigits).replace(/\D/g, '').slice(0, 12); + if (digits.length !== 12) throw new Error('ISBN-13 seed must contain 12 digits'); + let sum = 0; + for (let i = 0; i < digits.length; i++) { + sum += Number(digits[i]) * (i % 2 === 0 ? 1 : 3); + } + return digits + String((10 - (sum % 10)) % 10); +} + async function loginAsAdmin(page) { await page.goto(`${BASE}/accedi`); const emailField = page.locator('input[name="email"]'); @@ -70,6 +80,31 @@ async function loginAsAdmin(page) { } } +async function submitBookForm(page) { + const submit = page.locator('#bookForm button[type="submit"], button[type="submit"]').first(); + await submit.scrollIntoViewIfNeeded(); + await submit.click(); + const confirm = page.locator('.swal2-confirm:visible'); + if (await confirm.isVisible({ timeout: 5000 }).catch(() => false)) { + await confirm.click(); + } + await page.waitForFunction( + () => !window.location.pathname.endsWith('/admin/books/create') + && !window.location.pathname.includes('/admin/books/edit/'), + null, + { timeout: 30000 } + ); + await page.waitForLoadState('domcontentloaded'); +} + +async function createCatalogueBook(page, title, isbn13) { + await page.goto(`${BASE}/admin/books/create`); + await page.waitForLoadState('domcontentloaded'); + await page.locator('input[name="titolo"]').fill(title); + await page.locator('input[name="isbn13"]').fill(isbn13); + await submitBookForm(page); +} + /** Future datetime as the datetime-local input wants it (local wall clock). */ function futureLocal(daysAhead, hour) { const d = new Date(Date.now() + daysAhead * 86400000); @@ -102,6 +137,9 @@ test.describe.serial('Book Club — Uwe feedback', () => { expect(resp.ok(), 'plugin activation request should succeed').toBeTruthy(); await expect.poll(() => dbQuery(`SELECT is_active FROM plugins WHERE id=${bcId}`), { timeout: 8000 }).toBe('1'); } + expect(dbQuery( + `SELECT COUNT(*) FROM plugin_hooks WHERE plugin_id=${bcId} AND hook_name='book.save.after' AND callback_method='onCatalogueBookSaved' AND is_active=1` + ), 'active Book Club installs must receive the catalogue-save hook').toBe('1'); // Create a club through the real admin form; the creator becomes its owner. await page.goto(`${BASE}/admin/book-club/new`); @@ -284,22 +322,38 @@ test.describe.serial('Book Club — Uwe feedback', () => { expect(dbQuery(`SELECT COUNT(*) FROM libri WHERE titolo IN (${sqlStr(EXT_TITLE)}, ${sqlStr(EXT_TITLE_2)})`), 'opening a poll with external books must not create catalog rows').toBe('0'); - // Reproduce Uwe's failure exactly: the manager later records the purchase - // through the normal catalogue side, then revisits the club. The GET must - // link the proposal by exact ISBN without requiring the Acquire button and - // without duplicating the catalogue row. - dbQuery(`INSERT INTO libri (titolo, isbn13) VALUES (${sqlStr(EXT_TITLE_2)}, ${sqlStr(AUTO_ISBN)})`); + // Reproduce Uwe's flow through the real catalogue UI. book.save.after must + // link the exact ISBN immediately; no Book Club GET is needed. + await createCatalogueBook(page, EXT_TITLE_2, AUTO_ISBN); const manuallyAddedLibroId = Number(dbQuery(`SELECT id FROM libri WHERE isbn13=${sqlStr(AUTO_ISBN)} LIMIT 1`) || '0'); expect(manuallyAddedLibroId).toBeGreaterThan(0); - dbQuery(`INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (${manuallyAddedLibroId}, ${sqlStr(`LIB-${manuallyAddedLibroId}`)}, 'disponibile')`); - await page.goto(`${BASE}/book-club/${slug}`); - await page.waitForLoadState('networkidle'); await expect.poll(() => dbQuery( `SELECT CONCAT(IFNULL(libro_id,'NULL'),'|',IFNULL(external_book_id,'NULL')) FROM bookclub_books WHERE id=${second.clubBookId}` )).toBe(`${manuallyAddedLibroId}|NULL`); expect(dbQuery(`SELECT acquired_libro_id FROM bookclub_external_books WHERE id=${second.extBookId}`)).toBe(String(manuallyAddedLibroId)); expect(dbQuery(`SELECT COUNT(*) FROM libri WHERE isbn13=${sqlStr(AUTO_ISBN)}`), - 'automatic reconciliation must not duplicate the manually added catalogue row').toBe('1'); + 'hook reconciliation must not duplicate the manually added catalogue row').toBe('1'); + + // Prove the page render itself is read-only: simulate an import that + // bypassed book.save.after, visit the GET view, and confirm no link occurs. + // A normal catalogue update then fires the hook and repairs it. + dbQuery(`UPDATE bookclub_books SET libro_id=NULL, external_book_id=${second.extBookId} WHERE id=${second.clubBookId}`); + dbQuery(`UPDATE bookclub_external_books SET acquired_libro_id=NULL WHERE id=${second.extBookId}`); + await page.goto(`${BASE}/book-club/${slug}`); + await page.waitForLoadState('networkidle'); + expect(dbQuery( + `SELECT CONCAT(IFNULL(libro_id,'NULL'),'|',IFNULL(external_book_id,'NULL')) FROM bookclub_books WHERE id=${second.clubBookId}` + ), 'club GET must not reconcile catalogue data').toBe(`NULL|${second.extBookId}`); + + await page.goto(`${BASE}/admin/books/edit/${manuallyAddedLibroId}`); + await page.waitForLoadState('domcontentloaded'); + await submitBookForm(page); + await expect.poll(() => dbQuery( + `SELECT CONCAT(IFNULL(libro_id,'NULL'),'|',IFNULL(external_book_id,'NULL')) FROM bookclub_books WHERE id=${second.clubBookId}` + )).toBe(`${manuallyAddedLibroId}|NULL`); + + await page.goto(`${BASE}/book-club/${slug}`); + await page.waitForLoadState('domcontentloaded'); // Acquire it into the catalogue (manager action) — the one moment it enters libri. await page.locator(`form[action*="/books/${first.clubBookId}/acquire"] button`).click(); @@ -335,14 +389,23 @@ test.describe.serial('Book Club — Uwe feedback', () => { expect(dbQuery(`SELECT COUNT(*) FROM libri WHERE titolo=${sqlStr(EXT_TITLE_2)}`), 'the automatically reconciled poll option has exactly one catalogue row').toBe('1'); - // The main club page must also lazily close expired polls (cron is not a - // correctness dependency) and expose them in its explicit history. + // Expired polls are normally closed by full maintenance. GET is read-only; + // the first rejected vote POST is the immediate fallback and performs the + // same idempotent close before redirecting to the history. const pollId = Number(dbQuery(`SELECT id FROM bookclub_polls WHERE club_id=${clubId} AND title=${sqlStr(pollTitle)} LIMIT 1`) || '0'); expect(pollId).toBeGreaterThan(0); dbQuery(`UPDATE bookclub_polls SET closes_at=DATE_SUB(NOW(), INTERVAL 1 MINUTE), status='open' WHERE id=${pollId}`); await page.goto(`${BASE}/book-club/${slug}`); await page.waitForLoadState('networkidle'); + expect(dbQuery(`SELECT status FROM bookclub_polls WHERE id=${pollId}`), 'club GET must not close a poll').toBe('open'); + await page.goto(`${BASE}/book-club/${slug}/polls/${pollId}`); + await page.waitForLoadState('domcontentloaded'); + expect(dbQuery(`SELECT status FROM bookclub_polls WHERE id=${pollId}`), 'poll GET must not close a poll').toBe('open'); + await page.locator(`form[action$="/polls/${pollId}/vote"] button[type="submit"]`).click(); + await page.waitForLoadState('networkidle'); expect(dbQuery(`SELECT status FROM bookclub_polls WHERE id=${pollId}`)).toBe('closed'); + await page.goto(`${BASE}/book-club/${slug}`); + await page.waitForLoadState('domcontentloaded'); const closedHistory = page.locator('details:has-text("Votazioni chiuse")'); await closedHistory.locator('summary').click(); await expect(closedHistory.locator(`a[href$="/polls/${pollId}"]`)).toBeVisible(); diff --git a/tests/bookclub-acquire-reconcile-poll-history.unit.php b/tests/bookclub-acquire-reconcile-poll-history.unit.php index 8aa0f056d..bf61446c0 100644 --- a/tests/bookclub-acquire-reconcile-poll-history.unit.php +++ b/tests/bookclub-acquire-reconcile-poll-history.unit.php @@ -6,8 +6,9 @@ * * A. External-book reconciliation — when a proposed external book is already * in the catalogue (same ISBN, because the manager bought it and added it - * manually), the manager's next club visit links it automatically. The - * explicit "Add to catalogue" fallback also reuses an existing match. + * manually), `book.save.after` links it immediately. Scheduled maintenance + * backfills import paths that bypass the hook. The explicit "Add to + * catalogue" fallback also reuses an existing match. * Edge cases: isbn13/isbn10, idempotency, no match, a duplicate club entry, * no-ISBN create, and an ISBN still owned by a SOFT-DELETED row. * @@ -64,7 +65,8 @@ // install/upgrade the bundled plugin schema before seeding Book Club rows, // exactly as the real plugin lifecycle does. The operation is idempotent, so // local databases where the plugin is already active are left intact. -$schema = (new BookClubPlugin($db, new HookManager($db)))->ensureSchema(); +$plugin = new BookClubPlugin($db, new HookManager($db)); +$schema = $plugin->ensureSchema(); if ($schema['failed'] !== []) { fwrite(STDERR, 'FAIL: Book Club test schema could not be prepared: ' . implode(', ', $schema['failed']) . "\n"); exit(1); @@ -99,6 +101,7 @@ $isbn10 = (string) random_int(100000000, 999999999) . 'X'; // 9 digits + X $externalIsbn10 = substr($isbn10, 0, 3) . '-' . substr($isbn10, 3, 3) . '-' . strtolower(substr($isbn10, 6)); $autoIsbn13 = '9783333' . random_int(100000, 999999); + $backfillIsbn13 = '9784444' . random_int(100000, 999999); $delIsbn13 = '9781111' . random_int(100000, 999999); $q("INSERT INTO libri (titolo, isbn13) VALUES ('{$TOKEN} Cat13', '{$isbn13}')"); @@ -107,6 +110,8 @@ $catLibro10 = (int) $db->insert_id; $createdLibri[] = $catLibro10; $q("INSERT INTO libri (titolo, isbn13) VALUES ('{$TOKEN} CatAuto', '{$autoIsbn13}')"); $catLibroAuto = (int) $db->insert_id; $createdLibri[] = $catLibroAuto; + $q("INSERT INTO libri (titolo, isbn13) VALUES ('{$TOKEN} CatBackfill', '{$backfillIsbn13}')"); + $catLibroBackfill = (int) $db->insert_id; $createdLibri[] = $catLibroBackfill; // A soft-deleted catalogue book whose isbn13 is nulled on delete (per the // soft-delete rule) — seed it deleted WITH the isbn kept to prove we still // don't reconcile against deleted_at != NULL rows. @@ -124,40 +129,46 @@ echo "A. automatic and explicit external reconciliation (#138 bug)\n"; - // A1: the manager-view reconciliation path links an existing ISBN and is - // idempotent on subsequent page loads. + // A1: the catalogue-save hook links an existing ISBN immediately and is + // idempotent if the same save notification is delivered again. [$cbAuto, $extAuto] = $seedExternal("{$TOKEN} Prop Auto", $autoIsbn13); $libriBeforeAuto = (int) $scalar('SELECT COUNT(*) FROM libri'); - $autoCount = $repo->reconcileExternalBooksWithCatalogue($clubId); - $check($autoCount === 1, 'A1 automatic pass reconciles exactly one matching external proposal'); - $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cbAuto}") === $catLibroAuto, 'A2 automatic pass points the proposal to the existing catalogue book'); - $check($scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cbAuto}") === null, 'A3 automatic pass clears external_book_id'); - $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$extAuto}") === $catLibroAuto, 'A4 automatic pass stamps the external record'); - $check((int) $scalar('SELECT COUNT(*) FROM libri') === $libriBeforeAuto, 'A5 automatic pass creates no catalogue rows'); - $check($repo->reconcileExternalBooksWithCatalogue($clubId) === 0, 'A6 automatic pass is idempotent'); + $plugin->onCatalogueBookSaved($catLibroAuto); + $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cbAuto}") === $catLibroAuto, 'A1 book.save.after points the proposal to the existing catalogue book'); + $check($scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cbAuto}") === null, 'A2 book.save.after clears external_book_id'); + $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$extAuto}") === $catLibroAuto, 'A3 book.save.after stamps the external record'); + $check((int) $scalar('SELECT COUNT(*) FROM libri') === $libriBeforeAuto, 'A4 book.save.after creates no catalogue rows'); + $check($repo->reconcileExternalBooksForCatalogueBook($catLibroAuto) === 0, 'A5 book.save.after reconciliation is idempotent'); + + // A6: scheduled maintenance catches catalogue imports that did not fire + // book.save.after, without relying on a manager opening a GET page. + [$cbBackfill, $extBackfill] = $seedExternal("{$TOKEN} Prop Backfill", $backfillIsbn13); + $check($repo->reconcileAllExternalBooksWithCatalogue() === 1, 'A6 maintenance backfill reconciles a hook-bypassing import'); + $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cbBackfill}") === $catLibroBackfill, 'A7 maintenance backfill points to the existing catalogue book'); + $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$extBackfill}") === $catLibroBackfill, 'A8 maintenance backfill stamps the external record'); // Explicit fallback: isbn13 matches an existing catalogue book → link, no new libri row. [$cb1, $ext1] = $seedExternal("{$TOKEN} Prop A1", $isbn13); $libriBefore = (int) $scalar('SELECT COUNT(*) FROM libri'); $res1 = $repo->acquireExternalBook($cb1); $libriAfter = (int) $scalar('SELECT COUNT(*) FROM libri'); - $check($res1 === $catLibro13, 'A7 explicit isbn13 match returns the EXISTING catalogue libro id (no duplicate)'); - $check($libriAfter === $libriBefore, 'A8 explicit isbn13 match inserts no new libri row'); - $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cb1}") === $catLibro13, 'A9 explicit acquisition repoints the club book'); - $check($scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cb1}") === null, 'A10 explicit acquisition clears external_book_id'); - $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$ext1}") === $catLibro13, 'A11 explicit acquisition stamps the external record'); + $check($res1 === $catLibro13, 'A9 explicit isbn13 match returns the EXISTING catalogue libro id (no duplicate)'); + $check($libriAfter === $libriBefore, 'A10 explicit isbn13 match inserts no new libri row'); + $check((int) $scalar("SELECT libro_id FROM bookclub_books WHERE id={$cb1}") === $catLibro13, 'A11 explicit acquisition repoints the club book'); + $check($scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cb1}") === null, 'A12 explicit acquisition clears external_book_id'); + $check((int) $scalar("SELECT acquired_libro_id FROM bookclub_external_books WHERE id={$ext1}") === $catLibro13, 'A13 explicit acquisition stamps the external record'); // A2: isbn10 match → link. [$cb2] = $seedExternal("{$TOKEN} Prop A2", $externalIsbn10); $res2 = $repo->acquireExternalBook($cb2); - $check($res2 === $catLibro10, 'A12 formatted isbn10 with lowercase x matches the normalized catalogue ISBN'); + $check($res2 === $catLibro10, 'A14 formatted isbn10 with lowercase x matches the normalized catalogue ISBN'); // A3: no ISBN → creates a NEW libri row. [$cb3] = $seedExternal("{$TOKEN} Prop A3 NoIsbn", null); $res3 = $repo->acquireExternalBook($cb3); - $check(is_int($res3) && $res3 > 0 && !in_array($res3, [$catLibro13, $catLibro10, $catLibroAuto, $delLibro], true), 'A13 no-ISBN proposal creates a NEW libro'); + $check(is_int($res3) && $res3 > 0 && !in_array($res3, [$catLibro13, $catLibro10, $catLibroAuto, $catLibroBackfill, $delLibro], true), 'A15 no-ISBN proposal creates a NEW libro'); if (is_int($res3)) { $createdLibri[] = $res3; } - $check((int) $scalar("SELECT COUNT(*) FROM copie WHERE libro_id={$res3}") === 1, 'A14 created libro gets one physical copy'); + $check((int) $scalar("SELECT COUNT(*) FROM copie WHERE libro_id={$res3}") === 1, 'A16 created libro gets one physical copy'); // A4 (edge): the reconcile query's `deleted_at IS NULL` guard. This // intentionally inconsistent fixture keeps its ISBN after soft deletion; @@ -166,20 +177,20 @@ // row. The external proposal must remain untouched for a manager to repair. [$cb4] = $seedExternal("{$TOKEN} Prop A4 Deleted", $delIsbn13); $res4 = $repo->acquireExternalBook($cb4); - $check($res4 === null, 'A15 refuses an ISBN still owned by a soft-deleted book'); - $check((int) $scalar("SELECT external_book_id IS NOT NULL FROM bookclub_books WHERE id={$cb4}") === 1, 'A16 failed acquisition leaves the external proposal intact'); + $check($res4 === null, 'A17 refuses an ISBN still owned by a soft-deleted book'); + $check((int) $scalar("SELECT external_book_id IS NOT NULL FROM bookclub_books WHERE id={$cb4}") === 1, 'A18 failed acquisition leaves the external proposal intact'); // A5 (edge): ISBN matches nothing → creates new. [$cb5] = $seedExternal("{$TOKEN} Prop A5", '9782222' . random_int(100000, 999999)); $res5 = $repo->acquireExternalBook($cb5); - $check(is_int($res5) && !in_array($res5, [$catLibro13, $catLibro10, $catLibroAuto, $delLibro], true), 'A17 non-matching ISBN creates a new libro'); + $check(is_int($res5) && !in_array($res5, [$catLibro13, $catLibro10, $catLibroAuto, $catLibroBackfill, $delLibro], true), 'A19 non-matching ISBN creates a new libro'); if (is_int($res5)) { $createdLibri[] = $res5; } // If the same catalogue book is already a distinct row in this club, do // not merge ids: either may own polls, votes or reading history. [$cbDuplicate, $extDuplicate] = $seedExternal("{$TOKEN} Prop Duplicate", $isbn10); - $check($repo->reconcileExternalBooksWithCatalogue($clubId) === 0, 'A18 automatic pass refuses an ambiguous same-club duplicate'); - $check((int) $scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cbDuplicate}") === $extDuplicate, 'A19 ambiguous proposal remains external with its history intact'); + $check($repo->reconcileExternalBooksWithCatalogue($clubId) === 0, 'A20 automatic pass refuses an ambiguous same-club duplicate'); + $check((int) $scalar("SELECT external_book_id FROM bookclub_books WHERE id={$cbDuplicate}") === $extDuplicate, 'A21 ambiguous proposal remains external with its history intact'); echo "B. neverChosenProposals — proposed-but-not-chosen archive (#138 feature)\n"; @@ -244,8 +255,19 @@ $check(str_contains($clubView, "__('Votazioni chiuse')") && str_contains($clubView, '$closedPolls'), 'C1 club page exposes an explicit closed-poll history'); $check(str_contains($pollsView, "__('Votazioni chiuse')") && str_contains($pollsView, '$closedPolls'), 'C2 dedicated polls page exposes closed polls separately from active polls'); $publicController = (string) file_get_contents($root . '/storage/plugins/book-club/src/PublicController.php'); - $check(str_contains($publicController, 'if ($canManage)') && str_contains($publicController, 'reconcileExternalBooksWithCatalogue'), 'C3 manager club view runs automatic exact-ISBN reconciliation before rendering'); - $check(str_contains($publicController, 'closeExpiredForClub'), 'C4 club view closes expired polls before rendering its open/closed sections'); + $mobileController = (string) file_get_contents($root . '/storage/plugins/book-club/src/MobileApiController.php'); + $pollController = (string) file_get_contents($root . '/storage/plugins/book-club/src/PollController.php'); + $pluginSource = (string) file_get_contents($root . '/storage/plugins/book-club/BookClubPlugin.php'); + $check(!str_contains($publicController, 'reconcileExternalBooksWithCatalogue') + && !str_contains($publicController, 'closeExpiredForClub'), 'C3 public club GETs contain no reconciliation or poll-close writes'); + $check(!str_contains($mobileController, 'closeExpiredForClub') + && !str_contains($pollController, 'closeExpiredForClub'), 'C4 web and mobile poll GETs contain no lazy-close writes'); + $check(str_contains($pluginSource, "registerHookInDb('book.save.after', 'onCatalogueBookSaved', 10)") + && str_contains($pluginSource, 'reconcileExternalBooksForCatalogueBook($bookId)'), 'C5 book.save.after is the event-driven reconciliation path'); + $check(str_contains($pluginSource, 'reconcileAllExternalBooksWithCatalogue()') + && str_contains($pluginSource, 'closeExpiredPolls()'), 'C6 scheduled maintenance backfills reconciliation and closes expired polls'); + $check(str_contains($pollController, 'first attempted ballot performs the same idempotent close') + && str_contains($pollController, '$this->resolvePoll($poll, true);'), 'C7 expired vote POST is the immediate poll-close fallback'); } finally { // ── Cleanup (FK-safe) ──────────────────────────────────────────────────── $db->query("DELETE FROM bookclub_poll_options WHERE poll_id IN (SELECT id FROM bookclub_polls WHERE club_id IN (SELECT id FROM bookclub_clubs WHERE slug LIKE '{$TOKEN}-%'))"); diff --git a/tests/code-quality.spec.js b/tests/code-quality.spec.js index 132ab0ac4..892f00e99 100644 --- a/tests/code-quality.spec.js +++ b/tests/code-quality.spec.js @@ -311,10 +311,11 @@ test.describe.serial('Code Quality — 15 static analysis tests', () => { test('10. API endpoints documented in README.md exist in PHP code', () => { const readme = fs.readFileSync(path.join(ROOT, 'README.md'), 'utf-8'); - const phpSources = [ + const phpFiles = [ ...glob('app', '.php', ['vendor']), ...glob('storage/plugins', '.php'), - ].map(f => fs.readFileSync(f, 'utf-8')).join('\n'); + ].map(f => fs.readFileSync(f, 'utf-8')); + const phpSources = phpFiles.join('\n'); const DOC_RE = /`GET (\/(?:api|resync|oai|openurl|archives)[^\s`]+)/g; const violations = []; @@ -330,7 +331,20 @@ test.describe.serial('Code Quality — 15 static analysis tests', () => { // `{}` to align with the PHP-side normalisation above. const pathOnly = m[1].split('?')[0].split('#')[0]; const search = pathOnly.replace(/\/$/, '').replace(/\{[^}:]+(?::[^}]+)?\}/g, '{}'); - if (!normalizedRouteSources.includes(search)) violations.push(m[1]); + let found = normalizedRouteSources.includes(search); + + // Slim plugins commonly register `/api/v1` once with group(), then + // declare child routes as `/auth/...`. Require the group and the + // GET child literal to occur in the SAME PHP file so a documented + // API path cannot be satisfied accidentally by unrelated strings. + if (!found && search.startsWith('/api/v1/')) { + const child = search.slice('/api/v1'.length); + const escapedChild = child.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const groupRe = /->group\(\s*(['"])\/api\/v1\1/; + const getRe = new RegExp(`->get\\(\\s*(['"])${escapedChild}\\1`); + found = phpFiles.some(source => groupRe.test(source) && getRe.test(source)); + } + if (!found) violations.push(m[1]); } expect(violations, `Endpoints documented in README but no matching route found in PHP:\n${violations.map(v => ' GET ' + v).join('\n')}` diff --git a/tests/mobile-api-bookclub-fixes.spec.js b/tests/mobile-api-bookclub-fixes.spec.js index ddc36e25f..98393580c 100644 --- a/tests/mobile-api-bookclub-fixes.spec.js +++ b/tests/mobile-api-bookclub-fixes.spec.js @@ -8,9 +8,9 @@ * * Gate : mobile_api.enabled=0 → POST /auth/login|register|forgot-password * all answer 403 app_access_disabled; enabled=1 → login issues a token - * Lazy close: a poll whose closes_at is in the past is reported closed by - * GET /api/v1/bookclub/clubs/{slug}, with the winner advanced - * in the workflow (voting → selected) and the loser reset + * Poll close : an expired poll remains unchanged on the read-only club GET; + * the first vote POST closes it, advances the winner in the + * workflow (voting → selected), and resets the loser * ISO dates : review created_at/updated_at (GET /me/reviews) and device * created_at/last_used_at (GET /me/devices) match * ^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$ @@ -196,8 +196,8 @@ const SLUG = 'e2e-bookclub-fixes'; /** * Club + membership + two proposals + an EXPIRED open poll where book A got - * the only vote. The bridge's lazy close must resolve it on first read: - * winner voting → selected, loser voting → proposed (default workflow). + * the only vote. The mobile vote POST fallback resolves it without making the + * club-detail GET mutate state: winner voting → selected, loser → proposed. */ function seedClubWithExpiredPoll(userId) { dbExec(`DELETE FROM bookclub_clubs WHERE slug = '${SLUG}'`); // cascades to books/polls/members @@ -242,8 +242,8 @@ function seedClubWithExpiredPoll(userId) { dbExec( // closes_at seeded in UTC with a margin wider than any TZ offset: the app's // web connection runs SET time_zone='+00:00' (RememberMeService), while this - // CLI seed runs in the server's system TZ — NOW()-1h was still in the future - // in UTC on non-UTC hosts, so the lazy close never saw the poll as expired. + // CLI seed runs in the server's system TZ. The wide margin keeps the + // database-clock deadline assertion deterministic on non-UTC hosts. `INSERT INTO bookclub_polls (club_id, title, mode, votes_per_member, anonymity, closes_at, status, created_by) VALUES (${clubId}, 'E2E expired poll', 'simple', 1, 'public', DATE_SUB(UTC_TIMESTAMP(), INTERVAL 26 HOUR), 'open', ${userId})` ); @@ -256,7 +256,7 @@ function seedClubWithExpiredPoll(userId) { ), 10); dbExec(`INSERT INTO bookclub_votes (poll_id, option_id, user_id, value) VALUES (${pollId}, ${optA}, ${userId}, 1)`); - return { clubId, pollId, cbA, cbB, bookA }; + return { clubId, pollId, cbA, cbB, bookA, optA }; } // ─── Request helpers ────────────────────────────────────────────────────────── @@ -278,8 +278,8 @@ test.describe.serial('Mobile API contract fixes — E2E', () => { let token = ''; /** @type {number} */ let userId = 0; - /** @type {{clubId: number, pollId: number, cbA: number, cbB: number, bookA: number}} */ - let seed = { clubId: 0, pollId: 0, cbA: 0, cbB: 0, bookA: 0 }; + /** @type {{clubId: number, pollId: number, cbA: number, cbB: number, bookA: number, optA: number}} */ + let seed = { clubId: 0, pollId: 0, cbA: 0, cbB: 0, bookA: 0, optA: 0 }; test.beforeAll(async ({ browser }) => { await ensurePlugins(browser); @@ -336,9 +336,9 @@ test.describe.serial('Mobile API contract fixes — E2E', () => { token = body.data.token; }); - // ── 2. Lazy close of expired polls on the mobile read path ─────────────── + // ── 2. Read-only GET + expired-vote POST fallback ──────────────────────── - test('expired poll is closed (with winner transition) by GET /bookclub/clubs/{slug}', async ({ request }) => { + test('club GET is read-only; expired vote POST closes the poll with winner transition', async ({ request }) => { const res = await request.get(`${API}/bookclub/clubs/${SLUG}`, { headers: authHeaders(token) }); expect(res.status()).toBe(200); const body = await res.json(); @@ -346,12 +346,33 @@ test.describe.serial('Mobile API contract fixes — E2E', () => { const poll = body.data.polls.find((p) => p.id === seed.pollId); expect(poll, 'seeded poll must be listed').toBeTruthy(); - expect(poll.status).toBe('closed'); + expect(poll.status).toBe('open'); + + // The GET only reports persisted state: no status or workflow writes. + let winner = body.data.books.find((b) => b.id === seed.cbA); + let loser = body.data.books.find((b) => b.id === seed.cbB); + expect(winner.state).toBe('voting'); + expect(loser.state).toBe('voting'); + expect(dbQuery(`SELECT status FROM bookclub_polls WHERE id = ${seed.pollId}`)).toBe('open'); + + // The first mutating request after the deadline rejects the ballot but + // first performs the same idempotent close as scheduled maintenance. + const vote = await request.post( + `${API}/bookclub/clubs/${SLUG}/polls/${seed.pollId}/vote`, + { headers: authHeaders(token), data: { options: [seed.optA] } } + ); + expect(vote.status()).toBe(409); + expect((await vote.json()).error.code).toBe('poll_closed'); + + const after = await request.get(`${API}/bookclub/clubs/${SLUG}`, { headers: authHeaders(token) }); + expect(after.status()).toBe(200); + const afterBody = await after.json(); + expect(afterBody.data.polls.find((p) => p.id === seed.pollId).status).toBe('closed'); // Winner advanced one workflow step past 'voting' (default: selected); // the loser fell back to the entry state. - const winner = body.data.books.find((b) => b.id === seed.cbA); - const loser = body.data.books.find((b) => b.id === seed.cbB); + winner = afterBody.data.books.find((b) => b.id === seed.cbA); + loser = afterBody.data.books.find((b) => b.id === seed.cbB); expect(winner.state).toBe('selected'); expect(loser.state).toBe('proposed'); From 3d8588533adf1fb54e8a37574f2696e85e9b4b99 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 22 Jul 2026 14:13:40 +0200 Subject: [PATCH 04/10] =?UTF-8?q?fix(#138):=20address=20CodeRabbit=20revie?= =?UTF-8?q?w=20=E2=80=94=20US=20spelling=20+=20TZ-safe=20poll=20seed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- locale/en_US.json | 2 +- tests/book-club-uwe.spec.js | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/locale/en_US.json b/locale/en_US.json index 128160fc4..7eac7951c 100644 --- a/locale/en_US.json +++ b/locale/en_US.json @@ -6583,7 +6583,7 @@ "votazione": "poll", "votazioni": "polls", "ultima": "last", - "Non in catalogo": "Not in catalogue", + "Non in catalogo": "Not in catalog", "Votazioni chiuse": "Closed polls", "mai votata": "not yet voted on" } diff --git a/tests/book-club-uwe.spec.js b/tests/book-club-uwe.spec.js index cd85f7896..13cf10b75 100644 --- a/tests/book-club-uwe.spec.js +++ b/tests/book-club-uwe.spec.js @@ -394,7 +394,11 @@ test.describe.serial('Book Club — Uwe feedback', () => { // same idempotent close before redirecting to the history. const pollId = Number(dbQuery(`SELECT id FROM bookclub_polls WHERE club_id=${clubId} AND title=${sqlStr(pollTitle)} LIMIT 1`) || '0'); expect(pollId).toBeGreaterThan(0); - dbQuery(`UPDATE bookclub_polls SET closes_at=DATE_SUB(NOW(), INTERVAL 1 MINUTE), status='open' WHERE id=${pollId}`); + // closes_at seeded in UTC with a margin wider than any TZ offset: the mysql + // CLI seeds in the host time zone while the app connection runs at +00:00, + // so a NOW()-based past instant can still read as future to + // pollDeadlinePassed() on a non-UTC host (cf. mobile-api-bookclub-fixes). + dbQuery(`UPDATE bookclub_polls SET closes_at=DATE_SUB(UTC_TIMESTAMP(), INTERVAL 26 HOUR), status='open' WHERE id=${pollId}`); await page.goto(`${BASE}/book-club/${slug}`); await page.waitForLoadState('networkidle'); expect(dbQuery(`SELECT status FROM bookclub_polls WHERE id=${pollId}`), 'club GET must not close a poll').toBe('open'); From 778650b5e18da2f25b62ceada5246fcaa505f4cd Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 22 Jul 2026 18:04:06 +0200 Subject: [PATCH 05/10] fix(#138): address self-review findings on the GET->hook refactor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- app/Controllers/CsvImportController.php | 9 +++ .../LibraryThingImportController.php | 8 +++ locale/de_DE.json | 3 + locale/en_US.json | 3 + locale/fr_FR.json | 3 + locale/it_IT.json | 3 + storage/plugins/book-club/plugin.json | 4 +- .../book-club/src/MobileApiController.php | 4 ++ .../plugins/book-club/src/PollController.php | 2 +- .../book-club/src/PublicController.php | 13 ++-- storage/plugins/book-club/src/Repo.php | 72 ++++++++++++++++--- .../plugins/book-club/views/public/poll.php | 21 ++++-- .../plugins/book-club/views/public/polls.php | 8 ++- tests/book-club-uwe.spec.js | 4 ++ 14 files changed, 135 insertions(+), 22 deletions(-) diff --git a/app/Controllers/CsvImportController.php b/app/Controllers/CsvImportController.php index c204c180f..d3af6fb83 100644 --- a/app/Controllers/CsvImportController.php +++ b/app/Controllers/CsvImportController.php @@ -396,6 +396,15 @@ public function processChunk(Request $request, Response $response, \mysqli $db): $importData['updated']++; } + // Plugin hook: after book save. Fired OUTSIDE the per-row + // transaction (it just committed) because listeners such as the + // book-club external-proposal reconciler open their own + // transaction — nesting begin_transaction() would implicitly + // commit. Mirrors LibriController's create/edit dispatch so an + // imported book that matches an outstanding club proposal is + // reconciled at import time, not only on the next maintenance run. + \App\Support\Hooks::do('book.save.after', [$bookId, $parsedData]); + // Scraping (if enabled and ISBN exists) if ($enableScraping && !empty($parsedData['isbn13'])) { try { diff --git a/app/Controllers/LibraryThingImportController.php b/app/Controllers/LibraryThingImportController.php index eaa61befa..9570ad410 100644 --- a/app/Controllers/LibraryThingImportController.php +++ b/app/Controllers/LibraryThingImportController.php @@ -466,6 +466,14 @@ public function processChunk(Request $request, Response $response, \mysqli $db): $importData['updated']++; } + // Plugin hook: after book save. Fired OUTSIDE the per-row + // transaction (just committed) — listeners like the book-club + // external-proposal reconciler open their own transaction, and + // nesting begin_transaction() would implicitly commit. Mirrors + // LibriController so a LibraryThing-imported book that matches + // an outstanding club proposal is reconciled at import time. + \App\Support\Hooks::do('book.save.after', [$bookId, $parsedData]); + // NOTE: Cover download disabled during import to avoid timeout on shared hosting // Users can download covers later via the standard enrichment system // if (!empty($parsedData['isbn13']) || !empty($parsedData['isbn10'])) { diff --git a/locale/de_DE.json b/locale/de_DE.json index be4a14ed0..cddbb8b28 100644 --- a/locale/de_DE.json +++ b/locale/de_DE.json @@ -5658,6 +5658,9 @@ "proposto da": "vorgeschlagen von", "scade il": "endet am", "scaduta il": "endete am", + "Votazione terminata": "Abstimmung beendet", + "Questo libro è già collegato a un'altra proposta di questo club: non è stato acquisito di nuovo.": "Dieses Buch ist bereits mit einem anderen Vorschlag dieses Clubs verknüpft: Es wurde nicht erneut übernommen.", + "La scadenza della votazione è passata: la votazione è stata chiusa e il tuo voto non è stato registrato.": "Die Frist der Abstimmung ist abgelaufen: Die Abstimmung wurde geschlossen und deine Stimme wurde nicht gezählt.", "sì": "ja", "votanti": "Teilnehmende", "voti": "Stimmen", diff --git a/locale/en_US.json b/locale/en_US.json index 7eac7951c..90db4404d 100644 --- a/locale/en_US.json +++ b/locale/en_US.json @@ -5658,6 +5658,9 @@ "proposto da": "proposed by", "scade il": "closes on", "scaduta il": "closed on", + "Votazione terminata": "Voting ended", + "Questo libro è già collegato a un'altra proposta di questo club: non è stato acquisito di nuovo.": "This book is already linked to another proposal in this club: it was not acquired again.", + "La scadenza della votazione è passata: la votazione è stata chiusa e il tuo voto non è stato registrato.": "The poll's deadline has passed: the poll has been closed and your vote was not recorded.", "sì": "yes", "votanti": "voters", "voti": "votes", diff --git a/locale/fr_FR.json b/locale/fr_FR.json index 0a3ee2524..ebd02a640 100644 --- a/locale/fr_FR.json +++ b/locale/fr_FR.json @@ -5658,6 +5658,9 @@ "proposto da": "proposé par", "scade il": "expire le", "scaduta il": "expiré le", + "Votazione terminata": "Vote terminé", + "Questo libro è già collegato a un'altra proposta di questo club: non è stato acquisito di nuovo.": "Ce livre est déjà lié à une autre proposition de ce club : il n'a pas été acquis à nouveau.", + "La scadenza della votazione è passata: la votazione è stata chiusa e il tuo voto non è stato registrato.": "La date limite du vote est dépassée : le vote a été clôturé et votre voix n'a pas été enregistrée.", "sì": "oui", "votanti": "votants", "voti": "votes", diff --git a/locale/it_IT.json b/locale/it_IT.json index 57ed32c56..54c4aa8d1 100644 --- a/locale/it_IT.json +++ b/locale/it_IT.json @@ -5658,6 +5658,9 @@ "proposto da": "proposto da", "scade il": "scade il", "scaduta il": "scaduta il", + "Votazione terminata": "Votazione terminata", + "Questo libro è già collegato a un'altra proposta di questo club: non è stato acquisito di nuovo.": "Questo libro è già collegato a un'altra proposta di questo club: non è stato acquisito di nuovo.", + "La scadenza della votazione è passata: la votazione è stata chiusa e il tuo voto non è stato registrato.": "La scadenza della votazione è passata: la votazione è stata chiusa e il tuo voto non è stato registrato.", "sì": "sì", "votanti": "votanti", "voti": "voti", diff --git a/storage/plugins/book-club/plugin.json b/storage/plugins/book-club/plugin.json index 9bb1486b5..f3ff9f3bc 100644 --- a/storage/plugins/book-club/plugin.json +++ b/storage/plugins/book-club/plugin.json @@ -2,8 +2,8 @@ "name": "book-club", "display_name": "Book Club", "description": "Gestione della lettura collaborativa: club di lettura multipli con membri e ruoli, workflow del libro configurabile (Proposto → In votazione → Scelto → In lettura → Discussione → Concluso → Archivio), proposte dal catalogo, votazioni semplici o a preferenza multipla con scadenza e chiusura automatica, incontri con RSVP e feed iCal, dashboard personale multi-club e archivio storico. Progettato per la Discussion #138: dal piccolo circolo comunale alla rete bibliotecaria.", - "version": "1.4.3", - "changelog": "v1.4.3: le pagine club, votazioni e dashboard non riconciliano più libri né chiudono votazioni durante una GET; la riconciliazione delle proposte esterne parte da book.save.after con backfill nella manutenzione completa, mentre le votazioni scadute sono chiuse dalla manutenzione (cron o login admin) e, come fallback immediato, dal primo tentativo di voto POST. v1.4.1: feedback Uwe #138 — la card 'Prossimo incontro' mostra libro/agenda/link video/orario fine; heading proposta esterna più chiaro ('Proponi un libro non ancora in catalogo'); campo 'Proposto da' (un responsabile può attribuire una proposta a un membro attivo); pulsante 'Rimuovi' per togliere un libro dalla lista del club (pulisce anche l'external non acquisito); stampa PDF della lista libri per stato. v1.3.1: bookclub_review_meta non ha più la FK hard verso la tabella core recensioni — un plugin non può assumere presenza/tipo di una tabella core tra install diverse (recensioni è solo in schema.sql, mai in migration → su install vecchi può mancare (1824) o avere id int unsigned (3780), rompendo l attivazione). recensione_id resta colonna indicizzata (UNIQUE). Il bump ri-esegue onActivate/ensureSchema. v1.2.0: ponte API mobile (/api/v1/bookclub con token dell'app, discovery /health con app_access_enabled, timestamp ISO-8601 UTC), modulo prestito tra membri, permessi granulari nei controller, pesi voto configurabili, questionari con bozze modificabili e apertura programmata, citazioni pubbliche sulla scheda libro, restyle completo al design system del frontend. Il bump di versione fa rieseguire onActivate/ensureSchema sulle installazioni già attive (colonne closed_reason/weight_owner/weight_moderator/season_id, tabelle lending, hook book.frontend.details). v1.1.0: Fasi 2-4 del piano — architettura a 16 moduli attivabili per club (auto-discovery). Fase 2: lettura condivisa con sezioni e tracker, discussioni con spoiler protetti/reazioni/menzioni, integrazione biblioteca (copie, prestiti membri, lista d'attesa) e ponte recensioni, votazioni avanzate (stelle, classifica Borda, eliminazione a round, ponderato, quorum, spareggi), statistiche con rollup ed export JSON/CSV, stagioni, ruoli personalizzati con matrice permessi e automazioni. Fase 3: gamification (XP/livelli/badge/classifica), questionari con builder, citazioni e annotazioni con export, reading challenge, affinità tra lettori (opt-in) e suggerimenti, API REST read-only con OpenAPI. Fase 4: reading sprint, buddy reading, assistente IA opt-in (domande di discussione e riassunti verbali). v1.0.0: Fase 1 (MVP) — club, membri e ruoli, inviti email, workflow configurabile, proposte dal catalogo, votazioni simple/multi con chiusura automatica, incontri con RSVP, feed iCal, dashboard multi-club.", + "version": "1.4.4", + "changelog": "v1.4.4: fix da self-review — un sondaggio scaduto ma ancora 'open' e' mostrato come terminato (badge 'Votazione terminata', data 'scaduta il') invece che votabile, ma resta cliccabile perche' il primo voto POST e' il fallback di chiusura senza cron; messaggio chiaro quando il voto arriva a scadenza superata; l'acquisizione di una proposta il cui ISBN e' gia' collegato a un'altra proposta dello stesso club mostra un errore specifico invece che generico; la riconciliazione delle proposte esterne parte anche dagli import CSV/LibraryThing (hook book.save.after dopo il commit, fuori transazione) con guardia anti-nesting e uscita anticipata quando non ci sono proposte esterne pendenti; ORDER BY deterministico nella ricerca ISBN di catalogo. v1.4.3: le pagine club, votazioni e dashboard non riconciliano più libri né chiudono votazioni durante una GET; la riconciliazione delle proposte esterne parte da book.save.after con backfill nella manutenzione completa, mentre le votazioni scadute sono chiuse dalla manutenzione (cron o login admin) e, come fallback immediato, dal primo tentativo di voto POST. v1.4.1: feedback Uwe #138 — la card 'Prossimo incontro' mostra libro/agenda/link video/orario fine; heading proposta esterna più chiaro ('Proponi un libro non ancora in catalogo'); campo 'Proposto da' (un responsabile può attribuire una proposta a un membro attivo); pulsante 'Rimuovi' per togliere un libro dalla lista del club (pulisce anche l'external non acquisito); stampa PDF della lista libri per stato. v1.3.1: bookclub_review_meta non ha più la FK hard verso la tabella core recensioni — un plugin non può assumere presenza/tipo di una tabella core tra install diverse (recensioni è solo in schema.sql, mai in migration → su install vecchi può mancare (1824) o avere id int unsigned (3780), rompendo l attivazione). recensione_id resta colonna indicizzata (UNIQUE). Il bump ri-esegue onActivate/ensureSchema. v1.2.0: ponte API mobile (/api/v1/bookclub con token dell'app, discovery /health con app_access_enabled, timestamp ISO-8601 UTC), modulo prestito tra membri, permessi granulari nei controller, pesi voto configurabili, questionari con bozze modificabili e apertura programmata, citazioni pubbliche sulla scheda libro, restyle completo al design system del frontend. Il bump di versione fa rieseguire onActivate/ensureSchema sulle installazioni già attive (colonne closed_reason/weight_owner/weight_moderator/season_id, tabelle lending, hook book.frontend.details). v1.1.0: Fasi 2-4 del piano — architettura a 16 moduli attivabili per club (auto-discovery). Fase 2: lettura condivisa con sezioni e tracker, discussioni con spoiler protetti/reazioni/menzioni, integrazione biblioteca (copie, prestiti membri, lista d'attesa) e ponte recensioni, votazioni avanzate (stelle, classifica Borda, eliminazione a round, ponderato, quorum, spareggi), statistiche con rollup ed export JSON/CSV, stagioni, ruoli personalizzati con matrice permessi e automazioni. Fase 3: gamification (XP/livelli/badge/classifica), questionari con builder, citazioni e annotazioni con export, reading challenge, affinità tra lettori (opt-in) e suggerimenti, API REST read-only con OpenAPI. Fase 4: reading sprint, buddy reading, assistente IA opt-in (domande di discussione e riassunti verbali). v1.0.0: Fase 1 (MVP) — club, membri e ruoli, inviti email, workflow configurabile, proposte dal catalogo, votazioni simple/multi con chiusura automatica, incontri con RSVP, feed iCal, dashboard multi-club.", "author": "Fabiodalez", "author_url": "", "plugin_url": "https://github.com/fabiodalez-dev/Pinakes/discussions/138", diff --git a/storage/plugins/book-club/src/MobileApiController.php b/storage/plugins/book-club/src/MobileApiController.php index bd1d1db05..5b616a067 100644 --- a/storage/plugins/book-club/src/MobileApiController.php +++ b/storage/plugins/book-club/src/MobileApiController.php @@ -153,6 +153,10 @@ public function clubDetail(ServerRequestInterface $request, ResponseInterface $r 'id' => (int) $poll['id'], 'title' => (string) $poll['title'], 'mode' => (string) $poll['mode'], + // Persisted state only (read-only GET contract, intentional): an + // expired-but-open poll stays 'open' here; the app's vote attempt + // is answered with a 409 poll_closed once the deadline has passed + // (MobileApiController::vote closes it then). 'status' => (string) $poll['status'], 'closes_at' => self::isoUtc($poll['closes_at']), 'votes_per_member' => (int) $poll['votes_per_member'], diff --git a/storage/plugins/book-club/src/PollController.php b/storage/plugins/book-club/src/PollController.php index 792ab5c7c..88bbdce90 100644 --- a/storage/plugins/book-club/src/PollController.php +++ b/storage/plugins/book-club/src/PollController.php @@ -304,7 +304,7 @@ public function vote(ServerRequestInterface $request, ResponseInterface $respons // is rejected. GET routes remain read-only. if ($poll['status'] === 'open' && $this->repo->pollDeadlinePassed($pollId)) { $this->closeExpiredPollById($pollId); - $this->flash('error', __('La votazione è chiusa.')); + $this->flash('error', __('La scadenza della votazione è passata: la votazione è stata chiusa e il tuo voto non è stato registrato.')); return $this->redirect($response, '/book-club/' . $slug . '/polls/' . $pollId); } if ($poll['status'] !== 'open') { diff --git a/storage/plugins/book-club/src/PublicController.php b/storage/plugins/book-club/src/PublicController.php index ada5c4fb0..b0b754099 100644 --- a/storage/plugins/book-club/src/PublicController.php +++ b/storage/plugins/book-club/src/PublicController.php @@ -504,10 +504,15 @@ public function acquireBook(ServerRequestInterface $request, ResponseInterface $ $this->flash('error', __('Questo libro è già in catalogo.')); return $this->redirect($response, '/book-club/' . $slug); } - $libroId = $this->repo->acquireExternalBook($bookId); - $this->flash($libroId !== null ? 'success' : 'error', $libroId !== null - ? __('Libro acquisito e aggiunto al catalogo.') - : __('Acquisizione non riuscita, riprova.')); + $acquireReason = null; + $libroId = $this->repo->acquireExternalBook($bookId, $acquireReason); + if ($libroId !== null) { + $this->flash('success', __('Libro acquisito e aggiunto al catalogo.')); + } elseif ($acquireReason === 'already_linked') { + $this->flash('error', __('Questo libro è già collegato a un\'altra proposta di questo club: non è stato acquisito di nuovo.')); + } else { + $this->flash('error', __('Acquisizione non riuscita, riprova.')); + } return $this->redirect($response, '/book-club/' . $slug); } diff --git a/storage/plugins/book-club/src/Repo.php b/storage/plugins/book-club/src/Repo.php index a2f02160c..9bd6481b7 100644 --- a/storage/plugins/book-club/src/Repo.php +++ b/storage/plugins/book-club/src/Repo.php @@ -24,6 +24,18 @@ class Repo private mysqli $db; + /** + * Guards reconcileExternalBooksWithCatalogue() against opening a nested + * transaction on the same connection: mysqli's begin_transaction() + * implicitly commits any transaction already in progress. MySQL exposes no + * portable "am I in a transaction?" flag (@@in_transaction is MariaDB-only), + * so we track ownership ourselves. Callers on OTHER components must still + * never invoke reconcile while holding their own transaction — the hook + * dispatch sites (LibriController, the import controllers) fire it outside + * any transaction for exactly this reason. + */ + private bool $ownsReconcileTx = false; + public function __construct(mysqli $db) { $this->db = $db; @@ -835,11 +847,15 @@ private function activeCatalogueBookIdByIsbn(?string $isbn13, ?string $isbn10): if ($isbn13 === null && $isbn10 === null) { return null; } + // ORDER BY id keeps the pick deterministic when the core catalogue + // legitimately holds more than one active book with the same ISBN + // (homonym editions): always link to the oldest matching row. $row = $this->row( 'SELECT id FROM libri WHERE deleted_at IS NULL AND ((isbn13 IS NOT NULL AND isbn13 = ?) OR (isbn10 IS NOT NULL AND isbn10 = ?)) + ORDER BY id ASC LIMIT 1 FOR UPDATE', 'ss', @@ -908,7 +924,13 @@ private function linkExternalBookToCatalogue(int $clubId, int $clubBookId, int $ */ public function reconcileExternalBooksWithCatalogue(int $clubId): int { - $this->db->begin_transaction(); + // Reuse an in-flight reconcile transaction instead of nesting a new one + // (begin_transaction() would implicitly commit the outer one). + $ownsTx = !$this->ownsReconcileTx; + if ($ownsTx) { + $this->db->begin_transaction(); + $this->ownsReconcileTx = true; + } try { $candidates = $this->rows( "SELECT cb.id AS club_book_id, cb.external_book_id, ext.isbn @@ -941,10 +963,16 @@ public function reconcileExternalBooksWithCatalogue(int $clubId): int } } - $this->db->commit(); + if ($ownsTx) { + $this->db->commit(); + $this->ownsReconcileTx = false; + } return $reconciled; } catch (\Throwable $e) { - $this->db->rollback(); + if ($ownsTx) { + $this->db->rollback(); + $this->ownsReconcileTx = false; + } SecureLogger::error('[BookClub] automatic external reconciliation rolled back: ' . $e->getMessage()); return 0; } @@ -962,6 +990,17 @@ public function reconcileExternalBooksForCatalogueBook(int $libroId): int return 0; } + // Cheap guard: this fires on EVERY catalogue save (via book.save.after). + // When no club has an outstanding external proposal to reconcile — the + // common case — skip the book lookup and the candidate scan entirely. + if ($this->row( + "SELECT 1 FROM bookclub_external_books + WHERE acquired_libro_id IS NULL AND isbn IS NOT NULL AND TRIM(isbn) <> '' + LIMIT 1" + ) === null) { + return 0; + } + $book = $this->row( 'SELECT isbn13, isbn10 FROM libri WHERE id = ? AND deleted_at IS NULL LIMIT 1', 'i', @@ -1035,8 +1074,9 @@ public function reconcileAllExternalBooksWithCatalogue(): int * library), repoint the club-book to it, and stamp the external row. * Returns the new libro_id, or null if the club-book is not external / fails. */ - public function acquireExternalBook(int $clubBookId): ?int + public function acquireExternalBook(int $clubBookId, ?string &$reason = null): ?int { + $reason = null; $this->db->begin_transaction(); try { $row = $this->row( @@ -1107,7 +1147,13 @@ public function acquireExternalBook(int $clubBookId): ?int } if (!$this->linkExternalBookToCatalogue((int) $row['club_id'], $clubBookId, $extId, $libroId)) { - throw new \RuntimeException('catalog acquisition repoint failed'); + // Refuse-to-merge: this club already has another proposal linked + // to the same catalogue book. Not a retryable failure — signal + // the specific reason so the caller can tell the manager instead + // of showing a generic error. + $reason = 'already_linked'; + $this->db->rollback(); + return null; } $this->db->commit(); @@ -1259,7 +1305,13 @@ public function deleteClubBook(int $clubBookId): bool /** @return array|null */ public function poll(int $pollId): ?array { - return $this->row('SELECT * FROM bookclub_polls WHERE id = ?', 'i', [$pollId]); + return $this->row( + "SELECT *, + (status = 'open' AND closes_at IS NOT NULL AND closes_at <= NOW()) AS deadline_passed + FROM bookclub_polls WHERE id = ?", + 'i', + [$pollId] + ); } /** @return list> */ @@ -1267,7 +1319,8 @@ public function clubPolls(int $clubId): array { return $this->rows( "SELECT p.*, - (SELECT COUNT(DISTINCT v.user_id) FROM bookclub_votes v WHERE v.poll_id = p.id) AS voter_count + (SELECT COUNT(DISTINCT v.user_id) FROM bookclub_votes v WHERE v.poll_id = p.id) AS voter_count, + (p.status = 'open' AND p.closes_at IS NOT NULL AND p.closes_at <= NOW()) AS deadline_passed FROM bookclub_polls p WHERE p.club_id = ? ORDER BY FIELD(p.status,'open','closed'), p.created_at DESC", @@ -1681,7 +1734,10 @@ public function clubSnapshot(array $club): array ); } $openPolls = $this->rows( - "SELECT id, title, closes_at FROM bookclub_polls WHERE club_id = ? AND status = 'open' ORDER BY created_at DESC", + "SELECT id, title, closes_at FROM bookclub_polls + WHERE club_id = ? AND status = 'open' + AND (closes_at IS NULL OR closes_at > NOW()) + ORDER BY created_at DESC", 'i', [(int) $club['id']] ); diff --git a/storage/plugins/book-club/views/public/poll.php b/storage/plugins/book-club/views/public/poll.php index cf00468ef..cdc35579d 100644 --- a/storage/plugins/book-club/views/public/poll.php +++ b/storage/plugins/book-club/views/public/poll.php @@ -25,6 +25,13 @@ $e = static fn(mixed $v): string => htmlspecialchars((string) $v, ENT_QUOTES, 'UTF-8'); $slug = (string) $club['slug']; $csrf = \App\Support\Csrf::ensureToken(); +// A poll whose deadline (MySQL clock) has passed but whose status is still +// 'open' in the DB is shown as ended (an "ended" badge, past-tense deadline) +// so members know it is over — but the ballot stays reachable so the first +// vote POST still closes it (the intentional cron-free fallback), and any such +// attempt is answered with a clear "deadline passed" message. This is +// read-only: the display never writes; the authoritative close is the POST. +$deadlinePassed = $poll['status'] === 'open' && !empty($poll['deadline_passed']); $isOpen = $poll['status'] === 'open'; $mode = (string) ($poll['mode'] ?? 'simple'); $round = max(1, (int) ($poll['round'] ?? 1)); @@ -122,7 +129,7 @@ · - · + · @@ -140,9 +147,13 @@ - - - + + + + + + + @@ -281,7 +292,7 @@ class="d-flex align-items-center justify-content-between gap-3" - + 2; $confirmMsg = $isRoundClose diff --git a/storage/plugins/book-club/views/public/polls.php b/storage/plugins/book-club/views/public/polls.php index 9e0b5b659..aba739d45 100644 --- a/storage/plugins/book-club/views/public/polls.php +++ b/storage/plugins/book-club/views/public/polls.php @@ -105,11 +105,15 @@ · - · + · - + + + + + diff --git a/tests/book-club-uwe.spec.js b/tests/book-club-uwe.spec.js index 13cf10b75..e71fd8fb5 100644 --- a/tests/book-club-uwe.spec.js +++ b/tests/book-club-uwe.spec.js @@ -405,6 +405,10 @@ test.describe.serial('Book Club — Uwe feedback', () => { await page.goto(`${BASE}/book-club/${slug}/polls/${pollId}`); await page.waitForLoadState('domcontentloaded'); expect(dbQuery(`SELECT status FROM bookclub_polls WHERE id=${pollId}`), 'poll GET must not close a poll').toBe('open'); + // #280 self-review: an expired-but-still-open poll is shown as ended + // (read-only display, no DB write) instead of a votable "Aperta" poll — + // the ballot stays so the first vote POST can still perform the close. + await expect(page.locator('.bc-badge:has-text("Votazione terminata")').first()).toBeVisible(); await page.locator(`form[action$="/polls/${pollId}/vote"] button[type="submit"]`).click(); await page.waitForLoadState('networkidle'); expect(dbQuery(`SELECT status FROM bookclub_polls WHERE id=${pollId}`)).toBe('closed'); From 6b0a078fcb8285f679b2c9bff805453a28ff8341 Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 22 Jul 2026 19:11:29 +0200 Subject: [PATCH 06/10] =?UTF-8?q?fix(#138):=20address=20CodeRabbit=20revie?= =?UTF-8?q?w=20=E2=80=94=20fr=20grammar,=20nested-tx=20propagation,=20comp?= =?UTF-8?q?lexity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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). --- locale/fr_FR.json | 2 +- storage/plugins/book-club/src/Repo.php | 102 ++++++++++++++++--------- 2 files changed, 65 insertions(+), 39 deletions(-) diff --git a/locale/fr_FR.json b/locale/fr_FR.json index ebd02a640..ccfaf0ce3 100644 --- a/locale/fr_FR.json +++ b/locale/fr_FR.json @@ -5660,7 +5660,7 @@ "scaduta il": "expiré le", "Votazione terminata": "Vote terminé", "Questo libro è già collegato a un'altra proposta di questo club: non è stato acquisito di nuovo.": "Ce livre est déjà lié à une autre proposition de ce club : il n'a pas été acquis à nouveau.", - "La scadenza della votazione è passata: la votazione è stata chiusa e il tuo voto non è stato registrato.": "La date limite du vote est dépassée : le vote a été clôturé et votre voix n'a pas été enregistrée.", + "La scadenza della votazione è passata: la votazione è stata chiusa e il tuo voto non è stato registrato.": "La date limite du vote est dépassée : le vote a été clôturé et votre vote n'a pas été enregistré.", "sì": "oui", "votanti": "votants", "voti": "votes", diff --git a/storage/plugins/book-club/src/Repo.php b/storage/plugins/book-club/src/Repo.php index 9bd6481b7..78fb0a06c 100644 --- a/storage/plugins/book-club/src/Repo.php +++ b/storage/plugins/book-club/src/Repo.php @@ -972,9 +972,13 @@ public function reconcileExternalBooksWithCatalogue(int $clubId): int if ($ownsTx) { $this->db->rollback(); $this->ownsReconcileTx = false; + SecureLogger::error('[BookClub] automatic external reconciliation rolled back: ' . $e->getMessage()); + return 0; } - SecureLogger::error('[BookClub] automatic external reconciliation rolled back: ' . $e->getMessage()); - return 0; + // Nested call reusing a caller's transaction: propagate so the owner + // rolls back instead of committing partial state (swallowing here and + // returning 0 would hide the failure from the transaction owner). + throw $e; } } @@ -1108,42 +1112,7 @@ public function acquireExternalBook(int $clubBookId, ?string &$reason = null): ? // authors and publisher, so create nothing new. $libroId = $existingId; } else { - $publisherId = $this->findOrCreatePublisher(isset($row['editore']) ? (string) $row['editore'] : null); - - // Only `titolo` is mandatory in `libri`; everything else is optional. - $inserted = $publisherId !== null - ? $this->exec( - 'INSERT INTO libri (titolo, anno_pubblicazione, isbn13, isbn10, copertina_url, editore_id) VALUES (?, ?, ?, ?, ?, ?)', - 'sisssi', - [$titolo, $anno, $isbn13, $isbn10, $cover, $publisherId] - ) - : $this->exec( - 'INSERT INTO libri (titolo, anno_pubblicazione, isbn13, isbn10, copertina_url) VALUES (?, ?, ?, ?, ?)', - 'sisss', - [$titolo, $anno, $isbn13, $isbn10, $cover] - ); - if (!$inserted) { - throw new \RuntimeException('catalog insert failed'); - } - $libroId = (int) $this->db->insert_id; - $this->attachExternalAuthors($libroId, isset($row['autori']) ? (string) $row['autori'] : null); - $this->attachPrimaryPublisher($libroId, $publisherId); - SearchIndexBuilder::rebuild($this->db, $libroId); - - // Give the acquired book one physical copy, matching how the normal - // catalogue-creation flow (LibriController) seeds copies. `libri` - // defaults copie_totali/copie_disponibili to 1, so WITHOUT a matching - // `copie` row the book would claim one available copy but have none - // to lend — and the per-copy features (labels, loan/return by code) - // would find nothing. Inventory number LIB-{id}, same as a single - // manually-created copy; the whole thing is inside this transaction. - if (!$this->exec( - "INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (?, ?, 'disponibile')", - 'is', - [$libroId, 'LIB-' . $libroId] - )) { - throw new \RuntimeException('catalog copy creation failed'); - } + $libroId = $this->createCatalogueBookFromExternal($row, $titolo, $anno, $isbn13, $isbn10, $cover); } if (!$this->linkExternalBookToCatalogue((int) $row['club_id'], $clubBookId, $extId, $libroId)) { @@ -1165,6 +1134,63 @@ public function acquireExternalBook(int $clubBookId, ?string &$reason = null): ? } } + /** + * Create the real `libri` row (+ publisher, authors, search index, one + * physical copy) for an acquired external proposal. Runs INSIDE + * acquireExternalBook's transaction — it never commits/rolls back — and + * throws on any failure so the caller's catch rolls the whole acquisition + * back. Extracted from acquireExternalBook to keep its complexity in check. + * + * @param array $row the joined bookclub_books + external row + */ + private function createCatalogueBookFromExternal( + array $row, + string $titolo, + ?int $anno, + ?string $isbn13, + ?string $isbn10, + ?string $cover + ): int { + $publisherId = $this->findOrCreatePublisher(isset($row['editore']) ? (string) $row['editore'] : null); + + // Only `titolo` is mandatory in `libri`; everything else is optional. + $inserted = $publisherId !== null + ? $this->exec( + 'INSERT INTO libri (titolo, anno_pubblicazione, isbn13, isbn10, copertina_url, editore_id) VALUES (?, ?, ?, ?, ?, ?)', + 'sisssi', + [$titolo, $anno, $isbn13, $isbn10, $cover, $publisherId] + ) + : $this->exec( + 'INSERT INTO libri (titolo, anno_pubblicazione, isbn13, isbn10, copertina_url) VALUES (?, ?, ?, ?, ?)', + 'sisss', + [$titolo, $anno, $isbn13, $isbn10, $cover] + ); + if (!$inserted) { + throw new \RuntimeException('catalog insert failed'); + } + $libroId = (int) $this->db->insert_id; + $this->attachExternalAuthors($libroId, isset($row['autori']) ? (string) $row['autori'] : null); + $this->attachPrimaryPublisher($libroId, $publisherId); + SearchIndexBuilder::rebuild($this->db, $libroId); + + // Give the acquired book one physical copy, matching how the normal + // catalogue-creation flow (LibriController) seeds copies. `libri` + // defaults copie_totali/copie_disponibili to 1, so WITHOUT a matching + // `copie` row the book would claim one available copy but have none + // to lend — and the per-copy features (labels, loan/return by code) + // would find nothing. Inventory number LIB-{id}, same as a single + // manually-created copy; the whole thing is inside this transaction. + if (!$this->exec( + "INSERT INTO copie (libro_id, numero_inventario, stato) VALUES (?, ?, 'disponibile')", + 'is', + [$libroId, 'LIB-' . $libroId] + )) { + throw new \RuntimeException('catalog copy creation failed'); + } + + return $libroId; + } + public function changeBookState(int $clubBookId, string $fromState, string $toState, ?int $changedBy): bool { $ok = $this->exec('UPDATE bookclub_books SET state = ? WHERE id = ?', 'si', [$toState, $clubBookId]); From 10dc65f95bd6449a6226f4429b78af9b43601d9f Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Wed, 22 Jul 2026 20:58:58 +0200 Subject: [PATCH 07/10] test: harden API route validation and PR compatibility --- locale/de_DE.json | 16 +++++++- locale/en_US.json | 16 +++++++- locale/fr_FR.json | 16 +++++++- locale/it_IT.json | 16 +++++++- tests/code-quality.spec.js | 81 ++++++++++++++++++++++++++++++++++---- 5 files changed, 133 insertions(+), 12 deletions(-) diff --git a/locale/de_DE.json b/locale/de_DE.json index cddbb8b28..6bc4df30a 100644 --- a/locale/de_DE.json +++ b/locale/de_DE.json @@ -6588,5 +6588,19 @@ "ultima": "zuletzt", "Non in catalogo": "Nicht im Katalog", "Votazioni chiuse": "Geschlossene Abstimmungen", - "mai votata": "noch nicht abgestimmt" + "mai votata": "noch nicht abgestimmt", + "Seleziona tutti i prestiti estendibili": "Alle verlängerbaren Ausleihen auswählen", + "Estendi di": "Verlängern um", + "Estendi prestiti selezionati": "Ausgewählte Ausleihen verlängern", + "%d prestiti selezionati": "%d Ausleihen ausgewählt", + "Estendere i prestiti selezionati?": "Ausgewählte Ausleihen verlängern?", + "%n prestiti verranno estesi di %d giorni.": "%n Ausleihen werden um %d Tage verlängert.", + "Prestiti estesi": "Ausleihen verlängert", + "Nessun prestito esteso": "Keine Ausleihe verlängert", + "%n prestiti estesi di %d giorni.": "%n Ausleihen um %d Tage verlängert.", + "Selezione o numero di giorni non validi.": "Ungültige Auswahl oder Anzahl der Tage.", + "Estensione non riuscita, riprova.": "Verlängerung fehlgeschlagen, bitte erneut versuchen.", + "Estensione annullata: almeno un prestito entrerebbe in conflitto con un altro prestito o una prenotazione.": "Verlängerung abgebrochen: Mindestens eine Ausleihe würde mit einer anderen Ausleihe oder Reservierung in Konflikt geraten.", + "Estendi": "Verlängern", + "Lingue non disponibili.": "Sprachen sind nicht verfügbar." } diff --git a/locale/en_US.json b/locale/en_US.json index 90db4404d..3d5ff4a3b 100644 --- a/locale/en_US.json +++ b/locale/en_US.json @@ -6588,5 +6588,19 @@ "ultima": "last", "Non in catalogo": "Not in catalog", "Votazioni chiuse": "Closed polls", - "mai votata": "not yet voted on" + "mai votata": "not yet voted on", + "Seleziona tutti i prestiti estendibili": "Select all extendable loans", + "Estendi di": "Extend by", + "Estendi prestiti selezionati": "Extend selected loans", + "%d prestiti selezionati": "%d loans selected", + "Estendere i prestiti selezionati?": "Extend the selected loans?", + "%n prestiti verranno estesi di %d giorni.": "%n loans will be extended by %d days.", + "Prestiti estesi": "Loans extended", + "Nessun prestito esteso": "No loan extended", + "%n prestiti estesi di %d giorni.": "%n loans extended by %d days.", + "Selezione o numero di giorni non validi.": "Invalid selection or number of days.", + "Estensione non riuscita, riprova.": "Extension failed, please try again.", + "Estensione annullata: almeno un prestito entrerebbe in conflitto con un altro prestito o una prenotazione.": "Extension cancelled: at least one loan would conflict with another loan or reservation.", + "Estendi": "Extend", + "Lingue non disponibili.": "Languages are unavailable." } diff --git a/locale/fr_FR.json b/locale/fr_FR.json index ccfaf0ce3..df10f4cdd 100644 --- a/locale/fr_FR.json +++ b/locale/fr_FR.json @@ -6588,5 +6588,19 @@ "ultima": "dernière", "Non in catalogo": "Pas au catalogue", "Votazioni chiuse": "Votes clos", - "mai votata": "pas encore soumise au vote" + "mai votata": "pas encore soumise au vote", + "Seleziona tutti i prestiti estendibili": "Sélectionner tous les prêts prolongeables", + "Estendi di": "Prolonger de", + "Estendi prestiti selezionati": "Prolonger les prêts sélectionnés", + "%d prestiti selezionati": "%d prêts sélectionnés", + "Estendere i prestiti selezionati?": "Prolonger les prêts sélectionnés ?", + "%n prestiti verranno estesi di %d giorni.": "%n prêts seront prolongés de %d jours.", + "Prestiti estesi": "Prêts prolongés", + "Nessun prestito esteso": "Aucun prêt prolongé", + "%n prestiti estesi di %d giorni.": "%n prêts prolongés de %d jours.", + "Selezione o numero di giorni non validi.": "Sélection ou nombre de jours invalide.", + "Estensione non riuscita, riprova.": "La prolongation a échoué, veuillez réessayer.", + "Estensione annullata: almeno un prestito entrerebbe in conflitto con un altro prestito o una prenotazione.": "Prolongation annulée : au moins un prêt entrerait en conflit avec un autre prêt ou une réservation.", + "Estendi": "Prolonger", + "Lingue non disponibili.": "Les langues ne sont pas disponibles." } diff --git a/locale/it_IT.json b/locale/it_IT.json index 54c4aa8d1..31e21b6cd 100644 --- a/locale/it_IT.json +++ b/locale/it_IT.json @@ -6588,5 +6588,19 @@ "ultima": "ultima", "Non in catalogo": "Non in catalogo", "Votazioni chiuse": "Votazioni chiuse", - "mai votata": "mai votata" + "mai votata": "mai votata", + "Seleziona tutti i prestiti estendibili": "Seleziona tutti i prestiti estendibili", + "Estendi di": "Estendi di", + "Estendi prestiti selezionati": "Estendi prestiti selezionati", + "%d prestiti selezionati": "%d prestiti selezionati", + "Estendere i prestiti selezionati?": "Estendere i prestiti selezionati?", + "%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.", + "Selezione o numero di giorni non validi.": "Selezione o numero di giorni non validi.", + "Estensione non riuscita, riprova.": "Estensione non riuscita, riprova.", + "Estensione annullata: almeno un prestito entrerebbe in conflitto con un altro prestito o una prenotazione.": "Estensione annullata: almeno un prestito entrerebbe in conflitto con un altro prestito o una prenotazione.", + "Estendi": "Estendi", + "Lingue non disponibili.": "Lingue non disponibili." } diff --git a/tests/code-quality.spec.js b/tests/code-quality.spec.js index 892f00e99..93ab51d20 100644 --- a/tests/code-quality.spec.js +++ b/tests/code-quality.spec.js @@ -321,6 +321,67 @@ test.describe.serial('Code Quality — 15 static analysis tests', () => { const violations = []; const normalizedPhpSources = phpSources.replace(/\{([^}:]+):[^}]+\}/g, '{$1}'); const normalizedRouteSources = normalizedPhpSources.replace(/\{[^}:]+(?::[^}]+)?\}/g, '{}'); + + // Return only the lexical body of each /api/v1 group callback. Looking + // for a group and a child route anywhere in the same file is too weak: + // an unrelated top-level GET later in that file would be a false pass. + const apiV1GroupBodies = source => { + const bodies = []; + const groupRe = /->group\(\s*(['"])\/api\/v1\1\s*,\s*function\b[^{]*\{/g; + let groupMatch; + while ((groupMatch = groupRe.exec(source)) !== null) { + const open = groupRe.lastIndex - 1; + let depth = 0; + let quote = null; + let escaped = false; + let lineComment = false; + let blockComment = false; + for (let i = open; i < source.length; i++) { + const ch = source[i]; + const next = source[i + 1] ?? ''; + if (lineComment) { + if (ch === '\n') lineComment = false; + continue; + } + if (blockComment) { + if (ch === '*' && next === '/') { blockComment = false; i++; } + continue; + } + if (quote !== null) { + if (escaped) { escaped = false; continue; } + if (ch === '\\') { escaped = true; continue; } + if (ch === quote) quote = null; + continue; + } + if (ch === '/' && next === '/') { lineComment = true; i++; continue; } + if (ch === '#') { lineComment = true; continue; } + if (ch === '/' && next === '*') { blockComment = true; i++; continue; } + if (ch === "'" || ch === '"' || ch === '`') { quote = ch; continue; } + if (ch === '{') depth++; + if (ch === '}' && --depth === 0) { + bodies.push(source.slice(open + 1, i)); + groupRe.lastIndex = i + 1; + break; + } + } + } + return bodies; + }; + const groupedGetExists = (source, child) => { + const escapedChild = child.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const getRe = new RegExp(`->get\\(\\s*(['"])${escapedChild}\\1`); + return apiV1GroupBodies(source).some(body => { + const normalized = body + .replace(/\{([^}:]+):[^}]+\}/g, '{$1}') + .replace(/\{[^}:]+(?::[^}]+)?\}/g, '{}'); + return getRe.test(normalized); + }); + }; + const falsePositiveFixture = "$app->group('/api/v1', function ($g) {}); $app->get('/auth/registration-fields', $handler);"; + expect(groupedGetExists(falsePositiveFixture, '/auth/registration-fields'), + 'a top-level GET outside the /api/v1 callback must not satisfy the route check' + ).toBe(false); + let m; while ((m = DOC_RE.exec(readme)) !== null) { // Strip the query string + hash before matching against the PHP @@ -334,15 +395,19 @@ test.describe.serial('Code Quality — 15 static analysis tests', () => { let found = normalizedRouteSources.includes(search); // Slim plugins commonly register `/api/v1` once with group(), then - // declare child routes as `/auth/...`. Require the group and the - // GET child literal to occur in the SAME PHP file so a documented - // API path cannot be satisfied accidentally by unrelated strings. - if (!found && search.startsWith('/api/v1/')) { + // declare child routes as `/auth/...`. Require the GET child literal + // inside that exact group's callback so unrelated routes elsewhere + // in the same PHP file cannot satisfy a documented API path. + if (search.startsWith('/api/v1/')) { const child = search.slice('/api/v1'.length); - const escapedChild = child.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const groupRe = /->group\(\s*(['"])\/api\/v1\1/; - const getRe = new RegExp(`->get\\(\\s*(['"])${escapedChild}\\1`); - found = phpFiles.some(source => groupRe.test(source) && getRe.test(source)); + const escapedSearch = search.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const directGetRe = new RegExp(`->get\\(\\s*(['"])${escapedSearch}\\1`); + found = phpFiles.some(source => { + const normalized = source + .replace(/\{([^}:]+):[^}]+\}/g, '{$1}') + .replace(/\{[^}:]+(?::[^}]+)?\}/g, '{}'); + return directGetRe.test(normalized) || groupedGetExists(source, child); + }); } if (!found) violations.push(m[1]); } From fb72eb8b7bf5bcb2a2b2c34e0a8bb7e49cb6ceca Mon Sep 17 00:00:00 2001 From: fabiodalez-dev Date: Thu, 23 Jul 2026 16:07:51 +0200 Subject: [PATCH 08/10] fix(#138): address self-review findings (show.php poll badge, importer hook isolation) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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"
used the bc-summary class, which was defined only in show.php's