From 59ac9be07e70b23c2dfcd009f6d5f689d1065c9b Mon Sep 17 00:00:00 2001 From: Barton Chittenden 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 Signed-off-by: Kyle M Hall --- 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 6b518c6c42..f2aad89201 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 040bdc5909..7c2e1cf04d 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 98e267ab41..6775a45e65 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.39.5