From ee318d20f5506e9b4c955633f0e9eddc9c171f29 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 28 Mar 2023 11:31:42 +0000 Subject: [PATCH] Bug 33302: Send and display errors when a hold cannot be placed on the OPAC This patch changes opac-reserve.pl to return the error(s) when placing a hold as a pipe delimited list which is then translated to a message for the user To test: 1 - Find a record with items available on the opac 2 - Click 'place hold' and set things up, but do not confirm 3 - In staff client, do something to make hold invalid: - Make item damaged - Make library not a pickup location - Place other holds for patron up to limit - etc. 4 - Confirm hold on OPAC 5 - You are sent to patron's account, hold is not placed 6 - There is little or no message to explain why 7 - Apply patch 8 - Repeat 9 - Now errors are clear Signed-off-by: Andrew Auld Signed-off-by: Michaela Sieber Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- .../bootstrap/en/modules/opac-user.tt | 46 ++++++++++++++++++- opac/opac-reserve.pl | 31 +++++++++---- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt index ebaecc5d3d..33f4400f34 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt @@ -67,7 +67,51 @@ [% IF failed_holds %]

Notice:

-

One or more holds were not placed due to existing holds.

+

One or more holds were not placed due to following errors:

+
    + [% FOREACH fail IN failed_holds.split('\|') %] +
  • + [% SWITCH fail %] + [% CASE 'damaged' %] + Item damaged + [% CASE 'ageRestricted' %] + Age restricted + [% CASE 'tooManyHoldsForThisRecord' %] + Exceeded max holds per record + [% CASE 'tooManyReservesToday' %] + Daily hold limit reached for patron + [% CASE 'tooManyReserves' %] + Too many holds + [% CASE 'notReservable' %] + Not holdable + [% CASE 'cannotReserveFromOtherBranches' %] + Patron is from different library + [% CASE 'branchNotInHoldGroup' %] + Cannot place hold from patron's library + [% CASE 'itemAlreadyOnHold' %] + Patron already has hold for this item + [% CASE 'cannotBeTransferred' %] + Cannot be transferred to pickup library + [% CASE 'pickupNotInHoldGroup' %] + Only pickup locations within the same hold group are allowed + [% CASE 'noReservesAllowed' %] + No holds are allowed on this item + [% CASE 'libraryNotPickupLocation' %] + Library is not a pickup location + [% CASE 'no_valid_pickup_location' %] + No valid pickup location + [% CASE 'notforloan' %] + Not for loan + [% CASE 'items_available' %] + There are items available in the library, please visit to check them out + [% CASE 'not_placed' %] + Error when placing hold, please report this to the library + [% CASE %] + Error: [% fail | html %] + [% END %] +
    • + [% END %] +
[% END %] diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index 07440efa6a..0778ac5d04 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -212,7 +212,7 @@ if ( $query->param('place_reserve') ) { exit; } - my $failed_holds = 0; + my @failed_holds; while (@selectedItems) { my $biblioNum = shift(@selectedItems); my $itemNum = shift(@selectedItems); @@ -264,11 +264,21 @@ if ( $query->param('place_reserve') ) { my $biblio = Koha::Biblios->find($biblioNum); my $rank = $biblio->holds->search( { found => [ { "!=" => "W" }, undef ] } )->count + 1; if ( $item ) { - $canreserve = 1 if CanItemBeReserved( $patron, $item, $branch )->{status} eq 'OK'; + my $status = CanItemBeReserved( $patron, $item, $branch )->{status}; + if( $status eq 'OK' ){ + $canreserve = 1; + } else { + push @failed_holds, $status; + } + } else { - $canreserve = 1 - if CanBookBeReserved( $borrowernumber, $biblioNum, $branch, { itemtype => $itemtype } )->{status} eq 'OK'; + my $status = CanBookBeReserved( $borrowernumber, $biblioNum, $branch, { itemtype => $itemtype } )->{status}; + if( $status eq 'OK'){ + $canreserve = 1; + } else { + push @failed_holds, $status; + } # Inserts a null into the 'itemnumber' field of 'reserves' table. $itemNum = undef; @@ -279,6 +289,7 @@ if ( $query->param('place_reserve') ) { if ( $maxreserves && $reserve_cnt >= $maxreserves ) { + push @failed_holds, 'tooManyReserves'; $canreserve = 0; } @@ -287,7 +298,8 @@ if ( $query->param('place_reserve') ) { my $nb_of_items_issued = $items_in_this_library->search({ 'issue.itemnumber' => { not => undef }}, { join => 'issue' })->count; my $nb_of_items_unavailable = $items_in_this_library->search({ -or => { lost => { '!=' => 0 }, damaged => { '!=' => 0 }, } }); if ( $items_in_this_library->count > $nb_of_items_issued + $nb_of_items_unavailable ) { - $canreserve = 0 + $canreserve = 0; + push @failed_holds, 'items_available'; } } @@ -309,12 +321,15 @@ if ( $query->param('place_reserve') ) { item_group_id => $item_group_id, } ); - $failed_holds++ unless $reserve_id; - ++$reserve_cnt; + if( $reserve_id ){ + ++$reserve_cnt; + } else { + push @failed_holds, 'not_placed'; + } } } - print $query->redirect("/cgi-bin/koha/opac-user.pl?" . ( $failed_holds ? "failed_holds=$failed_holds" : q|| ) . "#opac-user-holds"); + print $query->redirect("/cgi-bin/koha/opac-user.pl?" . ( @failed_holds ? "failed_holds=" . join('|',@failed_holds) : q|| ) . "#opac-user-holds"); exit; } -- 2.39.5