Description
Marc Véron
2015-04-19 20:29:21 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 Created attachment 45660 [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, barcode that does not exist - Test with empty barcode (for printing slip) - Search for regressions Note: I tried to create an item with barcode '0' to test that case as well, but it was not possible. Giving the possibility to create an item with barcode '0' is not in the scope of this bug. Created attachment 45661 [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 Note: I tried to create an item with barcode '0' to test that case as well, but it was not possible. Giving the possibility to create an item with barcode '0' is not in the scope of this bug. (Amended for better formatting of commit message) 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. Hi Chris,thanks for testing. What happens if you enter a 0 and then hit "return"? Created attachment 45873 [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 Note: I tried to create an item with barcode '0' to test that case as well, but it was not possible. Giving the possibility to create an item with barcode '0' is not in the scope of this bug. (Amended for better formatting of commit message) I was able to reproduce the warn and patch works correctly. Signed-off-by: Aleisha <aleishaamohia@hotmail.com> 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' ?? (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) (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. Please clarify which steps are needed to replicate the problem in current master. I do not see this error (anymore?).. 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). > 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
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 Hi Marcel, a question: Can you reproduce / confirm the problem as described in comment #11? 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. Created attachment 45999 [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) Created attachment 46000 [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> 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> 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! 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" I can still reprduce the error on current master. Patch still applies. (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. Reset status as of comment #22 Thanks for the explanation Marc! Indeed I did misunderstand the intention of the patch. Pushed to Master - Should be in the May 2016 release. Thanks Patch pushed to 3.22.x, will be in 3.22.3 |