In the code: 3309 if ( C4::Context->preference('NoRenewalBeforePrecision') eq 'date' 3310 and $issuing_rule->{lengthunit} eq 'days' ) 3311 { 3312 $soonestrenewal->truncate( to => 'day' ); 3313 } 3314 return $soonestrenewal if $now < $soonestrenewal; We truncate soonest renewal, so when it matches todays date line 3314 compares Today at 00:00:00 to Today at (time renewal checked) So we don't return soonest renewal, but fall back to $now - which does allow renewal, but is not technically correct
Created attachment 132248 [details] [review] Bug 30362: Unit test
Yeah, this is driving me crazy. Consider the following scenario: No renew before = 4 days Due Date = 2023-07-24 23:59:59 Now = 2023-07-20 11:43:00 Soonest Exact Precision = 2023-07-20 23:59:59 Soonest Date Precision = 2023-07-20 00:00:00 -- So "Now" is only < "Soonest" when using "Exact Precision". It's almost always going to be > "Soonest" when using "Date Precision". I reckon if we have a "No renew before", then we should respect whatever value is produced by that algorithm.
Created attachment 153694 [details] [review] Bug 30362: Fix unit test
Created attachment 153695 [details] [review] Bug 30362: Fix GetSoonestRenewDate to really return soonest renew date This change fixes GetSoonestRenewDate so that it returns the soonest renew date as calculated using "No Renewal Before" and "NoRenewalBeforePrecision". In the past, it would only return the soonest renew date if "$now" was lesser than it, which would typically only happen when using an "exact" precision rather than a "date" precision. Test plan: 0. Apply the patch 1. prove t/db_dependent/Circulation.t
Created attachment 153736 [details] [review] Bug 30362: Unit test Signed-off-by: Sam Lau <samalau@gmail.com>
Created attachment 153737 [details] [review] Bug 30362: Fix unit test Signed-off-by: Sam Lau <samalau@gmail.com>
Created attachment 153738 [details] [review] Bug 30362: Fix GetSoonestRenewDate to really return soonest renew date This change fixes GetSoonestRenewDate so that it returns the soonest renew date as calculated using "No Renewal Before" and "NoRenewalBeforePrecision". In the past, it would only return the soonest renew date if "$now" was lesser than it, which would typically only happen when using an "exact" precision rather than a "date" precision. Test plan: 0. Apply the patch 1. prove t/db_dependent/Circulation.t Signed-off-by: Sam Lau <samalau@gmail.com>
Something is not working quite right there: * loan period 5 days * no renewal before 5 dates * today is 2023-07-29 and date in ktd also shows me that * message shown: no renewal before 2023-07-29 ... but I can't renew :(
(In reply to Katrin Fischer from comment #8) > Something is not working quite right there: > > * loan period 5 days > * no renewal before 5 dates > * today is 2023-07-29 and date in ktd also shows me that > * message shown: no renewal before 2023-07-29 > > ... but I can't renew :( Is your NoRenewalBeforePrecision set to "date" or "exact time"?
Test plan: 0. Apply patch 1. koha-plack --reload kohadev 2. Set http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=NoRenewalBeforePrecision to "exact time" 3. Go to http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl 4. Set "No renewal before" to 5 5. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51 and checkout 39999000001310 6. Note that you can't renew the item 7. Set http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=NoRenewalBeforePrecision to "date" 8. Note that you can renew the item -- Since the due date is at 23:59 5 days from now, it makes sense that we can't renew until it's 5 days before the due date (which would be tonight at 23:59). Hmm... but I changed NoRenewalBeforePrecision to "exact time" and Due Date to "08/06/2023" and it's saying "No renewal before 08/01/2023 13:45" but the time is "08/01/2023 13:49". This might be a timezone issue with the ktd... ah yes it is because ktd is using UTC and it thinks the time is "08/01/2023 03:49"
(In reply to Katrin Fischer from comment #8) > Something is not working quite right there: > > * loan period 5 days > * no renewal before 5 dates > * today is 2023-07-29 and date in ktd also shows me that > * message shown: no renewal before 2023-07-29 > > ... but I can't renew :( In fact, with "exact time", that's the norm with master even without this patch. Going to move this one back to "Signed Off"
(In reply to David Cook from comment #11) > (In reply to Katrin Fischer from comment #8) > > Something is not working quite right there: > > > > * loan period 5 days > > * no renewal before 5 dates > > * today is 2023-07-29 and date in ktd also shows me that > > * message shown: no renewal before 2023-07-29 > > > > ... but I can't renew :( > > In fact, with "exact time", that's the norm with master even without this > patch. > > Going to move this one back to "Signed Off" You were right, I was actually not aware of that system preference! It's still ab it wrong isn't it? Since we don't show the time, the user only gets this: No renewal before 01.08.2023 (which makes no sense, when that day is today. Maybe we should show the time or rephrase for 23:59:00 to "after". But not part of this patch.
Created attachment 154136 [details] [review] Bug 30362: Fix GetSoonestRenewDate to really return soonest renew date This change fixes GetSoonestRenewDate so that it returns the soonest renew date as calculated using "No Renewal Before" and "NoRenewalBeforePrecision". In the past, it would only return the soonest renew date if "$now" was lesser than it, which would typically only happen when using an "exact" precision rather than a "date" precision. Test plan: 0. Apply the patch 1. prove t/db_dependent/Circulation.t Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 154137 [details] [review] Bug 30362: Unit test Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 154138 [details] [review] Bug 30362: Fix unit test Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 154139 [details] [review] Bug 30362: Fix GetSoonestRenewDate to really return soonest renew date This change fixes GetSoonestRenewDate so that it returns the soonest renew date as calculated using "No Renewal Before" and "NoRenewalBeforePrecision". In the past, it would only return the soonest renew date if "$now" was lesser than it, which would typically only happen when using an "exact" precision rather than a "date" precision. Test plan: 0. Apply the patch 1. prove t/db_dependent/Circulation.t Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Katrin Fischer from comment #12) > It's still a bit wrong isn't it? Since we don't show the time, the user only > gets this: No renewal before 01.08.2023 (which makes no sense, when that day > is today. Maybe we should show the time or rephrase for 23:59:00 to "after". It is worth double-checking. When I set the due date to "08/06/2023 13:45" it said no renewal before "08/01/2023 13:45". But I think it just said no renewal before "08/01/2023" when I didn't specify a time which I think defaults to 23:59?
Is it correct or not?
(In reply to Tomás Cohen Arazi from comment #18) > Is it correct or not? The patch is correct. The patch just removes a small logic error. Currently, if the soonest renewal time is 1970-01-01 00:00:00, the time returned will be $now. This patch makes it so that 1970-01-01 will be returned instead. -- The (pre-existing) issue that Katrin has observed is that Kate::DateUtils->output_pref erases "11:59 PM" and "23:59" from the time when output_pref() takes an "as_due_date" parameter. In master: If due date is "2023-08-21 23:59:00" "NoRenewalBeforePrecision" set to "date" - "No renewal before 08/16/2023 00:00" "NoRenewalBeforePrecision" set to "exact" - "No renewal before 08/16/2023" If due date is "2023-08-21 22:00:00" "NoRenewalBeforePrecision" set to "date" - "No renewal before 08/16/2023 00:00" "NoRenewalBeforePrecision" set to "exact" - "No renewal before 08/16/2023 22:00" This behaviour dates back to bug 11148. It looks like Marcel noticed it during QA back then even: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11148#c25
(In reply to David Cook from comment #19) > The (pre-existing) issue that Katrin has observed is that > Kate::DateUtils->output_pref erases "11:59 PM" and "23:59" from the time > when output_pref() takes an "as_due_date" parameter. I've raised bug 34539 to document the issue, but as I note above it's related to the patch on this issue.
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.04
Nice work everyone! Pushed to oldstable for 22.11.x
Backport reverted - dependency 31375 is not in 22.11.x Setting to Resolved Fixed
Why has this been caused by bug 31735?
(In reply to Jonathan Druart from comment #25) > Why has this been caused by bug 31735? Hum, actually it has not been. Should we adjust the patch for 22.11 then?
The Jenkins builds failed due to a variable "$issue" not existing in 22.11 as it was added in 31735. Was probably a bad rebase on my part. I can try rebasing again by comparing with current 22.11.x code and removing any changes added in 31735 that the commits try to add in
Rebased and backported - resetting bug to pushed to stable