We have the ReservesMaxPickupDelay set to 7 and the ExpireReservesMaxPickupDelay system parameter set to Don’t Allow. Our workflow is that staff use the report generated by waitingreserves.pl to identify titles to retrieve from the Reservations / holds awaiting pickup shelf, and delete the reservation and return the items to the shelf, or have that action trigger reservation satisfaction for the next person in the queue. Since our upgrade for 16.05 to 17.05 we are finding that when we run the cancel_expired_holds.pl job at 01:00 daily, reservations marked as problem ones are being deleted in addition to expired ones (expiration date set by user has been reached, or the default 100 days has been reached). Koha 17.05 appears to be disregarding the ExpireReservesMaxPickupDelay system parameter. (http://translate.koha-community.org/manual/master/en/html/17_cron_jobs.html#expiredholdscron). We have suspended this cron job in the meantime, and will write a query to identify expired ones we need to delete. Ideally we want problem reservations NOT to be seen as expired ones. Ray Delahunty
As far as I can tell, this is a regression introduced by bug 12063. Attachment 63091 [details] on that bug moves the code that takes care of ExpireReservesMaxPickupDelay from C4/Reserves.pm (sub CancelExpiredReserves): - if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") ) { - my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); - my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); to Koha/Hold.pm (sub set_waiting): + if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") ) { + my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); + my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); + my $calendar = Koha::Calendar->new( branchcode => $self->branchcode ); Then attachment 63095 [details] [review] removes the check on ExpireReservesMaxPickUpDelay: - if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") ) { - my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); - my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); - my $calendar = Koha::Calendar->new( branchcode => $self->branchcode ); ... + my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); + my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); + my $calendar = Koha::Calendar->new( branchcode => $self->branchcode ); But it is not clear from the commit message that this is intentional... Perhaps the fix is as easy as putting the if on ExpireReservesMaxPickUpDelay back in? I will investigate this further, if noone beats me to it.
Created attachment 67428 [details] [review] Bug 19260 - Restore ExpireReservesMaxPickupDelay It looks like bug 12063 accidentally removed the functionality of ExpireReservesMaxPickupDelay. This patch aims to restore it. To test: Before applying the patch: - Make sure ExpireReservesMaxPickUpDelay = "Don't allow" - Check out a book to a user - Add a reservastion on it for another user - Check the hold in the database. You should have: found = W, waitingdate = today, expirationdate = today + the value of ReservesMaxPickUpDelay The problem here is the expirationdate, it should be empty when ExpireReservesMaxPickUpDelay = "Don't allow". After applying the patch: - Repeat the steps above - expirationdate should now be NULL Please be creative when testing this. I am not sure I have seen all the possible concequences.
We will need tests.
Created attachment 67489 [details] Bug 19260 - Restore ExpireReservesMaxPickupDelay It looks like bug 12063 accidentally removed the functionality of ExpireReservesMaxPickupDelay. This patch aims to restore it. To test: Before applying the patch: - Make sure ExpireReservesMaxPickUpDelay = "Don't allow" - Check out a book to a user - Add a reservastion on it for another user - Check the hold in the database. You should have: found = W, waitingdate = today, expirationdate = today + the value of ReservesMaxPickUpDelay The problem here is the expirationdate, it should be empty when ExpireReservesMaxPickUpDelay = "Don't allow". After applying the patch: - Repeat the steps above - expirationdate should now be NULL Please be creative when testing this. I am not sure I have seen all the possible concequences. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Works as advertised. There was a step missing from the test plan, however, you have to return the reserved item for the db to show W in "found" field and (today's) date in "waiting date". Once I figured that out, everything worked as you said. :)
(In reply to Magnus Enger from comment #2) > Created attachment 67428 [details] [review] [review] > Bug 19260 - Restore ExpireReservesMaxPickupDelay > > It looks like bug 12063 accidentally removed the functionality of > ExpireReservesMaxPickupDelay. This patch aims to restore it. > > To test: > > Before applying the patch: > - Make sure ExpireReservesMaxPickUpDelay = "Don't allow" > - Check out a book to a user > - Add a reservastion on it for another user > - Check the hold in the database. You should have: > found = W, waitingdate = today, expirationdate = today + the value of > ReservesMaxPickUpDelay > > The problem here is the expirationdate, it should be empty when > ExpireReservesMaxPickUpDelay = "Don't allow". > > After applying the patch: > - Repeat the steps above > - expirationdate should now be NULL > > Please be creative when testing this. I am not sure I have seen > all the possible concequences. I don't think, that is the right place to fix it... after 12063, we really need a date in expirationdate I think... ExpireReservesMaxPickUpDelay just has to say, if cancel also waiting reserves or not... Attachment 63095 [details] Bug 12063 - Fix QA failures added this to C4::Reserves.pm + sub CancelExpiredReserves { + return unless C4::Context->preference("ExpireReservesMaxPickUpDelay"); After this Marcel's comment: sub CancelExpiredReserves { + return unless C4::Context->preference("ExpireReservesMaxPickUpDelay"); This may have a unwanted side-effect. If we do not use the pickup delay, but still want to cancel expired reserves. It is no longer possible. (Note that patrons may have entered expiration dates too.) Not marking this as a blocker, since this is probably exceptional. Do you have an easy fix or can you open a new report for it? The return line was removed... The comment is right, when ExpireReservesMaxPickUpDelay is false, then no reserve is canceled - even that which should be (not waiting bud after expiration date) But the solution should be more complex (and yes, we will need tests for this) The CancelExpiredReserved has to deal with ExpireReservesMaxPickUpDelay - nad use the different SQL for this two cases.
(In reply to Josef Moravec from comment #6) > But the solution should be more complex (and yes, we will need tests for > this) > > The CancelExpiredReserved has to deal with ExpireReservesMaxPickUpDelay - > nad use the different SQL for this two cases. Could you make an alternative patch for that? I will sign off.
Created attachment 67653 [details] [review] Bug 19260: Add test for CancelExpiredReserves
Created attachment 67654 [details] [review] Bug 19260: Holds marked as problems being seen as expired ones and deleted wrongly Test plan: 0) Apply just the first patch - the one with test 1) Run t/db_dependent/Reserves.t - test for CancelExpiredReserves should fail 2) Apply the second patch 3) t/db_dependent/Reserves.t should pass now
(In reply to Magnus Enger from comment #7) > (In reply to Josef Moravec from comment #6) > > But the solution should be more complex (and yes, we will need tests for > > this) > > > > The CancelExpiredReserved has to deal with ExpireReservesMaxPickUpDelay - > > nad use the different SQL for this two cases. > > Could you make an alternative patch for that? I will sign off. I did try ;)
Created attachment 67659 [details] [review] Bug 19260: Add test for CancelExpiredReserves Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 67660 [details] [review] Bug 19260: Holds marked as problems being seen as expired ones and deleted wrongly Test plan: 0) Apply just the first patch - the one with test 1) Run t/db_dependent/Reserves.t - test for CancelExpiredReserves should fail 2) Apply the second patch 3) t/db_dependent/Reserves.t should pass now Followed test plan, patch worked as described. Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Oh Sorry Magnus I just saw your comment that you would test and sign off on these patches, apologies that I tested and found that everything worked and passed the QA test tool so I signed off
(In reply to Alex Buckley from comment #13) > Oh Sorry Magnus I just saw your comment that you would test and sign off on > these patches, apologies that I tested and found that everything worked and > passed the QA test tool so I signed off Hehe, absolutely no need to apologize for that! :-) Just very happy to see this moving along.
QA: Looking here now
Created attachment 67677 [details] [review] Bug 19260: Add test for CancelExpiredReserves Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67678 [details] [review] Bug 19260: Holds marked as problems being seen as expired ones and deleted wrongly Test plan: 0) Apply just the first patch - the one with test 1) Run t/db_dependent/Reserves.t - test for CancelExpiredReserves should fail 2) Apply the second patch 3) t/db_dependent/Reserves.t should pass now Followed test plan, patch worked as described. Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67679 [details] [review] Bug 19260: [QA Follow-up] Rearranging tests The first patch adds CancelExpiredReserves tests to Reserves.t. But note that we already have some tests in Holds/CancelReserves.t. This patch does: Renames Holds/CancelReserves.t to Reserves/CancelExpiredReserves.t. Rearranges modules there. Moves its existing tests into a first subtest. Moves the new subtest from Reserves.t to CancelExpiredReserves.t. Replaces $dbh->do('DELETE FROM reserves'). Adds some TestBuilder statements for missing data (by the move): adding biblio, item, borrower (removing slow AddMember call). Test plan: Run Reserves.t and Reserves/CancelExpiredReserves.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67680 [details] [review] Bug 19260: [QA Follow-up] Remove obsolete $dbh The variable is no longer used. Removed a few empty lines on the way. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment on attachment 67679 [details] [review] Bug 19260: [QA Follow-up] Rearranging tests I prefer to see this change done on a separate bug report.
Pushed to master for 17.11, thanks to everybody involved!
Two tests are failing, please fix ASAP t/db_dependent/Holds/CancelReserves.t .. 1/5 # Failed test 'reserve 2 should be canceled.' # at t/db_dependent/Holds/CancelReserves.t line 61. # got: 'Koha::Hold=HASH(0x9fbec48)' # expected: undef # Failed test 'Reserve 3 should be canceled.' # at t/db_dependent/Holds/CancelReserves.t line 96. # got: 'Koha::Hold=HASH(0x9f3a750)' # expected: undef # Looks like you failed 2 tests of 5.
(In reply to Marcel de Rooy from comment #18) > Created attachment 67679 [details] [review] [review] > Bug 19260: [QA Follow-up] Rearranging tests Tests fail even with this patch.
Created attachment 67733 [details] [review] Bug 19260: (followup) Fix CancelReserves.t test Test plan: Run t/db_dependent/Holds/CancelReserves.t
Thanks Josef, patch push to master!
(In reply to Jonathan Druart from comment #23) > (In reply to Marcel de Rooy from comment #18) > > Created attachment 67679 [details] [review] [review] [review] > > Bug 19260: [QA Follow-up] Rearranging tests > > Tests fail even with this patch. Yes, as Joseph mentioned, ExpireReservesMaxPickUpDelay should be Allow (as was my case already).
(In reply to Jonathan Druart from comment #20) > Comment on attachment 67679 [details] [review] [review] > Bug 19260: [QA Follow-up] Rearranging tests > > I prefer to see this change done on a separate bug report. See bug 19437
Patch "Holds marked as problems being seen as expired ones and deleted wrongly" does not apply easily to 17.05.x. Please provide a rebased patch.
Created attachment 67992 [details] [review] [17.05] Bug 19260: Add test for CancelExpiredReserves Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67993 [details] [review] [17.05] Bug 19260: Holds marked as problems being seen as expired ones and deleted wrongly Test plan: 0) Apply just the first patch - the one with test 1) Run t/db_dependent/Reserves.t - test for CancelExpiredReserves should fail 2) Apply the second patch 3) t/db_dependent/Reserves.t should pass now Followed test plan, patch worked as described. Passes QA test tool Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67994 [details] [review] [17.05] Bug 19260: [QA Follow-up] Remove obsolete $dbh The variable is no longer used. Removed a few empty lines on the way. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67995 [details] [review] [17.05] Bug 19260: (followup) Fix CancelReserves.t test Test plan: Run t/db_dependent/Holds/CancelReserves.t
(In reply to Fridolin SOMERS from comment #28) > Patch "Holds marked as problems being seen as expired ones and deleted > wrongly" does not apply easily to 17.05.x. > Please provide a rebased patch. I rebased whole patchset, qa tools passes, test passes
Pushed to 17.05.x, will be in 17.05.05. UT fails without correction and success with it. Thanks a lot Josef.
Depends on bug 12063 that is not in 16.11.x