Description
Alex Arnaud
2018-10-31 13:21:58 UTC
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. |