From bd6dc389042c5221dbfac32dba1a6bf707a460a0 Mon Sep 17 00:00:00 2001 From: Pedro Amorim Date: Wed, 16 Jul 2025 12:52:39 +0000 Subject: [PATCH] Bug 40517: Move creation of hold group into Patron class. Add exceptions Refactor: Update controller logic to make use of patron->create_hold_group This ensures consistency when creating a new hold group for a patron, regardless if on the Staff UI or REST API --- Koha/Exceptions/HoldGroup.pm | 64 ++++++++++++++++++++++++++++++++++ Koha/Patron.pm | 67 ++++++++++++++++++++++++++++++++++++ opac/opac-reserve.pl | 13 +++---- reserve/placerequest.pl | 15 ++++---- 4 files changed, 144 insertions(+), 15 deletions(-) create mode 100644 Koha/Exceptions/HoldGroup.pm diff --git a/Koha/Exceptions/HoldGroup.pm b/Koha/Exceptions/HoldGroup.pm new file mode 100644 index 00000000000..8ceea1a8e70 --- /dev/null +++ b/Koha/Exceptions/HoldGroup.pm @@ -0,0 +1,64 @@ +package Koha::Exceptions::HoldGroup; + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Koha::Exception; + +use Exception::Class ( + 'Koha::Exceptions::HoldGroup' => { + isa => 'Koha::Exception', + }, + 'Koha::Exceptions::HoldGroup::HoldDoesNotExist' => { + isa => 'Koha::Exceptions::HoldGroup', + description => "One or more holds do not exist", + fields => ['hold_ids'], + }, + 'Koha::Exceptions::HoldGroup::HoldDoesNotBelongToPatron' => { + isa => 'Koha::Exceptions::HoldGroup', + description => "One or more holds do not belong to patron", + fields => ['hold_ids'], + }, + 'Koha::Exceptions::HoldGroup::HoldHasAlreadyBeenFound' => { + isa => 'Koha::Exceptions::HoldGroup', + description => "One or more holds have already been found", + fields => ['barcodes'], + }, + 'Koha::Exceptions::HoldGroup::HoldAlreadyBelongsToHoldGroup' => { + isa => 'Koha::Exceptions::HoldGroup', + description => "One or more holds already belong to a hold group", + fields => ['hold_ids'], + }, +); + +=head1 NAME + +Koha::Exceptions::HoldGroup - Base class for HoldGroup exceptions + +=head1 Exceptions + +=head2 Koha::Exceptions::HoldGroup + +Generic HoldGroup exception + +=head2 Koha::Exceptions::HoldGroup::HoldDoesNotExist + +Exception to be used when one or more provided holds do not exist. + +=cut + +1; diff --git a/Koha/Patron.pm b/Koha/Patron.pm index dda09bff4a2..1f70c473e9c 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -45,6 +45,7 @@ use Koha::DateUtils qw( dt_from_string ); use Koha::Encryption; use Koha::Exceptions; use Koha::Exceptions::Password; +use Koha::Exceptions::HoldGroup; use Koha::Holds; use Koha::HoldGroups; use Koha::ILL::Requests; @@ -1843,6 +1844,72 @@ sub hold_groups { return Koha::HoldGroups->_new_from_dbic($hold_group_rs); } +=head3 create_hold_group + +my $hold_group = $patron->create_hold_group + +Creates and returns a hold group given a list of hold ids + +If force_grouped is supplied, the hold group will be created even if the holds are already grouped + +=cut + +sub create_hold_group { + my ( $self, $hold_ids, $force_grouped ) = @_; + + my @undef_holds; + foreach my $hold_id (@$hold_ids) { + my $hold = Koha::Holds->find($hold_id); + push @undef_holds, $hold_id unless $hold; + } + + Koha::Exceptions::HoldGroup::HoldDoesNotExist->throw( + hold_ids => \@undef_holds, + ) if (@undef_holds); + + my @not_own_holds; + foreach my $hold_id (@$hold_ids) { + my $hold = Koha::Holds->find($hold_id); + push @not_own_holds, $hold_id unless $hold->borrowernumber eq $self->borrowernumber; + } + + Koha::Exceptions::HoldGroup::HoldDoesNotBelongToPatron->throw( + hold_ids => \@not_own_holds, + ) if (@not_own_holds); + + my @already_found_holds; + foreach my $hold_id (@$hold_ids) { + my $hold = Koha::Holds->find($hold_id); + push @already_found_holds, $hold->item->barcode if $hold->found; + } + + Koha::Exceptions::HoldGroup::HoldHasAlreadyBeenFound->throw( + barcodes => \@already_found_holds, + ) if (@already_found_holds); + + my @already_in_group_holds; + foreach my $hold_id (@$hold_ids) { + my $hold = Koha::Holds->find($hold_id); + push @already_in_group_holds, $hold_id if $hold->hold_group_id; + } + + Koha::Exceptions::HoldGroup::HoldAlreadyBelongsToHoldGroup->throw( + hold_ids => \@already_in_group_holds, + ) if @already_in_group_holds && !$force_grouped; + + my $hold_group_rs = $self->_result->create_related( + 'hold_groups', + {} + ); + foreach my $hold_id (@$hold_ids) { + my $hold = Koha::Holds->find($hold_id); + + $hold->hold_group_id( $hold_group_rs->hold_group_id )->store; + } + + return Koha::HoldGroup->_new_from_dbic($hold_group_rs); +} + =head3 curbside_pickups my $curbside_pickups = $patron->curbside_pickups; diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index 0ba22c25f32..a00ad63fa61 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -195,7 +195,7 @@ if ( $op eq 'cud-place_reserve' ) { } my @failed_holds; - my $hold_group; + my @successful_hold_ids; while (@selectedItems) { my $biblioNum = shift(@selectedItems); my $itemNum = shift(@selectedItems); @@ -292,13 +292,6 @@ if ( $op eq 'cud-place_reserve' ) { # Here we actually do the reserveration. Stage 3. if ($canreserve) { - if ($add_to_hold_group) { - - #NOTE: We wait to create a hold group until we know that we can actually place a hold/reserve - if ( !$hold_group ) { - $hold_group = Koha::HoldGroup->new->store; - } - } my $reserve_id = AddReserve( { branchcode => $branch, @@ -313,17 +306,19 @@ if ( $op eq 'cud-place_reserve' ) { found => undef, itemtype => $itemtype, item_group_id => $item_group_id, - hold_group_id => $hold_group ? $hold_group->id : undef, } ); if ($reserve_id) { ++$reserve_cnt; + push @successful_hold_ids, $reserve_id; } else { push @failed_holds, 'not_placed'; } } } + $patron->create_hold_group( \@successful_hold_ids ) if $add_to_hold_group; + print $query->redirect( "/cgi-bin/koha/opac-user.pl?" . ( @failed_holds ? "failed_holds=" . join( '|', @failed_holds ) : q|| ) . "&opac-user-holds=1" ); diff --git a/reserve/placerequest.pl b/reserve/placerequest.pl index 00e3e3a64b7..cd096b80bfa 100755 --- a/reserve/placerequest.pl +++ b/reserve/placerequest.pl @@ -69,9 +69,7 @@ foreach my $bibnum (@holdable_bibs) { if ( $op eq 'cud-placerequest' && $patron ) { my %failed_holds; - if ($hold_group_param) { - $hold_group = Koha::HoldGroup->new->store; - } + my @successful_hold_ids; foreach my $biblionumber ( keys %bibinfos ) { @@ -95,7 +93,7 @@ if ( $op eq 'cud-placerequest' && $patron ) { if ( $can_item_be_reserved eq 'OK' || ( $can_item_be_reserved ne 'itemAlreadyOnHold' && $can_override ) ) { - AddReserve( + my $reserve_id = AddReserve( { branchcode => $item_pickup_location, borrowernumber => $patron->borrowernumber, @@ -113,6 +111,7 @@ if ( $op eq 'cud-placerequest' && $patron ) { } ); + push @successful_hold_ids, $reserve_id; $hold_priority++; } else { @@ -124,7 +123,7 @@ if ( $op eq 'cud-placerequest' && $patron ) { } elsif ( @biblionumbers > 1 || $multi_holds ) { my $bibinfo = $bibinfos{$biblionumber}; if ( $can_override || CanBookBeReserved( $patron->borrowernumber, $biblionumber )->{status} eq 'OK' ) { - AddReserve( + my $reserve_id = AddReserve( { branchcode => $bibinfo->{pickup}, borrowernumber => $patron->borrowernumber, @@ -141,13 +140,14 @@ if ( $op eq 'cud-placerequest' && $patron ) { hold_group_id => $hold_group ? $hold_group->id : undef, } ); + push @successful_hold_ids, $reserve_id; } } else { # place a request on 1st available for ( my $i = 0 ; $i < $holds_to_place_count ; $i++ ) { if ( $can_override || CanBookBeReserved( $patron->borrowernumber, $biblionumber )->{status} eq 'OK' ) { - AddReserve( + my $reserve_id = AddReserve( { branchcode => $branch, borrowernumber => $patron->borrowernumber, @@ -165,11 +165,14 @@ if ( $op eq 'cud-placerequest' && $patron ) { hold_group_id => $hold_group ? $hold_group->id : undef, } ); + push @successful_hold_ids, $reserve_id; } } } } + $patron->create_hold_group( \@successful_hold_ids ) if $hold_group_param; + my $redirect_url = URI->new("request.pl"); my @failed_hold_msgs = (); -- 2.39.5