Using ILS-DI's HoldTitle service, if we request for holding a title which has no item, we get this error: Can't use an undefined value as a HASH reference at /kohaclone/C4/ILSDI/Services.pm line 670 http://catalog.kohaclone/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
Created attachment 81754 [details] [review] Bug 21738: Move counting of items in C4::Reserves::CanBookBeReserved Test plan: - Try holding a title without items via ILS-DI => error, - try the same via UI => message is "Cannot place hold: this record has no items attached." - apply this patch, - try to hold a title without items via ILS-DI => "<code>NotHoldable</code>", - try again via UI => message is still "<code>NotHoldable</code>"
If I hit /reserve/request.pl?biblionumbers=1/2/3&multi_hold=1 I see: https://snag.gy/myA5aP.jpg The patron search box should not be there. If I search for a patron, I get: https://snag.gy/tuRJb1.jpg
Created attachment 82251 [details] [review] Bug 21738: check items count in C4:ILSDI::HoldTitle Test plan: - Try holding a title without items via ILS-DI (cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1), - you get an error, - apply this patch, - try again, - you should get the code "Notitems"Bug 21738: check items count in C4:ILSDI::HoldTitle
The patch above is a replacement one. Since we need to know the number of items before having the patron making the request, we can't check items count in CanBookBeReserved. So i simply write the same test in ILSDI::HoldTitle than in reserve/request.pl
This is a regression since 18.05, we need it into 18.11 Caused by commit 5f485e476b2c5521d59bda6d48b56ed36cff3f86 Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass limit to template
I am wondering if we should not move $canReserve->{status} //= '' from reserve/request.pl to CanBookBeReserved That will make the calls to CanBookBeReserved safe. Kyle, what are your thoughts about that?
(In reply to Jonathan Druart from comment #6) > I am wondering if we should not move > $canReserve->{status} //= '' > from reserve/request.pl to CanBookBeReserved > > That will make the calls to CanBookBeReserved safe. > > Kyle, what are your thoughts about that? Yes, that sounds like a good idea to me!
return { code => 'NotHoldable' } unless CanBookBeReserved( $borrowernumber, $biblionumber )->{status} eq 'OK'; This line just needs attention imo. If there are no items, you may get back an undef. So just check that. And do not assume you have a hash. No need to duplicate the hostitems stuff at all?
Created attachment 83637 [details] [review] Bug 21738: make call of CanBookBeReserved more safe
Created attachment 83752 [details] [review] Bug 21738: check items count in C4:ILSDI::HoldTitle Test plan: - Try holding a title without items via ILS-DI (cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1), - you get an error, - apply this patch, - try again, - you should get the code "Notitems"Bug 21738: check items count in C4:ILSDI::HoldTitle Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 83753 [details] [review] Bug 21738: make call of CanBookBeReserved more safe Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84673 [details] [review] Bug 21738: check items count in C4:ILSDI::HoldTitle Test plan: - Try holding a title without items via ILS-DI (cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1), - you get an error, - apply this patch, - try again, - you should get the code "Notitems"Bug 21738: check items count in C4:ILSDI::HoldTitle Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84674 [details] [review] Bug 21738: make call of CanBookBeReserved more safe Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Tests for changes to HoldTitle?
(In reply to Nick Clemens from comment #14) > Tests for changes to HoldTitle? You are right!
Created attachment 84853 [details] [review] Bug 21738: unit tests for HoldTitle
Set it back to Passed QA. An other QAer for checking unit tests ?
Awesome work all! Pushed to master for 19.05
Bug in enhancement not in 18.11.x series.