Skip to content

respect the timezone from the ical event#2361

Merged
kodiakhq[bot] merged 5 commits intocalcom:mainfrom
buschco:fix/timezone-from-calendar
Apr 4, 2022
Merged

respect the timezone from the ical event#2361
kodiakhq[bot] merged 5 commits intocalcom:mainfrom
buschco:fix/timezone-from-calendar

Conversation

@buschco
Copy link
Copy Markdown
Contributor

@buschco buschco commented Apr 3, 2022

Fixing offsets caused by timezone reasons

Digging down, I found that the calendar service ignores timezones form the calendar events.

I looked into the implementation of ical.js to find out how they handle timezones, since I was not able to find anything in CalendarService.
They will use toUnixTime which will subtract the UTC offset from utc_offset
-> https://github.com/mozilla-comm/ical.js/blob/ba8e2522ffd30ffbe65197a96a487689d6e6e9a1/lib/ical/time.js#L736-L747
-> https://github.com/mozilla-comm/ical.js/blob/ba8e2522ffd30ffbe65197a96a487689d6e6e9a1/lib/ical/time.js#L685-L692

This offset will be 0 if the timezone matches the local timezone or the UTC timezone. Otherwise they try to find the offset by the registered timezone. Since we haven't registered any timezone, I suppose the offset will also be 0:

The stock ical.js does not register any timezones, due to the additional size it brings. If you'd like to do timezone conversion, and the timezone definitions are not included in the respective ics files, you'll need to use ical.timezones.js or its minified counterpart.
https://github.com/mozilla-comm/ical.js#timezones

So when ical.js creates a new date from the ical data and it wont have the timezone since we never registered it. This results in a Date that is in local time but is labeled as UTC.

Potential fixes

Register timezones

We could fix this by registering the timezones according to the ical.js [documentation])(https://github.com/mozilla-comm/ical.js#timezones). I am not happy with this solution because first, the documentation says we can use ical.timezones.js, however, I was not able to find this (looked into the node_module and on npm). Also it seems the typedefintions are not well resulting in // @ts-ignores.

const timezoneComp = vcalendar.getFirstSubcomponent("vtimezone");
//@ts-ignore
const tzid = timezoneComp?.getFirstPropertyValue("tzid") || ICAL.Timezone.utcTimezone.tzid;

//@ts-ignore
if (!ICAL.TimezoneService.has(tzid)) {
    if (timezoneComp == null) throw new Error("cannot add timezone because there is no timezone given");
    const newTimezone = new ICAL.Timezone({ component: timezoneComp, tzid });
    //@ts-ignore
    ICAL.TimezoneService.register(tzid, newTimezone);
}

use dayjs

I prefer this one, since it seems like dayjs is the goto way handling timezone related challenges.

Fixes

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

The only way I could reproduce this issue, is by setting the TZ variable:
TZ='UTC' yarn dx
found here -> https://stackoverflow.com/questions/8083410/how-can-i-set-the-default-timezone-in-node-js

I was never able to reproduce this without on my mac. Seems like the vercel servers are using UTC then.

You also need a Calendar in a different timezone than UTC.

  • Create an event and go to troubleshooter. The time should match exactly if your cal account uses the same timezone

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 3, 2022

@buschco is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 3, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

docs – ./apps/docs

🔍 Inspect: https://vercel.com/cal/docs/FyQc4A95zo5GkEm6Zfrwar7nW9Dg
✅ Preview: https://docs-git-fork-buschco-fix-timezone-from-calendar-cal.vercel.app

[Deployment for 34f8018 canceled]

calendso – ./apps/web

🔍 Inspect: https://vercel.com/cal/calendso/F3pjgLyTRXgSjRai7syDLMLExY41
✅ Preview: https://calendso-git-fork-buschco-fix-timezone-from-calendar-cal.vercel.app

@vercel vercel Bot temporarily deployed to Preview – docs April 3, 2022 15:23 Inactive
@buschco buschco force-pushed the fix/timezone-from-calendar branch from df06ddb to ab51eda Compare April 3, 2022 15:27
@vercel vercel Bot temporarily deployed to Preview – docs April 3, 2022 15:27 Inactive
@PeerRich PeerRich requested review from emrysal and natelindev April 4, 2022 11:34
Copy link
Copy Markdown
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Code looks good, I'll let the time wizard @emrysal have the last word.

@zomars zomars added ♻️ autoupdate tells kodiak to keep this branch up-to-date automerge labels Apr 4, 2022
@vercel vercel Bot temporarily deployed to Preview – calendso April 4, 2022 15:49 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 4, 2022 15:55 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 4, 2022 20:28 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 4, 2022 20:41 Inactive
@vercel vercel Bot temporarily deployed to Preview – calendso April 4, 2022 20:54 Inactive
@zomars zomars requested a review from alannnc April 4, 2022 20:59
@zomars
Copy link
Copy Markdown
Contributor

zomars commented Apr 4, 2022

Also @alannnc you're working on something similar right?

@kodiakhq kodiakhq Bot merged commit 2d6cb1e into calcom:main Apr 4, 2022
@emrysal
Copy link
Copy Markdown
Contributor

emrysal commented Apr 4, 2022

🤯 Timezones weren't taken into account at all! Good fix 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge ♻️ autoupdate tells kodiak to keep this branch up-to-date

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants