From d340bc27991966f1aa61b989515258a9a03aa7ca Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 6 Aug 2019 11:58:01 -0500 Subject: [PATCH] Bug 23463: Remove DelItemCheck and ItemSafeToDelete Signed-off-by: Signed-off-by: Tomas Cohen Arazi Signed-off-by: Signed-off-by: Nick Clemens --- C4/Acquisition.pm | 4 +- C4/ImportBatch.pm | 5 +- C4/Items.pm | 87 ---------------------- Koha/Item.pm | 60 +++++++++++++++ cataloguing/additem.pl | 13 ++-- misc/cronjobs/delete_items.pl | 10 ++- misc/cronjobs/delete_records_via_leader.pl | 2 +- t/db_dependent/Circulation/IsItemIssued.t | 4 +- t/db_dependent/Items_DelItemCheck.t | 26 +++---- t/db_dependent/Reserves.t | 4 +- tools/batchMod.pl | 8 +- tools/batch_delete_records.pl | 8 +- 12 files changed, 104 insertions(+), 127 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index e7f489fb29..ffca1ffd4d 100644 --- a/C4/Acquisition.pm +++ b/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; } } diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm index eaf8b55c52..739f9f6786 100644 --- a/C4/ImportBatch.pm +++ b/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'}); diff --git a/C4/Items.pm b/C4/Items.pm index 2703bc10b0..8df39046cd 100644 --- a/C4/Items.pm +++ b/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'; diff --git a/Koha/Item.pm b/Koha/Item.pm index 91119bd9b6..0aeaee15b3 100644 --- a/Koha/Item.pm +++ b/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; diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl index 616e55a279..4281658612 100755 --- a/cataloguing/additem.pl +++ b/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 ) { diff --git a/misc/cronjobs/delete_items.pl b/misc/cronjobs/delete_items.pl index 6c2962a3af..5ca182daf1 100755 --- a/misc/cronjobs/delete_items.pl +++ b/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"; } } diff --git a/misc/cronjobs/delete_records_via_leader.pl b/misc/cronjobs/delete_records_via_leader.pl index e65fbbf11d..4e5315c6bc 100755 --- a/misc/cronjobs/delete_records_via_leader.pl +++ b/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) { diff --git a/t/db_dependent/Circulation/IsItemIssued.t b/t/db_dependent/Circulation/IsItemIssued.t index 32fd06044d..0baf8bd9a7 100644 --- a/t/db_dependent/Circulation/IsItemIssued.t +++ b/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', ); diff --git a/t/db_dependent/Items_DelItemCheck.t b/t/db_dependent/Items_DelItemCheck.t index 3ccd492fa2..51bd731b42 100644 --- a/t/db_dependent/Items_DelItemCheck.t +++ b/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; diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index d352472073..d3884c0152 100755 --- a/t/db_dependent/Reserves.t +++ b/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 ) } } ); diff --git a/tools/batchMod.pl b/tools/batchMod.pl index f9fb0a5f7c..eedf9b827a 100755 --- a/tools/batchMod.pl +++ b/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 { diff --git a/tools/batch_delete_records.pl b/tools/batch_delete_records.pl index 1f0e56a45e..7604a96652 100755 --- a/tools/batch_delete_records.pl +++ b/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', -- 2.20.1