From e5367af11159b33f43c0cef0968f204366dbad99 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 16 Oct 2020 15:04:31 +0100 Subject: [PATCH] Bug 26581: [20.05.x] 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: Christoper Brannon Signed-off-by: Martin Renvoize Bug 26581: Unit tests These tests cover the changes to ensure the AddReturn calls index_records once per call and that other calls pass the skip_record_index parameter correctly Signed-off-by: Séverine Queune Signed-off-by: Christoper Brannon Signed-off-by: Martin Renvoize Signed-off-by: Lucas Gass --- C4/Circulation.pm | 40 ++++++++----- C4/Items.pm | 18 +++--- t/db_dependent/Koha/SearchEngine/Indexer.t | 66 ++++++++++++++++++++-- 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 8edae54132..94fb4165ca 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -59,6 +59,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 ); @@ -1874,7 +1875,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; @@ -1902,9 +1903,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 { @@ -1913,7 +1914,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; } } @@ -1932,7 +1933,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; } } @@ -1946,7 +1947,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); } @@ -1966,7 +1968,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 ( @@ -1982,26 +1984,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 my $item_holding_branch = $item->holdingbranch; if ($item->holdingbranch ne $branch) { - $item->holdingbranch($branch)->store; + $item->holdingbranch($branch)->store({ skip_record_index => 1 }); } my $leave_item_lost = C4::Context->preference("BlockReturnOfLostItems") ? 1 : 0; - ModDateLastSeen( $item->itemnumber, $leave_item_lost ); + ModDateLastSeen( $item->itemnumber, $leave_item_lost, { skip_record_index => 1 } ); # check if we have a transfer for this document my ($datesent,$frombranch,$tobranch) = GetTransfers( $item->itemnumber ); @@ -2138,7 +2143,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; @@ -2159,12 +2164,15 @@ sub AddReturn { } } + my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); + $indexer->index_records( $item->biblionumber, "specialUpdate", "biblioserver" ); + return ( $doreturn, $messages, $issue, ( $patron ? $patron->unblessed : {} )); } =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 @@ -2180,10 +2188,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; @@ -2229,7 +2239,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 24a8ee1cc4..3485d40dc2 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 diff --git a/t/db_dependent/Koha/SearchEngine/Indexer.t b/t/db_dependent/Koha/SearchEngine/Indexer.t index 4515d69f8d..997f6f17ca 100755 --- a/t/db_dependent/Koha/SearchEngine/Indexer.t +++ b/t/db_dependent/Koha/SearchEngine/Indexer.t @@ -18,7 +18,9 @@ use t::lib::Mocks; use C4::AuthoritiesMarc; use C4::Biblio; use C4::Circulation; +use C4::Items; use Koha::Database; +use Koha::SearchEngine::Elasticsearch; use Koha::SearchEngine::Indexer; use t::lib::TestBuilder; @@ -58,13 +60,13 @@ subtest 'Test indexer object creation' => sub { }; subtest 'Test indexer calls' => sub { - plan tests => 24; + plan tests => 40; my @engines = ('Zebra'); eval { Koha::SearchEngine::Elasticsearch->get_elasticsearch_params; }; push @engines, 'Elasticsearch' unless $@; SKIP: { - skip 'Elasticsearch configuration not available', 12 + skip 'Elasticsearch configuration not available', 20 if scalar @engines == 1; } @@ -109,13 +111,53 @@ subtest 'Test indexer calls' => sub { my $item; my $item2; warnings_are{ - $item = $builder->build_sample_item({biblionumber => $biblio->biblionumber}); - $item2 = $builder->build_sample_item({biblionumber => $biblio->biblionumber}); + $item = $builder->build_sample_item({ + biblionumber => $biblio->biblionumber, + onloan => '2020-02-02', + datelastseen => '2020-01-01' + }); + $item2 = $builder->build_sample_item({ + biblionumber => $biblio->biblionumber, + onloan => '2020-12-12', + datelastseen => '2020-11-11' + }); } [$engine,"Koha::Item",$engine,"Koha::Item"], "index_records is called for $engine when adding an item (Item->store)"; warnings_are{ $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"; + my $issue = $builder->build({ + source => 'Issue', + value => { + itemnumber => $item->itemnumber + } + }); + my $issue2 = $builder->build({ + source => 'Issue', + value => { + itemnumber => $item2->itemnumber + } + }); + warnings_are{ + MarkIssueReturned( $issue->{borrowernumber}, $item->itemnumber); + } [$engine,"Koha::Item"], "index_records is called for $engine when calling MarkIssueReturned"; + warnings_are{ + MarkIssueReturned( $issue2->{borrowernumber}, $item2->itemnumber, undef, undef, { skip_record_index => 1}); + } undef, "index_records is not called for $engine when calling MarkIssueReturned if skip_record_index passed"; + + warnings_are{ + AddReturn($item->barcode, $item->homebranch, 0, undef); + } [$engine,'C4::Circulation'], "index_records is called once for $engine when calling AddReturn if item not issued"; + $issue = $builder->build({ + source => 'Issue', + value => { + itemnumber => $item->itemnumber + } + }); + warnings_are{ + AddReturn($item->barcode, $item->homebranch, 0, undef); + } [$engine,'C4::Circulation'], "index_records is called once for $engine when calling AddReturn if item not issued"; + $builder->build({ source => 'Branchtransfer', value => { @@ -135,6 +177,22 @@ subtest 'Test indexer calls' => sub { 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"; + $item->datelastseen('2020-02-02'); + $item->store({skip_record_index=>1}); + warnings_are{ + my $t1 = ModDateLastSeen( $item->itemnumber, 1, undef ); + } [$engine, "Koha::Item"], "index_records is called for $engine when calling ModDateLastSeen"; + warnings_are{ + ModDateLastSeen( $item->itemnumber, 1, { skip_record_index =>1 } ); + } undef, "index_records is not called for $engine when calling ModDateLastSeen if skip_record_index"; + + warnings_are{ + ModItemTransfer( $item->itemnumber, $item->homebranch, $item2->homebranch,'Manual'); + } [$engine,"Koha::Item"], "index_records is called for $engine when calling ModItemTransfer"; + warnings_are{ + ModItemTransfer( $item->itemnumber, $item2->homebranch, $item->homebranch,'Manual',{skip_record_index=>1}); + } undef, "index_records is not called for $engine when calling ModItemTransfer with skip_record_index"; + warnings_are{ $item->delete(); } [$engine,"Koha::Item"], "index_records is called for $engine when deleting an item (Item->delete)"; -- 2.39.5