From c0b4795c631150f74e8172d6930ca1f95f0cbec8 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Wed, 30 Sep 2020 16:52:43 +0000
Subject: [PATCH] Bug 26581: Only reindex records once per checkin

This patch simply passes skip_record_index calls to Koha:Item:store
for all the changes done in AddReturn.

Testing is really verifiying that items are still correctly indexed at the end

For both search engines
To test:
1 - Find or create a record with multipel items
2 - Populate both:
     UpdateItemLocationOnCheckin
     UpdateNotForLoanStatusOnCheckin
3 - Confirm that checking in an item correctly updates the item status in search results
4 - Test with items issued, and items not issued
5 - Test when generating a transfer (checkin at different branch)
6 - Test when item was marked lost
7 - Test when filling transfer
---
 C4/Circulation.pm | 40 +++++++++++++++++++++++++---------------
 C4/Items.pm       | 18 +++++++++++-------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index ed2b43f3d5..0603862e8e 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -58,6 +58,7 @@ use Koha::Config::SysPrefs;
 use Koha::Charges::Fees;
 use Koha::Util::SystemPreferences;
 use Koha::Checkouts::ReturnClaims;
+use Koha::SearchEngine::Indexer;
 use Carp;
 use List::MoreUtils qw( uniq any );
 use Scalar::Util qw( looks_like_number );
@@ -1887,7 +1888,7 @@ sub AddReturn {
                 . Dumper($issue->unblessed) . "\n";
     } else {
         $messages->{'NotIssued'} = $barcode;
-        $item->onloan(undef)->store if defined $item->onloan;
+        $item->onloan(undef)->store({skip_record_index=>1}) if defined $item->onloan;
 
         # even though item is not on loan, it may still be transferred;  therefore, get current branch info
         $doreturn = 0;
@@ -1915,9 +1916,9 @@ sub AddReturn {
         if (defined $update_loc_rules->{_ALL_}) {
             if ($update_loc_rules->{_ALL_} eq '_PERM_') { $update_loc_rules->{_ALL_} = $item->permanent_location; }
             if ($update_loc_rules->{_ALL_} eq '_BLANK_') { $update_loc_rules->{_ALL_} = ''; }
-            if ( $item->location ne $update_loc_rules->{_ALL_}) {
+            if ( defined $item->location && $item->location ne $update_loc_rules->{_ALL_}) {
                 $messages->{'ItemLocationUpdated'} = { from => $item->location, to => $update_loc_rules->{_ALL_} };
-                $item->location($update_loc_rules->{_ALL_})->store;
+                $item->location($update_loc_rules->{_ALL_})->store({skip_record_index=>1});
             }
         }
         else {
@@ -1926,7 +1927,7 @@ sub AddReturn {
                 if ( $update_loc_rules->{$key} eq '_BLANK_') { $update_loc_rules->{$key} = '' ;}
                 if ( ($item->location eq $key && $item->location ne $update_loc_rules->{$key}) || ($key eq '_BLANK_' && $item->location eq '' && $update_loc_rules->{$key} ne '') ) {
                     $messages->{'ItemLocationUpdated'} = { from => $item->location, to => $update_loc_rules->{$key} };
-                    $item->location($update_loc_rules->{$key})->store;
+                    $item->location($update_loc_rules->{$key})->store({skip_record_index=>1});
                     last;
                 }
             }
@@ -1945,7 +1946,7 @@ sub AddReturn {
             foreach my $key ( keys %$rules ) {
                 if ( $item->notforloan eq $key ) {
                     $messages->{'NotForLoanStatusUpdated'} = { from => $item->notforloan, to => $rules->{$key} };
-                    $item->notforloan($rules->{$key})->store({ log_action => 0 });
+                    $item->notforloan($rules->{$key})->store({ log_action => 0, skip_record_index => 1 });
                     last;
                 }
             }
@@ -1959,7 +1960,8 @@ sub AddReturn {
             Wrongbranch => $branch,
             Rightbranch => $message
         };
-        $doreturn = 0;
+        my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
+        $indexer->index_records( $item->biblionumber, "specialUpdate", "biblioserver" );
         return ( $doreturn, $messages, $issue, $patron_unblessed);
     }
 
@@ -1979,7 +1981,7 @@ sub AddReturn {
 
         if ($patron) {
             eval {
-                MarkIssueReturned( $borrowernumber, $item->itemnumber, $return_date, $patron->privacy );
+                MarkIssueReturned( $borrowernumber, $item->itemnumber, $return_date, $patron->privacy, { skip_record_index => 1} );
             };
             unless ( $@ ) {
                 if (
@@ -1995,26 +1997,29 @@ sub AddReturn {
             } else {
                 carp "The checkin for the following issue failed, Please go to the about page, section 'data corrupted' to know how to fix this problem ($@)" . Dumper( $issue->unblessed );
 
+                my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
+                $indexer->index_records( $item->biblionumber, "specialUpdate", "biblioserver" );
+
                 return ( 0, { WasReturned => 0, DataCorrupted => 1 }, $issue, $patron_unblessed );
             }
 
             # FIXME is the "= 1" right?  This could be the borrower hash.
             $messages->{'WasReturned'} = 1;
 
+        } else {
+            $item->onloan(undef)->store({ log_action => 0 , skip_record_index => 1 });
         }
-
-        $item->onloan(undef)->store({ log_action => 0 });
     }
 
     # the holdingbranch is updated if the document is returned to another location.
     # this is always done regardless of whether the item was on loan or not
     if ($item->holdingbranch ne $branch) {
-        $item->holdingbranch($branch)->store;
+        $item->holdingbranch($branch)->store({ skip_record_index => 1 });
     }
 
     my $item_was_lost = $item->itemlost;
     my $leave_item_lost = C4::Context->preference("BlockReturnOfLostItems") ? 1 : 0;
-    my $updated_item = ModDateLastSeen( $item->itemnumber, $leave_item_lost ); # will unset itemlost if needed
+    my $updated_item = ModDateLastSeen( $item->itemnumber, $leave_item_lost, { skip_record_index => 1 } ); # will unset itemlost if needed
 
     # fix up the accounts.....
     if ( $item_was_lost ) {
@@ -2139,7 +2144,7 @@ sub AddReturn {
            )) {
             $debug and warn sprintf "about to call ModItemTransfer(%s, %s, %s, %s)", $item->itemnumber,$branch, $returnbranch, $transfer_trigger;
             $debug and warn "item: " . Dumper($item->unblessed);
-            ModItemTransfer($item->itemnumber, $branch, $returnbranch, $transfer_trigger);
+            ModItemTransfer($item->itemnumber, $branch, $returnbranch, $transfer_trigger, { skip_record_index => 1 });
             $messages->{'WasTransfered'} = 1;
         } else {
             $messages->{'NeedsTransfer'} = $returnbranch;
@@ -2160,6 +2165,9 @@ sub AddReturn {
         }
     }
 
+    my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
+    $indexer->index_records( $item->biblionumber, "specialUpdate", "biblioserver" );
+
     if ( $doreturn and $issue ) {
         my $checkin = Koha::Old::Checkouts->find($issue->id);
 
@@ -2176,7 +2184,7 @@ sub AddReturn {
 
 =head2 MarkIssueReturned
 
-  MarkIssueReturned($borrowernumber, $itemnumber, $returndate, $privacy);
+  MarkIssueReturned($borrowernumber, $itemnumber, $returndate, $privacy, [$params] );
 
 Unconditionally marks an issue as being returned by
 moving the C<issues> row to C<old_issues> and
@@ -2192,10 +2200,12 @@ Ideally, this function would be internal to C<C4::Circulation>,
 not exported, but it is currently used in misc/cronjobs/longoverdue.pl
 and offline_circ/process_koc.pl.
 
+The last optional parameter allos passing skip_record_index to the item store call.
+
 =cut
 
 sub MarkIssueReturned {
-    my ( $borrowernumber, $itemnumber, $returndate, $privacy ) = @_;
+    my ( $borrowernumber, $itemnumber, $returndate, $privacy, $params ) = @_;
 
     # Retrieve the issue
     my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return;
@@ -2241,7 +2251,7 @@ sub MarkIssueReturned {
         # And finally delete the issue
         $issue->delete;
 
-        $issue->item->onloan(undef)->store({ log_action => 0 });
+        $issue->item->onloan(undef)->store({ log_action => 0, skip_record_index => $params->{skip_record_index} });
 
         if ( C4::Context->preference('StoreLastBorrower') ) {
             my $item = Koha::Items->find( $itemnumber );
diff --git a/C4/Items.pm b/C4/Items.pm
index 9da7f61fc6..b1e8de7029 100644
--- a/C4/Items.pm
+++ b/C4/Items.pm
@@ -334,14 +334,16 @@ sub ModItemFromMarc {
 
 =head2 ModItemTransfer
 
-  ModItemTransfer($itemnumber, $frombranch, $tobranch, $trigger);
+  ModItemTransfer($itemnumber, $frombranch, $tobranch, $trigger, [$params]);
 
 Marks an item as being transferred from one branch to another and records the trigger.
 
+The last optional parameter allows for passing skip_record_index through to the items store call.
+
 =cut
 
 sub ModItemTransfer {
-    my ( $itemnumber, $frombranch, $tobranch, $trigger ) = @_;
+    my ( $itemnumber, $frombranch, $tobranch, $trigger, $params ) = @_;
 
     my $dbh = C4::Context->dbh;
     my $item = Koha::Items->find( $itemnumber );
@@ -358,30 +360,32 @@ sub ModItemTransfer {
     $sth->execute($itemnumber, $frombranch, $tobranch, $trigger);
 
     # FIXME we are fetching the item twice in the 2 next statements!
-    Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store({ log_action => 0 });
-    ModDateLastSeen($itemnumber);
+    Koha::Items->find($itemnumber)->holdingbranch($frombranch)->store({ log_action => 0, skip_record_index => $params->{skip_record_index} });
+    ModDateLastSeen($itemnumber, undef, { skip_record_index => $params->{skip_record_index} });
     return;
 }
 
 =head2 ModDateLastSeen
 
-ModDateLastSeen( $itemnumber, $leave_item_lost );
+ModDateLastSeen( $itemnumber, $leave_item_lost, $params );
 
 Mark item as seen. Is called when an item is issued, returned or manually marked during inventory/stocktaking.
 C<$itemnumber> is the item number
 C<$leave_item_lost> determines if a lost item will be found or remain lost
 
+The last optional parameter allows for passing skip_record_index through to the items store call.
+
 =cut
 
 sub ModDateLastSeen {
-    my ( $itemnumber, $leave_item_lost ) = @_;
+    my ( $itemnumber, $leave_item_lost, $params ) = @_;
 
     my $today = output_pref({ dt => dt_from_string, dateformat => 'iso', dateonly => 1 });
 
     my $item = Koha::Items->find($itemnumber);
     $item->datelastseen($today);
     $item->itemlost(0) unless $leave_item_lost;
-    $item->store({ log_action => 0 });
+    $item->store({ log_action => 0, skip_record_index => $params->{skip_record_index} });
 }
 
 =head2 CheckItemPreSave
-- 
2.11.0