From d1b87d2705603754a4afb49ae12ea1a1aeb361af Mon Sep 17 00:00:00 2001 From: konrad Date: Wed, 24 Mar 2021 20:16:35 +0000 Subject: [PATCH] Add a "done" option to kanban buckets (#821) Co-authored-by: kolaente Reviewed-on: https://kolaente.dev/vikunja/api/pulls/821 Co-authored-by: konrad Co-committed-by: konrad --- docs/content/doc/usage/errors.md | 9 ++- pkg/db/fixtures/buckets.yml | 2 + pkg/migration/20210321185225.go | 43 ++++++++++ pkg/models/error.go | 29 +++++++ pkg/models/kanban.go | 36 ++++++++- pkg/models/kanban_test.go | 15 ++++ pkg/models/tasks.go | 131 ++++++++++++++++--------------- pkg/models/tasks_test.go | 80 +++++++++++++++++++ pkg/swagger/docs.go | 4 + pkg/swagger/swagger.json | 4 + pkg/swagger/swagger.yaml | 3 + 11 files changed, 288 insertions(+), 68 deletions(-) create mode 100644 pkg/migration/20210321185225.go diff --git a/docs/content/doc/usage/errors.md b/docs/content/doc/usage/errors.md index a418cff800..9b072cd0d9 100644 --- a/docs/content/doc/usage/errors.md +++ b/docs/content/doc/usage/errors.md @@ -131,10 +131,11 @@ This document describes the different errors Vikunja can return. | ErrorCode | HTTP Status Code | Description | |-----------|------------------|-------------| -| 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. | +| 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. | +| 10005 | 412 | There can be only one done bucket per list. | ## Saved Filters diff --git a/pkg/db/fixtures/buckets.yml b/pkg/db/fixtures/buckets.yml index 5c2c62d6eb..7f29402602 100644 --- a/pkg/db/fixtures/buckets.yml +++ b/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 diff --git a/pkg/migration/20210321185225.go b/pkg/migration/20210321185225.go new file mode 100644 index 0000000000..eec75a3dfd --- /dev/null +++ b/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 . + +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 + }, + }) +} diff --git a/pkg/models/error.go b/pkg/models/error.go index d71f5bb785..abcbe8af81 100644 --- a/pkg/models/error.go +++ b/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 // ============= diff --git a/pkg/models/kanban.go b/pkg/models/kanban.go index 1c19e76d67..94d28d5455 100644 --- a/pkg/models/kanban.go +++ b/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 } diff --git a/pkg/models/kanban_test.go b/pkg/models/kanban_test.go index 450afbe3c4..6dc6e6ba76 100644 --- a/pkg/models/kanban_test.go +++ b/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)) + }) } diff --git a/pkg/models/tasks.go b/pkg/models/tasks.go index f0d06b72ad..8295f64b3c 100644 --- a/pkg/models/tasks.go +++ b/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: diff --git a/pkg/models/tasks_test.go b/pkg/models/tasks_test.go index 705df45418..67a5db2839 100644 --- a/pkg/models/tasks_test.go +++ b/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) { diff --git a/pkg/swagger/docs.go b/pkg/swagger/docs.go index 616c9de9e5..3dad52f1d2 100644 --- a/pkg/swagger/docs.go +++ b/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" diff --git a/pkg/swagger/swagger.json b/pkg/swagger/swagger.json index de2b6b423e..218bb867b3 100644 --- a/pkg/swagger/swagger.json +++ b/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" diff --git a/pkg/swagger/swagger.yaml b/pkg/swagger/swagger.yaml index 71b7d6667e..f55d98cbfa 100644 --- a/pkg/swagger/swagger.yaml +++ b/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