From 8f34568f0cc7fc5cf409664adc82cbc7fb51f5f2 Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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 | 135 +++++++++++++----- reserve/request.pl | 59 +++++--- 2 files changed, 134 insertions(+), 60 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 14868aaff46..e9ad85bc8d0 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -406,22 +406,22 @@ Too many holds: Patron can only place a maximum of [% maxreserves | html %] total holds. [% END %] - [% IF ( patron.is_expired ) %] + [% FOREACH message IN messages %]
- Account has expired -
- [% END %] - [% IF patron.is_debarred %] -
- - Patron has restrictions -
- [% END %] - [% IF member.amount_outstanding && Koha.Preference('maxoutstanding') && member.amount_outstanding > Koha.Preference('maxoutstanding') %] -
- - Patron has outstanding fines: [% member.amount_outstanding | $Price %] + [% IF message.message == 'expired' %] + Account has expired + [% ELSIF message.message == 'restricted' %] + Patron has restrictions + [% ELSIF message.message == 'debt_limit' %] + Patron has outstanding fines: [% message.payload.total_outstanding | $Price %] + [% ELSIF message.message == 'bad_address' %] + Patron's address is flagged as incorrect + [% ELSIF message.message == 'card_lost' %] + Patron's library card is marked as lost + [% ELSIF message.message == 'hold_limit' %] + Too many holds: Patron can only place a maximum of [% message.payload.max_holds | html %] total holds. + [% END %]
[% END %] @@ -436,7 +436,14 @@ [% UNLESS ( multi_hold ) %]
- + [% IF messages.size && Koha.Preference('AllowHoldPolicyOverride') %] + [% FOREACH message IN messages %] + + [% END %] + + [% ELSE %] + + [% END %]
[% ELSE %] @@ -573,22 +580,26 @@ [% 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 %]
- [% END # /IF patron.is_expired || diffbranch ... %] + [% END # /IF messages.size || diffbranch %] [% IF ( messageborrower ) %]
@@ -735,8 +746,19 @@
[% IF ( patron.borrowernumber ) %] - [% IF ( override_required ) %] - + [% IF Koha.Preference('AllowHoldPolicyOverride') && (messages.size || override_required) %] + [% FOREACH message IN messages %] + + [% END %] + [% IF ( override_required ) %] + + [% ELSIF ( none_available ) %] + + [% ELSE %] + + [% END %] + [% ELSIF ( override_required ) %] + [% ELSIF ( none_available ) %] [% ELSE %] @@ -830,8 +852,19 @@ [% END %]
[% IF ( patron.borrowernumber ) %] - [% IF ( override_required ) %] - + [% IF Koha.Preference('AllowHoldPolicyOverride') && (messages.size || override_required) %] + [% FOREACH message IN messages %] + + [% END %] + [% IF ( override_required ) %] + + [% ELSIF ( none_available ) %] + + [% ELSE %] + + [% END %] + [% ELSIF ( override_required ) %] + [% ELSIF ( none_available ) %] [% ELSE %] @@ -1088,8 +1121,19 @@ [% END # /IF hiddencount %]
[% IF ( patron.borrowernumber ) %] - [% IF ( override_required ) %] - + [% IF Koha.Preference('AllowHoldPolicyOverride') && (messages.size || override_required) %] + [% FOREACH message IN messages %] + + [% END %] + [% IF ( override_required ) %] + + [% ELSIF ( none_available ) %] + + [% ELSE %] + + [% END %] + [% ELSIF ( override_required ) %] + [% ELSIF ( none_available ) %] [% ELSE %] @@ -1246,14 +1290,27 @@
[% IF ( patron AND patron.borrowernumber ) %] - [% IF ( override_required ) %] - + [% IF Koha.Preference('AllowHoldPolicyOverride') && (messages.size || override_required) %] + [% FOREACH message IN messages %] + + [% END %] + [% IF ( override_required ) %] + + [% ELSIF ( no_bibs_available ) %] + + [% ELSIF ( none_available ) %] + + [% ELSE %] + + [% END %] + [% ELSIF ( override_required ) %] + [% ELSIF ( no_bibs_available ) %] [% ELSIF ( none_available ) %] [% ELSE %] - + [% END %] [% END # /IF patron %]
diff --git a/reserve/request.pl b/reserve/request.pl index a00a6de53e1..fe339d5c0d2 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'); @@ -243,30 +242,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_holds; + # 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; @@ -276,9 +290,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.51.1