Description
Nick Clemens (kidclamp)
2016-04-01 15:31:24 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/) 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> Hi Nick, what's the use case for this? How is it different to cancel_expired_holds.pl? We are missing a 'feedback' status, so setting to 'failed qa' instead. Don't take it personally :) (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. 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 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. Created attachment 61173 [details] [review] Bug 16187 - (QA followup) Tidy, add confirm, minor changes (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? (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. Should we send a notification if we cancel a hold? Can remember to have seen such a report, but cannot find it now. You must create a new subroutine to Koha::Holds and cover it with tests. Is "waiting" in the name of the script appropriate? 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/) (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. 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/) (In reply to Jonathan Druart from comment #13) > Is "waiting" in the name of the script appropriate? I agree that waiting in the name is confusing. They should not be waiting. Unfilled as mentioned in the first comment would be better. Nick: Please change the filename. Hmm Seems OK already 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. 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/) 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' 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 (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.. 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> 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> 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> Pushed to master for 17.11, thanks to everybody involved! |