From 88179dfb194067f1f151e62210cce798216a081e Mon Sep 17 00:00:00 2001 From: Alex Buckley Date: Mon, 9 Apr 2018 08:59:41 +0000 Subject: [PATCH] WR 289187 - Avoid biblio index race condition after moving item The moveitem.pl script loops through a 2 value array containing "new" and "old". The first loop value "new" is handed as an optional argument to the MoveItemFromBiblio() function in C4/Items.pm. This updates the biblios and only reindexes the new biblio. If a 1 if returned from the call to MoveItemFromBilio() to moveitem.pl then the loop iterates through the second value in the array "old" this calls the MoveItemFromBiblio() function as well but this time the aforementioned function only does one thing which is add the old biblio to the zebraqueue. Due to calling the ModItemFromBiblio() function twice from two different iterations of the loop in moveitem.pl there is a 1 second gap between the new biblio and old biblio being added to the zebraqueue table. This reduces the chances of a race condition of the old biblio being indexed before the new biblio. https://bugs.koha-community.org/show_bug.cgi?id=20549 --- C4/Items.pm | 93 +++++++++++++++++++++---------- cataloguing/moveitem.pl | 50 ++++++++++------- t/db_dependent/Items/MoveItemFromBiblio.t | 10 ++-- 3 files changed, 101 insertions(+), 52 deletions(-) diff --git a/C4/Items.pm b/C4/Items.pm index e7279ff..7abb74b 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -1863,7 +1863,7 @@ sub _koha_new_item { =head2 MoveItemFromBiblio - MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio); + MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio, $bibliotoindex); Moves an item from a biblio to another @@ -1879,36 +1879,73 @@ sub MoveItemFromBiblio { 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 = ? + if ( defined $_[3]) { + my $bibliotoindex = $_[3]; + if ($bibliotoindex eq "new") { + my $return = $dbh->do(q| + UPDATE items + SET biblioitemnumber = ?, + biblionumber = ? WHERE itemnumber = ? - |, undef, $tobiblio, $itemnumber ); + AND biblionumber = ? + |, undef, $tobiblioitem, $tobiblio, $itemnumber, $frombiblio ); + if ($return == 1) { + ModZebra( $tobiblio, "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; + } + } elsif ($bibliotoindex eq "old") { + ModZebra( $frombiblio, "specialUpdate", "biblioserver" ); + return 1; } - return $tobiblio; - } - return; + } else { + 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; + } } =head2 ItemSafeToDelete diff --git a/cataloguing/moveitem.pl b/cataloguing/moveitem.pl index 5ef4ffe..2a3e72c 100755 --- a/cataloguing/moveitem.pl +++ b/cataloguing/moveitem.pl @@ -65,7 +65,7 @@ $template->param(bibliotitle => $biblio->title); $template->param(biblionumber => $biblionumber); # If we already have the barcode of the item to move and the biblionumber to move the item to -if ($barcode && $biblionumber) { +if ($barcode && $biblionumber) { my $itemnumber; my $item = Koha::Items->find({ barcode => $barcode }); @@ -74,25 +74,35 @@ if ($barcode && $biblionumber) { $itemnumber = $item->itemnumber; my $frombiblionumber = $item->biblionumber; - - my $moveresult = MoveItemFromBiblio($itemnumber, $frombiblionumber, $biblionumber); - if ($moveresult) { - $template->param(success => 1); - } else { - $template->param(error => 1, - errornonewitem => 1); - } - - - } else { - $template->param(error => 1, - errornoitem => 1); - } - $template->param( - barcode => $barcode, - itemnumber => $itemnumber, - ); - + my @bibliostoindex = ("new", "old"); + my $moveresult; + foreach my $bibliotoindex (@bibliostoindex) { + if ($bibliotoindex eq "new") { + $moveresult = MoveItemFromBiblio($itemnumber, $frombiblionumber, $biblionumber, $bibliotoindex); + if ($moveresult) { + $template->param(success => 1); + } else { + $template->param(error => 1, + errornonewitem => 1); + } + } elsif ($bibliotoindex eq "old" && $moveresult) { + my $moveresult = MoveItemFromBiblio($itemnumber, $frombiblionumber, $biblionumber, $bibliotoindex); + if ($moveresult) { + $template->param(success => 1); + } else { + $template->param(error => 1, + errornonewitem => 1); + } + } + } + } else { + $template->param(error => 1, + errornoitem => 1); + } + $template->param( + barcode => $barcode, + itemnumber => $itemnumber, + ); } else { $template->param(missingparameter => 1); if (!$barcode) { $template->param(missingbarcode => 1); } diff --git a/t/db_dependent/Items/MoveItemFromBiblio.t b/t/db_dependent/Items/MoveItemFromBiblio.t index 21855d9..b71e47d 100644 --- a/t/db_dependent/Items/MoveItemFromBiblio.t +++ b/t/db_dependent/Items/MoveItemFromBiblio.t @@ -16,7 +16,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 8; +use Test::More tests => 9; use C4::Items; use C4::Reserves; @@ -71,11 +71,13 @@ my $item_level_hold_to_move = $builder->build( } ); -my $to_biblionumber_after_moved = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber} ); +my $to_biblionumber_index_new_biblio = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber}, "new" ); +my $to_biblionumber_index_old_biblio = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber}, "old" ); -is( $to_biblionumber_after_moved, $to_biblio->{biblionumber}, 'MoveItemFromBiblio should return the to_biblionumber if success' ); +is( $to_biblionumber_index_new_biblio, $to_biblio->{biblionumber}, 'MoveItemFromBiblio should return the to_biblionumber if success' ); +is( $to_biblionumber_index_old_biblio, 1 , 'MoveItemFromBiblio should return the value 1 if the old biblio has been indexed if success' ); -$to_biblionumber_after_moved = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber} ); +my $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' ); -- 2.1.4