diff --git a/pkg/models/bulk_task_test.go b/pkg/models/bulk_task_test.go index baacf672f..ee2bf9630 100644 --- a/pkg/models/bulk_task_test.go +++ b/pkg/models/bulk_task_test.go @@ -18,6 +18,7 @@ package models import ( "testing" + "time" "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/user" @@ -116,4 +117,21 @@ func TestBulkTask_Update(t *testing.T) { assert.NotZero(t, bt.Tasks[0].DoneAt) assert.NotZero(t, bt.Tasks[1].DoneAt) }) + + t.Run("don't update done_at when bulk marking tasks done", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + userProvidedTime := time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC) + bt := &BulkTask{ + TaskIDs: []int64{1, 3}, + Fields: []string{"done", "done_at"}, + Values: &Task{Done: true, DoneAt: userProvidedTime}, + } + + err := bt.Update(s, u) + require.Error(t, err) + assert.IsType(t, ErrInvalidTaskColumn{}, err) + }) } diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index 584eda065..a50041437 100644 --- a/pkg/models/tasks.go +++ b/pkg/models/tasks.go @@ -59,7 +59,7 @@ type Task struct { Description string `xorm:"longtext null" json:"description"` // Whether a task is done or not. Done bool `xorm:"INDEX null" json:"done"` - // The time when a task was marked as done. + // The time when a task was marked as done. This field is system-controlled and cannot be set via API. DoneAt time.Time `xorm:"INDEX null 'done_at'" json:"done_at"` // The time when the task is due. DueDate time.Time `xorm:"DATETIME INDEX null 'due_date'" json:"due_date"` @@ -1047,7 +1047,6 @@ func (t *Task) updateSingleTask(s *xorm.Session, a web.Auth, fields []string) (e "start_date", "end_date", "hex_color", - "done_at", "percent_done", "project_id", "bucket_id", @@ -1190,7 +1189,10 @@ func (t *Task) updateSingleTask(s *xorm.Session, a web.Auth, fields []string) (e } // When a repeating task is marked as done, we update all deadlines and reminders and set it as undone - updateDone(&ot, t) + updateDoneAt := updateDone(&ot, t) + if updateDoneAt { + colsToUpdate = append(colsToUpdate, "done_at") + } // Update the reminders if err := ot.updateReminders(s, t); err != nil { @@ -1563,7 +1565,10 @@ func setTaskDatesFromCurrentDateRepeat(oldTask, newTask *Task) { // We make a few assumptions here: // 1. Everything in oldTask is the truth - we figure out if we update anything at all if oldTask.RepeatAfter has a value > 0 // 2. Because of 1., this functions should not be used to update values other than Done in the same go -func updateDone(oldTask *Task, newTask *Task) { +func updateDone(oldTask *Task, newTask *Task) (updateDoneAt bool) { + // Track if the done status changed before repeat helpers modify it + doneStatusChanged := oldTask.Done != newTask.Done + if !oldTask.Done && newTask.Done { switch oldTask.RepeatMode { case TaskRepeatModeMonth: @@ -1581,6 +1586,8 @@ func updateDone(oldTask *Task, newTask *Task) { if oldTask.Done && !newTask.Done { newTask.DoneAt = time.Time{} } + + return doneStatusChanged } // Set the absolute trigger dates for Reminders with relative period diff --git a/pkg/models/tasks_test.go b/pkg/models/tasks_test.go index e696eaa11..05d2362a8 100644 --- a/pkg/models/tasks_test.go +++ b/pkg/models/tasks_test.go @@ -334,6 +334,40 @@ func TestTask_Update(t *testing.T) { "bucket_id": 1, }, false) }) + t.Run("repeating tasks should set done_at when marked done", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + // Get the task before updating to check done_at was empty + taskBefore := &Task{ID: 28} + err := taskBefore.ReadOne(s, u) + require.NoError(t, err) + assert.True(t, taskBefore.DoneAt.IsZero()) + assert.False(t, taskBefore.Done) + + // Mark the repeating task as done + task := &Task{ + ID: 28, + Done: true, + RepeatAfter: 3600, + } + err = task.Update(s, u) + require.NoError(t, err) + err = s.Commit() + require.NoError(t, err) + + // Task should be reset to not done (because it repeats) but done_at should be set + assert.False(t, task.Done) + assert.False(t, task.DoneAt.IsZero(), "done_at should be set for repeating tasks when marked as done") + + // Verify in database + updatedTask := &Task{ID: 28} + err = updatedTask.ReadOne(s, u) + require.NoError(t, err) + assert.False(t, updatedTask.Done) + assert.False(t, updatedTask.DoneAt.IsZero(), "done_at should be persisted in database for repeating tasks") + }) t.Run("moving a task between projects should give it a correct index", func(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession() @@ -450,6 +484,44 @@ func TestTask_Update(t *testing.T) { err = s.Commit() require.NoError(t, err) }) + t.Run("don't allow done_at change when passing fields", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + task := &Task{ + ID: 1, + DoneAt: time.Now(), + } + + err := task.updateSingleTask(s, u, []string{"done_at"}) + + require.Error(t, err) + assert.Contains(t, err.Error(), `Task column done_at is invalid`) + require.NoError(t, s.Commit()) + }) + t.Run("ignore done_at when updating unrelated values", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + task := &Task{ + ID: 1, + Title: "updated", + DoneAt: time.Now(), + } + + err := task.Update(s, u) + + require.NoError(t, err) + require.NoError(t, s.Commit()) + + updatedTask := &Task{ID: 1} + err = updatedTask.ReadOne(s, u) + require.NoError(t, err) + assert.Equal(t, "updated", updatedTask.Title) + assert.True(t, updatedTask.DoneAt.IsZero()) + }) } func TestTask_Delete(t *testing.T) {