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
Created attachment 13714 [details] [review] Patch
Comment on attachment 13714 [details] [review] Patch As described above, this patch converts $start_dt and $end_dt to UTC before performing the calculation of the number of days between due date and actual check in date.
Robson, would you like me to convert your patch into one that has your email etc? (IE did you create this patch with git-format-patch ?)
(In reply to comment #3) > Robson, would you like me to convert your patch into one that has your email > etc? > > (IE did you create this patch with git-format-patch ?) Yes, it was created with git-format-patch. I guess I butchered it somehow before posting. Thanks.
Hmm yeah it seems to be missing a lot of stuff, it should look more like http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=13159
Anyway, I've sent it to the patches list, but it's now awaiting moderation because I used my GMail account, while my subscription uses Hotmail. Forgive the noobness, it's my first time doing this stuff. I still have the file though, I can post it again. It looks like this: From 588a40a14fb7e33229e07f8260b73532aa7ddd67 Mon Sep 17 00:00:00 2001 From: Robson Galluci <robsongalluci@gmail.com> Date: Tue, 27 Nov 2012 20:04:06 -0200 Subject: [PATCH] Bug 9031 - Overdue items crossing DST boundary throw invalid local time exception This patch converts $start_dt and $end_dt to UTC before performing the calculation of the number of days between due date and actual check in date. --- Koha/Calendar.pm | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm index 90069b6..7075c49 100644 --- a/Koha/Calendar.pm +++ b/Koha/Calendar.pm @@ -241,6 +241,8 @@ sub days_between { my $start_dt = shift; my $end_dt = shift; + $start_dt->set_time_zone('UTC'); + $end_dt->set_time_zone('UTC'); # start and end should not be closed days my $days = $start_dt->delta_days($end_dt)->delta_days; @@ -263,6 +265,10 @@ sub hours_between { my $duration = $end_dt->delta_ms($start_dt); $start_dt->truncate( to => 'day' ); $end_dt->truncate( to => 'day' ); + + $start_dt->set_time_zone('UTC'); + $end_dt->set_time_zone('UTC'); + # NB this is a kludge in that it assumes all days are 24 hours # However for hourly loans the logic should be expanded to # take into account open/close times then it would be a duration -- 1.7.2.5
Created attachment 13795 [details] [review] Bug 9031 Fixed patch
Hi Robson Thanks for the valid patch, however it is causing the unit tests to fail. perl t/Calendar.t 1..26 ok 1 - use Koha::Calendar; ok 2 - use C4::Calendar; ok 3 - Calendar class returned isa Koha::Calendar ok 4 - Sunday is a closed day ok 5 - Monday is not a closed day ok 6 - month/day closed day test ok 7 - special closed day test ok 8 - open day test ok 9 - addDate skips closed Sunday ok 10 - Negative call to addDate ok 11 - days_between calculates correctly ok 12 - is holiday for the next test not ok 13 - Date should be the same after is_holiday # Failed test 'Date should be the same after is_holiday' # at t/Calendar.t line 99. # got: '2012-09-17T16:30:00' # expected: '2012-09-17T17:30:00' ok 14 - test larger intervals ok 15 - test positive intervals ok 16 - test parameter order not relevant ok 17 - days_between calculates correctly not ok 18 - holiday correctly recognized # Failed test 'holiday correctly recognized' # at t/Calendar.t line 119. # got: 14 # expected: 13 not ok 19 - multiple holidays correctly recognized # Failed test 'multiple holidays correctly recognized' # at t/Calendar.t line 124. # got: 14 # expected: 12 ok 20 - Single day add (Datedue, matches holiday, shift) ok 21 - Two days add, skips holiday (Datedue) not ok 22 - Add 7 days (Datedue) # Failed test 'Add 7 days (Datedue)' # at t/Calendar.t line 145. # got: '2012-07-30T10:53:00' # expected: '2012-07-30T11:53:00' ok 23 - Single day add (Calendar) not ok 24 - Add 7 days (Calendar) # Failed test 'Add 7 days (Calendar)' # at t/Calendar.t line 162. # got: '2012-07-31T10:53:00' # expected: '2012-07-31T11:53:00' ok 25 - Single day add (Days) not ok 26 - Add 7 days (Days) # Failed test 'Add 7 days (Days)' # at t/Calendar.t line 179. # got: '2012-07-30T10:53:00' # expected: '2012-07-30T11:53:00' # Looks like you failed 6 tests of 26. Could you have a look at this please.
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
One test is failing on master currently for me (13 - Exception holiday is not a closed day test) This patch doesn't affect that
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>
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.
Created attachment 64910 [details] [review] Bug 9031 - Use floating instead of UTC
Created attachment 64911 [details] [review] Bug 9031 - Unit tests
# 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
Spending some time here.. Coming back tomorrow
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.
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.
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.
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>
Nick, Please confirm that these changes work for you. And the test passes with you too. After that I will pass QA.
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>
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>
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>
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>
Pushed to master for 17.11, thanks to everybody involved!