== 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
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,
I do not think it's link to ReservesNeedReturns, it happens when no expiration date is selected.
(In reply to Jonathan Druart from comment #2) > I do not think it's link to ReservesNeedReturns, it happens when no > expiration date is selected. Forget that, it's linked to this pref
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
That does not seem the best way to fix this issue but should work. Could you confirm? Which regressions possible?
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
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
Created attachment 75222 [details] [review] Bug 20724: Move the ReservesNeedReturns logic to AddReserve
Created attachment 75223 [details] [review] Bug 20724: Add tests for ReservesNeedReturns
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>
Created attachment 75308 [details] [review] Bug 20724: Move the ReservesNeedReturns logic to AddReserve Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Created attachment 75309 [details] [review] Bug 20724: Add tests for ReservesNeedReturns
Created attachment 75325 [details] [review] Bug 20724: Add tests for ReservesNeedReturns Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
- 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
== 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?
could we recalculate the expiration date for waiting items using the timestamp and ReservesMaxPickupDelay value?
Same bug or followup? So the patch will be quickly released to at least avoid generating corrupted data.
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.
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?
(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.
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.
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.
QA: Looking here now
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.
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
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>
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>
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>
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>
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>
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 on attachment 75367 [details] [review] Bug 20724: (QA follow-up) Add a simple db revision Moved to bug 20773.
Pushed to master for 18.05, thanks to everybody involved!
Pushed to 17.11.x for 17.11.09
Pushed to 17.05.x for 17.05.14