Bug 10262 - fine calculation at checkin not respecting CircControl
Summary: fine calculation at checkin not respecting CircControl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on: 10120
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-16 17:56 UTC by Melia Meggs
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10262 - fine calculation at checkin not respecting CircControl (4.19 KB, patch)
2013-05-16 18:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10262 - fine calculation at checkin not respecting CircControl (4.19 KB, patch)
2013-05-16 18:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
[Signed off] Bug 10262 - fine calculation at checkin not respecting CircControl (4.25 KB, patch)
2013-05-16 20:12 UTC, Melia Meggs
Details | Diff | Splinter Review
[Signed off] Bug 10262 - fine calculation at checkin not respecting CircControl (4.31 KB, patch)
2013-05-22 20:21 UTC, Melia Meggs
Details | Diff | Splinter Review
Bug 10262 - fine calculation at checkin not respecting CircControl (4.59 KB, patch)
2013-05-24 20:45 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Melia Meggs 2013-05-16 17:56:26 UTC
The fine calculation at checkin is not respecting CircControl the way fines.pl does.

(More details and test plan coming with a patch soon.)
Comment 1 Kyle M Hall 2013-05-16 18:15:30 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-05-16 18:16:57 UTC Comment hidden (obsolete)
Comment 3 Mickey Coalwell 2013-05-16 19:03:51 UTC
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
Comment 4 Mickey Coalwell 2013-05-16 20:07:45 UTC
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
Comment 5 Melia Meggs 2013-05-16 20:12:30 UTC Comment hidden (obsolete)
Comment 6 Melia Meggs 2013-05-16 20:13:19 UTC
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!
Comment 7 Johnny Rotten 2013-05-22 18:27:16 UTC
I tested this on the VALNet production server and the patch works as expected.

George Williams
georgew at latahlibrary.org
Comment 8 Melia Meggs 2013-05-22 20:21:29 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2013-05-24 20:35:49 UTC
This bug is present on 3.12.x and master also
Comment 10 Chris Cormack 2013-05-24 20:42:42 UTC
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)
Comment 11 Chris Cormack 2013-05-24 20:45:25 UTC
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>
Comment 12 Galen Charlton 2013-05-28 15:32:25 UTC
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
Comment 13 Kyle M Hall 2013-05-28 19:25:43 UTC
(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?
Comment 14 Galen Charlton 2013-05-28 21:38:23 UTC
(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.
Comment 15 Kyle M Hall 2013-05-29 11:27:42 UTC
> 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.
Comment 16 Tomás Cohen Arazi 2013-06-04 15:56:55 UTC
This patch has been pushed to 3.12.x, will be in 3.12.1.

Thanks Kyle for the fix!
Comment 17 Bernardo Gonzalez Kriegel 2013-06-10 22:50:29 UTC
Pushed to 3.10.x, will be in 3.10.7