Release 0.7.64: multiple copies per borrower + circulation durability (#238) - #372
Conversation
Add tests/multiple-copy-loans-238.spec.js: five browser scenarios exercising the opt-in loans.allow_multiple_loans_same_book setting through the real admin desk flow (Apache + loan controller + per-copy DB triggers): - the settings toggle persists from the UI - ON -> one borrower may take two distinct copies of the same title - ON -> the same copy cannot be lent twice to the same borrower - OFF -> the historical borrower/title uniqueness rule still rejects a second copy - ON -> 'save and register another copy' keeps the title and scans the next copy Self-provisions its own admin, borrowers, book and copies (tokenised) and cleans everything up in afterAll.
The multiple-copy loan feature ships as 0.7.64. No migration is required: the opt-in setting resolves to off on existing installs and is seeded off for fresh installs, so the upgrade is behaviour-preserving.
dbQuery() trims stdout, so a pre-existing empty setting_value row was read back as a missing row and afterAll deleted it; whitespace-carrying values were restored trimmed. Row existence is now detected with COUNT(*) and the value serialized losslessly via HEX()/UNHEX() (SQL NULL round-trips too), addressing the open CodeRabbit finding on this spec.
residual) Every approveLoan overlap predicate now treats a date-overdue 'in_corso' loan not yet flipped by the maintenance sweep as an open-ended commitment (same branch as CapacityService), copy selection only considers in-house copies ('disponibile'/'prenotato'), the pre-assigned copy is validated against the loan's book, and a request whose whole window is already past is rejected instead of becoming an unpickable 'da_ritirare'. confirmPickup re-checks the loan rows holding the copy (not just copie.stato) before issuing, mirroring activateScheduledLoans. cancelPickup normalises the reason like rejectLoan and records an audit note plus processed_by so a manual cancellation is distinguishable from the automatic expiry.
…uards Queue promotion re-checks borrower eligibility and skips (without cancelling) reservations whose user was suspended or lost their card while waiting, so they no longer burn their position on a pending loan approval must refuse. New reservations are refused on books without any copie rows — the queue can only convert physical copies, so those reservations silently never converted. The promotion allocator excludes copies already committed by the promoted borrower (parity with every other allocator in #368) and gains the #366-residual overdue branch. Admin reservation edits reject out-of-whitelist states instead of coercing them to 'attiva', refuse flipping a completed reservation while its promoted loan is still open, and the edit view finally renders the error codes. Cancelling a promoted pending loan releases and reassigns its copy immediately. The 'book available' email formats dates in the recipient's locale (#360 parity).
update() applies the strict multiplicity rule when the row being reassigned is itself a promoted queue commitment (origine='prenotazione' in 'prenotato'/'da_ritirare'), so a borrower can never end up holding a physical loan plus a promoted queue position for the same title. The desk create form disables its submit buttons on first submit: with the relaxed mode and auto-assignment a replayed POST registered a second real loan on another copy. A deadlock/lock-timeout during a return now reports a dedicated retry error instead of the generic failure. copy_code/save_and_new no longer linger in the retained session input, and the loans API closes its count statements.
reassignOnNewCopy/reassignOnReturn now return whether they assigned; reassignOnReturn keeps re-running the FIFO walk so one freed copy can serve several holds with disjoint windows in the same event instead of leaving the rest to the next maintenance sweep. Candidate selection gains the BUG8/D13 stale-window guard (data_scadenza >= today) so a hold whose whole window has passed no longer receives the returned copy ahead of valid candidates. The copy-available email formats dates in the recipient's locale (#360 parity).
…on lock hygiene The 'ready for pickup' email becomes claim-and-retry via a new prestiti.pickup_notification_sent flag (schema + migrate_0.7.64.sql + runtime self-healing, with behavioural coverage): an SMTP failure no longer silently costs the user their pickup window — the maintenance sweep retries it, activation/repair reset the flag, and the #301 auto-approval email claims it so no duplicate announcement goes out. Borrowers without an email keep their warning/overdue claim (ending the endless SMTP retry churn) while the in-app and admin notifications now fire anyway — previously admins were never told about their overdue loans. Wishlist notifications filter empty addresses. Both cron entrypoints refuse non-CLI execution and no longer unlink the lock file after unlock (the inode race scripts/maintenance.php already documents), and runAll() refreshes the cross-session cooldown marker so an admin login right after the cron does not re-run maintenance synchronously.
…ction log When at least one active partner carries an identifier (agency_id, code or ISIL), circulation messages must present a matching FromAgencyId — deactivating a partner now actually revokes access; installs whose partner table is empty or metadata-only keep the historical basic-auth-only behaviour. CheckInItem/RenewItem honour the optional UserId so the right loan is resolved when the same title is out to several borrowers via NCIP. CheckOut/CheckIn/Renew are now logged to ncip_transactions with the resolved partner (previously only requests were logged, always without partner_id). RenewItem claims the extension window from the day after the current due date (#336 parity with the web renew) so boundary-day commitments no longer bounce valid renewals.
…ew strings Records the booking/lending review follow-ups under 0.7.64 — including the reassignOnNewCopy FIFO semantics change that is observable with the multiplicity setting off — and adds the new user-facing strings to all five shipped locales.
…ad check processBookAvailability now reads a small FIFO batch (LIMIT 25) so it can skip ineligible users; the invariant under test is the LOCKING read (FOR UPDATE), not the batch size, so the regex accepts any LIMIT.
…he NCIP ignore The revert condition read $claimed after the !claimed path had already returned, so PHPStan flagged the operand as always true — the condition now keys off the precomputed $isReadyPickup flag. The property.onlyWritten ignore on NcipServerPlugin::$hookManager became stale (passing $this->currentPartnerId by reference to bind_param stops the class-wide only-written report) and reportUnmatchedIgnoredErrors treats a stale ignore as an error: dropped. Verified locally with phpstan 2.1.56 against the repo config: no errors.
- ReservationManager: the eligibility-aware promotion now pages the FIFO queue with bound LIMIT/OFFSET batches until the first eligible candidate (an eligible reservation beyond position 25 was unreachable when every earlier one was suspended); the mvcc static check accepts the bound placeholders. - MaintenanceService: ensurePickupNotificationColumn() reports whether the column is actually available and the activation/repair UPDATEs include it conditionally, so a DB user without ALTER on a pre-migration install keeps activating scheduled loans instead of failing every iteration. - LoanApprovalController: the auto-approval pickup claim ensures the notification columns first (addNotificationColumns is now public), so a legacy install cannot silently skip the claim and double-announce. - migrate_0.7.64.sql: drop the AFTER clause — overdue_notification_sent may be missing on legacy schemas (runtime self-heal only); the sandbox migration test now runs against a legacy table without any notification columns. - NCIP resolvePartner(): same no-namespace fallback as detectMessageType, so namespace-free messages with a valid FromAgencyId are not rejected when partner enforcement is active. - crea_prestito: stable form id instead of the action-based JS selector. - docs/settings.MD: pickup_expiry_days clamp is 1…30, not 1…365.
The #278 spec asserted a 1 -> 2 -> 3 column progression, but since the carousel redesign the section below 1024px is a single-row horizontal snap-scroll strip, where every card intentionally shares one row. The assertion 'perRow === 1' at 480px only passed while the un-ordered LIMIT 1 book pick happened to land on a book with exactly one related card; any catalog churn shifts the pick and the spec fails on a layout that is working as designed (deep-regression shard 4/4 failure). - Assert the real contract: below 1024px the grid computes to flex, keeps all cards on the strip row, and overflows sideways when there is more than one card; from 1024px up it computes to grid with all cards visible up to the 4-column cap. - Make findBookWithRelated() deterministic: ORDER BY libro_id and skip other specs' ZZ* fixture books so physical row order and sibling fixtures can no longer change the measured page.
The last test in the #278 spec still asserted wrapW <= 1000, a leftover from the old ~960px 3-card cap. Since .related-books-wrap was widened to max-width: 1320px (to fit up to 4 columns), the wrap measures 1320px on any ultra-wide viewport whenever the section renders, so the assertion could never pass; it went unnoticed because the serial spec skipped or failed earlier before reaching it. Verified in headless Chromium against the real page CSS: wrapW is 1320 at 2560px regardless of card count, while the container is ~92vw (~2355px), so asserting <= 1330 still proves the row stays grouped instead of spreading across the container.
Address the CodeRabbit review round on the loan-approval claim and the NCIP partner enforcement: - LoanApprovalController: claim pickup_notification_sent BEFORE sending the auto-approval email (reverting on send failure) so the retry sweep cannot interleave between send and claim and deliver a duplicate pickup announcement; skip the claim entirely when the column cannot be ensured instead of running a doomed UPDATE. - NotificationService::addNotificationColumns() now returns whether the notification columns are available, so callers updating flags directly can branch on it. - NCIP closeLoan(): a return-notification failure no longer converts a successful close into false, which made the caller take the idempotent branch and skip logTransaction() for a completed check-in. - NCIP resolvePartner(): reject ambiguous identifiers (two active partners matching the same value) instead of picking an arbitrary row via LIMIT 1, keeping authorization and transaction logging deterministic. - NCIP hasEnforceablePartners(): fail secure on DB errors — only a missing ncip_partners table (legacy install) disables enforcement; any other failure refuses unresolved write operations instead of degrading to Basic auth alone. - related-books spec: assert the strip's computed overflow-x (auto or scroll) instead of requiring measured overflow whenever more than one card exists.
zizmor's pedantic undocumented-permissions audit fails the workflow security job when a permission has no explanatory comment. Annotate attestations: read in ci-upgrade-smoke.yml the same way release.yml documents its attestations permission: it lets gh attestation verify check the downloaded release archive's provenance.
The upgrade-smoke job feeds three PHP scripts to the CLI via heredoc (php <<'PHP'). On PHP 8.2 - the version this workflow pins - the CLI SAPI does not define the STDIN/STDOUT/STDERR constants when the script is read from standard input, so the success-path fwrite(STDOUT, ...) crashed with "Undefined constant STDOUT" (and every fwrite(STDERR, ...) error branch would have crashed the same way). PHP 8.3+ always defines them, which is why the scripts work with a newer local CLI. Define both constants behind defined() guards at the top of each heredoc script, so the behavior is identical on either PHP line.
The copy-overlap triggers now treat a stale in_corso loan (data_scadenza < CURRENT_DATE) as open-ended, so a successor pinned to the same copy can no longer be INSERTed after its stale predecessor — exactly the defence the application predicates apply. Five fixtures across four DB suites arranged that legacy state predecessor-first and started failing with 'Esiste gia un prestito attivo e sovrapposto per questa copia'. Insert the successor first and the predecessor second (their windows are disjoint, so each row passes the gate on its own): same final state, reached through the constraint. Also make loan-auto-approval-301 deterministic: seed the cached Mailer::isSmtpReachable() probe with false so the auto-approval email fails identically on every host (CI has no sendmail; dev machines may), and assert the release contract for check 09b — a failed approval email must leave pickup_notification_sent = 0 with no dangling claim token so the retry sweep can still deliver the announcement. The success branch (claim kept, token cleared) needs a deliverable transport and cannot be asserted portably. All six previously failing suites now pass against a seeded MariaDB 10.11 (schema + data_it_IT + triggers), matching the failing CI matrix.
The ZAP gate blocked on PII Disclosure [10062] for a 13-digit number on /en/register — the themed 404 page. The number is not catalog content: the i18n dictionary inlined on every page ships the example-ISBN placeholder keys (e.g. "es. 9788842935780"), so a 13-digit EAN/ISBN can appear on ANY route ZAP happens to crawl, and scoping the allowlist to bibliographic routes made the gate fire at random depending on the crawl set (reproduced locally: the flagged digits on /en/register are the placeholder strings from the inlined translations, not book data). Allowlist rule 10062 for 13-digit evidence on every route instead. The real card-leak signal is unchanged: 14-16 digit matches (live card formats — 13-digit Visa ranges are long dead) still fail the build anywhere, an alert with any non-13-digit instance still blocks, and every other medium/high alert stays blocking. Filter behavior verified against synthetic reports for all five cases.
Verified and fixed thirteen findings from the latest review round: - NotificationService::addNotificationColumns() no longer aborts before creating recall_count/last_recall_at when the pickup schema fails: sendLoanRecalls() queries those columns regardless of the return value, so the early return silently zeroed automatic recalls. - NotificationService::retryUnsentPickupNotifications() joins libri (deleted_at IS NULL) like sendPickupReadyNotification() does, so pickups on archived titles cannot permanently clog the LIMIT 20 retry batch ahead of deliverable rows. - PickupNotificationSchema::addMissingColumns() absorbs only the 1060 duplicate-column error per ADD COLUMN, so concurrent ensure() calls from web/cron/maintenance cannot turn a complete schema into a transient false. - ReservationReassignmentService::reassignOnCopyLost() requires data_scadenza >= today on both the reservation lookup and the FOR UPDATE re-check, so losing a copy can no longer reactivate an already-expired hold on a fresh copy. - NCIP CancelRequestItem rejects a present-but-invalid UserId with invalid-data (same contract as CheckInItem/RenewItem) instead of degrading to a title-only LIMIT 1 lookup that could cancel another borrower's request. - ci-playwright-policy.js returns after the missing-release-files fail() so an absent orchestrator reports the policy message instead of an unhandled ENOENT from readFileSync. - create-release.sh replaces the two `--paginate | head -n 1` pipelines with --slurp + jq first: under pipefail, head closing the pipe kills gh with SIGPIPE (141) and the script died without its error message. - ci-verify-release.sh restores the historical critical-file ZIP check (TinyMCE model/theme/skin/icons, Swagger UI, index.php, Updater, composer autoload) alongside the three compiled assets, honoring the documented release contract. - da_DK/de_DE: the retry-error string uses fetch verbs (hentning / Abrufen) instead of restore verbs (gendannelse / Wiederherstellung). - multiple-copy-loans-238 unit test refuses to run without explicit E2E_DB_* variables (same guard as loan-reservation-real-world-25) instead of falling back to the application .env — it performs global maintenance sweeps and cleanup DELETEs. - pickup-notification-retry derives both lease timestamps from PickupNotificationSchema::claimLeaseWindow() (production's UTC reference) instead of the application-timezone DateHelper::now(), which masked a broken lease by 1-2 hours. - loan-auto-approval-301 guards the claim-column reads behind PickupNotificationSchema::ensure() so a non-migrated database fails descriptively instead of with Unknown column. All touched unit suites re-run green against MariaDB 10.11 locally; PHPStan level 5 clean; locale parity check green.
Deep-regression shard 2/4 failed on book-field-types-static.spec.js test 15: it pins the header labels and progress format of tests/loan-edge-cases.unit.php, which grew from 64 to 66 tests in the circulation-hardening commit without the static contract following. Update the pinned strings (all-66 header, 53-66 range, [%02d/66] printf) to the current suite; verified locally against the real file.
The Static quality job exports only E2E_DB_NAME for the unit-test step (credentials come from the job-created .env), so the guard added in d4bb10b - which also required E2E_DB_USER and dropped the .env credential fallback entirely - refused to run there and failed the job. Mirror loan-reservation-real-world-25's proven safety valve exactly: the database NAME must be explicitly exported (never .env, so a dev box cannot point the global sweeps at its live DB), while host/credentials may still fall back to .env like the rest of the suite. Verified locally in a CI-like environment (only E2E_DB_NAME set: 52/52 pass; without it: descriptive refusal).
PrestitiController::store now consumes a one-time loan_submission_token issued to the rendered create form (replay guard added in c7f1266). adminStoreLoan() POSTed only the CSRF token, so every C-group store returned ?error=duplicate_submission: C.18 failed on CI shard 1 and the 21 serial tests after it never ran. Fetch /admin/loans/create first and submit the token the form carries, like a real browser. 39/39 pass locally against the a6a0766 triggers.
'npx playwright install --with-deps' runs apt-get update, and the hosted runner image ships Google's chrome-stable apt source that none of these jobs install from. A mid-sync mirror on dl.google.com just failed the whole Packaged-app job before any test ran (File has unexpected size / Mirror sync in progress). Remove the unused source ahead of the install in all four --with-deps steps so unrelated mirror churn cannot take the gates down.
ZAP rule 10062 matches any Luhn-valid digit window, and every page embeds a per-session 64-hex CSRF token: an all-digit window inside one occasionally forms a plausible card number that exists only in the response ZAP received. That just failed the Packaged-app gate with 'Maestro' 6736206100179663 inside the token on /editore/E2E%20Editore — noise that can hit any page on a few percent of runs. Genuine PII is stored data and renders again on a fresh anonymous fetch, so ci-check-zap-report.sh now re-fetches each still-blocking 10062 instance twice (GET, no param/attack, scanned origin, 13-19 digit evidence only): the alert is dropped, with an explicit log line, only when the digits are absent from both fresh responses. Fetch errors, any re-appearance, and every other rule keep blocking. Validated against the failing run's actual report_json.json (passes, instance logged) and with two new wrapper tests: unreachable URI stays blocking everywhere; absent-evidence path asserts the drop when the target answers.
The two verification fetches never read the loop counter; use the underscore placeholder shellcheck exempts.
The DB matrices went red at 22:03 UTC — 00:03 Europe/Rome — because the circulation triggers' fallback CURRENT_DATE() (UTC) had fallen a day behind DateHelper::today(): flipping a loan due 'yesterday' to in_ritardo re-ran the overlap gate against its scheduled successor and SIGNALled in issue-366-full-scenario (scenario F) and real-world-25 (case 03). Production writers never hit this: container, cron and script bootstraps bind @pinakes_application_date on every connection; the unit suites' own mysqli connections did not. Bind the date right after connecting in every circulation/migration suite, mirroring the production bootstrap. The four suites that warp the DB clock with SET timestamp to seed past fixtures now warp the bound date too and re-sync the real day afterwards, since COALESCE prefers the bound value over the warped CURRENT_DATE(). Validated both ways locally: with the DB forced a day behind the application date (global time_zone -03:00) the exact CI failures reproduce before the change and all 23 suites pass after it; with aligned dates all 23 suites stay green.
Same failure class 5b2a46e closed for CSRF tokens, one token to the left: the per-response CSP nonce is still unbroken 32-char hex and is stamped on every inline script/style of every page. ZAP's PII rule just flagged a Luhn-valid 13-digit window inside it on /da/katalog (5898564844530, read as a Maestro card) and failed the Packaged-app gate again. Group the nonce's hex encoding into 8-char chunks joined by '-', mirroring Csrf::generateToken(): full 128-bit entropy, decimal runs capped at eight, and still within CSP's base64url nonce alphabet, which permits dashes. Both format validations and the unit-test contract follow, plus a sampled assertion that generated nonces never contain a PII-like decimal run. Verified against the live app: header and attribute nonces agree, pages render, and no 13+ digit run remains outside the allowlisted i18n ISBN examples; loan-overlap browser suite 39/39.
Six findings from the b00605d review, all verified against current code: - NCIP CancelRequestItem now answers with the borrower resolved by the locked cancellation row instead of the request-derived UserId, which is null whenever the client legitimately omits it (RenewItem already behaved this way). - findActiveLoan logs a failed prepare() through SecureLogger before returning null: the partner sees item-not-checked-out, a definitive outcome, so the real database cause must leave a trace. - ncip-server.spec.js commitHeldTransaction fails fast when the held mysql child already exited; 'exit' fires once, so a late registration used to hang the promise until the Playwright timeout. - ci-playwright-policy: a missing .github/workflows/release.yml now fails the policy instead of silently skipping every sole-publisher contract; the release-mutation detector also recognizes github/octokit.request({method, url}) object calls in either key order (URL templates carry their own braces, so a bounded window replaces brace matching), with mutation and read-only fixtures. - mvcc-lockfirst asserts the promotion locks the FIFO head: ORDER BY r.queue_position ASC required, OFFSET rejected — matching the actual single-row locking read in processBookAvailability. Validated: node --check + policy self-tests (155 specs mapped, all fixtures detected), php -l, PHPStan clean on the plugin, mvcc suite 30/30.
Map the M/F/Altro enum to __()-translated labels on the user-details page with a raw-value fallback, and add an E2E check that every stored value renders localized. Consolidates PR #371.
version_compare('0.7.64','0.7.64-rc.1','<=') is false, so a 0.7.64.sql file
is skipped when the release ships from a prerelease. Name it -rc.1 (repo
convention) so the Updater selects it on the RC upgrade too; update the CI
upgrade-smoke contract and the migration unit test accordingly.
Restore the pre-existing behaviour where an approval may reuse a pre-assigned copy (and the date-aware allocator may pick a copy) whose current state is 'prestato' as long as the requested window does not overlap the copy's open loan. #370 had narrowed both to stato IN ('disponibile','prenotato'), which over-blocked legitimate future-dated approvals when the only copy was out; the per-copy overlap NOT EXISTS already guarantees no real conflict.
update() only checked ISO format and scadenza>=prestito, so actively moving an active loan's due date into the past reached the per-copy overlap trigger and failed with an opaque 'loan_update_failed'. Reject it up front with the same 'expired_window' error store()/approve() use — but only when the due date actually changes, so editing an already-overdue loan's other fields still works. Add the message to the loans list and all five locales.
The panel was absolutely positioned against the right-hand bell button, so a near-full-width dropdown overflowed the right screen edge and 'Segna tutte come lette' plus the notification bodies were cut off on mobile. Pin it to the viewport (fixed, inset 1rem, centred, max-width 28rem) below the md breakpoint via an explicit #notifications-dropdown rule — main.css is a purged Tailwind build, so new utility classes would need a rebuild to take effect. Desktop keeps the original anchored w-96 panel.
New coverage for the review follow-ups, all against the real DB + triggers or the live browser: - unit (19): date-disjoint out-copy selection stays flexible (F2) while a real overlap is hidden; a stale in_corso overdue loan is open-ended per copy; multiplicity distinct-copy invariant; atomic pickup-notification claim/retry; NCIP stays strict with multiplicity ON. - e2e (6): backdating an active due date surfaces the clear expired_window error (F1), leaves the stored date unchanged, still allows editing an already-overdue loan, and keeps the distinct invalid_dates path. Also record F1/F2, #371 and the mobile notifications-dropdown fix in the 0.7.64 changelog.
|
Important Review skippedToo many files! This PR contains 107 files, which is 7 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (107)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The Updater derives a migration's version from its filename, so migrate_0.7.64-rc.1.sql is recorded as version '0.7.64-rc.1', not '0.7.64'. Update the upgrade-smoke contract (migrations WHERE version) and the range-predicate matrix accordingly — the -rc.1 migration must also run on an upgrade to the 0.7.64-rc.1 prerelease, which is the whole point of the rename.
|
@coderabbitai review |
|
Consolidates the open #238 work and the two follow-up PRs into a single 0.7.64 release.
What this bundles
prestiticolumns and an idempotent, RC-named migration; the DB triggers treat a stale overduein_corsoloan as open-ended; NCIP hardening (guarded cancel, closes a cross-user check-in hole) while staying strict; the release pipeline is split into a read-only build job and a privileged publish job with provenance.sessoenum renders through translated labels.Review fixes applied on top
expired_windowerror instead of the opaqueloan_update_failed, while still allowing edits to an already-overdue loan.migrate_0.7.64-rc.1.sqlso the Updater selects it even when the release ships from a prerelease.Tests
tests/loan-review-0764.unit.php19/19,tests/loan-review-0764.spec.js6/6) against the real DB + triggers and the live browser, plus the existing multiple-copy E2E and migration units. PHPStan level 5 clean, i18n parity across all five locales.Closes #368, #370, #371.