Bugzilla – Attachment 97910 Details for
Bug 23463
Move C4::Items CRUD subroutines to Koha::Item
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Bug-23463-Remove-DelItemCheck-and-ItemSafeToDelete.patch (text/plain), 15.63 KB, created by
Tomás Cohen Arazi (tcohen)
on 2020-01-24 19:45:37 UTC
(
hide
)
Description:
Bug 23463: Remove DelItemCheck and ItemSafeToDelete
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2020-01-24 19:45:37 UTC
Size:
15.63 KB
patch
obsolete
>From 5bef12daf79b5e988828d638accc97e09508f024 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Tue, 6 Aug 2019 11:58:01 -0500 >Subject: [PATCH] Bug 23463: Remove DelItemCheck and ItemSafeToDelete > >--- > 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 | 3 +- > tools/batchMod.pl | 8 +- > tools/batch_delete_records.pl | 8 +- > 12 files changed, 104 insertions(+), 126 deletions(-) > >diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm >index c8bdf1ba83..fb244cadc7 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 7e252da83e..03260cb44b 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 285a604a22..be1d3299b1 100644 >--- a/C4/Items.pm >+++ b/C4/Items.pm >@@ -39,8 +39,6 @@ BEGIN { > GetHostItemsInfo > get_hostitemnumbers_of > GetHiddenItemnumbers >- ItemSafeToDelete >- DelItemCheck > MoveItemFromBiblio > CartToShelf > GetAnalyticsCount >@@ -1364,91 +1362,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 6d335e0c13..c7766fc4ab 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; >@@ -539,6 +589,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 cf1610cd29..1886404f4c 100755 >--- a/cataloguing/additem.pl >+++ b/cataloguing/additem.pl >@@ -668,8 +668,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; >@@ -678,10 +679,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 80c9996781..782e50273f 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 01e1a6683d..c4f5d6df54 100755 >--- a/t/db_dependent/Reserves.t >+++ b/t/db_dependent/Reserves.t >@@ -365,8 +365,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', > ); >diff --git a/tools/batchMod.pl b/tools/batchMod.pl >index 3da1efb80c..8d075b6cf8 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.25.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23463
:
95899
|
95900
|
95901
|
95902
|
95903
|
95904
|
95905
|
95906
|
95907
|
95908
|
95909
|
95910
|
95911
|
95912
|
95913
|
95914
|
95915
|
95916
|
95917
|
95918
|
95919
|
95920
|
95921
|
95922
|
95923
|
95925
|
96145
|
97895
|
97896
|
97897
|
97898
|
97899
|
97900
|
97901
|
97902
|
97903
|
97904
|
97905
|
97906
|
97907
|
97908
|
97909
|
97910
|
97911
|
97912
|
97913
|
97914
|
97915
|
97916
|
97917
|
97918
|
97919
|
97920
|
97921
|
101151
|
101152
|
101153
|
101154
|
101156
|
101157
|
101158
|
101161
|
101163
|
101165
|
101167
|
101169
|
101170
|
101171
|
101172
|
101173
|
101174
|
101175
|
101176
|
101177
|
101178
|
101179
|
101180
|
101181
|
101182
|
101183
|
101184
|
101185
|
101186
|
101188
|
101189
|
101190
|
101191
|
101192
|
101193
|
101194
|
101195
|
101196
|
101197
|
101246
|
101247
|
101248
|
101249
|
101250
|
101251
|
101252
|
101253
|
101254
|
101255
|
101256
|
101257
|
101258
|
101259
|
101260
|
101261
|
101262
|
101263
|
101264
|
101265
|
101266
|
101267
|
101268
|
101269
|
101270
|
101271
|
101272
|
101273
|
101274
|
101275
|
101276
|
101277
|
101278
|
101279
|
101280
|
101281
|
101282
|
101283
|
101284
|
101413
|
101414
|
101415
|
101416
|
101432
|
101476
|
102071
|
103129
|
113581