Repository navigation
Settings: granular 0.9.0 controls, safe upgrades and selectable holidays - #512
fabiodalez-dev wants to merge 61 commits into
Conversation
Each country lists its public holidays as rules with the native name: fixed dates, Easter offsets, nth weekday of a month, weekday on or before/after a date, and a fixed date on given weekdays with a fallback. Weekend rules cover the US, UK, Irish, Canadian and Australian substitute days, Koningsdag and the Argentine movable holidays; from/until years and one-year overrides (VE Day 2020, the Jubilees) are part of the rule. Regions: German Länder, UK nations, Canadian provinces, Australian states, Alsace-Moselle and six Swiss cantons.
The library's choice (country, region, ticked rules) is saved in system_settings and materialised in calendario_chiusure, marked with regola_festivita: a fixed date as one yearly row, any other rule as one row per year up to two years ahead. Applying again adds nothing, unticking removes only that rule's rows, manual and ICS closures are never touched, and rows of the former Italian preset are adopted instead of duplicated. A closure with attiva = 0 stays stored and closes nothing. Migration section 'holiday presets' adds both columns and the index.
App\Support\Features holds the eleven keys with their upgrade default (also the code fallback) and their fresh-install default. ConfigStore maps the features category, so a saved switch is read back. Settings gains a Modules tab for administrators; the three circulation modules are switched from the Loans tab. FeatureGateMiddleware refuses the routes of a module that is off (302 to the dashboard, or 404 feature_disabled for JSON callers). Fresh defaults are seeded in the five data_*.sql files; the upgrade defaults go in a new feature toggles section of migrate_0.9.0.sql, which turns a module on when the library already has data for it.
… and cron Sidebar entries, book list and book record actions, user list and record actions, form fields and settings tabs follow their module. The controllers re-check the switch after the route gate. Services carry the switch for every channel at once: CirculationPolicy::rulesActive() (patron types, rules, consultation copies), PatronLedger::active() (charges, debt block, previews, backfill, fine emails), LibraryCalendar::fromDatabase() (an empty calendar), RDA overrides and supplier writes. The visitor log purge waits while the module is off, the statistics sections follow their module, and the Presenze entry now also disappears in catalogue mode. Mobile API: /health.features gains fines, closures_calendar and circulation_rules; /me/movements answers 404 feature_disabled with fines off. The z39-server copy cataloguing routes follow the MARC module.
The holiday card of /admin/calendar lists the holidays of the chosen country and region with their native names and the dates of the preview year; the librarian ticks them (select all / none) and applies. Holiday rows carry a badge in the closures list and are changed from the selection only. A new card switches the yearly closures added by hand or from ICS on and off without deleting them. Maintenance keeps the movable holidays dated two years ahead and logs holiday_rows_added. Interface strings in the five locales; the old Italian-only import strings go.
holidays-preset.spec.js picks countries and regions, previews dates, applies a subset, reapplies, unticks, checks a due date on a ticked and on an unticked holiday, switches country next to manual and ICS closures, suspends a yearly closure and adopts former Italian rows. The closures-calendar-admin preset tests use the new card. The spec joins the circulation group of the 0.9.0 E2E job.
…section, settings and stocktake The unit suites of the modules themselves switch their module on with Features::override(), so they test the module whatever the database says.
… the reader record hides the patron type with the module off
…eature toggle suites Each suite of a module that is off by default on some installs enables it for its run and restores the stored value. Zero configuration now means the fines module is off, so the reader record shows no Movimenti section. The debt-block CLI probe loads .env like the cron bootstrap, so ConfigStore reads the module switch of the test database. CHANGELOG entry for the toggles.
… calendar switch The holiday preset and yearly closures routes carry the closures_calendar gate like the rest of the calendar, and the maintenance run adds no holiday row while the module is off.
- The fines probe of the migration also keeps fines on when a debt threshold or a fine is configured globally, on a patron type or in the rules matrix, so an upgrade from a release candidate that used only the debt block keeps it. - An ICS closure on a day a ticked holiday already covers is imported as its own closure: unticking the holiday later no longer reopens a day the file says is closed. - Switching the closures calendar on or off from Settings > Loans re-checks the pending pickup deadlines at once, as saving the calendar does.
|
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. One ledger gap is noted inline.
Reviewed changes
I reviewed the full diff at 55926fe: the 11 features.* module switches, the merged holiday presets, the migration sections, views and tests.
App\Support\Featuresregistry:DEFAULTSis used for upgrades and as the code fallback, andFRESHholds the seed values. Catalogue mode turns off the four circulation-side modules throughenabled(), whilestored()returns the saved switch for the settings forms.ConfigStoremaps only the known keys.tests/feature-toggles-registry.unit.phppasses locally (68/68).- Route gating:
FeatureGateMiddlewaresits first on every module route inweb.phpand on the z39 copy-cataloguing routes. The controllers check again withFeatures::requireOrRedirect().tests/feature-toggles-routes.unit.phppasses locally (25/25). - Service-level switches:
CirculationPolicy::rulesActive()andPatronLedger::active()replace the oldschemaReady()checks, including the SQL twins (lendableCopySql,debtBlockSql,debtThresholdSql).LibraryCalendar::fromDatabase()returns an empty calendar when its module is off. Supplier and RDA writes, plus the RDA export overrides, follow their switches. In maintenance, the attendance purge and the holiday extension are skipped. - Settings UI: a new admin-only Modules tab. The three Loans switches are saved inside the existing
mayConfigure()block. Turning the calendar on or off runsextendPickupDeadlines()again. - Holiday presets:
HolidayRules,HolidayCalculatorandHolidayPresetsstore the selection incalendar.holiday_*and tag rows withregola_festivita. Apply is idempotent, legacy Italian rows are adopted, and maintenance runsensureYears(). A newattivaflag lets manual yearly closures be switched on and off.tests/holiday-calculator.unit.phppasses (145). I spot-checked US, UK, SE, NL, AR, AU-NSW and CA-ON dates for 2021–2028 (observed and substitute days, Midsommar, Argentine movable holidays) and they look correct. - Migration: the guarded
regola_festivita/attivacolumns and insert-if-missingfeatures.*rows, using data probes for upgrades from RCs.
claude-opus-5-5 ｜ 𝕏
|
Follow-up to the review: two more places where
|
|
Second follow-up: holiday presets. I checked these against
|
… once they are back on The legacy backfill marker is re-armed when the penalty is written with the module off, so the next maintenance run after fines are switched back on mirrors it, even in a library whose backfill was already recorded as done.
|
CodeRabbit skipped this PR (118 files, the limit is 100), so I reviewed the merged diff myself. I covered route gates, every channel per module, settings saves, migration defaults, the holiday rule table and the apply/ensureYears paths.
Holiday dates I checked with no errors: Easter offsets, nth and last weekdays, Buß- und Bettag, Jeûne genevois, Midsommar, Victoria Day, the UK/US/AU weekend substitutes for 2021–2022, the Argentine movable holidays and the from/until years. One gap, not an error: the UK one-off extra bank holidays (2002, 2012, 2022, 2023) and the German national Reformationstag 2017 are not in the table, because overrides can move a holiday but not add a day. These are past years, so no future due date is affected. |
There was a problem hiding this comment.
✅ No new issues found. The ledger gap from the previous review is fixed.
Reviewed changes
I reviewed the one commit added since 55926fe. It fixes the loss penalties that were lost while fines were off.
- Re-armed the legacy backfill marker: when fines are off and
prestiti.sanzione > 0,PatronLedger::chargeLossPenalty()now setsloans.legacy_penalties_backfilledback to'0'.backfillLegacyPenalties()already returnsnullwhile fines are off, so the marker stays'0'until the first maintenance run with fines on. That run mirrors the penalty under itspenalty:<loan>key and marks the scan done again. There is no window where the scan is recorded as done before the penalty is mirrored. - Added test case C2:
tests/feature-toggles-services.unit.phpcovers the whole sequence (marker already'1', penalty with fines off, fines on, maintenance writes exactly one ledger row). It also saves and restores the marker and cleans up the test loan.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found in the two new commits. The user import now respects the patron rules switch.
Reviewed changes
I reviewed the two commits added since a2f57c9 (cac20f9, 3960c19).
- Evaluated selected holidays on demand:
LibraryCalendarnow receives the saved rules fromHolidayPresets::calendarRules(), andisOpen()computes their dates for whatever year is asked, including an observed New Year that falls on 31 December. Once a selection is saved,loadClosures()skips rows withregola_festivitaset, so stale or missing preset rows no longer affect due dates or fines. Every query method still goes throughisOpen(), andclosedThrough()/closedFrom()only look at one-off closures, so the long-closure jumps don't skip a holiday check. Before a selection exists (null), every row is read as before. - Serialized apply and upkeep:
apply()andensureYears()first take the same row lock (lockSelection(), a no-op upsert oncalendar.holiday_rules). Upkeep then reads the selection withFOR UPDATE, and the row lookups use locking reads. A cron run inside an olderREPEATABLE READsnapshot therefore can't bring back an unticked country's rows or duplicate the new ones. Because both paths take the locks in the same order, they can't deadlock each other. Both also callLibraryCalendar::reset()afterwards. - Gated
PatronTypeImport::available()oncirculation_rules: the check runs before the schema cache, so a running import stops assigning types as soon as the module is switched off. The CSV mapping dropstipologiaas well. This fixes the user import gap from the first review. - Added coverage: unit tests for dynamic holidays in
library-calendar.unit.php, for concurrent apply/upkeep, lock contention and rollback inholiday-presets-apply.unit.php, and for the importer switch infeature-toggles-services.unit.php. There is also an E2E test for a holiday beyond the preview years.
Not changed in these commits, and still open from the earlier follow-ups: weekend-substitute holidays close only the substitute day, not the actual date (this now applies to the dynamic evaluation too), and copie_disponibili is not recalculated when circulation_rules is switched.
claude-opus-5-5 ｜ 𝕏
|
Both modules are off on an upgraded install, so the 0.9.0 write checks ran against modules that refuse to write. The test now switches them on for those checks and also asserts the opposite: with the modules off a save keeps the stored values and an import batch assigns no supplier.
The explicit section-to-test map stopped at the fines ledger, so the three sections this branch adds to migrate_0.9.0-rc.1.sql failed the runner as untested. Each now names its own behavioural test.
There was a problem hiding this comment.
ℹ️ No functional issues. One merge leftover is noted inline: a docblock is now in the wrong place.
Reviewed changes
I reviewed the two commits added since 23f23f2 (6112c06b, dc22152c) and how the conflicts were resolved in the three merges of test/0.9.0-coverage (c02a317f, f1c34f56, 83c8659d). The base branch's own changes are outside this PR's scope.
- Switched the modules on in the cataloguing extras test:
tests/cataloguing-extras-090.unit.phpturns onsuppliersandrda_typesfor its write checks. It also checks the reverse: with both off, a save keeps the stored values andassignSupplier()returnsfalse. - Mapped the PR's migration sections:
tests/migration-0.9.0-rc.1.unit.phpnow mapsholiday presets,custom holidaysandfeature togglesto their own tests. Without this the runner failed those sections as untested. The section check passes locally (the sub-tests need a database). - Combined the base's locked calendar writes with the PR's paths: the base's
commitChange()(named lock, one transaction, deadline move inside it) wraps the plain closure writes. The preset, yearly-switch and custom-holiday actions keepcalendarChanged(). The PR'sisPresetRow()guard is still on update and delete. - Kept the feature gates on the base's controller changes: the
finesgate runs before the base'smayActOn()inPatronLedgerController. Thestocktakegate comes before the base's action validation inInventoryController::close(). Thesuppliersgate comes beforeschemaMissing()inSuppliersController, andcreateFormnow takes$db, with the route still gated. InCataloguingExtras, theisEmptyChoice()checks sit behind the module gates, andassignSupplier(): boolreturnsfalsewhilesuppliersis off.PatronLedger::active()survives the base's switch to aWeakMapschema cache.php -lis clean,library-calendar.unit.phppasses (170/170) and so doesfeature-toggles-routes.unit.php(25/25).
claude-opus-5-5 ｜ 𝕏
calendarChanged() had been placed between commitChange()'s docblock and the method, so neither docblock attached to commitChange() and its locking contract and callable type were lost to PHPStan.
The whole-year guard from the closures branch now also covers the two calendar writes this branch adds: applying a holiday preset and switching stored yearly closures back on both run through commitChange() and roll back, holiday selection included, when they would close every day of a year. The guard counts only active closures, and the user-import cancel route keeps the user_import feature gate.
…hind custom-holidays: a calendar read from a lost connection now raises (loadClosures() never reads a failure as "no closures"), so the test asserts the exception instead of an open calendar. library-calendar-loans: the refused preset import restores the saved holiday selection and its rows even when the check fails, so a failing run cannot leak Ferragosto into the next suite.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run covered one PR commit and four merges of test/0.9.0-coverage since 5cd3a38. Only 2ef276b3 needed conflict resolutions.
- Routed holiday writes through
commitChange():importPresetandsaveYearlynow run under the calendar lock and transaction. They returnyearCoverageError()when a preset adds or adopts rows, or when a yearly closure is switched back on. A refused change rolls back everything, including the saved holiday selection. Nesting is safe:HolidayPresets::applygoes throughDbAtomic::run, which detects the outer transaction and leaves begin, commit and rollback to it. - Limited the whole-year guard to active closures:
yearCoverageError()now only counts rows withattiva = 1(when that column exists) and includes the weekly closed days. A switched-off yearly closure therefore no longer counts toward the guard. - Kept the PR filters in the base's stricter
loadClosures(): theattivaandregola_festivita IS NULLselection filters still apply. A failed read now raisesRuntimeException; before, it was treated as "no closures". - Updated calendar tests:
custom-holidays.unit.phpnow expects the exception on a killed connection.library-calendar-loans.unit.phpchecks that a refused preset import or yearly switch leaves the selection unchanged, and puts the selection back even when that check fails. - Gated the new user-import cancel route:
POST /admin/users/import/cancelfrom the base carriesFeatureGateMiddleware('user_import'), like the other import routes.
Local checks: library-calendar.unit.php 180/180, feature-toggles-routes.unit.php 25/25, php -l clean on the touched files.
claude-opus-5-5 ｜ 𝕏
When the stored holiday_country is not a country the calculator knows any more (a hand edit, a country dropped from the data), the selection reads as unsaved and the holiday card silently showed the default country as if nothing had ever been chosen, while the chosen holidays stopped being kept up to date. The selection now reports the stored code as unknown_country, the controller passes it to the view, and the card shows a translated warning naming it. What is applied to loans is unchanged.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run covered one PR commit and one merge of test/0.9.0-coverage since f8ccfb0. For the merge, I checked only the conflict resolutions.
- Warned about an unknown saved holiday country:
HolidayPresets::readSelection()now returnsunknown_countrywhen a storedholiday_countryis no longer a known country. The selection still counts as unsaved, andcalendarRules()still returnsnull, so loans keep reading the stored closure rows. The calendar card shows an escaped warning that names the code, and the string is translated in all five locales. A new case inholiday-presets-apply.unit.phpcovers it and restores the stored selection infinally. - Resolved the base merge correctly:
PatronLedger::schemaReady()now caches only a positive probe (from the base), and the PR'sactive()is kept. MobileGET /me/movementskeeps thedebt_blockthreshold gate and adds the base's pagingmetaandnext_cursor.debt-block.spec.jskeeps the.envsafeLoad()and adds the base's database-name guard. The fines-ledger migration test keeps the PR'sfeatures.fines/loss_penaltiesseed, plus the base's check that runs without a cache reset. - Checked the base's new upgrade backfill against the switches: the base adds
PatronLedger::completeLegacyPenaltyBackfill(), which runs after the migrations in the Updater and also from maintenance. It goes throughbackfillLegacyPenalties(), which still does nothing unless the ledger is active andloss_penaltiesis on.
php -l is clean on the PHP files that were touched.
claude-opus-5-5 ｜ 𝕏
With loss penalties off the backfill never completes and chargeLossPenalty() re-arms the marker, so debtBalanceCents() and debtBalanceSql() kept adding every unconverted prestiti.sanzione: patrons were refused loans for amounts the library chose not to mirror, while the balance shown to staff and patrons stayed under the threshold. legacyPenaltiesPending() now answers false when loss penalties are off, so the debt block reads the ledger alone in PHP and SQL.
There was a problem hiding this comment.
Important
The base merge adds a debt-block path that ignores the PR's loss_penalties switch. With loss mirroring off, every loss or damage amount counts toward the debt block permanently. See the inline comment.
Reviewed changes
I reviewed the merge of test/0.9.0-coverage (4eb5f51c, 43 base commits) since 6f6cdc0. For the merge I checked the conflict resolutions and how the base's changes interact with the PR's switches. The base branch's own work is outside this PR's scope.
- Combined the debt-block probe with the fines switch: the new
PatronLedger::activeOrFail()isFeatures::enabled('fines') && schemaReadyOrFail().CirculationPolicy::debtBlockError()andLoanEligibility::debtBlockSql()use it together withdebt_block, so a failed probe still throws instead of admitting the patron. - Kept the
attivafilter in the base's reworked whole-year guard:yearCoverageError()now carries state from one ICS event to the next. The first read, in the$state === nullbranch, still counts only yearly closures withattiva = 1. - Adjusted calendar tests to the base:
custom-holidays.unit.phpaccepts either "closed weekdays" or "closures not readable", because the weekdays are now read first. Inlibrary-calendar-loans.unit.php, preset cleanup now matchesregola_festivita IS NOT NULLrather than the holiday names.
php -l is clean on the touched PHP files and the locale JSON files parse.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The debt-block gap from the previous review is fixed, and no Pullfrog findings are still open.
Reviewed changes
I reviewed the one commit added since 4eb5f51 (4aef5ad).
- Stopped unconverted legacy penalties from blocking while loss mirroring is off:
PatronLedger::legacyPenaltiesPending()now returnsfalsewhenloss_penaltiesis not effectively enabled. Its only callers aredebtBalanceSql()anddebtBalanceCents(), so the debt block in bothCirculationPolicy::debtBlockError()andLoanEligibility::debtBlockSql()reads the ledger alone, matching the balance staff and patrons see. Withloss_penaltieson and the marker unset, unconvertedprestiti.sanzioneamounts still count until maintenance converts them, as the base intends. - Added coverage:
tests/patron-ledger.unit.phpchecks that withloss_penaltiesoverridden off and the marker unset, an 8.00 penalty over a 5.00 threshold blocks nobody in PHP or SQL. With the marker set, the block reads the ledger only.php -lis clean on both files.
claude-opus-5-5 ｜ 𝕏
|
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. |
This makes the 0.9.0 modules optional while preserving each library’s existing behaviour during upgrades. Printing cards no longer requires photos, consultation restrictions no longer require patron-type rules, and opening the ledger does not silently enable overdue fines or debt blocking.
Settings and behaviour
scripts/generate-holiday-dataset.py), with the own-language and English names, the category (government, bank, school, a single community — offered unticked) and lunar dates marked as estimated. A weekend holiday with a day in lieu closes both days; moved holidays keep only the new date. Manual and ICS closures are preserved; selected rules are evaluated for the requested year; preset writes and maintenance serialize selection changes. The preview year is a number field (1900–2199).festivita_personalizzatetable, evaluated for any year, editable, suspendable and deletable.Upgrade guarantees
The migration only inserts missing preferences. Explicit OFF values, zeroes, inherited rules, unknown settings and existing data survive repeated upgrades. New granular preferences inherit their former parent only when absent; old forms freeze those preferences before changing the parent.
A library whose ledger was off keeps automatic fines, debt blocking and loss mirroring off, even with positive rates or assessed penalties already stored. Subsequently enabling only the ledger leaves those automations off. Existing amounts are preserved; disabled ledger/loss controls prevent the migration from posting legacy penalties. Retention while collection is suspended remains opt-in. Existing loan due dates are preserved. No holiday selection and no custom holiday exist after an upgrade, so due dates stay those of a library open every day until the administrator chooses otherwise.
Validation
All ten CI workflows passed for
0471c66f, the head that includes the review follow-ups, the holidays of every country and the library's own holidays: Code Quality, the four-database matrix, E2E (240 circulation + 258 catalogue tests and the general suite), migrations, both upgrade gates, deep regression (four browser shards and five localized installers), three-browser/accessibility/DAST checks, supply-chain security and CodeQL. Locally all 327 standalone unit tests pass. Exact run links and earlier tested commits are in the review report.