Bug 14101 - auto renewal is tied to 'no renewal before'
Summary: auto renewal is tied to 'no renewal before'
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Holger Meißner
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 14214 14301 (view as bug list)
Depends on:
Blocks: 14395
  Show dependency treegraph
 
Reported: 2015-04-30 18:51 UTC by Nicole C. Engard
Modified: 2016-06-21 21:40 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14101: Automatic renewals exactly on due date (3.62 KB, patch)
2015-06-03 11:14 UTC, Holger Meißner
Details | Diff | Splinter Review
Table to illustrate how automatic renewals should work. (12.38 KB, image/png)
2015-06-03 11:26 UTC, Holger Meißner
Details
Test 1 "no renewals before" (77.59 KB, application/pdf)
2015-06-04 01:41 UTC, Claudio
Details
Bug 14101: Unit tests (4.42 KB, patch)
2015-09-22 12:11 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14101: Automatic renewals exactly on due date (4.07 KB, patch)
2015-10-23 09:04 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14101: Unit tests (4.56 KB, patch)
2015-10-23 09:04 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14101: Automatic renewals exactly on due date (4.07 KB, patch)
2015-11-18 09:29 UTC, Holger Meißner
Details | Diff | Splinter Review
Bug 14101: Unit tests (4.53 KB, patch)
2015-11-18 09:29 UTC, Holger Meißner
Details | Diff | Splinter Review
[PASSED QA] Bug 14101: Automatic renewals exactly on due date (4.14 KB, patch)
2015-11-19 11:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 14101: Unit tests (4.60 KB, patch)
2015-11-19 11:40 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2015-04-30 18:51:06 UTC
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

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.
Comment 1 Katrin Fischer 2015-05-15 22:29:27 UTC
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
Comment 2 Nicole C. Engard 2015-05-18 15:33:47 UTC
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?
Comment 3 Kyle M Hall 2015-05-18 15:34:51 UTC
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!
Comment 4 Katrin Fischer 2015-05-18 17:33:38 UTC
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?
Comment 5 Holger Meißner 2015-05-19 14:35:16 UTC
(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.
Comment 6 Holger Meißner 2015-05-19 14:46:41 UTC
(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" );
        }
    }
Comment 7 Kyle M Hall 2015-05-27 16:55:23 UTC
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" );
>         }
>     }
Comment 8 Katrin Fischer 2015-05-27 21:06:05 UTC
Hi Kyle, what are the loan period and no renewal before values you tested with?
Comment 9 Holger Meißner 2015-05-28 13:34:48 UTC
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?
Comment 10 Holger Meißner 2015-05-28 13:41:26 UTC
(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.
Comment 11 Holger Meißner 2015-06-03 11:14:27 UTC Comment hidden (obsolete)
Comment 12 Holger Meißner 2015-06-03 11:26:16 UTC
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.
Comment 13 Holger Meißner 2015-06-03 11:27:41 UTC
*** Bug 14214 has been marked as a duplicate of this bug. ***
Comment 14 Holger Meißner 2015-06-03 11:30:25 UTC
*** Bug 14301 has been marked as a duplicate of this bug. ***
Comment 15 Claudio 2015-06-04 01:39:57 UTC
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
Comment 16 Claudio 2015-06-04 01:41:06 UTC
Created attachment 39829 [details]
Test 1 "no renewals before"
Comment 17 Holger Meißner 2015-06-10 09:13:14 UTC
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?
Comment 18 Claudio 2015-06-10 13:59:37 UTC
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
Comment 19 Holger Meißner 2015-06-16 08:04:07 UTC
Okay. I'll work on it. I think it should be sysprefed and in a separate patch, depending on this one. See bug 14395.
Comment 20 Claudio 2015-06-16 12:53:42 UTC
(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
Comment 21 Jonathan Druart 2015-08-19 11:52:40 UTC
Holger, please provide tests for the changes done to CanBookBeRenewed.
Comment 22 Holger Meißner 2015-09-22 12:11:44 UTC Comment hidden (obsolete)
Comment 23 Holger Meißner 2015-09-22 12:15:54 UTC
Hello Jonathan, unit tests are now provided. Setting back to signed off.
Comment 24 Jonathan Druart 2015-10-01 10:47:00 UTC
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.
Comment 25 Holger Meißner 2015-10-23 09:04:30 UTC Comment hidden (obsolete)
Comment 26 Holger Meißner 2015-10-23 09:04:33 UTC Comment hidden (obsolete)
Comment 27 Holger Meißner 2015-10-23 09:09:23 UTC
Hi Jonathan, thanks for QA! I think I fixed everything :)
Comment 28 Holger Meißner 2015-11-18 09:29:49 UTC Comment hidden (obsolete)
Comment 29 Holger Meißner 2015-11-18 09:29:53 UTC Comment hidden (obsolete)
Comment 30 Holger Meißner 2015-11-18 09:32:28 UTC
Rebased on master.
Comment 31 Kyle M Hall 2015-11-19 11:40:27 UTC
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>
Comment 32 Kyle M Hall 2015-11-19 11:40:35 UTC
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>
Comment 33 Tomás Cohen Arazi 2015-11-20 13:19:43 UTC
Patch pushed to master.

Thanks Holger!