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>
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>
This patch adds the ability to set an item type as magentic for SIP
response purposes.
Test plan:
1) Run included database updates
2) Navigate to admin > itemtypes
3) Edit/Add an itemtype and scroll to the 'Sip media types' input
4) A new 'Sip magnetic' checkbox should be available
5) Confirm setting/unsetting it works as expected
6) The SIP magnetic flag bit should respect the setting
7) To test SIP functionality:
a) Start the SIP server (e.g., koha-sip service)
b) Create an item with an itemtype that has sip_magnetic checked
c) Use misc/sip_cli_emulator.pl to send a checkout request:
perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su <sip_username> -sp <sip_password> -l <location_code> --patron <patron_cardnumber> --password <patron_password> --item <item_barcode> -m checkout
d) Check the SIP response to ensure it handles magnetic media correctly (e.g., no desensitization flag)
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>
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>
This patch adds the new sip_magnetic boolean flag field to the itemtypes
table to allow flagging the itemtype as magnetic for sip responses.
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>
The `magnetic_media` flag was never set, it can be set from
`sip_media_type` if it's present. `sip_media_type` is set via the
Authorized Values system.
Checkins will not be sorted correctly by Biblioteca without this flag.
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 updates CanBookBeIssued and CanItemBeReserved to use the
new policy class instead of calling GetAgeRestriction directly.
Test plan:
1. Run tests without this patch:
$ ktd --shell
k$ prove t/db_dependent/Circulation.t \
t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat step 1
=> SUCCESS: Tests pass!
4. 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>
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>
The checkboxes this callback listens to events on are conditionally
rendered, depending on whether the user has permission to edit each
item. When moving from a page with no checkboxes displayed to one with
checkboxes displayed, the checkbox elements may not exist at the time of
this callback, so the listener is not attached and the checkboxes are
inoperable.
This patch fixes this by using a delegated event handler on the row,
which doesn't depend on the checkbox existing at the time.
Test plan:
1. Enable the system preference EnableItemGroups.
2. Set up a non-super-librarian patron, with permission to:
- view the catalogue (catalogue)
- create update and delete item groups (manage_item_groups)
3. Place this patron's library in a group by itself, with "limit item
editing" enabled. The patron should only be able to edit items from
their own library.
4. Pick an item in the catalogue, and add at least 40 items to it from
a library the patron cannot edit items from. Add at least one item
from the patron's library that they can edit. The 40 items are to
ensure there is a complete (20 item) page that the patron cannot
edit anything on.
5. Create an (empty) item group on this catalogue entry.
6. Log in as the unprivileged patron and navigate to the holdings view
for the relevant catalogue entry.
7. Navigate through the holdings table pages until there is a page with
no checkboxes.
8. Navigate to a table page with a checkbox. Select one of the
checkboxes, and verify no actions appear at the top of table, even
though you should have permission to add this item to a group.
9. Apply this patch, and hard refresh to load the new JavaScript.
10. Repeat steps 6-8, verifying the checkbox now appears correctly.
11. Verify you can add the item to an item group using the now-appearing
action.
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>
ill-availability.js renders results from ILL availability plugins.
Each result can carry a `url` (deep link to the record) and an
`opac_url` (generic fallback). The Title column correctly prefers
`url`, but the Source column links to `opac_url` unconditionally and
never checks `url`, so results always link to the bare OPAC URL even
when a deep link is available.
Make Source prefer `url`, falling back to `opac_url`, same as Title.
To test:
1) Install the latest release of the plugin:
https://github.com/openfifth/koha-plugin-ill-avail-z3950/releases
2) Enable ILLModule, ILLCheckAvailability.
3) Set AdditionalFieldsInZ3950ResultSearch = 856$a, 999$c, 001.
4) Add Z39.50 server: Name KTD, Hostname localhost, Port 2100,
Database biblios, Syntax MARC21/USMARC.
5) Configure the ILL z3950 plugin: tick the 'KTD' box, set bib ID
field 999$c, deep link
http://localhost:8081/cgi-bin/koha/opac-detail.pl?biblionumber=source_record_id,
OPAC URL http://localhost:8080, tick the last 3 display
checkboxes, leave Partner ID blank.
6) Add a new ILL request: type book, ISBN 0879307447, cardnumber 42,
destination library Centerville.
7) Go to availability search, confirm the result links to the
correct deep-linked biblio, not a bare fallback.
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Test plan:
Create a report with: SELECT marcxml from biblioitems
Hit /reports/guided_reports.pl?op=list
Notice the "Update SQL" button in the "Update" column, Click "Update"
=> JS error
"Uncaught TypeError: can't access property "backdrop", this._config is undefined" (in Firefox)
And nothing happens
Apply this patch, reload, click the button
=> Success, the report is updated
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The OPAC messaging table includes an empty first column header. This column
contains the message type, such as Item due, Advance notice, Hold filled, and
Auto renewal. Because the header only contains a non-breaking space, it is
flagged by accessibility tools as an empty <th> element.
Add a translated "Message type" label to the first column header so it has
meaningful accessible text.
Test plan:
1. Log in to the OPAC
2. Go to "Your messaging settings"
3. Inspect the messaging table header
4. Confirm the first column header contains no meaningful text
Apply the patch:
5. Reload the page
6. Confirm the first column header contains "Message type"
7. Confirm the messaging table still displays correctly
8. Run an accessibility checker and confirm the empty table header error is resolved
9. Sign off and have a wonderful day! :D
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 OPAC messaging table includes a DataTables responsive control column
with no header text. This results in an empty <th> element and an empty
aria-label, which is flagged by accessibility tools.
Add a translated "Expand" label to the control column header so it has
meaningful accessible text.
Test plan:
1. Log in to the OPAC
2. Go to "Your messaging settings"
3. Inspect the messaging table header
4. Confirm the control column header contains no text
Apply the patch:
5. Reload the page
6. Confirm the control column header contains "Expand"
7. Confirm responsive row expansion still works
8. Run an accessibility checker and confirm the error is resolved
9. Sign off and have a wonderful day!
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>
_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>
This patch displays the directory tested in the background jobs results
It updates the language when test results are not stored for the transport and
links to the job when results are stored
To test:
1 - Setup an ftp file transport with good credentials
2 - Enter non-existent directories in incoming/outgoing
3 - Save
4 - Verify on save 'Never used' shows for status
5 - Refresh the page, status now:
Connection ok
Change directory ok
Change directory failed
Change directory ok
Change directory failed
6 - Apply patch, restart all
7 - Edit/save the transport again, change nothing
8 - Confirm the page says 'No test results stored'
9 - Reload the page
10 - Confirm results are now:
Connection ok
Change directory ok
Change directory failed
Change directory ok
Change directory failed
Test job details
11 - Click on the job details link
12 - Confirm actual directory and errors are shown
13 - Edit the transport, use existing directories
14 - Save, reload
15 - Confirm the success results and link t background job
Signed-off-by: John Vinke <jvinke@austinseminary.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
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>
Sponsored-by: Ramapo Catskill Library System
Bug 16631: Update saved reports table to use use named DataTables columns.
RM amended: Fix commit title
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
holds.js declared `coded_values` as a bare top-level global, evaluated
immediately at script-parse time, reading `all_libraries` which is only
ever defined by the including template (request.tt, or
patron-detail-tabs.inc for circulation.tt/moremember.tt). This works
today only because those templates happen to emit their `all_libraries`
script block before Asset.js("js/holds.js") in the rendered HTML - a
fragile, easy-to-break ordering dependency between separate template
files, not something holds.js itself can guarantee.
Move the assignment into the existing $(document).ready() handler,
setting it explicitly on `window` so it remains visible as a global to
datatables.js's build_query(), which reads `coded_values` unqualified.
This defers evaluation until all synchronous inline scripts (including
whichever template defined all_libraries) have already run, regardless
of future include/ordering changes.
To test:
1. Place holds on a title for two different pickup libraries
2. Confirm the holds table on reserve/request.tt, the patron's Holds
tab (circulation.pl and moremember.pl) all load with no console
errors
3. Confirm filtering the holds table by pickup library name/branchcode
still works on all three pages
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
xt/tt_kohaTable_tidy.t forbids mixing Template::Toolkit tags with
.kohaTable() calls inside the same <script> block, since such blocks
are expected to be pure JS. The cashup modal init calls in
pos/register.tt and pos/registers.tt embedded TT-derived values
(bankable amount, register float, reconciliation note settings,
cashup progress) directly alongside kohaTable() initialisation,
tripping the check.
This moves those TT expressions into the existing TT-tag-only
<script> block that precedes the kohaTable block in each template,
exposing them as a plain JS object (cashupModalData) that the
kohaTable block then reads.
Test plan:
1. Apply the patch.
2. prove xt/tt_kohaTable_tidy.t -- register.tt and registers.tt
should no longer be listed among the failures.
3. Load the POS register and registers pages, confirm the trigger
and confirm cashup modals still receive the correct bankable
amount, register float, and reconciliation note settings.
Sponsored-by: OpenFifth <https://openfifth.co.uk/>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Apply this patch and confirm you can limit to 'Scripts' and that 'Scripts' is not in
all caps in the results
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>
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>
Pedro Amorim
Martin Renvoize
Colin Campbell
Jonathan Druart
Tomás Cohen Arazi
Olivia Reynolds
Laura_Escamilla
Nick Clemens
Casey Conlin
Paul Derscheid
Owen Leonard
Matt Blenkinsop
David Cook
Kyle M Hall