Skip to content

Add timezone parsing / serializing #455

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: full_tz
Choose a base branch
from
Open

Add timezone parsing / serializing #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a2e5d4
ignore Vagrant files
jorroll Aug 25, 2018
091e1d1
parse/serialize time zone info
jorroll Aug 25, 2018
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
5 changes: 5 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,8 @@

# rspec failure tracking
.rspec_status

# Vagrant files
Vagrantfile
.vagrant
ubuntu*
Comment on lines +13 to +17
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need these?

6 changes: 4 additions & 2 deletions lib/ice_cube/builders/ical_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,13 @@ def self.ical_utc_format(time)

def self.ical_format(time, force_utc)
time = time.dup.utc if force_utc

if time.utc?
":#{IceCube::I18n.l(time, format: '%Y%m%dT%H%M%SZ')}" # utc time
elsif time.respond_to?(:time_zone)
";TZID=#{time.time_zone.name}:#{IceCube::I18n.l(time, format: '%Y%m%dT%H%M%S')}" # time zone specified
else
time_zone = time.respond_to?(:time_zone) ? time.time_zone.name : time.zone
";TZID=#{time_zone}:#{IceCube::I18n.l(time, format: '%Y%m%dT%H%M%S')}" # local time specified
":#{IceCube::I18n.l(time, format: '%Y%m%dT%H%M%S')}" # local time specified
Copy link
Contributor Author

@jorroll jorroll Aug 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

As shown in the spec, dates in local / floating time should be serialized in the form :YYYYMMDDTHHMMSS. Previously they were serialized with ;TZID=${time.zone}:YYYYMMDDTHHMMSS.

While I don't think TZID=${time.zone} is invalid (if only because, as the spec states, there is no specified format for timezones), it isn't a local time and it also doesn't produce time zones which ActiveSupport can understand (ActiveSupport doesn't understand time zone abbreviations, which makes TZID=${time.zone} serializable but not parsable).

end
end

Expand Down
10 changes: 6 additions & 4 deletions lib/ice_cube/parsers/ical_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,19 @@ def self.schedule_from_ical(ical_string, options = {})
ical_string.each_line do |line|
(property, value) = line.split(':')
(property, tzid) = property.split(';')
(_, time_zone) = tzid.split('=') if tzid
Copy link

Choose a reason for hiding this comment

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

I believe this won't always work - looking at the spec, a component may have many parameters, of the form <KEY>=<VALUE>, separated by semicolons. If the key is TZID, then the value is the time zone that you want. But it there are other keys too.

For example, this is a valid ical line: DTSTART;VALUE=DATE:20200525. The code here would parse this and get the time zone DATE, which is not correct. The VALUE parameter tells us that the value (after the colon - 20200525 should be parsed as a date.

Another example would be DTSTART;VALUE=DATE-TIME;TZID:US-Eastern:19980119T020000. Here we do have a time zone, but it would be discarded by the code we have, and instead the time zone would be DATE-TIME.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iainbeeston you're correct.

This might fix it:

(property, *params) = property.split(';')
params.map! { |p| p.split('=') }
time_zone = params.select { |p| p&.first&.upcase == "TZID" }.first&.last
value_type = params.select { |p| p&.first&.upcase == "VALUE" }.first&.last

TimeUtil.deserialize_time should also be updated so that the Hash case accepts a type key and handles both "DATE" and "DATE-TIME" cases (I don't think the "TIME" value is applicable to this library).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this still needs to be resolved?


case property
when 'DTSTART'
data[:start_time] = TimeUtil.deserialize_time(value)
data[:start_time] = TimeUtil.deserialize_time({time: value, zone: time_zone})
when 'DTEND'
data[:end_time] = TimeUtil.deserialize_time(value)
data[:end_time] = TimeUtil.deserialize_time({time: value, zone: time_zone})
when 'RDATE'
data[:rtimes] ||= []
data[:rtimes] += value.split(',').map { |v| TimeUtil.deserialize_time(v) }
data[:rtimes] += value.split(',').map { |v| TimeUtil.deserialize_time({time: v, zone: time_zone}) }
when 'EXDATE'
data[:extimes] ||= []
data[:extimes] += value.split(',').map { |v| TimeUtil.deserialize_time(v) }
data[:extimes] += value.split(',').map { |v| TimeUtil.deserialize_time({time: v, zone: time_zone}) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the TimeUtil.deserialize_time to handle a hash argument where the :zone key might equal nil.

Also, while the TimeUtil.deserialize_time method could already handle parsing times with zone information, the ical_parser never sent zone information to the deserialize_time() method.

when 'DURATION'
data[:duration] # FIXME
when 'RRULE'
Expand Down
4 changes: 2 additions & 2 deletions lib/ice_cube/time_util.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,10 @@ def self.deserialize_time(time_or_hash)
when Time, Date
time_or_hash
when DateTime
Time.local(time.year, time.month, time.day, time.hour, time.min, time.sec)
Time.local(time_or_hash.year, time_or_hash.month, time_or_hash.day, time_or_hash.hour, time_or_hash.min, time_or_hash.sec)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I think that this was a bug. Not sure when, if ever, the when DateTime condition is triggered, but before you had Time.local(time.year ... but the variable time doesn't appear to be defined anywhere.

I think this was a copy-paste error from the serialize_time() method above.

when Hash
hash = FlexibleHash.new(time_or_hash)
hash[:time].in_time_zone(hash[:zone])
hash[:zone] ? hash[:time].in_time_zone(hash[:zone]) : hash[:time]
when String
Time.parse(time_or_hash)
end
Expand Down
6 changes: 3 additions & 3 deletions spec/examples/to_ical_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@
it 'should default to to_ical using local time' do
time = Time.now
schedule = IceCube::Schedule.new(Time.now)
expect(schedule.to_ical).to eq("DTSTART;TZID=#{time.zone}:#{time.strftime('%Y%m%dT%H%M%S')}") # default false
expect(schedule.to_ical).to eq("DTSTART:#{time.strftime('%Y%m%dT%H%M%S')}") # default false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would argue that this test's expectation was just plain incorrect before. As stated, it is testing to ensure a date is serialized in local time (i.e. without a TZID), but before the test expected a string with TZID.

end

it 'should not have an rtime that duplicates start time' do
Expand All @@ -214,8 +214,8 @@
it 'should be able to receive a to_ical in utc time' do
time = Time.now
schedule = IceCube::Schedule.new(Time.now)
expect(schedule.to_ical).to eq("DTSTART;TZID=#{time.zone}:#{time.strftime('%Y%m%dT%H%M%S')}") # default false
expect(schedule.to_ical(false)).to eq("DTSTART;TZID=#{time.zone}:#{time.strftime('%Y%m%dT%H%M%S')}")
expect(schedule.to_ical).to eq("DTSTART:#{time.strftime('%Y%m%dT%H%M%S')}") # default false
expect(schedule.to_ical(false)).to eq("DTSTART:#{time.strftime('%Y%m%dT%H%M%S')}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to the above, this spec was updated to expect local times to be serialized as local times.

expect(schedule.to_ical(true)).to eq("DTSTART:#{time.utc.strftime('%Y%m%dT%H%M%S')}Z")
end

Expand Down