Bug 29476

Summary: Earliest renewal date is displayed wrong in circ/renew.pl for issues with auto renewing
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: CirculationAssignee: Joonas Kylmälä <joonas.kylmala>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: andrew, dcook, fridolin.somers, gmcharlt, jonathan.druart+koha, kyle.m.hall, kyle, nick, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.02,21.05.09
Bug Depends on:    
Bug Blocks: 29537    
Attachments: Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal
Bug 29476: (follow-up) Fix return, add precision test
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal
Bug 29476: (follow-up) Fix return, add precision test

Description Joonas Kylmälä 2021-11-14 13:16:03 UTC
The code in circ/renew.pl uses the code C4::Circulation::GetSoonestRenewDate to check the soonest renewal date and then displays it to the librarian. However, the code called by CanBookBeRenewed() that actually does the final check uses different calculation method for issues with auto renewal enabled. For auto renewed issues without circulation rules defined for soonest renewal date the soonest renewal date is the due date, in normal loans case the soonest renewal date is NOW(). This is not probably a too big annoyance to librarians, I just wanna clean up this so I can refactor some code in CanBookBeRenewed, will attach a patch shortly.
Comment 1 Katrin Fischer 2021-11-14 14:19:37 UTC
Hi Joonas, for "normal" renewals the "not renewal x days before" is also used. Actually it was developed fro normal renewals and around before the auto renewals were added.
Comment 2 Joonas Kylmälä 2021-11-14 14:31:52 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2021-11-14 14:33:57 UTC
(In reply to Katrin Fischer from comment #1)
> Hi Joonas, for "normal" renewals the "not renewal x days before" is also
> used. Actually it was developed fro normal renewals and around before the
> auto renewals were added.

Yup, that's right. I think maybe you misunderstood my comment wrong and added this comment, please see the patch and maybe the bug is more clear, also a sign-off would be nice :)
Comment 4 Joonas Kylmälä 2021-11-14 14:37:01 UTC Comment hidden (obsolete)
Comment 5 Joonas Kylmälä 2021-11-14 14:41:43 UTC Comment hidden (obsolete)
Comment 6 Joonas Kylmälä 2021-11-14 14:42:22 UTC
I noticed a couple typos in the comments so re-attached the patch, ready for sign-off.
Comment 7 Joonas Kylmälä 2021-11-20 11:34:09 UTC
Created attachment 127900 [details] [review]
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal

If a checkout with auto-renewal enabled doesn't have a
"norenewalbefore" circulation rule set the code in CanBookBeRenewed()
falls back to using due date (to verify this please look for the
string "auto_too_soon" in C4/Circulation.pm), the calculation result
of GetSoonestRenewDate() however didn't do this, though luckily it was
not used in CanBookBeRenewed so we didn't get any issues
there. However, GetSoonestRenewDate() is used for displaying the
soonest renewal date in the staff interface on the circ/renew.pl page
so you would have gotten wrong results there.

This patch moves additionally the tests made for Bug 14395 under a new
subtest for GetSoonestRenewDate() as they should have been like that
already before.

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


Current status: Needs Signoff
Comment 8 David Nind 2021-11-29 21:52:50 UTC
Created attachment 128096 [details] [review]
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal

If a checkout with auto-renewal enabled doesn't have a
"norenewalbefore" circulation rule set the code in CanBookBeRenewed()
falls back to using due date (to verify this please look for the
string "auto_too_soon" in C4/Circulation.pm), the calculation result
of GetSoonestRenewDate() however didn't do this, though luckily it was
not used in CanBookBeRenewed so we didn't get any issues
there. However, GetSoonestRenewDate() is used for displaying the
soonest renewal date in the staff interface on the circ/renew.pl page
so you would have gotten wrong results there.

This patch moves additionally the tests made for Bug 14395 under a new
subtest for GetSoonestRenewDate() as they should have been like that
already before.

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Jonathan Druart 2021-12-02 10:07:13 UTC
1. There is a return in an assignment statement:
+        my $soonestrenewal = return dt_from_string( $itemissue->date_due );

2. Wondering, should we 
  return $soonestrenewal if $now < $soonestrenewal;
?

3. Shouldn't we finally call GetSoonestRenewDate from _CanBookBeAutoRenewed?
Comment 10 Jonathan Druart 2021-12-02 15:17:52 UTC
(In reply to Jonathan Druart from comment #9)
> 3. Shouldn't we finally call GetSoonestRenewDate from _CanBookBeAutoRenewed?

This is what you actually did on the patch "Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed"
Comment 11 Nick Clemens 2021-12-13 17:28:18 UTC
Created attachment 128477 [details] [review]
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal

If a checkout with auto-renewal enabled doesn't have a
"norenewalbefore" circulation rule set the code in CanBookBeRenewed()
falls back to using due date (to verify this please look for the
string "auto_too_soon" in C4/Circulation.pm), the calculation result
of GetSoonestRenewDate() however didn't do this, though luckily it was
not used in CanBookBeRenewed so we didn't get any issues
there. However, GetSoonestRenewDate() is used for displaying the
soonest renewal date in the staff interface on the circ/renew.pl page
so you would have gotten wrong results there.

This patch moves additionally the tests made for Bug 14395 under a new
subtest for GetSoonestRenewDate() as they should have been like that
already before.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2021-12-13 17:28:22 UTC
Created attachment 128478 [details] [review]
Bug 29476: (follow-up) Fix return, add precision test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2021-12-13 17:30:40 UTC
(In reply to Jonathan Druart from comment #9)
> 2. Wondering, should we 
>   return $soonestrenewal if $now < $soonestrenewal;
> ?
I don't think so

There are two cases: with undefined no renewal before:
1 - Automatic renewals wait until it is due
2 - Manual renewals are possible at any time 

Let me know if I missed something
Comment 14 Jonathan Druart 2021-12-15 10:32:04 UTC
Created attachment 128552 [details] [review]
Bug 29476: Correct soonest renewal date calculation for checkouts with auto-renewal

If a checkout with auto-renewal enabled doesn't have a
"norenewalbefore" circulation rule set the code in CanBookBeRenewed()
falls back to using due date (to verify this please look for the
string "auto_too_soon" in C4/Circulation.pm), the calculation result
of GetSoonestRenewDate() however didn't do this, though luckily it was
not used in CanBookBeRenewed so we didn't get any issues
there. However, GetSoonestRenewDate() is used for displaying the
soonest renewal date in the staff interface on the circ/renew.pl page
so you would have gotten wrong results there.

This patch moves additionally the tests made for Bug 14395 under a new
subtest for GetSoonestRenewDate() as they should have been like that
already before.

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2021-12-15 10:32:09 UTC
Created attachment 128553 [details] [review]
Bug 29476: (follow-up) Fix return, add precision test

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Fridolin Somers 2021-12-22 07:21:16 UTC
Hi,

Maybe a silly question :

-    return $now unless $issuing_rule;
I don't understand why do we remove this ?
issuing rule may be undef right ?
In this case the calls $issuing_rule->{xxx} will fail.
Comment 17 Nick Clemens 2021-12-22 11:04:01 UTC
(In reply to Fridolin Somers from comment #16)
> Hi,
> 
> Maybe a silly question :
> 
> -    return $now unless $issuing_rule;
> I don't understand why do we remove this ?
> issuing rule may be undef right ?
> In this case the calls $issuing_rule->{xxx} will fail.

The first check is 'defined' so it will be okay.

We remove the return because there are two cases, see comment 13
Comment 18 Fridolin Somers 2022-01-14 02:38:22 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 19 Kyle M Hall 2022-01-14 14:34:01 UTC
Pushed to 21.11.x for 21.11.02
Comment 20 Andrew Fuerste-Henry 2022-01-25 17:52:41 UTC
Pushed to 21.05.x for 21.05.09
Comment 21 Victor Grousset/tuxayo 2022-02-11 20:32:01 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.