I noticed that when I add a circulation rule, if I leave the Overdue fines cap (amount) field empty, it is displayed as 0.00. If I later edit this rule, the Overdue fines cap (amount) field is *set to* 0.00, even if that's not what I wanted. This causes the fines.pl script to charge no fine at all. To test: 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. MaxFine system preference must be empty 0.3. In Tools > Calendar, today must not be a holiday 1. Add a circulation rule with fines but no fines cap 1.1. Go to Administration > Circulation and fines rules 1.2. Add or modify a circulation rule, make sure - There is a quantity in "Current checkouts allowed" - There is a number of days in "Loan period" - There is an amount in "Fine amount" - There is a number in "Fine charging interval" (usually 1) - "Overdue fines cap (amount)" is **empty** 1.3. Save the circulation rule --> Note that the amount in "Overdue fines cap (amount)" is displayed as 0.00 2. Checkout an item with a date in the past to create an overdue 2.1. Find an item barcode 2.1.1. Go to Item search 2.2.2. Click Search 2.2.3. Copy a barcode from the search results 2.2. Go to a patron's file 2.2.1. Go to Patrons 2.2.2. Click on "Search" 2.2.3. Click on a patron's name 2.3. Click on the "Check out" tab on the left of the patron's file 2.4. Click on the settings icon in the "Enter item barcode" field 2.5. Pick a due date in the past 2.6. Paste the item barcode in the field 2.7. Click "Check out" or press Enter 2.8. Click "Yes, check out" --> Patron should have an overdue 3. Run fines.pl 3.1. In a terminal, run ./misc/cronjobs/fines.pl 4. Check that the patron has fines 4.1. Go to the patron's file 4.2. Click on the "Accounting" tab on the left --> Patron should have a fine (accruing) 5. Modify the circulation rule, change any field except "Overdue fines cap (amount)" (let's say the quantity of current checkouts has gone up) 5.1. Go to Administration > Circulation and fines rules 5.2. Click "Edit" next to the rule created in step 1 --> Note that the amount in "Overdue fines cap (amount)" is 0.00 5.3. Change any setting except "Overdue fines cap (amount)", e.g. change Current checkouts allowed to another number 5.4. Click "Save" --> Note that the amount in "Overdue fines cap (amount)" is displayed as 0.00 6. Redo step 2 7. Redo step 3 8. Redo step 4 --> Note that the patron has no fine If Overdue fines cap (amount) is empty, it should display as empty (or "Unlimited"), and when we edit the rule, the field should still be empty and save as empty.
I noticed this as far back as 22.05 (did not check 21.11, but 21.05 is OK).
The same thing is happening on 21.11.11. However, it isn't (consistently?) preventing patrons from accruing fines for overdue items - I'm seeing patrons who are accruing fines as they should be even though the applicable rule in the matrix shows the 0.00 fine cap.
(In reply to Sara Brown from comment #2) > The same thing is happening on 21.11.11. However, it isn't (consistently?) > preventing patrons from accruing fines for overdue items - I'm seeing > patrons who are accruing fines as they should be even though the applicable > rule in the matrix shows the 0.00 fine cap. Hi Sara, If you didn't edit the rule, the matrix will show 0.00, but in the database, the overdue fines cap is actually empty. So your patrons will still accrue (?) fines normally. Only beware if you need to edit the rule, make sure to empty the overdue fines cap field. Caroline
Created attachment 144430 [details] [review] BUG 32271 : Fix overdue fines cap (amount) set to 0.00 when editing rule. To test: 0. Preliminary settings 0.1. finesMode system preference must be set to Calculate and charge 0.2. MaxFine system preference must be empty 0.3. In Tools > Calendar, today must not be a holiday 1. Add a circulation rule with fines but no fines cap 1.1. Go to Administration > Circulation and fines rules 1.2. Add or modify a circulation rule, make sure - There is a quantity in "Current checkouts allowed" - There is a number of days in "Loan period" - There is an amount in "Fine amount" - There is a number in "Fine charging interval" (usually 1) - "Overdue fines cap (amount)" is **empty** 1.3. Save the circulation rule --> Note that the amount in "Overdue fines cap (amount)" is displayed as 0.00 2. Checkout an item with a date in the past to create an overdue 2.1. Find an item barcode 2.1.1. Go to Item search 2.2.2. Click Search 2.2.3. Copy a barcode from the search results 2.2. Go to a patron's file 2.2.1. Go to Patrons 2.2.2. Click on "Search" 2.2.3. Click on a patron's name 2.3. Click on the "Check out" tab on the left of the patron's file 2.4. Click on the settings icon in the "Enter item barcode" field 2.5. Pick a due date in the past 2.6. Paste the item barcode in the field 2.7. Click "Check out" or press Enter 2.8. Click "Yes, check out" --> Patron should have an overdue 3. Run fines.pl 3.1. In a terminal, run ./misc/cronjobs/fines.pl 4. Check that the patron has fines 4.1. Go to the patron's file 4.2. Click on the "Accounting" tab on the left --> Patron should have a fine (accruing) 5. Modify the circulation rule, change any field except "Overdue fines cap (amount)" (let's say the quantity of current checkouts has gone up) 5.1. Go to Administration > Circulation and fines rules 5.2. Click "Edit" next to the rule created in step 1 --> Note that the amount in "Overdue fines cap (amount)" is 0.00 5.3. Change any setting except "Overdue fines cap (amount)", e.g. change Current checkouts allowed to another number 5.4. Click "Save" --> Note that the amount in "Overdue fines cap (amount)" is displayed as 0.00 Apply patch : 6. Redo step 2 7. Redo step 3 8. Redo step 4 --> Note that the patron has a fine
Created attachment 144528 [details] [review] BUG 32271 : Fix overdue fines cap (amount) set to 0.00 when editing rule. Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
I tested with system preference CalculateFinesOnReturn.
There is also recall_overdue_fine, but it should not be a problem as it's not a max. However, shouldn't we revert the changes made to bug 20395 and restore the formatting instead of completely removing it?
I think the problem is not fixed in the right spot. Displaying the fine with comma is one thing, but the problem appears on editing. When the rule values are copied to the inputs for editing, they need to be "unformatted". Does that make sense?
We are open to anything but this needs some direction. If two QAs put the bz on pause to discuss, and a third put it "In Discussion", we mere mortals can't add much. Our team can code this many ways, but we need a patch to get through. Sooner than later.
I think bug 33028 is going in the right direction here... marking this one as a duplicate. *** This bug has been marked as a duplicate of bug 33028 ***
(In reply to Martin Renvoize from comment #10) > I think bug 33028 is going in the right direction here... marking this one > as a duplicate. > > *** This bug has been marked as a duplicate of bug 33028 *** It's actually not. The problem remains. I am deduplicating.
*** Bug 32905 has been marked as a duplicate of this bug. ***
To summarize (and because I already had that typed up for the duplicate bug: * In 20.11 (already using circulation_rules), if you left overdue fines cap empty, it remained empty on saving. This meant that fines would be calculated without a limit or only limited by the MaxFine system preference. * In master and other later versions we have started to convert empty values on saving to 0.00 which means: no fines will calculate. This is especially an issue for older databases that have the empty values and unknowingly change them when editing other values in circulation rules. -> chaos.
I don't believe the patch attached fully works. I made this change on bug 33028 and I still see the 0.00. We need to dig a little deeper. But I am splitting off this bug from 33028 again, because I hope to get that resolved as it's almost there. Both patch sets would need backporting later.
It would be great to get more eyes on this one. We are now nearly there with bug 33028 and I think this is the missing piece to get us back on track again.
*** Bug 34279 has been marked as a duplicate of this bug. ***
When I enter "0" with this patch, I get the error: Exception 'Koha::Exceptions::BadParameter' thrown 'set_rule expected decimal' at /usr/share/perl5/Exception/Class/Base.pm line 88
I have the same result as Kyle: Exception 'Koha::Exceptions::BadParameter' thrown 'set_rule expected decimal' at /usr/share/perl5/Exception/Class/Base.pm line 88
Created attachment 153486 [details] [review] Bug 32271: (QA follow-up) Convert all positive numbers to monetary float and convert all 0 equivilents to blank value
Created attachment 153487 [details] [review] Bug 32271: (QA follow-up) Convert all positive numbers to monetary float and convert all 0 equivilents to blank value Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 153488 [details] [review] Bug 32271: Fix overdue fines cap (amount) set to 0.00 when editing rule. Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 153489 [details] [review] Bug 32271: (QA follow-up) Convert all positive numbers to monetary float and convert all 0 equivilents to blank value Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Now we have the following behaviour: overdue fines cap = ofd * ofd empty on editing, stays empty after saving * ofd 0.00 on editing, will be empty after saving * ofd changed to 0.00, will be empty after saving * ofd changed to 0, will be empty after saving I believe with bug 34279 that will give us some nice consistency in behaviour again.
Created attachment 153638 [details] [review] Bug 32271: Fix overdue fines cap (amount) set to 0.00 when editing rule. Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153639 [details] [review] Bug 32271: (QA follow-up) Convert all positive numbers to monetary float and convert all 0 equivilents to blank value Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153640 [details] [review] Bug 32271: Unit test Add a unit test for the allow blank option added to the monetary check in CirculationRules. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153641 [details] [review] Bug 32271: (QA follow-up) Correction to 'can_be_blank' logic The logic introducing the can_be_blank check into the monetary decimal format check was flawed and meant we were no longer checking decimal formatting in the majority of cases. This patch updates that so we pass the unit tests and correctly check any value passed that's not an empty string. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 153642 [details] [review] Bug 32271: (QA follow-up) Restore $Price filtering for valid values Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Having a look here too today
timethis 500: 1 wallclock secs ( 0.64 usr + 0.06 sys = 0.70 CPU) @ 714.29/s (n=500) timethis 500: 1 wallclock secs ( 0.73 usr + 0.02 sys = 0.75 CPU) @ 666.67/s (n=500) timethis 500: 1 wallclock secs ( 0.67 usr + 0.03 sys = 0.70 CPU) @ 714.29/s (n=500) timethis 500: 1 wallclock secs ( 0.72 usr + 0.04 sys = 0.76 CPU) @ 657.89/s (n=500)
(In reply to Marcel de Rooy from comment #30) > timethis 500: 1 wallclock secs ( 0.64 usr + 0.06 sys = 0.70 CPU) @ > 714.29/s (n=500) > timethis 500: 1 wallclock secs ( 0.73 usr + 0.02 sys = 0.75 CPU) @ > 666.67/s (n=500) > timethis 500: 1 wallclock secs ( 0.67 usr + 0.03 sys = 0.70 CPU) @ > 714.29/s (n=500) > timethis 500: 1 wallclock secs ( 0.72 usr + 0.04 sys = 0.76 CPU) @ > 657.89/s (n=500) Not from this patch. But we should catch the output.
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.03
Nice work everyone! Pushed to 22.11.x for next release