Bug 21475 - Error in the OPAC when viewing a record which has no biblio-level itemtype
Summary: Error in the OPAC when viewing a record which has no biblio-level itemtype
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 17530
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-02 15:59 UTC by Owen Leonard
Modified: 2019-10-14 19:55 UTC (History)
5 users (show)

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


Attachments
Bug 21475: Fix crash on missing default itemtype (1.23 KB, patch)
2018-10-15 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21475: Fix crash on missing default itemtype (1.33 KB, patch)
2018-10-15 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21475: Fix crash on missing default itemtype (1.34 KB, patch)
2018-10-15 13:07 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21475: Fix crash on missing default itemtype (1.40 KB, patch)
2018-10-18 13:52 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2018-10-02 15:59:06 UTC
When I view in the OPAC a record which has no biblio-level itemtype I get an error:

Can't call method "may_article_request" on an undefined value at /home/vagrant/kohaclone/opac/opac-detail.pl line 761

My item-level_itypes system preference is set to "specific item."
Comment 1 Jonathan Druart 2018-10-02 16:02:00 UTC
Caused by:
  commit e85d6e12ea4dccd8fa08ec600864c2f985dcf886
  Bug 17530: (QA follow-up) Move may_article_request to ItemType
Comment 2 Jonathan Druart 2018-10-02 18:24:44 UTC
Also, from C4::Circulation::transferbook:

 324         my $code = C4::Context->preference("BranchTransferLimitsType") eq 'ccode' ? $item->ccode : $item->biblio->biblioitem->itemtype; # BranchTransferLimitsType is 'ccode' or 'itemtype'
Comment 3 Jonathan Druart 2018-10-02 18:35:11 UTC
(In reply to Jonathan Druart from comment #2)
> Also, from C4::Circulation::transferbook:
> 
>  324         my $code = C4::Context->preference("BranchTransferLimitsType")
> eq 'ccode' ? $item->ccode : $item->biblio->biblioitem->itemtype; #
> BranchTransferLimitsType is 'ccode' or 'itemtype'

Scratch that, it should not explode.
Comment 4 Marcel de Rooy 2018-10-15 12:00:35 UTC
(In reply to Jonathan Druart from comment #1)
> Caused by:
>   commit e85d6e12ea4dccd8fa08ec600864c2f985dcf886
>   Bug 17530: (QA follow-up) Move may_article_request to ItemType

O no
Comment 5 Marcel de Rooy 2018-10-15 12:23:06 UTC
Created attachment 80591 [details] [review]
Bug 21475: Fix crash on missing default itemtype

Test plan:
Enable ArticleRequests.
Find book without itemtype (942c). Maybe you need this:
  update biblioitems set itemtype=NULL where biblionumber=[...]
Goto opac detail for that book. No crash anymore?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2018-10-15 12:57:50 UTC
Created attachment 80593 [details] [review]
Bug 21475: Fix crash on missing default itemtype

Test plan:
Enable ArticleRequests.
Find book without itemtype (942c). Maybe you need this:
  update biblioitems set itemtype=NULL where biblionumber=[...]
Goto opac detail for that book. No crash anymore?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Owen Leonard 2018-10-15 13:07:10 UTC
Created attachment 80595 [details] [review]
Bug 21475: Fix crash on missing default itemtype

Test plan:
Enable ArticleRequests.
Find book without itemtype (942c). Maybe you need this:
  update biblioitems set itemtype=NULL where biblionumber=[...]
Goto opac detail for that book. No crash anymore?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Jonathan Druart 2018-10-16 16:16:13 UTC
Bug 21150 added a check on the different item types configuration, but actually biblioitems.itemtype must be set to a valid itemtypes whatever is the value of the pref. This bug highlights the problem quite well, and we certainly have it for holds as well.
I am in favor of workarounding it for now if incorrectly set, but what about adding a new check to the misc/maintenance/search_for_data_inconsistencies.pl script?
Comment 9 Julian Maurice 2018-10-18 13:23:39 UTC
Comment on attachment 80595 [details] [review]
Bug 21475: Fix crash on missing default itemtype

Review of attachment 80595 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-detail.pl
@@ +762,5 @@
>      my $artreqpossible = $patron
>          ? $biblio->can_article_request( $patron )
> +        : $itemtype
> +        ? $itemtype->may_article_request
> +        : q{};

I think it's odd to return an empty string for what seems to be a boolean value. I can see that can_article_request and may_article_request do the same thing so it's consistent.
But why not return simply 0 (as 1 is used for the true value) ? And why q{} instead of just '' ? Is there some kind of consensus about this that I'm not aware of ?
(Note that this will not block QA, I'm just curious)
Comment 10 Marcel de Rooy 2018-10-18 13:27:26 UTC
(In reply to Julian Maurice from comment #9)

> I think it's odd to return an empty string for what seems to be a boolean
> value. I can see that can_article_request and may_article_request do the
> same thing so it's consistent.
> But why not return simply 0 (as 1 is used for the true value) ? And why q{}
> instead of just '' ? Is there some kind of consensus about this that I'm not
> aware of ?
> (Note that this will not block QA, I'm just curious)

Empty string is in perl the default for false (not 0). q{} seems to be preferred above ''. The Koha codebase contains all variations. Nothing important.
Comment 11 Jonathan Druart 2018-10-18 13:34:40 UTC
Not important but I told the same few weeks ago, so we are at least 2 to think it's weird :)
Comment 12 Julian Maurice 2018-10-18 13:39:34 UTC
(In reply to Marcel de Rooy from comment #10)
> Empty string is in perl the default for false (not 0). q{} seems to be
> preferred above ''. The Koha codebase contains all variations. Nothing
> important.

Perl is weird. But it makes more sense now, thanks.

(In reply to Jonathan Druart from comment #11)
> Not important but I told the same few weeks ago, so we are at least 2 to
> think it's weird :)

I'm glad to know I'm not the only one :)
Comment 13 Julian Maurice 2018-10-18 13:52:35 UTC
Created attachment 80843 [details] [review]
Bug 21475: Fix crash on missing default itemtype

Test plan:
Enable ArticleRequests.
Find book without itemtype (942c). Maybe you need this:
  update biblioitems set itemtype=NULL where biblionumber=[...]
Goto opac detail for that book. No crash anymore?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 14 Nick Clemens 2018-10-19 17:32:40 UTC
Awesome work all!

Pushed to master for 18.11
Comment 15 Martin Renvoize 2018-11-05 12:41:13 UTC
Dependancy/Cause not in 18.05.x