Bug 16187 - Add a script to cancel unfilled holds after a specified number of days
Summary: Add a script to cancel unfilled holds after a specified number of days
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-01 15:31 UTC by Nick Clemens
Modified: 2017-10-20 20:15 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days (4.93 KB, patch)
2016-07-14 10:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days (4.99 KB, patch)
2016-10-12 13:06 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 16187 - (QA followup) Tidy, add confirm, minor changes (4.33 KB, patch)
2017-03-16 14:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days (9.40 KB, patch)
2017-04-06 18:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days (9.31 KB, patch)
2017-07-07 18:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days (9.31 KB, patch)
2017-10-19 09:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16187 - Followup (2.35 KB, patch)
2017-10-19 09:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days (9.41 KB, patch)
2017-10-20 06:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16187 - Followup (2.45 KB, patch)
2017-10-20 06:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16187: (QA follow-up) Additional fixes (1.97 KB, patch)
2017-10-20 06:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2016-04-01 15:31:24 UTC
Work to be done:
1) Create new script cancel_unfilled_holds.pl --days DAYS
2) This script would accept a --days parameter, and trigger C4::Reserves::CancelReserve for all unfilled holds with a 'reservedate' older than the the given number of days.
Comment 1 Nick Clemens 2016-07-14 10:55:05 UTC
Created attachment 53397 [details] [review]
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days

This script takes parameters:
days - how many days waiting to concal an unfilled hold on or after
library - (repeatable) branches to consider
holidays - whether or not to count holidays (default is no)

To test:
1 - Place some holds with varying reservedates
2 - Run script with different parameters to verify options are respected
(-v for verbosity will assist here)
3 - verify that script does nothing without days parameter

Sponsored by:
    Siskiyou County Library (http://www.siskiyoulibrary.info/)
Comment 2 Nicolas Legrand 2016-10-12 13:06:19 UTC
Created attachment 56275 [details] [review]
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days

This script takes parameters:
days - how many days waiting to concal an unfilled hold on or after
library - (repeatable) branches to consider
holidays - whether or not to count holidays (default is no)

To test:
1 - Place some holds with varying reservedates
2 - Run script with different parameters to verify options are respected
(-v for verbosity will assist here)
3 - verify that script does nothing without days parameter

Sponsored by:
    Siskiyou County Library (http://www.siskiyoulibrary.info/)

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 3 Katrin Fischer 2016-10-16 11:40:17 UTC
Hi Nick, what's the use case for this? How is it different to cancel_expired_holds.pl?
Comment 4 Katrin Fischer 2016-10-16 11:41:06 UTC
We are missing a 'feedback' status, so setting to 'failed qa' instead. Don't take it personally :)
Comment 5 Nick Clemens 2016-10-17 09:49:16 UTC
(In reply to Katrin Fischer from comment #3)
> Hi Nick, what's the use case for this? How is it different to
> cancel_expired_holds.pl?

Cancel expired holds is for holds that have been found and are waiting and expired. This script cancels holds that have never been found, so it can clean up holds placed on items that were lost or damaged and will never be filled.
Comment 6 Liz Rea 2016-12-13 03:26:34 UTC
Hi,

This script seems really dangerous, how will it know if it is cancelling a hold that is for something missing or lost, or one that someone legitimately placed on an item and it's just taking AGES to get through the (possibly very long) reserve list? I'd be concerned especially in libraries that routinely have very long waiting lists on popular titles.

I feel like this needs some safeguards.

Cheers,
Liz
Comment 7 Jonathan Druart 2016-12-13 14:36:17 UTC
Nick, do you have any good reasons to use DateTime instead of dt_from_string to get today's date?

Note that you could have use Koha::Libraries->search->get_column('branchcode') to get an iterator on the branchcodes.

Please resubmit tidying the whole file.
Comment 8 Nick Clemens 2017-03-16 14:40:52 UTC
Created attachment 61173 [details] [review]
Bug 16187 - (QA followup) Tidy, add confirm, minor changes
Comment 9 Nick Clemens 2017-03-16 14:44:29 UTC
(In reply to Liz Rea from comment #6)
> I feel like this needs some safeguards.
> 
> Cheers,
> Liz

I added a 'confirm' option so this can be run to see what would be deleted before going through with the deletion, is there another specific feedback you would like to see?
Comment 10 Nick Clemens 2017-03-16 14:45:12 UTC
(In reply to Jonathan Druart from comment #7)
> Nick, do you have any good reasons to use DateTime instead of dt_from_string
> to get today's date?
Nope, was a copy paste job


> Note that you could have use
> Koha::Libraries->search->get_column('branchcode') to get an iterator on the
> branchcodes.
Advice taken

 
> Please resubmit tidying the whole file.
Done.
Comment 11 Marcel de Rooy 2017-03-21 13:48:51 UTC
Should we send a notification if we cancel a hold?
Can remember to have seen such a report, but cannot find it now.
Comment 12 Jonathan Druart 2017-03-29 20:13:27 UTC
You must create a new subroutine to Koha::Holds and cover it with tests.
Comment 13 Jonathan Druart 2017-03-29 20:16:26 UTC
Is "waiting" in the name of the script appropriate?
Comment 14 Nick Clemens 2017-04-06 18:24:42 UTC
Created attachment 61951 [details] [review]
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days

This script takes parameters:
days - how many days waiting to concal an unfilled hold on or after
library - (repeatable) branches to consider
holidays - whether or not to count holidays (default is no)

This patchset adds two methods and covers them with tests:
Koha::Holds->unfilled(); To return holds where found = undef
Koha::Hold->age( $use_calendar ); To return the number of days since a
hold was placed (including or excluding holidays)

To test:
1 - Place some holds with varying reservedates
2 - Run script with different parameters to verify options are respected
(-v for verbosity will assist here)
3 - verify that script does nothing without days parameter

Sponsored by:
    Siskiyou County Library (http://www.siskiyoulibrary.info/)
Comment 15 Katrin Fischer 2017-04-12 06:22:15 UTC
(In reply to Marcel de Rooy from comment #11)
> Should we send a notification if we cancel a hold?
> Can remember to have seen such a report, but cannot find it now.

There is bug 12656, but no patch yet.
Comment 16 Nick Clemens 2017-07-07 18:05:58 UTC
Created attachment 64924 [details] [review]
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days

This script takes parameters:
days - how many days waiting to concal an unfilled hold on or after
library - (repeatable) branches to consider
holidays - whether or not to count holidays (default is no)

This patchset adds two methods and covers them with tests:
Koha::Holds->unfilled(); To return holds where found = undef
Koha::Hold->age( $use_calendar ); To return the number of days since a
hold was placed (including or excluding holidays)

To test:
1 - Place some holds with varying reservedates
2 - Run script with different parameters to verify options are respected
(-v for verbosity will assist here)
3 - verify that script does nothing without days parameter

Sponsored by:
    Siskiyou County Library (http://www.siskiyoulibrary.info/)
Comment 17 Marcel de Rooy 2017-10-19 08:56:45 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2017-10-19 08:57:35 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2017-10-19 09:40:08 UTC
No use for this change?
 sub waiting {
     my ( $self ) = @_;
 
-    return $self->search( { found => 'W' } );
+    return $self->search( { found => 'w' } );
I would prefer to keep the uppercase W here for finding it back.

+=head1 NAME
+
+cancel_waiting_holds.pl
+
+=head1 SYNOPSIS
+
+cancel_waiting_holds.pl
Old name remains.

+    -days                       cancel holds waiting this many days
+Specify the number of days waiting upon which to cancel holds.
The term waiting may still be confusing here. You mean holds that are so many days old or placed so many days ago.
Comment 20 Nick Clemens 2017-10-19 09:59:35 UTC
Created attachment 68289 [details] [review]
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days

This script takes parameters:
days - how many days waiting to concal an unfilled hold on or after
library - (repeatable) branches to consider
holidays - whether or not to count holidays (default is no)

This patchset adds two methods and covers them with tests:
Koha::Holds->unfilled(); To return holds where found = undef
Koha::Hold->age( $use_calendar ); To return the number of days since a
hold was placed (including or excluding holidays)

To test:
1 - Place some holds with varying reservedates
2 - Run script with different parameters to verify options are respected
(-v for verbosity will assist here)
3 - verify that script does nothing without days parameter

Sponsored by:
    Siskiyou County Library (http://www.siskiyoulibrary.info/)
Comment 21 Nick Clemens 2017-10-19 09:59:38 UTC
Created attachment 68290 [details] [review]
Bug 16187 - Followup

1 - Correct use of original (bad) script name
2 - Explain options better
3 - Remove change from 'W' to 'w'
Comment 22 Marcel de Rooy 2017-10-20 06:06:34 UTC
How was this tested ?

Can't locate object method "next" via package "13" (perhaps you forgot to load "13"?) at misc/cronjobs/holds/cancel_unfilled_holds.pl line 119.

Undefined subroutine &main::CancelReserve called at misc/cronjobs/holds/cancel_unfilled_holds.pl line 143.

Will submit a fix
Comment 23 Marcel de Rooy 2017-10-20 06:10:18 UTC
(In reply to Marcel de Rooy from comment #22)
> How was this tested ?
> 
> Undefined subroutine &main::CancelReserve called at
> misc/cronjobs/holds/cancel_unfilled_holds.pl line 143.

CancelReserve was moved to Koha::Hold of course..
Comment 24 Marcel de Rooy 2017-10-20 06:21:53 UTC
Created attachment 68309 [details] [review]
Bug 16187 - Add a script to cancel unfilled holds after a specified number of days

This script takes parameters:
days - how many days waiting to concal an unfilled hold on or after
library - (repeatable) branches to consider
holidays - whether or not to count holidays (default is no)

This patchset adds two methods and covers them with tests:
Koha::Holds->unfilled(); To return holds where found = undef
Koha::Hold->age( $use_calendar ); To return the number of days since a
hold was placed (including or excluding holidays)

To test:
1 - Place some holds with varying reservedates
2 - Run script with different parameters to verify options are respected
(-v for verbosity will assist here)
3 - verify that script does nothing without days parameter

Sponsored by:
    Siskiyou County Library (http://www.siskiyoulibrary.info/)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2017-10-20 06:21:57 UTC
Created attachment 68310 [details] [review]
Bug 16187 - Followup

1 - Correct use of original (bad) script name
2 - Explain options better
3 - Remove change from 'W' to 'w'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2017-10-20 06:22:01 UTC
Created attachment 68311 [details] [review]
Bug 16187: (QA follow-up) Additional fixes

Fix:
Can't locate object method "next" via package "13" (perhaps you forgot to load "13"?) at misc/cronjobs/holds/cancel_unfilled_holds.pl line 119.
Undefined subroutine &main::CancelReserve called at misc/cronjobs/holds/cancel_unfilled_holds.pl line 143.

The script does not use Koha::Object's get_column correctly for getting
the branch codes.
The call to CancelReserve is obsolete. Was moved in the meantime to
Koha::Hold->cancel.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Jonathan Druart 2017-10-20 20:15:57 UTC
Pushed to master for 17.11, thanks to everybody involved!