Repository navigation
Circulation: patron types, rules matrix, reservation limits, card duration (0.9.0) - #508
fabiodalez-dev wants to merge 52 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
@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 small rough edges inline.
Reviewed changes
Initial review of the full circulation policy stack: one rules service, patron types and the rules matrix, consultation-only copies, reservation limits and the card duration. Coverage: I read the controllers, services, plugins, migration and schema line by line. The views, locale files and tests got only a targeted check (escaping and form field names), not a full read.
CirculationPolicy+PolicyResolver— a pure resolver applies the order rule(type, media) → rule(type,*) → rule(all, media) → patron type → globals, field by field. Every former direct read ofloans.*goes through it, and a CI grep guard blocks new direct reads.- Gates rewired — staff store/update/renew, approval, reader requests (web and app), queue promotion (
totalLoanLimitSql),CapacityService, NCIP checkout/request/extend and the mobile APICirculationOutcometables all use the sharedloanLimitError()/reservationLimitError()codes. - Consultation-only copies —
lendableCopySql()is added to every copy allocation and capacity count.CopyController::toggleConsultationlocks book then copy, and refuses a copy that is still held by a loan or a request. - Patron types and matrix admin —
CirculationSettingsControllerroutes sit behindAdminAuthMiddleware+CsrfMiddleware(settings.manage). Integer inputs are range-checked, and column names come only from constants. - Card duration —
PatronTypeAssignmentsets the expiry on create, registration and approval. "Rinnova tessera" has its own route. Thecard_expiringmail is sent once per expiry date, usingGET_LOCKper patron and the ack columncard_expiry_warning_sent_for. - Migration — the guarded, idempotent "Circulation policy" section matches
schema.sql. The MARCimport_typeenum is now built from the currentCOLUMN_TYPE, so the order of the sections no longer matters.
claude-opus-5-5 ｜ 𝕏
|
Important Follow-up to the Pullfrog review: the enforcement-parity check finished after that review was posted and found a queue-promotion defect.
|
d151e9f to
d480ee2
Compare
There was a problem hiding this comment.
✅ No new issues found. The two inline threads and the three follow-up points from the last review are fixed.
Reviewed changes
Two commits since the last review at c9f629d: bff3f4d and cdd157b.
- Queue promotion skips patrons refused for the material. The new
CirculationPolicy::materialRefusedReservationUserIds()lists patrons whose type cannot borrow the title's material, or who already hold the per-material limit.ReservationManager::processBookAvailabilitynow skips them as candidates.CapacityService::activeReservationIntervalsno longer counts their reservations as held capacity, but only while those reservations are promotable. This mirrors the existing total-limit pause, so the two predicates stay aligned, and theloanLimitError(forLoan(...))check under the user lock is still the final gate. With no matrix rules, nothing changes. - Deactivated patron types are refused on the user form.
PatronTypeAssignment::fromSubmissionnow rejects a deactivated type unless the user already has it. - NCIP
RequestItemuses the request window. The due date now comes fromrequestDurationDays(), the same as the website and the app. StaffLoanReader::maxRenewalsForno longer queries per row. It resolves from patron type and material via the memoisedpatronTypeOf()/mediaOf(). This is equivalent: the consultation layer only changesloanable, nevermaxRenewals.- Fixed the garbled comment on the
card_expiry_warning_daysline inSettingsController::updateLoansSettings. - Added
tests/circulation-queue-material-rules.unit.php. It covers the per-material limit, a material the type cannot borrow, an unchanged queue head when nobody is refused, and thefromSubmissioncases.
claude-opus-5-5 ｜ 𝕏
|
Follow-up to the Pullfrog enforcement-parity notes:
|
|
@coderabbitai full review |
|
6825635 to
4ae6f89
Compare
|
@coderabbitai full review |
|
93f7848 to
b17224d
Compare
aee6cd4 to
3e181d3
Compare
|
Security fix in b032ce7: every |
There was a problem hiding this comment.
ℹ️ No critical issues. The server-side guard is complete; one UX gap is noted inline.
Reviewed changes
One commit since the last reviewed head 3e181d3: b032ce7.
- Circulation settings limited to administrators.
CirculationSettingsController::guard()checksPermissions::mayConfigure(), which passes administrators and custom roles that holdsettings.manage. All eight actions call it:index,editForm,createType,updateType,deleteType,setDefault,moveTypeandsaveRules. A refused request sets the flash message and redirects to/admin/dashboardbefore any read or write. Each action starts with the guard, so nothing is changed before the check. - Added
tests/circulation-settings-access.spec.js. It checks that staff are refused on both GET pages and all six POST forms (via a CRC snapshot oftipologie_lettore/regole_prestito), that the administrator can still edit and delete, and that staff can still reach/admin/loans. - Added the CHANGELOG entry "Administrators only".
claude-opus-5-5 ｜ 𝕏
3e075d5 to
5add09a
Compare
Loan duration, request window, renewals and the active-loan limit were read from the loans settings in about twenty places: the staff desk, reader requests on the website and in the app, approval, queue promotion, renewals and the NCIP server. They now go through App\Services\CirculationPolicy, which returns immutable UserPolicy/LoanPolicy value objects resolved by the pure PolicyResolver, and every due date comes from CirculationPolicy::dueDateFor(). This is a pure refactor: with only the global settings every number is the one the old read sites computed. The active-loan state set is one shared constant, the queue SQL predicates take the limit from the policy, and the SettingsRepository duration helpers are deprecated delegates. A unit test compares each historical expression with the service over a grid of stored values; a CI step rejects new direct reads of these settings.
Every reader now belongs to a patron type (tipologie_lettore) that decides the circulation rules, separate from the role that decides permissions. A type sets the total active-loan limit, loan duration, longest request, renewals, reservations and their limit, and the card duration; an empty field inherits the global loans setting. The matrix (regole_prestito) adds exceptions per material type: consultation only, a loan limit for that material on top of the total, duration and renewals. Per field the first value wins among rule (type, material), rule (type, any), rule (any, material), patron type, global settings. A single copy can be kept for in-house consultation: it is never allocated nor counted as lendable capacity, and a title whose every copy is kept for consultation is consultation only. Staff desk, reader requests, approval, queue promotion, renewals, the Mobile API and NCIP refuse with the same new codes (media_limit_reached, not_loanable, reservations_disabled, reservation_limit_reached). The settings page gets a Circulation tab, the user forms a patron type select, the reader page the effective rules, the user list a filter and a CSV column, the book page "Solo consultazione". The migration seeds a default "Standard" type that inherits everything and touches no existing user, so an install that configures nothing behaves as before.
The patron type's reservation rules are now visible and tested end to end: a type without reservations does not see "reserve when available" on the book page, and the admin reservation form can override the limits with an explicit tick. When the patron type sets a card duration, the card expiry is computed on creation and edit (optional, on by default when creating), on self-registration and on registration approval. The reader page gets a "Rinnova tessera" button: today + the type's duration (one year when it sets none), an expired account becomes active again, the renewal is in the activity log. A new "card_expiring" email (five languages) goes out loans.card_expiry_warning_days before the card expires (default 0 = off), sent by the notifications cron once per expiry date and re-armed by a renewal (utenti.card_expiry_warning_sent_for).
The MARC section spelled the whole enum out, so running it after a section that had already added its own value (users) would have dropped that value. It now appends marc to the current COLUMN_TYPE, as the patron-cards section does, and the test proves an earlier value and its rows survive.
With the patron types schema in the tree the Tipologia column is now offered and assigned. Only active types resolve, as in the user form; an unknown or deactivated code keeps the default type and is reported as a warning. The E2E imports codes in both cases, an empty one and an unknown one, and reads the result on the reader page.
loansByPatronType() gets its built-in provider: every active type, a deactivated one only when it has loans, readers without a type counted under the default type. A filter on statistics.section.patron_types still wins.
A matrix cell can now set giorni_rinnovo next to duration and renewals. The book page's renew button and its confirmation said a fixed +14 days while the renewal adds the days of the loan's circulation rules; they now say those.
…ivated patron type from the user form A reservation whose patron is at the limit for the title's material, or whose patron type may not borrow it, no longer stalls the queue: promotion and capacity occupancy both leave it aside, like a patron at the total limit. The user form keeps refusing a deactivated patron type server-side, as the form and the CSV import already did.
The debt-block bullet described the threshold as "own type → default type → global"; CirculationPolicy consults the default type only for a reader without a (valid) type, and an empty field of the type inherits the global setting. The bullet now says that. The Mobile API 1.9.0 changelog in plugin.json listed only /me/movements and debt_block; it now also names the API changes of #508/#509 released under the same version (closed_dates, loanable and reservations_enabled in the availability payload; the 409 codes media_limit_reached, not_loanable, reservations_disabled and reservation_limit_reached; override_limits on the staff reservation; eligibility_reason debt_block), and the OpenAPI descriptions tag them "Mobile API 1.9.0" instead of "Pinakes 0.9.0", the version a client can actually check. The French fine_charged body used "Livre:" where the subject and every other French string write "Livre :"; the six labels are fixed in the shipped template and in the installer seed, which stay identical.
c7359fc to
92d5591
Compare
71a8719 to
3fc5ec3
Compare
… new schema Reassigning a loan to a copy refused as media_limit_reached or not_loanable redirected to /admin/loans, which only had the generic update error for them; the list now shows the same messages as the loan form. After the migrations the Updater clears CirculationPolicy's per-connection schema cache, so the availability recalc of that request uses the consultation column the migration just created.
The method exists in the release that ships this Updater, and PHPStan flags the method_exists() guard as always true.
There was a problem hiding this comment.
✅ No new issues found. The loans list now explains the two new refusals, and the post-migration recalc reads a fresh schema probe.
Reviewed changes
Since the last review at 35ca0b0, there are two PR commits (6a14766, d219798) and a merge of feat/patron-cards-import (f51fcb7). The merge needed no conflict resolution, and its files belong to the base PRs.
- Loans list explains
media_limit_reachedandnot_loanable. Reassigning a loan inPrestitiController::updateredirects to/admin/loans?error=<code>fromloanLimitError(forExistingLoan(...)).prestiti/index.phpnow shows the same messages ascrea_prestito.phpfor these two codes, not the generic update error. Both strings already exist in all five locales. - The post-migration recalc probes the schema again.
Updater::recalculateAfterMigrations()now callsCirculationPolicy::resetSchemaCache()beforerecalculateAllBookAvailability(). AschemaReady()result cached before the migration createdcopie.solo_consultazionecan no longer makelendableCopySql()return1 = 1during that recalc.d219798removes themethod_existsguard that PHPStan flagged as always true. The Updater that runs this line ships in the same release as the method.php -lpasses on both files, and the locale JSONs parse.
claude-opus-5-5 ｜ 𝕏
|
@coderabbitai review |
|
There was a problem hiding this comment.
ℹ️ No code issues. One merge left duplicate bullets in the CHANGELOG (comment inline).
Reviewed changes
Since the last review at d219798, the PR has had only merges of feat/patron-cards-import. This run covers the three new ones on top of 2f856c3. The base files they bring in belong to the base PRs.
- Merged
feat/patron-cards-importthree times (7b863ec,dd6c348,7977fa4).dd6c348and7977fa4needed no conflict resolution. In7b863ecthe only resolved conflict is the CHANGELOG### Migrationlist. - Checked the net PR diff. The range-diff against the last head changes only
CHANGELOG.md. All five locale JSONs parse and have no duplicate keys.app/Support/UserImport.phppassesphp -l.
claude-opus-5-5 ｜ 𝕏
A merge kept both sides of the 0.9.0 Migration list, so the stocktake and MARC sections were each described twice. The newer stocktake bullet (with the close's choice and the frozen copy code and title) and this branch's MARC bullet stay.
|
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. |
Patron types and per-type/per-material circulation rules (SLiMS Member Types + Loan Rules), reservation limits and automatic card duration.
Phase 0 — one source for the rules (pure refactor, own commit). New
App\Services\CirculationPolicyreturningfinal readonlyUserPolicy/LoanPolicy; purePolicyResolverwith the precedence rule(type, material) → rule(type, *) → rule(all, material) → type → global setting. Every place that readloan_duration_days,max_active_loans_per_user,max_renewals,max_loan_duration_days(website, admin, approval, queue promotion, reservations, mobile-api, NCIP, plus a few the plan missed) now goes through it; the active-loan states are one shared constant. A new CI step ("Circulation policy guard") rejects direct reads outside the service. An equivalence unit test compares every old read site with the new service (50,598 checks). With no patron type configured the behaviour is exactly the same as before.Phase 1 — patron types and the rules matrix. Tables
tipologie_lettoreandregole_prestito(sentinels0= all types,'*'= all materials, so the UNIQUE key works without NULLs),utenti.tipologia_id(FK ON DELETE SET NULL),copie.solo_consultazione. The migration seeds a default "Standard" type with every field empty (inherits the global settings) and touches no user; role (tipo_utente) and patron type stay separate concepts. Admin tab "Circolazione" in settings (types CRUD, order, default; matrix with inherited values greyed, days per renewal), patron type select + effective rules summary in the user forms, list filter and export column, "Solo consultazione" on the book page, per-copy consultation flag. New refusal codesmedia_limit_reached,not_loanable,reservations_disabled,reservation_limit_reachedinLoanEligibility, mobile-apiCirculationOutcomeand NCIP (Item Does Not Circulate / Maximum Check Outs Exceeded). The renew button on the book page now shows the real renewal days instead of a fixed "+14".Phase 2 — reservations and card. Types without reservations lose the reserve button and are refused everywhere; reservation limit = queue reservations + pending loan requests, staff can override explicitly. Card expiry computed from the type on creation, self-registration and approval ("calcola" checkbox), "Rinnova tessera" on the reader page with ActivityLog, optional
card_expiringemail (loans.card_expiry_warning_days, 0 = off) sent once per expiry by the cron.Integration with the PRs below: user CSV import assigns the patron type by code (active types only, unknown codes reported per row); statistics show loans by patron type; the MARC migration section now builds the
import_logs.import_typeENUM from the current values so section order does not matter.Choices made: existing
premiumusers are not migrated to a type; material dimension =tipo_media;regole_prestito.prestabileis nullable so a rule that only sets a limit does not re-enable a material made consultation-only by a broader rule.Tests: E2E
circulation-patron-types9,circulation-rules-matrix10,circulation-consultation11,circulation-interop8 (mobile API + NCIP),circulation-reservations10,circulation-card15;user-import7 andstatistics-0907 extended; regressionloan-reservation21,mobile-api-circulation22,ncip-server32,inventory-stocktake9,marc-bulk-import7, full-test 162 passed / 9 skipped (install phase). Unit: equivalence, resolver, migration section (real SQL twice on a sandbox). PHPStan 0.