Attachment 135016 Details for Bug 30167 – Bug 30167: [21.11.x] Return soonest renewal date when renewal is to soon

[patch] Bug 30167: [21.11.x] Return soonest renewal date when renewal is to soon

Bug-30167-2111x-Return-soonest-renewal-date-when-r.patch (text/plain), 15.99 KB, created by Nick Clemens (kidclamp) on 2022-05-16 11:08:47 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2022-05-16 11:08:47 UTC

Size: 15.99 KB

patch

obsolete

>From 2f39ea1528d06184c0f08b14cc4c59600b03069c Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Wed, 23 Feb 2022 16:01:22 +0000
>Subject: [PATCH] Bug 30167: [21.11.x] Return soonest renewal date when renewal
> is to soon
>
>This patch adds an 'info' return param to CanBookBeRenewed and passes
>the soonest renewal date when returning too_soon errors
>
>To test:
>1 - prove -v t/db_dependent/Circulation.t
>
>Fix whitespace
>
>Signed-off-by: David Nind <david@davidnind.com>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 30167: Use returned renewal date rather than fetching
>
>This patch updates the three scripts that fetched the soonest renewal date
>to use the return from CanBookBeRenewed
>
>To test:
>1 - Set a circulation rule with a 'no renewal before' set to 3, loan length set to 5
>2 - Check out an item to a patron that uses this rule
>3 - Verify the checkouts for the patron show the correct 'No renewal before' date
>4 - Sign in to the patron's opac account
>5 - Verify the item shows it cannot be renewed, and shows the correct date
>6 - Go to Circulation->Renew
>7 - Attempt to renew using barcode
>8 - Confirm error shows the soonest renewal date
>
>Signed-off-by: David Nind <david@davidnind.com>
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 30167: (follow-up) Return a hash with soonest_renew_date
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>
>Bug 30167: (follow-up) Compare dt objects
>---
> C4/Circulation.pm                             | 24 ++++++++++++-------
> circ/renew.pl                                 | 10 ++++----
> .../bootstrap/en/modules/opac-user.tt         |  2 +-
> opac/opac-user.pl                             | 11 +++------
> svc/checkouts                                 |  6 ++---
> t/db_dependent/Circulation.t                  | 24 ++++++++++++-------
> 6 files changed, 42 insertions(+), 35 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 1ebc53a50e..8fdf9c0889 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -2727,7 +2727,7 @@ sub GetUpcomingDueIssues {
> 
> =head2 CanBookBeRenewed
> 
>-  ($ok,$error) = &CanBookBeRenewed($borrowernumber, $itemnumber[, $override_limit]);
>+  ($ok,$error,$info) = &CanBookBeRenewed($borrowernumber, $itemnumber[, $override_limit]);
> 
> Find out whether a borrowed item may be renewed.
> 
>@@ -2745,7 +2745,10 @@ that are automatically renewed.
> C<$CanBookBeRenewed> returns a true value if the item may be renewed. The
> item must currently be on loan to the specified borrower; renewals
> must be allowed for the item's type; and the borrower must not have
>-already renewed the loan. $error will contain the reason the renewal can not proceed
>+already renewed the loan.
>+    $error will contain the reason the renewal can not proceed
>+    $info will contain a hash of additional info
>+      currently 'soonest_renew_date' if error is 'too soon'
> 
> =cut
> 
>@@ -2753,6 +2756,7 @@ sub CanBookBeRenewed {
>     my ( $borrowernumber, $itemnumber, $override_limit, $cron ) = @_;
> 
>     my $auto_renew = "no";
>+    my $soonest;
> 
>     my $item      = Koha::Items->find($itemnumber)      or return ( 0, 'no_item' );
>     my $issue = $item->checkout or return ( 0, 'no_checkout' );
>@@ -2797,13 +2801,13 @@ sub CanBookBeRenewed {
>             return ( 0, 'overdue');
>         }
> 
>-        $auto_renew = _CanBookBeAutoRenewed({
>+        ( $auto_renew, $soonest ) = _CanBookBeAutoRenewed({
>             patron     => $patron,
>             item       => $item,
>             branchcode => $branchcode,
>             issue      => $issue
>         });
>-        return ( 0, $auto_renew  ) if $auto_renew =~ 'auto_too_soon' && $cron;
>+        return ( 0, $auto_renew, { soonest_renew_date => $soonest } ) if $auto_renew =~ 'auto_too_soon' && $cron;
>         # cron wants 'too_soon' over 'on_reserve' for performance and to avoid
>         # extra notices being sent. Cron also implies no override
>         return ( 0, $auto_renew  ) if $auto_renew =~ 'auto_account_expired';
>@@ -2890,9 +2894,10 @@ sub CanBookBeRenewed {
>     }
> 
>     return ( 0, "on_reserve" ) if $resfound;    # '' when no hold was found
>-    return ( 0, $auto_renew  ) if $auto_renew =~ 'too_soon';#$auto_renew ne "no" && $auto_renew ne "ok";
>-    if ( GetSoonestRenewDate($borrowernumber, $itemnumber) > dt_from_string() ){
>-        return (0, "too_soon") unless $override_limit;
>+    return ( 0, $auto_renew, { soonest_renew_date => $soonest } ) if $auto_renew =~ 'too_soon';#$auto_renew ne "no" && $auto_renew ne "ok";
>+    $soonest = GetSoonestRenewDate($borrowernumber, $itemnumber);
>+    if ( $soonest > dt_from_string() ){
>+        return (0, "too_soon", { soonest_renew_date => $soonest } ) unless $override_limit;
>     }
> 
>     return ( 0, "auto_renew" ) if $auto_renew eq "ok" && !$override_limit; # 0 if auto-renewal should not succeed
>@@ -4350,9 +4355,10 @@ sub _CanBookBeAutoRenewed {
>         }
>     }
> 
>-    if ( GetSoonestRenewDate( $patron->id, $item->id ) > dt_from_string() )
>+    my $soonest = GetSoonestRenewDate($patron->id, $item->id);
>+    if ( $soonest > dt_from_string() )
>     {
>-        return "auto_too_soon";
>+        return ( "auto_too_soon", $soonest );
>     }
> 
>     return "ok";
>diff --git a/circ/renew.pl b/circ/renew.pl
>index 97b6317067..5a1ca18ef8 100755
>--- a/circ/renew.pl
>+++ b/circ/renew.pl
>@@ -23,7 +23,7 @@ use CGI qw ( -utf8 );
> use C4::Context;
> use C4::Auth qw( get_template_and_user );
> use C4::Output qw( output_html_with_http_headers );
>-use C4::Circulation qw( barcodedecode CanBookBeRenewed GetSoonestRenewDate GetLatestAutoRenewDate AddRenewal );
>+use C4::Circulation qw( barcodedecode CanBookBeRenewed GetLatestAutoRenewDate AddRenewal );
> use Koha::DateUtils qw( dt_from_string output_pref );
> use Koha::Database;
> use Koha::BiblioFrameworks;
>@@ -67,7 +67,8 @@ if ($barcode) {
>             
>             if ( ( $borrower->debarred() || q{} ) lt dt_from_string()->ymd() ) {
>                 my $can_renew;
>-                ( $can_renew, $error ) =
>+                my $info;
>+                ( $can_renew, $error, $info ) =
>                   CanBookBeRenewed( $borrower->borrowernumber(),
>                     $item->itemnumber(), $override_limit );
> 
>@@ -82,10 +83,7 @@ if ($barcode) {
>                 }
> 
>                 if ( $error && ($error eq 'too_soon' or $error eq 'auto_too_soon') ) {
>-                    $soonest_renew_date = C4::Circulation::GetSoonestRenewDate(
>-                        $borrower->borrowernumber(),
>-                        $item->itemnumber(),
>-                    );
>+                    $soonest_renew_date = $info->{soonest_renew_date};
>                 }
>                 if ( $error && ( $error eq 'auto_too_late' ) ) {
>                     $latest_auto_renew_date = C4::Circulation::GetLatestAutoRenewDate(
>diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
>index a3f258e041..b2a46ab161 100644
>--- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
>+++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
>@@ -404,7 +404,7 @@
>                                                                 [% END %]
>                                                             )</span>
>                                                         [% ELSIF ( ISSUE.too_soon ) %]
>-                                                            No renewal before [% ISSUE.soonestrenewdate | html %]
>+                                                            No renewal before [% ISSUE.soonestrenewdate | $KohaDates as_due_date => 1 %]
>                                                             <span class="renewals">(
>                                                                 [% ISSUE.renewsleft | html %] of [% ISSUE.renewsallowed | html %] renewals remaining
>                                                                 [% IF Koha.Preference('UnseenRenewals') && ISSUE.unseenallowed %]
>diff --git a/opac/opac-user.pl b/opac/opac-user.pl
>index e610cabc2a..03dd3dd0ed 100755
>--- a/opac/opac-user.pl
>+++ b/opac/opac-user.pl
>@@ -27,7 +27,7 @@ use C4::Koha qw(
>     GetNormalizedISBN
>     GetNormalizedUPC
> );
>-use C4::Circulation qw( CanBookBeRenewed GetRenewCount GetIssuingCharges GetSoonestRenewDate );
>+use C4::Circulation qw( CanBookBeRenewed GetRenewCount GetIssuingCharges );
> use C4::External::BakerTaylor qw( image_url link_url );
> use C4::Reserves qw( GetReserveStatus );
> use C4::Members;
>@@ -222,7 +222,7 @@ if ( $pending_checkouts->count ) { # Useless test
>         $issue->{rentalfines} = $rental_fines->total_outstanding;
> 
>         # check if item is renewable
>-        my ($status,$renewerror) = CanBookBeRenewed( $borrowernumber, $issue->{'itemnumber'} );
>+        my ($status,$renewerror,$info) = CanBookBeRenewed( $borrowernumber, $issue->{'itemnumber'} );
>         (
>             $issue->{'renewcount'},
>             $issue->{'renewsallowed'},
>@@ -253,12 +253,7 @@ if ( $pending_checkouts->count ) { # Useless test
> 
>             if ( $renewerror eq 'too_soon' ) {
>                 $issue->{'too_soon'}         = 1;
>-                $issue->{'soonestrenewdate'} = output_pref(
>-                    C4::Circulation::GetSoonestRenewDate(
>-                        $issue->{borrowernumber},
>-                        $issue->{itemnumber}
>-                    )
>-                );
>+                $issue->{'soonestrenewdate'} = $info->{soonest_renew_date};
>             }
>         }
> 
>diff --git a/svc/checkouts b/svc/checkouts
>index 096d953d1f..78db2acf18 100755
>--- a/svc/checkouts
>+++ b/svc/checkouts
>@@ -23,7 +23,7 @@ use CGI;
> use JSON qw(to_json);
> 
> use C4::Auth qw(check_cookie_auth haspermission);
>-use C4::Circulation qw(GetIssuingCharges CanBookBeRenewed GetRenewCount GetSoonestRenewDate);
>+use C4::Circulation qw(GetIssuingCharges CanBookBeRenewed GetRenewCount );
> use C4::Overdues qw(GetFine);
> use C4::Context;
> 
>@@ -153,13 +153,13 @@ while ( my $c = $sth->fetchrow_hashref() ) {
>     my ($charge) = GetIssuingCharges( $c->{itemnumber}, $c->{borrowernumber} );
>     my $fine = GetFine( $c->{itemnumber}, $c->{borrowernumber} );
> 
>-    my ( $can_renew, $can_renew_error ) =
>+    my ( $can_renew, $can_renew_error, $info ) =
>       CanBookBeRenewed( $c->{borrowernumber}, $c->{itemnumber} );
>     my $can_renew_date =
>       $can_renew_error && $can_renew_error eq 'too_soon'
>       ? output_pref(
>         {
>-            dt => GetSoonestRenewDate( $c->{borrowernumber}, $c->{itemnumber} ),
>+            dt => $info->{soonest_renew_date},
>             as_due_date => 1
>         }
>       )
>diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
>index de4637de9b..6b41de4b2d 100755
>--- a/t/db_dependent/Circulation.t
>+++ b/t/db_dependent/Circulation.t
>@@ -418,7 +418,7 @@ subtest "GetIssuingCharges tests" => sub {
> 
> my ( $reused_itemnumber_1, $reused_itemnumber_2 );
> subtest "CanBookBeRenewed tests" => sub {
>-    plan tests => 93;
>+    plan tests => 100;
> 
>     C4::Context->set_preference('ItemsDeniedRenewal','');
>     # Generate test biblio
>@@ -794,11 +794,13 @@ subtest "CanBookBeRenewed tests" => sub {
>     );
> 
>     $issue = AddIssue( $renewing_borrower, $item_4->barcode, undef, undef, undef, undef, { auto_renew => 1 } );
>-    ( $renewokay, $error ) =
>+    my $info;
>+    ( $renewokay, $error, $info ) =
>       CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>     is( $renewokay, 0, 'Bug 14101: Cannot renew, renewal is automatic and premature' );
>     is( $error, 'auto_too_soon',
>         'Bug 14101: Cannot renew, renewal is automatic and premature, "No renewal before" = undef (returned code is auto_too_soon)' );
>+    is( $info->{soonest_renew_date} , dt_from_string($issue->date_due), "Due date is returned as earliest renewal date when error is 'auto_too_soon'" );
>     AddReserve(
>         {
>             branchcode       => $branch,
>@@ -817,9 +819,10 @@ subtest "CanBookBeRenewed tests" => sub {
>     ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>     is( $renewokay, 0, 'Still should not be able to renew' );
>     is( $error, 'on_reserve', 'returned code is on_reserve, reserve checked when not checking for cron' );
>-    ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber, undef, 1 );
>+    ( $renewokay, $error, $info ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber, undef, 1 );
>     is( $renewokay, 0, 'Still should not be able to renew' );
>     is( $error, 'auto_too_soon', 'returned code is auto_too_soon, reserve not checked when checking for cron' );
>+    is( $info->{soonest_renew_date}, dt_from_string($issue->date_due), "Due date is returned as earliest renewal date when error is 'auto_too_soon'" );
>     ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber, 1 );
>     is( $renewokay, 0, 'Still should not be able to renew' );
>     is( $error, 'on_reserve', 'returned code is on_reserve, auto_too_soon limit is overridden' );
>@@ -852,39 +855,44 @@ subtest "CanBookBeRenewed tests" => sub {
>         }
>     );
> 
>-    ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrowernumber, $item_1->itemnumber);
>+    ( $renewokay, $error, $info ) = CanBookBeRenewed($renewing_borrowernumber, $item_1->itemnumber);
>     is( $renewokay, 0, 'Bug 7413: Cannot renew, renewal is premature');
>     is( $error, 'too_soon', 'Bug 7413: Cannot renew, renewal is premature (returned code is too_soon)');
>+    is( $info->{soonest_renew_date}, dt_from_string($issue->date_due)->subtract( days => 7 ), "Soonest renew date returned when error is 'too_soon'");
> 
>     # Bug 14101
>     # Test premature automatic renewal
>-    ( $renewokay, $error ) =
>+    ( $renewokay, $error, $info ) =
>       CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>     is( $renewokay, 0, 'Bug 14101: Cannot renew, renewal is automatic and premature' );
>     is( $error, 'auto_too_soon',
>         'Bug 14101: Cannot renew, renewal is automatic and premature (returned code is auto_too_soon)'
>     );
>+    is( $info->{soonest_renew_date}, dt_from_string($issue->date_due)->subtract( days => 7 ), "Soonest renew date returned when error is 'auto_too_soon'");
> 
>     $renewing_borrower_obj->autorenew_checkouts(0)->store;
>-    ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>+    ( $renewokay, $error, $info ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>     is( $renewokay, 0, 'No renewal before is 7, patron opted out of auto_renewal still cannot renew early' );
>     is( $error, 'too_soon', 'Error is too_soon, no auto' );
>+    is( $info->{soonest_renew_date}, dt_from_string($issue->date_due)->subtract( days => 7 ), "Soonest renew date returned when error is 'too_soon'");
>     $renewing_borrower_obj->autorenew_checkouts(1)->store;
> 
>     # Change policy so that loans can only be renewed exactly on due date (0 days prior to due date)
>     # and test automatic renewal again
>     $dbh->do(q{UPDATE circulation_rules SET rule_value = '0' WHERE rule_name = 'norenewalbefore'});
>-    ( $renewokay, $error ) =
>+    ( $renewokay, $error, $info ) =
>       CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>     is( $renewokay, 0, 'Bug 14101: Cannot renew, renewal is automatic and premature' );
>     is( $error, 'auto_too_soon',
>         'Bug 14101: Cannot renew, renewal is automatic and premature, "No renewal before" = 0 (returned code is auto_too_soon)'
>     );
>+    is( $info->{soonest_renew_date}, dt_from_string($issue->date_due), "Soonest renew date returned when error is 'auto_too_soon'");
> 
>     $renewing_borrower_obj->autorenew_checkouts(0)->store;
>-    ( $renewokay, $error ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>+    ( $renewokay, $error, $info ) = CanBookBeRenewed( $renewing_borrowernumber, $item_4->itemnumber );
>     is( $renewokay, 0, 'No renewal before is 0, patron opted out of auto_renewal still cannot renew early' );
>     is( $error, 'too_soon', 'Error is too_soon, no auto' );
>+    is( $info->{soonest_renew_date}, dt_from_string($issue->date_due), "Soonest renew date returned when error is 'auto_too_soon'");
>     $renewing_borrower_obj->autorenew_checkouts(1)->store;
> 
>     # Change policy so that loans can be renewed 99 days prior to the due date
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 30167: 131046 | 131047 | 134082 | 134083 | 134197 | 134198 | 134380 | 134454 | 134455 | 134456 | 134576 | 134577 | 134578 | 134579 | 135016