Bugzilla – Attachment 114439 Details for
Bug 26634
Hold rules applied incorrectly when All Libraries rules are more specific than branch rules
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 26634: Remove GetHoldRule subroutine in C4::Reserves
Bug-26634-Remove-GetHoldRule-subroutine-in-C4Reser.patch (text/plain), 14.21 KB, created by
Josef Moravec
on 2020-12-16 14:42:32 UTC
(
hide
)
Description:
Bug 26634: Remove GetHoldRule subroutine in C4::Reserves
Filename:
MIME Type:
Creator:
Josef Moravec
Created:
2020-12-16 14:42:32 UTC
Size:
14.21 KB
patch
obsolete
>From bc1b02d691ca354c71a65f715533bc7b530a258f Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >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 <martin.renvoize@ptfs-europe.com> >Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> > >Signed-off-by: Josef Moravec <josef.moravec@gmail.com> >--- > 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.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 26634
:
111379
|
111383
|
111398
|
113966
|
114017
|
114260
| 114439 |
116519