We are all lazy, we aren't going to type --password everytime, we should
protect 'p' and 'pass' as well
This could likely get broader too later
TO test:
1 - perl misc/devel/create_superlibrarian.pl --branchcode=CPL --categorycode=S --cardnumber 3 --userid=ted3 --p=dog
2 - Note password is logged
3 - Apply this patch
4 - perl misc/devel/create_superlibrarian.pl --branchcode=CPL --categorycode=S --cardnumber 33 --userid=ted33 --p=dog
5 - Password redacted in log
6 - perl misc/devel/create_superlibrarian.pl --branchcode=CPL --categorycode=S --cardnumber 333 --userid=ted333 --pass=dog
7 - Password still redacted
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>
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>
Extends the INIT/END blocks added by Bug 42714 to also log non-cron
scripts (those using bare 'use Koha::Script') to the SCRIPTS action-log
module when the new ScriptLog system preference is enabled.
Also introduces _scrub_argv(), which redacts the value of any command-
line flag whose name contains a sensitive pattern (password, secret,
token, etc.) before the argument list is logged. Scrubbing applies to
both cron and non-cron scripts, covering both --flag value and
--flag=value forms. This prevents credentials passed on the command
line (e.g. misc/devel/create_superlibrarian.pl --password ...) from
appearing in the action log.
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>
Adds a logscriptaction() function parallel to cronlogaction() that logs
to the SCRIPTS action-log module rather than CRONJOBS, gated behind the
new ScriptLog system preference. Exported from C4::Log alongside the
existing cronlogaction().
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>
Adds a new YesNo system preference 'ScriptLog' in the Logging section
of system preferences, adjacent to CronjobLog. When enabled, the start
and end of non-cron Koha scripts (those using bare 'use Koha::Script'
rather than 'use Koha::Script -cron') will be recorded in the action
log under the SCRIPTS module.
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>
js:clean predates the OPAC islands build (bug 26355) and only removes
the intranet and Cypress dist directories, so stale chunks survive in
koha-tmpl/opac-tmpl/bootstrap/js/vue/dist/ across rebuilds.
- add the OPAC Vue dist directory to js:clean
Test plan:
1. yarn js:build, then apply this patch series and yarn js:clean
2. ls koha-tmpl/opac-tmpl/bootstrap/js/vue/dist/
=> empty, no stale staff-only chunks remain
3. yarn js:build
=> only islands.esm.js and a patron-self-renewal chunk are emitted
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Both islands builds share modules/islands.ts, which hardcodes every
island. The OPAC compilation therefore emits staff-only lazy chunks
(acquisitions-menu, vendor-menu, admin-menu) into
koha-tmpl/opac-tmpl/bootstrap/js/vue/dist/, and the OPAC islands.esm.js
bundles the staff Pinia stores.
- split modules/islands.ts into shared machinery and per-application
entry points (islands-intranet.ts, islands-opac.ts)
- declare each application's islands as a Map literal and feed it
through registerIsland(), the same path plugin islands take
- pass store definitions into hydrate() instead of importing every
store in the shared module
- keep the full plugin import surface (registerIsland, hydrate, Vue
re-exports) on both entries via star re-exports
- point the rspack islands entries at the per-application modules
Test plan:
1. yarn build
2. ls koha-tmpl/opac-tmpl/bootstrap/js/vue/dist/
=> only islands.esm.js and a patron-self-renewal chunk are emitted
3. ls koha-tmpl/intranet-tmpl/prog/js/vue/dist/
=> acquisitions-menu, vendor-menu and admin-menu chunks are still
present
4. In the staff interface, visit pages using the acquisitions, vendor
and admin menu islands and verify they render
5. Enable self-renewal for a patron's category and put the patron
inside the availability window, e.g.:
UPDATE categories SET self_renewal_enabled = 1
WHERE categorycode =
(SELECT categorycode FROM borrowers WHERE userid = 'koha');
UPDATE borrowers
SET dateexpiry = DATE_ADD(CURDATE(), INTERVAL 7 DAY),
debarred = NULL WHERE userid = 'koha';
6. Log into the OPAC as that patron and open 'your summary'
=> the patron-self-renewal chunk loads from
/opac-tmpl/bootstrap/js/vue/dist/ and the self-renewal modal
completes successfully
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Matt Blenkinsop <matt.blenkinsop@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This patch updates the cn_browser plugin output so that table rows are
highlighted using row classes and in colors which have sufficient
contrast.
To test, apply the patch and set your 952$o tag to use the cn_browser
plugin:
- Administration -> MARC bibliographic frameworks -> BKS -> MARC
Structure -> Tag 952 -> Edit subfields.
- Under the "Full call number" tab (o), set the "Plugin" dropdown to
"cn_browser.pl" and save your changes.
- Go to Cataloging and locate a bibliographic record using the
cataloging search.
- Add or edit items. NOTE: In the testing data there is only one item
with a callnumber and it is "FIC/kas"
- In the "o - Full call number" field, type "FIC" and then click the
plugin editor image.
- You should see the record with the "FIC/kas" call number and a
green row right above it (indicating where an item with
the call number "FIC" would be in the results).
- Revise your search using the form at the top, entering "FIC/kas".
- The record row should now be highlighted in pale yellow.
Sponsored-by: Athens County Public Libraries
Signed-off-by: Gretchen <maxeinergl@gcc.edu>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
_change_directory(), _list_files() and _is_connected() each called
$self->{connection}->pwd directly for their log payloads/connectivity
check, duplicating exactly what _current_directory() now provides as a
proper, documented internal accessor. Route them through it instead -
no behaviour change, just removing the duplication now that the
wrapper exists.
Test plan:
1. prove -r t/db_dependent/Koha/File/
=> All tests successful (32 tests), including the existing
current_directory() and list_files()/change_directory() coverage.
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>
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>
This patch updates the About page in the section for the Bridge Material
Type Icon Set. The link is updated to use the last valid saved version
on the Internet Archive.
Sponsored-by: Athens County Public Libraries
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>
Test plan:
1. Create an order with multiple items and receive the order in full
2. From the vendor record, click Receive shipments
3. You should be in parcels.pl and the table should show your shipments
4. The Items expected column will show 0 even though you ordered multiple items
5. Apply patch and restart_all
6. Refresh the page, the column should now show a value
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
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>
This change localises PERL5LIB and KOHA_HOME,
which means that any changes made to PERL5LIB or KOHA_HOME
within _do_instance() in koha-plack are confined to the specific
instance being worked on in that function.
See Bugzilla for steps on how to reproduce the problem.
Test plan:
- Apply the patch
- sudo vi debian/scripts/koha-plack
Within _do_instance after PERL5LIB is set, add the following:
echo $PERL5LIB
- sudo koha-create --request-db test1
- sudo ./debian/scripts/koha-plack --restart test1 test1 test1 test1
- Note that now PERL5LIB stays constant:
/usr/share/koha/lib:/usr/share/koha/installer:/usr/share/koha/lib/installer
- git restore debian/scripts/koha-plack
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>
Three searches pass large ID lists:
* Koha::BackgroundJob::BatchUpdateItem passes every itemnumber of the
batch, and batch item modifications can cover tens of thousands of
items
* Koha::Items::batch_update does the same with the modified
itemnumbers when collecting biblionumbers for reindexing
* Koha::Patrons::filter_by_safe_to_delete does it with the
borrowernumbers found safe to delete ( unlikely to approach
the same numbers as items/bibs, but still good to fix )
This patch switches all three to IN. The lists are still passed as
values, but the query stays a flat IN list the optimizer handles well
instead of a chain of ORs that can't be optimized.
Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Items/BatchUpdate.t
3) prove t/db_dependent/Koha/Patrons.t
4) Note all tests pass!
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
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>
On the hold request and the patron search pages, the search is not
deferred when a state exists.
Coming from
commit 2a34413601
Bug 41604: Save user decision per column when visibility changes
- if (default_save_state_search) {
+ if (default_save_state || default_save_state_search) {
$("#" + settings.nTable.id).data("loaded_from_state", true);
- } else {
+ }
+ if (!default_save_state_search) {
delete state.search;
state.columns.forEach(c => delete c.search);
I think this was a leftover from another try to fix bug 41604. I don't
see how it could help in the context of saving the user decision.
In the different patches from bug 41604 we don't test loaded_from_state.
Test plan:
Go to /cgi-bin/koha/members/members-home.pl
=> The table is not displayed
Search for patrons
=> The table is displayed
Reload the page
=> Without this patch the table is displayed
=> With this patch the table is not displayed
If you have "Save search state on page change" enabled for this table,
the table should be displayed if a previous search was made.
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: David Nind <david@davidnind.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Test plan:
1) Add multiple vendor payment method authorised values
2) Create a vendor and add multiple payment methods
3) Click to edit, the payment method will be displaying one option of a "|" concatenated string
4) Apply patch and run yarn js:build
5) Hard refresh the browser, the payment methods should now display correctly
6) Edit the payment methods and save, the saved changes should be correctly displayed on the vendor details page
7) Add a new vendor with multiple payment methods, it should save correctly
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>
Update the log viewer to change "Modify cardnumber" to
"Modify card number", as per the terminology guidelines
https://wiki.koha-community.org/wiki/Terminology#C
Test plan:
1. Edit a patron and change their card number under the library
management section.
2. Go to Tools > Additional tools > Log viewer.
3. Note under the actions section that there is an action:
Modify cardnumber
4. Click "Submit" to see the log viewer results.
5. Note that there is an entry for the patron card number change
with the action: "Modify cardnumber".
6. Apply the patch.
7. Refresh the log viewer page.
8. Note that "Modify cardnumber" is now "Modify card number"
in both places.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Gretchen <maxeinergl@gcc.edu>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
$errors is an array ref returned from a blank array variable
if( $errors ){ #this will be true
if( scalar @$errors ){ #only true when array has members
To test:
1 - tail -f /var/log/koha/kohadev/intranet-error.log
2 - Stage a marc file
3 - Note the error:
==> /var/log/koha/kohadev/intranet-error.log <==
[2026/07/23 19:11:37] [WARN] The following error(s) occurred during ISO2709 record import:
ERROR: Koha::ImportBatch::try {...} /kohadevbox/koha/Koha/ImportBatch.pm (105)
4 - Apply patch
5 - Restart all
6 - Stage a file
7 - No error!
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The account editor preselects "No" for "Holds get captured" when
creating a new SIP account, but the SIP checkin default is to capture
holds. This patch makes the editor default the option to "Yes" so a
new account behaves the same as one that never set the option.
Test Plan:
1) Apply this patch
2) yarn js:build
3) Restart all the things!
4) Create a new account in the SIP2 administration module
5) Note "Holds get captured" defaults to "Yes"!
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The migration from SIPconfig.xml to the database ( bug 37893 ) leaves
holds_get_captured NULL for any account that didn't define it in the
config file. At SIP checkin an unset holds_get_captured defaults to
on, but the REST API returns NULL boolean columns as false, so the
SIP account editor shows "No" selected. Saving the account without
touching the option then stores 0 and holds are no longer captured
at SIP checkin.
This patch changes the column default to 1 and sets existing NULL
values to 1 so the stored value matches the actual checkin behavior.
Test Plan:
1) If you have no SIP accounts, create one
2) Simulate an account migrated from a SIPconfig.xml that doesn't define holds_get_captured:
UPDATE sip_accounts SET holds_get_captured = NULL;
3) Edit the account, note "Holds get captured" shows "No" even though
holds *are* captured at SIP checkin while the value is unset
4) Apply this patch
5) Run updatedatabase.pl
6) SELECT login_id, holds_get_captured FROM sip_accounts;
7) Note holds_get_captured is now 1!
8) Edit the account again, note "Yes" is now selected for "Holds get captured"!
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
To test :
1. Apply the patch
2. Go to the OPAC advanced search page http://kohadev.mydnsname.org:8080/cgi-bin/koha/opac-search.pl
3. Click More options
4. Notice that all section titles now use colons
- "Search for:" = colon
- "Limit to any of the following:" = colon
- "Location and availability:" = colon
- "Sort by:" = colon
- "Publication range" = colon
- "Language" = colon
- "Audience" = colon
- "Content" = colon
- "Format" = colon
- "Additional content types..." = colon
5. Sign off and have an incredible day! :)
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The escape_str function in the OPAC requires the argument to be a string,
and will throw a fatal error if passed a Number. This is because
String.prototype.escapeHtml is created by Koha and is not a JS standard.
This patch converts the object to a string before applying escapeHtml.
To test, apply the patch and log in to the OPAC.
- If necessary, add a list.
- Search for records in the catalog and add one or more of the results
to your new list.
- Go to Lists -> Your lists.
- In the list of your lists, click the "Delete" link for the list you
added records to.
- You should see a confirmation modal, "Are you sure you want to
delete this list?" showing the name of the list and the number of
items on it.
- Clicking "Yes, delete" should delete the list.
Sponsored-by: Athens County Public Libraries
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Regenerated via `dbic --force` after adding the local_holdgroup_match
column to hold_fill_targets and tmp_holdsqueue in the previous patch.
Test plan:
1. Inspect Koha/Schema/Result/HoldFillTarget.pm and TmpHoldsqueue.pm
and confirm the new local_holdgroup_match accessor is defined
above the `DO NOT MODIFY` marker.
2. Confirm custom content below the marker (koha_object_class,
+item_level_request is_boolean) is preserved.
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>
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>
When LocalHoldsPriority is enabled, the holds queue picks a local-group
item for eligible holds. Until now there was nothing to reserve that
priority at checkin time: if a non-local item was returned first, it
would happily fill the hold and the local-priority decision was lost.
This patch adds a time-bounded exclusivity window so that, for a
configurable number of days after a hold is placed, a hold whose queue
target is a local-group item will not be filled by a non-local item.
Changes:
- C4/HoldsQueue.pm: in MapItemsToHoldRequests, flag %item_map entries
that matched on local library (GiveLibrary/GiveLibraryAndGroup) or
local hold group (GiveLibraryGroup/GiveLibraryAndGroup) with
local_holdgroup_match => 1, and propagate that into both the
tmp_holdsqueue and hold_fill_targets INSERTs.
- C4/Reserves.pm: in the CheckReserves loop, when
LocalHoldsPriorityExclusivityPeriod is set, the hold is still within
that window, and the item being evaluated is not itself a local
match, skip the reserve if its hold_fill_targets row has
local_holdgroup_match = 1. Missing hold_fill_targets row means "no
information" -- fall through so holds are never trapped between
queue runs.
- circulation.pref: expose the new LocalHoldsPriorityExclusivityPeriod
preference next to the existing LocalHoldsPriority* entries.
The feature is a no-op unless both LocalHoldsPriority is set to
something other than "Don't give" AND
LocalHoldsPriorityExclusivityPeriod is > 0.
Test plan:
1) Apply the patches and run the atomicupdate.
2) Set up a library group flagged as a local hold group containing
two libraries (call them A and B). Add a third library C that is
NOT in the group.
3) Add one item to each library (itemA, itemB, itemC) on the same
biblio.
4) Administration -> System preferences -> Circulation:
- Set LocalHoldsPriority to "Give library group".
- Set LocalHoldsPriorityPatronControl to "home library".
- Set LocalHoldsPriorityItemControl to "home library".
- Set LocalHoldsPriorityExclusivityPeriod to 7.
5) As a patron whose home library is A, place a title-level hold with
pickup at A.
6) Run the holds queue (misc/cronjobs/holds/build_holds_queue.pl).
-> The queue should target itemA (or itemB) for this hold.
-> Confirm in the database:
SELECT reserve_id, itemnumber, local_holdgroup_match
FROM hold_fill_targets;
The targeted row must have local_holdgroup_match = 1.
-> Holds to pull / holds queue report should also show the flag
(tmp_holdsqueue.local_holdgroup_match = 1).
7) At library C, check in itemC (the non-local item).
-> The hold must NOT be captured -- itemC simply goes back on the
shelf.
8) At library A, check in itemA (the local item).
-> The hold IS captured for the patron.
9) Cancel and re-place the hold, run the queue again. Manually age
the hold past the window:
UPDATE reserves
SET reservedate = DATE_SUB(CURRENT_DATE, INTERVAL 10 DAY)
WHERE reserve_id = <id>;
Now check in itemC again.
-> The hold IS captured (exclusivity window has expired).
10) Re-place the hold but do NOT run the queue. With no
hold_fill_targets row for the reserve, check in itemC.
-> The hold IS captured (we never trap holds between queue runs).
11) Flip LocalHoldsPriorityExclusivityPeriod back to 0 and repeat
step 7.
-> The hold IS captured; the feature is fully disabled.
12) Run:
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 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>
Adds groundwork for a time-bounded local holds exclusivity feature:
- Adds a local_holdgroup_match flag (tinyint, default 0) to both
hold_fill_targets (consulted by CheckReserves/_Findgroupreserve at
checkin) and tmp_holdsqueue (so queue/pull-list reports can display
it). The flag will be set by the queue builder when a local-group
match is made.
- Adds a new system preference LocalHoldsPriorityExclusivityPeriod
(Integer, default 0 = disabled) specifying the number of days from
hold creation during which only locally-targeted items may fill a
hold with a local queue target.
- Provides an atomicupdate that guards both ALTERs with column_exists
and INSERT IGNOREs the new syspref.
Test plan:
1. Apply atomicupdate: perl installer/data/mysql/updatedatabase.pl
2. Verify both columns exist:
SHOW COLUMNS FROM hold_fill_targets LIKE 'local%';
SHOW COLUMNS FROM tmp_holdsqueue LIKE 'local%';
3. Verify syspref row:
SELECT variable, value FROM systempreferences
WHERE variable = 'LocalHoldsPriorityExclusivityPeriod';
4. Confirm default value is 0 and type is Integer.
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>
As in opac-user.tt OD_password_required was replaced by Koha.prefs.OverDrivePasswordRequired. The script overdrive.js checks already for the Koha.prefs variable.
1) Set sysprefs: OPACOverDrive, OverDriveCirculation, OverDrivePasswordRequired
= Enable/Yes; OverDriveClientKey, OverDriveClientSecret, OverDriveLibraryID =
any non-empty value (e.g. 123).
2) Go to <opac_url>/cgi-bin/koha/opac-user.pl, click "Log in to your OverDrive
account" - password modal appears (correct).
3) Go to <opac_url>/cgi-bin/koha/opac-overdrive-search.pl?q=music, click the
same link, it fails with "No password provided" instead, no modal.
4) apply patch and repeat 3) The modal appears now.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
As OverDriveEnabled is declared via SET in the template and not a system preference and after all not used in that script it is removed.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
OpacHighlightedWords was set in quotation marks and resulted in either "1" or "0".
As "0" and "1" always evaluates to true in JS the following error was thrown when OpacHighlightedWords was set to Don't highlight: Uncaught TypeError: $(...).highlight is not a function
To test:
a) Set system preference OpacHighlightedWords to Don't highlight
b) search a title in the opac with open dev console and check that no error (like above) is thrown.
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Three issues introduced by the OPAC port of opac-results.tt:
- The tt-allowed block sets OverDriveEnabled via
`Koha.Preference("OverDriveEnabled")`, but OverDriveEnabled is a
TT-side SET variable (computed from OverDriveLibraryID +
OverDriveClientKey + OverDriveClientSecret), not a system
preference, so Koha.prefs.OverDriveEnabled is always "".
The code at line 621 then reads an unprefixed `OverDriveEnabled`
identifier which is not declared -- the OverDrive result block
never fires.
- The neighbouring block `if (OpenLibrarySearch) { ... }` reads an
unprefixed identifier for a value that is only exposed on
Koha.prefs, so the OpenLibrary result block also never fires.
- Both blocks build a throbber <img> with
`src=\`${interface}/${theme}/images/spinner-small.gif\`` inside a
single-quoted JS string; the backticks are literal characters
there, so the rendered src ends up as that literal template
text, not a usable URL.
Replace the OverDriveEnabled addPrefs entry with a plain
`const OverDriveEnabled = [% OverDriveEnabled ? 1 : 0 | html %];`
that reads the TT SET variable, prefix OpenLibrarySearch with
Koha.prefs, and rebuild the throbber src via normal string
concatenation.
To test:
- With OverDrive configured (OPACOverDrive, OverDriveLibraryID,
OverDriveClientKey, OverDriveClientSecret), run a search in the
OPAC: the OverDrive results panel shows the spinner and then
the API results.
- With OpenLibrarySearch enabled, the OpenLibrary panel does the
same.
- In both cases the spinner image loads (no broken-image icon).
- Disable both prefs -> neither panel is rendered, no console errors.
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Jan Kissig <bibliothek@th-wildau.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
In the new tt-allowed block, the NovelistSelect fallback declarations
redeclare `NovelistSelectProfile` instead of also exposing
`NovelistSelectPassword`:
var NovelistSelectProfile = "[% NovelistSelectProfile | html %]";
var NovelistSelectProfile = "[% NovelistSelectPassword | html %]";
So `NovelistSelectProfile` ends up holding the password, and
`NovelistSelectPassword` is never declared. The later
`novSelect.loadContentForQuery(..., NovelistSelectProfile,
NovelistSelectPassword, ...)` call then throws ReferenceError and
NovelistSelect content never loads on biblio detail pages.
Rename the second declaration to `NovelistSelectPassword` to match
the original template behaviour.
To test:
- Configure the NovelistSelect sysprefs (NovelistSelectEnabled,
NovelistSelectProfile, NovelistSelectPassword) in the OPAC. Real
NovelistSelect content only loads with valid EBSCO subscription
credentials -- with dummy values the JS path still runs but the
tab stays empty; that is still sufficient to verify this fix.
- Open an OPAC biblio detail page with a known ISBN.
- In the browser console, run
console.log(NovelistSelectProfile, NovelistSelectPassword)
and confirm both values are printed (before the fix,
NovelistSelectPassword is undefined).
- With valid subscription credentials, NovelistSelect content loads
in its tab/panel instead of throwing a ReferenceError in the
console.
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Jan Kissig <bibliothek@th-wildau.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The port declares `var print = [% print ? 1 : 0 | html %];` at the
top of the tt-allowed block, then later calls `window.print()` inside
`if (print) { ... }`. Declaring `var print` at top-level overwrites
the built-in `window.print` with 0 or 1, so the subsequent
`window.print()` call throws "print is not a function" and the print
view never fires its actual print dialog.
Rename the flag to `should_print` and update the single `if (print)`
check accordingly.
To test:
- Open a public list with items in the OPAC.
- Click "Print this list" (or load the page with op=print).
- The browser print dialog opens and the window closes afterwards.
- No "print is not a function" error in the console.
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Jan Kissig <bibliothek@th-wildau.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The OPAC port moved the dateformat pref into Koha.addPrefs and dropped
the local `var dateformat_pref = ...;` declaration, but four usages of
`dateformat_pref` remain in the same file (the debug alert, two
`switch` statements, and the get_dateformat_str call in validate_date).
At runtime the switches throw ReferenceError, so flatpickr ends up
with the default 'Y-m-d' format and date validation via validate_date
crashes before showing the invalid-date alert.
Replace all remaining occurrences with Koha.prefs.dateformat, which is
populated by the new addPrefs block at the top of the include.
To test:
- Set dateformat syspref to each of us / metric / dmydot / iso.
- Open any OPAC page using a flatpickr input (e.g. hold date, patron
dob on the registration form).
- Confirm the widget renders dates in the expected format and
entering an invalid string triggers the validate_date alert.
- No ReferenceError in the browser console on page load.
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Jan Kissig <bibliothek@th-wildau.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Jan Kissig <bibliothek@th-wildau.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Nick Clemens
Martin Renvoize
Paul Derscheid
Owen Leonard
Matt Blenkinsop
David Cook
Kyle M Hall
Jonathan Druart
David Nind
Pedro Amorim
Laura_Escamilla
Jan Kissig