Bug 41267: Add fill_other_biblio_holds_policy circulation rule

This patch adds a new rule for itemtypes to allow/prevent filling other holds on a bibliographic record
The rule will obey ReservesControlBranch for determining the rules

To test:
 1 - Apply patches
 2 - Setup/choose 2 item types in Administration->Item types
 3 - In Administration -> Circulation and fine rules scroll to the bottom
 4 - Set one item type to "Fill other holds on record" = 'Yes' and the other to 'No'
 5 - Place a next available hold on the record
 6 - Checkout the 'Yes' item type to the patron
 7 - Their hold should fill
 8 - Check in the items
 9 - Place a next available hold on the record
10 - Checkout the 'No' itemtype to the patron
11 - Their hold should not be filled
12 - Check in the item and confirm the hold
13 - Checkout the itme to the patron, the hold fills
     NOTE: While seemingly counter intuitive, the rule controls whether a hold NOT tied to this item will fill
     Confirming the hold adds an itemnumber to the hold. In most cases, these items will not be holdable, so won't trigger at checkin
14 - Set a rule for the patrons branch to No and set system preference ReservesControlBranch to 'Patrons library'
15 - repeat 5&6
16 - The hold should not fill
17 - Change reserves control branch to items home library
18 - checkin /checkout the yes item type
19 - The hold should fill

Signed-off-by: Andrew Auld <andrew.auld@openfifth.co.uk>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

C4/Reserves.pm

@@ -2144,13 +2144,13 @@ sub MoveReserve {
     my $lookahead = C4::Context->preference('ConfirmFutureHolds');    #number of days to look for future holds
     my ( $restype, $res, undef ) = CheckReserves( $item, $lookahead );
 
+    my $hold;
     if ( $res && $res->{borrowernumber} == $patron->borrowernumber ) {
-        my $hold = Koha::Holds->find( $res->{reserve_id} );
+        $hold = Koha::Holds->find( $res->{reserve_id} );
-        $hold->fill( { item_id => $item->id } );
     } else {
 
-        # The item is reserved by someone else.
+        # The next reserve is for someone else,
-        # Find this item in the reserves
+        # Check if this patron has a reserve on this biblio/item
 
         my $lookahead_date = output_pref(
             {
@@ -2158,7 +2158,7 @@ sub MoveReserve {
                 dateformat => 'iso', dateonly => 1
             }
         );
-        my $hold = $patron->holds->search(
+        $hold = $patron->holds->search(
             {
                 biblionumber => $item->biblionumber,
                 reservedate  => { '<=' => $lookahead_date },
@@ -2166,17 +2166,38 @@ sub MoveReserve {
             },
             { order_by => 'priority' }
         )->next();
+    }
 
-        if ($hold) {
+    if ($hold) {
+        if ( $hold->itemnumber && $hold->itemnumber == $item->id ) {
 
             # The item is reserved by the current patron
             $hold->fill( { item_id => $item->id } );
-        }
+        } else {
+            my $controlbranch = $patron->branchcode;
+            if ( C4::Context->preference('ReservesControlBranch') eq 'ItemHomeLibrary' ) {
+                $controlbranch = $item->homebranch;
+            }
 
+            # If no rule is set, the default behavior is to fill the hold
+            my $fill_other_biblio_holds_policy = Koha::CirculationRules->get_effective_rule_value(
+                {
+                    categorycode => undef,
+                    itemtype     => $item->itype,
+                    branchcode   => $controlbranch,
+                    rule_name    => 'fill_other_biblio_holds_policy',
+                }
+            ) // 1;
+
+            $hold->fill( { item_id => $item->id } ) if $fill_other_biblio_holds_policy;
+        }
+    }
+
+    if ( $cancelreserve && $res ) {
         $hold = Koha::Holds->find( $res->{reserve_id} );
         if ( $cancelreserve eq 'revert' ) {
             $hold->revert_found();
-        } elsif ( $cancelreserve eq 'cancel' || $cancelreserve ) {    # cancel reserves on this item
+        } else {    # cancel reserves on this item
             $hold->cancel;
         }
     }

Koha/CirculationRules.pm

@@ -78,6 +78,10 @@ our $RULE_KINDS = {
         scope        => [ 'branchcode', 'itemtype' ],
         can_be_blank => 0,
     },
+    fill_other_biblio_holds_policy => {
+        scope        => [ 'branchcode', 'itemtype' ],
+        can_be_blank => 0,
+    },
     returnbranch => {
         scope        => [ 'branchcode', 'itemtype' ],
         can_be_blank => 0,

admin/smart-rules.pl

@@ -214,11 +214,12 @@ if ( $op eq 'cud-delete' ) {
             branchcode => $rule_branch,
             itemtype   => $rule_itemtype,
             rules      => {
-                holdallowed             => undef,
+                holdallowed                    => undef,
-                hold_fulfillment_policy => undef,
+                hold_fulfillment_policy        => undef,
-                bookings_lead_period    => undef,
+                fill_other_biblio_holds_policy => undef,
-                bookings_trail_period   => undef,
+                bookings_lead_period           => undef,
-                returnbranch            => undef,
+                bookings_trail_period          => undef,
+                returnbranch                   => undef,
             }
         }
     );
@@ -470,12 +471,13 @@ elsif ( $op eq 'cud-add' ) {
         }
     );
 } elsif ( $op eq "cud-add-branch-item" ) {
-    my $itemtype                = $input->param('itemtype');
+    my $itemtype                       = $input->param('itemtype');
-    my $holdallowed             = $input->param('holdallowed');
+    my $holdallowed                    = $input->param('holdallowed');
-    my $hold_fulfillment_policy = $input->param('hold_fulfillment_policy');
+    my $hold_fulfillment_policy        = $input->param('hold_fulfillment_policy');
-    my $bookings_lead_period    = $input->param('bookings_lead_period');
+    my $fill_other_biblio_holds_policy = $input->param('fill_other_biblio_holds_policy');
-    my $bookings_trail_period   = $input->param('bookings_trail_period');
+    my $bookings_lead_period           = $input->param('bookings_lead_period');
-    my $returnbranch            = $input->param('returnbranch');
+    my $bookings_trail_period          = $input->param('bookings_trail_period');
+    my $returnbranch                   = $input->param('returnbranch');
 
     my $rule_branch   = $branch eq '*'   ? undef : $branch;
     my $rule_itemtype = $itemtype eq '*' ? undef : $itemtype;
@@ -485,11 +487,12 @@ elsif ( $op eq 'cud-add' ) {
             itemtype   => $rule_itemtype,
             branchcode => $rule_branch,
             rules      => {
-                holdallowed             => $holdallowed,
+                holdallowed                    => $holdallowed,
-                hold_fulfillment_policy => $hold_fulfillment_policy,
+                hold_fulfillment_policy        => $hold_fulfillment_policy,
-                bookings_lead_period    => $bookings_lead_period,
+                fill_other_biblio_holds_policy => $fill_other_biblio_holds_policy,
-                bookings_trail_period   => $bookings_trail_period,
+                bookings_lead_period           => $bookings_lead_period,
-                returnbranch            => $returnbranch,
+                bookings_trail_period          => $bookings_trail_period,
+                returnbranch                   => $returnbranch,
             }
         }
     );

koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt

@@ -1379,6 +1379,7 @@
                 <tr>
                     <th>Item type</th>
                     <th>Hold policy</th>
+                    <th>Fill other holds on record</th>
                     <th>Hold and booking pickup library match</th>
                     <th>Booking pre-processing (days)</th>
                     <th>Booking post-processing (days)</th>
@@ -1389,12 +1390,13 @@
                     [% SET it = i.itemtype %]
                     [% SET c = undef %]
                     [% SET holdallowed = all_rules.$c.$it.holdallowed %]
+                    [% SET fill_other_biblio_holds_policy = all_rules.$c.$it.fill_other_biblio_holds_policy %]
                     [% SET hold_fulfillment_policy = all_rules.$c.$it.hold_fulfillment_policy %]
                     [% SET bookings_lead_period = all_rules.$c.$it.bookings_lead_period %]
                     [% SET bookings_trail_period = all_rules.$c.$it.bookings_trail_period %]
                     [% SET returnbranch = all_rules.$c.$it.returnbranch %]
 
-                    [% IF holdallowed || hold_fulfillment_policy || returnbranch || bookings_lead_period || bookings_trail_period %]
+                    [% IF holdallowed || hold_fulfillment_policy || fill_other_biblio_holds_policy || returnbranch || bookings_lead_period || bookings_trail_period %]
                         <tr>
                             <td> [% i.translated_description | html %] </td>
                             <td>
@@ -1408,6 +1410,13 @@
                                     <span>No holds allowed</span>
                                 [% END %]
                             </td>
+                            <td>
+                                [% IF fill_other_biblio_holds_policy %]
+                                    <span> Yes </span>
+                                [% ELSE %]
+                                    <span> No </span>
+                                [% END %]
+                            </td>
                             <td>
                                 [% IF hold_fulfillment_policy == 'any' %]
                                     <span>any library</span>
@@ -1460,6 +1469,12 @@
                             <option value="not_allowed">No holds allowed</option>
                         </select>
                     </td>
+                    <td>
+                        <select name="fill_other_biblio_holds_policy">
+                            <option value="1" selected="selected"> Yes </option>
+                            <option value="0"> No </option>
+                        </select>
+                    </td>
                     <td>
                         <select name="hold_fulfillment_policy">
                             <option value="any"> any library </option>

t/db_dependent/Reserves.t

@@ -1072,7 +1072,7 @@ subtest 'Hold fee charging tests' => sub {
 
 subtest 'MoveReserve additional test' => sub {
 
-    plan tests => 8;
+    plan tests => 10;
 
     # Create the items and patrons we need
     my $biblio = $builder->build_sample_biblio();
@@ -1143,6 +1143,38 @@ subtest 'MoveReserve additional test' => sub {
     is( $patron_2->holds->count,       0, "The 2nd patron no longer has a hold" );
     is( $patron_2->old_holds->count(), 2, "The 2nd patron's hold was filled and moved to old holds" );
 
+    my $reserve_4 = AddReserve(
+        {
+            branchcode     => $item_2->homebranch,
+            borrowernumber => $patron_2->borrowernumber,
+            biblionumber   => $biblio->biblionumber,
+            priority       => 1,
+            itemnumber     => undef,
+        }
+    );
+    Koha::CirculationRules->set_rule(
+        {
+            branchcode => $item_1->homebranch,
+            itemtype   => $item_1->itype,
+            rule_name  => 'fill_other_biblio_holds_policy',
+            rule_value => 0
+        }
+    );
+    Koha::CirculationRules->set_rule(
+        {
+            branchcode => $patron_2->branchcode,
+            itemtype   => $item_1->itype,
+            rule_name  => 'fill_other_biblio_holds_policy',
+            rule_value => 1
+        }
+    );
+    t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'ItemHomeLibrary' );
+    MoveReserve( $item_1, $patron_2 );
+    is( $patron_2->holds->count, 1, "The hold is not filled because of circ rules" );
+    t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'PatronLibrary' );
+    MoveReserve( $item_1, $patron_2 );
+    is( $patron_2->holds->count, 0, "The hold is filled because of circ rules and affected by ReservesControlBranch" );
+
 };
 
 # FIXME: Should be in Circulation.t