From 3ff3158f5de1341187bfdc351c355a8a2680886e 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 | 15 ++++++--------- 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(+), 29 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index 0f2b161..9c2c834 100644 --- a/C4/Acquisition.pm +++ b/C4/Acquisition.pm @@ -1813,7 +1813,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 cb1a231..2bacac9 100644 --- a/C4/ImportBatch.pm +++ b/C4/ImportBatch.pm @@ -925,7 +925,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 274cfe4..a9d97b4 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -2299,7 +2299,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. @@ -2316,9 +2316,10 @@ returns 1 if the item is safe to delete, =cut sub ItemSafeToDelete { - my ( $dbh, $biblionumber, $itemnumber ) = @_; + my ( $biblionumber, $itemnumber ) = @_; my $status; + my $dbh = C4::Context->dbh; my $countanalytics = GetAnalyticsCount($itemnumber); # check that there is no issue on this item before deletion. @@ -2368,21 +2369,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 19279b3..8f33e9b 100755 --- a/cataloguing/additem.pl +++ b/cataloguing/additem.pl @@ -589,7 +589,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{ @@ -606,7 +606,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 18ea984..45b62df 100644 --- a/t/db_dependent/Circulation/IsItemIssued.t +++ b/t/db_dependent/Circulation/IsItemIssued.t @@ -36,7 +36,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', ); @@ -45,7 +45,7 @@ AddReturn('i_dont_exist', 'CPL'); 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 7301757..ca15a64 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -367,7 +367,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 055e407..6e5a820 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 b0523d5..d20dc3a 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', -- 1.7.10.4