Bug 15581 - Add a circ rule to not allow auto-renewals after defined loan period
Summary: Add a circ rule to not allow auto-renewals after defined loan period
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 15568 15570 17517
Blocks: 16344
  Show dependency treegraph
 
Reported: 2016-01-15 11:09 UTC by Jonathan Druart
Modified: 2017-12-07 22:21 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
In the circulation conditions there is a new value "no automatic renewal after". This means that "after you have checked out the item for more than X days, there will be no more automatic renewals".
Version(s) released in:
Circulation function:


Attachments
Bug 15581: Add a circ rule to limit the auto renewals given a delay (11.54 KB, patch)
2016-01-15 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the latest auto renew date possible when renewing manually (8.05 KB, patch)
2016-01-15 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: [DO NOT PUSH] Schema changes (1.60 KB, patch)
2016-01-15 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Add a circ rule to limit the auto renewals given a delay (11.38 KB, patch)
2016-04-18 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the latest auto renew date possible when renewing manually (8.05 KB, patch)
2016-04-18 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: [DO NOT PUSH] Schema changes (1.60 KB, patch)
2016-04-18 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table (2.19 KB, patch)
2016-04-18 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Add a circ rule to limit the auto renewals given a delay (12.37 KB, patch)
2016-04-25 21:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the latest auto renew date possible when renewing manually (8.05 KB, patch)
2016-04-25 21:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: [DO NOT PUSH] Schema changes (1.60 KB, patch)
2016-04-25 21:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table (2.19 KB, patch)
2016-04-25 21:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Add a circ rule to limit the auto renewals given a delay (12.45 KB, patch)
2016-04-26 18:06 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 15581: Display the latest auto renew date possible when renewing manually (8.13 KB, patch)
2016-04-26 18:06 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 15581: [DO NOT PUSH] Schema changes (1.67 KB, patch)
2016-04-26 18:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table (2.27 KB, patch)
2016-04-26 18:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 15581: Add a circ rule to limit the auto renewals given a delay (12.46 KB, patch)
2016-07-15 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the latest auto renew date possible when renewing manually (8.12 KB, patch)
2016-07-15 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: [DO NOT PUSH] Schema changes (1.66 KB, patch)
2016-07-15 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table (2.24 KB, patch)
2016-07-15 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Add a circ rule to limit the auto renewals given a delay (12.21 KB, patch)
2016-09-12 15:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the latest auto renew date possible when renewing manually (8.12 KB, patch)
2016-09-12 15:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table (2.24 KB, patch)
2016-09-12 15:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: [DO NOT PUSH] Schema changes (1.67 KB, patch)
2016-09-12 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Add a circ rule to limit the auto renewals given a delay (12.37 KB, patch)
2016-10-24 12:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the latest auto renew date possible when renewing manually (8.12 KB, patch)
2016-10-24 12:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table (2.24 KB, patch)
2016-10-24 12:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: [DO NOT PUSH] Schema changes (1.67 KB, patch)
2016-10-24 12:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Add a circ rule to limit the auto renewals given a delay (12.42 KB, patch)
2016-10-28 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the latest auto renew date possible when renewing manually (8.09 KB, patch)
2016-10-28 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table (2.63 KB, patch)
2016-10-28 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Change wording for auto too late (1.19 KB, patch)
2016-10-28 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15581: Change wording for auto too late (1.19 KB, patch)
2016-10-30 20:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 15581: Add a circ rule to limit the auto renewals given a delay (12.63 KB, patch)
2016-11-01 14:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15581: Display the latest auto renew date possible when renewing manually (8.02 KB, patch)
2016-11-01 14:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table (2.31 KB, patch)
2016-11-01 14:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15581: Change wording for auto too late (1.26 KB, patch)
2016-11-01 14:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15581: Follow-up - suggested change to the OPAC message (2.29 KB, patch)
2016-11-01 14:57 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-01-15 11:09:02 UTC
A new column issuingrules.no_auto_renewal_after will allow to restrict the auto renewals based on a delay.
For instance, if the issue date is 10 days before today, and no_auto_renewal_after is set to 10, tomorrow the issue won't be auto renewed.
Comment 1 Jonathan Druart 2016-01-15 12:53:53 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-01-15 12:53:57 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-01-15 12:54:15 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-04-18 15:02:36 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-04-18 15:02:39 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-04-18 15:02:45 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-04-18 15:03:56 UTC
Patches have been rebased.
Comment 8 Jonathan Druart 2016-04-18 15:35:11 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-04-25 21:49:35 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-04-25 21:49:39 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-04-25 21:49:44 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-04-25 21:49:48 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize (ashimema) 2016-04-26 18:06:18 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize (ashimema) 2016-04-26 18:06:52 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize (ashimema) 2016-04-26 18:07:17 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize (ashimema) 2016-04-26 18:08:10 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize (ashimema) 2016-04-26 18:09:16 UTC
Team Effort, 

I just uploaded the patched, Jonathan Field Signed them off with the testing help of Janet McGowan and Fiona Borthwick.. go PTFS Europe ;)

Progressing to signed off :)
Comment 18 Kyle M Hall (khall) 2016-04-28 12:31:09 UTC
Comment on attachment 50743 [details] [review]
Bug 15581: Display the latest auto renew date possible when renewing manually

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

::: C4/Circulation.pm
@@ +3191,5 @@
> +
> +=cut
> +
> +sub GetLatestAutoRenewDate {
> +    my ( $borrowernumber, $itemnumber ) = @_;

You already have the issue, item and patron data where this code is called, why not pass them in?

@@ +3195,5 @@
> +    my ( $borrowernumber, $itemnumber ) = @_;
> +
> +    my $dbh = C4::Context->dbh;
> +
> +    my $item      = GetItem($itemnumber)      or return;

Re-fetching item we already have.

@@ +3196,5 @@
> +
> +    my $dbh = C4::Context->dbh;
> +
> +    my $item      = GetItem($itemnumber)      or return;
> +    my $itemissue = GetItemIssue($itemnumber) or return;

Re-fetching issue we already have.

@@ +3199,5 @@
> +    my $item      = GetItem($itemnumber)      or return;
> +    my $itemissue = GetItemIssue($itemnumber) or return;
> +
> +    $borrowernumber ||= $itemissue->{borrowernumber};
> +    my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber )

Re-fetching patron data we already have.

::: circ/renew.pl
@@ +83,5 @@
>                      );
>                  }
> +                if ( $error && ( $error eq 'auto_too_late' ) ) {
> +                    $latest_auto_renew_date = C4::Circulation::GetLatestAutoRenewDate(
> +                        $borrower->borrowernumber(),

We should pass in the issue, item, and patron objects here.
Comment 19 Kyle M Hall (khall) 2016-04-28 12:33:30 UTC
Would you please update the code to pass the object instead of the keys? It makes sense to avoid fetching all this data multiple times when there is no need to do so.
Comment 20 Jonathan Druart 2016-04-29 13:00:43 UTC
(In reply to Kyle M Hall from comment #19)
> Would you please update the code to pass the object instead of the keys? It
> makes sense to avoid fetching all this data multiple times when there is no
> need to do so.

Kyle,
I have written GetLatestAutoRenewDate following the same pattern as GetSoonestRenewDate. I have noticed the inconsistencies in this part of code but opted to keep it as it instead of making the code diverge.
If you agree I'd prefer to fix the two subroutines on another bug report, instead of adding a follow-up on this one.
Comment 21 Kyle M Hall (khall) 2016-05-02 11:03:58 UTC
(In reply to Jonathan Druart from comment #20)
> (In reply to Kyle M Hall from comment #19)
> > Would you please update the code to pass the object instead of the keys? It
> > makes sense to avoid fetching all this data multiple times when there is no
> > need to do so.
> 
> Kyle,
> I have written GetLatestAutoRenewDate following the same pattern as
> GetSoonestRenewDate. I have noticed the inconsistencies in this part of code
> but opted to keep it as it instead of making the code diverge.
> If you agree I'd prefer to fix the two subroutines on another bug report,
> instead of adding a follow-up on this one.

That sounds like a good plan to me!
Comment 22 Jonathan Druart 2016-05-02 13:53:47 UTC
(In reply to Kyle M Hall from comment #21)
> (In reply to Jonathan Druart from comment #20)
> > (In reply to Kyle M Hall from comment #19)
> > > Would you please update the code to pass the object instead of the keys? It
> > > makes sense to avoid fetching all this data multiple times when there is no
> > > need to do so.
> > 
> > Kyle,
> > I have written GetLatestAutoRenewDate following the same pattern as
> > GetSoonestRenewDate. I have noticed the inconsistencies in this part of code
> > but opted to keep it as it instead of making the code diverge.
> > If you agree I'd prefer to fix the two subroutines on another bug report,
> > instead of adding a follow-up on this one.
> 
> That sounds like a good plan to me!

Patches submitted on bug 16413.
Comment 23 Jonathan Druart 2016-07-15 13:02:23 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2016-07-15 13:02:31 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2016-07-15 13:02:34 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2016-07-15 13:02:39 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2016-07-15 13:03:01 UTC
Patches rebased (conflict on t/db_dependent/Circulation.t).
Comment 28 Jonathan Druart 2016-09-12 15:34:38 UTC
Created attachment 55504 [details] [review]
Bug 15581: Add a circ rule to limit the auto renewals given a delay

This patch adds a new circulation rule (no_auto_renewal_after) to block/allow
auto renewals after a given delay.
For instance, if the issue date is 10 days before today, and
no_auto_renewal_after is set to 10, tomorrow the issue won't be auto
renewed.

Test plan:
0/ Execute the update DB entry
Note: You will have to manually change data in your DB, make sure you
have access to the sql cli.
1/ Define a rule with no_auto_renewal_after (10 for instance) and
norenewalbefore (5 for instance).
(This new rule will behave the same as norenewalbefore: the unit depends
on the lengthunit value).
The automatic renewals will be done from 5 to 10 days ahead.
2/ Modify the issues.issuedate, to simulate a checkout in the past:
    UPDATE issues
    SET issuedate = "yyyy-mm-dd hh:mm:ss"
    WHERE itemnumber = YOUR_ITEMNUMBER;
with issuedate = 2 days before for instance
3/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too soon)
4/ Repeat step 2 with a due date set as 11 days before
5/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too late)
6/ Repeat step 2 with a due date set as 7 days before
7/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has been renewed (issues.renewals has been
incremented and date_due has been updated according your circ rules).

Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 29 Jonathan Druart 2016-09-12 15:34:45 UTC
Created attachment 55505 [details] [review]
Bug 15581: Display the latest auto renew date possible when renewing manually

If an issue marked as auto_renew is renewed manually, we want to display
the latest auto renew date possible.

Test plan:
1/ Define circ rules as in the previous patch.
2/ Check a item out, mark it as an auto renewal
3/ Back date the issuedate and make sure it will be too late to renew it
4/ Use the Circulation > renew page (circ/renew.pl) to manually renew
this issue.
You should get a warning "You barcode has been scheduled for automatic renewal
and cannot be renewed anymore since DATE."
If the pref AllowRenewalLimitOverride is set, you will be allowed to
renew it anyway.

Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 30 Jonathan Druart 2016-09-12 15:34:52 UTC
Created attachment 55506 [details] [review]
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 31 Jonathan Druart 2016-09-12 15:35:20 UTC
Created attachment 55507 [details] [review]
Bug 15581: [DO NOT PUSH] Schema changes

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 32 Jonathan Druart 2016-10-24 12:15:23 UTC
Created attachment 56746 [details] [review]
Bug 15581: Add a circ rule to limit the auto renewals given a delay

This patch adds a new circulation rule (no_auto_renewal_after) to block/allow
auto renewals after a given delay.
For instance, if the issue date is 10 days before today, and
no_auto_renewal_after is set to 10, tomorrow the issue won't be auto
renewed.

Test plan:
0/ Execute the update DB entry
Note: You will have to manually change data in your DB, make sure you
have access to the sql cli.
1/ Define a rule with no_auto_renewal_after (10 for instance) and
norenewalbefore (5 for instance).
(This new rule will behave the same as norenewalbefore: the unit depends
on the lengthunit value).
The automatic renewals will be done from 5 to 10 days ahead.
2/ Modify the issues.issuedate, to simulate a checkout in the past:
    UPDATE issues
    SET issuedate = "yyyy-mm-dd hh:mm:ss"
    WHERE itemnumber = YOUR_ITEMNUMBER;
with issuedate = 2 days before for instance
3/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too soon)
4/ Repeat step 2 with a due date set as 11 days before
5/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too late)
6/ Repeat step 2 with a due date set as 7 days before
7/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has been renewed (issues.renewals has been
incremented and date_due has been updated according your circ rules).

Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 33 Jonathan Druart 2016-10-24 12:15:32 UTC
Created attachment 56747 [details] [review]
Bug 15581: Display the latest auto renew date possible when renewing manually

If an issue marked as auto_renew is renewed manually, we want to display
the latest auto renew date possible.

Test plan:
1/ Define circ rules as in the previous patch.
2/ Check a item out, mark it as an auto renewal
3/ Back date the issuedate and make sure it will be too late to renew it
4/ Use the Circulation > renew page (circ/renew.pl) to manually renew
this issue.
You should get a warning "You barcode has been scheduled for automatic renewal
and cannot be renewed anymore since DATE."
If the pref AllowRenewalLimitOverride is set, you will be allowed to
renew it anyway.

Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 34 Jonathan Druart 2016-10-24 12:15:37 UTC
Created attachment 56748 [details] [review]
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 35 Jonathan Druart 2016-10-24 12:15:42 UTC
Created attachment 56749 [details] [review]
Bug 15581: [DO NOT PUSH] Schema changes

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 36 Jonathan Druart 2016-10-24 12:16:48 UTC
Patches rebased.
( Make sure 'Bug 16850: Adjust number of tests' is pushed otherwise you will get a conflict. )
Comment 37 Katrin Fischer 2016-10-27 07:32:56 UTC
There is a conflict in the Schema changes file. I am regenerating them locally and obsolete the patch here.
Comment 38 Katrin Fischer 2016-10-28 11:38:01 UTC
Comment on attachment 56748 [details] [review]
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table

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

::: koha-tmpl/intranet-tmpl/prog/en/includes/strings.inc
@@ +17,4 @@
>      var NOT_RENEWABLE = _("Not renewable");
>      var NOT_RENEWABLE_TOO_SOON = _("No renewal before %s");
>      var NOT_RENEWABLE_AUTO_TOO_SOON = _("Scheduled for automatic renewal");
> +    var NOT_RENEWABLE_AUTO_TOO_LATE = _("Scheduled for automatic renewal");

I feel like the TOO_SOON and TOO_LATE messages should be more specific - it seems to hide important information here.
Comment 39 Katrin Fischer 2016-10-28 12:54:34 UTC
Writing a quick summary about how this works for the next reader:
In the circulation conditions there is a new value "no automatic renewal after". This means that "after you have checked out the item for more than X days, there will be no more automatic renewals".

Ok, 2 things:

1) Couldn't test in the OPAC, because of:
Software error:

Template process failed: undef error - The method is_expired is not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121.

For help, please send mail to the webmaster ([no address given]), giving this error message and the time and date of the error. 

2) In the circulation interface it will show "Scheduled for automatic renewal" when this won't happen again because the item has already been checked out for too long. I think this will lead to confusion. Could we make this message more specific?
Comment 40 Jonathan Druart 2016-10-28 14:35:24 UTC
(In reply to Katrin Fischer from comment #39)
> Writing a quick summary about how this works for the next reader:
> In the circulation conditions there is a new value "no automatic renewal
> after". This means that "after you have checked out the item for more than X
> days, there will be no more automatic renewals".
> 
> Ok, 2 things:
> 
> 1) Couldn't test in the OPAC, because of:
> Software error:
> 
> Template process failed: undef error - The method is_expired is not covered
> by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121.
> 
> For help, please send mail to the webmaster ([no address given]), giving
> this error message and the time and date of the error. 

Exists on master and will be fixed by bug 17517.

> 2) In the circulation interface it will show "Scheduled for automatic
> renewal" when this won't happen again because the item has already been
> checked out for too long. I think this will lead to confusion. Could we make
> this message more specific?

What do you suggest?
Comment 41 Janet McGowan 2016-10-28 15:23:25 UTC
(In reply to Jonathan Druart from comment #40)
> (In reply to Katrin Fischer from comment #39)
> > Writing a quick summary about how this works for the next reader:
> > In the circulation conditions there is a new value "no automatic renewal
> > after". This means that "after you have checked out the item for more than X
> > days, there will be no more automatic renewals".
> > 
> > Ok, 2 things:
> > 
> > 1) Couldn't test in the OPAC, because of:
> > Software error:
> > 
> > Template process failed: undef error - The method is_expired is not covered
> > by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121.
> > 
> > For help, please send mail to the webmaster ([no address given]), giving
> > this error message and the time and date of the error. 
> 
> Exists on master and will be fixed by bug 17517.
> 
> > 2) In the circulation interface it will show "Scheduled for automatic
> > renewal" when this won't happen again because the item has already been
> > checked out for too long. I think this will lead to confusion. Could we make
> > this message more specific?
> 
> What do you suggest?

Something like "Can no longer be auto-renewed - number of checkout days exceeded"
 ?(In reply to Jonathan Druart from comment #40)
> (In reply to Katrin Fischer from comment #39)
> > Writing a quick summary about how this works for the next reader:
> > In the circulation conditions there is a new value "no automatic renewal
> > after". This means that "after you have checked out the item for more than X
> > days, there will be no more automatic renewals".
> > 
> > Ok, 2 things:
> > 
> > 1) Couldn't test in the OPAC, because of:
> > Software error:
> > 
> > Template process failed: undef error - The method is_expired is not covered
> > by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121.
> > 
> > For help, please send mail to the webmaster ([no address given]), giving
> > this error message and the time and date of the error. 
> 
> Exists on master and will be fixed by bug 17517.
> 
> > 2) In the circulation interface it will show "Scheduled for automatic
> > renewal" when this won't happen again because the item has already been
> > checked out for too long. I think this will lead to confusion. Could we make
> > this message more specific?
> 
> What do you suggest?

(In reply to Jonathan Druart from comment #40)
> (In reply to Katrin Fischer from comment #39)
> > Writing a quick summary about how this works for the next reader:
> > In the circulation conditions there is a new value "no automatic renewal
> > after". This means that "after you have checked out the item for more than X
> > days, there will be no more automatic renewals".
> > 
> > Ok, 2 things:
> > 
> > 1) Couldn't test in the OPAC, because of:
> > Software error:
> > 
> > Template process failed: undef error - The method is_expired is not covered
> > by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121.
> > 
> > For help, please send mail to the webmaster ([no address given]), giving
> > this error message and the time and date of the error. 
> 
> Exists on master and will be fixed by bug 17517.
> 
> > 2) In the circulation interface it will show "Scheduled for automatic
> > renewal" when this won't happen again because the item has already been
> > checked out for too long. I think this will lead to confusion. Could we make
> > this message more specific?
> 
> What do you suggest?
Comment 42 Jonathan Druart 2016-10-28 16:09:05 UTC
Created attachment 56961 [details] [review]
Bug 15581: Add a circ rule to limit the auto renewals given a delay

This patch adds a new circulation rule (no_auto_renewal_after) to block/allow
auto renewals after a given delay.
For instance, if the issue date is 10 days before today, and
no_auto_renewal_after is set to 10, tomorrow the issue won't be auto
renewed.

Test plan:
0/ Execute the update DB entry
Note: You will have to manually change data in your DB, make sure you
have access to the sql cli.
1/ Define a rule with no_auto_renewal_after (10 for instance) and
norenewalbefore (5 for instance).
(This new rule will behave the same as norenewalbefore: the unit depends
on the lengthunit value).
The automatic renewals will be done from 5 to 10 days ahead.
2/ Modify the issues.issuedate, to simulate a checkout in the past:
    UPDATE issues
    SET issuedate = "yyyy-mm-dd hh:mm:ss"
    WHERE itemnumber = YOUR_ITEMNUMBER;
with issuedate = 2 days before for instance
3/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too soon)
4/ Repeat step 2 with a due date set as 11 days before
5/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too late)
6/ Repeat step 2 with a due date set as 7 days before
7/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has been renewed (issues.renewals has been
incremented and date_due has been updated according your circ rules).

Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 43 Jonathan Druart 2016-10-28 16:09:13 UTC
Created attachment 56962 [details] [review]
Bug 15581: Display the latest auto renew date possible when renewing manually

If an issue marked as auto_renew is renewed manually, we want to display
the latest auto renew date possible.

Test plan:
1/ Define circ rules as in the previous patch.
2/ Check a item out, mark it as an auto renewal
3/ Back date the issuedate and make sure it will be too late to renew it
4/ Use the Circulation > renew page (circ/renew.pl) to manually renew
this issue.
You should get a warning "You barcode has been scheduled for automatic renewal
and cannot be renewed anymore since DATE."
If the pref AllowRenewalLimitOverride is set, you will be allowed to
renew it anyway.

Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 44 Jonathan Druart 2016-10-28 16:09:17 UTC
Created attachment 56963 [details] [review]
Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 45 Jonathan Druart 2016-10-28 16:09:23 UTC
Created attachment 56964 [details] [review]
Bug 15581: Change wording for auto too late
Comment 46 Jonathan Druart 2016-10-28 16:12:11 UTC
Patches rebased and wording updated according to Janet's suggestion.
Comment 47 Jonathan Druart 2016-10-30 20:15:59 UTC
Created attachment 56982 [details] [review]
Bug 15581: Change wording for auto too late
Comment 48 Katrin Fischer 2016-11-01 14:36:18 UTC
This works alright, but my concern is the message we display to the user in the OPAC:

Automatic renewal (2 of 3 renewals remaining)

My suggestion would be to show a message about renewals being impossible and remove the (2 of 3 renewals remaining) both in staff and the OPAC.
Comment 49 Katrin Fischer 2016-11-01 14:56:55 UTC
Created attachment 57034 [details] [review]
[PASSED QA] Bug 15581: Add a circ rule to limit the auto renewals given a delay

This patch adds a new circulation rule (no_auto_renewal_after) to block/allow
auto renewals after a given delay.
For instance, if the issue date is 10 days before today, and
no_auto_renewal_after is set to 10, tomorrow the issue won't be auto
renewed.

Test plan:
0/ Execute the update DB entry
Note: You will have to manually change data in your DB, make sure you
have access to the sql cli.
1/ Define a rule with no_auto_renewal_after (10 for instance) and
norenewalbefore (5 for instance).
(This new rule will behave the same as norenewalbefore: the unit depends
on the lengthunit value).
The automatic renewals will be done from 5 to 10 days ahead.
2/ Modify the issues.issuedate, to simulate a checkout in the past:
    UPDATE issues
    SET issuedate = "yyyy-mm-dd hh:mm:ss"
    WHERE itemnumber = YOUR_ITEMNUMBER;
with issuedate = 2 days before for instance
3/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too soon)
4/ Repeat step 2 with a due date set as 11 days before
5/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too late)
6/ Repeat step 2 with a due date set as 7 days before
7/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has been renewed (issues.renewals has been
incremented and date_due has been updated according your circ rules).

Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 50 Katrin Fischer 2016-11-01 14:57:00 UTC
Created attachment 57035 [details] [review]
[PASSED QA] Bug 15581: Display the latest auto renew date possible when renewing manually

If an issue marked as auto_renew is renewed manually, we want to display
the latest auto renew date possible.

Test plan:
1/ Define circ rules as in the previous patch.
2/ Check a item out, mark it as an auto renewal
3/ Back date the issuedate and make sure it will be too late to renew it
4/ Use the Circulation > renew page (circ/renew.pl) to manually renew
this issue.
You should get a warning "You barcode has been scheduled for automatic renewal
and cannot be renewed anymore since DATE."
If the pref AllowRenewalLimitOverride is set, you will be allowed to
renew it anyway.

Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 51 Katrin Fischer 2016-11-01 14:57:05 UTC
Created attachment 57036 [details] [review]
[PASSED QA] Bug 15581: Display the generic "schedule for auto renew" message on the checkouts table

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 52 Katrin Fischer 2016-11-01 14:57:09 UTC
Created attachment 57037 [details] [review]
[PASSED QA] Bug 15581: Change wording for auto too late

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 53 Katrin Fischer 2016-11-01 14:57:14 UTC
Created attachment 57038 [details] [review]
Bug 15581: Follow-up - suggested change to the OPAC message

Instead of showing "Automatic renewal" to the user, this
changes it to show "No longer renewable" when the
automatic renewal won't happen any longer because of the
days the item has already been checked out.
Comment 54 Katrin Fischer 2016-11-01 15:00:00 UTC
Last patch is a suggestion - I think it's better than to indicate that renewals will still happen, but might need rephrasing. I was not sure how much information to give to the patron about the reasoning, so I kept it general.
Comment 55 Kyle M Hall (khall) 2016-11-02 10:30:59 UTC
(In reply to Katrin Fischer from comment #54)
> Last patch is a suggestion - I think it's better than to indicate that
> renewals will still happen, but might need rephrasing. I was not sure how
> much information to give to the patron about the reasoning, so I kept it
> general.

I think that reads fine to me. We can tweak the wording later based on real user feedback if necessary.
Comment 56 Kyle M Hall (khall) 2016-11-02 10:54:21 UTC
Pushed to master for 16.05, thanks Jonathan, Katrin!
Comment 57 Mason James 2016-11-30 08:11:00 UTC
(In reply to Kyle M Hall from comment #56)
> Pushed to master for 16.05, thanks Jonathan, Katrin!

hi Kyle, i think you mean 'Pushed to master for 16.11'? :)
Comment 58 Mason James 2016-11-30 08:11:48 UTC
Contains DB changes, passing for 16.05.x series
Comment 59 Kyle M Hall (khall) 2017-04-28 13:29:38 UTC
Pushed to master for 16.05, thanks Jonathan!