Skip to content
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

ICAL.Event duration should take timezones into account #395

Merged
merged 1 commit into from
Jan 13, 2020
Merged

ICAL.Event duration should take timezones into account #395

merged 1 commit into from
Jan 13, 2020

Conversation

georgehrke
Copy link
Contributor

@georgehrke georgehrke commented Jun 6, 2019

ICAL.Event has a getter for duration that either returns the duration property or calculates it from dtstart and dtend.

The calculation did not take timezones into account. Since RFC 5545 allows to define different timezones on DTSTART and DTEND, this behaviour is not appropriate.
Start: 01.01.2012 10:20 NYC
End: 01.01.2012 12:50 L.A.

previous duration was 2 1/2 hours,
when applying the fix the duration is correct: 5 1/2 hours

@georgehrke
Copy link
Contributor Author

There is a similar bug in https://github.com/mozilla-comm/ical.js/blob/62f873410ca0ca3428af47f0e3137358e7b81c5e/lib/ical/period.js#L111

Although in that case subtractDate and subtractDateTz won't make a difference, since start and end of a period are always given in UTC.

@georgehrke
Copy link
Contributor Author

Weird, I called grunt package. Not sure why the build fails 🤔

@georgehrke
Copy link
Contributor Author

georgehrke commented Jun 6, 2019

since start and end of a period are always given in UTC.

Actually RFC 5545 3.3.9 does not say anything about start and end having to be in UTC. Only RFC 5545 3.8.2.6 mentions that the time value must be in UTC.

There is also no errata about this.

@kewisch
Copy link
Owner

kewisch commented Jan 13, 2020

Thanks for the patch! I'm taking care of the conflicts.

To have timezones there needs to be a TZID parameter on the property. This is why it isn't explcitly mentioned in the spec for a value, because that has no indication of timezone other than differing between floating time (no suffix) and UTC (Z).

Let's keep the calculation in the period code as is.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.987% when pulling a8d4b84 on georgehrke:bugfix/noid/fix-vevent-duration into ac4df07 on mozilla-comm:master.

@kewisch kewisch merged commit f8174e8 into kewisch:master Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants