Attachment 157930 Details for Bug 34924 – Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed

[patch] Bug 34924: Add 'auto_renew_final' and 'auto_unseen_final' return to CanBookBeRenewed

Bug-34924-Add-autorenewfinal-and-autounseenfinal-r.patch (text/plain), 9.72 KB, created by Emily Lamancusa (emlam) on 2023-10-26 15:02:25 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Emily Lamancusa (emlam)

Created: 2023-10-26 15:02:25 UTC

Size: 9.72 KB

patch

obsolete

>From 9aff7e16ff8a68ad186e3c4a23a7a4f2e6bdf8de Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>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
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
>---
> 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 2e9ed52bdc..baab76fef6 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -2988,7 +2988,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(
>@@ -3012,6 +3015,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;
>@@ -3123,7 +3130,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.34.1

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