Bug 9031 - Overdue items crossing DST boundary throw invalid local time exception
Summary: Overdue items crossing DST boundary throw invalid local time exception
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-08 13:07 UTC by RG
Modified: 2017-10-27 17:18 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Patch (1.08 KB, patch)
2012-11-27 22:42 UTC, RG
Details | Diff | Splinter Review
Bug 9031 (1.33 KB, patch)
2012-11-30 00:25 UTC, RG
Details | Diff | Splinter Review
Bug 9031 - Overdue items crossing DST boundary throw invalid local time exception (1.40 KB, patch)
2017-03-14 16:50 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9031 - Overdue items crossing DST boundary throw invalid local time exception (1.47 KB, patch)
2017-05-25 19:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9031 - Use floating instead of UTC (1.09 KB, patch)
2017-07-07 13:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9031 - Unit tests (1.52 KB, patch)
2017-07-07 13:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9031: Unit tests for DST crossing in (days|hours)_between (1.91 KB, patch)
2017-10-26 08:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9031: (QA follow-up) Pass the same timezone in Calendar.t (1.95 KB, patch)
2017-10-26 08:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9031: Overdue items crossing DST boundary throw invalid local time exception (1.71 KB, patch)
2017-10-26 08:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9031: (QA follow-up) Final changes to Calendar::days_between (3.78 KB, patch)
2017-10-26 08:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9031: Unit tests for DST crossing in (days|hours)_between (1.93 KB, patch)
2017-10-26 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9031: (QA follow-up) Pass the same timezone in Calendar.t (1.96 KB, patch)
2017-10-26 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9031: Overdue items crossing DST boundary throw invalid local time exception (1.72 KB, patch)
2017-10-26 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9031: (QA follow-up) Final changes to Calendar::days_between (3.80 KB, patch)
2017-10-26 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description RG 2012-11-08 13:07:16 UTC
When checking in an overdue item with a DST boundary between original due date and actual check in date, we get the invalid local time error.

The issue happens with places that set DST by removing the hour between 0:00 and 0:59, as you can see here (http://www.nntp.perl.org/group/perl.datetime/2012/06/msg7854.html), not with every DST boundary crossing.

The problem seems to be in the days_between and hours_between subroutines at Koha::Calendar. I've made a quick fix converting the dates to UTC before the calculation of the number of days, but I don't know if it deserves to be a patch. Ideally, all internal date handling should be done in UTC, using local timezones for input and output only, as DateTime page recommends:

> - use UTC for all calculations
>
> If you do care about time zones (particularly DST) or leap seconds, try to use
> non-UTC time zones for presentation and user input only. Convert to UTC 
> immediately and convert back to the local time zone for presentation

https://metacpan.org/module/DateTime#Making-Things-Simple
Comment 1 RG 2012-11-27 22:42:32 UTC Comment hidden (obsolete)
Comment 2 RG 2012-11-27 22:43:57 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2012-11-27 22:46:40 UTC Comment hidden (obsolete)
Comment 4 RG 2012-11-27 23:15:42 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2012-11-27 23:50:51 UTC Comment hidden (obsolete)
Comment 6 RG 2012-11-28 00:01:37 UTC Comment hidden (obsolete)
Comment 7 RG 2012-11-30 00:25:38 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2012-11-30 00:46:42 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2017-03-14 16:50:56 UTC
Created attachment 61089 [details] [review]
Bug 9031 - Overdue items crossing DST boundary throw invalid local time exception

To test:
1 - Set TZ to America/New York
2 - Checkout item and set due date to '2016-03-09 02:29:00"
3 - Make sure fines are set for the item type, fine mode production,
  calculate fines on return
4 - Check in item - invalid date time warning in logs
5 - Apply patch
6 - Check in item - no error
7 - prove t/Calendar.t
Comment 10 Nick Clemens 2017-03-14 16:51:52 UTC
One test is failing on master currently for me 
(13 - Exception holiday is not a closed day test)
This patch doesn't affect that
Comment 11 Katrin Fischer 2017-05-25 19:07:31 UTC
Created attachment 63718 [details] [review]
[SIGNED OFF] Bug 9031 - Overdue items crossing DST boundary throw invalid local time exception

To test:
1 - Set TZ to America/New York
2 - Checkout item and set due date to '2016-03-09 02:29:00"
3 - Make sure fines are set for the item type, fine mode production,
  calculate fines on return
4 - Check in item - invalid date time warning in logs
5 - Apply patch
6 - Check in item - no error
7 - prove t/Calendar.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Marcel de Rooy 2017-05-26 09:48:48 UTC
Not sure if this is the right way to fix it.
Note that we also use time_zone floating. See for instance is_holiday.
Please convince me that we really need UTC here.

Also it would be helpful to add unit tests that make the error visible and show that you resolve it.
Comment 13 Nick Clemens 2017-07-07 13:10:59 UTC
Created attachment 64910 [details] [review]
Bug 9031 - Use floating instead of UTC
Comment 14 Nick Clemens 2017-07-07 13:11:34 UTC
Created attachment 64911 [details] [review]
Bug 9031 - Unit tests
Comment 15 Marcel de Rooy 2017-10-06 10:05:12 UTC
# Subtest: crossing_DST
    1..2
    # No tests run!
not ok 6 - No tests run for subtest "crossing_DST"
#   Failed test 'No tests run for subtest "crossing_DST"'
#   at t/db_dependent/Calendar.t line 84.
Invalid local time for date in time zone: Europe/Amsterdam
Comment 16 Marcel de Rooy 2017-10-25 14:56:41 UTC
Spending some time here.. Coming back tomorrow
Comment 17 Marcel de Rooy 2017-10-26 08:13:14 UTC
Created attachment 68575 [details] [review]
Bug 9031: Unit tests for DST crossing in (days|hours)_between

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Without the patch for Calendar.pm, this crashes on:
Invalid local time for date in time zone: America/New_York

But even with the original change to Calendar.pm, I would see:
Invalid local time for date in time zone: Europe/Amsterdam
Adding a follow-up for that.
Comment 18 Marcel de Rooy 2017-10-26 08:13:20 UTC
Created attachment 68576 [details] [review]
Bug 9031: (QA follow-up) Pass the same timezone in Calendar.t

We do not need to change $ENV{TZ} or call tzset.
Pass $tz too for the second date.
Replace checking the datetime hash by delta calls.
Replacing the number of minutes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
We will still crash with:
Invalid local time for date in time zone: America/New_York
But the changes in Calendar.pm will now resolve that.
Comment 19 Marcel de Rooy 2017-10-26 08:13:25 UTC
Created attachment 68577 [details] [review]
Bug 9031: Overdue items crossing DST boundary throw invalid local time exception

To test:
1 - Set TZ to America/New York
2 - Checkout item and set due date to '2016-03-09 02:29:00"
3 - Make sure fines are set for the item type, fine mode production,
  calculate fines on return
4 - Check in item - invalid date time warning in logs
5 - Apply patch
6 - Check in item - no error
7 - prove t/Calendar.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Bug 9031: Use floating instead of UTC

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Squashed the changes for Calendar.pm; will add a follow-up to finally
overcoming the crash on Invalid local time.
Comment 20 Marcel de Rooy 2017-10-26 08:13:29 UTC
Created attachment 68578 [details] [review]
Bug 9031: (QA follow-up) Final changes to Calendar::days_between

The crash is caused by comparing two datetimes where one datetime is
floating and the other one was not. In that case the floating is
converted. Note too that DateTime overloads comparison operators.

This patch clones the two dates first. Puts them in floating both. And
just after that starts comparing etc.

Similar small change in hours_between.

Adding a test where the parameters are swapped for days_between.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2017-10-26 08:14:51 UTC
Nick,
Please confirm that these changes work for you. And the test passes with you too.
After that I will pass QA.
Comment 22 Nick Clemens 2017-10-26 09:52:25 UTC
Created attachment 68579 [details] [review]
Bug 9031: Unit tests for DST crossing in (days|hours)_between

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Without the patch for Calendar.pm, this crashes on:
Invalid local time for date in time zone: America/New_York

But even with the original change to Calendar.pm, I would see:
Invalid local time for date in time zone: Europe/Amsterdam
Adding a follow-up for that.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens 2017-10-26 09:52:28 UTC
Created attachment 68580 [details] [review]
Bug 9031: (QA follow-up) Pass the same timezone in Calendar.t

We do not need to change $ENV{TZ} or call tzset.
Pass $tz too for the second date.
Replace checking the datetime hash by delta calls.
Replacing the number of minutes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
We will still crash with:
Invalid local time for date in time zone: America/New_York
But the changes in Calendar.pm will now resolve that.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2017-10-26 09:52:32 UTC
Created attachment 68581 [details] [review]
Bug 9031: Overdue items crossing DST boundary throw invalid local time exception

To test:
1 - Set TZ to America/New York
2 - Checkout item and set due date to '2016-03-09 02:29:00"
3 - Make sure fines are set for the item type, fine mode production,
  calculate fines on return
4 - Check in item - invalid date time warning in logs
5 - Apply patch
6 - Check in item - no error
7 - prove t/Calendar.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Bug 9031: Use floating instead of UTC

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Squashed the changes for Calendar.pm; will add a follow-up to finally
overcoming the crash on Invalid local time.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2017-10-26 09:52:35 UTC
Created attachment 68582 [details] [review]
Bug 9031: (QA follow-up) Final changes to Calendar::days_between

The crash is caused by comparing two datetimes where one datetime is
floating and the other one was not. In that case the floating is
converted. Note too that DateTime overloads comparison operators.

This patch clones the two dates first. Puts them in floating both. And
just after that starts comparing etc.

Similar small change in hours_between.

Adding a test where the parameters are swapped for days_between.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Jonathan Druart 2017-10-27 17:18:45 UTC
Pushed to master for 17.11, thanks to everybody involved!