Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions lib/ice_cube/parsers/ical_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ def self.schedule_from_ical(ical_string, options = {})
(property, tzid) = property.split(';')
case property
when 'DTSTART'
data[:start_time] = Time.parse(value)
data[:start_time] = _parse_in_tzid(value, tzid)
when 'DTEND'
data[:end_time] = Time.parse(value)
data[:end_time] = _parse_in_tzid(value, tzid)
when 'EXDATE'
data[:extimes] ||= []
data[:extimes] += value.split(',').map{|v| Time.parse(v)}
data[:extimes] += value.split(',').map do |v|
_parse_in_tzid(v, tzid)
end
when 'DURATION'
data[:duration] # FIXME
when 'RRULE'
Expand All @@ -23,6 +25,14 @@ def self.schedule_from_ical(ical_string, options = {})
Schedule.from_hash data
end

def self._parse_in_tzid(value, tzid)
time_parser = Time
if tzid
time_parser = ActiveSupport::TimeZone.new(tzid.split('=')[1]) || Time
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
time_parser.parse(value)
end

def self.rule_from_ical(ical)
params = { validations: { } }

Expand Down
34 changes: 31 additions & 3 deletions spec/examples/from_ical_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,22 @@ module IceCube

end

describe Schedule, 'from_ical' do
describe Schedule, 'from_ical', system_time_zone: "America/Chicago" do

ical_string = <<-ICAL.gsub(/^\s*/, '')
DTSTART:20130314T201500Z
DTEND:20130314T201545Z
RRULE:FREQ=WEEKLY;BYDAY=TH;UNTIL=20130531T100000Z
ICAL

ical_string_with_multiple_exdates = <<-ICAL.gsub(/^\s*/, '')
ical_string_with_time_zones = <<-ICAL.gsub(/^\s*/,'')
DTSTART;TZID=America/Denver:20130731T143000
DTEND:20130731T153000
RRULE:FREQ=WEEKLY
EXDATE;TZID=America/Chicago:20130823T143000
ICAL

ical_string_with_multiple_exdates = <<-ICAL.gsub(/^\s*/, '')
DTSTART;TZID=America/Denver:20130731T143000
DTEND;TZID=America/Denver:20130731T153000
RRULE:FREQ=WEEKLY;UNTIL=20140730T203000Z;BYDAY=MO,WE,FR
Expand Down Expand Up @@ -130,6 +137,28 @@ def sorted_ical(ical)
it "loads an ICAL string" do
expect(IceCube::Schedule.from_ical(ical_string)).to be_a(IceCube::Schedule)
end

describe "parsing time zones" do
it "sets the time zone of the start time" do
schedule = IceCube::Schedule.from_ical(ical_string_with_time_zones)
expect(schedule.start_time.time_zone).to eq ActiveSupport::TimeZone.new("America/Denver")
end

it "uses the system time if a time zone is not explicity provided" do
schedule = IceCube::Schedule.from_ical(ical_string_with_time_zones)
expect(schedule.end_time).not_to respond_to :time_zone
end

it "sets the time zone of the exception times" do
schedule = IceCube::Schedule.from_ical(ical_string_with_time_zones)
expect(schedule.exception_times[0].time_zone).to eq ActiveSupport::TimeZone.new("America/Chicago")
end

it "adding the offset doesnt also change the time" do
schedule = IceCube::Schedule.from_ical(ical_string_with_time_zones)
expect(schedule.exception_times[0].hour).to eq 14
end
end
end

describe "daily frequency" do
Expand Down Expand Up @@ -240,7 +269,6 @@ def sorted_ical(ical)
describe 'monthly frequency' do
it 'matches simple monthly' do
start_time = Time.now

schedule = IceCube::Schedule.new(start_time)
schedule.add_recurrence_rule(IceCube::Rule.monthly)

Expand Down
13 changes: 7 additions & 6 deletions spec/examples/hourly_rule_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,14 @@ module IceCube
end

it 'should not skip times in DST end hour' do
schedule = Schedule.new(t0 = Time.local(2013, 11, 3, 0, 0, 0))
tz = ActiveSupport::TimeZone["America/Vancouver"]
schedule = Schedule.new(t0 = tz.local(2013, 11, 3, 0, 0, 0))
schedule.add_recurrence_rule Rule.hourly
schedule.first(4).should == [
Time.local(2013, 11, 3, 0, 0, 0), # -0700
Time.local(2013, 11, 3, 1, 0, 0) - ONE_HOUR, # -0700
Time.local(2013, 11, 3, 1, 0, 0), # -0800
Time.local(2013, 11, 3, 2, 0, 0), # -0800
expect(schedule.first(4)).to eq [
tz.local(2013, 11, 3, 0, 0, 0), # -0700
tz.local(2013, 11, 3, 1, 0, 0), # -0700
tz.local(2013, 11, 3, 2, 0, 0) - ONE_HOUR, # -0800
tz.local(2013, 11, 3, 2, 0, 0), # -0800
]
end

Expand Down