Bug 9129

Summary: Add the ability to set the maximum fine for an item to its replacement price
Product: Koha Reporter: Kyle M Hall <kyle>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: new feature    
Priority: P5 - low CC: abl, cmurdock, gitbot, gmcharlt, h.meissner.82, katrin.fischer, kyle.m.hall, m.de.rooy, nengard
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13590    
Bug Blocks:    
Attachments: Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price - QA Followup
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price - QA Followup
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price - QA Followup
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price - QA Followup
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price - QA Followup
Bug 9129: Followup for moving two maxfine prefs to Circulation
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
[SIGNED OFF] [patch] Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 [QA Followup] - Add unit tests
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 [QA Followup] - Add unit tests
Bug 9129 - Update database
Bug 9129 - Update schema
Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
[PASSED QA] Bug 9129 - Update database
[PASSED QA] Bug 9129 - Update database
[PASSED QA] Bug 9129 - Update schema
[PASSED QA] Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Bug 9129 [QA Followup] - Make unit test file conform to PBP

Description Kyle M Hall 2012-11-21 19:53:03 UTC

    
Comment 1 Kyle M Hall 2012-11-21 20:10:33 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-11-21 20:13:13 UTC
Test plan:
1) Apply patch
2) Run updatedatabase.pl
3) Enable MaxFineIsReplacementPrice
4) Pick an item, set it's replacementprice to some amount ( e.g. $6.50 )
5) Check it out to a patron, back date the due date by an amount such that the fine should exceed the replacement price ( a year ago should be good ).
6) Run fines.pl
7) Verify the fine for the item is not exceeded the replacement price.
Comment 3 Kyle M Hall 2012-11-21 20:16:59 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-11-21 20:18:40 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-11-21 20:19:31 UTC Comment hidden (obsolete)
Comment 6 Nicole C. Engard 2012-11-26 13:58:17 UTC
I wanted to be a on the CC list for this bug, but had to set a patch complexity to do so - update if I was wrong in my assumptions.

Nicole
Comment 7 Chris Cormack 2013-01-14 06:46:32 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2013-03-17 14:45:13 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2013-03-17 14:52:30 UTC
QA Comment:
Code looks fine to me.

One remark. IMO this pref and MaxFine should really be at the Circulation tab under Fines policy. All other Patron prefs have no direct connection to Circulation as well as Fines.

If you agree, please send a followup. 

Passed QA
Comment 10 Jared Camins-Esakov 2013-03-22 00:14:51 UTC
The logic on this syspref is backwards. When set to "Don't allow," the fine assessed will be equal to the maximum fine allowed by the circ rules, and when set to "Allow" the fine assessed will be equal to the replacement price of the item.
Comment 11 Kyle M Hall 2013-05-07 16:08:52 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2013-05-07 16:09:22 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-05-07 16:09:38 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-05-07 16:09:44 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-05-07 16:10:48 UTC
(In reply to comment #10)
> The logic on this syspref is backwards. When set to "Don't allow," the fine
> assessed will be equal to the maximum fine allowed by the circ rules, and
> when set to "Allow" the fine assessed will be equal to the replacement price
> of the item.

I've attached a trivial followup to fix this issue. I've set "Passed QA" under the assumption that this is the only issue you found. If I am mistaken, please reset the status.
Comment 16 Galen Charlton 2013-05-21 19:26:38 UTC
(In reply to comment #9)
> One remark. IMO this pref and MaxFine should really be at the Circulation
> tab under Fines policy. All other Patron prefs have no direct connection to
> Circulation as well as Fines.

To agree with and intensify what Marcel said ... IMO this *really* needs to be an attribute of issuingrules, not a system preference.

Different libraries sharing a Koha database might have different preferences on setting the max fine, and I can imagine cases where a library might want this to vary by item type.
Comment 17 Katrin Fischer 2013-05-22 06:13:55 UTC
Hm, I think Marcel meant the the Patrons tab in the system preference editor is not the right place, but that it should be in Circulation. 

With 3.12 we have MaxFine in the circulation matrix - not sure how the best way would be to indicate that it should be using the replacement price there. It's an input field, so maybe a checkbox that implies 'replacement price' and deactivates the maxfine input field?
Comment 18 Marcel de Rooy 2013-05-22 12:33:26 UTC
(In reply to comment #17)
> Hm, I think Marcel meant the the Patrons tab in the system preference editor
> is not the right place, but that it should be in Circulation. 

That is actually what I meant to write, taking one step at a time. But Galen is two steps further and I agree with him too :) It is confusing to configure circulation on two platforms: prefs and rules, not even talking about making it itemtype dependent.
Comment 19 Galen Charlton 2013-05-22 14:35:38 UTC
(In reply to comment #17)
> Hm, I think Marcel meant the the Patrons tab in the system preference editor
> is not the right place, but that it should be in Circulation. 

Indeed, I had in fact misread what Marcel meant, although I see that he also agrees with where I ended up after my misreading. :)

> With 3.12 we have MaxFine in the circulation matrix - not sure how the best
> way would be to indicate that it should be using the replacement price
> there. It's an input field, so maybe a checkbox that implies 'replacement
> price' and deactivates the maxfine input field?

That makes sense to me -- a checkbox for "cap overdue fine at replacement price" that then disables the maxfine input field.
Comment 20 Kyle M Hall 2013-05-23 18:43:32 UTC
One nice feature of this patch as it stands is that you can specify both MaxFine and MaxFineIsReplacementPrice at once. If the replacement price is over the MaxFine, it will be reduced.

To move it to the circulation matrix would require a full re-write. The overduefinescap functions within CalcFine, rather than UpdateFine.

Perhaps we have fines logic in too many places. Maybe MaxFines logic needs to be moved to CalcFine as well.

(In reply to comment #19)
> (In reply to comment #17)
> > Hm, I think Marcel meant the the Patrons tab in the system preference editor
> > is not the right place, but that it should be in Circulation. 
> 
> Indeed, I had in fact misread what Marcel meant, although I see that he also
> agrees with where I ended up after my misreading. :)
> 
> > With 3.12 we have MaxFine in the circulation matrix - not sure how the best
> > way would be to indicate that it should be using the replacement price
> > there. It's an input field, so maybe a checkbox that implies 'replacement
> > price' and deactivates the maxfine input field?
> 
> That makes sense to me -- a checkbox for "cap overdue fine at replacement
> price" that then disables the maxfine input field.
Comment 21 Marcel de Rooy 2013-05-24 08:49:22 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2013-05-24 08:50:05 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2013-05-24 08:50:11 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2013-05-24 08:50:55 UTC
Resolved small merge conflicts.
Comment 25 Marcel de Rooy 2013-05-24 09:37:06 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2013-05-24 09:37:12 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2013-05-24 09:39:07 UTC
Had some problems understanding No=Allow to exceed ...
I think that No=Do not use replacement price as max is easier?
Attached a QA followup that also moves the two prefs to Circulation tab, as noted before.
Comment 28 Marcel de Rooy 2013-05-24 09:50:07 UTC
(In reply to comment #20)
> One nice feature of this patch as it stands is that you can specify both
> MaxFine and MaxFineIsReplacementPrice at once. If the replacement price is
> over the MaxFine, it will be reduced.
> 
> To move it to the circulation matrix would require a full re-write. The
> overduefinescap functions within CalcFine, rather than UpdateFine.
> 
> Perhaps we have fines logic in too many places. Maybe MaxFines logic needs
> to be moved to CalcFine as well.

I agree with Galen that moving them both out of the prefs would be great. But adjusting a lot of troublesome circulation code would be great too.
All together, I do no think that we should block this patch for that reason.
BTW Kyle should be the ideal candidate to get the code in CalcFine and UpdateFine on a higher level as followup :)

Moving the status back to Passed QA for final RM comment.
Comment 29 Galen Charlton 2013-05-24 14:33:26 UTC
(In reply to comment #28)
> > Perhaps we have fines logic in too many places. Maybe MaxFines logic needs
> > to be moved to CalcFine as well.
> 
> I agree with Galen that moving them both out of the prefs would be great.
> But adjusting a lot of troublesome circulation code would be great too.

The thing is, not taking the time to set a new circ policy switch at the right level of specificity just adds to the refactoring work that has to be done later.

Setting back to in discussion to ask the question of Kyle directly: are you willing to prepare a version of this patch that adds this limit as a issuingrules policy, not a system preference?
Comment 30 Kyle M Hall 2013-05-28 18:14:43 UTC
Galen, if you feel strongly this is the way to go, I will do it that way. I'm fully invested in making Koha the best ILS it can be!

Also, as far as the fines logic goes, for this I will do whatever is necessary to make it function, but I'm still preparing a full-rewrite of the accounts system, so I'll probably just shore this up with the minimum amount of code, as the rewrite will replace it eventually.

It really does seem more logical to have CalcFine deal with all fine limiting activities, rather than UpdateFine. However, there may be consequences I'm not seeing.

Kyle

(In reply to comment #29)
> (In reply to comment #28)
> > > Perhaps we have fines logic in too many places. Maybe MaxFines logic needs
> > > to be moved to CalcFine as well.
> > 
> > I agree with Galen that moving them both out of the prefs would be great.
> > But adjusting a lot of troublesome circulation code would be great too.
> 
> The thing is, not taking the time to set a new circ policy switch at the
> right level of specificity just adds to the refactoring work that has to be
> done later.
> 
> Setting back to in discussion to ask the question of Kyle directly: are you
> willing to prepare a version of this patch that adds this limit as a
> issuingrules policy, not a system preference?
Comment 31 Galen Charlton 2013-05-29 16:27:32 UTC
(In reply to comment #30)
> Galen, if you feel strongly this is the way to go, I will do it that way.
> I'm fully invested in making Koha the best ILS it can be!

Thanks, I will await your revised patch with pleasure.

> Also, as far as the fines logic goes, for this I will do whatever is
> necessary to make it function, but I'm still preparing a full-rewrite of the
> accounts system, so I'll probably just shore this up with the minimum amount
> of code, as the rewrite will replace it eventually.

That's OK.  Speaking of the fines write, have you put together a plan for it?
Comment 32 Kyle M Hall 2013-05-30 12:12:32 UTC
> That's OK.  Speaking of the fines write, have you put together a plan for it?

Yes, I submitted an RFC, and have already started development. The new database schema has deviated a bit from the RFC for technical reasons ( I had thought to split fees accruing and fees accrued into two separate tables, but it became unworkable in reality ). The biggest change will be the separation of fees and payments into two tables. Each 'payment' will have subpayments for each fee paid. So, if a patron pays $10 in fines, we'll know that person paid $10 total, $3.00 on Fine A, $5.00 on Fine B, and $2.00 on Fine C!

In addition, the biggest under the hood change will be to unify all payment subroutines into a single sub, so we don't have behavioral issues that crop up currently.

Also, all tables will have primary keys, so we won't have to update fees based on the description anymore ( which has lead to bugs in the past ).

In addition, the account offsets table will function again.
Comment 33 Katrin Fischer 2013-06-03 10:13:19 UTC
Hi Kyle,

I wasn't aware of your RFC until reading about it here on the bug - maybe you could update the wiki page and send a mail to the mailing list about it? People might have good suggestions/comments.
Comment 34 Kyle M Hall 2013-06-04 15:43:51 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2013-06-04 15:45:48 UTC Comment hidden (obsolete)
Comment 36 I'm just a bot 2013-09-18 11:14:23 UTC
Applying: Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Using index info to reconstruct a base tree...
M	C4/Overdues.pm
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging C4/Overdues.pm
Patch failed at 0001 Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 37 Kyle M Hall 2013-12-04 17:28:58 UTC Comment hidden (obsolete)
Comment 38 Holger Meißner 2014-02-06 13:04:13 UTC
Just tested this with an item's replacement price set to 7,95 and overdue fines cap set to 1000.

Directly after the checkout (backdated a few years) the fine was displayed to be 0. I guess that's ok, since you usually don't backdate except for testing.

Then I ran misc/cronjobs/fines.pl and the fine was updated to 1000.
Finally I returned the item and the fine was set to 7,95.

So apparently and confusingly fines are calculated in different places.
Comment 39 Katrin Fischer 2014-02-06 20:37:35 UTC
Hi Holger, thx for testing! Reading your comment I think this is a failed QA.
Comment 40 Kyle M Hall 2015-10-08 11:15:44 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2015-10-08 11:17:18 UTC Comment hidden (obsolete)
Comment 42 Cindy Murdock Ames 2015-10-09 13:21:07 UTC Comment hidden (obsolete)
Comment 43 Katrin Fischer 2015-10-18 22:07:15 UTC
There are some conflicts, due to recent changes in master (I suspect for the on-site quotas), please fix:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
Using index info to reconstruct a base tree...
M	Koha/Schema/Result/Issuingrule.pm
M	admin/smart-rules.pl
M	installer/data/mysql/kohastructure.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging admin/smart-rules.pl
CONFLICT (content): Merge conflict in admin/smart-rules.pl
Auto-merging Koha/Schema/Result/Issuingrule.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Issuingrule.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/SIGNED-OFF-patch-Bug-9129---Add-the-ability-to-set-G3gYvN.patch
Comment 44 Kyle M Hall 2015-10-19 13:23:55 UTC Comment hidden (obsolete)
Comment 45 Katrin Fischer 2015-10-20 22:10:20 UTC
Kyle, I have tested this and am almost ready to pass QA on this, but please add some tests highlighting the change in CalcFines.
Comment 46 Kyle M Hall 2015-10-21 11:18:51 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2015-10-21 11:19:03 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2015-10-21 11:39:40 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2015-10-21 11:39:47 UTC Comment hidden (obsolete)
Comment 50 Katrin Fischer 2015-10-21 21:47:18 UTC
Is this something to worry about?

$ perl t/db_dependent/Circulation/CalcFine.t
1..2
DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1489.
    # Subtest: Test basic functionality
    1..1
    ok 1 - Amount is calculated correctly
ok 1 - Test basic functionality
    # Subtest: Test cap_fine_to_replacement_price
    1..1
    ok 1 - Amount is calculated correctly
ok 2 - Test cap_fine_to_replacement_price
DBIx::Class::Storage::DBI::txn_rollback(): Storage transaction_depth 0 does not match false AutoCommit of DBI::db=HASH(0x432c8c0), attempting ROLLBACK anyway at /home/katrin/kohaclone/t/lib/TestBuilder.pm line 363
Comment 51 Katrin Fischer 2015-10-21 21:55:32 UTC
I thought it was problems with the Schema first and tried to regenerate, but this shows a problem:

Dumping manual schema for Koha::Schema to directory ./ ...
DBIx::Class::Schema::Loader::make_schema_at(): Checksum mismatch in './/Koha/Schema/Result/Issuingrule.pm', the auto-generated part of the file has been modified outside of this loader.  Aborting.
If you want to overwrite these modifications, set the 'overwrite_modifications' loader option.

Can you please put the Schema changes into a separate patch? I think it didn't like the merging when fixing the conflicts with the other bug.
Comment 52 Kyle M Hall 2015-10-22 10:40:44 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2015-10-22 10:41:00 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2015-10-22 10:41:03 UTC Comment hidden (obsolete)
Comment 55 Katrin Fischer 2015-10-22 20:16:53 UTC Comment hidden (obsolete)
Comment 56 Katrin Fischer 2015-10-22 20:17:17 UTC
Created attachment 43782 [details] [review]
[PASSED QA] Bug 9129 - Update database

Signed-off-by: Cindy Murdock Ames <cmurdock@ccfls.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 57 Katrin Fischer 2015-10-22 20:17:21 UTC
Created attachment 43783 [details] [review]
[PASSED QA] Bug 9129 - Update schema

Signed-off-by: Cindy Murdock Ames <cmurdock@ccfls.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 58 Katrin Fischer 2015-10-22 20:17:25 UTC
Created attachment 43784 [details] [review]
[PASSED QA] Bug 9129 - Add the ability to set the maximum fine for an item to its replacement price

This patch adds the ability to set the maximum fine for a given item to
its replacement price ( assuming the replacement price is set ). If
overduefinescap is also set, the fine will be the lesser of the two, if
both apply to the given overdue checkout.

To enable this new limit, create or edit your circulation rules and
check the checkbox for "Cap fines at replacement price"

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Pick an item, and set it's replacement price to 3.99
4) Edit the circulation rule that would apply to this item
   and the patron you will check it out to.
5) Check out the item to the patron, and backdate the due date
   such that the fine generated would be more than 3.99
6) Enable CalculateFinesOnReturn
7) Return the item, and view the fine generated, it should be 3.99

Signed-off-by: Cindy Murdock Ames <cmurdock@ccfls.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 59 Kyle M Hall 2015-12-31 15:42:06 UTC
Created attachment 46157 [details] [review]
Bug 9129 [QA Followup] - Make unit test file conform to PBP

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 60 Kyle M Hall 2015-12-31 15:42:42 UTC
Pushed to master!