-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
iCalendar timezone fix in CalDAV #5091
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self review added
packages/lib/CalendarService.ts
Outdated
const buildUtcOffset = (minutes: number): string => { | ||
const h = | ||
minutes > 0 | ||
? "+" + (Math.floor(minutes / 60) < 10 ? "0" + Math.floor(minutes / 60) : Math.floor(minutes / 60)) | ||
: "-" + | ||
(Math.ceil(minutes / 60) > -10 ? "0" + Math.ceil(minutes / 60) * -1 : Math.ceil(minutes / 60) * -1); | ||
const m = minutes > 0 ? minutes % 60 : (minutes % 60) * -1; | ||
const offset = `${h}:${m}`; | ||
return offset; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply takes in the offset in minutes and returns the offset in the format of +-hh:mm
. This is done to satisfy the needs of our beloved ICAL parser to adjust the timezone of the event's start and end date and to avoid the GMT shift caused in the absence of vTimezone
if (!vcalendar.getFirstSubcomponent("vtimezone") && tzid) { | ||
const timezoneComp = new ICAL.Component("vtimezone"); | ||
timezoneComp.addPropertyWithValue("tzid", tzid); | ||
const standard = new ICAL.Component("standard"); | ||
// get timezone offset | ||
const tzoffsetfrom = buildUtcOffset(dayjs(event.startDate.toJSDate()).tz(tzid, true).utcOffset()); | ||
const tzoffsetto = buildUtcOffset(dayjs(event.endDate.toJSDate()).tz(tzid, true).utcOffset()); | ||
// set timezone offset | ||
standard.addPropertyWithValue("tzoffsetfrom", tzoffsetfrom); | ||
standard.addPropertyWithValue("tzoffsetto", tzoffsetto); | ||
// provide a standard dtstart | ||
standard.addPropertyWithValue("dtstart", "1601-01-01T00:00:00"); | ||
timezoneComp.addSubcomponent(standard); | ||
vcalendar.addSubcomponent(timezoneComp); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If vTimezone
is absent and tzid
is present, we "build" the vTimezone
component from the tzid
and make sure to include the dtstart
, tzoffsetfrom
, and tzoffsetto
in the subcomponent standard
and then add it to the vCalendar
component to ensure proper timezone conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might still require consideration of daylight savings but I believe I'd need to add that as a follow up
standard.addPropertyWithValue("tzoffsetfrom", tzoffsetfrom); | ||
standard.addPropertyWithValue("tzoffsetto", tzoffsetto); | ||
// provide a standard dtstart | ||
standard.addPropertyWithValue("dtstart", "1601-01-01T00:00:00"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you get to 1601?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I just monitored a few calls and this seemed like a standard dtstart. It doesn't really matter as long as it is before the earliest start time of the calendar.
Co-authored-by: Alex van Andel <me@alexvanandel.com>
standard.addPropertyWithValue("dtstart", "1601-01-01T00:00:00"); | ||
timezoneComp.addSubcomponent(standard); | ||
vcalendar.addSubcomponent(timezoneComp); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know what sorcery is going on in this PR @alishaz-polymath @emrysal 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pleasing the Timezone Masters 🥲
Still having the issue with iCalendar timezone. Running on Docker, the commits are already in the latest as far as I can see. Anyone els can confirm this? |
Hey @davidwuest can you elaborate on the exact issue? There is still daylight issue to be taken care of so if you’re testing it post October dates, there will still be a discrepancy of 1 hour. If you’re testing in October, and with our latest main pull, please let me know. |
@alishaz-polymath I was only testing within October. Checked that the latest main pull was in the docker build. I will make another build, without any cache to see if that is the issue. Let me know what I can do to support to fix this issue. |
Can you please book me here and we can then take care of it on a call, explore it further |
What does this PR do?
Adds support for timezone change when vTimezone is not provided in the calendar object, but tzid is provided. This is the case with iCalendar, along with a few other service providers.
Fixes #2139
Loom
Environment: Staging(main branch)
Type of change
How should this be tested?
Checklist