This patch adds a parameter to call the method intranet_catalog_biblio_enhancements_toolbar_button with a biblionumber in the page catalogue/detail.pl.
Modification of cat-toolbar.inc to display new buttons correctly.
To test :
1. Apply the patch
2. Take a plugin who implements the method intranet_catalog_biblio_enhancements_toolbar_button
3. Check if the parameter biblionumber is passed correctly at this method
4. Create the new button in HTML in the plugin method
(button must be like the ones that are in the toolbar of the page detail.pl)
5. Go to the page in browser and check if the new button is displayed
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Added a unit test to confirm that search_for_display orders manually positioned news first, followed by unnumbered news by publication date descending.
Test passes:
prove t/db_dependent/Koha/AdditionalContents.t
Signed-off-by: Gretchen Maxeiner <MaxeinerGL@gcc.edu>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This test built a "bad" biblio by storing invalid MARCXML directly via
Koha::Biblio::Metadata->store(), which now rejects it. Simulate
legacy corrupt data already in the database by writing it straight to
the DBIC row, bypassing the new validation, instead.
Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Sponsored-by: OpenFifth
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
AddBiblio/ModBiblio/ModBiblioMarc gain an optional warnings arrayref
option: any repair messages recorded by Koha::Biblio::Metadata->store()
(see previous commit) are appended to it via object_messages, so a
caller can show the cataloguer what was automatically fixed instead of
the fix happening invisibly.
AddBiblio also now distinguishes Koha::Exceptions::Metadata::Invalid
from other errors in its catch block and rethrows it, so a caller can
tell "this record is unrecoverable" apart from an unexpected internal
error and react accordingly (see following bugs for callers doing so).
The inline StripNonXmlChars calls in TransformHtmlToMarc are removed,
since stripping is now handled centrally and consistently by
Koha::Biblio::Metadata->store() rather than ad-hoc at every call site.
Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Sponsored-by: OpenFifth
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Koha::Biblio::Metadata->store() now validates that MARCXML round-trips
through MARC::Record before saving it, rather than saving whatever was
given and letting corruption surface later as an opaque failure
somewhere downstream.
Two faults are recovered automatically, and either or both may apply
to the same record:
- non-XML characters (recorded as nonxml_stripped, recovered via
StripNonXmlChars)
- datafields with no subfields at all, which MARC::File::XML refuses
to parse (recorded as empty_datafield_stripped, recovered by
removing the empty datafield)
Each recovered fault is logged, reported via object_messages for the
caller to act on, and recorded as a biblio_metadata_errors row (tag
and subfield included where identifiable) for later review - except
where an identical row already exists for the same metadata_id, so a
source record that stays dirty across repeated saves (e.g. re-harvested
from an external repository) doesn't pile up duplicate rows for the
same fault. Existing rows are otherwise left alone on a clean save,
since they're review flags requiring explicit resolution, not a cache
to invalidate.
If the MARCXML cannot be recovered at all, Koha::Exceptions::Metadata::Invalid
is thrown. The recovery logic is exposed as a repair_marcxml class
method so other callers that parse MARCXML ahead of a store() (see
following bugs) can reuse it instead of re-implementing it.
Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Sponsored-by: OpenFifth
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Sanjar Tulkinov Anvar o'g'li <sanjarbiy11@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
login_core only cleared leftover session state on the failed-auth
branch. The earlier 'SIP access not authorized' return (unknown
terminal id) left any account/ils/institution/policy/credentials from
a prior connection on a reused prefork worker in place.
Move the reset to the top of login_core so every non-success exit path
leaves clean state, and only a successful api_auth re-establishes the
session. This keeps the no-authenticated-state-until-auth invariant in
a single place rather than duplicating the key list per branch.
Also extend the unit test to cover the unknown-terminal path on a
reused worker.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Sanjar Tulkinov Anvar o'g'li <sanjarbiy11@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
- t/C4/ClassSplitRoutine/RegEx.t (new): exercises _apply_substitution via
the public split_callnumber interface — basic substitution, alternate
delimiters, \n/$n backrefs, /gi combined flag, /e rejection, and malformed
or uncompilable rules (including the new logger warn path).
- t/Koha/Regex/Replacement.t: extend the subst() helper to handle the ig/gi
combined modifier and add a subtest that confirms case-insensitive global
replacement and backref case preservation.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Note: While the methodology in Koha::Regex::Replacement may seem
bespoke, it's similar to how Python's re.sub(pattern, repl, string)
regex substitution works.
Unit tests:
prove -v t/Koha/Regex/Replacement.t \
t/SimpleMARC.t \
t/db_dependent/MarcModificationTemplates.t \
t/Labels_split_Regex.t
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add a small single-pass helper, Koha::Regex::Replacement::expand_template,
that expands a user-supplied regex replacement string as data: numbered
backreferences ($1..$n, ${n}, the legacy \1..\9 form), named captures
($+{name}, ${name}) and the \n \r \t escapes. Every other character is
copied through literally, so \@{...}/${...} interpolation blocks end up as
text rather than being run. This is used to replace the s///ee double-eval.
The routine is fully qualified at the call sites rather than exported, and
case folding is left out to keep it small.
Signed-off-by: Sanjar Tulkinov Anvar o'g'li <sanjarbiy11@gmail.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The tests for desensitize and alert flags were failing because
items were created with random itemtypes that could have
sip_magnetic=1. This caused unexpected behavior since magnetic
media items don't get desensitized and trigger alert flags.
Fix by explicitly creating itemtypes with sip_magnetic=0 in:
- test_checkout_desensitize
- test_renew_desensitize
- test_checkin_v2
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Marcus Michel <marcus.michel@rbkc.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add unit tests for SIP magnetic media support.
Test plan
1) Run t/db_dependent/SIP/Transaction.t in ktd and confirm it passes
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Marcus Michel <marcus.michel@rbkc.gov.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
GetAgeRestriction is no longer used by any caller. This patch removes
it from the exports list and deletes the old test file.
Test plan:
1. Run tests without this patch:
$ ktd --shell
k$ prove t/db_dependent/Circulation.t \
t/db_dependent/Reserves.t \
t/Circulation/AgeRestrictionMarkers.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Run:
k$ prove t/db_dependent/Circulation.t \
t/db_dependent/Reserves.t \
t/db_dependent/Koha/Policy/Biblio/AgeRestriction.t
=> SUCCESS: Tests pass!
4. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Amended-by: Jonathan Druart
Removed this from the commit message:
"The function body is kept for now as it is not harmful and could be
used by third-party code. It is no longer exported."
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds age restriction handling to the Koha namespace:
- Koha::Biblio->age_restriction: metadata extractor that parses the
agerestriction field using the AgeRestrictionMarker system preference.
Returns the numeric age or undef. Uses single return with last pattern.
- Koha::Policy::Biblio::AgeRestriction->check($biblio, $patron): policy
class that evaluates whether a patron meets the age restriction.
Returns a Koha::Result::Boolean (with payload on restriction).
Test plan:
1. Apply this patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Policy/Biblio/AgeRestriction.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
_upload_file, _download_file, _list_files and _rename_file's 'no directory configured' checks still called add_message() directly, skipping the status persistence this bug added elsewhere in the file. Route them through _abort_operation() so all error paths in Local.pm stay consistent.
prove t/db_dependent/Koha/File/Transport/Local.t
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Two related inconsistencies remained across the three transport
backends' error-handling paths:
1) Only Koha::File::Transport::SFTP's _abort_operation() persisted an
"errors" status snapshot to the status column on every failed
operation. FTP never did, and Local had no _abort_operation() at
all - so an SFTP transport's displayed status could reflect a
single transient failure from real usage (e.g. an EDI cron job)
while the exact same failure on FTP/Local left the stored status
untouched (stale, possibly still showing the last explicit
test_connection() result). This is exactly the kind of unclear
status reporting bug 42656 is about.
2) FTP's _abort_operation() carried its extra detail under an
operation_params key, while SFTP (and Local's plain add_message
calls) used path. The change_directory() success payload had the
same split: FTP used directory, SFTP and Local used path. The
admin template's change_directory row display depended on FTP's
mismatched key names, which meant it silently failed to show the
tested directory for SFTP transports.
Adds a shared _record_error($operation, \%payload) to the
Koha::File::Transport base class - add_message() plus status
persistence - and routes all three backends' _abort_operation() (Local
gains one) through it. Renames FTP's operation_params/directory keys
to path throughout, matching SFTP and Local, and simplifies the
template to read operation.detail.path unconditionally instead of
ORing two differently-named keys.
_record_error() persists the full trace of operations recorded on the
transport object so far this session (every success leading up to the
failure), not just the failing operation in isolation - the same
"full trace" shape TestTransport::process() already builds for an
explicit test_connection() run. A long-lived transport used across a
multi-step operation (e.g. an EDI download loop touching several files
before one fails) will therefore show every successful step leading up
to the failure. It also logs the failure - including the built trace -
via Koha::Logger at warn level, so it's visible in the Koha logs
without needing to query the transport's status column.
Test plan:
1. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/BackgroundJob/TestTransport.t t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t
=> All tests successful (48 tests total), including:
- new "_abort_operation persists status consistently" subtests
in FTP.t, SFTP.t and Local.t that reload the transport from
the DB after a simulated failure and confirm the status
column was updated with the correct operation code and path
detail;
- a new "_record_error() tests" subtest in Transport.t proving
the persisted trace includes a prior success message plus the
failure (not just the failure alone), and that a warn-level
log line mentioning the operation and error was emitted.
2. koha-qa.pl -v 2 --more-tests
=> [PASS], no critic/pod warnings.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Two issues in the operation_description BLOCK's status text, both
flagged in review:
1) The IF/SET that computed the " passed."/" failed." status text sat
between the SWITCH statement and its first CASE. Template Toolkit
only executes CASE-labelled content inside a SWITCH block, so this
was dead code - status was never actually set, and every operation
row silently rendered with no passed/failed text at all. It also
checked operation.error, a field that doesn't exist on the
operation hash (the error message lives under operation.detail.error,
and whether the operation failed is operation.status eq 'error');
even if the code path were reachable it would always have taken the
"passed" branch.
2) Relying on a leading space baked into the " passed."/" failed."
string literals to separate them from the preceding </span> is
fragile. Made the space explicit in the markup instead, and dropped
the leading space from the string values.
Moves the status computation above the SWITCH (using operation.status),
and adds an explicit space before [% status | html %] on every CASE line.
Also adds t/db_dependent/Koha/BackgroundJob/TestTransport.t, which was
entirely missing: it exercises process() end-to-end (enqueue, then
process, then reload both the job and the transport) and confirms the
persisted trace records every operation in sequence - successes before
the failure that ended the run - rather than collapsing to just the
final error.
Test plan:
1. prove t/db_dependent/Koha/BackgroundJob/TestTransport.t
=> All tests successful, confirming process() persists the full
operation sequence (order preserved, at least one success and
one error present) on both the job and the transport's own
status column.
2. Manually rendered the fixed operation_description BLOCK with
Template Toolkit against mock operations (mixed success/error) and
confirmed every row now shows "Connection passed.", "Change
directory (/incoming) passed.", ..., "Change directory (/outgoing)
failed.- error:No such directory" - status text present with
correct spacing, where before the fix no status text rendered at
all.
3. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t
=> All tests successful (47 tests total), no regressions.
4. koha-qa.pl -v 2 --more-tests
=> [PASS], no critic/pod warnings.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Koha::File::Transport's three backends disagree on what
change_directory(undef) means: FTP navigates to the server root,
SFTP falls back to the SFTP session's own home directory, and Local
fell back to the configured download_directory or, if that's also
unset, silently to '.' - the Koha process's own working directory,
which has no relation to any configured transport location.
test_connection() calls change_directory(undef) unconditionally for
both the download and upload directory checks on every config save
(via the auto-enqueued TestTransport background job). For a Local
transport configured with only one of download_directory/
upload_directory, the untested direction's list_files() call would
silently list the Koha process's own cwd (e.g. the Koha install root)
instead of failing cleanly.
Add a private _working_directory($configured_field) helper that
returns the tracked change_directory() override or the given
configured field, but never falls back to '.'. _upload_file,
_download_file, _list_files and _rename_file now use it and record a
clear "not configured" error instead of silently acting on an
unrelated directory when neither is set.
FTP's root and SFTP's home-directory fallbacks are left as-is: unlike
Local's '.', they navigate to a real, meaningful location for that
specific connection rather than leaking unrelated filesystem content,
so they aren't part of this fix.
Test plan:
1. prove t/db_dependent/Koha/File/Transport/Local.t
=> All tests successful, including the new subtest proving that
list_files()/rename_file()/download_file() fail cleanly (with a
"not configured" error) rather than falling back to '.' when
their respective directory isn't configured, while the
configured direction (upload, in the test) still works.
2. prove -r t/db_dependent/Koha/File/ t/db_dependent/Koha/Edifact/Transport.t t/db_dependent/api/v1/file_transports.t
=> All tests successful (43 tests total), no regressions.
3. koha-qa.pl -v 2 --more-tests
=> [PASS], no critic/pod warnings.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
t/db_dependent/Log.t
- Add subtest verifying logscriptaction() logs to the SCRIPTS module,
respects the ScriptLog preference, prefixes loginfo with $0, appends
the info parameter, and honours custom action values.
t/Koha/Script_scrub_argv.t
- New test file exercising all branches of _scrub_argv():
--flag value form, --flag=value form, non-sensitive flags, case-
insensitive matching, multiple sensitive patterns (token, secret,
apikey, api_key), and a flag immediately followed by another flag.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
FTP, Local and SFTP list_files() were unified to return the same flat
shape, but consumers still had no portable way to ask a transport for
its current remote directory - the underlying libraries' own methods
of that name don't agree on what "cwd" means. Net::SFTP::Foreign's
cwd() is a pure read-only accessor, but Net::FTP's cwd() is a mutator:
called with no argument it defaults to "/", issues a real CWD command,
and returns a boolean rather than a path.
The koha-plugin-transport-browser plugin discovered this the hard way:
it read $transport->{connection}->cwd directly to capture the initial
working directory, which happened to work for SFTP but silently reset
the FTP connection to root and captured "1" (a boolean) as the "path"
on every request, breaking directory navigation for FTP transports.
Add a public current_directory() method to the Koha::File::Transport
base class, following the same connect-on-demand pattern as
list_files()/upload_file()/etc, with a _current_directory() hook each
subclass implements using its own real read-only accessor: pwd() for
FTP, cwd() for SFTP, and the tracked {current_directory} (falling back
to download_directory) for Local. Consumers should use this instead of
reaching into transport internals.
Test plan:
1. prove t/db_dependent/Koha/File/Transport.t
t/db_dependent/Koha/File/Transport/FTP.t
t/db_dependent/Koha/File/Transport/Local.t
t/db_dependent/Koha/File/Transport/SFTP.t
=> All tests successful, including the new current_directory()
tests subtest in each transport test file, which mock the
underlying libraries (Net::FTP / Net::SFTP::Foreign) to assert
each backend reads its own real read-only accessor.
2. koha-qa.pl -v 2 --more-tests
=> [PASS] for all four changed modules, no critic/pod warnings.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The FTP and Local transports both explicitly filter out '.' and '..'
pseudo-entries when listing a directory, but the SFTP transport never
did - it relied on the assumption that Net::SFTP::Foreign's ls() already
excludes them. It doesn't: ls() returns exactly whatever entries the
remote server includes in its SSH_FXP_READDIR response, with no default
filtering. Whether '.'/'..' show up therefore depends entirely on the
remote SFTP server implementation, producing inconsistent results between
otherwise identically configured SFTP accounts.
Test plan:
1. prove t/db_dependent/Koha/File/Transport/SFTP.t
=> fails before this patch (listing includes '.' and '..')
=> passes after this patch
2. prove -r t/db_dependent/Koha/File/Transport/ t/db_dependent/Koha/File/Transports.t
=> all still pass
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The MLSD path, SFTP, and Local always return all six documented
list_files() keys (filename, longname, size, perms, mtime, type) - undef
where the value is unknown, but the key always present. The NLST
fallback path (servers without MLSD support) only set filename, size,
mtime and type, omitting longname and perms entirely rather than
setting them to undef.
Functionally harmless for consumers reading $file->{perms} (Perl
returns undef for a missing hash key too), but it breaks the "same flat
shape" contract this bug established, and would trip up any consumer
using exists() instead of defined() to check for a key.
Test plan:
1. prove t/db_dependent/Koha/File/Transport/FTP.t
=> All tests successful, including the new assertion that longname
and perms keys are present (as undef) on the NLST fallback path.
2. koha-qa.pl -v 2 --more-tests
=> [PASS], no critic/pod warnings.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Net::SFTP::Foreign's ls() returned file metadata with size, permissions
and modification time nested inside a Net::SFTP::Foreign::Attributes
object, while FTP and Local returned flat hash keys and silently
dropped directories. Callers of list_files() needed backend-specific
parsing to get at file metadata or to know a directory even existed,
defeating the purpose of the shared transport API.
All three transports now return the same flat hashref shape per entry:
filename, longname, size, perms, mtime, type ('file'/'directory'/
'other'). Directories are included everywhere; '.' and '..' remain
excluded everywhere. FTP's MLSD path prefers the UNIX.mode fact for
exact Unix permissions, falling back to an approximation from the RFC
3659 perm capability fact when UNIX.mode isn't available, and parses
the modify fact into mtime; its NLST fallback (servers without MLSD)
probes each entry with SIZE/MDTM to recover size, mtime and a
best-effort type.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Koha::File::Transport::FTP::_list_files parsed the LIST / "ls -l" output
with a non-greedy date capture that only consumed the first of the three
date tokens (the month), leaving the day and time glued to the front of
the filename. Every filename returned for an FTP transport was therefore
prefixed with the listing date, e.g.
-rw-r--r-- 1 user group 1234 Jan 01 12:00 QUOTES_413514.CEQ
=> filename "01 12:00 QUOTES_413514.CEQ"
Callers (Koha::Edifact::Transport, marc_ordering_process.pl) then asked
the server for a file that does not exist, so no files were downloaded,
renamed or deleted over FTP. SFTP and Local transports were unaffected.
This regressed when the EDI transport was moved onto the file_transports
system (bug 38489). The previous Koha::Edifact::Transport::ftp_download
listed files with Net::FTP->ls (NLST, bare names); the refactor switched
to Net::FTP->dir (LIST) so it could return the richer hashref shape used
by the SFTP and Local transports, but the parse was faulty.
The date field in "ls -l" output is a fixed run of three whitespace
separated tokens (Mon DD HH:MM or Mon DD YYYY). Consume it explicitly so
the filename is captured cleanly, preserving spaces and still skipping
directories. The LIST-based approach and the filename/longname/size/perms
return shape are kept so the structure stays consistent across transport
classes (relied on by consumers, including out-of-tree ones).
Test plan:
1) prove t/db_dependent/Koha/File/Transport/FTP.t
2) git checkout Koha/File/Transport/FTP.pm and re-run: the list_files()
subtest fails on the filename/size/perms assertions
3) prove t/db_dependent/Koha/Edifact/Transport.t
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The default `_match=contains` matching builds a LIKE query even on
integer columns, so a plain `?object=42` also matches object values
like 420 or 142. The test's dynamically-assigned patron borrowernumber
(auto_increment, never reset by transaction rollback) could drift into
a value containing "42" as a substring, intermittently inflating the
match count in CI. Add `_match=exact` since the test's intent is an
exact object id match.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
To test:
- cypress run --spec t/cypress/integration/KohaTable/PatronSearch_spec.ts
- cypress run --spec t/cypress/integration/KohaTable/KohaTable_spec.ts
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
local_holdgroup_match was being hardcoded to 1 in the two LocalHoldsPriority
allocation paths (GiveLibrary and GiveLibraryGroup), regardless of whether the
chosen item actually satisfied the LocalHoldsExclusivity conditions. This
caused staff to be directed to pull an item that would not trigger the hold at
checkin when the two features used different patron/item controls.
The fix removes the premature local_holdgroup_match => 1 from both LHP paths and
lets _flag_local_holdgroup_matches evaluate all allocated items using the correct
LocalHoldsExclusivity* sysprefs.
Also fixes a pre-existing missing bind-variable placeholder in the
INSERT INTO tmp_holdsqueue statement (17 columns, only 16 ?).
Test updates:
- Existing local_holdgroup_match test gains the matching LocalHoldsExclusivity
sysprefs so the flag is set via the correct code path.
- New subtest reproduces the reported bug: LHP (HomeLibrary/homebranch) targets
a non-local item while exclusivity (PickupLibrary/holdingbranch) should not
flag it — confirms local_holdgroup_match is 0 after the fix.
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Sponsored-by: Main Library Alliance <https://www.mainlib.org/>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
LocalHoldsExclusivity and LocalHoldsPriority are adjacent but distinct
features: priority alters *which patron's hold* gets filled first, while
exclusivity restricts *which items* may fill a hold during a time window.
Each should be usable independently of the other.
Changes:
- Rename LocalHoldsPriorityExclusivityPeriod to LocalHoldsExclusivityPeriod
- Add LocalHoldsExclusivityPatronControl (PickupLibrary/HomeLibrary)
- Add LocalHoldsExclusivityItemControl (homebranch/holdingbranch)
- HoldsQueue: add _flag_local_holdgroup_matches() that flags
local_holdgroup_match on allocated items using hold-group matching
and the new exclusivity control prefs, regardless of LocalHoldsPriority
- Reserves: CheckReserves exclusivity check now computes its own
"is this item local" using the new prefs and hold-group siblings,
independent of the LocalHoldsPriority matching variables
- Atomicupdate handles both fresh install and rename-from-old-pref paths
- Pref UI note clarifies independence from LocalHoldsPriority
Test plan:
1. Apply patch, run updatedatabase.pl
2. Verify three new sysprefs exist in Administration > System preferences
> Circulation: LocalHoldsExclusivityPeriod, LocalHoldsExclusivityPatronControl,
LocalHoldsExclusivityItemControl
3. Set LocalHoldsPriority to "Don't give" (None)
4. Create a library hold group with at least two libraries (e.g. Lib-A, Lib-B)
5. Create a third library (Lib-C) NOT in the hold group
6. Create an item at Lib-B
7. Place a hold for a patron at Lib-A (pickup at Lib-A)
8. Set LocalHoldsExclusivityPeriod to 7, PatronControl to HomeLibrary,
ItemControl to homebranch
9. Run the holds queue builder (misc/cronjobs/holds/build_holds_queue.pl)
10. Verify hold_fill_targets.local_holdgroup_match = 1 for that hold
(item at Lib-B is in the same hold group as patron at Lib-A)
11. Check in the item at Lib-C — it should NOT fill the hold (exclusivity
blocks the non-local item within the 7-day window)
12. Check in the item at Lib-B — it SHOULD fill the hold (local match)
13. Reset: delete the hold, create a new one with a patron at Lib-C
(pickup at Lib-C). Run the queue again.
14. Verify hold_fill_targets.local_holdgroup_match = 0 for this hold
15. The item at Lib-B should now fill this hold at Lib-C checkin
(non-local patron, no exclusivity protection)
16. Set LocalHoldsExclusivityPeriod to 0 — verify non-local items can
fill holds normally regardless of hold_fill_targets flag
17. prove t/db_dependent/Holds/LocalHoldsPriority.t — passes
18. prove t/db_dependent/HoldsQueue.t — passes
19. prove t/db_dependent/Reserves.t — passes
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Covers the new LocalHoldsPriorityExclusivityPeriod behaviour and keeps
existing tests working with the schema change:
- t/db_dependent/Holds/LocalHoldsPriority.t: new subtest exercising
all four cases -- pref disabled, pref active on a fresh hold, pref
active past the window, and pref active with no hold_fill_targets
row (queue hasn't run).
- t/db_dependent/HoldsQueue.t: assert that after CreateQueue() with a
local-group match, both tmp_holdsqueue and hold_fill_targets rows
carry local_holdgroup_match = 1.
- t/db_dependent/Reserves.t, t/db_dependent/Koha/Hold.t: qualify the
two direct "INSERT INTO hold_fill_targets VALUES (...)" statements
with explicit column names so they continue to work now that
hold_fill_targets has one extra column.
Test plan:
prove t/db_dependent/Holds/LocalHoldsPriority.t \
t/db_dependent/HoldsQueue.t \
t/db_dependent/Reserves.t \
t/db_dependent/Koha/Hold.t
All tests should pass.
Sponsored-by: Main Library Alliance <https://www.mainlib.org/>
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
t/db_dependent/Log.t
- Add subtest verifying cronlogaction() prefixes loginfo with \$0
(the running script path), appends the info parameter, defaults the
action to 'Run', and respects custom action values.
t/Koha/Script.t
- Assert that \$Koha::Script::_cron is false when the module is loaded
without the -cron flag, confirming INIT/END blocks will not log.
t/Koha/Script_cron.t
- Assert that \$Koha::Script::_cron is true when the module is loaded
with the -cron flag, confirming INIT/END blocks will log.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch implements the library_limit concept for Report/Reports. Staff with the correct permission and superlibrarians can set libaries to which the report should be visible, if the system preference is enabled.
Test plan:
Confirm Initial Settings
1. Log in to Koha staff with username: koha and password: koha and set your library as Centerville if it's not already.
2. Visit the Saved reports page, and create and save three reports: Report 1, Report 2, and Report 3. The SQL query for each report doesn't matter, it should just be something valid.
3. Visit the Patrons page and create two staff users: StaffReportLimitUser and StaffNoLimitPermission. Set their home libraries to Centerville and Fairfield, respectively.
4. Under Allow access to the reports module, ensure both users have report management permissions for Create, Delete, Execute.
Verify Patch Settings
5. Apply the patch and restart services. If using Koha Testing Docker (KTD): from the Koha repo directory, apply the patch (e.g. `git bz apply 16631`, or check out the patched branch with `git fetch && git checkout <branch>`), then run `ktd --shell` to enter the KTD shell and run `restart_all`. If the patched branch is already checked out, just enter the KTD shell and run `restart_all`.
6. Click Patrons in the top navbar to go to the Patrons page.
7. Search for "StaffReportLimitUser" in the patron search field.
8. Click StaffReportLimitUser to edit the patron record for this user.
9. Click More -> Set permissions to access the permissions screens for the StaffReportLimitUser.
10. In the row for Allow access to the reports module (reports), click Show details to view relevant permissions.
11. Confirm Manage Report Limits (manage_report_limits) is an option under Allow access to the reports module (reports).
12. Click the checkbox for Manage Report Limits to give this user this permission, and click Save.
13. Go to the System Preferences page (Click the More dropdown from the top nav bar, click Administration, click System Preferences).
14. Verify Reports is an option in the left side navigation bar, and that when clicked, you see the LimitReportsByBranch option.
15. Enable LimitReportsByBranch as described in step 14.
16. Go back to Saved reports as in Step 2.
17. Click Edit in the row menu for Report 1.
18. At the bottom of the Report 1 edit page, you should see a Library limitation select box.
Verify the Library Limitation Settings are Persisted
19. Choose Centerville from the select box and click Update SQL.
20. Reload the page, or navigate away from the page and go back to the edit screen for Report 1.
21. Verify that Centerville is still in the Library limitation select box.
22. Add Fairfield in the Library limitation select box and click Update and run SQL.
23. Go back to editing Report 1.
24. Confirm the Library limitation select box has both Centerville and Fairfield selected.
25. Duplicate Report 1.
26. Confirm the Library limitation select box has both Centerville and Fairfield selected.
27. Click Save Report.
28. Confirm the Library limitation select box has both Centerville and Fairfield selected.
Verify Reports are Limited Based on Library Limitations
29. Edit Report 1 to add Centerville in the Library limitation select box as the only choice and click Update SQL.
30. Edit Report 2 to add Fairfield in the Library limitation select box as the only choice and click Update SQL.
31. Report 3 should have no library limitations, so it should be visible to all libraries.
32. Go to Saved reports as in Step 2.
33. Confirm you do not see Report 2, and only Report 1 and Report 3.
34. In the upper right corner, click the drop down that says koha Centerville and click Set library to change your library.
35. Set your library to Fairfield.
36. Go to Saved reports as in Step 2.
37. Confirm you do not see Report 1, and only Report 2 and Report 3.
Verify Disabling LimitReportsByBranch Preference Shows All Reports
38. Disable LimitReportsByBranch preference as in steps 13 to 15.
39. Go to Saved reports as in Step 2.
40. Confirm Report 1, Report 2, Report 3 are visible.
41. Click to edit Report 1 and verify there is no Library limitation select box.
Verify Library limitations are Persisted When EnableFilteringReports is Toggled On or Off
42. Enable LimitReportsByBranch preference as in steps 13 to 15.
43. Ensure your library is set to Fairfield as in step 35.
44. Go to Saved reports as in Step 2.
45. Confirm Report 1 is not visible, and Report 2 and Report 3 are visible.
46. Click to edit Report 2 and verify the Library limitation select box is at the bottom of the page and that it has Fairfield selected.
Verify the Show all reports option shows all reports for users with the Manage Report Limits permission and superlibrarians
47. Go to Saved reports as in Step 2 and in the Show reports dropdown, select Show all reports, and click Apply filter.
48. Confirm the page reloads with all reports visible: Report 1, Report 2, and Report 3 should be visible.
49. Select Show my branch reports from the dropdown and click Apply filter.
50. Confirm Report 1 is not visible, and Report 2 and Report 3 are visible (you should only see reports for your branch, Fairfield).
Verify Report filters return the correct results
51. Go to Saved reports as in Step 2.
52. Verify you're viewing only your branch's reports in the filter.
53. Under Filter in the left sidebar, type "2" in the Keyword field and click Apply filter.
54. Confirm only Report 2 is visible.
55. Under Filter in the left sidebar, type "1" in the Keyword field and click Apply filter.
56. Confirm there are no reports visible, and the message "No saved reports match your criteria" is displayed.
Verify Library Limits column is available to superlibrarians and users with the Manage Report Limits permission
57. Go to Saved reports as in Step 2.
58. Click the Columns dropdown and ensure that the Library limits column is available to select for display. Select it.
59. Confirm that the Library limits column is now visible in the list of reports and that it shows the correct library limitations for each report (e.g., Report 1 should show 1 library limitation and Centerville (CPL) on hover, Report 2 should show 1 library limitation and Fairfield (FFL) on hover, and Report 3 should show no limitation).
Verify the Show all reports and Limit reports options are available to users that are not superlibrarian but have the Manage Report Limits permission
60. For the StaffReportLimitUser, add the permission for Manage Report Limits as described in steps 9 to 12. This will allow this user to see the Show all reports and Limit reports options.
61. Log out of the koha admin account. In the upper right corner, click the drop down that says koha Fairfield and click Log out.
62. Log in as StaffReportLimitUser, the previously created staff user with report permissions.
63. Confirm the staff user's library is Centerville in the upper right corner of the screen.
64. Go to Saved reports as in Step 2.
65. Confirm the filter option for Show reports is visible.
66. Click to edit Report 1.
67. Verify there is a Library limitation select box and that it can be interacted with.
Verify the Show all reports and Limit reports options are not available to users that are not superlibrarian and do not have the Manage Report Limits permission
68. Log out of the StaffReportLimitUser account.
69. Log in as StaffNoLimitPermission, the previously created staff user without the Manage Report Limits permission.
70. Confirm the staff user's library is Fairfield in the upper right corner of the screen.
71. Go to Saved reports as in Step 2.
72. Click the Columns dropdown and ensure that the Library limits column is not in the list of available columns to select for display.
73. Confirm the filter option for Show reports is not available.
74. Click to edit Report 2.
75. Verify there is no Library limitation select box at the bottom of the page.
[NEW] Verify library limits are auto-assigned and preserved correctly for non-permissioned users
76. Still logged in as StaffNoLimitPermission (Fairfield) from the previous section. Go to Saved reports and click New report. Enter "Limit Auto-Assign Test" as the report name and SELECT 1 as the SQL. Confirm there is no Library limitations select box visible on the form. Click Save.
77. Go back to Saved reports. Click Edit for Report 2 (limited to Fairfield). Add `-- test` as a trailing SQL comment and click Update SQL. This simulates a non-permissioned user saving an existing report that already has limits.
78. Go back to Saved reports. Click Edit for Report 3 (no library limits). Add `-- test` as a trailing SQL comment and click Update SQL. This simulates a non-permissioned user saving an existing report that has no limits.
79. Log out and log back in as StaffReportLimitUser (Centerville, has manage_report_limits).
80. Go to Saved reports. Use Show all reports if needed. Find "Limit Auto-Assign Test" and click Edit. Confirm the Library limitations select box shows Fairfield as the only selected library — the report was automatically limited to the creator's home library on create.
81. Find Report 2 and click Edit. Confirm the Library limitations select box still shows only Fairfield — the existing limit was preserved when the non-permissioned user saved the report.
82. Find Report 3 and click Edit. Confirm the Library limitations select box is empty — no limits were introduced when the non-permissioned user saved a report that had no existing limits.
Verify limited reports are not visible to users without permissions
83. While still logged in as StaffReportLimitUser from step 79, copy the URL for the edit page of Report 1.
84. Log out of the StaffReportLimitUser account.
85. Log in as StaffNoLimitPermission, the user without the Manage Report Limits permission.
86. Paste the copied URL for the edit page of Report 1 into the browser and navigate to it.
87. Confirm that you see an error message that says "This report is restricted and cannot be accessed by the current user." and that the page does not load the report details.
Run Unit Tests
88. Using Koha Testing Docker, in the Koha home directory run the command: ktd --shell to start the ktd shell.
89. Run command: reset_all to drop and recreate the koha database.
90. Run command prove t/db_dependent/Koha/Reports.t to run the test.
91. This file contains the unit tests for the reports_branches table and the new subtest `library limits enforcement on save and update`, which covers auto-assignment of branch limits on create and preservation of limits on update for non-permissioned users.
92. All tests should pass.
93. Run command prove t/Koha/Auth/Permissions.t
94. Permissions unit tests in this file were updated.
95. All tests should pass.
96. Run command prove t/db_dependent/Reports/Guided.t
97. The Guided report tests were updated to account for the new library limitations and permissions.
98. All tests should pass.
Signoff
99. Sign off if the steps above check out.
Sponsored-by: Ramapo Catskill Library System
Co-authored-by: Casey Conlin <casey.conlin@gmail.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The ILL Vue app currently reads ILLModule and ILLPartnerCode through the system preferences service. That service requires parameters/manage_sysprefs, so a staff user with ILL permissions but without system preference administration permissions cannot load the ILL module.
Add an ILL-specific /ill/config REST endpoint protected by the ill permission, and switch the ILL Vue app to load its configuration from that endpoint. This follows the module-specific configuration endpoint pattern introduced by bug 33606 for the same manage_sysprefs problem in the ERM Vue app.
Security/compatibility considerations: the endpoint returns only allow-listed, non-secret ILL configuration values needed by the ILL Vue app. It does not expose the generic system preferences API, and /api/v1/sysprefs remains restricted to parameters/manage_sysprefs.
Regression test coverage: the new API test verifies that a staff user with the ill permission can read /ill/config, that a staff user without the ill permission cannot read it, and that the same ILL-only user still cannot access /api/v1/sysprefs.
Test plan:
1. prove -I. -It/lib t/db_dependent/api/v1/ill_config.t
2. prove -I. -It/lib t/db_dependent/api/v1/ill_users.t
Signed-off-by: David Nind <david@davidnind.com>
Amended-by: Jonathan Druart
chmod +x t/db_dependent/api/v1/ill_config.t
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
To test:
1. prove t/db_dependent/Circulation.t
2. Observe the following failures
t/db_dependent/Circulation.t .. 17/87
# Failed test 'Bug 42298: No autorenew before rule is respected'
# at t/db_dependent/Circulation.t line 1134.
# got: '0'
# expected: '1'
# Failed test 'Bug 42298: No autorenew before rule is respected'
# at t/db_dependent/Circulation.t line 1135.
# got: 'too_soon'
# expected: 'auto_renew'
# Failed test 'soonest_renew_date is not returned because this issue can be renewed'
# at t/db_dependent/Circulation.t line 1136.
# got: '2026-04-21T23:59:00'
# expected: undef
# Failed test 'No renewal before is 99, patron opted out of auto_renewal so can renew'
# at t/db_dependent/Circulation.t line 1141.
# got: '0'
# expected: '1'
# Looks like you failed 4 tests of 124.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Transitioning a booking to 'issued' no longer skipped clash detection,
and Item::check_booking treats any live checkout on the item as a
clash. Switching an on-site checkout of a booked item to a regular
checkout (SwitchOnSiteCheckouts) therefore crashed AddIssue with an
unhandled Koha::Exceptions::Booking::Clash; collection could fail the
same way whenever the biblio's bookable item pool had shrunk since the
booking was placed.
- short-circuit no-op stores before validation; circulation issues
redundant status writes that must not re-trigger the checks
- rename _is_final_status_transition to _skip_clash_detection and also
skip when transitioning to 'issued' from an active status, as long
as the booked period and item are unchanged
- keep validating period/item changes and reactivations of terminal
bookings
- cover the on-site switch in Circulation.t and the store() semantics
in Booking.t
Test plan:
1. Enable OnSiteCheckouts and SwitchOnSiteCheckouts
2. Place a booking starting today for a patron
3. Check the booked item out to that patron from Circulation > Check
out with 'On-site checkout' ticked. Do NOT use the bookings page's
'Transform to checkout' button: it submits a fixed field set that
does not carry the on-site flag (Bug 36789), so the checkout would
silently be a regular one and step 4 could not reproduce the switch
4. The booking shows 'Issued'. Scan the same barcode for the same
patron again to switch the on-site checkout to a regular one; this
crashed with Koha::Exceptions::Booking::Clash before the patch and
now succeeds with the booking still 'Issued'
5. prove t/db_dependent/Koha/Booking.t t/db_dependent/Circulation.t
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Always include the plural version, not the singular one (the plural
imports the singular)
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch replaces all calls to C4::Stats::UpdateStats with the wrapped
function from the Koha namespace. As this is the last function in C4::Stats, the
module is also deleted.
Several tests depended on transitive includes to Koha::Statistics through
C4::Stats. I've identified uses of that module by searching, and added the
necessary use in each case. I've run `prove -r t xt` with this patch applied and
didn't hit any relevant errors.
Test plan:
1. Inspect the patch, and verify each changed call maintains the same parameters.
2. Apply patch.
3. Prove the following files:
- t/db_dependent/Accounts.t
- t/db_dependent/Circulation.t
- t/db_dependent/Koha/Account.t
- t/db_dependent/Koha/ILL/Request.t
- t/db_dependent/Koha/Item.t
- t/db_dependent/Koha/Pseudonymization.t
- t/db_dependent/Koha/Recalls.t
Sponsored-by: Open Fifth
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
To test:
1. Set both system preferences NotifyBorrowerDeparture and
MembershipExpiryDaysNotice to 10 days
2. Create or edit a patron account with the following settings:
- expiry date is exactly 10 days in the future
- add an email address on the patron account
- set messaging preferences to receive email notices for Patron
account expiry
3. Run the membership expiry cron job to generate a notice:
ktd --shell
perl misc/cronjobs/membership_expiry.pl -c
4. Check notices tab on patron account
--> A membership expiry notice has been queued for the patron
5. Click back to the patron details
--> Note that there is no alert that the patron account will be expiring
soon
6. Apply patch and restart_all
7. Refresh the patron details page
--> You should now see an alert that the account will be expiring soon
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
t/db_dependent/Holds.t, t/db_dependent/ILSDI_Services.t, and
t/db_dependent/api/v1/holds.t all fail if a site has a global
'Holds Allowed (Daily)' circulation rule configured, because
C4::Reserves::CanItemBeReserved checks holds_per_day before
holds_per_record/reservesallowed, so an ambient daily-cap rule
returns tooManyReservesToday and masks the status these tests
actually exercise (tooManyReserves, noReservesAllowed, OK, or the
API's hold_limit override). The affected subtests never set
holds_per_day themselves, so they unintentionally depend on the
database having no such rule configured.
Test plan:
1. On a fresh db, set a global 'Holds Allowed (Daily)' circulation
rule (Administration > Circulation and fine rules) to 1.
2. prove t/db_dependent/Holds.t t/db_dependent/ILSDI_Services.t
t/db_dependent/api/v1/holds.t
=> fails before this patch, passes after.
3. Remove the circulation rule and re-run the same tests.
=> still passes (no regression for the default/unconfigured case).
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
INSERT INTO action_logs (timestamp, user, module, action, object, info, interface) VALUES
(NOW(), 5, 'CIRCULATION', 'ISSUE', 5,
'{"issue":1,"branchcode":"CPL","itemnumber":1,"confirmations":["TOO_MANY"],"forced":["DEBT"]}', 'intranet'),
(NOW(), 5, 'CIRCULATION', 'RETURN', 5,
'1', 'intranet'),
(NOW(), 5, 'HOLDS', 'CREATE', 7,
'{"hold":7,"biblionumber":1,"itemnumber":1,"confirmations":["HIGHHOLDS"],"forced":["HOLD_POLICY_OVERRIDE"]}', 'intranet');
Test plan:
1) Without the patch applied, run the populate SQL above against the kohadev database.
2) Go to Tools > Log viewer
3) Under Modules, select only Circulation - leave the date range empty and click Submit.
4) Observe the ISSUE row's Info column shows the raw JSON payload as plain text, and the RETURN row shows the bare itemnumber "1" as plain text - neither is a clickable link to the item.
5) Go back, select only the Holds module, click Submit, and observe the CREATE row's Info column likewise shows raw JSON with no hold/item links.
6) Apply the patch.
7) Run yarn build && restart_all, then hard refresh the browser.
8) Repeat step 3.
9) Confirm the ISSUE row's Info column now shows a link reading "Item <barcode> CPL" (e.g. Item 3999900000001 CPL) - the branchcode comes from the JSON payload - and clicking it opens the item page. Override information is also displayed correctly
10) Confirm the RETURN row's Info column now shows a link reading "Item <barcode>" (no branchcode) opening the same item's detail page - verifying the bare-itemnumber payload path.
11) Repeat step 5. Confirm the CREATE row now shows "Hold 7 on biblio 1" linking to reserve/request.pl?biblionumber=1, followed by " - Item 1" linking to the item detail page. Override information is also displayed correctly
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds Koha::REST::Plugin::CSRF which enforces CSRF token
validation on state-changing requests (POST/PUT/DELETE/PATCH) when
a CGISESSID cookie is present.
Requests using OAuth2 or HTTP Basic Auth are not affected since they
don't use session cookies and are inherently immune to CSRF.
The client-side already sends the CSRF-TOKEN header via http-client.js
(added in Bug 34478). This patch adds the server-side enforcement.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/REST/Plugin/CSRF.t
=> SUCCESS: Tests pass!
3. Run:
k$ prove t/db_dependent/api/v1/csrf.t
=> SUCCESS: Tests pass!
4. Tests cover:
- GET/HEAD/OPTIONS requests bypass CSRF check
- POST/PUT/DELETE without CGISESSID bypass CSRF check
- POST with CGISESSID but no CSRF-TOKEN returns 403
- POST with CGISESSID and invalid CSRF-TOKEN returns 403
- POST with CGISESSID and valid CSRF-TOKEN passes through
- PUT and DELETE enforcement
5. Sign off :-D
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch checks for waiting or in_transit items allocated to recalls, and excludes them from the Holds to Pull report.
To test:
1. Enable UseRecalls and set the recalls-related circulation rules
2. Check out an item to Patron A
3. Place an item-level hold on the item for Patron B
4. Log into the OPAC as Patron C and search for the item. Place an item-level recall on the item.
5. Go back to the staff interface and check in the item. This should prompt a pop-up to confirm the recall as waiting. Confirm the recall as waiting.
6. Go to Circulation -> Holds to pull.
7. Notice that even though the item is allocated for a recall, the hold is showing. It should not be showing in this page.
8. Apply the patch, restart services and refresh the page
9. The item allocated to a recall should no longer be showing in the Holds to pull report
10. Go to the record detail page and go to the Recalls tab
11. Revert the recall from its waiting status
12. Go back to the Holds to pull page. The record will now show for the hold.
13. Go to the Circulation -> Check in page
14. Go to the menu in the top-right of the page and click Set library
15. Change your library to a different one
16. Check in the item to confirm the recall as waiting and trigger a transfer
17. Go back to the Holds to pull page
19. The item should no longer show on the Holds to pull report
20. Confirm tests pass t/db_dependent/Koha/Holds.t
Sponsored-by: Catalyst IT
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When RealTimeHoldsQueueUnallocated is enabled, the background job
passes unallocated => 1 and delete => 0 to update_queue_for_biblio.
This skips deleting existing queue entries and only processes holds
not already allocated, matching the --unallocated behavior of
build_holds_queue.pl.
Test plan:
1. Apply patches
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/BackgroundJob/BatchUpdateBiblioHoldsQueue.t
=> SUCCESS: Tests pass!
3. Tests verify:
- delete => 1 and no unallocated when pref is off
- delete => 0 and unallocated => 1 when pref is on
4. Sign off :-D
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Sponsored-by: The Main Library Alliance <https://www.mainlib.org/>
Signed-off-by: Alex Carver [Acerock7] <alex@rcls.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch resolves three QA findings on the cashup reconciliation
work:
* Add a CSP nonce to the inline <style> block in cashup_summary.inc
so it is not flagged by the forbidden_patterns check.
* Document the private _get_session_end method on
Koha::Cash::Register::Cashup so pod_coverage passes.
* Mock CashupReconciliationNoteRequired to 0 in
t/db_dependent/Koha/Cash/Register/Cashup.t so the test does not
depend on the site preference value (it previously failed when
the syspref was set to "Require" because surpluses and deficits
are created without a reconciliation note in this test).
Test plan:
1. Apply the patch.
2. Set CashupReconciliationNoteRequired to "Require".
3. Run prove t/db_dependent/Koha/Cash/Register/Cashup.t -- it should
pass.
4. Set CashupReconciliationNoteRequired to "Don't require" and
re-run -- it should still pass.
5. Run koha-qa.pl on the bug branch -- pod_coverage on
Koha/Cash/Register/Cashup.pm and forbidden_patterns on
cashup_summary.inc should now pass.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds configuration options for cashup reconciliation and
enhances the user interface with preview functionality and improved
interaction patterns.
Configuration Options:
1. CashupReconciliationNoteRequired (System Preference):
- Optional validation requiring reconciliation notes when discrepancies exist
- Ensures documentation of surplus/deficit causes when needed
- Can be disabled for less stringent workflows
- Applied at cashup completion time
2. CashupReconciliationNoteAuthorisedValue (System Preference):
- Allows use of authorized value dropdown for reconciliation notes
- Provides standardized note options (e.g., "Till count error", "Cash removed")
- Falls back to free text textarea when not configured
- Improves consistency and reporting capabilities
UI Enhancements:
1. Cashup Summary Preview:
- Preview cashup summary for in-progress cashups
- Shows current transaction totals before completion
- Clearly indicates preview vs. final summary status
- Helps staff verify amounts before committing
2. Improved User Experience:
- Conditional note fields based on configuration
- Authorized value dropdowns when configured
- Clear validation messages
- Responsive UI updates
Backend changes:
- Enhanced Koha::REST::V1::CashRegisters::Cashups with preview support
- Database atomicupdate script for system preferences
- Validation logic for required notes
Frontend changes:
- Preview modal functionality in cashup_modal.js
- Conditional rendering of note input fields
- Authorized value dropdown integration
- Enhanced summary display logic
Test plan:
1. Apply patches and run database update
2. Configure CashupReconciliationNoteRequired:
- Enable preference
- Perform cashup with discrepancy without note - should fail
- Add note and retry - should succeed
- Disable preference - verify note optional
3. Configure CashupReconciliationNoteAuthorisedValue:
- Create AV category (e.g., CASHUP_NOTE) with values
- Set preference to category name
- Verify dropdown appears in cashup modal
- Clear preference - verify textarea shown
4. Test cashup preview:
- Start cashup on register
- Click "Preview cashup summary"
- Verify current totals displayed
- Add transaction and re-preview - verify updated
- Complete cashup - verify final summary
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch implements an optional two-phase cashup workflow for point
of sale operations, along with improved error handling and validation.
Two-Phase Cashup Workflow:
- Staff can initiate cashup (CASHUP_START) and complete later
- Prevents new transactions on registers with cashups in progress
- Tracks cashup sessions with start/end timestamps
- Supports both quick cashup (immediate) and staged cashup workflows
- Updates batch cashup workflow operations across multiple registers
Error Handling & Validation:
- Centralized exception handling for cashup operations
- Clear validation messages for missing parameters
- Prevents cashup when no cash transactions exist
- Improved handling of zero/negative amounts
- Informative error messages for user guidance
Negative Amount Support:
- Supports negative cashup amounts for cashup deficits
- Automatic detection when actual cash is less than expected amount
- Creates appropriate CASHUP_DEFICIT records
- UI handles both positive and negative reconciliation amounts
- Updated calculations and display logic throughout
Modal Refactoring:
- Eliminates code duplication between register.tt and registers.tt
- Centralized cashup modal functionality in cashup_modals.js
- Shared confirm_cashup.inc and trigger_cashup.inc templates
- Consistent user experience across all cashup workflows
- Improved maintainability and code organization
Backend changes (Koha::Cash::Register):
- start_cashup(): Creates CASHUP_START action to begin session
- cashup_in_progress(): Detects active cashup sessions
- add_cashup(): Enhanced with two-phase completion support
- outstanding_accountlines(): Respects cashup session boundaries
Frontend changes:
- Interactive modals for starting and completing cashups
- Real-time calculation displays
- Support for reconciliation with actual amounts
- Batch operations UI for multiple registers
- Responsive error messaging and validation feedback
Test plan:
1. Apply patches and restart services
2. Run prove t/db_dependent/Koha/Cash/Register.t
3. Run prove t/db_dependent/Koha/Cash/Register/Cashup.t
4. Test single register workflows:
- Start cashup, add transactions, verify blocked
- Complete cashup with reconciliation amounts
- Test with positive, negative, and zero amounts
5. Test multi-register workflows:
- Select multiple registers on registers page
- Perform batch cashup operations
- Verify each register processes correctly
6. Test error conditions:
- Attempt cashup with no transactions
- Attempt cashup with invalid amounts
- Verify clear error messages displayed
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The previous follow-up restored warn in Object::store but missed
several test files that trigger DB exceptions via ->store(). These
tests now fail Test::NoWarnings because the warning is emitted but
not expected.
This patch adds warning_like wrappers to 15 additional test files,
asserting that the expected DB exception warning is emitted.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/api/v1/biblios.t \
t/db_dependent/Koha/Holds.t \
t/db_dependent/api/v1/return_claims.t \
t/db_dependent/api/v1/libraries.t \
t/db_dependent/Koha/Booking.t \
t/db_dependent/api/v1/patrons.t \
t/db_dependent/SIP/Message.t \
t/db_dependent/Koha/Patron.t \
t/db_dependent/Koha/Acquisition/Fund.t \
t/db_dependent/api/v1/acquisitions_orders.t \
t/db_dependent/Koha/ILL/Requests.t \
t/db_dependent/Koha/Patron/Relationship.t \
t/db_dependent/Koha/Hold.t \
t/db_dependent/Circulation/Returns.t \
t/db_dependent/api/v1/bookings.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Bug 42391 removed the warn from Koha::Object::store when it moved
exception translation to the exception_action hook. This lost useful
debugging information from the Plack error logs.
This patch restores the warning by adding a try/catch in store() that
stringifies and warns the exception before rethrowing. The warning now
contains the translated Koha exception message (e.g. 'Broken FK
constraint') rather than the raw DBI error, since translation happens
before the exception reaches store().
Note: delete() does not warn. Ideally the logging would live in the
ExceptionMapper itself (via an injected logger), covering all code paths
uniformly. That is a larger refactor tracked separately.
Tests that assert on these warnings are updated to match the new
message format.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Object.t \
t/db_dependent/Koha/Objects.t \
t/db_dependent/Koha/Schema/ExceptionMapper.t \
t/db_dependent/Koha/Item.t \
t/db_dependent/Koha/Suggestions.t \
t/db_dependent/Koha/Account.t \
t/db_dependent/Koha/ApiKey.t \
t/db_dependent/Koha/Checkouts/ReturnClaim.t \
t/db_dependent/Circulation/MarkIssueReturned.t \
t/db_dependent/Suggestions.t \
t/db_dependent/Biblio.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Bug 41604 made every page do a bit more work when it loads. This
Selenium test didn't wait for that work to finish before typing into
the search box, so sometimes it searched too early and got the wrong
results.
This adds a short wait after each page load so the test only searches
once the page is actually ready.
Introduced by: Bug 41604: Save user decision per column when visibility changes
Test plan:
prove t/db_dependent/selenium/patrons_search.t
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Warning: The allowCypressEnv configuration option is enabled. This allows any browser code to read values from Cypress.env(). This is insecure and will be removed in a future major version.
1. Replace Cypress.env() calls with cy.env() (for sensitive values) or Cypress.expose() (for public configuration)
2. Set allowCypressEnv: false in your Cypress configuration to disable Cypress.env()
Learn more: https://on.cypress.io/cypress-env-migration
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: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The Koha::Schema::Util:: namespace adds unnecessary nesting for a
single-purpose class. This follow-up moves it to
Koha::Schema::ExceptionMapper, which sits alongside Koha::Schema
itself — a better fit given it's tightly coupled to the Schema's
exception_action hook.
Changes:
- Koha::Schema::Util::ExceptionTranslator → Koha::Schema::ExceptionMapper
- Update Koha::Schema to use the new module
- Move test file accordingly
- Remove now-empty Koha/Schema/Util/ directory
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Extend the ExceptionTranslator to handle two additional MySQL error
patterns:
- NOT NULL violations ('Column X cannot be null' and 'X doesn't have
a default value') → Koha::Exceptions::Object::NotNull
- Not in database errors → Koha::Exceptions::Object::NotInStorage
Add Koha::Exceptions::Object::NotNull exception class with a property
field identifying the offending column.
Add Koha::Exceptions::Object::UnhandledDBError as a catch-all for DBI
errors that don't match any known pattern. This closes the gap where
unmatched MySQL errors could leak as DBIx::Class::Exception to callers
(e.g. templates that no longer catch that type).
The exception_action handler now distinguishes three cases:
- Koha exception objects (from txn_do re-throws): passed through via
'die $msg' to preserve the original exception type. Without this,
txn_do's rollback-and-rethrow would stringify the Koha exception
and wrap it in UnhandledDBError, losing the specific type.
- DBI errors (prefixed 'DBI Exception:'): translated if possible,
otherwise wrapped in UnhandledDBError.
- DBIC internal errors (e.g. 'Not in database'): translated if
possible, otherwise thrown as DBIx::Class::Exception.
Note: NotInStorage translation works for update operations but not for
delete-on-detached-Row where DBIC bypasses the Schema's exception_action.
Those cases remain DBIx::Class::Exception.
Update tests:
- OAIServers.t, Z3950Servers.t: assert NotNull with property check
- Hold.t: assert NotNull, remove warn suppression hack
- Objects.t: update-not-in-storage cases assert NotInStorage
- Upload.t: delete-not-in-storage stays DBIx::Class::Exception
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The 'Delete a patron that cannot be deleted' test (case 3) was using a
stale $r_us object from case 2 (not in storage), so it was not actually
testing FK constraint deletion. The assertion on DBIx::Class::Exception
passed by accident.
This follow-up:
- Fetches a fresh Koha::Patron object for case 3
- Clears $e_us/$e_them before the new test
- Asserts Koha::Exceptions::Object::FKConstraintDeletion, which is what
exception_action now produces for FK deletion errors
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
DBIx::Class provides an exception_action hook that intercepts every
throw_exception call before it becomes a DBIx::Class::Exception. By
installing this hook on Koha::Schema via a connection() override, all
database exceptions are automatically translated into Koha exceptions
at the source.
This eliminates the need for translate_exception catch blocks in every
caller:
- Remove try/catch from Koha::Object::store
- Remove try/catch from Koha::Object::delete
- Remove try/catch from Koha::Item::add_to_bundle
- Remove translate_exception convenience method from Koha::Schema
- Remove safe_do (already removed in previous follow-up)
ExceptionTranslator is refactored to work on plain error message strings
instead of DBIx::Class::Exception objects, since exception_action
receives the message before wrapping.
Unmatched errors fall through to DBIx::Class::Exception->throw() as
the default, preserving backward compatibility.
The warn $_->{msg} that was in Object::store is no longer needed since
exceptions are translated before they propagate. Tests that asserted on
that warning (via warning_like) are updated to use throws_ok directly.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t
=> SUCCESS: Tests pass!
k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
k$ prove t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
k$ prove t/db_dependent/Koha/Patron/Modifications.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Bug 21820 made the Zebra queue update conditional on the new
ElasticsearchEnableZebraQueue system preference, which defaults to off.
The 'basic_quote_processing' subtest in t/db_dependent/Koha/EDI.t counts
the log lines process_quote emits, and two of those are the
"specialUpdate biblioserver" traces that ModZebra logs only when the
Zebra queue path runs. When the SearchEngine preference is set to
Elasticsearch, that path no longer runs by default, so the subtest now
records 6 log lines instead of 8 and fails:
# Failed test '8 log lines recorded for passed quote file'
# got: '6'
# expected: '8'
# Failed test 'Trace recorded biblioserver index'
Test Plan:
1) Set the SearchEngine system preference to 'Elasticsearch'
2) prove t/db_dependent/Koha/EDI.t
3) Note the basic_quote_processing subtest fails with "got: '6', expected: '8'"
4) Apply this patch
5) prove t/db_dependent/Koha/EDI.t
6) Note the tests pass!
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
cy.get("#report_builder").contains("Submit").click();
cy.intercept("GET", "/api/v1/erm/eUsage/monthly_report/*").as(
"reportRequest"
);
cy.wait("@reportRequest");
We should intercept before the click.
Test plan:
yarn cypress run --spec t/cypress/integration/ERM/UsageReports_spec.ts
Run it several times and confirm that it always passes
The failure was:
<testcase name="Custom reports Should show the Display by year filter when filter is required" time="0.000" classname="Should show the Display by year filter when filter is required">
<failure message="Timed out retrying after 10000ms: `cy.wait()` timed out waiting `10000ms` for the 1st request to the route: `reportRequest`. No request ever occurred.
https://on.cypress.io/wait" type="CypressError"><![CDATA[CypressError: Timed out retrying after 10000ms: `cy.wait()` timed out waiting `10000ms` for the 1st request to the route: `reportRequest`. No request ever occurred.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
1) Set system preference 'PatronDuplicateMatchingAddFields' to empty (no fields ticked)
2) Go to <staff_url>/cgi-bin/koha/members/memberentry.pl?op=add and fill in the mandatory fields for a brand new patron (one that definitely doesn't exist yet)
3) Click 'Save'
4) Notice you get the 'Duplicate patron record?' dialog, even though no patron like this exists
5) Apply the patch. Restart plack
6) Repeat steps 2-3 with another new patron
7) Confirm the patron is created fine this time, no false duplicate error
8) Also try setting 'PatronDuplicateMatchingAddFields' to a field (e.g. 'city') and submit a new patron with that field left blank - confirm no false duplicate there either
9) Run:
prove t/db_dependent/Koha/Patrons.t
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The bug this fixed only manifests when the library's configured
timezone (C4::Context->tz) is not UTC: Koha::Object::_recursive_fixup
converts an incoming RFC3339 instant to that timezone before storing
it in the timezone-naive 'bookings.start_date'/'end_date' columns, so
a UTC-anchored boundary shifts by the timezone offset on write.
None of the existing Cypress specs catch this, because they never run
with the browser/test-runner timezone diverging from the configured
library timezone - both always come from the same container clock.
Add a Perl-level API test that mocks C4::Context->tz to a non-UTC zone
(America/New_York) and asserts the boundaries sent by place_booking.js
round-trip to the correct library-local date, pinning this regression
independently of any particular environment's timezone.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When there is a filter with the "status" param, we add a validation step
to secure the use of such a filter.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Note that t/Makefile.t and xt/check_makefile.t were doing the same test.
Merging them.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
$Test::Strict::TEST_STRICT = 0 is only set for all_perl_files_ok
It's no longer used since bug 42126
Test plan:
prove t/db_dependent/00-strict.t
should still return green
Remove a use Modern::Perl for mainpage.pl
=> Should still return green
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Bug 40866 introduced JSON logging for HOLDS CREATE but stored null for
'confirmations' and 'forced' when no overrides were present. This patch
mirrors the earlier change to CIRCULATION ISSUE by defaulting them to
empty arrays, keeping the two log streams structurally identical and
simplifying reporting queries.
Changes:
C4/Reserves.pm:
- Default confirmations/forced to [] in the AddReserve log payload.
t/db_dependent/Reserves.t:
- Update the Bug 40866 subtest to assert the empty-array defaults on
normal holds, and fix a 'my $log_data' redeclaration.
tools/viewlog.pl:
- Extend the CSV export JSON-decode block to also cover HOLDS logs,
guarding the item lookup on itemnumber being present (HOLDS payloads
may lack one) and filling the CSV item columns from the decoded
payload. The legacy bare-itemnumber fallback is restricted to
CIRCULATION so a legacy HOLDS hold_id is not mistaken for an
itemnumber.
koha-tmpl/intranet-tmpl/prog/js/viewlog.js:
- Extend the info column override rendering (renderCircInfo) to HOLDS
entries so forced/confirmations arrays display translated, exactly as
they do for CIRCULATION. The log viewer moved to an ajax datatable in
bug 29016, so this replaces the earlier json_found change to
viewlog.pl/viewlog.tt.
misc/maintenance/migrate_action_logs_to_json.pl:
- Extended to handle legacy pre-Bug 40866 HOLDS CREATE logs where info
is a bare hold_id, recovering branchcode / biblionumber / itemnumber
from old_reserves / reserves.
- Uses the same keyset-pagination pattern as the circulation loop so
rows are not skipped under --commit.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Previously, CIRCULATION ISSUE logs would store just the itemnumber when
there were no confirmations or forced overrides, but would store a full
JSON structure when overrides were present. This inconsistency makes
reporting difficult as queries need to handle both formats.
This patch changes AddIssue to always store logs in JSON format,
matching the pattern already used in AddRenewal. The JSON structure
now consistently includes:
- issue: issue_id
- branchcode: branchcode
- itemnumber: itemnumber
- confirmations: array of confirmation codes (empty array if none)
- forced: array of forced override codes (empty array if none)
Benefits:
- Consistent data format simplifies reporting and analysis
- Matches existing RENEWAL logging pattern
- Display code (tools/viewlog.pl) already handles both formats,
so backward compatibility is maintained
Test plan:
1. prove t/db_dependent/Circulation.t
2. Perform a normal checkout - verify action log contains JSON
3. Perform a checkout with overrides - verify confirmations/forced
are populated in the JSON
4. Check Tools > Log viewer - both types should display correctly
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Example: supplier_report on the basket page
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
- Modify column-visibility.dt event handler to store the visibility
preference of the user
- Improve _dt_force_visibility() to correctly pick the correctly visibility value depending
on visibility_condition, is_hidden, and the user preference
- Add _dt_get_expected_column_visibility() to guess what's the expected visibility
- Force boolean true/false (vs 1/0) using !!
- Rely on a global variable kohaTableUserChoices to store the user
choice. It will be part of the state and store in localStorage.
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
So we protect other calls
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The previous two commits on this bug fixed two ordering assumptions in
the RETRY loop of _allocateWithTransportCostMatrix(): resetting $inf at
the start of each attempt, and splicing unallocated task indices out of
@requests in descending order (removing a lower index first would shift
the position of any higher index still waiting to be removed).
This is a third instance of the same underlying problem, a few lines
earlier in the loop that compacts the working cost matrix $m before
that splice runs. @unallocated is built by iterating over agents (item
rows), pushing the column index Munkres assigned to each row that
turned out infeasible - and Munkres' row-to-column assignment is a
permutation, not a monotonic function of the row index, so
@unallocated is no more reliably sorted here than it was for the
splice.
That compaction loop scans matrix columns in ascending order and
advances a single pointer into @unallocated, expecting to meet each
entry in that same ascending order. When @unallocated arrives out of
order, the pointer never catches up: columns that should have been
dropped are left in place, and the columns after them are never
shifted left to fill the gap. From that point on, $m no longer lines
up with @requests, which *is* correctly reduced by the splice - so on
the next retry pass a hold's transport cost gets evaluated using a
different hold's matrix row. Depending on the exact misalignment, this
can silently drop a fillable hold from the queue while a genuinely
available item sits unused, or fill a hold via the wrong-cost item.
It didn't show up in the "many holds" regression test added earlier on
this bug, because that test's transport cost matrix is uniform (every
disabled pair costs the same); this defect only surfaces once the
columns competing for a scarce item have distinct costs.
Sort @unallocated once, ascending, and use that copy for the
compaction pass; reverse it for the splice, which needs descending
order.
Test plan:
1. Run the new t/db_dependent/HoldsQueue/RetryAllocationOrdering.t -
it calls _allocateWithTransportCostMatrix() directly with a small
hand-built scarce transport cost matrix and a pinned srand() (the
specific item-to-branch permutation needed to trigger the bug
depends on the shuffle() inside the routine). Reverting just this
patch makes 3 of its 6 subtests fail: the L2 pickup hold is dropped
even though an available item could have filled it.
2. Run t/db_dependent/HoldsQueue/TransportCostOptimizations.t to
confirm no regression in the existing retry-loop coverage.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
InstallPlugins forces every discovered plugin class to be deleted and
re-required from disk on every call, so plugin_methods reflects the
.pm file as it currently exists rather than whatever was already
loaded into a long-lived Plack worker. That deletion used
Symbol::delete_package, which clears the target package's whole
symbol table -- including any already-loaded packages nested under it
(e.g. a REST API controller a plugin bundles alongside itself, such as
Koha/Plugin/Foo/Controller.pm). Those nested classes keep their own
%INC entry, so they are never re-required afterwards and are left
permanently blanked out for the rest of the worker's life.
This was caught when running
`prove -r t/db_dependent/Koha/Plugins/ t/db_dependent/Koha/REST/Plugin/`
on top of the original patch. Tests fail several files that have nothing
to do with the bug being fixed (Biblio_and_Items_plugin_hooks.t,
Holds_hooks.t, TemplateIncludePathHook.t, REST/Plugin/PluginRoutes.t),
because Koha::Plugin::Test::Controller (a bundled test-fixture
controller living in t/lib/plugins/Koha/Plugin/Test/Controller.pm)
gets its symbol table wiped as collateral damage every time
Koha::Plugin::Test is reloaded.
_forget_plugin_class() replaces the delete_package call with the same
technique used by the CPAN module Class::Unload (not added as a
dependency here, just the technique): only clear the target class's
own symbols and reset its @ISA, explicitly leaving any nested package
entries in its symbol table alone. Module::Pluggable discovers bundled
nested classes as their own entries in the plugin list regardless, so
they still get reloaded correctly on their own turn.
I also annotate the no strict 'refs' block added by the previous commit
with a '## no critic' justification, as flagged by koha-qa.pl.
Test plan:
1. Confirm the bug exists without this patch:
a) git checkout <previous commit> (drop this commit, keeping the
first 3 commits of the bug applied)
b) Inside KTD:
prove -r t/db_dependent/Koha/Plugins/ t/db_dependent/Koha/REST/Plugin/
c) Observe failing files unrelated to this bug's own tests:
Biblio_and_Items_plugin_hooks.t, Holds_hooks.t,
TemplateIncludePathHook.t and REST/Plugin/PluginRoutes.t.
2. Confirm the fix:
a) Re-apply this commit
b) Re-run the same command from 1(b) -- all tests now pass
3. Confirm the original bug this branch set out to fix is still fixed,
both via the manual UI steps from the first commit's test plan
(install Batch Permission Modifier Plugin 1.3.1, upload 1.4.1 over
it via Tools > Plugins, confirm the renamed method takes effect
without restarting Plack) and via:
prove t/db_dependent/Koha/Plugins/Plugins.t
(see the 'InstallPlugins() reloads stale symbol tables tests'
subtest)
4. Confirm QA tooling is clean:
koha-qa.pl -c 4
=> PASS for Koha/Plugins.pm and all touched test files, no critic
or tidiness failures.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The FK-chain resolution added in the previous patch only handles ONE
level of nesting: it requires $parent_alias to be a direct relationship
of $main_source. This holds for every +count embed currently declared
in api/v1/swagger/ (all exactly one level deep, e.g.
biblio.items+count), but if that assumption is ever violated - a
deeper nested +count, a parent relation with a coderef (custom join)
condition, or no matching FK column name - the previous code silently
fell back to the pre-fix expression ($parent_alias.$fk_self), which can
reference a join alias that isn't available inside DBIC's limiting
subquery, reproducing the original 500 with no diagnostic.
This patch makes _build_count_subquery decline to build a subquery
(return undef) when the correlation can't be safely resolved, instead
of emitting SQL that may be broken. The caller already has an existing,
tested degradation path for "no DBIC relationship info" (prefetch the
association directly; to_api falls back to the Perl-level ->count call;
sorting on it is rejected with a clean 400) - unresolved nested
correlations now go through that same path rather than risking a 500.
Also:
- Extract the repeated "_resultset vs _result" result-source lookup
into a shared _result_source() helper (was duplicated in
dbic_merge_prefetch and _build_count_subquery).
- Document the one-level-deep limitation in _build_count_subquery's POD.
- Add a regression test pinning the 2-level-nested-+count case: it must
fall back to a plain prefetch (no COUNT subquery) rather than error
or emit unsafe SQL.
Test plan:
1. ktd --shell
2. prove t/Koha/REST/Plugin/Query.t t/db_dependent/Koha/Object.t \
t/db_dependent/Koha/REST/Plugin/Objects.t \
t/db_dependent/api/v1/acquisitions_orders.t \
t/db_dependent/api/v1/acquisitions_vendors.t \
t/db_dependent/api/v1/ill_requests.t \
t/db_dependent/api/v1/tickets.t \
t/db_dependent/api/v1/patrons.t
=> All pass, no regressions.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
When nested +count embeds (e.g. biblio.items+count) are used alongside a
has_many prefetch (e.g. biblio.suggestions or the implicitly added
biblio.biblioitem) and pagination, DBIC's limiting subquery optimization
places +select scalar refs in the inner subquery where the parent join
alias (e.g. 'biblio') is not yet available.
This patch fixes _build_count_subquery to resolve the correlation back to
the main table (me) via the FK chain. Instead of generating:
(SELECT COUNT(*) FROM items
WHERE items.biblionumber = biblio.biblionumber)
It now generates:
(SELECT COUNT(*) FROM items
WHERE items.biblionumber = me.biblionumber)
This works because me.biblionumber IS the FK to biblio, holding the same
value. The main table columns are always available regardless of DBIC's
query structure.
Changes:
- Add $main_source parameter to _build_count_subquery for FK resolution
- Thread main_source through _parse_prefetch and dbic_merge_prefetch
- When parent_alias != 'me', resolve through parent relationship condition
- Update Query.t assertion to match the new correlation expression
Test plan:
1. Apply the regression tests from the previous commit
2. Run:
$ ktd --shell
k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: The Bug 42995 subtest fails with 500 errors
3. Apply this patch, repeat from step 2
k$ prove t/Koha/REST/Plugin/Query.t \
t/db_dependent/Koha/Object.t \
t/db_dependent/Koha/REST/Plugin/Objects.t \
t/db_dependent/api/v1/acquisitions_orders.t \
t/db_dependent/api/v1/acquisitions_vendors.t \
t/db_dependent/api/v1/ill_requests.t \
t/db_dependent/api/v1/tickets.t \
t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
4. Sign off :-D
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds regression tests for the 500 Internal Server Error
triggered when GET /api/v1/acquisitions/orders is called with nested
+count embeds (e.g. biblio.items+count) combined with a has_many
prefetch (e.g. biblio.suggestions.suggester) and pagination.
The root cause is that DBIC's limiting subquery optimization places
+select scalar refs in the inner subquery where the parent join alias
(biblio) is not yet available, resulting in:
DBD::mysql: Unknown column 'biblio.biblionumber' in 'WHERE'
The regression was introduced in:
commit e51334604f
Bug 41950: Make +count embeds sortable via SQL COUNT subqueries
Test plan:
1. Apply this patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: The new subtest fails with 500 errors instead of 200
3. Sign off :-D
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
[FAIL] t/db_dependent/Koha/Plugins/C3.t
FAIL file_permissions
File must have the exec flag
FAIL test_no_warnings
'use Test::NoWarnings' must be present in this test file.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
If a patron is deleted and there's a checkout associated with them, an api call to {url}/api/v1/checkouts?checked_in=true
will result in an error. This patch updates the api to allow for a null value to be returned for patron_id
To test:
Before applying the patch:
1. In Administration->System preference enable the RESTBasicAuth system preference.
2. Checkout an item to a patron (I used item barcode: 39999000000252 and patron barcode 23529000121682)
3. Check the item back in
4. In your brower go to {sandbox/ktdurl}/api/v1/checkouts?checked_in=1
5. Observe there is a patron ID in the JSON response
6. Delete the patron
7. Repeat step 4
8. Observe an error because a "null" value was returned
9. Apply patch
Sandbox:
In sandbox management Actions->Apply patches
Once complete Actions->Build CSS and JS
Actions->Restart services
KTD:
ktd --shell
git bz apply 41889
yarn build
restart_all
10. Repeat step 4
11. Observe the checkout loads, but now has a null patron id.
12. Sign off
On KTD can also run the script: qa
to run tests.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The tests were lacking a 'real' scenario proving the
sorting on a _counts works at the DB level
prove t/Koha/REST/Plugin/Query.t
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Adds DBIC relationship aliases so that +count embeds advertised in
the API specs are backed by actual database relationships, making
them sortable via the SQL COUNT subquery infrastructure.
New aliases:
- Borrower: checkouts (issues)
- Aqbookseller: baskets (aqbaskets), invoices (aqinvoices)
- Ticket: updates (ticket_updates)
- ErmEholdingsPackage: resources (erm_eholdings_resources)
- Biblio: holds (reserves)
Koha methods are updated to use the new relationship names directly,
following the one-liner style: Koha::Foo->_new_from_dbic( scalar ... )
Note: checkouts on Borrower and subscriptions on Aqbookseller already
had matching DBIC relationships. comments on Illrequest, requests on
IllBatch, and items on Biblio also already existed.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/Koha/REST/Plugin/Query.t \
t/db_dependent/api/v1/patrons.t \
t/db_dependent/api/v1/acquisitions_vendors.t \
t/db_dependent/Koha/REST/Plugin/Objects.t \
t/db_dependent/Koha/Biblio.t
=> SUCCESS: All tests pass
3. Verify sorting works:
GET /api/v1/patrons
x-koha-embed: checkouts+count
_order_by: -me.checkouts_count
=> SUCCESS: Patrons sorted by checkout count
4. Sign off :-D
Assisted-by: Sonnet 4.6 (Anthropic)
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add tests to verify that +count embeds produce +select/+as attributes
instead of a prefetch, enabling SQL-level sorting by count columns.
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
As with guided reports through the admin interface, this patch switches
EmailReport to use the same function for processing templates. This
gives patron emails sent via cronjobs access to the same variables as
other report locations.
Test plan:
1. Under Tools > Notices and Slips, create a new notice in the Reports
module. Choose a code and a name, and under Email assign a subject
and the following body:
Date: [% today %]
2. Under Reports, create a new report from SQL with a name and the
following query:
SELECT 1 AS borrowernumber, 'to@example.com' AS email
3. Run the following command from the base of Koha, replacing CODE with
the notice code created earlier, and ID with the report ID created
earlier:
misc/cronjobs/patron_emailer.pl -module report -from from@example.com -notice CODE -report ID
4. Verify that the report runs successfully, and the console output
includes "Date:" on a line with nothing following it.
5. Apply patch.
6. Run the following command from the base of Koha, replacing CODE with
the notice code created earlier, and ID with the report ID created
earlier:
misc/cronjobs/patron_emailer.pl -module report -from from@example.com -notice CODE -report ID
7. Verify that the report runs successfully, and the console output
includes "Date:" on a line with the current time following it.
8. Run `prove t/db_dependent/Reports/Guided.t`, and verify it passes.
Sponsored-by: Open Fifth
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Verify that the SMS fallback in ModSuggestion() respects EmailFieldPrimary
and EmailFieldPrecedence: a patron with smsalertnumber and an emailpro
address (but no borrowers.email) must receive the notice by email, not SMS.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Apply encode_text function to all text fields in EDIfact Order messages
to properly escape special characters (?, ', :, +) that could break
message parsing. Previously only bibliographic description fields were
escaped, but GIR segments containing collection codes, branch codes,
item types, call numbers, and other data were vulnerable.
Test plan:
- Verify all existing tests pass
- Added comprehensive tests for special character escaping in GIR segments
- Confirmed that fields containing ?, ', :, + are properly escaped
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The booking date picker tests fake "today" with cy.clock and create bookings on
hardcoded June 2026 dates. The modal fetches existing bookings from from
the api and that query uses the real current time, not the fake one in the browser.
Once the real date passes the hardcoded dates, the server stops returning those bookings (
they are now in the past ), so the modal sees no conflicts and assertions such as "the date
should have the leadDisable class" fail.
This patch anchors each test's "today" to the start of next month based on the current
date so the bookings always stay in the future and are returned no
matter when the test runs.
The 'any item' test also sets cy.clock before its second page reload
because the freeze does not survive the reload.
Test Plan:
1) On main run:
yarn cypress run --spec t/cypress/integration/Circulation/bookingsModalDatePicker_spec.ts
2) Note the test fails
3) Apply this patch
4) Run the command again
5) Note all tests pass!
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
SIP2 accounts/institutions are stored in the database and the SIP server is supposed to reload its configuration when they change.
In practice, after adding or editing a SIP2 account, most SIP worker processes keep serving the old configuration (e.g. new accounts get "SIP access not authorized") until the SIP server is restarted ( I tested multiple times over 30 minutes ).
There seem to be two issues:
* L1 cache flushed too late. process_request() calls _config_up_to_date() before Koha::Caches->flush_L1_caches(). The SIP parent reads sip2_resource_last_modified into its own L1 cache at startup and never flushes it. Every worker inherits that stale L1, so a worker's first connection compares against the startup-era value instead of the current shared value.
* sip2_config_read_timestamp is stored in the shared (memcached) cache, but the configuration it tracks is per-process. When one worker reloads the config from the database, it bumps the shared timestamp, so every other worker then evaluates as "up to date" and never reloads its own stale in-memory config.
Test Plan:
1) Configure the SIP server with several worker processes (e.g. set
min_servers and max_servers to 5) and confirm an existing SIP2
account works
2) Add a new SIP2 account in the staff interface
3) Without restarting the SIP server, test the new account with misc/sip_cli_emulator.pl
4) Note the login fails
5) Apply this patch
6) Restart all the things!
7) Add another new SIP2 account
8) Without restarting the SIP server test this new account with sip_cli_emulator.pl
9) Note that the new account works!
10) prove t/db_dependent/SIP/SIPServer.t
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Some item statuses can correspond to multiple authorised values: Not
for loan, Withdrawn, Lost and Damaged. Until now, only a single entry
could be selected, and filtering applied to all values at once.
This change introduces finer-grained filtering by exposing each
authorised value individually in the filter.
Test plan:
Assign multiple authorised values to items on a bibliographic record
for the four affected attributes: Not for loan, Withdrawn, Lost and
Damaged. Use the "Status" filter and verify that each value behaves as
expected.
Display note:
The current dropdown presentation is not ideal.
Suggestions for improvement are welcome, although avoiding optgroup
would be preferable to keep the scope small.
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
lib/fontawesome/css/fontawesome.min.css was used as an example for this
test, let's use datatables.min.css
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Unit tests
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
To test:
Test the cronjob purge_suggestions.pl
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
ConnectDuplicate does not appear to be used anywhere.
No test plan to provide here.
git grep connectDuplicate should return nothing
Since there is no cud-, it would at least have been broken for a year
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
MARC21 4xx/5xx $w is a control subfield with up to four positions;
only position 0 encodes the hierarchical relationship. The previous
regex required the whole subfield to equal g or h, so values such as
"g " or "gnna" would fall into the middle bucket and lose the
broader/narrower distinction.
This case is hypothetical and likely rare in practice (most catalogs
store a single character in $w), but the correctness improvement still
makes sense and aligns with how BuildSummary reads the same subfield
elsewhere via substr($w, 0, 1).
To test:
- prove -v t/db_dependent/AuthoritiesMarc.t
- Verify BuildSummary/_marc21_sort_hierarchy_alpha subtest passes,
including the new multi-position $w case.
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Resolve critic warn:
"return" statement followed by "sort" at line 1238, column 5. Behavior is undefined if called in scalar context. (Severity: 5)
Tidied with lastest perltidy version.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Test plan:
Run t/db_dependent/AuthoritiesMarc.t
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch adds autoMemberNumValue to the Patrons system preferences
page immediately below autoMemberNum, so superlibrarians can view and
correct a runaway counter without needing vendor support.
The entry carries a caution note explaining:
- the value is managed automatically and should not normally be changed
- Koha generates the next cardnumber from max(counter, live_db_max),
so lowering the counter only has effect once the patron responsible
for the inflated number has been deleted or had their cardnumber
corrected first
- the risk of setting it too low: potential re-issuance of a previously
used cardnumber, which may grant a new patron access to third-party
services linked to the original holder
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add new tests to assert the usage of the new ILL_BATCHES action_logs modulename.
Update existing tests to utilize real log_something instead of mocking it.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
These highlight why we may have made this choice. With only 'edit_borrowers' you can make
changes, but you cannot view your changes as you aren't allowed to see other borrowers.
Test now pass, but not sure if this answers the question of why we did it this way, or if there is more
work here. Should you be able to get back the fields that you updated at the least, to confirm your changes? Or
should we send a success with a message that you can't see the data?
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Test plan:
1. Navigate to Reports > Create from SQL.
2. Create a new report with a name and the following SQL query:
SELECT 'test', 1, '2'
3. Run the report.
4. Under the Download dropdown, select "Open Document Spreadsheet".
5. Verify a ".ods" file is downloaded.
6. Open the downloaded spreadsheet.
7. Verify the two number-like columns are handled as such. This depends
on your choice of spreadsheet software:
- In Excel, the numbers should not have a leading single quote in the
edit-bar when selected.
- In Google Sheets, the numbers should be right-aligned, and
selecting both of them should display their sum in the bottom
toolbar.
8. Run `prove t/Koha/Util/OpenDocument.t` and verify all tests were
successful.
Sponsored-by: Open Fifth
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
To test:
1. prove t/db_dependent/MarcModificationTemplates.t
Observe test failing with the following error
t/db_dependent/MarcModificationTemplates.t .. 1/135 Control fields (generally, just tags below 010) do not have subfields, use data() at /home/koha/Koha/Koha/SimpleMARC.pm line 490.
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Before this patch, control fields were not modifiable using this subroutine.
Control fields use character positions instead of subfields. When modifying
control fields, this patch makes the subroutine's parameter "subfield" to indicate
a character position. An example
Koha::SimpleMARC::update_field({
record => $record,
field => '008',
subfield => '2',
values => [09],
});
changes field 008 starting from character position 2 to contain values "09".
This replaces char pos 2 with "0", and char pos 3 with "9".
To test:
1. prove t/SimpleMARC.t
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add explicit `use JSON::PP ()` to Elasticsearch.pm and the test file
to make the dependency on JSON::PP::false visible rather than relying
on YAML::XS loading it as a side effect.
Extend the get_elasticsearch_mappings() test subtest to assert that
date_detection and numeric_detection are disabled at the mapping root.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch corrects the capitalization of the phrase "No Barcode" and
several other instances of incorrect capitalization.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
If import_batch_profiles is slow to respond, it'll trigger select.change() in stage-marc-import.tt and change the format to ISO2709 before the form is submitted, causing the error.
cypress run --spec t/cypress/integration/Tools/ManageMarcImport_spec.ts
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Testing plan:
- Set EmailPurchaseSuggestions to BranchEmailAddress
- Go to create suggestion on the staff side /suggestion/suggestion.pl?op=add_form
- Create a suggestion with Library: Any
- 500 error: inbound_email_address is not a method of undef
- Apply patch & repeat
- Suggestion created
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Rather than excluding the entire about.tt files from codespell scanning,
annotate only the specific lines containing the proper noun "Provence"
(SAN-Ouest Provence, a French library co-operative and Koha sponsor) with
an inline TT comment that codespell recognises as a per-line ignore.
This lets codespell continue to check the rest of each file for genuine
spelling mistakes, while still catching accidental "Provence" for
"Province" in any other template not carrying this annotation.
Replaces the file-level exceptions added to Koha::Devel::Files in
favour of the more surgical inline approach.
Test plan:
1. Apply patch
2. Verify "Provence" on the annotated lines produces no codespell output:
codespell -d --ignore-words .codespell-ignore \
koha-tmpl/intranet-tmpl/prog/en/modules/about.tt
=> no output
3. Verify the rest of about.tt is still checked by introducing a
deliberate typo elsewhere in the file and confirming codespell flags it
4. prove xt/author/codespell.t
=> All tests pass
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Correct genuine spelling errors identified by the codespell 2.4.x
full scan after the KTD container OS upgrade. Changes are in
comments, POD, strings, and template text across 55 files.
"SAN Ouest Provence" (the French library co-operative and Koha
sponsor) is preserved as a proper noun. Perl copyright lines
receive an inline # codespell:ignore annotation; about.tt files
(contributor attribution pages) are added to the Koha::Devel::Files
codespell exception list so that future uses of "Provence" as a
typo for "Province" are still caught in all other files.
Notable user-visible fixes:
- "submiting" -> "submitting" in authority and bibliographic merge screens
- "deleteable" -> "deletable" in patron exception messages
- "Acquistions" -> "Acquisitions" in reports dictionary
Test plan:
1. Apply patches
2. In the staff interface, go to Cataloguing > Merge records and
confirm the submit button reads "Submitting" (not "Submiting")
3. In the staff interface, go to Authorities > Merge and confirm
the same correction
4. Attempt to delete a patron with outstanding issues and confirm
the error message reads "deletable" (not "deleteable")
5. prove xt/author/codespell.t
=> All tests pass
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
cy.login() in beforeEach broke the test in two ways: the intranet staff session cookie bled into the OPAC request (HTTP cookies have no port scope in
browsers), and navigating from intranet→OPAC mid-test hit a cross-origin block in Cypress. The afterEach suffered the same race condition - the syspref POST fired before the login redirect completed.
Replace with cy.request(), which makes HTTP calls at the runner level without touching the browser's current origin. cy.visitOpac() is then the first
browser navigation, so no cross-origin issue arises. A three-step flow satisfies Koha's CSRF middleware:
1) GET the login page to obtain a session and its token
2) POST credentials to authenticate
3) POST to the svc endpoint.
This fixes the #barcode and #patronlogin not found issues
The CSRF token also requires encodeURIComponent() — base64 uses +, which a
URL-encoded form body decodes as a space, silently corrupting the token and causing the login to fail.
This resolves the 403 Forbidden issue
Everything must pass:
$ cypress run --spec t/cypress/integration/OPAC/SCO_spec.ts
as well as npm run cypress open (cypress ui)
ktd selenium:
$ perl /kohadevbox/misc4dev/run_tests.pl --run-only t/cypress/integration/OPAC/SCO_spec.ts
squash
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This fails without the fix patch in this set:
cypress run --spec t/cypress/integration/HttpClient_spec.ts
It passes with the fix
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Apply tests patch only, run:
yarn cypress run --spec t/cypress/integration/Tools/ManageMarcImport_spec.ts
Confirm it fails with the same error as Jenkins:
Timed out retrying after 10000ms: `cy.wait()` timed out waiting `10000ms` for the 2nd request to the route: `jobPoll`. No request ever occurred.
Apply fix patch. Run again.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
On Jenkins, SCO is not setup like it is in ktd after a reset_all
from insert_data.pl
251 sub configure_selfcheckout {
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Three fixes to make all 27 EdifactModal Cypress tests reliably pass:
1. insertData.js: fix currency FK constraint violation (use an existing
currency rather than creating an undefined one).
2. Koha/Edifact/File.pm: add missing use statements for File::Basename
and File::Temp so the Cypress task can load the module.
3. EdifactModal_spec.ts:
- Replace arbitrary cy.wait() calls with condition-based assertions
throughout (expand/collapse animations, Bootstrap modal state).
- Fix search timing: add cy.intercept + cy.wait("@ediMsg") in the
Search Functionality beforeEach so initializeSearch is wired before
input events fire; use invoke("val")+trigger("input") instead of
type() to bypass the 500ms debounce race.
- Fix modal close tests: attach a shown.bs.modal listener before
opening the modal so we can wait for Bootstrap's 300ms show
animation to complete (_isTransitioning → false) before calling
hide(). Without this wait, hide() is silently blocked when the Ajax
response is faster than the animation (common in KTD). Use
bootstrap.Modal.getInstance().hide() as the programmatic fallback
since data-bs-dismiss and keyboard events are unreliable in headless
Cypress/Electron.
- Fix edifact_interchange.js: remove duplicate &format=json appended
to URLs that already contain it.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Three issues caused the EdifactModal_spec.ts test suite to fail:
1. insertSampleEdifactMessages inserted a vendor with listprice='USD' and
invoiceprice='USD', but those columns have FK constraints against the
currency table and USD is not pre-populated in KTD's default install.
Drop the currency columns from the INSERT so they default to NULL.
2. Koha::Edifact::File::vendor() and basket() called
Koha::Acquisition::Bookseller and Koha::Acquisition::Basket without
use-ing them. The edifactmsgs.pl table embeds both on every API call,
so a missing module caused a 500 and an empty table.
3. showMessage() already appended &format=json before delegating to
showMessageFromUrl(), which then appended it a second time. Remove the
redundant copy from showMessage().
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Adds a dedicated subtest covering the specific scenario: daily loans have a
due time of 23:59, so a return earlier in the day on a holiday would
previously cause days_between to exit its loop before processing the end
date, silently skipping the holiday subtraction.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The existing 'days: 289 hours, 2 holidays' test asserted 8-1=7 but its own
description named 2 holidays. The expected value matched the buggy behaviour
(loop exiting before the holiday end date) rather than the correct result.
Update to 8-2=6 to reflect the fix.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
In Koha, the sip2_resource_last_modified key is updated any time the SIP config is edited from the SIP2 editor in the staff interface ( institutions, accounts, system preference overrides ).
If sip2_resource_last_modified is missing from memcached, Koha SIP emits "Couldn't find sip2_resource_last_modified, considering configuration not up to date" in the logs, and fetches the entire configuration from the database, *for each request*.
This causes SIP performance to drop significantly. In my production testing with 83 institutions and 171 accounts the login message response time dropped from 0.283 seconds to 2.899 seconds.
I think the solution is pretty simple. If there is a cache miss on sip2_resource_last_modified, SIP should load the config from database as it does already, but then it should also set sip2_resource_last_modified!
Test Plan:
1) Apply this patch
2) Prove t/db_dependent/Koha/SIP2/SIP2ModuleMigration.t
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>
We must use t::lib::Dates::compare to compare dates in test.
Test plan:
Confirm that `prove t/db_dependent/Koha/Hold.t` still returns green
after this patch
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
prove t/db_dependent/Reserves.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>
An exception is not implemented as progress_request is called in
different circulation contexts (hold is placed, checkout happens,
etc) and those actions should not be dependent on the outcome of
progress_request, i.e. if progress_request fails due to schema
validation failure, the circulation action should happen as before.
prove t/db_dependent/Koha/ILL/ISO18626/Request.t
prove t/db_dependent/api/v1/iso18626/request.t
cypress run --spec t/cypress/integration/ILL/SupplyingRequests.ts
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Miranda Nero <mnero@oslri.net>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This also prevents clicking other action buttons while an action is midflight
cypress run --spec t/cypress/integration/ILL/SupplyingRequests.ts
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit applies changes to V1.pm to be able to utilize the same XML methods.
We're also updating tests to reflect this new functionality.
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
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>
Lari Taskula
Hammat Wele
Pedro Amorim
Laura_Escamilla
Martin Renvoize
Jonathan Druart
Kyle M Hall
David Cook
Sanjar Tulkinov Anvar o'g'li
Tomás Cohen Arazi
Paul Derscheid
Baptiste Wojtkowski
Casey Conlin
Cameron E. Tidd
Andrii Nugged
Lucas Gass
Olivia Reynolds
Emily Lamancusa
Matt Blenkinsop
Aleisha Amohia
Nick Clemens
Lisette Scheer
Janusz Kaczmarek
Marcel de Rooy
Owen Leonard
Eric Phetteplace
Brendan Lawlor
Andreas Roussos
Andrew Fuerste Henry
Michael Hafen