Repository navigation
Visitor log, catalogue cards, suppliers, RDA types, statistics (0.9.0) - #506
fabiodalez-dev wants to merge 43 commits into
Conversation
Findings
 |
| Layer / File(s) | Summary |
|---|---|
Schema e migrazione installer/database/schema.sql, installer/database/migrations/migrate_0.9.0-rc.1.sql, tests/migration-0.9.0-minor-features.unit.php, CHANGELOG.md |
Lo schema e la migrazione aggiungono le tabelle fornitori e presenze, il riferimento al fornitore e i campi RDA nullable. I test verificano la ripetibilità della migrazione e la conservazione dei dati. |
Registro presenze e conservazione app/Models/AttendanceRepository.php, app/Controllers/AttendanceController.php, app/Views/attendance/index.php, app/Support/MaintenanceService.php, app/Routes/web.php, tests/attendance-090.spec.js |
Il registro registra visite di soci e ospiti, mostra statistiche ed esporta CSV. Gli amministratori possono impostare la retention ed eseguire la pulizia; la manutenzione pianificata applica la stessa impostazione. |
Gestione e assegnazione dei fornitori app/Models/SupplierRepository.php, app/Controllers/SuppliersController.php, app/Views/fornitori/*, app/Controllers/LibriController.php, app/Controllers/LibriApiController.php, app/Views/libri/index.php, app/Support/CataloguingExtras.php, app/Controllers/CsvImportController.php, app/Controllers/MarcImportController.php, app/Routes/web.php, tests/suppliers-090.spec.js, tests/marc-bulk-import.spec.js |
Le pagine amministrative permettono di creare, modificare ed eliminare fornitori e di filtrare i libri. Gli import CSV e MARC assegnano il fornitore ai record previsti in base all’azione e alla modalità selezionate. |
Override RDA e output BIBFRAME app/Support/RdaTypes.php, app/Views/libri/partials/book_form.php, app/Views/libri/scheda_libro.php, app/Support/CataloguingExtras.php, app/Controllers/LibriController.php, storage/plugins/bibframe-linked-data/BibframeLinkedDataPlugin.php, tests/rda-types-090.spec.js |
Il modulo del libro mostra i valori RDA derivati dal tipo di media e consente di salvarne gli override. La scheda del libro e l’output BIBFRAME usano i valori effettivi. |
Schede catalografiche ISBD app/Support/CatalogueCard.php, app/Controllers/CatalogueCardController.php, app/Views/libri/index.php, app/Views/libri/scheda_libro.php, app/Routes/web.php, tests/catalogue-cards-090.spec.js |
Il catalogo genera PDF singoli o in blocco, con una scheda da 125 × 75 mm per libro. La selezione in blocco limita gli ID a 200. |
Sezioni statistiche app/Support/StatisticsSections.php, app/Controllers/Admin/StatsController.php, app/Views/admin/stats.php, tests/statistics-090.spec.js |
La pagina statistiche mostra i prestiti per tipo di materiale, le presenze e le sezioni aggiuntive disponibili, incluso il riepilogo dell’ultima revisione inventariale chiusa. |
Verifiche condivise e traduzioni tests/helpers/minor-features-090.js, tests/minor-features-090.unit.php, locale/* |
Gli helper supportano le suite di test con configurazione, accesso, dati di prova e lettura dei PDF. Le traduzioni aggiungono etichette e messaggi per le nuove funzioni. |
Estimated code review effort: 4 (Complex) | ~60 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
actor Operatore
participant AttendanceController
participant AttendanceRepository
participant Database
Operatore->>AttendanceController: invia scansione tessera o dati ospite
AttendanceController->>AttendanceRepository: cerca tessera o registra visita
AttendanceRepository->>Database: legge dati o inserisce presenza
Database-->>AttendanceRepository: restituisce record o risultato
AttendanceController-->>Operatore: mostra conferma o errore
Possibly related PRs
- fabiodalez-dev/Pinakes#499: definisce lo schema 0.8.9 usato come base nei test della migrazione 0.9.0.
Merge Risk: 
71580
-
Resolve account visibility, supplier/RDA preservation and import recovery before merging, or explicitly accept their bounded risks. Supplier listing also fails on MariaDB with ONLY_FULL_GROUP_BY enabled. Previously identified pagination, escaping, retention and BIBFRAME inconsistencies are corrected.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check Il titolo elenca le principali aree del changeset: registro presenze (visitor log), schede catalografiche, fornitori, tipi RDA e statistiche per la versione 0.9.0. Il titolo è coerente con le modifich… Docstring Coverage Docstring coverage is 71.59% which is sufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
Il titolo elenca le principali aree del changeset: registro presenze (visitor log), schede catalografiche, fornitori, tipi RDA e statistiche per la versione 0.9.0. Il titolo è coerente con le modifiche, anche se è un elenco di funzionalità più che una sintesi di un'unica modifica principale. Non è vago né fuorviante.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 71.59% which is sufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 88 functions across 39 files. (7 skipped: 7 unsupported.)
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
|
@coderabbitai full review |
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues. Two rough edges in the catalogue cards are worth a look.
Reviewed changes
I reviewed the full PR (7 commits): visitor log, ISBD catalogue cards, suppliers, RDA overrides and the new statistics sections, plus the migration, schema, locales and tests.
- Visitor log: the
/admin/attendancekiosk records card scans and guests inpresenze. Day and hour statistics use app-timezone windows, the CSV export is included, and retention is purged byMaintenanceService. Settings and purge are admin-only. - Catalogue cards:
CatalogueCard::lines()builds the ISBD text andrender()produces a 125 × 75 mm TCPDF card. Cards print from the book page or from a list selection. - Suppliers: there is a
fornitoriCRUD, andlibri.fornitore_id(FKON DELETE SET NULL) is written throughCataloguingExtras::save()only when the form posts the key. Suppliers also appear as a book list filter and on the book page. - RDA types:
RdaTypesderives content, media and carrier fromtipo_media, with per-record overrides. BIBFRAME uses an override only when one is set. - Statistics: adds loans by material, visitors, and hook-based extension sections. Stocktake outcomes come from the last closed session.
- Schema: the migration is idempotent and matches
schema.sql.
What I checked: every new __() string exists in all four non-Italian locales, php -l is clean on every changed PHP file, and tests/minor-features-090.unit.php passes 50/50 locally. The helper APIs the new code calls (Hooks::apply, DateHelper::today/now, SettingsRepository::get/set, RateLimitMiddleware, SwalApp.confirm) exist with matching signatures. The permission mapping is right: /admin/suppliers and /admin/books/... fall under catalog.manage, and /admin/attendance under loans.manage.
ℹ️ BIBFRAME and the book page disagree on RDA types for non-book media
For a DVD, record or audiobook with no override, the admin book page now shows the derived triple (e.g. tdi / v / vd). The BIBFRAME output for the same record still says text / unmediated / volume (txt / n / nc). The PR description says keeping the old output is deliberate. Since RdaTypes::effective() already exists, emitting the derived values would make linked data match what cataloguers see. Your call whether that belongs in this PR or a later one.
claude-opus-5-5 ｜ 𝕏
88abf7d to
c6b5354
Compare
ec68993 to
89a5acd
Compare
There was a problem hiding this comment.
✅ No new issues found. Both catalogue card issues from the last review are fixed.
Reviewed changes
Since the last review, the branch was rebased onto the updated feat/marc-bulk and gained one commit, 89a5acd. A range-diff shows the seven earlier commits are unchanged apart from CHANGELOG.md context lines.
- Changed cards to always fit on one page:
SetAutoPageBreak(false)plus a newfitBlock()keep the heading, body, ISBN and tracings above the inventory line. The text shrinks to 5.5 pt first, then trailing words are replaced with…. The new unit case (a 40-keyword card next to a plain one, exactly 2 pages) passes locally. The full suite passes 51/51. - Added a bulk limit warning: selecting more than
CatalogueCardController::MAX_BULKbooks now shows a SweetAlert warning and sends no request. The limit comes from the PHP constant, and both new strings are present in all five locales. - Changed "select all" to follow the current page:
syncSelectAll()runs fromdrawCallbackandhandleRowSelect, so the header box can be ticked again after paging. E2E test 6 selects 205 books across three pages and confirms the warning appears and no PDF is requested.
claude-opus-5-5 ｜ 𝕏
89a5acd to
0d4d3a3
Compare
4f49cc3 to
46c9455
Compare
0d4d3a3 to
20d1937
Compare
|
@coderabbitai full review |
|
46c9455 to
55f4cdd
Compare
20d1937 to
cf662a4
Compare
|
@coderabbitai full review |
|
Adds the presenze and fornitori tables, libri.fornitore_id with an ON DELETE SET NULL foreign key and the three nullable RDA override columns, both in schema.sql and as the delimited minor-features section of migrate_0.9.0.sql. The migration runner test executes every migration-0.9.0-<feature> section test so stacked branches only add their own file.
Suppliers get an admin CRUD modelled on the publishers pages, a select in the book form, a filter in the book list and are unlinked (not deleted) from their books when removed. The book form shows the RDA types derived from the media type and lets a cataloguer override each one; BIBFRAME uses an override when it is set and keeps its current output otherwise. This commit also registers the routes, sidebar entries and translations of the other 0.9.0 minor features.
One card per page with the call number, the main entry heading, the ISBD description, the standard number and the subject tracings. Printable from the book page and from a selection in the book list; soft-deleted books are never printed. The unit test covers the ISBD assembly and the other pure helpers of the 0.9.0 minor features.
/admin/attendance records a visit from a member card scan or a short guest form, shows visits per day and per hour slot with a CSV export, and keeps the data only for the configured number of days (admin setting, default 365, 0 = forever). The scheduled maintenance purges older visits and the admin can apply the retention immediately.
/admin/statistics shows the loans of the last 12 months by media type and the visits of the last 30 days. Loans by patron type, fines and stocktake outcomes are extension points in StatisticsSections (one function and one filter hook each) that stay hidden until a feature returns data.
SweetAlert2 renders title as HTML, so a supplier named with markup ran in the delete confirmation (stored DOM XSS). The name now goes into the plain-text body and the title is a fixed label. The suppliers spec creates a supplier named with an onerror image and checks that the dialog shows it literally and that nothing executes.
StatisticsSections::stocktakeOutcomes() now reads the stocktake tables: expected, found, misplaced, on loan, missing and unknown codes of the most recently closed session, through StocktakeService::summary(). A filter on statistics.section.stocktake still takes precedence, and the section stays hidden without the tables or a closed session.
…bulk limit A card no longer spills onto a second page: auto page break is off and each block shrinks down to 5.5 pt, then the tracings are shortened with an ellipsis, so the bulk PDF stays one card per page with the inventory number on it. Printing more books than CatalogueCardController::MAX_BULK now stops with a warning instead of silently printing only the first 200. The "select all" box of the book list follows the rows on the current page, so it can be ticked again after paging.
cf662a4 to
e2baed8
Compare
|
@coderabbitai full review |
|
|
@coderabbitai full review |
|
|
Replies to the follow-up and nitpick findings listed in the body of the CodeRabbit reviews on this PR:
|
There was a problem hiding this comment.
✅ No new issues found. The hardening commits since the last Pullfrog review are sound, and the merges from
feat/marc-bulkonly bring base content.
Reviewed changes
I reviewed the PR commits added after bd5ff16 (the base merges carry only base content, such as the stocktake locale strings):
- The book form no longer clears values by accident:
CataloguingExtras::save()now clears the supplier or an RDA override only when the empty option is chosen. A malformed id, a deleted supplier or an unknown code keeps the stored value.book_form.phpdisables the supplier select when the stored supplier is not among the options, so the form does not post it back as "no supplier". - Supplier writes inside the CSV row transaction:
importParsedRow()now assigns the batch supplier to created books beforecommit(), and its return value gainssupplier_missing.assignSupplier()returnsbool. A supplier deleted during an import is now listed with typesupplierin both CSV and MARC. NeitherfailedCounttreats it as a failed row, and the MARC importer passes its own$counters, so the in-transaction write does not fire twice for MARC. - Supplier form: a refused submission is kept as typed. It is stored in the session under
supplier_form_old, keyed by id, read once, and dropped if it belongs to another form.update()now distinguishes a deleted row from a save that changes nothing by re-reading afteraffected_rows === 0. Websites are no longer truncated: anything over 255 characters, scheme included, is refused.createFormandindexare now guarded by the schema check. - Schema probe and list query:
SupplierRepository::isAvailable()now also requireslibri.fornitore_id.listWithCounts()uses a correlated count, soONLY_FULL_GROUP_BYaccepts it. - Visitor log scope: a card lookup uses
Permissions::accountScopeSql(..., SCOPE_BORROWER), the same reach as the borrower search, and now runs inside the scan'stry. Therecent()feed applies the scope in theLEFT JOIN ... ON, so an out-of-scope visit is still listed but without a name. - Small guards: the
?fornitore=filter is validated withFILTER_VALIDATE_INT, and the stats note check usesissetplus non-empty, so a note of"0"is still shown. - Tests: added
tests/cataloguing-extras-090.unit.php, which runs on TEMPORARY shadow tables. Also new: website length cases, a MediaBox check on every page of the cards PDF, an assertion thatfk_presenze_utenteisON DELETE SET NULL, and RDA E2E tests that run without BIBFRAME (only the BIBFRAME tests skip) with a content override.
What I ran on the new code: php -l is clean on every PHP file touched, tests/minor-features-090.unit.php passes 60/60, and all locale files parse. I did not run the DB-backed unit test or the E2E suites.
claude-opus-5-5 ｜ 𝕏
Since a refused supplier form gives back the submitted values, the invalid website stayed in the field and the final save was refused again. The test now checks that the typed values come back and fixes the website before saving.
|
@coderabbitai review |
Action not completed
|
|
@coderabbitai review |
Action not completed
|
|
@coderabbitai review |
Action not completed
|
|
@coderabbitai review |
Action not completed
|
|
@coderabbitai review |
Action not completed
|
|
@coderabbitai review |
Action not completed
|
|
@coderabbitai review |
Action not completed
|
|
@coderabbitai review |
Action not completed
|
|
This change shipped in 0.9.0-rc.1: the whole stack landed in main through the release PR #514 (https://github.com/fabiodalez-dev/Pinakes/releases/tag/v0.9.0-rc.1). I'm closing this one because its base branch was part of the stack, so GitHub cannot mark it as merged on its own. |
Smaller SLiMS features: visitor log, catalogue cards, suppliers, RDA types, statistics.
/admin/attendance: card scan for members, guest form, optional branch, statistics per day and hour slot with CSV, retention setting (default 365 days, 0 = forever) purged by the existing maintenance cron, GDPR note. Retention changes and manual purge are admin-only./admin/suppliersCRUD modelled on publishers,libri.fornitore_id(FK ON DELETE SET NULL) in the book form, on the book page and as a list filter.StatisticsSectionsextension points for patron types, fines and stocktake. The stocktake one is implemented here (outcomes of the last closed session).The supplier delete confirmation puts the name in SweetAlert2's plain-text body; an E2E test with a markup name proves it is shown literally.
Tests: unit 50 + migration section 19; E2E
attendance-0907,catalogue-cards-0905,suppliers-0906,rda-types-0906,statistics-0906; book form, BIBFRAME and statistics regressions green (71). PHPStan 0.Summary by CodeRabbit