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.
Created attachment 153471 [details] [review] Bug 34279: Unit tests
Created attachment 153472 [details] [review] Bug 34279: Don't enforce overduefinescap unless it is greater than 0
Testing...
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 ***
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.
Created attachment 153475 [details] [review] Bug 34279: Unit tests
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!
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>
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.
Created attachment 153484 [details] [review] Bug 34279: Unit tests Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
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>
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>
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>
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
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
Backported to 22.05.x for upcoming 22.05.16