From 288d71a8735760d1ecc8f9ce8cbf77c7cd7356e5 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 3 Dec 2020 01:33:07 +0000 Subject: [PATCH] Bug 27138: Index host items in child records when sending to search engine On the Koha side we seperate the items from the MARC record but we do put them in the search engines For easy analytics we embed host items when displaying results after fetching from the search engine We can simpify things and improve results by including the host items when indexing To test: 1 - Enable EasyAnalyticalRecords 2 - Find a record in the staff client 3 - From details page select Edit->Link to host record 4 - Attach a barcode from a different record 5 - Search for that barcode in the staff client 6 - Only host record is returned 7 - Perform a search that returns the child record and others 8 - Note the host item is listed on the child record in the results page 9 - Repeat on OPAC 10 - Note on opac that host item is missing on results 11 - Apply patch 12 - Search for barcode 13 - Child and host record returned on both staff and OPAC 14 - Child record correctly includes host item on staff and OPAC 15 - Connect to the Z3950 responder or zebra directly yaz-client localhost:2100 yaz-client unix:/var/run/koha/kohadev/bibliosocket 16 - Search for the child record base biblios find record_keyword show 1+10 17 - Note the record includes the host item 18 - Repeat tests for Elasticsearch Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Michael Sutherland --- C4/XSLT.pm | 6 +- Koha/Biblio/Metadata.pm | 11 +- Koha/SearchEngine/Elasticsearch/Indexer.pm | 3 +- misc/migration_tools/rebuild_zebra.pl | 14 +- t/db_dependent/Items.t | 155 +++++++++++++++++++++ 5 files changed, 183 insertions(+), 6 deletions(-) diff --git a/C4/XSLT.pm b/C4/XSLT.pm index 0fb7be0d6c..42d5b4e47d 100644 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -326,6 +326,7 @@ Is only used in this module currently. sub buildKohaItemsNamespace { my ($biblionumber, $hidden_items, $items_rs) = @_; + my @host_itemnumbers = C4::Items::get_hostitemnumbers_of( $biblionumber ); $hidden_items ||= []; my $query = {}; @@ -333,7 +334,10 @@ sub buildKohaItemsNamespace { if $hidden_items; unless ( $items_rs && ref($items_rs) eq 'Koha::Items' ) { - $query->{'me.biblionumber'} = $biblionumber; + $query->{ '-or' } = [ + 'me.biblionumber' => $biblionumber, + 'me.itemnumber' => { -in => \@host_itemnumbers } + ]; $items_rs = Koha::Items->new; } diff --git a/Koha/Biblio/Metadata.pm b/Koha/Biblio/Metadata.pm index 78361ab261..825a3cf61f 100644 --- a/Koha/Biblio/Metadata.pm +++ b/Koha/Biblio/Metadata.pm @@ -21,7 +21,7 @@ use MARC::File::XML; use Scalar::Util qw( blessed ); use C4::Biblio qw( GetMarcFromKohaField ); -use C4::Items qw( GetMarcItem ); +use C4::Items qw( GetMarcItem get_hostitemnumbers_of ); use Koha::Database; use Koha::Exceptions::Metadata; @@ -157,9 +157,14 @@ sub _embed_items { $record->delete_field($field); } - my $biblio = Koha::Biblios->find($biblionumber); + push @$itemnumbers, C4::Items::get_hostitemnumbers_of( $biblionumber ) if $params->{embed_host_items}; - my $items = $biblio->items; + my $items = Koha::Items->search([ + 'me.biblionumber' => $biblionumber, + 'me.itemnumber' => { + -in => $itemnumbers + } + ]); if ( @$itemnumbers ) { $items = $items->search({ itemnumber => { -in => $itemnumbers } }); } diff --git a/Koha/SearchEngine/Elasticsearch/Indexer.pm b/Koha/SearchEngine/Elasticsearch/Indexer.pm index 6101a3a372..d1201e4927 100644 --- a/Koha/SearchEngine/Elasticsearch/Indexer.pm +++ b/Koha/SearchEngine/Elasticsearch/Indexer.pm @@ -341,13 +341,14 @@ sub _get_record { if ( $self->index eq $Koha::SearchEngine::BIBLIOS_INDEX ) { my $biblio = Koha::Biblios->find($record_id); - $record = $biblio->metadata->record( { embed_items => 1 } ) + $record = $biblio->metadata->record( { embed_items => 1, embed_host_items => 1 } ) if $biblio; } else { $record = C4::AuthoritiesMarc::GetAuthority($record_id); } return $record; + } =head2 delete_index($biblionums) diff --git a/misc/migration_tools/rebuild_zebra.pl b/misc/migration_tools/rebuild_zebra.pl index a73e7bdd2f..3d92b6e507 100755 --- a/misc/migration_tools/rebuild_zebra.pl +++ b/misc/migration_tools/rebuild_zebra.pl @@ -511,7 +511,15 @@ sub export_marc_records_from_sth { if ($record_type eq 'biblio'){ my $biblio = Koha::Biblios->find($record_number); next unless $biblio; - my $items = $biblio->items; + my @host_itemnumbers = C4::Items::get_hostitemnumbers_of( $record_number ); + my $items = Koha::Items->search({ + -or => [ + biblionumber => $record_number, + itemnumber => { + -in => \@host_itemnumbers + } + ] + }); if ($items->count){ my $record = MARC::Record->new; $record->encoding('UTF-8'); @@ -681,10 +689,14 @@ sub get_raw_marc_record { my $marc; if ($record_type eq 'biblio') { +<<<<<<< HEAD eval { my $biblio = Koha::Biblios->find($record_number); $marc = $biblio->metadata->record({ embed_items => 1 }); }; +======= + eval { $marc = C4::Biblio::GetMarcBiblio({ biblionumber => $record_number, embed_items => 1, embed_host_items => 1 }); }; +>>>>>>> Bug 27138: Index host items in child records when sending to search engine if ($@ || !$marc) { # here we do warn since catching an exception # means that the bib was found but failed diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index c39a0b963f..b16c6cfc4c 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -665,6 +665,161 @@ subtest 'Koha::Item(s) tests' => sub { $schema->storage->txn_rollback; }; +subtest 'C4::Biblio::EmbedItemsInMarcBiblio' => sub { + plan tests => 11; + + $schema->storage->txn_begin(); + + my $builder = t::lib::TestBuilder->new; + my $library1 = $builder->build({ + source => 'Branch', + }); + my $library2 = $builder->build({ + source => 'Branch', + }); + my $itemtype = $builder->build({ + source => 'Itemtype', + }); + + my $biblio = $builder->build_sample_biblio(); + my $item_infos = [ + { homebranch => $library1->{branchcode}, holdingbranch => $library1->{branchcode} }, + { homebranch => $library1->{branchcode}, holdingbranch => $library1->{branchcode} }, + { homebranch => $library1->{branchcode}, holdingbranch => $library1->{branchcode} }, + { homebranch => $library2->{branchcode}, holdingbranch => $library2->{branchcode} }, + { homebranch => $library2->{branchcode}, holdingbranch => $library2->{branchcode} }, + { homebranch => $library1->{branchcode}, holdingbranch => $library2->{branchcode} }, + { homebranch => $library1->{branchcode}, holdingbranch => $library2->{branchcode} }, + { homebranch => $library1->{branchcode}, holdingbranch => $library2->{branchcode} }, + ]; + my $number_of_items = scalar @$item_infos; + my $number_of_items_with_homebranch_is_CPL = + grep { $_->{homebranch} eq $library1->{branchcode} } @$item_infos; + + my @itemnumbers; + for my $item_info (@$item_infos) { + my $itemnumber = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + homebranch => $item_info->{homebranch}, + holdingbranch => $item_info->{holdingbranch}, + itype => $itemtype->{itemtype} + } + )->itemnumber; + + push @itemnumbers, $itemnumber; + } + + my $host_item = $builder->build_sample_item({ homebranch => $library2->{branchcode} }); + my $child_field = PrepHostMarcField( $host_item->biblionumber, $host_item->itemnumber, 'MARC21'); + my $child_record = $biblio->metadata->record; + $child_record->append_fields($child_field); + ModBiblio( $child_record, $biblio->biblionumber, '' ); + + # Emptied the OpacHiddenItems pref + t::lib::Mocks::mock_preference( 'OpacHiddenItems', '' ); + + my ($itemfield) = + C4::Biblio::GetMarcFromKohaField( 'items.itemnumber' ); + my $record = C4::Biblio::GetMarcBiblio({ biblionumber => $biblio->biblionumber }); + warning_is { C4::Biblio::EmbedItemsInMarcBiblio() } + { carped => 'EmbedItemsInMarcBiblio: No MARC record passed' }, + 'Should carp is no record passed.'; + + C4::Biblio::EmbedItemsInMarcBiblio({ + marc_record => $record, + biblionumber => $biblio->biblionumber }); + my @items = $record->field($itemfield); + is( scalar @items, $number_of_items, 'Should return all items' ); + + my $marc_with_items = C4::Biblio::GetMarcBiblio({ + biblionumber => $biblio->biblionumber, + embed_items => 1 }); + is_deeply( $record, $marc_with_items, 'A direct call to GetMarcBiblio with items matches'); + + C4::Biblio::EmbedItemsInMarcBiblio({ + marc_record => $record, + biblionumber => $biblio->biblionumber, + embed_host_items => 1 + }); + @items = $record->field($itemfield); + is( scalar @items, $number_of_items + 1, 'Should return all items plus host items' ); + + $marc_with_items = C4::Biblio::GetMarcBiblio({ + biblionumber => $biblio->biblionumber, + embed_items => 1, + embed_host_items => 1 + }); + is_deeply( $record, $marc_with_items, 'A direct call to GetMarcBiblio with items matches'); + + C4::Biblio::EmbedItemsInMarcBiblio({ + marc_record => $record, + biblionumber => $biblio->biblionumber, + item_numbers => [ $itemnumbers[1], $itemnumbers[3] ] }); + @items = $record->field($itemfield); + is( scalar @items, 2, 'Should return all items present in the list' ); + + C4::Biblio::EmbedItemsInMarcBiblio({ + marc_record => $record, + biblionumber => $biblio->biblionumber, + opac => 1 }); + @items = $record->field($itemfield); + is( scalar @items, $number_of_items, 'Should return all items for opac' ); + + my $opachiddenitems = " + homebranch: ['$library1->{branchcode}']"; + t::lib::Mocks::mock_preference( 'OpacHiddenItems', $opachiddenitems ); + + C4::Biblio::EmbedItemsInMarcBiblio({ + marc_record => $record, + biblionumber => $biblio->biblionumber }); + @items = $record->field($itemfield); + is( scalar @items, + $number_of_items, + 'Even with OpacHiddenItems set, all items should have been embedded' ); + + C4::Biblio::EmbedItemsInMarcBiblio({ + marc_record => $record, + biblionumber => $biblio->biblionumber, + opac => 1 }); + @items = $record->field($itemfield); + is( + scalar @items, + $number_of_items - $number_of_items_with_homebranch_is_CPL, +'For OPAC, the pref OpacHiddenItems should have been take into account. Only items with homebranch ne CPL should have been embedded' + ); + + C4::Biblio::EmbedItemsInMarcBiblio({ + marc_record => $record, + biblionumber => $biblio->biblionumber, + opac => 1, + embed_host_items => 1 + }); + @items = $record->field($itemfield); + is( + scalar @items, + $number_of_items - $number_of_items_with_homebranch_is_CPL + 1, +'For OPAC, the pref OpacHiddenItems should have been take into account and host items embedded. Only items with homebranch ne CPL should have been embedded' + ); + + + $opachiddenitems = " + homebranch: ['$library1->{branchcode}', '$library2->{branchcode}']"; + t::lib::Mocks::mock_preference( 'OpacHiddenItems', $opachiddenitems ); + C4::Biblio::EmbedItemsInMarcBiblio({ + marc_record => $record, + biblionumber => $biblio->biblionumber, + opac => 1 }); + @items = $record->field($itemfield); + is( + scalar @items, + 0, +'For OPAC, If all items are hidden, no item should have been embedded' + ); + + $schema->storage->txn_rollback; +}; + subtest 'get_hostitemnumbers_of' => sub { plan tests => 3; -- 2.30.2