Bug 19260 - Reservations / holds marked as problems being seen as expired ones and deleted wrongly.
Summary: Reservations / holds marked as problems being seen as expired ones and delete...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Josef Moravec
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 12063
Blocks: 19437
  Show dependency treegraph
 
Reported: 2017-09-06 07:23 UTC by Ray Delahunty
Modified: 2017-10-14 12:40 UTC (History)
11 users (show)

See Also:
Change 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 19260 - Restore ExpireReservesMaxPickupDelay (3.30 KB, patch)
2017-09-28 11:29 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 19260 - Restore ExpireReservesMaxPickupDelay (3.37 KB, text/plain)
2017-09-29 19:49 UTC, Caroline Cyr La Rose
Details
Bug 19260: Add test for CancelExpiredReserves (2.78 KB, patch)
2017-10-05 20:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19260: Holds marked as problems being seen as expired ones and deleted wrongly (1.50 KB, patch)
2017-10-05 20:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19260: Add test for CancelExpiredReserves (2.85 KB, patch)
2017-10-05 21:43 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 19260: Holds marked as problems being seen as expired ones and deleted wrongly (1.62 KB, patch)
2017-10-05 21:44 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 19260: Add test for CancelExpiredReserves (2.95 KB, patch)
2017-10-06 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19260: Holds marked as problems being seen as expired ones and deleted wrongly (1.72 KB, patch)
2017-10-06 09:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19260: [QA Follow-up] Rearranging tests (11.70 KB, patch)
2017-10-06 09:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19260: [QA Follow-up] Remove obsolete $dbh (1.34 KB, patch)
2017-10-06 09:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19260: (followup) Fix CancelReserves.t test (983 bytes, patch)
2017-10-06 16:43 UTC, Josef Moravec
Details | Diff | Splinter Review
[17.05] Bug 19260: Add test for CancelExpiredReserves (2.92 KB, patch)
2017-10-12 12:10 UTC, Josef Moravec
Details | Diff | Splinter Review
[17.05] Bug 19260: Holds marked as problems being seen as expired ones and deleted wrongly (2.23 KB, patch)
2017-10-12 12:10 UTC, Josef Moravec
Details | Diff | Splinter Review
[17.05] Bug 19260: [QA Follow-up] Remove obsolete $dbh (1.14 KB, patch)
2017-10-12 12:10 UTC, Josef Moravec
Details | Diff | Splinter Review
[17.05] Bug 19260: (followup) Fix CancelReserves.t test (991 bytes, patch)
2017-10-12 12:10 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ray Delahunty 2017-09-06 07:23:40 UTC
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
Comment 1 Magnus Enger 2017-09-27 13:39:06 UTC
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.
Comment 2 Magnus Enger 2017-09-28 11:29:20 UTC
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.
Comment 3 Jonathan Druart 2017-09-28 16:06:19 UTC
We will need tests.
Comment 4 Caroline Cyr La Rose 2017-09-29 19:49:52 UTC Comment hidden (obsolete)
Comment 5 Caroline Cyr La Rose 2017-09-29 19:53:22 UTC
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.

:)
Comment 6 Josef Moravec 2017-10-04 08:22:59 UTC
(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.
Comment 7 Magnus Enger 2017-10-04 20:15:13 UTC
(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.
Comment 8 Josef Moravec 2017-10-05 20:40:30 UTC
Created attachment 67653 [details] [review]
Bug 19260: Add test for CancelExpiredReserves
Comment 9 Josef Moravec 2017-10-05 20:40:33 UTC
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
Comment 10 Josef Moravec 2017-10-05 20:41:32 UTC
(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 ;)
Comment 11 Alex Buckley 2017-10-05 21:43:06 UTC
Created attachment 67659 [details] [review]
Bug 19260: Add test for CancelExpiredReserves

Passes QA test tool
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 12 Alex Buckley 2017-10-05 21:44:40 UTC
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>
Comment 13 Alex Buckley 2017-10-05 21:46:17 UTC
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
Comment 14 Magnus Enger 2017-10-05 22:02:18 UTC
(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.
Comment 15 Marcel de Rooy 2017-10-06 07:49:05 UTC
QA: Looking here now
Comment 16 Marcel de Rooy 2017-10-06 09:16:58 UTC
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>
Comment 17 Marcel de Rooy 2017-10-06 09:17:02 UTC
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>
Comment 18 Marcel de Rooy 2017-10-06 09:17:07 UTC
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>
Comment 19 Marcel de Rooy 2017-10-06 09:17:11 UTC
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 20 Jonathan Druart 2017-10-06 14:28:47 UTC
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.
Comment 21 Jonathan Druart 2017-10-06 15:18:31 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 22 Jonathan Druart 2017-10-06 16:08:16 UTC
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.
Comment 23 Jonathan Druart 2017-10-06 16:11:19 UTC
(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.
Comment 24 Josef Moravec 2017-10-06 16:43:18 UTC
Created attachment 67733 [details] [review]
Bug 19260: (followup) Fix CancelReserves.t test

Test plan:
Run t/db_dependent/Holds/CancelReserves.t
Comment 25 Jonathan Druart 2017-10-06 19:47:01 UTC
Thanks Josef, patch push to master!
Comment 26 Marcel de Rooy 2017-10-09 07:32:32 UTC
(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).
Comment 27 Marcel de Rooy 2017-10-09 07:46:11 UTC
(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
Comment 28 Fridolin SOMERS 2017-10-12 11:12:28 UTC
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.
Comment 29 Josef Moravec 2017-10-12 12:10:11 UTC
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>
Comment 30 Josef Moravec 2017-10-12 12:10:15 UTC
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>
Comment 31 Josef Moravec 2017-10-12 12:10:19 UTC
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>
Comment 32 Josef Moravec 2017-10-12 12:10:23 UTC
Created attachment 67995 [details] [review]
[17.05] Bug 19260: (followup) Fix CancelReserves.t test

Test plan:
Run t/db_dependent/Holds/CancelReserves.t
Comment 33 Josef Moravec 2017-10-12 12:15:18 UTC
(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
Comment 34 Fridolin SOMERS 2017-10-12 12:30:11 UTC
Pushed to 17.05.x, will be in 17.05.05.

UT fails without correction and success with it.
Thanks a lot Josef.
Comment 35 Katrin Fischer 2017-10-14 12:39:43 UTC
Depends on bug 12063 that is not in 16.11.x