From 400fc00a744752772b2927aee815b8c6a828e2da Mon Sep 17 00:00:00 2001
From: Ere Maijala <ere.maijala@helsinki.fi>
Date: Mon, 16 Sep 2019 13:55:37 +0300
Subject: [PATCH] Bug 22690: Refactor merging of records to improve performance
(Elasticsearch)
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
---
C4/Items.pm | 53 ++++-------
Koha/Biblio.pm | 21 +++++
Koha/Item.pm | 56 ++++++++++++
cataloguing/merge.pl | 25 +++--
t/db_dependent/Items/MoveItemFromBiblio.t | 3 +
t/db_dependent/Koha/Item.t | 106 +++++++++++++++++++++-
6 files changed, 212 insertions(+), 52 deletions(-)
diff --git a/C4/Items.pm b/C4/Items.pm
index cca700258d..f1479c058a 100644
--- a/C4/Items.pm
+++ b/C4/Items.pm
@@ -73,6 +73,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;
@@ -1650,7 +1651,7 @@ sub _koha_new_item {
=head2 MoveItemFromBiblio
- MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio);
+ MoveItemFromBiblio($itemnumber, $frombiblio, $tobiblio);
Moves an item from a biblio to another
@@ -1660,42 +1661,20 @@ 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) {
- ModZebra( $tobiblio, "specialUpdate", "biblioserver" );
- ModZebra( $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);
+ ModZebra($tobiblio, 'specialUpdate', 'biblioserver');
+ ModZebra($frombiblio, 'specialUpdate', 'biblioserver');
+
+ return $tobiblio;
}
=head2 ItemSafeToDelete
diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm
index 1de0c6f417..8c6d5a7a65 100644
--- a/Koha/Biblio.pm
+++ b/Koha/Biblio.pm
@@ -659,6 +659,27 @@ sub is_serial {
return 0;
}
+=head3 move_items_from_biblio
+
+$biblio->move_items_from_biblio($from_biblio);
+
+Move items from the given biblio
+
+=cut
+
+sub move_items_from_biblio {
+ my ( $self, $from_biblio ) = @_;
+
+ my $items = $from_biblio->items;
+ if ($items) {
+ while (my $item = $items->next()) {
+ $item->move_to_biblio($self);
+ }
+ C4::Biblio::ModZebra( $self->biblionumber, "specialUpdate", "biblioserver" );
+ C4::Biblio::ModZebra( $from_biblio->biblionumber, "specialUpdate", "biblioserver" );
+ }
+}
+
=head3 type
=cut
diff --git a/Koha/Item.pm b/Koha/Item.pm
index 2b2eb5fffc..6419c64ebf 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -429,6 +429,62 @@ sub as_marc_field {
return $field;
}
+=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();
+
+ my $schema = Koha::Database->new()->schema();
+
+ # Acquisition orders related to the item
+ my $orders = $schema->resultset('Aqorder')->search(
+ { itemnumber => $self->itemnumber },
+ { join => 'aqorders_items'}
+ );
+ $orders->update_all({ biblionumber => $biblionumber });
+
+ # reserves
+ my $reserves = $self->_result->reserves;
+ $reserves->update_all({ biblionumber => $biblionumber });
+
+ # hold_fill_target
+ 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);
+ }
+ );
+ #my $tmp_holdsqueues = $self->_result->tmp_holdsqueues;
+ #$tmp_holdsqueues->update_all({ biblionumber => $biblionumber });
+
+ # linktrackers
+ my $linktrackers = $schema->resultset('Linktracker')->search({ itemnumber => $self->itemnumber });
+ $linktrackers->update_all({ biblionumber => $biblionumber });
+}
+
=head2 Internal methods
=head3 _type
diff --git a/cataloguing/merge.pl b/cataloguing/merge.pl
index 146e88e598..1dac64f0d3 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;
@@ -81,25 +82,15 @@ 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
+ 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->move_items_from_biblio($from_biblio);
}
my $sth_subscription = $dbh->prepare("
@@ -114,6 +105,9 @@ if ($merge) {
my $sth_suggestions = $dbh->prepare("
UPDATE suggestions SET biblionumber = ? WHERE biblionumber = ?
");
+ my $sth_articlerequests = $dbh->prepare("
+ UPDATE article_requests SET biblionumber = ? WHERE biblionumber = ?
+ ");
my $report_header = {};
foreach my $biblionumber ($ref_biblionumber, @biblionumbers) {
@@ -163,7 +157,10 @@ 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 article requests
+ $sth_articlerequests->execute($ref_biblionumber, $biblionumber);
+
+ # Moving orders (orders linked to items of frombiblio have already been moved by move_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 fec7e51442..956d4cee01 100644
--- 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' );
+
# NOTE This is a trick, if we want to populate the biblioitems table, we should not create a Biblio but a Biblioitem
my $param = { source => 'Biblioitem' };
my $from_biblio = {
diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t
index b9ff871097..b0125671c6 100644
--- 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 => 3;
+use Test::More tests => 4;
use C4::Biblio;
@@ -148,3 +148,107 @@ subtest "as_marc_field() tests" => sub {
$schema->storage->txn_rollback;
};
+
+subtest 'move_items_from_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(
+ $library->branchcode, $borrowernumber,
+ $source_biblionumber, '',
+ 1, undef,
+ undef, '',
+ "title", $itemnumber1,
+ );
+
+ my $reserve_id2 = C4::Reserves::AddReserve(
+ $library->branchcode, $patron->borrowernumber,
+ $source_biblionumber, '',
+ 1, undef,
+ undef, '',
+ "title", $itemnumber2,
+ );
+
+ $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