Parse time zones when creating schedules from ICAL; fix DST test bug

[barelyknown]

No description provided.

[barelyknown]

I'm not seeing the CI errors in my test environment. I'll look closer.

[johnhamelink]

I'd love to see this merged - what's stopping it from getting included?

[johnhamelink]

FYI I found a bug in that PR which I fixed and added a spec for here: togglepro#1

[wkrsz]

Without time zone it falls back to Time.parse which uses system time zone, ignoring zone configured in ActiveSupport. IceCube docs mention using ActiveSupport time zone support, so would be great to try Time.zone.parse first, and then fall back to Time.parse.

UPDATE: Actually, IceCube seems to ignore time zone too, so I suppose the problem it's beyond scope of this PR.

2.2.3 :035 > Time.zone.to_s
 => "(GMT+00:00) UTC"
2.2.3 :033 > IceCube::Schedule.new.start_time.zone
 => "CET"

[dcosson]

What's blocking this from being merged? This is a really bad bug, the from_ical method imports things at the wrong time whenever a timezone is used in the ical (which is completely legit by the standard)

Example:

[0] pry(main)> now = Time.now.in_time_zone('America/Los_Angeles');
[1] pry(main)> now.utc
=> 2016-05-10 00:02:05 UTC
[2] pry(main)> IceCube::Schedule.from_ical(IceCube::Schedule.new(now).to_ical).start_time
=> 2016-05-09 17:02:05 +0000

As you can see, it changes the time anytime you deserialize the value.

[dcosson]

This is actually inconsistent with ice_cube's behavior for to_ical. Ice_cube's to_ical uses the %Z strftime string which (at least for en_US based locale's, behavior may be different elsewhere) outputs a string like PDT which is effectively a utc offset not a timezone (PDT= UTC - 7 hours vs PST = UTC - 8 hours).

But ActiveSupport::TimeZone.new requires an actual timezone, either the Olson format timezone such as America/Los_Angeles or the other version of Pacific Time (US & Canada) (which is some US only format that I don't even know if it's a standard with a name). Furthermore, there's not an ActiveSupport method for going from a timezone offset like PDT to a region's timezone because they're not the same thing.

So, in order for this PR to really make sense in the library, the to_ical method should be changed to use the .time_zone.name property of ActiveSupport::TimeWithZone instead of the strftime string.

I checked the ical RFC and it does not specify timezone identifiers, but says Implementers may want to use the naming conventions defined in existing time zone specifications such as the public-domain TZ database [TZDB] and links to the olson timzone database

[Tybot204]

Any new news regarding getting this PR updated and merged? I just ran into this problem in a project of mine creating an event system. I was about to fork the repo but found this PR does what I need!

EDIT: Nevermind to this comment, I missed the reference to the PR that expands on this topic: #335

[avit]

Closing this to keep discussion on the more recent #335. However, ical parsing is something that we might consider moving out of this gem, see #382 for discussion.