recurring_events icon indicating copy to clipboard operation
recurring_events copied to clipboard

Feature/timezone support

Open pbogut opened this issue 6 years ago • 9 comments

Attempt to resolve #14

The idea is to make Timex optional dependency and if available use different date helper to manipulate dates with timezone awareness.

At this point it should respect timezone for dates.

Todo:

  • [ ] Add more tests
  • [ ] Make sure hour changes are handled correctly

pbogut avatar Sep 02 '18 10:09 pbogut

Pull Request Test Coverage Report for Build 75

  • 46 of 46 (100.0%) changed or added relevant lines in 8 files are covered.
  • 15 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-5.08%) to 94.915%

Files with Coverage Reduction New Missed Lines %
lib/recurring_events/date.ex 15 83.15%
<!-- Total: 15
Totals Coverage Status
Change from base Build 62: -5.08%
Covered Lines: 280
Relevant Lines: 295

💛 - Coveralls

coveralls avatar Sep 02 '18 10:09 coveralls

So if the rule is :hourly, :minutely etc. module is using Timex.shift, so it should handle timezone correctly.

Now, I'm not sure what to do with by_hourif it goes into timezone change. Two examples:

Let say we are working with Europe/Warsaw timezone. When we have by_hour: 2 rule and time is changed form summer time (GTM+2) to winter time (GTM+1). We can have 2 AM GTM+1 and 2 AM GTM+2 as we are moving clock back from 3am to 2am. How to decide which one should be used?

In reverse example we would change clock from 1:59am to 3:00am, so there is no 2am at all. Should 3am be set instead?

pbogut avatar Sep 02 '18 16:09 pbogut

Let say we are working with Europe/Warsaw timezone. When we have by_hour: 2 rule and time is changed form summer time (GTM+2) to winter time (GTM+1). We can have 2 AM GTM+1 and 2 AM GTM+2 as we are moving clock back from 3am to 2am. How to decide which one should be used?

In reverse example we would change clock from 1:59am to 3:00am, so there is no 2am at all. Should 3am be set instead?

I submitted #16 with broken tests for both of these cases. The expected behavior is outlined by RFC5545:

If, based on the definition of the referenced time zone, the local time described occurs more than once (when changing from daylight to standard time), the DATE-TIME value refers to the first occurrence of the referenced time. Thus, TZID=America/ New_York:20071104T013000 indicates November 4, 2007 at 1:30 A.M. EDT (UTC-04:00). If the local time described does not occur (when changing from standard to daylight time), the DATE-TIME value is interpreted using the UTC offset before the gap in local times. Thus, TZID=America/New_York:20070311T023000 indicates March 11, 2007 at 3:30 A.M. EDT (UTC-04:00), one hour after 1:30 A.M. EST (UTC-05:00).

bgentry avatar Sep 02 '18 19:09 bgentry

@pbogut I just want to make sure you saw the following note from #16 about one of those broken tests needing some changes:

Note that the expected value here is also wrong because it has an ambiguous timezone result (since 1:30AM occurs twice). We likely need to explicitly choose the expected timezone for this result to make sure it is the UTC-7 / PDT value, not the UTC-8 PST value (or ambiguous).

bgentry avatar Sep 03 '18 16:09 bgentry

@bgentry yes, I've read the comment, thank you for that. I will take it into account and make sure tests are checking correct timezone as well.

pbogut avatar Sep 04 '18 06:09 pbogut

So, I think we can go off this:

When using Timex, Timex.to_datetime({{2007, 11, 4}, {1, 30, 0}}, "America/New_York") will return an AmbiguousDateTime struct. Because RFC5545 says to refer to the first occurrence, we can safely select the :before field in the struct. When the time is impossible (like 2007-3-11 2:30AM America/New_York) we get an {:error, _} tuple. In all other cases, we should get a %DateTime{} with the correct tz ref.

Here's an example of what I'm thinking about:

dt = {{2017, 11, 4}, {1, 30, 0}}
tz = "America/New_York"
 case Timex.to_datetime(dt, tz) do
   %DateTime{} = datetime ->
     datetime
   %Timex.AmbiguousDateTime{before: before, after: _after} ->
     # ambiguous, use the first occurrence per the spec
     before
   {:error, {:could_not_resolve_timezone, ^tz, _gregorian_seconds, :wall}} ->
     # non-existent time, add 1 hour
     {date, {h, m, s}} = dt
     Timex.to_datetime({date, {h + 1, m, s}}, tz)
 end

Now the last case with the {:error, _} tuple may need to be reworked a little bit because this assumes the only resolution error would be due to daylight savings and that all DST offsets are always 1 hour. (and the possibility that h+1 ends up being something like 24)

voughtdq avatar Nov 27 '18 14:11 voughtdq

@voughtdq that's very helpful. I did implementation for dates but couldn't figure out good way to do time. I think your idea should work all right. Will test it and see how its doing. Thanks.

pbogut avatar Nov 27 '18 15:11 pbogut

@pbogut have you been able to make some progress on this ?

tlvenn avatar Jul 30 '19 11:07 tlvenn

I didn't spent too much time on this I'm afraid, I started it but its still in progress. I'm happy to accept PR if someone is able to do that, I may not have time to look at it for another few months unfortunately.

pbogut avatar Aug 11 '19 15:08 pbogut