Bugzilla – Attachment 171064 Details for
Bug 30856
Remove CanReserveBeCanceledFromOpac
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy
Bug-30856-Move-CanReserveBeCanceledFromOpac-to-Koh.patch (text/plain), 19.52 KB, created by
Jonathan Druart
on 2024-09-05 12:17:02 UTC
(
hide
)
Description:
Bug 30856: Move CanReserveBeCanceledFromOpac to Koha::Policy
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2024-09-05 12:17:02 UTC
Size:
19.52 KB
patch
obsolete
>From 3f3947d08615a27464972cdff1d24932d6c922c3 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >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 | 104 ++++++++++++++------------------ > 5 files changed, 115 insertions(+), 85 deletions(-) > create mode 100644 Koha/Policy/Patrons/Holds.pm > >diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm >index 6964e14b59f..c98e7d44f71 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 > >@@ -942,7 +943,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 bc96dc9c3c1..fa300799461 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -118,7 +118,6 @@ BEGIN { > CheckReserves > CanBookBeReserved > CanItemBeReserved >- CanReserveBeCanceledFromOpac > CancelExpiredReserves > > AutoUnsuspendReserves >@@ -687,26 +686,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 ChargeReserveFee > > $fee = ChargeReserveFee( $borrowernumber, $fee, $title ); >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 <http://www.gnu.org/licenses>. >+ >+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 14fb88d5cbf..8a6f46b26a8 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; > >@@ -32,7 +32,7 @@ use C4::Items; > use C4::Biblio qw( GetMarcFromKohaField ModBiblio ); > use C4::HoldsQueue; > use C4::Members; >-use C4::Reserves qw( AddReserve AlterPriority CheckReserves ModReserve ModReserveAffect ReserveSlip CalculatePriority CanReserveBeCanceledFromOpac CanBookBeReserved IsAvailableForItemLevelRequest MoveReserve ChargeReserveFee RevertWaitingStatus CanItemBeReserved MergeHolds ); >+use C4::Reserves qw( AddReserve AlterPriority CheckReserves ModReserve ModReserveAffect ReserveSlip CalculatePriority CanBookBeReserved IsAvailableForItemLevelRequest MoveReserve ChargeReserveFee RevertWaitingStatus CanItemBeReserved MergeHolds ); > use Koha::ActionLogs; > use Koha::Biblios; > use Koha::Caches; >@@ -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'); >@@ -136,24 +137,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 >@@ -222,7 +223,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, > } >@@ -230,7 +231,7 @@ AddReserve( > AddReserve( > { > branchcode => $branch_2, >- borrowernumber => $requesters{$branch_2}, >+ borrowernumber => $requesters{$branch_2}->borrowernumber, > biblionumber => $bibnum2, > priority => 2, > } >@@ -238,12 +239,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 ); >@@ -252,16 +253,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, > } >@@ -269,7 +270,7 @@ AddReserve( > AddReserve( > { > branchcode => $branch_2, >- borrowernumber => $requesters{$branch_2}, >+ borrowernumber => $requesters{$branch_2}->borrowernumber, > biblionumber => $bibnum2, > priority => 2, > } >@@ -278,7 +279,7 @@ AddReserve( > AddReserve( > { > branchcode => $branch_1, >- borrowernumber => $requesters{$branch_1}, >+ borrowernumber => $requesters{$branch_1}->borrowernumber, > biblionumber => $bibnum2, > priority => 3, > } >@@ -293,7 +294,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 >@@ -305,7 +306,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 ); >@@ -320,7 +321,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, > } >@@ -338,7 +339,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, >@@ -372,7 +373,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(), >@@ -391,12 +392,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)'); > >@@ -422,7 +423,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, >@@ -438,7 +439,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, >@@ -448,7 +449,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 >@@ -460,7 +461,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, > } >@@ -475,7 +476,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, >@@ -483,14 +484,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, > } >@@ -504,7 +505,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, >@@ -561,54 +562,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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=171064">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 30856
:
171063
|
171064
|
171195
|
171429
|
171473