Kanban bucket limits (#652)

Fix integration tests

Generate swagger docs

Add test for moving a task between buckets

Add check for bucket limit when updating a task

Add fixture to ensure a bucket with a high limit will never exceed the limit

Refactor bucket limit check into seperate function

Add test for creating and fix

Fix unexported field

Add error in case a task was added to a bucket which has its limit already exceeded

Add migration to add new task field

Add limit field to buckets

Co-authored-by: kolaente <k@knt.li>
Reviewed-on: https://kolaente.dev/vikunja/api/pulls/652

docs/content/doc/usage/errors.md

@@ -134,3 +134,4 @@ This document describes the different errors Vikunja can return.
 | 10001 | 404 | The bucket does not exist. | 
 | 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. | 

pkg/db/fixtures/buckets.yml

@@ -2,12 +2,14 @@
   title: testbucket1
   list_id: 1
   created_by_id: 1
+  limit: 9999999 # This bucket has a limit we will never exceed in the tests to make sure the logic allows for buckets with limits
   created: 2020-04-18 21:13:52
   updated: 2020-04-18 21:13:52
 - id: 2
   title: testbucket2
   list_id: 1
   created_by_id: 1
+  limit: 3
   created: 2020-04-18 21:13:52
   updated: 2020-04-18 21:13:52
 - id: 3

pkg/integrations/task_test.go

@@ -289,9 +289,9 @@ func TestTask(t *testing.T) {
 		})
 		t.Run("Bucket", func(t *testing.T) {
 			t.Run("Normal", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"listtask": "1"}, `{"bucket_id":2}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"listtask": "1"}, `{"bucket_id":3}`)
 				assert.NoError(t, err)
-				assert.Contains(t, rec.Body.String(), `"bucket_id":2`)
+				assert.Contains(t, rec.Body.String(), `"bucket_id":3`)
 				assert.NotContains(t, rec.Body.String(), `"bucket_id":1`)
 			})
 			t.Run("Different List", func(t *testing.T) {
@@ -472,9 +472,9 @@ func TestTask(t *testing.T) {
 		})
 		t.Run("Bucket", func(t *testing.T) {
 			t.Run("Normal", func(t *testing.T) {
-				rec, err := testHandler.testCreateWithUser(nil, map[string]string{"list": "1"}, `{"title":"Lorem Ipsum","bucket_id":2}`)
+				rec, err := testHandler.testCreateWithUser(nil, map[string]string{"list": "1"}, `{"title":"Lorem Ipsum","bucket_id":3}`)
 				assert.NoError(t, err)
-				assert.Contains(t, rec.Body.String(), `"bucket_id":2`)
+				assert.Contains(t, rec.Body.String(), `"bucket_id":3`)
 				assert.NotContains(t, rec.Body.String(), `"bucket_id":1`)
 			})
 			t.Run("Different List", func(t *testing.T) {

pkg/migration/20200904101559.go

@@ -0,0 +1,43 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-2020 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 General Public License 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 General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package migration
+
+import (
+	"src.techknowlogick.com/xormigrate"
+	"xorm.io/xorm"
+)
+
+type buckets20200904101559 struct {
+	Limit int64 `xorm:"default 0" json:"limit"`
+}
+
+func (buckets20200904101559) TableName() string {
+	return "buckets"
+}
+
+func init() {
+	migrations = append(migrations, &xormigrate.Migration{
+		ID:          "20200904101559",
+		Description: "",
+		Migrate: func(tx *xorm.Engine) error {
+			return tx.Sync2(buckets20200904101559{})
+		},
+		Rollback: func(tx *xorm.Engine) error {
+			return nil
+		},
+	})
+}

pkg/models/error.go

@@ -1334,3 +1334,32 @@ func (err ErrCannotRemoveLastBucket) HTTPError() web.HTTPError {
 		Message:  "You cannot remove the last bucket on this list.",
 	}
 }
+
+// ErrBucketLimitExceeded represents an error where a task is being created or moved to a bucket which has its limit already exceeded.
+type ErrBucketLimitExceeded struct {
+	BucketID int64
+	Limit    int64
+	TaskID   int64 // may be 0
+}
+
+// IsErrBucketLimitExceeded checks if an error is ErrBucketLimitExceeded.
+func IsErrBucketLimitExceeded(err error) bool {
+	_, ok := err.(ErrBucketLimitExceeded)
+	return ok
+}
+
+func (err ErrBucketLimitExceeded) Error() string {
+	return fmt.Sprintf("Cannot add a task to this bucket because it would exceed the limit [BucketID: %d, Limit: %d, TaskID: %d]", err.BucketID, err.Limit, err.TaskID)
+}
+
+// ErrCodeBucketLimitExceeded holds the unique world-error code of this error
+const ErrCodeBucketLimitExceeded = 10004
+
+// HTTPError holds the http error description
+func (err ErrBucketLimitExceeded) HTTPError() web.HTTPError {
+	return web.HTTPError{
+		HTTPCode: http.StatusPreconditionFailed,
+		Code:     ErrCodeBucketLimitExceeded,
+		Message:  "You cannot add the task to this bucket as it already exceeded the limit of tasks it can hold.",
+	}
+}

pkg/models/kanban.go

@@ -35,6 +35,9 @@ type Bucket struct {
 	// All tasks which belong to this bucket.
 	Tasks []*Task `xorm:"-" json:"tasks"`
 
+	// How many tasks can be at the same time on this board max
+	Limit int64 `xorm:"default 0" json:"limit"`
+
 	// A timestamp when this bucket was created. You cannot change this value.
 	Created time.Time `xorm:"created not null" json:"created"`
 	// A timestamp when this bucket was last updated. You cannot change this value.

pkg/models/tasks.go

@@ -545,6 +545,32 @@ func checkBucketAndTaskBelongToSameList(s *xorm.Session, fullTask *Task, bucketI
 	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).
+			Count(&Task{})
+		if err != nil {
+			return err
+		}
+		if taskCount >= bucket.Limit {
+			return ErrBucketLimitExceeded{TaskID: t.ID, BucketID: bucket.ID, Limit: bucket.Limit}
+		}
+	}
+	return nil
+}
+
 // Create is the implementation to create a list task
 // @Summary Create a task
 // @Description Inserts a task into a list.
@@ -608,12 +634,18 @@ func createTask(s *xorm.Session, t *Task, a web.Auth, updateAssignees bool) (err
 	}
 
 	// Get the default bucket and move the task there
+	var bucket *Bucket
 	if t.BucketID == 0 {
-		defaultBucket, err := getDefaultBucket(s, t.ListID)
+		bucket, err = getDefaultBucket(s, t.ListID)
 		if err != nil {
 			return err
 		}
-		t.BucketID = defaultBucket.ID
+		t.BucketID = bucket.ID
+	}
+
+	// Bucket Limit
+	if err := checkBucketLimit(s, t, bucket); err != nil {
+		return err
 	}
 
 	// Get the index for this task
@@ -730,15 +762,19 @@ func (t *Task) Update() (err error) {
 		"repeat_from_current_date",
 	}
 
-	// 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 {
-		b, err := getDefaultBucket(s, t.ListID)
+	// 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 {
 			_ = s.Rollback()
 			return err
 		}
-		t.BucketID = b.ID
+		t.BucketID = bucket.ID
+	}
 
+	// 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{}
 		_, err = s.Where("list_id = ?", t.ListID).OrderBy("id desc").Get(latestTask)
 		if err != nil {
@@ -750,6 +786,12 @@ func (t *Task) Update() (err error) {
 		colsToUpdate = append(colsToUpdate, "index")
 	}
 
+	// Check the bucket limit
+	if err := checkBucketLimit(s, t, bucket); err != nil {
+		_ = s.Rollback()
+		return err
+	}
+
 	// Update the labels
 	//
 	// Maybe FIXME:

pkg/models/tasks_test.go

@@ -85,6 +85,18 @@ func TestTask_Create(t *testing.T) {
 		assert.Error(t, err)
 		assert.True(t, user.IsErrUserDoesNotExist(err))
 	})
+	t.Run("full bucket", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		task := &Task{
+			Title:       "Lorem",
+			Description: "Lorem Ipsum Dolor",
+			ListID:      1,
+			BucketID:    2, // Bucket 2 already has 3 tasks and a limit of 3
+		}
+		err := task.Create(usr)
+		assert.Error(t, err)
+		assert.True(t, IsErrBucketLimitExceeded(err))
+	})
 }
 
 func TestTask_Update(t *testing.T) {
@@ -111,6 +123,19 @@ func TestTask_Update(t *testing.T) {
 		assert.Error(t, err)
 		assert.True(t, IsErrTaskDoesNotExist(err))
 	})
+	t.Run("full bucket", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		task := &Task{
+			ID:          1,
+			Title:       "test10000",
+			Description: "Lorem Ipsum Dolor",
+			ListID:      1,
+			BucketID:    2, // Bucket 2 already has 3 tasks and a limit of 3
+		}
+		err := task.Update()
+		assert.Error(t, err)
+		assert.True(t, IsErrBucketLimitExceeded(err))
+	})
 }
 
 func TestTask_Delete(t *testing.T) {

pkg/swagger/docs.go

@@ -6236,6 +6236,10 @@ var doc = `{
                     "description": "The unique, numeric id of this bucket.",
                     "type": "integer"
                 },
+                "limit": {
+                    "description": "How many tasks can be at the same time on this board max",
+                    "type": "integer"
+                },
                 "list_id": {
                     "description": "The list this bucket belongs to.",
                     "type": "integer"

pkg/swagger/swagger.json

@@ -6219,6 +6219,10 @@
                     "description": "The unique, numeric id of this bucket.",
                     "type": "integer"
                 },
+                "limit": {
+                    "description": "How many tasks can be at the same time on this board max",
+                    "type": "integer"
+                },
                 "list_id": {
                     "description": "The list this bucket belongs to.",
                     "type": "integer"

pkg/swagger/swagger.yaml

@@ -53,6 +53,9 @@ definitions:
       id:
         description: The unique, numeric id of this bucket.
         type: integer
+      limit:
+        description: How many tasks can be at the same time on this board max
+        type: integer
       list_id:
         description: The list this bucket belongs to.
         type: integer