Bug 10030 - MaxFines checks against amount, not amount outstanding
Summary: MaxFines checks against amount, not amount outstanding
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-11 19:46 UTC by Kyle M Hall
Modified: 2019-03-20 13:34 UTC (History)
4 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 10030 - MaxFines checks against amount, not amount outstanding (1.38 KB, patch)
2013-04-11 19:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10030 - MaxFines checks against amount, not amount outstanding (1.43 KB, patch)
2013-04-20 10:40 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10030 - MaxFines checks against amount, not amount outstanding (1.53 KB, patch)
2013-05-15 14:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10030: QA Followup for trivial false warning detected while testing (1.48 KB, patch)
2013-05-15 14:21 UTC, Marcel de Rooy
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 2013-04-11 19:46:45 UTC
If a patron has over time accumulated fines greater than the amount defined in MaxFine, the patron will never get more fines even if they have previously paid off those fines.

This bug was introduced by the patch for Bug 7420.
Comment 1 Kyle M Hall 2013-04-11 19:50:00 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2013-04-20 10:40:17 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-04-25 06:32:07 UTC
Working on this.
Comment 4 Katrin Fischer 2013-04-25 06:52:41 UTC
Kyle, I see some strange behaviour with your patch applied:

System preferences:
    maxfine: empty
Circulation rules: 
    mpl-staff-books
    maxfine 15.00
    fine 5.00
    grace period: 0
    interval 1

Logged in branch: mpl
Home library of items: mpl
Patron home library: mpl

I set the due date to about 8-9 days ago using the specify due date feature.
I then returned the book using the checkbox on the checkout tab.
Suddenly I have credit of 15.00 on my patron account...
Couldn't repeat that switching back to master.

Could you please check?
Comment 5 Katrin Fischer 2013-04-25 06:56:41 UTC
Now I can't reproduce on master too - or with your patch. It happened 3 times, I have no time now to figure out what's going on there. Someone taking a closer look would be appreciated.
Comment 6 Katrin Fischer 2013-05-15 06:40:07 UTC
The problems I experienced were related to the fact that you should use 'new' items/due dates to make sure this works consistently.

I am continuing the tests, but reading the code of the sub I couldn't help wondering - do we really want to count in Sundry fines?

http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Overdues.pm;h=bc1c7a77dbfe451180f6d4262e0351152336b197;hb=1b877167f5231f7eef6c7f82cba9322bdba0dbac#l502
Comment 7 Marcel de Rooy 2013-05-15 14:07:50 UTC
Looking at this one now..
Comment 8 Marcel de Rooy 2013-05-15 14:21:16 UTC
Created attachment 18156 [details] [review]
Bug 10030 - MaxFines checks against amount, not amount outstanding

If a patron has over time accumulated fines greater than the amount
defined in MaxFine, the patron will never get more fines even if they
have previously paid off those fines.

This bug was introduced by the patch for Bug 7420.

Test Plan:
1) Create a patron
2) Create a fine of 10.00 for that patron
3) Pay off the fine
4) Set MaxFines to 5.00
5) Check out an item to the patron, backdate the due date
   so the item should generate fines.
6) Run fines.pl, observe that no fine was created
7) Apply the patch
8) Rerun fines.pl
9) Observe that the fine was created correctly

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2013-05-15 14:21:58 UTC
Created attachment 18157 [details] [review]
Bug 10030: QA Followup for trivial false warning detected while testing

Moving the warn line below the line that returns if amount<=0.
If amount<=0, a false warn is now raised because of the return after it.
We should only warn here if we do not return.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with fines.pl on overdue.
Before this patch:
Reducing fine for item 199709 borrower 23 from 44 to -1 - MaxFine reached.
This did not happen however because of the return.
After this change: no false warning.
Comment 10 Marcel de Rooy 2013-05-15 14:25:31 UTC
QA Comment:
Tested. Works. Added a trivial followup too for false warning raised (detected while testing this).
Simple code change looks good.

The question Katrin raised:
> I am continuing the tests, but reading the code of the sub I couldn't help wondering - do we really want to count in Sundry fines?
This question is certainly valid, but imo falls outside the scope of this report. This patch just fixes the wrongly used column in adding the total amount.

Passed QA
Comment 11 Katrin Fischer 2013-05-15 14:28:48 UTC
Marcel, totally agreed - thx for QA'ing! :) 
I will try to write up a new bug for the other problem.
Comment 12 Jared Camins-Esakov 2013-05-16 01:52:39 UTC
This patch has been pushed to master and 3.12.x.