Repository navigation
Release 0.8.5-rc.1: Mobile API 1.7.0 staff area, offline sync, private library calendar - #478
Merged
Merged
Conversation
Admin and staff accounts can now manage the catalogue from the companion app under /api/v1/staff, behind a new role guard (any other role gets 403 forbidden, re-checked on every request): - lookups for the book form, ISBN/EAN lookup through the full scraping pipeline with duplicate detection, and an operator book search that also lists wanted titles; - book create and update delegated in-process to LibriController::store() and update(), so hooks, identifier locks, copies, collocation, series and the activity log behave exactly as from the web form. A PUT re-posts the complete current record, so absent keys keep their value and only the sent fields change; - copies, cover upload (re-encoded with GD) or cover from URL, and soft delete guarded by the same active-circulation check as the web; - authors, publishers, genres, bookcases and levels, with merge-style updates that keep the fields the app does not edit. The circulation check lives in the plugin (pinned to the core queries by a unit test) so requires_app stays unchanged. Every new route is documented in the OpenAPI document and has its two-call row in the idempotency manifest.
GET /api/v1/catalog/sync lets the app keep a local copy of the public catalogue. Without `since` (or with one older than the 30-day tombstone window) it returns a full resync; with the previous server_time it returns only what changed since: edits as upserts, soft deletes and withdrawn (wanted again) titles as deletes. A wanted title that never reached the catalogue is never announced, following the same rules as ResourceSync. Pages are keyset-ordered on (changed_at, id) and all pages of one pass share the database-clock watermark carried in the cursor. The health payload advertises catalog_sync.
Borrowers can subscribe their calendar app to their own loans:
- GET /me/calendar shows the subscription, POST /me/calendar/token creates
or rotates the secret URL (the old one stops working at once) and
DELETE /me/calendar/token revokes it;
- GET /calendar/feed/{token}.ics is public, because calendar clients cannot
send a bearer token: the 256-bit token in the path is the credential. It is
stored only as a sha256 hash plus an encrypted copy, so the app can show
the same URL again.
The feed lists only the owner's approved loans, without any name or e-mail
address, with the same UIDs as the library calendar, SEQUENCE taken from the
loan's updated_at, a reminder the day before the due date, RFC 5545 escaping
and 75-octet folding. It answers 304 to a matching ETag and 404 for unknown
or revoked tokens, a disabled app access, catalogue mode or a suspended
account.
The token table is created by ensureSchema() and declared in
expectedTables(), so existing installations get it on upgrade or on the next
boot; the unit test drives that through PluginManager.
The version bump makes PluginManager re-run onActivate() on upgrade, so the calendar token table reaches installations that already run the plugin. The changelog replaces the stale 1.4.4 entry, and docs/api.MD points to the OpenAPI document for the staff, sync and calendar routes. requires_app is unchanged: the plugin uses no core API newer than before.
The personal calendar feed URL is a credential: whoever holds it reads the owner's loans. CredentialRevoker::revokeAll(), which a password reset or change runs, now revokes it together with the remember-me sessions and the device tokens, through the same guarded path (the table belongs to the optional Mobile API plugin and is skipped only when absent). The result gains a `calendar` count; the existing keys are unchanged. Token resolution also requires the owner to be allowed to sign in (active and verified, the rule AppAuthMiddleware applies), so suspending an account closes its feed at once, without waiting for a revocation. Tests cover a password change through POST /me/password, a suspended, expired or unverified owner, and revokeAll() on the token table.
A PUT carrying only authors_new or publishers_new used to replace the whole list, so adding one author dropped every existing one. Each key is now independent, as the OpenAPI description already said: author_ids / publisher_ids replace, an absent key keeps the current links, the *_new keys only add. I also moved the create-time reference validation inside the try block so a database failure there still answers with the API envelope.
…lendar /calendar/events.ics (and its legacy /storage/calendar alias) needs no login and has a guessable URL, yet every event description carried the borrower's full name and email address. The anonymous feed now keeps the same events without personal data, so existing subscriptions keep working. The dashboard hands operators a link with an HMAC token derived from the existing per-installation calendar secret; only that link includes borrower details, and it is served with a private Cache-Control. The generated file under storage/calendar follows the anonymous rule. tests/library-calendar-privacy.spec.js covers the three cases and fails on the previous code.
…t its constructor tests/desiderata-visibility.integration.php instantiates CatalogController through reflection without calling the constructor, so the new operatorView flag was read before initialization. A false default keeps every such instance on the public visibility predicate.
Settings -> Registration gets a "Campi visibili" group with two switches, show_sesso and show_cod_fiscale, stored next to the require_* toggles and mapped in ConfigStore. Both default to on, so existing installs change nothing. When a field is off it disappears from the public registration form, the member profile, the admin user create/edit forms and the admin user detail. Every save path keeps the value already stored for a hidden field (profile, admin edit, PATCH /me), and registration ignores it even if a crafted request sends it. Exports are untouched. Mobile API: /auth/registration-fields gains profile_fields with a visible flag for data_nascita, sesso and cod_fiscale (builtin_fields is unchanged), /health mirrors it as show_sesso / show_cod_fiscale, and /me omits hidden fields and lists them in hidden_fields. The OpenAPI document describes the new shape. I added an E2E suite that drives the whole flow as admin, visitor, member and app, a DB-backed unit test for the setting mapping, and the new strings in all five locales.
On the admin author and publisher pages every card used .card, whose padding main.css removes below 768px, so the info card (total books, created, updated) and the catalogue had their text against the border. They now use the same wrapper as the user detail cards (bg-white border border-gray-200 rounded-lg p-6), which keeps its padding at every width. Book and article covers in those catalogues switch from object-cover to object-contain inside the same fixed box, like the book record, so a cover is never cut; the hover zoom that cropped it on desktop is gone. The new phone-width E2E test measures the card insets, the painted cover size against its box and the page width on both pages.
Below 768px main.css turns every table into a scrolling block. FullCalendar draws its month grid with tables, so on the dashboard and on the book copy availability calendar the "Mese" view collapsed to a 4px strip. admin-ui.css gives .fc tables their table layout back on phones; the list view and desktop are unchanged. On the new loan form flatpickr replaced its availability-coloured calendar with the native date picker on mobile browsers, so a librarian on a phone never saw which days were taken. Both pickers now set disableMobile, as the public loan request calendar already does. The new E2E test opens every month calendar at 375px with an Android user agent (dashboard, book copy calendar, public loan request, new loan) and checks the month grid size, that it stays on screen and that the page does not scroll sideways.
The standard FullCalendar bundle ships no locale files, so with any locale the empty list said "No events to display" and the overflow and all-day labels stayed English. Both admin calendars now pass those texts through __(), translated in every locale; the phone calendar spec checks the empty message follows the app language and fails on the previous views. The mobile-api changelog now mentions the hidden registration fields.
The author and publisher pages no longer use group-hover:scale-105, so the purged Tailwind build drops that rule; the committed asset still had it and the reproducible-build checks failed on the diff.
… file The dashboard route is shared with patrons, yet the tokenized calendar link was built for every role, and its token was one global HMAC that could never be withdrawn. Only admin and staff now get a link, bound to their own account (op + token derived from a dedicated library_ics_secret) and re-checked on every fetch: a demoted, suspended or deleted operator falls back to the anonymous variant without personal data, and deleting the secret revokes every operator link without touching the per-loan links sent by email. Installs that serve storage/calendar straight from Apache kept publishing the file written by earlier releases, so the once-per-version plugin maintenance pass now rewrites it when its descriptions still carry an email address, or removes it if the rewrite fails.
Renames and merges only touch the related tables, so /catalog/sync (and ResourceSync) kept serving the old names until a full reload. BookChangeStamp moves libri.updated_at of the non-deleted books linked to the renamed author, publisher or genre. The sync also looks back five seconds behind since, so a write committed just after the previous watermark is not lost; re-sending a few rows is harmless. tests/book-change-stamp.integration.php drives the real repositories, the public ICS self-heal and the operator link checks.
…uthor and publisher counts Re-sending the shelf and level a book already has, without a position, made the core pick the next free slot and moved the book to the end of the level; the stored position is now kept, while a different level still gets the next free slot. Author and publisher responses gain books_count, created_at and updated_at so the app's card matches the web page. The circulation check doc now names the right test file.
…a revoke error generate(), saveToFile() and the description helpers now default to no personal data, so a caller that forgets the flag publishes nothing; the per-loan file emailed to the borrower passes true explicitly because it is their own. CalendarTokenService::revoke() threw nothing when its UPDATE failed, so the API answered enabled:false while the feed URL kept working; it now raises like rotate() does.
A Regenerate link button, shown to admins only and confirmed first, deletes the dedicated library_ics_secret so every operator link copied so far falls back to the anonymous feed, while the per-loan links emailed to borrowers keep their own secret. The POST route sits behind CSRF and AdminAuth and re-checks the admin role inline, since AdminAuth also admits staff. The privacy spec rotates the link through the real UI and checks the old link loses borrower details while the new one keeps them.
…s in tests Both specs write the registration settings straight into the database and then cleared ConfigStore from a CLI process. In CI the web server keeps those settings for 60 s in APCu, which no CLI process can reach, so /health could still report the previous require_* values and test 7 of mobile-api.spec.js failed depending on timing. clearConfigCache() now also calls the shared flushCache() helper, which hits /_e2e/flush-cache inside the app process.
…estores the install locale The spec flips languages.is_default and app.locale with direct SQL, but the language list is cached for five minutes inside the web server (APCu in CI). After the restore, the specs that followed in the same shard still rendered German routes and labels: mobile-tabbar saw Startseite/Katalog/Anmelden and openurl-resolver got /zeitschriften/ links. The spec now calls the shared flushCache() helper after both writes.
BookChangeStamp only logged a failed UPDATE, so a rename or merge could succeed while libri.updated_at stayed put and the offline sync never saw it. It now throws, and the writes run as one unit: author, publisher and genre renames go through the new DbAtomic helper (own transaction, or the caller's when one is already open, detected with the same autocommit + SAVEPOINT probe), and author and publisher merges stamp the books inside their transaction, before the commit. CredentialRevoker::atomically() now delegates to DbAtomic, with no change in behaviour.
Scraping sources often return the same person in both forms ("Primo Levi" and "Levi, Primo"), and the app preview listed both. The preview now merges them through AuthorNormalizer, the same canonical form the save path uses.
The feed already spanned each loan from its start to the due date (or to the pickup deadline) with a reminder the day before; active reservations now appear too, from the requested start to the requested end, with a stable reservation UID. Cancelled reservations and other users stay out; the calendar spec checks both and the exclusive all-day DTEND.
…g it In the CI regression shard an earlier spec left the Mobile API app-access gate closed, so /auth/registration-fields answered 403 while /health still answered 200 and test 6 failed. The spec now opens the gate through the shared ensureMobileApiReady() helper, flushes the web server's settings cache, and restores the previous gate in afterAll.
PublisherRepository::update() now throws when the rename and the books' change stamp are rolled back together, and EditorsController::update() never checked the result anyway, so a failed save ended in a 500 or in a silent redirect. The controller now catches the failure, logs it and redirects to the publisher list with a translated error message. The author and genre controllers already handle both cases.
…le API The staff API runs the admin controllers in-process. Those controllers leave flash messages, form drafts, import job state and one-time form tokens in the session, so a delegated call could leak them into the next page a browser renders with the same session. Delegation takes every such key out before the call, captures what the action wrote and restores the prior values afterwards, also when the action throws. StaffBooksController now delegates through it with the same behaviour. The unit test greps the delegated controllers for session writes, so a new key fails there instead of leaking.
Operators can now run the loan desk from the app: desk counters, loans by bucket (pending, pickup, scheduled, active, overdue, history), create with an optional scanned copy code, edit, approve and reject requests, confirm or cancel a pickup, return with the copy outcome and a penalty, return by scanned code, renew, bulk extend, single and bulk recall, receipt PDF and receipt email, reservations with their queue (create, edit, cancel, reactivate), borrower search with eligibility, copy lookup by code, a title's queue and its availability calendar for a given borrower. Every write is delegated to the web controllers (PrestitiController, LoanApprovalController, ReservationsAdminController) with the form body the admin pages post, so locks, eligibility, capacity, queue promotion, emails and the activity log are exactly those of the website. Core outcome codes map to stable API error codes through CirculationOutcome; the unit test pins every ?error= literal of the core controllers against the tables. A new loan's id is resolved from the high-water mark taken before the call, since the core redirect does not carry it. Partial PUTs merge over the stored row. The whole area answers 404 feature_disabled in catalogue mode. Also: /health gains the 1.7.0 staff_* flags, /staff/lookups gains series_types, return_outcomes and copy_states[].editable, the OpenAPI document covers every new path and the idempotency manifest has a row for each. Mobile API 1.7.0.
The 1.7.0 health payload listed thirteen staff areas as available although only the circulation desk is registered so far. StaffAreas is now the single source of truth: the plugin mounts an area's routes only when it is listed as implemented, and HealthController sets staff_<area> to true only for such an area whose prerequisite also holds (catalogue mode off, plugin active, collane table present). The OpenAPI document lists only the mounted paths and explains the flag semantics. The unit test checks every flag against the list, and the E2E health test checks that each true flag has its paths served.
POST /staff/loans minted a fresh one-time form token on every call, so the core replay guard never fired: a client retrying after a timeout could lend a second copy when several loans of one title are allowed, and two concurrent creates could resolve to the same new id. Both creates now accept an Idempotency-Key header (or body idempotency_key). The first successful create is remembered for 24 hours per operator in mobile_idempotency_keys; the same key with the same payload answers the original record with 200 and meta.replayed, with a different payload 422 idempotency_conflict. Serialisation uses MySQL advisory locks, because the delegated core controller commits its own transaction on the shared connection: a key lock makes a concurrent duplicate wait and replay, and a per-title create lock makes the high-water mark id resolution belong to this request. The new loan is resolved only when exactly one diretto row processed by this operator lies above the mark; anything else is a 500, never someone else's loan. Tests: a retry returns the same loan, a different payload is refused, two concurrent creates with one key make one loan, three concurrent creates without a key each get their own loan, reservations follow the same rules; the sandbox unit covers the table on activation, upgrade from 1.6.0 and self-heal, plus key parsing, expiry and lock exclusion.
A duplicate-key error quotes the offending value, which can be a contact email or a tax code, and SecureLogger does not mask the message field. The controller now logs only the publisher id, the exception class and its code, and logs the plain false outcome too, so both failure paths leave a trace.
…orks, digital files, periodicals desk, book clubs Desiderata: the proposal list, accept, reject, receive (creating the copy and closing the request), delete, the requested titles with their open proposals and the direct receipt, all delegated to DesiderataPlugin with stable codes for offer_closed, open_offers and not_wanted. Archives: units and authority records created, edited, published or hidden and deleted through the Archives plugin's own admin actions, with its own validators run first (ArchiveValidator calls them, nothing is copied) so the app gets 422 with the field; cover and document uploads, authority links; reads include unpublished units through an operator view of CollectionsController. Tier 2: FRBR works and expressions with the book link (frbr-lrm), e-book and audio uploads through the digital-library handler, the periodicals desk in the emeroteca bridge (barcode lookup, receive, claim, new years and issues, subscriptions) and club management in the book-club bridge (clubs, workflow, members). The two bridges mount only when mobile-api ships its staff guard; emeroteca 1.15.0 and book-club 1.5.0. Every area is listed in StaffAreas and advertised in /health only when served. Specs: mobile-api-staff-plugins.spec.js, mobile-api-staff-tier2.spec.js, idempotency rows for every new path; unit: the archive validator against the plugin's private helpers.
collane.nome uses a case- and accent-insensitive collation, so renaming "Foo" to "foo" found the same row under the new name, went down the merge path, merged the series into itself and deleted it together with every libri_collane membership. This hit both the admin rename form and the staff API. renameSeries() now merges only when the new name resolves to a different row and otherwise renames the row in place by id. mergeSeries() refuses a source and target that resolve to the same row, and deletes the source by id instead of by name. The new unit test runs on a disposable database and fails on the previous code; the staff API spec covers the API and the web rename form for the same case-only rename.
DELETE /staff/imports/{id} now takes the same per-job lock as the chunk endpoint, waiting a few seconds, and answers 409 job_busy while a chunk is still running instead of flipping the job to cancelled under it. Cancelling also no longer turns a job that finished meanwhile into a cancelled one.
ImportJobStore::saveChunk() is now a compare-and-set on the running status the chunk sets before delegating, so a chunk that finishes after a cancel can never resurrect the job: it answers 409 job_cancelled with meta.processed, the rows already written stay and the job stays cancelled. The OpenAPI document lists both codes.
The unit test covers the compare-and-set on the store; the imports spec drives both races end to end through a trigger on libri (a slow insert to hold a chunk in flight, and a status flip mid-chunk), and both cases fail on the previous code.
The CSV importer clears its session state on the final chunk, so the job kept the state saved after the previous chunk: error_details missed the errors of the last chunk while the errors counter, taken from the final answer, included them. The job now takes error_details from the final chunk answer whenever it carries the list (the CSV importer returns the cumulative list, capped at 50) and falls back to the saved state only for the LibraryThing importer, which does not. The imports spec runs a two-chunk CSV with an error in each chunk and fails on the previous code.
POST /staff/books/{id}/work turned any expression_id that is not a clean positive id ("abc", "12x", -3, true, 0) into 0 and delegated it as an empty value, which silently cleared the book's expression. When the key is present it must now be null (clear) or a positive id; anything else answers 422 validation_error with meta.field expression_id before anything is delegated. The tier 2 spec sends each malformed value, checks the expression is untouched, and fails on the previous code.
The desiderata plugin answers 422 both when it refuses an action and when its database write failed and was rolled back. The staff API mapped every non-redirect answer of a delete to 404 not_found and of a direct receipt to 409 not_wanted, so a real failure looked like a stale request. After a non-redirect answer the API now re-runs the checks it already makes up front: the offer that vanished meanwhile keeps its 404, a title that is no longer wanted or has open offers keeps its 409, and the offer actions return 404 when the offer was deleted meanwhile. Any other answer is a 500 internal_error, logged with the plugin status only. The staff plugins spec forces a failing DELETE and a failing receipt insert through triggers, checks nothing was written, and fails on the previous code.
When an assertion in the job_busy test fails, the chunk held by the slow trigger used to land after afterAll had already removed the run's books and accounts, leaving a book and the operators behind. The test now always awaits the chunk before dropping the trigger.
The spec's mysql client ran with the client default charset, latin1 for the MariaDB client in PATH, so the accented series names in test 29 were stored and compared mangled and "Álfa" sorted after "Èvent". The helper now passes --default-character-set=utf8mb4, as the other specs that seed accented data already do; the whole spec passes locally.
Status labels, lookups and error messages came back in the installation's language even when the app was set to another one, so an English app showed "Prestato" next to English text. A LocaleMiddleware on the /api/v1 group now negotiates Accept-Language against the installed languages (exact region first, then the same language, honouring q) and echoes the choice in Content-Language; a header naming no installed language changes nothing. Covered by tests/mobile-api-locale.unit.php and a new case in mobile-api.spec.js.
|
Important Review skippedToo many files! This PR contains 133 files, which is 33 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 configuration
📒 Files selected for processing: 133
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The only commit not already in #477 is the release commit ca941e34. git diff c1831190 ca941e34 touches just CHANGELOG.md, README.md and version.json. The 42 commits before it match #477's head, and Pullfrog's last review there (at c1831190) found no new issues. I checked every claim in the release commit against the code:
version.json→0.8.5-rc.1. This matches theX.Y.Z-rc.Npattern thatscripts/create-release.shandscripts/ci-verify-release-source.shaccept. The changelog heading is exactly## [0.8.5-rc.1], whichcreate-release.shrequires.- Plugin compatibility. The highest
requires_appamong the bundled plugins is0.7.88(emeroteca). Each is below0.8.5-rc.1underversion_compare, soPluginManagerkeeps every plugin loaded on the RC. - Changelog accuracy. Each claim matches the code:
- The plugin versions are Mobile API
1.7.0, Book Club1.5.0and Emeroteca1.15.0. UPDATER_CHANNEL=rcandUPDATER_ALLOW_PRERELEASE=1are whatUpdater::prereleaseChannelEnabled()reads.- Sync deletions are kept for 30 days (
SyncController::TOMBSTONE_DAYS). - A genre rename stamps its books through
BookChangeStamp::touchForGenre. A genre merge writeslibridirectly, soON UPDATE CURRENT_TIMESTAMPstamps those books. - A password change revokes the calendar link with the other credentials, through
CredentialRevoker::revokeAllinProfileControllerandPasswordController.
- The plugin versions are Mobile API
- README. The new
v0.8.5-rc.1 — release candidateblock sits abovev0.8.4 — latest. That is correct, because stable installs never see a prerelease.
claude-opus-5-5 ｜ 𝕏
…e plugin rejected The periodicals desk now finds a new issue, year or subscription by what the request sent (issue number in its year, year and volume in its masthead, supplier and dates) instead of the highest new id, so two operators adding at once each get their own row back. Work and expression edits, and book-club member updates, answer 422 when the plugin action refused the write; work, expression and club deletes re-read the record and answer 500 if it is still there. The idempotency spec names the fixture endpoint when a creating call fails.
…release/0.8.5-rc.1
…alues A subscription has no natural key, so two identical creates are two rows and matching on supplier and dates could still hand both callers the same one. The admin create action now carries the new id in an X-Created-Id header on its redirect (a browser ignores it) and the periodicals desk reads it. Issues and years keep the lookup by their unique key and lose the MAX(id) fallback, which could answer with another request's row.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the last review (ca941e34), the only new code is 87a160b3, merged in from #477. It touches 7 files: git diff ca941e34 a7e9eefc.
- Works and expressions.
StaffWorksControllernow answers a PUT with 422 if the frbr action refuses it, meaning it does not redirect or it leaves anerror_messageflash. After a DELETE it re-reads the row and returns 500 if the row is still live.FrbrLrmPlugin::adminUpdateAction,adminEspressioneUpdateActionand both delete actions always redirect and never flash today, so these checks change nothing for current code. - Book clubs.
MobileStaffBridgenow returns 422 when club delete or member update leaves abookclub_flashwithtypeerror, and member update also returns 422 when there is no redirect. A delete that leaves the club visible toclubById, which filters ondeleted_at IS NULL, now becomes a 500. The flash shape matchesBaseController::flash(['type', 'message']), andbookclub_flashis in this bridge's isolated session keys. - Periodicals. After delegating,
MobileStaffModulenow looks up the new issue, year or subscription by the values this request sent (number, year and volume, or supplier and dates). Each lookup falls back to the oldMAX(id)query.stored()cleans values the same wayIssueAdminControllerandSubscriptionAdminControllerdo: it callstrim(strip_tags()), caps the length and turns''intoNULL.addAnnatasaves an empty volume as'', which matchesstored() ?? '', and dates that may beNULLare compared with<=>. A narrow match can only find a row created after$beforewith the same unique key or the same values, and when it finds nothing the code runs the old query. So this cannot do worse than before. - Repositories and spec. Added
@phpstan-impuretoRepo::clubById,OpereRepository::existsandEspressioniRepository::getById. The idempotency spec fixture now expects 201 from every non-GETstaffJsoncall. Every such call is a create.
All six changed PHP files pass php -l.
claude-opus-5-5 ｜ 𝕏
…release/0.8.5-rc.1
…e work The expression create action redirects to its work, so the staff API guessed the new row with MAX(id) and two concurrent creates for one work could both get the same id back. The action now carries its insert id in an X-Created-Id header and the API reads it. The book-club create drops its MAX(id) fallback too: the redirect already names the club, and a guess could hand back another request's club.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the last review (a7e9eefc), the only new code is 819c593d, merged in from #477. git diff a7e9eefc fa2b60be touches 2 emeroteca files:
- Subscription create returns its own row. On success,
SubscriptionAdminController::createSubmitnow adds the insert id to its redirect as anX-Created-Idheader (CREATED_ID_HEADER).MobileStaffModulereads that header fromDelegationOutcome->response, which is the controller's own response, and no longer guesses the row withMAX(id)lookups. The header is only set after a successful insert, and the module refuses error flashes and non-redirects before reading it, so a correctly saved subscription always has an id to resolve. Edits still use thesidfrom the path. - Issue and year creates no longer fall back to
MAX(id). Each lookup now relies only on the unique key. For issues that isUNIQUE(annata_id, numero), whichensureIssueNumberIndexadds. For years it isUNIQUE(testata_id, anno, volume), wherevolumehas beenNOT NULL DEFAULT ''since 1.4.0.stored()produces the same value asIssueAdminController, which storesmb_substr(trim(strip_tags()))for bothnumeroandvolume, and an emptynumerois refused before the insert. So the narrow lookup always finds the new row, and dropping the fallback removes the race where two concurrent adds could both get back the same row.
Both changed files pass php -l.
claude-opus-5-5 ｜ 𝕏
The fixture took the first public active club without checking deleted_at, so a club soft-deleted by an earlier run answered 404 to GET /bookclub/clubs/{slug} and stopped the rest of the spec.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the last review (fa2b60be), the only new code is 7e29d12e and 54cb42af, merged in from #477. git diff fa2b60be a09051e0 touches 4 files:
- Expression create returns its own row.
FrbrLrmPlugin::adminEspressioneStoreActionnow adds the id fromEspressioniRepository::create(theinsert_id) to its redirect as anX-Created-Idheader (CREATED_ID_HEADER).StaffWorksController::createExpressionreads that header fromDelegationOutcome->responseand no longer runs theMAX(id)lookup. The only path that redirects without the header is the "work missing" branch, and the mobile controller already returns 404 before it delegates. A missing or zero id still ends in the existingRuntimeException→ 500. - Book club create takes the id from the redirect.
MobileStaffBridgedropped the$beforeandMAX(id)fallback.AdminController::savealways redirects a successful create to/admin/book-club/{clubId}, whichlocationId('#/admin/book-club/(\d+)$#')matches. A failedcreateClubleaves an error flash, and that is answered with 422 before the lookup runs. - Idempotency spec. The book club fixture now also filters on
deleted_at IS NULL, so the spec never picks a soft-deleted club.
All three changed PHP files pass php -l.
claude-opus-5-5 ｜ 𝕏
The 2026 admin palette repaints every coloured text class in the neutral body colour with !important, so the latest version, its icon and the "new update available" banner read exactly like the installed version. They now carry pk-update-available, which keeps them in the theme's success green (the darker token, so the banner's small text stays readable).
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the last review (a09051e0), the only new commit is c3b960e2. It was made directly on the release branch and touches app/Views/admin/updates.php and public/assets/workspace-ui.css.
- Installable update shows in green again. When
$updateInfo['available']is true, the latest-version icon, the version number and the "update available" banner now carrypk-update-available. The new rule colours that class and thep/ielements inside it withvar(--success-color). It overrides the neutral!importantrepaint of[class*='text-green-']because it has higher specificity (0,2,2 and 0,2,3 against 0,2,1) and comes later in the file.--success-color(#187044) is defined on:rootininterface-tokens.css, whichlayout.phploads beforeworkspace-ui.css. The "up to date" and error states are unchanged.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the last review (c3b960e2), the only new commit is 8f95c6ad, made directly on the release branch. It turns the release candidate into the stable 0.8.5 and touches CHANGELOG.md, README.md and version.json.
- Version promoted to stable.
version.jsonis now0.8.5, which matches the stableX.Y.Zpattern increate-release.shandci-verify-release-source.sh. The changelog heading is exactly## [0.8.5]and appears only once. Every bundledrequires_app(the highest is0.7.88) is still below0.8.5. - Changelog updated. Dropped the RC prerelease note. Added a Fixed bullet for the updates-page colour fix from
c3b960e2, which describes that change accurately. - README highlights updated.
v0.8.5 — latestreplaces the RC block, andv0.8.4is no longer marked latest. The summary now mentions the updates-page fix.
ℹ️ The PR title and description still describe the RC flow
The title says 0.8.5-rc.1, and the description says the PR "is not merged" and that the tag will be a GitHub prerelease. The head now carries the stable version, and v0.8.5-rc.1 is already on the remote at a09051e0. A stable tag can only come from main: create-release.sh requires the local branch to be main and equal to origin/main, and ci-verify-release-source.sh rejects a stable tag whose commit is not in origin/main. So v0.8.5 can only be cut after this PR merges. Updating the title and description would keep the record accurate. Nothing in the code needs to change.
claude-opus-5-5 ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release candidate 0.8.5-rc.1
This is the release PR for
v0.8.5-rc.1. It contains #477 (Mobile API 1.7.0: staff cataloguing and circulation, plugin content, offline sync and a personal loans calendar, plus calendar privacy and phone layout fixes) and the release commit on top of it:version.jsonat0.8.5-rc.1, the## [0.8.5-rc.1]section inCHANGELOG.mdand the README highlights.The tag will be published as a GitHub prerelease, so the updater offers it only to installations that opt in with
UPDATER_CHANNEL=rcorUPDATER_ALLOW_PRERELEASE=1.Upgrade notes
No core migration. Mobile API 1.7.0 creates three new tables (
mobile_calendar_tokens,mobile_idempotency_keys,mobile_import_jobs) throughensureSchema()when it updates. Book Club goes to 1.5.0 and Emeroteca to 1.15.0 without schema changes. Every bundled plugin keeps arequires_appbelow0.8.5-rc.1.The anonymous library calendar (
/calendar/events.ics) stops publishing borrowers' names and emails; the static copy understorage/calendaris rewritten once per version if it still contains personal data.Before the tag
CLEANci-quality-local.sh,verify-schema.shandreinstall-test.sh(fresh install and real admin upgrade from 0.8.4) green locallyThis PR is not merged: the RC is tagged on its head, and #477 stays open for the stable release.