Bug 24331

Summary: Internal server error when placing hold on item if itemtype undefined
Product: Koha Reporter: Nick Clemens <nick>
Component: Hold requestsAssignee: Nick Clemens <nick>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: gmcharlt, jonathan.druart+koha, m.de.rooy, martin.renvoize, mkstephens, phil
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21842
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25449
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 24331: Don't die on items with no type
Bug 24331: Don't die on items with no type

Description Nick Clemens 2020-01-02 16:05:28 UTC
Error is:
Can't call method "notforloan" on an undefined value at /usr/share/koha/lib/C4/Reserves.pm line 1199.


C4::Reserves::IsAvailableForItemLevelRequest:

1198     my $itemtype = $item->effective_itemtype;
1199     my $notforloan_per_itemtype = Koha::ItemTypes->find($itemtype)->notforloan;

We should check that we found an itemtype before calling notforloan
Comment 1 Nick Clemens 2020-01-03 13:30:49 UTC
Created attachment 96793 [details] [review]
Bug 24331: Don't die on items with no type

To test:
1 - Find or create an item with no itemtype set
2 - Attempt to place a hold
3 - INternal server error
4 - Apply patch
5 - Repeat
6 - No error
7 - Set the items itemtype to an itemtype marked not for loan
8 - Set circ rules to allow holds on this itemtype
9 - Attempt to place hold, item is not holdable
10 - Remove not for loan from itemtype
11 - Item is holdable
Comment 2 Myka Kennedy Stephens 2020-01-18 23:53:10 UTC
Created attachment 97584 [details] [review]
Bug 24331: Don't die on items with no type

To test:
1 - Find or create an item with no itemtype set
2 - Attempt to place a hold
3 - INternal server error
4 - Apply patch
5 - Repeat
6 - No error
7 - Set the items itemtype to an itemtype marked not for loan
8 - Set circ rules to allow holds on this itemtype
9 - Attempt to place hold, item is not holdable
10 - Remove not for loan from itemtype
11 - Item is holdable

Signed-off-by: Myka Kennedy Stephens <mkstephens@lancasterseminary.edu>
Comment 3 Myka Kennedy Stephens 2020-01-18 23:53:45 UTC
Patch works as described in test plan. Thank you!
Comment 4 Jonathan Druart 2020-01-21 15:24:31 UTC
Nick, did not we (more or less) agree that we should not test if items had valid item type?
See bug 21842.
Comment 5 Marcel de Rooy 2020-01-24 06:52:37 UTC
(In reply to Jonathan Druart from comment #4)
> Nick, did not we (more or less) agree that we should not test if items had
> valid item type?
> See bug 21842.

Although I still intuitively feel that such tests are appropriate, I also had the impression that we agreed not to add such tests everywhere.
If this still needs discussion, please change status accordingly. It would be wise to have a specific coding guideline for this, Jonathan.
Comment 6 Katrin Fischer 2020-05-10 10:20:11 UTC
I've added an item about this to the next dev meeting agenda:
https://wiki.koha-community.org/wiki/Development_IRC_meeting_20_May_2020#Agenda

Please feel free to add any arguments to it! Goal would be to move towards a general guideline here.
Comment 7 Jonathan Druart 2020-05-20 15:10:54 UTC
https://meetings.koha-community.org/2020/development_irc_meeting_20_may_2020.2020-05-20-14.00.log.html

Search for "bug 24331".

We agreed on making it mandatory, see bug 25449.

*** This bug has been marked as a duplicate of bug 25449 ***