We currently have to set options, explanation and type in sysprefs.sql and the atomic/dbrev files.
However we (almost) never use them, we rely on the values in the yml files.
We could remove them and don't require them when when add new sysprefs.
It will avoid the mess we currently have (mismatch between new and updated installations, see bug 41682 and bug 41800).
Instead of fixing the discrepancies (again, see bug 41800) we should
NULL the 3 columns in sysprefs.sql and so at the DB level to rely only
on the yaml files.
This patch does several things:
1. Remove the 3 columns from sysprefs.sql to use the default NULL
2. Atomic update:
* Set to NULL for existing installations as well
* Rename UseICUStyleQUotes => UseICUStyleQuotes
* Delete OpacMoreSearches and OPACMySummaryNote, old prefs that have been moved
to additional contents
3. Fix some errors in yaml files (mostly empty entries)
4. Move parsing of yaml files to a dedicated Koha::Devel::Syspref module
5. Retrieve all yaml content using new methods added to Koha::Config::SysPrefs
(that will be reused in follow-up bugs)
Test plan:
1. Run updatedatabase and confirm that now all systempreferences rows in DB
have options, explanation and type set to NULL (except local-use prefs)
2. `prove t/db_dependent/check_sysprefs.t` should return green
3. Confirm that the UI is still working correctly
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
This patch changes "op" to "login_op" when logging in, so that the original op can
flow through to whichever controller is processing the login and subsequent op.
Test plan:
0. apply the patch and koha-plack --restart kohadev
1. Without logging in, go to the following:
http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?id=1&op=run
2. Log in and note that you can see the report
3. Try logging into the OPAC
4. Try logging into the self-checkout
5. Create a new self-reg account and try logging in with the auto-filled details
6. Test anything else you can think to test here
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
It appears that edi_cron.pl will run disabled EDI plugins. It uses Koha::Plugins::Handler::run that does not check if the plugins is gets are disabled.
Koha::Plugins::Handler::run needs to be fixed to not run disabled plugins.
Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Plugins/Plugins.t
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Test plan:
0. Apply the patch
1. Run the following:
prove t/db_dependent/FrameworkPlugin.t \
t/db_dependent/Koha/Plugins/Valuebuilder_hooks.t \
t/db_dependent/Koha/UI/Form/Builder/Biblio.t \
t/db_dependent/Koha/UI/Form/Builder/Item.t \
t/db_dependent/Serials.t
Squashed version of the following patches:
Bug 38365: Fix Koha/UI/Form/Builder/Biblio.t
# Failed test at t/db_dependent/Koha/UI/Form/Builder/Biblio.t line 216.
# '<script nonce="">
# $(document).ready(function () {
#
# });
# </script>
# '
# doesn't match '(?^us:<script>.*</script>)'
# Looks like you failed 1 test of 4.
Bug 38365: Fix Koha/UI/Form/Builder/Item.t
# There were 16 warning(s)
# Previous test 0 ''
# Use of uninitialized value $csp_nonce in concatenation (.) or string at /kohadevbox/koha/Koha/FrameworkPlugin.pm line 381.
# at /kohadevbox/koha/Koha/FrameworkPlugin.pm line 381.
Bug 38365: Fix Serials.t
Bug 38365: Fix FrameworkPlugin.t
Schema type [array,object] is not valid in OpenAPI version 2.
There are two different CSP reporting directives.
One is the now deprecated "report-uri" directive. Browsers typically send a single
JSON object following the CSP2 specification.
"report-uri" is deprecated in CSP3 in favor of the newer "report-to" directive. It
is expected to contain an array of report objects.
There are browser that do not yet support the "report-to" directive, so both
directives should be supported.
There are also variations among the "report-uri" format. See
https://www.tollmanz.com/content-security-policy-report-samples/
Because of these variations, it becomes quite complex to handle the most common
cases in a single API endpoint.
We could build two separate endpoints for both directives.
Or we can accept any type of input and handle the variations in a single endpoint.
This patch attempts to do that.
Before this patch input parameters were validated by JSON::Validator. That is the
normal and recommended way to build OpenAPI endpoints in Koha. After this patch we
remove all input parameter restrictions from the OpenAPI spec, and simply accept
any type of body.
This means the controller will now always return a HTTP 204 regardless of the type
of data passed to the controller. It instead truncates too long input before
writing them into log files.
As this is not an endpoint that developers will build new Koha clients for, we
do not have to offer a very in-depth input parameter spec for this endpoint.
To test:
1. t/db_dependent/api/v1/public/csp_reports.t
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
By defining a default policy in the Perl module, we can easily update
it over time. Using koha-conf.xml will just be for overrides, so people
can manage their own there.
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
To test:
1. prove t/db_dependent/api/v1/public/csp_reports.t
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
The violated-directive is a historic name for the effective-directive field and
contains the same value.
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
report-uri is deprecated, replaced by report-to and Reporting-Endpoints.
To test:
1. prove t/db_dependent/Koha/Middleware/ContentSecurityPolicy.t
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This patch adds the default configuration to log4perl.conf.
Note we are passing a non-standard interface 'csp' to Koha::Logger. This is to
avoid log entries from propagating up to api-error.log.
To test:
1. prove t/db_dependent/api/v1/public/csp_reports.t
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Add /api/v1/public/csp-reports endpoint to receive and log
Content-Security-Policy violation reports from browsers.
When CSP is configured with a report-uri directive pointing to this
endpoint, browsers will POST violation reports whenever a CSP rule
is violated. These reports are logged using Koha's logging system
for administrator review.
Features:
- Public endpoint (no authentication required, as browsers send
these reports automatically)
- Logs violations at WARN level with key details (violated directive,
blocked URI, document URI, source location)
- Logs full report at DEBUG level for detailed analysis
- Accepts both application/csp-report and application/json content types
Configuration example (in koha-conf.xml csp_header_value):
...; report-uri /api/v1/public/csp-reports
Log4perl configuration to capture CSP violations:
log4perl.logger.api.csp = WARN, CSP
log4perl.appender.CSP = Log::Log4perl::Appender::File
log4perl.appender.CSP.filename = /var/log/koha/csp-violations.log
log4perl.appender.CSP.layout = PatternLayout
log4perl.appender.CSP.layout.ConversionPattern = [%d] %m%n
Also updates koha-conf.xml documentation with CSP configuration guidance.
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
The //= (defined-or assignment) operator on a freshly declared
variable is a no-op since the variable is always undef at that
point. Replace with simple = assignment.
Affects header_name(), header_value(), and is_enabled() methods.
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
OWASP recommends CSP nonces have at least 128 bits of entropy.
The previous 10-character alphanumeric nonce provided only ~59 bits
(log2(62^10) ≈ 59.5).
Increase to 22 characters which provides ~130 bits of entropy
(log2(62^22) ≈ 130.5), exceeding the OWASP recommendation.
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This patch adds a new Template plugin method, Koha.CSPNonce, that can be used to
attach the Content Security Policy nonce into our templates.
To test:
1. prove t/Koha/Middleware/ContentSecurityPolicy.t
2. prove t/db_dependent/Koha/Template/Plugin/Koha.t
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This patch adds a new Perl package, Koha::Middleware::ContentSecurityPolicy.
It is called for each Plack requests, and its job is to add the
Content-Security-Policy[-Report-Only] header with the configured value.
To test:
1. prove t/Koha/Middleware/ContentSecurityPolicy.t
2. prove t/db_dependent/Koha/Middleware/ContentSecurityPolicy.t
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This patch adds a new Perl package, Koha::ContentSecurityPolicy.
It provides the following methods:
$csp->header_name
Returns the name of the CSP header, based on koha-conf configuration.
$csp->header_value
Returns the value of the CSP header, based on koha-conf configuration.
$csp->is_enabled
Returns true or false, based on koha-conf configuration.
$csp->nonce
Generates or sets, and returns the nonce. A CSP nonce is a random token that is
used both in the inline scripts and the Content-Security-Policy[-Report-Only]
response header.
To test:
1. prove t/Koha/ContentSecurityPolicy.t
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
When a staff member writes off an accruing fine and the item is
subsequently checked in (backdated or in fine-free mode), the fine
amount is recalculated downward via adjust(). The previous code
unconditionally created an OVERPAYMENT credit whenever the adjusted
outstanding went negative, regardless of whether the existing credits
were reversible payments or non-reversible writeoffs/discounts.
This caused patrons to receive an erroneous credit equal to the
written-off amount, since writeoffs should simply be absorbed when
the underlying fine shrinks - no refund is due.
The fix calculates the total non-reversible credits (writeoffs,
discounts, cancellations) applied to the debit using the existing
filter_by_non_reversible method, and only generates an OVERPAYMENT
credit for the portion attributable to reversible cash payments that
exceed the recalculated fine amount.
Test plan:
1. Write off an accruing fine in full
2. Check the item in (backdated or fine-free)
3. Confirm no overpayment credit is generated for the patron
Signed-off-by: Benjamin Daeuber <BDaeuber@fargolibrary.org>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Add validation to ensure AnonymousPatron preference contains a valid
borrowernumber before attempting to use it in the anonymization process.
This prevents foreign key constraint errors and orphaned records if the
preference contains an invalid value (non-existent borrowernumber,
string instead of number, etc.).
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit refactors Koha::Patrons->anonymize_last_borrowers() to use
a single bulk UPDATE instead of looping through patrons individually.
Performance Problem:
-------------------
The original implementation had a severe N+1 query problem:
1. Query all patrons with items_last_borrower records
2. Loop through each patron (First N+1)
3. For each patron, search their items_last_borrowers (Second N+1)
4. Update each result set
For a library with 10,000 patrons and 100,000 items, this would
execute 20,000+ database queries when run as a cronjob.
The Fix:
--------
Query the items_last_borrower table directly with all conditions
in a single search, then perform one bulk UPDATE:
$schema->resultset('ItemsLastBorrower')->search({...})->update({...})
This reduces execution from potentially thousands of queries to a
single SQL UPDATE statement, regardless of data volume.
Why This Pattern:
-----------------
While Koha::Objects->update() exists, it loops through records
individually if the object class has update()/store() methods,
unless {no_triggers => 1} is passed. Working directly with the
resultset guarantees bulk update behavior.
This pattern is used throughout Koha for bulk operations (see
Koha/Item.pm, Koha/Hold.pm, admin/koha2marclinks.pl, etc).
Testing:
--------
prove t/db_dependent/Koha/Patrons.t
All existing tests pass, confirming identical behavior with
dramatically improved performance.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
To test:
1 - configure StoreLastBorrower but not AnonymousPatron
2 - perl misc/cronjobs/anonymize_last_borrowers.pl -c
3 - the cron tells 'Preference 'AnonymousPatron' must be enabled to anonymize item's last borrower'
4 - put a value in AnonymousPatron
5 - repeat 2
6 - cron runs good
Signed-off-by: Peter Moore <peter@mrcpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch adds a new routine to allow anonymizing the items_last_borrower table
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Peter Moore <peter@mrcpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Test plan:
1- Change to a language other than english
2- Search in the catalog with "!"
3- Notice the string "Unable to understand your search query, please
rephrase and try again." is not translated
4- Apply the patch
5- Add translation in .po file of used language for "Unable to
understand your search query, please rephrase and try again.\n"
6- Update translation
7- Redo the search and notice the string is now translated
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Testing plan:
- Enable the ILLModule system preference
- Sign into the OPAC and make an ILL request at /cgi-bin/koha/opac-illrequests.pl
- Note there's no option to specify notes
- Apply the patch and restart services
- Make another ILL request
- Use the new notes field
- View the request and note that the notes value was saved
Sponsored-By: California College of the Arts
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch fixes an omission that caused t/Koha/CSV.t to fail. To test, prove t/Koha/CSV.t and notice how it now passes.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Improves POD documentation for branch_for_fee_context to:
- Document all parameters (patron, item, issue objects)
- Explain the resolution logic for each control preference
- Clarify that returning undef is acceptable behavior
- Note how undef is handled by calling code
Adds inline comment explaining that undef return value is
gracefully handled by Koha::Account->add_debit.
This addresses concerns about unclear undef handling and
improves maintainability.
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch fixes two issues:
1. C4::Accounts::chargelostitem now properly uses the $interface
variable that was set from $opts->{interface} instead of calling
C4::Context->interface directly. This ensures the interface
parameter is honored, particularly important for cronjob context.
2. Removes the self-referential 'use Koha::Checkout;' statement in
Koha/Checkout.pm which is logically incorrect (a module should
not use itself).
Test plan:
1. Run prove t/db_dependent/Accounts.t
2. Run prove t/db_dependent/Koha/Checkout.t
3. Verify all tests pass
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch ensures that the accountlines.branchcode field correctly records
which branch's circulation rules were applied when creating OVERDUE, LOST,
and PROCESSING fee debits. Previously, this field was often NULL or set
to the logged-in user's branch, making it difficult to track which branch's
policies governed the fee calculation.
Changes made:
1. New helper method: Koha::Checkout->branch_for_fee_context()
- Centralizes branch resolution logic based on system preferences
- For LOST/PROCESSING: honors LostChargesControl preference
- For OVERDUE: honors CircControl preference
- Respects HomeOrHoldingBranch when choosing item's branch
- Supports both Koha objects and plain hashrefs
- Returns branchcode or undef if context unavailable
2. C4::Accounts::chargelostitem()
- Now accepts optional $opts hashref parameter
- Accepts interface, library_id, and issue_id in options
- Determines appropriate branch using branch_for_fee_context
- Passes resolved branch to add_debit as library_id
- Issue_id lookup handles bundled items correctly
3. C4::Circulation::LostItem()
- Calls branch_for_fee_context before chargelostitem
- Passes interface context ('cron' for cronjob, otherwise current)
- Passes resolved branch and issue_id to chargelostitem
4. C4::Overdues::UpdateFine()
- Calls branch_for_fee_context to determine OVERDUE branch
- Passes result as library_id to add_debit
- Resolves long-standing FIXME about branch selection
5. misc/cronjobs/longoverdue.pl
- Determines branch context before marking items lost
- Passes library_id to LostItem when charging fees
The branch resolution logic respects these system preferences:
- CircControl: PatronLibrary | PickupLibrary | ItemHomeLibrary
- LostChargesControl: PatronLibrary | PickupLibrary | ItemHomeLibrary
- HomeOrHoldingBranch: homebranch | holdingbranch
Test plan:
1. Create three test libraries (e.g., FPL, CPL, PVL)
2. Create a test patron with home library PVL
3. Create an item with:
- homebranch = FPL
- holdingbranch = CPL
4. Set system preferences:
- FinesMode: Calculate and charge
- Create circulation rule that charges overdue fines
5. Test OVERDUE fees with CircControl:
a. Set CircControl = PatronLibrary, HomeOrHoldingBranch = homebranch
b. Check out item to patron at CPL, backdate to be overdue
c. Run: perl /usr/share/koha/bin/cronjobs/fines.pl
d. Check accountlines: SELECT accountlines_id, branchcode, debit_type_code
FROM accountlines ORDER BY accountlines_id DESC LIMIT 5;
e. Verify branchcode = PVL (patron's home library)
f. Delete the fine, repeat with CircControl = PickupLibrary
g. Verify branchcode = CPL (checkout library)
h. Delete the fine, repeat with CircControl = ItemHomeLibrary
i. Verify branchcode = FPL (item's home library)
j. Change HomeOrHoldingBranch = holdingbranch, repeat
k. Verify branchcode = CPL (item's holding library)
6. Test LOST fees with LostChargesControl:
a. Set WhenLostChargeReplacementFee = Charge
b. Set item replacement price > 0
c. Ensure item is checked out to patron
d. Set LostChargesControl = PatronLibrary
e. Mark item as lost in staff interface
f. Check accountlines for LOST debit
g. Verify branchcode = PVL (patron's home library)
h. Return item, re-checkout, repeat with LostChargesControl = PickupLibrary
i. Verify branchcode = CPL (checkout library)
j. Repeat with LostChargesControl = ItemHomeLibrary + homebranch
k. Verify branchcode = FPL (item's home)
l. Repeat with LostChargesControl = ItemHomeLibrary + holdingbranch
m. Verify branchcode = CPL (item's holding)
7. Test longoverdue cronjob:
a. Re-checkout item with due date triggering longoverdue
b. Run: perl /usr/share/koha/bin/cronjobs/longoverdue.pl --lost 30=1 --charge 1 --confirm --verbose
c. Verify LOST debit created with correct branchcode per LostChargesControl
8. Test edge cases:
- WhenLostChargeReplacementFee = Don't charge → no LOST debit
- Replacement price = 0 → no LOST debit
- FinesMode = Don't calculate → no OVERDUE debits
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Sponsored-by: The Main Library Alliance <https://www.mainlib.org/>
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Sponsored-by: The Main Library Alliance <https://www.mainlib.org/>
Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
- Booking::store runs item-level and biblio-level clash
detection unconditionally on every call
- During checkout, C4::Circulation sets the booking status to
'completed' and calls ->store(), which triggers clash checks
- Combined with false positives from Biblio::check_booking
(e.g. checkouts on non-bookable sibling items inflating the
unavailable count), this throws
Koha::Exceptions::Booking::Clash resulting in a 500 error
- Running clash detection on a terminal transition is
unnecessary regardless; skip it for cancelled/completed
- Add _is_final_status_transition() helper that inspects the
dirty columns to detect completed/cancelled transitions
- Guard both clash detection blocks with the new helper
To test:
1. Have a biblio with two items: one bookable, one not
(bookable = 0).
2. Create a booking on the bookable item.
3. Check out the non-bookable item to a patron.
4. Check out the bookable item to the booking patron (this
triggers the completed transition via C4::Circulation).
5. Without the patch: 500 error from
Koha::Exceptions::Booking::Clash.
6. Apply the patch.
7. Repeat steps 2-4.
8. Verify the checkout succeeds and the booking transitions
to completed.
9. Run: prove t/db_dependent/Koha/Booking.t
and confirm the new subtest "store() skips clash
detection on terminal status transition" passes.
Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
- Biblio::check_booking counts current checkouts toward the
unavailable pool to detect booking clashes
- It counts checkouts on ALL items for the biblio, including
non-bookable ones (e.g. items with an excluded item type)
- When only a subset of items are bookable, checkouts on
non-bookable siblings incorrectly reduce the available pool,
producing false clashes
- Restrict the checkout query to only count items whose
itemnumber is in the bookable items set
To test:
1. Have a biblio with at least two items, where one item is
bookable and the other is not (bookable = 0).
2. Check out the non-bookable item to a patron.
3. Attempt to create a booking for the bookable item.
4. Without the patch: a clash is detected even though the
bookable item is available.
5. Apply the patch.
6. Repeat steps 2-3.
7. Verify the booking succeeds without a false clash.
8. Run: prove t/db_dependent/Koha/Biblio.t
and confirm the new subtest
"checkouts on non-bookable items do not cause false clashes"
passes.
Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
To test:
1. prove t/db_dependent/Koha/ImportBatch.t
2. Observe success
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This bug adds Koha::Patron::Message::message_without_newlines, which has been added to C4::SIP::ILS::Patron in lieu of Koha::Patron::Message::message. The aim of this change is to prevent SIP2 units from crashing out when they encounter a patron message with newlines (\n or \r) in it, as units generally use a newline character to denote the end of a SIP2 transaction.
To test:
a) go to /cgi-bin/koha/sip2/institutions
*) create an institution, name CPL, timeout 300, the rest default
b) go to /cgi-bin/koha/sip2/accounts
*) create an account, userid koha, institution CPL, terminator crlf, the rest default
c) go to /cgi-bin/koha/members/moremember.pl?borrowernumber=51
*) click add message in toolbar
*) enter some multi-line message e.g. test[RETURN]1[RETURN]2[RETURN]3[RETURN][RETURN][RETURN]
d) run tail -f /var/log/koha/kohadev/sip-output.log, keep this window open
e) run ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su koha -sp koha -l CPL --patron 42 -m patron_information
f) look at the tail log, notice how the MSG 64 line has a newline before the final pipe (|) character
== APPLY PATCH ==
g) restart_all
h) repeat steps e-f
i) notice how in the tail log, the MSG 64 line now has the final pipe character contiguously placed with the successive characters
== SIGN OFF ==
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This patch fixes xt/author/Text_CSV_Various.t by using a regex that can consider logical defined-or statements (|| and //, etc) when checking which formula has been used.
TO TEST:
a) run prove -vv xt/author/Text_CSV_Various.t
*) notice the test fails
== APPLY PATCH ==
b) repeat step a)
*) notice the test now passes
c) inspect diff for xt/author/Text_CSV_Various.t
*) notice we are now using a wildcard (.*) between => and the first ' to skip variable declarations
d) inspect diff for Koha/CSV.pm
*) notice we are now hard-coding 'empty' as a defined-or in case someone decides not to define a format at the class level
(which was not being done before anyway and is now defaulted to undefined)
== SIGN OFF ==
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
remaning ocurrences in test files seem okay, this covers the others
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Thinking about this today, we really just need the plugins initialized anytime we are using Koha::Plugins
Moving the loading into the BEGIN there seems to resolve the issue
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Jonathan Druart
David Cook
Kyle M Hall
Lari Taskula
Martin Renvoize
Lucas Gass
Andrew Fuerste Henry
Nick Clemens
Shi Yao Wang
phette23
Jake Deery
Laura_Escamilla
Paul Derscheid