Hi Nicole,

I think this is how it's intended to work and not sure how it could be changed to work differently. See http://wiki.koha-community.org/wiki/Automatic_renewal_RFC

I'm a bit confused then on how it works if I check the auto renew box when I check out - in that scenario you're overriding the default and there is no way to set 'no renewal before' in that scenario ... make sense?

Maybe the documentation on this feature is wrong, but when I've tried to set up automatica renewals I have a serious issue. 

I enable the nightly cron and all my automatic renewals are renewed every night! So if I set up automatic renewals on an item with 3 renewals and a renewal period of 7 days, I find that the item is renewed on the 1st night, on the 2nd night, and on the 3rd night. At that point it's out of renewals!

This seems to happen even if I set a no renewal before value.

Katrin, have you tried the feature? I'd love to get confirmation or clarification on how this should work!

I think it should take the 'no renewal before' into account - I am not sure why it wouldn't. I haven't tested this myself, but I have added the developer in CC.

Maybe the feature could fall back to try a renewal on the due date if there is nothing else specified in the circulation rules?

(In reply to Nicole C. Engard from comment #0)
> It seems as though the autorenewal feature needs to have the column "no
> renewal before" filled in or it will auto renew everyday after the duedate

Hello Nicole. Yes, this is intentional. The "no renewal before" setting is the only way for the cronjob to know whether it is too soon for renewal or not. So without it, it will just renew every time it runs. Unless another restriction is met.

> I'm not sure (haven't had time to test) how this effects auto renewals that
> happen from the check box on the check out screen.

There is a flag in the database called issues.auto_renew. If automatic renewal is scheduled by issing rule, this flag will be set for all issues where the rule applies. By checkbox the same flag is set, just for one issue. It makes no difference for the automatic renewal cronjob. It just tries to renew every flagged issue and uses the "no renewal before" settig from the issuing rule that applies.

(In reply to Kyle M Hall from comment #3)
> This seems to happen even if I set a no renewal before value.

Hi Kyle, which values did you test? It should work with values equal to or greater than 1. Maybe the feature broke. I am going to re-test, but this is how it used to work. The cronjob calls CanBookBeRenewed. There "no renewal before" is calculated like this:

    if ( $issuingrule->{norenewalbefore} ) {

        # Get current time and add norenewalbefore.
        # If this is smaller than date_due, it's too soon for renewal.
        if (
            DateTime->now( time_zone => C4::Context->tz() )->add(
                $issuingrule->{lengthunit} => $issuingrule->{norenewalbefore}
            ) < $itemissue->{date_due}
          )
        {
            return ( 0, "auto_too_soon" ) if $itemissue->{auto_renew};
            return ( 0, "too_soon" );
        }
    }

Right. My current testing on 3.18 makes me think the entire feature is broken ( or needs more documentation because I don't know how to set it up ).

I set up a test and included a no renewal before value and the nightly cron still renewed all my items each night!

(In reply to Holger Meißner from comment #6)
> (In reply to Kyle M Hall from comment #3)
> > This seems to happen even if I set a no renewal before value.
> 
> Hi Kyle, which values did you test? It should work with values equal to or
> greater than 1. Maybe the feature broke. I am going to re-test, but this is
> how it used to work. The cronjob calls CanBookBeRenewed. There "no renewal
> before" is calculated like this:
> 
>     if ( $issuingrule->{norenewalbefore} ) {
> 
>         # Get current time and add norenewalbefore.
>         # If this is smaller than date_due, it's too soon for renewal.
>         if (
>             DateTime->now( time_zone => C4::Context->tz() )->add(
>                 $issuingrule->{lengthunit} => $issuingrule->{norenewalbefore}
>             ) < $itemissue->{date_due}
>           )
>         {
>             return ( 0, "auto_too_soon" ) if $itemissue->{auto_renew};
>             return ( 0, "too_soon" );
>         }
>     }

Hi Kyle, what are the loan period and no renewal before values you tested with?

I am unable to reproduce this problem. We already use the feature in 3.18.

Loan period: 30 days
Renewal period: 20 days
No renewal before: 11 days
Renewals allowed (count): 16

I checked out some items on 29/04/2015, the cronjob is running nightly. So far it renewed only once. 15 renewals are still left.

Today I also tested in 3.21 with hour-based issues.

Loan period: 2 hours
Renewal period: 2 hours
No renewal before: 1 hour
Renewals allowed (count): 3

I ran automatic_renewals.pl multiple times, before an hour had passed and after. It renewed just once, an hour after check out.

Kyle, could there be some problem with DateTime->now() in your setup? Does it give you the correct time?

(In reply to Katrin Fischer from comment #4) 
> Maybe the feature could fall back to try a renewal on the due date if there
> is nothing else specified in the circulation rules?
I think that's a great idea! I'll implement that.

Created attachment 39802 [details]
Table to illustrate how automatic renewals should work.

To make testing easier and for documentation purposes I include a table about how the feature is supposed to work. Please let me know if you think the behavior should be altered.

Note: I can't think of a use case, but negative values for 'No renewal before' work.

*** Bug 14214 has been marked as a duplicate of this bug. ***

*** Bug 14301 has been marked as a duplicate of this bug. ***

Holger,

I've upload a PDF with a case I'd like you to consider.

I've made a test as you suggested:

1) Now is possible to set "no renewals before" to 0. It works fine.
2) Calculation of days to disable renewals is OK.
3) But Koha fails when it must to enable renewal possibility

Created attachment 39829 [details]
Test 1 "no renewals before"

Bondiurbano,

the calculation uses the exact DateTime. When the unit for the loan period is days, as far as I know koha always sets the time for the due date to 23:59.

So with due date 04/06/2015 23:59 and 'No renewal before' 1, renewal will be possible 03/06/2015 23:59. In your Example it is 20:59, three hours too early.

I admit that this is not intuitive, but this way the calculation works for hours as well as days.

Maybe I should develop two different formulas, based on unit?

Hi Holger!

Ah! I understand.

In my opinion "no renewal before" has no sense in hours. But it's only my point of view. Not because it is not needed by me, but I can't imagine a situation like this.

As you suggest, I think that the best way is separating the calculation in hours and days

Okay. I'll work on it. I think it should be sysprefed and in a separate patch, depending on this one. See bug 14395.

(In reply to Holger Meißner from comment #19)
> Okay. I'll work on it. I think it should be sysprefed and in a separate
> patch, depending on this one. See bug 14395.

-------------
Good idea Holger! I agree. So, as a summing up for me:

1) In this bug you have enabled 0 (zero) value for "no renewals before" parameter into the issuing rule
2) And bug 14395 will separate (sysprefering) "no renewals before" into days and hours

Holger, please provide tests for the changes done to CanBookBeRenewed.

Hello Jonathan, unit tests are now provided. Setting back to signed off.

Comment on attachment 39800 [details] [review]
Bug 14101: Automatic renewals exactly on due date

Review of attachment 39800 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +2737,4 @@
>      return ( 0, "too_many" )
>        if $issuingrule->{renewalsallowed} <= $itemissue->{renewals};
>  
> +    if ( defined $issuingrule->{norenewalbefore} ) {

What about the empty string? I am not sure it's relevant, but I would like to be sure.

@@ +2751,5 @@
>              return ( 0, "too_soon" );
>          }
> +        else {
> +            return ( 0, "auto_renew" ) if $itemissue->{auto_renew};
> +            return ( 1, undef );

This last return is not needed, the process should continue I think.
Imagine someone else adds a new check in the futur.

@@ +2759,5 @@
> +    # Fallback for automatic renewals:
> +    # If norenewalbefore is undef, don't renew before due date.
> +    elsif ( $itemissue->{auto_renew} ) {
> +        return ( 0, "auto_renew" )
> +          if DateTime->now( time_zone => C4::Context->tz() ) >=

Prefer dt_from_string to get a DateTime object.

Hi Jonathan, thanks for QA! I think I fixed everything :)

Rebased on master.

Created attachment 44990 [details] [review]
[PASSED QA] Bug 14101: Automatic renewals exactly on due date

If no value for 'no renewal before' is specified, automatic renewal now
falls back on the due date. Also 'no renewal before' can now be zero, so
both automatic and manual renewals can be delayed until due date.

Test plan:

1) Create some circulation rules with different settings for 'No renewal
   before' and 'Automatic renewal'. Both daily and hourly loans should
   work.

2) Try to renew both manually and automatically before and after a renewal
   should be possible. You can run misc/cronjobs/automatic_renewals.pl for
   automatic renewal.

3) Confirm that:
   * Both automatic and manual renewal with 'No renewal before' set
     to 0 do not happen before the due date (exact DateTime).
   * Manual renewal with 'No renewal before' set to undef (enter empty
     string) is unrestricted.
   * Automatic renewal with 'No renewal before' set to undef does not
     happen before the due date.

Sponsored-by: Hochschule für Gesundheit (hsg), Germany

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 44991 [details] [review]
[PASSED QA] Bug 14101: Unit tests

This patch adds new test cases to check if CanBookBeRenewed provides
correct return values and error codes for premature renewals. Both manual
and automatic renewals and different settings for 'No renewal before'
are tested.

To test:
1) prove t/db_dependent/Circulation.t

Sponsored-by: Hochschule für Gesundheit (hsg), Germany

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Patch pushed to master.

Thanks Holger!