From 4a13aeb1c3fddd962324166df6cf13d9788db53a Mon Sep 17 00:00:00 2001 From: Baptiste Wojtkowski Date: Thu, 19 Dec 2024 11:29:17 +0100 Subject: [PATCH] Bug 7376: (follow-up) Add tests on no_return Note: tests have to be performed on addReturn and have therefore been added in a separate test Signed-off-by: Baptiste Wojtkowski Signed-off-by: Mathieu Saby Rebased-by: Victor Grousset/tuxayo --- C4/Circulation.pm | 7 +- t/db_dependent/Circulation.t | 137 +++++++++++++++++++++++++++++++---- 2 files changed, 128 insertions(+), 16 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 86f36379dc..15dd065488 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1385,6 +1385,8 @@ Check whether the item can be returned to the provided branch =item C<$branch> is the branchcode where the return is taking place +=item C<$transferbranch> the branch where the book should be transferred after return, if the transfer is impossible, the book will be prevented from returning. + =back Returns: @@ -1403,11 +1405,10 @@ sub CanBookBeReturned { my ( $item, $returnbranch, $transferbranch ) = @_; my $allowreturntobranch = C4::Context->preference("AllowReturnToBranch") || 'anywhere'; - # assume return is allowed to start my $allowed = 1; my $message; - # identify all cases where return is forbidden + # Refuse check-in if it does not respect AllowReturnToBranch rules if ( $allowreturntobranch eq 'homebranch' && $returnbranch ne $item->homebranch ) { $allowed = 0; $message = $item->homebranch; @@ -1422,7 +1423,7 @@ sub CanBookBeReturned { $message = $item->homebranch; # FIXME: choice of homebranch is arbitrary } - # identify cases where transfer rules prohibit return + # Refuse check-in if book cannot be transferred to $transferbranch due to transfer limits if ( defined($transferbranch) && $allowed ) { my $from_library = Koha::Libraries->find($returnbranch); my $to_library = Koha::Libraries->find($transferbranch); diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 375c351cd2..39b0f69e3b 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -19,7 +19,7 @@ use Modern::Perl; use utf8; use Test::NoWarnings; -use Test::More tests => 84; +use Test::More tests => 85; use Test::Exception; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -1185,7 +1185,7 @@ subtest "CanBookBeRenewed tests" => sub { ); my $ten_days_before = dt_from_string->add( days => -10 ); - my $ten_days_ahead = dt_from_string->add( days => 10 ); + my $ten_days_ahead = dt_from_string->add( days => 10 ); my $issue = AddIssue( $renewing_borrower_obj, $item_to_auto_renew->barcode, $ten_days_ahead, undef, $ten_days_before, undef, { auto_renew => 1 } @@ -1335,7 +1335,7 @@ subtest "CanBookBeRenewed tests" => sub { ); my $ten_days_before = dt_from_string->add( days => -10 ); - my $ten_days_ahead = dt_from_string->add( days => 10 ); + my $ten_days_ahead = dt_from_string->add( days => 10 ); my $issue = AddIssue( $renewing_borrower_obj, $item_to_auto_renew->barcode, $ten_days_ahead, undef, $ten_days_before, undef, { auto_renew => 1 } @@ -1445,7 +1445,7 @@ subtest "CanBookBeRenewed tests" => sub { ); my $ten_days_before = dt_from_string->add( days => -10 ); - my $ten_days_ahead = dt_from_string->add( days => 10 ); + my $ten_days_ahead = dt_from_string->add( days => 10 ); # Patron is expired and BlockExpiredPatronOpacActions='' # => auto renew is allowed @@ -1497,7 +1497,7 @@ subtest "CanBookBeRenewed tests" => sub { ); my $ten_days_before = dt_from_string->add( days => -10 ); - my $ten_days_ahead = dt_from_string->add( days => 10 ); + my $ten_days_ahead = dt_from_string->add( days => 10 ); my $issue = AddIssue( $renewing_borrower_obj, $item_to_auto_renew->barcode, $ten_days_ahead, undef, $ten_days_before, undef, { auto_renew => 1 } @@ -1618,7 +1618,7 @@ subtest "CanBookBeRenewed tests" => sub { ); my $ten_days_before = dt_from_string->add( days => -10 ); - my $ten_days_ahead = dt_from_string->add( days => 10 ); + my $ten_days_ahead = dt_from_string->add( days => 10 ); my $issue = AddIssue( $renewing_borrower_obj, $item_to_auto_renew->barcode, $ten_days_ahead, undef, $ten_days_before, undef, { auto_renew => 1 } @@ -1969,7 +1969,7 @@ subtest "GetUpcomingDueIssues" => sub { my $a_borrower = Koha::Patrons->find($a_borrower_borrowernumber); my $yesterday = DateTime->today( time_zone => C4::Context->tz() )->add( days => -1 ); - my $two_days_ahead = DateTime->today( time_zone => C4::Context->tz() )->add( days => 2 ); + my $two_days_ahead = DateTime->today( time_zone => C4::Context->tz() )->add( days => 2 ); my $today = DateTime->today( time_zone => C4::Context->tz() ); my $issue = AddIssue( $a_borrower, $item_1->barcode, $yesterday ); @@ -2961,6 +2961,119 @@ subtest 'CanBookBeReturned + UseBranchTransfertLimits + CirculationRules' => sub is( $holdingbranch->{branchcode}, $message, 'Redirect to holdingbranch' ); }; +subtest 'AddReturn + TransferLimits' => sub { + plan tests => 3; + + ######################################################################## + # + # Prepare test + # + ######################################################################## + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $homebranch = $builder->build( { source => 'Branch' } ); + my $holdingbranch = $builder->build( { source => 'Branch' } ); + my $returnbranch = $builder->build( { source => 'Branch' } ); + my $item = $builder->build_sample_item( + { + homebranch => $homebranch->{branchcode}, + holdingbranch => $holdingbranch->{branchcode}, + } + ); + + # Default circulation rules + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => $item->itype, + rules => { + maxissueqty => 1, + reservesallowed => 25, + issuelength => 7, + lengthunit => 'days', + renewalsallowed => 5, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + } + } + ); + t::lib::Mocks::mock_userenv( { branchcode => $holdingbranch->{branchcode} } ); + + # Each transfer from returnbranch is forbidden + my $limit = Koha::Item::Transfer::Limit->new( + { + fromBranch => $returnbranch->{branchcode}, + toBranch => $holdingbranch->{branchcode}, + itemtype => $item->effective_itemtype, + } + )->store(); + my $limit2 = Koha::Item::Transfer::Limit->new( + { + fromBranch => $returnbranch->{branchcode}, + toBranch => $homebranch->{branchcode}, + itemtype => $item->effective_itemtype, + } + )->store(); + + ######################################################################## + # + # Begin test + # + # Each transfer is forbidden by transfer limits + # Checkin must be forbidden except if there is no transfer to perform + # + ######################################################################## + + # Case 1: There is a transfer to to do homebranch + Koha::CirculationRules->set_rules( + { + branchcode => undef, + itemtype => undef, + rules => { + returnbranch => 'homebranch', + }, + } + ); + my $issue = AddIssue( $patron, $item->barcode ); + my ( $doreturn, $messages, $iteminfo, $borrowerinfo ) = AddReturn( $item->barcode, $returnbranch->{branchcode} ); + is( $doreturn, 0, "Item cannot be returned if there is a transfer to do" ); + + # Case 2: There is a transfer to do to holdingbranch + Koha::CirculationRules->set_rules( + { + branchcode => undef, + itemtype => undef, + rules => { + returnbranch => 'holdingbranch', + }, + } + ); + $issue->delete; + $item->holdingbranch( $holdingbranch->{branchcode} )->store(); + $issue = AddIssue( $patron, $item->barcode ); + ( $doreturn, $messages, $iteminfo, $borrowerinfo ) = AddReturn( $item->barcode, $returnbranch->{branchcode} ); + is( $doreturn, 0, "Item cannot be returned if there is a transfer to do (item is floating)" ); + + # Case 3: There is no transfer to do + Koha::CirculationRules->set_rules( + { + branchcode => undef, + itemtype => undef, + rules => { + returnbranch => 'noreturn', + }, + } + ); + $issue->delete; + $issue = AddIssue( $patron, $item->barcode ); + ( $doreturn, $messages, $iteminfo, $borrowerinfo ) = AddReturn( $item->barcode, $returnbranch->{branchcode} ); + is( $doreturn, 1, "Item cannot be returned if there is a transfer to do" ); +}; + subtest 'Statistic patrons "X"' => sub { plan tests => 15; @@ -3048,8 +3161,6 @@ subtest 'Statistic patrons "X"' => sub { Koha::Statistics->search( { itemnumber => $item_4->itemnumber } )->count, 3, 'Issue, return, and localuse should be recorded in statistics table for item 4.' ); - - # TODO There are other tests to provide here }; subtest "Bug 27753 - Add AutoClaimReturnStatusOnCheckin" => sub { @@ -3996,7 +4107,7 @@ subtest 'AddReturn | is_overdue' => sub { is( $line->status, 'RETURNED', "Overdue fine is fixed" ); $line = $lines->next; is( $line->amount + 0, -2, "Original payment amount remains as 2" ); - is( $line->amountoutstanding + 0, 0, "Original payment remains applied" ); + is( $line->amountoutstanding + 0, 0, "Original payment remains applied" ); $line = $lines->next; is( $line->amount + 0, -1, "Refund amount correctly set to 1" ); is( $line->amountoutstanding + 0, -1, "Refund amount outstanding unspent" ); @@ -4950,7 +5061,7 @@ subtest '_FixOverduesOnReturn' => sub { my $credit = $offset->credit; is( ref $credit, "Koha::Account::Line", "Found matching credit for fine forgiveness" ); is( $credit->amount + 0, -99, "Credit amount is set correctly" ); - is( $credit->amountoutstanding + 0, 0, "Credit amountoutstanding is correctly set to 0" ); + is( $credit->amountoutstanding + 0, 0, "Credit amountoutstanding is correctly set to 0" ); # Bug 25417 - Only forgive fines where there is an amount outstanding to forgive $accountline->set( @@ -7701,7 +7812,7 @@ subtest 'NoRefundOnLostFinesPaidAge' => sub { { borrowernumber => $patron->id, date => '1970-01-01 14:00:01', - amountoutstanding => 0, + amountoutstanding => 0, amount => -5, interface => 'commandline', credit_type_code => 'PAYMENT' @@ -7757,7 +7868,7 @@ subtest 'NoRefundOnLostFinesPaidAge' => sub { borrowernumber => $patron2->id, date => '1970-01-01 14:00:01', amount => -5, - amountoutstanding => 0, + amountoutstanding => 0, interface => 'commandline', credit_type_code => 'PAYMENT' } -- 2.52.0