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>
This patch addresses the circular dependency issues and code quality
concerns identified in QA by introducing a clean architectural solution.
Changes:
1. Created Koha::Plugins::Loader - A lightweight plugin loader with
minimal dependencies that can be safely called during early
initialization. Uses DBIx::Class when available for proper
transaction support, falls back to raw DBI for early init.
2. Added Koha::Database::table_exists() - Consolidated table checking
logic into a reusable method that can be called as both class and
instance method.
3. Simplified C4::Context BEGIN block - Reduced from ~120 lines to
~13 lines (91% reduction). Removed XML::Simple dependency and
manual database connection code.
4. Updated Koha::Plugins::get_enabled_plugins() - Now delegates to
the Loader module for consistency.
5. Updated C4::Installer::TableExists - Delegates to the new
Koha::Database::table_exists method.
This addresses all QA concerns:
- Eliminates XML::Simple usage (David Cook)
- Uses Koha::Database instead of manual DBI (David Cook)
- Reuses table checking logic (Nick Clemens)
- Eliminates circular dependencies (Paul Derscheid)
- Much cleaner and maintainable code
Test plan:
1. Run tests:
prove t/db_dependent/Koha/Database.t
prove t/db_dependent/Koha/Plugins/Loader.t
prove t/db_dependent/Koha/Plugins/Plugins.t
2. All tests should pass
3. Install and enable a plugin via the UI
4. Verify plugin hooks are called correctly
5. Verify no regressions in plugin functionality
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: 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>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: 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>
This patch prevents deletion of items that have item level holds on
them.
Test plan:
1. Create an item level hold.
2. Delete the item using alternative A or B:
A. Open the biblios Detail-view -> From the toolbar, open
"Edit"-dropdown menu -> Manage items -> Delete the item that has
an item level hold attached to it from "Actions"-dropdown menu
B. Open the biblios Detail-view -> Select the item that has an item
level hold attached to it -> Click "Delete selected items" ->
Confirm deletion
3. Notice that the item is deleted without any warning and the item
level hold has disappeared.
4. Apply this patch.
5. Repeat steps 1-3.
6. Notice that the item cannot be deleted and a warning is shown.
7. prove t/db_dependent/Koha/Item.t
8. Run the REST API tests: prove t/db_dependent/api/v1/items.t
Sponsored-by: Koha-Suomi Oy
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Fixed a pre-existing bug where branch transfer limits would prevent ANY
transfer if the optimal library was blocked, instead of trying the next
best candidate.
Original behavior:
- Sort candidates by best ratio
- Check if transfer to best candidate is allowed
- If not allowed: exit loop immediately (bug!) and return undef
- Result: No transfer created even if other valid candidates exist
Fixed behavior:
- Sort candidates by best ratio
- Check if transfer to best candidate is allowed
- If allowed: use that library and exit loop
- If not allowed: continue to next candidate
- Result: Uses the best ALLOWED candidate
This fix ensures that when branch transfer limits block the optimal
destination, the system will try the next-best option instead of
giving up entirely.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Refactored lowest_ratio_library() to eliminate the N+1 query problem
by using aggregated queries with GROUP BY instead of per-branch queries.
Performance improvement:
- Before: O(3n) queries where n = number of branches with float limits
Example: 20 branches = 60 database queries
- After: O(3) queries regardless of number of branches
Example: 20 branches = 3 database queries
The optimization uses three aggregated queries:
1. Single query with GROUP BY to get item counts at all branches
2. Single query with GROUP BY to get in-transit-to counts for all branches
3. Single query with GROUP BY to get in-transit-from counts for all branches
Results are combined in memory to calculate ratios, maintaining identical
logic and behavior while dramatically improving database efficiency.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
- Add comprehensive POD documentation for lowest_ratio_library() method
including parameter descriptions, return values, and algorithm explanation
- Add database index on itemtype column in library_float_limits table
to improve query performance when searching by itemtype
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Lari Taskula
David Cook
Martin Renvoize
Lucas Gass
Andrew Fuerste Henry
Nick Clemens
Shi Yao Wang
phette23
Jake Deery
Laura_Escamilla
Paul Derscheid
Kyle M Hall
Olli Kautonen