Building on bug 41918, it would be good to limit the number of reports
a given user can run at a time. This control is config-file based as
it is tied to the server resources provided by the administrator.
Adds a new koha-conf.xml option total_running_reports_per_user_limit
(0 = disabled). When the configured limit is reached, the user is told
"You have too many reports running at the moment. Please wait for some
of them to finish before running this report again."
Note on implementation: the original patch on bz (attachment 198034)
performed a second pre-flight Koha::Reports->running call in
reports/guided_reports.pl. The bug 41918 QA follow-ups already pushed
the duplicate-running guard into Koha::Report::prep_report so it would
fire across the staff UI, svc/report and opac/svc/report. This patch
extends that same model-layer architecture rather than reintroducing
controller-layer enforcement:
- New Koha::Exceptions::Report::TotalRunning subclass of
Koha::Exceptions::Report.
- prep_report now performs a second check after the duplicate one and
throws TotalRunning when the user has total_running_reports_per_user_limit
reports of any kind already in flight.
- All three callers (guided_reports.pl, svc/report, opac/svc/report)
catch the new exception alongside DuplicateRunning, mapping it to a
new template error key total_running_reports_limit_exceeded for the
staff UI and a JSON {error: total_running_reports_limit_exceeded}
with HTTP 429 for the JSON endpoints.
Test Plan:
1) Set total_running_reports_per_user_limit to 3 in your koha-conf.xml.
2) Create 3 long-running reports like:
SELECT COUNT(*) FROM items i1 JOIN items i2 JOIN items i3 JOIN items i4 JOIN items i5 JOIN items i6
3) Run each report in turn by opening a new tab, browsing to the
reports module, and choosing to run that report.
4) Note the first three continue to run, but a fourth attempt gives
the new "too many reports running" alert in the staff UI.
5) Repeat against /cgi-bin/koha/svc/report?id=N and confirm an HTTP
429 response with JSON {error: total_running_reports_limit_exceeded}.
6) Set total_running_reports_per_user_limit back to 0 and confirm
normal behaviour resumes.
7) prove t/db_dependent/Koha/Reports.t -> 13 tests pass, including a
new subtest for TotalRunning.
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Koha::Reports->running queries information_schema.processlist. Without
the PROCESS privilege MariaDB/MySQL already restrict the view to the
caller's own threads, so the previous code worked correctly on stock
deployments. However, if a site has granted broader privileges (eg. a
shared MariaDB server where the Koha DB user happens to also have
PROCESS, or a custom role) the query would happily count threads
belonging to other database users - another Koha instance, a reporting
tool, or any unrelated application sharing the server.
Add an explicit
user = SUBSTRING_INDEX(CURRENT_USER(), '@', 1)
clause so the query is always scoped to the connection's MySQL user,
regardless of what privileges that user has been granted. The
processlist.user column holds just the username with no host, so we
strip the '@host' off CURRENT_USER() before comparing.
The new clause has no bind parameters - it is a constant per
connection - so the existing parameterised-bind assertions in the
running subtest still hold. A small additional assertion verifies the
clause is present in the SQL text.
Test plan:
1) prove t/db_dependent/Koha/Reports.t (now 12 tests, all pass).
2) Optional: in a multi-tenant MariaDB, grant PROCESS to the Koha DB
user, start a long query as a different DB user with the marker
/* { saved_sql.id: 1 } { user_id: 1 } */ in its info, then call
Koha::Reports->running. Confirm the foreign thread is NOT counted.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The previous patches enforced duplicate_running_reports_per_user_limit
only in reports/guided_reports.pl. Two other code paths run saved SQL
reports for an authenticated user and bypassed the guard entirely:
- svc/report (staff JSON endpoint, eg. used for chart widgets and any
caller hitting /cgi-bin/koha/svc/report?id=N directly)
- opac/svc/report (OPAC JSON endpoint)
A user who hit their staff-UI limit could still spawn additional copies
of the same long-running report via either svc endpoint, defeating the
guard.
Push the check into Koha::Report::prep_report itself, which is the one
function all three callers go through. When the limit is reached
prep_report now throws a new Koha::Exceptions::Report::DuplicateRunning
(carrying report_id, user_id and the configured limit). The guided
reports CGI catches it and re-renders the existing "report is already
running" alert; both svc/report endpoints catch it and return HTTP 429
Too Many Requests with a JSON { error: "duplicate_running_report" }
payload.
Notes:
- The pre-flight Koha::Reports->running call is removed from the
guided-reports CGI; the model now owns that policy and the
controller is just rendering. The TOCTOU window remains identical
in size and is the same intentional trade-off as before.
- Limit=0 (the default) and absent userenv both bypass the check, so
CLI cronjobs (misc/cronjobs/runreport.pl) and direct DB users are
unaffected.
Test plan:
1) prove t/db_dependent/Koha/Reports.t - now includes a new subtest
covering limit=0 (lives), under limit (lives) and at limit (throws
Koha::Exceptions::Report::DuplicateRunning).
2) Set duplicate_running_reports_per_user_limit=1 in koha-conf.xml,
restart_all.
3) Open a long-running report in two staff tabs - second tab shows
the "report is already running" alert (unchanged behaviour).
4) While one run is in flight, hit /cgi-bin/koha/svc/report?id=N as
the same user - now returns HTTP 429 with the JSON error payload
(previously: ran a second copy of the query).
5) Same check on /cgi-bin/koha/opac/svc/report?id=N.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Adds a 'running' subtest covering the new method:
- Returns a Koha::Reports resultset (object identity, not opaque ids).
- Returns an empty resultset when nothing is in flight, and when the
user_id / report_id filter does not match any process.
- Parses saved_sql.id markers out of synthetic processlist rows and
resolves them to real Koha::Report objects, ignoring rows without
the marker.
- Sends parameterised LIKE binds for user_id (proves the WHERE clause
is built without string interpolation).
- Falls back to an empty resultset and does not propagate the
exception when the underlying selectall_arrayref call dies, eg.
when the database user lacks PROCESS privilege.
To make the failure path mockable without trying to mock DBI::db
(which is not loadable as a separate file), the dbh selectall is
extracted into a tiny Koha::Reports::_processlist_rows helper so
Test::MockModule can replace it cleanly. The helper is otherwise
behaviour-preserving.
Test plan:
1) prove t/db_dependent/Koha/Reports.t -> 11 tests, all pass.
2) Existing prep_report subtest still passes.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The first patch in this series changed the SQL comment appended by
Koha::Report::prep_report from
/* saved_sql.id: <id> */
to
/* { saved_sql.id: <id> } { user_id: <user_id> } */
but did not update the existing assertions in t/db_dependent/Koha/Reports.t,
leaving three subtest failures in the prep_report subtest. Refresh the
expected strings to match the new format and pick up the userenv-derived
user_id (or 0 when no userenv is in scope, matching prep_report's fallback).
Test plan:
1) Apply this follow-up.
2) prove t/db_dependent/Koha/Reports.t
3) Verify all 10 tests pass (previously 3 subtests of prep_report failed).
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
1. The DELETE in afterEach was not needed
2. The test was failing randomly on the UPDATE of the beforeEach
`Duplicate entry '+42+' for key 'itembarcodeidx'`
I don't explain it, but this patch should fix the failure.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Test plan (ktd)
0) Update the database (ktd, ignore if sandbox):
$ updatedatabase
1) Enable 'RealTimeHoldsQueue' and 'DisplayAddHoldGroups' system preferences
2) restart_all # This is required or the worker won't pick up the new sys pref values
3) Place at least 2 holds for a patron on different items/biblios, without grouping them:
<staff_url>/cgi-bin/koha/members/moremember.pl?borrowernumber=19
4) Build the holds queue once to get a baseline:
$ perl misc/cronjobs/holds/build_holds_queue.pl --force
5) Visit the holds queue and confirm the holds appear without a group indicator:
<staff_url>/cgi-bin/koha/circ/view_holdsqueue.pl?branchlimit=&itemtypeslimit=&run_report=1
6) Create a hold group for those holds via the patron page
<staff_url>/cgi-bin/koha/circ/circulation.pl?borrowernumber=<patron_borrowernumber>
7) Without running build_holds_queue.pl, refresh the holds queue, confirm the 'part of a hold group' indicator now appears
8) Ungroup the holds, refresh the holds queue, confirm the 'part of a hold group' is no longer showing
9) Run the unit tests:
$ prove t/db_dependent/Reserves/HoldGroup.t
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The 'ModBiblio on invalid record' subtest asserted the action log info
column contained 'biblionumber', but Koha::Biblio::_unblessed_for_log
restricts the logged hash to frameworkcode, opac_suppressed, and serial.
The biblionumber lives in action_logs.object, not in the info JSON.
Update the regex to match the new "biblio { ... }" prefix format set by
C4::Log::logaction so the assertion confirms biblio data is logged
without depending on a field that is intentionally excluded.
Test plan:
1. prove t/db_dependent/Biblio.t
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Build on the new ActionLog DBIC relations to expose them as opt-in
embeds, and shore up gaps in the test coverage that QA flagged.
Swagger
* paths/action_logs.yaml declares an x-koha-embed header parameter
constrained to the supported relations (librarian, patron).
* definitions/action_log.yaml adds the corresponding optional
`librarian` and `patron` object fields.
Tests
* Convert the single 24-test subtest into focused subtests:
- authorization (403 vs 200)
- simple typed filters
- q-DSL: multi-value module, info LIKE, combined filter
- embeds: librarian and patron, with and without x-koha-embed
- pagination + X-Total-Count and order_by
* Mock AuthSuccessLog/AuthFailureLog/BorrowersLog to 0 to remove the
flakiness where the API GET itself appended an AUTH log row between
delete and assertion.
* Build the test librarian with both `tools` and `borrowers` flags so
embedded patron fields are not redacted by Koha::Patron->is_accessible.
* Use a small build_fixtures() helper to avoid copy-pasting setup
across subtests.
Test plan:
1. yarn build && restart_all (refresh swagger bundle)
2. prove t/db_dependent/api/v1/action_logs.t (all subtests green)
3. curl -H 'x-koha-embed: librarian,patron' \
/api/v1/action_logs?module=MEMBERS -- patron and librarian
objects appear in the response.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Authority MODIFY action log diffs were emitting bogus entries for every
field with a default ' ' indicator on every save, producing output like:
_marc.fields.1.040.ind1 ' ' ' '
_marc.fields.1.040.ind2 ' ' ' '
_marc.fields.2.150.ind1 ' ' ' '
_marc.fields.3.667.ind1 ' ' ' '
...
even when only a single subfield was edited.
Root cause: Struct::Diff treats a utf8-flagged ' ' as different from a
non-utf8 ' ' SV, even though Perl's eq returns true. ModAuthority was
sourcing the two sides of the diff from different parse paths:
- $oldrecord = GetAuthority($authid)
-> MARC::Record->new_from_xml(...) -> indicators arrive utf8-flagged
- $record (the "after" side) was the in-memory MARC built by the
editor from CGI parameters -> indicators are bare byte strings
The XML round-trip through AddAuthority normalises the stored copy, but
the in-memory $record we passed to logaction had not been re-parsed, so
every default indicator looked like a change to Struct::Diff.
Fix: after AddAuthority commits the new state, re-read it via
$new_authority->record and pass that to _authority_log_data. Both sides
of the diff now traverse the same new_from_xml path, utf8 flags match,
and Struct::Diff falls silent on unchanged indicators.
This is the symmetric counterpart of how biblios already work in
C4::Biblio: ModBiblio sources both $original_marc and $updated_marc_log
through Koha::Biblio::Metadata::record_strip_nonxml (which also calls
new_from_xml), so biblios don't hit this.
The deeper asymmetry between in-memory editor MARC and stored MARC
(default indicators only get normalised on the XML round-trip) is left
for a separate bug; this patch keeps the action log diff clean without
touching the storage path.
Test plan:
1. prove t/db_dependent/AuthoritiesMarc.t
2. Enable AuthoritiesLog. Edit any authority via the staff UI making a
single subfield change. Open Tools -> Log viewer for that authority
and verify the diff column lists only the changed subfield (plus the
005 timestamp), with no rows for ind1/ind2 of untouched fields.
3. Re-run the same edit a second time changing nothing else; the diff
should again be limited to 005 + the touched subfield.
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Mirrors bug 42188's biblio info-column convention. Generalise the
CATALOGUING-only info-column rewrite branch in C4::Log::logaction to
cover AUTHORITIES as well, using a module-aware prefix map so each
module gets its own prefix ("biblio" / "authority"). For AUTHORITIES
this means MODIFY now stores "authority {pre-change JSON}" and DELETE
stores "authority {final-state JSON}", while ADD stays bare
"authority" (the call site passes the string directly and ADD is
guarded anyway). The biblio behaviour is preserved bit-for-bit.
Test plan:
1. prove t/db_dependent/AuthoritiesMarc.t
2. Enable AuthoritiesLog. Create / modify / delete an authority via
the staff UI, then check action_logs:
- ADD row's info column reads just "authority"
- MODIFY row's info column reads "authority { ... pre-change JSON ... }"
- DELETE row's info column reads "authority { ... final-state JSON ... }"
3. prove t/db_dependent/Koha/Biblio.t
4. prove t/db_dependent/Log.t
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Mirrors the bug 42188 main commit for biblios. Replace the custom
_marc_record_to_diffable helper with MARC::Record::MiJ's to_mij_structure
so AUTHORITIES action log diffs use the same MARC-in-JSON shape as
CATALOGUING diffs. Add Koha::Authority::_unblessed_for_log to expose
only the auth_header columns that exist independently of the MARC
record (authtypecode, origincode, linkid), avoiding duplication of
MARC-derived data (heading, authtrees), raw marcxml, and system
timestamps. The explicit whitelist also removes the need for the
post-hoc empty-field stripping added in a previous follow-up.
Test plan:
1. prove t/db_dependent/AuthoritiesMarc.t
2. Enable AuthoritiesLog. Create, edit, and delete an authority via
the staff UI, then inspect action_logs.diff in the DB:
- _marc contains MiJ shape { leader: "...", fields: [ { tag: {...} }, ... ] }
- Changes to heading fields produce diffs on
_marc.fields[N].TAG.subfields[0].a
- The diff no longer carries a redundant top-level heading entry
(it's MARC-derived and now only appears in _marc).
3. prove t/db_dependent/Log.t
4. prove t/db_dependent/Koha/Authority.t
5. prove t/db_dependent/Koha/Biblio.t
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The AUTHORITIES action log entries for authority operations previously
stored minimal data: ADD and DELETE logged just the string "authority",
while MODIFY logged a raw MARC-formatted dump of the pre-change record
as a plain text string ("authority BEFORE=>..."). None of these populated
the diff column.
This patch updates AddAuthority, ModAuthority and DelAuthority in
C4/AuthoritiesMarc.pm to follow the same pattern introduced for biblios
in bug 40135: passing combined hashrefs to logaction so that the diff
column is automatically populated with Struct::Diff JSON.
The combined hashref contains the auth_header table columns (authtypecode,
heading, origincode, etc. — excluding the raw marcxml blob) plus a _marc
key built by the new _marc_record_to_diffable helper, which converts a
MARC::Record into a tag => array-of-formatted-strings structure readable
by the existing diff viewer.
A new _authority_log_data helper packages the authority unblessed data
and MARC record into the combined hashref, keeping the three call sites
concise.
For ADD, the logaction is now emitted after the authority record has been
fully stored (after _after_authority_action_hooks), so the diff captures
the complete saved state. For MODIFY, the before-state is captured from
Koha::Authorities before calling AddAuthority, and the after-state is
read back once AddAuthority has committed the new data. For DELETE, the
MARC is captured before the authority is removed.
The diff column for MODIFY will now show both authority table column
changes (e.g. heading) and MARC field-level changes (e.g. _marc.151.0)
in the existing diff viewer table in the log viewer.
Tests are added to verify that ADD, MODIFY, and DELETE all populate the
diff column and that _marc content is captured correctly.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The info column for CATALOGUING action logs is, by convention, the
record state before modification -- MODIFY writes the pre-change JSON
payload, DELETE writes the state at time of deletion, but ADD has no
pre-change state. Writing the new record state into info on ADD
duplicates the state that the subsequent MODIFY (if any) captures as
its "before" snapshot, and conflicts with that convention.
Skip the JSON payload on ADD so info degrades to the bare "biblio"
prefix. The diff column already captures the full creation (every
field "added" against an empty hashref), so no information is lost.
Test plan:
1. prove t/db_dependent/Koha/Biblio.t
2. Observe that the "CataloguingLog MARC-in-JSON diff tests" subtest
now also asserts:
- ADD info == "biblio"
- MODIFY info matches /^biblio \{/
- DELETE info matches /^biblio \{/
3. Create / modify / delete a record via the staff UI with
CataloguingLog on, then check action_logs: the ADD row's info
column should read just "biblio".
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Update the existing _unblessed_for_log() subtest to reflect the new
behaviour: MARC-derived columns (title, author, copyrightdate, etc.)
are now excluded, while frameworkcode is always included even when it
is an empty string (the Default framework).
Add a new subtest 'CataloguingLog MARC-in-JSON diff tests' covering:
- ADD: diff _marc key is MARC-in-JSON (has leader + fields array),
indicators and subfield values are correct, and MARC-derived
biblio columns are absent from the diff.
- MODIFY: diff _marc key present, MARC-derived columns absent.
- DELETE: diff _marc key present with correct MARC-in-JSON structure.
Use from_json() instead of decode_json() when reading diffs back from
the database, as the DBD layer returns decoded Perl Unicode strings
which decode_json (which expects UTF-8 bytes) would reject for any
non-ASCII content.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch makes sure we inject the __sort form of local-number, rather
than the field directly - this ensurs searching works even if mappings mayu
be different and local-number itself is not numeric
To test:
1 - Edit mappings.yaml and set local-number from 'number' to 'string'
NOTE: There are two copies of local-number - one in auth and one in biblio - the latter is the important one here
2 - perl misc/search_tools/rebuild_elasticsearch.pl -r -v
3 - Confirm in staff interface Admin -> Search Engine Configuration that local-number is now a string
4 - Search and get an error
Error: Unable to perform your search. Please try again.
In the logs:
[illegal_argument_exception] Text fields are not optimised for operations that require per-document field data like aggregations and sorting, so these operations are disabled by default. Please use a keyword field instead. Alternatively, set fielddata=true on [local-number] in order to load field data by uninverting the inverted index. Note that this can use significant memory.
5 - Apply patch, restart all
6 - Search again
7 - Success!
Signed-off-by: Bernard Scaife <bernard.scaife@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Thomas Klausner <domm@plix.at>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Previously, t/AuthoritiesMarc_MARC21.t would fail if
you tried to run it from within a UNIMARC instance.
This patch fixes that.
Test plan:
1) Inside a UNIMARC instance, run the test and
notice the subtest failure:
$ prove -v t/AuthoritiesMarc_MARC21.t
2) Apply this patch.
3) Re-run the test and notice that it now completes
without any failures.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The cash register transaction history page (pos/register.pl) allows
access with either cashup OR anonymous_refund permission, but the
API endpoints for fetching cashups only checked for cashup permission.
This caused a 403 error when users with only anonymous_refund permission
tried to view the transaction history page, as the cashups table failed
to load.
This patch updates the API permissions for:
- GET /cash_registers/{id}/cashups
- GET /cashups/{id}
to accept either cashup or anonymous_refund permission, matching the
page access logic.
Test plan:
1. Create a staff user with only cash_management > anonymous_refund
permission (not cashup)
2. Navigate to Point of Sale > Transaction history for any cash register
3. Verify the cashups table loads successfully (previously returned 403)
4. Run: prove t/db_dependent/api/v1/cashups.t
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Rename /libraries/{id}/holidays to /libraries/{id}/closed_dates
for consistency with existing sub-resource naming (desks,
cash_registers).
Efficiency: instead of calling $calendar->is_holiday() per day
(which re-checks all three data sources each time), access the
pre-loaded data structures directly: weekly_closed_days array,
day_month_closed_days hash, and _holidays hash. The logic properly
handles exceptions (dates marked open despite a weekly/annual
closure).
Additional fixes:
- Add 365-day max range guard
- Add missing 401/403 responses in swagger spec
- Add request_id_header parameter
- Test exception handling (open on normally-closed day)
- Remove fragile 'empty defaults' assertion
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
- Add list_holidays controller to Koha::REST::V1::Libraries
- Returns closed days for a library within a configurable date range
- Uses Koha::Calendar to determine holidays
- Defaults to today + 3 months if no range specified
- Add OpenAPI spec for the new endpoint
- Register route in swagger.yaml
- Add unit tests in t/db_dependent/api/v1/libraries.t
To test:
1. Apply patch
2. prove t/db_dependent/api/v1/libraries.t
3. Verify all tests pass, including the new list_holidays subtest
which covers:
- Holidays returned within a specified date range
- Empty list when no holidays exist in range
- 400 response when 'to' date precedes 'from' date
- 404 response for non-existent library
- Default range (no params) returns successfully
Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Update suggestion action logging to populate the diff column using the
correct before/after semantics:
- CREATE: empty before-state ({}), new suggestion data as after-state
- MODIFY: pre-change unblessed data as before, post-store object as after
- DELETE: log before the SQL DELETE so the object can still be fetched;
pass the suggestion as both $infos (info column) and $original (diff
before-state), logaction sets after-state to {}
Also add tests verifying the diff column is populated for all three actions.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The OPAC REST self-checkout path (guarded by OpacTrustedCheckout) is
functionally SCO-over-API, so it should honour the same hold policy
as the SCO web interface. Previously _check_availability unconditionally
promoted RESERVED, RESERVE_WAITING, TRANSFERRED and PROCESSING from
the confirmation hash to the blockers hash for any public caller. This
meant the OPAC REST endpoint always refused held items regardless of
the librarian's AllowItemsOnHoldCheckoutSCO configuration.
With this patch the promotion is gated on AllowItemsOnHoldCheckoutSCO:
* 0 - Promote all four hold confirmations to blockers (previous
behaviour for pending holds)
* 1 - Promote only waiting/transit/processing; silently allow
checkout of items with a pending hold
* 2 - Promote none; silently allow checkout of items with any hold
Remaining hold confirmations are stripped from the response so the
caller does not have to resolve a confirmation token for a policy the
admin has already granted.
Test plan:
1. prove t/db_dependent/api/v1/checkouts.t
2. Enable OpacTrustedCheckout. POST an item with a pending hold for
another patron to /api/v1/public/patrons/X/checkouts:
- AllowItemsOnHoldCheckoutSCO=0: 403 Checkout not authorized
- AllowItemsOnHoldCheckoutSCO=1: 201 Created
3. Place a waiting hold for another patron on an item. Repeat:
- AllowItemsOnHoldCheckoutSCO=1: 403
- AllowItemsOnHoldCheckoutSCO=2: 201
Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch extends the SIP checkout handler to honour the new
three-valued AllowItemsOnHoldCheckoutSIP preference:
* 0 - Don't allow checkout of any held item
* 1 - Allow checkout when the hold is still pending (not attached)
* 2 - Also allow checkout when the hold is waiting, in transit or
being processed for another patron
Previously the preference was a simple YesNo toggle that, when
enabled, allowed the checkout regardless of whether the hold was
pending or already attached to an item on the hold shelf. Libraries
could not block the more disruptive case (checking out a waiting
hold destined for another patron) without also blocking pending
holds.
The existing post-confirmation filter is reworked into a single
branch per confirmation type that consults the preference value.
Test plan:
1. prove t/db_dependent/SIP/Transaction.t
2. With AllowItemsOnHoldCheckoutSIP=0, place a pending hold for
patron A on an item, attempt SIP checkout to patron B: refused.
3. Repeat with the hold marked waiting: refused.
4. Set AllowItemsOnHoldCheckoutSIP=1, pending hold: allowed;
waiting hold: refused.
5. Set AllowItemsOnHoldCheckoutSIP=2, waiting hold: allowed.
Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
to_api now returns the derived self_renewal_available field, so
reusing a to_api'd patron as the POST body for /api/v1/patrons fails
with 500 (the value has no matching DB column). Adjacent subtests
already delete this read-only field before POSTing; do the same in
the EmailPatronRegistrations subtest.
Test plan:
1. prove t/db_dependent/api/v1/patrons.t — all tests pass.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The "identify_updated_extended_attributes" subtest was added without
bumping the top-level plan, so prove reported a "Bad plan" failure
even though every subtest passed.
Test plan:
1. prove t/db_dependent/Koha/Patron.t — all tests pass, no plan
mismatch.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Extended attributes weren't being identified as changes and so need to be checked to see if any of them have changed
Sponsored-by: Westminster City Council <https://www.westminster.gov.uk/>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Adds categories to the db insertion mechanics and allows values to be passed to the patron insertion method
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Remove explicit "return undef" in favour of bare "return" to
satisfy Perl::Critic (PBP p.199).
Signed-off-by: Brian J. Barr <brian.barr@churchofjesuschrist.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Extends the existing `post() tests` subtest in
t/db_dependent/api/v1/authorities.t with coverage for the new
`x-koha-matchrule` header that selects a Koha record matching rule
(`C4::Matcher`, record_type=authority) for duplicate detection when
creating authorities via POST /api/v1/authorities.
The new cases exercise:
* non-integer id (rejected by the OpenAPI schema validator)
* unknown matcher id -> 400 `matching_rule_not_found`
* biblio matcher id -> 400 `invalid_matching_rule_type`
* valid authority matcher returning duplicates -> 409 with a
`matches` array of `{authid, score}` objects
* valid authority matcher returning no duplicates -> 201
* matcher combined with `x-koha-override: duplicate` -> 201
(override wins, matcher is skipped)
`C4::Matcher` is mocked deterministically to avoid relying on the
authority search engine in the test database.
These tests fail without the controller/spec changes introduced in
the follow-up commit.
Test plan:
1) Apply only this patch and run
`prove t/db_dependent/api/v1/authorities.t`.
=> Failures are expected in the `post() tests` subtest for the
`x-koha-matchrule` cases.
2) Apply the next patch in the series and re-run the tests.
=> All subtests pass.
Signed-off-by: Brian J. Barr <brian.barr@churchofjesuschrist.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit improves upon the original fix by making the intent more
explicit when handling empty string date parameters in AddRenewal and
AddIssue functions.
WHY THIS IS BETTER:
The original fix (changing `defined $datedue` to just `$datedue`)
works correctly but relies on Perl's implicit truthy/falsy semantics:
if ( $datedue && ref $datedue ne 'DateTime' )
This is subtle because:
- Empty string "" is falsy, so parsing is skipped ✓
- But it's not immediately obvious that empty strings are being
handled specially
- The code doesn't explicitly show its intent
THIS APPROACH:
if ( defined $datedue && $datedue ne '' && ref $datedue ne 'DateTime' )
Benefits:
1. EXPLICIT: Clearly states "only parse if defined AND not empty
AND not already a DateTime"
2. SELF-DOCUMENTING: Intent is obvious to future developers
3. NO RELIANCE on truthy/falsy semantics
4. CONSISTENT: Same pattern applied across all date parameter handling
COMPREHENSIVE CHANGES:
1. AddRenewal (C4/Circulation.pm:3449)
- Made datedue empty string check explicit
2. AddIssue (C4/Circulation.pm:1611)
- Made datedue empty string check explicit
- Also updated issuedate handling (line 1616) to explicitly handle
empty strings
3. Test Coverage (t/db_dependent/Circulation/issue.t)
- Added test for AddIssue with empty string datedue
- Added test for AddIssue with empty string issuedate
- Ensures both functions properly calculate dates from circulation
rules instead of using "now"
The existing test for AddRenewal already covered empty string datedue
(added in the original Bug 37966 commit). These new tests ensure the
same correct behavior for AddIssue.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Adds a new subtest to t/db_dependent/Koha/Patron.t covering
fixup_cardnumber and the autoMemberNumValue counter:
- Normal increment bumps the counter
- Renaming a patron's cardnumber does not free it for reuse
- Deleting a patron does not free their cardnumber for reuse
- Counter wins when ahead of db_max
- db_max wins when ahead of counter
- Legacy fallback when the syspref row is absent
Also adds autoMemberNumValue to the exceptions list in
t/db_dependent/check_sysprefs.t. The preference is managed internally
by Koha::Patron::fixup_cardnumber -- it is not user-configurable and
has no entry in the admin preference YAML files.
Test plan:
1. prove t/db_dependent/Koha/Patron.t
2. prove t/db_dependent/check_sysprefs.t
3. prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t
(confirm the patron_barcode_transform hook is still called twice
during store when autoMemberNum is enabled).
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
PA amended: Removed bug number from subtest
Signed-off-by: Charlie Arthur <carthur@slolibrary.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The safe_do wrapper adds indirection without value:
- In Patron::Modification::approve, every operation inside safe_do
goes through Object::store or Object::delete, which already call
translate_exception internally. The wrapper was translating
already-translated exceptions.
- In Item::add_to_bundle, the outer try/catch just called rethrow,
making the safe_do + try/catch nesting a no-op.
This follow-up removes safe_do and aligns both callers with the
pattern already used by Object::store and Object::delete: call
translate_exception directly in the catch block when raw DBIC
operations are involved.
Changes:
- Remove safe_do from Koha::Schema (and its Try::Tiny import)
- Flatten add_to_bundle to use translate_exception in its catch
- Flatten Patron::Modification::approve to rely on Object::store
- Remove safe_do test, adjust test count
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Replace 'no strict refs' usage with a proper TestObject package to avoid
Perl::Critic violations while maintaining the same test functionality.
The mock object now uses standard OOP patterns instead of symbolic
references for method creation.
Sponsored-by: OpenFifth <https://openfifth.co.uk>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Complete the migration of enum data truncation exception handling from
Koha::Object to the centralized ExceptionTranslator utility. This eliminates
the last remaining manual DBIx::Class exception handling in Object code.
Changes:
- Add optional object parameter to ExceptionTranslator->translate_exception()
- Update Schema wrapper methods to accept and pass object parameter
- Enhance enum truncation handling to extract actual property values when object is available
- Remove manual enum exception handling from Koha::Object::store()
- Add comprehensive test coverage for enum handling with and without object context
- Maintain contextual warning behavior for store() operations
The centralized system now handles ALL DBIx::Class exception scenarios:
- FK constraint failures (add/update and delete operations)
- Duplicate key violations
- Invalid data type values
- Enum data truncation with actual property values
This completes the centralization effort while providing better error reporting
than the previous manual approach.
Sponsored-by: OpenFifth <https://openfifth.co.uk>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Pedro Amorim
Martin Renvoize
Matt Blenkinsop
Jonathan Druart
Lucas Gass
Kyle M Hall
Nick Clemens
Hammat Wele
Andreas Roussos
David Cook
Tomás Cohen Arazi
Paul Derscheid
Andrew Fuerste Henry
Michael Hafen