Skip to content

fix(caldav): Fix reminder timezone drift for all-day events - #36192

Merged
ChristophWurst merged 1 commit into
masterfrom
fix/caldav-reminders/fix-timezone-drift-all-day-reminders
Feb 9, 2023
Merged

fix(caldav): Fix reminder timezone drift for all-day events#36192
ChristophWurst merged 1 commit into
masterfrom
fix/caldav-reminders/fix-timezone-drift-all-day-reminders

Conversation

@ChristophWurst

@ChristophWurstChristophWurst commented Jan 17, 2023

Copy link
Copy Markdown
Member

Contributes to nextcloud/calendar#4778

Summary

All day events contain no timezone information. Yet reminders are specified relative to the event start. Each calendar has an optional timezone. If it's set, we should use that to determine the correct UTC timestamp for the reminder. As a fallback we should use UTC.

Ref https://www.rfc-editor.org/rfc/rfc4791#section-5.2.2

 The CALDAV:calendar-timezone property is used to
specify the time zone the server should rely on to resolve "date"
values and "date with local time" values (i.e., floating time) to
"date with UTC time" values. The server will require this
information to determine if a calendar component scheduled with
"date" values or "date with local time" values overlaps a CALDAV:
time-range specified in a CALDAV:calendar-query REPORT.

TODO

  • Test all day events when calendar has timezone set
  • Test all day events when calendar does not have timezone set
  • Test event when calendar has timezone set
  • Test event when calendar does not have timezone set

Checklist

Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
@blizzzblizzz mentioned this pull request Feb 1, 2023
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
Comment threadapps/dav/lib/CalDAV/Reminder/ReminderService.php Fixed
@ChristophWurstChristophWurst added bug 3. to review Waiting for reviews feature: dav feature: caldav Related to CalDAV internals and removed 2. developing Work in progress labels Feb 8, 2023
@ChristophWurst
ChristophWurst marked this pull request as ready for review February 8, 2023 13:32
@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

/backport to stable25

@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

/backport to stable24

@miaulalalamiaulalala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@st3inyst3iny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested and works. Nice one!

Setting the calendar timezone is not possible from our UI yet, right?

@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

Setting the calendar timezone is not possible from our UI yet, right?

Correct. Coming with nextcloud/calendar#4935 hopefully

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
ChristophWurstforce-pushed the fix/caldav-reminders/fix-timezone-drift-all-day-reminders branch from bbe17e6 to b5f7afdCompareFebruary 9, 2023 14:19
@ChristophWurstChristophWurst changed the title fix(CalDAV reminders): Fix timezone drift for all-day eventsfix(caldav): Fix reminder timezone drift for all-day eventsFeb 9, 2023
@ChristophWurstChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 9, 2023
@tcitworldtcitworld mentioned this pull request Feb 9, 2023
5 tasks
@ChristophWurst
ChristophWurst merged commit 7aa7868 into masterFeb 9, 2023
@ChristophWurst
ChristophWurst deleted the fix/caldav-reminders/fix-timezone-drift-all-day-reminders branch February 9, 2023 18:28
@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

/backport to stable24

@Luncheon3462

Copy link
Copy Markdown

I’m just an end user, not a dev but running NC 26.0.3. The time zone drift for all day event reminders still occurs for me. Doesn’t matter if I use the personal calendar that is automatically created or a new calendar that I create. My time zone is set correctly in the nc calendar app settings app. Is this expected still?

Luncheon3462

This comment was marked as off-topic.

@st3iny

Copy link
Copy Markdown
Member

@Luncheon3462 That is unfortunate. Please open a new issue and follow the bug template so that we can track and investigate the issue.

@Luncheon3462

Luncheon3462 commented Jul 31, 2023

Copy link
Copy Markdown

@st3iny I'll do my best. Not a developer or github user. Only created an account to let you, @ChristophWurst@tcitworld@miaulalala know that the bug may still be present. I also posted on the nextcloud forum, and at least one other person is reporting the same. That thread is at https://help.nextcloud.com/t/calendar-timezone-bug-36192/167212

@Luncheon3462

Copy link
Copy Markdown

@st3iny i did my best. see report at #39640.

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

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: caldavRelated to CalDAV internalsfeature: dav

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants

@ChristophWurst@Luncheon3462@st3iny@tcitworld@miaulalala@github-advanced-security@skjnldsv