The fine calculation at checkin is not respecting CircControl the way fines.pl does. (More details and test plan coming with a patch soon.)
Created attachment 18172 [details] [review] Bug 10262 - fine calculation at checkin not respecting CircControl The fines.pl script uses the system preference CircControl to decide what branches circ rules to use for fine generation. Recently, code was added to the returns system to recalculate the fine at checkin time ( to support hourly loans ). The problem is that this code does not respect CircControl. Test Plan: 1) Set circ control to "the library you are logged in at" 2) Set different fines rules for two different librarys 3) Check an item out at library A, backdate the due date so it's overdue and will have fines. 4) Check the item in at library B 5) Observe that the fines should be generated based on library A's rules, but the fines will be based on library B's rules instead! 5) Apply the patch 6) Repeat steps 3 and 4. 7) Observe now that the fines should reflect the fines rules for Library A Note: it seems counter-intuitive for the fines system to behave this way based on the preference being set to "the library you are logged in at" but it does make sense. The rules used are from "the library you are logged in at" when the item is first checked out. If the fines system really did use the rules for the library the item was returned to, it would be easy to exploit the library system. Some Koha using system have branches that charge fines, and others that don't, so a patron could just return any overdue item's to a non-charging branch to avoid ever paying fines! Furthermore, it would mean that the fines.pl script would be using one set of rules to charge fines, and the returns system could possibly be using another. Since fines.pl has been around far longer, it makes sense to assume the fines.pl behavior is canonical.
Created attachment 18173 [details] [review] Bug 10262 - fine calculation at checkin not respecting CircControl The fines.pl script uses the system preference CircControl to decide what branches circ rules to use for fine generation. Recently, code was added to the returns system to recalculate the fine at checkin time ( to support hourly loans ). The problem is that this code does not respect CircControl. Test Plan: 1) Set circ control to "the library you are logged in at" 2) Set different fines rules for two different librarys 3) Check an item out at library A, backdate the due date so it's overdue and will have fines. 4) Check the item in at library B 5) Observe that the fines should be generated based on library A's rules, but the fines will be based on library B's rules instead! 5) Apply the patch 6) Repeat steps 3 and 4. 7) Observe now that the fines should reflect the fines rules for Library A Note: it seems counter-intuitive for the fines system to behave this way based on the preference being set to "the library you are logged in at" but it does make sense. The rules used are from "the library you are logged in at" when the item is first checked out. If the fines system really did use the rules for the library the item was returned to, it would be easy to exploit the library system. Some Koha using systems have branches that charge fines, and others that don't, so a patron could just return any overdue items to a non-charging branch to avoid ever paying fines! Furthermore, it would mean that the fines.pl script would be using one set of rules to charge fines, and the returns system could possibly be using another. Since fines.pl has been around far longer, it makes sense to assume the fines.pl behavior is canonical.
The problem was first identified by a situation in which fines were applied to an overdue item which was checked out an "no-fines" library was checked in at a fining library. I would want to see this situation tested as well. Did the overdue returned item check in fine-free at a fining library? Thanks, Mickey
I tested an overdue item checked out at a non-fining library and checked it in at a fining library. No fines accrued. Works for me. Mickey
Created attachment 18175 [details] [review] [Signed off] Bug 10262 - fine calculation at checkin not respecting CircControl The fines.pl script uses the system preference CircControl to decide what branches circ rules to use for fine generation. Recently, code was added to the returns system to recalculate the fine at checkin time ( to support hourly loans ). The problem is that this code does not respect CircControl. Test Plan: 1) Set circ control to "the library you are logged in at" 2) Set different fines rules for two different librarys 3) Check an item out at library A, backdate the due date so it's overdue and will have fines. 4) Check the item in at library B 5) Observe that the fines should be generated based on library A's rules, but the fines will be based on library B's rules instead! 5) Apply the patch 6) Repeat steps 3 and 4. 7) Observe now that the fines should reflect the fines rules for Library A Note: it seems counter-intuitive for the fines system to behave this way based on the preference being set to "the library you are logged in at" but it does make sense. The rules used are from "the library you are logged in at" when the item is first checked out. If the fines system really did use the rules for the library the item was returned to, it would be easy to exploit the library system. Some Koha using systems have branches that charge fines, and others that don't, so a patron could just return any overdue items to a non-charging branch to avoid ever paying fines! Furthermore, it would mean that the fines.pl script would be using one set of rules to charge fines, and the returns system could possibly be using another. Since fines.pl has been around far longer, it makes sense to assume the fines.pl behavior is canonical. Signed-off-by: Mickey Coalwell <mcoalwell@nekls.org>
Mickey has done before and after tests on his production server, and this patch is working, so I am helping with the Git part of the sign off. Thanks, Mickey!
I tested this on the VALNet production server and the patch works as expected. George Williams georgew at latahlibrary.org
Created attachment 18322 [details] [review] [Signed off] Bug 10262 - fine calculation at checkin not respecting CircControl The fines.pl script uses the system preference CircControl to decide what branches circ rules to use for fine generation. Recently, code was added to the returns system to recalculate the fine at checkin time ( to support hourly loans ). The problem is that this code does not respect CircControl. Test Plan: 1) Set circ control to "the library you are logged in at" 2) Set different fines rules for two different librarys 3) Check an item out at library A, backdate the due date so it's overdue and will have fines. 4) Check the item in at library B 5) Observe that the fines should be generated based on library A's rules, but the fines will be based on library B's rules instead! 5) Apply the patch 6) Repeat steps 3 and 4. 7) Observe now that the fines should reflect the fines rules for Library A Note: it seems counter-intuitive for the fines system to behave this way based on the preference being set to "the library you are logged in at" but it does make sense. The rules used are from "the library you are logged in at" when the item is first checked out. If the fines system really did use the rules for the library the item was returned to, it would be easy to exploit the library system. Some Koha using systems have branches that charge fines, and others that don't, so a patron could just return any overdue items to a non-charging branch to avoid ever paying fines! Furthermore, it would mean that the fines.pl script would be using one set of rules to charge fines, and the returns system could possibly be using another. Since fines.pl has been around far longer, it makes sense to assume the fines.pl behavior is canonical. Signed-off-by: Mickey Coalwell <mcoalwell@nekls.org> Signed-off-by: George Williams <georgew@latahlibrary.org>
This bug is present on 3.12.x and master also
In order to QA this patch on master, I needed to fix a conflict with bug 10120. This makes this patch now dependent on it. If 10120 is not pushed to 3.12.x and 3.10.x then the original patch would work (I'd of course prefer 10120 to be pushed as it is blocking some of my libraries from upgrading)
Created attachment 18393 [details] [review] Bug 10262 - fine calculation at checkin not respecting CircControl The fines.pl script uses the system preference CircControl to decide what branches circ rules to use for fine generation. Recently, code was added to the returns system to recalculate the fine at checkin time ( to support hourly loans ). The problem is that this code does not respect CircControl. Test Plan: 1) Set circ control to "the library you are logged in at" 2) Set different fines rules for two different librarys 3) Check an item out at library A, backdate the due date so it's overdue and will have fines. 4) Check the item in at library B 5) Observe that the fines should be generated based on library A's rules, but the fines will be based on library B's rules instead! 5) Apply the patch 6) Repeat steps 3 and 4. 7) Observe now that the fines should reflect the fines rules for Library A Note: it seems counter-intuitive for the fines system to behave this way based on the preference being set to "the library you are logged in at" but it does make sense. The rules used are from "the library you are logged in at" when the item is first checked out. If the fines system really did use the rules for the library the item was returned to, it would be easy to exploit the library system. Some Koha using systems have branches that charge fines, and others that don't, so a patron could just return any overdue items to a non-charging branch to avoid ever paying fines! Furthermore, it would mean that the fines.pl script would be using one set of rules to charge fines, and the returns system could possibly be using another. Since fines.pl has been around far longer, it makes sense to assume the fines.pl behavior is canonical. Signed-off-by: Mickey Coalwell <mcoalwell@nekls.org> Signed-off-by: George Williams <georgew@latahlibrary.org> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Pushed to master because this patch resolves the discrepancy, but I must confess to a number of reservations: - the same section of code sets and uses both $control_branchcode and $circControlBranch - the patch effectively adds copy-pasta rather than moving more of the fine-calculation code into a routine that could be shared by fines.pl and AddReturn() - the fact that the patch couldn't use _GetCircControlBranch and is overloading the PickupLibrary value for CircControl, while consistent with fines.pl, is IMO stretching the CircControl syspref a bit too far. - the patch include whitespace changes that obscured its purpose - this patch is crying out for DB-dependent test cases
(In reply to comment #12) > Pushed to master because this patch resolves the discrepancy, but I must > confess to a number of reservations: > > - the same section of code sets and uses both $control_branchcode and > $circControlBranch Yes, that is rather ugly. _GetCircControlBranch does a lot of under the hood logic, far more than the new statement, but I think the results would be this same. So we can unify that. > - the patch effectively adds copy-pasta rather than moving more of the > fine-calculation code into a routine that could be shared by fines.pl and > AddReturn() Agreed, that should be remedied. > - the fact that the patch couldn't use _GetCircControlBranch and is > overloading the PickupLibrary value for CircControl, while consistent with > fines.pl, is IMO stretching the CircControl syspref a bit too far. Should we make fines.pl use _GetCircControlBranch as well? > - the patch include whitespace changes that obscured its purpose My bad, I have a perltidy addiction. I just can't help myself ; ) > - this patch is crying out for DB-dependent test cases Agreed! I'm willing to followup on this to clean up this code. It seems logical to unify all this logic under the banner of _GetCircControlBranch, would you agree?
(In reply to comment #13) > Yes, that is rather ugly. _GetCircControlBranch does a lot of under the hood > logic, far more than the new statement, but I think the results would be > this same. So we can unify that. [snip] > > - the fact that the patch couldn't use _GetCircControlBranch and is > > overloading the PickupLibrary value for CircControl, while consistent with > > fines.pl, is IMO stretching the CircControl syspref a bit too far. > > Should we make fines.pl use _GetCircControlBranch as well? First we'll have to figure out what we want _GetCircControlBranch to do. However, it might be as simple as passing it an optional issues hash so that it handle the special PickupLibrary case for loans. > > - the patch include whitespace changes that obscured its purpose > > My bad, I have a perltidy addiction. I just can't help myself ; ) That addiction is best indulged in separate patches that follow the substantive ones.
> First we'll have to figure out what we want _GetCircControlBranch to do. > However, it might be as simple as passing it an optional issues hash so that > it handle the special PickupLibrary case for loans. So, _GetCircControlBranch returns the following: * If CircControl is set to "PickupLibrary", it returns the currently logged in library branchcode. * If CircControl is set to 'PatronLibrary', it returns the patron's home library branchcode * If CircControl is set to 'ItemHomeLibrary', it uses the system preference HomeOrHoldingBranch to return either the item's homebranch or its holdingbranch. So _GetCircControlBranch is actually a bit more configurable than the other code ( it behaves exactly the same except it can use either the hold or the holding branch ). In addition, if CircControl is set to ItemHomeLibrary and HomeOrHoldingBranch is set to "Holding branch" we will get very unexpected behavior for some items, as we will have some code using the item's holding branch, and some of the code using the item's home branch for circulation rule purposes. So, for _GetCircControlBranch, if we add $issue to the parameters list, and returned the branchcode of the issuing library in cases where a) CircControl is set to 'ItemHomeLibrary' and b) $issue is not undefined, I think everything will be good.
This patch has been pushed to 3.12.x, will be in 3.12.1. Thanks Kyle for the fix!
Pushed to 3.10.x, will be in 3.10.7