Bugzilla – Attachment 171429 Details for
Bug 30856
Remove CanReserveBeCanceledFromOpac
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 30856: Remove C4::Reserves::CanReserveBeCanceledFromOpac
Bug-30856-Remove-C4ReservesCanReserveBeCanceledFro.patch (text/plain), 17.03 KB, created by
Olivier Vezina
on 2024-09-12 19:57:32 UTC
(
hide
)
Description:
Bug 30856: Remove C4::Reserves::CanReserveBeCanceledFromOpac
Filename:
MIME Type:
Creator:
Olivier Vezina
Created:
2024-09-12 19:57:32 UTC
Size:
17.03 KB
patch
obsolete
>From 7d983a8bcb1b4f9e664db7a539dd8461c0eb43ab 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: Remove C4::Reserves::CanReserveBeCanceledFromOpac > >This subroutine can easily be replaced and is not really needed. > >Test plan: >No changes expected, try to suspend/resume holds from the OPAC > >Note that you cannot affect somebody's else holds. > >Note for QA: The extra fetch of Koha::Hold will be removed on bug 37868. > >Signed-off-by: Olivier V <olivier.vezina@inLibro.com> >--- > C4/ILSDI/Services.pm | 5 +- > C4/Reserves.pm | 21 ------ > opac/opac-modrequest-suspend.pl | 10 ++- > t/db_dependent/Reserves.t | 116 +++++++++----------------------- > 4 files changed, 41 insertions(+), 111 deletions(-) > >diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm >index 6964e14b59..c2f9984b96 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 ); >@@ -942,7 +942,8 @@ 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' } >+ if $hold->borrowernumber ne $patron->borrowernumber || !$hold->is_cancelable_from_opac; > > $hold->cancel; > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index bc96dc9c3c..fa30079946 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/opac/opac-modrequest-suspend.pl b/opac/opac-modrequest-suspend.pl >index c870671893..d004baba13 100755 >--- a/opac/opac-modrequest-suspend.pl >+++ b/opac/opac-modrequest-suspend.pl >@@ -19,8 +19,11 @@ 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::Patrons; >+ > my $query = CGI->new; > my ( $template, $borrowernumber, $cookie ) = get_template_and_user( > { >@@ -36,7 +39,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 = $patron->holds->find($reserve_id); >+ ToggleSuspend( $reserve_id, $suspend_until ) >+ if $hold && $hold->is_cancelable_from_opac; > } > 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 14fb88d5cb..616506b9b7 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 => 69; > 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; >@@ -136,24 +136,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 +222,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 +230,7 @@ AddReserve( > AddReserve( > { > branchcode => $branch_2, >- borrowernumber => $requesters{$branch_2}, >+ borrowernumber => $requesters{$branch_2}->borrowernumber, > biblionumber => $bibnum2, > priority => 2, > } >@@ -238,12 +238,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 +252,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 +269,7 @@ AddReserve( > AddReserve( > { > branchcode => $branch_2, >- borrowernumber => $requesters{$branch_2}, >+ borrowernumber => $requesters{$branch_2}->borrowernumber, > biblionumber => $bibnum2, > priority => 2, > } >@@ -278,7 +278,7 @@ AddReserve( > AddReserve( > { > branchcode => $branch_1, >- borrowernumber => $requesters{$branch_1}, >+ borrowernumber => $requesters{$branch_1}->borrowernumber, > biblionumber => $bibnum2, > priority => 3, > } >@@ -293,7 +293,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 +305,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 +320,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 +338,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 +372,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 +391,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 +422,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 +438,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 +448,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 +460,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 +475,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 +483,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 +504,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, >@@ -556,62 +556,6 @@ is( $r1[0]->priority, 3, 'priority for hold in future should be correct'); > is( $r2[0]->priority, 4, 'priority for hold not in future should be correct'); > # end of tests for bug 12630 > >-# Tests for cancel reserves by users from OPAC. >-$dbh->do('DELETE FROM reserves', undef, ($bibnum)); >-AddReserve( >- { >- branchcode => $branch_1, >- borrowernumber => $requesters{$branch_1}, >- 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'); >- >-$cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_2}); >-is($cancancel, 0, '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' ); >- >-AddReserve( >- { >- branchcode => $branch_1, >- borrowernumber => $requesters{$branch_1}, >- 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'); >- >-# End of tests for bug 12876 >- > #### > ####### Testing Bug 13113 - Prevent juvenile/children from reserving ageRestricted material >>> > #### >-- >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=171429">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