Bug 23463: Remove DelItemCheck and ItemSafeToDelete

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

C4/Acquisition.pm

@@ -1926,9 +1926,9 @@ sub DelOrder {
     my $order = Koha::Acquisition::Orders->find($ordernumber);
     my $items = $order->items;
     while ( my $item = $items->next ) { # Should be moved to Koha::Acquisition::Order->delete
-        my $delcheck = C4::Items::DelItemCheck( $bibnum, $item->itemnumber );
+        my $delcheck = $item->safe_delete;
 
-        if($delcheck != 1) {
+        if($delcheck ne '1') {
             $error->{'delitem'} = 1;
         }
     }

C4/ImportBatch.pm

@@ -910,8 +910,9 @@ sub BatchRevertItems {
     $sth->bind_param(1, $import_record_id);
     $sth->execute();
     while (my $row = $sth->fetchrow_hashref()) {
-        my $error = DelItemCheck( $biblionumber, $row->{'itemnumber'});
-        if ($error == 1){
+        my $item = Koha::Items->find($row->{itemnumber});
+        my $error = $item->safe_delete;
+        if ($error eq '1'){
             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'});

C4/Items.pm

@@ -39,8 +39,6 @@ BEGIN {
         GetHostItemsInfo
         get_hostitemnumbers_of
         GetHiddenItemnumbers
-        ItemSafeToDelete
-        DelItemCheck
         MoveItemFromBiblio
         CartToShelf
         GetAnalyticsCount
@@ -1367,91 +1365,6 @@ sub MoveItemFromBiblio {
     return;
 }
 
-=head2 ItemSafeToDelete
-
-   ItemSafeToDelete( $biblionumber, $itemnumber);
-
-Exported function (core API) for checking whether an item record is safe to delete.
-
-returns 1 if the item is safe to delete,
-
-"book_on_loan" if the item is checked out,
-
-"not_same_branch" if the item is blocked by independent branches,
-
-"book_reserved" if the there are holds aganst the item, or
-
-"linked_analytics" if the item has linked analytic records.
-
-=cut
-
-sub ItemSafeToDelete {
-    my ( $biblionumber, $itemnumber ) = @_;
-    my $status;
-    my $dbh = C4::Context->dbh;
-
-    my $error;
-
-    my $countanalytics = GetAnalyticsCount($itemnumber);
-
-    my $item = Koha::Items->find($itemnumber) or return;
-
-    if ($item->checkout) {
-        $status = "book_on_loan";
-    }
-    elsif ( defined C4::Context->userenv
-        and !C4::Context->IsSuperLibrarian()
-        and C4::Context->preference("IndependentBranches")
-        and ( C4::Context->userenv->{branch} ne $item->homebranch ) )
-    {
-        $status = "not_same_branch";
-    }
-    else {
-        # check it doesn't have a waiting reserve
-        my $sth = $dbh->prepare(
-            q{
-            SELECT COUNT(*) FROM reserves
-            WHERE (found = 'W' OR found = 'T')
-            AND itemnumber = ?
-        }
-        );
-        $sth->execute($itemnumber);
-        my ($reserve) = $sth->fetchrow;
-        if ($reserve) {
-            $status = "book_reserved";
-        }
-        elsif ( $countanalytics > 0 ) {
-            $status = "linked_analytics";
-        }
-        else {
-            $status = 1;
-        }
-    }
-    return $status;
-}
-
-=head2 DelItemCheck
-
-   DelItemCheck( $biblionumber, $itemnumber);
-
-Exported function (core API) for deleting an item record in Koha if there no current issue.
-
-DelItemCheck wraps ItemSafeToDelete around DelItem.
-
-=cut
-
-sub DelItemCheck {
-    my ( $biblionumber, $itemnumber ) = @_;
-    my $status = ItemSafeToDelete( $biblionumber, $itemnumber );
-
-    if ( $status == 1 ) {
-        my $item = Koha::Items->find($itemnumber);
-        $item->move_to_deleted;
-        $item->delete;
-    }
-    return $status;
-}
-
 sub _mod_item_dates { # date formatting for date fields in item hash
     my ( $item ) = @_;
     return if !$item || ref($item) ne 'HASH';

Koha/Item.pm

@@ -176,6 +176,56 @@ sub delete {
     return $self->SUPER::delete;
 }
 
+=head3 safe_delete
+
+=cut
+
+sub safe_delete {
+    my ($self) = @_;
+
+    my $safe_to_delete = $self->safe_to_delete;
+    return $safe_to_delete unless $safe_to_delete eq '1';
+
+    $self->move_to_deleted;
+
+    return $self->delete;
+}
+
+=head3 safe_to_delete
+
+returns 1 if the item is safe to delete,
+
+"book_on_loan" if the item is checked out,
+
+"not_same_branch" if the item is blocked by independent branches,
+
+"book_reserved" if the there are holds aganst the item, or
+
+"linked_analytics" if the item has linked analytic records.
+
+=cut
+
+sub safe_to_delete {
+    my ($self) = @_;
+
+    return "book_on_loan" if $self->checkout;
+
+    return "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"
+      if $self->holds->search( { found => [ 'W', 'T' ] } )->count;
+
+    return "linked_analytics"
+      if C4::Items::GetAnalyticsCount( $self->itemnumber ) > 0;
+
+    return 1;
+}
+
 =head3 move_to_deleted
 
 my $is_moved = $item->move_to_deleted;
@@ -546,6 +596,16 @@ sub current_holds {
     return Koha::Holds->_new_from_dbic($hold_rs);
 }
 
+=head3 holds
+
+=cut
+
+sub holds {
+    my ( $self ) = @_;
+    my $hold_rs = $self->_result->reserves->search;
+    return Koha::Holds->_new_from_dbic($hold_rs);
+}
+
 =head3 stockrotationitem
 
   my $sritem = Koha::Item->stockrotationitem;

cataloguing/additem.pl

@@ -669,8 +669,9 @@ if ($op eq "additem") {
 } elsif ($op eq "delitem") {
 #-------------------------------------------------------------------------------
     # check that there is no issue on this item before deletion.
-    $error = &DelItemCheck( $biblionumber,$itemnumber);
-    if($error == 1){
+    my $item = Koha::Items->find($itemnumber);
+    $error = $item->safe_to_delete;
+    if($error ne '1'){
         print $input->redirect("additem.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid");
     }else{
         push @errors,$error;
@@ -679,10 +680,10 @@ if ($op eq "additem") {
 #-------------------------------------------------------------------------------
 } elsif ($op eq "delallitems") {
 #-------------------------------------------------------------------------------
-    my @itemnumbers = Koha::Items->search({ biblionumber => $biblionumber })->get_column('itemnumber');
-    foreach my $itemnumber ( @itemnumbers ) {
-        $error = C4::Items::DelItemCheck( $biblionumber, $itemnumber );
-        next if $error == 1; # Means ok
+    my $items = Koha::Items->search({ biblionumber => $biblionumber });
+    while ( my $item = $items->next ) {
+        $error = $item->safe_delete;
+        next if $error eq '1'; # Means ok
         push @errors,$error;
     }
     if ( @errors ) {

misc/cronjobs/delete_items.pl

@@ -54,18 +54,20 @@ my $where_clause = ' where ' . join ( " and ", @where );
 
 verbose "Where statement: $where_clause";
 
+# FIXME Use Koha::Items instead
 $GLOBAL->{sth}->{target_items} = $dbh->prepare( $query->{target_items} . $where_clause  );
 $GLOBAL->{sth}->{target_items}->execute();
 
 DELITEM: while ( my $item = $GLOBAL->{sth}->{target_items}->fetchrow_hashref() ) {
 
-    my $status = C4::Items::ItemSafeToDelete( $item->{biblionumber}, $item->{itemnumber} );
-    if( $status eq '1' )  {
-        C4::Items::DelItemCheck( $item->{biblionumber}, $item->{itemnumber} )
+    my $item_object = Koha::Items->find($item->{itemnumber});
+    my $safe_to_delete = $item_object->safe_to_delete;
+    if( $safe_to_delete eq '1' )  {
+        $item->safe_delete
             if $OPTIONS->{flags}->{commit};
         verbose "Deleting '$item->{itemnumber}'";
     } else {
-        verbose "Item '$item->{itemnumber}' not deletd: $status";
+        verbose "Item '$item->{itemnumber}' not deleted: $safe_to_delete";
     }
 }
 

misc/cronjobs/delete_records_via_leader.pl

@@ -99,7 +99,7 @@ foreach my $m (@metadatas) {
         foreach my $item ( @items ) {
             my $itemnumber = $item->itemnumber();
 
-            my $error = $test ? "Test mode enabled" : DelItemCheck( $biblionumber, $itemnumber );
+            my $error = $test ? "Test mode enabled" : $item->safe_delete;
             $error = undef if $error eq '1';
 
             if ($error) {

t/db_dependent/Circulation/IsItemIssued.t

@@ -68,7 +68,7 @@ AddIssue($borrower, $item->barcode);
 is ( IsItemIssued( $item->itemnumber ), 1, "item is now on loan" );
 
 is(
-    DelItemCheck( $biblionumber, $item->itemnumber),
+    $item->safe_delete,
     'book_on_loan',
     'item that is on loan cannot be deleted',
 );
@@ -77,7 +77,7 @@ AddReturn($item->barcode, $library->{branchcode});
 is ( IsItemIssued( $item->itemnumber ), 0, "item has been returned" );
 
 is(
-    DelItemCheck( $biblionumber, $item->itemnumber),
+    $item->safe_delete,
     1,
     'item that is not on loan can be deleted',
 );

t/db_dependent/Items_DelItemCheck.t

@@ -98,13 +98,13 @@ my $item = $builder->build_object(
 AddIssue( $patron, $item->barcode );
 
 is(
-    ItemSafeToDelete( $biblio->{biblionumber}, $item->itemnumber ),
+    $item->safe_to_delete,
     'book_on_loan',
-    'ItemSafeToDelete reports item on loan',
+    'Koha::Item->safe_to_delete reports item on loan',
 );
 
 is(
-    DelItemCheck( $biblio->{biblionumber}, $item->itemnumber ),
+    $item->safe_delete,
     'book_on_loan',
     'item that is on loan cannot be deleted',
 );
@@ -118,13 +118,13 @@ t::lib::Mocks::mock_preference('IndependentBranches', 1);
 $item->set( { homebranch => $branch2->{branchcode}, holdingbranch => $branch2->{branchcode} })->store;
 
 is(
-    ItemSafeToDelete( $biblio->{biblionumber}, $item->itemnumber ),
+    $item->safe_to_delete,
     'not_same_branch',
-    'ItemSafeToDelete reports IndependentBranches restriction',
+    'Koha::Item->safe_to_delete reports IndependentBranches restriction',
 );
 
 is(
-    DelItemCheck( $biblio->{biblionumber}, $item->itemnumber ),
+    $item->safe_delete,
     'not_same_branch',
     'IndependentBranches prevents deletion at another branch',
 );
@@ -139,13 +139,13 @@ $item->set( { homebranch => $branch->{branchcode}, holdingbranch => $branch->{br
     $module->mock( GetAnalyticsCount => sub { return 1 } );
 
     is(
-        ItemSafeToDelete( $biblio->{biblionumber}, $item->itemnumber ),
+        $item->safe_to_delete,
         'linked_analytics',
-        'ItemSafeToDelete reports linked analytics',
+        'Koha::Item->safe_to_delete reports linked analytics',
     );
 
     is(
-        DelItemCheck( $biblio->{biblionumber}, $item->itemnumber ),
+        $item->safe_delete,
         'linked_analytics',
         'Linked analytics prevents deletion of item',
     );
@@ -153,17 +153,17 @@ $item->set( { homebranch => $branch->{branchcode}, holdingbranch => $branch->{br
 }
 
 is(
-    ItemSafeToDelete( $biblio->{biblionumber}, $item->itemnumber ),
+    $item->safe_to_delete,
     1,
-    'ItemSafeToDelete shows item safe to delete'
+    'Koha::Item->safe_to_delete shows item safe to delete'
 );
 
-DelItemCheck( $biblio->{biblionumber}, $item->itemnumber );
+$item->safe_delete,
 
 my $test_item = Koha::Items->find( $item->itemnumber );
 
 is( $test_item, undef,
-    "DelItemCheck should delete item if ItemSafeToDelete returns true"
+    "Koha::Item->safe_delete should delete item if safe_to_delete returns true"
 );
 
 $schema->storage->txn_rollback;

t/db_dependent/Reserves.t

@@ -404,8 +404,9 @@ is($new_count, $hold_notice_count + 1, 'patron not notified a second time (bug 1
 
 # avoiding the not_same_branch error
 t::lib::Mocks::mock_preference('IndependentBranches', 0);
+my $item = Koha::Items->find($itemnumber);
 is(
-    DelItemCheck( $bibnum, $itemnumber),
+    $item->safe_delete,
     'book_reserved',
     'item that is captured to fill a hold cannot be deleted',
 );
@@ -431,7 +432,6 @@ AddReserve(
     }
 );
 
-my $item = Koha::Items->find( $itemnumber );
 $holds = $item->current_holds;
 my $dtf = Koha::Database->new->schema->storage->datetime_parser;
 my $future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } );

tools/batchMod.pl

@@ -186,11 +186,11 @@ if ($op eq "action") {
 	foreach my $itemnumber(@itemnumbers){
 
 		$job->progress($i) if $runinbackground;
-        my $itemdata = Koha::Items->find($itemnumber);
-        next unless $itemdata; # Should have been tested earlier, but just in case...
-        $itemdata = $itemdata->unblessed;
+        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 = DelItemCheck( $itemdata->{'biblionumber'}, $itemdata->{'itemnumber'});
+            my $return = $item->safe_delete;
 			if ($return == 1) {
 			    $deleted_items++;
 			} else {

tools/batch_delete_records.pl

@@ -175,10 +175,10 @@ if ( $op eq 'form' ) {
             }
 
             # Delete items
-            my @itemnumbers = Koha::Items->search({ biblionumber => $biblionumber })->get_column('itemnumber');
-            ITEMNUMBER: for my $itemnumber ( @itemnumbers ) {
-                my $error = eval { C4::Items::DelItemCheck( $biblionumber, $itemnumber ) };
-                if ( $error != 1 or $@ ) {
+            my $items = Koha::Items->search({ biblionumber => $biblionumber });
+            while ( my $item = $items->next ) {
+                my $error = eval { $item->safe_delete };
+                if ( $error ne '1' or $@ ) {
                     push @messages, {
                         type => 'error',
                         code => 'item_not_deleted',