From 1f858d09b14a45efe687f14aad3a543a99847e7e Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 24 Nov 2020 20:21:07 +0000 Subject: [PATCH] Bug 26634: Remove GetHoldRule subroutine in C4::Reserves This routine is only used internally and incorrectly overrides the precedence of holds rules - it should be removed This patch removes the routine, adjusts tests, and adds test to confirm correct precedence is followed To test: 1 - At the All Libraries level, create a circ rule for a specific patron category and a specific item type that only allows 1 hold 2 - At the branch-specific level for Branch A, create an All/All rule that allows 2 holds 3 - confirm ReservesControll is set to patron's library 4 - find a patron from Branch A of the category for which you made your rule 5 - find two bibs with items of the itype got which you made your rule 6 - place a hold on one bib. success! 7 - try to place a hold on the second bib. you're told you cannot because the patron is only allowed 1 hold 8 - apply patch, restart services 9 - try to place your second hold again, success! Signed-off-by: Martin Renvoize Signed-off-by: Victor Grousset/tuxayo --- C4/Reserves.pm | 86 +++++++-------------- t/db_dependent/Holds.t | 71 ++++++++++++++++- t/db_dependent/Reserves/MultiplePerRecord.t | 69 ++++------------- 3 files changed, 114 insertions(+), 112 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 2994ff608b..a658646407 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -370,9 +370,7 @@ sub CanItemBeReserved { my $dbh = C4::Context->dbh; my $ruleitemtype; # itemtype of the matching issuing rule - my $allowedreserves = 0; # Total number of holds allowed across all records - my $holds_per_record = 1; # Total number of holds allowed for this one given record - my $holds_per_day; # Default to unlimited + my $allowedreserves = 0; # Total number of holds allowed across all records, default to none # we retrieve borrowers and items informations # # item->{itype} will come for biblioitems if necessery @@ -425,16 +423,30 @@ sub CanItemBeReserved { } # we retrieve rights - if ( my $rights = GetHoldRule( $borrower->{'categorycode'}, $item->effective_itemtype, $branchcode ) ) { - $ruleitemtype = $rights->{itemtype}; - $allowedreserves = $rights->{reservesallowed} // $allowedreserves; - $holds_per_record = $rights->{holds_per_record} // $holds_per_record; - $holds_per_day = $rights->{holds_per_day}; + if ( + my $reservesallowed = Koha::CirculationRules->get_effective_rule({ + itemtype => $item->effective_itemtype, + categorycode => $borrower->{categorycode}, + branchcode => $branchcode, + rule_name => 'reservesallowed', + }) + ) { + $ruleitemtype = $reservesallowed->itemtype; + $allowedreserves = $reservesallowed->rule_value // 0; #undefined is 0, blank is unlimited } else { $ruleitemtype = undef; } + my $rights = Koha::CirculationRules->get_effective_rules({ + categorycode => $borrower->{'categorycode'}, + itemtype => $item->effective_itemtype, + branchcode => $branchcode, + rules => ['holds_per_record','holds_per_day'] + }); + my $holds_per_record = $rights->{holds_per_record} // 1; + my $holds_per_day = $rights->{holds_per_day}; + my $search_params = { borrowernumber => $borrowernumber, biblionumber => $item->biblionumber, @@ -2243,61 +2255,17 @@ sub GetMaxPatronHoldsForRecord { $branchcode = $item->homebranch if ( $controlbranch eq "ItemHomeLibrary" ); - my $rule = GetHoldRule( $categorycode, $itemtype, $branchcode ); - my $holds_per_record = $rule ? $rule->{holds_per_record} : 0; - $max = $holds_per_record if $holds_per_record > $max; - } - - return $max; -} - -=head2 GetHoldRule - -my $rule = GetHoldRule( $categorycode, $itemtype, $branchcode ); - -Returns the matching hold related issuingrule fields for a given -patron category, itemtype, and library. - -=cut - -sub GetHoldRule { - my ( $categorycode, $itemtype, $branchcode ) = @_; - - my $reservesallowed = Koha::CirculationRules->get_effective_rule( - { - itemtype => $itemtype, + my $rule = Koha::CirculationRules->get_effective_rule({ categorycode => $categorycode, - branchcode => $branchcode, - rule_name => 'reservesallowed', - order_by => { - -desc => [ 'categorycode', 'itemtype', 'branchcode' ] - } - } - ); - - my $rules; - if ( $reservesallowed ) { - $rules->{reservesallowed} = $reservesallowed->rule_value; - $rules->{itemtype} = $reservesallowed->itemtype; - $rules->{categorycode} = $reservesallowed->categorycode; - $rules->{branchcode} = $reservesallowed->branchcode; - } - - my $holds_per_x_rules = Koha::CirculationRules->get_effective_rules( - { itemtype => $itemtype, - categorycode => $categorycode, branchcode => $branchcode, - rules => ['holds_per_record', 'holds_per_day'], - order_by => { - -desc => [ 'categorycode', 'itemtype', 'branchcode' ] - } - } - ); - $rules->{holds_per_record} = $holds_per_x_rules->{holds_per_record}; - $rules->{holds_per_day} = $holds_per_x_rules->{holds_per_day}; + rule_name => 'holds_per_record' + }); + my $holds_per_record = $rule ? $rule->rule_value : 0; + $max = $holds_per_record if $holds_per_record > $max; + } - return $rules; + return $max; } =head1 AUTHOR diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 7ad86b9783..3e736f6774 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -7,7 +7,7 @@ use t::lib::TestBuilder; use C4::Context; -use Test::More tests => 67; +use Test::More tests => 68; use MARC::Record; use C4::Biblio; @@ -1328,3 +1328,72 @@ subtest 'non priority holds' => sub { $schema->storage->txn_rollback; }; + +subtest 'CanItemBeReserved rule precedence tests' => sub { + + plan tests => 3; + + t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); + $schema->storage->txn_begin; + my $library = $builder->build_object( { class => 'Koha::Libraries', value => { + pickup_location => 1, + }}); + my $item = $builder->build_sample_item({ + homebranch => $library->branchcode, + holdingbranch => $library->branchcode + }); + my $item2 = $builder->build_sample_item({ + homebranch => $library->branchcode, + holdingbranch => $library->branchcode, + itype => $item->itype + }); + my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { + branchcode => $library->branchcode + }}); + Koha::CirculationRules->set_rules( + { + branchcode => undef, + categorycode => $patron->categorycode, + itemtype => $item->itype, + rules => { + reservesallowed => 1, + } + } + ); + is_deeply( + CanItemBeReserved( $patron->borrowernumber, $item->itemnumber, $library->branchcode ), + { status => 'OK' }, + 'Patron of specified category can place 1 hold on specified itemtype' + ); + my $hold = $builder->build_object({ class => 'Koha::Holds', value => { + biblionumber => $item2->biblionumber, + itemnumber => $item2->itemnumber, + found => undef, + priority => 1, + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + }}); + is_deeply( + CanItemBeReserved( $patron->borrowernumber, $item->itemnumber, $library->branchcode ), + { status => 'tooManyReserves', limit => 1 }, + 'Patron of specified category can place 1 hold on specified itemtype, cannot place a second' + ); + Koha::CirculationRules->set_rules( + { + branchcode => $library->branchcode, + categorycode => undef, + itemtype => undef, + rules => { + reservesallowed => 2, + } + } + ); + is_deeply( + CanItemBeReserved( $patron->borrowernumber, $item->itemnumber, $library->branchcode ), + { status => 'OK' }, + 'Patron of specified category can place 1 hold on specified itemtype if library rule for all types and categories set to 2' + ); + + $schema->storage->txn_rollback; + +}; diff --git a/t/db_dependent/Reserves/MultiplePerRecord.t b/t/db_dependent/Reserves/MultiplePerRecord.t index 8d5cd68396..6e88740702 100755 --- a/t/db_dependent/Reserves/MultiplePerRecord.t +++ b/t/db_dependent/Reserves/MultiplePerRecord.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 40; +use Test::More tests => 16; use t::lib::TestBuilder; use t::lib::Mocks; @@ -89,7 +89,7 @@ my $item3 = $builder->build_sample_item( Koha::CirculationRules->delete(); -# Test GetMaxPatronHoldsForRecord and GetHoldRule +# Test GetMaxPatronHoldsForRecord Koha::CirculationRules->set_rules( { categorycode => undef, @@ -106,16 +106,6 @@ t::lib::Mocks::mock_preference('item-level_itypes', 1); # Assuming the item type my $max = GetMaxPatronHoldsForRecord( $patron->{borrowernumber}, $biblio->biblionumber ); is( $max, 1, 'GetMaxPatronHoldsForRecord returns max of 1' ); -my $rule = C4::Reserves::GetHoldRule( - $category->{categorycode}, - $itemtype1->{itemtype}, - $library->{branchcode} -); -is( $rule->{categorycode}, undef, 'Got rule with universal categorycode' ); -is( $rule->{itemtype}, undef, 'Got rule with universal itemtype' ); -is( $rule->{branchcode}, undef, 'Got rule with universal branchcode' ); -is( $rule->{reservesallowed}, 1, 'Got reservesallowed of 1' ); -is( $rule->{holds_per_record}, 1, 'Got holds_per_record of 1' ); Koha::CirculationRules->set_rules( { @@ -131,16 +121,6 @@ Koha::CirculationRules->set_rules( $max = GetMaxPatronHoldsForRecord( $patron->{borrowernumber}, $biblio->biblionumber ); is( $max, 2, 'GetMaxPatronHoldsForRecord returns max of 2' ); -$rule = C4::Reserves::GetHoldRule( - $category->{categorycode}, - $itemtype1->{itemtype}, - $library->{branchcode} -); -is( $rule->{categorycode}, $category->{categorycode}, 'Got rule with specific categorycode' ); -is( $rule->{itemtype}, undef, 'Got rule with universal itemtype' ); -is( $rule->{branchcode}, undef, 'Got rule with universal branchcode' ); -is( $rule->{reservesallowed}, 2, 'Got reservesallowed of 2' ); -is( $rule->{holds_per_record}, 2, 'Got holds_per_record of 2' ); Koha::CirculationRules->set_rules( { @@ -156,16 +136,6 @@ Koha::CirculationRules->set_rules( $max = GetMaxPatronHoldsForRecord( $patron->{borrowernumber}, $biblio->biblionumber ); is( $max, 3, 'GetMaxPatronHoldsForRecord returns max of 3' ); -$rule = C4::Reserves::GetHoldRule( - $category->{categorycode}, - $itemtype1->{itemtype}, - $library->{branchcode} -); -is( $rule->{categorycode}, $category->{categorycode}, 'Got rule with specific categorycode' ); -is( $rule->{itemtype}, $itemtype1->{itemtype}, 'Got rule with universal itemtype' ); -is( $rule->{branchcode}, undef, 'Got rule with universal branchcode' ); -is( $rule->{reservesallowed}, 3, 'Got reservesallowed of 3' ); -is( $rule->{holds_per_record}, 3, 'Got holds_per_record of 3' ); Koha::CirculationRules->set_rules( { @@ -181,16 +151,6 @@ Koha::CirculationRules->set_rules( $max = GetMaxPatronHoldsForRecord( $patron->{borrowernumber}, $biblio->biblionumber ); is( $max, 4, 'GetMaxPatronHoldsForRecord returns max of 4' ); -$rule = C4::Reserves::GetHoldRule( - $category->{categorycode}, - $itemtype2->{itemtype}, - $library->{branchcode} -); -is( $rule->{categorycode}, $category->{categorycode}, 'Got rule with specific categorycode' ); -is( $rule->{itemtype}, $itemtype2->{itemtype}, 'Got rule with universal itemtype' ); -is( $rule->{branchcode}, undef, 'Got rule with universal branchcode' ); -is( $rule->{reservesallowed}, 4, 'Got reservesallowed of 4' ); -is( $rule->{holds_per_record}, 4, 'Got holds_per_record of 4' ); Koha::CirculationRules->set_rules( { @@ -205,17 +165,22 @@ Koha::CirculationRules->set_rules( ); $max = GetMaxPatronHoldsForRecord( $patron->{borrowernumber}, $biblio->biblionumber ); -is( $max, 5, 'GetMaxPatronHoldsForRecord returns max of 1' ); -$rule = C4::Reserves::GetHoldRule( - $category->{categorycode}, - $itemtype2->{itemtype}, - $library->{branchcode} +is( $max, 5, 'GetMaxPatronHoldsForRecord returns max of 5' ); + +Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => $library->{branchcode}, + rules => { + reservesallowed => 9, + holds_per_record => 9, + } + } ); -is( $rule->{categorycode}, $category->{categorycode}, 'Got rule with specific categorycode' ); -is( $rule->{itemtype}, $itemtype2->{itemtype}, 'Got rule with universal itemtype' ); -is( $rule->{branchcode}, $library->{branchcode}, 'Got rule with specific branchcode' ); -is( $rule->{reservesallowed}, 5, 'Got reservesallowed of 5' ); -is( $rule->{holds_per_record}, 5, 'Got holds_per_record of 5' ); + +$max = GetMaxPatronHoldsForRecord( $patron->{borrowernumber}, $biblio->biblionumber ); +is( $max, 9, 'GetMaxPatronHoldsForRecord returns max of 9 because Library specific all itemtypes all categories rule comes before All libraries specific type and specific category' ); Koha::CirculationRules->delete(); -- 2.29.2