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.
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.
Created attachment 127618 [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 display 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
(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 :)
Created attachment 127619 [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 display 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
Created attachment 127620 [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
I noticed a couple typos in the comments so re-attached the patch, ready for sign-off.
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
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>
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?
(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"
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>
Created attachment 128478 [details] [review] Bug 29476: (follow-up) Fix return, add precision test Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(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
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>
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>
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.
(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
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.02
Pushed to 21.05.x for 21.05.09
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.