Bug 41386 - Adding 0.00 as value for "Expired hold charge" in circulation rules can lead to exception Koha::Exceptions::Account::AmountNotPositive
Summary: Adding 0.00 as value for "Expired hold charge" in circulation rules can lead ...
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-12-05 13:37 UTC by Emmi Takkinen
Modified: 2025-12-10 07:39 UTC (History)
0 users

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 41386: Do not allow 0.00 values to be passed from Koha::Hold->cancel (2.17 KB, patch)
2025-12-10 07:39 UTC, Emmi Takkinen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2025-12-05 13:37:19 UTC
If one adds 0.00 as "Expired hold charge" rules value in circulation rules and charge is then tried to add to a patron via Koha::Account->add_debit, exception Koha::Exceptions::Account::AmountNotPositive is raised. This can be really confusing for the users since monetary values in circulation rules typically allow using decimal values in them.
Comment 1 Emmi Takkinen 2025-12-09 12:24:04 UTC
The problem seems to be in the Koha::Hold->cancel. In the section where we charge a cancel fee there is a if statement "if $charge" after calling sub add_debit. This allows value "0.00" to be passed to the add_debit and it then gets caught in statement "unless ( $amount > 0 )" which then calls exception AmountNotPositive. This also happens when syspref ExpireReservesMaxPickUpDelayCharge has value 0.00, not just with rule "Expired hold charge".

To reproduce:
1. Enable syspref ExpireReservesMaxPickUpDelay.
2. Set syspref ReservesMaxPickUpDelay value as 1.
3. Set syspref ExpireReservesMaxPickUpDelayCharge value as 0.00.
4. Find a waiting hold which has expiration date further than  ReservesMaxPickUpDelay value is.
5. Run cronscript cancel_expired_holds.pl.
=> Exception 'Koha::Exceptions::Account::AmountNotPositive' thrown 'Debit amount passed is not positive' is raised.
6. Set syspref ExpireReservesMaxPickUpDelayCharge value as 0 and run cron again.
=> NO exception is raised.
Comment 2 Emmi Takkinen 2025-12-10 07:39:05 UTC
Created attachment 190387 [details] [review]
Bug 41386: Do not allow 0.00 values to be passed from Koha::Hold->cancel

In method Koha::Hold->cancel in the section where we charge a cancel
fee there is a if statement "if $charge" after calling sub add_debit.
This allows value "0.00" to be passed to the add_debit and it then
gets caught in statement "unless ( $amount > 0 )" which calls exception
AmountNotPositive. This happens at least when syspref
ExpireReservesMaxPickUpDelayCharge or circulation rule "Expired hold charge"
has value 0.00 and cronscript cancel_expired_holds.pl is run. This patch
fixes the if statement in Koha::Hold->cancel.

To test:
1. Enable syspref ExpireReservesMaxPickUpDelay.
2. Set syspref ReservesMaxPickUpDelay value as 1.
3. Set syspref ExpireReservesMaxPickUpDelayCharge value as 0.00.
4. Make sure you have a waiting hold which has expiration date further
in the past than ReservesMaxPickUpDelay value in your database. If not,
create one.
5. Run cronscript cancel_expired_holds.pl.
=> Exception 'Koha::Exceptions::Account::AmountNotPositive' thrown
'Debit amount passed is not positive' is raised.
6. Set syspref ExpireReservesMaxPickUpDelayCharge value as 0 and run
cronscript again.
=> No exception is raised.
7. Apply this patch and restart services if needed.
8. Create a new waiting hold with expiration date in the past.
9. Run cronscript again.
=> No exception is raised.
=> Confirm that no cancel fee was added for patron.

Also prove t/db_dependent/Koha/Holds.t.

Sponsored-by: Koha-Suomi Oy