From 18f9518af6969c913062cd98a030e456bf71ce60 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 4 Jan 2022 15:34:48 +0100 Subject: [PATCH] Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean This patch reuse the (awesome) Koha::Result::Boolean module to retrieve the return of Koha::Item->safe_to_delete. Test plan: Try to delete an item that has previously been checked out and confirm that you are still blocked. Try using the cronjobs, the item and biblio detail pages, as well as the batch delete item tool. Signed-off-by: Tomas Cohen Arazi --- C4/ImportBatch.pm | 3 +- Koha/Acquisition/Order.pm | 4 +-- Koha/BackgroundJob/BatchDeleteBiblio.pm | 6 ++-- Koha/BackgroundJob/BatchDeleteItem.pm | 2 +- Koha/Item.pm | 21 +++++++++----- cataloguing/additem.pl | 13 ++++----- .../prog/en/includes/html_helpers.inc | 17 ++++++----- misc/cronjobs/delete_items.pl | 4 +-- misc/cronjobs/delete_records_via_leader.pl | 14 +++++---- t/db_dependent/Circulation/IsItemIssued.t | 7 ++--- t/db_dependent/Koha/Item.t | 29 ++++++++++++------- 11 files changed, 68 insertions(+), 52 deletions(-) diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm index 6ae148d6d2..bf060758b1 100644 --- a/C4/ImportBatch.pm +++ b/C4/ImportBatch.pm @@ -937,8 +937,7 @@ sub BatchRevertItems { $sth->execute(); while (my $row = $sth->fetchrow_hashref()) { my $item = Koha::Items->find($row->{itemnumber}); - my $error = $item->safe_delete; - if ($error eq '1'){ + if ($item->safe_delete){ my $updsth = $dbh->prepare("UPDATE import_items SET status = ? WHERE import_items_id = ?"); $updsth->bind_param(1, 'reverted'); $updsth->bind_param(2, $row->{'import_items_id'}); diff --git a/Koha/Acquisition/Order.pm b/Koha/Acquisition/Order.pm index 090920d92c..517c88af18 100644 --- a/Koha/Acquisition/Order.pm +++ b/Koha/Acquisition/Order.pm @@ -127,11 +127,11 @@ sub cancel { my $items = $self->items; while ( my $item = $items->next ) { my $deleted = $item->safe_delete; - unless ( ref($deleted) eq 'Koha::Item' ) { + unless ( $deleted ) { $self->add_message( { message => 'error_delitem', - payload => { item => $item, reason => $deleted } + payload => { item => $item, reason => @{$deleted->messages}[0]->messages } } ); } diff --git a/Koha/BackgroundJob/BatchDeleteBiblio.pm b/Koha/BackgroundJob/BatchDeleteBiblio.pm index 6931759286..63e12935af 100644 --- a/Koha/BackgroundJob/BatchDeleteBiblio.pm +++ b/Koha/BackgroundJob/BatchDeleteBiblio.pm @@ -112,14 +112,14 @@ sub process { # Delete items my $items = Koha::Items->search({ biblionumber => $biblionumber }); while ( my $item = $items->next ) { - my $error = $item->safe_delete; - if(ref($error) ne 'Koha::Item'){ + my $deleted = $item->safe_delete; + if( $deleted ) { push @messages, { type => 'error', code => 'item_not_deleted', biblionumber => $biblionumber, itemnumber => $item->itemnumber, - error => $error, + error => @{$deleted->messages}[0]->messages, }; $schema->storage->txn_rollback; $job->progress( ++$job_progress )->store; diff --git a/Koha/BackgroundJob/BatchDeleteItem.pm b/Koha/BackgroundJob/BatchDeleteItem.pm index d5882e1c93..66f0044624 100644 --- a/Koha/BackgroundJob/BatchDeleteItem.pm +++ b/Koha/BackgroundJob/BatchDeleteItem.pm @@ -114,7 +114,7 @@ sub process { my $item = Koha::Items->find($record_id) || next; my $return = $item->safe_delete; - unless ( ref($return) ) { + unless ( $return ) { # FIXME Do we need to rollback the whole transaction if a deletion failed? push @not_deleted_itemnumbers, $item->itemnumber; diff --git a/Koha/Item.pm b/Koha/Item.pm index d70fbc3f8f..57a04656cf 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -45,6 +45,7 @@ use Koha::Libraries; use Koha::StockRotationItem; use Koha::StockRotationRotas; use Koha::TrackedLinks; +use Koha::Result::Boolean; use base qw(Koha::Object); @@ -248,7 +249,7 @@ sub safe_delete { my $params = @_ ? shift : {}; my $safe_to_delete = $self->safe_to_delete; - return $safe_to_delete unless $safe_to_delete eq '1'; + return $safe_to_delete unless $safe_to_delete; $self->move_to_deleted; @@ -274,22 +275,24 @@ returns 1 if the item is safe to delete, sub safe_to_delete { my ($self) = @_; - return "book_on_loan" if $self->checkout; + my $error; - return "not_same_branch" + $error = "book_on_loan" if $self->checkout; + + $error = "not_same_branch" if defined C4::Context->userenv and !C4::Context->IsSuperLibrarian() and C4::Context->preference("IndependentBranches") and ( C4::Context->userenv->{branch} ne $self->homebranch ); # check it doesn't have a waiting reserve - return "book_reserved" + $error = "book_reserved" if $self->holds->search( { found => [ 'W', 'T' ] } )->count; - return "linked_analytics" + $error = "linked_analytics" if C4::Items::GetAnalyticsCount( $self->itemnumber ) > 0; - return "last_item_for_hold" + $error = "last_item_for_hold" if $self->biblio->items->count == 1 && $self->biblio->holds->search( { @@ -297,7 +300,11 @@ sub safe_to_delete { } )->count; - return 1; + if ( $error ) { + return Koha::Result::Boolean->new(0)->add_message({ message => $error }); + } + + return Koha::Result::Boolean->new(1); } =head3 move_to_deleted diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl index c98eacae84..4588b13fb5 100755 --- a/cataloguing/additem.pl +++ b/cataloguing/additem.pl @@ -376,11 +376,11 @@ if ($op eq "additem") { #------------------------------------------------------------------------------- # check that there is no issue on this item before deletion. my $item = Koha::Items->find($itemnumber); - my $error = $item->safe_delete; - if(ref($error) eq 'Koha::Item'){ + my $deleted = $item->safe_delete; + if($deleted) { print $input->redirect("additem.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid"); - }else{ - push @errors,$error; + } else { + push @errors, @{$deleted->messages}[0]->message; $nextop="additem"; } #------------------------------------------------------------------------------- @@ -388,9 +388,8 @@ if ($op eq "additem") { #------------------------------------------------------------------------------- my $items = Koha::Items->search({ biblionumber => $biblionumber }); while ( my $item = $items->next ) { - my $error = $item->safe_delete({ skip_record_index => 1 }); - next if ref $error eq 'Koha::Item'; # Deleted item is returned if deletion successful - push @errors,$error; + my $deleted = $item->safe_delete({ skip_record_index => 1 }); + push @errors, @{$deleted->messages}[0]->message; } my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); $indexer->index_records( $biblionumber, "specialUpdate", "biblioserver" ); diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc index a2ea6ebd20..56707256be 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc @@ -264,13 +264,16 @@ [% IF item.safe_to_delete == 1 %] [% ELSE %] - [% SWITCH item.safe_to_delete%] - [% 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 belong 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 with biblio-level hold on it") %] - [% CASE %][% SET cannot_delete_reason = t("Unknown reason") _ '(' _ item.safe_to_delete _ ')' %] + [% SET messages = item.safe_to_delete.messages %] + [% FOR m IN messages %] + [% SWITCH m %] + [% 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 belong 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 with biblio-level hold on it") %] + [% CASE %][% SET cannot_delete_reason = t("Unknown reason") _ '(' _ item.safe_to_delete _ ')' %] + [% END %] [% END %] diff --git a/misc/cronjobs/delete_items.pl b/misc/cronjobs/delete_items.pl index 93a00c6c1f..7d242f87d7 100755 --- a/misc/cronjobs/delete_items.pl +++ b/misc/cronjobs/delete_items.pl @@ -60,12 +60,12 @@ DELITEM: while ( my $item = $GLOBAL->{sth}->{target_items}->fetchrow_hashref() ) my $item_object = Koha::Items->find($item->{itemnumber}); my $safe_to_delete = $item_object->safe_to_delete; - if( $safe_to_delete eq '1' ) { + if( $safe_to_delete ) { $item_object->safe_delete if $OPTIONS->{flags}->{commit}; verbose "Deleting '$item->{itemnumber}'"; } else { - verbose "Item '$item->{itemnumber}' not deleted: $safe_to_delete"; + verbose sprintf "Item '%s' not deleted: %s", $item->{itemnumber}, @{$safe_to_delete->messages}[0] } } diff --git a/misc/cronjobs/delete_records_via_leader.pl b/misc/cronjobs/delete_records_via_leader.pl index 2794efb8d7..bc919ec6ba 100755 --- a/misc/cronjobs/delete_records_via_leader.pl +++ b/misc/cronjobs/delete_records_via_leader.pl @@ -91,19 +91,21 @@ foreach my $m (@metadatas) { my $itemnumber = $item->itemnumber; if( $test ){ - my $result = $item->safe_to_delete; - if ( $result eq "1") { + my $deleted = $item->safe_to_delete; + if ( $deleted ) { say "TEST MODE: Item $itemnumber would have been deleted"; } else { - say "TEST MODE: ERROR DELETING ITEM $itemnumber: $result"; + my $error = @{$deleted->messages}[0]->message + say "TEST MODE: ERROR DELETING ITEM $itemnumber: $error"; } } else { - my $result = $item->safe_delete; - if ( ref $result eq "Koha::Item" ){ + my $deleted = $item->safe_to_delete; + if ( $deleted ) { say "DELETED ITEM $itemnumber" if $verbose; $deleted_items_count++; } else { - say "ERROR DELETING ITEM $itemnumber: $result"; + my $error = @{$deleted->messages}[0]->message + say "ERROR DELETING ITEM $itemnumber: $error"; } } $total_items_count++; diff --git a/t/db_dependent/Circulation/IsItemIssued.t b/t/db_dependent/Circulation/IsItemIssued.t index b2625617ed..8238969566 100755 --- a/t/db_dependent/Circulation/IsItemIssued.t +++ b/t/db_dependent/Circulation/IsItemIssued.t @@ -67,7 +67,7 @@ AddIssue($borrower, $item->barcode); is ( IsItemIssued( $item->itemnumber ), 1, "item is now on loan" ); is( - $item->safe_delete, + @{$item->safe_delete->messages}[0]->message, 'book_on_loan', 'item that is on loan cannot be deleted', ); @@ -77,9 +77,8 @@ is ( IsItemIssued( $item->itemnumber ), 0, "item has been returned" ); $item->discard_changes; # FIXME We should not need that # If we do not, $self->checkout in safe_to_delete will not know the item has been checked out -is( - ref($item->safe_delete), - 'Koha::Item', +ok( + $item->safe_delete, 'item that is not on loan can be deleted', ); diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index db3ff9a987..0139cd40af 100755 --- a/t/db_dependent/Koha/Item.t +++ b/t/db_dependent/Koha/Item.t @@ -571,7 +571,7 @@ subtest 'request_transfer' => sub { }; subtest 'deletion' => sub { - plan tests => 12; + plan tests => 13; $schema->storage->txn_begin; @@ -606,17 +606,22 @@ subtest 'deletion' => sub { C4::Circulation::AddIssue( $patron->unblessed, $item->barcode ); is( - $item->safe_to_delete, + @{$item->safe_to_delete->messages}[0]->message, 'book_on_loan', 'Koha::Item->safe_to_delete reports item on loan', ); is( - $item->safe_delete, + @{$item->safe_to_delete->messages}[0]->message, 'book_on_loan', 'item that is on loan cannot be deleted', ); + ok( + ! $item->safe_to_delete, + 'Koha::Item->safe_to_delete shows item NOT safe to delete' + ); + AddReturn( $item->barcode, $library->branchcode ); # book_reserved is tested in t/db_dependent/Reserves.t @@ -626,13 +631,13 @@ subtest 'deletion' => sub { my $item_2 = $builder->build_sample_item({ library => $library_2->branchcode }); is( - $item_2->safe_to_delete, + @{$item_2->safe_to_delete->messages}[0]->message, 'not_same_branch', 'Koha::Item->safe_to_delete reports IndependentBranches restriction', ); is( - $item_2->safe_delete, + @{$item_2->safe_to_delete->messages}[0]->message, 'not_same_branch', 'IndependentBranches prevents deletion at another branch', ); @@ -646,13 +651,13 @@ subtest 'deletion' => sub { $item->discard_changes; is( - $item->safe_to_delete, + @{$item->safe_to_delete->messages}[0]->message, 'linked_analytics', 'Koha::Item->safe_to_delete reports linked analytics', ); is( - $item->safe_delete, + @{$item->safe_to_delete->messages}[0]->message, 'linked_analytics', 'Linked analytics prevents deletion of item', ); @@ -661,15 +666,17 @@ subtest 'deletion' => sub { { # last_item_for_hold C4::Reserves::AddReserve({ branchcode => $patron->branchcode, borrowernumber => $patron->borrowernumber, biblionumber => $item->biblionumber }); - is( $item->safe_to_delete, 'last_item_for_hold', 'Item cannot be deleted if a biblio-level is placed on the biblio and there is only 1 item attached to the biblio' ); - + is( + @{$item->safe_to_delete->messages}[0]->message, + 'last_item_for_hold', + 'Item cannot be deleted if a biblio-level is placed on the biblio and there is only 1 item attached to the biblio' + ); # With another item attached to the biblio, the item can be deleted $builder->build_sample_item({ biblionumber => $item->biblionumber }); } - is( + ok( $item->safe_to_delete, - 1, 'Koha::Item->safe_to_delete shows item safe to delete' ); -- 2.34.1