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>
This patch implements an algorithm for selecting the item that would
leave the largest gap after the proposed booking to maximize the
opportunity for future bookings.
Key Changes:
Koha::Booking:
- Added set_itemtype_filter() method for transient itemtype parameter
- Added _select_optimal_item() method using single aggregated query
* Groups bookings by item_id with MIN(start_date)
* Selects items with no future bookings (optimal)
* Among items with bookings, selects one with latest next booking
* O(1) complexity - single database query with GROUP BY
- Updated _assign_item_for_booking() to use optimal selection
* Filters by itemtype when itemtype_id is provided
* Replaces previous random selection (RAND())
API Changes:
- Added itemtype_id parameter support to add() endpoint
- Added itemtype_id parameter support to update() endpoint
- Validates exactly one of item_id or itemtype_id is provided
- itemtype_id is transient - not persisted to database
- Extracted and applied via set_itemtype_filter() before store()
Architecture:
- itemtype_id is NOT a database column (transient only)
- Used as selection criterion, then discarded
Test plan:
1. Apply patch and restart services
2. Create "any item" booking via staff interface
3. Verify item with longest future availability is selected
4. Test via API with itemtype_id parameter
5. Verify itemtype_id is not persisted to database
6. Run t/db_dependent/Koha/Booking.t
7. Run t/db_dependent/api/v1/bookings.t
Signed-off-by: Kristi Krueger <kkrueger@cuyahogalibrary.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit completely modernizes the hold fee system by migrating from
the deprecated categories.reservefee column to the circulation rules framework
and implementing modern Koha::Objects patterns.
WHAT CHANGED:
1. MIGRATION TO CIRCULATION RULES:
- Moved hold fees from categories.reservefee to circulation_rules table
- Added database migration script with atomic update
- Maintains existing fee amounts during migration
- Provides rule hierarchy: category+itemtype+branch specific rules
2. NEW SYSTEM PREFERENCE: TitleHoldFeeStrategy
- Options: highest, lowest, most_common
- Addresses inconsistent title-level hold fees when items have different fees
- Default: 'highest' (safest approach for library revenue)
3. MODERN Koha::Objects API:
- Koha::Item->holds_fee($patron): Calculate fee for specific item
- Koha::Hold->calculate_hold_fee(): Enhanced item/title-level logic
- Koha::Hold->should_charge($stage): Centralized charging logic
- Koha::Hold->charge_hold_fee(): Modern fee charging with proper accounting
- Removed C4::Reserves::GetReserveFee and ChargeReserveFee
4. ENHANCED FUNCTIONALITY:
- Proper ReservesControlBranch logic (patron home vs item home library)
- Complete HoldFeeMode implementation (placement/collection/not_always)
- Clear OPAC messaging with "You will be charged..." commitment
- Comprehensive test coverage for all fee scenarios
5. TECHNICAL IMPROVEMENTS:
- Object-oriented design following Koha patterns
- Proper error handling and edge case management
- Modern test structure with comprehensive coverage
- Clean separation between fee calculation and charging
TEST PLAN:
A) MIGRATION AND SETUP:
1. Apply all patches and run database update
2. Verify existing hold fees migrated to circulation rules:
- Administration > Circulation and fine rules
- Previous categories.reservefee values now in "Hold fee" column
3. Confirm old categories.reservefee column no longer used
B) BASIC FEE CALCULATION:
1. Set up patron categories with different hold fees in circulation rules
2. Create items with different item types, each with different hold fees
3. Test item-level holds charge correct fees based on specific item
4. Test title-level holds use TitleHoldFeeStrategy preference
C) TITLE-LEVEL HOLD STRATEGIES:
1. Set TitleHoldFeeStrategy to "highest":
- Create title with items having fees $1.00, $2.00, $3.00
- Verify title-level holds charge $3.00
2. Set TitleHoldFeeStrategy to "lowest":
- Verify same title charges $1.00
3. Set TitleHoldFeeStrategy to "most_common":
- Add multiple items with $2.00 fee
- Verify title-level holds charge $2.00 (most common)
D) HOLDFEEMODE PREFERENCES:
1. Set HoldFeeMode to "any_time_is_placed":
- Place holds and verify immediate fee charging
- Check patron account for correct hold fee debits
2. Set HoldFeeMode to "any_time_is_collected":
- Place holds, verify no immediate charges
- Check out items to patron, verify fee charged on collection
3. Set HoldFeeMode to "not_always":
- With available items: verify no fee charged
- With all items checked out but no other holds: verify no fee
- With all items checked out and other holds exist: verify fee charged
E) RULE HIERARCHY TESTING:
1. Set different fees for different category/itemtype/branch combinations
2. Verify most specific rule applies:
- Branch + Category + ItemType (most specific)
- Category + ItemType
- Category only (most general)
3. Test with different patron categories and item types
F) USER INTERFACE TESTING:
1. OPAC interface:
- Verify "You will be charged..." messages display correct amounts
- Test both item-level and title-level hold fee messages
- Confirm fees match actual charges in patron account
2. Staff interface:
- Place holds for patrons and verify fee calculations
- Check Administration > Circulation rules interface works correctly
G) EDGE CASES AND ERROR HANDLING:
1. Test $0.00 fees (no charge scenarios)
2. Test patrons with no applicable fee rules (should default to $0.00)
3. Test items with no holdable copies (should return $0.00)
4. Test invalid/missing itemtypes or categories
5. Test holds on biblios with no items
H) REGRESSION TESTING:
1. Verify existing hold placement workflows unchanged
2. Check patron account charging remains accurate
3. Confirm OPAC and staff interfaces display correctly
4. Test that existing holds continue to work properly
I) UNIT TESTS:
Run the following test suites to verify functionality:
- prove t/db_dependent/Koha/Hold.t
- prove t/db_dependent/Koha/Item.t
- prove t/db_dependent/Reserves.t
- prove t/db_dependent/Circulation.t
BACKWARDS COMPATIBILITY:
- All existing functionality preserved
- Hold fee amounts maintained during migration
- Existing workflows continue unchanged
- Legacy code updated to use new API seamlessly
Sponsored-by: OpenFifth <https://openfifth.co.uk>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit integrates hold fee configuration into the main circulation
rules system, providing granular control over hold fees based on patron
category, item type, and branch combinations.
Changes:
- Add hold_fee as a new circulation rule type with full scope support
- Remove deprecated reservefee column mapping from Koha::Patron::Category
- Integrate hold fee input into the circulation rules admin interface
- Add hold fee column to the main circulation rules matrix
- Enable proper rule hierarchy resolution for hold fees
Hold fees can now be configured at multiple levels with automatic
fallback handling following standard circulation rule patterns.
Sponsored-by: OpenFifth <https://openfifth.co.uk>
Signed-off-by: Jackie Usher <jackie.usher@westsussex.gov.uk>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The get_plugins_intranet_js method now passes the current script name
(controller) to plugins via a 'page' parameter, allowing plugins to
determine which page/controller is being displayed.
This will allow plugin authors to only return JS that's relevant to the
current page when they wish to do so.
Test plan:
1. Create a plugin with an intranet_js method that logs the received parameters
2. Navigate to different pages in the staff interface
3. Verify that the plugin receives the page parameter containing the script name
4. Confirm that the script name reflects the actual controller (e.g.,
'/cgi-bin/koha/admin/admin-home.pl' when on the admin home page)
5. Run the test: prove t/db_dependent/Koha/Template/Plugin/KohaPlugins.t
6. Verify that the new test passes, confirming plugins receive the page parameter
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This change impacts the display of patron fields listed in system preferences:
BorrowerMandatoryField
BorrowerUnwantedField
PatronSelfModificationBorrowerUnwantedField
PatronSelfRegistrationBorrowerMandatoryField
PatronSelfRegistrationBorrowerUnwantedField
To test:
1 - confirm that the sysprefs listed above include "Alternate contact: First name (contactfirstname)" and "Alternate contact: Surname (contactname)"
2 - confirm that toggling the display of these fields hides/shows the Guarantor first name and Guarantor surname fields in a patron record
3 - apply patch, restart_all
4 - confirm that sysprefs now show "Guarantor first name (contactfirstname)" and "Guarnator surname (contactname)"
5 - confirm toggling the display of these fields still hides/shows the correct fields in a patron record in the staff client, in self-ref, and in OPAC self-modification
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
The ES query builder correctly handles this parameters (passed from
search.pl and opac-search.pl) when it builds the ES query. But the
returned 'query_cgi' that is used later for keeping the search results
query context for building things like facets, just doesn't pass this
values around, leading to a loss of them on the generated links.
This patch makes sure they are passed around.
To test:
1. Apply the regression tests
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
=> FAIL: Tests fail! These params are not passed around!
3. Set ElasticsearchMARCFormat="Searchable array"
4. Reindex all the things:
k$ koha-elasticsearch --rebuild -d -v kohadev
5. Go to advanced search, using the 'More options' option.
6. Check 'Search entire MARC record' and 'Apply weights to search'
7. Perform a search on 'FULL TEXT'
=> SUCCESS: The URL contains `whole_record=on` and `weighted_fields=on`
8. Hover on any facet and look at the link
=> FAIL: It doesn't contain them!
9. Apply this patch
10. Restart all
k$ restart_all
11. Refresh the search results
12. Repeat 6
=> SUCCESS: Links have the needed parameters!
13. Repeat 2
=> SUCCESS: Tests pass! the parameters are passed around!
14. Sign off :-D
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>
start_server from Net::Z3950::SimpleServer is going to fork and the DB
server is aborting the connection
db-1 | 2025-10-03 12:02:12 272 [Warning] Aborted connection 272 to db: 'koha_kohadev' user: 'koha_kohadev' host: '172.19.0.6' (Got an error reading communication packets)
Test plan:
prove t/db_dependent/Koha/Z3950Responder/ZebraSession.t
=> Without this patch you will see a warning in the db container log
=> With this patch apply the warning no longer appears
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>
Signed-off-by: Richard Bridgen <richard.bridgen@nhs.net>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
TEST PLAN:
1 - Apply tests patch
2 - Run 'prove t/db_dependent/Koha/Suggestions.t' -> it fails on
rejecter
3 - Apply this patch
4 - Run 'prove t/db_dependent/Koha/Suggestions.t' -> it works
Note: this patch also provide tests for remaining accessors in the
subtest
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Martin Renvoize
Lucas Gass
Andrew Fuerste Henry
Nick Clemens
Shi Yao Wang
phette23
Jake Deery
Laura_Escamilla
Paul Derscheid
Lari Taskula
Kyle M Hall
Olli Kautonen
Tomás Cohen Arazi
Jonathan Druart
Pedro Amorim
Baptiste Wojtkowski