From 9e08e6e86fdbbf665a44f5dea57dc87c693a2116 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Fri, 21 Jun 2019 09:00:27 +0000 Subject: [PATCH] Bug 23177: [18.11.x] (QA follow-up) Move three subs from the middle to the top in Circulation.t Removed trailing comma for last sub too. Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize Bug 23177: (QA follow-up) Move rollback to the end Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize Bug 23177: (QA follow-up) Remove subtest txn_begin and rollback This resolves: DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction is ineffective! (depth 1) Removing unneeded subtest txn's. We should probably add them everywhere or just at global level. Test plan: With all three patches applied, run Circulation.t a few times. Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize Bug 23177: (RM follow-up) Further test clarifications This patch makes a few minor improvements to Circulation.t 1) Adds a name to some of the scoped blocks by converting them to subtests. 2) Adds output messages to some tests where they were missing. Signed-off-by: Martin Renvoize Bug 23177: Rebase fixes --- t/db_dependent/Circulation.t | 158 +++++++++++++++++++++---------------------- 1 file changed, 78 insertions(+), 80 deletions(-) diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 156be69c0b..eca86fd357 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -18,7 +18,8 @@ use Modern::Perl; use utf8; -use Test::More tests => 123; +use Test::More tests => 40; +use Test::MockModule; use Data::Dumper; use DateTime; @@ -43,6 +44,53 @@ use Koha::Subscriptions; use Koha::Account::Lines; use Koha::Account::Offsets; +sub set_userenv { + my ( $library ) = @_; + t::lib::Mocks::mock_userenv({ branchcode => $library->{branchcode} }); +} + +sub str { + my ( $error, $question, $alert ) = @_; + my $s; + $s = %$error ? ' (error: ' . join( ' ', keys %$error ) . ')' : ''; + $s .= %$question ? ' (question: ' . join( ' ', keys %$question ) . ')' : ''; + $s .= %$alert ? ' (alert: ' . join( ' ', keys %$alert ) . ')' : ''; + return $s; +} + +sub test_debarment_on_checkout { + my ($params) = @_; + my $item = $params->{item}; + my $library = $params->{library}; + my $patron = $params->{patron}; + my $due_date = $params->{due_date} || dt_from_string; + my $return_date = $params->{return_date} || dt_from_string; + my $expected_expiration_date = $params->{expiration_date}; + + $expected_expiration_date = output_pref( + { + dt => $expected_expiration_date, + dateformat => 'sql', + dateonly => 1, + } + ); + my @caller = caller; + my $line_number = $caller[2]; + AddIssue( $patron, $item->{barcode}, $due_date ); + + my ( undef, $message ) = AddReturn( $item->{barcode}, $library->{branchcode}, undef, undef, $return_date ); + is( $message->{WasReturned} && exists $message->{Debarred}, 1, 'AddReturn must have debarred the patron' ) + or diag('AddReturn returned message ' . Dumper $message ); + my $debarments = Koha::Patron::Debarments::GetDebarments( + { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); + is( scalar(@$debarments), 1, 'Test at line ' . $line_number ); + + is( $debarments->[0]->{expiration}, + $expected_expiration_date, 'Test at line ' . $line_number ); + Koha::Patron::Debarments::DelUniqueDebarment( + { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); +}; + my $schema = Koha::Database->schema; $schema->storage->txn_begin; my $builder = t::lib::TestBuilder->new; @@ -214,8 +262,9 @@ $dbh->do( ); my ( $reused_itemnumber_1, $reused_itemnumber_2 ); -{ -# CanBookBeRenewed tests +subtest "CanBookBeRenewed tests" => sub { + plan tests => 68; + C4::Context->set_preference('ItemsDeniedRenewal',''); # Generate test biblio my $biblio = $builder->build_sample_biblio(); @@ -485,7 +534,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); is ($new_log_size, $old_log_size + 1, 'renew log successfully added'); my $fines = Koha::Account::Lines->search( { borrowernumber => $renewing_borrower->{borrowernumber}, itemnumber => $item_7->itemnumber } ); - is( $fines->count, 2 ); + is( $fines->count, 2, 'AddRenewal left both fines' ); is( $fines->next->accounttype, 'F', 'Fine on renewed item is closed out properly' ); is( $fines->next->accounttype, 'F', 'Fine on renewed item is closed out properly' ); $fines->delete(); @@ -883,10 +932,11 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); LostItem( $item_3->itemnumber, 'test', 0 ); my $accountline = Koha::Account::Lines->find( { itemnumber => $item_3->itemnumber } ); is( $accountline->issue_id, $checkout->id, "Issue id added for lost replacement fee charge" ); - } +}; + +subtest "GetUpcomingDueIssues" => sub { + plan tests => 12; -{ - # GetUpcomingDueIssues tests my $branch = $library2->{branchcode}; #Create another record @@ -964,9 +1014,9 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); $upcoming_dues = C4::Circulation::GetUpcomingDueIssues(); is ( scalar ( @$upcoming_dues), 2, "days_in_advance is 7 in GetUpcomingDueIssues if not provided" ); -} +}; -{ +subtest "Bug 13841 - Do not create new 0 amount fines" => sub { my $branch = $library2->{branchcode}; my $biblio = $builder->build_sample_biblio(); @@ -1006,9 +1056,9 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); my $count = $hr->{count}; is ( $count, 0, "Calling UpdateFine on non-existant fine with an amount of 0 does not result in an empty fine" ); -} +}; -{ +subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { $dbh->do('DELETE FROM issues'); $dbh->do('DELETE FROM items'); $dbh->do('DELETE FROM issuingrules'); @@ -1094,7 +1144,7 @@ my ( $reused_itemnumber_1, $reused_itemnumber_2 ); ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); is( $renewokay, 0, 'Bug 14337 - Verify the borrower can not renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled but the only available item is notforloan' ); -} +}; { # Don't allow renewing onsite checkout @@ -1218,7 +1268,7 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub { ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} ); is( keys(%$question) + keys(%$alerts), 0, 'There should not be any errors or alerts (impossible)' . str($error, $question, $alerts) ); is( exists $error->{RETURN_IMPOSSIBLE}, 1, 'RETURN_IMPOSSIBLE must be set' ); - is( $error->{branch_to_return}, $holdingbranch->{branchcode} ); + is( $error->{branch_to_return}, $holdingbranch->{branchcode}, 'branch_to_return matched holdingbranch' ); ## Can be issued from holdinbranch set_userenv($holdingbranch); ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} ); @@ -1229,7 +1279,7 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub { ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} ); is( keys(%$question) + keys(%$alerts), 0, 'There should not be any errors or alerts (impossible)' . str($error, $question, $alerts) ); is( exists $error->{RETURN_IMPOSSIBLE}, 1, 'RETURN_IMPOSSIBLE must be set' ); - is( $error->{branch_to_return}, $holdingbranch->{branchcode} ); + is( $error->{branch_to_return}, $holdingbranch->{branchcode}, 'branch_to_return matches holdingbranch' ); # AllowReturnToBranch == homebranch t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'homebranch' ); @@ -1243,13 +1293,13 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub { ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} ); is( keys(%$question) + keys(%$alerts), 0, 'There should not be any errors or alerts (impossible)' . str($error, $question, $alerts) ); is( exists $error->{RETURN_IMPOSSIBLE}, 1, 'RETURN_IMPOSSIBLE must be set' ); - is( $error->{branch_to_return}, $homebranch->{branchcode} ); + is( $error->{branch_to_return}, $homebranch->{branchcode}, 'branch_to_return matches homebranch' ); ## Cannot be issued from holdinbranch set_userenv($otherbranch); ( $error, $question, $alerts ) = CanBookBeIssued( $patron_2, $item->{barcode} ); is( keys(%$question) + keys(%$alerts), 0, 'There should not be any errors or alerts (impossible)' . str($error, $question, $alerts) ); is( exists $error->{RETURN_IMPOSSIBLE}, 1, 'RETURN_IMPOSSIBLE must be set' ); - is( $error->{branch_to_return}, $homebranch->{branchcode} ); + is( $error->{branch_to_return}, $homebranch->{branchcode}, 'branch_to_return matches homebranch' ); # TODO t::lib::Mocks::mock_preference('AllowReturnToBranch', 'homeorholdingbranch'); }; @@ -1288,42 +1338,42 @@ subtest 'AddIssue & AllowReturnToBranch' => sub { t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); ## Can be issued from homebranch set_userenv($homebranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - anywhere | Can be issued from homebranch'); set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue ## Can be issued from holdinbranch set_userenv($holdingbranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - anywhere | Can be issued from holdingbranch'); set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue ## Can be issued from another branch set_userenv($otherbranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - anywhere | Can be issued from otherbranch'); set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue # AllowReturnToBranch == holdinbranch t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'holdingbranch' ); ## Cannot be issued from homebranch set_userenv($homebranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '' ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '', 'AllowReturnToBranch - holdingbranch | Cannot be issued from homebranch'); ## Can be issued from holdingbranch set_userenv($holdingbranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - holdingbranch | Can be issued from holdingbranch'); set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue ## Cannot be issued from another branch set_userenv($otherbranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '' ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '', 'AllowReturnToBranch - holdingbranch | Cannot be issued from otherbranch'); # AllowReturnToBranch == homebranch t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'homebranch' ); ## Can be issued from homebranch set_userenv($homebranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), $ref_issue, 'AllowReturnToBranch - homebranch | Can be issued from homebranch' ); set_userenv($holdingbranch); AddIssue( $patron_1, $item->{barcode} ); # Reinsert the original issue ## Cannot be issued from holdinbranch set_userenv($holdingbranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '' ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '', 'AllowReturnToBranch - homebranch | Cannot be issued from holdingbranch' ); ## Cannot be issued from another branch set_userenv($otherbranch); - is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '' ); + is ( ref( AddIssue( $patron_2, $item->{barcode} ) ), '', 'AllowReturnToBranch - homebranch | Cannot be issued from otherbranch' ); # TODO t::lib::Mocks::mock_preference('AllowReturnToBranch', 'homeorholdingbranch'); }; @@ -2772,15 +2822,11 @@ subtest 'AddReturn should clear items.onloan for unissued items' => sub { is( $item->onloan, undef, 'AddReturn did clear items.onloan' ); }; -$schema->storage->txn_rollback; -C4::Context->clear_syspref_cache(); -$cache->clear_from_cache('single_holidays'); subtest 'AddRenewal and AddIssuingCharge tests' => sub { plan tests => 10; - $schema->storage->txn_begin; my $issuing_charges = 15; my $title = 'A title'; @@ -2860,14 +2906,12 @@ subtest 'AddRenewal and AddIssuingCharge tests' => sub { is( $line->accounttype, 'Rent', 'Fine on renewed item is closed out properly' ); is( $line->description, "Renewal of Rental Item $title $barcode", 'AddRenewal set a hardcoded description for the accountline' ); - $schema->storage->txn_rollback; }; subtest 'ProcessOfflinePayment() tests' => sub { plan tests => 3; - $schema->storage->txn_begin; my $amount = 123; @@ -2883,55 +2927,9 @@ subtest 'ProcessOfflinePayment() tests' => sub { my $line = $lines->next; is( $line->amount+0, $amount * -1, 'amount picked from params' ); - $schema->storage->txn_rollback; }; - -sub set_userenv { - my ( $library ) = @_; - t::lib::Mocks::mock_userenv({ branchcode => $library->{branchcode} }); -} - -sub str { - my ( $error, $question, $alert ) = @_; - my $s; - $s = %$error ? ' (error: ' . join( ' ', keys %$error ) . ')' : ''; - $s .= %$question ? ' (question: ' . join( ' ', keys %$question ) . ')' : ''; - $s .= %$alert ? ' (alert: ' . join( ' ', keys %$alert ) . ')' : ''; - return $s; -} - -sub test_debarment_on_checkout { - my ($params) = @_; - my $item = $params->{item}; - my $library = $params->{library}; - my $patron = $params->{patron}; - my $due_date = $params->{due_date} || dt_from_string; - my $return_date = $params->{return_date} || dt_from_string; - my $expected_expiration_date = $params->{expiration_date}; - - $expected_expiration_date = output_pref( - { - dt => $expected_expiration_date, - dateformat => 'sql', - dateonly => 1, - } - ); - my @caller = caller; - my $line_number = $caller[2]; - AddIssue( $patron, $item->{barcode}, $due_date ); - - my ( undef, $message ) = AddReturn( $item->{barcode}, $library->{branchcode}, - undef, undef, $return_date ); - is( $message->{WasReturned} && exists $message->{Debarred}, 1, 'AddReturn must have debarred the patron' ) - or diag('AddReturn returned message ' . Dumper $message ); - my $debarments = Koha::Patron::Debarments::GetDebarments( - { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); - is( scalar(@$debarments), 1, 'Test at line ' . $line_number ); - - is( $debarments->[0]->{expiration}, - $expected_expiration_date, 'Test at line ' . $line_number ); - Koha::Patron::Debarments::DelUniqueDebarment( - { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); -} +$schema->storage->txn_rollback; +C4::Context->clear_syspref_cache(); +$cache->clear_from_cache('single_holidays'); -- 2.11.0