Bug 20724 - ReservesNeedReturns syspref breaks "Holds awaiting pickup"
Summary: ReservesNeedReturns syspref breaks "Holds awaiting pickup"
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 12063
Blocks: 20773
  Show dependency treegraph
 
Reported: 2018-05-07 22:04 UTC by Victor Grousset/tuxayo
Modified: 2018-09-24 13:10 UTC (History)
7 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 20724: Try to fix ReservesNeedReturns wrong behaviour (1.92 KB, patch)
2018-05-09 17:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20724 [Alternate]: Try to fix ReservesNeedReturns wrong behaviour (981 bytes, patch)
2018-05-10 12:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20724: Correctly mark an hold as waiting - ReservesNeedReturns (962 bytes, patch)
2018-05-10 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20724: Move the ReservesNeedReturns logic to AddReserve (2.82 KB, patch)
2018-05-10 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20724: Add tests for ReservesNeedReturns (3.54 KB, patch)
2018-05-10 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20724: Correctly mark an hold as waiting - ReservesNeedReturns (2.04 KB, patch)
2018-05-14 14:45 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 20724: Move the ReservesNeedReturns logic to AddReserve (2.86 KB, patch)
2018-05-14 14:45 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 20724: Add tests for ReservesNeedReturns (3.54 KB, patch)
2018-05-14 14:45 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 20724: Add tests for ReservesNeedReturns (3.60 KB, patch)
2018-05-15 09:06 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 20724: Correctly mark an hold as waiting - ReservesNeedReturns (2.14 KB, patch)
2018-05-16 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20724: Move the ReservesNeedReturns logic to AddReserve (2.95 KB, patch)
2018-05-16 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20724: Add tests for ReservesNeedReturns (3.69 KB, patch)
2018-05-16 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20724: (QA follow-up) Remove two obsolete comment lines (1007 bytes, patch)
2018-05-16 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20724: (QA follow-up) Add a simple db revision (1.50 KB, patch)
2018-05-16 13:45 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 Victor Grousset/tuxayo 2018-05-07 22:04:48 UTC
== Summary ==
waitingreserves.pl crashes when expirationdate is null.
And ReservesNeedReturns has a regression causing expirationdate to be null.

== Reproduce ==
1. Set ReservesNeedReturns to "Don't automatically"
2. Place a hold on a specific item
3. Check it in and confirm hold
4. The hold should have an expiration date
     record page → Hold → "Expiration" column
5. It should be listed in staff:/cgi-bin/koha/circ/waitingreserves.pl
6. Set ReservesNeedReturns to "Automatically"
7. Place a hold on a specific item
    (which should also behave like we check the item in to
    keep it for the patron)
8. The hold should have an empty expiration date
     record page → Hold → "Expiration" column
9. Holds awaiting pickup page should crash
     staff:/cgi-bin/koha/circ/waitingreserves.pl


=== Affected versions ===
Bug on master and 17.11.
Works on 16.11, date isn't null
Comment 1 Jonathan Druart 2018-05-09 16:48:42 UTC
It has been caused by
  commit 26634151dbce0bf39ff6c9eda5b58bb993996a2d
  Bug 12063 - Fix QA failures

Commit message says:
      - remove expiration date calculation in circ/waitingreserves.pl. Use
        the one in DB,
Comment 2 Jonathan Druart 2018-05-09 16:57:04 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2018-05-09 17:07:10 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2018-05-09 17:32:05 UTC
Created attachment 75211 [details] [review]
Bug 20724: Try to fix ReservesNeedReturns wrong behaviour

We need to call Koha::Hold->set_waiting to correctly calculate the
expiration date.

It fixes a regression introduced by
  commit 26634151dbce0bf39ff6c9eda5b58bb993996a2d
  Bug 12063 - Fix QA failures

The expiration date has to be set for waiting holds

TODO: same fix has to be provided for the OPAC side
TODO LATER: We certainly should move code from controllers to
Koha::Hold
Comment 5 Jonathan Druart 2018-05-09 17:32:58 UTC
That does not seem the best way to fix this issue but should work. Could you confirm?
Which regressions possible?
Comment 6 Kyle M Hall 2018-05-10 12:14:07 UTC
Created attachment 75219 [details] [review]
Bug 20724 [Alternate]: Try to fix ReservesNeedReturns wrong behaviour

We need to call Koha::Hold->set_waiting to correctly
calculate the expiration date.

It fixes a regression introduced by
  commit 26634151dbce0bf39ff6c9eda5b58bb993996a2d
  Bug 12063 - Fix QA failures

The expiration date has to be set for waiting holds
Comment 7 Jonathan Druart 2018-05-10 14:42:37 UTC
Created attachment 75221 [details] [review]
Bug 20724: Correctly mark an hold as waiting - ReservesNeedReturns

We need to call Koha::Hold->set_waiting to correctly
calculate the expiration date.

It fixes a regression introduced by
  commit 26634151dbce0bf39ff6c9eda5b58bb993996a2d
  Bug 12063 - Fix QA failures

The expiration date has to be set for waiting holds
Comment 8 Jonathan Druart 2018-05-10 14:42:41 UTC
Created attachment 75222 [details] [review]
Bug 20724: Move the ReservesNeedReturns logic to AddReserve
Comment 9 Jonathan Druart 2018-05-10 14:42:45 UTC
Created attachment 75223 [details] [review]
Bug 20724: Add tests for ReservesNeedReturns
Comment 10 Victor Grousset/tuxayo 2018-05-14 14:45:01 UTC
Created attachment 75307 [details] [review]
Bug 20724: Correctly mark an hold as waiting - ReservesNeedReturns

We need to call Koha::Hold->set_waiting to correctly
calculate the expiration date.

It fixes a regression introduced by
  commit 26634151dbce0bf39ff6c9eda5b58bb993996a2d
  Bug 12063 - Fix QA failures

The expiration date has to be set for waiting holds

== Test plan (time to execute: less than 4min) ==
1. Set ReservesNeedReturns to "Don't automatically"
2. Place a hold on a specific item
3. Check it in and confirm hold
4. The hold should have an expiration date
     record page → Hold → "Expiration" column
5. It should be listed in staff:/cgi-bin/koha/circ/waitingreserves.pl
6. Set ReservesNeedReturns to "Automatically"
7. Place a hold on a specific item
    (which should also behave like we check the item in to
    keep it for the patron)
8. The hold should have an *empty* expiration date
     record page → Hold → "Expiration" column
9. Holds awaiting pickup page should crash
     staff:/cgi-bin/koha/circ/waitingreserves.pl
10. Cancel the hold to remove the corrupted data
     record page → Hold → the red X
11. Apply this patch
12. Place a hold on a specific item
13. The hold should have an expiration date (not empty)
14. It should be listed in staff:/cgi-bin/koha/circ/waitingreserves.pl
15. Celebrate!

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Comment 11 Victor Grousset/tuxayo 2018-05-14 14:45:07 UTC
Created attachment 75308 [details] [review]
Bug 20724: Move the ReservesNeedReturns logic to AddReserve

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Comment 12 Victor Grousset/tuxayo 2018-05-14 14:45:10 UTC Comment hidden (obsolete)
Comment 13 Victor Grousset/tuxayo 2018-05-15 09:06:42 UTC
Created attachment 75325 [details] [review]
Bug 20724: Add tests for ReservesNeedReturns

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Comment 14 Victor Grousset/tuxayo 2018-05-15 10:05:57 UTC
- Added a test plan
- Confirmed that the patch works
- Ran the tests
  - with the patches to ensure that they pass
  - and without the patches to ensure that the test are actually testing something
Comment 15 Victor Grousset/tuxayo 2018-05-15 12:31:40 UTC
== Going further: existing data ==
This bug creates corrupted data that will still break the page. (waiting reserves with a NULL expirationdate)

What do we do in these case?
Comment 16 Katrin Fischer 2018-05-15 22:31:17 UTC
could we recalculate the expiration date for waiting items using the timestamp and ReservesMaxPickupDelay value?
Comment 17 Victor Grousset/tuxayo 2018-05-16 07:28:30 UTC
Same bug or followup? So the patch will be quickly released to at least avoid generating corrupted data.
Comment 18 Katrin Fischer 2018-05-16 07:54:17 UTC
If it's just a little database update I'd put it here I think. I can sign off this afternoon if noone else is faster.
Comment 19 Katrin Fischer 2018-05-16 07:56:56 UTC
Hm, might be a little more complicated if you want the absolute correct date (with/without holidays). So we'd need to do what the code should have done?
Comment 20 Victor Grousset/tuxayo 2018-05-16 08:11:42 UTC
(In reply to Katrin Fischer from comment #18)
> If it's just a little database update I'd put it here I think. 

+1


(In reply to Katrin Fischer from comment #19)
> Hm, might be a little more complicated if you want the absolute correct date
> (with/without holidays). So we'd need to do what the code should have done?

Most of the existing corrupted data[1] should be in the past so it will be expired anyway. Is that correct? Is yes then no need to worry about making an accurate correction.

[1] And there is not much considering that it hasn't been reported until now.
Comment 21 Victor Grousset/tuxayo 2018-05-16 08:13:38 UTC
Posting the error here so someone searching for it will find this bug.

Date::Calc::Date_to_Days(): not a valid date at /home/koha/src/circ/waitingreserves.pl line 122.
Comment 22 Katrin Fischer 2018-05-16 08:44:53 UTC
I am not sure how big of a problem it is, but it probably depends on how long a library allows for items to be picked up.
Comment 23 Marcel de Rooy 2018-05-16 11:19:59 UTC
QA: Looking here now
Comment 24 Marcel de Rooy 2018-05-16 12:41:02 UTC
for ( my $i = 0 ; $i < $holds_to_place_count ; $i++ ) {
                AddReserve( $branch, $borrower->{'borrowernumber'},
                    $biblionumber, \@realbi, $rank[0], $startdate, $expirationdate, $notes, $title,
                    $checkitem, $found, $itemtype );
            }

Horrible existing code with realbi and holds_to_place_count.. Also checkitem is passed to AddReserve each time in a for loop with biblionumbers. (It should be undef but you never know..)

Note that the second patch removes $holds_to_place_count = 1.

So lots of distraction here again with holds.
Comment 25 Marcel de Rooy 2018-05-16 12:55:57 UTC
This might affect C4/ILSDI/Services.pm => One of the calls includes an itemnumber. Note that the code did not contain a reference to ReservesNeedsReturns.

Koha/REST/V1/Hold.pm is also affected => Includes an itemnumber. Note that the code did not contain a reference to ReservesNeedsReturns.

C4/SIP/ILS/Transaction/Hold.pm is NOT affected (no item number)
serials/routing-preview.pl is not affected
Comment 26 Marcel de Rooy 2018-05-16 13:45:39 UTC
Created attachment 75363 [details] [review]
Bug 20724: Correctly mark an hold as waiting - ReservesNeedReturns

We need to call Koha::Hold->set_waiting to correctly
calculate the expiration date.

It fixes a regression introduced by
  commit 26634151dbce0bf39ff6c9eda5b58bb993996a2d
  Bug 12063 - Fix QA failures

The expiration date has to be set for waiting holds

== Test plan (time to execute: less than 4min) ==
1. Set ReservesNeedReturns to "Don't automatically"
2. Place a hold on a specific item
3. Check it in and confirm hold
4. The hold should have an expiration date
     record page → Hold → "Expiration" column
5. It should be listed in staff:/cgi-bin/koha/circ/waitingreserves.pl
6. Set ReservesNeedReturns to "Automatically"
7. Place a hold on a specific item
    (which should also behave like we check the item in to
    keep it for the patron)
8. The hold should have an *empty* expiration date
     record page → Hold → "Expiration" column
9. Holds awaiting pickup page should crash
     staff:/cgi-bin/koha/circ/waitingreserves.pl
10. Cancel the hold to remove the corrupted data
     record page → Hold → the red X
11. Apply this patch
12. Place a hold on a specific item
13. The hold should have an expiration date (not empty)
14. It should be listed in staff:/cgi-bin/koha/circ/waitingreserves.pl
15. Celebrate!

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2018-05-16 13:45:43 UTC
Created attachment 75364 [details] [review]
Bug 20724: Move the ReservesNeedReturns logic to AddReserve

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2018-05-16 13:45:47 UTC
Created attachment 75365 [details] [review]
Bug 20724: Add tests for ReservesNeedReturns

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2018-05-16 13:45:52 UTC
Created attachment 75366 [details] [review]
Bug 20724: (QA follow-up) Remove two obsolete comment lines

No test plan :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2018-05-16 13:45:56 UTC
Created attachment 75367 [details] [review]
Bug 20724: (QA follow-up) Add a simple db revision

We should correct bad data too.
This is a trivial, pragmatic approach. Instead of doing a complex
calculation with holidays etc. (as was done on 12063), we just add
MaxPickupDelay or 7 days to today probably resulting in a little bonus
time. Only for waiting holds. When the ReservesNeedReturns pref was not
Automatic, there should be no changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2018-05-16 13:47:24 UTC
With previous comments in mind, balance goes to Passed QA for this patch set.
Added a simple db rev (we should imo). 
@RM: If you want to refine, please do.
Comment 32 Jonathan Druart 2018-05-16 14:22:28 UTC
Comment on attachment 75367 [details] [review]
Bug 20724: (QA follow-up) Add a simple db revision

Moved to bug 20773.
Comment 33 Jonathan Druart 2018-05-16 15:28:26 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 34 Fridolin SOMERS 2018-08-27 12:52:09 UTC
Pushed to 17.11.x for 17.11.09
Comment 35 Fridolin SOMERS 2018-09-24 13:10:17 UTC
Pushed to 17.05.x for 17.05.14