Bug 11872 - Lost overdue items should not generate fines
Summary: Lost overdue items should not generate fines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 9559 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-02-28 14:44 UTC by Kyle M Hall
Modified: 2015-12-03 22:02 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11872 - Lost overdue items should not generate fines (1.68 KB, patch)
2014-02-28 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11872 - Lost overdue items should not generate fines (1.72 KB, patch)
2014-03-04 18:40 UTC, wajasu
Details | Diff | Splinter Review
Bug 11872 [QA Followup] - Unit Tests (1.86 KB, patch)
2014-06-25 16:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11872 - Lost overdue items should not generate fines (2.65 KB, patch)
2014-07-29 16:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11872 - Lost overdue items should not generate fines (2.70 KB, patch)
2014-11-25 15:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 11872 - Lost overdue items should not generate fines (2.79 KB, patch)
2014-11-27 09:00 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-02-28 14:44:34 UTC
An item can be marked as lost by longoverdue.pl, but left checked out to the patron. In this case, the item will continue to accrue fines.
Comment 1 Kyle M Hall 2014-02-28 14:47:44 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-02-28 14:48:41 UTC
*** Bug 9559 has been marked as a duplicate of this bug. ***
Comment 3 wajasu 2014-03-04 18:40:22 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-03-18 14:38:29 UTC
At least one unit test is required for this change.
Marked as Failed QA.
Comment 5 Kyle M Hall 2014-06-25 16:29:46 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2014-06-26 08:32:24 UTC
Hi Kyle,
I am wondering if the 2 routines checkoverdues and GetOverduesForBranch should not be changed too.
If you consider that lost items are not overdues, both routines should be changed too, but I am not sure this is right.
Otherwise, a routine named "Getoverdues" should return all overdues (even lost items). In this case you could remove the lost items in the 2 scripts calling this routine.
Comment 7 Katrin Fischer 2014-06-26 08:44:54 UTC
I think Jonathan raises a good point here - we need to decide if lost items count as overdues and then treat them consistently. 

I tend to say they are overdue AND lost, so should still show up with your overdue items. If we want the logic in the routine, maybe it could be a parameter.
Comment 8 Katrin Fischer 2014-07-06 20:54:44 UTC
Kyle, could you please take a look at Jonathan's and my comments?
Comment 9 Kyle M Hall 2014-07-29 16:40:08 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2014-07-29 16:41:21 UTC
(In reply to Jonathan Druart from comment #6)
> Otherwise, a routine named "Getoverdues" should return all overdues (even
> lost items). In this case you could remove the lost items in the 2 scripts
> calling this routine.

I agree, I've submitted an alternative patch that uses this path to resolution.
Comment 11 Srdjan Jankovic 2014-08-15 03:00:22 UTC
I'm not saying that the patch is wrong, but can someone explain reported numbers from those scripts please. They seem to be out of sync.
Comment 12 Jonathan Druart 2014-11-25 14:46:09 UTC
(In reply to Srdjan Jankovic from comment #11)
> I'm not saying that the patch is wrong, but can someone explain reported
> numbers from those scripts please. They seem to be out of sync.

Srdjan,
Sorry but I don't understand your question.
What do you mean by "reported numbers"?
Comment 13 Jonathan Druart 2014-11-25 15:25:34 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-11-25 15:27:29 UTC
This patch fixes an issue, but to pass the QA step, we need tests on C4::Overdues::Getoverdues.
Maybe additional documentations (doc + help online) could be great.
Comment 15 Srdjan Jankovic 2014-11-27 05:28:01 UTC
(In reply to Jonathan Druart from comment #12)

> Srdjan,
> Sorry but I don't understand your question.
> What do you mean by "reported numbers"?

I do apologise, I cannot remember myself any more. I probably meant "what is the difference between fines.pl and staticfines.p, they report different numbers". But on the other hand, why would there be two scripts if they did the same thing...
Comment 16 Martin Renvoize 2014-11-27 09:00:40 UTC
Created attachment 33980 [details] [review]
[PASSED QA] Bug 11872 - Lost overdue items should not generate fines

An item can be marked as lost by longoverdue.pl, but left checked out to
the patron. In this case, the item will continue to accrue fines.

Test Plan:
1) Check out an item and back date it so it is overdue and should
   generate fines.
2) Mark the item as lost by either using longoverdue.pl, or just
   by setting itemlost to 1 by directly accessing the database
3) Run fines.pl
4) Note the overdue generated a fine
5) Repeat steps 1-2
6) Apply this patch
7) Run fines.pl
8) Note a fine was not generated

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2014-11-27 09:01:40 UTC
Works as expected, super simple patch, passes qa scripts..

Passing QA
Comment 18 Tomás Cohen Arazi 2015-01-07 23:44:42 UTC
Patch pushed to master.

Thanks Kyle!
Comment 19 Chris Cormack 2015-01-11 08:06:39 UTC
Pushed to 3.18.x will be in 3.18.3
Comment 20 Mason James 2015-01-16 23:26:41 UTC
Pushed to 3.16.x, will be in 3.16.7
Comment 21 Fridolin Somers 2015-03-23 15:27:46 UTC
Pushed to 3.14, will be in 3.14.14