From fb542fb48223d99f8299b77f6e2c5681c2aed739 Mon Sep 17 00:00:00 2001 From: Barton Chittenden Date: Thu, 29 Oct 2015 06:06:08 -0700 Subject: [PATCH] bug 14504: (QA followup) fixing DelItemCheck arguments Remove $dbh as argument to C4::Items::DelItemCheck and C4::Items::ItemSafeToDelete, also change all calls to these functions throughout the codebase. Also remove remaining reference to 'do_not_commit' in t/db_dependent/Items_DelItemCheck.t Fixed doubled "$$" in C4/ImportBatch.pm --- C4/Acquisition.pm | 2 +- C4/ImportBatch.pm | 2 +- C4/Items.pm | 16 ++++++---------- cataloguing/additem.pl | 4 ++-- misc/cronjobs/delete_items.pl | 4 ++-- t/db_dependent/Circulation/IsItemIssued.t | 4 ++-- t/db_dependent/Items_DelItemCheck.t | 18 +++++++++--------- t/db_dependent/Reserves.t | 2 +- tools/batchMod.pl | 2 +- tools/batch_delete_records.pl | 2 +- 10 files changed, 26 insertions(+), 30 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index 45554da..ede475a 100644 --- a/C4/Acquisition.pm +++ b/C4/Acquisition.pm @@ -1800,7 +1800,7 @@ sub DelOrder { my @itemnumbers = GetItemnumbersFromOrder( $ordernumber ); foreach my $itemnumber (@itemnumbers){ - my $delcheck = C4::Items::DelItemCheck( $dbh, $bibnum, $itemnumber ); + my $delcheck = C4::Items::DelItemCheck( $bibnum, $itemnumber ); if($delcheck != 1) { $error->{'delitem'} = 1; diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm index 4d63e8d..046b859 100644 --- a/C4/ImportBatch.pm +++ b/C4/ImportBatch.pm @@ -924,7 +924,7 @@ sub BatchRevertItems { $sth->bind_param(1, $import_record_id); $sth->execute(); while (my $row = $sth->fetchrow_hashref()) { - my $error = DelItemCheck($dbh, $biblionumber, $row->{'itemnumber'}); + my $error = DelItemCheck( $biblionumber, $row->{'itemnumber'}); if ($error == 1){ my $updsth = $dbh->prepare("UPDATE import_items SET status = ? WHERE import_items_id = ?"); $updsth->bind_param(1, 'reverted'); diff --git a/C4/Items.pm b/C4/Items.pm index f2aad89..2f31897 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -2220,7 +2220,7 @@ sub MoveItemFromBiblio { =head2 ItemSafeToDelete - ItemSafeToDelete($dbh, $biblionumber, $itemnumber); + ItemSafeToDelete( $biblionumber, $itemnumber); Exported function (core API) for checking whether an item record is safe to delete. @@ -2237,9 +2237,9 @@ returns 1 if the item is safe to delete, =cut sub ItemSafeToDelete { - my ( $dbh, $biblionumber, $itemnumber ) = @_; + my ( $biblionumber, $itemnumber ) = @_; my $status; - $dbh ||= C4::Context->dbh; + my $dbh = C4::Context->dbh; my $error; @@ -2293,21 +2293,17 @@ sub ItemSafeToDelete { =head2 DelItemCheck - DelItemCheck($dbh, $biblionumber, $itemnumber); + DelItemCheck( $biblionumber, $itemnumber); Exported function (core API) for deleting an item record in Koha if there no current issue. DelItemCheck wraps ItemSafeToDelete around DelItem. -It takes a database handle, biblionumber and itemnumber as arguments: - - DelItemCheck( $dbh, $biblionumber, $itemnumber ); - =cut sub DelItemCheck { - my ( $dbh, $biblionumber, $itemnumber ) = @_; - my $status = ItemSafeToDelete( $dbh, $biblionumber, $itemnumber ); + my ( $biblionumber, $itemnumber ) = @_; + my $status = ItemSafeToDelete( $biblionumber, $itemnumber ); if ( $status == 1 ) { DelItem( diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl index 28fd247..1b4bd58 100755 --- a/cataloguing/additem.pl +++ b/cataloguing/additem.pl @@ -622,7 +622,7 @@ if ($op eq "additem") { } elsif ($op eq "delitem") { #------------------------------------------------------------------------------- # check that there is no issue on this item before deletion. - $error = &DelItemCheck($dbh,$biblionumber,$itemnumber); + $error = &DelItemCheck( $biblionumber,$itemnumber); if($error == 1){ print $input->redirect("additem.pl?biblionumber=$biblionumber&frameworkcode=$frameworkcode&searchid=$searchid"); }else{ @@ -639,7 +639,7 @@ if ($op eq "additem") { my $items = &GetItemsByBiblioitemnumber( $biblioitem->{biblioitemnumber} ); foreach my $item (@$items) { - $error =&DelItemCheck( $dbh, $biblionumber, $item->{itemnumber} ); + $error =&DelItemCheck( $biblionumber, $item->{itemnumber} ); $itemfail =$item; if($error == 1){ next diff --git a/misc/cronjobs/delete_items.pl b/misc/cronjobs/delete_items.pl index 7c2e1cf..f3d3de6 100755 --- a/misc/cronjobs/delete_items.pl +++ b/misc/cronjobs/delete_items.pl @@ -57,9 +57,9 @@ $GLOBAL->{sth}->{target_items}->execute(); DELITEM: while ( my $item = $GLOBAL->{sth}->{target_items}->fetchrow_hashref() ) { - my $status = C4::Items::ItemSafeToDelete( $dbh, $item->{itemnumber}, $item->{biblionumber} ); + my $status = C4::Items::ItemSafeToDelete( $item->{itemnumber}, $item->{biblionumber} ); if( $status == 1 ) { - C4::Items::DelItemCheck( $dbh, $item->{itemnumber}, $item->{biblionumber} ); + C4::Items::DelItemCheck( $item->{itemnumber}, $item->{biblionumber} ); verbose "Deleting '$item->{itemnumber}'"; } else { verbose "Item '$item->{itemnumber}' not deletd: $status"; diff --git a/t/db_dependent/Circulation/IsItemIssued.t b/t/db_dependent/Circulation/IsItemIssued.t index 2b7e67e..907a3cf 100644 --- a/t/db_dependent/Circulation/IsItemIssued.t +++ b/t/db_dependent/Circulation/IsItemIssued.t @@ -44,7 +44,7 @@ AddIssue($borrower, 'i_dont_exist'); is ( IsItemIssued( $item->{itemnumber} ), 1, "item is now on loan" ); is( - DelItemCheck($dbh, $biblionumber, $itemnumber), + DelItemCheck( $biblionumber, $itemnumber), 'book_on_loan', 'item that is on loan cannot be deleted', ); @@ -53,7 +53,7 @@ AddReturn('i_dont_exist', $library->{branchcode}); is ( IsItemIssued( $item->{itemnumber} ), 0, "item has been returned" ); is( - DelItemCheck($dbh, $biblionumber, $itemnumber), + DelItemCheck( $biblionumber, $itemnumber), 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 6775a45..abe6600 100644 --- a/t/db_dependent/Items_DelItemCheck.t +++ b/t/db_dependent/Items_DelItemCheck.t @@ -72,13 +72,13 @@ my $item = $builder->build( AddIssue( $patron, $item->{barcode} ); is( - ItemSafeToDelete($dbh, $biblio->{biblionumber}, $item->{itemnumber} ), + ItemSafeToDelete( $biblio->{biblionumber}, $item->{itemnumber} ), 'book_on_loan', 'ItemSafeToDelete reports item on loan', ); is( - DelItemCheck($dbh, $biblio->{biblionumber}, $item->{itemnumber} ), + DelItemCheck( $biblio->{biblionumber}, $item->{itemnumber} ), 'book_on_loan', 'item that is on loan cannot be deleted', ); @@ -92,13 +92,13 @@ t::lib::Mocks::mock_preference('IndependentBranches', 1); ModItem( { homebranch => $branch2->{branchcode}, holdingbranch => $branch2->{branchcode} }, $biblio->{biblionumber}, $item->{itemnumber} ); is( - ItemSafeToDelete($dbh, $biblio->{biblionumber}, $item->{itemnumber} ), + ItemSafeToDelete( $biblio->{biblionumber}, $item->{itemnumber} ), 'not_same_branch', 'ItemSafeToDelete reports IndependentBranches restriction', ); is( - DelItemCheck($dbh, $biblio->{biblionumber}, $item->{itemnumber} ), + DelItemCheck( $biblio->{biblionumber}, $item->{itemnumber} ), 'not_same_branch', 'IndependentBranches prevents deletion at another branch', ); @@ -113,13 +113,13 @@ ModItem( { homebranch => $branch->{branchcode}, holdingbranch => $branch->{branc $module->mock( GetAnalyticsCount => sub { return 1 } ); is( - ItemSafeToDelete($dbh, $biblio->{biblionumber}, $item->{itemnumber} ), + ItemSafeToDelete( $biblio->{biblionumber}, $item->{itemnumber} ), 'linked_analytics', 'ItemSafeToDelete reports linked analytics', ); is( - DelItemCheck($dbh, $biblio->{biblionumber}, $item->{itemnumber} ), + DelItemCheck( $biblio->{biblionumber}, $item->{itemnumber} ), 'linked_analytics', 'Linked analytics prevents deletion of item', ); @@ -127,17 +127,17 @@ ModItem( { homebranch => $branch->{branchcode}, holdingbranch => $branch->{branc } is( - ItemSafeToDelete($dbh, $biblio->{biblionumber}, $item->{itemnumber} ), + ItemSafeToDelete( $biblio->{biblionumber}, $item->{itemnumber} ), 1, 'ItemSafeToDelete shows item safe to delete' ); -DelItemCheck( $dbh, $biblio->{biblionumber}, $item->{itemnumber} ); +DelItemCheck( $biblio->{biblionumber}, $item->{itemnumber} ); my $test_item = GetItem( $item->{itemnumber} ); is( $test_item->{itemnumber}, undef, - "DelItemCheck should delete item if 'do_not_commit' not set" + "DelItemCheck should delete item if ItemSafeToDelete returns true" ); # End of testing diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 1852ae6..db13f82 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -371,7 +371,7 @@ 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); is( - DelItemCheck($dbh, $bibnum, $itemnumber), + DelItemCheck( $bibnum, $itemnumber), 'book_reserved', 'item that is captured to fill a hold cannot be deleted', ); diff --git a/tools/batchMod.pl b/tools/batchMod.pl index 0961ba5..0ef4248 100755 --- a/tools/batchMod.pl +++ b/tools/batchMod.pl @@ -166,7 +166,7 @@ if ($op eq "action") { $job->progress($i) if $runinbackground; my $itemdata = GetItem($itemnumber); if ( $del ){ - my $return = DelItemCheck(C4::Context->dbh, $itemdata->{'biblionumber'}, $itemdata->{'itemnumber'}); + my $return = DelItemCheck( $itemdata->{'biblionumber'}, $itemdata->{'itemnumber'}); if ($return == 1) { $deleted_items++; } else { diff --git a/tools/batch_delete_records.pl b/tools/batch_delete_records.pl index a5d59d3..2c2d32e 100755 --- a/tools/batch_delete_records.pl +++ b/tools/batch_delete_records.pl @@ -159,7 +159,7 @@ if ( $op eq 'form' ) { # Delete items my @itemnumbers = @{ C4::Items::GetItemnumbersForBiblio( $biblionumber ) }; ITEMNUMBER: for my $itemnumber ( @itemnumbers ) { - my $error = eval { C4::Items::DelItemCheck( $dbh, $biblionumber, $itemnumber ) }; + my $error = eval { C4::Items::DelItemCheck( $biblionumber, $itemnumber ) }; if ( $error != 1 or $@ ) { push @messages, { type => 'error', -- 2.1.4