From 832b3af36b8086db5a4b3b1c64a5e75487bb7c6e Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 2 Jun 2021 15:20:08 +0000 Subject: [PATCH] Bug 28503: Compare item homebranch to patron branch when hold policy set to 'from_home_library' This fixes an issue in the way we calculate the check for hold policy 'from_home_library' Currently we change the comparison based on ReservesControlBranch, however, that should only control the rule we fetch, not how we compare When ReservesControlBranch is set to "patron's home library" we compare the patron's branch to the patron's branch, this is useless and means we pass the check for all branches all of the time We should instead compare the patron's branch to the item's branch, and only fetch the rule using ReservesControlBranch To test: 1 - Have a record with an item from library A and library B 2 - Set the 'Default checkout, hold and return policy'->Hold policy->From home library for all libraries and ensure you have no branch specific/itemtype specific rules set 3 - Attempt to place a hold on the record for a patron from library B 4 - Note that only the library B item is holdable - place a title level hold (do not choose an item) 5 - Check in the item from library A 6 - It fills the hold - This is incorrect - ignore the hold 7 - Apply patch 8 - Restart all the things 9 - Check in the item from library A 10 - No hold found 11 - Check in the item from library B 12 - Hold found, correctly Signed-off-by: David Nind Signed-off-by: Tomas Cohen Arazi Bug 28503: Clarify what ReservesControlBranch controls Signed-off-by: David Nind Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall (cherry picked from commit 19660a25fa9421373a41fb6aba71215d71c541be) Signed-off-by: Fridolin Somers Bug 28503: Unit tests Signed-off-by: David Nind Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall (cherry picked from commit a4cdeaae3f82ea47fe3fba5e79f419e6911fb524) Signed-off-by: Fridolin Somers Bug 28503: (follow-up) Get rid of tests warnings Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall (cherry picked from commit 1e677a8755dfa4b5df3ff8df8f2644aedf388eb3) Signed-off-by: Fridolin Somers Bug 28503: [20.11.x] fix unit tests Impact of Bug 27069 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27069 Bug 28503: [20.11.x] Adjust bad test assumption The tests set holdpolicy to 'from home library', this is the case we were fixing Adjusted test to set policy to 'any library' and tests pass --- C4/Reserves.pm | 2 +- .../admin/preferences/circulation.pref | 2 +- misc/cronjobs/update_patrons_category.pl | 1 + t/db_dependent/Circulation.t | 2 +- t/db_dependent/Reserves.t | 74 ++++++++++++++++++- 5 files changed, 74 insertions(+), 7 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 0e20c77ae5..0d8f4aac7e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -894,7 +894,7 @@ sub CheckReserves { my $branch = GetReservesControlBranch( $item->unblessed, $patron->unblessed ); my $branchitemrule = C4::Circulation::GetBranchItemRule($branch,$item->effective_itemtype); next if ($branchitemrule->{'holdallowed'} == 0); - next if (($branchitemrule->{'holdallowed'} == 1) && ($branch ne $patron->branchcode)); + next if (($branchitemrule->{'holdallowed'} == 1) && ($item->homebranch ne $patron->branchcode)); my $library = Koha::Libraries->find({branchcode=>$item->homebranch}); next if (($branchitemrule->{'holdallowed'} == 3) && (!$library->validate_hold_sibling({branchcode => $patron->branchcode}) )); my $hold_fulfillment_policy = $branchitemrule->{hold_fulfillment_policy}; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index d290cabfa4..9742cd468c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -716,7 +716,7 @@ Circulation: class: integer - days from now) at checkin time. Note that this number of days will be used too in calculating the default end date for the Holds to pull-report. But it does not interfere with issuing, renewing or transferring items. - - - Check the + - Check the rule from the - pref: ReservesControlBranch choices: ItemHomeLibrary: "item's home library" diff --git a/misc/cronjobs/update_patrons_category.pl b/misc/cronjobs/update_patrons_category.pl index 86541a818d..fd4bc4a7ea 100755 --- a/misc/cronjobs/update_patrons_category.pl +++ b/misc/cronjobs/update_patrons_category.pl @@ -47,6 +47,7 @@ update_patrons_category.pl -f=categorycode -t=categorycode update_patrons_category.pl --help | --man Options: + --help brief help message --man full documentation -too_old update if over maximum age for current category diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 46d629d65f..23c034bd44 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -291,7 +291,7 @@ subtest "CanBookBeRenewed AllowRenewalIfOtherItemsAvailable multiple borrowers a branchcode => undef, itemtype => undef, rule_name => 'holdallowed', - rule_value => 1 + rule_value => 2 } ); Koha::CirculationRules->set_rule( diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 5ff4e2aacd..a0c502a574 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -1076,9 +1076,9 @@ subtest 'RevertWaitingStatus' => sub { ); }; -subtest 'CheckReserves additional test' => sub { +subtest 'CheckReserves additional tests' => sub { - plan tests => 3; + plan tests => 8; my $item = $builder->build_sample_item; my $reserve1 = $builder->build_object( @@ -1151,6 +1151,68 @@ subtest 'CheckReserves additional test' => sub { is( $matched_reserve->{reserve_id}, $reserve1->reserve_id, "We got the Transit reserve" ); is( scalar @$possible_reserves, 2, 'We do get both reserves' ); + + my $patron_B = $builder->build_object({ class => "Koha::Patrons" }); + my $item_A = $builder->build_sample_item; + my $item_B = $builder->build_sample_item({ + homebranch => $patron_B->branchcode, + biblionumber => $item_A->biblionumber, + itype => $item_A->itype + }); + Koha::CirculationRules->set_rules( + { + branchcode => undef, + categorycode => undef, + itemtype => $item_A->itype, + rules => { + reservesallowed => 25, + holds_per_record => 1, + } + } + ); + Koha::CirculationRules->set_rule({ + branchcode => undef, + itemtype => $item_A->itype, + rule_name => 'holdallowed', + rule_value => 1 + }); + my $reserve_id = AddReserve( + { + branchcode => $patron_B->branchcode, + borrowernumber => $patron_B->borrowernumber, + biblionumber => $item_A->biblionumber, + priority => 1, + itemnumber => undef, + } + ); + + ok( $reserve_id, "We can place a record level hold because one item is owned by patron's home library"); + t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); + ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A->itemnumber ); + is( $status, "", "We do not fill the hold with item A because it is not from the patron's homebranch"); + Koha::CirculationRules->set_rule({ + branchcode => $item_A->homebranch, + itemtype => $item_A->itype, + rule_name => 'holdallowed', + rule_value => 2 + }); + ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A->itemnumber ); + is( $status, "Reserved", "We fill the hold with item A because item's branch rule says allow any"); + + + # Changing the control branch should change only the rule we get + t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); + ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A->itemnumber ); + is( $status, "", "We do not fill the hold with item A because it is not from the patron's homebranch"); + Koha::CirculationRules->set_rule({ + branchcode => $patron_B->branchcode, + itemtype => $item_A->itype, + rule_name => 'holdallowed', + rule_value => 2 + }); + ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A->itemnumber ); + is( $status, "Reserved", "We fill the hold with item A because patron's branch rule says allow any"); + }; subtest 'AllowHoldOnPatronPossession test' => sub { @@ -1250,11 +1312,15 @@ subtest 'ModReserveAffect logging' => sub { $hold = Koha::Holds->find($reserve_id); is( $hold->timestamp, $previous_timestamp, 'Make sure the previous timestamp has been used' ); + # Avoid warnings + my $reserve_mock = Test::MockModule->new('C4::Reserves'); + $reserve_mock->mock( '_koha_notify_reserve', undef ); + # Mark it waiting ModReserveAffect( $item->itemnumber, $patron->borrowernumber ); - $hold = Koha::Holds->find($reserve_id); - is( $hold->found, 'W', 'Hold has been set waiting' ); + $hold->discard_changes; + ok( $hold->is_waiting, 'Hold has been set waiting' ); isnt( $hold->timestamp, $previous_timestamp, 'The timestamp has been modified' ); my $log = Koha::ActionLogs->search({ module => 'HOLDS', action => 'MODIFY', object => $hold->reserve_id })->next; -- 2.20.1