Time zone support in `schedule_from_ical` #526

epologee · 2022-04-28T10:45:21Z

The base implementation of IceCube::IcalParser.schedule_from_ical ignores time zone identifiers, resulting in all times being parsed in the current time zone of the host.

This PR exposes the issue with a series of specs, and fixes it with a (somewhat lengthy) time zone lookup. This last part was added because the TZID strings of the iCal format don't map cleanly onto Rails time zones.

Input schedule

DTSTART;TZID=MDT:20130731T143000
RRULE:FREQ=WEEKLY;UNTIL=20140730T203000Z;BYDAY=MO,WE,FR
RDATE;TZID=MDT:20150812T143000
RDATE;TZID=MDT:20150807T143000
EXDATE;TZID=MDT:20130823T143000
EXDATE;TZID=MDT:20130812T143000
EXDATE;TZID=MDT:20130807T143000
DTEND;TZID=MDT:20130731T153000

Output schedule on master

DTSTART;TZID=CEST:20130731T143000
RRULE:FREQ=WEEKLY;UNTIL=20140730T203000Z;BYDAY=MO,WE,FR
RDATE;TZID=CEST:20150812T143000
RDATE;TZID=CEST:20150807T143000
EXDATE;TZID=CEST:20130823T143000
EXDATE;TZID=CEST:20130812T143000
EXDATE;TZID=CEST:20130807T143000
DTEND;TZID=CEST:20130731T153000

Output schedule on this branch

DTSTART;TZID=MDT:20130731T143000
RRULE:FREQ=WEEKLY;UNTIL=20140730T203000Z;BYDAY=MO,WE,FR
RDATE;TZID=MDT:20150812T143000
RDATE;TZID=MDT:20150807T143000
EXDATE;TZID=MDT:20130823T143000
EXDATE;TZID=MDT:20130812T143000
EXDATE;TZID=MDT:20130807T143000
DTEND;TZID=MDT:20130731T153000

P.S. Note that time zones will not always match the exact same string, because something like "America/Denver" will get converted to "MDT" along the way. The times will still match semantically.

epologee · 2022-04-28T11:00:14Z

lib/ice_cube/parsers/ical_parser.rb

@@ -4,18 +4,27 @@ def self.schedule_from_ical(ical_string, options = {})
      data = {}
      ical_string.each_line do |line|
        (property, value) = line.split(":")
-        (property, _tzid) = property.split(";")


This _tzid contained the ignored time zone information.

epologee · 2022-04-28T11:01:23Z

lib/ice_cube/parsers/ical_parser.rb

+        Time.find_zone!(zone) if zone.present?
+      rescue ArgumentError
+        (rails_zone, _tzinfo_id) = ActiveSupport::TimeZone::MAPPING.find do |(k, _)|
+          Time.find_zone!(k).now.strftime("%Z") == zone


This is where time zone identifiers like "MDT" are matched with Rails time zones, using strftime.

epologee · 2022-04-29T09:28:28Z

spec/examples/from_ical_spec.rb

+      it "round trips from and to ical with time zones in the summer (MDT)" do
+        original = <<-ICAL.gsub(/^\s*/, "").strip
+          DTSTART;TZID=MDT:20130731T143000
+          RRULE:FREQ=WEEKLY;UNTIL=20140730T203000Z;BYDAY=MO,WE,FR
+          RDATE;TZID=MDT:20150812T143000
+          RDATE;TZID=MDT:20150807T143000
+          EXDATE;TZID=MDT:20130823T143000
+          EXDATE;TZID=MDT:20130812T143000
+          EXDATE;TZID=MDT:20130807T143000
+          DTEND;TZID=MDT:20130731T153000
+        ICAL
+
+        schedule_from_ical = IceCube::Schedule.from_ical original
+        expect(schedule_from_ical.to_ical).to eq original
+      end


This may seem like a superfluous test compared to the one below, but before the last commit on this branch, this test would have failed when run during the winter (standard time vs. daylight saving time period).

I tested this using Timecop locally, but I didn't want to add a dependency to your repository just for this PR, so I didn't commit that part.

pacso · 2022-05-03T07:25:54Z

Hey @epologee, thanks for sending this through!

I'll find some time in the next day or two to review this properly. In the mean time, can you add a changelog entry and ensure your tests pass?

Co-authored-by: Jankees van Woezik <[email protected]>

Using `.now` causes a search for "MST" in the winter, even if the time of the given schedule is in summer and would only match "MDT". By parsing the time value and using the components as values in the candidate time zone, this issue is fixed.

epologee · 2022-05-03T11:28:39Z

Hi, thanks, will do / already done!

In the mean time, can you add a changelog entry and ensure your tests pass?

standardrb reports an indentation issue on lines that I did not touch (in a file I did touch), would you like me to add a commit to fix that indentation? Because the workflow needs your approval to run, I'm not sure if it passes or errors right now:

  lib/ice_cube/time_util.rb:289:18: Layout/CaseIndentation: Indent `when` as deep as `end`.
  lib/ice_cube/time_util.rb:290:18: Layout/CaseIndentation: Indent `when` as deep as `end`.
  lib/ice_cube/time_util.rb:291:18: Layout/CaseIndentation: Indent `when` as deep as `end`.
  lib/ice_cube/time_util.rb:292:18: Layout/CaseIndentation: Indent `when` as deep as `end`.
  lib/ice_cube/time_util.rb:293:18: Layout/CaseIndentation: Indent `when` as deep as `end`.
  lib/ice_cube/time_util.rb:294:18: Layout/CaseIndentation: Indent `when` as deep as `end`.

Cheers!

pacso · 2022-05-03T14:23:04Z

I think once you get your first PR through, you don't need approval for future PR test runs.

epologee · 2022-05-05T08:11:38Z

would you like me to add a commit to fix that indentation?

Update: I added a commit because the standardrb lint prevents the other tests from running.

suzumejakku · 2022-05-20T16:58:19Z

Hi - any chance this will be merged ?

jankeesvw · 2022-06-01T14:47:19Z

We are running this PR in production now, would be great to get this in master.

pacso · 2022-06-01T20:19:17Z

Can you resolve the failing tests? I'll then do a full review and merge it 👍🏻

Although ... these might be tests that I've broken by reverting another branch 🤔 If so ... I'll hopefully sort it soon, and release everything together.

suzumejakku · 2022-07-28T16:48:35Z

Hi @pacso,
Did you have any chance to review this ?

suzumejakku · 2022-10-04T08:14:27Z

Hi @pacso
Sorry to insist, but is there any way to make it into production ?

frsantos · 2023-07-06T14:11:26Z

Hi @seejohnrun @avit @pacso

I'm wondering... are any of you still maintaining this repo?

pacso · 2023-07-08T06:27:04Z

Yes, we are. However I sadly don't have time to resolve the issues on this branch. If you can push changes to fix the failing tests, I'll happily merge it.

epologee · 2023-07-18T10:50:57Z

I pushed a minor edit to re-run the specs. I recall that the failures looked like they weren't created by the changes in this PR, but can have another look after the workflow is approved.

epologee force-pushed the time-zones-from-ical branch from 7409e6f to b2e0af3 Compare April 28, 2022 10:48

epologee marked this pull request as ready for review April 28, 2022 10:49

epologee commented Apr 28, 2022

View reviewed changes

epologee force-pushed the time-zones-from-ical branch from b2e0af3 to 14da4a5 Compare April 28, 2022 11:00

epologee commented Apr 28, 2022

View reviewed changes

epologee commented Apr 29, 2022

View reviewed changes

epologee added a commit to stekker/ice_cube that referenced this pull request May 3, 2022

Changelog entry for PR ice-cube-ruby#526

ab6265b

epologee force-pushed the time-zones-from-ical branch from b4cd303 to ab6265b Compare May 3, 2022 08:14

epologee and others added 2 commits May 3, 2022 10:23

Expose time zone issue when parsing from iCal format

ee6ce49

Co-authored-by: Jankees van Woezik <[email protected]>

Spec that from_ical and to_ical can perform round trip conversion

740125c

Co-authored-by: Jankees van Woezik <[email protected]>

epologee added a commit to stekker/ice_cube that referenced this pull request May 3, 2022

Changelog entry for PR ice-cube-ruby#526

9b2faeb

epologee force-pushed the time-zones-from-ical branch from ab6265b to 9b2faeb Compare May 3, 2022 08:23

epologee and others added 4 commits May 3, 2022 10:25

Lookup the Rails time zone using the tzid (fails for many zone ids)

f1d7f6e

Co-authored-by: Jankees van Woezik <[email protected]>

Alternative time zone lookup using strftime, to complement Rails mapping

3a4ab88

Co-authored-by: Jankees van Woezik <[email protected]>

Changelog entry for PR ice-cube-ruby#526

c3a7ede

epologee force-pushed the time-zones-from-ical branch from 9b2faeb to c3a7ede Compare May 3, 2022 08:27

Indentation fix by standardrb

c607f17

suzumejakku mentioned this pull request May 11, 2022

Add timezone parsing / serializing #455

Open

Edit description to trigger GH actions

e91465a

marcus-deans mentioned this pull request Jun 22, 2024

Time zone parsing support from iCal #555

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time zone support in `schedule_from_ical` #526

Time zone support in `schedule_from_ical` #526

epologee commented Apr 28, 2022

epologee Apr 28, 2022

epologee Apr 28, 2022 •

edited

Loading

epologee Apr 29, 2022

pacso commented May 3, 2022

epologee commented May 3, 2022 •

edited

Loading

pacso commented May 3, 2022

epologee commented May 5, 2022

suzumejakku commented May 20, 2022

jankeesvw commented Jun 1, 2022

pacso commented Jun 1, 2022 •

edited

Loading

suzumejakku commented Jul 28, 2022

suzumejakku commented Oct 4, 2022

frsantos commented Jul 6, 2023

pacso commented Jul 8, 2023

epologee commented Jul 18, 2023

Time zone support in schedule_from_ical #526

Are you sure you want to change the base?

Time zone support in schedule_from_ical #526

Conversation

epologee commented Apr 28, 2022

Input schedule

Output schedule on master

Output schedule on this branch

epologee Apr 28, 2022

Choose a reason for hiding this comment

epologee Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

epologee Apr 29, 2022

Choose a reason for hiding this comment

pacso commented May 3, 2022

epologee commented May 3, 2022 • edited Loading

pacso commented May 3, 2022

epologee commented May 5, 2022

suzumejakku commented May 20, 2022

jankeesvw commented Jun 1, 2022

pacso commented Jun 1, 2022 • edited Loading

suzumejakku commented Jul 28, 2022

suzumejakku commented Oct 4, 2022

frsantos commented Jul 6, 2023

pacso commented Jul 8, 2023

epologee commented Jul 18, 2023

Time zone support in `schedule_from_ical` #526

Time zone support in `schedule_from_ical` #526

epologee Apr 28, 2022 •

edited

Loading

epologee commented May 3, 2022 •

edited

Loading

pacso commented Jun 1, 2022 •

edited

Loading