@@ -, +, @@ --- C4/Biblio.pm | 49 ++++++++++++++++++----------- C4/Items.pm | 68 +++++++++++++++++++++++++++------------- Koha/Items.pm | 66 +++++++++++++++++++++++++++++++++++++++ t/db_dependent/Items.t | 85 +++++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 223 insertions(+), 45 deletions(-) --- a/C4/Biblio.pm +++ a/C4/Biblio.pm @@ -2810,9 +2810,10 @@ sub EmbedItemsInMarcBiblio { carp 'EmbedItemsInMarcBiblio: No MARC record passed'; return; } - $itemnumbers = [] unless defined $itemnumbers; + # Could this be moved lower down and run only when items + # exists for improved performance? Probably.. my $frameworkcode = GetFrameworkCode($biblionumber); _strip_item_fields($marc, $frameworkcode); @@ -2820,26 +2821,36 @@ sub EmbedItemsInMarcBiblio { my $dbh = C4::Context->dbh; my $sth = $dbh->prepare("SELECT itemnumber FROM items WHERE biblionumber = ?"); $sth->execute($biblionumber); - my @item_fields; - my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode ); - my @items; + my @all_itemnumbers; + while ( my ($itemnumber) = $sth->fetchrow_array ) { + push @all_itemnumbers, $itemnumber; + } + # Remove invalid item numbers by intersecting with all valid item numbers + if(@$itemnumbers) { + my %h = map { $_ => undef } @{$itemnumbers}; + $itemnumbers = [grep { exists $h{$_} } @all_itemnumbers]; + } + else { + $itemnumbers = \@all_itemnumbers; + } + # If no item numbers then no point in continuing + return unless (@{$itemnumbers}); + my $opachiddenitems = $opac - && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ ); + && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ ); require C4::Items; - while ( my ($itemnumber) = $sth->fetchrow_array ) { - next if @$itemnumbers and not grep { $_ == $itemnumber } @$itemnumbers; - my $i = $opachiddenitems ? C4::Items::GetItem($itemnumber) : undef; - push @items, { itemnumber => $itemnumber, item => $i }; - } - my @hiddenitems = - $opachiddenitems - ? C4::Items::GetHiddenItemnumbers( map { $_->{item} } @items ) - : (); - # Convert to a hash for quick searching - my %hiddenitems = map { $_ => 1 } @hiddenitems; - foreach my $itemnumber ( map { $_->{itemnumber} } @items ) { - next if $hiddenitems{$itemnumber}; - my $item_marc = C4::Items::GetMarcItem( $biblionumber, $itemnumber ); + + my $items = C4::Items::GetItems($itemnumbers); + if ($opachiddenitems) { + my %hidden_items = map { $_ => undef } C4::Items::GetHiddenItemnumbers(@{$items}); + # Reduce items to non hidden items + $items = [grep { !(exists $hidden_items{$_->{itemnumber}}) } @{$items}]; + } + + my ($itemtag) = GetMarcFromKohaField("items.itemnumber", $frameworkcode); + my @item_fields; + foreach my $item (@{$items}) { + my $item_marc = C4::Items::GetMarcItem($biblionumber, $item); push @item_fields, $item_marc->field($itemtag); } $marc->append_fields(@item_fields); --- a/C4/Items.pm +++ a/C4/Items.pm @@ -144,34 +144,60 @@ of C Return item information, for a given itemnumber or barcode. The return value is a hashref mapping item column -names to values. If C<$serial> is true, include serial publication data. +names to values, or undef if item was not found. If C<$serial> is true, include serial publication data. =cut sub GetItem { - my ($itemnumber,$barcode, $serial) = @_; - my $dbh = C4::Context->dbh; + my ($itemnumber, $barcode, $serial) = @_; + my $items = GetItems($itemnumber ? [$itemnumber] : undef, $barcode ? [$barcode] : undef, $serial); + return @{$items} ? $items->[0] : undef; +} - my $item; - if ($itemnumber) { - $item = Koha::Items->find( $itemnumber ); - } else { - $item = Koha::Items->find( { barcode => $barcode } ); - } +=head2 GetItems - return unless ( $item ); + $item = GetItems($itemnumber,$barcode,$serial); - my $data = $item->unblessed(); - $data->{itype} = $item->effective_itemtype(); # set the correct itype +Return list of item information, for given itemnumbers or barcodes. +The return value is an arrayref with hashrefs mapping item column +names to values, or an empty arrayref if no items were found. +If C<$serial> is true, include serial publication data. +=cut + +sub GetItems { + my ($itemnumbers, $barcodes, $serial) = @_; + my $dbh = C4::Context->dbh; + my $items_data = []; + + # Important: If 'item-level_itypes' preference is not set Koha::Item::effective_itemtype() + # will be equal to $item->{itype} and it is possible to fetch the data directly from items table. + # This will have a much smaller footprint then unblessing the item objects fetched through + # find/search. If more calculated attributes are added in the future, this code will need + # to account for that. + if (C4::Context->preference('item-level_itypes')) { + $items_data = Koha::Items->search_unblessed($itemnumbers ? $itemnumbers : { barcode => $barcodes }); + } + else { + my @items = Koha::Items->search( + $itemnumbers ? { itemnumber => { IN => $itemnumbers } } : { barcode => { IN => $barcodes } } + ); + foreach my $item (@items) { + my $data = $item->unblessed(); + # Set the correct itype + $data->{itype} = $item->effective_itemtype(); + push @{$items_data}, $data; + } + } if ($serial) { - my $ssth = $dbh->prepare("SELECT serialseq,publisheddate from serialitems left join serial on serialitems.serialid=serial.serialid where serialitems.itemnumber=?"); - $ssth->execute( $data->{'itemnumber'} ); - ( $data->{'serialseq'}, $data->{'publisheddate'} ) = $ssth->fetchrow_array(); + foreach my $data (@{$items_data}) { + my $ssth = $dbh->prepare("SELECT serialseq,publisheddate from serialitems left join serial on serialitems.serialid=serial.serialid where serialitems.itemnumber=?"); + $ssth->execute( $data->{'itemnumber'} ); + ( $data->{'serialseq'}, $data->{'publisheddate'} ) = $ssth->fetchrow_array(); + } } - - return $data; -} # sub GetItem + return $items_data; +} =head2 CartToShelf @@ -1481,13 +1507,13 @@ sub GetMarcItem { # while the other treats the MARC representation as authoritative # under certain circumstances. - my $itemrecord = GetItem($itemnumber); + my $itemrecord = ref($itemnumber) ? $itemnumber : GetItem($itemnumber); # Tack on 'items.' prefix to column names so that C4::Biblio::TransformKohaToMarc will work. # Also, don't emit a subfield if the underlying field is blank. - - return Item2Marc($itemrecord,$biblionumber); + + return Item2Marc($itemrecord, $biblionumber); } sub Item2Marc { --- a/Koha/Items.pm +++ a/Koha/Items.pm @@ -53,6 +53,72 @@ sub object_class { return 'Koha::Item'; } +=head2 search_unblessed + + $items = Koha::Items->search_unblessed($conditions); + +In scalar context, returns an array reference with hashes containing item data, in list context returns an array. +Calling search_unblessed should produce the same result as calling Koha::Items->search and iterating +though the result unblessing each item. In cases where you only need the item data using this optimized +method is much faster. The arguments accepted are a subset and approximation of those supported by +Koha::items->search, with only the "barcode" and "itemnumber" attributes supported as conditions. +If called with a non-hash conditions argument a search will be performed as if "itemnumber" was the +provided condition. Only one condition is accepted and if more are provided the method will die with +an error. + +=cut + +sub search_unblessed { + my ($self, $conditions) = @_; + my $items = []; + my $field_name; + if (ref($conditions) eq 'HASH') { + my %valid_condition_fields = ( + 'barcode' => undef, + 'itemnumber' => undef, + ); + my @fields = keys %{$conditions}; + if (@fields == 1) { + foreach my $field (@fields) { + die("Invalid condition: \"$field\"") unless exists $valid_condition_fields{$field}; + } + } + elsif(@fields > 1) { + die("Multiple conditions are not supported"); + } + else { + die("No conditions provided"); + } + } + else { + $conditions = { + 'itemnumber' => $conditions, + }; + } + # Only accepts one condition + my ($field, $values) = (%{$conditions}); + + if (ref($values) eq 'ARRAY' && @{$values} == 1) { + ($values) = @{$values}; + } + if (!ref($values)) { + my $item = C4::Context->dbh->selectrow_hashref(qq{SELECT * FROM items WHERE $field = ?}, undef, $values); + push @{$items}, $item; + } + elsif (ref($values) eq 'ARRAY') { + my $in = join ', ', (('?') x @{$values}); + my $sth = C4::Context->dbh->prepare(qq{SELECT * FROM items WHERE $field IN($in)}); + $sth->execute(@{$values}); + while (my $item = $sth->fetchrow_hashref) { + push @{$items}, $item; + } + } + else { + die("Invalid value for field: \"$field\""); + } + return wantarray ? @{$items} : $items; +} + =head1 AUTHOR Kyle M Hall --- a/t/db_dependent/Items.t +++ a/t/db_dependent/Items.t @@ -44,7 +44,9 @@ my $location = 'My Location'; subtest 'General Add, Get and Del tests' => sub { - plan tests => 16; + plan tests => 25; + + my $item_barcode = 123; $schema->storage->txn_begin; @@ -61,28 +63,96 @@ subtest 'General Add, Get and Del tests' => sub { my ($bibnum, $bibitemnum) = get_biblio(); # Add an item. - my ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $library->{branchcode}, holdingbranch => $library->{branchcode}, location => $location, itype => $itemtype->{itemtype} } , $bibnum); + my ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ + homebranch => $library->{branchcode}, + holdingbranch => $library->{branchcode}, + location => $location, + itype => $itemtype->{itemtype}, + barcode => $item_barcode + }, $bibnum + ); cmp_ok($item_bibnum, '==', $bibnum, "New item is linked to correct biblionumber."); cmp_ok($item_bibitemnum, '==', $bibitemnum, "New item is linked to correct biblioitemnumber."); # Get item. my $getitem = GetItem($itemnumber); cmp_ok($getitem->{'itemnumber'}, '==', $itemnumber, "Retrieved item has correct itemnumber."); + cmp_ok($getitem->{'barcode'}, '==', $item_barcode, "Retrieved item has correct barcode."); cmp_ok($getitem->{'biblioitemnumber'}, '==', $item_bibitemnum, "Retrieved item has correct biblioitemnumber."); is( $getitem->{location}, $location, "The location should not have been modified" ); is( $getitem->{permanent_location}, $location, "The permanent_location should have been set to the location value" ); + $getitem = GetItem(undef, $item_barcode); + cmp_ok($getitem->{'itemnumber'}, '==', $itemnumber, "Item retrieved by barcode has correct itemnumber."); + # Modify item; setting barcode. - ModItem({ barcode => '987654321' }, $bibnum, $itemnumber); + $item_barcode = '987654321'; + ModItem({ barcode => $item_barcode }, $bibnum, $itemnumber); my $moditem = GetItem($itemnumber); - cmp_ok($moditem->{'barcode'}, '==', '987654321', 'Modified item barcode successfully to: '.$moditem->{'barcode'} . '.'); + cmp_ok($moditem->{'barcode'}, '==', $item_barcode, 'Modified item barcode successfully to: ' . $moditem->{'barcode'} . '.'); + + # Update some more properties to get larger coverage + my $item_properties = { + 'dateaccessioned' => '2012-12-12', + 'datelastborrowed' => '2013-12-12', + 'datelastseen' => '2013-12-12', + 'notforloan' => '0', + 'damaged' => '0', + 'itemlost' => '0', + 'itemcallnumber' => '1234', + 'restricted' => '0', + }; + + # Clone item propreties since ModItem modifies this argument + # causing later tests to fail + ModItem({%{$item_properties}}, $bibnum, $itemnumber); + + # Search unblessed tests + my $search_conditions = { + 'itemnumber' => $itemnumber, + }; + my ($item_object) = Koha::Items->search($search_conditions); + my $item_object_data = $item_object->unblessed; + cmp_ok($item_object_data->{'itemnumber'}, '==', $itemnumber, "Item object retrieved by Koha::Items->search using \"itemnumber\" condition has correct itemnumber."); + + # Intersect with updated properties + my $updated_item_properties = { map { ($_ => $item_object_data->{$_}) } keys %{$item_properties} }; + + is_deeply($updated_item_properties, $item_properties, "Updated item properties have correct values."); + + my ($item_data) = Koha::Items->search_unblessed($search_conditions); + cmp_ok( + $item_data->{'itemnumber'}, + '==', + $itemnumber, + "Item data retrieved by Koha::Items->search_unblessed using \"itemnumber\" condition has correct itemnumber." + ); + + ($item_data) = Koha::Items->search_unblessed({ 'barcode' => $item_barcode }); + cmp_ok( + $item_data->{'itemnumber'}, + '==', + $itemnumber, + "Item data retrieved by Koha::Items->search_unblessed using \"barcode\" condition has correct itemnumber." + ); + + is_deeply($item_object_data, $item_data, "Item data retrieved by unblessing item object is identical to item data from Koha::Items->search_unblessed."); # Delete item. DelItem({ biblionumber => $bibnum, itemnumber => $itemnumber }); my $getdeleted = GetItem($itemnumber); is($getdeleted->{'itemnumber'}, undef, "Item deleted as expected."); - ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $library->{branchcode}, holdingbranch => $library->{branchcode}, location => $location, permanent_location => 'my permanent location', itype => $itemtype->{itemtype} } , $bibnum); + ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ + homebranch => $library->{branchcode}, + holdingbranch => $library->{branchcode}, + location => $location, + permanent_location => 'my permanent location', + itype => $itemtype->{itemtype}, + barcode => $item_barcode + }, + $bibnum + ); $getitem = GetItem($itemnumber); is( $getitem->{location}, $location, "The location should not have been modified" ); is( $getitem->{permanent_location}, 'my permanent location', "The permanent_location should not have modified" ); @@ -100,6 +170,11 @@ subtest 'General Add, Get and Del tests' => sub { t::lib::Mocks::mock_preference('item-level_itypes', '1'); $getitem = GetItem($itemnumber); is( $getitem->{itype}, $itemtype->{itemtype}, "Itemtype set correctly when using item-level_itypes" ); + # Good to test this since different code for retrieving items is run in GetItems() depending on item-level_itypes preference + is( $getitem->{itemnumber}, $itemnumber, "Item successfully retrieved by itemnumber when using item-level_itypes" ); + $getitem = GetItem(undef, $item_barcode); + is( $getitem->{itemnumber}, $itemnumber, "Item successfully retrieved by barcode when using item-level_itypes" ); + t::lib::Mocks::mock_preference('item-level_itypes', '0'); $getitem = GetItem($itemnumber); is( $getitem->{itype}, undef, "Itemtype set correctly when not using item-level_itypes" ); --