From 2f39ea1528d06184c0f08b14cc4c59600b03069c Mon Sep 17 00:00:00 2001 From: Nick Clemens 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 Signed-off-by: Marcel de Rooy 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 Signed-off-by: Marcel de Rooy Bug 30167: (follow-up) Return a hash with soonest_renew_date Signed-off-by: Marcel de Rooy 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 %] ) [% ELSIF ( ISSUE.too_soon ) %] - No renewal before [% ISSUE.soonestrenewdate | html %] + No renewal before [% ISSUE.soonestrenewdate | $KohaDates as_due_date => 1 %] ( [% 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