From 14d706c91e002b1dfd9639c0e6f8dcbcec4cf2db Mon Sep 17 00:00:00 2001 From: konrad Date: Fri, 4 Sep 2020 14:37:56 +0000 Subject: [PATCH] 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 Reviewed-on: https://kolaente.dev/vikunja/api/pulls/652 --- docs/content/doc/usage/errors.md | 1 + pkg/db/fixtures/buckets.yml | 2 ++ pkg/integrations/task_test.go | 8 ++--- pkg/migration/20200904101559.go | 43 +++++++++++++++++++++++++ pkg/models/error.go | 29 +++++++++++++++++ pkg/models/kanban.go | 3 ++ pkg/models/tasks.go | 54 ++++++++++++++++++++++++++++---- pkg/models/tasks_test.go | 25 +++++++++++++++ pkg/swagger/docs.go | 4 +++ pkg/swagger/swagger.json | 4 +++ pkg/swagger/swagger.yaml | 3 ++ 11 files changed, 166 insertions(+), 10 deletions(-) create mode 100644 pkg/migration/20200904101559.go diff --git a/docs/content/doc/usage/errors.md b/docs/content/doc/usage/errors.md index 4c308aed..3e6650ec 100644 --- a/docs/content/doc/usage/errors.md +++ b/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. | diff --git a/pkg/db/fixtures/buckets.yml b/pkg/db/fixtures/buckets.yml index 890ca7b5..5c2c62d6 100644 --- a/pkg/db/fixtures/buckets.yml +++ b/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 diff --git a/pkg/integrations/task_test.go b/pkg/integrations/task_test.go index 5cb6bca2..62dacf97 100644 --- a/pkg/integrations/task_test.go +++ b/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) { diff --git a/pkg/migration/20200904101559.go b/pkg/migration/20200904101559.go new file mode 100644 index 00000000..6c6ab8f3 --- /dev/null +++ b/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 . + +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 + }, + }) +} diff --git a/pkg/models/error.go b/pkg/models/error.go index 10a2512b..1e95ce93 100644 --- a/pkg/models/error.go +++ b/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.", + } +} diff --git a/pkg/models/kanban.go b/pkg/models/kanban.go index 6dac185f..c07fe769 100644 --- a/pkg/models/kanban.go +++ b/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. diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index 1f082532..05b8d505 100644 --- a/pkg/models/tasks.go +++ b/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: diff --git a/pkg/models/tasks_test.go b/pkg/models/tasks_test.go index cf47f108..80f8b101 100644 --- a/pkg/models/tasks_test.go +++ b/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) { diff --git a/pkg/swagger/docs.go b/pkg/swagger/docs.go index 38d56d3d..ed155cbc 100644 --- a/pkg/swagger/docs.go +++ b/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" diff --git a/pkg/swagger/swagger.json b/pkg/swagger/swagger.json index 34d95888..1b88b067 100644 --- a/pkg/swagger/swagger.json +++ b/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" diff --git a/pkg/swagger/swagger.yaml b/pkg/swagger/swagger.yaml index 1b4038c9..df96a673 100644 --- a/pkg/swagger/swagger.yaml +++ b/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