Summary: | Error in the OPAC when viewing a record which has no biblio-level itemtype | ||
---|---|---|---|
Product: | Koha | Reporter: | Owen Leonard <oleonard> |
Component: | OPAC | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | critical | ||
Priority: | P5 - low | CC: | jonathan.druart, julian.maurice, m.de.rooy, martin.renvoize, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Bug Depends on: | 17530 | ||
Bug Blocks: | |||
Attachments: |
Bug 21475: Fix crash on missing default itemtype
Bug 21475: Fix crash on missing default itemtype Bug 21475: Fix crash on missing default itemtype Bug 21475: Fix crash on missing default itemtype |
Description
Owen Leonard
2018-10-02 15:59:06 UTC
Caused by: commit e85d6e12ea4dccd8fa08ec600864c2f985dcf886 Bug 17530: (QA follow-up) Move may_article_request to ItemType 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' (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. (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 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> 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> 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> 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 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) (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. Not important but I told the same few weeks ago, so we are at least 2 to think it's weird :) (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 :) 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> Awesome work all! Pushed to master for 18.11 Dependancy/Cause not in 18.05.x |