From c256a30998763c7962a26c72a7c34de284e04f35 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 3 Dec 2019 10:13:38 +0100 Subject: [PATCH] Bug 8132: Delete the items in a transaction and rollback if something wrong The last_item_for_hold case cannot be guessed (easily), and so we are going to delete the items in a transaction. If something wrong happened we rollback and display a list of items that caused the rollback. Signed-off-by: Kelly McElligott Signed-off-by: Victor Grousset/tuxayo Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize --- .../prog/en/modules/tools/batchMod-del.tt | 23 +- tools/batchMod.pl | 227 ++++++++++-------- 2 files changed, 152 insertions(+), 98 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-del.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-del.tt index 8701d9e2c6..38e946b38a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-del.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batchMod-del.tt @@ -120,7 +120,8 @@ [% CASE "not_same_branch" %][% SET cannot_delete_reason = t("Item does not belongs to your library") %] [% CASE "book_reserved" %][% SET cannot_delete_reason = t("Item has a waiting hold") %] [% CASE "linked_analytics" %][% SET cannot_delete_reason = t("Item has linked analytics") %] - [% CASE %][% SET cannot_delete_reason = t("Unknown reason") %] + [% CASE "last_item_for_hold" %][% SET cannot_delete_reason = t("Last item for bibliographic record wich biblio-level hold on it") %] + [% CASE %][% SET cannot_delete_reason = t("Unknown reason") _ '(' _ can_be_deleted _ ')' %] [% END %] @@ -210,7 +211,12 @@ [% END %] [% IF ( action ) %] -
+ [% IF deletion_failed %] +
+ At least one item blocked the deletion. The operation rolled back and nothing happened! +
+ [% ELSE %] +

[% deleted_items | html %] item(s) deleted.

[% IF delete_records %]

[% deleted_records | html %] record(s) deleted.

[% END %] [% IF src == 'CATALOGUING' # from catalogue/detail.pl > Delete items in a batch%] @@ -225,6 +231,7 @@ Return to batch item deletion [% END %]
+ [% END %] [% IF ( not_deleted_items ) %]

[% not_deleted_items | html %] item(s) could not be deleted: [% FOREACH not_deleted_itemnumber IN not_deleted_itemnumbers %][% not_deleted_itemnumber.itemnumber | html %][% END %]

@@ -242,7 +249,17 @@ [% not_deleted_loo.itemnumber | html %] [% IF ( CAN_user_editcatalogue_edit_items ) %][% not_deleted_loo.barcode | html %][% ELSE %][% not_deleted_loo.barcode | html %][% END %] - [% IF ( not_deleted_loo.book_on_loan ) %]Item is checked out[% ELSIF ( not_deleted_loo.book_reserved ) %]Item has a waiting hold[% END %] + + [% SWITCH not_deleted_loo.reason %] + [% CASE "book_on_loan" %][% SET cannot_delete_reason = t("Item is checked out") %] + [% CASE "not_same_branch" %][% SET cannot_delete_reason = t("Item does not belongs to your library") %] + [% CASE "book_reserved" %][% SET cannot_delete_reason = t("Item has a waiting hold") %] + [% CASE "linked_analytics" %][% SET cannot_delete_reason = t("Item has linked analytics") %] + [% CASE "last_item_for_hold" %][% SET cannot_delete_reason = t("Last item for bibliographic record wich biblio-level hold on it") %] + [% CASE %][% SET cannot_delete_reason = t("Unknown reason") _ '(' _ can_be_deleted _ ')' %] + [% END %] + [% cannot_delete_reason %] + [% END %] diff --git a/tools/batchMod.pl b/tools/batchMod.pl index a7e3188ef0..91ede79b1e 100755 --- a/tools/batchMod.pl +++ b/tools/batchMod.pl @@ -20,6 +20,8 @@ use CGI qw ( -utf8 ); use Modern::Perl; +use Try::Tiny; + use C4::Auth; use C4::Output; use C4::Biblio; @@ -34,6 +36,8 @@ use C4::Members; use MARC::File::XML; use List::MoreUtils qw/uniq/; +use Koha::Database; +use Koha::Exceptions::Exception; use Koha::AuthorisedValues; use Koha::Biblios; use Koha::DateUtils; @@ -181,107 +185,140 @@ if ($op eq "action") { } } - # For each item - my $i = 1; - foreach my $itemnumber(@itemnumbers){ - - $job->progress($i) if $runinbackground; - my $item = Koha::Items->find($itemnumber); - next unless $item; # Should have been tested earlier, but just in case... - my $itemdata = $item->unblessed; - if ( $del ){ - my $return = $item->safe_delete; - if (ref($return)) { - $deleted_items++; - } else { - $not_deleted_items++; - push @not_deleted, - { biblionumber => $itemdata->{'biblionumber'}, - itemnumber => $itemdata->{'itemnumber'}, - barcode => $itemdata->{'barcode'}, - title => $itemdata->{'title'}, - $return => 1 - }; - } + try { + my $schema = Koha::Database->new->schema; + $schema->txn_do( + sub { + # For each item + my $i = 1; + foreach my $itemnumber (@itemnumbers) { + $job->progress($i) if $runinbackground; + my $item = Koha::Items->find($itemnumber); + next + unless $item + ; # Should have been tested earlier, but just in case... + my $itemdata = $item->unblessed; + if ($del) { + my $return = $item->safe_delete; + if ( ref( $return ) ) { + $deleted_items++; + } + else { + $not_deleted_items++; + push @not_deleted, + { + biblionumber => $itemdata->{'biblionumber'}, + itemnumber => $itemdata->{'itemnumber'}, + barcode => $itemdata->{'barcode'}, + title => $itemdata->{'title'}, + reason => $return, + }; + } - # If there are no items left, delete the biblio - if ($del_records) { - my $itemscount = Koha::Biblios->find( $itemdata->{'biblionumber'} )->items->count; - if ( $itemscount == 0 ) { - my $error = DelBiblio( $itemdata->{'biblionumber'} ); - unless ($error) { - $deleted_records++; - if ( $src eq 'CATALOGUING' ) { - # We are coming catalogue/detail.pl, there were items from a single bib record - $template->param( biblio_deleted => 1 ); + # If there are no items left, delete the biblio + if ($del_records) { + my $itemscount = Koha::Biblios->find( $itemdata->{'biblionumber'} )->items->count; + if ( $itemscount == 0 ) { + my $error = DelBiblio( $itemdata->{'biblionumber'} ); + unless ($error) { + $deleted_records++; + if ( $src eq 'CATALOGUING' ) { + # We are coming catalogue/detail.pl, there were items from a single bib record + $template->param( biblio_deleted => 1 ); + } + } + } } } - } - } - } else { - if ($values_to_modify || $values_to_blank) { - my $localmarcitem = Item2Marc($itemdata); - my $modified = 0; - - for ( my $i = 0 ; $i < @tags ; $i++ ) { - my $search = $searches[$i]; - next unless $search; - - my $tag = $tags[$i]; - my $subfield = $subfields[$i]; - my $replace = $replaces[$i]; - - my $value = $localmarcitem->field( $tag )->subfield( $subfield ); - my $old_value = $value; - - my @available_modifiers = qw( i g ); - my $retained_modifiers = q||; - for my $modifier ( split //, $modifiers[$i] ) { - $retained_modifiers .= $modifier - if grep {/$modifier/} @available_modifiers; - } - if ( $retained_modifiers =~ m/^(ig|gi)$/ ) { - $value =~ s/$search/$replace/ig; - } - elsif ( $retained_modifiers eq 'i' ) { - $value =~ s/$search/$replace/i; - } - elsif ( $retained_modifiers eq 'g' ) { - $value =~ s/$search/$replace/g; - } - else { - $value =~ s/$search/$replace/; - } - - my @fields_to = $localmarcitem->field($tag); - foreach my $field_to_update ( @fields_to ) { - unless ( $old_value eq $value ) { - $modified++; - $field_to_update->update( $subfield => $value ); + else { + if ( $values_to_modify || $values_to_blank ) { + my $localmarcitem = Item2Marc($itemdata); + my $modified = 0; + + for ( my $i = 0 ; $i < @tags ; $i++ ) { + my $search = $searches[$i]; + next unless $search; + + my $tag = $tags[$i]; + my $subfield = $subfields[$i]; + my $replace = $replaces[$i]; + + my $value = $localmarcitem->field( $tag )->subfield( $subfield ); + my $old_value = $value; + + my @available_modifiers = qw( i g ); + my $retained_modifiers = q||; + for my $modifier ( split //, $modifiers[$i] ) { + $retained_modifiers .= $modifier + if grep {/$modifier/} @available_modifiers; + } + if ( $retained_modifiers =~ m/^(ig|gi)$/ ) { + $value =~ s/$search/$replace/ig; + } + elsif ( $retained_modifiers eq 'i' ) { + $value =~ s/$search/$replace/i; + } + elsif ( $retained_modifiers eq 'g' ) { + $value =~ s/$search/$replace/g; + } + else { + $value =~ s/$search/$replace/; + } + + my @fields_to = $localmarcitem->field($tag); + foreach my $field_to_update ( @fields_to ) { + unless ( $old_value eq $value ) { + $modified++; + $field_to_update->update( $subfield => $value ); + } + } + } + + $modified += UpdateMarcWith( $marcitem, $localmarcitem ); + if ($modified) { + eval { + if ( + my $item = ModItemFromMarc( + $localmarcitem, + $itemdata->{biblionumber}, + $itemnumber + ) + ) + { + LostItem( $itemnumber, 'batchmod' ) + if $item->{itemlost} + and not $itemdata->{itemlost}; + } + }; + } + if ($runinbackground) { + $modified_items++ if $modified; + $modified_fields += $modified; + $job->set( + { + modified_items => $modified_items, + modified_fields => $modified_fields, + } + ); + } + } } + $i++; + } + if (@not_deleted) { + Koha::Exceptions::Exception->throw( + 'Some items have not been deleted, rolling back'); } } - - $modified += UpdateMarcWith( $marcitem, $localmarcitem ); - if ( $modified ) { - eval { - if ( my $item = ModItemFromMarc( $localmarcitem, $itemdata->{biblionumber}, $itemnumber ) ) { - LostItem($itemnumber, 'batchmod') if $item->{itemlost} and not $itemdata->{itemlost}; - } - }; - } - if ( $runinbackground ) { - $modified_items++ if $modified; - $modified_fields += $modified; - $job->set({ - modified_items => $modified_items, - modified_fields => $modified_fields, - }); - } - } - } - $i++; - } + ); + } + catch { + if ( $_->isa('Koha::Exceptions::Exception') ) { + $template->param( deletion_failed => 1 ); + } + die "Something terrible has happened!" + if ($_ =~ /Rollback failed/); # Rollback failed + } } } # -- 2.39.5