Bug 30362 - GetSoonestRenewDate is technically wrong when NoRenewalBeforePrecision set to date soonest renewal is today
Summary: GetSoonestRenewDate is technically wrong when NoRenewalBeforePrecision set to...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: David Cook
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 31735
Blocks:
  Show dependency treegraph
 
Reported: 2022-03-25 15:18 UTC by Nick Clemens
Modified: 2024-02-12 12:45 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04,22.11.10


Attachments
Bug 30362: Unit test (1.31 KB, patch)
2022-03-25 15:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30362: Fix unit test (1.20 KB, patch)
2023-07-20 02:57 UTC, David Cook
Details | Diff | Splinter Review
Bug 30362: Fix GetSoonestRenewDate to really return soonest renew date (1.26 KB, patch)
2023-07-20 02:57 UTC, David Cook
Details | Diff | Splinter Review
Bug 30362: Unit test (1.38 KB, patch)
2023-07-20 16:03 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 30362: Fix unit test (1.24 KB, patch)
2023-07-20 16:03 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 30362: Fix GetSoonestRenewDate to really return soonest renew date (1.30 KB, patch)
2023-07-20 16:03 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 30362: Fix GetSoonestRenewDate to really return soonest renew date (2.12 KB, patch)
2023-08-01 21:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30362: Unit test (1.44 KB, patch)
2023-08-01 21:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30362: Fix unit test (1.30 KB, patch)
2023-08-01 21:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30362: Fix GetSoonestRenewDate to really return soonest renew date (2.12 KB, patch)
2023-08-01 21:45 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 Nick Clemens 2022-03-25 15:18:11 UTC
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
Comment 1 Nick Clemens 2022-03-25 15:18:45 UTC
Created attachment 132248 [details] [review]
Bug 30362: Unit test
Comment 2 David Cook 2023-07-20 01:48:28 UTC
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.
Comment 3 David Cook 2023-07-20 02:57:55 UTC
Created attachment 153694 [details] [review]
Bug 30362: Fix unit test
Comment 4 David Cook 2023-07-20 02:57:58 UTC
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
Comment 5 Sam Lau 2023-07-20 16:03:12 UTC
Created attachment 153736 [details] [review]
Bug 30362: Unit test

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 6 Sam Lau 2023-07-20 16:03:15 UTC
Created attachment 153737 [details] [review]
Bug 30362: Fix unit test

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 7 Sam Lau 2023-07-20 16:03:17 UTC
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>
Comment 8 Katrin Fischer 2023-07-29 18:27:39 UTC
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 :(
Comment 9 David Cook 2023-08-01 03:37:30 UTC
(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"?
Comment 10 David Cook 2023-08-01 03:50:45 UTC
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"
Comment 11 David Cook 2023-08-01 03:52:42 UTC
(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"
Comment 12 Katrin Fischer 2023-08-01 20:51:41 UTC
(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.
Comment 13 Katrin Fischer 2023-08-01 21:00:42 UTC
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>
Comment 14 Katrin Fischer 2023-08-01 21:45:17 UTC
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>
Comment 15 Katrin Fischer 2023-08-01 21:45:19 UTC
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>
Comment 16 Katrin Fischer 2023-08-01 21:45:22 UTC
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>
Comment 17 David Cook 2023-08-02 01:02:21 UTC
(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?
Comment 18 Tomás Cohen Arazi 2023-08-15 12:03:32 UTC
Is it correct or not?
Comment 19 David Cook 2023-08-15 23:50:17 UTC
(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
Comment 20 David Cook 2023-08-15 23:56:13 UTC
(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.
Comment 21 Tomás Cohen Arazi 2023-08-30 12:03:52 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 22 Fridolin Somers 2023-08-30 20:07:00 UTC
Pushed to 23.05.x for 23.05.04
Comment 23 Matt Blenkinsop 2023-08-31 10:37:08 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 24 Matt Blenkinsop 2023-08-31 11:39:08 UTC
Backport reverted - dependency 31375 is not in 22.11.x

Setting to Resolved Fixed
Comment 25 Jonathan Druart 2023-08-31 13:02:10 UTC
Why has this been caused by bug 31735?
Comment 26 Jonathan Druart 2023-08-31 13:03:07 UTC
(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?
Comment 27 Matt Blenkinsop 2023-08-31 13:16:41 UTC
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
Comment 28 Matt Blenkinsop 2023-08-31 13:35:31 UTC
Rebased and backported - resetting bug to pushed to stable
Comment 29 Matt Blenkinsop 2023-08-31 13:36:34 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x