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

fix(dav): Prevent out-of-office event time drifts #42142

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Dec 11, 2023

Summary

This fixes both symptoms of #42139

Users whose calendar has no timezone property had wrong calendar start dates when their actual timezone has an offset from UTC. This happens for CET for example. The fix is to use the new timezone service that has more sources for a timezone and therefore higher changing of finding data.

The known downside of this fix is that in contrast to the calendar timezone, the timezone service only returns the timezone id, not the full VTIMEZONE object. Therefore the ooo events have DTSTART and DTEND with TZID=x but no VTIMEZONE component. This is not ideal but IMO a better solution than the current drifts.

How to test

  1. Clear your absence if you already had one
  2. Set your default calendar timezone to NULL in oc_calendars
  3. Schedule a new absence
  4. Open the calendar

master: event is created, but at UTC and it starts one day before absence if you live in UTC+1
here: event is created, has DTSTART and DTEND set to your timezone id and starts at the first day of absence.

Checklist

@ChristophWurst ChristophWurst added bug 3. to review Waiting for reviews feature: dav feature: caldav Related to CalDAV internals labels Dec 11, 2023
@ChristophWurst ChristophWurst added this to the Nextcloud 29 milestone Dec 11, 2023
@ChristophWurst ChristophWurst self-assigned this Dec 11, 2023
@ChristophWurst
Copy link
Member Author

/backport to stable28

$vCalendar->add($vtimezone);

/** @psalm-suppress UndefinedMethod */
$vEvent->DTSTART->setDateTime($start->setTimezone($calendarTimeZone)->setTime(0, 0));
Copy link
Member Author

Choose a reason for hiding this comment

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

the day drift happened here because we set the time above without a timezone. if the actual time is on UTC+1 and you set to 00:00 the date moves by one day. So even if we set the time again here, it's one day too early.

setting timezone first, then the time ensures that the day does not change unexpectedly

@tcitworld
Copy link
Member

Therefore the ooo events have DTSTART and DTEND with TZID=x but no VTIMEZONE component. This is not ideal but IMO a better solution than the current drifts.

Want to create an issue to have such a service that generates VTIMEZONE data? There's probably a lot of places it could be used.

@ChristophWurst
Copy link
Member Author

Want to create an issue to have such a service that generates VTIMEZONE data? There's probably a lot of places it could be used.

#42145

Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

Looks reasonable

@ChristophWurst
Copy link
Member Author

@miaulalala could you please test it, too?

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Makes sense

@miaulalala
Copy link
Contributor

Tested it, works.

@ChristophWurst ChristophWurst force-pushed the fix/dav/ooo-event-time-zone-drift branch from 8067a49 to 9abfcad Compare December 11, 2023 16:52
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 11, 2023
@ChristophWurst ChristophWurst merged commit 0907cc9 into master Dec 11, 2023
50 checks passed
@ChristophWurst ChristophWurst deleted the fix/dav/ooo-event-time-zone-drift branch December 11, 2023 17:23
@blizzz
Copy link
Member

blizzz commented Dec 11, 2023

/backport to stable28

@blizzz
Copy link
Member

blizzz commented Dec 11, 2023

accidentally merged the wrong PR, sorry. ^ recreating the backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: caldav Related to CalDAV internals feature: dav
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Absence event starts one day too early and is always set to UTC
5 participants