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

MKCALENDAR: send VTIMEZONE in calendar-timezone #1044

Merged
merged 22 commits into from
Oct 8, 2024

Conversation

ArnyminerZ
Copy link
Member

Purpose

See #1031

Short description

  • Timezone id is now written to CalendarTimezoneId
  • ID is converted to VTIMEZONE using getVTimeZone, and if successful, append as CalendarTimezone

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Sep 25, 2024
@ArnyminerZ ArnyminerZ self-assigned this Sep 25, 2024
@ArnyminerZ ArnyminerZ linked an issue Sep 25, 2024 that may be closed by this pull request
2 tasks
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ

This comment was marked as outdated.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ

This comment was marked as outdated.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ
Copy link
Member Author

Alright, now it's good:

<?xml version="1.0" encoding="UTF-8"?>
<CAL:mkcalendar xmlns="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CARD="urn:ietf:params:xml:ns:carddav">
  <set>
    <prop>
      <resourcetype>
        <collection/>
        <CAL:calendar/>
      </resourcetype>
      <displayname>Calendar with timezone</displayname>
      <CAL:calendar-description/>
      <n0:calendar-color xmlns:n0="http://apple.com/ns/ical/">#B22222FF</n0:calendar-color>
      <CAL:calendar-timezone-id>Africa/Algiers</CAL:calendar-timezone-id>
      <CAL:calendar-timezone>BEGIN:VTIMEZONE
        TZID:Africa/Algiers
        LAST-MODIFIED:20240422T053450Z
        TZURL:https://www.tzurl.org/zoneinfo/Africa/Algiers
        X-LIC-LOCATION:Africa/Algiers
        X-PROLEPTIC-TZNAME:LMT
        BEGIN:STANDARD
        TZNAME:PMT
        TZOFFSETFROM:+001212
        TZOFFSETTO:+000921
        DTSTART:18910316T011444
        END:STANDARD
        BEGIN:STANDARD
        TZNAME:WET
        TZOFFSETFROM:+000921
        TZOFFSETTO:+0000
        DTSTART:19110311T010000
        END:STANDARD
        BEGIN:DAYLIGHT
        TZNAME:WEST
        TZOFFSETFROM:+0000
        TZOFFSETTO:+0100
        DTSTART:19160615T000000
        RDATE:19170325T000000
        RDATE:19180310T000000
        RDATE:19190302T000000
        RDATE:19200215T000000
        RDATE:19210315T000000
        RDATE:19390912T000000
        RDATE:19710425T230000
        RDATE:19770506T000000
        RDATE:19800425T000000
        END:DAYLIGHT
        BEGIN:STANDARD
        TZNAME:WET
        TZOFFSETFROM:+0100
        TZOFFSETTO:+0000
        DTSTART:19161002T010000
        RRULE:FREQ=YEARLY;UNTIL=19191005T230000Z;BYMONTH=10;BYMONTHDAY=2,3,4,5,6,7,8;BYDAY=MO
        END:STANDARD
        BEGIN:STANDARD
        TZNAME:WET
        TZOFFSETFROM:+0100
        TZOFFSETTO:+0000
        DTSTART:19201024T010000
        RDATE:19210622T010000
        RDATE:19391119T020000
        RDATE:19461007T000000
        RDATE:19630414T000000
        RDATE:19710927T000000
        RDATE:19791026T000000
        RDATE:19801031T020000
        END:STANDARD
        BEGIN:STANDARD
        TZNAME:CET
        TZOFFSETFROM:+0000
        TZOFFSETTO:+0100
        DTSTART:19400225T030000
        RDATE:19560129T000000
        RDATE:19810501T000000
        END:STANDARD
        BEGIN:DAYLIGHT
        TZNAME:CEST
        TZOFFSETFROM:+0100
        TZOFFSETTO:+0200
        DTSTART:19440403T020000
        RRULE:FREQ=YEARLY;UNTIL=19450402T010000Z;BYMONTH=4;BYDAY=1MO
        END:DAYLIGHT
        BEGIN:STANDARD
        TZNAME:CET
        TZOFFSETFROM:+0200
        TZOFFSETTO:+0100
        DTSTART:19441008T020000
        RDATE:19450916T010000
        RDATE:19780922T030000
        END:STANDARD
        BEGIN:STANDARD
        TZNAME:CET
        TZOFFSETFROM:+0100
        TZOFFSETTO:+0100
        DTSTART:19771021T000000
        END:STANDARD
        BEGIN:DAYLIGHT
        TZNAME:CEST
        TZOFFSETFROM:+0100
        TZOFFSETTO:+0200
        DTSTART:19780324T010000
        END:DAYLIGHT
        END:VTIMEZONE
      </CAL:calendar-timezone>
    </prop>
  </set>
</CAL:mkcalendar>

@ArnyminerZ ArnyminerZ marked this pull request as ready for review September 25, 2024 12:04
@ArnyminerZ ArnyminerZ requested a review from sunkup September 25, 2024 12:04
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Did not check the code, but in your last example the VTIMEZONE is not wrapped in an icalendar object VCALENDAR as the standard says it should be.

From the spec on calendar-timezone
https://www.rfc-editor.org/rfc/rfc4791#section-5.2.2

5.2.2.  CALDAV:calendar-timezone Property

   Name:  calendar-timezone
...
"an iCalendar object with exactly one VTIMEZONE component.

See also the example in section 5.2.2.

@ArnyminerZ
Copy link
Member Author

but in your last example the VTIMEZONE is not wrapped in an icalendar object VCALENDAR as the standard says it should be.

Yup, my bad, I misread that. I'll update it asap

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ
Copy link
Member Author

Okay, should be fixed now.

<?xml version='1.0' encoding='UTF-8' ?>
<CAL:mkcalendar
	xmlns="DAV:"
	xmlns:CAL="urn:ietf:params:xml:ns:caldav"
	xmlns:CARD="urn:ietf:params:xml:ns:carddav">
	<set>
		<prop>
			<resourcetype>
				<collection />
				<CAL:calendar />
			</resourcetype>
			<displayname>Calendar with Timezone</displayname>
			<CAL:calendar-description></CAL:calendar-description>
			<n0:calendar-color
				xmlns:n0="http://apple.com/ns/ical/">#DCDCDCFF
			</n0:calendar-color>
			<CAL:calendar-timezone-id>Africa/Asmera</CAL:calendar-timezone-id>
			<CAL:calendar-timezone>BEGIN:VCALENDAR
                 BEGIN:VTIMEZONE
                 TZID:Africa/Asmera
                 BEGIN:STANDARD
                 TZNAME:+0230
                 TZOFFSETFROM:+022716
                 TZOFFSETTO:+0230
                 DTSTART:19080501T010000
                 END:STANDARD
                 BEGIN:STANDARD
                 TZNAME:EAT
                 TZOFFSETFROM:+0230
                 TZOFFSETTO:+0300
                 DTSTART:19280701T010000
                 END:STANDARD
                 BEGIN:STANDARD
                 TZNAME:+0230
                 TZOFFSETFROM:+0300
                 TZOFFSETTO:+0230
                 DTSTART:19300105T010000
                 END:STANDARD
                 BEGIN:STANDARD
                 TZNAME:+0245
                 TZOFFSETFROM:+0230
                 TZOFFSETTO:+0245
                 DTSTART:19370101T010000
                 END:STANDARD
                 BEGIN:STANDARD
                 TZNAME:EAT
                 TZOFFSETFROM:+0245
                 TZOFFSETTO:+0300
                 DTSTART:19420801T000000
                 END:STANDARD
                 END:VTIMEZONE
                 END:VCALENDAR
                 </CAL:calendar-timezone>
		</prop>
	</set>
</CAL:mkcalendar>

@ArnyminerZ ArnyminerZ requested a review from sunkup September 30, 2024 11:20
@sunkup sunkup requested a review from rfc2822 October 1, 2024 08:40
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

At some time we should probably replace the timezone in the DB by a timezoneId, too.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ
Copy link
Member Author

At some time we should probably replace the timezone in the DB by a timezoneId, too.

We should do it on a separate PR then, no?

@ArnyminerZ ArnyminerZ requested a review from rfc2822 October 2, 2024 05:28
@rfc2822
Copy link
Member

rfc2822 commented Oct 2, 2024

We should do it on a separate PR then, no?

Yes because it would also require a DB migration. Originally I wanted the VTIMEZONE because calendar servers might have a non-tzdb timezone, but we can't make use of that anyway. And I think ical4j 4.x also only uses tzdb timezones (will have to check).

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@rfc2822 rfc2822 force-pushed the 1031-mkcalendar-send-vtimezone-in-calendar-timezone branch from 816bcb2 to e958378 Compare October 2, 2024 10:56
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Nice, looks correct!

However I have tried with Fastmail again (which was the original reason why the problem came up).

And while the new MKCALENDAR request seems correct to me, Fastmail answers

<?xml version="1.0" encoding="utf-8"?>
<multistatus xmlns="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CARD="urn:ietf:params:xml:ns:carddav" xmlns:n0="http://apple.com/ns/ical/">
  <propstat>
    <prop>
      <CAL:calendar-timezone-id/>
    </prop>
    <status>HTTP/1.1 403 Forbidden</status>
  </propstat>
  <propstat>
    <prop>
      <resourcetype/>
      <displayname/>
      <CAL:calendar-description/>
      <n0:calendar-color/>
      <CAL:calendar-timezone/>
    </prop>
    <status>HTTP/1.1 424 Failed Dependency</status>
  </propstat>
</multistatus>

Can you have a look at that? Is it maybe forbidden to set the calendar-timezone-id or do we have some error? And does it work when

  • only the calendar-timezone (this time correct),
  • or only the calendar-timezone-id is set,
  • and is the error even reproducible for you when you set both (as with the PR in its current condition)

@ArnyminerZ
Copy link
Member Author

I have also tried creating a calendar with timezone, which generates the following:

Request
<?xml version='1.0' encoding='UTF-8' ?>
<CAL:mkcalendar
	xmlns="DAV:"
	xmlns:CAL="urn:ietf:params:xml:ns:caldav"
	xmlns:CARD="urn:ietf:params:xml:ns:carddav">
	<set>
		<prop>
			<resourcetype>
				<collection />
				<CAL:calendar />
			</resourcetype>
			<displayname>Calendar with timezone</displayname>
			<CAL:calendar-description></CAL:calendar-description>
			<n0:calendar-color
				xmlns:n0="http://apple.com/ns/ical/">#FFFAFAFF
			</n0:calendar-color>
			<CAL:calendar-timezone-id>Africa/Bamako</CAL:calendar-timezone-id>
			<CAL:calendar-timezone>BEGIN:VCALENDAR
BEGIN:VTIMEZONE
TZID:Africa/Bamako
BEGIN:STANDARD
TZNAME:GMT
TZOFFSETFROM:+001608
TZOFFSETTO:+0000
DTSTART:19120101T000000
END:STANDARD
END:VTIMEZONE
END:VCALENDAR

                 </CAL:calendar-timezone>
		</prop>
	</set>
</CAL:mkcalendar>

but the answer is still

Response
<?xml version="1.0" encoding="utf-8"?>
<multistatus
	xmlns="DAV:"
	xmlns:CAL="urn:ietf:params:xml:ns:caldav"
	xmlns:CARD="urn:ietf:params:xml:ns:carddav"
	xmlns:n0="http://apple.com/ns/ical/">
	<propstat>
		<prop>
			<CAL:calendar-timezone-id/>
		</prop>
		<status>HTTP/1.1 403 Forbidden</status>
	</propstat>
	<propstat>
		<prop>
			<resourcetype/>
			<displayname/>
			<CAL:calendar-description/>
			<n0:calendar-color/>
			<CAL:calendar-timezone/>
		</prop>
		<status>HTTP/1.1 424 Failed Dependency</status>
	</propstat>
</multistatus>

@ArnyminerZ
Copy link
Member Author

I've seen that fastmail doesn't support choosing a timezone for the calendar you are creating from the UI:

image

Is it possible that this is simply not allowed? They are returning a 403, which may mean that the operation is forbidden, which would be pretty self-explanatory. I couldn't find any information on what features does Fastmail support or not support on CalDAV

@rfc2822
Copy link
Member

rfc2822 commented Oct 2, 2024

Does it work without time zone? If yes, we could also add a "no preferred timezone" option, or ask the Fastmail people again here: #860 (comment).

I think it should work, otherwise we should Rob have anwerered with the details about the timezone definition?

@ArnyminerZ
Copy link
Member Author

I've tried setting the default timezone to null (which displays as - in the UI), and creating a new calendar with it

Request
<?xml version='1.0' encoding='UTF-8' ?>
<CAL:mkcalendar
	xmlns="DAV:"
	xmlns:CAL="urn:ietf:params:xml:ns:caldav"
	xmlns:CARD="urn:ietf:params:xml:ns:carddav">
	<set>
		<prop>
			<resourcetype>
				<collection />
				<CAL:calendar />
			</resourcetype>
			<displayname>Calendar without timezone</displayname>
			<CAL:calendar-description></CAL:calendar-description>
			<n0:calendar-color
				xmlns:n0="http://apple.com/ns/ical/">#DC143CFF
			</n0:calendar-color>
		</prop>
	</set>
</CAL:mkcalendar>

And it gets created correctly (returns 201 empty response).

@ArnyminerZ
Copy link
Member Author

I'll ask Rob and see if he has anything to say

@rfc2822
Copy link
Member

rfc2822 commented Oct 3, 2024

I'll ask Rob and see if he has anything to say

For reference: #860 (reply in thread)

@rfc2822
Copy link
Member

rfc2822 commented Oct 7, 2024

So, shall we

  • include the calendar-timezone-id by default,
  • be content with the fixed calendar-timezone,
  • and/or change the default timezone to (I don't prefer that, because RFC 4791 says that the time zone is an important property for various calculations and it should be set if possible).

Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Without calendar-timezone-id it works with Fastmail, right?

So I suggest to comment out the timezone-id for now and check again when we do #1052.

@rfc2822 rfc2822 added bug Something isn't working and removed enhancement New feature or request labels Oct 7, 2024
@ArnyminerZ
Copy link
Member Author

I'd just change the default value for timezone in the state of the viewmodel to null. Everything is handled already, it just won't set any timezone. Maybe adding a text below saying something like "if not set, will use the timezone of your server". It will be really strange that someone manually picks a timezone, since we don't have any search feature, so it's a bit inconvenient. Then, if someone actually uses it, it's up to the server to work or not.

@rfc2822
Copy link
Member

rfc2822 commented Oct 8, 2024

Hm… we don't use floating time at all, and it seems to be rarely used. The timezone is only used for floating time calculations (and even then only when the specific request doesn't set a timezone), so … agreed, let's

  • choose "–" by default,
  • when a timezone is set, send calendar-timezone and calendar-timezone-id.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ
Copy link
Member Author

@rfc2822 Should be ready then

@ArnyminerZ ArnyminerZ requested a review from rfc2822 October 8, 2024 13:14
@rfc2822
Copy link
Member

rfc2822 commented Oct 8, 2024

Nice 👍🏻

@rfc2822 rfc2822 merged commit 5b54c9d into main-ose Oct 8, 2024
8 checks passed
@rfc2822 rfc2822 deleted the 1031-mkcalendar-send-vtimezone-in-calendar-timezone branch October 8, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MKCALENDAR: send VTIMEZONE in calendar-timezone
3 participants