search

nextcloud / calendar

📆 Calendar app for Nextcloud
https://apps.nextcloud.com/apps/calendar
GNU Affero General Public License v3.0
968 stars 236 forks source link

Reminder sent relative to server time because all day events have no TZID #4778

Open ChristophWurst opened 1 year ago

ChristophWurst commented 1 year ago

Steps to reproduce

  1. Have a server in another timezone
  2. Create an all day event
  3. Add a reminder x hours before

Expected behavior

Reminder x hours before the day starts for me

Actual behaviour

Reminder x hours before the day starts for the server

Calendar app version

4.x

CalDAV-clients used

No response

Browser

No response

Client operating system

No response

Server operating system

No response

Web server

No response

Database engine version

No response

PHP engine version

No response

Nextcloud version

24

Updated from an older installed version or fresh install

No response

List of activated apps

No response

Nextcloud configuration

No response

Web server error log

No response

Log file

No response

Browser log

No response

Additional info

No response

miaulalala commented 1 year ago

The reminder itself could contain a proper DATE-TIME value with a timezone:

https://www.kanzaki.com/docs/ical/trigger.html

st3iny commented 1 year ago

@miaulalala How would we implement it for relative alarms? Just save them as absolute reminders for all-day events?

miaulalala commented 1 year ago

Fixes need to be backported to stable22 and calendar3.5

st3iny commented 1 year ago

This can be fixed by moving the timezone settings from the browser local storage to the database. There already is a timezone select in the bottom left settings menu.

This task can be split into two sub tasks:

  1. Save user timezone in the database via an app config or preference. (Fall back to current default: automatic selection.) This also brings the benefit of syncing the preferred timezone across all devices. Currently, they have to be separately configured on each device/browser.
  2. Enhance the notification cron job to use the user defined timezone. If none is configured, use the timezone of the server. This fallback also ensures a safe backport, because we can just fallback to the current behavior.

Mentioning the default behavior in our Calendar user documentation is also advisable. Notifications for all day events only work correctly if a timezone is configured or the user is in the same timezone as the server.

tcitworld commented 1 year ago

There's a CALDAV:calendar-timezone property that's already used internally by SabreDAV free-busy stuff.

ChristophWurst commented 1 year ago

Then https://github.com/sabre-io/dav/blob/1ddd7733a3dbec1880f695e979e9272dfb0795aa/lib/CalDAV/ICSExportPlugin.php#L231-L247 looks like a reasonable logic to determine the tz.

ChristophWurst commented 1 year ago

I just can't find code that ever sets the property.

st3iny commented 1 year ago

I just realized that the oc_calendars table contains a timezone column, although it is null for most calendars on my dev instance. I wonder if it is related to this property.

ChristophWurst commented 1 year ago

Based on the Sabre code I would say yes, it's the same property.

tcitworld commented 1 year ago

It's the clients that set the property through CalDAV, but we can set it programmatically as well.

ChristophWurst commented 1 year ago

I'm able to set my calendar's TZ to Asia/Shanghai and create an all day event for Saturday. My default reminder is 1h before the event.

With https://github.com/nextcloud/server/pull/36192 I get a reminder at 1674226800 = Fri Jan 20 23:00:00 CST 2023 With master I get a reminder at 1674255600 = Sat Jan 21 07:00:00 CST 2023

That is what we expect, right?

ChristophWurst commented 1 year ago

I would split this topic into the following pieces

1) Use the calendar's timezone data, if available, to generate correct reminder dates for all day events 2) Make sure our frontend sets a timezone for new calendars 3) Add a UI option to change the timezone of a calendar

tcitworld commented 1 year ago

https://github.com/nextcloud/server/pull/36001 made me think we also already have an core app config named timezone which we could set as default for the user's new calendars if none is set (including by this front-end).

ChristophWurst commented 1 year ago
  1. Make sure our frontend sets a timezone for new calendars

That is already the case thanks to Georg and his https://github.com/nextcloud/calendar/pull/1581

ChristophWurst commented 1 year ago

Apparently timezone is set for calendars created from the UI but not the auto-generated personal calendar. Something to investigate and fix, too.

tcitworld commented 1 year ago

Indeed https://github.com/nextcloud/server/blob/1fed79982669f6e691ac3c799af1281662f0d2e9/apps/dav/lib/HookManager.php#L177-L181

tcitworld commented 1 year ago

However I don't think we have a way to generate VTIMEZONE data from a TZID on PHP side…

tcitworld commented 1 year ago

https://github.com/nextcloud/server/pull/36522 just reuses data we have in calendar-js.

StyXman commented 1 year ago

I know what I'm going to ask is crazy, but can we also set the TZ of both the start and end times independently? We would like to use the calendar for out employees flights, and they cross timezones often. Or should I open a new issue for that?

miaulalala commented 1 year ago

I know what I'm going to ask is crazy, but can we also set the TZ of both the start and end times independently? We would like to use the calendar for out employees flights, and they cross timezones often. Or should I open a new issue for that?

I think there's already a provision for a use case like that because of summer / winter time, at least that's what I gather from the RFC. Please open a ticket and I will look further into it (although it might take some time).

tcitworld commented 1 year ago

For clarification, apart from the reminders handling, you can already set different timezones for start and end times.