From 3d41eaa9798bcde460daea4671133b31ea20c0e4 Mon Sep 17 00:00:00 2001
From: Barton Chittenden <barton@bywatersolutions.com>
Date: Mon, 26 Oct 2015 09:01:50 -0700
Subject: [PATCH] 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>
---
 C4/Items.pm                         |  10 ++-
 misc/cronjobs/delete_items.pl       |   7 +-
 t/db_dependent/Items_DelItemCheck.t | 135 ++++++++++++++++++------------------
 3 files changed, 76 insertions(+), 76 deletions(-)

diff --git a/C4/Items.pm b/C4/Items.pm
index 6b518c6..f2aad89 100644
--- a/C4/Items.pm
+++ b/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,
diff --git a/misc/cronjobs/delete_items.pl b/misc/cronjobs/delete_items.pl
index 040bdc5..7c2e1cf 100755
--- a/misc/cronjobs/delete_items.pl
+++ b/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";
diff --git a/t/db_dependent/Items_DelItemCheck.t b/t/db_dependent/Items_DelItemCheck.t
index 98e267a..6775a45 100644
--- a/t/db_dependent/Items_DelItemCheck.t
+++ b/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::Mocks;
 
+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;
-- 
2.9.2