From f18deb345820f846cde4445c274c746a4c650e93 Mon Sep 17 00:00:00 2001 From: Ere Maijala Date: Mon, 16 Sep 2019 13:55:37 +0300 Subject: [PATCH] Bug 22690: Refactor merging of records to improve performance (Elasticsearch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch allows merging of records with many items without the web server timing out. Test plan: Without the patch: - Create 2 records (one with e.g. 1000 items). - Do a cataloguing search that displays both records, select them and click "Merge selected". - Choose the record with many items as the one to be eliminated. - Start the merging. - After a while the web server should give you a timeout error (the merging process may still continue) With the patch: - Do the same as above - This time verify that the records are merged without timeout - Create a new biblio with an item - Add with the item: * acquisition order * hold (reserve) - Merge the biblio to another one - Verify that the item and its related data was moved - Verify that tests pass: prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Items/MoveItemFromBiblio.t Rebased-by: Joonas Kylmälä --- C4/Items.pm | 55 ++++------- Koha/Biblio.pm | 24 +++++ Koha/Item.pm | 63 +++++++++++++ Koha/Schema/Result/Item.pm | 13 +++ cataloguing/merge.pl | 22 ++--- t/db_dependent/Items/MoveItemFromBiblio.t | 3 + t/db_dependent/Koha/Item.t | 108 +++++++++++++++++++++- 7 files changed, 233 insertions(+), 55 deletions(-) diff --git a/C4/Items.pm b/C4/Items.pm index 9da7f61fc6..9a6ceb2050 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -66,6 +66,7 @@ use Koha::AuthorisedValues; use Koha::DateUtils qw(dt_from_string); use Koha::Database; +use Koha::Biblios; use Koha::Biblioitems; use Koha::Items; use Koha::ItemTypes; @@ -1084,7 +1085,7 @@ the inner workings of C. =head2 MoveItemFromBiblio - MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio); + MoveItemFromBiblio($itemnumber, $frombiblio, $tobiblio); Moves an item from a biblio to another @@ -1094,43 +1095,21 @@ Returns undef if the move failed or the biblionumber of the destination record o sub MoveItemFromBiblio { my ($itemnumber, $frombiblio, $tobiblio) = @_; - my $dbh = C4::Context->dbh; - my ( $tobiblioitem ) = $dbh->selectrow_array(q| - SELECT biblioitemnumber - FROM biblioitems - WHERE biblionumber = ? - |, undef, $tobiblio ); - my $return = $dbh->do(q| - UPDATE items - SET biblioitemnumber = ?, - biblionumber = ? - WHERE itemnumber = ? - AND biblionumber = ? - |, undef, $tobiblioitem, $tobiblio, $itemnumber, $frombiblio ); - if ($return == 1) { - 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); - if ($order) { - # Replacing the biblionumber within the order if necessary - $order->{'biblionumber'} = $tobiblio; - C4::Acquisition::ModOrder($order); - } - - # Update reserves, hold_fill_targets, tmp_holdsqueue and linktracker tables - for my $table_name ( qw( reserves hold_fill_targets tmp_holdsqueue linktracker ) ) { - $dbh->do( qq| - UPDATE $table_name - SET biblionumber = ? - WHERE itemnumber = ? - |, undef, $tobiblio, $itemnumber ); - } - return $tobiblio; - } - return; + + my $item = Koha::Items->find($itemnumber); + return unless $item; + + my $biblio = Koha::Biblios->find($tobiblio); + return unless $biblio; + + return unless $item->biblionumber != $biblio->biblionumber; + + $item->move_to_biblio($biblio); + my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); + $indexer->index_records( $tobiblio, "specialUpdate", "biblioserver" ); + $indexer->index_records( $frombiblio, "specialUpdate", "biblioserver" ); + + return $tobiblio; } =head2 _marc_from_item_hash diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm index 830b9c4d73..ec7c7bdfde 100644 --- a/Koha/Biblio.pm +++ b/Koha/Biblio.pm @@ -41,6 +41,7 @@ use Koha::CirculationRules; use Koha::Item::Transfer::Limits; use Koha::Items; use Koha::Libraries; +use Koha::SearchEngine::Indexer; use Koha::Suggestions; use Koha::Subscriptions; @@ -816,6 +817,29 @@ sub to_api_mapping { }; } +=head3 adopt_items_from_biblio + +$biblio->adopt_items_from_biblio($from_biblio); + +Move items from the given biblio to this one. + +=cut + +sub adopt_items_from_biblio { + my ( $self, $from_biblio ) = @_; + + my $items = $from_biblio->items; + if ($items) { + while (my $item = $items->next()) { + $item->move_to_biblio($self); + } + my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); + $indexer->index_records( $tobiblio, "specialUpdate", "biblioserver" ); + $indexer->index_records( $frombiblio, "specialUpdate", "biblioserver" ); + } +} + + =head2 Internal methods =head3 type diff --git a/Koha/Item.pm b/Koha/Item.pm index 0da3a46f00..95c4eba369 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -968,6 +968,69 @@ sub itemtype { return Koha::ItemTypes->find( $self->effective_itemtype ); } +=head3 item_orders + +my $orders = $item->item_orders(); + +Returns a Koha::Acquisition::Orders object + +=cut + +sub item_orders { + my ( $self ) = @_; + + my $orders = $self->_result->item_orders; + return Koha::Acquisition::Orders->_new_from_dbic($orders); +} + +=head3 move_to_biblio + + $item->move_to_biblio($to_biblio); + +Move the item to another biblio and update any references also in other tables. + +=cut + +sub move_to_biblio { + my ( $self, $to_biblio ) = @_; + + my $biblionumber = $to_biblio->biblionumber; + + # Own biblionumber and biblioitemnumber + $self->set({ + biblionumber => $biblionumber, + biblioitemnumber => $to_biblio->biblioitem->biblioitemnumber + })->store({ skip_modzebra_update => 1 }); + + # Acquisition orders + $self->item_orders->update({ biblionumber => $biblionumber }, { no_triggers => 1 }); + + # Holds + $self->holds->update({ biblionumber => $biblionumber }, { no_triggers => 1 }); + + # hold_fill_target (there's no Koha object available) + my $hold_fill_target = $self->_result->hold_fill_target; + if ($hold_fill_target) { + $hold_fill_target->update({ biblionumber => $biblionumber }); + } + + # tmp_holdsqueues - Can't update with DBIx since the table is missing a primary key + # and can't even fake one since the significant columns are nullable. + my $storage = $self->_result->result_source->storage; + $storage->dbh_do( + sub { + my ($storage, $dbh, @cols) = @_; + + $dbh->do("UPDATE tmp_holdsqueue SET biblionumber=? WHERE itemnumber=?", undef, $biblionumber, $self->itemnumber); + } + ); + + # linktrackers + my $schema = Koha::Database->new()->schema(); + my $linktrackers = $schema->resultset('Linktracker')->search({ itemnumber => $self->itemnumber }); + $linktrackers->update_all({ biblionumber => $biblionumber }); +} + =head2 Internal methods =head3 _after_item_action_hooks diff --git a/Koha/Schema/Result/Item.pm b/Koha/Schema/Result/Item.pm index 7d8b9a6cc6..8138df912b 100644 --- a/Koha/Schema/Result/Item.pm +++ b/Koha/Schema/Result/Item.pm @@ -770,6 +770,19 @@ __PACKAGE__->add_columns( '+exclude_from_local_holds_priority' => { is_boolean => 1 }, ); +# Relationship with orders via the aqorders_item table that not have foreign keys +__PACKAGE__->has_many( + "aqorders_item", + "Koha::Schema::Result::AqordersItem", + { "foreign.itemnumber" => "self.itemnumber" }, + { cascade_copy => 0, cascade_delete => 0 }, +); +__PACKAGE__->many_to_many( + "item_orders", + "aqorders_item", + "ordernumber", +); + use C4::Context; sub effective_itemtype { my ( $self ) = @_; diff --git a/cataloguing/merge.pl b/cataloguing/merge.pl index baf2b3057a..b0a97aa8b5 100755 --- a/cataloguing/merge.pl +++ b/cataloguing/merge.pl @@ -31,6 +31,7 @@ use C4::Reserves qw/MergeHolds/; use C4::Acquisition qw/ModOrder GetOrdersByBiblionumber/; use Koha::BiblioFrameworks; +use Koha::Biblios; use Koha::Items; use Koha::MetadataRecord; @@ -80,25 +81,16 @@ if ($merge) { $record->leader(GetMarcBiblio({ biblionumber => $ref_biblionumber })->leader()); my $frameworkcode = $input->param('frameworkcode'); - my @notmoveditems; # Modifying the reference record ModBiblio($record, $ref_biblionumber, $frameworkcode); - # Moving items from the other record to the reference record + # Moving items and article requests from the other record to the reference record + my $biblio = Koha::Biblios->find($ref_biblionumber); foreach my $biblionumber (@biblionumbers) { - my $items = Koha::Items->search({ biblionumber => $biblionumber }); - while ( my $item = $items->next) { - my $res = MoveItemFromBiblio( $item->itemnumber, $biblionumber, $ref_biblionumber ); - if ( not defined $res ) { - push @notmoveditems, $item->itemnumber; - } - } - } - # If some items could not be moved : - if (scalar(@notmoveditems) > 0) { - my $itemlist = join(' ',@notmoveditems); - push @errors, { code => "CANNOT_MOVE", value => $itemlist }; + my $from_biblio = Koha::Biblios->find($biblionumber); + $biblio->adopt_items_from_biblio($from_biblio); + $from_biblio->article_requests->update({ biblionumber => $ref_biblionumber }, { no_triggers => 1 }); } my $sth_subscription = $dbh->prepare(" @@ -162,7 +154,7 @@ if ($merge) { # Moving suggestions $sth_suggestions->execute($ref_biblionumber, $biblionumber); - # Moving orders (orders linked to items of frombiblio have already been moved by MoveItemFromBiblio) + # Moving orders (orders linked to items of frombiblio have already been moved by adopt_items_from_biblio) my @allorders = GetOrdersByBiblionumber($biblionumber); foreach my $myorder (@allorders) { $myorder->{'biblionumber'} = $ref_biblionumber; diff --git a/t/db_dependent/Items/MoveItemFromBiblio.t b/t/db_dependent/Items/MoveItemFromBiblio.t index 75ae92afe0..137806ebd1 100755 --- a/t/db_dependent/Items/MoveItemFromBiblio.t +++ b/t/db_dependent/Items/MoveItemFromBiblio.t @@ -25,12 +25,15 @@ use Koha::Holds; use Koha::Items; use t::lib::TestBuilder; +use t::lib::Mocks; use Data::Dumper qw|Dumper|; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; my $builder = t::lib::TestBuilder->new; +t::lib::Mocks::mock_preference( 'SearchEngine', 'Zebra' ); + my $from_biblio = $builder->build_sample_biblio; my $to_biblio = $builder->build_sample_biblio; diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index 0309fa94f6..ccb064acf2 100755 --- a/t/db_dependent/Koha/Item.t +++ b/t/db_dependent/Koha/Item.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 7; +use Test::More tests => 8; use C4::Biblio; use C4::Circulation; @@ -163,8 +163,8 @@ subtest 'pickup_locations' => sub { my $dbh = C4::Context->dbh; # Cleanup database - Koha::Holds->search->delete; $dbh->do('DELETE FROM issues'); + Koha::Holds->search->delete; Koha::Patrons->search->delete; Koha::Items->search->delete; Koha::Libraries->search->delete; @@ -516,3 +516,107 @@ subtest 'Tests for itemtype' => sub { $schema->storage->txn_rollback; }; + +subtest 'move_to_biblio() tests' => sub { + + plan tests => 12; + + $schema->storage->txn_begin; + + my $dbh = C4::Context->dbh; + + my $source_biblio = $builder->build_sample_biblio(); + my $target_biblio = $builder->build_sample_biblio(); + + my $source_biblionumber = $source_biblio->biblionumber; + + my $item1 = $builder->build_sample_item({ biblionumber => $source_biblionumber }); + my $item2 = $builder->build_sample_item({ biblionumber => $source_biblionumber }); + + my $itemnumber1 = $item1->itemnumber; + my $itemnumber2 = $item2->itemnumber; + + my $library = $builder->build_object({ class => 'Koha::Libraries' }); + + my $patron = $builder->build_object({ + class => 'Koha::Patrons', + value => { branchcode => $library->branchcode, } + }); + my $borrowernumber = $patron->borrowernumber; + + $dbh->do("INSERT INTO aqbudgets (budget_notes) VALUES ('test')"); + my $budget_id = $dbh->last_insert_id(undef, undef, 'aqbudgets', 'budget_id'); + + $dbh->do('INSERT INTO aqorders (biblionumber, budget_id) VALUES (?, ?)', undef, $source_biblionumber, $budget_id); + my $ordernumber1 = $dbh->last_insert_id(undef, undef, 'aqorders', 'ordernumber'); + $dbh->do('INSERT INTO aqorders_items (ordernumber, itemnumber) VALUES (?, ?)', undef, $ordernumber1, $itemnumber1); + + $dbh->do('INSERT INTO aqorders (biblionumber, budget_id) VALUES (?, ?)', undef, $source_biblionumber, $budget_id); + my $ordernumber2 = $dbh->last_insert_id(undef, undef, 'aqorders', 'ordernumber'); + $dbh->do('INSERT INTO aqorders_items (ordernumber, itemnumber) VALUES (?, ?)', undef, $ordernumber2, $source_biblionumber); + + my $reserve_id1 = C4::Reserves::AddReserve({ + branch => $library->branchcode, + borrowernumber => $borrowernumber, + biblionumber => $source_biblionumber, + itemnumber => $itemnumber1, + title => 'title' + }); + + my $reserve_id2 = C4::Reserves::AddReserve({ + branch => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $source_biblionumber, + itemnumber => $itemnumber2, + title => 'title' + }); + + $dbh->do("INSERT INTO tmp_holdsqueue (surname,borrowernumber,itemnumber,biblionumber) VALUES ('Clamp',?,?,?)", undef, $borrowernumber, $itemnumber1, $source_biblionumber); + $dbh->do("INSERT INTO tmp_holdsqueue (surname,borrowernumber,itemnumber,biblionumber) VALUES ('Clamp',?,?,?)", undef, $borrowernumber, $itemnumber2, $source_biblionumber); + + $dbh->do('INSERT INTO hold_fill_targets (borrowernumber,itemnumber,biblionumber) VALUES (?,?,?)', undef, $borrowernumber, $itemnumber1, $source_biblionumber); + $dbh->do('INSERT INTO hold_fill_targets (borrowernumber,itemnumber,biblionumber) VALUES (?,?,?)', undef, $borrowernumber, $itemnumber2, $source_biblionumber); + + $dbh->do('INSERT INTO linktracker (borrowernumber,itemnumber,biblionumber) VALUES (?,?,?)', undef, $borrowernumber, $itemnumber1, $source_biblionumber); + $dbh->do('INSERT INTO linktracker (borrowernumber,itemnumber,biblionumber) VALUES (?,?,?)', undef, $borrowernumber, $itemnumber2, $source_biblionumber); + + $item1->move_to_biblio($target_biblio); + + my @result = $dbh->selectrow_array('SELECT biblionumber FROM items WHERE itemnumber = ?', undef, $itemnumber1); + is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in the moved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM items WHERE itemnumber = ?', undef, $itemnumber2); + is($result[0], $source_biblionumber, 'Correct biblionumber in the unmoved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM aqorders WHERE ordernumber = ?', undef, $ordernumber1); + is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in aqorders for order with the moved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM aqorders WHERE ordernumber = ?', undef, $ordernumber2); + is($result[0], $source_biblionumber, 'Correct biblionumber in aqorders for order with the unmoved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM reserves WHERE reserve_id = ?', undef, $reserve_id1); + is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in reserves for moved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM reserves WHERE reserve_id = ?', undef, $reserve_id2); + is($result[0], $source_biblionumber, 'Correct biblionumber in reserves for unmoved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM tmp_holdsqueue WHERE itemnumber = ?', undef, $itemnumber1); + is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in tmp_holdsqueue for moved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM tmp_holdsqueue WHERE itemnumber = ?', undef, $itemnumber2); + is($result[0], $source_biblionumber, 'Correct biblionumber in tmp_holdsqueue for unmoved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM hold_fill_targets WHERE itemnumber = ?', undef, $itemnumber1); + is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in hold_fill_targets for moved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM hold_fill_targets WHERE itemnumber = ?', undef, $itemnumber2); + is($result[0], $source_biblionumber, 'Correct biblionumber in hold_fill_targets for unmoved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM linktracker WHERE itemnumber = ?', undef, $itemnumber1); + is($result[0], $target_biblio->biblionumber, 'Correct biblionumber in linktracker for moved item'); + + @result = $dbh->selectrow_array('SELECT biblionumber FROM linktracker WHERE itemnumber = ?', undef, $itemnumber2); + is($result[0], $source_biblionumber, 'Correct biblionumber in linktracker for unmoved item'); + + $schema->storage->txn_rollback; +}; -- 2.17.1