From fc92fd3325d4d79b04aa30990d09d8d19b550372 Mon Sep 17 00:00:00 2001
From: kolaente <k@knt.li>
Date: Sun, 21 Mar 2021 22:07:06 +0100
Subject: [PATCH] Refactor checking if bucket belongs to the same list when
 moving a task

---
 pkg/models/tasks.go      | 68 +++++++++++++++++++++-------------------
 pkg/models/tasks_test.go | 16 ++++++++++
 2 files changed, 52 insertions(+), 32 deletions(-)

diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go
index f0d06b72a..e43e0b1a1 100644
--- a/pkg/models/tasks.go
+++ b/pkg/models/tasks.go
@@ -707,33 +707,19 @@ func addMoreInfoToTasks(s *xorm.Session, taskMap map[int64]*Task) (err error) {
 	return
 }
 
-func checkBucketAndTaskBelongToSameList(s *xorm.Session, fullTask *Task, bucketID int64) (err error) {
-	if bucketID != 0 {
-		b, err := getBucketByID(s, bucketID)
-		if err != nil {
-			return err
-		}
-		if fullTask.ListID != b.ListID {
-			return ErrBucketDoesNotBelongToList{
-				ListID:   fullTask.ListID,
-				BucketID: fullTask.BucketID,
-			}
+func checkBucketAndTaskBelongToSameList(s *xorm.Session, fullTask *Task, bucket *Bucket) (err error) {
+	if fullTask.ListID != bucket.ListID {
+		return ErrBucketDoesNotBelongToList{
+			ListID:   fullTask.ListID,
+			BucketID: fullTask.BucketID,
 		}
 	}
+
 	return
 }
 
 // Checks if adding a new task would exceed the bucket limit
 func checkBucketLimit(s *xorm.Session, t *Task, bucket *Bucket) (err error) {
-
-	// We need the bucket to check if it has more tasks than the limit allows
-	if bucket == nil {
-		bucket, err = getBucketByID(s, t.BucketID)
-		if err != nil {
-			return err
-		}
-	}
-
 	// Check the limit
 	if bucket.Limit > 0 {
 		taskCount, err := s.
@@ -799,12 +785,6 @@ func createTask(s *xorm.Session, t *Task, a web.Auth, updateAssignees bool) (err
 		t.UID = utils.MakeRandomString(40)
 	}
 
-	// If there is a bucket set, make sure they belong to the same list as the task
-	err = checkBucketAndTaskBelongToSameList(s, t, t.BucketID)
-	if err != nil {
-		return
-	}
-
 	// Get the default bucket and move the task there
 	var bucket *Bucket
 	if t.BucketID == 0 {
@@ -815,6 +795,19 @@ func createTask(s *xorm.Session, t *Task, a web.Auth, updateAssignees bool) (err
 		t.BucketID = bucket.ID
 	}
 
+	if bucket == nil {
+		bucket, err = getBucketByID(s, t.BucketID)
+		if err != nil {
+			return err
+		}
+	}
+
+	// If there is a bucket set, make sure they belong to the same list as the task
+	err = checkBucketAndTaskBelongToSameList(s, t, bucket)
+	if err != nil {
+		return
+	}
+
 	// Bucket Limit
 	if err := checkBucketLimit(s, t, bucket); err != nil {
 		return err
@@ -886,6 +879,10 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
 		return
 	}
 
+	if t.ListID == 0 {
+		t.ListID = ot.ListID
+	}
+
 	// Get the reminders
 	reminders, err := getRemindersForTasks(s, []int64{t.ID})
 	if err != nil {
@@ -910,12 +907,6 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
 		return err
 	}
 
-	// If there is a bucket set, make sure they belong to the same list as the task
-	err = checkBucketAndTaskBelongToSameList(s, &ot, t.BucketID)
-	if err != nil {
-		return
-	}
-
 	// All columns to update in a separate variable to be able to add to them
 	colsToUpdate := []string{
 		"title",
@@ -946,6 +937,19 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
 		t.BucketID = bucket.ID
 	}
 
+	if bucket == nil {
+		bucket, err = getBucketByID(s, t.BucketID)
+		if err != nil {
+			return err
+		}
+	}
+
+	// If there is a bucket set, make sure they belong to the same list as the task
+	err = checkBucketAndTaskBelongToSameList(s, &ot, bucket)
+	if err != nil {
+		return
+	}
+
 	// If the task is being moved between lists, make sure to move the bucket + index as well
 	if t.ListID != 0 && ot.ListID != t.ListID {
 		latestTask := &Task{}
diff --git a/pkg/models/tasks_test.go b/pkg/models/tasks_test.go
index 705df4541..33a2fbcb6 100644
--- a/pkg/models/tasks_test.go
+++ b/pkg/models/tasks_test.go
@@ -202,6 +202,22 @@ func TestTask_Update(t *testing.T) {
 		err := task.Update(s, u)
 		assert.NoError(t, err)
 	})
+	t.Run("bucket on other list", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		task := &Task{
+			ID:          1,
+			Title:       "test10000",
+			Description: "Lorem Ipsum Dolor",
+			ListID:      1,
+			BucketID:    4, // Bucket 4 belongs to list 2
+		}
+		err := task.Update(s, u)
+		assert.Error(t, err)
+		assert.True(t, IsErrBucketDoesNotBelongToList(err))
+	})
 }
 
 func TestTask_Delete(t *testing.T) {