From 17a839ca65316d8d981412b9e29af7681c2b8dd7 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)
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ä <joonas.kylmala@helsinki.fi>
---
 C4/Items.pm                               |  53 +++++----------
 Koha/Biblio.pm                            |  22 ++++++
 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, 230 insertions(+), 54 deletions(-)

diff --git a/C4/Items.pm b/C4/Items.pm
index 22ac2de12c..44359a0e77 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;
@@ -1070,7 +1071,7 @@ the inner workings of C<C4::Items>.
 
 =head2 MoveItemFromBiblio
 
-  MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio);
+  MoveItemFromBiblio($itemnumber, $frombiblio, $tobiblio);
 
 Moves an item from a biblio to another
 
@@ -1080,42 +1081,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 _marc_from_item_hash
diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm
index 830b9c4d73..6f32922b39 100644
--- a/Koha/Biblio.pm
+++ b/Koha/Biblio.pm
@@ -816,6 +816,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 84ba2da33c..46e4fe0e0c 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -965,6 +965,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 cb77d96ee0..3ae05fec16 100644
--- a/Koha/Schema/Result/Item.pm
+++ b/Koha/Schema/Result/Item.pm
@@ -759,6 +759,19 @@ __PACKAGE__->belongs_to(
   { is_deferrable => 1, on_delete => "CASCADE", on_update => "CASCADE" },
 );
 
+# 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 146e88e598..558032f578 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,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("
@@ -163,7 +155,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 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 44cd0b8e2b..5b76f54c0e 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 => 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;
@@ -518,3 +518,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.11.0