WIP: feat(caldav): Add support for subtasks in CalDAV. #1442

Closed
zewaren wants to merge 13 commits from zewaren/api:feature/caldav-subtasks into main
Contributor

When exporting tasks with CalDAV, export their relationship to their
parent tasks.
When creating/updating tasks from CalDAV, create/maintain their parent
relationship using task relations.

When exporting tasks with CalDAV, export their relationship to their parent tasks. When creating/updating tasks from CalDAV, create/maintain their parent relationship using task relations.
zewaren added 1 commit 2023-03-22 19:53:48 +00:00
continuous-integration/drone/pr Build is failing Details
0eda1d9785
feat(caldav): Add support for subtasks in CalDAV.
When exporting tasks with CalDAV, export their relationship to their
parent tasks.
When creating/updating tasks from CalDAV, create/maintain their parent
relationship using task relations.
zewaren added 1 commit 2023-03-22 20:20:11 +00:00
continuous-integration/drone/pr Build is failing Details
a1f25d3671
fix: Typo in a comment
zewaren changed title from feat(caldav): Add support for subtasks in CalDAV. to WIP: feat(caldav): Add support for subtasks in CalDAV. 2023-03-22 21:08:01 +00:00
zewaren added 1 commit 2023-03-22 21:11:56 +00:00
continuous-integration/drone/pr Build is passing Details
f9040b402d
fix: lint
konrad requested changes 2023-03-23 20:48:53 +00:00
@ -523,0 +536,4 @@
UID: "randommduid",
Timestamp: time.Unix(1543626724, 0).In(config.GetTimeZone()),
Color: "affffe",
RelatedToParentUID: "another_random_uid",
Owner

Shouldnt the test also include that other task?

Shouldnt the test also include that other task?
Author
Contributor

This test file only tests parsing CALDAV entries. It doesn't touch the database at all. We don't really care if the relation actually exists. We only care that the field was parsed into the correct field.

This test file only tests parsing CALDAV entries. It doesn't touch the database at all. We don't really care if the relation actually exists. We only care that the field was parsed into the correct field.
konrad marked this conversation as resolved
@ -42,0 +42,4 @@
// Find the UID of the parent task, if it exists:
var parentTaskUID string
if parentTasks, ok := t.RelatedTasks[models.RelationKindParenttask]; ok {
parentTaskUID = parentTasks[0].UID
Owner

Doesn't this always set the first parent task as the parent in caldav? What if a task has multiple parents?

Doesn't this always set the first parent task as the parent in caldav? What if a task has multiple parents?
Author
Contributor

Do you know any CALDAV TODO list client/app that supports multiple parents for tasks? I'd be possible to support them here, but would it make any sense for anyone at all?

Do you know any CALDAV TODO list client/app that supports multiple parents for tasks? I'd be possible to support them here, but would it make any sense for anyone at all?
Owner

Not sure what the spec says about this. Let's go with one parent for now and change this when we see it causes problems.

Not sure what the spec says about this. Let's go with one parent for now and change this when we see it causes problems.
konrad marked this conversation as resolved
@ -60,3 +60,3 @@
// InitTests handles the actual bootstrapping of the test env
func InitTests() {
func InitTests(loadFixtures bool) {
Owner

Why would you not want to load this? The files fixtures are mostly only required for file fixtures, but I feel like this adds unnecessary overhead.

Why would you not want to load this? The files fixtures are mostly only required for file fixtures, but I feel like this adds unnecessary overhead.
Author
Contributor

So, the way the test infrastructure is currently configured on the main branch is that every HTTP request resets the database by loading all the fixtures.

This prevents us from being able to run more complicated integration tests, that require multiple HTTP calls.

The best example is the test with the grand children.
That test creates a hierarchy of tasks with multiple parent/child levels. It checks that the tasks can be synchronized in any order (example: grand-children first), with the end result being always consistent.

How would you achieve such an integration test, without being able to keep some state between HTTP requests the same way an actual live client would expect?

So, the way the test infrastructure is currently configured on the main branch is that every HTTP request resets the database by loading all the fixtures. This prevents us from being able to run more complicated integration tests, that require multiple HTTP calls. The best example is the test with the grand children. That test creates a hierarchy of tasks with multiple parent/child levels. It checks that the tasks can be synchronized in any order (example: grand-children first), with the end result being always consistent. How would you achieve such an integration test, without being able to keep some state between HTTP requests the same way an actual live client would expect?
Owner

Shouldn't the relation be present already when the test starts? (loaded from fixtures)

The test you described should probably be multiple tests, one for the creation of a hierarchy, one for the modification, one to check if it is populated properly, etc.

Shouldn't the relation be present already when the test starts? (loaded from fixtures) The test you described should probably be multiple tests, one for the creation of a hierarchy, one for the modification, one to check if it is populated properly, etc.
@ -72,3 +72,1 @@
err = db.LoadFixtures()
if err != nil {
return
if loadFixtures {
Owner

Same here, why wouldn't you want to load the fixtures? This creates a kind of unpredictable state for tests.

Same here, why wouldn't you want to load the fixtures? This creates a kind of unpredictable state for tests.
@ -433,2 +451,4 @@
}
// When a VTODO entry doesn't have a parent anymore, but we do, we need to remove it as well.
func removeLegacyParentRelations(s *xorm.Session, a web.Auth, task *models.Task, newRelations map[models.RelationKind][]*models.Task) (err error) {
Owner

Why call this "legacy"?

Why call this "legacy"?
Author
Contributor

Here "legacy" means that the tasks had a parent, but that parent is gone.

This case is when a CALDAV client removes the parent relationship and pushes the task to us. We still have the relationship in our DB, and we need to remove it.

I'm happy to use a better word if you have one in mind.

Here "legacy" means that the tasks had a parent, but that parent is gone. This case is when a CALDAV client removes the parent relationship and pushes the task to us. We still have the relationship in our DB, and we need to remove it. I'm happy to use a better word if you have one in mind.
Owner

Something like removeDeletedParentRelations?

Something like `removeDeletedParentRelations`?
First-time contributor

@zewaren are you willing to make the requested code changes? Maybe someone else can pick this up if not?

@zewaren are you willing to make the requested code changes? Maybe someone else can pick this up if not?
Author
Contributor

Hey.
Sorry, I've been super busy the last few months.
I can fix it in a few weeks, if that's ok.
Erwan.

Hey. Sorry, I've been super busy the last few months. I can fix it in a few weeks, if that's ok. Erwan.
zewaren added 3 commits 2023-09-10 15:45:47 +00:00
3865219e6e Merge branch 'main' into feature/caldav-subtasks
# Conflicts:
#	go.mod
#	pkg/caldav/caldav.go
#	pkg/caldav/caldav_test.go
#	pkg/caldav/parsing.go
#	pkg/caldav/parsing_test.go
#	pkg/db/fixtures/projects.yml
#	pkg/integrations/caldav_test.go
continuous-integration/drone/pr Build is failing Details
24773ad6b7
Fix the tests after bumping to master.
zewaren added 3 commits 2023-09-17 17:53:53 +00:00
Owner

hey @zewaren is this ready to review?

hey @zewaren is this ready to review?
zewaren added 3 commits 2023-10-29 20:17:32 +00:00
zewaren added 1 commit 2023-10-29 21:01:41 +00:00
continuous-integration/drone/pr Build is passing Details
cbae9b7a87
Lint
Contributor

I thought this PR may have fallen by the wayside so I took to cleaning up a few things myself earlier today to submit for review. However, I just noticed it was updated. 🤦‍♂️ for me 🥇 for @zewaren lol

I think it might be useful though as:

  • I worked around the loadFixtures issue (vikunja/api#1442 (comment)) in a different way which is more self-contained so take a look.
  • I handled the RELATED-TOs more generically in order to stick to the RFC spec.
  • I cleaned up the commit history to match the standards as mentioned in the Vikunja docs.

I tested with DAVx5 & Tasks (Android) and it's been working great AFAICT.

If you find it helpful and want to merge branches somehow let me know how I can be of help.
https://kolaente.dev/mayanez/api/src/branch/feat/caldav-relatedto

I thought this PR may have fallen by the wayside so I took to cleaning up a few things myself earlier today to submit for review. However, I just noticed it was updated. 🤦‍♂️ for me 🥇 for @zewaren lol I think it might be useful though as: * I worked around the `loadFixtures` issue (https://kolaente.dev/vikunja/api/pulls/1442#issuecomment-53845) in a different way which is more self-contained so take a look. * I handled the `RELATED-TO`s more generically in order to stick to the RFC spec. * I cleaned up the commit history to match the standards as mentioned in the Vikunja docs. I tested with DAVx5 & Tasks (Android) and it's been working great AFAICT. If you find it helpful and want to merge branches somehow let me know how I can be of help. https://kolaente.dev/mayanez/api/src/branch/feat/caldav-relatedto
Author
Contributor

Hey @mayanez

Thanks for the addition! I've checked the diff between our branches and I'm good with everything.

I've been running my branch for a month now on DAVx5 & Tasks on Android as well. I've used subtasks a lot and I didn't find any bug there. I'm confident the feature is not going to be broken soon, with the amount of automated tests I added 😀

Let's submit your branch instead of mine. Can you ping me when you create your pull-request?

Hey @mayanez Thanks for the addition! I've checked the diff between our branches and I'm good with everything. I've been running my branch for a month now on DAVx5 & Tasks on Android as well. I've used subtasks a lot and I didn't find any bug there. I'm confident the feature is not going to be broken soon, with the amount of automated tests I added 😀 Let's submit your branch instead of mine. Can you ping me when you create your pull-request?
zewaren closed this pull request 2023-10-30 08:13:03 +00:00
Contributor

Hey @zewaren,

I opened the PR here vikunja/api#1634

Hey @zewaren, I opened the PR here https://kolaente.dev/vikunja/api/pulls/1634
All checks were successful
continuous-integration/drone/pr Build is passing
Required
Details

Pull request closed

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#1442
No description provided.