Bug 23079 - Checkouts page broken because of problems with date calculation (TZAmerica/Sao_Paulo)
Summary: Checkouts page broken because of problems with date calculation (TZAmerica/Sa...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-07 13:10 UTC by Katrin Fischer
Modified: 2019-09-09 08:16 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.03


Attachments
Bug 23079: Add tests (4.12 KB, patch)
2019-06-23 01:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23079: Handle invalid timezones when adding/subtracting durations (3.71 KB, patch)
2019-06-23 01:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23079: Add tests (4.13 KB, patch)
2019-09-01 20:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23079: Handle invalid timezones when adding/subtracting durations (3.76 KB, patch)
2019-09-01 21:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23079: Add tests (4.18 KB, patch)
2019-09-01 21:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23079: Handle invalid timezones when adding/subtracting durations (3.82 KB, patch)
2019-09-03 14:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23079: Add tests (4.24 KB, patch)
2019-09-03 14:35 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 Katrin Fischer 2019-06-07 13:10:37 UTC
We keep having issues especially with the TZ America/Sao_Paulo in certain areas of Koha. The most recent error was the checkouts page showing "Internal server error" resolving to:

Invalid local time for date in time zone: America/Sao_Paulo

The date expiry of this patron was: 2019-12-03

Changing the date expiry date to be a day later (2019-12-04) fixed the issue. 

The patron currently has no checkouts. I am not sure yet if other settings factor into this.
Comment 1 Jonathan Druart 2019-06-08 19:24:01 UTC
I have no idea how to provide a global and quick fix for this. It will be a big problem for Brazilian libraries using Koha (are there?):

% perl -MDateTime -e "DateTime->new(year => 2019, month => 12, day => 3, time_zone => 'America/Sao_Paulo')->subtract(days => 30);"
Invalid local time for date in time zone: America/Sao_Paulo

% perl -MDateTime -e "DateTime->new(year => 2019, month => 11, day => 3, time_zone => 'America/Sao_Paulo');"
Invalid local time for date in time zone: America/Sao_Paulo
Comment 2 Jonathan Druart 2019-06-08 19:25:53 UTC
You specific issue could be fixed with:

@ Patron.pm:625 @ sub is_going_to_expire {
     return 0 unless $delay;
     return 0 unless $self->dateexpiry;
     return 0 if $self->dateexpiry =~ '^9999';
-    return 1 if dt_from_string( $self->dateexpiry )->subtract( days => $delay ) < dt_from_string->truncate( to => 'day' );
+    return 1
+      if dt_from_string( $self->dateexpiry, undef, 'floating')->subtract( days => $delay ) <
+      dt_from_string->truncate( to => 'day' );
     return 0;
 }


But it will not help much.
Comment 3 Katrin Fischer 2019-06-08 19:28:35 UTC
(In reply to Jonathan Druart from comment #1)
> I have no idea how to provide a global and quick fix for this. It will be a
> big problem for Brazilian libraries using Koha (are there?):

We alone support 4 in this timezone. I was wondering because we don't see issues in other timezones so far and we got quite a few with the Goethe institute libraries. Can you explain the issue a bit?
Comment 4 Jonathan Druart 2019-06-10 15:46:32 UTC
(In reply to Katrin Fischer from comment #3)
> (In reply to Jonathan Druart from comment #1)
> Can you explain the issue a bit?

On Nov 3rd 2019, Brazil will skip from 00:00 to 1:00 (http://www.currenttimeonline.com/dst/dst.do?tz=America/Sao_Paulo), DateTime consider it as an invalid date.
It is a problem when we are playing with dates without the time part (always 00:00).
When we instantiate a DateTime (from dt_from_string) we are already handling this issue, and use the floating timezone (since bug 12669).

The problem remains when we generate a DateTime then add or subtract a duration, which will result in an invalid date:

DateTime->new(year => 2019, month => 12, day => 3, time_zone => 'America/Sao_Paulo')->subtract(days => 30);

=> Nov 3rd 2019, kaboom.

I think we should replace all the occurrences of dt_from_string->subtract (or ->add) with dt_from_string(undef, undef, 'floating'), to use the floating timezone and avoid the error.

Technical note: we must update the prototype of dt_from_string before, to send a hashref instead: dt_from_string({ timezone => 'floating'});
Comment 5 Jonathan Druart 2019-06-23 01:50:00 UTC
Created attachment 90939 [details] [review]
Bug 23079: Add tests
Comment 6 Jonathan Druart 2019-06-23 01:50:04 UTC
Created attachment 90940 [details] [review]
Bug 23079: Handle invalid timezones when adding/subtracting durations

On Nov 3rd 2019, Brazil will skip from 00:00 to 1:00 (http://www.currenttimeonline.com/dst/dst.do?tz=America/Sao_Paulo), DateTime consider it as an invalid date.
It is a problem when we are playing with dates without the time part (always 00:00).
When we instantiate a DateTime (from dt_from_string) we are already handling this issue, and use the floating timezone (since bug 12669).

The problem remains when we generate a DateTime then add or subtract a duration, which will result in an invalid date:

DateTime->new(year => 2019, month => 12, day => 3, time_zone => 'America/Sao_Paulo')->subtract(days => 30);

=> Nov 3rd 2019, kaboom.

We should replace all the problematic occurrences of dt_from_string->subtract (or ->add)
with dt_from_string(undef, undef, 'floating'), to use the floating timezone and avoid the error.

Actually there are not many of them, I have found only 3 that could
produce real problems.

The other occurrences are:
- in tests => Not a big deal (for now)
- called on a datetime, so it will explode if called at midnight
00:00:00 (and nobody should work at that time).

Test plan:
0/ Define the timezone to 'America/Sao_Paulo' (in your koha-conf.xml file), restart_all
1/ Set a patron's expiry date to Dec 3rd 2019, and
NotifyBorrowerDeparture to 30 (default value)
2/ See the checkouts page for this user
=> Without this patch you get "Invalid local time for date in time zone:
America/Sao_Paulo"
=> With this patch apply the page displays correctly

QA will review the 2 other occurrences.
Comment 7 Jonathan Druart 2019-06-23 01:52:04 UTC
Actually the situation is not as dramatic as I thought. Those patches should fix the problematic cases, and especially the one you reported, Katrin.
Comment 8 M. Tompsett 2019-06-24 18:22:04 UTC
(In reply to Jonathan Druart from comment #4)
> I think we should replace all the occurrences of dt_from_string->subtract
> (or ->add) with dt_from_string(undef, undef, 'floating'), to use the
> floating timezone and avoid the error.

Personally, I prefer all dates in the DB were GMT, and all calculations were GMT, and then dates displayed were in local based on the user signed on. Sadly, that would be a massive undertaking.
Comment 9 M. Tompsett 2019-06-24 18:31:10 UTC
Comment on attachment 90940 [details] [review]
Bug 23079: Handle invalid timezones when adding/subtracting durations

Review of attachment 90940 [details] [review]:
-----------------------------------------------------------------

::: Koha/Patron.pm
@@ +622,4 @@
>      return 0 unless $delay;
>      return 0 unless $self->dateexpiry;
>      return 0 if $self->dateexpiry =~ '^9999';
> +    return 1 if dt_from_string( $self->dateexpiry, undef, 'floating' )->subtract( days => $delay ) < dt_from_string(undef, undef, 'floating')->truncate( to => 'day' );

I'd prefer GMT.

::: acqui/duplicate_orders.pl
@@ +74,4 @@
>  
>  unless ( $input->param('from') ) {
>      # Fill the form with year-1
> +    $from_placed_on->set_time_zone('floating')->subtract( years => 1 );

Why not:
$from_placed_on->set_year($from_placed_on->year() - 1);
-- no timezone needed.

::: acqui/histsearch.pl
@@ +94,4 @@
>  my $to_placed_on   = eval { dt_from_string( scalar $input->param('to')   ) } || dt_from_string;
>  unless ( $input->param('from') ) {
>      # Fill the form with year-1
> +    $from_placed_on->set_time_zone('floating')->subtract( years => 1 );

Why not:
$from_placed_on->set_year($from_placed_on->year() - 1);
-- no timezone needed.
Comment 10 Jonathan Druart 2019-06-24 19:29:47 UTC
(In reply to M. Tompsett from comment #9)
> Comment on attachment 90940 [details] [review] [review]
> Bug 23079: Handle invalid timezones when adding/subtracting durations
> 
> Review of attachment 90940 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Patron.pm
> @@ +622,4 @@
> >      return 0 unless $delay;
> >      return 0 unless $self->dateexpiry;
> >      return 0 if $self->dateexpiry =~ '^9999';
> > +    return 1 if dt_from_string( $self->dateexpiry, undef, 'floating' )->subtract( days => $delay ) < dt_from_string(undef, undef, 'floating')->truncate( to => 'day' );
> 
> I'd prefer GMT.

Why?

> ::: acqui/duplicate_orders.pl
> @@ +74,4 @@
> >  
> >  unless ( $input->param('from') ) {
> >      # Fill the form with year-1
> > +    $from_placed_on->set_time_zone('floating')->subtract( years => 1 );
> 
> Why not:
> $from_placed_on->set_year($from_placed_on->year() - 1);
> -- no timezone needed.

Hum? I do not understand how it will fix the problem.
Comment 11 M. Tompsett 2019-07-02 16:05:20 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to M. Tompsett from comment #9)
> > Comment on attachment 90940 [details] [review] [review] [review]
> > Bug 23079: Handle invalid timezones when adding/subtracting durations
> > 
> > Review of attachment 90940 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: Koha/Patron.pm
> > @@ +622,4 @@
> > >      return 0 unless $delay;
> > >      return 0 unless $self->dateexpiry;
> > >      return 0 if $self->dateexpiry =~ '^9999';
> > > +    return 1 if dt_from_string( $self->dateexpiry, undef, 'floating' )->subtract( days => $delay ) < dt_from_string(undef, undef, 'floating')->truncate( to => 'day' );
> > 
> > I'd prefer GMT.
> 
> Why?

https://metacpan.org/pod/DateTime#How-DateTime-Math-Works
Nice long read of recommendations. Floating is fine if we don't care about leap days/seconds, but if we are subtracting days, then perhaps we do care?

"If you can always use the floating or UTC time zones, you can skip ahead to "Leap Seconds and Date Math" "

So, this uses floating, I'd prefer UTC. Floating is good if we don't care about partial days, but...


"$dt->subtract( $duration_object )
A synonym of $dt->subtract_duration( $duration_object )."
-- implying we care about partial days.

"If you only care about the date (calendar) portion of a datetime, you should use either delta_md() or delta_days(), not subtract_datetime(). This will give predictable, unsurprising results, free from DST-related complications."

In short, floating is perfectly okay if we don't care about partial days, do we?


> 
> > ::: acqui/duplicate_orders.pl
> > @@ +74,4 @@
> > >  
> > >  unless ( $input->param('from') ) {
> > >      # Fill the form with year-1
> > > +    $from_placed_on->set_time_zone('floating')->subtract( years => 1 );
> > 
> > Why not:
> > $from_placed_on->set_year($from_placed_on->year() - 1);
> > -- no timezone needed.
> 
> Hum? I do not understand how it will fix the problem.

When timezones are involved weird and wonderful maths are necessary. Since we are just looking for last year, and the time in the current date is valid (otherwise it would have blown up sooner) merely changing just the year back one year avoids anything that might pass-through bizarre timezone stuff. Though, perhaps we care about that?

If we care about partial days, then floating is wrong. If we don't, then floating is okay.
Comment 12 M. Tompsett 2019-07-02 16:14:57 UTC
> > Why not:
> > $from_placed_on->set_year($from_placed_on->year() - 1);
> > -- no timezone needed.
> 
> Hum? I do not understand how it will fix the problem.

"Do not use this method to do date math. Use the add() and subtract() methods instead."
https://metacpan.org/pod/DateTime#%22Set%22-Methods
-- Okay, bad suggestion.
Comment 13 Jonathan Druart 2019-08-04 18:08:26 UTC
We are subtracting days, so to me floating is correct here. If you think we must use GMT because floating is problematic, could you provide an example?
Comment 14 Katrin Fischer 2019-08-11 11:30:46 UTC
Hi Jonathan, thx for your work and input on this!

I've tried signing off, but the tests don't pass for me:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/Patrons.t
t/db_dependent/Koha/Patrons.t .. 8/40 
    #   Failed test at t/db_dependent/Koha/Patrons.t line 275.
    #          got: ''
    #     expected: 'On invalid "is going to expire" date, the method should not crash with "Invalid local time for date in time zone"'
    # Looks like you failed 1 test of 9.

#   Failed test 'is_going_to_expire'
#   at t/db_dependent/Koha/Patrons.t line 280.

Could you have a look?
Comment 15 Jonathan Druart 2019-09-01 20:03:00 UTC
Created attachment 92521 [details] [review]
Bug 23079: Add tests
Comment 16 Katrin Fischer 2019-09-01 21:00:09 UTC
Created attachment 92525 [details] [review]
Bug 23079: Handle invalid timezones when adding/subtracting durations

On Nov 3rd 2019, Brazil will skip from 00:00 to 1:00 (http://www.currenttimeonline.com/dst/dst.do?tz=America/Sao_Paulo), DateTime consider it as an invalid date.
It is a problem when we are playing with dates without the time part (always 00:00).
When we instantiate a DateTime (from dt_from_string) we are already handling this issue, and use the floating timezone (since bug 12669).

The problem remains when we generate a DateTime then add or subtract a duration, which will result in an invalid date:

DateTime->new(year => 2019, month => 12, day => 3, time_zone => 'America/Sao_Paulo')->subtract(days => 30);

=> Nov 3rd 2019, kaboom.

We should replace all the problematic occurrences of dt_from_string->subtract (or ->add)
with dt_from_string(undef, undef, 'floating'), to use the floating timezone and avoid the error.

Actually there are not many of them, I have found only 3 that could
produce real problems.

The other occurrences are:
- in tests => Not a big deal (for now)
- called on a datetime, so it will explode if called at midnight
00:00:00 (and nobody should work at that time).

Test plan:
0/ Define the timezone to 'America/Sao_Paulo' (in your koha-conf.xml file), restart_all
1/ Set a patron's expiry date to Dec 3rd 2019, and
NotifyBorrowerDeparture to 30 (default value)
2/ See the checkouts page for this user
=> Without this patch you get "Invalid local time for date in time zone:
America/Sao_Paulo"
=> With this patch apply the page displays correctly

QA will review the 2 other occurrences.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2019-09-01 21:00:27 UTC
Created attachment 92526 [details] [review]
Bug 23079: Add tests

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Nick Clemens 2019-09-03 14:35:14 UTC
Created attachment 92552 [details] [review]
Bug 23079: Handle invalid timezones when adding/subtracting durations

On Nov 3rd 2019, Brazil will skip from 00:00 to 1:00 (http://www.currenttimeonline.com/dst/dst.do?tz=America/Sao_Paulo), DateTime consider it as an invalid date.
It is a problem when we are playing with dates without the time part (always 00:00).
When we instantiate a DateTime (from dt_from_string) we are already handling this issue, and use the floating timezone (since bug 12669).

The problem remains when we generate a DateTime then add or subtract a duration, which will result in an invalid date:

DateTime->new(year => 2019, month => 12, day => 3, time_zone => 'America/Sao_Paulo')->subtract(days => 30);

=> Nov 3rd 2019, kaboom.

We should replace all the problematic occurrences of dt_from_string->subtract (or ->add)
with dt_from_string(undef, undef, 'floating'), to use the floating timezone and avoid the error.

Actually there are not many of them, I have found only 3 that could
produce real problems.

The other occurrences are:
- in tests => Not a big deal (for now)
- called on a datetime, so it will explode if called at midnight
00:00:00 (and nobody should work at that time).

Test plan:
0/ Define the timezone to 'America/Sao_Paulo' (in your koha-conf.xml file), restart_all
1/ Set a patron's expiry date to Dec 3rd 2019, and
NotifyBorrowerDeparture to 30 (default value)
2/ See the checkouts page for this user
=> Without this patch you get "Invalid local time for date in time zone:
America/Sao_Paulo"
=> With this patch apply the page displays correctly

QA will review the 2 other occurrences.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens 2019-09-03 14:35:17 UTC
Created attachment 92553 [details] [review]
Bug 23079: Add tests

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 20 Martin Renvoize 2019-09-06 12:12:18 UTC
Nice work!

Pushed to master for 19.11.00
Comment 21 Fridolin SOMERS 2019-09-09 08:16:52 UTC
Pushed to 19.05.x for 19.05.04