Bug 7420 - Add max fines to circulation matrix
Summary: Add max fines to circulation matrix
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.10
Hardware: All All
: P3 enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Ian Walls
URL:
Keywords:
Depends on: 8486
Blocks: 10490
  Show dependency treegraph
 
Reported: 2012-01-09 00:43 UTC by Srdjan Jankovic
Modified: 2013-09-02 12:16 UTC (History)
7 users (show)

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


Attachments
patch (12.45 KB, patch)
2012-01-11 03:29 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (12.49 KB, patch)
2012-01-23 07:12 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_7420: Added overduefinescap to issuingrules (12.78 KB, patch)
2012-02-07 17:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_7420: Added overduefinescap to issuingrules (13.25 KB, patch)
2012-02-07 18:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_7420: Added overduefinescap to issuingrules (13.25 KB, patch)
2012-02-07 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
follow-up patch (2.20 KB, patch)
2012-02-21 04:13 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (12.39 KB, patch)
2012-03-20 06:44 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (13.52 KB, patch)
2012-04-02 01:01 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (13.12 KB, patch)
2012-04-02 01:32 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (13.11 KB, patch)
2012-05-09 03:23 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_7420: Added overduefinescap to issuingrules (15.54 KB, patch)
2012-05-31 00:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Updated update database file to resolve conflict (15.68 KB, patch)
2012-07-11 15:14 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 7420 tiny follow-up, mysql want a space after the -- (1.08 KB, patch)
2012-07-23 16:26 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2012-01-09 00:43:24 UTC
Capped charge is applied per item.

Work to be done:
* Add a numeric overduefinescap field to the issuingrules table
* Add "Overdue Fines Cap" section at the bottom of the Corculation Rules page, inputs:
  -  item type
  -  patron category
  -  cap in $ or whatever currency
* C4::Overdues::UpdateFine() - observe overduefinescap rule when updating overdue charges - leave the charge at overduefinescap if it goes over
* create tests

How to test:
* Check that new field values stick when maintained on the circ rules matrix page
* Check that the cap is applied. This is a tricky one. A quick test would be:
  -  set the cap to a value less then the overdue charge
  -  have an overdue item
  -  run fines.pl cronjob
  -  check that the capped charge is applied
A more complex test would be to set the cap anywhere between one day charge and two day charges, and run fines.pl cronjob on two consecutive days, but I'm not sure how practical it is, or is it possible to have a testing facility that can push the date forward
Comment 1 Srdjan Jankovic 2012-01-11 03:29:44 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2012-01-11 03:31:16 UTC
It actually made no sense to place "Overdue Fines Cap" on its own, so it is in the main table
Comment 3 Srdjan Jankovic 2012-01-23 07:12:23 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-02-07 17:40:54 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-02-07 17:43:08 UTC
Tested with multiple category/item type combinations, and with global maxfine set. All appear to be working.
Comment 6 Katrin Fischer 2012-02-07 18:24:05 UTC
I was looking at the code, because this is an interesting feature and I was working on documentation for fines & notices today.

I spotted some things:

+                <th>Overdue Fines Cap ($)</th>
The Dollar sign should not be there - perhaps you can do a follow up removing it? Dollars are not the active currency everywhere :)

There are also some unconditional(?) warns in Overdue.pm.

And I wonder how that works? Perhaps worth adding some documentation to the wiki?
+######  XXX space  ########
+# This is an attempt to make .XXX versions mergeable with master version
+# Please keep new XXX patches between XXX space markers
+# Master submitters: when making a real version, please push patch outside
+# (above) the markers and leave the markers with this instructions in the file
+
Comment 7 Kyle M Hall 2012-02-07 18:39:41 UTC
(In reply to comment #6)

That's a really good point. I'll amend the patch and resubmit. I'll take a look at the warns as well.
Comment 8 Kyle M Hall 2012-02-07 18:44:18 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-02-07 18:59:06 UTC Comment hidden (obsolete)
Comment 10 Srdjan Jankovic 2012-02-07 23:46:47 UTC
Sorry about the $ sign, I just wanted to indicate quickly and space efficiently it is money, not days.

Re XXX space, I wanted to wait to see if it works before being more vocal about it. The idea is to provide a fenced area for bug request related updates (marked with XXX), so when other updates that get pushed to master do not overlap. 95% of the conflicts that I need to resolve when rebasing are in updatedatabase.
Comment 11 Kyle M Hall 2012-02-08 12:29:09 UTC
(In reply to comment #10)

I imagine the manual will explain that.

I think the XXX space should be made a separate bug and a simple patch to add that area, assuming everyone is behind the idea.
Comment 12 Srdjan Jankovic 2012-02-09 00:02:03 UTC
I can separate it, however it has been signed off. What do you want me to do?
Comment 13 Kyle M Hall 2012-02-09 11:09:36 UTC
Really, it is such a trivial code addition, I don't think splitting it off is necessary. It would be less work to remove it after the fact, if neccessary. Besides, I think it's a pretty good idea.

(In reply to comment #12)
> I can separate it, however it has been signed off. What do you want me to do?
Comment 14 Paul Poulain 2012-02-20 21:08:49 UTC
QA comments:
* the logic behind UpdateFines is changed. Maybe/probably it's worth a few comment lines in the script itself. I think it's OK, but a comment would help me
* the indent of your patch is 2 spaces, should be 4 (PERL6 coding guideline)

marking failed QA, once the 2 comments are fixed (either by a follow-up or a new patch), directly go to signed off, as it won't change anything to the feature itself, thanks
Comment 15 Srdjan Jankovic 2012-02-21 04:13:57 UTC Comment hidden (obsolete)
Comment 16 Ian Walls 2012-03-12 11:05:23 UTC
First patch renames 'MaxFine' to 'MAXFINE' in sysprefs.sql, which will cause breakage for new installations.  The usage of 900 as an 'arbitrary value' for determining whether or not to port over the values from sysprefs to circ matrix is... unclear.  Marking Failed QA.

I'm concerned about this patch changing too much out from under the library.  The meaning of MaxFine will change, and while this will port over *most* values of MaxFine into the circ matrix, this may still cause some confusion for folks who always thought that MaxFine was the total maximum for fines.  Don't get me wrong, I'm behind separating this logic so folks can have both an item-specific max and a global max, but we need to make it VERY CLEAR on upgrade that this has changed.  Perhaps that's more the job of the documenters and trainers than the coders, but I wanted to raise the point.

I'm in favour making updatedatabase patches easier to merge, but I think the XXX markers are not the right way to go about it.  What about just adding more whitespace between DB update lines?  If that distance is greater than 3, Git won't grumble about a conflict, and so long as the spacing is less than 3 between the last DB update and the DropAllForeignKeys call, then the new update will be properly placed even if other updates have slipped in after it.

An aside:  this patch touches two very shaky areas of the codebase, Accounts and Circ Rules.  These areas are already very prone to bugs, and need to be re-examined and probably rewritten.  However, I don't think it's practical for me hold this up waiting on these large fixes.  Get the MAXFINE issue solved, and make some attempt at forcing users to notice the change in behaviour, and we'll get a Passed QA.
Comment 17 Srdjan Jankovic 2012-03-20 06:44:18 UTC Comment hidden (obsolete)
Comment 18 Nicole C. Engard 2012-03-22 10:59:42 UTC
Once the patch is pushed I can/will update the manual if that's what we mean by documentation. If you mean that the update note in updatedatabase.pl should be very very clear then that's up to Srdjan :)

Nicole
Comment 19 Nicole C. Engard 2012-03-30 17:41:57 UTC
patch
Apply? [yn] y

Applying: bug_7420: Added overduefinescap to issuingrules
error: patch failed: C4/Overdues.pm:298
error: C4/Overdues.pm: patch does not apply
error: patch failed: admin/smart-rules.pl:101
error: admin/smart-rules.pl: patch does not apply
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt:77
error: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt: patch does not apply
Patch failed at 0001 bug_7420: Added overduefinescap to issuingrules
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/patch-B525Sz.patch
nengard@kohavm:~/kohaclone$
Comment 20 Srdjan Jankovic 2012-04-02 01:01:29 UTC Comment hidden (obsolete)
Comment 21 Srdjan Jankovic 2012-04-02 01:32:37 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2012-05-07 16:27:02 UTC
12:26 ~/kohaclone (bug7420-qa $% u=)$ git am -3iu /tmp/patch-Gs1Vwl.patch
Commit Body is:
--------------------------
bug_7420: Added overduefinescap to issuingrules

Replaced existing MaxFine syspref logic with overduefinescap.
Repurposed MaxFine to be the overall overdue limit for all items
overdue. Implemented new MaxFine logic in UpdateFine().
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: bug_7420: Added overduefinescap to issuingrules
/home/koha/kohaclone/.git/rebase-apply/patch:21: trailing whitespace.
C<$unitcount> is the number of chargeable units (days between start and end dates, Calendar adjusted where needed, 
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
<stdin>:21: trailing whitespace.
C<$unitcount> is the number of chargeable units (days between start and end dates, Calendar adjusted where needed, 
warning: 1 line applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging C4/Overdues.pm
CONFLICT (content): Merge conflict in C4/Overdues.pm
Auto-merging admin/smart-rules.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Failed to merge in the changes.
Patch failed at 0001 bug_7420: Added overduefinescap to issuingrule
Comment 23 Srdjan Jankovic 2012-05-09 03:23:09 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2012-05-30 14:00:39 UTC
Global symbol "$days_minus_grace" requires explicit package name at /home/koha/kohaclone/C4/Overdues.pm line 272.
Global symbol "$days_minus_grace" requires explicit package name at /home/koha/kohaclone/C4/Overdues.pm line 273.
Comment 25 Srdjan Jankovic 2012-05-31 00:11:45 UTC Comment hidden (obsolete)
Comment 26 Elliott Davis 2012-07-10 21:41:33 UTC
Tested according to Srdjan's test plan and everything worked like he said it would.  I set fined equal to $2 and max fine equal to $1.  When I ran the fines script for overdue items fines assessed were only $1.

Marking as signed off.
Comment 27 Elliott Davis 2012-07-11 15:14:08 UTC
Created attachment 10772 [details] [review]
Updated update database file to resolve conflict

Tested according to Srdjan's test plan and everything worked like he said it would.  I set fined equal to $2 and max fine equal to $1.  When I ran the fines script for overdue items fines assessed were only $1.

Signed-off-by: Elliott Davis <elliott@bywatersolutions.com>
Comment 28 Mason James 2012-07-16 23:37:12 UTC
(In reply to comment #27)
> Created attachment 10772 [details] [review]
> Updated update database file to resolve conflict
> 
> Tested according to Srdjan's test plan and everything worked like he said it
> would.  I set fined equal to $2 and max fine equal to $1.  When I ran the
> fines script for overdue items fines assessed were only $1.
> 
> Signed-off-by: Elliott Davis <elliott@bywatersolutions.com>


passing QA on this one...

all tests pass, and patch applies
Comment 29 Paul Poulain 2012-07-20 16:08:15 UTC
QA comment
I tried to test this bug, with syspref  finesCalendar set "Use Calendar to skip all days the library is closed", and for a late document I get a negative number for days_between, resulting in no fine being  set.

I think we should give more details on how days_between works. What we should say is:
* if the 1st date is BEFORE the 2nd date, return a negative number
* if the 1nd date is AFTER the 1st date, return a positive number
(to be // with ->compare that says:
Compare two DateTime objects. The semantics are compatible with Perl's "sort()" function; it returns -1 if $dt1 < $dt2, 0 if $dt1 == $dt2, 1 if $dt1 > $dt2.

ATM, in fines calculation, days_between is called with $datedue, $today. if $date_due < $today, it returns a negative number, which is wrong.
The CalcFine sub should be called with $today,$datedue

Could you please check I'm right. For now, for me, I can't test this patch properly. Note that the problem is not related to this patch

This is probably related to bug 8521, I tested with and without the patch provided by Koha Team Lyon 3, same result.
I tried with $today,$duedate, it seems to have fixed the problem (with Koha Team Lyon 3 patch applied)

I won't push this patch before having feedback
Comment 30 Paul Poulain 2012-07-23 16:26:30 UTC Comment hidden (obsolete)
Comment 31 Paul Poulain 2012-07-24 16:09:52 UTC
Comment on attachment 11083 [details] [review]
Bug 7420 tiny follow-up, mysql want a space after the --

Yikes, I made a mistake when attaching another patch
Comment 32 Paul Poulain 2012-07-25 16:37:28 UTC
Now that patch on bug 8486 fixes the date calculations problem, I've been able to test properly, confirm everything is OK, could not find any problem (outside from accountlines management being crazy & to rewrite, but that was already the case before this patch)

Patch pushed !
(will announce the feature in my monthly RM newsletter)