Fix updating dates when marking a task as done (#145)

Fix lint

Fix reminders not being updated

Fix updating dates when marking a task as done

Co-authored-by: kolaente <k@knt.li>
Reviewed-on: vikunja/api#145

pkg/models/tasks.go

@@ -540,12 +540,23 @@ func (t *Task) Create(a web.Auth) (err error) {
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /tasks/{id} [post]
 func (t *Task) Update() (err error) {
-	// Check if the task exists
+	// Check if the task exists and get the old values
 	ot, err := GetTaskByIDSimple(t.ID)
 	if err != nil {
 		return
 	}
 
+	// Get the reminders
+	reminders, err := getRemindersForTasks([]int64{t.ID})
+	if err != nil {
+		return
+	}
+
+	ot.Reminders = make([]timeutil.TimeStamp, len(reminders))
+	for i, r := range reminders {
+		ot.Reminders[i] = r.Reminder
+	}
+
 	// When a repeating task is marked as done, we update all deadlines and reminders and set it as undone
 	updateDone(&ot, t)
 
@@ -577,7 +588,7 @@ func (t *Task) Update() (err error) {
 	//t.Labels = ot.Labels
 
 	// For whatever reason, xorm dont detect if done is updated, so we need to update this every time by hand
-	// Which is why we merge the actual task struct with the one we got from the
+	// Which is why we merge the actual task struct with the one we got from the db
 	// The user struct overrides values in the actual one.
 	if err := mergo.Merge(&ot, t, mergo.WithOverride); err != nil {
 		return err
@@ -646,14 +657,32 @@ func (t *Task) Update() (err error) {
 	return
 }
 
-// This helper function updates the reminders and doneAtUnix of the *old* task (since that's the one we're inserting
-// with updated values into the db)
+// This helper function updates the reminders, doneAtUnix, start and end dates of the *old* task
+// and saves the new values in the newTask object.
+// We make a few assumtions 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) {
 	if !oldTask.Done && newTask.Done && oldTask.RepeatAfter > 0 {
-		oldTask.DueDate = timeutil.FromTime(oldTask.DueDate.ToTime().Add(time.Duration(oldTask.RepeatAfter) * time.Second)) // assuming we'll save the old task (merged)
 
+		repeatDuration := time.Duration(oldTask.RepeatAfter) * time.Second
+
+		// assuming we'll merge the new task over the old task
+		if oldTask.DueDate > 0 {
+			newTask.DueDate = timeutil.FromTime(oldTask.DueDate.ToTime().Add(repeatDuration))
+		}
+
+		newTask.Reminders = oldTask.Reminders
 		for in, r := range oldTask.Reminders {
-			oldTask.Reminders[in] = timeutil.FromTime(r.ToTime().Add(time.Duration(oldTask.RepeatAfter) * time.Second))
+			newTask.Reminders[in] = timeutil.FromTime(r.ToTime().Add(repeatDuration))
+		}
+
+		if oldTask.StartDate > 0 {
+			newTask.StartDate = timeutil.FromTime(oldTask.StartDate.ToTime().Add(repeatDuration))
+		}
+
+		if oldTask.EndDate > 0 {
+			newTask.EndDate = timeutil.FromTime(oldTask.EndDate.ToTime().Add(repeatDuration))
 		}
 
 		newTask.Done = false
@@ -661,15 +690,15 @@ func updateDone(oldTask *Task, newTask *Task) {
 
 	// Update the "done at" timestamp
 	if !oldTask.Done && newTask.Done {
-		oldTask.DoneAt = timeutil.FromTime(time.Now())
+		newTask.DoneAt = timeutil.FromTime(time.Now())
 	}
 	// When unmarking a task as done, reset the timestamp
 	if oldTask.Done && !newTask.Done {
-		oldTask.DoneAt = 0
+		newTask.DoneAt = 0
 	}
 }
 
-// Creates or deletes all necessary remindes without unneded db operations.
+// Creates or deletes all necessary reminders without unneded db operations.
 // The parameter is a slice with unix dates which holds the new reminders.
 func (t *Task) updateReminders(reminders []timeutil.TimeStamp) (err error) {
 

pkg/models/tasks_test.go

@@ -128,14 +128,82 @@ func TestUpdateDone(t *testing.T) {
 		oldTask := &Task{Done: false}
 		newTask := &Task{Done: true}
 		updateDone(oldTask, newTask)
-		assert.NotEqual(t, timeutil.TimeStamp(0), oldTask.DoneAt)
+		assert.NotEqual(t, timeutil.TimeStamp(0), newTask.DoneAt)
 	})
 	t.Run("unmarking a task as done", func(t *testing.T) {
 		db.LoadAndAssertFixtures(t)
 		oldTask := &Task{Done: true}
 		newTask := &Task{Done: false}
 		updateDone(oldTask, newTask)
-		assert.Equal(t, timeutil.TimeStamp(0), oldTask.DoneAt)
+		assert.Equal(t, timeutil.TimeStamp(0), newTask.DoneAt)
+	})
+	t.Run("repeating interval", func(t *testing.T) {
+		t.Run("normal", func(t *testing.T) {
+			oldTask := &Task{
+				Done:        false,
+				RepeatAfter: 8600,
+				DueDate:     timeutil.TimeStamp(1550000000),
+			}
+			newTask := &Task{
+				Done: true,
+			}
+			updateDone(oldTask, newTask)
+			assert.Equal(t, timeutil.TimeStamp(1550008600), newTask.DueDate)
+		})
+		t.Run("don't update if due date is zero", func(t *testing.T) {
+			oldTask := &Task{
+				Done:        false,
+				RepeatAfter: 8600,
+				DueDate:     timeutil.TimeStamp(0),
+			}
+			newTask := &Task{
+				Done:    true,
+				DueDate: timeutil.TimeStamp(1543626724),
+			}
+			updateDone(oldTask, newTask)
+			assert.Equal(t, timeutil.TimeStamp(1543626724), newTask.DueDate)
+		})
+		t.Run("update reminders", func(t *testing.T) {
+			oldTask := &Task{
+				Done:        false,
+				RepeatAfter: 8600,
+				Reminders: []timeutil.TimeStamp{
+					1550000000,
+					1555000000,
+				},
+			}
+			newTask := &Task{
+				Done: true,
+			}
+			updateDone(oldTask, newTask)
+			assert.Len(t, newTask.Reminders, 2)
+			assert.Equal(t, timeutil.TimeStamp(1550008600), newTask.Reminders[0])
+			assert.Equal(t, timeutil.TimeStamp(1555008600), newTask.Reminders[1])
+		})
+		t.Run("update start date", func(t *testing.T) {
+			oldTask := &Task{
+				Done:        false,
+				RepeatAfter: 8600,
+				StartDate:   timeutil.TimeStamp(1550000000),
+			}
+			newTask := &Task{
+				Done: true,
+			}
+			updateDone(oldTask, newTask)
+			assert.Equal(t, timeutil.TimeStamp(1550008600), newTask.StartDate)
+		})
+		t.Run("update end date", func(t *testing.T) {
+			oldTask := &Task{
+				Done:        false,
+				RepeatAfter: 8600,
+				EndDate:     timeutil.TimeStamp(1550000000),
+			}
+			newTask := &Task{
+				Done: true,
+			}
+			updateDone(oldTask, newTask)
+			assert.Equal(t, timeutil.TimeStamp(1550008600), newTask.EndDate)
+		})
 	})
 }