Bug 19431 - Error when trying to checkout an unknown barcode
Summary: Error when trying to checkout an unknown barcode
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 18276
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-08 09:30 UTC by Katrin Fischer
Modified: 2017-11-20 13:21 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19431: Fix error when checking out an unknown barcode (1.83 KB, patch)
2017-10-09 22:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19431: Fix error when checking out an unknown barcode (1.87 KB, patch)
2017-10-10 04:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19431: Fix error when checking out an unknown barcode (1.93 KB, patch)
2017-10-11 00:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19431: Fix error when checking out an unknown barcode (1.83 KB, patch)
2017-10-11 15:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19431: Fix error when checking out an unknown barcode (1.88 KB, patch)
2017-10-13 12:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 19431: Fix error when checking out an unknown barcode (1.97 KB, patch)
2017-10-24 12:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2017-10-08 09:30:33 UTC
When trying to checkout an unknown barcode, I receive an internal server error:

Can't call method "materials" on an undefined value at /home/vagrant/kohaclone/circ/circulation.pl line 387.

It should ask me to do a fast add instead.
Comment 1 Josef Moravec 2017-10-09 18:50:59 UTC
(In reply to Katrin Fischer from comment #0)
> When trying to checkout an unknown barcode, I receive an internal server
> error:
> 
> Can't call method "materials" on an undefined value at
> /home/vagrant/kohaclone/circ/circulation.pl line 387.
> 
> It should ask me to do a fast add instead.

For me, it asks for fast cataloguing the item - I can't reproduce it on my devbox...
Comment 2 Jonathan Druart 2017-10-09 22:25:10 UTC Comment hidden (obsolete)
Comment 3 Josef Moravec 2017-10-10 04:29:20 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2017-10-11 00:02:35 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2017-10-11 00:04:07 UTC
Works in my testing.

I would suggest adding parens to the second conditional:
if ( (a and b) or (c) )
just for clarity, but not necessary if you don't want to
Comment 6 Jonathan Druart 2017-10-11 13:04:28 UTC
I will take a look again later but I think the condition is wrong, I guess it should be 
    if( $item and ( !$blocker or $force_allow_issue ) ){
Comment 7 Jonathan Druart 2017-10-11 15:39:54 UTC
Created attachment 67966 [details] [review]
Bug 19431: Fix error when checking out an unknown barcode

It seems this has been caused by
  commit 1544f9a5d4a8acd47c97d7c6ac55dee8e759d3ff
    Bug 18276: Remove GetBiblioFromItemNumber - circulation pages

To reproduce the problem you need switch on the two prefs
 - OnSiteCheckouts
 - OnSiteCheckoutsForce

Test plan:
Try to check an item out using an unknown barcode
With the 2 prefs set to on and without this patch, you will get the
following error in the log
  Can't call method "materials" on an undefined value at /home/vagrant/kohaclone/circ/circulation.pl line 387.

With this patch applied you should not get this error and a correct
behaviour for the different pref combinations.
Comment 8 Jonathan Druart 2017-10-11 15:40:25 UTC
Sorry about that, the condition was wrong!
Comment 9 Owen Leonard 2017-10-13 12:05:12 UTC
Created attachment 68069 [details] [review]
Bug 19431: Fix error when checking out an unknown barcode

It seems this has been caused by
  commit 1544f9a5d4a8acd47c97d7c6ac55dee8e759d3ff
    Bug 18276: Remove GetBiblioFromItemNumber - circulation pages

To reproduce the problem you need switch on the two prefs
 - OnSiteCheckouts
 - OnSiteCheckoutsForce

Test plan:
Try to check an item out using an unknown barcode
With the 2 prefs set to on and without this patch, you will get the
following error in the log

  Can't call method "materials" on an undefined value at
  /home/vagrant/kohaclone/circ/circulation.pl line 387.

With this patch applied you should not get this error and a correct
behaviour for the different pref combinations.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Marcel de Rooy 2017-10-20 08:27:38 UTC
QA: Looking here now
Comment 11 Marcel de Rooy 2017-10-20 09:13:08 UTC
-    unless( $onsite_checkout and C4::Context->preference("OnSiteCheckoutsForce") ) {
+    if ( $error->{UNKNOWN_BARCODE} or not $onsite_checkout or not C4::Context->preference("OnSiteCheckoutsForce") ) {

If CanBookBeIssued sets the UNKNOWN_BARCODE flag, it returns no DEBT question and no other impossible errors. But since there is one error, it will set $blocker to 1. 
Just wondering if there are other errors that should trigger the blocker flag when doing onsite checkouts here. If CanBookBeIssued said No, when is an onsite checkout still possible?

-    if( !$blocker || $force_allow_issue ){
+
+    if( $item and ( !$blocker or $force_allow_issue ) ){

Since you raised the blocker flag in the unknown barcode case, you do not need to test for $item here. Note that item is found by barcode and CanBookBeIssued calls GetItem with barcode parameter.

Please clarify/adjust.
Comment 12 Jonathan Druart 2017-10-20 17:08:28 UTC
(In reply to Marcel de Rooy from comment #11)
> -    unless( $onsite_checkout and
> C4::Context->preference("OnSiteCheckoutsForce") ) {
> +    if ( $error->{UNKNOWN_BARCODE} or not $onsite_checkout or not
> C4::Context->preference("OnSiteCheckoutsForce") ) {
> 
> If CanBookBeIssued sets the UNKNOWN_BARCODE flag, it returns no DEBT
> question and no other impossible errors. But since there is one error, it
> will set $blocker to 1. 
> Just wondering if there are other errors that should trigger the blocker
> flag when doing onsite checkouts here. If CanBookBeIssued said No, when is
> an onsite checkout still possible?

I put the test there to avoid to copy paste the blocker=1 and the IMPOSSIBLE flag. Do you have something better to suggest?

> -    if( !$blocker || $force_allow_issue ){
> +
> +    if( $item and ( !$blocker or $force_allow_issue ) ){
> 
> Since you raised the blocker flag in the unknown barcode case, you do not
> need to test for $item here. Note that item is found by barcode and
> CanBookBeIssued calls GetItem with barcode parameter.

Yes definitely, but this code is very confusing and I think explicitly tell we need $item here makes sense (even if useless in a algorithm pov, in my opinion it helps the read).
Comment 13 Marcel de Rooy 2017-10-23 06:50:52 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Marcel de Rooy from comment #11)
> > -    unless( $onsite_checkout and
> > C4::Context->preference("OnSiteCheckoutsForce") ) {
> > +    if ( $error->{UNKNOWN_BARCODE} or not $onsite_checkout or not
> > C4::Context->preference("OnSiteCheckoutsForce") ) {
> > 
> > If CanBookBeIssued sets the UNKNOWN_BARCODE flag, it returns no DEBT
> > question and no other impossible errors. But since there is one error, it
> > will set $blocker to 1. 
> > Just wondering if there are other errors that should trigger the blocker
> > flag when doing onsite checkouts here. If CanBookBeIssued said No, when is
> > an onsite checkout still possible?
> 
> I put the test there to avoid to copy paste the blocker=1 and the IMPOSSIBLE
> flag. Do you have something better to suggest?

My question was: Is UNKNOWN_BARCODE the only one needed here? Should the blocker flag be set (among other things) for other types of errors ?
Comment 14 Jonathan Druart 2017-10-23 14:30:55 UTC
UNKNOWN_BARCODE is the one we need to add to fix this bug.
Comment 15 Marcel de Rooy 2017-10-24 12:58:33 UTC
(In reply to Jonathan Druart from comment #14)
> UNKNOWN_BARCODE is the one we need to add to fix this bug.

Hmm I never thought of that ;)
I am not blocking this patch but I raised a valid question and got no answer.
Comment 16 Marcel de Rooy 2017-10-24 12:58:51 UTC
Created attachment 68462 [details] [review]
Bug 19431: Fix error when checking out an unknown barcode

It seems this has been caused by
  commit 1544f9a5d4a8acd47c97d7c6ac55dee8e759d3ff
    Bug 18276: Remove GetBiblioFromItemNumber - circulation pages

To reproduce the problem you need switch on the two prefs
 - OnSiteCheckouts
 - OnSiteCheckoutsForce

Test plan:
Try to check an item out using an unknown barcode
With the 2 prefs set to on and without this patch, you will get the
following error in the log

  Can't call method "materials" on an undefined value at
  /home/vagrant/kohaclone/circ/circulation.pl line 387.

With this patch applied you should not get this error and a correct
behaviour for the different pref combinations.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Jonathan Druart 2017-10-24 14:44:29 UTC
(In reply to Marcel de Rooy from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > UNKNOWN_BARCODE is the one we need to add to fix this bug.
> 
> Hmm I never thought of that ;)
> I am not blocking this patch but I raised a valid question and got no answer.

To be honest I have no idea what we need to do more. The subroutine and the script are very ugly and need a full refurnish to be readable and less error-prone.
Comment 18 Jonathan Druart 2017-10-25 15:19:40 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 19 Fridolin SOMERS 2017-11-20 13:21:31 UTC
Does not appear in 17.05.x