Bug 7639 - system preference to forgive fines on lost items
Summary: system preference to forgive fines on lost items
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-01 22:52 UTC by Melia Meggs
Modified: 2014-12-07 20:02 UTC (History)
10 users (show)

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


Attachments
Attaching Patch. (8.46 KB, patch)
2012-04-24 05:44 UTC, Meenakshi
Details | Diff | Splinter Review
Sending improved Patch. (9.76 KB, patch)
2012-04-24 07:11 UTC, Meenakshi
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items (9.84 KB, patch)
2012-05-07 16:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Sending Improved Patch. (8.29 KB, patch)
2012-07-06 05:52 UTC, Meenakshi
Details | Diff | Splinter Review
Patch (10.02 KB, patch)
2012-07-06 06:15 UTC, Meenakshi
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items (10.39 KB, patch)
2012-10-23 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - QA Followup (1012 bytes, patch)
2012-10-23 12:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items (10.33 KB, patch)
2013-01-30 18:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - QA Followup (977 bytes, patch)
2013-01-30 18:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items (9.96 KB, patch)
2013-02-06 12:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - QA Followup (977 bytes, patch)
2013-02-06 12:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 tiny QA follow-up (1.13 KB, patch)
2013-03-21 10:25 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items (10.03 KB, patch)
2013-03-21 11:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - QA Followup (1.49 KB, patch)
2013-03-21 11:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 tiny QA follow-up (1.14 KB, patch)
2013-03-21 11:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items (9.99 KB, patch)
2013-03-22 09:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - QA Followup (1.49 KB, patch)
2013-03-22 09:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 tiny QA follow-up (1.14 KB, patch)
2013-03-22 09:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items (9.96 KB, patch)
2013-06-13 12:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - QA Followup (1.49 KB, patch)
2013-06-13 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 tiny QA follow-up (1.14 KB, patch)
2013-06-13 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
[Signed off] Bug 7639 - system preference to forgive fines on lost items (10.02 KB, patch)
2013-06-13 21:36 UTC, Melia Meggs
Details | Diff | Splinter Review
[Signed off] Bug 7639 - system preference to forgive fines on lost items - QA Followup (1.55 KB, patch)
2013-06-13 21:36 UTC, Melia Meggs
Details | Diff | Splinter Review
[Signed off] Bug 7639 tiny QA follow-up (1.20 KB, patch)
2013-06-13 21:36 UTC, Melia Meggs
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items (10.03 KB, patch)
2013-07-15 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - QA Followup (1.53 KB, patch)
2013-07-15 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 tiny QA follow-up (1.18 KB, patch)
2013-07-15 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - QA Followup - Fix update description (1.35 KB, patch)
2013-07-15 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 7639 - system preference to forgive fines on lost items (9.69 KB, patch)
2013-08-18 15:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7639 - system preference to forgive fines on lost items - QA Followup (1.60 KB, patch)
2013-08-18 15:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7639 tiny QA follow-up (1.25 KB, patch)
2013-08-18 15:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7639 - system preference to forgive fines on lost items - QA Followup - Fix update description (1.99 KB, patch)
2013-08-18 15:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7639 - system preference to forgive fines on lost items - Add Unit Tests (4.72 KB, patch)
2013-08-19 12:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 7639 - system preference to forgive fines on lost items - Add Unit Tests (4.87 KB, patch)
2013-08-19 20:23 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Melia Meggs 2012-03-01 22:52:10 UTC
Create a new system preference that would allow a librarian to decide to forgive/not forgive all fines when an item goes to lost status.
Comment 1 Savitra Sirohi 2012-03-12 14:11:31 UTC
Draft Design

syspref: forgivefinewhenlost
Syspref description: Forgive/Don't forgive fines when item status is set to lost

Item status can be set to "lost" from:
catalogue/moredetail.pl
cataloguing/additem.pl
tools/batchMod.pl

Design:

Change sub C4::Circulation::LostItem to forgive fines on the item depending on the value of syspref forgivefinewhenlost. This sub is currently used to return an item and charge a replacement cost. 


Currently this sub is not called in additem.pl and therefore an item is not returned/charged when item status is set to "lost" from here. As part of this change, we will make a call to the LostItem sub from this script as well.
Comment 2 Nicole C. Engard 2012-03-13 12:47:20 UTC
I'd add to this "Item status can be set to "lost" from:" that it can be marked lost from the longoverdue cron as well (not sure of the file).

Before changing additem.pl to charge for lost items, we might need to add another system preference that asks if marking lost charges patrons the replacement fee .. anyone else have any ideas on this? The last time changes were made to how lost is assigned there were some issues.

Nicole
Comment 3 Kyle M Hall 2012-03-13 13:35:22 UTC
I agree with Nicole's comment, and want to add a very minor detail. I would suggest instead of forgivefinewhenlost, use ForgiveFineWhenLost. It seems to be the custom now to capitalize each word in a system preference, it certainly makes them easier to read.
Comment 4 Savitra Sirohi 2012-03-21 15:55:00 UTC
Nicole, Kyle, thanks for the feedback.

On second sys pref, agreed, it is a different topic, but we might as well add that now, when we are changing the LostItem sub.

Yes, ForgiveFineWhenLost is more readable.

And we will check on the longoverdue cronjob as well to see if there is anything there that sets items to lost status.
Comment 5 Meenakshi 2012-04-24 05:44:56 UTC Comment hidden (obsolete)
Comment 6 Meenakshi 2012-04-24 07:11:12 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2012-04-24 12:48:29 UTC
*** Bug 7070 has been marked as a duplicate of this bug. ***
Comment 8 Kyle M Hall 2012-04-24 12:48:52 UTC
*** Bug 7189 has been marked as a duplicate of this bug. ***
Comment 9 Kyle M Hall 2012-05-07 16:13:50 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2012-05-16 15:04:11 UTC
Question on defaults of the new prefs: By default, you do not forgive fines but also you do not charge replacement fee. Is that correct? In current code the borrower gets charged for replacement.
Additionally: Could or should the value of $mark_returned make a difference in that case?

I feel that this patch needs some more community consensus. Could author or signer trigger that by mailing the list?
Comment 11 Marcel de Rooy 2012-05-24 12:08:32 UTC
Changing status to reflect need for clarification (see comment10).
Comment 12 Meenakshi 2012-07-06 05:52:58 UTC Comment hidden (obsolete)
Comment 13 Meenakshi 2012-07-06 06:15:49 UTC Comment hidden (obsolete)
Comment 14 Paul Poulain 2012-07-13 14:19:55 UTC
QA comments:

* hardcoded MARC21 detected !!!! = +    my $newitemlost = $itemtosave->subfield('952','1'); => this does not work at all for UNIMARC and will break UNIMARC setups !!! To find the proper MARC field& subfield use my ($item_tag,$item_subfield) = &GetMarcFromKohaField("items.itemlost",'');

* your patch adds tabs instead of 4 spaces for indentation, plus I think it has a wrong indentation
+	if ( my $borrowernumber = $issues->{borrowernumber} ){
should be at the same column as # if a borrower lost the item, add a replacement cost to the their record, it's not the case when I apply the patch

* The patch does not apply cleanly anymore, there are some conflicts on:
CONFLICT (content): Merge conflict in C4/Circulation.pm
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl

Please fix #1 and #2, rebase for #3 and resubmit, thanks
Comment 15 Kyle M Hall 2012-10-23 12:02:38 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2012-10-23 12:02:54 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2013-01-07 15:55:14 UTC
I think this just needs to be QA'ed again, as my followup is a qa followup.
Comment 18 Kyle M Hall 2013-01-30 18:35:07 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-01-30 18:35:24 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-02-06 12:38:12 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-02-06 12:38:29 UTC Comment hidden (obsolete)
Comment 22 Paul Poulain 2013-03-21 10:25:45 UTC Comment hidden (obsolete)
Comment 23 Paul Poulain 2013-03-21 10:29:08 UTC
QA comment:
 * passes koha-qa.pl
 * tiny QA comment, I made a tiny follow-up
 * lack a test plan

Please provide test plan and switch back to signed off.
Comment 24 Kyle M Hall 2013-03-21 11:31:38 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2013-03-21 11:31:46 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2013-03-21 11:31:54 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2013-03-22 09:27:42 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2013-03-22 09:27:54 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-03-22 09:28:02 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2013-04-12 05:36:05 UTC
Hm, reading through the bug this morning, the functionality sounds a bit similar to RefundLostItemFeeOnReturn (except for the part about the replacement price). Kyle, could you check the new pref in master and that ForgiveFineWhenLost is still needed?
Comment 31 Melia Meggs 2013-04-12 16:30:32 UTC
I think they are two different things - one is concerned with overdue fines only, and the other is concerned with paying/refunding the replacement price.

ForgiveFineWhenLost - an item goes overdue and starts to accrue fines.  Eventually, it goes to lost status but has maybe accrued $300 in overdue fines already.  Now that the item is lost, the library wants to charge the replacement price for the lost item but is not concerned about collecting the $300 in overdue fines.  So this sys pref lets you forgive all of the overdue fines when the item goes to lost.

RefundLostItemFeeOnReturn - an item goes to lost, and the replacement price is charged.  But then later the patron finds the lost item and returns it.  Right now, I think Koha refunds the replacement price to the patron.  Some libraries aren't allowed to refund money that patrons have already paid them, so this sys pref makes it so that you can choose whether the replacement price is refunded to the patron or not.  ('He found the book, so he is not required to pay the replacement price any more and gets a refund' versus 'He kept the book for so long that we thought it was lost, so now he must pay regardless').

At least I think that's the idea...  Kyle, please correct me if I got any of that is wrong!
Comment 32 Kyle M Hall 2013-06-04 18:18:49 UTC
Melia is correct, the each enable different functionality.

(In reply to comment #31)
> I think they are two different things - one is concerned with overdue fines
> only, and the other is concerned with paying/refunding the replacement price.
> 
> ForgiveFineWhenLost - an item goes overdue and starts to accrue fines. 
> Eventually, it goes to lost status but has maybe accrued $300 in overdue
> fines already.  Now that the item is lost, the library wants to charge the
> replacement price for the lost item but is not concerned about collecting
> the $300 in overdue fines.  So this sys pref lets you forgive all of the
> overdue fines when the item goes to lost.
> 
> RefundLostItemFeeOnReturn - an item goes to lost, and the replacement price
> is charged.  But then later the patron finds the lost item and returns it. 
> Right now, I think Koha refunds the replacement price to the patron.  Some
> libraries aren't allowed to refund money that patrons have already paid
> them, so this sys pref makes it so that you can choose whether the
> replacement price is refunded to the patron or not.  ('He found the book, so
> he is not required to pay the replacement price any more and gets a refund'
> versus 'He kept the book for so long that we thought it was lost, so now he
> must pay regardless').
> 
> At least I think that's the idea...  Kyle, please correct me if I got any of
> that is wrong!
Comment 33 Kyle M Hall 2013-06-13 12:44:55 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2013-06-13 12:45:12 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2013-06-13 12:45:40 UTC Comment hidden (obsolete)
Comment 36 alexh 2013-06-13 15:47:26 UTC
I have tested this and it works as planned. I would like to sign this patch off in my name.
Comment 37 Melia Meggs 2013-06-13 21:36:23 UTC Comment hidden (obsolete)
Comment 38 Melia Meggs 2013-06-13 21:36:38 UTC Comment hidden (obsolete)
Comment 39 Melia Meggs 2013-06-13 21:36:49 UTC Comment hidden (obsolete)
Comment 40 Melia Meggs 2013-06-13 21:37:26 UTC
Adding Git signoff on Alex's behalf.

Thanks,

Melia
Comment 41 Katrin Fischer 2013-07-13 07:39:16 UTC
Comment on attachment 18964 [details] [review]
[Signed off] Bug 7639 - system preference to forgive fines on lost items

Review of attachment 18964 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +3281,3 @@
>  
>  
>  sub LostItem{

LostItem seems a bit undertested. Could you throw in some regression tests?

::: cataloguing/additem.pl
@@ +607,5 @@
>          $itemnumber="";
>      }
> +  my $item = GetItem( $itemnumber );
> +    my $olditemlost =  $item->{'itemlost'};
> +    my $newitemlost = $itemtosave->subfield('952','1');

(Major) This won't work for UNIMARC, please check which field is linked to items.lost instead.

::: installer/data/mysql/updatedatabase.pl
@@ +6993,5 @@
> +$DBversion = "3.13.00.XXX";
> +if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
> +    $dbh->do("INSERT INTO systempreferences (variable,value,options,explanation,type) VALUES('WhenLostForgiveFine','0',NULL,'If ON, Forgives the fines on an item when it is lost.','YesNo')");
> +    $dbh->do("INSERT INTO systempreferences (variable,value,options,explanation,type) VALUES('WhenLostChargeReplacementFee','1',NULL,'If ON, Charge the replacement price when a patron loses an item.','YesNo')");
> +    print "Upgrade to $DBversion done (Added sysprefs WhenLostForgiveFine and WhenLostChargeReplacementFee)\n";

(Trivial) - Please add the bug number here for reference.
Comment 42 Kyle M Hall 2013-07-15 12:42:33 UTC Comment hidden (obsolete)
Comment 43 Kyle M Hall 2013-07-15 12:42:49 UTC Comment hidden (obsolete)
Comment 44 Kyle M Hall 2013-07-15 12:43:17 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2013-07-15 12:43:39 UTC Comment hidden (obsolete)
Comment 46 Katrin Fischer 2013-08-18 15:44:40 UTC
Created attachment 20436 [details] [review]
[PASSED QA] Bug 7639 - system preference to forgive fines on lost items

Forgive fine when an item is set to lost status, fines are forgiven only when syspref ForgiveFineWhenLost is set to yes. Item can be set to lost status from:
    catalogue/moredetail.pl
    cataloguing/additem.pl
    tools/batchMod.pl
    misc/cronjobs/longoverdue.pl

    Changed sub C4::Circulation::LostItem to forgive fines on the item depending on the value of syspref ForgiveFineWhenLost. This sub is currently used to return an item and charge a replacement cost.

    Also added a new syspref in C4::Circulation::LostItem - ChargeReplFeeWhenLost. Replacement fee will now be charged only if this syspref is set to yes.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Alex Hatley <alexh@cctexas.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Testing notes on last patch in series.
Comment 47 Katrin Fischer 2013-08-18 15:44:55 UTC
Created attachment 20437 [details] [review]
[PASSED QA] Bug 7639 - system preference to forgive fines on lost items - QA Followup

Test Plan:
 1) Apply the patch
 2) Run updatedatabase.pl
 3) Set WhenLostForgiveFine to Forgive & WhenLostChargeReplacementFee to Charge
 4) Issue an item to a borrower, then set to lost from catalogue/moredetail.pl
 5) Observe the replacement fee has been charged
 6) Return the item
 7) Observe the charged fee has been forgiven
 8) Set WhenLostForgiveFine to Don't Forgive
 9) Repeat 4-6
10) Observe the charged fee has not been forgiven
11) Set WhenLostChargeReplacementFee to Don't Charge
12) Repeat 4
13) Observe that a fee was not charged

Signed-off-by: Alex Hatley <alexh@cctexas.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 48 Katrin Fischer 2013-08-18 15:45:06 UTC
Created attachment 20438 [details] [review]
[PASSED QA] Bug 7639 tiny QA follow-up

no need to create a variable just to call to send it as parameter the line after

Signed off by: Alex Hatley <alexh@cctexas.com>

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 49 Katrin Fischer 2013-08-18 15:45:15 UTC
Created attachment 20439 [details] [review]
[PASSED QA] Bug 7639 - system preference to forgive fines on lost items - QA Followup - Fix update description

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
With these patches we now have 3 preferences that deal with
fines/fees and lost items:

Replacement price
- On marking lost:  WhenLostChargeReplacementFee
- On return: RefundLostItemFeeOnReturn (preexisting)

Overdue fines
- On marking lost: WhenLostForgiveFine

System preferences work according to their description.

Further notes:
- Patches pass all tests and QA script.
- LostItem is changed and needs unit tests.
  All occurrences of LostItem have been changed.
- Database update is ok. I had to fix the sysprefs.sql
  file that has been recently rewritten and alphabetized.
Comment 50 Katrin Fischer 2013-08-18 15:47:24 UTC
I couldn't find any problems with these patches, but I would _really_ like to see some unit tests/regression tests for LostItem before setting this to 'Passed QA'.
Comment 51 Kyle M Hall 2013-08-19 12:01:51 UTC Comment hidden (obsolete)
Comment 52 Katrin Fischer 2013-08-19 20:23:57 UTC
Created attachment 20487 [details] [review]
[PASSED QA] Bug 7639 - system preference to forgive fines on lost items - Add Unit Tests

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests and QA script pass.
I fixed a redundant 'my' that the QA script pointed out.
Comment 53 Kyle M Hall 2013-08-21 13:10:58 UTC
(In reply to Katrin Fischer from comment #52)
> I fixed a redundant 'my' that the QA script pointed out.

Thanks Katrin!
Comment 54 Galen Charlton 2013-09-17 15:30:23 UTC
Pushed to master.  Thanks, Meenakshi!
Comment 55 Liz Rea 2014-02-10 02:31:52 UTC
Out of curiosity - is anyone seeing issues with charging from longoverdue with this patch?