Bug 21181 - Cancel expired holds cronjob does not cancel holds in transit if ExpireReservesMaxPickUpDelay not set
Summary: Cancel expired holds cronjob does not cancel holds in transit if ExpireReserv...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-08 17:18 UTC by Nick Clemens
Modified: 2019-10-14 20:02 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
This patch corrects a behaviour where an in transit hold would not be cancelled if even the patron specified they did not need the hold after a certain date. In some cases they would receive a notice to pickup a hold they no longer wanted. Now these holds will be cancelled while in transit, and should be routed to their home location when checked in with no notice to the patron.
Version(s) released in:
19.11.00


Attachments
Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set (1.25 KB, patch)
2018-08-08 17:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set (1.38 KB, patch)
2018-10-24 05:59 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21181: Unit tests (2.25 KB, patch)
2019-04-24 01:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set (1.26 KB, patch)
2019-04-24 01:23 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 21181: Unit tests (2.32 KB, patch)
2019-06-11 13:40 UTC, Claire Gravely
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set (1.33 KB, patch)
2019-06-11 13:40 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 21181: Unit tests (2.41 KB, patch)
2019-07-05 06:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set (1.42 KB, patch)
2019-07-05 06:34 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 2018-08-08 17:18:51 UTC
Debatable, but that problem as it arose:
1 - Patron places a hold, sets 'not needed after' to July 1
2 - Hold is put in transit
3 - Time passes, it is now July 2
4 - Cancel expired holds cron does not cancel hold - found='W'
5 - Hold arrives at destination on July 3
6 - Hold is trapped, patron is notified
Comment 1 Nick Clemens 2018-08-08 17:28:15 UTC
Created attachment 77582 [details] [review]
Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set

To test:
0 - Make sure ExpireReservesMaxPickupDelay is set to 'Don't allow'
1 - Place a hold for a patron
2 - Check in at another library to set it in transit
3 - Update expirationdate of hold to be in the past
    UPDATE reserves SET expirationdate='1999-12-31'
4 - Run cancel_expired_holds.pl
5 - Hold is not canceled
6 - Apply patch
7 - Run again
8 - Hold is cancelled
Comment 2 Alex Buckley 2018-10-24 05:59:19 UTC
Created attachment 81049 [details] [review]
Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set

To test:
0 - Make sure ExpireReservesMaxPickupDelay is set to 'Don't allow'
1 - Place a hold for a patron
2 - Check in at another library to set it in transit
3 - Update expirationdate of hold to be in the past
    UPDATE reserves SET expirationdate='1999-12-31'
4 - Run cancel_expired_holds.pl
5 - Hold is not canceled
6 - Apply patch
7 - Run again
8 - Hold is cancelled

Followed test plan and patch works as described, also passes QA test
tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 3 Jonathan Druart 2018-10-31 19:23:18 UTC
It seems that you forgot to attach the tests you wrote before this patch.
Comment 4 Nick Clemens 2019-04-24 01:23:48 UTC
Created attachment 88583 [details] [review]
Bug 21181: Unit tests
Comment 5 Nick Clemens 2019-04-24 01:23:51 UTC
Created attachment 88584 [details] [review]
Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set

To test:
0 - Make sure ExpireReservesMaxPickupDelay is set to 'Don't allow'
1 - Place a hold for a patron
2 - Check in at another library to set it in transit
3 - Update expirationdate of hold to be in the past
    UPDATE reserves SET expirationdate='1999-12-31'
4 - Run cancel_expired_holds.pl
5 - Hold is not canceled
6 - Apply patch
7 - Run again
8 - Hold is cancelled
Comment 6 Claire Gravely 2019-06-11 13:40:07 UTC
Created attachment 90489 [details] [review]
[SIGNED-OFF] Bug 21181: Unit tests

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 7 Claire Gravely 2019-06-11 13:40:10 UTC
Created attachment 90490 [details] [review]
[SIGNED-OFF] Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set

To test:
0 - Make sure ExpireReservesMaxPickupDelay is set to 'Don't allow'
1 - Place a hold for a patron
2 - Check in at another library to set it in transit
3 - Update expirationdate of hold to be in the past
    UPDATE reserves SET expirationdate='1999-12-31'
4 - Run cancel_expired_holds.pl
5 - Hold is not canceled
6 - Apply patch
7 - Run again
8 - Hold is cancelled

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 8 Jonathan Druart 2019-06-19 01:15:27 UTC
(In reply to Nick Clemens from comment #0)
> Debatable, but that problem as it arose:

Should we ask the list then?
Comment 9 Marcel de Rooy 2019-06-21 09:44:58 UTC
QAing
Comment 10 Marcel de Rooy 2019-06-21 09:55:43 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Nick Clemens from comment #0)
> > Debatable, but that problem as it arose:
> 
> Should we ask the list then?

Yeah, might be safer way to go.
Comment 11 Katrin Fischer 2019-06-30 16:41:51 UTC
+1 (little late for a comment on the list)
Comment 12 Nick Clemens 2019-07-03 12:18:11 UTC
List comments were positive, I also taked with our trainers. They agreed, if a patron has said they don't want a book after a certain date, then they don't want it after that date.

In terms of patron experience, they don't know the book is in-transit, so would be a mystery why they sometimes get a notice for a hold they said they didn't want.

Back to Signed Off
Comment 13 Marcel de Rooy 2019-07-05 06:34:23 UTC
Created attachment 91313 [details] [review]
Bug 21181: Unit tests

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2019-07-05 06:34:27 UTC
Created attachment 91314 [details] [review]
Bug 21181: Make cancel_expired_reserves.pl Cancel all holds not waiting if ExpireReservesMaxPickupDelay not set

To test:
0 - Make sure ExpireReservesMaxPickupDelay is set to 'Don't allow'
1 - Place a hold for a patron
2 - Check in at another library to set it in transit
3 - Update expirationdate of hold to be in the past
    UPDATE reserves SET expirationdate='1999-12-31'
4 - Run cancel_expired_holds.pl
5 - Hold is not canceled
6 - Apply patch
7 - Run again
8 - Hold is cancelled

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Martin Renvoize 2019-07-05 07:45:31 UTC
Nice work!

Pushed to master for 19.11.00
Comment 16 Fridolin SOMERS 2019-07-25 13:46:46 UTC
This is a small behavior change but I think we should avoid backport to stable.

This should have a text to go in the release notes no ?
Comment 17 Fridolin SOMERS 2019-07-29 15:02:42 UTC
(In reply to Fridolin SOMERS from comment #16)
> This should have a text to go in the release notes no ?

Thanks a lot Nick ;)