From 00bb35cb7bb5bd322b3b7dc80b9842d2815ecdfc Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 26 Sep 2023 15:03:45 +0000 Subject: [PATCH] Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed There is a desire for auto_renewals to treat the final renewal differently. We would like to notify the patron of the final renewal - but not again when the next renewal fails. This patch adds the new return value and tests. To test: prove -v t/db_dependent/Circulation.t --- C4/Circulation.pm | 12 ++++- misc/cronjobs/automatic_renewals.pl | 42 +++++++++--------- t/db_dependent/Circulation.t | 69 ++++++++++++++++++++++++++++- 3 files changed, 99 insertions(+), 24 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index b415674cb2..2acf69df0d 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2985,7 +2985,10 @@ sub CanBookBeRenewed { return ( 0, 'item_issued_to_other_patron') if $issue->borrowernumber != $patron->borrowernumber; return ( 0, 'item_denied_renewal') if $item->is_denied_renewal; - # override_limit will override anything else except on_reserve + my $final_renewal = 0; + my $final_unseen_renewal = 0; + + # override_limit will override anything else except on_reserve unless ( $override_limit ){ my $branchcode = _GetCircControlBranch( $item, $patron ); my $issuing_rule = Koha::CirculationRules->get_effective_rules( @@ -3009,6 +3012,10 @@ sub CanBookBeRenewed { looks_like_number($issuing_rule->{unseen_renewals_allowed}) && $issuing_rule->{unseen_renewals_allowed} <= $issue->unseen_renewals; + $final_renewal = $issuing_rule->{renewalsallowed} == ( $issue->renewals_count + 1 ) ? 1 : 0; + $final_unseen_renewal = ( C4::Context->preference('UnseenRenewals') + && $issuing_rule->{unseen_renewals_allowed} == ( $issue->unseen_renewals + 1 ) ) ? 1 : 0; + my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing'); my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing'); my $restricted = $patron->is_debarred; @@ -3120,7 +3127,8 @@ sub CanBookBeRenewed { return (0, "too_soon", { soonest_renew_date => $soonest } ) unless $override_limit; } - return ( 1, "auto_renew" ) if $auto_renew eq "ok" || $auto_renew eq "auto_too_soon" && !$override_limit; # 0 if auto-renewal should not succeed + my $auto_renew_code = $final_renewal ? 'auto_renew_final' : $final_unseen_renewal ? 'auto_unseen_final' : 'auto_renew'; + return ( 1, $auto_renew_code ) if $auto_renew eq "ok" || $auto_renew eq "auto_too_soon" && !$override_limit; return ( 1, undef ); } diff --git a/misc/cronjobs/automatic_renewals.pl b/misc/cronjobs/automatic_renewals.pl index c6098d10fb..72ed54fb31 100755 --- a/misc/cronjobs/automatic_renewals.pl +++ b/misc/cronjobs/automatic_renewals.pl @@ -153,7 +153,7 @@ while ( my $auto_renew = $auto_renews->next ) { print "examining item '" . $auto_renew->itemnumber . "' to auto renew\n" if $verbose; my ( $borrower_preferences, $wants_messages, $wants_digest ) = ( undef, 0, 0 ); - if ( $send_notices_pref eq 'preferences' ){ + if ( $send_notices_pref eq 'preferences' ) { $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $auto_renew->borrowernumber, @@ -166,13 +166,13 @@ while ( my $auto_renew = $auto_renews->next ) { $wants_digest = 1 if $wants_messages && $borrower_preferences->{wants_digest}; - } else { # Preference is never or cron + } else { # Preference is never or cron $wants_messages = $send_notices; - $wants_digest = 0; + $wants_digest = 0; } - my ( $success, $error, $updated ) = $auto_renew->attempt_auto_renew({ confirm => $confirm }); - if ( $success ) { + 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, @@ -182,27 +182,29 @@ while ( my $auto_renew = $auto_renews->next ) { push @item_renewal_ids, $auto_renew->itemnumber; } push @{ $report{ $auto_renew->borrowernumber } }, $auto_renew - if ( $wants_messages ) && !$wants_digest; + 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 ) { + $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; } - if ( $updated ) { + 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/Circulation.t b/t/db_dependent/Circulation.t index fcd68e9bbe..fc6e96b735 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -431,7 +431,7 @@ subtest "GetIssuingCharges tests" => sub { my ( $reused_itemnumber_1, $reused_itemnumber_2 ); subtest "CanBookBeRenewed tests" => sub { - plan tests => 112; + plan tests => 113; C4::Context->set_preference('ItemsDeniedRenewal',''); # Generate test biblio @@ -1412,8 +1412,72 @@ subtest "CanBookBeRenewed tests" => sub { ); }; - # Too many renewals + subtest "auto_renew_final" => sub { + plan tests => 6; + my $item_to_auto_renew = $builder->build_sample_item(); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => $item_to_auto_renew->itype, + rules => { + norenewalbefore => undef, + renewalsallowed => 1, + } + } + ); + + my $ten_days_before = dt_from_string->add( days => -10 ); + my $ten_days_ahead = dt_from_string->add( days => 10 ); + my $issue = AddIssue( + $renewing_borrower_obj, $item_to_auto_renew->barcode, $ten_days_ahead, undef, $ten_days_before, + undef, { auto_renew => 1 } + ); + $issue->renewals_count(0)->store; + + ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrower_obj, $issue ); + is( $renewokay, 1, 'Success for auto_renewal' ); + is( $error, 'auto_renew_final', 'Auto renewal allowed, but it is the last one' ); + + # Final unseen renewal + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => $item_to_auto_renew->itype, + rules => { + unseen_renewals_allowed => 2, + renewalsallowed => 10, + } + } + ); + t::lib::Mocks::mock_preference( 'UnseenRenewals', 1 ); + $issue->unseen_renewals(1)->renewals_count(1)->store; + + ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrower_obj, $issue ); + is( $renewokay, 1, 'Success for auto_renewal' ); + is( $error, 'auto_unseen_final', 'Final unseen renewal reported, not final overall' ); + + # Final unseen renewal + Koha::CirculationRules->set_rules( + { + categorycode => undef, + branchcode => undef, + itemtype => $item_to_auto_renew->itype, + rules => { + unseen_renewals_allowed => 2, + renewalsallowed => 2, + } + } + ); + ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrower_obj, $issue ); + is( $renewokay, 1, 'Success for auto_renewal' ); + is( $error, 'auto_renew_final', 'Final auto renewal reported' ); + + }; + + # Too many renewals # set policy to forbid renewals Koha::CirculationRules->set_rules( { @@ -1444,6 +1508,7 @@ subtest "CanBookBeRenewed tests" => sub { } ); t::lib::Mocks::mock_preference('UnseenRenewals', 1); + $issue_1->unseen_renewals(2)->store; ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrower_obj, $issue_1); -- 2.30.2