Bug 14015 - Checkout: Fix software error if barcode '0' is given
Summary: Checkout: Fix software error if barcode '0' is given
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
Depends on: 11759
Blocks: 16499
  Show dependency treegraph
 
Reported: 2015-04-19 20:29 UTC by Marc Véron
Modified: 2016-12-05 21:25 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14015 - Checkout: Fix software error if barcode '0' is given (2.27 KB, patch)
2015-12-14 14:57 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14015 - Checkout: Fix software error if barcode '0' is given (2.33 KB, patch)
2015-12-14 15:00 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14015 - Checkout: Fix software error if barcode '0' is given (2.45 KB, patch)
2015-12-20 22:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14015 - Checkout: Fix software error if barcode '0' is given (1.42 KB, patch)
2015-12-28 11:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14015 - Checkout: Fix software error if barcode '0' is given (1.52 KB, patch)
2015-12-28 11:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14015 - Checkout: Fix software error if barcode '0' is given (1.55 KB, patch)
2015-12-28 12:07 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-04-19 20:29:21 UTC
To reproduce:

- Go to Home > Circulation > Checkouts > [a patron]
- Enter 0 (zero) in barcode field and hit Enter

Result: circ/circulation.pl reloads without patron selected (Home > Circulation > Checkouts). You have to search the patron again to proceed.
Comment 1 Marc Véron 2015-12-08 19:23:45 UTC
New behaviour on current master:

- Go to Home > Circulation > Checkouts > [a patron]
- Enter 0 (zero) in barcode field and hit Enter

Result:
Can't use string ("1") as a HASH ref while "strict refs" in use at /usr/share/kohaclone/circ/circulation.pl line 473
Comment 2 Marc Véron 2015-12-14 14:57:44 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2015-12-14 15:00:43 UTC Comment hidden (obsolete)
Comment 4 Chris Kirby 2015-12-15 17:41:54 UTC
Hi Marc,
I have tried testing this bug on Sandbox 6 (with patch applied) and Sandbox 7 (without patch applied), and I am not seeing the software error described.
Comment 5 Marc Véron 2015-12-16 01:21:31 UTC
Hi Chris,thanks for testing.

What happens if you enter a 0 and then hit "return"?
Comment 6 Aleisha Amohia 2015-12-20 22:39:43 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-12-21 11:42:55 UTC
Comment on attachment 45873 [details] [review]
Bug 14015 - Checkout: Fix software error if barcode '0' is given

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

::: circ/circulation.pl
@@ +217,5 @@
>  }
>  
>  # check and see if we should print
> +# but do not set to 'yes' if barcode given by user is '0'
> +if ( ( @$barcodes == 0 )  && ( ! @$barcodes eq '0')  && ( $print eq 'maybe' )  ) {

What is supposed to test !@$barcodes eq '0' ??
Comment 8 Marc Véron 2015-12-21 13:24:36 UTC
(In reply to Jonathan Druart from comment #7)
> Comment on attachment 45873 [details] [review] [review]
> Bug 14015 - Checkout: Fix software error if barcode '0' is given
> 
> Review of attachment 45873 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: circ/circulation.pl
> @@ +217,5 @@
> >  }
> >  
> >  # check and see if we should print
> > +# but do not set to 'yes' if barcode given by user is '0'
> > +if ( ( @$barcodes == 0 )  && ( ! @$barcodes eq '0')  && ( $print eq 'maybe' )  ) {
> 
> What is supposed to test !@$barcodes eq '0' ??

Hi Jonathan,

The first (already existing) part @$barcodes == 0 tests @$barcodes for numerical equality (no barcodes given). While testing I found out that it evaluates to true for a barcode that is '0'. Because it evaluated to true with a barcode '0', $print was set to 'yes' (but we do not want to print, since we have a barcode). With $print set to 'yes' the borrowernumber is set to '' in line 235, and that leads to the software error in line 473.
By adding a test for the barcode not to be equal to '0' by the string equality oparator eq ( ! @$barcodes eq '0'), the value of $print is not set to 'yes' and the barcode '0' is treated as such. Koha will then display the message 'The barcode was not found 0' (as expected)
Comment 9 Jonathan Druart 2015-12-23 09:36:09 UTC
(In reply to Marc Véron from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > Comment on attachment 45873 [details] [review] [review] [review]
> > Bug 14015 - Checkout: Fix software error if barcode '0' is given
> > 
> > Review of attachment 45873 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: circ/circulation.pl
> > @@ +217,5 @@
> > >  }
> > >  
> > >  # check and see if we should print
> > > +# but do not set to 'yes' if barcode given by user is '0'
> > > +if ( ( @$barcodes == 0 )  && ( ! @$barcodes eq '0')  && ( $print eq 'maybe' )  ) {
> > 
> > What is supposed to test !@$barcodes eq '0' ??
> 
> Hi Jonathan,
> 
> The first (already existing) part @$barcodes == 0 tests @$barcodes for
> numerical equality (no barcodes given). While testing I found out that it
> evaluates to true for a barcode that is '0'. Because it evaluated to true
> with a barcode '0', $print was set to 'yes' (but we do not want to print,
> since we have a barcode). With $print set to 'yes' the borrowernumber is set
> to '' in line 235, and that leads to the software error in line 473.
> By adding a test for the barcode not to be equal to '0' by the string
> equality oparator eq ( ! @$barcodes eq '0'), the value of $print is not set
> to 'yes' and the barcode '0' is treated as such. Koha will then display the
> message 'The barcode was not found 0' (as expected)

my $a = ['0'];
say "1.is 0" if @$a eq '0';
$a = [0];
say "2.is 0" if @$a eq '0';

Does not print anything.

I have tried your changes and remove the last 2 tests and everything looks ok.
Comment 10 Marcel de Rooy 2015-12-24 09:26:53 UTC
Please clarify which steps are needed to replicate the problem in current master. I do not see this error (anymore?)..
Comment 11 Marc Véron 2015-12-24 12:20:27 UTC
Hi Marcel,

I re-tested and reproduced with 3.22.00.000 on our demo installation and with current master 3.23.00.000 on my development vm.

Test 1:
- Go to "My Checkouts"
- Enter 0 (zero) in barcode field and hit Enter

Expected result: Message that barcode 0 does not exist

Result: 
Software error.
Can't use string ("1") as a HASH ref while "strict refs" in use at /usr/share/kohaclone/circ/circulation.pl line 470
(Like in comment #2)

Test 2:
- Search a patron and got to the checkout page
- Enter 0 (zero) in barcode filed and hit Enter

Expected result: Message that barcode 0 does not exist

Result: Checkout page loads (circ/circulation.pl). You have to search the patron again to proceed.
(Like in comment #1)

For some patrons I get the result 1 (patrons with staff permissions), for others I get the result 2 ('normal' patrons).

The reason for both is that the borrowernumber is set to '' in circ/circulation.pl line 235 (because of $print eq 'yes' evaluates to true which is wrong, $print should not be 'yes' here).
Comment 12 Marcel de Rooy 2015-12-28 08:42:12 UTC
> Go to Home > Circulation > Checkouts > [a patron]

This was quite confusing actually.
It is: Go to Home > Patrons > [enter patron] > Check out

Back to SO
Comment 13 Marcel de Rooy 2015-12-28 08:59:51 UTC
if ( ( @$barcodes == 0 )  && ( ! @$barcodes eq '0')  && ( $print eq 'maybe' ) )

I agree with Jonathan. This code needs to be adjusted.
If $barcodes == [ '0'], the second test cannot be true since @$barcodes will be 1.

Failed QA
Comment 14 Marc Véron 2015-12-28 09:21:44 UTC
Hi Marcel, a question: Can you reproduce / confirm the problem as described in comment #11?
Comment 15 Jonathan Druart 2015-12-28 09:53:17 UTC
Marc,
Try with only this change:

diff --git a/circ/circulation.pl b/circ/circulation.pl
index f16475e..fa62dcc 100755
--- a/circ/circulation.pl
+++ b/circ/circulation.pl
@@ -88,7 +88,9 @@ if (!C4::Context->userenv && !$branch){
 }
 
 my $barcodes = [];
-if ( my $barcode = $query->param('barcode') ) {
+my $barcode =  $query->param('barcode');
+# Barcode given by user could be '0'
+if ( $barcode || $barcode eq '0' ) {
     $barcodes = [ $barcode ];
 } else {
     my $filefh = $query->upload('uploadfile');
@@ -215,12 +217,13 @@ if( $onsite_checkout && !$duedatespec_allow ) {
 }

It will work.
Comment 16 Marc Véron 2015-12-28 11:19:28 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2015-12-28 11:31:49 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2015-12-28 12:07:54 UTC
Created attachment 46001 [details] [review]
Bug 14015 - Checkout: Fix software error if barcode '0' is given

To reproduce the bug:
- Go to Home > Circulation > Checkouts > [a patron]
- Enter 0 (zero) in barcode field and hit Enter

Result:
Software error
  Can't use string ("1") as a HASH ref while "strict refs" in use
  at /usr/share/kohaclone/circ/circulation.pl line 473

To test:
- Apply patch
- Repeat steps above
- Verify that the sofware error is gone and that you get a
  message as with other invalid barcodes.
- Test with other values: '00', '000', existing barcode,
  and a barcode that does not exist
- Test with empty barcode (for printing slip)
- Search for regressions

This patch implements Jonathan's solution (see comment #15)

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Kyle M Hall 2015-12-30 14:33:35 UTC
Not sure if I understand the problem correctly, but it seems that this patch doesn't work.

I created an item with barcode 0.

Without patch:
Trying to check out gives blank screen
Trying to check acts as if no barcode passed in

With patch:
Trying to check out gives "The barcode was not found 0"
Trying to check acts as if no barcode passed in

Is the intention of this patch to allow the use of '0' as an item barcode?

Thanks!
Comment 20 Marc Véron 2015-12-30 15:05:34 UTC
Hi Kyle,

You do not need an item with barcode '0' to reproduce the bug, and it is not the aim of this bug to make it possible to use a barcode '0'

The aim of this bug is to get rid of the software error if you (per accident) enter 0 in the barcode field of the checkout page.

I think the best description to reproduce is in comment #11

There are two different behaviours without patch:

1)
If you try it for a patron with staff permissions, an error shows up:
Can't use string ("1") as a HASH ref while "strict refs" in use at /usr/share/kohaclone/circ/circulation.pl line 470

2)
If you try it for a user without staff permissions, the empty page appears (as in your test)

With the patch applied, you should get the following message (for both cases):
"The barcode was not found 0 Fast cataloging"
Comment 21 Marc Véron 2016-01-05 09:59:16 UTC
I can still reprduce the error on current master.
Patch still applies.
Comment 22 Marcel de Rooy 2016-01-05 12:20:14 UTC
(In reply to Marc Véron from comment #21)
> I can still reprduce the error on current master.
> Patch still applies.

It seems that Kyle missed the purpose of this report if I read comment19.
If you reset the status, it will receive attention again from the RM.
Comment 23 Marc Véron 2016-01-05 13:00:34 UTC
Reset status as of comment #22
Comment 24 Kyle M Hall 2016-01-07 15:29:37 UTC
Thanks for the explanation Marc! Indeed I did misunderstand the intention of the patch.
Comment 25 Brendan Gallagher 2016-01-27 02:21:49 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks
Comment 26 Julian Maurice 2016-01-29 11:13:39 UTC
Patch pushed to 3.22.x, will be in 3.22.3