Bugzilla – Attachment 183487 Details for
Bug 40237
Update reserves/request.pl to use Koha::Patron->can_place_holds
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40237: Make reserve/request.pl use $patron->can_place_holds()
Bug-40237-Make-reserverequestpl-use-patron-canplac.patch (text/plain), 20.35 KB, created by
Martin Renvoize (ashimema)
on 2025-06-25 11:40:36 UTC
(
hide
)
Description:
Bug 40237: Make reserve/request.pl use $patron->can_place_holds()
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-06-25 11:40:36 UTC
Size:
20.35 KB
patch
obsolete
>From b3ba8c0285cb501bbdf4e17895a0e9ad5a5e4457 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Wed, 25 Jun 2025 12:33:46 +0100 >Subject: [PATCH] Bug 40237: Make reserve/request.pl use > $patron->can_place_holds() > >This patch updates the staff interface hold placement script to use >the centralized patron hold validation method instead of duplicated >manual validation logic. > >Changes: >- Replaces manual maxreserves, debt, expiration, and restriction checks > with $patron->can_place_holds() method call >- Adds staff override support by reading override_* parameters and > passing them to the validation method >- Updates template to iterate through validation messages array >- Enhances all "Place hold" buttons to show red "Place hold with > overrides" buttons when AllowHoldPolicyOverride is enabled and > validation issues exist >- Maintains backward compatibility with existing item-level override > functionality > >This consolidates patron-level hold validation logic across the >codebase and provides consistent staff override capabilities in >both OPAC and staff interfaces. > >Test plan: >1. Apply patches and restart services >2. Enable system preference AllowHoldPolicyOverride >3. Create a test patron with various restrictions: > - Set expiration date in the past > - Add fines exceeding maxoutstanding preference > - Mark address as bad (gonenoaddress = 1) > - Mark card as lost (lost = 1) > - Add a debarment/restriction > - Create holds to exceed maxreserves preference >4. Log into staff interface and try to place holds for this patron >5. Verify validation messages appear for each restriction >6. Verify "Place hold with overrides" red buttons appear instead of normal blue buttons >7. Click override button and verify hold is placed successfully >8. Test with single holds, multi-holds, item-level holds, and group holds >9. Disable AllowHoldPolicyOverride and verify normal restriction behavior >10. Verify existing item-level override functionality still works >--- > .../prog/en/modules/reserve/request.tt | 127 +++++++++++++----- > reserve/request.pl | 59 +++++--- > 2 files changed, 130 insertions(+), 56 deletions(-) > >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt >index 1a949d3c4b8..905c3c79d02 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt >@@ -342,22 +342,22 @@ > <strong>Too many holds: </strong> Patron can only place a maximum of [% maxreserves | html %] total holds. > </div> > [% END %] >- [% IF ( patron.is_expired ) %] >+ [% FOREACH message IN messages %] > <div> > <i class="fa-solid fa-triangle-exclamation"></i> >- <strong>Account has expired</strong> >- </div> >- [% END %] >- [% IF patron.is_debarred %] >- <div> >- <i class="fa-solid fa-triangle-exclamation"></i> >- <strong>Patron has restrictions</strong> >- </div> >- [% END %] >- [% IF member.amount_outstanding && Koha.Preference('maxoutstanding') && member.amount_outstanding > Koha.Preference('maxoutstanding') %] >- <div> >- <i class="fa-solid fa-triangle-exclamation"></i> >- <strong>Patron has outstanding fines: [% member.amount_outstanding | $Price %]</strong> >+ [% IF message.message == 'expired' %] >+ <strong>Account has expired</strong> >+ [% ELSIF message.message == 'restricted' %] >+ <strong>Patron has restrictions</strong> >+ [% ELSIF message.message == 'debt_limit' %] >+ <strong>Patron has outstanding fines: [% message.payload.total_outstanding | $Price %]</strong> >+ [% ELSIF message.message == 'bad_address' %] >+ <strong>Patron's address is flagged as incorrect</strong> >+ [% ELSIF message.message == 'card_lost' %] >+ <strong>Patron's library card is marked as lost</strong> >+ [% ELSIF message.message == 'hold_limit' %] >+ <strong>Too many holds: </strong> Patron can only place a maximum of [% message.payload.max_holds | html %] total holds. >+ [% END %] > </div> > [% END %] > >@@ -372,7 +372,14 @@ > </ol> > [% UNLESS ( multi_hold ) %] > <fieldset class="action"> >- <input type="submit" class="btn btn-primary" value="Place hold" /> >+ [% IF messages.size && Koha.Preference('AllowHoldPolicyOverride') %] >+ [% FOREACH message IN messages %] >+ <input type="hidden" name="override_[% message.message | html %]" value="1" /> >+ [% END %] >+ <input type="submit" class="btn btn-danger" value="Place hold with overrides" /> >+ [% ELSE %] >+ <input type="submit" class="btn btn-primary" value="Place hold" /> >+ [% END %] > </fieldset> > [% ELSE %] > <table id="requesttitles"> >@@ -509,22 +516,26 @@ > </div> > [% END # /IF ( exceeded_maxreserves || ... %] > >- [% IF ( patron.is_expired || diffbranch || patron.is_debarred || ( amount_outstanding && Koha.Preference('maxoutstanding') && amount_outstanding > Koha.Preference('maxoutstanding') ) ) %] >+ [% IF messages.size || diffbranch %] > <div class="alert alert-info"> > <ul> >- [% IF ( patron.is_expired ) %] >- <li>[% INCLUDE 'patron-title.inc' patron => patron no_title => 1 no_cardnumber => 1 hide_patron_infos_if_needed => 1 %]: <strong>Account has expired</strong></li> >- [% END %] >- >- [% IF patron.is_debarred %] >- <li>[% INCLUDE 'patron-title.inc' patron => patron no_title => 1 no_cardnumber => 1 hide_patron_infos_if_needed => 1 %]: <strong>Patron has restrictions</strong></li> >- [% END %] >- >- [% IF amount_outstanding && Koha.Preference('maxoutstanding') && amount_outstanding > Koha.Preference('maxoutstanding') %] >- <li >- >[% INCLUDE 'patron-title.inc' patron => patron no_title => 1 no_cardnumber => 1 hide_patron_infos_if_needed => 1 link_to => 'members_pay' %]: >- <strong>Patron has outstanding fines: [% amount_outstanding | $Price %]</strong></li >- > >+ [% FOREACH message IN messages %] >+ <li> >+ [% INCLUDE 'patron-title.inc' patron => patron no_title => 1 no_cardnumber => 1 hide_patron_infos_if_needed => 1 %]: >+ [% IF message.message == 'expired' %] >+ <strong>Account has expired</strong> >+ [% ELSIF message.message == 'restricted' %] >+ <strong>Patron has restrictions</strong> >+ [% ELSIF message.message == 'debt_limit' %] >+ <strong>Patron has outstanding fines: [% message.payload.total_outstanding | $Price %]</strong> >+ [% ELSIF message.message == 'bad_address' %] >+ <strong>Patron's address is flagged as incorrect</strong> >+ [% ELSIF message.message == 'card_lost' %] >+ <strong>Patron's library card is marked as lost</strong> >+ [% ELSIF message.message == 'hold_limit' %] >+ <strong>Too many holds: </strong> Patron can only place a maximum of [% message.payload.max_holds | html %] total holds. >+ [% END %] >+ </li> > [% END %] > > [% IF ( diffbranch ) %] >@@ -536,7 +547,7 @@ > </ul> > <!-- /.dialog.message --> > </div> >- [% END # /IF patron.is_expired || diffbranch ... %] >+ [% END # /IF messages.size || diffbranch %] > > [% IF ( messageborrower ) %] > <div class="alert alert-warning"> >@@ -671,7 +682,18 @@ > > <fieldset class="action"> > [% IF ( patron.borrowernumber ) %] >- [% IF ( override_required ) %] >+ [% IF Koha.Preference('AllowHoldPolicyOverride') && (messages.size || override_required) %] >+ [% FOREACH message IN messages %] >+ <input type="hidden" name="override_[% message.message | html %]" value="1" /> >+ [% END %] >+ [% IF ( override_required ) %] >+ <button type="submit" id="hold_grp_btn" class="btn btn-danger"><i class="fa fa-exclamation-triangle "></i> Place hold with overrides</button> >+ [% ELSIF ( none_available ) %] >+ <button type="submit" id="hold_grp_btn" disabled="disabled" class="btn btn-danger btn-disabled">Place hold with overrides</button> >+ [% ELSE %] >+ <button type="submit" id="hold_grp_btn" class="btn btn-danger">Place hold with overrides</button> >+ [% END %] >+ [% ELSIF ( override_required ) %] > <button type="submit" id="hold_grp_btn" class="btn btn-primary warning"><i class="fa fa-exclamation-triangle "></i> Place hold</button> > [% ELSIF ( none_available ) %] > <button type="submit" id="hold_grp_btn" disabled="disabled" class="btn btn-primary btn-disabled">Place hold</button> >@@ -766,7 +788,18 @@ > [% END %] > <fieldset class="action"> > [% IF ( patron.borrowernumber ) %] >- [% IF ( override_required ) %] >+ [% IF Koha.Preference('AllowHoldPolicyOverride') && (messages.size || override_required) %] >+ [% FOREACH message IN messages %] >+ <input type="hidden" name="override_[% message.message | html %]" value="1" /> >+ [% END %] >+ [% IF ( override_required ) %] >+ <button type="submit" id="hold_any_btn" class="btn btn-danger"><i class="fa fa-exclamation-triangle "></i> Place hold with overrides</button> >+ [% ELSIF ( none_available ) %] >+ <button type="submit" id="hold_any_btn" disabled="disabled" class="btn btn-danger btn-disabled">Place hold with overrides</button> >+ [% ELSE %] >+ <button type="submit" id="hold_any_btn" class="btn btn-danger">Place hold with overrides</button> >+ [% END %] >+ [% ELSIF ( override_required ) %] > <button type="submit" id="hold_any_btn" class="btn btn-primary warning"><i class="fa fa-exclamation-triangle "></i> Place hold</button> > [% ELSIF ( none_available ) %] > <button type="submit" id="hold_any_btn" disabled="disabled" class="btn btn-primary btn-disabled">Place hold</button> >@@ -1016,7 +1049,18 @@ > [% END # /IF hiddencount %] > <fieldset class="action"> > [% IF ( patron.borrowernumber ) %] >- [% IF ( override_required ) %] >+ [% IF Koha.Preference('AllowHoldPolicyOverride') && (messages.size || override_required) %] >+ [% FOREACH message IN messages %] >+ <input type="hidden" name="override_[% message.message | html %]" value="1" /> >+ [% END %] >+ [% IF ( override_required ) %] >+ <button type="submit" id="hold_item_btn" class="btn btn-danger"><i class="fa fa-exclamation-triangle "></i> Place hold with overrides</button> >+ [% ELSIF ( none_available ) %] >+ <button type="submit" id="hold_item_btn" disabled="disabled" class="btn btn-danger btn-disabled">Place hold with overrides</button> >+ [% ELSE %] >+ <button type="submit" id="hold_item_btn" class="btn btn-danger">Place hold with overrides</button> >+ [% END %] >+ [% ELSIF ( override_required ) %] > <button type="submit" id="hold_item_btn" class="btn btn-primary warning"><i class="fa fa-exclamation-triangle "></i> Place hold</button> > [% ELSIF ( none_available ) %] > <button type="submit" id="hold_item_btn" disabled="disabled" class="btn btn-primary btn-disabled">Place hold</button> >@@ -1166,14 +1210,27 @@ > </fieldset> > <fieldset class="action"> > [% IF ( patron AND patron.borrowernumber ) %] >- [% IF ( override_required ) %] >+ [% IF Koha.Preference('AllowHoldPolicyOverride') && (messages.size || override_required) %] >+ [% FOREACH message IN messages %] >+ <input type="hidden" name="override_[% message.message | html %]" value="1" /> >+ [% END %] >+ [% IF ( override_required ) %] >+ <button type="submit" id="hold_multi_btn" class="btn btn-danger"><i class="fa fa-exclamation-triangle "></i> Place holds with overrides</button> >+ [% ELSIF ( no_bibs_available ) %] >+ <button type="submit" id="hold_multi_btn" class="btn btn-danger btn-disabled" disabled="disabled">Place holds with overrides</button> >+ [% ELSIF ( none_available ) %] >+ <button type="submit" id="hold_multi_btn" class="btn btn-danger">Place holds with overrides</button> >+ [% ELSE %] >+ <button type="submit" id="hold_multi_btn" class="btn btn-danger">Place holds with overrides</button> >+ [% END %] >+ [% ELSIF ( override_required ) %] > <button type="submit" id="hold_multi_btn" class="btn btn-primary warning"><i class="fa fa-exclamation-triangle "></i> Place holds</button> > [% ELSIF ( no_bibs_available ) %] > <button type="submit" id="hold_multi_btn" class="btn btn-primary btn-disabled" disabled="disabled">Place holds</button> > [% ELSIF ( none_available ) %] > <button type="submit" id="hold_multi_btn" class="btn btn-primary">Place holds</button> > [% ELSE %] >- <button type="submit" id="hold_multi_btn" class="btn btn-primary" id="multi_hold_submit">Place holds</button> >+ <button type="submit" id="hold_multi_btn" class="btn btn-primary">Place holds</button> > [% END %] > [% END # /IF patron %] > </fieldset> >diff --git a/reserve/request.pl b/reserve/request.pl >index 5908ab692b7..1f5451d59af 100755 >--- a/reserve/request.pl >+++ b/reserve/request.pl >@@ -85,7 +85,6 @@ my $club_hold = $input->param('club') || ''; > my $messageborrower; > my $messageclub; > my $warnings; >-my $messages; > my $exceeded_maxreserves; > my $exceeded_holds_per_record; > my @failed_holds = $input->multi_param('failed_holds'); >@@ -188,30 +187,45 @@ if ( $borrowernumber_hold && !$op ) { > # we check the reserves of the user, and if they can reserve a document > # FIXME At this time we have a simple count of reservs, but, later, we could improve the infos "title" ... > >- my $reserves_count = $patron->holds->count; >+ # Check for staff overrides >+ my %overrides = (); >+ if ( C4::Context->preference('AllowHoldPolicyOverride') ) { >+ $overrides{expired} = 1 if $input->param('override_expired'); >+ $overrides{debt_limit} = 1 if $input->param('override_debt_limit'); >+ $overrides{bad_address} = 1 if $input->param('override_bad_address'); >+ $overrides{card_lost} = 1 if $input->param('override_card_lost'); >+ $overrides{restricted} = 1 if $input->param('override_restricted'); >+ $overrides{hold_limit} = 1 if $input->param('override_hold_limit'); >+ } > >- my $new_reserves_count = scalar(@biblionumbers); >+ # Check if we can place holds for this patron >+ my $can_place_holds_result = $patron->can_place_holds( >+ { >+ no_short_circuit => 1, # Collect all validation errors for staff review >+ overrides => \%overrides >+ } >+ ); > >- my $maxreserves = C4::Context->preference('maxreserves'); >- $template->param( maxreserves => $maxreserves ); >+ my @hold_validation_messages = $can_place_holds_result->messages; > >- if ( $maxreserves >- && ( $reserves_count + $new_reserves_count > $maxreserves ) ) >- { >- my $new_reserves_allowed = >- $maxreserves - $reserves_count > 0 >- ? $maxreserves - $reserves_count >- : 0; >- $warnings = 1; >- $exceeded_maxreserves = 1; >- $template->param( >- new_reserves_allowed => $new_reserves_allowed, >- new_reserves_count => $new_reserves_count, >- reserves_count => $reserves_count, >- maxreserves => $maxreserves, >- ); >+ # Set warnings flag if there are any validation issues >+ if (@hold_validation_messages) { >+ $warnings = 1; >+ >+ # Check specifically for hold limit exceeded to set legacy variable >+ for my $message (@hold_validation_messages) { >+ if ( $message->message eq 'hold_limit' ) { >+ $exceeded_maxreserves = 1; >+ last; >+ } >+ } > } > >+ # Basic counts for template use >+ my $reserves_count = $patron->holds->count; >+ my $new_reserves_count = scalar(@biblionumbers); >+ my $maxreserves = C4::Context->preference('maxreserves'); >+ > # check if the borrower make the reserve in a different branch > if ( $patron->branchcode ne C4::Context->userenv->{'branch'} ) { > $diffbranch = 1; >@@ -221,9 +235,12 @@ if ( $borrowernumber_hold && !$op ) { > $template->param( > patron => $patron, > diffbranch => $diffbranch, >- messages => $messages, >+ messages => \@hold_validation_messages, > warnings => $warnings, > amount_outstanding => $amount_outstanding, >+ reserves_count => $reserves_count, >+ new_reserves_count => $new_reserves_count, >+ maxreserves => $maxreserves, > ); > } > >-- >2.49.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 40237
:
183487
|
183494