Bug 24083: (follow-up) Squashed follow ups

This squashed commit fixes:
-  a small error in the checkouts related JS

- GetRenewCount now returns 6 values when a call to it succeeds, a
failed call should also return the same number of values. This commit
adds these additional values.

- Some changes in issue.t had broken the tests for unseen renewals (the
unseen tests were using variables that had been moved out of the tests'
scope).

- Also now using Koha::CirculationRules::set_rules to set circ rules
rather than using SQL queries.

- Fixed expected number of return values from GetRenewCount

- Moved unseen tests in issue.t to the bottom of the file to remove the
risk of interference with other test circ rules.

- There was a real mess in C4/Circulation.pm due to a bad rebase back in
February. Frankly it's a wonder anything worked at all. This commit
fixes that problem and reinstates the correct patch for
C4/Circulation.pm

- Somehow I'd never noticed this before but the columns in smart-rules.tt
were misaligned when UnseenRenewals was turned off. This was due to the
display of a <td> not being conditional when it should have been. This
is now fixed.

- This commit also fixes items 1 & 2 descibed by Katrin in comment #74 ->
comment #76.

- Fixed missing check for too_unseen in opac-user.tt, this test did used
to exist but got lost during sizeable rebase a few weeks ago :-(
- Added test for too_unseen to all AUTO_RENEWAL notice templates apart
from de-DE (as previously requested by Katrin)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

C4/Circulation.pm

@@ -2781,6 +2781,11 @@ sub CanBookBeRenewed {
         return ( 0, "too_many" )
           if not $issuing_rule->{renewalsallowed} or $issuing_rule->{renewalsallowed} <= $issue->renewals;
 
+        return ( 0, "too_unseen" )
+          if C4::Context->preference('UnseenRenewals') &&
+            $issuing_rule->{unseen_renewals_allowed} &&
+            $issuing_rule->{unseen_renewals_allowed} <= $issue->unseen_renewals;
+
         my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing');
         my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing');
         $patron         = Koha::Patrons->find($borrowernumber); # FIXME Is this really useful?
@@ -2952,118 +2957,6 @@ sub CanBookBeRenewed {
 
     return ( 0, "auto_renew" ) if $auto_renew eq "ok" && !$override_limit; # 0 if auto-renewal should not succeed
 
-    return ( 1, undef ) if $override_limit;
-
-    my $branchcode = _GetCircControlBranch( $item->unblessed, $patron->unblessed );
-    my $issuing_rule = Koha::CirculationRules->get_effective_rules(
-        {
-            categorycode => $patron->categorycode,
-            itemtype     => $item->effective_itemtype,
-            branchcode   => $branchcode,
-            rules => [
-                'renewalsallowed',
-                'no_auto_renewal_after',
-                'no_auto_renewal_after_hard_limit',
-                'lengthunit',
-                'norenewalbefore',
-                'unseen_renewals_allowed'
-            ]
-        }
-    );
-
-    return ( 0, "too_many" )
-      if not $issuing_rule->{renewalsallowed} or $issuing_rule->{renewalsallowed} <= $issue->renewals;
-
-    return ( 0, "too_unseen" )
-      if C4::Context->preference('UnseenRenewals') &&
-        $issuing_rule->{unseen_renewals_allowed} &&
-        $issuing_rule->{unseen_renewals_allowed} <= $issue->unseen_renewals;
-
-    my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing');
-    my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing');
-    $patron         = Koha::Patrons->find($borrowernumber); # FIXME Is this really useful?
-    my $restricted  = $patron->is_debarred;
-    my $hasoverdues = $patron->has_overdues;
-
-    if ( $restricted and $restrictionblockrenewing ) {
-        return ( 0, 'restriction');
-    } elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($issue->is_overdue and $overduesblockrenewing eq 'blockitem') ) {
-        return ( 0, 'overdue');
-    }
-
-    if ( $issue->auto_renew ) {
-
-        if ( $patron->category->effective_BlockExpiredPatronOpacActions and $patron->is_expired ) {
-            return ( 0, 'auto_account_expired' );
-        }
-
-        if ( defined $issuing_rule->{no_auto_renewal_after}
-                and $issuing_rule->{no_auto_renewal_after} ne "" ) {
-            # Get issue_date and add no_auto_renewal_after
-            # If this is greater than today, it's too late for renewal.
-            my $maximum_renewal_date = dt_from_string($issue->issuedate, 'sql');
-            $maximum_renewal_date->add(
-                $issuing_rule->{lengthunit} => $issuing_rule->{no_auto_renewal_after}
-            );
-            my $now = dt_from_string;
-            if ( $now >= $maximum_renewal_date ) {
-                return ( 0, "auto_too_late" );
-            }
-        }
-        if ( defined $issuing_rule->{no_auto_renewal_after_hard_limit}
-                      and $issuing_rule->{no_auto_renewal_after_hard_limit} ne "" ) {
-            # If no_auto_renewal_after_hard_limit is >= today, it's also too late for renewal
-            if ( dt_from_string >= dt_from_string( $issuing_rule->{no_auto_renewal_after_hard_limit} ) ) {
-                return ( 0, "auto_too_late" );
-            }
-        }
-
-        if ( C4::Context->preference('OPACFineNoRenewalsBlockAutoRenew') ) {
-            my $fine_no_renewals = C4::Context->preference("OPACFineNoRenewals");
-            my $amountoutstanding =
-              C4::Context->preference("OPACFineNoRenewalsIncludeCredit")
-              ? $patron->account->balance
-              : $patron->account->outstanding_debits->total_outstanding;
-            if ( $amountoutstanding and $amountoutstanding > $fine_no_renewals ) {
-                return ( 0, "auto_too_much_oweing" );
-            }
-        }
-    }
-
-    if ( defined $issuing_rule->{norenewalbefore}
-        and $issuing_rule->{norenewalbefore} ne "" )
-    {
-
-        # Calculate soonest renewal by subtracting 'No renewal before' from due date
-        my $soonestrenewal = dt_from_string( $issue->date_due, 'sql' )->subtract(
-            $issuing_rule->{lengthunit} => $issuing_rule->{norenewalbefore} );
-
-        # Depending on syspref reset the exact time, only check the date
-        if ( C4::Context->preference('NoRenewalBeforePrecision') eq 'date'
-            and $issuing_rule->{lengthunit} eq 'days' )
-        {
-            $soonestrenewal->truncate( to => 'day' );
-        }
-
-        if ( $soonestrenewal > dt_from_string() )
-        {
-            return ( 0, "auto_too_soon" ) if $issue->auto_renew;
-            return ( 0, "too_soon" );
-        }
-        elsif ( $issue->auto_renew ) {
-            return ( 0, "auto_renew" );
-        }
-    }
-
-    # Fallback for automatic renewals:
-    # If norenewalbefore is undef, don't renew before due date.
-    if ( $issue->auto_renew ) {
-        my $now = dt_from_string;
-        return ( 0, "auto_renew" )
-          if $now >= dt_from_string( $issue->date_due, 'sql' );
-        return ( 0, "auto_too_soon" );
-    }
-
     return ( 1, undef );
 }
 
@@ -3287,7 +3180,7 @@ sub GetRenewCount {
     my $patron = Koha::Patrons->find( $bornum );
     my $item   = Koha::Items->find($itemno);
 
-    return (0, 0, 0) unless $patron or $item; # Wrong call, no renewal allowed
+    return (0, 0, 0, 0, 0, 0) unless $patron or $item; # Wrong call, no renewal allowed
 
     # Look in the issues table for this item, lent to this borrower,
     # and not yet returned.

installer/data/mysql/en/mandatory/sample_notices.yml

@@ -1436,6 +1436,8 @@ tables:
             - "It's too late to renew this item."
             - "[% ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' %]"
             - "Your total unpaid fines are too high."
+            - "[% ELSIF checkout.auto_renew_error == 'too_unseen' %]"
+            - "This item must be renewed at the library."
             - "[% END %]"
             - "[% ELSE %]"
             - "The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]"

installer/data/mysql/fr-CA/obligatoire/sample_notices.sql

@@ -285,6 +285,8 @@ You have overdue items.
 It\'s too late to renew this item.
 [% ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' %]
 Your total unpaid fines are too high.
+[% ELSIF checkout.auto_renew_error == 'too_unseen' %]
+This item must be renewed at the library.
 [% END %]
 [% ELSE %]
 The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]

installer/data/mysql/fr-FR/1-Obligatoire/sample_notices.sql

@@ -396,6 +396,8 @@ You have overdue items.
 It\'s too late to renew this item.
 [% ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' %]
 Your total unpaid fines are too high.
+[% ELSIF checkout.auto_renew_error == 'too_unseen' %]
+This item must be renewed at the library.
 [% END %]
 [% ELSE %]
 The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]

installer/data/mysql/it-IT/necessari/notices.sql

@@ -399,6 +399,8 @@ You have overdue items.
 It\'s too late to renew this item.
 [% ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' %]
 Your total unpaid fines are too high.
+[% ELSIF checkout.auto_renew_error == 'too_unseen' %]
+This item must be renewed at the library.
 [% END %]
 [% ELSE %]
 The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]

installer/data/mysql/nb-NO/1-Obligatorisk/sample_notices.sql

@@ -414,6 +414,8 @@ You have overdue items.
 It\'s too late to renew this item.
 [% ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' %]
 Your total unpaid fines are too high.
+[% ELSIF checkout.auto_renew_error == 'too_unseen' %]
+This item must be renewed at the library.
 [% END %]
 [% ELSE %]
 The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]

installer/data/mysql/pl-PL/mandatory/sample_notices.sql

@@ -393,6 +393,8 @@ You have overdue items.
 It\'s too late to renew this item.
 [% ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' %]
 Your total unpaid fines are too high.
+[% ELSIF checkout.auto_renew_error == 'too_unseen' %]
+This item must be renewed at the library.
 [% END %]
 [% ELSE %]
 The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]

installer/data/mysql/ru-RU/mandatory/sample_notices.sql

@@ -395,6 +395,8 @@ You have overdue items.
 It\'s too late to renew this item.
 [% ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' %]
 Your total unpaid fines are too high.
+[% ELSIF checkout.auto_renew_error == 'too_unseen' %]
+This item must be renewed at the library.
 [% END %]
 [% ELSE %]
 The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]

installer/data/mysql/uk-UA/mandatory/sample_notices.sql

@@ -486,6 +486,8 @@ You have overdue items.
 It\'s too late to renew this item.
 [% ELSIF checkout.auto_renew_error == 'auto_too_much_oweing' %]
 Your total unpaid fines are too high.
+[% ELSIF checkout.auto_renew_error == 'too_unseen' %]
+This item must be renewed at the library.
 [% END %]
 [% ELSE %]
 The following item, [% biblio.title %], has correctly been renewed and is now due on [% checkout.date_due | $KohaDates as_due_date => 1 %]

koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt

@@ -417,7 +417,9 @@
                     <td><input type="text" name="maxsuspensiondays" id="maxsuspensiondays" size="3" /> </td>
                     <td><input type="text" name="suspension_chargeperiod" id="suspension_chargeperiod" size="3" /> </td>
                     <td><input type="text" name="renewalsallowed" id="renewalsallowed" size="2" /></td>
-                    <td><input type="text" name="unseen_renewals_allowed" id="unseen_renewals_allowed" size="2" /></td>
+                    [% IF Koha.Preference('UnseenRenewals') %]
+                        <td><input type="text" name="unseen_renewals_allowed" id="unseen_renewals_allowed" size="2" /></td>
+                    [% END %]
                     <td><input type="text" name="renewalperiod" id="renewalperiod" size="3" /></td>
                     <td><input type="text" name="norenewalbefore" id="norenewalbefore" size="3" /></td>
                     <td>

koha-tmpl/intranet-tmpl/prog/js/checkouts.js

@@ -559,13 +559,11 @@ $(document).ready(function() {
                             content += "<span class='renewals'>(";
                             content + __("%s of %s renewals remaining").format(oObj.renewals_remaining, oObj.renewals_allowed);
                             if (UnseenRenewals && oObj.unseen_allowed) {
-                                content += __(" / %s of %s unseen renewals remaining").format(oObj.unseen_remaining, oObj.unseen_allowed);
+                                content += __("%s of %s unseen renewals remaining").format(oObj.unseen_remaining, oObj.unseen_allowed);
                             }
-                                    + ")</span>";
+                            content += ")</span>";
                         }
 
-                        content += "</span>";
-
                         return content;
                     }
                 },

koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt

@@ -378,6 +378,8 @@
                                                             Not renewable <span class="renewals">(on hold)</span>
                                                         [% ELSIF ( ISSUE.too_many ) %]
                                                             Not renewable
+                                                        [% ELSIF ( ISSUE.too_unseen ) %]
+                                                            Item must be renewed at the library. [% ISSUE.renewsleft | html %] renewals remaining
                                                         [% ELSIF ( ISSUE.norenew_overdue ) %]
                                                             Not allowed <span class="renewals">(overdue)</span>
                                                         [% ELSIF ( ISSUE.auto_too_late ) %]

t/db_dependent/Circulation.t

@@ -284,7 +284,7 @@ Koha::CirculationRules->set_rules(
 
 my ( $reused_itemnumber_1, $reused_itemnumber_2 );
 subtest "CanBookBeRenewed tests" => sub {
-    plan tests => 87;
+    plan tests => 89;
 
     C4::Context->set_preference('ItemsDeniedRenewal','');
     # Generate test biblio
@@ -1164,12 +1164,34 @@ subtest "CanBookBeRenewed tests" => sub {
     is( $error, 'too_many', 'Cannot renew, 0 renewals allowed (returned code is too_many)');
 
     # Too many unseen renewals
-    $dbh->do('UPDATE issuingrules SET unseen_renewals_allowed = 2, renewalsallowed = 10');
+    Koha::CirculationRules->set_rules(
+        {
+            categorycode => undef,
+            branchcode   => undef,
+            itemtype     => undef,
+            rules        => {
+                unseen_renewals_allowed => 2,
+                renewalsallowed => 10,
+            }
+        }
+    );
+    t::lib::Mocks::mock_preference('UnseenRenewals', 1);
     $dbh->do('UPDATE issues SET unseen_renewals = 2 where borrowernumber = ? AND itemnumber = ?', undef, ($renewing_borrowernumber, $item_1->itemnumber));
     ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrowernumber, $item_1->itemnumber);
     is( $renewokay, 0, 'Cannot renew, 0 unseen renewals allowed');
     is( $error, 'too_unseen', 'Cannot renew, returned code is too_unseen');
-    $dbh->do('UPDATE issuingrules SET norenewalbefore = NULL, renewalsallowed = 0');
+    Koha::CirculationRules->set_rules(
+        {
+            categorycode => undef,
+            branchcode   => undef,
+            itemtype     => undef,
+            rules        => {
+                norenewalbefore => undef,
+                renewalsallowed => 0,
+            }
+        }
+    );
+    t::lib::Mocks::mock_preference('UnseenRenewals', 0);
 
     # Test WhenLostForgiveFine and WhenLostChargeReplacementFee
     t::lib::Mocks::mock_preference('WhenLostForgiveFine','1');

t/db_dependent/Circulation/issue.t

@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 46;
+use Test::More tests => 48;
 use DateTime::Duration;
 
 use t::lib::Mocks;
@@ -297,20 +297,6 @@ subtest 'Show that AddRenewal respects OpacRenewalBranch and interface' => sub {
     }
 };
 
-# Unseen rewnewals
-t::lib::Mocks::mock_preference('UnseenRenewals', 1);
-
-# Does an unseen renewal increment the issue's count
-my ( $unseen_before ) = ( C4::Circulation::GetRenewCount( $opac_renew_issue->{borrowernumber}, $opac_renew_issue->{itemnumber} ) )[3];
-AddRenewal( $opac_renew_issue->{borrowernumber}, $opac_renew_issue->{itemnumber}, $branchcode_1, undef, undef, 0 );
-my ( $unseen_after ) = ( C4::Circulation::GetRenewCount( $opac_renew_issue->{borrowernumber}, $opac_renew_issue->{itemnumber} ) )[3];
-is( $unseen_after, $unseen_before + 1, 'unseen_renewals increments' );
-
-# Does a seen renewal reset the unseen count
-AddRenewal( $opac_renew_issue->{borrowernumber}, $opac_renew_issue->{itemnumber}, $branchcode_1, undef, undef, 1 );
-my ( $unseen_reset ) = ( C4::Circulation::GetRenewCount( $opac_renew_issue->{borrowernumber}, $opac_renew_issue->{itemnumber} ) )[3];
-is( $unseen_reset, 0, 'seen renewal resets the unseen count' );
-
 #Test GetBiblioIssues
 is( GetBiblioIssues(), undef, "GetBiblio Issues without parameters" );
 
@@ -327,19 +313,19 @@ my $issue3 = C4::Circulation::AddIssue( $borrower_1, $barcode_1 );
 @renewcount = C4::Circulation::GetRenewCount();
 is_deeply(
     \@renewcount,
-    [ 0, 0, 0 ], # FIXME Need to be fixed, see FIXME in GetRenewCount
+    [ 0, 0, 0, 0, 0, 0 ], # FIXME Need to be fixed, see FIXME in GetRenewCount
     "Without issuing rules and without parameter, GetRenewCount returns renewcount = 0, renewsallowed = undef, renewsleft = 0"
 );
 @renewcount = C4::Circulation::GetRenewCount(-1);
 is_deeply(
     \@renewcount,
-    [ 0, 0, 0 ], # FIXME Need to be fixed
+    [ 0, 0, 0, 0, 0, 0 ], # FIXME Need to be fixed
     "Without issuing rules and without wrong parameter, GetRenewCount returns renewcount = 0, renewsallowed = undef, renewsleft = 0"
 );
 @renewcount = C4::Circulation::GetRenewCount($borrower_id1, $item_id1);
 is_deeply(
     \@renewcount,
-    [ 2, 0, 0 ],
+    [ 2, 0, 0, 0, 0, 0 ],
     "Without issuing rules and with a valid parameter, renewcount = 2, renewsallowed = undef, renewsleft = 0"
 );
 
@@ -347,19 +333,19 @@ is_deeply(
 @renewcount = C4::Circulation::GetRenewCount();
 is_deeply(
     \@renewcount,
-    [ 0, 0, 0 ],
+    [ 0, 0, 0, 0, 0, 0 ],
     "With issuing rules (renewal disallowed) and without parameter, GetRenewCount returns renewcount = 0, renewsallowed = 0, renewsleft = 0"
 );
 @renewcount = C4::Circulation::GetRenewCount(-1);
 is_deeply(
     \@renewcount,
-    [ 0, 0, 0 ],
+    [ 0, 0, 0, 0, 0, 0 ],
     "With issuing rules (renewal disallowed) and without wrong parameter, GetRenewCount returns renewcount = 0, renewsallowed = 0, renewsleft = 0"
 );
 @renewcount = C4::Circulation::GetRenewCount($borrower_id1, $item_id1);
 is_deeply(
     \@renewcount,
-    [ 2, 0, 0 ],
+    [ 2, 0, 0, 0, 0, 0 ],
     "With issuing rules (renewal disallowed) and with a valid parameter, Getrenewcount returns renewcount = 2, renewsallowed = 0, renewsleft = 0"
 );
 
@@ -377,7 +363,7 @@ Koha::CirculationRules->set_rules(
 @renewcount = C4::Circulation::GetRenewCount($borrower_id1, $item_id1);
 is_deeply(
     \@renewcount,
-    [ 2, 3, 1 ],
+    [ 2, 3, 1, 0, 0, 0 ],
     "With issuing rules (renewal allowed) and with a valid parameter, Getrenewcount of item1 returns 3 renews left"
 );
 
@@ -386,7 +372,7 @@ AddRenewal( $borrower_id1, $item_id1, $branchcode_1,
 @renewcount = C4::Circulation::GetRenewCount($borrower_id1, $item_id1);
 is_deeply(
     \@renewcount,
-    [ 3, 3, 0 ],
+    [ 3, 3, 0, 0, 0, 0 ],
     "With issuing rules (renewal allowed, 1 remaining) and with a valid parameter, Getrenewcount of item1 returns 0 renews left"
 );
 
@@ -504,5 +490,37 @@ AddIssue( $borrower_2, $barcode_1, dt_from_string, 'cancel' );
 my $hold = Koha::Holds->find( $reserve_id );
 is( $hold, undef, 'The reserve should have been correctly cancelled' );
 
+# Unseen rewnewals
+t::lib::Mocks::mock_preference('UnseenRenewals', 1);
+# Add a default circ rule: 3 unseen renewals allowed
+Koha::CirculationRules->set_rules(
+    {
+        categorycode => undef,
+        itemtype     => undef,
+        branchcode   => undef,
+        rules        => {
+            renewalsallowed => 10,
+            unseen_renewals_allowed => 3
+        }
+    }
+);
+
+my $unseen_library = $builder->build_object( { class => 'Koha::Libraries' } );
+my $unseen_patron  = $builder->build_object( { class => 'Koha::Patrons' } );
+my $unseen_item = $builder->build_sample_item(
+    { library => $unseen_library->branchcode, itype => $itemtype } );
+my $unseen_issue = C4::Circulation::AddIssue( $unseen_patron->unblessed, $unseen_item->barcode );
+
+# Does an unseen renewal increment the issue's count
+my ( $unseen_before ) = ( C4::Circulation::GetRenewCount( $unseen_patron->borrowernumber, $unseen_item->itemnumber ) )[3];
+AddRenewal( $unseen_patron->borrowernumber, $unseen_item->itemnumber, $branchcode_1, undef, undef, undef, 0 );
+my ( $unseen_after ) = ( C4::Circulation::GetRenewCount( $unseen_patron->borrowernumber, $unseen_item->itemnumber ) )[3];
+is( $unseen_after, $unseen_before + 1, 'unseen_renewals increments' );
+
+# Does a seen renewal reset the unseen count
+AddRenewal( $unseen_patron->borrowernumber, $unseen_item->itemnumber, $branchcode_1, undef, undef, undef, 1 );
+my ( $unseen_reset ) = ( C4::Circulation::GetRenewCount( $unseen_patron->borrowernumber, $unseen_item->itemnumber ) )[3];
+is( $unseen_reset, 0, 'seen renewal resets the unseen count' );
+
 #End transaction
 $schema->storage->txn_rollback;