WIP: feat(caldav): Add support for subtasks in CalDAV. #1442
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "zewaren/api:feature/caldav-subtasks"
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?
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.
feat(caldav): Add support for subtasks in CalDAV.to WIP: feat(caldav): Add support for subtasks in CalDAV.@ -523,0 +536,4 @@
UID: "randommduid",
Timestamp: time.Unix(1543626724, 0).In(config.GetTimeZone()),
Color: "affffe",
RelatedToParentUID: "another_random_uid",
Shouldnt the test also include that other task?
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.
@ -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
Doesn't this always set the first parent task as the parent in caldav? What if a task has multiple parents?
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?
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.
@ -60,3 +60,3 @@
// InitTests handles the actual bootstrapping of the test env
func InitTests() {
func InitTests(loadFixtures bool) {
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.
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?
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 {
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) {
Why call this "legacy"?
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.
Something like
removeDeletedParentRelations
?@zewaren are you willing to make the requested code changes? Maybe someone else can pick this up if not?
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 @zewaren is this ready to review?
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:
loadFixtures
issue (vikunja/api#1442 (comment)) in a different way which is more self-contained so take a look.RELATED-TO
s more generically in order to stick to the RFC spec.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
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 @zewaren,
I opened the PR here vikunja/api#1634
Pull request closed