Bug 15561 - Add ability to charge cancellation fee from "Holds over" tab
Summary: Add ability to charge cancellation fee from "Holds over" tab
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 14137 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-01-12 17:36 UTC by Kyle M Hall
Modified: 2019-01-27 14:18 UTC (History)
7 users (show)

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


Attachments
Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab (6.96 KB, patch)
2016-01-12 17:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab (101.14 KB, patch)
2016-04-18 13:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab (101.21 KB, patch)
2016-05-24 17:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab (7.45 KB, patch)
2016-06-27 17:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Add unit tests (3.69 KB, patch)
2016-06-27 17:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Add unit tests (3.77 KB, patch)
2016-06-27 17:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee (9.00 KB, patch)
2016-07-26 14:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab (7.43 KB, patch)
2017-05-10 16:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Add unit tests (3.77 KB, patch)
2017-05-10 16:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee (8.97 KB, patch)
2017-05-10 16:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Add atomic update, fix other issues (4.26 KB, patch)
2017-05-10 16:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 - Add ability to charge cancelation fee from "Holds over" tab (6.98 KB, patch)
2018-11-02 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Add unit tests (3.77 KB, patch)
2018-11-02 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee (7.67 KB, patch)
2018-11-02 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15561 [QA Followup] - Add atomic update, fix other issues (4.27 KB, patch)
2018-11-02 15:17 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-01-12 17:36:46 UTC
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.
Comment 1 Kyle M Hall 2016-01-12 17:38:08 UTC
*** Bug 14137 has been marked as a duplicate of this bug. ***
Comment 2 Kyle M Hall 2016-01-12 17:41:49 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2016-04-05 09:05:24 UTC
There is one other exception: if you cancel the hold when returning it - this will also charge the fee.
Comment 4 Owen Leonard 2016-04-12 16:01:41 UTC
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.
Comment 5 Kyle M Hall 2016-04-18 13:10:02 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2016-04-18 13:10:13 UTC
(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!
Comment 7 Kyle M Hall 2016-05-24 17:33:31 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-06-12 11:06:07 UTC
Kyle, please provide tests.

Your patch deletes 2 images, is it expected?
Comment 9 Kyle M Hall 2016-06-27 17:28:06 UTC
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>
Comment 10 Kyle M Hall 2016-06-27 17:28:13 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2016-06-27 17:29:01 UTC
Created attachment 52908 [details] [review]
Bug 15561 [QA Followup] - Add unit tests
Comment 12 Kyle M Hall 2016-06-27 17:30:16 UTC
(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!
Comment 13 Jonathan Druart 2016-07-02 13:20:37 UTC
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.
Comment 14 Kyle M Hall 2016-07-08 15:16:23 UTC
(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?
Comment 15 Jonathan Druart 2016-07-09 11:48:27 UTC
Sounds good to me :)
Katrin, do you agree?
Comment 16 Katrin Fischer 2016-07-26 06:02:49 UTC
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.
Comment 17 Katrin Fischer 2016-07-26 06:03:49 UTC
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.
Comment 18 Kyle M Hall 2016-07-26 14:13:44 UTC
Created attachment 53724 [details] [review]
Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee
Comment 19 Katrin Fischer 2016-07-26 20:26:31 UTC
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! :) )
Comment 20 Kyle M Hall 2017-05-10 16:54:52 UTC
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>
Comment 21 Kyle M Hall 2017-05-10 16:55:00 UTC
Created attachment 63359 [details] [review]
Bug 15561 [QA Followup] - Add unit tests
Comment 22 Kyle M Hall 2017-05-10 16:55:03 UTC
Created attachment 63360 [details] [review]
Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee
Comment 23 Kyle M Hall 2017-05-10 16:55:07 UTC
Created attachment 63361 [details] [review]
Bug 15561 [QA Followup] - Add atomic update, fix other issues
Comment 24 Brian 2018-11-01 19:42:42 UTC
Any updates on the 15561 bug?

Carnegie Stout Team would be grateful
Comment 25 Kyle M Hall 2018-11-02 15:17:07 UTC
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>
Comment 26 Kyle M Hall 2018-11-02 15:17:17 UTC
Created attachment 81905 [details] [review]
Bug 15561 [QA Followup] - Add unit tests
Comment 27 Kyle M Hall 2018-11-02 15:17:20 UTC
Created attachment 81906 [details] [review]
Bug 15561 [QA Followup] - Rename ExpireReservesMaxPickUpDelayCharge to WaitingHoldCancelationFee
Comment 28 Kyle M Hall 2018-11-02 15:17:24 UTC
Created attachment 81907 [details] [review]
Bug 15561 [QA Followup] - Add atomic update, fix other issues
Comment 29 Katrin Fischer 2019-01-27 14:18:43 UTC
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