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
Created attachment 7106 [details] [review] patch Repurposed MaxFine syspref. This means that new translations are in order. MaxFine is now a global maximum for all overdue charges for a patron combined.
It actually made no sense to place "Overdue Fines Cap" on its own, so it is in the main table
Created attachment 7287 [details] [review] patch
Created attachment 7487 [details] [review] 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(). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Patch didn't apply cleanly to updatadatabase, simple fix. http://bugs.koha-community.org/show_bug.cgi?id=7287
Tested with multiple category/item type combinations, and with global maxfine set. All appear to be working.
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 +
(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.
Created attachment 7489 [details] [review] 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(). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Patch didn't apply cleanly to updatadatabase, simple fix. Fixed improper addition of currency sign ($) in template. Fix warns without $debug. http://bugs.koha-community.org/show_bug.cgi?id=7287
Created attachment 7491 [details] [review] bug_7420: Added overduefinescap to issuingrules
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.
(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.
I can separate it, however it has been signed off. What do you want me to do?
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?
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
Created attachment 7782 [details] [review] follow-up patch
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.
Created attachment 8327 [details] [review] patch I do apologise, it was probably my attempt at "making it more obvious" by renaming the syspref, but then I realised that it would make more problems than it would solve, so I backed up but not fully. Sorry. On a related note, I have changed the description, and I have no objections changing existing documentation, but I need some guidance, I have never done that. Apart from that, I'm not sure how to make it more obvious. I have also removed the "Insert here" comments from updatedatabase.pl. I actually have an even better idea, so can you please tell me who is the king of release builds, I'd like to discuss it.
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
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$
Created attachment 8748 [details] [review] patch
Created attachment 8749 [details] [review] patch Sorry, previous patch included temp fix for master brokenness.
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
Created attachment 9469 [details] [review] patch I have no idea where the conflict came in, this patch is (almost) identical...
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.
Created attachment 9853 [details] [review] 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().
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.
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>
(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
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
Created attachment 11083 [details] [review] Bug 7420 tiny follow-up, mysql want a space after the --
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
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)