From 8a5cda8b7ad9af8fcbaa9d5a850946000066a281 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 16 Oct 2020 15:04:31 +0100 Subject: [PATCH] Bug 26581: Only reindex records once per checkin MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Séverine Queune Signed-off-by: Christopher Brannon Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart --- 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 29b3347666..18c4dacda5 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 ); @@ -1913,7 +1914,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; @@ -1941,9 +1942,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 { @@ -1952,7 +1953,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; } } @@ -1971,7 +1972,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; } } @@ -1985,7 +1986,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); } @@ -2005,7 +2007,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 ( @@ -2021,26 +2023,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 ) { @@ -2166,7 +2171,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; @@ -2187,6 +2192,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); @@ -2203,7 +2211,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 row to C and @@ -2219,10 +2227,12 @@ Ideally, this function would be internal to C, 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; @@ -2268,7 +2278,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.39.5