Add a "done" option to kanban buckets (#821)

Co-authored-by: kolaente <k@knt.li>
Reviewed-on: https://kolaente.dev/vikunja/api/pulls/821
Co-authored-by: konrad <konrad@kola-entertainments.de>
Co-committed-by: konrad <konrad@kola-entertainments.de>

docs/content/doc/usage/errors.md

@@ -135,6 +135,7 @@ This document describes the different errors Vikunja can return.
 | 10002 | 400 | The bucket does not belong to that list. |
 | 10003 | 412 | You cannot remove the last bucket on a list. |
 | 10004 | 412 | You cannot add the task to this bucket as it already exceeded the limit of tasks it can hold. |
+| 10005 | 412 | There can be only one done bucket per list. |
 
 ## Saved Filters
 

pkg/db/fixtures/buckets.yml

@@ -16,12 +16,14 @@
   title: testbucket3
   list_id: 1
   created_by_id: 1
+  is_done_bucket: 1
   created: 2020-04-18 21:13:52
   updated: 2020-04-18 21:13:52
 - id: 4
   title: testbucket4 - other list
   list_id: 2
   created_by_id: 1
+  is_done_bucket: 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

pkg/migration/20210321185225.go

@@ -0,0 +1,43 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-2021 Vikunja and contributors. All rights reserved.
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public Licensee as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public Licensee for more details.
+//
+// You should have received a copy of the GNU Affero General Public Licensee
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package migration
+
+import (
+	"src.techknowlogick.com/xormigrate"
+	"xorm.io/xorm"
+)
+
+type buckets20210321185225 struct {
+	IsDoneBucket bool `xorm:"BOOL" json:"is_done_column"`
+}
+
+func (buckets20210321185225) TableName() string {
+	return "buckets"
+}
+
+func init() {
+	migrations = append(migrations, &xormigrate.Migration{
+		ID:          "20210321185225",
+		Description: "Add is done bucket to buckets",
+		Migrate: func(tx *xorm.Engine) error {
+			return tx.Sync2(buckets20210321185225{})
+		},
+		Rollback: func(tx *xorm.Engine) error {
+			return nil
+		},
+	})
+}

pkg/models/error.go

@@ -1365,6 +1365,35 @@ func (err ErrBucketLimitExceeded) HTTPError() web.HTTPError {
 	}
 }
 
+// ErrOnlyOneDoneBucketPerList represents an error where a bucket is set to the done bucket but one already exists for its list.
+type ErrOnlyOneDoneBucketPerList struct {
+	BucketID     int64
+	ListID       int64
+	DoneBucketID int64
+}
+
+// IsErrOnlyOneDoneBucketPerList checks if an error is ErrBucketLimitExceeded.
+func IsErrOnlyOneDoneBucketPerList(err error) bool {
+	_, ok := err.(*ErrOnlyOneDoneBucketPerList)
+	return ok
+}
+
+func (err *ErrOnlyOneDoneBucketPerList) Error() string {
+	return fmt.Sprintf("There can be only one done bucket per list [BucketID: %d, ListID: %d]", err.BucketID, err.ListID)
+}
+
+// ErrCodeOnlyOneDoneBucketPerList holds the unique world-error code of this error
+const ErrCodeOnlyOneDoneBucketPerList = 10005
+
+// HTTPError holds the http error description
+func (err *ErrOnlyOneDoneBucketPerList) HTTPError() web.HTTPError {
+	return web.HTTPError{
+		HTTPCode: http.StatusPreconditionFailed,
+		Code:     ErrCodeOnlyOneDoneBucketPerList,
+		Message:  "There can be only one done bucket per list.",
+	}
+}
+
 // =============
 // Saved Filters
 // =============

pkg/models/kanban.go

@@ -38,6 +38,8 @@ type Bucket struct {
 
 	// How many tasks can be at the same time on this board max
 	Limit int64 `xorm:"default 0" json:"limit"`
+	// If this bucket is the "done bucket". All tasks moved into this bucket will automatically marked as done. All tasks marked as done from elsewhere will be moved into this bucket.
+	IsDoneBucket bool `xorm:"BOOL" json:"is_done_bucket"`
 
 	// A timestamp when this bucket was created. You cannot change this value.
 	Created time.Time `xorm:"created not null" json:"created"`
@@ -81,6 +83,21 @@ func getDefaultBucket(s *xorm.Session, listID int64) (bucket *Bucket, err error)
 	return
 }
 
+func getDoneBucketForList(s *xorm.Session, listID int64) (bucket *Bucket, err error) {
+	bucket = &Bucket{}
+	exists, err := s.
+		Where("list_id = ? and is_done_bucket = ?", listID, true).
+		Get(bucket)
+	if err != nil {
+		return nil, err
+	}
+	if !exists {
+		bucket = nil
+	}
+
+	return
+}
+
 // ReadAll returns all buckets with their tasks for a certain list
 // @Summary Get all kanban buckets of a list
 // @Description Returns all kanban buckets with belong to a list including their tasks.
@@ -239,9 +256,26 @@ func (b *Bucket) Create(s *xorm.Session, a web.Auth) (err error) {
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /lists/{listID}/buckets/{bucketID} [post]
 func (b *Bucket) Update(s *xorm.Session, a web.Auth) (err error) {
+	doneBucket, err := getDoneBucketForList(s, b.ListID)
+	if err != nil {
+		return err
+	}
+
+	if doneBucket != nil && doneBucket.IsDoneBucket && b.IsDoneBucket {
+		return &ErrOnlyOneDoneBucketPerList{
+			BucketID:     b.ID,
+			ListID:       b.ListID,
+			DoneBucketID: doneBucket.ID,
+		}
+	}
+
 	_, err = s.
 		Where("id = ?", b.ID).
-		Cols("title", "limit").
+		Cols(
+			"title",
+			"limit",
+			"is_done_bucket",
+		).
 		Update(b)
 	return
 }

pkg/models/kanban_test.go

@@ -182,4 +182,19 @@ func TestBucket_Update(t *testing.T) {
 
 		testAndAssertBucketUpdate(t, b, s)
 	})
+	t.Run("only one done bucket per list", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		b := &Bucket{
+			ID:           1,
+			ListID:       1,
+			IsDoneBucket: true,
+		}
+
+		err := b.Update(s, &user.User{ID: 1})
+		assert.Error(t, err)
+		assert.True(t, IsErrOnlyOneDoneBucketPerList(err))
+	})
 }

pkg/models/tasks.go

@@ -707,34 +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(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.
 			Where("bucket_id = ?", bucket.ID).
@@ -749,6 +734,56 @@ func checkBucketLimit(s *xorm.Session, t *Task, bucket *Bucket) (err error) {
 	return nil
 }
 
+// Contains all the task logic to figure out what bucket to use for this task.
+func setTaskBucket(s *xorm.Session, task *Task, originalTask *Task, doCheckBucketLimit bool) (err error) {
+	// Make sure we have a bucket
+	var bucket *Bucket
+	if task.Done && originalTask != nil && !originalTask.Done {
+		bucket, err := getDoneBucketForList(s, task.ListID)
+		if err != nil {
+			return err
+		}
+		if bucket != nil {
+			task.BucketID = bucket.ID
+		}
+	}
+
+	if task.BucketID == 0 || (originalTask != nil && task.ListID != 0 && originalTask.ListID != task.ListID) {
+		bucket, err = getDefaultBucket(s, task.ListID)
+		if err != nil {
+			return err
+		}
+		task.BucketID = bucket.ID
+	}
+
+	if bucket == nil {
+		bucket, err = getBucketByID(s, task.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(task, bucket)
+	if err != nil {
+		return
+	}
+
+	// Check the bucket limit
+	// Only check the bucket limit if the task is being moved between buckets, allow reordering the task within a bucket
+	if doCheckBucketLimit {
+		if err := checkBucketLimit(s, task, bucket); err != nil {
+			return err
+		}
+	}
+
+	if bucket.IsDoneBucket && originalTask != nil && !originalTask.Done {
+		task.Done = true
+	}
+
+	return nil
+}
+
 // Create is the implementation to create a list task
 // @Summary Create a task
 // @Description Inserts a task into a list.
@@ -799,27 +834,12 @@ 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)
+	// Get the default bucket and move the task there
+	err = setTaskBucket(s, t, nil, true)
 	if err != nil {
 		return
 	}
 
-	// Get the default bucket and move the task there
-	var bucket *Bucket
-	if t.BucketID == 0 {
-		bucket, err = getDefaultBucket(s, t.ListID)
-		if err != nil {
-			return err
-		}
-		t.BucketID = bucket.ID
-	}
-
-	// Bucket Limit
-	if err := checkBucketLimit(s, t, bucket); err != nil {
-		return err
-	}
-
 	// Get the index for this task
 	latestTask := &Task{}
 	_, err = s.Where("list_id = ?", t.ListID).OrderBy("id desc").Get(latestTask)
@@ -886,6 +906,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 {
@@ -897,9 +921,6 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
 		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)
-
 	// Update the assignees
 	if err := ot.updateTaskAssignees(s, t.Assignees, a); err != nil {
 		return err
@@ -910,12 +931,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",
@@ -936,16 +951,14 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
 		"is_favorite",
 	}
 
-	// Make sure we have a bucket
-	var bucket *Bucket
-	if t.BucketID == 0 || (t.ListID != 0 && ot.ListID != t.ListID) {
-		bucket, err = getDefaultBucket(s, t.ListID)
-		if err != nil {
-			return err
-		}
-		t.BucketID = bucket.ID
+	err = setTaskBucket(s, t, &ot, t.BucketID != ot.BucketID)
+	if err != nil {
+		return err
 	}
 
+	// When a repeating task is marked as done, we update all deadlines and reminders and set it as undone
+	updateDone(&ot, t)
+
 	// 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{}
@@ -958,14 +971,6 @@ func (t *Task) Update(s *xorm.Session, a web.Auth) (err error) {
 		colsToUpdate = append(colsToUpdate, "index")
 	}
 
-	// Check the bucket limit
-	// Only check the bucket limit if the task is being moved between buckets, allow reordering the task within a bucket
-	if t.BucketID != ot.BucketID {
-		if err := checkBucketLimit(s, t, bucket); err != nil {
-			return err
-		}
-	}
-
 	// Update the labels
 	//
 	// Maybe FIXME:

pkg/models/tasks_test.go

@@ -202,6 +202,86 @@ 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))
+	})
+	t.Run("moving a task to the done bucket", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		task := &Task{
+			ID:       1,
+			Title:    "test",
+			ListID:   1,
+			BucketID: 3, // Bucket 3 is the done bucket
+		}
+		err := task.Update(s, u)
+		assert.NoError(t, err)
+		err = s.Commit()
+		assert.NoError(t, err)
+		assert.True(t, task.Done)
+
+		db.AssertExists(t, "tasks", map[string]interface{}{
+			"id":        1,
+			"done":      true,
+			"title":     "test",
+			"list_id":   1,
+			"bucket_id": 3,
+		}, false)
+	})
+	t.Run("default bucket when moving a task between lists", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		task := &Task{
+			ID:     1,
+			ListID: 2,
+		}
+		err := task.Update(s, u)
+		assert.NoError(t, err)
+		err = s.Commit()
+		assert.NoError(t, err)
+
+		assert.Equal(t, int64(4), task.BucketID) // bucket 4 is the default bucket on list 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)
+		s := db.NewSession()
+		defer s.Close()
+
+		task := &Task{
+			ID:   1,
+			Done: true,
+		}
+		err := task.Update(s, u)
+		assert.NoError(t, err)
+		err = s.Commit()
+		assert.NoError(t, err)
+		assert.True(t, task.Done)
+		assert.Equal(t, int64(3), task.BucketID)
+
+		db.AssertExists(t, "tasks", map[string]interface{}{
+			"id":        1,
+			"done":      true,
+			"bucket_id": 3,
+		}, false)
+	})
 }
 
 func TestTask_Delete(t *testing.T) {

pkg/swagger/docs.go

@@ -7189,6 +7189,10 @@ var doc = `{
                     "description": "The unique, numeric id of this bucket.",
                     "type": "integer"
                 },
+                "is_done_bucket": {
+                    "description": "If this bucket is the \"done bucket\". All tasks moved into this bucket will automatically marked as done. All tasks marked as done from elsewhere will be moved into this bucket.",
+                    "type": "boolean"
+                },
                 "limit": {
                     "description": "How many tasks can be at the same time on this board max",
                     "type": "integer"

pkg/swagger/swagger.json

@@ -7172,6 +7172,10 @@
                     "description": "The unique, numeric id of this bucket.",
                     "type": "integer"
                 },
+                "is_done_bucket": {
+                    "description": "If this bucket is the \"done bucket\". All tasks moved into this bucket will automatically marked as done. All tasks marked as done from elsewhere will be moved into this bucket.",
+                    "type": "boolean"
+                },
                 "limit": {
                     "description": "How many tasks can be at the same time on this board max",
                     "type": "integer"

pkg/swagger/swagger.yaml

@@ -81,6 +81,9 @@ definitions:
       id:
         description: The unique, numeric id of this bucket.
         type: integer
+      is_done_bucket:
+        description: If this bucket is the "done bucket". All tasks moved into this bucket will automatically marked as done. All tasks marked as done from elsewhere will be moved into this bucket.
+        type: boolean
       limit:
         description: How many tasks can be at the same time on this board max
         type: integer