feat(caldav): Add support for subtasks (i.e. RELATED-TO
property) in CalDAV
#1634
No reviewers
Labels
No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
No Milestone
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/vikunja#1634
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "mayanez/api:feat/caldav-relatedto"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
As I mentioned here, this is mainly a cleanup of @zewaren 's original PR.
It adds support for the
RELATED-TO
property in CalDAV'sVTODO
and theRELTYPE=PARENT
andRELTYPE=CHILD
relationships. In other words, it allows forParentTask->SubTask
relations to be handled supported through CalDAV.In addition to the included tests, this has been tested by both @zewaren & myself with DAVx5 & Tasks (Android) and it's been working great.
Fixes vikunja/api#1345
2bf1cd56f7
to3498bdd9b4
Thank you very much for the PR! I have a few comments but nothing major.
@ -56,0 +43,4 @@
Completed time.Time
Organizer *user.User
Priority int64 // 0-9, 1 is highest
RelatedTos []Relation
Can you rename the field to
Relations
?@ -222,6 +223,47 @@ END:VALARM`
return caldavalarms
}
func ParseRelatedTos(relatedtos []Relation) (caldavrelatedtos string) {
Same here for the name
@ -153,0 +161,4 @@
relationKind = models.RelationKindParenttask
case "CHILD":
relationKind = models.RelationKindSubtask
default:
What if
relTypeStr
is empty?Fixed.
@ -148,3 +148,3 @@
err = s.Where("bucket_id = ?", 1).Find(&tasks)
assert.NoError(t, err)
assert.Len(t, tasks, 16)
assert.Len(t, tasks, 15)
why this change?
We move one of the tasks from bucket 1 to 38 in
pkg/db/fixtures/tasks.yml:377
.@ -433,0 +452,4 @@
// Get the existing task with details:
existingTask := &models.Task{ID: task.ID}
err = existingTask.ReadOne(s, a)
This will also get all other task attributes which might not be needed here. I think it's fine to do it like this right now because the request happens as part of a sync in 99% of cases in the background but please add a
FIXME
comment so that we remember to optimize this later.@ -433,0 +493,4 @@
for _, relatedTask := range relatedTasks {
// Get the task from the DB:
has, err := s.Get(relatedTask)
Please use
models.GetTaskSimpleByUUID
instead.I was running into a bug with
GetTaskSimplyByUUID
in that thexorm
Get()
requires an object, but the function signature had a*Task
.I addressed it so please double check it as I'm not familiar with
xorm
. Moreover, I exposed thehas
boolean as you can no longer rely on the nullptr to check whether the item has been successfully fetched from the DB.@ -433,0 +499,4 @@
}
// If the related task doesn't exist, create a dummy one now in the same list.
// It'll probably be populated right after in a following request:
What happens if that's not the case?
Worst case, we'll end up with the dummy task in the list.
I haven't encountered it yet as I suppose this would mean the client is broken to begin with. It would need to be requesting tasks to be created with invalid relations.
To properly automatically cleanup these dummy tasks later (should they be a real problem), I believe the options are probably:
isDummy
field to theTask
to track them. This allows a cleanup later.I updated the comment to reflect this. Let me know if you have any other proposals.
I added that logic initially from personal experience.
When I had some network errors with some buggy clients, I was able to end up with task trees being pushed in the wrong order.
It really doesn't happen often, but when it does, having to manually re-create the relations between the tasks is real pain in the neck (especially when you have many similar ones with the same title).
Makes sense @zewaren 👍
I should clarify what I meant when I said that the dummy task will end up in the list.
It will only remain as the "DUMMY" task if for some reason the client fails to push the task (regardless of the order). For example, if the sync is ended abruptly and never re-transmitted. If/when the client eventually res-ends the task, it will be properly updated.
The problem I see is you can never trust clients or even assume they will always send correct data.
I think we can leave it as it is now and think about cleaning it up once this becomes a problem.
f55b2c2fe1
to3dfad50791
3dfad50791
toc6a3380e69
Might be worth putting
Co-authored-by: Erwan Martin <public@fzwte.net>
in the commit messages so that Erwan gets some credit for this contribution?c6a3380e69
to90052ca5e5
Of course! I hadn't personally used this feature before. I edited the commit messages so hopefully it should reflect it now.
90052ca5e5
to6607169332
6607169332
to93ba7d5267
Rebased to the latest
main
. There seems to be some errors on some unrelatedtodoist
files, otherwise everything seems to be in order.That's fixed now!
@ -374,3 +374,2 @@
func GetTaskSimpleByUUID(s *xorm.Session, uid string) (task *Task, err error) {
_, err = s.In("uid", uid).Get(task)
func GetTaskSimpleByUUID(s *xorm.Session, uid string) (has bool, task Task, err error) {
Why not return a
*Task
here?The API for the
Get
needs memory allocated which it will then fill.The reason for the error (and this change) was that the
*Task
was null thus causing a segfault. We could potentially change the function signature to take the*Task
as an argument putting the responsibility on the caller.The implemented approach seemed more appropriate given the fact that the function fetches the task though I'm happy to change it.
It should work if you initiate the task before - something like
before calling
Get
.Since we're returning a pointer to a task I'd like to keep that consistent.
Fair enough. I made the change back to a
*Task
as requested.93ba7d5267
tod38f83bd9d
Great! I rebased again so hopefully everything should go 🟢.
Thanks!