http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 Melia Meggs <melia@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |critical -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18172 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18172&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 Kyle M Hall <kyle.m.hall@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle.m.hall@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 --- Comment #4 from Mickey Coalwell <mcoalwell@nekls.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 Melia Meggs <melia@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #6 from Melia Meggs <melia@bywatersolutions.com> --- 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! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 Melia Meggs <melia@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18175|0 |1 is obsolete| | --- Comment #8 from Melia Meggs <melia@bywatersolutions.com> --- Created attachment 18322 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18322&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |10120 --- Comment #10 from Chris Cormack <chris@bigballofwax.co.nz> --- 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) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18322|0 |1 is obsolete| | --- Comment #11 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 18393 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18393&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 --- Comment #13 from Kyle M Hall <kyle.m.hall@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 --- Comment #15 from Kyle M Hall <kyle.m.hall@gmail.com> ---

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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10262 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |tomascohen@gmail.com --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This patch has been pushed to 3.12.x, will be in 3.12.1. Thanks Kyle for the fix! -- You are receiving this mail because: You are watching all bug changes.