Bug 32271 - Overdue fines cap (amount) set to 0.00 when editing rule
Summary: Overdue fines cap (amount) set to 0.00 when editing rule
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: geraud.frappier
QA Contact: Martin Renvoize
URL:
Keywords:
: 32905 (view as bug list)
Depends on: 33028 20395
Blocks:
  Show dependency treegraph
 
Reported: 2022-11-18 15:43 UTC by Caroline Cyr La Rose
Modified: 2023-09-13 20:57 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.03,22.11.09


Attachments
BUG 32271 : Fix overdue fines cap (amount) set to 0.00 when editing rule. (1.45 KB, patch)
2022-12-05 17:40 UTC, geraud.frappier
Details | Diff | Splinter Review
BUG 32271 : Fix overdue fines cap (amount) set to 0.00 when editing rule. (1.60 KB, patch)
2022-12-09 14:03 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 32271: (QA follow-up) Convert all positive numbers to monetary float and convert all 0 equivilents to blank value (2.26 KB, patch)
2023-07-14 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32271: (QA follow-up) Convert all positive numbers to monetary float and convert all 0 equivilents to blank value (2.31 KB, patch)
2023-07-14 13:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32271: Fix overdue fines cap (amount) set to 0.00 when editing rule. (1.65 KB, patch)
2023-07-14 13:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32271: (QA follow-up) Convert all positive numbers to monetary float and convert all 0 equivilents to blank value (2.31 KB, patch)
2023-07-14 13:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32271: Fix overdue fines cap (amount) set to 0.00 when editing rule. (1.71 KB, patch)
2023-07-19 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32271: (QA follow-up) Convert all positive numbers to monetary float and convert all 0 equivilents to blank value (2.58 KB, patch)
2023-07-19 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32271: Unit test (2.46 KB, patch)
2023-07-19 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32271: (QA follow-up) Correction to 'can_be_blank' logic (1.46 KB, patch)
2023-07-19 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32271: (QA follow-up) Restore $Price filtering for valid values (1.53 KB, patch)
2023-07-19 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Caroline Cyr La Rose 2022-11-18 15:43:29 UTC
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.
Comment 1 Caroline Cyr La Rose 2022-11-18 15:45:17 UTC
I noticed this as far back as 22.05 (did not check 21.11, but 21.05 is OK).
Comment 2 Sara Brown 2022-11-22 19:52:50 UTC
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.
Comment 3 Caroline Cyr La Rose 2022-11-22 20:07:35 UTC
(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
Comment 4 geraud.frappier 2022-12-05 17:40:52 UTC
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
Comment 5 PTFS Europe Sandboxes 2022-12-09 14:03:45 UTC
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>
Comment 6 Anneli Österman 2022-12-09 14:06:17 UTC
I tested with system preference CalculateFinesOnReturn.
Comment 7 Jonathan Druart 2023-02-08 07:50:57 UTC
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?
Comment 8 Katrin Fischer 2023-02-08 13:16:16 UTC
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?
Comment 9 Blou 2023-02-21 15:06:31 UTC
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.
Comment 10 Martin Renvoize 2023-03-03 11:19:16 UTC
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 ***
Comment 11 Katrin Fischer 2023-06-21 11:10:29 UTC
(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.
Comment 12 Katrin Fischer 2023-06-21 11:10:55 UTC
*** Bug 32905 has been marked as a duplicate of this bug. ***
Comment 13 Katrin Fischer 2023-06-21 11:11:48 UTC
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.
Comment 14 Katrin Fischer 2023-06-21 11:14:14 UTC
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.
Comment 15 Katrin Fischer 2023-07-11 13:29:21 UTC
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.
Comment 16 Katrin Fischer 2023-07-14 11:48:47 UTC
*** Bug 34279 has been marked as a duplicate of this bug. ***
Comment 17 Kyle M Hall 2023-07-14 12:08:19 UTC
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
Comment 18 Katrin Fischer 2023-07-14 12:09:06 UTC
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
Comment 19 Kyle M Hall 2023-07-14 13:09:18 UTC
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
Comment 20 Katrin Fischer 2023-07-14 13:26:56 UTC
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>
Comment 21 Katrin Fischer 2023-07-14 13:31:03 UTC
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>
Comment 22 Katrin Fischer 2023-07-14 13:31:06 UTC
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>
Comment 23 Katrin Fischer 2023-07-14 13:33:54 UTC
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.
Comment 24 Martin Renvoize 2023-07-19 11:42:24 UTC
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>
Comment 25 Martin Renvoize 2023-07-19 11:42:27 UTC
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>
Comment 26 Martin Renvoize 2023-07-19 11:42:31 UTC
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>
Comment 27 Martin Renvoize 2023-07-19 11:42:34 UTC
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>
Comment 28 Martin Renvoize 2023-07-19 11:42:38 UTC
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>
Comment 29 Marcel de Rooy 2023-07-19 11:46:58 UTC
Having a look here too today
Comment 30 Marcel de Rooy 2023-07-19 12:51:01 UTC
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)
Comment 31 Marcel de Rooy 2023-07-19 12:58:05 UTC
(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.
Comment 32 Tomás Cohen Arazi 2023-07-19 16:00:08 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 33 Fridolin Somers 2023-08-04 01:42:15 UTC
Pushed to 23.05.x for 23.05.03
Comment 34 Pedro Amorim 2023-08-18 10:53:29 UTC
Nice work everyone!

Pushed to 22.11.x for next release