Bugzilla – Attachment 112563 Details for
Bug 26581
Elasticsearch - Records can be indexed multiple times during returns
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 26581: [20.05.x] Only reindex records once per checkin
Bug-26581-2005x-Only-reindex-records-once-per-chec.patch (text/plain), 16.19 KB, created by
Nick Clemens (kidclamp)
on 2020-10-27 11:53:19 UTC
(
hide
)
Description:
Bug 26581: [20.05.x] Only reindex records once per checkin
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2020-10-27 11:53:19 UTC
Size:
16.19 KB
patch
obsolete
>From 53353402f6157d45e08f87189f896f1026b1a3d7 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >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=UTF-8 >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 <severine.queune@bulac.fr> >Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >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 <severine.queune@bulac.fr> >Signed-off-by: Christoper Brannon <cbrannon@cdalibrary.org> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > 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 23a609d05e..73805a2f0a 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 ); >@@ -1880,7 +1881,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; >@@ -1908,9 +1909,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 { >@@ -1919,7 +1920,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; > } > } >@@ -1938,7 +1939,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; > } > } >@@ -1952,7 +1953,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); > } > >@@ -1972,7 +1974,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 ( >@@ -1988,26 +1990,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 ); >@@ -2144,7 +2149,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; >@@ -2165,12 +2170,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<issues> row to C<old_issues> and >@@ -2186,10 +2194,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; >@@ -2235,7 +2245,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.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 26581
:
111001
|
111002
|
111596
|
111597
|
111878
|
111879
| 112563 |
112578
|
112829