|
Description
Melia Meggs
2012-03-01 22:52:10 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. 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 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. 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. Created attachment 9281 [details] [review] Attaching Patch. Attached a patch which can add sys prefs "ForgiveFineWhenLost" and "ChargeReplFeeWhenLost". Thank you, Created attachment 9282 [details] [review] Sending improved Patch. Sending improved Patch. *** Bug 7070 has been marked as a duplicate of this bug. *** *** Bug 7189 has been marked as a duplicate of this bug. *** Created attachment 9436 [details] [review] 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> 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? Changing status to reflect need for clarification (see comment10). Created attachment 10662 [details] [review] Sending Improved Patch. Created attachment 10663 [details] [review] Patch Set the default value as 1 for system preference 'ChargeReplFeeWhenLost'. 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
Created attachment 13006 [details] [review] 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> Created attachment 13007 [details] [review] Bug 7639 - system preference to forgive fines on lost items - QA Followup I think this just needs to be QA'ed again, as my followup is a qa followup. Created attachment 14961 [details] [review] 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> Created attachment 14962 [details] [review] Bug 7639 - system preference to forgive fines on lost items - QA Followup Created attachment 15088 [details] [review] 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> Created attachment 15089 [details] [review] Bug 7639 - system preference to forgive fines on lost items - QA Followup Created attachment 16593 [details] [review] Bug 7639 tiny QA follow-up no need to create a variable just to call to send it as parameter the line after 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. Created attachment 16607 [details] [review] 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> Created attachment 16608 [details] [review] 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 Created attachment 16609 [details] [review] Bug 7639 tiny QA follow-up no need to create a variable just to call to send it as parameter the line after Created attachment 16729 [details] [review] 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> Created attachment 16730 [details] [review] 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 Created attachment 16731 [details] [review] Bug 7639 tiny QA follow-up no need to create a variable just to call to send it as parameter the line after 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? 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!
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! Created attachment 18929 [details] [review] 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> Created attachment 18930 [details] [review] 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 Created attachment 18932 [details] [review] Bug 7639 tiny QA follow-up no need to create a variable just to call to send it as parameter the line after I have tested this and it works as planned. I would like to sign this patch off in my name. Created attachment 18964 [details] [review] [Signed off] 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> Created attachment 18965 [details] [review] [Signed off] 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> Created attachment 18966 [details] [review] [Signed off] 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> Adding Git signoff on Alex's behalf. Thanks, Melia 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. Created attachment 19654 [details] [review] 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> Created attachment 19655 [details] [review] 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> Created attachment 19656 [details] [review] 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> Created attachment 19657 [details] [review] Bug 7639 - system preference to forgive fines on lost items - QA Followup - Fix update description 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. 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> 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> 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. 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'. Created attachment 20456 [details] [review] Bug 7639 - system preference to forgive fines on lost items - Add Unit Tests 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. (In reply to Katrin Fischer from comment #52) > I fixed a redundant 'my' that the QA script pointed out. Thanks Katrin! Pushed to master. Thanks, Meenakshi! Out of curiosity - is anyone seeing issues with charging from longoverdue with this patch? |