Bug 14504: (QA followup) use TestBuilder, remove do_not_commit

Use t::lib::TestBuilder in t/db_dependent/Items_DelItemCheck.t

Remove the option 'do_not_commit' from C4::Items::DelItemCheck.

Whitespace cleanup.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

C4/Items.pm

@@ -2299,19 +2299,17 @@ Exported function (core API) for deleting an item record in Koha if there no cur
 
 DelItemCheck wraps ItemSafeToDelete around DelItem.
 
-It takes a database handle, biblionumber and itemnumber as arguments, and can optionally take a hashref with a 'do_not_commit' flag:
+It takes a database handle, biblionumber and itemnumber as arguments:
 
-    DelItemCheck(  $dbh, $biblionumber, $itemnumber, { do_not_commit => 1 } );
+    DelItemCheck(  $dbh, $biblionumber, $itemnumber );
 
-This is done so that command line scripts calling DelItemCheck have the option of doing a 'dry-run'.
 =cut
 
 sub DelItemCheck {
-    my ( $dbh, $biblionumber, $itemnumber, $options ) = @_;
-    my $commit = ( defined $options && $options->{do_not_commit} eq 1 ) ? 0 : 1;
+    my ( $dbh, $biblionumber, $itemnumber ) = @_;
     my $status = ItemSafeToDelete( $dbh, $biblionumber, $itemnumber );
 
-    if ( $status == 1 && $commit ) {
+    if ( $status == 1 ) {
         DelItem(
             {
                 biblionumber => $biblionumber,

misc/cronjobs/delete_items.pl

@@ -56,11 +56,10 @@ $GLOBAL->{sth}->{target_items} = $dbh->prepare( $query->{target_items} . $where_
 $GLOBAL->{sth}->{target_items}->execute();
 
 DELITEM: while ( my $item = $GLOBAL->{sth}->{target_items}->fetchrow_hashref() ) {
-    my $del_check_options = $OPTIONS->{flags}->{commit}
-                            ? undef
-                            : { do_not_commit => 1 };
-    my $status = C4::Items::DelItemCheck( $dbh, $item->{itemnumber}, $item->{biblionumber}, $del_check_options );
+
+    my $status = C4::Items::ItemSafeToDelete( $dbh, $item->{itemnumber}, $item->{biblionumber} );
     if( $status == 1 )  {
+        C4::Items::DelItemCheck( $dbh, $item->{itemnumber}, $item->{biblionumber} );
         verbose "Deleting '$item->{itemnumber}'";
     } else {
         verbose "Item '$item->{itemnumber}' not deletd: $status";

t/db_dependent/Items_DelItemCheck.t

@@ -1,13 +1,11 @@
 use Modern::Perl;
 
-use MARC::Record;
-use C4::Biblio;
 use C4::Circulation;
-use C4::Members;
+
+use t::lib::TestBuilder;
 use t::lib::Mocks;
 
-
-use Test::More tests => 10;
+use Test::More tests => 9;
 
 *C4::Context::userenv = \&Mock_userenv;
 
@@ -16,50 +14,96 @@ BEGIN {
 }
 
 my $dbh = C4::Context->dbh;
-$dbh->{AutoCommit} = 0;
-$dbh->{RaiseError} = 1;
 
-my ( $biblionumber, $bibitemnum ) = get_biblio();
+my $builder = t::lib::TestBuilder->new();
+
+my $branch = $builder->build(
+    {
+        source => 'Branch',
+    }
+);
+
+my $branch2 = $builder->build(
+    {
+        source => 'Branch',
+    }
+);
+
+my $category = $builder->build(
+    {
+        source => 'Category',
+    }
+);
+
+my $patron = $builder->build(
+    {
+        source => 'Borrower',
+        value  => {
+            categorycode => $category->{categorycode},
+            branchcode   => $branch->{branchcode},
+        },
+    }
+);
+
+my $biblio = $builder->build(
+    {
+        source => 'Biblio',
+        value  => {
+            branchcode => $branch->{branchcode},
+        },
+    }
+);
+
+my $item = $builder->build(
+    {
+        source => 'Item',
+        value  => {
+            biblionumber  => $biblio->{biblionumber},
+            homebranch    => $branch->{branchcode},
+            holdingbranch => $branch->{branchcode},
+            withdrawn    => 0,       # randomly assigned value may block return.
+            withdrawn_on => undef,
+        },
+    }
+);
 
 # book_on_loan
 
-my ( $borrowernumber, $borrower ) = get_borrower();
-my ( $itemnumber, $item )         = get_item( $biblionumber );
-AddIssue( $borrower, 'i_dont_exist' );
+AddIssue( $patron, $item->{barcode} );
 
 is(
-    ItemSafeToDelete($dbh, $biblionumber, $itemnumber),
+    ItemSafeToDelete($dbh, $biblio->{biblionumber}, $item->{itemnumber} ),
     'book_on_loan',
     'ItemSafeToDelete reports item on loan',
 );
 
 is(
-    DelItemCheck($dbh, $biblionumber, $itemnumber),
+    DelItemCheck($dbh, $biblio->{biblionumber}, $item->{itemnumber} ),
     'book_on_loan',
     'item that is on loan cannot be deleted',
 );
 
-AddReturn('i_dont_exist', 'CPL');
+AddReturn( $item->{barcode}, $branch->{branchcode} );
 
 # book_reserved is tested in t/db_dependent/Reserves.t
 
 # not_same_branch
 t::lib::Mocks::mock_preference('IndependentBranches', 1);
-ModItem( { homebranch => 'FPL', holdingbranch => 'FPL' }, $biblionumber, $itemnumber );
+ModItem( { homebranch => $branch2->{branchcode}, holdingbranch => $branch2->{branchcode} }, $biblio->{biblionumber}, $item->{itemnumber} );
 
 is(
-    ItemSafeToDelete($dbh, $biblionumber, $itemnumber),
+    ItemSafeToDelete($dbh, $biblio->{biblionumber}, $item->{itemnumber} ),
     'not_same_branch',
     'ItemSafeToDelete reports IndependentBranches restriction',
 );
 
 is(
-    DelItemCheck($dbh, $biblionumber, $itemnumber),
+    DelItemCheck($dbh, $biblio->{biblionumber}, $item->{itemnumber} ),
     'not_same_branch',
     'IndependentBranches prevents deletion at another branch',
 );
 
-ModItem( { homebranch => 'CPL', holdingbranch => 'CPL' }, $biblionumber, $itemnumber );
+ModItem( { homebranch => $branch->{branchcode}, holdingbranch => $branch->{branchcode} }, $biblio->{biblionumber}, $item->{itemnumber} );
 
 # linked_analytics
 
@@ -69,13 +113,13 @@ ModItem( { homebranch => 'CPL', holdingbranch => 'CPL' }, $biblionumber, $itemnu
     $module->mock( GetAnalyticsCount => sub { return 1 } );
 
     is(
-        ItemSafeToDelete($dbh, $biblionumber, $itemnumber),
+        ItemSafeToDelete($dbh, $biblio->{biblionumber}, $item->{itemnumber} ),
         'linked_analytics',
         'ItemSafeToDelete reports linked analytics',
     );
 
     is(
-        DelItemCheck($dbh, $biblionumber, $itemnumber),
+        DelItemCheck($dbh, $biblio->{biblionumber}, $item->{itemnumber} ),
         'linked_analytics',
         'Linked analytics prevents deletion of item',
     );
@@ -83,63 +127,22 @@ ModItem( { homebranch => 'CPL', holdingbranch => 'CPL' }, $biblionumber, $itemnu
 }
 
 is(
-    ItemSafeToDelete($dbh, $biblionumber, $itemnumber),
+    ItemSafeToDelete($dbh, $biblio->{biblionumber}, $item->{itemnumber} ),
     1,
     'ItemSafeToDelete shows item safe to delete'
 );
 
-DelItemCheck($dbh, $biblionumber, $itemnumber, { do_not_commit => 1 } );
+DelItemCheck( $dbh, $biblio->{biblionumber}, $item->{itemnumber} );
 
-my $testitem = GetItem( $itemnumber );
+my $test_item = GetItem( $item->{itemnumber} );
 
-is( $testitem->{itemnumber} ,  $itemnumber,
-    "DelItemCheck should not delete item if 'do_not_commit' is set"
-);
-
-DelItemCheck( $dbh, $biblionumber, $itemnumber );
-
-$testitem = GetItem( $itemnumber );
-
-is( $testitem->{itemnumber}, undef,
+is( $test_item->{itemnumber}, undef,
     "DelItemCheck should delete item if 'do_not_commit' not set"
 );
 
 # End of testing
 
-# Helper methods to set up Biblio, Item, and Borrower.
-sub get_biblio {
-    my $bib = MARC::Record->new();
-    $bib->append_fields(
-        MARC::Field->new( '100', ' ', ' ', a => 'Moffat, Steven' ),
-        MARC::Field->new( '245', ' ', ' ', a => 'Silence in the library' ),
-    );
-    my ( $bibnum, $bibitemnum ) = AddBiblio( $bib, '' );
-    return ( $bibnum, $bibitemnum );
-}
-
-sub get_item {
-    my $biblionumber = shift;
-    my ( $item_bibnum, $item_bibitemnum, $itemnumber ) =
-      AddItem( { homebranch => 'CPL', holdingbranch => 'CPL', barcode => 'i_dont_exist' }, $biblionumber );
-    my $item = GetItem( $itemnumber );
-    return ( $itemnumber, $item );
-}
-
-sub get_borrower {
-    my $borrowernumber = AddMember(
-        firstname =>  'my firstname',
-        surname => 'my surname',
-        categorycode => 'S',
-        branchcode => 'CPL',
-    );
-
-    my $borrower = GetMember( borrowernumber => $borrowernumber );
-    return ( $borrowernumber, $borrower );
-}
-
 # C4::Context->userenv
 sub Mock_userenv {
-        return { flags => 0, branch => 'CPL' };
+    return { flags => 0, branch => $branch->{branchcode} };
 }
-
-$dbh->rollback;