From bc85f1a9cf9a033b5181349b196dbea2a0fe4988 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. --- 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 6ae148d6d24..bf060758b1c 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 090920d92c1..517c88af186 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 69317592866..63e12935af0 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 d5882e1c933..66f0044624d 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 d70fbc3f8f5..57a04656cf8 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 c98eacae848..4588b13fb58 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 a2ea6ebd209..56707256be0 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 2aa9e575d97..2eab937b210 100755 --- a/misc/cronjobs/delete_items.pl +++ b/misc/cronjobs/delete_items.pl @@ -78,12 +78,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]->message } } diff --git a/misc/cronjobs/delete_records_via_leader.pl b/misc/cronjobs/delete_records_via_leader.pl index 2794efb8d74..374c2d1061c 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 b2625617edf..82389695668 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 db3ff9a987d..0139cd40af8 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.25.1