Check in some barcodes on master and you get the perl error above tcohen says git bisect points to 7431f8cfe29e330e2232b0df591afc4d923b0a52
Created attachment 37412 [details] [review] Bug 13934: C4::ItemType->get should return undef if no parameter given The issue: If you try to check in an item with a non existent barcode, the application will exploded with a software error: "Can't bless non-reference at .../ItemType.pm Line 64". It's caused by: commit 7431f8cfe29e330e2232b0df591afc4d923b0a52 Bug 11944: Fix encoding issue in C4::ItemType and the following change: @@ -105,9 +104,6 @@ sub get { my $data = $dbh->selectrow_hashref( "SELECT * FROM itemtypes WHERE itemtype = ?", undef, $itemtype ); - if ( $data->{description} ) { - $data->{description} = Encode::encode('UTF-8', $data->{description}); - } because of the following: my $s; $s->{foo} = "bar" if $s->{foo}; use Data::Dumper;warn Dumper $s; => {} # not undef So later, bless $opts => $class; will fail because $opts is undef and was not (i.e. {}) before. Test plan: Try to check in an item with a non existent barcode and confirm you have the friendly warning message instead of the ugly software error. Note for QA: C4::ItemType->get is only uses in circ/return.pl. So even if the behavior is changed, it should not introduce any regression somewhere else.
Comment on attachment 37412 [details] [review] Bug 13934: C4::ItemType->get should return undef if no parameter given Review of attachment 37412 [details] [review]: ----------------------------------------------------------------- ::: C4/ItemType.pm @@ +99,5 @@ > > sub get { > my ($class, $itemtype) = @_; > + > + return unless $itemtype; Hypothetical question: Could not someone do something crazy like have an itemtype code of 0?
Created attachment 37427 [details] [review] Bug 13934: C4::ItemType->get should return undef if no parameter given The issue: If you try to check in an item with a non existent barcode, the application will exploded with a software error: "Can't bless non-reference at .../ItemType.pm Line 64". It's caused by: commit 7431f8cfe29e330e2232b0df591afc4d923b0a52 Bug 11944: Fix encoding issue in C4::ItemType and the following change: @@ -105,9 +104,6 @@ sub get { my $data = $dbh->selectrow_hashref( "SELECT * FROM itemtypes WHERE itemtype = ?", undef, $itemtype ); - if ( $data->{description} ) { - $data->{description} = Encode::encode('UTF-8', $data->{description}); - } because of the following: my $s; $s->{foo} = "bar" if $s->{foo}; use Data::Dumper;warn Dumper $s; => {} # not undef So later, bless $opts => $class; will fail because $opts is undef and was not (i.e. {}) before. More explicit test plan: 1) Log in to staff client 2) Circulation -> Check in 3) Type a non-existent barcode into 'Enter item barcode:' textbox 4) Click 'Submit' -- Should receive nasty error. 5) apply patch 6) repeat steps 2-4 -- Should be told 'No item with barcode: {what you typed}' 7) prove -v t/ItemType.t -- All tests should run successfully. 7) run koha qa test tools Note: Having tried to create and use an itemtype '0', this only demonstrates a lack of validation on the itemtype creation screen. Unable to use it without tweaking back end. That is beyond the scope of this bug. Note for QA: C4::ItemType->get is only uses in circ/return.pl. So even if the behavior is changed, it should not introduce any regression somewhere else. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 37452 [details] [review] [PASSED QA] Bug 13934: C4::ItemType->get should return undef if no parameter given The issue: If you try to check in an item with a non existent barcode, the application will exploded with a software error: "Can't bless non-reference at .../ItemType.pm Line 64". It's caused by: commit 7431f8cfe29e330e2232b0df591afc4d923b0a52 Bug 11944: Fix encoding issue in C4::ItemType and the following change: @@ -105,9 +104,6 @@ sub get { my $data = $dbh->selectrow_hashref( "SELECT * FROM itemtypes WHERE itemtype = ?", undef, $itemtype ); - if ( $data->{description} ) { - $data->{description} = Encode::encode('UTF-8', $data->{description}); - } because of the following: my $s; $s->{foo} = "bar" if $s->{foo}; use Data::Dumper;warn Dumper $s; => {} # not undef So later, bless $opts => $class; will fail because $opts is undef and was not (i.e. {}) before. More explicit test plan: 1) Log in to staff client 2) Circulation -> Check in 3) Type a non-existent barcode into 'Enter item barcode:' textbox 4) Click 'Submit' -- Should receive nasty error. 5) apply patch 6) repeat steps 2-4 -- Should be told 'No item with barcode: {what you typed}' 7) prove -v t/ItemType.t -- All tests should run successfully. 7) run koha qa test tools Note: Having tried to create and use an itemtype '0', this only demonstrates a lack of validation on the itemtype creation screen. Unable to use it without tweaking back end. That is beyond the scope of this bug. Note for QA: C4::ItemType->get is only uses in circ/return.pl. So even if the behavior is changed, it should not introduce any regression somewhere else. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 37536 [details] [review] Bug 13934: C4::ItemType->get should return undef if no parameter given The issue: If you try to check in an item with a non existent barcode, the application will exploded with a software error: "Can't bless non-reference at .../ItemType.pm Line 64". It's caused by: commit 7431f8cfe29e330e2232b0df591afc4d923b0a52 Bug 11944: Fix encoding issue in C4::ItemType and the following change: @@ -105,9 +104,6 @@ sub get { my $data = $dbh->selectrow_hashref( "SELECT * FROM itemtypes WHERE itemtype = ?", undef, $itemtype ); - if ( $data->{description} ) { - $data->{description} = Encode::encode('UTF-8', $data->{description}); - } because of the following: my $s; $s->{foo} = "bar" if $s->{foo}; use Data::Dumper;warn Dumper $s; => {} # not undef So later, bless $opts => $class; will fail because $opts is undef and was not (i.e. {}) before. More explicit test plan: 1) Log in to staff client 2) Circulation -> Check in 3) Type a non-existent barcode into 'Enter item barcode:' textbox 4) Click 'Submit' -- Should receive nasty error. 5) apply patch 6) repeat steps 2-4 -- Should be told 'No item with barcode: {what you typed}' 7) prove -v t/ItemType.t -- All tests should run successfully. 7) run koha qa test tools Note: Having tried to create and use an itemtype '0', this only demonstrates a lack of validation on the itemtype creation screen. Unable to use it without tweaking back end. That is beyond the scope of this bug. Note for QA: C4::ItemType->get is only uses in circ/return.pl. So even if the behavior is changed, it should not introduce any regression somewhere else.
(In reply to M. Tompsett from comment #2) > Hypothetical question: Could not someone do something crazy like have an > itemtype code of 0? I think this is consistent. The last patch replaced the line return unless $itemtype; with return unless defined $itemtype;
Created attachment 37558 [details] [review] [SIGNED-OFF] Bug 13934: C4::ItemType->get should return undef if no parameter given The issue: If you try to check in an item with a non existent barcode, the application will exploded with a software error: "Can't bless non-reference at .../ItemType.pm Line 64". It's caused by: commit 7431f8cfe29e330e2232b0df591afc4d923b0a52 Bug 11944: Fix encoding issue in C4::ItemType and the following change: @@ -105,9 +104,6 @@ sub get { my $data = $dbh->selectrow_hashref( "SELECT * FROM itemtypes WHERE itemtype = ?", undef, $itemtype ); - if ( $data->{description} ) { - $data->{description} = Encode::encode('UTF-8', $data->{description}); - } because of the following: my $s; $s->{foo} = "bar" if $s->{foo}; use Data::Dumper;warn Dumper $s; => {} # not undef So later, bless $opts => $class; will fail because $opts is undef and was not (i.e. {}) before. More explicit test plan: 1) Log in to staff client 2) Circulation -> Check in 3) Type a non-existent barcode into 'Enter item barcode:' textbox 4) Click 'Submit' -- Should receive nasty error. 5) apply patch 6) repeat steps 2-4 -- Should be told 'No item with barcode: {what you typed}' 7) prove -v t/ItemType.t -- All tests should run successfully. 7) run koha qa test tools Note: Having tried to create and use an itemtype '0', this only demonstrates a lack of validation on the itemtype creation screen. Unable to use it without tweaking back end. That is beyond the scope of this bug. Note for QA: C4::ItemType->get is only uses in circ/return.pl. So even if the behavior is changed, it should not introduce any regression somewhere else. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 37630 [details] [review] [SIGNED OFF] Bug 13934: C4::ItemType->get should return undef if no parameter given The issue: If you try to check in an item with a non existent barcode, the application will exploded with a software error: "Can't bless non-reference at .../ItemType.pm Line 64". It's caused by: commit 7431f8cfe29e330e2232b0df591afc4d923b0a52 Bug 11944: Fix encoding issue in C4::ItemType and the following change: @@ -105,9 +104,6 @@ sub get { my $data = $dbh->selectrow_hashref( "SELECT * FROM itemtypes WHERE itemtype = ?", undef, $itemtype ); - if ( $data->{description} ) { - $data->{description} = Encode::encode('UTF-8', $data->{description}); - } because of the following: my $s; $s->{foo} = "bar" if $s->{foo}; use Data::Dumper;warn Dumper $s; => {} # not undef So later, bless $opts => $class; will fail because $opts is undef and was not (i.e. {}) before. More explicit test plan: 1) Log in to staff client 2) Circulation -> Check in 3) Type a non-existent barcode into 'Enter item barcode:' textbox 4) Click 'Submit' -- Should receive nasty error. 5) apply patch 6) repeat steps 2-4 -- Should be told 'No item with barcode: {what you typed}' 7) prove -v t/ItemType.t -- All tests should run successfully. 7) run koha qa test tools Note: Having tried to create and use an itemtype '0', this only demonstrates a lack of validation on the itemtype creation screen. Unable to use it without tweaking back end. That is beyond the scope of this bug. Note for QA: C4::ItemType->get is only uses in circ/return.pl. So even if the behavior is changed, it should not introduce any regression somewhere else. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Works as expected. Fixes the problem and no regressions found. It even has regression tests :-D
Created attachment 37631 [details] [review] [PASSED QA] Bug 13934: C4::ItemType->get should return undef if no parameter given The issue: If you try to check in an item with a non existent barcode, the application will exploded with a software error: "Can't bless non-reference at .../ItemType.pm Line 64". It's caused by: commit 7431f8cfe29e330e2232b0df591afc4d923b0a52 Bug 11944: Fix encoding issue in C4::ItemType and the following change: @@ -105,9 +104,6 @@ sub get { my $data = $dbh->selectrow_hashref( "SELECT * FROM itemtypes WHERE itemtype = ?", undef, $itemtype ); - if ( $data->{description} ) { - $data->{description} = Encode::encode('UTF-8', $data->{description}); - } because of the following: my $s; $s->{foo} = "bar" if $s->{foo}; use Data::Dumper;warn Dumper $s; => {} # not undef So later, bless $opts => $class; will fail because $opts is undef and was not (i.e. {}) before. More explicit test plan: 1) Log in to staff client 2) Circulation -> Check in 3) Type a non-existent barcode into 'Enter item barcode:' textbox 4) Click 'Submit' -- Should receive nasty error. 5) apply patch 6) repeat steps 2-4 -- Should be told 'No item with barcode: {what you typed}' 7) prove -v t/ItemType.t -- All tests should run successfully. 7) run koha qa test tools Note: Having tried to create and use an itemtype '0', this only demonstrates a lack of validation on the itemtype creation screen. Unable to use it without tweaking back end. That is beyond the scope of this bug. Note for QA: C4::ItemType->get is only uses in circ/return.pl. So even if the behavior is changed, it should not introduce any regression somewhere else. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Works as expected. Fixes the problem and no regressions found. It even has regression tests :-D Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Patch pushed to master. Thanks Jonathan!
Pushed to 3.18.x will be in 3.18.6