diff --git a/frontend/src/i18n/lang/el-GR.json b/frontend/src/i18n/lang/el-GR.json index 14837735b..ba98141ec 100644 --- a/frontend/src/i18n/lang/el-GR.json +++ b/frontend/src/i18n/lang/el-GR.json @@ -393,6 +393,7 @@ "title": "Αντιγραφή του έργου", "label": "Αντιγραφή", "text": "Επιλέξτε ένα γονικό έργο που θα περιλαμβάνει το αντίγραφο του έργου:", + "shares": "Αντιγραφή διαμοιρασμών (χρήστες, ομάδες και σύνδεσμοι διαμοιρασμού) στο αντίγραφο", "success": "Το έργο αντιγράφηκε με επιτυχία." }, "edit": { diff --git a/pkg/modules/migration/todoist/todoist.go b/pkg/modules/migration/todoist/todoist.go index d96338197..4e0884de0 100644 --- a/pkg/modules/migration/todoist/todoist.go +++ b/pkg/modules/migration/todoist/todoist.go @@ -22,8 +22,10 @@ import ( "fmt" "net/http" "net/url" + "regexp" "sort" "strconv" + "strings" "time" "code.vikunja.io/api/pkg/config" @@ -253,6 +255,72 @@ func parseDate(dateString string) (date time.Time, err error) { return date, err } +// Matching the existing migration importers, months are treated as 30 days and years as 365. +const ( + secondsPerDay int64 = 60 * 60 * 24 + secondsPerWeek = secondsPerDay * 7 + secondsPerMonth = secondsPerDay * 30 + secondsPerYear = secondsPerDay * 365 +) + +var repeatUnitSeconds = map[string]int64{ + "day": secondsPerDay, + "week": secondsPerWeek, + "month": secondsPerMonth, + "year": secondsPerYear, +} + +var ( + todoistRepeatRegex = regexp.MustCompile(`^(?:every\s+)?(?:(\d+)\s+|(other)\s+)?(day|week|month|year)s?$`) + todoistRepeatTimeRegex = regexp.MustCompile(`\s+(?:at|@)\s+.*$`) +) + +// parseTodoistRepeat translates Todoist's recurrence into a repeat interval in seconds. +// Todoist exposes recurrence only as free text (e.g. "every 3 weeks"), so we parse the +// common, unambiguous interval phrases. Patterns we can't represent (specific weekdays, +// days of the month, non-English strings) return 0, leaving the task non-repeating. Only +// the cadence is kept - the due date already anchors the actual day and time. +func parseTodoistRepeat(due *dueDate) int64 { + if due == nil || !due.IsRecurring { + return 0 + } + + s := strings.ToLower(strings.TrimSpace(due.String)) + // The time of day is already on the due date, drop it so "every day at 9am" still matches. + s = todoistRepeatTimeRegex.ReplaceAllString(s, "") + + switch s { + case "daily": + return secondsPerDay + case "weekly": + return secondsPerWeek + case "monthly": + return secondsPerMonth + case "yearly", "annually": + return secondsPerYear + } + + matches := todoistRepeatRegex.FindStringSubmatch(s) + if matches == nil { + log.Debugf("[Todoist Migration] Could not parse recurrence %q, leaving task non-repeating", due.String) + return 0 + } + + interval := int64(1) + switch { + case matches[1] != "": + n, err := strconv.ParseInt(matches[1], 10, 64) + if err != nil || n < 1 { + return 0 + } + interval = n + case matches[2] == "other": + interval = 2 + } + + return interval * repeatUnitSeconds[matches[3]] +} + func convertTodoistToVikunja(sync *sync, doneItems map[string]*doneItem) (fullVikunjaHierachie []*models.ProjectWithTasksAndBuckets, err error) { var pseudoParentID int64 = 1 @@ -358,6 +426,7 @@ func convertTodoistToVikunja(sync *sync, doneItems map[string]*doneItem) (fullVi return nil, err } task.DueDate = dueDate.In(config.GetTimeZone()) + task.RepeatAfter = parseTodoistRepeat(i.Due) } // Put all labels together from earlier diff --git a/pkg/modules/migration/todoist/todoist_test.go b/pkg/modules/migration/todoist/todoist_test.go index 100b4a885..c1c7322c1 100644 --- a/pkg/modules/migration/todoist/todoist_test.go +++ b/pkg/modules/migration/todoist/todoist_test.go @@ -651,3 +651,47 @@ func TestConvertTodoistToVikunja(t *testing.T) { t.Errorf("converted todoist data = %v, want %v, diff: %v", hierachie, expectedHierachie, diff) } } + +func TestParseTodoistRepeat(t *testing.T) { + tests := []struct { + name string + due *dueDate + want int64 + }{ + {name: "nil due", due: nil, want: 0}, + {name: "not recurring", due: &dueDate{String: "every day", IsRecurring: false}, want: 0}, + + {name: "every day", due: &dueDate{String: "every day", IsRecurring: true}, want: secondsPerDay}, + {name: "daily", due: &dueDate{String: "daily", IsRecurring: true}, want: secondsPerDay}, + {name: "every other day", due: &dueDate{String: "every other day", IsRecurring: true}, want: 2 * secondsPerDay}, + {name: "every 3 days", due: &dueDate{String: "every 3 days", IsRecurring: true}, want: 3 * secondsPerDay}, + + {name: "every week", due: &dueDate{String: "every week", IsRecurring: true}, want: secondsPerWeek}, + {name: "weekly", due: &dueDate{String: "weekly", IsRecurring: true}, want: secondsPerWeek}, + {name: "every other week", due: &dueDate{String: "every other week", IsRecurring: true}, want: 2 * secondsPerWeek}, + {name: "every 2 weeks", due: &dueDate{String: "every 2 weeks", IsRecurring: true}, want: 2 * secondsPerWeek}, + + {name: "every month", due: &dueDate{String: "every month", IsRecurring: true}, want: secondsPerMonth}, + {name: "monthly", due: &dueDate{String: "monthly", IsRecurring: true}, want: secondsPerMonth}, + {name: "every 3 months", due: &dueDate{String: "every 3 months", IsRecurring: true}, want: 3 * secondsPerMonth}, + + {name: "every year", due: &dueDate{String: "every year", IsRecurring: true}, want: secondsPerYear}, + {name: "yearly", due: &dueDate{String: "yearly", IsRecurring: true}, want: secondsPerYear}, + {name: "annually", due: &dueDate{String: "annually", IsRecurring: true}, want: secondsPerYear}, + + {name: "case insensitive", due: &dueDate{String: "Every Day", IsRecurring: true}, want: secondsPerDay}, + {name: "time of day stripped", due: &dueDate{String: "every day at 9am", IsRecurring: true}, want: secondsPerDay}, + + // Tier 1 doesn't understand these, so the task stays non-repeating. + {name: "specific weekday", due: &dueDate{String: "every monday", IsRecurring: true}, want: 0}, + {name: "day of month", due: &dueDate{String: "every 27th", IsRecurring: true}, want: 0}, + {name: "non-english", due: &dueDate{String: "cada día", IsRecurring: true}, want: 0}, + {name: "gibberish", due: &dueDate{String: "whenever", IsRecurring: true}, want: 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, parseTodoistRepeat(tt.due)) + }) + } +} diff --git a/pkg/routes/api/v2/buckets.go b/pkg/routes/api/v2/buckets.go new file mode 100644 index 000000000..dcd70d02c --- /dev/null +++ b/pkg/routes/api/v2/buckets.go @@ -0,0 +1,155 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present 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 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 Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package apiv2 + +import ( + "context" + "fmt" + "net/http" + + "code.vikunja.io/api/pkg/models" + "code.vikunja.io/api/pkg/web/handler" + + "github.com/danielgtaylor/huma/v2" +) + +// bucketListBody is the list-response envelope. models.Bucket.ReadAll returns +// []*models.Bucket, so that's the element type. +type bucketListBody struct { + Body Paginated[*models.Bucket] +} + +// RegisterBucketRoutes wires the nested kanban-bucket CRUD onto the Huma API. +// Buckets live under /projects/{project}/views/{view}/buckets; every operation +// binds {project} → ProjectID and {view} → ProjectViewID, the write operations +// additionally {bucket} → ID. There is intentionally no read-one route +// (mirroring v1: the Bucket model has no ReadOne/CanRead), so AutoPatch +// synthesises no PATCH either. +func RegisterBucketRoutes(api huma.API) { + tags := []string{"projects"} + + Register(api, huma.Operation{ + OperationID: "buckets-list", + Summary: "List the buckets of a kanban view", + Description: "Returns all kanban buckets of a project view, ordered by position. Requires read access to the project. The list is not paginated by the server but is returned in the standard list envelope. To get the buckets together with their tasks, use the buckets/tasks endpoint instead.", + Method: http.MethodGet, + Path: "/projects/{project}/views/{view}/buckets", + Tags: tags, + }, bucketsList) + + Register(api, huma.Operation{ + OperationID: "buckets-create", + Summary: "Create a bucket in a kanban view", + Description: "Creates a kanban bucket in the given project view. The project and view come from the URL, not the body. Requires write access to the project.", + Method: http.MethodPost, + Path: "/projects/{project}/views/{view}/buckets", + Tags: tags, + }, bucketsCreate) + + Register(api, huma.Operation{ + OperationID: "buckets-update", + Summary: "Update a bucket of a kanban view", + Description: "Replaces a kanban bucket's title, limit and position. The bucket is identified by the URL, which also scopes it to the project and view. Requires write access to the project.", + Method: http.MethodPut, + Path: "/projects/{project}/views/{view}/buckets/{bucket}", + Tags: tags, + }, bucketsUpdate) + + Register(api, huma.Operation{ + OperationID: "buckets-delete", + Summary: "Delete a bucket of a kanban view", + Description: "Deletes a kanban bucket and moves its tasks to the view's default bucket; no tasks are deleted. You cannot delete the last bucket of a view (rejected with 412). Requires write access to the project.", + Method: http.MethodDelete, + Path: "/projects/{project}/views/{view}/buckets/{bucket}", + Tags: tags, + }, bucketsDelete) +} + +func init() { AddRouteRegistrar(RegisterBucketRoutes) } + +func bucketsList(ctx context.Context, in *struct { + ProjectID int64 `path:"project"` + ViewID int64 `path:"view"` + ListParams +}) (*bucketListBody, error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + result, _, total, err := handler.DoReadAll(ctx, &models.Bucket{ProjectID: in.ProjectID, ProjectViewID: in.ViewID}, a, in.Q, in.Page, in.PerPage) + if err != nil { + return nil, translateDomainError(err) + } + buckets, ok := result.([]*models.Bucket) + if !ok { + return nil, fmt.Errorf("buckets.ReadAll returned unexpected type %T (expected []*models.Bucket)", result) + } + return &bucketListBody{Body: NewPaginated(buckets, total, in.Page, in.PerPage)}, nil +} + +func bucketsCreate(ctx context.Context, in *struct { + ProjectID int64 `path:"project"` + ViewID int64 `path:"view"` + Body models.Bucket +}) (*singleBody[models.Bucket], error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + b := &in.Body + b.ProjectID = in.ProjectID // URL wins over body + b.ProjectViewID = in.ViewID // URL wins over body + if err := handler.DoCreate(ctx, b, a); err != nil { + return nil, translateDomainError(err) + } + return &singleBody[models.Bucket]{Body: b}, nil +} + +func bucketsUpdate(ctx context.Context, in *struct { + ProjectID int64 `path:"project"` + ViewID int64 `path:"view"` + BucketID int64 `path:"bucket"` + Body models.Bucket +}) (*singleBody[models.Bucket], error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + b := &in.Body + b.ID = in.BucketID // URL wins over body + b.ProjectID = in.ProjectID // URL wins over body + b.ProjectViewID = in.ViewID // URL wins over body + if err := handler.DoUpdate(ctx, b, a); err != nil { + return nil, translateDomainError(err) + } + return &singleBody[models.Bucket]{Body: b}, nil +} + +func bucketsDelete(ctx context.Context, in *struct { + ProjectID int64 `path:"project"` + ViewID int64 `path:"view"` + BucketID int64 `path:"bucket"` +}) (*emptyBody, error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + if err := handler.DoDelete(ctx, &models.Bucket{ID: in.BucketID, ProjectID: in.ProjectID, ProjectViewID: in.ViewID}, a); err != nil { + return nil, translateDomainError(err) + } + return &emptyBody{}, nil +} diff --git a/pkg/webtests/huma_bucket_test.go b/pkg/webtests/huma_bucket_test.go new file mode 100644 index 000000000..e643b6ca8 --- /dev/null +++ b/pkg/webtests/huma_bucket_test.go @@ -0,0 +1,265 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present 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 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 Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package webtests + +import ( + "encoding/json" + "net/http" + "testing" + + "code.vikunja.io/api/pkg/db" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestBucket covers the nested kanban-bucket CRUD on /api/v2. Buckets live under +// /projects/{project}/views/{view}/buckets, so the harness binds the project and +// view in basePath and idParam picks {bucket}. +// +// Permission model — Bucket.Can{Create,Update,Delete} all delegate to +// Project.CanUpdate, which resolves to write access (not admin). Bucket.ReadAll +// only needs the view's read access. So write is the boundary for mutation, +// unlike project views where admin is required. +// +// Fixture topology (see pkg/db/fixtures): +// - project 1 (owned by testuser1), kanban view 4: buckets 1, 2, 3. +// - project 2 (owned by user3, no share to testuser1), kanban view 8: +// buckets 4, 40 — the forbidden / non-member negatives. +// - projects 9/10/11 are owned by user6 and shared to testuser1 read/write/admin; +// their kanban views 36/40/44 carry buckets {9,25}/{10,26}/{11,27}. The same +// user exercises every rung by switching the parent path. +func TestHumaBucket(t *testing.T) { + // project 1 is owned by testuser1. + owned := webHandlerTestV2{ + user: &testuser1, + basePath: "/api/v2/projects/1/views/4/buckets", + idParam: "bucket", + t: t, + } + require.NoError(t, owned.ensureEnv()) + // project 2 is owned by user3; testuser1 has no access. Share owned's Echo + // instance: each setupTestEnv() regenerates the global JWT signing secret, + // so two independent harnesses would invalidate each other's tokens. + forbidden := webHandlerTestV2{ + user: &testuser1, + basePath: "/api/v2/projects/2/views/8/buckets", + idParam: "bucket", + t: t, + e: owned.e, + } + // project 9 is shared to testuser1 read-only — enough to list, below the + // write bar mutation requires. + readShared := webHandlerTestV2{ + user: &testuser1, + basePath: "/api/v2/projects/9/views/36/buckets", + idParam: "bucket", + t: t, + e: owned.e, + } + // project 10 is shared with write — the rung that clears Project.CanUpdate, + // so it can create/update/delete buckets. + writeShared := webHandlerTestV2{ + user: &testuser1, + basePath: "/api/v2/projects/10/views/40/buckets", + idParam: "bucket", + t: t, + e: owned.e, + } + // project 11 is shared with admin — write access is a subset, so it can do + // everything too. + adminShared := webHandlerTestV2{ + user: &testuser1, + basePath: "/api/v2/projects/11/views/44/buckets", + idParam: "bucket", + t: t, + e: owned.e, + } + + t.Run("ReadAll", func(t *testing.T) { + t.Run("Normal", func(t *testing.T) { + rec, err := owned.testReadAllWithUser(nil, nil) + require.NoError(t, err) + // view 4 has exactly buckets 1, 2, 3 in position order. + ids, viewIDs := bucketsFromReadAll(t, rec.Body.Bytes()) + assert.ElementsMatch(t, []int64{1, 2, 3}, ids) + for _, vid := range viewIDs { + assert.Equal(t, int64(4), vid, "every returned bucket must belong to view 4") + } + assert.Contains(t, rec.Body.String(), `"total":3`) + }) + t.Run("Read-only share can list", func(t *testing.T) { + // ReadAll only needs the view's read access; a read share suffices. + rec, err := readShared.testReadAllWithUser(nil, nil) + require.NoError(t, err) + ids, _ := bucketsFromReadAll(t, rec.Body.Bytes()) + assert.ElementsMatch(t, []int64{9, 25}, ids) + }) + t.Run("Write share can list", func(t *testing.T) { + rec, err := writeShared.testReadAllWithUser(nil, nil) + require.NoError(t, err) + ids, _ := bucketsFromReadAll(t, rec.Body.Bytes()) + assert.ElementsMatch(t, []int64{10, 26}, ids) + }) + t.Run("Forbidden", func(t *testing.T) { + _, err := forbidden.testReadAllWithUser(nil, nil) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + }) + + t.Run("Create", func(t *testing.T) { + t.Run("Normal", func(t *testing.T) { + rec, err := owned.testCreateWithUser(nil, nil, `{"title":"New bucket","limit":5}`) + require.NoError(t, err) + assert.Equal(t, http.StatusCreated, rec.Code) + assert.Contains(t, rec.Body.String(), `"title":"New bucket"`) + assert.Contains(t, rec.Body.String(), `"limit":5`) + // ownership: the view from the URL wins over the body. + assert.Contains(t, rec.Body.String(), `"project_view_id":4`) + }) + t.Run("Write share can create", func(t *testing.T) { + // write access clears Project.CanUpdate → Bucket.CanCreate passes. + rec, err := writeShared.testCreateWithUser(nil, nil, `{"title":"Write made"}`) + require.NoError(t, err) + assert.Equal(t, http.StatusCreated, rec.Code) + assert.Contains(t, rec.Body.String(), `"title":"Write made"`) + assert.Contains(t, rec.Body.String(), `"project_view_id":40`) + }) + t.Run("Admin share can create", func(t *testing.T) { + rec, err := adminShared.testCreateWithUser(nil, nil, `{"title":"Admin made"}`) + require.NoError(t, err) + assert.Equal(t, http.StatusCreated, rec.Code) + assert.Contains(t, rec.Body.String(), `"title":"Admin made"`) + assert.Contains(t, rec.Body.String(), `"project_view_id":44`) + }) + t.Run("Read share cannot create", func(t *testing.T) { + // read share is below the write bar Bucket.CanCreate enforces. + _, err := readShared.testCreateWithUser(nil, nil, `{"title":"Nope"}`) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Forbidden", func(t *testing.T) { + _, err := forbidden.testCreateWithUser(nil, nil, `{"title":"Nope"}`) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Empty title", func(t *testing.T) { + // Title has valid:"required" / minLength:"1" → 422 before the model. + _, err := owned.testCreateWithUser(nil, nil, `{"title":""}`) + require.Error(t, err) + assert.Equal(t, http.StatusUnprocessableEntity, getHTTPErrorCode(err)) + }) + }) + + t.Run("Update", func(t *testing.T) { + t.Run("Normal", func(t *testing.T) { + rec, err := owned.testUpdateWithUser(nil, map[string]string{"bucket": "1"}, `{"title":"Renamed bucket"}`) + require.NoError(t, err) + assert.Contains(t, rec.Body.String(), `"title":"Renamed bucket"`) + assert.Contains(t, rec.Body.String(), `"id":1`) + // Only the sent fields are written: the server-managed creator and the + // view scoping from the URL are preserved, not clobbered to zero. + db.AssertExists(t, "buckets", map[string]interface{}{ + "id": 1, + "title": "Renamed bucket", + "project_view_id": 4, + "created_by_id": 1, + }, false) + }) + t.Run("Write share can update", func(t *testing.T) { + // bucket 10 belongs to view 40 (project 10, write share). + rec, err := writeShared.testUpdateWithUser(nil, map[string]string{"bucket": "10"}, `{"title":"Write renamed"}`) + require.NoError(t, err) + assert.Contains(t, rec.Body.String(), `"title":"Write renamed"`) + assert.Contains(t, rec.Body.String(), `"id":10`) + }) + t.Run("Read share cannot update", func(t *testing.T) { + // bucket 9 belongs to view 36 (project 9, read share) → needs write. + _, err := readShared.testUpdateWithUser(nil, map[string]string{"bucket": "9"}, `{"title":"x"}`) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Nonexisting", func(t *testing.T) { + _, err := owned.testUpdateWithUser(nil, map[string]string{"bucket": "9999"}, `{"title":"x"}`) + require.Error(t, err) + assert.Equal(t, http.StatusNotFound, getHTTPErrorCode(err)) + }) + t.Run("Forbidden", func(t *testing.T) { + // bucket 4 belongs to view 8 (project 2) — testuser1 has no access. + _, err := forbidden.testUpdateWithUser(nil, map[string]string{"bucket": "4"}, `{"title":"x"}`) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + }) + + t.Run("Delete", func(t *testing.T) { + t.Run("Read share cannot delete", func(t *testing.T) { + // bucket 25 belongs to view 36 (project 9, read share) → needs write. + _, err := readShared.testDeleteWithUser(nil, map[string]string{"bucket": "25"}) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Write share can delete", func(t *testing.T) { + // bucket 26 belongs to view 40 (project 10, write share); view 40 still + // has bucket 10 (plus the one created above), so it isn't the last. + rec, err := writeShared.testDeleteWithUser(nil, map[string]string{"bucket": "26"}) + require.NoError(t, err) + assert.Equal(t, http.StatusNoContent, rec.Code) + assert.Empty(t, rec.Body.String()) + }) + t.Run("Forbidden", func(t *testing.T) { + _, err := forbidden.testDeleteWithUser(nil, map[string]string{"bucket": "40"}) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Normal", func(t *testing.T) { + // view 4 has buckets 1, 2, 3 (plus the one created above), so deleting + // bucket 2 leaves more than one behind. + rec, err := owned.testDeleteWithUser(nil, map[string]string{"bucket": "2"}) + require.NoError(t, err) + assert.Equal(t, http.StatusNoContent, rec.Code) + assert.Empty(t, rec.Body.String()) + db.AssertMissing(t, "buckets", map[string]interface{}{"id": 2}) + }) + t.Run("Nonexisting", func(t *testing.T) { + _, err := owned.testDeleteWithUser(nil, map[string]string{"bucket": "9999"}) + require.Error(t, err) + assert.Equal(t, http.StatusNotFound, getHTTPErrorCode(err)) + }) + }) +} + +// bucketsFromReadAll extracts the bucket ids and their project_view_ids from a v2 +// paginated list body so the visible set can be asserted exactly. +func bucketsFromReadAll(t *testing.T, body []byte) (ids []int64, viewIDs []int64) { + t.Helper() + var resp struct { + Items []struct { + ID int64 `json:"id"` + ProjectViewID int64 `json:"project_view_id"` + } `json:"items"` + } + require.NoError(t, json.Unmarshal(body, &resp), "ReadAll body must be a paginated envelope: %s", string(body)) + ids = make([]int64, 0, len(resp.Items)) + viewIDs = make([]int64, 0, len(resp.Items)) + for _, it := range resp.Items { + ids = append(ids, it.ID) + viewIDs = append(viewIDs, it.ProjectViewID) + } + return ids, viewIDs +} diff --git a/veans/AGENTS.md b/veans/AGENTS.md index adc4d420c..1b0d6c902 100644 --- a/veans/AGENTS.md +++ b/veans/AGENTS.md @@ -46,9 +46,36 @@ this file is veans-specific. ## Vikunja wire-format gotchas -Most failures surface when crossing the JSON boundary. The list below is -what's bitten me; if a new endpoint behaves oddly, suspect one of these: +veans targets the Huma-backed **`/api/v2`** exclusively (`apiBasePath` in +`internal/client/client.go`). v1 is frozen, and the kanban-bucket CRUD veans +relies on only exists on v2. Most failures surface when crossing the JSON +boundary. The list below is what's bitten me; if a new endpoint behaves +oddly, suspect one of these: +- **Lists come wrapped in the standard envelope.** Every v2 list returns + `{"items":[...],"total":N,"page":N,"per_page":N,"total_pages":N}`, not a + bare array, and there is no `x-pagination-total-pages` header anymore. + Decode with the generic `Paginated[T]` helper. **Most lists are + server-paginated** — their model's `ReadAll` applies a 50-item page limit: + tasks, projects, labels, comments and bots. Page through those with + `doListAll` until `page >= total_pages`; returning only page 1 silently + truncates (>50 comments on a task is realistic). **Buckets and project + views are the exception**: their `ReadAll` takes `_ int, _ int` and returns + every row in one page, so fetch them with a single `doList` and unwrap + `.items` — paging those would re-fetch the full set and duplicate it. + Single-object responses (create/update/read of one entity) stay UNWRAPPED. +- **v2 flips the create/update verbs.** Creates are **POST** (v1 used PUT): + projects, labels, tokens, bot users, project shares, task create, + comments, relations, assignees, label-attach, bucket create. Task update + is **PATCH** (see below). The bucket-task move is **PUT**. +- **Task update is `PATCH /tasks/{id}` with `application/merge-patch+json`** + (`client.DoMerge` → `UpdateTask(*TaskPatch)`). Only the fields present in + the body are written; absent fields are left intact. Build the body from + `TaskPatch` (pointer fields, omitempty) — never a whole `client.Task`, + whose no-omitempty `done`/`title` would clobber those columns on every + call (this was issue #2962). +- **List search is `q`**, not v1's `s` (`ListParams.Q`). Task-list + `filter`/`expand`/`page`/`per_page` keep their names. - **`ProjectView.view_kind` and `bucket_configuration_mode` are strings**, not ints. The parent enums (`ProjectViewKind`, `BucketConfigurationModeKind`) have custom `MarshalJSON` that emits @@ -58,11 +85,12 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: `xorm:"-"` on it — the actual bucket lives in a separate `task_buckets` table. Fetch with `?expand=buckets` and use `task.CurrentBucketID(viewID)` to read it. -- **`POST /tasks/{id}` does NOT move tasks between buckets.** The - task↔bucket relation is row-shaped; use `client.MoveTaskToBucket()` - which hits `POST /projects/{p}/views/{v}/buckets/{b}/tasks`. The - Update path on the server only auto-moves on `done` flips. -- **Bot user creation is `PUT /user/bots`**, not `/bots` — the routes +- **Task updates do NOT move tasks between buckets.** The task↔bucket + relation is row-shaped; use `client.MoveTaskToBucket()` which hits + **`PUT /projects/{p}/views/{v}/buckets/{b}/tasks`** with a `{"task_id":N}` + body (project/view/bucket all come from the URL). The Update path on the + server only auto-moves on `done` flips. +- **Bot user creation is `POST /user/bots`**, not `/bots` — the routes are registered under the `/user` subgroup. Same prefix for `GET /user/bots`. - **`APIToken.expires_at` is required.** The struct field has @@ -88,6 +116,16 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: - `/projects/:project/views/:view/buckets/:bucket/tasks` → group `projects`, action `views_buckets_tasks` - `/tasks/:task/comments` → group `tasks_comments`, action `create` +- v1 and v2 deliberately share `(group, permission)` keys: + `pkg/models/api_routes.go` normalizes the inverted verbs (v2 POST-create + and v1 PUT-create both → `create`; v2 PUT/PATCH-update and v1 POST-update + both → `update`), and `CanDoAPIRoute` consults both route tables, treating + PATCH as an alias for the stored PUT. So `PermissionsForBot`'s scope map + authorizes the v2 calls unchanged, including the PATCH task update. +- The bucket-task MOVE (`PUT …/buckets/:bucket/tasks`) and the + buckets-with-tasks LIST (`GET …/buckets/tasks`) collide on subkey + `views_buckets_tasks`; which one gets the bare key vs `views_buckets_tasks_put` + depends on unspecified route-init order, so the bot requests **both**. - `client.PermissionsForBot()` calls `GET /routes` at runtime and grants only the intersection of what we want and what the server exposes. **Don't hard-code permission group names** — they drift @@ -96,9 +134,9 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: ## Bot ownership and token minting -- Creating a bot via `PUT /user/bots` automatically sets the bot's +- Creating a bot via `POST /user/bots` automatically sets the bot's `bot_owner_id` to the calling user. Only the owner can mint tokens - for the bot via `PUT /tokens` with `owner_id=`. The init + for the bot via `POST /tokens` with `owner_id=`. The init flow does these as a single human-JWT-authenticated batch. - Bots have no password and **cannot** authenticate via `POST /login`. After init, `veans login` re-authenticates as the human (not the @@ -115,9 +153,11 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: browser, and captures the callback. The `Shutdown` defer uses `context.WithoutCancel(ctx)` so cancellation at the outer scope still drains the loopback server cleanly. -- Token exchange is **JSON only**. Form-encoded POSTs to `/oauth/token` - fail; the standard `golang.org/x/oauth2` client speaks form encoding, - which is why we have a hand-rolled `client.ExchangeOAuthCode`. +- Token exchange goes out as **JSON**. v2's `/oauth/token` accepts both JSON + and form-encoded bodies (Huma picks the decoder off the `Content-Type` + header), but the standard `golang.org/x/oauth2` client hard-codes form + encoding and its own response shape, so we keep the hand-rolled + `client.ExchangeOAuthCode` that speaks JSON. ## Credential store diff --git a/veans/e2e/init_test.go b/veans/e2e/init_test.go index cc432d99a..972812169 100644 --- a/veans/e2e/init_test.go +++ b/veans/e2e/init_test.go @@ -102,11 +102,14 @@ func TestInit_HappyPath(t *testing.T) { t.Fatalf("bot %q not found on server", ws.BotUsername) } - // Project shared with the bot at write permission. - var shares []map[string]any + // Project shared with the bot at write permission. v2 lists come wrapped + // in the standard {items,...} envelope. + var shares struct { + Items []map[string]any `json:"items"` + } _ = h.AdminClient.Do(t.Context(), "GET", fmt.Sprintf("/projects/%d/users", project.ID), nil, nil, &shares) shareFound := false - for _, s := range shares { + for _, s := range shares.Items { if u, _ := s["username"].(string); u == ws.BotUsername { if p, _ := s["permission"].(float64); int(p) >= 1 { shareFound = true diff --git a/veans/internal/bootstrap/bootstrap.go b/veans/internal/bootstrap/bootstrap.go index a1251be89..5ea9a5e07 100644 --- a/veans/internal/bootstrap/bootstrap.go +++ b/veans/internal/bootstrap/bootstrap.go @@ -257,7 +257,7 @@ func Init(ctx context.Context, opts *Options) (*Result, error) { return nil, output.Wrap(output.CodeUnknown, err, "mint bot token: %v", err) } if mintedToken.Token == "" { - return nil, output.New(output.CodeUnknown, "PUT /tokens did not return a token plaintext — cannot continue") + return nil, output.New(output.CodeUnknown, "POST /tokens did not return a token plaintext — cannot continue") } // 11. Persist credentials. Discard human JWT immediately after. diff --git a/veans/internal/bootstrap/bootstrap_test.go b/veans/internal/bootstrap/bootstrap_test.go index 8c70fb333..6edf52e1b 100644 --- a/veans/internal/bootstrap/bootstrap_test.go +++ b/veans/internal/bootstrap/bootstrap_test.go @@ -211,8 +211,9 @@ func TestConfirmOverwriteExistingConfig(t *testing.T) { } // bucketServer is a minimal httptest server modelling -// GET/PUT /api/v1/projects/{p}/views/{v}/buckets. The caller pre-seeds -// existing buckets; PUT requests append to that list with a synthetic ID. +// GET/POST /api/v2/projects/{p}/views/{v}/buckets. The caller pre-seeds +// existing buckets; POST requests append to that list with a synthetic ID. +// GET returns the standard v2 list envelope; POST returns the bare bucket. type bucketServer struct { mu sync.Mutex existing []*client.Bucket @@ -232,7 +233,7 @@ func newBucketServer(seed []*client.Bucket) *bucketServer { func (s *bucketServer) handler() http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Path is /api/v1/projects/{p}/views/{v}/buckets. + // Path is /api/v2/projects/{p}/views/{v}/buckets. if !strings.HasSuffix(r.URL.Path, "/buckets") || !strings.Contains(r.URL.Path, "/views/") { http.Error(w, "unexpected path: "+r.URL.Path, http.StatusInternalServerError) return @@ -242,8 +243,15 @@ func (s *bucketServer) handler() http.Handler { switch r.Method { case http.MethodGet: w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(s.existing) - case http.MethodPut: + // v2 list envelope; the buckets list isn't server-paginated. + _ = json.NewEncoder(w).Encode(map[string]any{ + "items": s.existing, + "total": len(s.existing), + "page": 1, + "per_page": 50, + "total_pages": 1, + }) + case http.MethodPost: var b client.Bucket if err := json.NewDecoder(r.Body).Decode(&b); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) diff --git a/veans/internal/client/assignees.go b/veans/internal/client/assignees.go index 2e63b46db..6478acd01 100644 --- a/veans/internal/client/assignees.go +++ b/veans/internal/client/assignees.go @@ -23,5 +23,5 @@ import ( // AddAssignee assigns a user (typically the bot) to a task. func (c *Client) AddAssignee(ctx context.Context, taskID, userID int64) error { - return c.Do(ctx, "PUT", fmt.Sprintf("/tasks/%d/assignees", taskID), nil, &TaskAssignee{UserID: userID}, nil) + return c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d/assignees", taskID), nil, &TaskAssignee{UserID: userID}, nil) } diff --git a/veans/internal/client/buckets.go b/veans/internal/client/buckets.go index ba4c324e5..43aa8c398 100644 --- a/veans/internal/client/buckets.go +++ b/veans/internal/client/buckets.go @@ -21,25 +21,28 @@ import ( "fmt" ) -// ListBuckets returns the buckets configured on a Kanban view. +// ListBuckets returns the buckets configured on a Kanban view. Bucket.ReadAll +// ignores page/per_page and returns every bucket in a single page (the envelope +// total reflects the full set), so one GET gets them all — paging would +// re-fetch the same buckets and duplicate them. Unwrap .items. func (c *Client) ListBuckets(ctx context.Context, projectID, viewID int64) ([]*Bucket, error) { - var out []*Bucket path := fmt.Sprintf("/projects/%d/views/%d/buckets", projectID, viewID) - if err := c.Do(ctx, "GET", path, nil, nil, &out); err != nil { + items, _, err := doList[*Bucket](ctx, c, path, nil) + if err != nil { return nil, err } - return out, nil + return items, nil } -// CreateBucket inserts a new bucket into a Kanban view. +// CreateBucket inserts a new bucket into a Kanban view. The project and view +// come from the URL; the v2 handler ignores project_view_id in the body. func (c *Client) CreateBucket(ctx context.Context, projectID, viewID int64, b *Bucket) (*Bucket, error) { var out Bucket path := fmt.Sprintf("/projects/%d/views/%d/buckets", projectID, viewID) if b == nil { b = &Bucket{} } - b.ProjectViewID = viewID - if err := c.Do(ctx, "PUT", path, nil, b, &out); err != nil { + if err := c.Do(ctx, "POST", path, nil, b, &out); err != nil { return nil, err } return &out, nil @@ -47,17 +50,13 @@ func (c *Client) CreateBucket(ctx context.Context, projectID, viewID int64, b *B // MoveTaskToBucket positions an existing task in `bucketID` on the // project's view. Vikunja stores task↔bucket relations in a separate -// table (`task_buckets`), so POST /tasks/{id} with bucket_id does not -// reliably move tasks — this dedicated endpoint is the one the Kanban -// UI's drag-and-drop uses. +// table (`task_buckets`); a task update with bucket_id does not reliably +// move tasks — this dedicated endpoint is the one the Kanban UI's +// drag-and-drop uses. On v2 it's a PUT, and project/view/bucket all come +// from the URL, so the body only carries the task id. func (c *Client) MoveTaskToBucket(ctx context.Context, projectID, viewID, bucketID, taskID int64) error { path := fmt.Sprintf("/projects/%d/views/%d/buckets/%d/tasks", projectID, viewID, bucketID) - body := map[string]int64{ - "task_id": taskID, - "project_view_id": viewID, - "bucket_id": bucketID, - "project_id": projectID, - } - return c.Do(ctx, "POST", path, nil, body, nil) + body := map[string]int64{"task_id": taskID} + return c.Do(ctx, "PUT", path, nil, body, nil) } diff --git a/veans/internal/client/client.go b/veans/internal/client/client.go index 19ad0827a..6e45bb53d 100644 --- a/veans/internal/client/client.go +++ b/veans/internal/client/client.go @@ -33,7 +33,7 @@ import ( // Client is a thin JSON wrapper around the Vikunja REST API. It holds the // server base URL and a bearer token (either a JWT from POST /login or an -// API token minted via PUT /tokens). Every method in this package is a thin +// API token minted via POST /tokens). Every method in this package is a thin // shim over Do. type Client struct { BaseURL string @@ -41,6 +41,19 @@ type Client struct { HTTPClient *http.Client } +// apiBasePath is the version prefix every request is mounted under. veans +// targets the Huma-backed /api/v2 exclusively — v1 is frozen and the bucket +// CRUD endpoints veans needs only exist on v2. +const apiBasePath = "/api/v2" + +// contentTypeJSON / contentTypeMergePatch are the request body content types +// Do and DoMerge send. Merge-patch (RFC 7396) is how v2 does partial updates: +// only the fields present in the body are written, the rest are left intact. +const ( + contentTypeJSON = "application/json" + contentTypeMergePatch = "application/merge-patch+json" +) + // UserAgent is the value sent in the User-Agent header on every request. // main sets this at startup with the linker-injected version + the // runtime os/arch (e.g. "veans/0.3.1 (linux/amd64)"). Tests get the @@ -61,17 +74,36 @@ func New(baseURL, token string) *Client { } } -// vikunjaError matches `web.HTTPError` on the wire. +// vikunjaError matches the RFC 9457 problem+json body /api/v2 returns +// (huma.ErrorModel augmented with Vikunja's numeric domain `code`). The +// human-readable message lives in `detail`; `title` is the status text +// fallback. `message` is v1's legacy field, kept only as a fallback so a +// stray legacy/proxy error body still yields a readable message instead of +// raw JSON. The HTTP status used for output.Code mapping comes from the +// response status line, not this body. type vikunjaError struct { - Code int `json:"code"` + Title string `json:"title"` + Detail string `json:"detail"` Message string `json:"message"` + Code int `json:"code"` } -// Do performs a single JSON request against /api/v1. body, if non-nil, +// Do performs a single JSON request against /api/v2. body, if non-nil, // is JSON-marshalled. out, if non-nil, is JSON-unmarshalled. query is appended // as URL-encoded params. func (c *Client) Do(ctx context.Context, method, path string, query url.Values, body, out any) error { - full := c.BaseURL + "/api/v1" + path + return c.do(ctx, method, path, query, body, out, contentTypeJSON) +} + +// DoMerge is like Do but sends the body as a JSON Merge Patch +// (application/merge-patch+json). Used for PATCH updates so only the fields +// present in `body` are written server-side — see UpdateTask. +func (c *Client) DoMerge(ctx context.Context, method, path string, query url.Values, body, out any) error { + return c.do(ctx, method, path, query, body, out, contentTypeMergePatch) +} + +func (c *Client) do(ctx context.Context, method, path string, query url.Values, body, out any, contentType string) error { + full := c.BaseURL + apiBasePath + path if len(query) > 0 { full += "?" + query.Encode() } @@ -91,7 +123,7 @@ func (c *Client) Do(ctx context.Context, method, path string, query url.Values, } req.Header.Set("Accept", "application/json") if body != nil { - req.Header.Set("Content-Type", "application/json") + req.Header.Set("Content-Type", contentType) } if c.Token != "" { req.Header.Set("Authorization", "Bearer "+c.Token) @@ -122,51 +154,55 @@ func (c *Client) Do(ctx context.Context, method, path string, query url.Values, return nil } -// DoPaginated is like Do but also returns the total page count parsed from -// the `x-pagination-total-pages` response header (0 if the header is -// missing or unparseable). Used by the list endpoints so paging terminates -// against the authoritative server count, not a `len(batch) < per_page` -// heuristic that loops one extra time on exact-multiple totals. -func (c *Client) DoPaginated(ctx context.Context, method, path string, query url.Values, out any) (totalPages int, err error) { - full := c.BaseURL + "/api/v1" + path - if len(query) > 0 { - full += "?" + query.Encode() - } - req, err := http.NewRequestWithContext(ctx, method, full, nil) - if err != nil { - return 0, fmt.Errorf("build request: %w", err) - } - req.Header.Set("Accept", "application/json") - if c.Token != "" { - req.Header.Set("Authorization", "Bearer "+c.Token) - } - req.Header.Set("User-Agent", UserAgent) +// Paginated mirrors the standard /api/v2 list envelope. Every v2 list +// operation returns this shape (v1 returned a bare array plus an +// x-pagination-total-pages header, which is gone). Single-object responses +// stay unwrapped. +type Paginated[T any] struct { + Items []T `json:"items"` + Total int64 `json:"total"` + Page int `json:"page"` + PerPage int `json:"per_page"` + TotalPages int `json:"total_pages"` +} - resp, err := c.HTTPClient.Do(req) - if err != nil { - return 0, output.Wrap(output.CodeUnknown, err, "%s %s: %v", method, path, err) +// doList GETs `path` and decodes the standard v2 list envelope, returning the +// items plus the server's total page count so a caller can page until +// page >= totalPages. Generic so each list endpoint reuses it without a +// per-type wrapper struct. +func doList[T any](ctx context.Context, c *Client, path string, query url.Values) (items []T, totalPages int, err error) { + var env Paginated[T] + if err := c.Do(ctx, "GET", path, query, nil, &env); err != nil { + return nil, 0, err } - defer resp.Body.Close() + return env.Items, env.TotalPages, nil +} - respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes)) - if err != nil { - return 0, fmt.Errorf("read response: %w", err) - } - if resp.StatusCode >= 400 { - return 0, mapHTTPError(method, path, resp.StatusCode, respBody, - parseRetryAfter(resp.Header.Get("Retry-After"))) - } - if out != nil && len(respBody) > 0 { - if err := json.Unmarshal(respBody, out); err != nil { - return 0, fmt.Errorf("decode %s %s: %w", method, path, err) +// doListAll pages through a v2 list endpoint, accumulating every item until +// page >= total_pages. +// +// Use it ONLY for endpoints whose model honours page/per_page — the +// server-paginated lists (tasks, projects, labels, comments, bots). For the +// endpoints whose ReadAll ignores pagination and returns every row in a single +// page (buckets, views), call doList instead: looping those re-fetches the full +// set on every page and duplicates it. +func doListAll[T any](ctx context.Context, c *Client, path string) ([]T, error) { + var all []T + page := 1 + for { + q := url.Values{} + q.Set("page", strconv.Itoa(page)) + q.Set("per_page", "50") + batch, totalPages, err := doList[T](ctx, c, path, q) + if err != nil { + return nil, err } - } - if v := resp.Header.Get("x-pagination-total-pages"); v != "" { - if n, perr := strconv.Atoi(v); perr == nil { - totalPages = n + all = append(all, batch...) + if page >= totalPages { + return all, nil } + page++ } - return totalPages, nil } // DoRaw is the escape hatch used by `veans api`. It returns the raw response @@ -176,7 +212,7 @@ func (c *Client) DoPaginated(ctx context.Context, method, path string, query url // "stdout is for the success payload; errors go through the envelope on // stderr"); see commands/api.go for the canonical handling. func (c *Client) DoRaw(ctx context.Context, method, path string, query url.Values, body []byte) (status int, respBody []byte, retryAfter time.Duration, err error) { - full := c.BaseURL + "/api/v1" + path + full := c.BaseURL + apiBasePath + path if len(query) > 0 { full += "?" + query.Encode() } @@ -205,18 +241,6 @@ func (c *Client) DoRaw(ctx context.Context, method, path string, query url.Value return resp.StatusCode, respBody, parseRetryAfter(resp.Header.Get("Retry-After")), err } -// paginationDone reports whether a paged GET has consumed every page, -// preferring the server's x-pagination-total-pages count when present and -// falling back to the len(batch) < per_page heuristic when the header is -// missing (older server / proxy stripped). Centralized so all list -// endpoints terminate identically. -func paginationDone(page, batchLen, perPage, totalPages int) bool { - if totalPages > 0 { - return page >= totalPages - } - return batchLen < perPage -} - // maxBodyBytes caps the size of any response body we'll read into memory. // Vikunja JSON payloads are far smaller; the cap exists so a misbehaving // proxy can't OOM the CLI by streaming an unbounded body. @@ -244,7 +268,16 @@ func parseRetryAfter(v string) time.Duration { func mapHTTPError(method, path string, status int, body []byte, retryAfter time.Duration) error { var ve vikunjaError _ = json.Unmarshal(body, &ve) - msg := strings.TrimSpace(ve.Message) + // v2's problem+json carries the human-readable text in `detail`; fall back + // to `title`, then v1's legacy `message`, then the raw body, then the + // status text. + msg := strings.TrimSpace(ve.Detail) + if msg == "" { + msg = strings.TrimSpace(ve.Title) + } + if msg == "" { + msg = strings.TrimSpace(ve.Message) + } if msg == "" { msg = strings.TrimSpace(string(body)) if msg == "" { diff --git a/veans/internal/client/client_test.go b/veans/internal/client/client_test.go index 5f7144fa2..19bb8ddd7 100644 --- a/veans/internal/client/client_test.go +++ b/veans/internal/client/client_test.go @@ -45,7 +45,7 @@ func TestMapHTTPError_StatusCodeMapping(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := mapHTTPError("GET", "/foo", tc.status, []byte(`{"message":"boom"}`), 0) + err := mapHTTPError("GET", "/foo", tc.status, []byte(`{"detail":"boom"}`), 0) var oe *output.Error if !errors.As(err, &oe) { t.Fatalf("expected *output.Error, got %T", err) @@ -59,7 +59,7 @@ func TestMapHTTPError_StatusCodeMapping(t *testing.T) { func TestMapHTTPError_RetryAfterAppendedToMessage(t *testing.T) { retry := 7 * time.Second - err := mapHTTPError("GET", "/foo", http.StatusTooManyRequests, []byte(`{"message":"slow down"}`), retry) + err := mapHTTPError("GET", "/foo", http.StatusTooManyRequests, []byte(`{"detail":"slow down"}`), retry) var oe *output.Error if !errors.As(err, &oe) { t.Fatalf("expected *output.Error, got %T", err) @@ -89,20 +89,53 @@ func TestMapHTTPError_BodyTruncation(t *testing.T) { } } -func TestMapHTTPError_VikunjaJSONTakesPrecedenceOverRawBody(t *testing.T) { - body := []byte(`{"code":404,"message":"x"}`) +func TestMapHTTPError_VikunjaProblemJSONTakesPrecedenceOverRawBody(t *testing.T) { + // v2 returns RFC 9457 problem+json: the message is in `detail`, and `code` + // carries Vikunja's numeric domain error code (not the HTTP status). + body := []byte(`{"status":404,"title":"Not Found","detail":"x","code":3001}`) err := mapHTTPError("GET", "/foo", http.StatusNotFound, body, 0) var oe *output.Error if !errors.As(err, &oe) { t.Fatalf("expected *output.Error, got %T", err) } // The formatted message is "METHOD PATH: STATUS MSG"; assert it carries - // the decoded message and not the raw JSON envelope. + // the decoded `detail` and not the raw JSON envelope. if !strings.HasSuffix(oe.Message, ": 404 x") { t.Errorf("expected formatted message to end with %q, got %q", ": 404 x", oe.Message) } - if strings.Contains(oe.Message, `"code":404`) { - t.Errorf("expected raw JSON body to be replaced by decoded message, got %q", oe.Message) + if strings.Contains(oe.Message, `"code"`) { + t.Errorf("expected raw JSON body to be replaced by decoded detail, got %q", oe.Message) + } +} + +func TestMapHTTPError_FallsBackToTitleWhenNoDetail(t *testing.T) { + // A problem+json body with no `detail` (e.g. Huma's own schema-validation + // 422 sometimes only sets title) falls back to `title`. + body := []byte(`{"status":422,"title":"Unprocessable Entity"}`) + err := mapHTTPError("PATCH", "/tasks/1", http.StatusUnprocessableEntity, body, 0) + var oe *output.Error + if !errors.As(err, &oe) { + t.Fatalf("expected *output.Error, got %T", err) + } + if !strings.HasSuffix(oe.Message, ": 422 Unprocessable Entity") { + t.Errorf("expected title fallback, got %q", oe.Message) + } +} + +func TestMapHTTPError_FallsBackToLegacyMessage(t *testing.T) { + // Defensive: a stray legacy/proxy body with only v1's `message` field + // still yields the message text rather than the raw JSON. + body := []byte(`{"code":403,"message":"forbidden"}`) + err := mapHTTPError("GET", "/foo", http.StatusForbidden, body, 0) + var oe *output.Error + if !errors.As(err, &oe) { + t.Fatalf("expected *output.Error, got %T", err) + } + if !strings.HasSuffix(oe.Message, ": 403 forbidden") { + t.Errorf("expected legacy message fallback, got %q", oe.Message) + } + if strings.Contains(oe.Message, `"message"`) { + t.Errorf("expected raw JSON to be replaced by the message text, got %q", oe.Message) } } @@ -146,36 +179,9 @@ func TestParseRetryAfter(t *testing.T) { } } -func TestPaginationDone(t *testing.T) { - cases := []struct { - name string - page int - batchLen int - perPage int - totalPages int - want bool - }{ - {"server says single page complete", 1, 50, 50, 1, true}, - {"server says more pages remain", 1, 50, 50, 2, false}, - {"server says we're on the last page", 2, 10, 50, 2, true}, - {"no header, full page -> not done", 1, 50, 50, 0, false}, - {"no header, short page -> done", 1, 10, 50, 0, true}, - {"no header, empty page -> done", 1, 0, 50, 0, true}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - got := paginationDone(tc.page, tc.batchLen, tc.perPage, tc.totalPages) - if got != tc.want { - t.Errorf("paginationDone(page=%d, batch=%d, per=%d, total=%d) = %v, want %v", - tc.page, tc.batchLen, tc.perPage, tc.totalPages, got, tc.want) - } - }) - } -} - func TestCreateBotUser_404TranslatesToBotUsersUnavailable(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPut || r.URL.Path != "/api/v1/user/bots" { + if r.Method != http.MethodPost || r.URL.Path != "/api/v2/user/bots" { http.Error(w, "unexpected route", http.StatusInternalServerError) return } @@ -196,3 +202,129 @@ func TestCreateBotUser_404TranslatesToBotUsersUnavailable(t *testing.T) { t.Errorf("got code %q, want %q", oe.Code, output.CodeBotUsersUnavailable) } } + +// TestListProjects_PaginatesEnvelope verifies the v2 list shape: each page is +// the {items,total,page,per_page,total_pages} envelope, and ListProjects keeps +// requesting until page >= total_pages, accumulating every item. +func TestListProjects_PaginatesEnvelope(t *testing.T) { + var gotPages []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v2/projects" { + http.Error(w, "unexpected path "+r.URL.Path, http.StatusInternalServerError) + return + } + page := r.URL.Query().Get("page") + gotPages = append(gotPages, page) + w.Header().Set("Content-Type", "application/json") + switch page { + case "1": + _, _ = w.Write([]byte(`{"items":[{"id":1,"title":"a"},{"id":2,"title":"b"}],"total":3,"page":1,"per_page":2,"total_pages":2}`)) + case "2": + _, _ = w.Write([]byte(`{"items":[{"id":3,"title":"c"}],"total":3,"page":2,"per_page":2,"total_pages":2}`)) + default: + t.Errorf("unexpected page %q (would loop past the end)", page) + http.Error(w, "no such page", http.StatusBadRequest) + } + })) + defer srv.Close() + + projects, err := New(srv.URL, "tk").ListProjects(context.Background()) + if err != nil { + t.Fatalf("ListProjects: %v", err) + } + if len(projects) != 3 { + t.Fatalf("expected 3 projects accumulated across 2 pages, got %d", len(projects)) + } + if len(gotPages) != 2 || gotPages[0] != "1" || gotPages[1] != "2" { + t.Fatalf("expected exactly pages [1 2], got %v", gotPages) + } +} + +// TestListTaskComments_PaginatesEnvelope guards the truncation bug: the v2 +// comments endpoint is server-paginated, so a task with >50 comments spans +// multiple pages and ListTaskComments must accumulate them all, not stop at +// page 1. +func TestListTaskComments_PaginatesEnvelope(t *testing.T) { + var pages []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v2/tasks/9/comments" { + http.Error(w, "unexpected path "+r.URL.Path, http.StatusInternalServerError) + return + } + page := r.URL.Query().Get("page") + pages = append(pages, page) + w.Header().Set("Content-Type", "application/json") + switch page { + case "1": + _, _ = w.Write([]byte(`{"items":[{"id":1,"comment":"a"},{"id":2,"comment":"b"}],"total":3,"page":1,"per_page":2,"total_pages":2}`)) + case "2": + _, _ = w.Write([]byte(`{"items":[{"id":3,"comment":"c"}],"total":3,"page":2,"per_page":2,"total_pages":2}`)) + default: + t.Errorf("unexpected page %q", page) + http.Error(w, "no such page", http.StatusBadRequest) + } + })) + defer srv.Close() + + comments, err := New(srv.URL, "tk").ListTaskComments(context.Background(), 9) + if err != nil { + t.Fatalf("ListTaskComments: %v", err) + } + if len(comments) != 3 { + t.Fatalf("expected 3 comments across 2 pages, got %d (truncation regression?)", len(comments)) + } + if len(pages) != 2 { + t.Fatalf("expected to fetch 2 pages, got %v", pages) + } +} + +// TestListBuckets_SingleFetchDoesNotPage pins the opposite invariant: the +// buckets model returns every row in one page, so ListBuckets must issue a +// single request even when the envelope's total_pages is >1 — paging would +// re-fetch and duplicate the buckets. +func TestListBuckets_SingleFetchDoesNotPage(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + if requests > 1 { + t.Errorf("ListBuckets paged a single-page endpoint (request %d) — would duplicate", requests) + } + w.Header().Set("Content-Type", "application/json") + // total_pages deliberately > 1 to prove ListBuckets ignores it. + _, _ = w.Write([]byte(`{"items":[{"id":1,"title":"Todo"},{"id":2,"title":"Doing"}],"total":2,"page":1,"per_page":1,"total_pages":2}`)) + })) + defer srv.Close() + + buckets, err := New(srv.URL, "tk").ListBuckets(context.Background(), 7, 3) + if err != nil { + t.Fatalf("ListBuckets: %v", err) + } + if requests != 1 { + t.Fatalf("expected exactly 1 request, got %d", requests) + } + if len(buckets) != 2 { + t.Fatalf("expected the 2 buckets from the single page, got %d", len(buckets)) + } +} + +// TestListProjectViews_UnwrapsEnvelope pins that a previously-single-GET list +// (project views) now unwraps .items from the standard list envelope. +func TestListProjectViews_UnwrapsEnvelope(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v2/projects/7/views" { + http.Error(w, "unexpected path "+r.URL.Path, http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"items":[{"id":10,"title":"Kanban","view_kind":"kanban"}],"total":1,"page":1,"per_page":50,"total_pages":1}`)) + })) + defer srv.Close() + + views, err := New(srv.URL, "tk").ListProjectViews(context.Background(), 7) + if err != nil { + t.Fatalf("ListProjectViews: %v", err) + } + if len(views) != 1 || views[0].ViewKind != ViewKindKanban { + t.Fatalf("expected one kanban view unwrapped from .items, got %+v", views) + } +} diff --git a/veans/internal/client/comments.go b/veans/internal/client/comments.go index 8f4cc493b..331b24591 100644 --- a/veans/internal/client/comments.go +++ b/veans/internal/client/comments.go @@ -24,17 +24,15 @@ import ( // AddTaskComment posts a new comment on a task. func (c *Client) AddTaskComment(ctx context.Context, taskID int64, body string) (*TaskComment, error) { var out TaskComment - if err := c.Do(ctx, "PUT", fmt.Sprintf("/tasks/%d/comments", taskID), nil, &TaskComment{Comment: body}, &out); err != nil { + if err := c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d/comments", taskID), nil, &TaskComment{Comment: body}, &out); err != nil { return nil, err } return &out, nil } -// ListTaskComments returns all comments on a task. +// ListTaskComments returns all comments on a task. The v2 comments endpoint is +// server-paginated (TaskComment.ReadAll applies a 50-item page limit), so page +// through to the end instead of returning only the first page. func (c *Client) ListTaskComments(ctx context.Context, taskID int64) ([]*TaskComment, error) { - var out []*TaskComment - if err := c.Do(ctx, "GET", fmt.Sprintf("/tasks/%d/comments", taskID), nil, nil, &out); err != nil { - return nil, err - } - return out, nil + return doListAll[*TaskComment](ctx, c, fmt.Sprintf("/tasks/%d/comments", taskID)) } diff --git a/veans/internal/client/discover.go b/veans/internal/client/discover.go index 8cad249e7..1e71e404c 100644 --- a/veans/internal/client/discover.go +++ b/veans/internal/client/discover.go @@ -31,11 +31,15 @@ import ( const defaultAPIPort = "3456" // DiscoverServer normalizes `input` and probes a small set of plausible -// URLs for /api/v1/info, returning the canonical base URL (without the -// /api/v1 suffix — that's what client.New expects) and the parsed Info. +// URLs for /api/v2/info, returning the canonical base URL (without the +// /api/v2 suffix — that's what client.New expects) and the parsed Info. +// +// Probing /api/v2/info doubles as the "is this server new enough" check: a +// Vikunja without /api/v2 fails discovery cleanly rather than limping along +// against endpoints veans needs. // // Mirrors the discovery the Vikunja web frontend does in -// helpers/checkAndSetApiUrl.ts: try the URL as-given, with /api/v1 +// helpers/checkAndSetApiUrl.ts: try the URL as-given, with the API path // appended, and with the default :3456 port — across http / https. The // first response that parses as Info wins. func DiscoverServer(ctx context.Context, input string) (string, *Info, error) { @@ -53,7 +57,7 @@ func DiscoverServer(ctx context.Context, input string) (string, *Info, error) { var attempts []string var lastErr error for _, base := range candidates { - attempts = append(attempts, base+"/api/v1/info") + attempts = append(attempts, base+"/api/v2/info") info, err := New(base, "").Info(ctx) if err == nil && info != nil { return base, info, nil @@ -67,15 +71,16 @@ func DiscoverServer(ctx context.Context, input string) (string, *Info, error) { } // serverCandidates expands `input` into the ordered list of base URLs -// to probe for /api/v1/info. A "base URL" here is what client.New wants: -// the origin + the path that should sit BEFORE /api/v1 (typically empty -// or a reverse-proxy prefix). The probe itself adds /api/v1/info. +// to probe for /api/v2/info. A "base URL" here is what client.New wants: +// the origin + the path that should sit BEFORE /api/v2 (typically empty +// or a reverse-proxy prefix). The probe itself adds /api/v2/info. func serverCandidates(input string) ([]string, error) { - // Strip a trailing /api/v1[/] the user might have copied from a - // curl example. We add it back in the probe, and otherwise we'd - // end up calling /api/v1/api/v1/info. + // Strip a trailing /api/v1 or /api/v2[/] the user might have copied + // from a curl example. We add the API path back in the probe, and + // otherwise we'd end up calling /api/v2/api/v2/info. trimmed := strings.TrimRight(input, "/") trimmed = strings.TrimSuffix(trimmed, "/api/v1") + trimmed = strings.TrimSuffix(trimmed, "/api/v2") trimmed = strings.TrimRight(trimmed, "/") withScheme := trimmed diff --git a/veans/internal/client/labels.go b/veans/internal/client/labels.go index 1681b49a1..24ce13c28 100644 --- a/veans/internal/client/labels.go +++ b/veans/internal/client/labels.go @@ -33,15 +33,15 @@ func (c *Client) ListLabels(ctx context.Context, search string) ([]*Label, error q.Set("page", strconv.Itoa(page)) q.Set("per_page", "50") if search != "" { - q.Set("s", search) + // v2's list search param is `q` (v1 used `s`). + q.Set("q", search) } - var batch []*Label - total, err := c.DoPaginated(ctx, "GET", "/labels", q, &batch) + batch, totalPages, err := doList[*Label](ctx, c, "/labels", q) if err != nil { return nil, err } all = append(all, batch...) - if paginationDone(page, len(batch), 50, total) { + if page >= totalPages { return all, nil } page++ @@ -51,7 +51,7 @@ func (c *Client) ListLabels(ctx context.Context, search string) ([]*Label, error // CreateLabel creates a new label owned by the authenticated user. func (c *Client) CreateLabel(ctx context.Context, l *Label) (*Label, error) { var out Label - if err := c.Do(ctx, "PUT", "/labels", nil, l, &out); err != nil { + if err := c.Do(ctx, "POST", "/labels", nil, l, &out); err != nil { return nil, err } return &out, nil @@ -59,7 +59,7 @@ func (c *Client) CreateLabel(ctx context.Context, l *Label) (*Label, error) { // AddLabelToTask attaches an existing label to a task. func (c *Client) AddLabelToTask(ctx context.Context, taskID, labelID int64) error { - return c.Do(ctx, "PUT", fmt.Sprintf("/tasks/%d/labels", taskID), nil, &LabelTask{LabelID: labelID}, nil) + return c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d/labels", taskID), nil, &LabelTask{LabelID: labelID}, nil) } // RemoveLabelFromTask detaches a label. diff --git a/veans/internal/client/projects.go b/veans/internal/client/projects.go index e0f23eccb..eb2357430 100644 --- a/veans/internal/client/projects.go +++ b/veans/internal/client/projects.go @@ -23,8 +23,8 @@ import ( "strconv" ) -// ListProjects pages through GET /projects, accumulating until the server's -// x-pagination-total-pages header says we're done. +// ListProjects pages through GET /projects, accumulating until the list +// envelope's total_pages says we're done. func (c *Client) ListProjects(ctx context.Context) ([]*Project, error) { var all []*Project page := 1 @@ -32,13 +32,12 @@ func (c *Client) ListProjects(ctx context.Context) ([]*Project, error) { q := url.Values{} q.Set("page", strconv.Itoa(page)) q.Set("per_page", "50") - var batch []*Project - total, err := c.DoPaginated(ctx, "GET", "/projects", q, &batch) + batch, totalPages, err := doList[*Project](ctx, c, "/projects", q) if err != nil { return nil, err } all = append(all, batch...) - if paginationDone(page, len(batch), 50, total) { + if page >= totalPages { return all, nil } page++ @@ -58,7 +57,7 @@ func (c *Client) GetProject(ctx context.Context, id int64) (*Project, error) { // auto-creates the default views (List, Gantt, Table, Kanban) on insert. func (c *Client) CreateProject(ctx context.Context, p *Project) (*Project, error) { var out Project - if err := c.Do(ctx, "PUT", "/projects", nil, p, &out); err != nil { + if err := c.Do(ctx, "POST", "/projects", nil, p, &out); err != nil { return nil, err } return &out, nil @@ -67,17 +66,20 @@ func (c *Client) CreateProject(ctx context.Context, p *Project) (*Project, error // ShareProjectWithUser grants `username` `permission` on project `id`. func (c *Client) ShareProjectWithUser(ctx context.Context, projectID int64, share *ProjectUser) (*ProjectUser, error) { var out ProjectUser - if err := c.Do(ctx, "PUT", fmt.Sprintf("/projects/%d/users", projectID), nil, share, &out); err != nil { + if err := c.Do(ctx, "POST", fmt.Sprintf("/projects/%d/users", projectID), nil, share, &out); err != nil { return nil, err } return &out, nil } // ListProjectViews returns saved views (Kanban, List, …) on a project. +// ProjectView.ReadAll ignores page/per_page and returns every view in a single +// page, so one GET gets them all — paging would re-fetch the same views and +// duplicate them. Unwrap .items. func (c *Client) ListProjectViews(ctx context.Context, projectID int64) ([]*ProjectView, error) { - var out []*ProjectView - if err := c.Do(ctx, "GET", fmt.Sprintf("/projects/%d/views", projectID), nil, nil, &out); err != nil { + items, _, err := doList[*ProjectView](ctx, c, fmt.Sprintf("/projects/%d/views", projectID), nil) + if err != nil { return nil, err } - return out, nil + return items, nil } diff --git a/veans/internal/client/relations.go b/veans/internal/client/relations.go index 136e57c58..8de7e3920 100644 --- a/veans/internal/client/relations.go +++ b/veans/internal/client/relations.go @@ -26,7 +26,7 @@ import ( func (c *Client) CreateRelation(ctx context.Context, taskID int64, otherTaskID int64, relationKind string) (*TaskRelation, error) { var out TaskRelation body := &TaskRelation{OtherTaskID: otherTaskID, RelationKind: relationKind} - if err := c.Do(ctx, "PUT", fmt.Sprintf("/tasks/%d/relations", taskID), nil, body, &out); err != nil { + if err := c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d/relations", taskID), nil, body, &out); err != nil { return nil, err } return &out, nil diff --git a/veans/internal/client/routes.go b/veans/internal/client/routes.go index 179773f20..84cab0b41 100644 --- a/veans/internal/client/routes.go +++ b/veans/internal/client/routes.go @@ -40,7 +40,7 @@ func (c *Client) Routes(ctx context.Context) (map[string]RouteGroup, error) { // PermissionsForBot picks a curated subset of route groups the veans bot // needs and projects the available actions of each. Groups not present on // the server are silently dropped, so the resulting permission map is -// always valid for PUT /tokens regardless of Vikunja version. +// always valid for POST /tokens regardless of Vikunja version. // // The action names reflect Vikunja's actual route map (see GET /routes): // bucket CRUD and the bucket-task move endpoint live under the `projects` @@ -56,10 +56,16 @@ func PermissionsForBot(routes map[string]RouteGroup) map[string][]string { }, // Project access: read project metadata, manage buckets & move // tasks between them. tasks_by-index resolves #NN / PROJ-NN. + // The bucket-task MOVE (PUT .../buckets/:bucket/tasks) and the + // buckets-with-tasks LIST (GET .../buckets/tasks) collide on subkey + // `views_buckets_tasks`; which one gets the bare key vs the + // `_put`-suffixed key depends on unspecified route-init order, so we + // request BOTH and let the runtime intersection drop whichever the + // server didn't register. "projects": { "read_one", "read_all", "tasks_by-index", "views_buckets", "views_buckets_put", "views_buckets_post", - "views_buckets_delete", "views_buckets_tasks", + "views_buckets_delete", "views_buckets_tasks", "views_buckets_tasks_put", }, "projects_views": {"read_one", "read_all"}, "labels": {"read_one", "read_all", "create", "update", "delete"}, diff --git a/veans/internal/client/routes_test.go b/veans/internal/client/routes_test.go index 8fc2ab369..fc9687518 100644 --- a/veans/internal/client/routes_test.go +++ b/veans/internal/client/routes_test.go @@ -16,7 +16,10 @@ package client -import "testing" +import ( + "slices" + "testing" +) func TestPermissionsForBot_DropsUnknownGroups(t *testing.T) { // Server only exposes a subset of what we ask for. @@ -61,3 +64,42 @@ func TestPermissionsForBot_EmptyWhenServerIsEmpty(t *testing.T) { t.Fatalf("expected empty map, got %v", got) } } + +// TestPermissionsForBot_ProjectsBucketScopes pins the project-group scopes the +// bot needs for the v2 kanban-bucket calls: list/create/update/delete buckets +// plus the bucket-task MOVE. The MOVE and the buckets-with-tasks LIST collide +// on the `views_buckets_tasks` subkey and the bare-vs-_put assignment depends +// on unspecified route-init order, so the bot must request BOTH keys; the +// runtime intersection keeps whichever the server actually exposes. +func TestPermissionsForBot_ProjectsBucketScopes(t *testing.T) { + // A server that registered the move under the bare key and the list under + // the _put key (one of the two possible orderings). + server := map[string]RouteGroup{ + "projects": { + "read_one": {}, + "read_all": {}, + "tasks_by-index": {}, + "views_buckets": {}, // list buckets + "views_buckets_post": {}, // create bucket + "views_buckets_put": {}, // update bucket + "views_buckets_delete": {}, // delete bucket + "views_buckets_tasks": {}, // bucket-task move OR buckets-with-tasks list + "views_buckets_tasks_put": {}, // the other of the colliding pair + }, + } + got := PermissionsForBot(server) + projects, ok := got["projects"] + if !ok { + t.Fatalf("expected projects group in result") + } + want := []string{ + "read_one", "read_all", "tasks_by-index", + "views_buckets", "views_buckets_post", "views_buckets_put", + "views_buckets_delete", "views_buckets_tasks", "views_buckets_tasks_put", + } + for _, w := range want { + if !slices.Contains(projects, w) { + t.Errorf("projects scope %q missing from bot grant; got %v", w, projects) + } + } +} diff --git a/veans/internal/client/tasks.go b/veans/internal/client/tasks.go index c53c219c5..dbbdbbc2b 100644 --- a/veans/internal/client/tasks.go +++ b/veans/internal/client/tasks.go @@ -52,7 +52,7 @@ func (o *TaskListOptions) values() url.Values { } // ListProjectTasks paginates `GET /projects/{id}/tasks` exhaustively, -// terminating against the server's x-pagination-total-pages header. +// terminating against the list envelope's total_pages. func (c *Client) ListProjectTasks(ctx context.Context, projectID int64, opts *TaskListOptions) ([]*Task, error) { if opts == nil { opts = &TaskListOptions{} @@ -61,19 +61,19 @@ func (c *Client) ListProjectTasks(ctx context.Context, projectID int64, opts *Ta if per <= 0 { per = 50 } + path := fmt.Sprintf("/projects/%d/tasks", projectID) var all []*Task page := 1 for { o := *opts o.Page = page o.PerPage = per - var batch []*Task - total, err := c.DoPaginated(ctx, "GET", fmt.Sprintf("/projects/%d/tasks", projectID), o.values(), &batch) + batch, totalPages, err := doList[*Task](ctx, c, path, o.values()) if err != nil { return nil, err } all = append(all, batch...) - if paginationDone(page, len(batch), per, total) { + if page >= totalPages { return all, nil } page++ @@ -114,24 +114,26 @@ func (t *Task) CurrentBucketID(viewID int64) int64 { return 0 } -// CreateTask inserts a task into a project (PUT /projects/{id}/tasks). +// CreateTask inserts a task into a project (POST /projects/{id}/tasks). func (c *Client) CreateTask(ctx context.Context, projectID int64, t *Task) (*Task, error) { var out Task - if err := c.Do(ctx, "PUT", fmt.Sprintf("/projects/%d/tasks", projectID), nil, t, &out); err != nil { + if err := c.Do(ctx, "POST", fmt.Sprintf("/projects/%d/tasks", projectID), nil, t, &out); err != nil { return nil, err } return &out, nil } -// UpdateTask updates a task (POST /tasks/{id}). This endpoint does NOT -// move tasks between buckets — the task↔bucket relation is row-shaped in -// task_buckets, and bucket_id on the request body is ignored. Use -// MoveTaskToBucket() for that. The server does auto-flip the bucket -// when `done` toggles, but only between the canonical "todo" and "done" -// buckets the project view is configured with. -func (c *Client) UpdateTask(ctx context.Context, id int64, t *Task) (*Task, error) { +// UpdateTask partially updates a task via PATCH /tasks/{id} with a JSON Merge +// Patch body: only the fields set on `patch` are written, the rest are left +// intact (the fix for issue #2962, where a status-only update used to zero +// description and priority). This endpoint does NOT move tasks between +// buckets — the task↔bucket relation is row-shaped in task_buckets, and +// bucket_id on the request body is ignored. Use MoveTaskToBucket() for that. +// The server still auto-flips the bucket when `done` toggles, between the +// canonical "todo" and "done" buckets the project view is configured with. +func (c *Client) UpdateTask(ctx context.Context, id int64, patch *TaskPatch) (*Task, error) { var out Task - if err := c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d", id), nil, t, &out); err != nil { + if err := c.DoMerge(ctx, "PATCH", fmt.Sprintf("/tasks/%d", id), nil, patch, &out); err != nil { return nil, err } return &out, nil diff --git a/veans/internal/client/tokens.go b/veans/internal/client/tokens.go index 58a99b682..a8c0bc054 100644 --- a/veans/internal/client/tokens.go +++ b/veans/internal/client/tokens.go @@ -23,7 +23,7 @@ import "context" // the bot in step 8 of init). func (c *Client) CreateToken(ctx context.Context, t *APIToken) (*APIToken, error) { var out APIToken - if err := c.Do(ctx, "PUT", "/tokens", nil, t, &out); err != nil { + if err := c.Do(ctx, "POST", "/tokens", nil, t, &out); err != nil { return nil, err } return &out, nil diff --git a/veans/internal/client/types.go b/veans/internal/client/types.go index edcabc766..930004c6c 100644 --- a/veans/internal/client/types.go +++ b/veans/internal/client/types.go @@ -29,7 +29,7 @@ type User struct { Email string `json:"email,omitempty"` } -// BotUser is what `PUT /bots` returns. +// BotUser is what `POST /user/bots` returns. type BotUser struct { ID int64 `json:"id"` Username string `json:"username"` @@ -38,7 +38,7 @@ type BotUser struct { Created time.Time `json:"created,omitempty"` } -// BotUserCreate is the request body for PUT /bots. +// BotUserCreate is the request body for POST /user/bots. type BotUserCreate struct { Username string `json:"username"` Name string `json:"name,omitempty"` @@ -119,6 +119,20 @@ type Task struct { PercentDone float64 `json:"percent_done,omitempty"` } +// TaskPatch is the JSON Merge Patch body for UpdateTask (PATCH /tasks/{id}). +// Every field is a pointer with omitempty so only the fields the caller sets +// are serialized; absent fields are left untouched server-side. This is the +// fix for issue #2962 — a status-only update no longer zeroes description or +// priority the way the old whole-object write did. A non-nil pointer to a zero +// value (e.g. *Priority = 0, *Done = false) still serializes, which is how an +// explicit "clear priority" or "reopen" reaches the server. +type TaskPatch struct { + Title *string `json:"title,omitempty"` + Description *string `json:"description,omitempty"` + Done *bool `json:"done,omitempty"` + Priority *int64 `json:"priority,omitempty"` +} + // TaskComment matches pkg/models/task_comments.TaskComment. type TaskComment struct { ID int64 `json:"id"` @@ -138,12 +152,12 @@ type Label struct { Updated time.Time `json:"updated,omitempty"` } -// LabelTask is the body for `PUT /tasks/{id}/labels`. +// LabelTask is the body for `POST /tasks/{id}/labels`. type LabelTask struct { LabelID int64 `json:"label_id"` } -// TaskRelation is the body for `PUT /tasks/{id}/relations` and the row +// TaskRelation is the body for `POST /tasks/{id}/relations` and the row // returned. RelationKind is one of: subtask, parenttask, related, duplicates, // duplicateof, blocking, blocked, precedes, follows, copiedfrom, copiedto. type TaskRelation struct { @@ -152,12 +166,12 @@ type TaskRelation struct { RelationKind string `json:"relation_kind"` } -// TaskAssignee is the body for `PUT /tasks/{id}/assignees`. +// TaskAssignee is the body for `POST /tasks/{id}/assignees`. type TaskAssignee struct { UserID int64 `json:"user_id"` } -// ProjectUser is the body and response for `PUT /projects/{id}/users`. +// ProjectUser is the body and response for `POST /projects/{id}/users`. type ProjectUser struct { ID int64 `json:"id,omitempty"` Username string `json:"username"` @@ -171,7 +185,7 @@ const ( PermissionAdmin = 2 ) -// APIToken is the request and response shape for `PUT /tokens`. The plaintext +// APIToken is the request and response shape for `POST /tokens`. The plaintext // `Token` field is only populated on creation. Vikunja requires ExpiresAt; // callers that want a long-lived token use FarFuture (year 9999). type APIToken struct { @@ -223,8 +237,9 @@ type LoginResponse struct { Token string `json:"token"` } -// OAuthTokenRequest is the JSON body for POST /api/v1/oauth/token. Vikunja's -// OAuth server explicitly rejects form-encoded requests; everything is JSON. +// OAuthTokenRequest is the JSON body for POST /api/v2/oauth/token. The v2 +// endpoint accepts both JSON and form-encoded bodies; veans sends JSON, which +// Huma decodes off the Content-Type header regardless of the declared form. type OAuthTokenRequest struct { GrantType string `json:"grant_type"` Code string `json:"code,omitempty"` diff --git a/veans/internal/client/users.go b/veans/internal/client/users.go index eb2335610..9308fd9ae 100644 --- a/veans/internal/client/users.go +++ b/veans/internal/client/users.go @@ -23,17 +23,17 @@ import ( "code.vikunja.io/veans/internal/output" ) -// CreateBotUser provisions a bot user via PUT /user/bots. The username must +// CreateBotUser provisions a bot user via POST /user/bots. The username must // be prefixed `bot-` (Vikunja enforces this). The caller becomes the bot's // owner, which is what allows them to mint API tokens for the bot via -// PUT /tokens with owner_id. +// POST /tokens with owner_id. // // On Vikunja versions that predate the /user/bots endpoint, the server // returns 404, which we surface as BOT_USERS_UNAVAILABLE so init can fail // fast with a clear message. func (c *Client) CreateBotUser(ctx context.Context, username, name string) (*BotUser, error) { var out BotUser - err := c.Do(ctx, "PUT", "/user/bots", nil, &BotUserCreate{Username: username, Name: name}, &out) + err := c.Do(ctx, "POST", "/user/bots", nil, &BotUserCreate{Username: username, Name: name}, &out) if err != nil { var oe *output.Error if errors.As(err, &oe) && oe.Code == output.CodeNotFound { @@ -45,13 +45,11 @@ func (c *Client) CreateBotUser(ctx context.Context, username, name string) (*Bot return &out, nil } -// ListBotUsers returns all bot users owned by the authenticated user. +// ListBotUsers returns all bot users owned by the authenticated user. The v2 +// endpoint is server-paginated (BotUser.ReadAll applies a 50-item page limit), +// so page through to the end instead of returning only the first page. func (c *Client) ListBotUsers(ctx context.Context) ([]*BotUser, error) { - var out []*BotUser - if err := c.Do(ctx, "GET", "/user/bots", nil, nil, &out); err != nil { - return nil, err - } - return out, nil + return doListAll[*BotUser](ctx, c, "/user/bots") } // FindMyBotByUsername scans the caller's owned bots for one with the given diff --git a/veans/internal/commands/api.go b/veans/internal/commands/api.go index d1e9669c9..b36964ba2 100644 --- a/veans/internal/commands/api.go +++ b/veans/internal/commands/api.go @@ -37,14 +37,14 @@ func newAPICmd() *cobra.Command { cmd := &cobra.Command{ Use: "api ", Short: "Raw REST passthrough — escape hatch for endpoints veans doesn't wrap", - Long: `Sends a request to /api/v1 as the bot. Use this when curated + Long: `Sends a request to /api/v2 as the bot. Use this when curated commands don't shape the data the way you need. The response body is written to stdout verbatim. Examples: veans api GET /projects veans api GET /tasks/123 - veans api POST /tasks/123 --data '{"description":"updated"}' + veans api POST /tasks/123/comments --data '{"comment":"

note

"}' veans api GET /tasks --query expand=reactions --query per_page=100`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/veans/internal/commands/login.go b/veans/internal/commands/login.go index dbc2c9cf4..6d5b9411e 100644 --- a/veans/internal/commands/login.go +++ b/veans/internal/commands/login.go @@ -99,7 +99,7 @@ you want to rotate.`, return err } if minted.Token == "" { - return output.New(output.CodeUnknown, "PUT /tokens did not return token plaintext") + return output.New(output.CodeUnknown, "POST /tokens did not return token plaintext") } if err := credentials.Default().Set(cfg.Server, cfg.Bot.Username, minted.Token); err != nil { diff --git a/veans/internal/commands/update.go b/veans/internal/commands/update.go index a0d272864..63540ee99 100644 --- a/veans/internal/commands/update.go +++ b/veans/internal/commands/update.go @@ -88,7 +88,7 @@ func newUpdateCmd() *cobra.Command { } // runUpdate is intentionally a single linear flow — the steps it performs -// (concurrency check → status → field changes → comments → field POST → +// (concurrency check → status → field changes → comments → field PATCH → // bucket move → label add/remove → refetch) all share the same task, // flag set, and error-handling shape. Splitting them produces five tiny // functions that each take the same five arguments. @@ -126,17 +126,18 @@ func runUpdate(ctx context.Context, rt *runtime, id int64, f *updateFlags) (*cli } } - // Build the update payload incrementally so we don't clobber unmentioned - // fields. The base must include the ID; bucket/done are conditional. - body := &client.Task{ID: id} + // Build the merge-patch payload from only the changed fields. PATCH leaves + // absent fields untouched, so omitting a field preserves it — the id rides + // in the URL, not the body. + body := &client.TaskPatch{} dirty := false if f.title != "" { - body.Title = f.title + body.Title = &f.title dirty = true } if f.priorityIsSet { - body.Priority = f.priority + body.Priority = &f.priority dirty = true } @@ -147,7 +148,7 @@ func runUpdate(ctx context.Context, rt *runtime, id int64, f *updateFlags) (*cli return nil, err } if descChanged { - body.Description = newDesc + body.Description = &newDesc dirty = true } @@ -162,7 +163,8 @@ func runUpdate(ctx context.Context, rt *runtime, id int64, f *updateFlags) (*cli return nil, err } bucketTransitionTarget = bid - body.Done = newStatus.Done() + done := newStatus.Done() + body.Done = &done dirty = true } diff --git a/veans/internal/commands/update_test.go b/veans/internal/commands/update_test.go index 716ab9e8c..a53739a85 100644 --- a/veans/internal/commands/update_test.go +++ b/veans/internal/commands/update_test.go @@ -19,6 +19,7 @@ package commands import ( "context" "encoding/json" + "io" "net/http" "net/http/httptest" "reflect" @@ -155,28 +156,29 @@ func startRecordingServer(t *testing.T) (*httptest.Server, *[]recordedCall) { w.Header().Set("Content-Type", "application/json") switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/tasks/42": + case r.Method == http.MethodGet && r.URL.Path == "/api/v2/tasks/42": // Initial fetch + the final refetch both land here. Return a // fixed task with an empty label set — labels.go's // findLabelOnTask only iterates t.Labels. _ = json.NewEncoder(w).Encode(map[string]any{ "id": 42, "title": "t", "updated": "2026-01-01T00:00:00Z", }) - case r.Method == http.MethodPut && r.URL.Path == "/api/v1/tasks/42/comments": + case r.Method == http.MethodPost && r.URL.Path == "/api/v2/tasks/42/comments": _ = json.NewEncoder(w).Encode(map[string]any{"id": 1, "comment": ""}) - case r.Method == http.MethodPost && r.URL.Path == "/api/v1/tasks/42": - // UpdateTask. Echo back the id so the encoder downstream is - // happy with a non-nil Task. + case r.Method == http.MethodPatch && r.URL.Path == "/api/v2/tasks/42": + // UpdateTask (merge-patch). Echo back the id so the encoder + // downstream is happy with a non-nil Task. _ = json.NewEncoder(w).Encode(map[string]any{"id": 42}) - case r.Method == http.MethodPost && strings.HasPrefix(r.URL.Path, "/api/v1/projects/") && strings.HasSuffix(r.URL.Path, "/tasks"): + case r.Method == http.MethodPut && strings.HasPrefix(r.URL.Path, "/api/v2/projects/") && strings.HasSuffix(r.URL.Path, "/tasks"): + // Bucket-task move (PUT .../buckets/{b}/tasks). _ = json.NewEncoder(w).Encode(map[string]any{"id": 42}) - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/labels": - // getOrCreateLabelByTitle's lookup. Empty array → falls through + case r.Method == http.MethodGet && r.URL.Path == "/api/v2/labels": + // getOrCreateLabelByTitle's lookup. Empty envelope → falls through // to label creation. - _ = json.NewEncoder(w).Encode([]any{}) - case r.Method == http.MethodPut && r.URL.Path == "/api/v1/labels": + _ = json.NewEncoder(w).Encode(map[string]any{"items": []any{}, "total_pages": 1}) + case r.Method == http.MethodPost && r.URL.Path == "/api/v2/labels": _ = json.NewEncoder(w).Encode(map[string]any{"id": 99, "title": "veans:bug"}) - case r.Method == http.MethodPut && r.URL.Path == "/api/v1/tasks/42/labels": + case r.Method == http.MethodPost && r.URL.Path == "/api/v2/tasks/42/labels": _ = json.NewEncoder(w).Encode(map[string]any{"id": 99}) default: t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) @@ -222,11 +224,11 @@ func TestRunUpdate_ScrappedOrdersCommentUpdateMove(t *testing.T) { } want := []recordedCall{ - {http.MethodGet, "/api/v1/tasks/42"}, // current task fetch - {http.MethodPut, "/api/v1/tasks/42/comments"}, // "Scrapped: obsolete" - {http.MethodPost, "/api/v1/tasks/42"}, // field update (done=true) - {http.MethodPost, "/api/v1/projects/7/views/1/buckets/14/tasks"}, // bucket move to Scrapped - {http.MethodGet, "/api/v1/tasks/42"}, // refetch with new bucket + {http.MethodGet, "/api/v2/tasks/42"}, // current task fetch + {http.MethodPost, "/api/v2/tasks/42/comments"}, // "Scrapped: obsolete" + {http.MethodPatch, "/api/v2/tasks/42"}, // field update (done=true) + {http.MethodPut, "/api/v2/projects/7/views/1/buckets/14/tasks"}, // bucket move to Scrapped + {http.MethodGet, "/api/v2/tasks/42"}, // refetch with new bucket } if !reflect.DeepEqual(*calls, want) { t.Fatalf("call order mismatch:\nwant: %#v\ngot: %#v", want, *calls) @@ -253,15 +255,108 @@ func TestRunUpdate_BucketMoveBeforeLabelAdd(t *testing.T) { } want := []recordedCall{ - {http.MethodGet, "/api/v1/tasks/42"}, // current task fetch - {http.MethodPost, "/api/v1/tasks/42"}, // field update (done=false) - {http.MethodPost, "/api/v1/projects/7/views/1/buckets/11/tasks"}, // bucket move to In Progress - {http.MethodGet, "/api/v1/labels"}, // getOrCreateLabelByTitle lookup - {http.MethodPut, "/api/v1/labels"}, // create veans:bug - {http.MethodPut, "/api/v1/tasks/42/labels"}, // attach label - {http.MethodGet, "/api/v1/tasks/42"}, // refetch + {http.MethodGet, "/api/v2/tasks/42"}, // current task fetch + {http.MethodPatch, "/api/v2/tasks/42"}, // field update (done=false) + {http.MethodPut, "/api/v2/projects/7/views/1/buckets/11/tasks"}, // bucket move to In Progress + {http.MethodGet, "/api/v2/labels"}, // getOrCreateLabelByTitle lookup + {http.MethodPost, "/api/v2/labels"}, // create veans:bug + {http.MethodPost, "/api/v2/tasks/42/labels"}, // attach label + {http.MethodGet, "/api/v2/tasks/42"}, // refetch } if !reflect.DeepEqual(*calls, want) { t.Fatalf("call order mismatch:\nwant: %#v\ngot: %#v", want, *calls) } } + +// startPatchCapturingServer answers the endpoints a field-only runUpdate +// touches (no labels) and records the raw JSON body of the merge-patch +// PATCH /tasks/{id} request so a test can assert exactly which fields were +// sent. The seeded task carries a description and priority that a partial +// update must leave untouched (issue #2962). +func startPatchCapturingServer(t *testing.T) (*httptest.Server, *[]byte) { + t.Helper() + var ( + mu sync.Mutex + patchBody []byte + ) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v2/tasks/42": + _ = json.NewEncoder(w).Encode(map[string]any{ + "id": 42, "title": "t", "description": "keep me", + "priority": 4, "updated": "2026-01-01T00:00:00Z", + }) + case r.Method == http.MethodPatch && r.URL.Path == "/api/v2/tasks/42": + b, _ := io.ReadAll(r.Body) + mu.Lock() + patchBody = b + mu.Unlock() + _ = json.NewEncoder(w).Encode(map[string]any{"id": 42}) + case r.Method == http.MethodPut && strings.HasPrefix(r.URL.Path, "/api/v2/projects/") && strings.HasSuffix(r.URL.Path, "/tasks"): + _ = json.NewEncoder(w).Encode(map[string]any{"id": 42}) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected", http.StatusInternalServerError) + } + })) + t.Cleanup(srv.Close) + return srv, &patchBody +} + +// TestRunUpdate_TitleOnlyPatchOmitsOtherFields is the #2962 acceptance test for +// a title-only update: the merge-patch body must contain ONLY title, so the +// stored description/priority/done are left intact server-side. +func TestRunUpdate_TitleOnlyPatchOmitsOtherFields(t *testing.T) { + srv, patchBody := startPatchCapturingServer(t) + rt := newTestRuntime(srv.URL) + + if _, err := runUpdate(context.Background(), rt, 42, &updateFlags{title: "new title"}); err != nil { + t.Fatalf("runUpdate: %v", err) + } + + body := decodePatchBody(t, *patchBody) + if body["title"] != "new title" { + t.Errorf("patch should set title; got %v", body["title"]) + } + for _, k := range []string{"description", "priority", "done"} { + if _, present := body[k]; present { + t.Errorf("title-only update must not send %q (merge-patch would clobber it); body=%s", k, *patchBody) + } + } +} + +// TestRunUpdate_StatusOnlyPatchPreservesDescriptionAndPriority is the #2962 +// acceptance test for a status-only update: the merge-patch body carries the +// done flag (and nothing else), so description and priority survive — the +// regression the whole-object POST caused. +func TestRunUpdate_StatusOnlyPatchPreservesDescriptionAndPriority(t *testing.T) { + srv, patchBody := startPatchCapturingServer(t) + rt := newTestRuntime(srv.URL) + + if _, err := runUpdate(context.Background(), rt, 42, &updateFlags{statusName: "in-progress"}); err != nil { + t.Fatalf("runUpdate: %v", err) + } + + body := decodePatchBody(t, *patchBody) + if d, ok := body["done"].(bool); !ok || d { + t.Errorf("in-progress status should send done=false; got %v", body["done"]) + } + for _, k := range []string{"description", "priority", "title"} { + if _, present := body[k]; present { + t.Errorf("status-only update must not send %q (#2962: it would clobber the stored value); body=%s", k, *patchBody) + } + } +} + +func decodePatchBody(t *testing.T, raw []byte) map[string]any { + t.Helper() + if len(raw) == 0 { + t.Fatal("no PATCH body was captured") + } + var body map[string]any + if err := json.Unmarshal(raw, &body); err != nil { + t.Fatalf("decode patch body %q: %v", string(raw), err) + } + return body +}