From 9c02c2be9420992d15dd734744a18e194d39951a 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 --- C4/Circulation.pm | 7 +- t/db_dependent/Circulation.t | 207 +++++++++++++++++++++++++++-------- 2 files changed, 166 insertions(+), 48 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index ed4a9835..9fd6e004 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1347,6 +1347,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: @@ -1365,11 +1367,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; @@ -1381,7 +1382,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 cce67e4f..946a8831 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -18,7 +18,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 76; +use Test::More tests => 77; use Test::Exception; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -2600,7 +2600,6 @@ subtest 'CanBookBeIssued + Koha::Patron->is_debarred|has_overdues' => sub { is( $error->{USERBLOCKEDNOENDDATE}, '9999-12-31', 'USERBLOCKEDNOENDDATE should be 9999-12-31 for unlimited debarments' ); }; - subtest 'CanBookBeReturned + UseBranchTransfertLimits + CirculationRules' => sub { plan tests => 22; t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '1' ); @@ -2608,7 +2607,8 @@ subtest 'CanBookBeReturned + UseBranchTransfertLimits + CirculationRules' => sub my $homebranch = $builder->build( { source => 'Branch' } ); my $holdingbranch = $builder->build( { source => 'Branch' } ); my $returnbranch = $builder->build( { source => 'Branch' } ); - my $patron = $builder->build_object( { class => 'Koha::Patrons', value => { categorycode => $patron_category->{categorycode} } } ); + my $patron = $builder->build_object( + { class => 'Koha::Patrons', value => { categorycode => $patron_category->{categorycode} } } ); my $item = $builder->build_sample_item( { @@ -2623,48 +2623,52 @@ subtest 'CanBookBeReturned + UseBranchTransfertLimits + CirculationRules' => sub # Attempt returns at returnbranch t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'homebranch' ); - my ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}); - is ( 0 , $allowed , 'Prevent return where returnbranch != homebranch'); - is ( $homebranch->{branchcode} , $message , 'Redirect to homebranch'); + my ( $allowed, $message ) = C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode} ); + is( 0, $allowed, 'Prevent return where returnbranch != homebranch' ); + is( $homebranch->{branchcode}, $message, 'Redirect to homebranch' ); t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'holdingbranch' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}); - is ( 0 , $allowed , 'Prevent return where returnbranch != holdingbranch'); - is ( $holdingbranch->{branchcode} , $message , 'Redirect to holdingbranch'); + ( $allowed, $message ) = C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode} ); + is( 0, $allowed, 'Prevent return where returnbranch != holdingbranch' ); + is( $holdingbranch->{branchcode}, $message, 'Redirect to holdingbranch' ); t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'homeorholdingbranch' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}); - is ( 0 , $allowed , 'Prevent return where returnbranch != homebranch or holdingbranch'); - is ( $homebranch->{branchcode} , $message , 'Redirect to homebranch'); + ( $allowed, $message ) = C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode} ); + is( 0, $allowed, 'Prevent return where returnbranch != homebranch or holdingbranch' ); + is( $homebranch->{branchcode}, $message, 'Redirect to homebranch' ); t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}); - is ( 1 , $allowed , 'with AllowReturnToBranch = anywhere and no limits return to returnbranch is allowed'); - is ( undef , $message , 'without limits provides no message'); + ( $allowed, $message ) = C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode} ); + is( 1, $allowed, 'with AllowReturnToBranch = anywhere and no limits return to returnbranch is allowed' ); + is( undef, $message, 'without limits provides no message' ); # Set limit (Holding -> Return denied) diag("Transfer limit: Holding -> Return"); t::lib::Mocks::mock_preference( 'BranchTransferLimitsType', 'itemtype' ); - my $limit = Koha::Item::Transfer::Limit->new({ - toBranch => $returnbranch->{branchcode}, - fromBranch => $holdingbranch->{branchcode}, - itemtype => $item->effective_itemtype, - })->store(); + my $limit = Koha::Item::Transfer::Limit->new( + { + toBranch => $returnbranch->{branchcode}, + fromBranch => $holdingbranch->{branchcode}, + itemtype => $item->effective_itemtype, + } + )->store(); t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}); - is ( 1 , $allowed , 'Allow return where transferbranch is not passed'); - is ( undef , $message , 'without limits provides no message'); + ( $allowed, $message ) = C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode} ); + is( 1, $allowed, 'Allow return where transferbranch is not passed' ); + is( undef, $message, 'without limits provides no message' ); t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}, $homebranch->{branchcode}); - is ( 1 , $allowed , 'Allow return where there is no transfer limit between returnbranch and homebranch'); - is ( undef , $message , 'without limits provides no message'); + ( $allowed, $message ) = + C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode}, $homebranch->{branchcode} ); + is( 1, $allowed, 'Allow return where there is no transfer limit between returnbranch and homebranch' ); + is( undef, $message, 'without limits provides no message' ); t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}, $holdingbranch->{branchcode}); - is ( 1 , $allowed , 'Allow return where there is no transfer limit between returnbranch and holdingbranch'); - is ( undef , $message , 'without limits provides no message'); + ( $allowed, $message ) = + C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode}, $holdingbranch->{branchcode} ); + is( 1, $allowed, 'Allow return where there is no transfer limit between returnbranch and holdingbranch' ); + is( undef, $message, 'without limits provides no message' ); # Set limit ( Return -> Home denied) diag("Transfer limit: Return -> Home"); @@ -2676,15 +2680,17 @@ subtest 'CanBookBeReturned + UseBranchTransfertLimits + CirculationRules' => sub )->store()->discard_changes; t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}, $homebranch->{branchcode}); - is ( 0 , $allowed , 'Prevent return where returnbranch cannot transfer to homebranch'); - is ( $homebranch->{branchcode} , $message , 'Redirect to homebranch'); + ( $allowed, $message ) = + C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode}, $homebranch->{branchcode} ); + is( 0, $allowed, 'Prevent return where returnbranch cannot transfer to homebranch' ); + is( $homebranch->{branchcode}, $message, 'Redirect to homebranch' ); t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}, $holdingbranch->{branchcode}); - is ( 1 , $allowed , 'Allow return where there is no transfer limit between returnbranch and holdingbranch'); - is ( undef , $message , 'without limits provides no message'); - + ( $allowed, $message ) = + C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode}, $holdingbranch->{branchcode} ); + is( 1, $allowed, 'Allow return where there is no transfer limit between returnbranch and holdingbranch' ); + is( undef, $message, 'without limits provides no message' ); + # Set limit ( Return -> Holding denied) diag("Transfer limit: Return -> Holding"); @@ -2697,16 +2703,130 @@ subtest 'CanBookBeReturned + UseBranchTransfertLimits + CirculationRules' => sub )->store()->discard_changes; t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}, $homebranch->{branchcode}); - is ( 1 , $allowed , 'Allow return where there is no transfer limit between returnbranch and homebranch'); - is ( undef , $message , 'without limits provides no message'); + ( $allowed, $message ) = + C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode}, $homebranch->{branchcode} ); + is( 1, $allowed, 'Allow return where there is no transfer limit between returnbranch and homebranch' ); + is( undef, $message, 'without limits provides no message' ); t::lib::Mocks::mock_preference( 'AllowReturnToBranch', 'anywhere' ); - ($allowed, $message) = C4::Circulation::CanBookBeReturned($item, $returnbranch->{branchcode}, $holdingbranch->{branchcode}); - is ( 0 , $allowed , 'Prevent return where returnbranch cannot transfer to holdingbranch'); - is ( $holdingbranch->{branchcode} , $message , 'Redirect to holdingbranch'); + ( $allowed, $message ) = + C4::Circulation::CanBookBeReturned( $item, $returnbranch->{branchcode}, $holdingbranch->{branchcode} ); + is( 0, $allowed, 'Prevent return where returnbranch cannot transfer to holdingbranch' ); + 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; @@ -2795,11 +2915,8 @@ 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 { plan tests => 8; -- 2.30.2