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>
The original fix only whitelisted agefield, condition_field, and
substitution_field at the point they're saved in
tools/automatic_item_modification_by_age.pl, leaving the actual
consumer of the stored syspref, C4::Items::ToggleNewStatus, trusting
agefield and substitution field unchecked. agefield is spliced
directly into raw SQL (TO_DAYS($agefield)) and so can't be
parameterized like an ordinary value; substitution field reaches a
dynamic method call ($item->$field($value)). Any future or
alternate writer of the automatic_item_modification_by_age_configuration
syspref would have reopened both holes without touching the form at
all.
This adds the same allow-list validation directly in ToggleNewStatus
(falling back to the safe default agefield, or skipping the
substitution/condition, exactly as before for missing values), shares
the age-field allow-list between the two files via a new
GetAutomaticItemModificationAgeFields() in C4::Items so it can't drift
between them, and logs rejected values via Koha::Logger instead of
silently discarding them so tampering leaves an audit trail.
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>
Amended-by: Jonathan Druart:
Replace , with ;
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The custom/authorized-value status branch concatenated row.status
directly into a class="..." attribute string. escape_str() only
escapes &, <, > (not quotes), so a status code containing a double
quote could still break out of the attribute.
Build the status <span> via DOM APIs instead, the same way the
reason field was fixed: fixed statuses get a hardcoded class, and
the free-form status code goes into a data-status attribute via
dataset, which the DOM escapes safely regardless of content.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
escape_str all the things
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
The angle-bracket delimiter notation in the POD prose (e.g.
PATTERN<DELIM>) caused the POD parser to treat the trailing letter of
each word as an unknown formatting code (N<>, T<>). Replace with
C<s/PATTERN/REPLACEMENT/FLAGS> and a parenthetical note that any
non-space delimiter is accepted.
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>
- 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>
When eval { qr/$pattern/ } fails (e.g. the admin-configured s/// contains
an invalid regex), _apply_substitution previously returned silently with no
indication of why the rule was skipped. Add a Koha::Logger warn so the
failure appears in the log and is diagnosable without a debugger.
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>
C4::ClassSplitRoutine::RegEx::split_callnumber ran the admin-configured rule
through eval "\$cn_item =~ $regex", executing arbitrary Perl. Parse the
rule as s<DELIM>PATTERN<DELIM>REPLACEMENT<DELIM>FLAGS, reject the /e flag,
compile the pattern with qr// and expand the replacement as data via
Koha::Regex::Replacement::expand_template, so the rule is applied without
eval.
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>
Koha::SimpleMARC::_modify_values and Koha::Items::apply_regex wrapped the
user replacement in a string and ran it through s///ee, so an interpolation
block such as \@{[ system(...) ]} executed arbitrary Perl. Expand the
replacement as data via Koha::Regex::Replacement::expand_template under a
single /e instead, which keeps capture-group replacements working but never
runs the input as code.
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>
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>
This patch makes some style changes to the list transfer form. Since it
needs the same CSS that is used on the circulation page, the CSS block
is moved from circulation.tt to staff-global.scss.
The patch also changes the behavior of the form so that the "New owner"
line doesn't appear until after a patron has been selected.
To test, apply the patch and rebuild the staff interface CSS.
- If necessary, create a public list.
- From the list of public lists, click the "Transfer" button.
- Verify that the form looks correct. The patron search field should
match the search field shown when you navigate directly to
circulation.pl
- Search for a patron who has permission to manage public lists.
- After you select a patron, a new line should appear:
"New owner: [name]"
- Click "Transfer" and confirm that the process works correctly.
Sponsored-by: Athens County Public Libraries
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Add information about the data that is sent to Google if the
OPACSuggestionAutoFill system preference is enabled.
Test plan:
1. Go to Administration > System preferences.
2. Search for the OPACSuggestionAutoFill system preference.
3. Note that there is no note about the data that is sent to
Google if the system prference is enabled.
4. Apply the patch.
5. Refresh the page.
6. There is now a note about the data that is sent to Google.
7. Check that it reads well and makes ense.
Sponsored-by: Catalyst IT
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>
When an item is checked in via SIP2 at the correct target branch after a
transfer, the system was incorrectly triggering an alert flag. This occurred
because the TransferArrived message was not being filtered out for accounts
with checked_in_ok="1", causing the alert logic to treat successful transfer
completion as a failure condition.
The TransferArrived message indicates successful transfer completion and is
purely informational - it should not trigger SIP alerts. This fix ensures
that TransferArrived messages are filtered out before alert evaluation,
preventing false positive alerts on successful operations.
Test Plan:
1. Set up a SIP2 account with checked_in_ok="1" in SIPconfig.xml
2. Via staff client, check out an item to a patron
3. Return the item to a different branch to trigger a transfer
4. Use misc/sip_cli_emulator.pl to check in the item at the target branch
5. Verify SIP response shows alert flag 'N' (no alert) instead of 'Y'
6. Test wrong branch checkin still shows alert flag 'Y' with CV04 type
Before: Transfer completion at correct branch incorrectly shows alert='Y'
After: Transfer completion at correct branch correctly shows alert='N'
Sponsored-by: OpenFifth <https://openfifth.co.uk>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.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>
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>
Jonathan Druart
Martin Renvoize
David Cook
Sanjar Tulkinov Anvar o'g'li
Owen Leonard
Jennifer Sutton
Pedro Amorim
Colin Campbell
Tomás Cohen Arazi
Olivia Reynolds
Laura_Escamilla
Nick Clemens
Casey Conlin