Bug 19431

Summary: Error when trying to checkout an unknown barcode
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, josef.moravec, kyle.m.hall, m.de.rooy, nick
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18276    
Bug Blocks:    
Attachments: Bug 19431: Fix error when checking out an unknown barcode
Bug 19431: Fix error when checking out an unknown barcode
Bug 19431: Fix error when checking out an unknown barcode
Bug 19431: Fix error when checking out an unknown barcode
Bug 19431: Fix error when checking out an unknown barcode
Bug 19431: Fix error when checking out an unknown barcode

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