gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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/Biblio.t
  prove -v t/db_dependent/Koha/Item.t
  prove -v t/db_dependent/Koha/SearchEngine/Indexer.t

Signed-off-by: Michal Denar <black23@gmail.com>
Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This commit is contained in:
Ere Maijala 2019-09-16 13:55:37 +03:00 committed by Jonathan Druart
parent 1c834f18d5
commit 2615ee1010
9 changed files with 322 additions and 155 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
C4/Items.pm
View File

@ -40,7 +40,6 @@ BEGIN {
get_hostitemnumbers_of
GetHiddenItemnumbers
GetMarcItem
MoveItemFromBiblio
CartToShelf
GetAnalyticsCount
SearchItems
@ -65,6 +64,7 @@ use Koha::AuthorisedValues;
use Koha::DateUtils qw( dt_from_string output_pref );
use Koha::Database;
use Koha::Biblios;
use Koha::Biblioitems;
use Koha::Items;
use Koha::ItemTypes;
@ -1101,57 +1101,6 @@ the inner workings of C<C4::Items>.
=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]);

24
Koha/Biblio.pm
View File

@ -39,6 +39,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;
@ -952,6 +953,29 @@ sub get_marc_host {
}
}
=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

84
Koha/Item.pm
View File

@ -1172,6 +1172,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

13
Koha/Schema/Result/Item.pm
View File

@ -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 ) = @_;

22
cataloguing/merge.pl
View File

@ -39,6 +39,7 @@ use C4::Reserves qw( MergeHolds );
use C4::Acquisition qw( ModOrder GetOrdersByBiblionumber );
use Koha::BiblioFrameworks;
use Koha::Biblios;
use Koha::Items;
use Koha::MetadataRecord;
@ -88,25 +89,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("
@ -170,7 +162,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;

3
cataloguing/moveitem.pl
View File

@ -69,8 +69,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,

85
t/db_dependent/Items/MoveItemFromBiblio.t
View File

@ -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 <http://www.gnu.org/licenses>.
use Modern::Perl;
use Test::More tests => 8;
use C4::Items qw( MoveItemFromBiblio );
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;

181
t/db_dependent/Koha/Item.t
View File

@ -19,7 +19,7 @@
use Modern::Perl;
use Test::More tests => 9;
use Test::More tests => 11;
use Test::Exception;
use C4::Biblio qw( GetMarcSubfieldStructure );
@ -816,3 +816,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;
};

12
t/db_dependent/Koha/SearchEngine/Indexer.t
View File

@ -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 => {