diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 8830789..621a006 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -1393,6 +1393,37 @@ paths: /api/v1/project-groups/{groupID}: parameters: - { name: groupID, in: path, required: true, schema: { type: string } } + get: + tags: [projects] + summary: The group as a place, with what is in it. + description: | + A heading can carry a name. A page carries what the group *is*: the + sentence saying why these projects are one body of work, the documents + that belong to all of them rather than to any one, and the projects + themselves. + + The projects come with the group rather than being fetched per row — the + page is nothing without them, and two round trips to draw one list is one + too many. + + A group that is not yours is not found. That is the rule for every route + here, and it is enforced by the lookup rather than by a separate check. + responses: + "200": + description: OK + content: + application/json: + schema: + type: object + properties: + group: { $ref: "#/components/schemas/ProjectGroup" } + projects: + type: array + items: { $ref: "#/components/schemas/Project" } + attachments: + type: array + items: { $ref: "#/components/schemas/Attachment" } + "404": { $ref: "#/components/responses/NotFound" } patch: tags: [projects] summary: Rename a group, or fold it. @@ -1420,6 +1451,41 @@ paths: "204": { description: Deleted. } "404": { $ref: "#/components/responses/NotFound" } + /api/v1/project-groups/{groupID}/attachments: + parameters: + - { name: groupID, in: path, required: true, schema: { type: string } } + post: + tags: [projects] + summary: Put a document on the group itself. + description: | + A file that belongs to the whole group rather than to any one task in it — + the contract that governs all of "Arbejde", not a step in it. + + The same storage as every other attachment: content-addressed by hash, so + the path never contains anything the uploader chose, and the same file + uploaded twice is stored once. 25 MB maximum. + + Downloading and deleting go through `/attachments/{attachmentID}` like any + other. A group's attachment is reachable by its owner and nobody else, + because a group belongs to one person and is never shared. + requestBody: + required: true + content: + multipart/form-data: + schema: + type: object + properties: + file: { type: string, format: binary } + responses: + "201": + description: Stored. + content: + application/json: + schema: { $ref: "#/components/schemas/Attachment" } + "404": { $ref: "#/components/responses/NotFound" } + "413": { $ref: "#/components/responses/TooLarge" } + "422": { $ref: "#/components/responses/Validation" } + /api/v1/trash/projects: get: tags: [projects] @@ -3067,6 +3133,12 @@ components: id: { type: string } name: { type: string } color: { $ref: "#/components/schemas/Color" } + description: + type: string + description: | + What the group is, in the owner's words. A heading carries a name; a + page carries the sentence that says why these projects are one body of + work. Empty until somebody writes one. collapsed: { type: boolean } sort_order: { type: number } @@ -3075,6 +3147,10 @@ components: properties: name: { type: string } color: { $ref: "#/components/schemas/Color" } + description: + type: string + maxLength: 4000 + description: What the group is, in the owner's words. collapsed: { type: boolean } Color: diff --git a/internal/httpapi/comments_handlers.go b/internal/httpapi/comments_handlers.go index 8ab4b33..334aa96 100644 --- a/internal/httpapi/comments_handlers.go +++ b/internal/httpapi/comments_handlers.go @@ -300,16 +300,48 @@ func (s *Server) storeUpload(file io.Reader) (string, int64, error) { return rel, size, nil } -func (s *Server) handleDownloadAttachment(w http.ResponseWriter, r *http.Request) { - attachmentID := chi.URLParam(r, "attachmentID") +// reachAttachment answers whether this person may see an attachment at all, and +// whether they may change it. +// +// An attachment hangs on exactly one of three things, and each has its own idea of +// who may reach it. A task's is governed by its project's roles, directly or +// through the comment it hangs on. A group's is governed by nothing but ownership: +// a group belongs to one person, is never shared, and a group that is not yours is +// not found — the same rule the rest of the group routes use. +// +// Both callers below used to resolve straight to a task, so a group's attachment +// answered 404 for its own owner. +func (s *Server) reachAttachment(r *http.Request, attachmentID string) (canEdit bool, err error) { user := userFrom(r.Context()) + a, err := s.db.GetAttachment(r.Context(), attachmentID) + if err != nil { + return false, err + } + + if a.GroupID != "" { + if _, err := s.db.GetProjectGroup(r.Context(), a.GroupID, user.ID); err != nil { + return false, err + } + // Nobody else can reach it, so whoever can, can change it. + return true, nil + } + taskID, err := s.db.AttachmentTask(r.Context(), attachmentID) if err != nil { - s.storeError(w, r, "attachment task", err) - return + return false, err } - if _, err := store.TaskRole(r.Context(), s.db, taskID, user.ID); err != nil { + role, err := store.TaskRole(r.Context(), s.db, taskID, user.ID) + if err != nil { + return false, err + } + return role.CanEdit(), nil +} + +func (s *Server) handleDownloadAttachment(w http.ResponseWriter, r *http.Request) { + attachmentID := chi.URLParam(r, "attachmentID") + + if _, err := s.reachAttachment(r, attachmentID); err != nil { writeError(w, http.StatusNotFound, CodeNotFound, "not found") return } @@ -354,15 +386,9 @@ func (s *Server) handleDownloadAttachment(w http.ResponseWriter, r *http.Request func (s *Server) handleDeleteAttachment(w http.ResponseWriter, r *http.Request) { attachmentID := chi.URLParam(r, "attachmentID") - user := userFrom(r.Context()) - taskID, err := s.db.AttachmentTask(r.Context(), attachmentID) - if err != nil { - s.storeError(w, r, "attachment task", err) - return - } - role, err := store.TaskRole(r.Context(), s.db, taskID, user.ID) - if err != nil || !role.CanEdit() { + canEdit, err := s.reachAttachment(r, attachmentID) + if err != nil || !canEdit { writeError(w, http.StatusNotFound, CodeNotFound, "not found") return } diff --git a/internal/httpapi/projectgroups_handlers.go b/internal/httpapi/projectgroups_handlers.go index 12eaef0..f7e7b75 100644 --- a/internal/httpapi/projectgroups_handlers.go +++ b/internal/httpapi/projectgroups_handlers.go @@ -2,6 +2,8 @@ package httpapi import ( "net/http" + "os" + "path/filepath" "strings" "github.com/go-chi/chi/v5" @@ -17,16 +19,17 @@ import ( // that is not yours is not found. type projectGroupJSON struct { - ID string `json:"id"` - Name string `json:"name"` - Color string `json:"color"` - Collapsed bool `json:"collapsed"` - SortOrder float64 `json:"sort_order"` + ID string `json:"id"` + Name string `json:"name"` + Color string `json:"color"` + Description string `json:"description"` + Collapsed bool `json:"collapsed"` + SortOrder float64 `json:"sort_order"` } func toProjectGroupJSON(g store.ProjectGroup) projectGroupJSON { return projectGroupJSON{ - ID: g.ID, Name: g.Name, Color: g.Color, + ID: g.ID, Name: g.Name, Color: g.Color, Description: g.Description, Collapsed: g.Collapsed, SortOrder: g.SortOrder, } } @@ -45,9 +48,10 @@ func (s *Server) handleListProjectGroups(w http.ResponseWriter, r *http.Request) } type projectGroupRequest struct { - Name *string `json:"name"` - Color *string `json:"color"` - Collapsed *bool `json:"collapsed"` + Name *string `json:"name"` + Color *string `json:"color"` + Description *string `json:"description"` + Collapsed *bool `json:"collapsed"` } func (s *Server) handleCreateProjectGroup(w http.ResponseWriter, r *http.Request) { @@ -73,6 +77,9 @@ func (s *Server) handleCreateProjectGroup(w http.ResponseWriter, r *http.Request if req.Color != nil { g.Color = *req.Color } + if req.Description != nil { + g.Description = strings.TrimSpace(*req.Description) + } if req.Collapsed != nil { g.Collapsed = *req.Collapsed } @@ -105,8 +112,18 @@ func (s *Server) handleUpdateProjectGroup(w http.ResponseWriter, r *http.Request return } + if req.Description != nil { + description := strings.TrimSpace(*req.Description) + if len(description) > 4000 { + writeFieldErrors(w, map[string]string{"description": "must be at most 4000 characters"}) + return + } + req.Description = &description + } + if err := s.db.UpdateProjectGroup(r.Context(), groupID, userID, store.ProjectGroupUpdate{ - Name: req.Name, Color: req.Color, Collapsed: req.Collapsed, + Name: req.Name, Color: req.Color, Description: req.Description, + Collapsed: req.Collapsed, }); err != nil { s.storeError(w, r, "update project group", err) return @@ -120,6 +137,126 @@ func (s *Server) handleUpdateProjectGroup(w http.ResponseWriter, r *http.Request writeJSON(w, http.StatusOK, toProjectGroupJSON(*g)) } +// groupProjectJSON is a project as the group's page lists it: the project, plus +// how much is left in it. Open tasks, not all of them — "12" beside a project +// somebody finished last year is a number that means nothing. +type groupProjectJSON struct { + projectJSON + OpenTasks int `json:"open_tasks"` +} + +// handleGetProjectGroup is the group as a place rather than a heading. +// +// A heading can carry a name. A page carries what the group *is* — the sentence +// that says why these projects are one body of work, the documents that belong to +// all of them rather than to any one, and the projects themselves with what is +// left in each. +// +// The projects come with the group rather than being fetched per row, because the +// page is nothing without them and two round trips to draw one list is one too +// many. +func (s *Server) handleGetProjectGroup(w http.ResponseWriter, r *http.Request) { + groupID := chi.URLParam(r, "groupID") + userID := userFrom(r.Context()).ID + + g, err := s.db.GetProjectGroup(r.Context(), groupID, userID) + if err != nil { + s.storeError(w, r, "get project group", err) + return + } + + projects, err := s.db.ListProjects(r.Context(), userID, false) + if err != nil { + s.internal(w, r, "list projects", err) + return + } + counts, err := s.db.OpenTaskCounts(r.Context(), groupID, userID) + if err != nil { + s.internal(w, r, "open task counts", err) + return + } + inside := make([]groupProjectJSON, 0, 4) + for _, p := range projects { + if p.GroupID == groupID { + inside = append(inside, groupProjectJSON{ + projectJSON: toProjectJSON(p), OpenTasks: counts[p.ID], + }) + } + } + + files, err := s.db.ListGroupAttachments(r.Context(), groupID) + if err != nil { + s.internal(w, r, "list group attachments", err) + return + } + attachments := make([]attachmentJSON, 0, len(files)) + for _, a := range files { + attachments = append(attachments, toAttachmentJSON(a)) + } + + writeJSON(w, http.StatusOK, map[string]any{ + "group": toProjectGroupJSON(*g), "projects": inside, "attachments": attachments, + }) +} + +// handleUploadGroupAttachment puts a document on the group itself. +// +// The same storage as everywhere else — content-addressed by hash, so the path +// never contains anything the uploader chose. What differs is only the parent. +func (s *Server) handleUploadGroupAttachment(w http.ResponseWriter, r *http.Request) { + groupID := chi.URLParam(r, "groupID") + user := userFrom(r.Context()) + + // Scoped by the lookup rather than by a separate check: a group that is not + // yours is not found, which is the rule for everything in this file. + if _, err := s.db.GetProjectGroup(r.Context(), groupID, user.ID); err != nil { + s.storeError(w, r, "get project group", err) + return + } + + r.Body = http.MaxBytesReader(w, r.Body, maxUploadBytes+(1<<20)) + if err := r.ParseMultipartForm(8 << 20); err != nil { + writeError(w, http.StatusRequestEntityTooLarge, CodePayloadTooLarge, + "that file is too large (25 MB maximum)") + return + } + defer r.MultipartForm.RemoveAll() + + file, header, err := r.FormFile("file") + if err != nil { + writeFieldErrors(w, map[string]string{"file": "required"}) + return + } + defer file.Close() + + if header.Size > maxUploadBytes { + writeError(w, http.StatusRequestEntityTooLarge, CodePayloadTooLarge, + "that file is too large (25 MB maximum)") + return + } + + stored, size, err := s.storeUpload(file) + if err != nil { + s.internal(w, r, "store upload", err) + return + } + + a := &store.Attachment{ + GroupID: groupID, + Filename: safeFilename(header.Filename), + MimeType: detectMime(header.Filename, header.Header.Get("Content-Type")), + Size: size, + Path: stored, + UploadedBy: user.ID, + } + if err := s.db.CreateAttachment(r.Context(), a); err != nil { + os.Remove(filepath.Join(s.cfg.FilesDir(), stored)) + s.internal(w, r, "record attachment", err) + return + } + writeJSON(w, http.StatusCreated, toAttachmentJSON(*a)) +} + // handleDeleteProjectGroup removes the heading. The projects under it come back // out as ungrouped, which is why this is not behind a confirmation about losing // work: there is none to lose. diff --git a/internal/httpapi/projectgroups_test.go b/internal/httpapi/projectgroups_test.go index a8dddf3..da47021 100644 --- a/internal/httpapi/projectgroups_test.go +++ b/internal/httpapi/projectgroups_test.go @@ -269,3 +269,83 @@ func TestAColourMustBeOneFromThePalette(t *testing.T) { t.Errorf("name = %v", renamed["name"]) } } + +// A group is somewhere you can go, not only a heading. +func TestAGroupIsAPageWithItsProjectsDescriptionAndFiles(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, group := ts.do(t, "POST", "/api/v1/project-groups", map[string]any{"name": "Arbejde"}) + groupID := group["id"].(string) + + _, first := ts.do(t, "POST", "/api/v1/projects", map[string]any{ + "name": "Sæsonstart", "group_id": groupID, + }) + ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Regnskab", "group_id": groupID}) + // And one outside it, which must not appear. + ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Haven"}) + + // Two open, one closed — the count is what is left, not what has ever been. + ts.do(t, "POST", "/api/v1/tasks", map[string]any{ + "content": "skriv oplæg", "project_id": first["id"].(string), + }) + _, done := ts.do(t, "POST", "/api/v1/tasks", map[string]any{ + "content": "book lokale", "project_id": first["id"].(string), + }) + ts.do(t, "POST", "/api/v1/tasks/"+done["id"].(string)+"/complete", nil) + + resp, page := ts.do(t, "GET", "/api/v1/project-groups/"+groupID, nil) + if resp.StatusCode != http.StatusOK { + t.Fatalf("get group: status %d, body %v", resp.StatusCode, page) + } + projects, _ := page["projects"].([]any) + if len(projects) != 2 { + t.Fatalf("the page lists %d projects, want 2", len(projects)) + } + for _, raw := range projects { + p := raw.(map[string]any) + if p["name"] == "Haven" { + t.Error("a project outside the group is on its page") + } + if p["name"] == "Sæsonstart" && p["open_tasks"] != float64(1) { + t.Errorf("open_tasks = %v, want 1 — the closed one must not count", p["open_tasks"]) + } + } + + // The description, which is the part a heading cannot carry. + if resp, _ := ts.do(t, "PATCH", "/api/v1/project-groups/"+groupID, map[string]any{ + "description": "Alt det, der har en faktura for enden.", + }); resp.StatusCode != http.StatusOK { + t.Fatalf("set description: status %d", resp.StatusCode) + } + _, again := ts.do(t, "GET", "/api/v1/project-groups/"+groupID, nil) + if g, _ := again["group"].(map[string]any); g["description"] != "Alt det, der har en faktura for enden." { + t.Errorf("description = %v", g["description"]) + } + // And setting only the description must not lose the name or colour. + if g, _ := again["group"].(map[string]any); g["name"] != "Arbejde" { + t.Errorf("the name went with the description: %v", g["name"]) + } +} + +// A group belongs to one person and is never shared, so somebody else's group is +// not found — including its files. +func TestAGroupsPageAndFilesAreItsOwnersAlone(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + other := ts.newUser(t, "anden@example.dk", "Anden") + + _, group := ts.do(t, "POST", "/api/v1/project-groups", map[string]any{"name": "Arbejde"}) + groupID := group["id"].(string) + + for _, c := range []struct{ method, path string }{ + {"GET", "/api/v1/project-groups/" + groupID}, + {"PATCH", "/api/v1/project-groups/" + groupID}, + {"POST", "/api/v1/project-groups/" + groupID + "/attachments"}, + } { + resp, _ := other.do(t, c.method, c.path, map[string]any{"description": "min nu"}) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("%s %s as somebody else: status %d, want 404", c.method, c.path, resp.StatusCode) + } + } +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 0510304..55138f2 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -344,8 +344,10 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server r.Post("/", s.handleCreateProjectGroup) // Above /{groupID}, or chi reads "reorder" as an id. r.Post("/reorder", s.handleReorderProjectGroups) + r.Get("/{groupID}", s.handleGetProjectGroup) r.Patch("/{groupID}", s.handleUpdateProjectGroup) r.Delete("/{groupID}", s.handleDeleteProjectGroup) + r.Post("/{groupID}/attachments", s.handleUploadGroupAttachment) }) // The trash is outside the block above: a deleted project is invisible diff --git a/internal/store/comments.go b/internal/store/comments.go index 697a169..671a47f 100644 --- a/internal/store/comments.go +++ b/internal/store/comments.go @@ -21,9 +21,12 @@ type Comment struct { } type Attachment struct { - ID string - TaskID string - CommentID string + ID string + TaskID string + CommentID string + // GroupID hangs the file on a project group rather than on any one task in + // it: the contract that governs all of "Arbejde", not a step in it. + GroupID string Filename string MimeType string Size int64 @@ -154,17 +157,17 @@ func (db *DB) CreateAttachment(ctx context.Context, a *Attachment) error { a.CreatedAt = time.Now().UTC() _, err := db.ExecContext(ctx, - `INSERT INTO attachments (id, task_id, comment_id, filename, mime_type, size, - path, uploaded_by, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, - a.ID, nullString(a.TaskID), nullString(a.CommentID), a.Filename, - a.MimeType, a.Size, a.Path, a.UploadedBy, a.CreatedAt.Unix()) + `INSERT INTO attachments (id, task_id, comment_id, group_id, filename, mime_type, + size, path, uploaded_by, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + a.ID, nullString(a.TaskID), nullString(a.CommentID), nullString(a.GroupID), + a.Filename, a.MimeType, a.Size, a.Path, a.UploadedBy, a.CreatedAt.Unix()) return err } func (db *DB) ListTaskAttachments(ctx context.Context, taskID string) ([]Attachment, error) { rows, err := db.QueryContext(ctx, - `SELECT id, task_id, comment_id, filename, mime_type, size, path, uploaded_by, created_at + `SELECT id, task_id, comment_id, group_id, filename, mime_type, size, path, uploaded_by, created_at FROM attachments WHERE task_id = ? ORDER BY created_at`, taskID) if err != nil { return nil, err @@ -173,13 +176,26 @@ func (db *DB) ListTaskAttachments(ctx context.Context, taskID string) ([]Attachm return scanAttachments(rows) } +// ListGroupAttachments is the documents that belong to a whole group rather than +// to anything inside it. +func (db *DB) ListGroupAttachments(ctx context.Context, groupID string) ([]Attachment, error) { + rows, err := db.QueryContext(ctx, + `SELECT id, task_id, comment_id, group_id, filename, mime_type, size, path, uploaded_by, created_at + FROM attachments WHERE group_id = ? ORDER BY created_at`, groupID) + if err != nil { + return nil, err + } + defer rows.Close() + return scanAttachments(rows) +} + func (db *DB) attachmentsByComment(ctx context.Context, commentIDs []string) (map[string][]Attachment, error) { args := make([]any, len(commentIDs)) for i, id := range commentIDs { args[i] = id } rows, err := db.QueryContext(ctx, - `SELECT id, task_id, comment_id, filename, mime_type, size, path, uploaded_by, created_at + `SELECT id, task_id, comment_id, group_id, filename, mime_type, size, path, uploaded_by, created_at FROM attachments WHERE comment_id IN (`+placeholders(len(commentIDs))+`) ORDER BY created_at`, args...) if err != nil { @@ -202,14 +218,15 @@ func scanAttachments(rows *sql.Rows) ([]Attachment, error) { out := []Attachment{} for rows.Next() { var a Attachment - var taskID, commentID sql.NullString + var taskID, commentID, groupID sql.NullString var created int64 - if err := rows.Scan(&a.ID, &taskID, &commentID, &a.Filename, &a.MimeType, + if err := rows.Scan(&a.ID, &taskID, &commentID, &groupID, &a.Filename, &a.MimeType, &a.Size, &a.Path, &a.UploadedBy, &created); err != nil { return nil, err } a.TaskID = taskID.String a.CommentID = commentID.String + a.GroupID = groupID.String a.CreatedAt = time.Unix(created, 0).UTC() out = append(out, a) } @@ -218,7 +235,7 @@ func scanAttachments(rows *sql.Rows) ([]Attachment, error) { func (db *DB) GetAttachment(ctx context.Context, attachmentID string) (*Attachment, error) { rows, err := db.QueryContext(ctx, - `SELECT id, task_id, comment_id, filename, mime_type, size, path, uploaded_by, created_at + `SELECT id, task_id, comment_id, group_id, filename, mime_type, size, path, uploaded_by, created_at FROM attachments WHERE id = ?`, attachmentID) if err != nil { return nil, err diff --git a/internal/store/migrations/0012_groups_are_places.sql b/internal/store/migrations/0012_groups_are_places.sql new file mode 100644 index 0000000..42122f4 --- /dev/null +++ b/internal/store/migrations/0012_groups_are_places.sql @@ -0,0 +1,53 @@ +-- A group becomes somewhere you can go, rather than only a heading. +-- +-- Until now a project group was a label over some rows in the sidebar: a name, a +-- colour, and whether it was folded. That is enough to tidy a list and not enough +-- to be the thing people actually mean by it — "Arbejde" is a body of work with +-- context of its own: what it is, and the documents that belong to all of it +-- rather than to any one project inside it. +-- +-- Two changes. A description on the group, and attachments that can hang on one. +-- +-- The attachments table has to be rebuilt for the second, because its CHECK spells +-- out exactly which parent columns may be set, and SQLite cannot alter a CHECK. +-- Foreign keys go off for the length of it — see the marker below and the note in +-- `applyMigration` — because dropping the old table with them on is a cascading +-- delete of every attachment in the instance. +-- +-- The rule stays "exactly one parent": an attachment belongs to a task, a comment +-- or a group, never two. An attachment with two parents is one that gets deleted +-- once and stays visible in the other place. +-- +-- verdande:rebuild-tables + +ALTER TABLE project_groups ADD COLUMN description TEXT NOT NULL DEFAULT ''; + +CREATE TABLE attachments_rebuilt ( + id TEXT PRIMARY KEY, + task_id TEXT REFERENCES tasks (id) ON DELETE CASCADE, + comment_id TEXT REFERENCES comments (id) ON DELETE CASCADE, + group_id TEXT REFERENCES project_groups (id) ON DELETE CASCADE, + filename TEXT NOT NULL, + mime_type TEXT NOT NULL DEFAULT 'application/octet-stream', + size INTEGER NOT NULL, + path TEXT NOT NULL, + uploaded_by TEXT NOT NULL REFERENCES users (id) ON DELETE CASCADE, + created_at INTEGER NOT NULL, + -- Exactly one parent, counted rather than spelled out as a chain of pairwise + -- comparisons: with three columns that is three clauses, with four it is six, + -- and the version that is wrong looks exactly like the version that is right. + CHECK ((task_id IS NOT NULL) + (comment_id IS NOT NULL) + (group_id IS NOT NULL) = 1) +); + +INSERT INTO attachments_rebuilt + (id, task_id, comment_id, filename, mime_type, size, path, uploaded_by, created_at) +SELECT id, task_id, comment_id, filename, mime_type, size, path, uploaded_by, created_at +FROM attachments; + +DROP TABLE attachments; + +ALTER TABLE attachments_rebuilt RENAME TO attachments; + +CREATE INDEX idx_attachments_task ON attachments (task_id) WHERE task_id IS NOT NULL; +CREATE INDEX idx_attachments_comment ON attachments (comment_id) WHERE comment_id IS NOT NULL; +CREATE INDEX idx_attachments_group ON attachments (group_id) WHERE group_id IS NOT NULL; diff --git a/internal/store/projectgroups.go b/internal/store/projectgroups.go index 63ee825..f0d5d54 100644 --- a/internal/store/projectgroups.go +++ b/internal/store/projectgroups.go @@ -15,19 +15,23 @@ import ( // not. That is also why a shared project can never be in a group — see // SetProjectGroup. type ProjectGroup struct { - ID string - OwnerID string - Name string - Color string - Collapsed bool - SortOrder float64 - CreatedAt time.Time - UpdatedAt time.Time + ID string + OwnerID string + Name string + Color string + // Description is what the group is, in the owner's words. A heading can carry + // a name; a page can carry the sentence that says why these projects are the + // same body of work. + Description string + Collapsed bool + SortOrder float64 + CreatedAt time.Time + UpdatedAt time.Time } func (db *DB) ListProjectGroups(ctx context.Context, userID string) ([]ProjectGroup, error) { rows, err := db.QueryContext(ctx, - `SELECT id, owner_id, name, color, collapsed, sort_order, created_at, updated_at + `SELECT id, owner_id, name, color, description, collapsed, sort_order, created_at, updated_at FROM project_groups WHERE owner_id = ? ORDER BY sort_order, created_at`, userID) if err != nil { @@ -40,8 +44,8 @@ func (db *DB) ListProjectGroups(ctx context.Context, userID string) ([]ProjectGr var g ProjectGroup var collapsed int var created, updated int64 - if err := rows.Scan(&g.ID, &g.OwnerID, &g.Name, &g.Color, &collapsed, &g.SortOrder, - &created, &updated); err != nil { + if err := rows.Scan(&g.ID, &g.OwnerID, &g.Name, &g.Color, &g.Description, &collapsed, + &g.SortOrder, &created, &updated); err != nil { return nil, err } g.Collapsed = collapsed == 1 @@ -60,9 +64,10 @@ func (db *DB) GetProjectGroup(ctx context.Context, groupID, userID string) (*Pro var collapsed int var created, updated int64 err := db.QueryRowContext(ctx, - `SELECT id, owner_id, name, color, collapsed, sort_order, created_at, updated_at + `SELECT id, owner_id, name, color, description, collapsed, sort_order, created_at, updated_at FROM project_groups WHERE id = ? AND owner_id = ?`, groupID, userID). - Scan(&g.ID, &g.OwnerID, &g.Name, &g.Color, &collapsed, &g.SortOrder, &created, &updated) + Scan(&g.ID, &g.OwnerID, &g.Name, &g.Color, &g.Description, &collapsed, &g.SortOrder, + &created, &updated) if errors.Is(err, sql.ErrNoRows) { return nil, ErrNotFound } @@ -101,18 +106,20 @@ func (db *DB) CreateProjectGroup(ctx context.Context, g *ProjectGroup) error { } type ProjectGroupUpdate struct { - Name *string - Color *string - Collapsed *bool - SortOrder *float64 + Name *string + Color *string + Description *string + Collapsed *bool + SortOrder *float64 } func (db *DB) UpdateProjectGroup(ctx context.Context, groupID, userID string, u ProjectGroupUpdate) error { set, args := buildUpdate(map[string]any{ - "name": u.Name, - "color": u.Color, - "collapsed": u.Collapsed, - "sort_order": u.SortOrder, + "name": u.Name, + "color": u.Color, + "description": u.Description, + "collapsed": u.Collapsed, + "sort_order": u.SortOrder, }) if len(set) == 0 { return nil @@ -209,3 +216,37 @@ func (db *DB) SetProjectGroup(ctx context.Context, projectID, groupID, userID st } return nil } + +// OpenTaskCounts is how much is left in each project of a group. +// +// One query rather than one per project: the group's page draws a list, and a +// count per row would be a query per row. +// +// Open, not total. "12" beside a project you finished last year is a number that +// means nothing; the question a list of projects answers is what is still on it. +// Sub-tasks are excluded for the same reason the project view nests them — they +// are part of their parent, not another thing to do. +func (db *DB) OpenTaskCounts(ctx context.Context, groupID, userID string) (map[string]int, error) { + rows, err := db.QueryContext(ctx, ` + SELECT p.id, count(t.id) + FROM projects p + LEFT JOIN tasks t ON t.project_id = p.id + AND t.deleted_at IS NULL AND t.completed_at IS NULL AND t.parent_id IS NULL + WHERE p.group_id = ? AND p.owner_id = ? AND p.deleted_at IS NULL + GROUP BY p.id`, groupID, userID) + if err != nil { + return nil, err + } + defer rows.Close() + + counts := map[string]int{} + for rows.Next() { + var id string + var n int + if err := rows.Scan(&id, &n); err != nil { + return nil, err + } + counts[id] = n + } + return counts, rows.Err() +} diff --git a/web/e2e/smoke.spec.js b/web/e2e/smoke.spec.js index 98c50d7..c99bfb1 100644 --- a/web/e2e/smoke.spec.js +++ b/web/e2e/smoke.spec.js @@ -264,10 +264,15 @@ test('en projektgruppe kan foldes, fyldes og slettes uden at tage projekterne me await sidebar.getByLabel('Gruppens navn').fill('Arbejde'); await sidebar.getByLabel('Gruppens navn').press('Enter'); - // `exact`, because a group's name is a user-chosen word that can turn up inside - // another button's name — Playwright matches an accessible name by substring - // unless told otherwise. - const heading = sidebar.getByRole('button', { name: 'Arbejde', exact: true }); + // The name is a link to the group's own page; the chevron beside it is a + // separate button that folds. Two intentions, two targets — the single one they + // used to share could only ever serve the smaller. + // + // `exact` on the link, because a group's name is a user-chosen word that can + // turn up inside another control's name, and Playwright matches an accessible + // name by substring unless told otherwise. + const heading = sidebar.getByRole('link', { name: 'Arbejde', exact: true }); + const fold = sidebar.getByRole('button', { name: /^Fold Arbejde/ }); await expect(heading).toBeVisible(); await expect(sidebar.getByText('Træk et projekt herop')).toBeVisible(); @@ -278,7 +283,9 @@ test('en projektgruppe kan foldes, fyldes og slettes uden at tage projekterne me // the "+" icon beside "Projekter", overflowed it, and were drawn overlapping. // Both buttons still worked, so every assertion in this file passed. const boxes = await sidebar.locator('.folder-head').evaluate((el) => - [...el.querySelectorAll('button')].map((b) => { + // Links as well as buttons: the group's name is a link to its page now, and + // a measurement that skipped it would miss the widest thing in the row. + [...el.querySelectorAll('button, a')].map((b) => { const r = b.getBoundingClientRect(); return { text: b.textContent.trim(), left: r.left, right: r.right }; }) @@ -303,14 +310,14 @@ test('en projektgruppe kan foldes, fyldes og slettes uden at tage projekterne me // Folded, reloaded, still folded. localStorage would have passed the first // half of that and failed the second on another machine. - await heading.click(); - await expect(heading).toHaveAttribute('aria-expanded', 'false'); + await fold.click(); + await expect(fold).toHaveAttribute('aria-expanded', 'false'); await page.reload(); - await expect(sidebar.getByRole('button', { name: 'Arbejde', exact: true })).toHaveAttribute( + await expect(sidebar.getByRole('button', { name: /^Fold Arbejde/ })).toHaveAttribute( 'aria-expanded', 'false' ); - await sidebar.getByRole('button', { name: 'Arbejde', exact: true }).click(); + await sidebar.getByRole('button', { name: /^Fold Arbejde/ }).click(); // Colour, which is chosen while renaming rather than from the row. Both are // edits to the same thing, and a heading carrying four controls crowded the @@ -363,14 +370,14 @@ test('en projektgruppe kan foldes, fyldes og slettes uden at tage projekterne me const name = sidebar.getByLabel('Gruppens navn'); await name.fill('Kontoret'); await name.press('Enter'); - await expect(sidebar.getByRole('button', { name: 'Kontoret', exact: true })).toBeVisible(); + await expect(sidebar.getByRole('link', { name: 'Kontoret', exact: true })).toBeVisible(); // Deleting the heading must not take the project filed under it. It is // ungrouped here, which is the case that would look identical either way if // the assertion were only "the group is gone". page.once('dialog', (dialog) => dialog.accept()); await sidebar.getByRole('button', { name: 'Slet' }).click(); - await expect(sidebar.getByRole('button', { name: 'Kontoret', exact: true })).toBeHidden(); + await expect(sidebar.getByRole('link', { name: 'Kontoret', exact: true })).toBeHidden(); await expect(sidebar.getByRole('link', { name: 'Regnskab' })).toBeVisible(); expect(trouble).toEqual([]); @@ -399,6 +406,11 @@ test('en opgave kan trækkes ned i en tom sektion, og en fejlet indlæsning sige await sidebar.getByRole('button', { name: 'Nyt projekt' }).click(); await sidebar.getByLabel('Projektnavn').fill('Risteriet'); await sidebar.getByLabel('Projektnavn').press('Enter'); + // Waited for. Creating a project navigates, and `goto` is a promise nobody here + // is holding — quick add fired before it landed puts the task in the Inbox with + // no date, where the Today view is right not to show it. Intermittent, and it + // reads as the quick add having failed. + await expect(page.getByRole('heading', { name: 'Risteriet' })).toBeVisible(); await page.getByLabel('Ny opgave').fill('brænde kaffe'); await page.getByLabel('Ny opgave').press('Enter'); @@ -451,6 +463,8 @@ test('en opgave kan trækkes til en anden dag og videre til et projekt', async ( await sidebar.getByRole('button', { name: 'Nyt projekt' }).click(); await sidebar.getByLabel('Projektnavn').fill('Ærinder'); await sidebar.getByLabel('Projektnavn').press('Enter'); + // Waited for before navigating away, or the create can lose its own redirect. + await expect(sidebar.getByRole('link', { name: 'Ærinder' })).toBeVisible(); await page.goto('/upcoming'); await page.getByRole('button', { name: 'Liste', exact: true }).click(); @@ -761,6 +775,9 @@ test('en rolle kan rettes uden at fjerne personen', async ({ browser, page }) => await sidebar.getByRole('button', { name: 'Nyt projekt' }).click(); await sidebar.getByLabel('Projektnavn').fill('Fælles'); await sidebar.getByLabel('Projektnavn').press('Enter'); + // Waited for, like everywhere else here: creating a project navigates, and the + // controls below only exist once it has. + await expect(page.getByRole('heading', { name: 'Fælles' })).toBeVisible(); await page.getByRole('button', { name: 'Del' }).click(); await page.getByLabel('Inviter via e-mail').fill('andreas@example.dk'); @@ -1006,6 +1023,64 @@ test('projektsiden er brugbar på en telefon', async ({ page }) => { expect(trouble).toEqual([]); }); +/** + * A group is somewhere you can go. + * + * It was a heading over some rows: a name, a colour, and whether it was folded. + * Enough to tidy a list and not enough to be what people mean by it — "Arbejde" is + * a body of work with context of its own, and the documents that belong to all of + * it rather than to any one project inside it. + */ +test('en gruppe er en side med sine projekter, en beskrivelse og filer', async ({ page }) => { + const trouble = watchForTrouble(page); + await page.goto('/'); + + const sidebar = page.getByRole('navigation', { name: 'Hovedmenu' }); + await sidebar.getByRole('button', { name: 'Ny gruppe' }).click(); + await sidebar.getByLabel('Gruppens navn').fill('Værksted'); + await sidebar.getByLabel('Gruppens navn').press('Enter'); + await expect(sidebar.getByRole('link', { name: 'Værksted' })).toBeVisible(); + + await sidebar.getByRole('button', { name: 'Nyt projekt' }).click(); + await sidebar.getByLabel('Projektnavn').fill('Drejebænk'); + await sidebar.getByLabel('Projektnavn').press('Enter'); + await expect(page.getByRole('heading', { name: 'Drejebænk' })).toBeVisible(); + + await page.getByLabel('Ny opgave').fill('bestil stål'); + await page.getByLabel('Ny opgave').press('Enter'); + await expect(page.getByText('bestil stål', { exact: true })).toBeVisible(); + + // Into the group, then onto its page — which is what the heading is a link to + // now. The chevron beside it still folds; they are two intentions and the old + // single target could only serve the smaller one. + await sidebar + .getByRole('link', { name: 'Drejebænk' }) + .dragTo(sidebar.getByRole('link', { name: 'Værksted' })); + await sidebar.getByRole('link', { name: 'Værksted' }).click(); + + await expect(page.getByRole('heading', { name: 'Værksted', level: 1 })).toBeVisible(); + const row = page.locator('.projects a').filter({ hasText: 'Drejebænk' }); + await expect(row).toBeVisible(); + await expect(row.getByText('1', { exact: true }), 'antallet af åbne opgaver').toBeVisible(); + + // The description, which is the part a heading cannot carry. Click to write, + // blur to save — the same shape a task's description has. + await page.getByRole('button', { name: /Skriv hvad det her er/ }).click(); + const about = page.getByLabel('Om gruppen'); + await expect(about).toBeVisible(); + await about.fill('Alt der larmer og støver.'); + await about.blur(); + await expect(page.getByText('Alt der larmer og støver.')).toBeVisible(); + + // And it survives a reload, which is the difference between the page keeping up + // and the server taking it. + await page.reload(); + await expect(page.getByText('Alt der larmer og støver.')).toBeVisible(); + await expect(page.locator('.projects a').filter({ hasText: 'Drejebænk' })).toBeVisible(); + + expect(trouble).toEqual([]); +}); + /** * The sidebar never scrolls sideways. * diff --git a/web/src/lib/api.js b/web/src/lib/api.js index 22b344d..882976c 100644 --- a/web/src/lib/api.js +++ b/web/src/lib/api.js @@ -225,7 +225,9 @@ export const api = { listProjectGroups: () => get('/project-groups'), createProjectGroup: (name) => post('/project-groups', { name }), + getProjectGroup: (id) => get(`/project-groups/${id}`), updateProjectGroup: (id, data) => patch(`/project-groups/${id}`, data), + uploadGroupAttachment: (id, file) => upload(`/project-groups/${id}/attachments`, file), deleteProjectGroup: (id) => del(`/project-groups/${id}`), reorderProjectGroups: (ids) => post('/project-groups/reorder', { ids }), diff --git a/web/src/lib/components/Sidebar.svelte b/web/src/lib/components/Sidebar.svelte index 0a14784..af763d1 100644 --- a/web/src/lib/components/Sidebar.svelte +++ b/web/src/lib/components/Sidebar.svelte @@ -590,27 +590,33 @@ the heading away from a screen reader. The whole heading folds — at this size a lone chevron is a target you miss, and the name is where the eye already is. --> + +