Attachment 110937 Details for Bug 25265 – Bug 25265: [20.05.x] Prevent double reindex of the same item in batchmod

[patch] Bug 25265: [20.05.x] Prevent double reindex of the same item in batchmod

Bug-25265-2005x-Prevent-double-reindex-of-the-same.patch (text/plain), 33.42 KB, created by Nick Clemens (kidclamp) on 2020-09-29 18:17:29 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2020-09-29 18:17:29 UTC

Size: 33.42 KB

patch

obsolete

>From 0903961728237d956f7feb73b17151661afaeb51 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Wed, 5 Aug 2020 14:20:47 +0200
>Subject: [PATCH] Bug 25265: [20.05.x] Prevent double reindex of the same item
> in batchmod
>
>When batch editing, 2 reindex calls are sent to ES/Zebra.
>We can easily avoid that reusing the skip_modzebra_update (renamed skip_record_index)
>
>Additionally we should only send one request for biblio, and we should
>only do it if we succeed
>
>As the whole batch mod is in a transaction it is possible to fail in which case
>Zebra queue is reset, but ES indexes have already been set
>
>In addition to the skip param this patchset moves Zebra and Elasticsearch calls to
>Indexer modules and introduces a generic Koha::SearchEngine::Indexer so that we don't
>need to check the engine when calling for index
>
>The new index_records routine takes an array so that we can reduce the calls to
>the ES server.
>
>The index_records routine for Zebra loops over ModZebra to avoid affecting current behaviour
>
>Test plan:
>
>General tests, under both search engines:
>  1 - Add a biblio and confirm it is searchable
>  2 - Edit the biblio and confirm changes are searchable
>  3 - Add an item, confirm it is searchable
>  4 - Delete an item, confirm it is not searchable
>  5 - Delete a biblio, confirm it is not searchable
>  6 - Add an authority and confirm it is searchable
>  7 - Delete an authority and confirm it is not searchable
>
>Batch mod tests, under both search engines
>  1 - Have a bib with several items, none marked 'not for loan'
>  2 - Do a staff search that returns this biblio
>  3 - Items show as available
>  4 - Click on title to go to details page
>  5 - Edit->Item in a batch
>  6 - Set the not for loan status for all items
>  7 - Repeat your search
>  8 - Items show as not for loan
>  9 - Test batch deleting items
>    a - Test with a list of items, not deleting bibs
>    b - Test with a list of items, deleting bibs if no items remain where all items are only item on a biblio:
>     SELECT MAX(barcode) FROM items GROUP BY biblionumber HAVING COUNT(barcode) IN (1)
>    c - Test with a list of items, deleting bibs if no items remain where some items are the only item on a biblio:
>     SELECT MAX(barcode) FROM items GROUP BY biblionumber HAVING COUNT(barcode) IN (1,2)
> 10 - Confirm records are update/deleted as appropriate
>
>Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 25265: Rename skip_modzebra_update to skip_record_index
>
>Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 25265: Fix copy paste error for parameter
>
>Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 25265: (follow-up) Don't index malformed records
>
>This is analogous to 26522, we shoudl skip record that cannot be retrieved for indexing
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 25265: (QA follow-up) Add shebang to Indexer.t
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 25265: (QA follow-up) Rename biblionumber in ModZebra, index_records
>
>ModZebra:
>The name is very misleading: we can index authid's too here.
>And yes, it should not be in C4/Biblio too ;) A first step..
>
>Adding the same change here in Koha/SearchEngine/Zebra/Indexer.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 25265: (QA follow-up) Check server type in Elasticsearch::index_records
>
>Doing the same change as previously (renaming biblionumber), but fixing
>at the same the record fetch. If (theoretically) an authority is passed
>without a record, it would have fetched a biblio record.
>
>Test plan:
>You need Elasticsearch here.
>Replaced this line in AddAuthority:
>    $indexer->index_records( $authid, "specialUpdate", "authorityserver", $record );
>by
>    $indexer->index_records( $authid, "specialUpdate", "authorityserver", undef );
>And updated an authority record. Check if you can search for the change.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>---
> C4/AuthoritiesMarc.pm                              |  7 ++-
> C4/Biblio.pm                                       | 63 ++++++---------------
> C4/Circulation.pm                                  | 18 +++++-
> C4/Items.pm                                        | 37 ++++++++----
> Koha/Item.pm                                       | 16 +++---
> Koha/SearchEngine/Elasticsearch/Indexer.pm         | 46 +++++++++++++++
> Koha/SearchEngine/Indexer.pm                       | 59 ++++++++++++++++++++
> Koha/SearchEngine/Zebra/Indexer.pm                 | 65 ++++++++++++++++++++++
> cataloguing/additem.pl                             | 11 ++--
> .../Koha/SearchEngine/Elasticsearch/Indexer.t      |  2 +
> t/db_dependent/Koha/SearchEngine/Indexer.t         | 12 ++--
> tools/batchMod.pl                                  | 30 ++++++++--
> 12 files changed, 282 insertions(+), 84 deletions(-)
> create mode 100644 Koha/SearchEngine/Indexer.pm
> create mode 100644 Koha/SearchEngine/Zebra/Indexer.pm
>
>diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
>index bd25837c36..cbf59cdc03 100644
>--- a/C4/AuthoritiesMarc.pm
>+++ b/C4/AuthoritiesMarc.pm
>@@ -34,6 +34,7 @@ use Koha::Authority::Types;
> use Koha::Authority;
> use Koha::Libraries;
> use Koha::SearchEngine;
>+use Koha::SearchEngine::Indexer;
> use Koha::SearchEngine::Search;
> 
> use vars qw(@ISA @EXPORT);
>@@ -628,7 +629,8 @@ sub AddAuthority {
>     $record->insert_fields_ordered( MARC::Field->new( '001', $authid ) );
>     # Update
>     $dbh->do( "UPDATE auth_header SET authtypecode=?, marc=?, marcxml=? WHERE authid=?", undef, $authtypecode, $record->as_usmarc, $record->as_xml_record($format), $authid ) or die $DBI::errstr;
>-    ModZebra( $authid, 'specialUpdate', 'authorityserver', $record );
>+    my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::AUTHORITIES_INDEX });
>+    $indexer->index_records( $authid, "specialUpdate", "authorityserver", $record );
> 
>     return ( $authid );
> }
>@@ -656,7 +658,8 @@ sub DelAuthority {
>     merge({ mergefrom => $authid }) if !$skip_merge;
>     $dbh->do( "DELETE FROM auth_header WHERE authid=?", undef, $authid );
>     logaction( "AUTHORITIES", "DELETE", $authid, "authority" ) if C4::Context->preference("AuthoritiesLog");
>-    ModZebra( $authid, "recordDelete", "authorityserver", undef);
>+    my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::AUTHORITIES_INDEX });
>+    $indexer->index_records( $authid, "recordDelete", "authorityserver", undef );
> }
> 
> =head2 ModAuthority
>diff --git a/C4/Biblio.pm b/C4/Biblio.pm
>index 0f0e9e62b2..5d2e1a2c25 100644
>--- a/C4/Biblio.pm
>+++ b/C4/Biblio.pm
>@@ -103,6 +103,7 @@ use Koha::Holds;
> use Koha::ItemTypes;
> use Koha::Plugins;
> use Koha::SearchEngine;
>+use Koha::SearchEngine::Indexer;
> use Koha::Libraries;
> use Koha::Util::MARC;
> 
>@@ -366,7 +367,7 @@ C<$error> : undef unless an error occurs
> =cut
> 
> sub DelBiblio {
>-    my ($biblionumber) = @_;
>+    my ($biblionumber, $params) = @_;
> 
>     my $biblio = Koha::Biblios->find( $biblionumber );
>     return unless $biblio; # Should we throw an exception instead?
>@@ -391,11 +392,10 @@ sub DelBiblio {
>         $hold->cancel;
>     }
> 
>-    # Delete in Zebra. Be careful NOT to move this line after _koha_delete_biblio
>-    # for at least 2 reasons :
>-    # - if something goes wrong, the biblio may be deleted from Koha but not from zebra
>-    #   and we would have no way to remove it (except manually in zebra, but I bet it would be very hard to handle the problem)
>-    ModZebra( $biblionumber, "recordDelete", "biblioserver" );
>+    unless ( $params->{skip_record_index} ){
>+        my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
>+        $indexer->index_records( $biblionumber, "recordDelete", "biblioserver" );
>+    }
> 
>     # delete biblioitems and items from Koha tables and save in deletedbiblioitems,deleteditems
>     $sth = $dbh->prepare("SELECT biblioitemnumber FROM biblioitems WHERE biblionumber=?");
>@@ -2489,50 +2489,19 @@ sub CountItemsIssued {
> 
> =head2 ModZebra
> 
>-  ModZebra( $biblionumber, $op, $server, $record );
>+  ModZebra( $record_number, $op, $server, $record );
> 
>-$biblionumber is the biblionumber we want to index
>+$record_number is the authid or biblionumber we want to index
> 
>-$op is specialUpdate or recordDelete, and is used to know what we want to do
>+$op is the operation: specialUpdate or recordDelete
> 
>-$server is the server that we want to update
>-
>-$record is the updated MARC record. If it's not supplied
>-and is needed it will be loaded from the database.
>+$server is authorityserver or biblioserver
> 
> =cut
> 
> sub ModZebra {
>-###Accepts a $server variable thus we can use it for biblios authorities or other zebra dbs
>-    my ( $biblionumber, $op, $server, $record ) = @_;
>-    $debug && warn "ModZebra: update requested for: $biblionumber $op $server\n";
>-    if ( C4::Context->preference('SearchEngine') eq 'Elasticsearch' ) {
>-
>-        # TODO abstract to a standard API that'll work for whatever
>-        require Koha::SearchEngine::Elasticsearch::Indexer;
>-        my $indexer = Koha::SearchEngine::Elasticsearch::Indexer->new(
>-            {
>-                index => $server eq 'biblioserver'
>-                ? $Koha::SearchEngine::BIBLIOS_INDEX
>-                : $Koha::SearchEngine::AUTHORITIES_INDEX
>-            }
>-        );
>-        if ( $op eq 'specialUpdate' ) {
>-            unless ($record) {
>-                $record = GetMarcBiblio({
>-                    biblionumber => $biblionumber,
>-                    embed_items  => 1 });
>-            }
>-            $indexer->update_index_background( [$biblionumber], [$record] );
>-        }
>-        elsif ( $op eq 'recordDelete' ) {
>-            $indexer->delete_index_background( [$biblionumber] );
>-        }
>-        else {
>-            croak "ModZebra called with unknown operation: $op";
>-        }
>-    }
>-
>+    my ( $record_number, $op, $server ) = @_;
>+    $debug && warn "ModZebra: updates requested for: $record_number $op $server\n";
>     my $dbh = C4::Context->dbh;
> 
>     # true ModZebra commented until indexdata fixes zebraDB crashes (it seems they occur on multiple updates
>@@ -2545,17 +2514,16 @@ sub ModZebra {
>         AND   operation = ?
>         AND   done = 0";
>     my $check_sth = $dbh->prepare_cached($check_sql);
>-    $check_sth->execute( $server, $biblionumber, $op );
>+    $check_sth->execute( $server, $record_number, $op );
>     my ($count) = $check_sth->fetchrow_array;
>     $check_sth->finish();
>     if ( $count == 0 ) {
>         my $sth = $dbh->prepare("INSERT INTO zebraqueue  (biblio_auth_number,server,operation) VALUES(?,?,?)");
>-        $sth->execute( $biblionumber, $server, $op );
>+        $sth->execute( $record_number, $server, $op );
>         $sth->finish;
>     }
> }
> 
>-
> =head2 EmbedItemsInMarcBiblio
> 
>     EmbedItemsInMarcBiblio({
>@@ -3147,7 +3115,8 @@ sub ModBiblioMarc {
>     $m_rs->metadata( $record->as_xml_record($encoding) );
>     $m_rs->store;
> 
>-    ModZebra( $biblionumber, "specialUpdate", "biblioserver" );
>+    my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
>+    $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" );
> 
>     return $biblionumber;
> }
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 849c7cdd2c..23a609d05e 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -3775,9 +3775,23 @@ sub ReturnLostItem{
>     MarkIssueReturned( $borrowernumber, $itemnum );
> }
> 
>+=head2 LostItem
>+
>+  LostItem( $itemnumber, $mark_lost_from, $force_mark_returned, [$params] );
>+
>+The final optional parameter, C<$params>, expected to contain
>+'skip_record_index' key, which relayed down to Koha::Item/store,
>+there it prevents calling of ModZebra index_records,
>+which takes most of the time in batch adds/deletes: index_records better
>+to be called later in C<additem.pl> after the whole loop.
>+
>+$params:
>+    skip_record_index => 1|0
>+
>+=cut
> 
> sub LostItem{
>-    my ($itemnumber, $mark_lost_from, $force_mark_returned) = @_;
>+    my ($itemnumber, $mark_lost_from, $force_mark_returned, $params) = @_;
> 
>     unless ( $mark_lost_from ) {
>         # Temporary check to avoid regressions
>@@ -3828,7 +3842,7 @@ sub LostItem{
> 
>     #When item is marked lost automatically cancel its outstanding transfers and set items holdingbranch to the transfer source branch (frombranch)
>     if (my ( $datesent,$frombranch,$tobranch ) = GetTransfers($itemnumber)) {
>-        Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store;
>+        Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store({ skip_record_index => $params->{skip_record_index} });
>     }
>     my $transferdeleted = DeleteTransfer($itemnumber);
> }
>diff --git a/C4/Items.pm b/C4/Items.pm
>index f2db7a4894..24a8ee1cc4 100644
>--- a/C4/Items.pm
>+++ b/C4/Items.pm
>@@ -70,6 +70,7 @@ use Koha::Biblioitems;
> use Koha::Items;
> use Koha::ItemTypes;
> use Koha::SearchEngine;
>+use Koha::SearchEngine::Indexer;
> use Koha::SearchEngine::Search;
> use Koha::Libraries;
> 
>@@ -143,13 +144,13 @@ Given a MARC::Record object containing an embedded item
> record and a biblionumber, create a new item record.
> 
> The final optional parameter, C<$params>, expected to contain
>-'skip_modzebra_update' key, which relayed down to Koha::Item/store,
>-there it prevents calling of ModZebra (and Elasticsearch update),
>-which takes most of the time in batch adds/deletes: ModZebra better
>+'skip_record_index' key, which relayed down to Koha::Item/store,
>+there it prevents calling of index_records,
>+which takes most of the time in batch adds/deletes: index_records
> to be called later in C<additem.pl> after the whole loop.
> 
> $params:
>-    skip_modzebra_update => 1|0
>+    skip_record_index => 1|0
> 
> =cut
> 
>@@ -173,7 +174,7 @@ sub AddItemFromMarc {
>     $item_values->{biblionumber} = $biblionumber;
>     $item_values->{cn_source} = delete $item_values->{'items.cn_source'}; # Because of C4::Biblio::_disambiguate
>     $item_values->{cn_sort}   = delete $item_values->{'items.cn_sort'};   # Because of C4::Biblio::_disambiguate
>-    my $item = Koha::Item->new( $item_values )->store({ skip_modzebra_update => $params->{skip_modzebra_update} });
>+    my $item = Koha::Item->new( $item_values )->store({ skip_record_index => $params->{skip_record_index} });
>     return ( $item->biblionumber, $item->biblioitemnumber, $item->itemnumber );
> }
> 
>@@ -281,10 +282,23 @@ sub AddItemBatchFromMarc {
>     return (\@itemnumbers, \@errors);
> }
> 
>+=head2 ModItemFromMarc
>+
>+my $item = ModItemFromMarc($item_marc, $biblionumber, $itemnumber[, $params]);
>+
>+The final optional parameter, C<$params>, expected to contain
>+'skip_record_index' key, which relayed down to Koha::Item/store,
>+there it prevents calling of index_records,
>+which takes most of the time in batch adds/deletes: index_records better
>+to be called later in C<additem.pl> after the whole loop.
>+
>+$params:
>+    skip_record_index => 1|0
>+
>+=cut
>+
> sub ModItemFromMarc {
>-    my $item_marc = shift;
>-    my $biblionumber = shift;
>-    my $itemnumber = shift;
>+    my ( $item_marc, $biblionumber, $itemnumber, $params ) = @_;
> 
>     my $frameworkcode = C4::Biblio::GetFrameworkCode($biblionumber);
>     my ( $itemtag, $itemsubfield ) = C4::Biblio::GetMarcFromKohaField( "items.itemnumber" );
>@@ -313,7 +327,7 @@ sub ModItemFromMarc {
>     $item_object = $item_object->set_or_blank($item);
>     my $unlinked_item_subfields = _get_unlinked_item_subfields( $localitemmarc, $frameworkcode );
>     $item_object->more_subfields_xml(_get_unlinked_subfields_xml($unlinked_item_subfields));
>-    $item_object->store;
>+    $item_object->store({ skip_record_index => $params->{skip_record_index} });
> 
>     return $item_object->unblessed;
> }
>@@ -1094,8 +1108,9 @@ sub MoveItemFromBiblio {
>             AND biblionumber = ?
>     |, undef, $tobiblioitem, $tobiblio, $itemnumber, $frombiblio );
>     if ($return == 1) {
>-        ModZebra( $tobiblio, "specialUpdate", "biblioserver" );
>-        ModZebra( $frombiblio, "specialUpdate", "biblioserver" );
>+        my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
>+        $indexer->index_records( $tobiblio, "specialUpdate", "biblioserver" );
>+        $indexer->index_records( $frombiblio, "specialUpdate", "biblioserver" );
> 	    # Checking if the item we want to move is in an order 
>         require C4::Acquisition;
>         my $order = C4::Acquisition::GetOrderFromItemnumber($itemnumber);
>diff --git a/Koha/Item.pm b/Koha/Item.pm
>index d02817b1f2..bc22f70df9 100644
>--- a/Koha/Item.pm
>+++ b/Koha/Item.pm
>@@ -30,12 +30,12 @@ use Koha::DateUtils qw( dt_from_string );
> use C4::Context;
> use C4::Circulation;
> use C4::Reserves;
>-use C4::Biblio qw( ModZebra ); # FIXME This is terrible, we should move the indexation code outside of C4::Biblio
> use C4::ClassSource; # FIXME We would like to avoid that
> use C4::Log qw( logaction );
> 
> use Koha::Checkouts;
> use Koha::CirculationRules;
>+use Koha::SearchEngine::Indexer;
> use Koha::Item::Transfer::Limits;
> use Koha::Item::Transfers;
> use Koha::Patrons;
>@@ -60,8 +60,8 @@ Koha::Item - Koha Item object class
> 
>     $item->store;
> 
>-$params can take an optional 'skip_modzebra_update' parameter.
>-If set, the reindexation process will not happen (ModZebra not called)
>+$params can take an optional 'skip_record_index' parameter.
>+If set, the reindexation process will not happen (index_records not called)
> 
> NOTE: This is a temporary fix to answer a performance issue when lot of items
> are added (or modified) at the same time.
>@@ -176,8 +176,9 @@ sub store {
>     }
> 
>     my $result = $self->SUPER::store;
>-    C4::Biblio::ModZebra( $self->biblionumber, "specialUpdate", "biblioserver" )
>-        unless $params->{skip_modzebra_update};
>+    my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
>+    $indexer->index_records( $self->biblionumber, "specialUpdate", "biblioserver" )
>+        unless $params->{skip_record_index};
> 
>     return $result;
> }
>@@ -193,8 +194,9 @@ sub delete {
>     # FIXME check the item has no current issues
>     # i.e. raise the appropriate exception
> 
>-    C4::Biblio::ModZebra( $self->biblionumber, "specialUpdate", "biblioserver" )
>-        unless $params->{skip_modzebra_update};
>+    my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
>+    $indexer->index_records( $self->biblionumber, "specialUpdate", "biblioserver" )
>+        unless $params->{skip_record_index};
> 
>     $self->_after_item_action_hooks({ action => 'delete' });
> 
>diff --git a/Koha/SearchEngine/Elasticsearch/Indexer.pm b/Koha/SearchEngine/Elasticsearch/Indexer.pm
>index e2c1c91fbd..4e67a77e87 100644
>--- a/Koha/SearchEngine/Elasticsearch/Indexer.pm
>+++ b/Koha/SearchEngine/Elasticsearch/Indexer.pm
>@@ -25,6 +25,8 @@ use base qw(Koha::SearchEngine::Elasticsearch);
> use Data::Dumper;
> 
> use Koha::Exceptions;
>+use Koha::SearchEngine::Zebra::Indexer;
>+use C4::Biblio;
> use C4::Context;
> 
> =head1 NAME
>@@ -277,6 +279,50 @@ sub update_index_background {
>     $self->update_index(@_);
> }
> 
>+=head2 index_records
>+
>+This function takes an array of record numbers and fetches the records to send to update_index
>+for actual indexing.
>+
>+If $records parameter is provided the records will be used as-is, this is only utilized for authorities
>+at the moment.
>+
>+The other variables are used for parity with Zebra indexing calls. Currently the calls are passed through
>+to Zebra as well.
>+
>+=cut
>+
>+sub index_records {
>+    my ( $self, $record_numbers, $op, $server, $records ) = @_;
>+    $record_numbers = [$record_numbers] if ref $record_numbers ne 'ARRAY' && defined $record_numbers;
>+    $records = [$records] if ref $records ne 'ARRAY' && defined $records;
>+    if ( $op eq 'specialUpdate' ) {
>+        my $index_record_numbers;
>+        unless ($records) {
>+            foreach my $record_number ( @$record_numbers ){
>+                my $record = _get_record( $record_number, $server );
>+                if( $record ){
>+                    push @$records, $record;
>+                    push @$index_record_numbers, $record_number;
>+                }
>+            }
>+        }
>+        $self->update_index_background( $index_record_numbers, $records ) if $index_record_numbers && $records;
>+    }
>+    elsif ( $op eq 'recordDelete' ) {
>+        $self->delete_index_background( $record_numbers );
>+    }
>+    #FIXME Current behaviour is to index Zebra when using ES, at some point we should stop
>+    Koha::SearchEngine::Zebra::Indexer::index_records( $self, $record_numbers, $op, $server, undef );
>+}
>+
>+sub _get_record {
>+    my ( $id, $server ) = @_;
>+    return $server eq 'biblioserver'
>+        ? C4::Biblio::GetMarcBiblio({ biblionumber => $id, embed_items  => 1 })
>+        : C4::AuthoritiesMarc::GetAuthority($id);
>+}
>+
> =head2 delete_index($biblionums)
> 
> C<$biblionums> is an arrayref of biblionumbers to delete from the index.
>diff --git a/Koha/SearchEngine/Indexer.pm b/Koha/SearchEngine/Indexer.pm
>new file mode 100644
>index 0000000000..64262ff587
>--- /dev/null
>+++ b/Koha/SearchEngine/Indexer.pm
>@@ -0,0 +1,59 @@
>+package Koha::SearchEngine::Indexer;
>+
>+# This file is part of Koha.
>+#
>+# Copyright 2020 ByWater Solutions
>+#
>+# Koha is free software; you can redistribute it and/or modify it
>+# under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# Koha is distributed in the hope that it will be useful, but
>+# WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with Koha; if not, see <http://www.gnu.org/licenses>.
>+
>+# This is a shim that gives you the appropriate search object for your
>+# system preference.
>+
>+=head1 NAME
>+
>+Koha::SearchEngine::Indexer - instantiate the search object that corresponds to
>+the C<SearchEngine> system preference.
>+
>+=head1 DESCRIPTION
>+
>+This allows you to be agnostic about what the search engine configuration is
>+and just get whatever indexer object you need.
>+
>+=head1 SYNOPSIS
>+
>+    use Koha::SearchEngine::Indexer;
>+    my $searcher = Koha::SearchEngine::Indexer->new({ index => Koha::SearchEngine::Index});
>+
>+=head1 METHODS
>+
>+=head2 new
>+
>+Creates a new C<Search> of whatever the relevant type is.
>+
>+=cut
>+
>+use Modern::Perl;
>+use C4::Context;
>+use C4::Biblio qw//;
>+
>+sub new {
>+    my $engine = C4::Context->preference("SearchEngine") // 'Zebra';
>+    my $file = "Koha/SearchEngine/${engine}/Indexer.pm";
>+    my $class = "Koha::SearchEngine::${engine}::Indexer";
>+    require $file;
>+    shift @_;
>+    return $class->new(@_);
>+}
>+
>+1;
>diff --git a/Koha/SearchEngine/Zebra/Indexer.pm b/Koha/SearchEngine/Zebra/Indexer.pm
>new file mode 100644
>index 0000000000..8ead358d9e
>--- /dev/null
>+++ b/Koha/SearchEngine/Zebra/Indexer.pm
>@@ -0,0 +1,65 @@
>+package Koha::SearchEngine::Zebra::Indexer;
>+
>+# Copyright 2020 ByWater Solutions
>+#
>+# This file is part of Koha.
>+#
>+# Koha is free software; you can redistribute it and/or modify it
>+# under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# Koha is distributed in the hope that it will be useful, but
>+# WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with Koha; if not, see <http://www.gnu.org/licenses>.
>+
>+use Modern::Perl;
>+use C4::Biblio qw(ModZebra); # FIXME This is terrible, we should move the indexation code outside of C4::Biblio
>+use base qw(Class::Accessor);
>+
>+=head1 NAME
>+
>+Koha::SearchEngine::Elasticsearch::Indexer - handles adding new records to the index
>+
>+=head1 SYNOPSIS
>+
>+    my $indexer = Koha::SearchEngine::Zebra::Indexer->new();
>+    $indexer->index_records( $record_numbers, $op, $server, $records);
>+
>+
>+=head1 FUNCTIONS
>+
>+=head2 new
>+
>+    This is a dummy function to create the object. C4::Biblio->ModZebra is doing the real work
>+    now and needed variables are passed to index_records
>+
>+=cut
>+
>+sub new {
>+    my $class = shift @_;
>+    my $self = $class->SUPER::new(@_);
>+}
>+
>+=head2 index_records($record_numbers, $op, $server, $records)
>+
>+    This is simply a wrapper to C4::Biblio::ModZebra that takes an array of records and
>+    passes them through individually
>+
>+    The final parameter $records is not used in Zebra, it exists for parity with Elasticsearch calls
>+
>+=cut
>+
>+sub index_records {
>+    my ( $self, $record_numbers, $op, $server, $records ) = @_;
>+    $record_numbers = [$record_numbers] if ref $record_numbers ne 'ARRAY' && defined $record_numbers;
>+    foreach my $record_number ( @$record_numbers ){
>+        ModZebra( $record_number, $op, $server );
>+    }
>+}
>+
>+1;
>diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl
>index aeaa7536bc..510ae1bc02 100755
>--- a/cataloguing/additem.pl
>+++ b/cataloguing/additem.pl
>@@ -35,6 +35,7 @@ use Koha::Items;
> use Koha::ItemTypes;
> use Koha::Libraries;
> use Koha::Patrons;
>+use Koha::SearchEngine::Indexer;
> use List::MoreUtils qw/any/;
> use C4::Search;
> use Storable qw(thaw freeze);
>@@ -596,7 +597,7 @@ if ($op eq "additem") {
> 		# Adding the item
>         if (!$exist_itemnumber) {
>             my ( $oldbiblionumber, $oldbibnum, $oldbibitemnum ) =
>-                AddItemFromMarc( $record, $biblionumber, { skip_modzebra_update => 1 } );
>+                AddItemFromMarc( $record, $biblionumber, { skip_record_index => 1 } );
>             set_item_default_location($oldbibitemnum);
> 
>             # We count the item only if it was really added
>@@ -611,7 +612,8 @@ if ($op eq "additem") {
> 		$oldbarcode = $barcodevalue;
> 	    }
> 
>-        C4::Biblio::ModZebra( $biblionumber, "specialUpdate", "biblioserver" );
>+        my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
>+        $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" );
> 
> 	    undef($itemrecord);
> 	}
>@@ -685,11 +687,12 @@ if ($op eq "additem") {
> #-------------------------------------------------------------------------------
>     my $items = Koha::Items->search({ biblionumber => $biblionumber });
>     while ( my $item = $items->next ) {
>-        $error = $item->safe_delete({ skip_modzebra_update => 1 });
>+        $error = $item->safe_delete({ skip_record_index => 1 });
>         next if ref $error eq 'Koha::Item'; # Deleted item is returned if deletion successful
>         push @errors,$error;
>     }
>-    C4::Biblio::ModZebra( $biblionumber, "specialUpdate", "biblioserver" );
>+    my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
>+    $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" );
>     if ( @errors ) {
>         $nextop="additem";
>     } else {
>diff --git a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t
>index 6eebea84cc..7a26b70950 100644
>--- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t
>+++ b/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t
>@@ -1,3 +1,5 @@
>+#!/usr/bin/perl
>+
> # Copyright 2015 Catalyst IT
> #
> # This file is part of Koha.
>diff --git a/t/db_dependent/Koha/SearchEngine/Indexer.t b/t/db_dependent/Koha/SearchEngine/Indexer.t
>index abcbc9a9cb..4515d69f8d 100755
>--- a/t/db_dependent/Koha/SearchEngine/Indexer.t
>+++ b/t/db_dependent/Koha/SearchEngine/Indexer.t
>@@ -113,8 +113,8 @@ subtest 'Test indexer calls' => sub {
>             $item2 = $builder->build_sample_item({biblionumber => $biblio->biblionumber});
>         } [$engine,"Koha::Item",$engine,"Koha::Item"], "index_records is called for $engine when adding an item (Item->store)";
>         warnings_are{
>-            $item->store({ skip_modzebra_update => 1 });
>-        } undef, "index_records is not called for $engine when adding an item (Item->store) if skip_modzebra_update passed";
>+            $item->store({ skip_record_index => 1 });
>+        } undef, "index_records is not called for $engine when adding an item (Item->store) if skip_record_index passed";
> 
>         $builder->build({
>             source => 'Branchtransfer',
>@@ -132,15 +132,15 @@ subtest 'Test indexer calls' => sub {
>                 datearrived => undef}
>         });
>         warnings_are{
>-            LostItem( $item->itemnumber, "tests", undef, { skip_modzebra_update => 1 });
>-        } undef, "index_records is not called for $engine when calling LostItem and transfer exists if skip_modzebra_update";
>+            LostItem( $item->itemnumber, "tests", undef, { skip_record_index => 1 });
>+        } undef, "index_records is not called for $engine when calling LostItem and transfer exists if skip_record_index";
> 
>         warnings_are{
>             $item->delete();
>         } [$engine,"Koha::Item"], "index_records is called for $engine when deleting an item (Item->delete)";
>         warnings_are{
>-            $item2->delete({ skip_modzebra_update => 1 });
>-        } undef, "index_records is not called for $engine when adding an item (Item->store) if skip_modzebra_update passed";
>+            $item2->delete({ skip_record_index => 1 });
>+        } undef, "index_records is not called for $engine when adding an item (Item->store) if skip_record_index passed";
> 
>         warnings_are{
>             DelBiblio( $biblio->biblionumber );
>diff --git a/tools/batchMod.pl b/tools/batchMod.pl
>index 430bb0f8aa..8b030881f3 100755
>--- a/tools/batchMod.pl
>+++ b/tools/batchMod.pl
>@@ -44,6 +44,7 @@ use Koha::DateUtils;
> use Koha::Items;
> use Koha::ItemTypes;
> use Koha::Patrons;
>+use Koha::SearchEngine::Indexer;
> 
> my $input = new CGI;
> my $dbh = C4::Context->dbh;
>@@ -183,6 +184,8 @@ if ($op eq "action") {
> 	    }
>         }
> 
>+        my $upd_biblionumbers;
>+        my $del_biblionumbers;
>         try {
>             my $schema = Koha::Database->new->schema;
>             $schema->txn_do(
>@@ -200,6 +203,7 @@ if ($op eq "action") {
>                             my $return = $item->safe_delete;
>                             if ( ref( $return ) ) {
>                                 $deleted_items++;
>+                                push @$upd_biblionumbers, $itemdata->{'biblionumber'};
>                             }
>                             else {
>                                 $not_deleted_items++;
>@@ -217,9 +221,10 @@ if ($op eq "action") {
>                             if ($del_records) {
>                                 my $itemscount = Koha::Biblios->find( $itemdata->{'biblionumber'} )->items->count;
>                                 if ( $itemscount == 0 ) {
>-                                    my $error = DelBiblio( $itemdata->{'biblionumber'} );
>+                                    my $error = DelBiblio( $itemdata->{'biblionumber'}, { skip_record_index => 1 } );
>                                     unless ($error) {
>                                         $deleted_records++;
>+                                        push @$del_biblionumbers, $itemdata->{'biblionumber'};
>                                         if ( $src eq 'CATALOGUING' ) {
>                                             # We are coming catalogue/detail.pl, there were items from a single bib record
>                                             $template->param( biblio_deleted => 1 );
>@@ -279,15 +284,21 @@ if ($op eq "action") {
>                                             my $item = ModItemFromMarc(
>                                                 $localmarcitem,
>                                                 $itemdata->{biblionumber},
>-                                                $itemnumber
>+                                                $itemnumber,
>+                                                { skip_record_index => 1 },
>                                             )
>                                           )
>                                         {
>-                                            LostItem( $itemnumber, 'batchmod' )
>-                                              if $item->{itemlost}
>+                                            LostItem(
>+                                                $itemnumber,
>+                                                'batchmod',
>+                                                undef,
>+                                                { skip_record_index => 1 }
>+                                            ) if $item->{itemlost}
>                                               and not $itemdata->{itemlost};
>                                         }
>                                     };
>+                                    push @$upd_biblionumbers, $itemdata->{'biblionumber'};
>                                 }
>                                 if ($runinbackground) {
>                                     $modified_items++ if $modified;
>@@ -316,7 +327,16 @@ if ($op eq "action") {
>             }
>             die "Something terrible has happened!"
>                 if ($_ =~ /Rollback failed/); # Rollback failed
>-        }
>+        };
>+        $upd_biblionumbers = [ uniq @$upd_biblionumbers ]; # Only update each bib once
>+
>+        # Don't send specialUpdate for records we are going to delete
>+        my %del_bib_hash = map{ $_ => undef } @$del_biblionumbers;
>+        @$upd_biblionumbers = grep( ! exists( $del_bib_hash{$_} ), @$upd_biblionumbers );
>+
>+        my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
>+        $indexer->index_records( $upd_biblionumbers, 'specialUpdate', "biblioserver", undef ) if @$upd_biblionumbers;
>+        $indexer->index_records( $del_biblionumbers, 'recordDelete', "biblioserver", undef ) if @$del_biblionumbers;
>     }
> }
> #
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 25265: 107832 | 107855 | 107867 | 109538 | 109556 | 110262 | 110263 | 110264 | 110380 | 110381 | 110382 | 110457 | 110458 | 110459 | 110560 | 110561 | 110562 | 110565 | 110594 | 110595 | 110596 | 110597 | 110615 | 110618 | 110729 | 110730 | 110731 | 110732 | 110733 | 110734 | 110736 | 110737 | 110882 | 110883 | 110937