Bugzilla – Attachment 157929 Details for
Bug 34924
Add ability to send 'final auto renewal notice'
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 34924: Add Koha::Checkout->attempt_auto_renew
Bug-34924-Add-KohaCheckout-attemptautorenew.patch (text/plain), 9.72 KB, created by
Emily Lamancusa (emlam)
on 2023-10-26 15:02:22 UTC
(
hide
)
Description:
Bug 34924: Add Koha::Checkout->attempt_auto_renew
Filename:
MIME Type:
Creator:
Emily Lamancusa (emlam)
Created:
2023-10-26 15:02:22 UTC
Size:
9.72 KB
patch
obsolete
>From 0b0db3909d85b38a9877e591a84c4871d073c69e Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Tue, 26 Sep 2023 13:53:44 +0000 >Subject: [PATCH] Bug 34924: Add Koha::Checkout->attempt_auto_renew > >This patch moves the actual renewal out of the auto_renewals cronjob script and into the object and adds tests. The logic for notices is still handled in the script. > >To test: >1 - prove -v t/db_dependent/Koha/Checkouts.t >2 - Add a circ rule with auto_renew checked >3 - Checkout an item to a patron and set due date in the past >4 - Checkout an item to a patron and set due date in the future >5 - perl misc/cronjobs/automatic_renewals.pl -v >6 - Confirm one would be renewed and the other is too_soon >7 - perl misc/cronjobs/automatic_renewals.pl -v --confirm >8 - Confirm the expected issue is successfully renewed > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> >--- > Koha/Checkout.pm | 44 +++++++++++++++++- > misc/cronjobs/automatic_renewals.pl | 52 ++++++++------------- > t/db_dependent/Koha/Checkouts.t | 70 ++++++++++++++++++++++++++++- > 3 files changed, 130 insertions(+), 36 deletions(-) > >diff --git a/Koha/Checkout.pm b/Koha/Checkout.pm >index 178a8c0da4..dab86db7b2 100644 >--- a/Koha/Checkout.pm >+++ b/Koha/Checkout.pm >@@ -23,7 +23,7 @@ use Modern::Perl; > use DateTime; > use Try::Tiny qw( catch try ); > >-use C4::Circulation qw( LostItem MarkIssueReturned ); >+use C4::Circulation qw( AddRenewal CanBookBeRenewed LostItem MarkIssueReturned ); > use Koha::Checkouts::Renewals; > use Koha::Checkouts::ReturnClaims; > use Koha::Database; >@@ -151,6 +151,48 @@ sub renewals { > return Koha::Checkouts::Renewals->_new_from_dbic( $renewals_rs ); > } > >+=head3 attempt_auto_renew >+ >+ my ($success, $error, $updated) = $checkout->auto_renew({ confirm => 1 }); >+ >+Attempt to automatically renew a book. Return error reason if it cannot be renewed. >+Also return whether a change has been made to avoid notifying on more than one attempt. >+ >+If not passed confirm, we will only report and no changes will be made. >+ >+=cut >+ >+sub attempt_auto_renew { >+ my ( $self, $params ) = @_; >+ my $confirm = $params->{confirm} // 0; >+ >+ # CanBookBeRenewed returns 'auto_renew' when the renewal should be done by this script >+ my ( $ok, $error ) = C4::Circulation::CanBookBeRenewed( $self->patron, $self, undef, 1 ); >+ if ( $error eq 'auto_renew' ) { >+ if ($confirm) { >+ my $date_due = C4::Circulation::AddRenewal( >+ { >+ borrowernumber => $self->borrowernumber, >+ itemnumber => $self->itemnumber, >+ branch => $self->branchcode, >+ seen => 0, >+ automatic => 1, >+ } >+ ); >+ $self->auto_renew_error(undef)->store; >+ } >+ return ( 1, undef, 1 ); >+ } else { >+ my $updated = 0; >+ if ( !$self->auto_renew_error || $error ne $self->auto_renew_error ) { >+ $self->auto_renew_error($error)->store if $confirm; >+ $updated = 1; >+ } >+ return ( 0, $error, $updated ); >+ } >+ >+} >+ > =head3 to_api_mapping > > This method returns the mapping for representing a Koha::Checkout object >diff --git a/misc/cronjobs/automatic_renewals.pl b/misc/cronjobs/automatic_renewals.pl >index 16006d5989..c6098d10fb 100755 >--- a/misc/cronjobs/automatic_renewals.pl >+++ b/misc/cronjobs/automatic_renewals.pl >@@ -171,52 +171,38 @@ while ( my $auto_renew = $auto_renews->next ) { > $wants_digest = 0; > } > >- # CanBookBeRenewed returns 'auto_renew' when the renewal should be done by this script >- my ( $ok, $error ) = CanBookBeRenewed( $auto_renew->patron, $auto_renew, undef, 1 ); >- my $updated; >- if ( $error eq 'auto_renew' ) { >- $updated = 1; >+ my ( $success, $error, $updated ) = $auto_renew->attempt_auto_renew({ confirm => $confirm }); >+ if ( $success ) { > if ($verbose) { > say sprintf "Issue id: %s for borrower: %s and item: %s %s be renewed.", > $auto_renew->issue_id, $auto_renew->borrowernumber, $auto_renew->itemnumber, > $confirm ? 'will' : 'would'; > } > if ($confirm) { >- my $date_due = AddRenewal( >- { >- borrowernumber => $auto_renew->borrowernumber, >- itemnumber => $auto_renew->itemnumber, >- branch => $auto_renew->branchcode, >- seen => 0, >- automatic => 1, >- } >- ); > push @item_renewal_ids, $auto_renew->itemnumber; >- $auto_renew->auto_renew_error(undef)->store; > } > push @{ $report{ $auto_renew->borrowernumber } }, $auto_renew >- if ($wants_messages) && !$wants_digest; >- } elsif ( $error eq 'too_many' >- || $error eq 'on_reserve' >- || $error eq 'restriction' >- || $error eq 'overdue' >- || $error eq 'too_unseen' >- || $error eq 'auto_account_expired' >- || $error eq 'auto_too_late' >- || $error eq 'auto_too_much_oweing' >- || $error eq 'auto_too_soon' >- || $error eq 'too_soon' >- || $error eq 'item_denied_renewal' >- || $error eq 'item_issued_to_other_patron' ) >- { >- if ($verbose) { >+ if ( $wants_messages ) && !$wants_digest; >+ } elsif ( >+ # FIXME Do we need to list every status? Why not simply else? >+ $error eq 'too_many' || >+ $error eq 'on_reserve' || >+ $error eq 'restriction' || >+ $error eq 'overdue' || >+ $error eq 'too_unseen' || >+ $error eq 'auto_account_expired' || >+ $error eq 'auto_too_late' || >+ $error eq 'auto_too_much_oweing' || >+ $error eq 'auto_too_soon' || >+ $error eq 'item_denied_renewal' || >+ $error eq 'item_issued_to_other_patron' >+ ) { >+ if ( $verbose ) { > say sprintf "Issue id: %s for borrower: %s and item: %s %s not be renewed. (%s)", > $auto_renew->issue_id, $auto_renew->borrowernumber, $auto_renew->itemnumber, > $confirm ? 'will' : 'would', $error; > } >- $updated = 1 if ( !$auto_renew->auto_renew_error || $error ne $auto_renew->auto_renew_error ); >- if ($updated) { >- $auto_renew->auto_renew_error($error)->store if $confirm; >+ if ( $updated ) { > push @{ $report{ $auto_renew->borrowernumber } }, $auto_renew > if $error ne 'auto_too_soon' && ( $wants_messages && !$wants_digest ); # Do not notify if it's too soon > } >diff --git a/t/db_dependent/Koha/Checkouts.t b/t/db_dependent/Koha/Checkouts.t >index 60c977acc7..35ab5214ce 100755 >--- a/t/db_dependent/Koha/Checkouts.t >+++ b/t/db_dependent/Koha/Checkouts.t >@@ -19,7 +19,9 @@ > > use Modern::Perl; > >-use Test::More tests => 11; >+use Test::More tests => 12; >+use Test::MockModule; >+use Test::Warn; > > use C4::Circulation qw( MarkIssueReturned AddReturn ); > use C4::Reserves qw( AddReserve ); >@@ -481,4 +483,68 @@ subtest 'automatic_checkin' => sub { > }; > > $schema->storage->txn_rollback; >-} >+}; >+ >+subtest 'attempt_auto_renew' => sub { >+ >+ plan tests => 17; >+ >+ $schema->storage->txn_begin; >+ >+ my $renew_error = 'auto_renew'; >+ my $module = Test::MockModule->new('C4::Circulation'); >+ $module->mock( 'CanBookBeRenewed', sub { return ( 1, $renew_error ) } ); >+ my $around_now = dt_from_string(); >+ $module->mock( 'AddRenewal', sub { warn "AddRenewal called" } ); >+ my $checkout = $builder->build_object( >+ { >+ class => 'Koha::Checkouts', >+ value => { >+ date_due => '2023-01-01 23:59:59', >+ returndate => undef, >+ auto_renew => 1, >+ auto_renew_error => undef, >+ onsite_checkout => 0, >+ renewals_count => 0, >+ } >+ } >+ ); >+ >+ my ( $success, $error, $updated ); >+ warning_is { >+ ( $success, $error, $updated ) = $checkout->attempt_auto_renew(); >+ } >+ undef, "AddRenewal not called without confirm"; >+ ok( $success, "Issue is renewed when error is 'auto_renew'" ); >+ is( $error, undef, "No error when renewed" ); >+ ok( $updated, "Issue reported as updated when renewed" ); >+ >+ warning_is { >+ ( $success, $error, $updated ) = $checkout->attempt_auto_renew( { confirm => 1 } ); >+ } >+ "AddRenewal called", "AddRenewal called when confirm is passed"; >+ ok( $success, "Issue is renewed when error is 'auto_renew'" ); >+ is( $error, undef, "No error when renewed" ); >+ ok( $updated, "Issue reported as updated when renewed" ); >+ >+ $renew_error = 'anything_else'; >+ ( $success, $error, $updated ) = $checkout->attempt_auto_renew(); >+ ok( !$success, "Success is untrue for any other status" ); >+ is( $error, 'anything_else', "The error is passed through" ); >+ ok( $updated, "Issue reported as updated when status changes" ); >+ $checkout->discard_changes(); >+ is( $checkout->auto_renew_error, undef, "Error not updated if confirm not passed" ); >+ >+ ( $success, $error, $updated ) = $checkout->attempt_auto_renew( { confirm => 1 } ); >+ ok( !$success, "Success is untrue for any other status" ); >+ is( $error, 'anything_else', "The error is passed through" ); >+ ok( $updated, "Issue updated when confirm passed" ); >+ $checkout->discard_changes(); >+ is( $checkout->auto_renew_error, 'anything_else', "Error updated if confirm passed" ); >+ >+ # Error now equals 'anything_else' >+ ( $success, $error, $updated ) = $checkout->attempt_auto_renew(); >+ ok( !$updated, "Issue not reported as updated when status has not changed" ); >+ >+ $schema->storage->txn_rollback; >+}; >-- >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=157929">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 34924
:
156239
|
156240
|
156241
|
157290
|
157291
|
157292
|
157687
|
157688
|
157689
|
157690
|
157895
|
157896
|
157897
|
157898
| 157929 |
157930
|
157931
|
157932
|
157933
|
168642