vikunja/vikunja
6
43
Fork 73
Code Issues 151 Pull Requests 16 Packages Releases 32 Activity

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
Conversation 2 Commits 30 Files Changed 10 +48 -40
4 changed files with 48 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 287a21da93 - Show all commits

5
pkg/caldav/caldav.go
View File

@ -204,13 +204,10 @@ func ParseAlarms(alarms []Alarm, taskDescription string) (caldavalarms string) {
caldavalarms += `
BEGIN:VALARM`
switch a.RelativeTo {
case models.ReminderRelationDueDate:
caldavalarms += `
TRIGGER:` + makeCalDavDuration(a.Duration)
case models.ReminderRelationStartDate:
caldavalarms += `
TRIGGER;RELATED=START:` + makeCalDavDuration(a.Duration)
case models.ReminderRelationEndDate:
case models.ReminderRelationEndDate, models.ReminderRelationDueDate:
konrad commented 2023-03-31 13:52:28 +00:00
Review
Copy Link

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?
ce72 commented 2023-03-31 14:23:19 +00:00
Review
Copy Link

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.
👍 1
konrad commented 2023-04-01 08:35:50 +00:00
Review
Copy Link

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!
ce72 commented 2023-04-01 09:07:41 +00:00
Review
Copy Link

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.
👍 1
caldavalarms += `
TRIGGER;RELATED=END:` + makeCalDavDuration(a.Duration)
default:

2
pkg/caldav/caldav_test.go
View File

@ -311,7 +311,7 @@ ACTION:DISPLAY
DESCRIPTION:alarm description
END:VALARM
BEGIN:VALARM
TRIGGER:-PT2H0M0S
TRIGGER;RELATED=END:-PT2H0M0S
ACTION:DISPLAY
DESCRIPTION:Todo #1
END:VALARM

63
pkg/caldav/parsing.go
View File

@ -141,53 +141,56 @@ func ParseTaskFromVTODO(content string) (vTask *models.Task, err error) {
vTask.EndDate = vTask.StartDate.Add(duration)
}
reminders := make([]*models.TaskReminder, 0)
for _, vAlarm := range vTodo.SubComponents() {
if vAlarm, ok := vAlarm.(*ics.VAlarm); ok {
reminders = parseVAlarm(vAlarm, reminders)
vTask = parseVAlarm(vAlarm, vTask)
}
}
if len(reminders) > 0 {
vTask.Reminders = reminders
}
return
}
func parseVAlarm(vAlarm *ics.VAlarm, reminders []*models.TaskReminder) []*models.TaskReminder {
func parseVAlarm(vAlarm *ics.VAlarm, vTask *models.Task) *models.Task {
for _, property := range vAlarm.UnknownPropertiesIANAProperties() {
if property.IANAToken == "TRIGGER" {
switch {
case len(property.ICalParameters["VALUE"]) > 0:
if property.ICalParameters["VALUE"][0] == "DATE-TIME" {
// Example: TRIGGER;VALUE=DATE-TIME:20181201T011210Z
reminders = append(reminders, &models.TaskReminder{
Reminder: caldavTimeToTimestamp(property.Value)})
}
case len(property.ICalParameters["RELATED"]) > 0:
duration := utils.ParseISO8601Duration(property.Value)
switch property.ICalParameters["RELATED"][0] {
case "START":
if contains(property.ICalParameters["VALUE"], "DATE-TIME") {
// Example: TRIGGER;VALUE=DATE-TIME:20181201T011210Z
vTask.Reminders = append(vTask.Reminders, &models.TaskReminder{
Reminder: caldavTimeToTimestamp(property.Value)})
} else {
if contains(property.ICalParameters["RELATED"], "END") {
// Example: TRIGGER;RELATED=END:-P2D
duration := utils.ParseISO8601Duration(property.Value)
if vTask.EndDate.IsZero() {
vTask.Reminders = append(vTask.Reminders, &models.TaskReminder{
RelativePeriod: int64(duration.Seconds()),
RelativeTo: models.ReminderRelationDueDate})
} else {
vTask.Reminders = append(vTask.Reminders, &models.TaskReminder{
konrad commented 2023-03-28 10:37:14 +00:00
Outdated
Review
Copy Link

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?
ce72 commented 2023-03-29 21:48:24 +00:00
Outdated
Review
Copy Link

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?
konrad commented 2023-04-01 10:45:48 +00:00
Outdated
Review
Copy Link

I think this is fine now.

I think this is fine now.
RelativePeriod: int64(duration.Seconds()),
RelativeTo: models.ReminderRelationEndDate})
}
} else {
// Example: TRIGGER;RELATED=START:-P2D
reminders = append(reminders, &models.TaskReminder{
// Example: TRIGGER:-PT60M
duration := utils.ParseISO8601Duration(property.Value)
vTask.Reminders = append(vTask.Reminders, &models.TaskReminder{
RelativePeriod: int64(duration.Seconds()),
RelativeTo: models.ReminderRelationStartDate})
case "END":
// Example: TRIGGER;RELATED=END:-P2D
reminders = append(reminders, &models.TaskReminder{
RelativePeriod: int64(duration.Seconds()),
RelativeTo: models.ReminderRelationEndDate})
}
default:
duration := utils.ParseISO8601Duration(property.Value)
// Example: TRIGGER:-PT60M
reminders = append(reminders, &models.TaskReminder{
RelativePeriod: int64(duration.Seconds()),
RelativeTo: models.ReminderRelationDueDate})
}
}
}
return reminders
return vTask
}
func contains(array []string, str string) bool {
for _, value := range array {
if value == str {
return true
}
}
return false
}
// https://tools.ietf.org/html/rfc5545#section-3.3.5

18
pkg/caldav/parsing_test.go
View File

@ -170,7 +170,11 @@ TRIGGER:PT0S
ACTION:DISPLAY
END:VALARM
BEGIN:VALARM
TRIGGER:-PT60M
TRIGGER;VALUE=DURATION:-PT60M
ACTION:DISPLAY
END:VALARM
BEGIN:VALARM
TRIGGER:-PT61M
ACTION:DISPLAY
END:VALARM
BEGIN:VALARM
@ -192,19 +196,23 @@ END:VCALENDAR`,
DueDate: time.Date(2023, 3, 4, 15, 0, 0, 0, config.GetTimeZone()),
Reminders: []*models.TaskReminder{
{
RelativeTo: models.ReminderRelationDueDate,
RelativeTo: models.ReminderRelationStartDate,
RelativePeriod: 0,
},
{
RelativeTo: models.ReminderRelationDueDate,
RelativeTo: models.ReminderRelationStartDate,
RelativePeriod: -3600,
},
{
RelativeTo: models.ReminderRelationStartDate,
RelativePeriod: -3660,
},
{
RelativeTo: models.ReminderRelationStartDate,
RelativePeriod: -86400,
},
{
RelativeTo: models.ReminderRelationEndDate,
RelativeTo: models.ReminderRelationDueDate,
RelativePeriod: -1800,
},
},
@ -309,7 +317,7 @@ ACTION:DISPLAY
DESCRIPTION:Task 1
END:VALARM
BEGIN:VALARM
TRIGGER:-PT1H0M0S
TRIGGER;RELATED=END:-PT1H0M0S
ACTION:DISPLAY
DESCRIPTION:Task 1
END:VALARM