Bug 34279 - overduefinescap of 0 is ignored, but overduefinescap of 0.00 is enforced
Summary: overduefinescap of 0 is ignored, but overduefinescap of 0.00 is enforced
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-07-14 11:28 UTC by Kyle M Hall
Modified: 2023-12-28 20:47 UTC (History)
5 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,22.05.16


Attachments
Bug 34279: Unit tests (3.09 KB, patch)
2023-07-14 11:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34279: Don't enforce overduefinescap unless it is greater than 0 (1.07 KB, patch)
2023-07-14 11:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34279: Unit tests (3.09 KB, patch)
2023-07-14 12:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34279: Don't enforce overduefinescap unless it is greater than 0 (1.98 KB, patch)
2023-07-14 12:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 34279: Don't enforce overduefinescap unless it is greater than 0 (3.72 KB, patch)
2023-07-14 12:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34279: Unit tests (3.15 KB, patch)
2023-07-14 12:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34279: Don't enforce overduefinescap unless it is greater than 0 (3.72 KB, patch)
2023-07-14 12:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34279: Unit tests (3.21 KB, patch)
2023-07-19 13:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34279: Don't enforce overduefinescap unless it is greater than 0 (3.78 KB, patch)
2023-07-19 13:07 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 Kyle M Hall 2023-07-14 11:28:08 UTC
When creating a circ rule, we can set overduefinescap to blank or 0 and no cap is enforced. If we edit that rule, the blank/0 is converted to "0.00" which perl considers true, thus zero-ing out any calculated fine.

Considering we've always ignored an overdue fines cap of 0, we should also ignore 0.00. However, perl is evaluating it as a string which makes it true instead of false as 0 is.
Comment 1 Kyle M Hall 2023-07-14 11:37:04 UTC
Created attachment 153471 [details] [review]
Bug 34279: Unit tests
Comment 2 Kyle M Hall 2023-07-14 11:37:10 UTC
Created attachment 153472 [details] [review]
Bug 34279: Don't enforce overduefinescap unless it is greater than 0
Comment 3 Katrin Fischer 2023-07-14 11:44:10 UTC
Testing...
Comment 4 Katrin Fischer 2023-07-14 11:48:47 UTC
I think this is an indirect duplicate of bug 32271.

The difference between 0 and 0.00 is hard to understand.

It used to be you could leave it empty and it would count as unlimited - the same behaviour we have for checkouts.

I still see this behaviour in 20.11 (don't have the versions in between to test).

I think it would be better than relying on 0 vs. 0.00 - what do you think?

*** This bug has been marked as a duplicate of bug 32271 ***
Comment 5 Katrin Fischer 2023-07-14 11:55:19 UTC
To make sure I didn't misunderstand, I tested the patch and it dosn't work:

* Apply patch
* restart_all
* Add a circulation rule, set Overdue fines cap to 0, save:
  It still shows as 0.00
* Edit a circulation rule, set Overdue fines cap to empty, save:
  It still shows as 0.00

I think the duplicates could also be the other way around, but neither patch works.
Comment 6 Kyle M Hall 2023-07-14 12:17:34 UTC
Created attachment 153475 [details] [review]
Bug 34279: Unit tests
Comment 7 Kyle M Hall 2023-07-14 12:17:40 UTC
Created attachment 153476 [details] [review]
Bug 34279: Don't enforce overduefinescap unless it is greater than 0

When creating a circ rule, we can set overduefinescap to blank or 0 and no cap is enforced. If we edit that rule, the blank/0 is converted to "0.00" which perl considers true, thus zero-ing out any calculated fine.

Considering we've always ignored an overdue fines cap of 0, we should also ignore 0.00. However, perl is evaluating it as a string which makes it true instead of false as 0 is.

Test Plan:
1) Apply the first patch ( unit tests )
2) prove t/db_dependent/Circulation/CalcFine.t
3) Note the test fails
4) Apply the second patch as well
5) prove t/db_dependent/Circulation/CalcFine.t
6) Note the test passes

Test Plan 2:
1) Create an all/all/all rule with an overduefinescap of 0.00, with a
   daily fine. Enable CalculateFinesOnReturn
2) Backdate a checkout so it is overdue
3) Return this item, note the lack of a fine
4) Apply this patch set
5) Backdate a checkout and return it again
6) Note the fine is generated!
Comment 8 Katrin Fischer 2023-07-14 12:57:28 UTC
Created attachment 153483 [details] [review]
Bug 34279: Don't enforce overduefinescap unless it is greater than 0

When creating a circ rule, we can set overduefinescap to blank or 0 and no cap is enforced. If we edit that rule, the blank/0 is converted to "0.00" which perl considers true, thus zero-ing out any calculated fine.

Considering we've always ignored an overdue fines cap of 0, we should also ignore 0.00. However, perl is evaluating it as a string which makes it true instead of false as 0 is.

Test Plan:
1) Apply the first patch ( unit tests )
2) prove t/db_dependent/Circulation/CalcFine.t
3) Note the test fails
4) Apply the second patch as well
5) prove t/db_dependent/Circulation/CalcFine.t
6) Note the test passes

Test Plan 2:
1) Create an all/all/all rule with an overduefinescap of 0.00, with a
   daily fine. Enable CalculateFinesOnReturn
2) Backdate a checkout so it is overdue
3) Return this item, note the lack of a fine
4) Apply this patch set
5) Backdate a checkout and return it again
6) Note the fine is generated!

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 9 Katrin Fischer 2023-07-14 12:59:08 UTC
Kyle and I discussed this and bug 32271 on IRC. This fixes a regression and somewhat helps with bug 32271 as editing a rule with empty changed to 0.00 will no longer turn off your fine calculation.
Comment 10 Katrin Fischer 2023-07-14 12:59:56 UTC
Created attachment 153484 [details] [review]
Bug 34279: Unit tests

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 11 Katrin Fischer 2023-07-14 12:59:59 UTC
Created attachment 153485 [details] [review]
Bug 34279: Don't enforce overduefinescap unless it is greater than 0

When creating a circ rule, we can set overduefinescap to blank or 0 and no cap is enforced. If we edit that rule, the blank/0 is converted to "0.00" which perl considers true, thus zero-ing out any calculated fine.

Considering we've always ignored an overdue fines cap of 0, we should also ignore 0.00. However, perl is evaluating it as a string which makes it true instead of false as 0 is.

Test Plan:
1) Apply the first patch ( unit tests )
2) prove t/db_dependent/Circulation/CalcFine.t
3) Note the test fails
4) Apply the second patch as well
5) prove t/db_dependent/Circulation/CalcFine.t
6) Note the test passes

Test Plan 2:
1) Create an all/all/all rule with an overduefinescap of 0.00, with a
   daily fine. Enable CalculateFinesOnReturn
2) Backdate a checkout so it is overdue
3) Return this item, note the lack of a fine
4) Apply this patch set
5) Backdate a checkout and return it again
6) Note the fine is generated!

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 12 Martin Renvoize 2023-07-19 13:07:26 UTC
Created attachment 153648 [details] [review]
Bug 34279: Unit tests

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2023-07-19 13:07:28 UTC
Created attachment 153649 [details] [review]
Bug 34279: Don't enforce overduefinescap unless it is greater than 0

When creating a circ rule, we can set overduefinescap to blank or 0 and no cap is enforced. If we edit that rule, the blank/0 is converted to "0.00" which perl considers true, thus zero-ing out any calculated fine.

Considering we've always ignored an overdue fines cap of 0, we should also ignore 0.00. However, perl is evaluating it as a string which makes it true instead of false as 0 is.

Test Plan:
1) Apply the first patch ( unit tests )
2) prove t/db_dependent/Circulation/CalcFine.t
3) Note the test fails
4) Apply the second patch as well
5) prove t/db_dependent/Circulation/CalcFine.t
6) Note the test passes

Test Plan 2:
1) Create an all/all/all rule with an overduefinescap of 0.00, with a
   daily fine. Enable CalculateFinesOnReturn
2) Backdate a checkout so it is overdue
3) Return this item, note the lack of a fine
4) Apply this patch set
5) Backdate a checkout and return it again
6) Note the fine is generated!

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2023-07-19 13:08:51 UTC
All works as expected, tests included and QA script happy.. no regressions spotted.

Passing QA.

I tested this on top of bugs 33028 and 32271
Comment 15 Tomás Cohen Arazi 2023-07-19 16:00:17 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 16 Fridolin Somers 2023-08-04 01:43:30 UTC
Pushed to 23.05.x for 23.05.03
Comment 17 Pedro Amorim 2023-08-18 10:53:45 UTC
Nice work everyone!

Pushed to 22.11.x for next release
Comment 18 Lucas Gass 2023-09-13 20:58:57 UTC
Backported to 22.05.x for upcoming 22.05.16