From 916f0e1ba8a4a471fff32c5ea26b72e5d0cfaa42 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 5 Sep 2024 14:03:44 +0200 Subject: [PATCH] Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy --- C4/ILSDI/Services.pm | 5 +- C4/Reserves.pm | 21 ------- Koha/Policy/Patrons/Holds.pm | 58 ++++++++++++++++++ opac/opac-modrequest-suspend.pl | 12 +++- t/db_dependent/Reserves.t | 102 ++++++++++++++------------------ 5 files changed, 114 insertions(+), 84 deletions(-) create mode 100644 Koha/Policy/Patrons/Holds.pm diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 07cea34da30..cb4b235103a 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -24,7 +24,7 @@ use C4::Members; use C4::Items qw( get_hostitemnumbers_of ); use C4::Circulation qw( CanBookBeRenewed barcodedecode CanBookBeIssued AddRenewal ); use C4::Accounts; -use C4::Reserves qw( CanBookBeReserved IsAvailableForItemLevelRequest CalculatePriority AddReserve CanItemBeReserved CanReserveBeCanceledFromOpac ); +use C4::Reserves qw( CanBookBeReserved IsAvailableForItemLevelRequest CalculatePriority AddReserve CanItemBeReserved ); use C4::Context; use C4::Auth; use CGI qw ( -utf8 ); @@ -39,6 +39,7 @@ use Koha::I18N qw(__); use Koha::Items; use Koha::Libraries; use Koha::Patrons; +use Koha::Policy::Patrons::Holds; =head1 NAME @@ -940,7 +941,7 @@ sub CancelHold { return { code => 'RecordNotFound' } unless $hold; # Check if reserve belongs to the borrower and if it is in a state which allows cancellation - return { code => 'BorrowerCannotCancelHold' } unless CanReserveBeCanceledFromOpac( $reserve_id, $borrowernumber ); + return { code => 'BorrowerCannotCancelHold' } unless Koha::Policy::Patrons::Holds->can_cancel_from_opac( $patron, $hold ); $hold->cancel; diff --git a/C4/Reserves.pm b/C4/Reserves.pm index fb18f85c76f..6a2b468606e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -120,7 +120,6 @@ BEGIN { CheckReserves CanBookBeReserved CanItemBeReserved - CanReserveBeCanceledFromOpac CancelExpiredReserves AutoUnsuspendReserves @@ -692,26 +691,6 @@ sub CanItemBeReserved { return _cache { status => 'OK' }; } -=head2 CanReserveBeCanceledFromOpac - - $number = CanReserveBeCanceledFromOpac($reserve_id, $borrowernumber); - - returns 1 if reserve can be cancelled by user from OPAC. - First check if reserve belongs to user, next checks if reserve is not in - transfer or waiting status - -=cut - -sub CanReserveBeCanceledFromOpac { - my ($reserve_id, $borrowernumber) = @_; - - return unless $reserve_id and $borrowernumber; - my $reserve = Koha::Holds->find($reserve_id) or return; - - return 0 unless $reserve->borrowernumber == $borrowernumber; - return $reserve->is_cancelable_from_opac; -} - =head2 GetOtherReserves ($messages,$nextreservinfo)=$GetOtherReserves(itemnumber); diff --git a/Koha/Policy/Patrons/Holds.pm b/Koha/Policy/Patrons/Holds.pm new file mode 100644 index 00000000000..19cca44a5d5 --- /dev/null +++ b/Koha/Policy/Patrons/Holds.pm @@ -0,0 +1,58 @@ +package Koha::Policy::Patrons::Holds; + +# Copyright 2024 Koha Development team +# +# 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 C4::Context; + +use Koha::Result::Boolean; + +=head1 NAME + +Koha::Policy::Patrons::Holds - module to deal with holds policy for patrons + +=head1 API + +=head2 Class Methods + +=head3 new + +=cut + +sub new { + return bless {}, shift; +} + + +=head3 can_cancel_from_opac + +my $can_cancel = Koha::Policy::Patrons::Holds->can_cancel_from_opac( $patron, $hold ); + +Returns whether a patron can cancel a given hold from the OPAC + +=cut + +sub can_cancel_from_opac { + my ( $class, $patron, $hold ) = @_; + + my $can_cancel = ( $patron->borrowernumber == $hold->borrowernumber && $hold->is_cancelable_from_opac ) ? 1 : 0; + return Koha::Result::Boolean->new($can_cancel); +} + +1; diff --git a/opac/opac-modrequest-suspend.pl b/opac/opac-modrequest-suspend.pl index c870671893f..cc6eae801b6 100755 --- a/opac/opac-modrequest-suspend.pl +++ b/opac/opac-modrequest-suspend.pl @@ -19,8 +19,13 @@ use Modern::Perl; use CGI qw ( -utf8 ); use C4::Output; -use C4::Reserves qw( CanReserveBeCanceledFromOpac ToggleSuspend SuspendAll ); +use C4::Reserves qw( ToggleSuspend SuspendAll ); use C4::Auth qw( get_template_and_user ); + +use Koha::Holds; +use Koha::Patrons; +use Koha::Policy::Patrons::Holds; + my $query = CGI->new; my ( $template, $borrowernumber, $cookie ) = get_template_and_user( { @@ -36,7 +41,10 @@ my $suspend_until = $query->param('suspend_until') || undef; my $reserve_id = $query->param('reserve_id'); if ( ( $op eq 'cud-suspend' || $op eq 'cud-unsuspend' ) && $reserve_id ) { - ToggleSuspend( $reserve_id, $suspend_until ) if CanReserveBeCanceledFromOpac($reserve_id, $borrowernumber); + my $patron = Koha::Patrons->find($borrowernumber); + my $hold = Koha::Holds->find($reserve_id); + ToggleSuspend( $reserve_id, $suspend_until ) + if $patron && $hold && Koha::Policy::Patrons::Holds->can_cancel_from_opac( $patron, $hold ); } elsif( $op eq 'cud-suspend_all' || $op eq 'cud-unsuspend_all' ) { SuspendAll( diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 83b84bbc2d0..13b1c8fd1dc 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 77; +use Test::More tests => 73; use Test::MockModule; use Test::Warn; @@ -44,6 +44,7 @@ use Koha::Notice::Templates; use Koha::Patrons; use Koha::Patron::Categories; use Koha::CirculationRules; +use Koha::Policy::Patrons::Holds; BEGIN { require_ok('C4::Reserves'); @@ -137,24 +138,24 @@ $requesters{$branch_1} = Koha::Patron->new({ branchcode => $branch_1, categorycode => $category_2, surname => "borrower from $branch_1", -})->store->borrowernumber; +})->store; for my $i ( 2 .. 5 ) { $requesters{"CPL$i"} = Koha::Patron->new({ branchcode => $branch_1, categorycode => $category_2, surname => "borrower $i from $branch_1", - })->store->borrowernumber; + })->store; } $requesters{$branch_2} = Koha::Patron->new({ branchcode => $branch_2, categorycode => $category_2, surname => "borrower from $branch_2", -})->store->borrowernumber; +})->store; $requesters{$branch_3} = Koha::Patron->new({ branchcode => $branch_3, categorycode => $category_2, surname => "borrower from $branch_3", -})->store->borrowernumber; +})->store; # Configure rules so that $branch_1 allows only $branch_1 patrons # to request its items, while $branch_2 will allow its items @@ -223,7 +224,7 @@ $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum2)); AddReserve( { branchcode => $branch_3, - borrowernumber => $requesters{$branch_3}, + borrowernumber => $requesters{$branch_3}->borrowernumber, biblionumber => $bibnum2, priority => 1, } @@ -231,7 +232,7 @@ AddReserve( AddReserve( { branchcode => $branch_2, - borrowernumber => $requesters{$branch_2}, + borrowernumber => $requesters{$branch_2}->borrowernumber, biblionumber => $bibnum2, priority => 2, } @@ -239,12 +240,12 @@ AddReserve( AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum2, priority => 3, } ); -ModReserveAffect($itemnum_cpl, $requesters{$branch_3}, 0); +ModReserveAffect($itemnum_cpl, $requesters{$branch_3}->borrowernumber, 0); # Now it should have different priorities. my $biblio = Koha::Biblios->find( $bibnum2 ); @@ -253,16 +254,16 @@ is($holds->next->priority, 0, 'Item is correctly waiting'); is($holds->next->priority, 1, 'Item is correctly priority 1'); is($holds->next->priority, 2, 'Item is correctly priority 2'); -my @reserves = Koha::Holds->search({ borrowernumber => $requesters{$branch_3} })->waiting->as_list; +my @reserves = Koha::Holds->search({ borrowernumber => $requesters{$branch_3}->borrowernumber })->waiting->as_list; is( @reserves, 1, 'GetWaiting got only the waiting reserve' ); -is( $reserves[0]->borrowernumber(), $requesters{$branch_3}, 'GetWaiting got the reserve for the correct borrower' ); +is( $reserves[0]->borrowernumber(), $requesters{$branch_3}->borrowernumber, 'GetWaiting got the reserve for the correct borrower' ); $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum2)); AddReserve( { branchcode => $branch_3, - borrowernumber => $requesters{$branch_3}, + borrowernumber => $requesters{$branch_3}->borrowernumber, biblionumber => $bibnum2, priority => 1, } @@ -270,7 +271,7 @@ AddReserve( AddReserve( { branchcode => $branch_2, - borrowernumber => $requesters{$branch_2}, + borrowernumber => $requesters{$branch_2}->borrowernumber, biblionumber => $bibnum2, priority => 2, } @@ -279,7 +280,7 @@ AddReserve( AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum2, priority => 3, } @@ -294,7 +295,7 @@ my $messages; # requests cannot be filled by that item per policy. (undef, $messages, undef, undef) = AddReturn('bug10272_CPL', $branch_2); is( $messages->{ResFound}->{borrowernumber}, - $requesters{$branch_1}, + $requesters{$branch_1}->borrowernumber, 'restrictive library\'s items only fill requests by own patrons (bug 10272)'); # Return the FPL item at FPL. The hold that should be triggered is @@ -306,7 +307,7 @@ t::lib::Mocks::mock_preference( 'LocalHoldsPriority', '' ); (undef, $messages, undef, undef) = AddReturn('bug10272_FPL', $branch_2); is( $messages->{ResFound}->{borrowernumber}, - $requesters{$branch_3}, + $requesters{$branch_3}->borrowernumber, 'for generous library, its items fill first hold request in line (bug 10272)'); $biblio = Koha::Biblios->find( $biblionumber ); @@ -321,7 +322,7 @@ $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum, priority => 1, } @@ -339,7 +340,7 @@ $resdate->add_duration(DateTime::Duration->new(days => 4)); my $reserve_id = AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum, priority => 1, reservation_date => $resdate, @@ -373,7 +374,7 @@ my $now_holder = $builder->build_object({ class => 'Koha::Patrons', value => { my $now_reserve_id = AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum, priority => 2, reservation_date => dt_from_string(), @@ -392,12 +393,12 @@ ModReserve({ reserve_id => $now_reserve_id, rank => 'del', cancellation_reason = # test marking a hold as captured my $hold_notice_count = count_hold_print_messages(); -ModReserveAffect($item->itemnumber, $requesters{$branch_1}, 0); +ModReserveAffect($item->itemnumber, $requesters{$branch_1}->borrowernumber, 0); my $new_count = count_hold_print_messages(); is($new_count, $hold_notice_count + 1, 'patron notified when item set to waiting'); # test that duplicate notices aren't generated -ModReserveAffect($item->itemnumber, $requesters{$branch_1}, 0); +ModReserveAffect($item->itemnumber, $requesters{$branch_1}->borrowernumber, 0); $new_count = count_hold_print_messages(); is($new_count, $hold_notice_count + 1, 'patron not notified a second time (bug 11445)'); @@ -423,7 +424,7 @@ $resdate->add_duration(DateTime::Duration->new(days => 2)); AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum, priority => 1, reservation_date => $resdate, @@ -439,7 +440,7 @@ $dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum, priority => 1, reservation_date => $resdate, @@ -449,7 +450,7 @@ AddReserve( $future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } ); is( $future_holds->count, 0, 'current_holds does not return a future item level hold' ); # 9788c: current_holds returns future wait (confirmed future hold) -ModReserveAffect( $item->itemnumber, $requesters{$branch_1} , 0); #confirm hold +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}->borrowernumber, 0); #confirm hold $future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } ); is( $future_holds->count, 1, 'current_holds returns a future wait (confirmed future hold)' ); # End of tests for bug 9788 @@ -461,7 +462,7 @@ is($p, 4, 'CalculatePriority should now return priority 4'); AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{'CPL2'}, + borrowernumber => $requesters{'CPL2'}->borrowernumber, biblionumber => $bibnum2, priority => $p, } @@ -476,7 +477,7 @@ is($p, 1, 'CalculatePriority should now return priority 1'); AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum, priority => $p, itemnumber => $item->itemnumber, @@ -484,14 +485,14 @@ AddReserve( ); $p = C4::Reserves::CalculatePriority($bibnum); is($p, 2, 'CalculatePriority should now return priority 2'); -ModReserveAffect( $item->itemnumber, $requesters{$branch_1} , 0); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}->borrowernumber, 0); $p = C4::Reserves::CalculatePriority($bibnum); is($p, 1, 'CalculatePriority should now return priority 1'); #add another biblio hold, no resdate AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{'CPL2'}, + borrowernumber => $requesters{'CPL2'}->borrowernumber, biblionumber => $bibnum, priority => $p, } @@ -505,7 +506,7 @@ $resdate->add_duration(DateTime::Duration->new(days => 1)); AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{'CPL2'}, + borrowernumber => $requesters{'CPL2'}->borrowernumber, biblionumber => $bibnum, priority => $p, reservation_date => $resdate, @@ -562,54 +563,37 @@ $dbh->do('DELETE FROM reserves', undef, ($bibnum)); AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum, priority => 1, } ); my (undef, $canres, undef) = CheckReserves( $item ); -is( CanReserveBeCanceledFromOpac(), undef, - 'CanReserveBeCanceledFromOpac should return undef if called without any parameter' -); -is( - CanReserveBeCanceledFromOpac( $canres->{resserve_id} ), - undef, - 'CanReserveBeCanceledFromOpac should return undef if called without the reserve_id' -); -is( - CanReserveBeCanceledFromOpac( undef, $requesters{CPL} ), - undef, - 'CanReserveBeCanceledFromOpac should return undef if called without borrowernumber' -); - -my $cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_1}); -is($cancancel, 1, 'Can user cancel its own reserve'); +my $hold = Koha::Holds->find( $canres->{reserve_id} ); +my $cancancel = Koha::Policy::Patrons::Holds->can_cancel_from_opac( $requesters{$branch_1}, $hold ); +ok( $cancancel, 'Can user cancel its own reserve' ); -$cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_2}); -is($cancancel, 0, 'Other user cant cancel reserve'); +$cancancel = Koha::Policy::Patrons::Holds->can_cancel_from_opac( $requesters{$branch_2}, $hold ); +ok( !$cancancel, 'Other user cant cancel reserve' ); -ModReserveAffect($item->itemnumber, $requesters{$branch_1}, 1); -$cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_1}); -is($cancancel, 0, 'Reserve in transfer status cant be canceled'); - -$dbh->do('DELETE FROM reserves', undef, ($bibnum)); -is( CanReserveBeCanceledFromOpac($canres->{resserve_id}, $requesters{$branch_1}), undef, - 'Cannot cancel a deleted hold' ); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}->borrowernumber, 1 ); +$cancancel = Koha::Policy::Patrons::Holds->can_cancel_from_opac( $requesters{$branch_1}, $hold->get_from_storage ); +ok( !$cancancel, 'Reserve in transfer status cant be canceled' ); AddReserve( { branchcode => $branch_1, - borrowernumber => $requesters{$branch_1}, + borrowernumber => $requesters{$branch_1}->borrowernumber, biblionumber => $bibnum, priority => 1, } ); (undef, $canres, undef) = CheckReserves( $item ); -ModReserveAffect($item->itemnumber, $requesters{$branch_1}, 0); -$cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_1}); -is($cancancel, 0, 'Reserve in waiting status cant be canceled'); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}->borrowernumber, 0 ); +$cancancel = Koha::Policy::Patrons::Holds->can_cancel_from_opac( $requesters{$branch_1}, $hold->get_from_storage ); +ok( !$cancancel, 'Reserve in waiting status cant be canceled' ); # End of tests for bug 12876 -- 2.34.1