Bug 21738 - [ILS-DI] Error placing a hold on a title without item
Summary: [ILS-DI] Error placing a hold on a title without item
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Keywords:
Depends on: 15524
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-31 13:21 UTC by Alex Arnaud
Modified: 2020-01-06 20:14 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21738: Move counting of items in C4::Reserves::CanBookBeReserved (3.19 KB, patch)
2018-10-31 15:46 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 21738: check items count in C4:ILSDI::HoldTitle (1.31 KB, patch)
2018-11-13 08:45 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 21738: make call of CanBookBeReserved more safe (2.39 KB, patch)
2019-01-04 15:34 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 21738: check items count in C4:ILSDI::HoldTitle (1.38 KB, patch)
2019-01-09 15:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21738: make call of CanBookBeReserved more safe (2.47 KB, patch)
2019-01-09 15:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21738: check items count in C4:ILSDI::HoldTitle (1.48 KB, patch)
2019-02-02 21:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21738: make call of CanBookBeReserved more safe (2.57 KB, patch)
2019-02-02 21:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21738: unit tests for HoldTitle (1.27 KB, patch)
2019-02-07 16:22 UTC, Alex Arnaud
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2018-10-31 13:21:58 UTC
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
Comment 1 Alex Arnaud 2018-10-31 15:46:32 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2018-11-01 16:02:24 UTC
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
Comment 3 Alex Arnaud 2018-11-13 08:45:46 UTC
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
Comment 4 Alex Arnaud 2018-11-13 08:49:27 UTC
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
Comment 5 Jonathan Druart 2018-11-13 15:35:49 UTC
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
Comment 6 Jonathan Druart 2018-11-13 16:25:37 UTC
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?
Comment 7 Kyle M Hall 2018-11-13 17:52:32 UTC
(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!
Comment 8 Marcel de Rooy 2018-11-26 14:17:06 UTC
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?
Comment 9 Alex Arnaud 2019-01-04 15:34:19 UTC
Created attachment 83637 [details] [review]
Bug 21738: make call of CanBookBeReserved more safe
Comment 10 Kyle M Hall 2019-01-09 15:15:36 UTC
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>
Comment 11 Kyle M Hall 2019-01-09 15:15:45 UTC
Created attachment 83753 [details] [review]
Bug 21738: make call of CanBookBeReserved more safe

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Katrin Fischer 2019-02-02 21:45:22 UTC
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>
Comment 13 Katrin Fischer 2019-02-02 21:45:26 UTC
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>
Comment 14 Nick Clemens 2019-02-04 13:48:58 UTC
Tests for changes to HoldTitle?
Comment 15 Katrin Fischer 2019-02-04 16:25:03 UTC
(In reply to Nick Clemens from comment #14)
> Tests for changes to HoldTitle?

You are right!
Comment 16 Alex Arnaud 2019-02-07 16:22:55 UTC
Created attachment 84853 [details] [review]
Bug 21738: unit tests for HoldTitle
Comment 17 Alex Arnaud 2019-02-07 16:24:17 UTC
Set it back to Passed QA. An other QAer for checking unit tests ?
Comment 18 Nick Clemens 2019-02-11 18:38:17 UTC
Awesome work all!

Pushed to master for 19.05
Comment 19 Martin Renvoize 2019-02-12 11:52:40 UTC
Bug in enhancement not in 18.11.x series.