feat(caldav): Sync Reminders / VALARM #1415

Merged
konrad merged 30 commits from ce72/api:1408_caldav_alarms into main 2023-04-01 11:09:13 +00:00
Contributor

Relates to vikunja/api#1408

Relates to https://kolaente.dev/vikunja/api/issues/1408
ce72 changed title from feat(caldav): Sync Reminders / VALARM to WIP: feat(caldav): Sync Reminders / VALARM 2023-03-04 12:39:40 +00:00
Author
Contributor
See https://kolaente.dev/vikunja/api/issues/1408, https://kolaente.dev/vikunja/api/issues/1416
ce72 force-pushed 1408_caldav_alarms from 9ff547da16 to 30c1d698e9 2023-03-16 13:14:56 +00:00 Compare
ce72 added 1 commit 2023-03-16 16:10:42 +00:00
feat: convert Reminders to VALARMs
Some checks failed
continuous-integration/drone/pr Build is failing
06508f0287
ce72 added 1 commit 2023-03-16 20:29:21 +00:00
feat: convert VALARMs to Reminders
Some checks failed
continuous-integration/drone/pr Build is failing
32cb200f87
ce72 added 1 commit 2023-03-17 07:24:02 +00:00
fix: lint errors
Some checks failed
continuous-integration/drone/pr Build is failing
4018bd6078
ce72 added 17 commits 2023-03-26 09:23:26 +00:00
fix(deps): update module github.com/imdario/mergo to v0.3.14
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
769db0dab2
fix: update redoc
Some checks failed
continuous-integration/drone/push Build is failing
8916de0366
fix(lint): disable misspell linter on redoc
All checks were successful
continuous-integration/drone/push Build is passing
a0b3a444df
fix(deps): update github.com/arran4/golang-ical digest to 19abf92
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
085b9222bb
chore(deps): update goreleaser/nfpm docker tag to v2.27.1 (#1438)
All checks were successful
continuous-integration/drone/push Build is passing
6d8db0ce1e
Co-authored-by: kolaente <k@knt.li>
Reviewed-on: vikunja/api#1438
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
fix(deps): update module github.com/swaggo/swag to v1.8.11
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
6566f0e81d
chore: rename files (fix typo)
Some checks failed
continuous-integration/drone/push Build is failing
6aadaaaffc
fix(deps): update module github.com/imdario/mergo to v0.3.15 (#1443)
All checks were successful
continuous-integration/drone/push Build is passing
f6e6c5c8fc
Reviewed-on: vikunja/api#1443
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
chore(deps): update golangci-lint to 1.52.1
Some checks failed
continuous-integration/drone/push Build is failing
4d5ad8f50e
fix: update xgo in dockerfile to 1.20.2
All checks were successful
continuous-integration/drone/push Build is passing
33f0d0f85a
Resolves vikunja/api#1445
fix(caldav): Do not create label if it exists by title (#1444)
All checks were successful
continuous-integration/drone/push Build is passing
5961e56d16
Resolves vikunja/api#1435

Co-authored-by: ce72 <christoph.ernst72@googlemail.com>
Reviewed-on: vikunja/api#1444
Co-authored-by: cernst <ce72@noreply.kolaente.de>
Co-committed-by: cernst <ce72@noreply.kolaente.de>
fix: add missing license header
All checks were successful
continuous-integration/drone/push Build is passing
f4e12dab27
fix(deps): update github.com/gocarina/gocsv digest to 9a18a84
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
84c3d0ef6d
Merge branch 'main' into 1408_caldav_alarms
All checks were successful
continuous-integration/drone/pr Build is passing
4b35f83278
ce72 added 2 commits 2023-03-27 20:33:54 +00:00
fix(caldav): Do not create label if it exists by title
All checks were successful
continuous-integration/drone/pr Build is passing
6abd212c57
Resolves vikunja/api#1435
Merge branch 'main' into 1408_caldav_alarms
Some checks failed
continuous-integration/drone/pr Build is failing
69ba1d16f0
ce72 added 1 commit 2023-03-27 20:46:27 +00:00
chore: merge upstream changes
All checks were successful
continuous-integration/drone/pr Build is passing
9d67a5f628
ce72 added 1 commit 2023-03-27 20:50:24 +00:00
chore: merge upstream changes
All checks were successful
continuous-integration/drone/pr Build is passing
7bd6e910c3
ce72 changed title from WIP: feat(caldav): Sync Reminders / VALARM to feat(caldav): Sync Reminders / VALARM 2023-03-27 21:16:47 +00:00
konrad reviewed 2023-03-28 10:38:55 +00:00
@ -74,2 +62,4 @@
type Alarm struct {
Time time.Time
Duration time.Duration
RelativeTo string
Owner

I think this should be a string enum (yes, Go does not have real enums but the const type thing)

I think this should be a string enum (yes, Go does not have real enums but the const type thing)
Author
Contributor

ok

ok
@ -133,0 +166,4 @@
}
case len(property.ICalParameters["RELATED"]) > 0:
duration := parseDuration(property.Value)
switch property.ICalParameters["RELATED"][0] {
Owner

Doesn't calDAV have a concept of a due date? And if that's the case, can't we use that as a relative anchor point?

Doesn't calDAV have a concept of a due date? And if that's the case, can't we use that as a relative anchor point?
Author
Contributor

I modified the implementation to follow more closely the CalDav standard
https://icalendar.org/iCalendar-RFC-5545/3-8-6-3-trigger.html

I also took a look at tasks.org CalDav implementation https://github.com/tasks/tasks/blob/main/app/src/main/java/org/tasks/caldav/extensions/VAlarm.kt

Can you please look at it again?

I modified the implementation to follow more closely the CalDav standard https://icalendar.org/iCalendar-RFC-5545/3-8-6-3-trigger.html I also took a look at tasks.org CalDav implementation https://github.com/tasks/tasks/blob/main/app/src/main/java/org/tasks/caldav/extensions/VAlarm.kt Can you please look at it again?
Owner

I think this is fine now.

I think this is fine now.
@ -156,0 +217,4 @@
var durationRegex = regexp.MustCompile(`([-+])?P([\d\.]+Y)?([\d\.]+M)?([\d\.]+D)?T?([\d\.]+H)?([\d\.]+M)?([\d\.]+?S)?`)
// ParseDuration converts a ISO8601 duration into a time.Duration
func parseDuration(str string) time.Duration {
Owner

We have this exact function already in the ticktick migration. Can you move this in a general helper function and use the same function in both places?

We have this exact function already in the ticktick migration. Can you move this in a general helper function and use the same function in both places?
Author
Contributor

ok

ok
ce72 added 1 commit 2023-03-29 19:49:54 +00:00
fix: review findings
All checks were successful
continuous-integration/drone/pr Build is passing
3e49c27ad1
ce72 changed title from feat(caldav): Sync Reminders / VALARM to WIP: feat(caldav): Sync Reminders / VALARM 2023-03-29 20:15:02 +00:00
ce72 added 1 commit 2023-03-29 21:37:44 +00:00
fix: match reminder paring with caldav spec and tasks.org code
All checks were successful
continuous-integration/drone/pr Build is passing
287a21da93
Follow the RFC more strictly:
https://icalendar.org/iCalendar-RFC-5545/3-8-6-3-trigger.html

Compare with tasks.org, to make sure their alarms will be correctly synced:
https://github.com/tasks/tasks/blob/main/app/src/main/java/org/tasks/caldav/extensions/VAlarm.kt
ce72 changed title from WIP: feat(caldav): Sync Reminders / VALARM to feat(caldav): Sync Reminders / VALARM 2023-03-29 21:50:43 +00:00
konrad reviewed 2023-03-31 13:52:28 +00:00
@ -260,0 +207,4 @@
case models.ReminderRelationStartDate:
caldavalarms += `
TRIGGER;RELATED=START:` + makeCalDavDuration(a.Duration)
case models.ReminderRelationEndDate, models.ReminderRelationDueDate:
Owner

Won't this add a reminder relative to the end date when there's a due date but no end date?

Won't this add a reminder relative to the end date when there's a due date but no end date?
Author
Contributor

Yes, that's intended. CalDav standard does not know RELATED=DUE or something like that. The same is true for the implementation at tasks.org.
According to the standard https://icalendar.org/iCalendar-RFC-5545/3-8-6-3-trigger.html due dates are backed by triggers set relative to "END".

After looking more closely at it, there is maybe an issue in the existing code. The VTODO element does not know DTEND at all. I did not change that, but this implies that we don't need ReminderRelationEndDate (we won't be able to sync it anyway, because clients will ignore it.)
There seems to be room for more cleanup PRs... But I think the statement you commented is not incorrect by itself.

Yes, that's intended. CalDav standard does not know RELATED=DUE or something like that. The same is true for the implementation at tasks.org. According to the standard https://icalendar.org/iCalendar-RFC-5545/3-8-6-3-trigger.html due dates are backed by triggers set relative to "END". After looking more closely at it, there is maybe an issue in the existing code. The VTODO element does not know DTEND at all. I did not change that, but this implies that we don't need ReminderRelationEndDate (we won't be able to sync it anyway, because clients will ignore it.) There seems to be room for more cleanup PRs... But I think the statement you commented is not incorrect by itself.
Owner

Okay, that makes sense. Thanks for the explanation.

The VTODO element does not know DTEND at all. I did not change that, but this implies that we don't need ReminderRelationEndDate (we won't be able to sync it anyway, because clients will ignore it.)

So if a users sets a reminder to be relative to the end date in the frontend, it will be ignored by the CalDAV clients?

There seems to be room for more cleanup PRs

There almost certainly is!

Okay, that makes sense. Thanks for the explanation. > The VTODO element does not know DTEND at all. I did not change that, but this implies that we don't need ReminderRelationEndDate (we won't be able to sync it anyway, because clients will ignore it.) So if a users sets a reminder to be relative to the end date in the frontend, it will be ignored by the CalDAV clients? > There seems to be room for more cleanup PRs There almost certainly is!
Author
Contributor

So if a users sets a reminder to be relative to the end date in the frontend, it will be ignored by the CalDAV clients?

We would serialize this like

BEGIN:VTODO
DTEND=<timestamp>
...
BEGIN:VALARM
TRIGGER;RELATED=END:<duration>
...

The client will most certainly ignore DTEND and the alarm might get lost. If the task additionally has a due_date given in the frontend, the client will relate the trigger to this date, transferred as DUE.

The other direction probably will not happen: clients will never send DTEND (and the parsing code ignores it anyway..)
If they send DUE=<timestamp> and TRIGGER;RELATED=END:<duration> then we will create a reminder relative to the due date (which might be the 95% scenario).

I am looking forward to testing this with tasks.org once it's in the unstable version. Then maybe we need another iteration. And maybe even some edge cases will never work at all, because Vikunjas model and CalDav do not overlap by 100%. I think in part it is tolerable as long as reminders relative to due_date and then to start_date work as expected, and we understand (and document) what's going on.

> So if a users sets a reminder to be relative to the end date in the frontend, it will be ignored by the CalDAV clients? > We would serialize this like ``` BEGIN:VTODO DTEND=<timestamp> ... BEGIN:VALARM TRIGGER;RELATED=END:<duration> ... ``` The client will most certainly ignore DTEND and the alarm might get lost. If the task additionally has a due_date given in the frontend, the client will relate the trigger to this date, transferred as DUE. The other direction probably will not happen: clients will never send DTEND (and the parsing code ignores it anyway..) If they send `DUE=<timestamp>` and `TRIGGER;RELATED=END:<duration>` then we will create a reminder relative to the due date (which might be the 95% scenario). I am looking forward to testing this with tasks.org once it's in the unstable version. Then maybe we need another iteration. And maybe even some edge cases will never work at all, because Vikunjas model and CalDav do not overlap by 100%. I think in part it is tolerable as long as reminders relative to due_date and then to start_date work as expected, and we understand (and document) what's going on.
konrad added 1 commit 2023-04-01 10:52:15 +00:00
chore: refactor parseVAlarm
All checks were successful
continuous-integration/drone/pr Build is passing
0ef9b55536
konrad approved these changes 2023-04-01 10:52:16 +00:00
konrad scheduled this pull request to auto merge when all checks succeed 2023-04-01 10:52:52 +00:00
konrad merged commit f45648a6f7 into main 2023-04-01 11:09:13 +00:00
Member

🥳

🥳
Sign in to join this conversation.
No Reviewers
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vikunja/vikunja#1415
No description provided.