The fee set in ExpireReservesMaxPickUpDelayCharge can only be charged by the automatic expiration cron script. Some libraries would prefer to manually cancel holds via the "Holds over" tab of the "Holds awaiting pickup" page but still be able to charge the cancellation fee.
*** Bug 14137 has been marked as a duplicate of this bug. ***
Created attachment 46558 [details] [review] Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab The fee set in ExpireReservesMaxPickUpDelayCharge can only be charged by the automatic expiration cron script. Some libraries would prefer to manually cancel holds via the "Holds over" tab of the "Holds awaiting pickup" page but still be able to charge the cancellation fee. Test Plan: 1) Apply this patch 2) Set ExpireReservesMaxPickUpDelayCharge to some number 3) Go to the Holds awaiting pickup page 4) Go to the Holds over tab 5) Note the new checkbox for charging the fee 6) Test canceling a single hold with the checkbox checked 7) Note a fee is charged 8) Test canceling a single hold with the checkbox unchecked 9) Note a fee is not charged 10) Test canceling all holds with the checkbox checked 11) Note the patrons are charged fees 12) Test canceling all holds without the checkbox checked 13) Note the patrons are not charged fees
There is one other exception: if you cancel the hold when returning it - this will also charge the fee.
The cancel buttons for individual items appear to be working correctly, but when I use the "cancel all" button a fine is not charged even when the checkbox is checked.
Created attachment 50325 [details] [review] Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab The fee set in ExpireReservesMaxPickUpDelayCharge can only be charged by the automatic expiration cron script. Some libraries would prefer to manually cancel holds via the "Holds over" tab of the "Holds awaiting pickup" page but still be able to charge the cancellation fee. Test Plan: 1) Apply this patch 2) Set ExpireReservesMaxPickUpDelayCharge to some number 3) Go to the Holds awaiting pickup page 4) Go to the Holds over tab 5) Note the new checkbox for charging the fee 6) Test canceling a single hold with the checkbox checked 7) Note a fee is charged 8) Test canceling a single hold with the checkbox unchecked 9) Note a fee is not charged 10) Test canceling all holds with the checkbox checked 11) Note the patrons are charged fees 12) Test canceling all holds without the checkbox checked 13) Note the patrons are not charged fees
(In reply to Owen Leonard from comment #4) > The cancel buttons for individual items appear to be working correctly, but > when I use the "cancel all" button a fine is not charged even when the > checkbox is checked. Fixed!
Created attachment 51742 [details] [review] [SIGNED-OFF] Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab The fee set in ExpireReservesMaxPickUpDelayCharge can only be charged by the automatic expiration cron script. Some libraries would prefer to manually cancel holds via the "Holds over" tab of the "Holds awaiting pickup" page but still be able to charge the cancellation fee. Test Plan: 1) Apply this patch 2) Set ExpireReservesMaxPickUpDelayCharge to some number 3) Go to the Holds awaiting pickup page 4) Go to the Holds over tab 5) Note the new checkbox for charging the fee 6) Test canceling a single hold with the checkbox checked 7) Note a fee is charged 8) Test canceling a single hold with the checkbox unchecked 9) Note a fee is not charged 10) Test canceling all holds with the checkbox checked 11) Note the patrons are charged fees 12) Test canceling all holds without the checkbox checked 13) Note the patrons are not charged fees Signed-off-by: Jason Burds <JBurds@dubuque.lib.ia.us>
Kyle, please provide tests. Your patch deletes 2 images, is it expected?
Created attachment 52906 [details] [review] Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab The fee set in ExpireReservesMaxPickUpDelayCharge can only be charged by the automatic expiration cron script. Some libraries would prefer to manually cancel holds via the "Holds over" tab of the "Holds awaiting pickup" page but still be able to charge the cancellation fee. Test Plan: 1) Apply this patch 2) Set ExpireReservesMaxPickUpDelayCharge to some number 3) Go to the Holds awaiting pickup page 4) Go to the Holds over tab 5) Note the new checkbox for charging the fee 6) Test canceling a single hold with the checkbox checked 7) Note a fee is charged 8) Test canceling a single hold with the checkbox unchecked 9) Note a fee is not charged 10) Test canceling all holds with the checkbox checked 11) Note the patrons are charged fees 12) Test canceling all holds without the checkbox checked 13) Note the patrons are not charged fees Signed-off-by: Jason Burds <JBurds@dubuque.lib.ia.us>
Created attachment 52907 [details] [review] Bug 15561 [QA Followup] - Add unit tests
Created attachment 52908 [details] [review] Bug 15561 [QA Followup] - Add unit tests
(In reply to Jonathan Druart from comment #8) > Kyle, please provide tests. Done! > Your patch deletes 2 images, is it expected? Nope! No idea how that happened! New patch fixes that!
Should not we adjust the description of ExpireReservesMaxPickUpDelayCharge? It says "ExpireReservesMaxPickUpDelay is enabled, and this field has a non-zero value, than a borrower whose waiting hold has expired will be charged this amount". But actually this pref is used even if ExpireReservesMaxPickUpDelay is off. Or maybe should we use rename one of these 2 prefs? It seems odd to use the same suffix but they are not so related.
(In reply to Jonathan Druart from comment #13) > Should not we adjust the description of ExpireReservesMaxPickUpDelayCharge? > It says "ExpireReservesMaxPickUpDelay is enabled, and this field has a > non-zero value, than a borrower whose waiting hold has expired will be > charged this amount". But actually this pref is used even if > ExpireReservesMaxPickUpDelay is off. > Or maybe should we use rename one of these 2 prefs? It seems odd to use the > same suffix but they are not so related. I propose we change the explantation for ExpireReservesMaxPickUpDelayCharge to: If using ExpireReservesMaxPickUpDelay, charge a borrower who allows his or her waiting hold to expire a fee of to: Charge an ammount of $amount if a waiting hold cancellation fee is to be charged. and the syspref name to be: WaitingHoldCancelationFee Does that sound good Jonathan?
Sounds good to me :) Katrin, do you agree?
Yes, I agree. Just having a go at the sys pref description too - pick what you like best :) Charge an ammount of $amount when a hold is cancelled, that has been waiting more days than defined by ReservesMaxPickUpDelay.
Putting into 'failed qa' just for the follow up for the description/syspref naming - please put back to 'signed off' once there is a follow-up.
Created attachment 53724 [details] [review] Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee
Hi Kyle, can you please add an atomicupdate for the syspref renaming? Also 2 more tiny thing: - correct alphabetic sorting in syspref.sql - Sypref description has "if a waiting hold cancellation fee is to be charged" instead of cancelation with one l (consistency! :) )
Created attachment 63358 [details] [review] Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab The fee set in ExpireReservesMaxPickUpDelayCharge can only be charged by the automatic expiration cron script. Some libraries would prefer to manually cancel holds via the "Holds over" tab of the "Holds awaiting pickup" page but still be able to charge the cancellation fee. Test Plan: 1) Apply this patch 2) Set ExpireReservesMaxPickUpDelayCharge to some number 3) Go to the Holds awaiting pickup page 4) Go to the Holds over tab 5) Note the new checkbox for charging the fee 6) Test canceling a single hold with the checkbox checked 7) Note a fee is charged 8) Test canceling a single hold with the checkbox unchecked 9) Note a fee is not charged 10) Test canceling all holds with the checkbox checked 11) Note the patrons are charged fees 12) Test canceling all holds without the checkbox checked 13) Note the patrons are not charged fees Signed-off-by: Jason Burds <JBurds@dubuque.lib.ia.us>
Created attachment 63359 [details] [review] Bug 15561 [QA Followup] - Add unit tests
Created attachment 63360 [details] [review] Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee
Created attachment 63361 [details] [review] Bug 15561 [QA Followup] - Add atomic update, fix other issues
Any updates on the 15561 bug? Carnegie Stout Team would be grateful
Created attachment 81904 [details] [review] Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab The fee set in ExpireReservesMaxPickUpDelayCharge can only be charged by the automatic expiration cron script. Some libraries would prefer to manually cancel holds via the "Holds over" tab of the "Holds awaiting pickup" page but still be able to charge the cancellation fee. Test Plan: 1) Apply this patch 2) Set ExpireReservesMaxPickUpDelayCharge to some number 3) Go to the Holds awaiting pickup page 4) Go to the Holds over tab 5) Note the new checkbox for charging the fee 6) Test canceling a single hold with the checkbox checked 7) Note a fee is charged 8) Test canceling a single hold with the checkbox unchecked 9) Note a fee is not charged 10) Test canceling all holds with the checkbox checked 11) Note the patrons are charged fees 12) Test canceling all holds without the checkbox checked 13) Note the patrons are not charged fees Signed-off-by: Jason Burds <JBurds@dubuque.lib.ia.us>
Created attachment 81905 [details] [review] Bug 15561 [QA Followup] - Add unit tests
Created attachment 81906 [details] [review] Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee
Created attachment 81907 [details] [review] Bug 15561 [QA Followup] - Add atomic update, fix other issues
Patches no longer apply, can you please rebase? warning: 1 line applied after fixing whitespace errors. Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt Auto-merging circ/waitingreserves.pl CONFLICT (content): Merge conflict in circ/waitingreserves.pl Auto-merging C4/Reserves.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab The copy of the patch that failed is found in: .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/Bug-15561---Add-ability-to-charge-cancelation-fee--N7o9xj.patch