Bug 13934 - Check in fails on master "Can't bless non-reference at .../ItemType.pm Line 64"
Summary: Check in fails on master "Can't bless non-reference at .../ItemType.pm Line 64"
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 11944
Blocks: 13885
  Show dependency treegraph
 
Reported: 2015-03-31 18:08 UTC by Nick Clemens
Modified: 2015-12-03 22:02 UTC (History)
6 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 13934: C4::ItemType->get should return undef if no parameter given (2.35 KB, patch)
2015-04-01 08:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13934: C4::ItemType->get should return undef if no parameter given (2.86 KB, patch)
2015-04-02 04:54 UTC, M. Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 13934: C4::ItemType->get should return undef if no parameter given (2.93 KB, patch)
2015-04-03 12:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13934: C4::ItemType->get should return undef if no parameter given (2.81 KB, patch)
2015-04-07 08:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13934: C4::ItemType->get should return undef if no parameter given (2.89 KB, patch)
2015-04-08 06:54 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13934: C4::ItemType->get should return undef if no parameter given (3.03 KB, patch)
2015-04-09 17:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[PASSED QA] Bug 13934: C4::ItemType->get should return undef if no parameter given (3.08 KB, patch)
2015-04-09 22:03 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2015-03-31 18:08:27 UTC
Check in some barcodes on master and you get the perl error above

tcohen says git bisect points to 7431f8cfe29e330e2232b0df591afc4d923b0a52
Comment 1 Jonathan Druart 2015-04-01 08:23:24 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2015-04-02 04:49:08 UTC
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?
Comment 3 M. Tompsett 2015-04-02 04:54:11 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2015-04-03 12:26:57 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-04-07 08:08:16 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-04-07 08:09:12 UTC
(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;
Comment 7 Josef Moravec 2015-04-08 06:54:17 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2015-04-09 17:53:18 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2015-04-09 22:03:38 UTC
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>
Comment 10 Tomás Cohen Arazi 2015-04-12 03:54:55 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 11 Chris Cormack 2015-04-20 10:10:32 UTC
Pushed to 3.18.x will be in 3.18.6