Bugzilla – Attachment 190518 Details for
Bug 41267
It should be possible to prevent some itemtypes from filling other biblio level holds
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41267: Add fill_other_biblio_holds_policy circulation rule
Bug-41267-Add-fillotherbiblioholdspolicy-circulati.patch (text/plain), 18.91 KB, created by
Nick Clemens (kidclamp)
on 2025-12-15 16:16:40 UTC
(
hide
)
Description:
Bug 41267: Add fill_other_biblio_holds_policy circulation rule
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2025-12-15 16:16:40 UTC
Size:
18.91 KB
patch
obsolete
>From 565a2146d8b8e289dba0f6a11c608c4c5ec82a99 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Tue, 18 Nov 2025 19:07:19 +0000 >Subject: [PATCH] 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 >--- > C4/Reserves.pm | 35 ++++-- > Koha/CirculationRules.pm | 4 + > admin/smart-rules.pl | 101 ++++++++++-------- > .../prog/en/modules/admin/smart-rules.tt | 17 ++- > t/db_dependent/Reserves.t | 34 +++++- > 5 files changed, 136 insertions(+), 55 deletions(-) > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 624cf08444d..7ef2f40918f 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -2198,13 +2198,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->fill( { item_id => $item->id } ); >+ $hold = Koha::Holds->find( $res->{reserve_id} ); > } else { > >- # The item is reserved by someone else. >- # Find this item in the reserves >+ # The next reserve is for someone else, >+ # Check if this patron has a reserve on this biblio/item > > my $lookahead_date = output_pref( > { >@@ -2212,7 +2212,7 @@ sub MoveReserve { > dateformat => 'iso', dateonly => 1 > } > ); >- my $hold = $patron->holds->search( >+ $hold = $patron->holds->search( > { > biblionumber => $item->biblionumber, > reservedate => { '<=' => $lookahead_date }, >@@ -2220,17 +2220,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; > } > } >diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm >index b0b2bd00c2b..b5fbc3c1ab4 100644 >--- a/Koha/CirculationRules.pm >+++ b/Koha/CirculationRules.pm >@@ -74,6 +74,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, >diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl >index 7f5448f511c..3e2488644f9 100755 >--- a/admin/smart-rules.pl >+++ b/admin/smart-rules.pl >@@ -212,11 +212,12 @@ if ( $op eq 'cud-delete' ) { > branchcode => undef, > itemtype => undef, > rules => { >- holdallowed => undef, >- hold_fulfillment_policy => undef, >- bookings_lead_period => undef, >- bookings_trail_period => undef, >- returnbranch => undef, >+ holdallowed => undef, >+ hold_fulfillment_policy => undef, >+ fill_other_biblio_holds_policy => undef, >+ bookings_lead_period => undef, >+ bookings_trail_period => undef, >+ returnbranch => undef, > } > } > ); >@@ -226,11 +227,12 @@ if ( $op eq 'cud-delete' ) { > branchcode => undef, > itemtype => $itemtype, > rules => { >- holdallowed => undef, >- hold_fulfillment_policy => undef, >- bookings_lead_period => undef, >- bookings_trail_period => undef, >- returnbranch => undef, >+ holdallowed => undef, >+ hold_fulfillment_policy => undef, >+ fill_other_biblio_holds_policy => undef, >+ bookings_lead_period => undef, >+ bookings_trail_period => undef, >+ returnbranch => undef, > } > } > ); >@@ -241,11 +243,12 @@ if ( $op eq 'cud-delete' ) { > branchcode => $branch, > itemtype => undef, > rules => { >- holdallowed => undef, >- hold_fulfillment_policy => undef, >- bookings_lead_period => undef, >- bookings_trail_period => undef, >- returnbranch => undef, >+ holdallowed => undef, >+ hold_fulfillment_policy => undef, >+ fill_other_biblio_holds_policy => undef, >+ bookings_lead_period => undef, >+ bookings_trail_period => undef, >+ returnbranch => undef, > } > } > ); >@@ -255,11 +258,12 @@ if ( $op eq 'cud-delete' ) { > branchcode => $branch, > itemtype => $itemtype, > rules => { >- holdallowed => undef, >- hold_fulfillment_policy => undef, >- bookings_lead_period => undef, >- bookings_trail_period => undef, >- returnbranch => undef, >+ holdallowed => undef, >+ hold_fulfillment_policy => undef, >+ fill_other_biblio_holds_policy => undef, >+ bookings_lead_period => undef, >+ bookings_trail_period => undef, >+ returnbranch => undef, > } > } > ); >@@ -609,12 +613,13 @@ elsif ( $op eq 'cud-add' ) { > } > ); > } elsif ( $op eq "cud-add-branch-item" ) { >- my $itemtype = $input->param('itemtype'); >- my $holdallowed = $input->param('holdallowed'); >- my $hold_fulfillment_policy = $input->param('hold_fulfillment_policy'); >- my $bookings_lead_period = $input->param('bookings_lead_period'); >- my $bookings_trail_period = $input->param('bookings_trail_period'); >- my $returnbranch = $input->param('returnbranch'); >+ my $itemtype = $input->param('itemtype'); >+ my $holdallowed = $input->param('holdallowed'); >+ my $hold_fulfillment_policy = $input->param('hold_fulfillment_policy'); >+ my $fill_other_biblio_holds_policy = $input->param('fill_other_biblio_holds_policy'); >+ my $bookings_lead_period = $input->param('bookings_lead_period'); >+ my $bookings_trail_period = $input->param('bookings_trail_period'); >+ my $returnbranch = $input->param('returnbranch'); > > if ( $branch eq "*" ) { > if ( $itemtype eq "*" ) { >@@ -623,11 +628,12 @@ elsif ( $op eq 'cud-add' ) { > itemtype => undef, > branchcode => undef, > rules => { >- holdallowed => $holdallowed, >- hold_fulfillment_policy => $hold_fulfillment_policy, >- bookings_lead_period => $bookings_lead_period, >- bookings_trail_period => $bookings_trail_period, >- returnbranch => $returnbranch, >+ holdallowed => $holdallowed, >+ hold_fulfillment_policy => $hold_fulfillment_policy, >+ fill_other_biblio_holds_policy => $fill_other_biblio_holds_policy, >+ bookings_lead_period => $bookings_lead_period, >+ bookings_trail_period => $bookings_trail_period, >+ returnbranch => $returnbranch, > } > } > ); >@@ -637,11 +643,12 @@ elsif ( $op eq 'cud-add' ) { > itemtype => $itemtype, > branchcode => undef, > rules => { >- holdallowed => $holdallowed, >- hold_fulfillment_policy => $hold_fulfillment_policy, >- bookings_lead_period => $bookings_lead_period, >- bookings_trail_period => $bookings_trail_period, >- returnbranch => $returnbranch, >+ holdallowed => $holdallowed, >+ hold_fulfillment_policy => $hold_fulfillment_policy, >+ fill_other_biblio_holds_policy => $fill_other_biblio_holds_policy, >+ bookings_lead_period => $bookings_lead_period, >+ bookings_trail_period => $bookings_trail_period, >+ returnbranch => $returnbranch, > } > } > ); >@@ -652,11 +659,12 @@ elsif ( $op eq 'cud-add' ) { > itemtype => undef, > branchcode => $branch, > rules => { >- holdallowed => $holdallowed, >- hold_fulfillment_policy => $hold_fulfillment_policy, >- bookings_lead_period => $bookings_lead_period, >- bookings_trail_period => $bookings_trail_period, >- returnbranch => $returnbranch, >+ holdallowed => $holdallowed, >+ hold_fulfillment_policy => $hold_fulfillment_policy, >+ fill_other_biblio_holds_policy => $fill_other_biblio_holds_policy, >+ bookings_lead_period => $bookings_lead_period, >+ bookings_trail_period => $bookings_trail_period, >+ returnbranch => $returnbranch, > } > } > ); >@@ -666,11 +674,12 @@ elsif ( $op eq 'cud-add' ) { > itemtype => $itemtype, > branchcode => $branch, > rules => { >- holdallowed => $holdallowed, >- hold_fulfillment_policy => $hold_fulfillment_policy, >- bookings_lead_period => $bookings_lead_period, >- bookings_trail_period => $bookings_trail_period, >- returnbranch => $returnbranch, >+ holdallowed => $holdallowed, >+ fill_other_biblio_holds_policy => $fill_other_biblio_holds_policy, >+ hold_fulfillment_policy => $hold_fulfillment_policy, >+ bookings_lead_period => $bookings_lead_period, >+ bookings_trail_period => $bookings_trail_period, >+ returnbranch => $returnbranch, > } > } > ); >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt >index bd0526017da..7df73fa6b27 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt >@@ -1310,6 +1310,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> >@@ -1320,12 +1321,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> >@@ -1339,6 +1341,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> >@@ -1391,6 +1400,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> >diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t >index 33a99d714e5..d4e01a196e6 100755 >--- a/t/db_dependent/Reserves.t >+++ b/t/db_dependent/Reserves.t >@@ -1027,7 +1027,7 @@ subtest 'ChargeReserveFee 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(); >@@ -1098,6 +1098,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 >-- >2.39.5
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 41267
:
189682
| 190518