From 9b7858473416011e567401c5d7e845c98b21d1e8 Mon Sep 17 00:00:00 2001 From: kolaente Date: Sun, 16 Nov 2025 11:01:15 +0100 Subject: [PATCH] fix(events): only trigger task.updated once when marking task done Resolves https://github.com/go-vikunja/vikunja/issues/1724 --- pkg/events/testing.go | 17 ++++++++++++ pkg/models/kanban_task_bucket.go | 47 +++++++++++++++++++------------- pkg/models/task_position.go | 40 +++++++++++++++++---------- pkg/models/tasks.go | 8 +++--- pkg/models/tasks_test.go | 21 ++++++++++++++ 5 files changed, 95 insertions(+), 38 deletions(-) diff --git a/pkg/events/testing.go b/pkg/events/testing.go index 951316cb1..b422e7c20 100644 --- a/pkg/events/testing.go +++ b/pkg/events/testing.go @@ -61,3 +61,20 @@ func TestListener(t *testing.T, event Event, listener Listener) { err = listener.Handle(msg) require.NoError(t, err) } + +// ClearDispatchedEvents clears the list of dispatched test events. This is useful when you want to +// test event dispatch counts in a specific section of code without events from previous test operations. +func ClearDispatchedEvents() { + dispatchedTestEvents = nil +} + +// CountDispatchedEvents counts how many events of a specific type have been dispatched. +func CountDispatchedEvents(eventName string) int { + count := 0 + for _, testEvent := range dispatchedTestEvents { + if testEvent.Name() == eventName { + count++ + } + } + return count +} diff --git a/pkg/models/kanban_task_bucket.go b/pkg/models/kanban_task_bucket.go index d852e0560..13c5c7cd3 100644 --- a/pkg/models/kanban_task_bucket.go +++ b/pkg/models/kanban_task_bucket.go @@ -83,23 +83,8 @@ func (b *TaskBucket) upsert(s *xorm.Session) (err error) { return } -// Update is the handler to update a task bucket -// @Summary Update a task bucket -// @Description Updates a task in a bucket -// @tags task -// @Accept json -// @Produce json -// @Security JWTKeyAuth -// @Param project path int true "Project ID" -// @Param view path int true "Project View ID" -// @Param bucket path int true "Bucket ID" -// @Param taskBucket body models.TaskBucket true "The id of the task you want to move into the bucket." -// @Success 200 {object} models.TaskBucket "The updated task bucket." -// @Failure 400 {object} web.HTTPError "Invalid task bucket object provided." -// @Failure 500 {object} models.Message "Internal error" -// @Router /projects/{project}/views/{view}/buckets/{bucket}/tasks [post] -func (b *TaskBucket) Update(s *xorm.Session, a web.Auth) (err error) { - +// updateTaskBucket is internally used to actually do the update. +func updateTaskBucket(s *xorm.Session, a web.Auth, b *TaskBucket) (err error) { oldTaskBucket := &TaskBucket{} _, err = s. Where("task_id = ? AND project_view_id = ?", b.TaskID, b.ProjectViewID). @@ -192,7 +177,7 @@ func (b *TaskBucket) Update(s *xorm.Session, a web.Auth) (err error) { err = task.updateReminders(s, task) if err != nil { - return err + return } // Since the done state of the task was changed, we need to move the task into all done buckets everywhere @@ -230,9 +215,33 @@ func (b *TaskBucket) Update(s *xorm.Session, a web.Auth) (err error) { b.Task = task b.Bucket = bucket + return +} + +// Update is the handler to update a task bucket +// @Summary Update a task bucket +// @Description Updates a task in a bucket +// @tags task +// @Accept json +// @Produce json +// @Security JWTKeyAuth +// @Param project path int true "Project ID" +// @Param view path int true "Project View ID" +// @Param bucket path int true "Bucket ID" +// @Param taskBucket body models.TaskBucket true "The id of the task you want to move into the bucket." +// @Success 200 {object} models.TaskBucket "The updated task bucket." +// @Failure 400 {object} web.HTTPError "Invalid task bucket object provided." +// @Failure 500 {object} models.Message "Internal error" +// @Router /projects/{project}/views/{view}/buckets/{bucket}/tasks [post] +func (b *TaskBucket) Update(s *xorm.Session, a web.Auth) (err error) { + err = updateTaskBucket(s, a, b) + if err != nil { + return err + } + doer, _ := user.GetFromAuth(a) return events.Dispatch(&TaskUpdatedEvent{ - Task: task, + Task: b.Task, Doer: doer, }) } diff --git a/pkg/models/task_position.go b/pkg/models/task_position.go index d218544eb..aa7e82f03 100644 --- a/pkg/models/task_position.go +++ b/pkg/models/task_position.go @@ -54,21 +54,9 @@ func (tp *TaskPosition) CanUpdate(s *xorm.Session, a web.Auth) (bool, error) { return t.CanUpdate(s, a) } -// Update is the handler to update a task position -// @Summary Updates a task position -// @Description Updates a task position. -// @tags task -// @Accept json -// @Produce json -// @Security JWTKeyAuth -// @Param id path int true "Task ID" -// @Param view body models.TaskPosition true "The task position with updated values you want to change." -// @Success 200 {object} models.TaskPosition "The updated task position." -// @Failure 400 {object} web.HTTPError "Invalid task position object provided." -// @Failure 500 {object} models.Message "Internal error" -// @Router /tasks/{id}/position [post] -func (tp *TaskPosition) Update(s *xorm.Session, a web.Auth) (err error) { - +// updateTaskPosition is the internal function that performs the task position update logic +// without dispatching events. This is used by moveTaskToDoneBuckets to avoid duplicate events. +func updateTaskPosition(s *xorm.Session, a web.Auth, tp *TaskPosition) (err error) { // Update all positions if the newly saved position is < 0.1 var shouldRecalculate bool var view *ProjectView @@ -110,6 +98,28 @@ func (tp *TaskPosition) Update(s *xorm.Session, a web.Auth) (err error) { return RecalculateTaskPositions(s, view, a) } + return nil +} + +// Update is the handler to update a task position +// @Summary Updates a task position +// @Description Updates a task position. +// @tags task +// @Accept json +// @Produce json +// @Security JWTKeyAuth +// @Param id path int true "Task ID" +// @Param view body models.TaskPosition true "The task position with updated values you want to change." +// @Success 200 {object} models.TaskPosition "The updated task position." +// @Failure 400 {object} web.HTTPError "Invalid task position object provided." +// @Failure 500 {object} models.Message "Internal error" +// @Router /tasks/{id}/position [post] +func (tp *TaskPosition) Update(s *xorm.Session, a web.Auth) (err error) { + err = updateTaskPosition(s, a, tp) + if err != nil { + return err + } + return triggerTaskUpdatedEventForTaskID(s, a, tp.TaskID) } diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index 81c921e1a..e0532f088 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -1173,7 +1173,7 @@ func (t *Task) updateSingleTask(s *xorm.Session, a web.Auth, fields []string) (e ProjectViewID: view.ID, ProjectID: t.ProjectID, } - err = tb.Update(s, a) + err = updateTaskBucket(s, a, tb) if err != nil { return err } @@ -1183,7 +1183,7 @@ func (t *Task) updateSingleTask(s *xorm.Session, a web.Auth, fields []string) (e return err } - err = tp.Update(s, a) + err = updateTaskPosition(s, a, tp) if err != nil { return err } @@ -1404,7 +1404,7 @@ func (t *Task) moveTaskToDoneBuckets(s *xorm.Session, a web.Auth, views []*Proje ProjectViewID: view.ID, ProjectID: t.ProjectID, } - err = tb.Update(s, a) + err = updateTaskBucket(s, a, tb) if err != nil { return err } @@ -1414,7 +1414,7 @@ func (t *Task) moveTaskToDoneBuckets(s *xorm.Session, a web.Auth, views []*Proje ProjectViewID: view.ID, Position: calculateDefaultPosition(t.Index, t.Position), } - err = tp.Update(s, a) + err = updateTaskPosition(s, a, &tp) if err != nil { return err } diff --git a/pkg/models/tasks_test.go b/pkg/models/tasks_test.go index 05d2362a8..57652b990 100644 --- a/pkg/models/tasks_test.go +++ b/pkg/models/tasks_test.go @@ -261,6 +261,27 @@ func TestTask_Update(t *testing.T) { "bucket_id": 3, }, false) }) + t.Run("marking a task as done should fire exactly ONE task.updated event", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Clear any events from previous operations + events.ClearDispatchedEvents() + + task := &Task{ + ID: 1, + Done: true, + } + err := task.Update(s, u) + require.NoError(t, err) + err = s.Commit() + require.NoError(t, err) + + // Verify exactly ONE task.updated event was dispatched + count := events.CountDispatchedEvents("task.updated") + assert.Equal(t, 1, count, "Expected exactly 1 task.updated event, got %d", count) + }) t.Run("move task to another project should use the default bucket", func(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession()