diff --git a/app/Controllers/CsvImportController.php b/app/Controllers/CsvImportController.php index c204c180f..3aa353e64 100644 --- a/app/Controllers/CsvImportController.php +++ b/app/Controllers/CsvImportController.php @@ -396,6 +396,23 @@ 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. + // Isolated in its own try/catch: the row is already committed and + // counted, so a throwing listener must NOT reach the per-row catch + // below (which would rollback — a no-op here — and additionally + // record the committed row as a failed import, double-counting it). + try { + \App\Support\Hooks::do('book.save.after', [$bookId, $parsedData]); + } catch (\Throwable $hookError) { + \App\Support\SecureLogger::warning('[CsvImport] book.save.after listener failed (row already committed)', ['book_id' => $bookId, 'error' => $hookError->getMessage()]); + } + // 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..d9979ba38 100644 --- a/app/Controllers/LibraryThingImportController.php +++ b/app/Controllers/LibraryThingImportController.php @@ -466,6 +466,23 @@ 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. + // Isolated in its own try/catch: the row is already committed + // and counted, so a throwing listener must NOT reach the + // per-row catch below (which would rollback — a no-op here — + // and additionally record the committed row as a failed + // import, double-counting it). + try { + \App\Support\Hooks::do('book.save.after', [$bookId, $parsedData]); + } catch (\Throwable $hookError) { + \App\Support\SecureLogger::warning('[LibraryThingImport] book.save.after listener failed (row already committed)', ['book_id' => $bookId, 'error' => $hookError->getMessage()]); + } + // 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 5e5bd7d5f..923e6dbd0 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", @@ -6577,5 +6580,27 @@ "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", + "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", + "Votazioni chiuse": "Geschlossene Abstimmungen", + "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?", + "%s prestiti verranno estesi di %d giorni.": "%s Ausleihen werden um %d Tage verlängert.", + "Prestiti estesi": "Ausleihen verlängert", + "Nessun prestito esteso": "Keine Ausleihe verlängert", + "%s prestiti estesi di %d giorni.": "%s 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 0bd39013f..ac9813d6f 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", @@ -6577,5 +6580,27 @@ "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", + "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 catalog", + "Votazioni chiuse": "Closed polls", + "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?", + "%s prestiti verranno estesi di %d giorni.": "%s loans will be extended by %d days.", + "Prestiti estesi": "Loans extended", + "Nessun prestito esteso": "No loan extended", + "%s prestiti estesi di %d giorni.": "%s 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 2ea7a0674..117184a4d 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 vote n'a pas été enregistré.", "sì": "oui", "votanti": "votants", "voti": "votes", @@ -6577,5 +6580,27 @@ "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", + "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", + "Votazioni chiuse": "Votes clos", + "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 ?", + "%s prestiti verranno estesi di %d giorni.": "%s prêts seront prolongés de %d jours.", + "Prestiti estesi": "Prêts prolongés", + "Nessun prestito esteso": "Aucun prêt prolongé", + "%s prestiti estesi di %d giorni.": "%s 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 00a96034b..7a97a440d 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", @@ -6577,5 +6580,27 @@ "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", + "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", + "Votazioni chiuse": "Votazioni chiuse", + "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?", + "%s prestiti verranno estesi di %d giorni.": "%s prestiti verranno estesi di %d giorni.", + "Prestiti estesi": "Prestiti estesi", + "Nessun prestito esteso": "Nessun prestito esteso", + "%s prestiti estesi di %d giorni.": "%s 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/storage/plugins/book-club/BookClubPlugin.php b/storage/plugins/book-club/BookClubPlugin.php index 5f668a8c1..78cee7ca7 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 @@ -454,6 +457,22 @@ private function ensureBookclubBooksExternalSupport(): void if (!$probe("SELECT 1 FROM information_schema.TABLE_CONSTRAINTS WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'bookclub_books' AND CONSTRAINT_NAME = 'fk_bcbooks_external' AND CONSTRAINT_TYPE = 'FOREIGN KEY'")) { $db->query("ALTER TABLE bookclub_books ADD CONSTRAINT fk_bcbooks_external FOREIGN KEY (external_book_id) REFERENCES bookclub_external_books (id) ON DELETE CASCADE"); } + // 4. Self-heal the (club_id, libro_id) unique key. It has shipped in + // CREATE TABLE since the plugin's first version, so a real install + // always has it; this only re-adds it if it was somehow dropped. + // We REFUSE to dedupe: dropping a bookclub_books row cascades into + // its polls, votes, discussions and reading history (the same + // reason linkExternalBookToCatalogue never merges two club-books), + // so on the impossible-in-practice case of existing duplicates we + // leave the rows intact and log instead of destroying data. + if (!$probe("SELECT 1 FROM information_schema.STATISTICS WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'bookclub_books' AND INDEX_NAME = 'uq_bcbooks'")) { + $hasDupes = $probe("SELECT 1 FROM (SELECT 1 FROM bookclub_books WHERE libro_id IS NOT NULL GROUP BY club_id, libro_id HAVING COUNT(*) > 1 LIMIT 1) d"); + if ($hasDupes) { + SecureLogger::error('[BookClub] cannot add uq_bcbooks: duplicate (club_id, libro_id) rows exist; leaving them intact to preserve poll/vote/discussion history — deduplicate manually'); + } else { + $db->query("ALTER TABLE bookclub_books ADD UNIQUE KEY uq_bcbooks (club_id, libro_id)"); + } + } } catch (\Throwable $e) { SecureLogger::error('[BookClub] external-book schema upgrade failed: ' . $e->getMessage()); } @@ -827,27 +846,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..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.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.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 3afac20a6..5b616a067 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'; @@ -156,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'], @@ -220,16 +221,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 +365,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 7c7458cf3..88bbdce90 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,15 +124,16 @@ 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)); return $this->renderPublic($response, 'public/polls', [ 'club' => $club, 'polls' => $this->repo->clubPolls((int) $club['id']), 'eligible' => $eligible, + // 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'), @@ -305,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 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') { $this->flash('error', __('La votazione è chiusa.')); return $this->redirect($response, '/book-club/' . $slug . '/polls/' . $pollId); } @@ -477,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 4fa4c1398..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); } @@ -696,12 +701,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 a39682ea3..e526595d4 100644 --- a/storage/plugins/book-club/src/Repo.php +++ b/storage/plugins/book-club/src/Repo.php @@ -4,6 +4,7 @@ namespace App\Plugins\BookClub; +use App\Support\IsbnFormatter; use App\Support\SearchIndexBuilder; use App\Support\SecureLogger; use mysqli; @@ -24,6 +25,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; @@ -826,17 +839,261 @@ public function proposeExternalBook(int $clubId, array $data, string $state, ?in } /** - * Acquire an external proposal into the catalogue: create the real `libri` - * row from its metadata (this is the ONLY moment the book enters the - * 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. + * 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; + } + // 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', + [$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)) + : ''; + // Only keep a part whose checksum is valid. A 13/10-length run of + // otherwise-arbitrary digits must never become a reconcile match key, + // or two unrelated books sharing a malformed code would get linked. + return [ + (strlen($digits) === 13 && IsbnFormatter::isValidIsbn13($digits)) ? $digits : null, + (strlen($digits) === 10 && IsbnFormatter::isValidIsbn10($digits)) ? $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. 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 + { + // 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 + 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++; + } + } + + if ($ownsTx) { + $this->db->commit(); + $this->ownsReconcileTx = false; + } + return $reconciled; + } catch (\Throwable $e) { + if ($ownsTx) { + $this->db->rollback(); + $this->ownsReconcileTx = false; + 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; + } + } + + /** + * 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; + } + + // 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', + [$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 acquireExternalBook(int $clubBookId): ?int + 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. If a non-deleted + * catalogue book already has the same normalized ISBN (the manager entered + * the purchase manually), reuse it instead of violating libri's unique ISBN + * keys; otherwise create the real `libri` row from the proposal's metadata. + * Either way, repoint the club-book to that libro_id and stamp the external + * row. Returns the new/reused libro_id, or null on failure. On a refuse-to- + * merge (this club already links another proposal to that book) it returns + * null and sets $reason='already_linked' so the caller can show a specific + * message rather than a generic error. + */ + public function acquireExternalBook(int $clubBookId, ?string &$reason = null): ?int { + $reason = null; $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,59 +1108,30 @@ 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; - $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'); + // 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, + // authors and publisher, so create nothing new. + $libroId = $existingId; + } else { + $libroId = $this->createCatalogueBookFromExternal($row, $titolo, $anno, $isbn13, $isbn10, $cover); } - $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) { - throw new \RuntimeException('catalog acquisition repoint failed'); + if (!$this->linkExternalBookToCatalogue((int) $row['club_id'], $clubBookId, $extId, $libroId)) { + // 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(); @@ -915,6 +1143,63 @@ public function acquireExternalBook(int $clubBookId): ?int } } + /** + * 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]); @@ -1055,7 +1340,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> */ @@ -1063,7 +1354,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", @@ -1072,6 +1364,56 @@ public function clubPolls(int $clubId): array ); } + /** + * 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, 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, + 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_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.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, 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] + ); + } + /** * Options with book info and aggregated score. * @@ -1207,21 +1549,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; } // ------------------------------------------------------------------ @@ -1429,7 +1769,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 25515745f..4a4d30dba 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'), @@ -52,6 +61,7 @@ .bc-badge-open{background:rgba(16,185,129,.12);color:var(--success-color)} .bc-badge-closed{background:var(--accent-color);color:var(--text-light)} .bc-badge-warn{background:rgba(245,158,11,.14);color:#92400e} + .bc-summary{color:var(--primary-color);font-weight:600;font-size:.9rem;cursor:pointer} .bc-muted{color:var(--text-light);font-size:.85rem} .bc-hero{background:var(--primary-color);color:#fff;border-radius:22px;padding:clamp(1.75rem,4vw,2.5rem);margin-bottom:2rem} .bc-hero h1{font-size:clamp(1.8rem,4vw,2.5rem);font-weight:800;letter-spacing:-.03em;margin:0 0 .5rem;color:#fff} @@ -79,10 +89,10 @@

- -

+ +

- +
@@ -96,17 +106,77 @@ · - · + ·
- - - + + + + + + + +
+ () +
+ +
+
+ +
+ + · + + · + + · + +
+
+ +
+ +
+
+ + +
+
+ +

+
+

+ +
+
+ + + · + +
+ + + + + + + · + + + · + +
+
+
+ +
+ +
diff --git a/storage/plugins/book-club/views/public/show.php b/storage/plugins/book-club/views/public/show.php index a9dc7293c..3ea5fc3c3 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,52 @@ class="form-control mb-3">

- -

+ +

- +
- · + ·
- - - + + + + +
+ +
+ () +
+ +
+
+ +
+ + + · + + · + +
+
+ +
+ +
+
+ + 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); @@ -101,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`); @@ -228,7 +267,7 @@ test.describe.serial('Book Club — Uwe feedback', () => { 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 +277,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 +300,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 +322,40 @@ 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 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); + 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)}`), + '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(); await page.waitForLoadState('networkidle'); @@ -314,7 +387,39 @@ 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'); + + // 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); + // 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'); + // The club home page (show.php) must also show the expired-but-open poll as + // ended, consistent with the poll list and detail pages (read-only display). + await expect(page.locator('.bc-badge:has-text("Votazione terminata")').first()).toBeVisible(); + 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'); + 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(); }); 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 new file mode 100644 index 000000000..9ac9e5cb9 --- /dev/null +++ b/tests/bookclub-acquire-reconcile-poll-history.unit.php @@ -0,0 +1,312 @@ += 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'); +$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, $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. +$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); +} + +$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. + // Fixtures must be checksum-VALID: reconcile now rejects malformed codes + // (normalizedIsbnParts validates the ISBN checksum), so a synthetic ISBN + // needs a correct check digit or it would never match. + $mkIsbn13 = static function (string $base12): string { + $base12 = substr(str_pad($base12, 12, '0'), 0, 12); + $sum = 0; + for ($i = 0; $i < 12; $i++) { + $sum += (int) $base12[$i] * (($i % 2) === 0 ? 1 : 3); + } + return $base12 . ((10 - ($sum % 10)) % 10); + }; + $rand5 = static fn(): string => str_pad((string) random_int(0, 99999), 5, '0', STR_PAD_LEFT); + $isbn13 = $mkIsbn13('9780000' . $rand5()); + // A valid ISBN-10 whose check digit is 'X' — exercises the lowercase-x path. + do { + $base9 = str_pad((string) random_int(0, 999999999), 9, '0', STR_PAD_LEFT); + $sum = 0; + for ($i = 0; $i < 9; $i++) { + $sum += (int) $base9[$i] * (10 - $i); + } + } while ((11 - ($sum % 11)) % 11 !== 10); + $isbn10 = $base9 . 'X'; // 9 digits + valid X check char + $externalIsbn10 = substr($isbn10, 0, 3) . '-' . substr($isbn10, 3, 3) . '-' . strtolower(substr($isbn10, 6)); + $autoIsbn13 = $mkIsbn13('9783333' . $rand5()); + $backfillIsbn13 = $mkIsbn13('9784444' . $rand5()); + $delIsbn13 = $mkIsbn13('9781111' . $rand5()); + + $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; + $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. + $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. automatic and explicit external reconciliation (#138 bug)\n"; + + // 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'); + $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, '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, '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, $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, '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; + // 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 === 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, $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, '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"; + + // Seed club books (catalogue-side, simplest) to be poll options. + $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}, '" . $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", '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; + $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, '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 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'); + + // 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, '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'); + $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}-%'))"); + $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); diff --git a/tests/bookclub-uq-bcbooks-and-isbn-checksum.unit.php b/tests/bookclub-uq-bcbooks-and-isbn-checksum.unit.php new file mode 100644 index 000000000..bdea5ec9f --- /dev/null +++ b/tests/bookclub-uq-bcbooks-and-isbn-checksum.unit.php @@ -0,0 +1,180 @@ += 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'); +$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, $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'); + +// Make the test hermetic: install/upgrade the bundled plugin schema exactly as +// the real plugin lifecycle does. Idempotent, so an already-active local DB is +// left intact and ends with uq_bcbooks present. +$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); +} + +$TOKEN = 'zzuqbc_' . bin2hex(random_bytes(4)); +$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"; } +}; + +$indexExists = static function (string $index) use ($db): bool { + $r = $db->query( + "SELECT 1 FROM information_schema.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'bookclub_books' + AND INDEX_NAME = '" . $db->real_escape_string($index) . "' LIMIT 1" + ); + return ($r instanceof mysqli_result) && $r->num_rows > 0; +}; + +// ── Part A. ISBN checksum gate in normalizedIsbnParts ──────────────────────── +echo "A. normalizedIsbnParts validates the checksum\n"; +$repo = new Repo($db); +$rp = new ReflectionMethod(Repo::class, 'normalizedIsbnParts'); +$rp->setAccessible(true); +$parts = static fn(string $isbn): array => $rp->invoke($repo, $isbn); + +// Sanity-check the fixtures against the validator so the assertions below are +// grounded in genuinely (in)valid codes, not my hand-computed guesses. +$check(IsbnFormatter::isValidIsbn13('9780306406157'), 'fixture 9780306406157 is a valid ISBN-13'); +$check(IsbnFormatter::isValidIsbn10('0306406152'), 'fixture 0306406152 is a valid ISBN-10'); +$check(!IsbnFormatter::isValidIsbn13('1111111111111'), 'fixture 1111111111111 is NOT a valid ISBN-13'); +$check(!IsbnFormatter::isValidIsbn10('1234567890'), 'fixture 1234567890 is NOT a valid ISBN-10'); + +$check($parts('9780306406157') === ['9780306406157', null], 'valid ISBN-13 kept in the 13 slot'); +$check($parts('978-0-306-40615-7') === ['9780306406157', null], 'valid ISBN-13 kept after stripping hyphens/spaces'); +$check($parts('1111111111111') === [null, null], 'garbage 13-digit run rejected (bug fixed)'); +$check($parts('0306406152') === [null, '0306406152'], 'valid ISBN-10 kept in the 10 slot'); +$check($parts('1234567890') === [null, null], 'garbage 10-digit run rejected (bug fixed)'); +$check($parts('') === [null, null], 'empty string yields no parts'); +$check($parts('978030640615') === [null, null], '12-digit near-miss yields no parts'); + +// ── Part B. uq_bcbooks self-heal + duplicate-safe refusal ──────────────────── +echo "B. ensureBookclubBooksExternalSupport self-heals uq_bcbooks\n"; +$rm = new ReflectionMethod(BookClubPlugin::class, 'ensureBookclubBooksExternalSupport'); +$rm->setAccessible(true); +$runMigration = static function () use ($rm, $plugin): void { $rm->invoke($plugin); }; + +$dupClubId = null; $dupLibroId = null; + +// Guarantee the live table is restored to a correct state no matter what. +$restore = static function () use ($db, $indexExists, &$dupClubId, &$dupLibroId): void { + // Remove any duplicate seed rows first so ADD UNIQUE can succeed. + if ($dupClubId !== null) { + $db->query("DELETE FROM bookclub_books WHERE club_id = " . (int) $dupClubId); + } + if (!$indexExists('uq_bcbooks')) { + $db->query("ALTER TABLE bookclub_books ADD UNIQUE KEY uq_bcbooks (club_id, libro_id)"); + } + if ($dupLibroId !== null) { + $db->query("DELETE FROM libri WHERE id = " . (int) $dupLibroId); + } + if ($dupClubId !== null) { + $db->query("DELETE FROM bookclub_clubs WHERE id = " . (int) $dupClubId); + } +}; + +try { + $check($indexExists('uq_bcbooks'), 'baseline: uq_bcbooks present after ensureSchema()'); + + // B1 — add path: drop the key (no dupes can exist, it was enforcing them), + // run the real migration, expect it re-added. + $db->query("ALTER TABLE bookclub_books DROP INDEX uq_bcbooks"); + $check(!$indexExists('uq_bcbooks'), 'setup: uq_bcbooks dropped'); + $runMigration(); + $check($indexExists('uq_bcbooks'), 'migration re-adds uq_bcbooks when missing and no dupes'); + $check(count($schema['failed']) === 0, 'migration ran without touching failed[]'); + + // B2 — refuse path: drop again, seed a club + libro, insert TWO rows with + // the same (club_id, libro_id), run migration, expect it to REFUSE (key + // stays absent) and to PRESERVE both rows. + $db->query("ALTER TABLE bookclub_books DROP INDEX uq_bcbooks"); + $db->query("INSERT INTO bookclub_clubs (ics_token, name, slug) VALUES ('" . bin2hex(random_bytes(16)) . "', '{$TOKEN} Club', '{$TOKEN}-club')"); + $dupClubId = (int) $db->insert_id; + $db->query("INSERT INTO libri (titolo) VALUES ('" . $db->real_escape_string($TOKEN . ' Book') . "')"); + $dupLibroId = (int) $db->insert_id; + $db->query("INSERT INTO bookclub_books (club_id, libro_id, state) VALUES ({$dupClubId}, {$dupLibroId}, 'proposed')"); + $db->query("INSERT INTO bookclub_books (club_id, libro_id, state) VALUES ({$dupClubId}, {$dupLibroId}, 'proposed')"); + $before = (int) $db->query("SELECT COUNT(*) c FROM bookclub_books WHERE club_id = {$dupClubId}")->fetch_assoc()['c']; + $check($before === 2, 'setup: two duplicate (club_id, libro_id) rows inserted'); + + $runMigration(); + $check(!$indexExists('uq_bcbooks'), 'migration REFUSES to add uq_bcbooks while duplicates exist'); + $after = (int) $db->query("SELECT COUNT(*) c FROM bookclub_books WHERE club_id = {$dupClubId}")->fetch_assoc()['c']; + $check($after === 2, 'both duplicate rows preserved — no destructive dedupe'); +} finally { + $restore(); +} + +$check($indexExists('uq_bcbooks'), 'teardown: uq_bcbooks restored on the live table'); +// Confirm the seed rows are gone (FK-safe cleanup succeeded). +$leftBooks = $dupClubId !== null + ? (int) $db->query("SELECT COUNT(*) c FROM bookclub_books WHERE club_id = " . (int) $dupClubId)->fetch_assoc()['c'] + : 0; +$check($leftBooks === 0, 'teardown: seeded bookclub_books rows removed'); + +echo "\n{$pass} PASS, {$fail} FAIL\n"; +exit($fail === 0 ? 0 : 1); diff --git a/tests/code-quality.spec.js b/tests/code-quality.spec.js index 132ab0ac4..93ab51d20 100644 --- a/tests/code-quality.spec.js +++ b/tests/code-quality.spec.js @@ -311,15 +311,77 @@ 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 = []; 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 @@ -330,7 +392,24 @@ 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 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 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]); } 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');