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 <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

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'});

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 }
                 }
             );
         }

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;

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;

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

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;
+        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" );

koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc

@@ -264,7 +264,9 @@
                             [% IF item.safe_to_delete == 1 %]
                                 <td><input type="checkbox" name="itemnumber" value="[% item.itemnumber | html %]" id="row[% item.itemnumber | html %]" checked="checked" /></td>
                             [% ELSE %]
-                                [% SWITCH 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") %]
@@ -272,6 +274,7 @@
                                     [% 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 %]
 
                                 <td><input type="checkbox" name="itemnumber" value="[% item.itemnumber | html %]" id="row[% item.itemnumber | html %]" disabled="disabled" title="[% cannot_delete_reason | html %]"/></td>
                             [% END %]

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
     }
 }
 

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++;

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',
 );
 

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'
     );