From 343c75e16eedfe3c75a8f148185f6427e63c470f Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Wed, 25 May 2022 14:58:36 +0000
Subject: [PATCH] Bug 30847: Consolidate code to check if patron can place
 holds and exit if they cannot

This wil save us fetching more info if we are denying the holds
---
 opac/opac-reserve.pl | 136 +++++++++++++++++++++----------------------
 1 file changed, 67 insertions(+), 69 deletions(-)

diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl
index 5d3933a2adc..6c518c7b8d8 100755
--- a/opac/opac-reserve.pl
+++ b/opac/opac-reserve.pl
@@ -72,18 +72,6 @@ unless ( $can_place_hold_if_available_at_pickup ) {
     }
 }
 
-# check if this user can place a reserve, -1 means use sys pref, 0 means dont block, 1 means block
-if ( $category->effective_BlockExpiredPatronOpacActions ) {
-
-    if ( $patron->is_expired ) {
-
-        # cannot reserve, their card has expired and the rules set mean this is not allowed
-        $template->param( message => 1, expired_patron => 1 );
-        output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 };
-        exit;
-    }
-}
-
 # Pass through any reserve charge
 my $reservefee = $category->reservefee;
 if ( $reservefee > 0){
@@ -123,6 +111,69 @@ if (($#biblionumbers < 0) && (! $query->param('place_reserve'))) {
     exit;
 }
 
+#
+#
+# Here we check that the borrower can actually make reserves Stage 1.
+#
+#
+my $noreserves     = 0;
+if ( $category->effective_BlockExpiredPatronOpacActions ) {
+    if ( $patron->is_expired ) {
+        # cannot reserve, their card has expired and the rules set mean this is not allowed
+        $noreserves = 1;
+        $template->param( message => 1, expired_patron => 1 );
+    }
+}
+
+my $maxoutstanding = C4::Context->preference("maxoutstanding");
+my $amountoutstanding = $patron->account->balance;
+if ( $amountoutstanding && ($amountoutstanding > $maxoutstanding) ) {
+    my $amount = sprintf "%.02f", $amountoutstanding;
+    $template->param( message => 1 );
+    $noreserves = 1;
+    $template->param( too_much_oweing => $amount );
+}
+
+if ( $patron->gonenoaddress && ($patron->gonenoaddress == 1) ) {
+    $noreserves = 1;
+    $template->param(
+        message => 1,
+        GNA     => 1
+    );
+}
+
+if ( $patron->lost && ($patron->lost == 1) ) {
+    $noreserves = 1;
+    $template->param(
+        message => 1,
+        lost    => 1
+    );
+}
+
+if ( $patron->is_debarred ) {
+    $noreserves = 1;
+    $template->param(
+        message          => 1,
+        debarred         => 1,
+        debarred_comment => $patron->debarredcomment,
+        debarred_date    => $patron->debarred,
+    );
+}
+
+my $holds = $patron->holds;
+my $reserves_count = $holds->count;
+$template->param( RESERVES => $holds->unblessed );
+if ( $maxreserves && ( $reserves_count >= $maxreserves ) ) {
+    $template->param( message => 1 );
+    $noreserves = 1;
+    $template->param( too_many_reserves => $holds->count );
+}
+
+if( $noreserves ){
+    output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 };
+    exit;
+}
+
 
 # pass the pickup branch along....
 my $branch = $query->param('branch') || $patron->branchcode || C4::Context->userenv->{branch} || '' ;
@@ -312,66 +363,13 @@ foreach my $biblioNumber (@biblionumbers) {
     $biblioData->{rank} = $reservecount + 1;
 }
 
-#
-#
-# Here we check that the borrower can actually make reserves Stage 1.
-#
-#
-my $noreserves     = 0;
-my $maxoutstanding = C4::Context->preference("maxoutstanding");
-my $amountoutstanding = $patron->account->balance;
-if ( $amountoutstanding && ($amountoutstanding > $maxoutstanding) ) {
-    my $amount = sprintf "%.02f", $amountoutstanding;
-    $template->param( message => 1 );
-    $noreserves = 1;
-    $template->param( too_much_oweing => $amount );
-}
 
-if ( $patron->gonenoaddress && ($patron->gonenoaddress == 1) ) {
-    $noreserves = 1;
-    $template->param(
-        message => 1,
-        GNA     => 1
-    );
+my $requested_reserves_count = scalar( @biblionumbers );
+if ( $maxreserves && ( $reserves_count + $requested_reserves_count > $maxreserves ) ) {
+    $template->param( new_reserves_allowed => $maxreserves - $reserves_count );
 }
 
-if ( $patron->lost && ($patron->lost == 1) ) {
-    $noreserves = 1;
-    $template->param(
-        message => 1,
-        lost    => 1
-    );
-}
-
-if ( $patron->is_debarred ) {
-    $noreserves = 1;
-    $template->param(
-        message          => 1,
-        debarred         => 1,
-        debarred_comment => $patron->debarredcomment,
-        debarred_date    => $patron->debarred,
-    );
-}
-
-my $holds = $patron->holds;
-my $reserves_count = $holds->count;
-$template->param( RESERVES => $holds->unblessed );
-if ( $maxreserves && ( $reserves_count >= $maxreserves ) ) {
-    $template->param( message => 1 );
-    $noreserves = 1;
-    $template->param( too_many_reserves => $holds->count );
-}
-
-unless ( $noreserves ) {
-    my $requested_reserves_count = scalar( @biblionumbers );
-    if ( $maxreserves && ( $reserves_count + $requested_reserves_count > $maxreserves ) ) {
-        $template->param( new_reserves_allowed => $maxreserves - $reserves_count );
-    }
-}
-
-unless ($noreserves) {
-    $template->param( select_item_types => 1 );
-}
+$template->param( select_item_types => 1 );
 
 
 #
-- 
2.25.1