From 667cbde0317545e1c92c95779fa013b3a3513c2c Mon Sep 17 00:00:00 2001
From: Aleisha Amohia <aleishaamohia@hotmail.com>
Date: Wed, 25 Mar 2020 23:17:42 +0000
Subject: [PATCH] Bug 23172: (follow-up) Fixing test

Ensure that the following tests pass:
t/db_dependent/Hold.t
t/db_dependent/Reserves.t

And that the original test plan works as expected
---
 C4/Reserves.pm               |  11 +-
 Koha/Hold.pm                 |  43 +++++---
 t/db_dependent/Circulation.t |   3 -
 t/db_dependent/Hold.t        | 235 +++++++++++++++++++++++++++----------------
 t/db_dependent/Reserves.t    | 186 +++++++++++++++++++++++-----------
 5 files changed, 309 insertions(+), 169 deletions(-)

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 5af75d967c0..b73e84b3aa0 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -595,6 +595,10 @@ sub CanReserveBeCanceledFromOpac {
 
     return 0 unless $reserve->borrowernumber == $borrowernumber;
     return $reserve->is_cancelable_from_opac;
+    return 0 if ( $reserve->found && $reserve->found eq 'W' ) or ( $reserve->found && $reserve->found eq 'T' );
+
+    return 1;
+
 }
 
 =head2 GetOtherReserves
@@ -854,7 +858,12 @@ sub CheckReserves {
         my $priority = 10000000;
         foreach my $res (@reserves) {
             #Before allocating to bib-level hold check if reserve respects hold rule, i.e. can patron category/item type combination allow reserves
-            my $checkreserve = Koha::Hold->check_if_existing_hold_matches_issuingrules( $res->{'borrowernumber'}, $itemnumber );
+            my $checkreserve;
+            if ( $item ){
+                $checkreserve = Koha::Holds->find( $res->{reserve_id} )->matches_circulation_rules({ itemnumber => $item });
+            } else {
+                $checkreserve = Koha::Holds->find( $res->{reserve_id} )->matches_circulation_rules({ barcode => $barcode });
+            }
             next unless ($checkreserve eq 'OK');
             if ($res->{'found'} && $res->{'found'} eq 'W') {
                 return ( "Waiting", $res, \@reserves ); # Found it, it is waiting
diff --git a/Koha/Hold.pm b/Koha/Hold.pm
index b23b0445333..37ccd9227cb 100644
--- a/Koha/Hold.pm
+++ b/Koha/Hold.pm
@@ -36,7 +36,6 @@ use Koha::Items;
 use Koha::Libraries;
 use Koha::Old::Holds;
 use Koha::Calendar;
-
 use Koha::Exceptions::Hold;
 
 use base qw(Koha::Object);
@@ -476,41 +475,53 @@ sub is_suspended {
     return $self->suspend();
 }
 
-=head3 check_if_existing_hold_matches_issuingrules
+=head3 matches_circulation_rules
 
-my $checkreserve = Koha::Hold->check_if_existing_hold_matches_issuingrules($res->{'borrowernumber'}, $itemnumber );
+    my $checkreserve = Koha::Hold->matches_circulation_rules({ itemnumber => $itemnumber });
+    my $checkreserve = Koha::Hold->matches_circulation_rules({ barcode => $barcode });
 
 Upon checking in an item of a bibliographic record with a biblio-level hold on it, check if a bib-level hold can be allocated to a patron based on the current values in the issuingrules table
 
+Parameter is an attribute of the item that is being checked in
+
 =cut
 
-sub check_if_existing_hold_matches_issuingrules {
-    my ( $self, $borrowernumber, $itemnumber ) = @_;
+sub matches_circulation_rules {
+    my ( $self, $params ) = @_;
 
     #Get patron and item objects
-    my $patron = Koha::Patrons->find( $borrowernumber );
-    my $item = Koha::Items->find( $itemnumber );
-    $item = $item->unblessed();
+    my $patron = $self->borrower;
+    my $item = Koha::Items->find( $params->{itemnumber} );
+    if ( !defined $item ){
+        $item = Koha::Items->find({ barcode => $params->{barcode} });
+    }
 
     #Get branchcode
     my $branchcode;
     my $controlbranch = C4::Context->preference('ReservesControlBranch');
-
-    if ( $controlbranch eq "ItemHomeLibrary" ) {
-         $branchcode  = $item->{homebranch};
-    }
-    elsif ( $controlbranch eq "PatronLibrary" ) {
+    if ( $controlbranch eq "ItemHomeLibrary" && defined $item ) {
+         $branchcode  = $item->homebranch;
+    } else {
          $branchcode  = $patron->branchcode;
     }
 
-    my $issuingrule = Koha::IssuingRules->get_effective_issuing_rule({
+    my $issuingrule = Koha::CirculationRules->get_effective_rules({
         branchcode => $branchcode,
         categorycode => $patron->categorycode,
-        itemtype => $item->{itype},
+        itemtype => $item->effective_itemtype,
+        rules => [
+            'reservesallowed',
+            'holds_per_record',
+            'holds_per_day',
+        ],
     });
 
     #Check if the patron category/item type combination is valid
-    if ( ($issuingrule->reservesallowed == 0 || $issuingrule->holds_per_record == 0 || $issuingrule->holds_per_day == 0 )) {
+    if (
+        ( defined $issuingrule->{reservesallowed} && $issuingrule->{reservesallowed} == 0 ) ||
+        ( defined $issuingrule->{holds_per_record} && $issuingrule->{holds_per_record} == 0 ) ||
+        ( defined $issuingrule->{holds_per_day} && $issuingrule->{holds_per_day} == 0 )
+    ) {
         return 'InvalidHold';
     } else {
         return 'OK';
diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
index 2ec5b83c3b2..6f2a84fa09a 100755
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@@ -3385,9 +3385,6 @@ subtest 'Set waiting flag' => sub {
     my ( $status ) = CheckReserves($item->itemnumber);
     is( $status, 'Transferred', 'Hold is not waiting yet');
 
-    my ( $status ) = CheckReserves($item->{itemnumber});
-    is( $status, 'Reserved', 'Hold is not waiting yet');
-
     set_userenv( $library_2 );
     $do_transfer = 0;
     AddReturn( $item->barcode, $library_2->{branchcode} );
diff --git a/t/db_dependent/Hold.t b/t/db_dependent/Hold.t
index 74e39c00703..e4e599b06f1 100755
--- a/t/db_dependent/Hold.t
+++ b/t/db_dependent/Hold.t
@@ -28,7 +28,8 @@ use Koha::Holds;
 use Koha::Item;
 use Koha::DateUtils;
 use t::lib::TestBuilder;
-use Koha::IssuingRules;
+use Koha::CirculationRules;
+use C4::Reserves;
 
 use Test::More tests => 33;
 use Test::Exception;
@@ -288,99 +289,155 @@ subtest 'suspend() tests' => sub {
     $schema->storage->txn_rollback;
 };
 
-subtest "check_if_existing_hold_matches_issuingrules tests" => sub {
+subtest "matches_circulation_rules tests" => sub {
     plan tests => 9;
 
     $schema->storage->txn_begin();
 
     #Create test data
-    my $branchcode = $builder->build( { source => 'Branch' } )->{ branchcode };
-    my $categorycode1 = $builder->build({ source => 'Category' })->{categorycode};
-    my $categorycode2 = $builder->build({ source => 'Category' })->{categorycode};
-    my $patron1 = $builder->build({source => 'Borrower', value => { branchcode => $branchcode }});
-
-    my $item1 = $builder->build({ source => 'Item', value => { homebranch => $branchcode }});
-    my $item2 = $builder->build({ source => 'Item', value => { homebranch => $branchcode }});
-
-    my $itemtype1 = $item1->{'itype'};
-    my $itemtype2 = $item2->{'itype'};
-
-    Koha::IssuingRules->delete;
-
-    #Get branchcode
-    my $controlbranch = C4::Context->preference('ReservesControlBranch');
-
-    if ( $controlbranch eq "ItemHomeLibrary" ) {
-       $branchcode  = $item->{homebranch};
-    }
-    elsif ( $controlbranch eq "PatronLibrary" ) {
-       $branchcode  = $patron1->{branchcode};
-    }
-
-    #Test all cases when the check_return
-    #check_if_existing_hold_matches_issuingrules() returns 'InvalidHold' when reservesallowed, holds_per_record and holds_per_day are all 0
-    my $rule1 = Koha::IssuingRule->new({
-        branchcode =>  $branchcode,
-        categorycode => $patron1->{'categorycode'},
-        itemtype => $item1->{'itype'},
-        reservesallowed => 0,
-        holds_per_record => 0,
-        holds_per_day => 0,
-    })->store;
-    my $checkreserve1 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item1->{'itemnumber'} );
-    is( $checkreserve1, 'InvalidHold', 'Allocating item1 to a bib-reserve is not allowed as the issuingrules reservesallowed, holds_per_record and holds_per_day is 0 for this patron category/itemtype combination' );
-
-    #Confirm that when any of the 4 reserves related issuingrules fields (reservesallowed, holds_per_record, holds_per_day) are set to 0 then check_if_existing_hold_matches_issuingrules() returns 'InvalidHold' and so a newly returned item is not allocated to a specific bib-level reserve
-
-    #check_if_existing_hold_matches_issuingrules() returns 'InvalidHold' when reservesallowed is 0 for the patron category/itemtype combination
-    $rule1->set({ reservesallowed => 0, holds_per_record => 1, holds_per_day => 1 })->store;
-    my $checkreserve2 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item1->{'itemnumber'} );
-    is( $checkreserve2, 'InvalidHold', 'Allocating item1 to a bib-reserve is not allowed as the issuingrules reservesallowed is 0 for this patron category/itemtype combination' );
-
-    #Now change to reservesallowed = 1, holds_per_record = 0, holds_per_day = 1
-    $rule1->set({ reservesallowed => 1, holds_per_record => 0, holds_per_day => 1 })->store;
-    my $checkreserve3 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item1->{'itemnumber'} );
-    is( $checkreserve3, 'InvalidHold', 'Allocating item1 to a bib-reserve is not allowed as the issuingrules holds_per_record is set to 0 for this patron category/itemtype combination' );
-
-   #Now change to reservesallowed = 1, holds_per_record = 1, holds_per_day = 0
-   $rule1->set({ reservesallowed => 1, holds_per_record => 1, holds_per_day => 0 })->store;
-   my $checkreserve4 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item1->{'itemnumber'} );
-   is( $checkreserve4, 'InvalidHold', 'Allocating item1 to a bib-reserve is not allowed as the issuingrules holds_per_record is set to0 for this patron category/itemtype combination' );
-
-   #Now change to reservesallowed = 1, holds_per_record = 1, holds_per_day = 1 and confirm 'OK' is returned
-   $rule1->set({ reservesallowed => 1, holds_per_record => 1, holds_per_day => 1 })->store;
-   my $checkreserve5 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item1->{'itemnumber'} );
-   is( $checkreserve5, 'OK', 'Allocating item2 to a bib-reserve is allowed as there is no specific issuingrule for this patron category/itemtype combination and so we default to the all patron category/all item type rule which allows reserves' );
-
-   #Create a default rule (patron categories: ALL, itemtypes: ALL)
-   my $rule3 = Koha::IssuingRule->new({
-       branchcode => $branchcode,
-       categorycode => '*',
-       itemtype => '*',
-       reservesallowed => 1,
-       holds_per_record => 1,
-       holds_per_day => 1,
-    })->store;
-
-    #Check that when no specific patron category/item type issuingrule combo exists we revert to using the default ALL categories/ALL
-    #itemtypes issuingrule. When generic rule is reservesallowed, holds_per_record and holds_per_day = 1 then 'OK' is returned
-    my $checkreserve6 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item2->{'itemnumber'} );
-    is( $checkreserve6, 'OK', 'Allocating item2 to a bib-reserve is allowed as there is no specific issuingrule for this patron category/itemtype combination and so we default to the all patron category/all item type rule which allows reserves' );
-
-    #When default rule is reservesallowed = 0, holds_per_record = 1, holds_per_day = 1 then 'InvalidHold is returned
-    $rule3->set({ reservesallowed => 0, holds_per_record => 1, holds_per_day => 1 })->store;
-    my $checkreserve7 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item2->{'itemnumber'} );
-    is( $checkreserve7, 'InvalidHold', 'Allocating item2 to a bib-reserve is not allowed as there is no specific issuingrule for this patron category/itemtype combination and so we default to the all patron category/all item type rule which doesnt allows reserves as reservesallowed is 0' );
-
-    #When default rule is reservesallowed = 1, holds_per_record = 0, holds_per_day = 1 then 'InvalidHold' is returned
-    $rule3->set({ reservesallowed => 1, holds_per_record => 0, holds_per_day => 1 })->store;
-    my $checkreserve8 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item2->{'itemnumber'} );
-    is( $checkreserve8, 'InvalidHold', 'Allocating item2 to a bib-reserve is not allowed as there is no specific issuingrule for this patron category/itemtype combination and so we default to the all patron category/all item type rule which doesnt allows reserves as holds_per_record is 0' );
-
-    #When default rule is reservesallowed = 1, holds_per_record = 1, holds_per_day = 0 then 'InvalidHold' is returned
-    $rule3->set({ reservesallowed => 1, holds_per_record => 1, holds_per_day => 0 })->store;
-    my $checkreserve9 = Koha::Hold->check_if_existing_hold_matches_issuingrules($patron1->{'borrowernumber'}, $item2->{'itemnumber'} );
-    is( $checkreserve9, 'InvalidHold', 'Allocating item2 to a bib-reserve is not allowed as there is no specific issuingrule for this patron category/itemtype combination and so we default to the all patron category/all item type rule which doesnt allows reserves as holds_per_day is 0' );
+    my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode };
+    my $categorycode = $builder->build({ source => 'Category' })->{ categorycode };
+    my $patron = $builder->build({ source => 'Borrower', value => { categorycode => $categorycode, branchcode => $branchcode }});
+    my $biblionumber = $builder->build({ source => 'Biblio' })->{biblionumber};
+    my $itemtype1 = $builder->build({ source => 'Itemtype' })->{itemtype};
+    my $itemtype2 = $builder->build({ source => 'Itemtype' })->{itemtype};
+    my $item1 = $builder->build({ source => 'Item', value => { homebranch => $branchcode, biblionumber => $biblionumber, itype => $itemtype1 }});
+    my $item2 = $builder->build({ source => 'Item', value => { homebranch => $branchcode, biblionumber => $biblionumber, itype => $itemtype2 }});
+
+    Koha::CirculationRules->set_rules({
+        branchcode => $branchcode,
+        categorycode => $categorycode,
+        itemtype => $itemtype1,
+        rules => {
+            reservesallowed => 0,
+            holds_per_record => 0,
+            holds_per_day => 0,
+        }
+    });
+
+    my $reserveid1 = AddReserve({
+        branchcode => $branchcode,
+        borrowernumber => $patron->{borrowernumber},
+        biblionumber => $biblionumber,
+    });
+    my $reserve1 = Koha::Holds->find($reserveid1);
+
+    my $checkreserve1 = $reserve1->matches_circulation_rules({ itemnumber => $item1->{itemnumber} });
+    is( $checkreserve1, 'InvalidHold', 'Circulation rules disallow item being allocated to the hold' );
+
+    #Confirm that when any of the 3 reserves-related circulaion rules fields (reservesallowed, holds_per_record, holds_per_day) are set to 0 then matches_circulation_rules returns 'InvalidHold' and so a newly returned item is not allocated to a specific bib-level reserve
+
+    Koha::CirculationRules->set_rules({
+        branchcode => $branchcode,
+        categorycode => $categorycode,
+        itemtype => $itemtype1,
+        rules => {
+            reservesallowed => 0,
+            holds_per_record => 1,
+            holds_per_day => 1,
+        }
+    });
+    my $checkreserve2 = $reserve1->matches_circulation_rules({ itemnumber => $item1->{itemnumber} });
+    is( $checkreserve2, 'InvalidHold', 'Reserves allowed is 0' );
+
+    Koha::CirculationRules->set_rules({
+        branchcode => $branchcode,
+        categorycode => $categorycode,
+        itemtype => $itemtype1,
+        rules => {
+            reservesallowed => 1,
+            holds_per_record => 0,
+            holds_per_day => 1,
+        }
+    });
+    my $checkreserve3 = $reserve1->matches_circulation_rules({ itemnumber => $item1->{itemnumber} });
+    is( $checkreserve3, 'InvalidHold', 'Holds per record is 0' );
+
+    Koha::CirculationRules->set_rules({
+        branchcode => $branchcode,
+        categorycode => $categorycode,
+        itemtype => $itemtype1,
+        rules => {
+            reservesallowed => 1,
+            holds_per_record => 1,
+            holds_per_day => 0,
+        }
+    });
+    my $checkreserve4 = $reserve1->matches_circulation_rules({ itemnumber => $item1->{itemnumber} });
+   is( $checkreserve4, 'InvalidHold', 'Holds per day is 0' );
+
+    Koha::CirculationRules->set_rules({
+        branchcode => $branchcode,
+        categorycode => $categorycode,
+        itemtype => $itemtype1,
+        rules => {
+            reservesallowed => 1,
+            holds_per_record => 1,
+            holds_per_day => 1,
+        }
+    });
+    my $checkreserve5 = $reserve1->matches_circulation_rules({ itemnumber => $item1->{itemnumber} });
+   is( $checkreserve5, 'OK', 'Hold is allowed, item allocated to hold' );
+
+    #Check that when no specific patron category/item type circulation rule combo exists we revert to using the default ALL categories/ALL
+    my $reserveid2 = AddReserve({
+        branchcode => $branchcode,
+        borrowernumber => $patron->{borrowernumber},
+        biblionumber => $biblionumber,
+    });
+    my $reserve2 = Koha::Holds->find($reserveid2);
+
+    Koha::CirculationRules->set_rules({
+        branchcode => undef,
+        categorycode => undef,
+        itemtype => undef,
+        rules => {
+            reservesallowed => 0,
+            holds_per_record => 1,
+            holds_per_day => 1,
+        }
+    });
+    my $checkreserve6 = $reserve2->matches_circulation_rules({ itemnumber => $item2->{itemnumber} });
+    is( $checkreserve6, 'InvalidHold', 'Reserves allowed is 0' );
+
+    Koha::CirculationRules->set_rules({
+        branchcode => undef,
+        categorycode => undef,
+        itemtype => undef,
+        rules => {
+            reservesallowed => 1,
+            holds_per_record => 0,
+            holds_per_day => 1,
+        }
+    });
+    my $checkreserve7 = $reserve2->matches_circulation_rules({ itemnumber => $item2->{itemnumber} });
+    is( $checkreserve7, 'InvalidHold', 'Holds per record is 0' );
+
+    Koha::CirculationRules->set_rules({
+        branchcode => undef,
+        categorycode => undef,
+        itemtype => undef,
+        rules => {
+            reservesallowed => 1,
+            holds_per_record => 1,
+            holds_per_day => 0,
+        }
+    });
+    my $checkreserve8 = $reserve2->matches_circulation_rules({ itemnumber => $item2->{itemnumber} });
+    is( $checkreserve8, 'InvalidHold', 'Holds per day is 0' );
+
+    Koha::CirculationRules->set_rules({
+        branchcode => undef,
+        categorycode => undef,
+        itemtype => undef,
+        rules => {
+            reservesallowed => 1,
+            holds_per_record => 1,
+            holds_per_day => 1,
+        }
+    });
+    my $checkreserve9 = $reserve2->matches_circulation_rules({ itemnumber => $item2->{itemnumber} });
+    is( $checkreserve9, 'OK', 'Hold is allowed, item allocated to hold' );
 
     $schema->storage->txn_rollback;
 };
diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t
index a342f59cf42..3ca36fdfab6 100755
--- a/t/db_dependent/Reserves.t
+++ b/t/db_dependent/Reserves.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 66;
+use Test::More tests => 71;
 use Test::MockModule;
 use Test::Warn;
 
@@ -529,13 +529,14 @@ AddReserve(
         priority       => 1,
     }
 );
+
 my (undef, $canres, undef) = CheckReserves($item->itemnumber);
 
 is( CanReserveBeCanceledFromOpac(), undef,
     'CanReserveBeCanceledFromOpac should return undef if called without any parameter'
 );
 is(
-    CanReserveBeCanceledFromOpac( $canres->{resserve_id} ),
+    CanReserveBeCanceledFromOpac( $canres->{reserve_id} ),
     undef,
     'CanReserveBeCanceledFromOpac should return undef if called without the reserve_id'
 );
@@ -724,67 +725,101 @@ MoveReserve( $item->itemnumber, $borrowernumber );
 is( $status, 'Reserved', 'MoveReserve did not fill future hold of 3 days');
 $dbh->do('DELETE FROM reserves', undef, ($bibnum));
 
-
-
-
-
-
-#Test bug 23172. Check when returning an item that any patron category/item type combinations that forbid reservation are skipped over
-#when allocating the item to a bib-level hold
-$dbh->do('DELETE FROM issuingrules');
-
-#Create two new patrons
-my $patronnumcat1 = Koha::Patron->new({branchcode => $branch_1, categorycode => $category_1})->store->borrowernumber;
-my $patronnumcat2 = Koha::Patron->new({branchcode => $branch_1, categorycode => $category_2})->store->borrowernumber;
-
-#Create a new biblio record
-my $bibnum3 = $builder->build_sample_biblio({frameworkcode => $frameworkcode})->biblionumber;
+#Test bug 23172. Check when returning an item that any patron category/item type combinations that forbid reservation are skipped over when allocating the item to a bib-level hold
+$dbh->do('DELETE FROM circulation_rules');
 
 #Create a second item type
 my $itemtype2 = $builder->build({ source => 'Itemtype', value => { notforloan => undef } } )->{itemtype};
 
-# Create an item from new itemtype
-my ( $item_bibnum2, $item_bibitemnum2, $itemnumber2 ) = AddItem(
-    {   homebranch    => $branch_1,
-        holdingbranch => $branch_1,
-        itype         => $itemtype2,
-        barcode       => 'bug23172_CPL'
-    },
-    $bibnum3
-);
-
 #Make a general All patron category/all item type rule
-$dbh->do(
-    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record)
-      VALUES (?, ?, ?, ?, ?, ?)},
-    {},
-    '*', '*', '*', 25, 1, 1
+Koha::CirculationRules->set_rules(
+    {
+        branchcode   => undef,
+        categorycode => undef,
+        itemtype     => undef,
+        rules        => {
+            reservesallowed => 25,
+            holds_per_record => 1,
+            holds_per_day => 1,
+        }
+    }
 );
 
-#When patron category = $category_1 and itemtype = $itemtype1 then reservesallowed, holds_per_day and holds_per_record are 0
-$dbh->do(
-    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record)
-     VALUES (?, ?, ?, ?, ?, ?)},
-    {},
-    $category_1, $branch_1, $itemtype2, 0, 1, 1
+#When patron category = $category_1 and itemtype = $itemtype2 then reservesallowed, holds_per_day and holds_per_record are 0
+Koha::CirculationRules->set_rules(
+    {
+        branchcode   => $branch_1,
+        categorycode => $category_1,
+        itemtype     => $itemtype2,
+        rules        => {
+            reservesallowed => 0,
+            holds_per_record => 0,
+            holds_per_day => 0,
+        }
+    }
 );
 
 #When patron category = $category_2 and itemtype = $itemtype2 then reservesallowed, holds_per_day and holds_per_record are 1
-$dbh->do(
-    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record)
-     VALUES (?, ?, ?, ?, ?, ?)},
-    {},
-    $category_2, $branch_1, $itemtype2, 1, 1, 1
+Koha::CirculationRules->set_rules(
+    {
+        branchcode   => $branch_1,
+        categorycode => $category_2,
+        itemtype     => $itemtype2,
+        rules        => {
+            reservesallowed => 1,
+            holds_per_record => 1,
+            holds_per_day => 1,
+        }
+    }
+);
+
+#When patron category = $category_1 and itemtype = $itemtype2 AND different branch, then reservesallowed, holds_per_day and holds_per_record are 0
+Koha::CirculationRules->set_rules(
+    {
+        branchcode   => $branch_2,
+        categorycode => $category_1,
+        itemtype     => $itemtype2,
+        rules        => {
+            reservesallowed => 0,
+            holds_per_record => 0,
+            holds_per_day => 0,
+        }
+    }
 );
 
+#Create a new biblio record
+my $bibnum3 = $builder->build_sample_biblio({frameworkcode => $frameworkcode})->biblionumber;
+
+# Create an item from new itemtype
+my $item2 = $builder->build_sample_item({
+    homebranch => $branch_1,
+    holdingbranch => $branch_1,
+    itype => $itemtype2,
+    barcode => 'bug23172_CPL',
+    biblionumber => $bibnum3
+});
+
+#Create two new patrons
+my $patronnumcat1 = Koha::Patron->new({branchcode => $branch_1, categorycode => $category_1})->store->borrowernumber;
+my $patronnumcat2 = Koha::Patron->new({branchcode => $branch_1, categorycode => $category_2})->store->borrowernumber;
+my $patronnumcat3 = Koha::Patron->new({branchcode => $branch_2, categorycode => $category_1})->store->borrowernumber;
+
 #Remove existing reserves
 $dbh->do("DELETE FROM reserves");
 
-#Create Bib-level hold for borrower who has a patron category of $category_2
-my $reserveid2 = AddReserve($branch_1,  $patronnumcat2, $bibnum3);
-
 #Bib-level hold for borrower who has a patron category of $category_1
-my $reserveid1 = AddReserve($branch_1,  $patronnumcat1, $bibnum3 );
+my $reserveid1 = AddReserve({
+    branchcode => $branch_1,
+    borrowernumber => $patronnumcat1,
+    biblionumber => $bibnum3
+});
+
+#Create Bib-level hold for borrower who has a patron category of $category_2
+my $reserveid2 = AddReserve({
+    branchcode => $branch_1,
+    borrowernumber => $patronnumcat2,
+    biblionumber => $bibnum3
+});
 
 #Check bib-level hold priority and confirm hold made by patronnumcat1 (who fits into issuingrule with reserveallowed = 0) has first priority
 my $reserve1 = Koha::Holds->find($reserveid1);
@@ -793,27 +828,58 @@ is( $reserve1->priority, 1, 'patronnumcat1 reserve is first priority');
 my $reserve2 = Koha::Holds->find($reserveid2);
 is( $reserve2->priority, 2, 'patronnumcat2 reserve is second priority');
 
-#Return the item
+#Test with ReservesControlBranch system preference set to ItemHomeLibrary
+t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary');
+
+# Reserve 1 should be skipped because the circ rule for that patron's category does not allow reserves
 (undef, $messages, undef, undef) = AddReturn('bug23172_CPL', $branch_1);
 
 #First priority hold is skipped as issuingrule is checked and the first hold does not respect issuingrules (as one of the three fields: reservesallowed, holds_per_day is 0) therefore not allocating item to this bib-level hold
-is( $messages->{ResFound}->{borrowernumber}, $patronnumcat2,'Skip allocating item to first priority hold as it does not respect issuingrules instead allocate to next valid hold (bug 23172)');
+is( $messages->{ResFound}->{borrowernumber}, $patronnumcat2,'First priority hold is skipped because circulation rules disallow holds for this patron category, so item is allocated to next hold.');
 
-#Remove and re-add the issuing rule that prevented patronnumcat1 bib-level hold from being allocated upon item return making reservesallowed, holds_per_day and holds_per_record > 0
-$dbh->do(q{DELETE FROM issuingrules WHERE categorycode = ? AND branchcode = ? AND itemtype = ?},
-        {},
-        $category_1, $branch_1, $itemtype2
-);
-$dbh->do(
-   q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record)
-    VALUES (?, ?, ?, ?, ?, ?)},
-   {},
-   $category_1, $branch_1, $itemtype2, 1, 1, 1
+#Remove existing reserves
+$dbh->do("DELETE FROM reserves");
+
+#Bib-level hold for borrower who has a patron category of $category_1
+my $reserveid3 = AddReserve({
+    branchcode => $branch_1,
+    borrowernumber => $patronnumcat3,
+    biblionumber => $bibnum3
+});
+
+#Create Bib-level hold for borrower who has a patron category of $category_2
+my $reserveid4 = AddReserve({
+    branchcode => $branch_1,
+    borrowernumber => $patronnumcat2,
+    biblionumber => $bibnum3
+});
+
+#Test with ReservesControlBranch system preference set to PatronLibrary
+t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary');
+
+# Reserve 1 should be skipped because the circ rule for that patron's library does not allow reserves
+(undef, $messages, undef, undef) = AddReturn('bug23172_CPL', $branch_1);
+
+#First priority hold is skipped as issuingrule is checked and the first hold does not respect issuingrules (as one of the three fields: reservesallowed, holds_per_day is 0) therefore not allocating item to this bib-level hold
+is( $messages->{ResFound}->{borrowernumber}, $patronnumcat2,'First priority hold is skipped because circulation rules disallow holds for this branch, so item is allocated to next hold.');
+
+# Update the circulation rule that prevented patronnumcat3 bib-level hold from being allocated upon item return making reservesallowed, holds_per_day and holds_per_record > 0
+Koha::CirculationRules->set_rules(
+    {
+        branchcode   => $branch_2,
+        categorycode => $category_1,
+        itemtype     => $itemtype2,
+        rules        => {
+            reservesallowed => 1,
+            holds_per_record => 1,
+            holds_per_day => 1,
+        }
+    }
 );
 
 #Return the item again and notice this time it is allocated to the bib-level hold made by patronnumcat1
 (undef, $messages, undef, undef) = AddReturn('bug23172_CPL', $branch_1);
-is( $messages->{ResFound}->{borrowernumber}, $patronnumcat1,'Skip allocating item to first priority hold as it does not respect issuingrules instead allocate to next valid hold (bug 23172)');
+is( $messages->{ResFound}->{borrowernumber}, $patronnumcat3,'Skip allocating item to first priority hold as it does not respect issuingrules instead allocate to next valid hold (bug 23172)');
 
 $cache->clear_from_cache("MarcStructure-0-$frameworkcode");
 $cache->clear_from_cache("MarcStructure-1-$frameworkcode");
-- 
2.11.0