From 913c44720776c9eb75295b7c7668eb173e7f541e 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

Signed-off-by: Michal Denar <black23@gmail.com>
---
 C4/Items.pm                               |  53 ++++-------
 Koha/Biblio.pm                            |  22 +++++
 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, 213 insertions(+), 52 deletions(-)

diff --git a/C4/Items.pm b/C4/Items.pm
index 925afc2397..6b7f76ceec 100644
--- a/C4/Items.pm
+++ b/C4/Items.pm
@@ -72,6 +72,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;
@@ -1648,7 +1649,7 @@ sub _koha_new_item {
 
 =head2 MoveItemFromBiblio
 
-  MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio);
+  MoveItemFromBiblio($itemnumber, $frombiblio, $tobiblio);
 
 Moves an item from a biblio to another
 
@@ -1658,42 +1659,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 9c0b6e8762..9962a16eaf 100644
--- a/Koha/Biblio.pm
+++ b/Koha/Biblio.pm
@@ -847,6 +847,28 @@ 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);
+        }
+        C4::Biblio::ModZebra( $self->biblionumber, "specialUpdate", "biblioserver" );
+        C4::Biblio::ModZebra( $from_biblio->biblionumber, "specialUpdate", "biblioserver" );
+    }
+}
+
+
 =head2 Internal methods
 
 =head3 type
diff --git a/Koha/Item.pm b/Koha/Item.pm
index e77f8191c4..cf01c8332c 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -540,6 +540,62 @@ sub to_api_mapping {
     };
 }
 
+=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..c95c49424d 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->adopt_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 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 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 459c59fc3a..703623750a 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 => 4;
+use Test::More tests => 5;
 
 use C4::Biblio;
 
@@ -330,3 +330,107 @@ subtest 'pickup_locations' => 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(
+        $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