fix(task): move done tasks to the done bucket when they are moved between projects and the new project has a done bucket

pkg/db/fixtures/buckets.yml

@@ -25,6 +25,7 @@
   title: testbucket4 - other project
   project_id: 2
   created_by_id: 1
+  position: 1
   created: 2020-04-18 21:13:52
   updated: 2020-04-18 21:13:52
 # The following are not or only partly owned by user 1
@@ -241,4 +242,11 @@
   project_id: 38
   created_by_id: 15
   created: 2020-04-18 21:13:52
-  updated: 2020-04-18 21:13:52
+  updated: 2020-04-18 21:13:52
+- id: 40
+  title: testbucket40
+  project_id: 2
+  created_by_id: 1
+  position: 10
+  created: 2020-04-18 21:13:52
+  updated: 2020-04-18 21:13:52

pkg/db/fixtures/projects.yml

@@ -16,6 +16,7 @@
   owner_id: 3
   position: 2
   done_bucket_id: 4
+  default_bucket_id: 40
   updated: 2018-12-02 15:13:12
   created: 2018-12-01 15:13:12
 -

pkg/models/tasks.go

@@ -657,7 +657,8 @@ func setTaskBucket(s *xorm.Session, task *Task, originalTask *Task, doCheckBucke
 	}
 
 	var bucket *Bucket
-	if task.Done && originalTask != nil && !originalTask.Done {
+	if task.Done && originalTask != nil &&
+		(!originalTask.Done || task.ProjectID != originalTask.ProjectID) {
 		task.BucketID = project.DoneBucketID
 	}
 
@@ -666,7 +667,10 @@ func setTaskBucket(s *xorm.Session, task *Task, originalTask *Task, doCheckBucke
 	}
 
 	// Either no bucket was provided or the task was moved between projects
-	if task.BucketID == 0 || (originalTask != nil && task.ProjectID != 0 && originalTask.ProjectID != task.ProjectID) {
+	// But if the task was moved between projects, don't update the done bucket
+	// because then we have it already updated to the done bucket.
+	if task.BucketID == 0 ||
+		(originalTask != nil && task.ProjectID != 0 && originalTask.ProjectID != task.ProjectID && !task.Done) {
 		task.BucketID, err = getDefaultBucketID(s, project)
 		if err != nil {
 			return
@@ -860,17 +864,18 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
 	// Old task has the stored reminders
 	ot.Reminders = reminders
 
-	targetBucket, err := setTaskBucket(s, t, &ot, t.BucketID != 0 && t.BucketID != ot.BucketID, nil)
+	project, err := GetProjectSimpleByID(s, t.ProjectID)
+	if err != nil {
+		return err
+	}
+
+	targetBucket, err := setTaskBucket(s, t, &ot, t.BucketID != 0 && t.BucketID != ot.BucketID, project)
 	if err != nil {
 		return err
 	}
 
 	// If the task was moved into the done bucket and the task has a repeating cycle we should not update
 	// the bucket.
-	project, err := GetProjectSimpleByID(s, t.ProjectID)
-	if err != nil {
-		return err
-	}
 	if targetBucket.ID == project.DoneBucketID && t.RepeatAfter > 0 {
 		t.Done = true // This will trigger the correct re-scheduling of the task (happening in updateDone later)
 		t.BucketID = ot.BucketID

pkg/models/tasks_test.go

@@ -345,8 +345,7 @@ func TestTask_Update(t *testing.T) {
 		err = s.Commit()
 		require.NoError(t, err)
 
-		assert.Equal(t, int64(4), task.BucketID) // bucket 4 is the default bucket on project 2
+		assert.Equal(t, int64(40), task.BucketID) // bucket 40 is the default bucket on project 2
-		assert.True(t, task.Done)                // bucket 4 is the done bucket, so the task should be marked as done as well
 	})
 	t.Run("marking a task as done should move it to the done bucket", func(t *testing.T) {
 		db.LoadAndAssertFixtures(t)
@@ -387,7 +386,29 @@ func TestTask_Update(t *testing.T) {
 		db.AssertExists(t, "tasks", map[string]interface{}{
 			"id":         1,
 			"project_id": 2,
-			"bucket_id":  4,
+			"bucket_id":  40,
+		}, false)
+	})
+	t.Run("move done task to another project with a done bucket", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		task := &Task{
+			ID:        2,
+			Done:      true,
+			ProjectID: 2,
+		}
+		err := task.Update(s, u)
+		require.NoError(t, err)
+		err = s.Commit()
+		require.NoError(t, err)
+
+		db.AssertExists(t, "tasks", map[string]interface{}{
+			"id":         2,
+			"project_id": 2,
+			"bucket_id":  4, // 4 is the done bucket
+			"done":       true,
 		}, false)
 	})
 	t.Run("repeating tasks should not be moved to the done bucket", func(t *testing.T) {