As agreed on IRC today, recalculation of fines on return needs to be tied to a system preference as it changes the fines workflow. Ideally the system preference would have 3 settings: - recalculate always - recalculate only for hourly loans - don't recalculate
There is currently no way to know something is an hourly loan or not. So the first implementation of this will only be on or off. If someone wants to change circulation to store if something was an hourly loan or not on the issue row then it could be expanded to 3 choices.
Created attachment 17824 [details] [review] Bug 10120 : Adding a syspref to control if overdue charges are updated /on return On by default, To Test 1/ Create an overdue item, that should get fines 2/ Return the item 3/ Check the borrowers record to see if the fine has been added/updated Apply patch 1/ Make sure preference is set to do Repeat steps 1-3 above 2/ Switch the preference to don't Repeat stes 1-2 3/ Check the fine hasn't been added/updated
Created attachment 17825 [details] [review] Bug 10120 : Tidy up the code so we can see what is going on
Only briefly tested, please test thoroughly
Created attachment 17885 [details] [review] [Signed off] Bug 10120 : Adding a syspref to control if overdue charges are updated /on return On by default, To Test 1/ Create an overdue item, that should get fines 2/ Return the item 3/ Check the borrowers record to see if the fine has been added/updated Apply patch 1/ Make sure preference is set to do Repeat steps 1-3 above 2/ Switch the preference to don't Repeat stes 1-2 3/ Check the fine hasn't been added/updated Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 17886 [details] [review] [Signed off] Bug 10120 : Adding a syspref to control if overdue charges are updated /on return On by default, To Test 1/ Create an overdue item, that should get fines 2/ Return the item 3/ Check the borrowers record to see if the fine has been added/updated Apply patch 1/ Make sure preference is set to do Repeat steps 1-3 above 2/ Switch the preference to don't Repeat stes 1-2 3/ Check the fine hasn't been added/updated Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 17887 [details] [review] [Signed off] Bug 10120 : Tidy up the code so we can see what is going on Signed-off-by: David Cook <dcook@prosentient.com.au>
Works as described. I doubt a library would want to have this system preference on for any case except hourly loans, but since there is no way to tell if a loan is hourly or not, this binary method is the best we've got.
Created attachment 18143 [details] [review] [PASSED QA] Bug 10120 : Tidy up the code so we can see what is going on Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Amended test plan used for testing: Note: Use a new item for each test as we determine fines by item number. If you have the same item twice on the same day on the same account with the same due date, fine calculation is going to fail. 1/ Create an overdue item, that should get fines 2/ Return the item from the patron account checkout tab 3/ Check the borrowers record to see if the fine has been added/updated Repeat, but for 2/ return the item from the check in page Apply patch 1/ Make sure preference is set to do Repeat steps 1-3 from above 2/ Switch the preference to don't Repeat steps 1-2 3/ Check the fine hasn't been added/updated Again, check for returns from patron account and check in page.
Created attachment 18144 [details] [review] [PASSED QA] Bug 10120 : Adding a syspref to control if overdue charges are updated /on return On by default, To Test 1/ Create an overdue item, that should get fines 2/ Return the item 3/ Check the borrowers record to see if the fine has been added/updated Apply patch 1/ Make sure preference is set to do Repeat steps 1-3 above 2/ Switch the preference to don't Repeat stes 1-2 3/ Check the fine hasn't been added/updated Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass, works as described. I would categorize this a bug fix for libraries that don't want the new changed behaviour that was introduced by recalculating fines on return.
Created attachment 18145 [details] [review] [PASSED QA] Bug 10120 : Tidy up the code so we can see what is going on Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Amended test plan used for testing: Note: Use a new item for each test as we determine fines by item number. If you have the same item twice on the same day on the same account with the same due date, fine calculation is going to fail. 1/ Create an overdue item, that should get fines 2/ Return the item from the patron account checkout tab 3/ Check the borrowers record to see if the fine has been added/updated Repeat, but for 2/ return the item from the check in page Apply patch 1/ Make sure preference is set to do Repeat steps 1-3 from above 2/ Switch the preference to don't Repeat steps 1-2 3/ Check the fine hasn't been added/updated Again, check for returns from patron account and check in page.
String changes, so this will be held for 3.14/3.12.1.
Back to master.
For future reference, the IRC discussion in question starts at http://irc.koha-community.org/koha/2013-04-25#i_1239245
And a general comment: perhaps we should consider having loan records either store a copy of the circ policy that applies to the loan (e.g., issuelength, lengthunit, fine, finedays, etc.) or a reference to it (e.g,. by first giving issuingrules an opaque PK, then adding that as a FK to issues).
And of course, Chris already suggested that. :)
I have pushed the main patch to master, though with a bit of reluctance as I suspect the need for this syspref will go away if/when the underpinnings of fine accounting are improved. I have *not* pushed the follow-up whitespace patch; it might just be me, but I found that it made that section of code *less* readable, not more. Among other reasons, I really prefer that comments be indented at the same level as the block they apply to. I suggest that we just plan on running C4/Circulation.pm through perltidy at some point in the near future. Thanks, Chris! RMaint 3.12 please take note: I believed that this is considered a blocker by at least one library to upgrading.
This patch has been pushed to 3.12.x, will be in 3.12.1.
Pushed to 3.10.x, will be in 3.10.7
Created attachment 44078 [details] [review] Bug 10120: (QA followup) avoid raising warnings on upgrade Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>