@@ -, +, @@ (Elasticsearch) - 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) - 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/Biblio.t prove -v t/db_dependent/Koha/Item.t prove -v t/db_dependent/Koha/SearchEngine/Indexer.t --- C4/Items.pm | 53 +-------- Koha/Biblio.pm | 24 ++++ Koha/Item.pm | 84 +++++++++++++ Koha/Schema/Result/Item.pm | 13 +++ cataloguing/merge.pl | 22 ++-- cataloguing/moveitem.pl | 3 +- t/db_dependent/Items/MoveItemFromBiblio.t | 85 -------------- t/db_dependent/Koha/Item.t | 181 ++++++++++++++++++++++++++++- t/db_dependent/Koha/SearchEngine/Indexer.t | 12 +- 9 files changed, 322 insertions(+), 155 deletions(-) delete mode 100755 t/db_dependent/Items/MoveItemFromBiblio.t --- a/C4/Items.pm +++ a/C4/Items.pm @@ -39,7 +39,6 @@ BEGIN { GetHostItemsInfo get_hostitemnumbers_of GetHiddenItemnumbers - MoveItemFromBiblio CartToShelf GetAnalyticsCount SearchItems @@ -65,6 +64,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; @@ -1096,57 +1096,6 @@ the inner workings of C. =cut -=head2 MoveItemFromBiblio - - MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio); - -Moves an item from a biblio to another - -Returns undef if the move failed or the biblionumber of the destination record otherwise - -=cut - -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; -} - =head2 _marc_from_item_hash my $item_marc = _marc_from_item_hash($item, $frameworkcode[, $unlinked_item_subfields]); --- a/Koha/Biblio.pm +++ a/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; @@ -887,6 +888,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, { skip_record_index => 1 }); + } + my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); + $indexer->index_records( $self->biblionumber, "specialUpdate", "biblioserver" ); + $indexer->index_records( $from_biblio->biblionumber, "specialUpdate", "biblioserver" ); + } +} + + =head2 Internal methods =head3 type --- a/Koha/Item.pm +++ a/Koha/Item.pm @@ -1175,6 +1175,90 @@ 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 unless $orders; + return Koha::Acquisition::Orders->_new_from_dbic($orders); +} + +=head3 move_to_biblio + + $item->move_to_biblio($to_biblio[, $params]); + +Move the item to another biblio and update any references also in other tables. + +The final optional parameter, C<$params>, is expected to contain the +'skip_record_index' key, which is relayed down to Koha::Item->store. +There it prevents calling index_records, which takes most of the +time in batch adds/deletes. The caller must take care of calling +index_records separately. + +$params: + skip_record_index => 1|0 + +Returns undef if the move failed or the biblionumber of the destination record otherwise + +=cut + +sub move_to_biblio { + my ( $self, $to_biblio, $params ) = @_; + + $params //= {}; + + return unless $self->biblionumber != $to_biblio->biblionumber; + + my $biblionumber = $to_biblio->biblionumber; + + # Own biblionumber and biblioitemnumber + $self->set({ + biblionumber => $biblionumber, + biblioitemnumber => $to_biblio->biblioitem->biblioitemnumber + })->store({ skip_record_index => $params->{skip_record_index} }); + + # Acquisition orders + my $orders = $self->item_orders; + if ($orders) { + $orders->update({ biblionumber => $biblionumber }, { no_triggers => 1 }); + } + + # Holds + $self->holds->update({ biblionumber => $biblionumber }); + + # 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 }); + + return $biblionumber; +} + =head2 Internal methods =head3 _after_item_action_hooks --- a/Koha/Schema/Result/Item.pm +++ a/Koha/Schema/Result/Item.pm @@ -866,6 +866,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 ) = @_; --- a/cataloguing/merge.pl +++ a/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; --- a/cataloguing/moveitem.pl +++ a/cataloguing/moveitem.pl @@ -73,8 +73,9 @@ if ( $barcode && $biblionumber ) { $itemnumber = $item->itemnumber; my $frombiblionumber = $item->biblionumber; + my $to_biblio = Koha::Biblios->find($biblionumber); - my $moveresult = MoveItemFromBiblio( $itemnumber, $frombiblionumber, $biblionumber ); + my $moveresult = $item->move_to_biblio($to_biblio); if ($moveresult) { $template->param( success => 1, --- a/t/db_dependent/Items/MoveItemFromBiblio.t +++ a/t/db_dependent/Items/MoveItemFromBiblio.t @@ -1,85 +0,0 @@ -#!/usr/bin/perl -# -# This file is part of Koha. -# -# Koha is free software; you can redistribute it and/or modify it -# under the terms of the GNU General Public License as published by -# the Free Software Foundation; either version 3 of the License, or -# (at your option) any later version. -# -# Koha is distributed in the hope that it will be useful, but -# WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Koha; if not, see . - -use Modern::Perl; -use Test::More tests => 8; - -use C4::Items; -use C4::Reserves; -use Koha::Database; -use Koha::Holds; -use Koha::Items; - -use t::lib::TestBuilder; -use Data::Dumper qw|Dumper|; - -my $schema = Koha::Database->new->schema; -$schema->storage->txn_begin; -my $builder = t::lib::TestBuilder->new; - -my $from_biblio = $builder->build_sample_biblio; -my $to_biblio = $builder->build_sample_biblio; - -my $item1 = $builder->build_sample_item( - { biblionumber => $from_biblio->biblionumber, } ); -my $item2 = $builder->build_sample_item( - { biblionumber => $from_biblio->biblionumber, } ); -my $item3 = $builder->build_sample_item( - { biblionumber => $to_biblio->biblionumber, } ); - - -my $bib_level_hold_not_to_move = $builder->build( - { source => 'Reserve', - value => { biblionumber => $from_biblio->biblionumber, }, - } -); -my $item_level_hold_not_to_move = $builder->build( - { source => 'Reserve', - value => { biblionumber => $from_biblio->biblionumber, itemnumber => $item1->itemnumber }, - } -); -my $item_level_hold_to_move = $builder->build( - { source => 'Reserve', - value => { biblionumber => $from_biblio->biblionumber, itemnumber => $item2->itemnumber }, - } -); - -my $to_biblionumber_after_moved = C4::Items::MoveItemFromBiblio( $item2->itemnumber, $from_biblio->biblionumber, $to_biblio->biblionumber ); - -is( $to_biblionumber_after_moved, $to_biblio->biblionumber, 'MoveItemFromBiblio should return the to_biblionumber if success' ); - -$to_biblionumber_after_moved = C4::Items::MoveItemFromBiblio( $item2->itemnumber, $from_biblio->biblionumber, $to_biblio->biblionumber ); - -is( $to_biblionumber_after_moved, undef, 'MoveItemFromBiblio should return undef if the move has failed. If called twice, the item is not attached to the first biblio anymore' ); - -my $get_item1 = Koha::Items->find( $item1->itemnumber ); -is( $get_item1->biblionumber, $from_biblio->biblionumber, 'The item1 should not have been moved' ); -my $get_item2 = Koha::Items->find( $item2->itemnumber ); -is( $get_item2->biblionumber, $to_biblio->biblionumber, 'The item2 should have been moved' ); -my $get_item3 = Koha::Items->find( $item3->itemnumber ); -is( $get_item3->biblionumber, $to_biblio->biblionumber, 'The item3 should not have been moved' ); - -my $get_bib_level_hold = Koha::Holds->find( $bib_level_hold_not_to_move->{reserve_id} ); -my $get_item_level_hold_1 = Koha::Holds->find( $item_level_hold_not_to_move->{reserve_id} ); -my $get_item_level_hold_2 = Koha::Holds->find( $item_level_hold_to_move->{reserve_id} ); - -is( $get_bib_level_hold->biblionumber, $from_biblio->biblionumber, 'MoveItemFromBiblio should not have moved the biblio-level hold' ); -is( $get_item_level_hold_1->biblionumber, $from_biblio->biblionumber, 'MoveItemFromBiblio should not have moved the item-level hold placed on item 1' ); -is( $get_item_level_hold_2->biblionumber, $to_biblio->biblionumber, 'MoveItemFromBiblio should have moved the item-level hold placed on item 2' ); - -$schema->storage->txn_rollback; - --- a/t/db_dependent/Koha/Item.t +++ a/t/db_dependent/Koha/Item.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 9; +use Test::More tests => 11; use Test::Exception; use C4::Biblio; @@ -795,3 +795,182 @@ subtest 'get_transfers' => sub { $schema->storage->txn_rollback; }; + +subtest 'move_to_biblio() tests' => sub { + + plan tests => 16; + + $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 $target_biblionumber = $target_biblio->biblionumber; + + my $item1 = $builder->build_sample_item({ biblionumber => $source_biblionumber }); + my $item2 = $builder->build_sample_item({ biblionumber => $source_biblionumber }); + my $item3 = $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; + + my $aq_budget = $builder->build({ + source => 'Aqbudget', + value => { + budget_notes => 'test', + }, + }); + + my $aq_order1 = $builder->build_object({ + class => 'Koha::Acquisition::Orders', + value => { + biblionumber => $source_biblionumber, + budget_id => $aq_budget->{budget_id}, + }, + }); + my $aq_order_item1 = $builder->build({ + source => 'AqordersItem', + value => { + ordernumber => $aq_order1->ordernumber, + itemnumber => $itemnumber1, + }, + }); + my $aq_order2 = $builder->build_object({ + class => 'Koha::Acquisition::Orders', + value => { + biblionumber => $source_biblionumber, + budget_id => $aq_budget->{budget_id}, + }, + }); + my $aq_order_item2 = $builder->build({ + source => 'AqordersItem', + value => { + ordernumber => $aq_order2->ordernumber, + itemnumber => $itemnumber2, + }, + }); + + my $bib_level_hold = $builder->build_object({ + class => 'Koha::Holds', + value => { + biblionumber => $source_biblionumber, + }, + }); + my $item_level_hold1 = $builder->build_object({ + class => 'Koha::Holds', + value => { + biblionumber => $source_biblionumber, + itemnumber => $itemnumber1, + }, + }); + my $item_level_hold2 = $builder->build_object({ + class => 'Koha::Holds', + value => { + biblionumber => $source_biblionumber, + itemnumber => $itemnumber2, + } + }); + + my $tmp_holdsqueue1 = $builder->build({ + source => 'TmpHoldsqueue', + value => { + borrowernumber => $borrowernumber, + biblionumber => $source_biblionumber, + itemnumber => $itemnumber1, + } + }); + my $tmp_holdsqueue2 = $builder->build({ + source => 'TmpHoldsqueue', + value => { + borrowernumber => $borrowernumber, + biblionumber => $source_biblionumber, + itemnumber => $itemnumber2, + } + }); + my $hold_fill_target1 = $builder->build({ + source => 'HoldFillTarget', + value => { + borrowernumber => $borrowernumber, + biblionumber => $source_biblionumber, + itemnumber => $itemnumber1, + } + }); + my $hold_fill_target2 = $builder->build({ + source => 'HoldFillTarget', + value => { + borrowernumber => $borrowernumber, + biblionumber => $source_biblionumber, + itemnumber => $itemnumber2, + } + }); + my $linktracker1 = $builder->build({ + source => 'Linktracker', + value => { + borrowernumber => $borrowernumber, + biblionumber => $source_biblionumber, + itemnumber => $itemnumber1, + } + }); + my $linktracker2 = $builder->build({ + source => 'Linktracker', + value => { + borrowernumber => $borrowernumber, + biblionumber => $source_biblionumber, + itemnumber => $itemnumber2, + } + }); + + my $to_biblionumber_after_move = $item1->move_to_biblio($target_biblio); + is($to_biblionumber_after_move, $target_biblionumber, 'move_to_biblio returns the target biblionumber if success'); + + $to_biblionumber_after_move = $item1->move_to_biblio($target_biblio); + is($to_biblionumber_after_move, undef, 'move_to_biblio returns undef if the move has failed. If called twice, the item is not attached to the first biblio anymore'); + + my $get_item1 = Koha::Items->find( $item1->itemnumber ); + is($get_item1->biblionumber, $target_biblionumber, 'item1 is moved'); + my $get_item2 = Koha::Items->find( $item2->itemnumber ); + is($get_item2->biblionumber, $source_biblionumber, 'item2 is not moved'); + my $get_item3 = Koha::Items->find( $item3->itemnumber ); + is($get_item3->biblionumber, $source_biblionumber, 'item3 is not moved'); + + $aq_order1->discard_changes; + $aq_order2->discard_changes; + is($aq_order1->biblionumber, $target_biblionumber, 'move_to_biblio moves aq_orders for item 1'); + is($aq_order2->biblionumber, $source_biblionumber, 'move_to_biblio does not move aq_orders for item 2'); + + $bib_level_hold->discard_changes; + $item_level_hold1->discard_changes; + $item_level_hold2->discard_changes; + is($bib_level_hold->biblionumber, $source_biblionumber, 'move_to_biblio does not move the biblio-level hold'); + is($item_level_hold1->biblionumber, $target_biblionumber, 'move_to_biblio moves the item-level hold placed on item 1'); + is($item_level_hold2->biblionumber, $source_biblionumber, 'move_to_biblio does not move the item-level hold placed on item 2'); + + my $get_tmp_holdsqueue1 = $schema->resultset('TmpHoldsqueue')->search({ itemnumber => $tmp_holdsqueue1->{itemnumber} })->single; + my $get_tmp_holdsqueue2 = $schema->resultset('TmpHoldsqueue')->search({ itemnumber => $tmp_holdsqueue2->{itemnumber} })->single; + is($get_tmp_holdsqueue1->biblionumber, $target_biblionumber, 'move_to_biblio moves tmp_holdsqueue for item 1'); + is($get_tmp_holdsqueue2->biblionumber, $source_biblionumber, 'move_to_biblio does not move tmp_holdsqueue for item 2'); + + my $get_hold_fill_target1 = $schema->resultset('HoldFillTarget')->search({ itemnumber => $hold_fill_target1->{itemnumber} })->single; + my $get_hold_fill_target2 = $schema->resultset('HoldFillTarget')->search({ itemnumber => $hold_fill_target2->{itemnumber} })->single; + # Why does ->biblionumber return a Biblio object??? + is($get_hold_fill_target1->biblionumber->biblionumber, $target_biblionumber, 'move_to_biblio moves hold_fill_targets for item 1'); + is($get_hold_fill_target2->biblionumber->biblionumber, $source_biblionumber, 'move_to_biblio does not move hold_fill_targets for item 2'); + + my $get_linktracker1 = $schema->resultset('Linktracker')->search({ itemnumber => $linktracker1->{itemnumber} })->single; + my $get_linktracker2 = $schema->resultset('Linktracker')->search({ itemnumber => $linktracker2->{itemnumber} })->single; + is($get_linktracker1->biblionumber, $target_biblionumber, 'move_to_biblio moves linktracker for item 1'); + is($get_linktracker2->biblionumber, $source_biblionumber, 'move_to_biblio does not move linktracker for item 2'); + + $schema->storage->txn_rollback; +}; --- a/t/db_dependent/Koha/SearchEngine/Indexer.t +++ a/t/db_dependent/Koha/SearchEngine/Indexer.t @@ -61,7 +61,7 @@ subtest 'Test indexer object creation' => sub { }; subtest 'Test indexer calls' => sub { - plan tests => 40; + plan tests => 44; my @engines = ('Zebra'); eval { Koha::SearchEngine::Elasticsearch->get_elasticsearch_params; }; @@ -160,6 +160,16 @@ subtest 'Test indexer calls' => sub { 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"; + + my $item3 = $builder->build_sample_item({biblionumber => $biblio->biblionumber}); + my $item4 = $builder->build_sample_item({biblionumber => $biblio->biblionumber}); + warnings_are{ + $item3->move_to_biblio($biblio2); + } [$engine,"Koha::Item"], "index_records is called for $engine when moving an item to another biblio (Item->move_to_biblio)"; + warnings_are{ + $item4->move_to_biblio($biblio2, { skip_record_index => 1 }); + } undef, "index_records is not called for $engine when moving an item to another biblio (Item->move_to_biblio) if skip_record_index passed"; + $builder->build({ source => 'Issue', value => { --