From 9a7815f34f5c33f8d12a8ec89fd9a82bfbb75e53 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 21d270e3df..97898f550e 100644 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -265,6 +265,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 = {}; @@ -272,7 +273,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 b6d532ea40..20017a2943 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 ec19ce1f5c..25df7826b5 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'); @@ -680,10 +688,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 3f8bb8fd62..b4fa518a13 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -585,6 +585,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