Bugzilla – Attachment 82870 Details for
Bug 19884
Improve performance of GetItem
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 19884 - Improve performance of GetItem
Bug-19884---Improve-performance-of-GetItem.patch (text/plain), 28.56 KB, created by
David Gustafsson
on 2018-12-05 14:38:06 UTC
(
hide
)
Description:
Bug 19884 - Improve performance of GetItem
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2018-12-05 14:38:06 UTC
Size:
28.56 KB
patch
obsolete
>From b738ce18e6a59aefadf5f8648bc430c51eaf5553 Mon Sep 17 00:00:00 2001 >From: David Gustafsson <david.gustafsson@ub.gu.se> >Date: Fri, 15 Dec 2017 11:36:38 +0100 >Subject: [PATCH] Bug 19884 - Improve performance of GetItem > >Provide optimized search_unblessed method for Koha::Items to increase >performance of GetItem. Also extend GetMarcItem() to also accept an item >record as second parameter. This way an extra call to GetItem() can be >avoided by passing an item record directly instead of an item number. > >Also add new function GetItems() making it possible to load multiple >items at once, which is more efficient than loading them one by one. > >How to test: >1) Run tests in t/db_dependent/Items.t > >Sponsored-by: Gothenburg University Library > >Add new functions GetMarcBiblios and EmbedItemsInMarcBiblios for embedding items in multiple records at once for improved performance > >https://bugs.koha-community.org/show_bug.cgi?id=19884 >--- > C4/Biblio.pm | 224 +++++++++++++++++++--------- > C4/Items.pm | 68 ++++++--- > Koha/BiblioUtils.pm | 80 +++++++--- > Koha/Items.pm | 66 ++++++++ > misc/search_tools/rebuild_elastic_search.pl | 2 +- > t/db_dependent/Items.t | 84 ++++++++++- > 6 files changed, 404 insertions(+), 120 deletions(-) > >diff --git a/C4/Biblio.pm b/C4/Biblio.pm >index 808ed8d067..91033fe433 100644 >--- a/C4/Biblio.pm >+++ b/C4/Biblio.pm >@@ -1108,6 +1108,26 @@ sub GetMarcSubfieldStructureFromKohaField { > return wantarray ? @{$mss->{$kohafield}} : $mss->{$kohafield}->[0]; > } > >+ >+sub GetMarcBiblio { >+ my ($params) = @_; >+ >+ if (not defined $params) { >+ carp 'GetMarcBiblio called without parameters'; >+ return; >+ } >+ my $biblionumber = $params->{biblionumber}; >+ >+ if (not defined $biblionumber) { >+ carp 'GetMarcBiblio called with undefined biblionumber'; >+ return; >+ } >+ delete $params->{biblionumber}; >+ $params->{biblionumbers} = [$biblionumber]; >+ my ($biblio) = GetMarcBiblios($params); >+ return $biblio; >+} >+ > =head2 GetMarcBiblio > > my $record = GetMarcBiblio({ >@@ -1148,57 +1168,68 @@ It only makes sense in combination both embed_items and opac values true. > > =cut > >-sub GetMarcBiblio { >+sub GetMarcBiblios { > my ($params) = @_; > > if (not defined $params) { >- carp 'GetMarcBiblio called without parameters'; >+ carp 'GetMarcBiblios called without parameters'; > return; > } > >- my $biblionumber = $params->{biblionumber}; >+ my $biblionumbers = $params->{biblionumbers}; > my $embeditems = $params->{embed_items} || 0; > my $opac = $params->{opac} || 0; > my $borcat = $params->{borcat} // q{}; > >- if (not defined $biblionumber) { >- carp 'GetMarcBiblio called with undefined biblionumber'; >+ ## START HERE ## >+ >+ if (not defined $biblionumbers) { >+ carp 'GetMarcBiblio called with undefined biblionumbers'; > return; > } > >- my $dbh = C4::Context->dbh; >- my $sth = $dbh->prepare("SELECT biblioitemnumber FROM biblioitems WHERE biblionumber=? "); >- $sth->execute($biblionumber); >- my $row = $sth->fetchrow_hashref; >- my $biblioitemnumber = $row->{'biblioitemnumber'}; >- my $marcxml = GetXmlBiblio( $biblionumber ); >- $marcxml = StripNonXmlChars( $marcxml ); >- my $frameworkcode = GetFrameworkCode($biblionumber); >- MARC::File::XML->default_record_format( C4::Context->preference('marcflavour') ); >- my $record = MARC::Record->new(); >+ my $in_placeholders = join ', ', (('?') x @{$biblionumbers}); >+ my $dbh = C4::Context->dbh; >+ my $sth = $dbh->prepare("SELECT biblionumber, biblioitemnumber FROM biblioitems WHERE biblionumber IN ($in_placeholders)"); >+ $sth->execute(@{$biblionumbers}); >+ >+ my %marc_records; >+ my ($biblionumber, $biblioitemnumber); >+ my $marc_flavour = C4::Context->preference('marcflavour'); >+ while (my $biblio_keys = $sth->fetchrow_arrayref) { >+ ($biblionumber, $biblioitemnumber) = @{$biblio_keys}; >+ >+ my $marcxml = GetXmlBiblio( $biblionumber ); >+ $marcxml = StripNonXmlChars( $marcxml ); >+ my $frameworkcode = GetFrameworkCode($biblionumber); >+ MARC::File::XML->default_record_format( $marc_flavour ); >+ my $record = MARC::Record->new(); >+ >+ if ($marcxml) { >+ $record = eval { >+ MARC::Record::new_from_xml( $marcxml, "utf8", $marc_flavour); >+ }; >+ if ($@) { warn " problem with :$biblionumber : $@ \n$marcxml"; } >+ next unless $record; >+ >+ C4::Biblio::_koha_marc_update_bib_ids( >+ $record, >+ $frameworkcode, >+ $biblionumber, >+ $biblioitemnumber >+ ); >+ $marc_records{$biblionumber} = $record; >+ } >+ } > >- if ($marcxml) { >- $record = eval { >- MARC::Record::new_from_xml( $marcxml, "utf8", >- C4::Context->preference('marcflavour') ); >- }; >- if ($@) { warn " problem with :$biblionumber : $@ \n$marcxml"; } >- return unless $record; >- >- C4::Biblio::_koha_marc_update_bib_ids( $record, $frameworkcode, $biblionumber, >- $biblioitemnumber ); >- C4::Biblio::EmbedItemsInMarcBiblio({ >- marc_record => $record, >- biblionumber => $biblionumber, >+ if ($embeditems) { >+ C4::Biblio::EmbedItemsInMarcBiblios({ >+ marc_records => \%marc_records, > opac => $opac, >- borcat => $borcat }) >- if ($embeditems); >- >- return $record; >- } >- else { >- return; >+ borcat => $borcat >+ }); > } >+ return wantarray ? values %marc_records : \%marc_records; > } > > =head2 GetXmlBiblio >@@ -2790,57 +2821,104 @@ override if necessary. > > sub EmbedItemsInMarcBiblio { > my ($params) = @_; >- my ($marc, $biblionumber, $itemnumbers, $opac, $borcat); >- $marc = $params->{marc_record}; >- if ( !$marc ) { >+ >+ if ( !$params->{marc_record} ) { > carp 'EmbedItemsInMarcBiblio: No MARC record passed'; > return; > } >- $biblionumber = $params->{biblionumber}; >+ if ( !$params->{biblionumber} ) { >+ carp 'EmbedItemsInMarcBiblio: No biblionumber passed'; >+ return; >+ } >+ >+ $params->{marc_records} = { $params->{biblionumber} => $params->{marc_record} }; >+ delete $params->{marc_record}; >+ delete $params->{biblionumber}; >+ >+ my ($marc_with_items) = EmbedItemsInMarcBiblios($params); >+ return $marc_with_items; >+} >+ >+sub EmbedItemsInMarcBiblios { >+ my ($params) = @_; >+ my ($marc_records, $biblionumber, $itemnumbers, $opac, $borcat); >+ $marc_records = $params->{marc_records}; >+ > $itemnumbers = $params->{item_numbers}; > $opac = $params->{opac}; > $borcat = $params->{borcat} // q{}; > > $itemnumbers = [] unless defined $itemnumbers; > >- my $frameworkcode = GetFrameworkCode($biblionumber); >- _strip_item_fields($marc, $frameworkcode); >- > # ... and embed the current items > my $dbh = C4::Context->dbh; >- my $sth = $dbh->prepare("SELECT itemnumber FROM items WHERE biblionumber = ?"); >- $sth->execute($biblionumber); >- my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode ); > >- my @item_fields; # Array holding the actual MARC data for items to be included. >- my @items; # Array holding items which are both in the list (sitenumbers) >- # and on this biblionumber >+ my @biblionumbers = keys %{$marc_records}; >+ my $in_placeholders = join ', ', (('?') x @biblionumbers); > >- # Flag indicating if there is potential hiding. >- my $opachiddenitems = $opac >- && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ ); >+ my $biblio_itemnumbers = $dbh->selectcol_arrayref( >+ "SELECT itemnumber FROM items WHERE biblionumber IN ($in_placeholders)", >+ undef, >+ @biblionumbers >+ ); >+ >+ # Remove invalid item numbers by intersecting with all valid item numbers >+ # if $itemnumbers argument was provided > >+ # TODO: MAKE SURE THERE IS A TEST FOR THIS >+ if(@{$itemnumbers}) { >+ my %h = map { $_ => undef } @{$itemnumbers}; >+ $itemnumbers = [grep { exists $h{$_} } @{$biblio_itemnumbers}]; >+ } >+ else { >+ $itemnumbers = $biblio_itemnumbers; >+ } >+ # If no item numbers then no point in continuing >+ return unless (@{$itemnumbers}); >+ >+ >+ my $opachiddenitems = $opac >+ && ( 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 @items2pass = map { $_->{item} } @items; >- my @hiddenitems = >- $opachiddenitems >- ? C4::Items::GetHiddenItemnumbers({ >- items => \@items2pass, >- borcat => $borcat }) >- : (); >- # 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 ); >- push @item_fields, $item_marc->field($itemtag); >- } >- $marc->append_fields(@item_fields); >+ >+ my $items = C4::Items::GetItems($itemnumbers); >+ # We have now fetched all items, time to partion by biblionumber >+ my %items_by_biblionumber = (map { ($_, []) } @biblionumbers); >+ foreach my $item (@{$items}) { >+ push @{$items_by_biblionumber{$item->{biblionumber}}}, $item; >+ } >+ >+ foreach my $biblionumber (@biblionumbers) { >+ my $marc_record = $marc_records->{$biblionumber}; >+ >+ # Array holding items which are both in itemnumbers >+ # (if provided) and in biblionumbers itemnumbers >+ my $items = $items_by_biblionumber{$biblionumber}; >+ >+ # Could this be moved lower down and run only when items >+ # exists for improved performance? Probably.. >+ my $frameworkcode = GetFrameworkCode($biblionumber); >+ _strip_item_fields($marc_record, $frameworkcode); >+ >+ # Flag indicating if there is potential hiding >+ if ($opachiddenitems) { >+ my %hidden_items = map { $_ => undef } C4::Items::GetHiddenItemnumbers({ >+ items => @{$items}, >+ borcat => $borcat >+ }); >+ # Reduce items to non hidden items >+ $items = [grep { !(exists $hidden_items{$_->{itemnumber}}) } @{$items}]; >+ } >+ >+ my ($itemtag) = GetMarcFromKohaField("items.itemnumber", $frameworkcode); >+ my @item_fields; # Array holding the actual MARC data for items to be included. >+ foreach my $item (@{$items}) { >+ my $item_marc = C4::Items::GetMarcItem($biblionumber, $item); >+ push @item_fields, $item_marc->field($itemtag); >+ } >+ $marc_record->append_fields(@item_fields); >+ } >+ return $marc_records; > } > > =head1 INTERNAL FUNCTIONS >@@ -3292,7 +3370,7 @@ sub ModBiblioMarc { > > # deal with UNIMARC field 100 (encoding) : create it if needed & set encoding to unicode > if ( $encoding eq "UNIMARC" ) { >- my $defaultlanguage = C4::Context->preference("UNIMARCField100Language"); >+ my $defaultlanguage = C4::Context->preference("UNIMARCField100Language"); > $defaultlanguage = "fre" if (!$defaultlanguage || length($defaultlanguage) != 3); > my $string = $record->subfield( 100, "a" ); > if ( ($string) && ( length( $record->subfield( 100, "a" ) ) == 36 ) ) { >@@ -3302,7 +3380,7 @@ sub ModBiblioMarc { > $string = POSIX::strftime( "%Y%m%d", localtime ); > $string =~ s/\-//g; > $string = sprintf( "%-*s", 35, $string ); >- substr ( $string, 22, 3, $defaultlanguage); >+ substr ( $string, 22, 3, $defaultlanguage); > } > substr( $string, 25, 3, "y50" ); > unless ( $record->subfield( 100, "a" ) ) { >diff --git a/C4/Items.pm b/C4/Items.pm >index 3b006c686e..d94dd61c3d 100644 >--- a/C4/Items.pm >+++ b/C4/Items.pm >@@ -129,34 +129,60 @@ of C<C4::Items> > > 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 > >@@ -1331,13 +1357,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 { >diff --git a/Koha/BiblioUtils.pm b/Koha/BiblioUtils.pm >index 73fc12aa4b..44cd563aed 100644 >--- a/Koha/BiblioUtils.pm >+++ b/Koha/BiblioUtils.pm >@@ -111,29 +111,69 @@ The iterator is a Koha::MetadataIterator object. > =cut > > sub get_all_biblios_iterator { >+ my ($self, $params) = @_; >+ my $batched; >+ my $batch_size; >+ if ($params) { >+ $batched //= $params->{batched}; >+ $batch_size = $params->{batch_size} || 2000; >+ } >+ > my $database = Koha::Database->new(); > my $schema = $database->schema(); >- my $rs = >- $schema->resultset('Biblio')->search( {}, >- { columns => [qw/ biblionumber /] } ); >- my $next_func = sub { >- # Warn and skip bad records, otherwise we break the loop >- while (1) { >- my $row = $rs->next(); >- return if !$row; >- my $marc = C4::Biblio::GetMarcBiblio({ >- biblionumber => $row->biblionumber, >- embed_items => 1 }); >- my $next = eval { >- __PACKAGE__->new($marc, $row->biblionumber); >- }; >- if ($@) { >- warn "Something went wrong reading record for biblio $row->biblionumber: $@\n"; >- next; >+ my $rs = $schema->resultset('Biblio')->search( >+ {}, >+ { columns => [qw/ biblionumber /] } >+ ); >+ >+ my $next_func; >+ if ($batched) { >+ my @marc_records_batch; >+ $next_func = sub { >+ unless (@marc_records_batch) { >+ #Batch empty, nead to buffer up more records >+ my $limit = $batch_size; >+ my @biblionumbers; >+ while ($limit--) { >+ my $row = $rs->next(); >+ last if !$row; >+ push @biblionumbers, $row->biblionumber; >+ } >+ if (@biblionumbers) { >+ my $marc_records = C4::Biblio::GetMarcBiblios({ >+ biblionumbers => \@biblionumbers, >+ embed_items => 1 >+ }); >+ while (my ($biblionumber, $marc_record) = each %{$marc_records}) { >+ my $next = __PACKAGE__->new($marc_record, $biblionumber); >+ push @marc_records_batch, $next; >+ } >+ } > } >- return $next; >- } >- }; >+ return pop @marc_records_batch; >+ }; >+ } >+ else { >+ $next_func = sub { >+ # Warn and skip bad records, otherwise we break the loop >+ while (1) { >+ my $row = $rs->next(); >+ return if !$row; >+ my $marc = C4::Biblio::GetMarcBiblio({ >+ biblionumber => $row->biblionumber, >+ embed_items => 1 >+ }); >+ my $next = eval { >+ __PACKAGE__->new($marc, $row->biblionumber); >+ }; >+ if ($@) { >+ warn "Something went wrong reading record for biblio $row->biblionumber: $@\n"; >+ next; >+ } >+ return $next; >+ } >+ }; >+ } > return Koha::MetadataIterator->new($next_func); > } > >diff --git a/Koha/Items.pm b/Koha/Items.pm >index 7b86c5a5e0..29b46949de 100644 >--- a/Koha/Items.pm >+++ b/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_placeholders = join ', ', (('?') x @{$values}); >+ my $sth = C4::Context->dbh->prepare(qq{SELECT * FROM items WHERE $field IN($in_placeholders)}); >+ $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 <kyle@bywatersolutions.com> >diff --git a/misc/search_tools/rebuild_elastic_search.pl b/misc/search_tools/rebuild_elastic_search.pl >index caa9a3d870..bde30e4453 100755 >--- a/misc/search_tools/rebuild_elastic_search.pl >+++ b/misc/search_tools/rebuild_elastic_search.pl >@@ -131,7 +131,7 @@ if ($index_biblios) { > return ($r, Koha::BiblioUtils->get_from_biblionumber($r, item_data => 1 )); > }; > } else { >- my $records = Koha::BiblioUtils->get_all_biblios_iterator(); >+ my $records = Koha::BiblioUtils->get_all_biblios_iterator({ batched => 1 }); > $next = sub { > $records->next(); > } >diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t >index bc7aaa9feb..387e750f33 100755 >--- a/t/db_dependent/Items.t >+++ b/t/db_dependent/Items.t >@@ -42,7 +42,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; > >@@ -59,13 +61,21 @@ 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" ); >@@ -75,18 +85,77 @@ subtest 'General Add, Get and Del tests' => sub { > my $dbh = C4::Context->dbh; > local $dbh->{RaiseError} = 1; > ModItem({}, $bibnum, $itemnumber); >+ $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" ); >@@ -104,6 +173,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" ); >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 19884
:
70172
|
70182
|
70183
|
70184
|
70185
|
70247
|
70248
|
70251
|
70284
|
70285
|
70286
|
70606
|
82870
|
82871
|
82979
|
83111
|
83112
|
93949