feat(caldav): Add support for subtasks (i.e. RELATED-TO property) in CalDAV #1634

Merged
konrad merged 7 commits from mayanez/api:feat/caldav-relatedto into main 2023-11-10 22:44:07 +00:00
Contributor

As I mentioned here, this is mainly a cleanup of @zewaren 's original PR.

It adds support for the RELATED-TO property in CalDAV's VTODO and the RELTYPE=PARENT and RELTYPE=CHILD relationships. In other words, it allows for ParentTask->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

As I mentioned [here](https://kolaente.dev/vikunja/api/pulls/1442#issuecomment-55215), this is mainly a cleanup of @zewaren 's original [PR](https://kolaente.dev/vikunja/api/pulls/1442). It adds support for the `RELATED-TO` property in CalDAV's `VTODO` and the `RELTYPE=PARENT` and `RELTYPE=CHILD` relationships. In other words, it allows for `ParentTask->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 https://kolaente.dev/vikunja/api/issues/1345
mayanez force-pushed feat/caldav-relatedto from 2bf1cd56f7 to 3498bdd9b4 2023-10-30 18:55:17 +00:00 Compare
konrad requested changes 2023-10-31 19:53:38 +00:00
konrad left a comment
Owner

Thank you very much for the PR! I have a few comments but nothing major.

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
Owner

Can you rename the field to Relations?

Can you rename the field to `Relations`?
mayanez marked this conversation as resolved
@ -222,6 +223,47 @@ END:VALARM`
return caldavalarms
}
func ParseRelatedTos(relatedtos []Relation) (caldavrelatedtos string) {
Owner

Same here for the name

Same here for the name
mayanez marked this conversation as resolved
@ -153,0 +161,4 @@
relationKind = models.RelationKindParenttask
case "CHILD":
relationKind = models.RelationKindSubtask
default:
Owner

What if relTypeStr is empty?

What if `relTypeStr` is empty?
Author
Contributor

Fixed.

Fixed.
mayanez marked this conversation as resolved
@ -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)
Owner

why this change?

why this change?
Author
Contributor

We move one of the tasks from bucket 1 to 38 in pkg/db/fixtures/tasks.yml:377.

We move one of the tasks from bucket 1 to 38 in `pkg/db/fixtures/tasks.yml:377`.
mayanez marked this conversation as resolved
@ -433,0 +452,4 @@
// Get the existing task with details:
existingTask := &models.Task{ID: task.ID}
err = existingTask.ReadOne(s, a)
Owner

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.

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.
mayanez marked this conversation as resolved
@ -433,0 +493,4 @@
for _, relatedTask := range relatedTasks {
// Get the task from the DB:
has, err := s.Get(relatedTask)
Owner

Please use models.GetTaskSimpleByUUID instead.

Please use `models.GetTaskSimpleByUUID` instead.
Author
Contributor

I was running into a bug with GetTaskSimplyByUUID in that the xorm 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 the has boolean as you can no longer rely on the nullptr to check whether the item has been successfully fetched from the DB.

I was running into a bug with `GetTaskSimplyByUUID` in that the `xorm` `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 the `has` boolean as you can no longer rely on the nullptr to check whether the item has been successfully fetched from the DB.
mayanez marked this conversation as resolved
@ -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:
Owner

probably

What happens if that's not the case?

> probably What happens if that's not the case?
Author
Contributor

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:

  1. Add a isDummy field to the Task to track them. This allows a cleanup later.
  2. Create an "invisible" list where we add these dummy tasks to. This again allows for cleanup later.

I updated the comment to reflect this. Let me know if you have any other proposals.

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: 1. Add a `isDummy` field to the `Task` to track them. This allows a cleanup later. 2. Create an "invisible" list where we add these dummy tasks to. This again allows for cleanup later. I updated the comment to reflect this. Let me know if you have any other proposals.
Contributor

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).

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).
Author
Contributor

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.

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.
Owner

as I suppose this would mean the client is broken to begin with

The problem I see is you can never trust clients or even assume they will always send correct data.

To properly automatically cleanup these dummy tasks later (should they be a real problem), I believe the options are probably:

I think we can leave it as it is now and think about cleaning it up once this becomes a problem.

> as I suppose this would mean the client is broken to begin with The problem I see is you can never trust clients or even assume they will always send correct data. > To properly automatically cleanup these dummy tasks later (should they be a real problem), I believe the options are probably: I think we can leave it as it is now and think about cleaning it up once this becomes a problem.
mayanez marked this conversation as resolved
mayanez force-pushed feat/caldav-relatedto from f55b2c2fe1 to 3dfad50791 2023-11-02 02:57:56 +00:00 Compare
mayanez force-pushed feat/caldav-relatedto from 3dfad50791 to c6a3380e69 2023-11-02 05:17:02 +00:00 Compare
mayanez requested review from konrad 2023-11-02 05:28:48 +00:00
First-time contributor

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?

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?
mayanez force-pushed feat/caldav-relatedto from c6a3380e69 to 90052ca5e5 2023-11-06 01:45:28 +00:00 Compare
Author
Contributor

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?

Of course! I hadn't personally used this feature before. I edited the commit messages so hopefully it should reflect it now.

> 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? Of course! I hadn't personally used this feature before. I edited the commit messages so hopefully it should reflect it now.
mayanez force-pushed feat/caldav-relatedto from 90052ca5e5 to 6607169332 2023-11-07 03:13:03 +00:00 Compare
mayanez force-pushed feat/caldav-relatedto from 6607169332 to 93ba7d5267 2023-11-07 03:23:49 +00:00 Compare
Author
Contributor

Rebased to the latest main. There seems to be some errors on some unrelated todoist files, otherwise everything seems to be in order.

Rebased to the latest `main`. There seems to be some errors on some unrelated `todoist` files, otherwise everything seems to be in order.
Owner

Rebased to the latest main. There seems to be some errors on some unrelated todoist files, otherwise everything seems to be in order.

That's fixed now!

> Rebased to the latest `main`. There seems to be some errors on some unrelated `todoist` files, otherwise everything seems to be in order. That's fixed now!
konrad reviewed 2023-11-07 14:41:34 +00:00
@ -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) {
Owner

Why not return a *Task here?

Why not return a `*Task` here?
Author
Contributor

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.

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.
Owner

It should work if you initiate the task before - something like

task = &Task{}

before calling Get.

Since we're returning a pointer to a task I'd like to keep that consistent.

It should work if you initiate the task before - something like ``` task = &Task{} ``` before calling `Get`. Since we're returning a pointer to a task I'd like to keep that consistent.
Author
Contributor

Fair enough. I made the change back to a *Task as requested.

Fair enough. I made the change back to a `*Task` as requested.
konrad marked this conversation as resolved
mayanez force-pushed feat/caldav-relatedto from 93ba7d5267 to d38f83bd9d 2023-11-08 03:21:02 +00:00 Compare
Author
Contributor

Rebased to the latest main. There seems to be some errors on some unrelated todoist files, otherwise everything seems to be in order.

That's fixed now!

Great! I rebased again so hopefully everything should go 🟢.

> > Rebased to the latest `main`. There seems to be some errors on some unrelated `todoist` files, otherwise everything seems to be in order. > > That's fixed now! Great! I rebased again so hopefully everything should go 🟢.
mayanez added 1 commit 2023-11-10 03:50:52 +00:00
refactor: PR Feedback
All checks were successful
continuous-integration/drone/pr Build is passing
bd36afbd1e
konrad approved these changes 2023-11-10 22:43:13 +00:00
konrad left a comment
Owner

Thanks!

Thanks!
konrad merged commit 225d65268d into main 2023-11-10 22:44:07 +00:00
konrad deleted branch feat/caldav-relatedto 2023-11-10 22:44:07 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#1634
No description provided.