diff --git a/docs/draft/editable-workflow-lanes-analysis.md b/docs/draft/editable-workflow-lanes-analysis.md new file mode 100644 index 0000000..08d9e83 --- /dev/null +++ b/docs/draft/editable-workflow-lanes-analysis.md @@ -0,0 +1,647 @@ +# Editable Workflow Lanes — Codebase Analysis & Rollout Plan + +**Date:** 2026-03-31 +**Branch:** Prefer a **dedicated feature branch** for this work (not necessarily `feature/mcp-adapter`). Practical merge flow: after **Phase 1** lands, merge it to `main`, then merge/rebase `main` into `feature/mcp-adapter` to reduce conflict surface. Normal conflicts may still occur in shared files (`routing.go`, store, board/settings frontend); that is merge management, not a reason to avoid the feature. +**Scope:** Post-creation editing of project workflow lanes. **Near-term rollout:** rename display label only, then dashboard hardening, then add lane, then delete empty non-done lane. **Deferred:** changing which lane is `is_done` (see §3). + +--- + +## Table of Contents + +1. [Findings](#1-findings) + - 1.1 [Current Workflow Model](#11-current-workflow-model) + - 1.2 [Todo-to-Lane Relationship](#12-todo-to-lane-relationship) + - 1.3 [Done Semantics](#13-done-semantics) + - 1.4 [Stats and Charts Impact](#14-stats-and-charts-impact) + - 1.5 [Frontend / UX Impact](#15-frontend--ux-impact) + - 1.6 [Import/Export and API Impact](#16-importexport-and-api-impact) + - 1.7 [Permissions and Validation](#17-permissions-and-validation) + - 1.8 [Testing Impact](#18-testing-impact) +2. [Risk Matrix](#2-risk-matrix) +3. [Recommended Rollout Phases](#3-recommended-rollout-phases) +4. [Dashboard Hardening and Optional Refactors](#4-dashboard-hardening-and-optional-refactors) +5. [Concrete Implementation Checklist](#5-concrete-implementation-checklist) + +--- + +## 1. Findings + +### 1.1 Current Workflow Model + +#### Storage + +Workflows are stored as rows in the `project_workflow_columns` table (migration `036_add_project_workflow_columns.sql`): + +```sql +CREATE TABLE project_workflow_columns ( + id INTEGER PRIMARY KEY, + project_id INTEGER NOT NULL REFERENCES projects(id), + key TEXT NOT NULL, + name TEXT NOT NULL, + position INTEGER NOT NULL, + is_done INTEGER NOT NULL, + UNIQUE(project_id, key) +); +``` + +Additional columns added by later migrations: +- `system INTEGER NOT NULL DEFAULT 0` — migration `039_add_workflow_system_flag.sql` +- `color TEXT NOT NULL DEFAULT '#64748b'` — migration `040_add_workflow_column_color.sql` + +#### Go struct + +`internal/store/types.go` lines 90–99: + +```go +type WorkflowColumn struct { + ID int64 + ProjectID int64 + Key string + Name string + Color string + Position int + IsDone bool + System bool +} +``` + +#### Lane identity model + +| Mechanism | Role | +|-----------|------| +| `id` (integer PK) | Stable DB primary key; not used in any cross-table joins | +| **`key`** (string, per-project unique) | **Logical identity**. Used by `todos.column_key`, all API paths, drag-drop, MCP, import/export. Constrained by `UNIQUE(project_id, key)` | +| `name` (string) | **Display label only**. Rendered in board headers, API JSON, export. Not referenced by todo association | +| `position` (integer) | **Ordering** for board display. Set from array index on insert; `ORDER BY position ASC, id ASC` | + +**Key validation** — `internal/store/workflows.go` lines 22–28: regex `^[a-z0-9](?:[a-z0-9_]*[a-z0-9])?$`, max length 32 chars (`maxSlugLen` from `internal/store/slug.go`). + +**Verdict: Lane identity is key-based, not name-based.** Renaming the display `name` does not affect todo association or any functional cross-reference. This is the single most important architectural fact for safe editability. + +#### Default lanes + +`internal/store/workflows.go` `defaultWorkflowColumns()` lines 42–50: + +| Key | Name | Color | IsDone | System | +|-----|------|-------|--------|--------| +| `backlog` | Backlog | `#9CA3AF` | false | true | +| `not_started` | Not Started | `#F59E0B` | false | true | +| `doing` | In Progress | `#10B981` | false | true | +| `testing` | Testing | `#3B82F6` | false | true | +| `done` | Done | `#EF4444` | true | true | + +#### Creation invariants + +`insertWorkflowColumnsExec` — `internal/store/workflows.go` lines 100–145: +- Minimum 2 columns +- Non-empty trimmed `name` +- Valid `key` (regex + ≤ 32 chars) +- Valid hex `color` (`#RRGGBB`), defaults to `#64748b` +- No duplicate keys +- **Exactly one** `IsDone == true` column +- `System` forced to `false` for user-supplied workflows +- `Position` forced to slice index (user-supplied position ignored) + +`validateExactlyOneDoneColumn` — lines 226–236: post-insert sanity check. + +#### No post-creation edit API exists today + +There is **no HTTP handler** for updating workflow columns on an existing project. The only paths that modify workflow post-creation are: +- Backup import/merge (`internal/store/backup.go`): deletes + re-inserts workflow columns +- Store-level `InsertWorkflowColumns` used in tests + +--- + +### 1.2 Todo-to-Lane Relationship + +#### How todos reference lanes + +`todos.column_key` (TEXT NOT NULL) stores the lane **key** string. Established in migration `038_drop_todos_status.sql`. + +`Todo` struct — `internal/store/types.go` lines 209–226: +```go +type Todo struct { + // ... + ColumnKey string + // ... + DoneAt *time.Time +} +``` + +**Validation on write**: `validateProjectColumnKeyQueryer` (`internal/store/workflows.go` lines 204–219) confirms the key exists in `project_workflow_columns` for the project. Used by: +- `CreateTodo` — `internal/store/todos.go` lines 181–185 +- `MoveTodo` — `internal/store/todos.go` lines 836–843 + +#### Legacy status + +The integer `todos.status` column was **removed** in migration `038_drop_todos_status.sql`. The Go `Status` type (lines 26–88 of `types.go`) is **deprecated** and only kept for import/export compatibility mapping. + +API layer still accepts `status` / `toStatus` as aliases, normalized to `column_key` via `normalizeLaneKey` (`internal/httpapi/routing.go` lines 2444–2458), which maps legacy uppercase enums (e.g., `"IN_PROGRESS"` → `"doing"`) and lowercases unknown values. + +Frontend also has compatibility: `todo.columnKey || todo.status` in `internal/httpapi/web/modules/dialogs/todo.ts` line 1031. + +#### Will renaming a lane label preserve todo association? + +**Yes — proven by code inspection.** Todos store `column_key` which matches `WorkflowColumn.Key`, not `Name`. All validation and querying joins on `key`. Changing only `name` in `project_workflow_columns` leaves all todo associations intact. + +#### Back-compat risks + +1. **Backup merge refuses workflow replacement when orphaned todos exist** — `internal/store/backup.go` ~lines 750–806: if new workflow lacks a key that existing todos reference, import is rejected ("stranded"). +2. **`normalizeLaneKey` maps only the five default uppercase keys** — custom lane keys pass through as lowercase. This is already correct for custom workflows. +3. **`resolveImportColumnKey`** (`internal/store/backup_bulk.go` lines 53–68) handles legacy enum OR direct column_key. + +--- + +### 1.3 Done Semantics + +#### Explicit `is_done` flag — the primary mechanism + +The `project_workflow_columns.is_done` column is the **source of truth**. It is enforced to be exactly one per project. + +Key functions: +- `GetWorkflowDoneColumnKey` — `internal/store/workflows.go` lines 182–194: `SELECT key WHERE is_done = 1` +- `resolveDoneAtForColumnTransition` — `internal/store/todos.go` lines 310–316: sets `done_at` timestamp when a todo enters the `is_done` column from a non-done column; **never clears `done_at` on reopen** +- `validateExactlyOneDoneColumn` — `internal/store/workflows.go` lines 226–236 + +#### All locations where "done" is determined + +| Location | File | Mechanism | Notes | +|----------|------|-----------|-------| +| Burndown `isIncompleteAtDayEnd` | `internal/store/burndown.go:51–61` | Compares `t.columnKey != doneColumnKey` (from `GetWorkflowDoneColumnKey`) | **Correct**: uses `is_done` flag dynamically | +| `GetBacklogSize` | `internal/store/burndown.go:169–281` | Calls `GetWorkflowDoneColumnKey` | **Correct** | +| `GetRealBurndown` | `internal/store/burndown.go:289–388` | Calls `GetWorkflowDoneColumnKey` | **Correct** | +| `GetRealBurndownForSprint` | `internal/store/burndown.go:394–477` | Calls `GetWorkflowDoneColumnKey` | **Correct** | +| Todo creation `done_at` | `internal/store/todos.go:216` | Via `resolveDoneAtForColumnTransition` using `WorkflowColumn.IsDone` | **Correct** | +| Todo move `done_at` | `internal/store/todos.go:849–858` | `targetCol.IsDone` / `currentCol.IsDone` | **Correct** | +| **Dashboard: sprint completion** | `internal/store/dashboard.go:323` | **`t.column_key = 'done'`** (string literal) | **HARDCODED — BUG for custom done key** | +| **Dashboard: weekly throughput** | `internal/store/dashboard.go:388` | **`column_key = 'done'`** (string literal) | **HARDCODED** | +| **Dashboard: avg lead time (with sprint)** | `internal/store/dashboard.go:428` | **`t.column_key = 'done'`** (string literal) | **HARDCODED** | +| **Dashboard: avg lead time (fallback)** | `internal/store/dashboard.go:443` | **`column_key = 'done'`** (string literal) | **HARDCODED** | +| **Dashboard: oldest WIP** | `internal/store/dashboard.go:462` | **`column_key IN ('doing', 'testing')`** (string literals) | **HARDCODED to default keys** | +| **Dashboard: assigned filter** | `internal/store/dashboard.go:124,191,205,250,299` | `column_key != ?` with `DefaultColumnDone` constant | Uses constant, but still assumes key is `"done"` | +| Board JSON `isDone` | `internal/httpapi/json.go:504,521` | `wc.IsDone` from struct | **Correct** | +| Frontend `getBoardColumns` | `web/modules/views/board.ts:94–99` | Uses `columnOrder[].isDone` from API | **Correct** when `columnOrder` present | +| Frontend `resolveColumnKey` | `web/modules/dialogs/todo.ts:51–62` | Maps legacy uppercase to lowercase | **Correct** | + +#### Critical hidden assumption — DASHBOARD IS BROKEN FOR CUSTOM WORKFLOWS + +The dashboard (`internal/store/dashboard.go`) has **10+ SQL queries that hardcode `'done'` as the done column key** rather than using `GetWorkflowDoneColumnKey`. This means: +- Projects with custom workflows where the done column key is not `"done"` will have **zero throughput, zero completion, incorrect lead time, and missing WIP** on the personal dashboard. +- This is a **pre-existing correctness bug** independent of rename-label-only (which does not change keys, `is_done`, todo association, or stats query semantics). It matters most for custom done/WIP keys and would matter acutely for any future done-lane reassignment without a deliberate historical-data strategy. + +The burndown system (`internal/store/burndown.go`) is **correctly implemented** — it dynamically resolves the done column via `GetWorkflowDoneColumnKey`. + +--- + +### 1.4 Stats and Charts Impact + +#### Code that powers stats/charts + +| Component | Files | Historical? | +|-----------|-------|-------------| +| Burndown chart | `internal/store/burndown.go` | Yes — reconstructs day-by-day from `created_at`, `column_key`, `done_at` | +| Backlog size chart | `internal/store/burndown.go` | Yes — same replay | +| Dashboard throughput | `internal/store/dashboard.go:382–392` | Yes — buckets by `done_at` | +| Dashboard lead time | `internal/store/dashboard.go:414–449` | Yes — `done_at - created_at` | +| Dashboard sprint completion | `internal/store/dashboard.go:310–351` | Mixed — `done_at` within sprint window | +| Dashboard WIP | `internal/store/dashboard.go:354–363` | Current state | +| Board lane counts | `internal/store/board.go` | Current state | +| Tag counts | `internal/store/tags.go` | Current state | + +#### Impact by operation + +| Operation | Burndown/Backlog | Dashboard Throughput/Lead/Completion | Dashboard WIP | Board Counts | Safe? | +|-----------|-----------------|-------------------------------------|---------------|-------------|-------| +| **Rename lane** (change `name` only, keep `key`) | No impact | No impact | No impact | No impact (keys unchanged) | **YES — safe** | +| **Add non-done lane** | No impact (new key is not the done key) | No impact (queries filter on done key) | No impact (queries filter on specific keys) | New lane appears automatically | **YES — safe** | +| **Delete non-done lane** (no todos in it) | No impact | No impact | WIP count may drop if deleting `doing`/`testing` (dashboard hardcode) | Lane disappears | **YES — safe if empty** | +| **Delete non-done lane** (has todos) | **UNSAFE**: todos in that lane are orphaned; burndown replay may miscategorize | **UNSAFE**: same | **UNSAFE**: same | **UNSAFE**: same | **NO — must relocate todos first** | +| **Change done lane** (move `is_done` to different column) | **MIXED**: burndown uses `GetWorkflowDoneColumnKey` (reads new done key) but `done_at` timestamps are historical — previously completed todos retain old `done_at` and old `column_key`, so burndown replay sees them as "completed in old done lane" which now has a different key | **BROKEN**: dashboard hardcodes `'done'` — if new done key differs from `"done"`, dashboard stops working for the project | Same | Counts shift | **HIGH RISK** | + +#### Historical data problem with "change done lane" + +Burndown reconstructs history by checking `t.columnKey != doneColumnKey` for each day. If the done column key changes from `"done"` to `"shipped"`: +- Todos completed under the old regime have `column_key = "done"` and `done_at` set +- The new `doneColumnKey` is `"shipped"` +- `isIncompleteAtDayEnd` will see those old todos as **incomplete** (wrong key), despite `done_at` being set +- This **rewrites historical chart meaning**: old completed work appears as still open + +**No event-sourcing or lane transition history exists** — burndown relies only on current `column_key` + timestamps, not on historical transition log. The `audit_events` table logs `todo_moved` events but burndown does not read it. + +--- + +### 1.5 Frontend / UX Impact + +#### Board rendering + +`internal/httpapi/web/modules/views/board.ts`: +- `getBoardColumns(board)` (lines 94–99): uses `board.columnOrder` from API when present; falls back to hardcoded five columns from `columnsSpec()` +- `renderBoardFromData` (lines 1308–1502) and `updateBoardContent` (lines 1159–1306): render one `
` per column from `getBoardColumns()` +- Column headers use `c.key` for CSS class, `c.color` for border, `c.name` for title +- Card lists use `data-status="${c.key}"` which is the drop target identifier for drag-drop + +**Verdict**: Board rendering is **fully dynamic** when `columnOrder` is populated. No hardcoded column count or fixed column assumptions in the render path. + +#### Drag and drop + +`internal/httpapi/web/modules/features/drag-drop.ts`: +- **SortableJS** library +- `setDnDColumns(columns)` (line 29): receives columns from board; falls back to `columnsSpec()` (hardcoded five lanes, line 19–26) when empty +- `initDnD()` (lines 111–224): creates Sortable instance per lane via `document.getElementById(\`list_${c.key}\`)` +- Drop target: `list.getAttribute("data-status")` → sent as `toStatus` in API call (line 136) +- `LANE_CARD_CLASSES` (line 33): hardcoded list `['card--backlog', 'card--not_started', 'card--in_progress', 'card--testing', 'card--done']` — stripped before applying new color; custom keys use `card--${targetKey.toLowerCase()}` + +**Risk**: The `columnsSpec()` fallback and `LANE_CARD_CLASSES` array assume exactly five default lanes. If `columnOrder` is empty/missing for a board with custom columns, DnD breaks. + +#### Hardcoded fallbacks in state management + +`internal/httpapi/web/modules/state/state.ts` line 72: +```typescript +boardLaneMeta: { + BACKLOG: { hasMore: false, nextCursor: null, loading: false }, + NOT_STARTED: { ... }, IN_PROGRESS: { ... }, TESTING: { ... }, DONE: { ... } +} +``` + +`internal/httpapi/web/modules/state/selectors.ts` lines 131–138: +```typescript +getBoardLaneMeta() fallback uses same five hardcoded keys +``` + +These fallbacks are **only used before board data loads** or when `boardLaneMeta` is null. Once `buildLaneMetaFromBoard` (board.ts lines 113–140) runs, it dynamically builds meta from the board's actual columns. **Low risk** — the fallback is effectively dead code once the board loads. + +#### SSE refresh + +`internal/httpapi/web/modules/views/board.ts`: +- `connectBoardEvents(slug)` (lines 335–397): `EventSource` on `/api/board/${slug}/events` +- On message: calls `refetchBoardFromRealtime` → `invalidateBoard` → `loadBoardBySlug` → full board re-fetch including `columnOrder` +- **No separate workflow refresh needed** — workflow metadata comes embedded in the board GET response + +#### Caching + +- Board prefetch cache in `internal/httpapi/web/modules/views/projects.ts` (lines 25–27, 529–552): `resolvedBoardBySlug` Map. After a workflow edit, this would serve stale data until next navigation or invalidation. +- `lastUpdateBoardContent*` skip-optimization in `board.ts` (lines 195–198): based on same board object reference — a fresh fetch bypasses this correctly. + +**Required**: After a workflow edit from settings, drive the same invalidation/refetch paths the board already uses (e.g. clear `resolvedBoardBySlug`, call `invalidateBoard` / `loadBoardBySlug`) so headers and `columnOrder` update; add a new SSE event type only if existing realtime events cannot accomplish that. + +--- + +### 1.6 Import/Export and API Impact + +#### Export gap (pre-existing) + +The HTTP export (`GET /api/backup/export`) uses `exportDataToJSON` (`internal/httpapi/json.go` lines 647+) which maps to `projectExportJSON` — this struct **omits `workflowColumns`, `sprints`, and `doneAt`** from the export. So the public export is a subset of what `ExportAllProjects` builds in memory. + +**Impact**: Editing workflows does not break export because workflow data is already absent from the public export JSON. However, the store-level `ExportData.ProjectExport` **does** include `WorkflowColumns` (populated when workflow differs from defaults — `backup.go` line 351–363). + +#### Import + +Import paths (`internal/store/backup.go`): +- `validateImportPreflight` (lines 608+): validates workflow keys, names, colors, exactly-one-done, todo-status-resolves-in-workflow +- **Merge import**: refuses workflow replacement when existing todos reference keys not in the new workflow ("stranded" guard, ~lines 750–806) +- `insertWorkflowColumnsExec` is called after `deleteProjectWorkflowColumnsExec` during replacement + +**Impact**: Import already handles custom workflows correctly. Editable lanes introduce no new import risk. + +#### API endpoints needing changes + +Currently **no workflow update endpoint exists**. Required new endpoints: + +| Operation | Proposed endpoint | Notes | +|-----------|------------------|-------| +| Rename lane (label only) | `PATCH /api/board/{slug}/workflow/{key}` | Body: `{ "name": "..." }`; **lane `key` immutable**. | +| Add lane | `POST /api/board/{slug}/workflow` | Body: `{ "key": "...", "name": "...", "color": "...", "position": N }` | +| Delete lane | `DELETE /api/board/{slug}/workflow/{key}` | Precondition: no todos in lane | +| Reorder lanes | *(optional / future)* | e.g. `PUT .../workflow/order` — **not part of the core incremental rollout** unless existing UI requires it. | +| Change done lane | *(deferred)* | Not in near-term scope; see §3. | + +#### MCP exposure + +**MCP is out of scope for this workflow-editing rollout** — do not design the HTTP/store API around future MCP tools. No `workflows.*` tools exist today (`internal/mcp/registry.go`); `board.get` is planned only (`internal/mcp/adapter.go`). Todo tools still use `columnKey` / `toColumnKey`. A future `workflow.*` surface can be a separate product decision. + +--- + +### 1.7 Permissions and Validation + +#### Current permission model + +Project roles (`internal/store/types.go` lines 280–361): +- **Maintainer**: full project control (create/delete/move todos, settings, export) +- **Contributor**: body edits on assigned todos only +- **Viewer**: read-only + +Todo permissions (`internal/store/permissions.go` lines 12–41): +- Create/Delete/Move: Maintainer only +- Edit: Maintainer = full; Contributor = body only if assigned + +**Recommendation**: Workflow editing should require **Maintainer** role, consistent with other structural project changes (board settings, sprints, members). No new permission type needed. + +#### Required server-side validations for each operation + +**Rename (label only — first increment)**: +- Caller must be project Maintainer +- New `name` must be non-empty, trimmed, ≤ 200 chars +- Key remains unchanged (reject key/`isDone`/`color` in body for this narrow endpoint, or ignore non-name fields) +- **Lane color**: optional / deferred to a later increment if not trivially isolated on the same PATCH. + +**Add**: +- Caller must be project Maintainer +- New `key` must be valid (regex, ≤ 32 chars, unique in project) +- New `name` must be non-empty +- New lane must NOT be `is_done` (done-lane reassignment is deferred; see §3) +- `position` must be valid (0..len); reassign positions for all lanes +- Project must not exceed a reasonable lane limit (recommend max 12) + +**Delete**: +- Caller must be project Maintainer +- Lane must NOT be `is_done` +- Lane must NOT contain any todos (`SELECT COUNT(*) FROM todos WHERE project_id = ? AND column_key = ?`) +- Project must retain ≥ 2 lanes after deletion +- Reassign positions for remaining lanes + +**Change done** *(deferred — not shipping in this incremental rollout)*: If revisited, requires Maintainer, atomic `is_done` swap, `validateExactlyOneDoneColumn`, and a **deliberate historical-data strategy** for burndown/dashboard (`done_at`, `column_key`, replay semantics). Do not ship on “warn only” without that design. + +#### Guardrails for first rollout + +1. Block delete when any todos exist in the lane (no bulk-move yet) +2. Do **not** expose done-lane reassignment in the near-term phases below +3. Enforce `Maintainer` role on all workflow edit endpoints +4. Require minimum 2 lanes invariant on delete +5. Prefer **existing** board refresh paths: after workflow mutations, trigger the same invalidation/refetch the app already uses for board updates (e.g. `invalidateBoard` / `loadBoardBySlug`, clear `resolvedBoardBySlug` where needed). Add a **dedicated** SSE event type only if existing events cannot drive a metadata refresh reliably. + +--- + +### 1.8 Testing Impact + +#### Existing tests relevant to workflows/todos/stats + +| Area | File | Key tests | +|------|------|-----------| +| Workflow creation/import | `internal/store/backup_integrity_test.go` | `TestExportImportRoundTrip_WorkflowColumns`, `TestImportMerge_ExistingCustomWorkflow`, `TestImportMerge_WorkflowReplacementRejectedWhenStrandedTodos` | +| Todo move + done_at | `internal/store/done_at_test.go` | `TestDoneAt_MoveTodo_IntoDONE`, `TestDoneAt_MoveTodo_ReopenPreserves` | +| Todo ordering across lanes | `internal/store/ordering_test.go` | `TestMoveTodo_MoveAcrossColumns`, `TestMoveTodo_ReorderWithinColumn` | +| Board paging + lanes | `internal/store/board_test.go` | `TestGetBoardPaged_ReturnsColumnsMeta`, `TestListTodosForBoardLane_Pagination` | +| Board API shape | `internal/httpapi/server_test.go` | `TestAPI_BoardPagedAndLaneEndpoint`, `TestAPI_CreateMoveAndFetchBoard` | +| Burndown | `internal/store/status_test.go` | `TestBurndown_TestingIsIncomplete`, `TestBurndown_TestingCompletedAfterDay` | +| Permissions | `internal/store/todos_assignee_test.go` | Full permission matrix for create/update/move/delete | +| MCP | `internal/mcp/adapter_test.go` | `TestMCPTodosCreate*`, `TestMCPTodosMoveSuccess*` | +| Import auth | `internal/store/backup_auth_test.go` | `TestBackupAuth_MaintainerCanExport` | + +**No frontend tests exist** (no `.test.ts`, `.spec.ts`, or Playwright/Cypress files). + +#### Minimum new test coverage needed + +| Phase | Tests needed | +|-------|-------------| +| **Phase 1 (rename label)** | `TestRenameLane_UpdatesNameOnly`, `TestRenameLane_PreservesTodosColumnKey`, `TestRenameLane_RequiresMaintainer`, `TestRenameLane_EmptyNameRejected`, `TestRenameLane_BoardRefreshShowsNewName`, `TestRenameLane_BurndownUnaffected` | +| **Phase 2 (dashboard hardening)** | `TestDashboardSummary_CustomDoneKey`, `TestDashboardSummary_CustomWIPKeys` (and fix queries until they pass) | +| **Phase 3 (add lane)** | `TestAddLane_ValidKey`, `TestAddLane_DuplicateKeyRejected`, `TestAddLane_InsertsBeforeDone`, `TestAddLane_BoardRendersNewLane`, `TestAddLane_StatsUnaffected`, `TestAddLane_MaxLanesEnforced` | +| **Phase 4 (delete)** | `TestDeleteLane_EmptyLaneRemoved`, `TestDeleteLane_NonEmptyLaneRejected`, `TestDeleteLane_DoneLaneRejected`, `TestDeleteLane_MinLanesEnforced`, `TestDeleteLane_BoardRefreshRemovesLane`, `TestDeleteLane_StatsUnaffected` | +| **Deferred (done-lane reassignment)** | When a historical-data strategy exists: `TestChangeDone_*` and burndown/dashboard regression tests (see §3 deferred) | + +--- + +## 2. Risk Matrix + +| Risk | Severity | Likelihood | Affected phases | Mitigation | +|------|----------|------------|-----------------|------------| +| **Dashboard hardcodes `'done'` key** — pre-existing bug; custom-workflow projects can have wrong dashboard metrics | HIGH | CERTAIN (for any project not using default `done` key) | Phase 2 (fix); *not* a release-blocker for Phase 1 rename-label | Fix early in **Phase 2** (ideally right after Phase 1 or in parallel): replace hardcoded `'done'` with per-project done key; parameterize dashboard SQL | +| **Dashboard hardcodes `'doing'`/`'testing'` for WIP** — oldest WIP and WIP count assume these keys | MEDIUM | CERTAIN (for any project without those keys) | Phase 2 | Same Phase 2 hardening: parameterize WIP / aging-WIP | +| **Burndown rewrites history on done-lane change** — completed todos under old done key appear as "incomplete" | HIGH | If done-lane reassignment ships without design | Deferred | Options (a)–(c) in §3 remain documented; **do not adopt any without a deliberate historical-data strategy** | +| **Frontend fallback columns are hardcoded** — `columnsSpec()`, `LANE_CARD_CLASSES`, initial `boardLaneMeta` | LOW | Low (only triggered when `columnOrder` is empty) | All phases | Clean up fallbacks or ensure `columnOrder` is always populated | +| **Board prefetch cache serves stale workflow** — `resolvedBoardBySlug` not invalidated on edit | MEDIUM | Moderate | Phase 1+ | Clear prefetch cache on workflow edit or navigation to settings | +| **Export JSON omits workflow columns** — public export (`projectExportJSON`) has no `workflowColumns` | LOW | Low (store-level export includes them) | Phase 1+ | Consider adding to public export; not blocking | +| **`done_at` never clears on reopen** — moving a todo out of done does not reset `done_at` | LOW | By design (documented) | Deferred / future done-lane work | Revisit only if done-lane edits are designed | +| **MCP has no workflow tools** | LOW | N/A for this rollout | *Out of scope* | MCP exposure does not gate this feature | +| **No frontend tests** — cannot automatically verify board rendering after changes | MEDIUM | Moderate | All phases | Manual QA + consider adding Playwright smoke tests | + +--- + +## 3. Recommended Rollout Phases + +**Order:** Phase 1 → Phase 2 → Phase 3 → Phase 4. Rename-label-only is first and narrow. Dashboard hardening is **not** a release-blocker for Phase 1 (see §1.3–1.4): it fixes a **pre-existing** bug; shipping rename does not change keys, `is_done`, todo association, or stats query semantics. Land Phase 2 **immediately after** Phase 1 or **in parallel**; do not hold Phase 1 on it. + +### Phase 1: Rename display label only + +**Goal**: Rename a lane's **display `name` only**. **Lane `key` stays immutable.** **Lane color is optional / deferred** (follow-up) unless trivially isolated on the same endpoint. + +**Scope**: +- `PATCH /api/board/{slug}/workflow/{key}` with body `{ "name": "string" }` +- **Strict contract**: reject unknown or mutating fields (`key`, `isDone`, `color`, `position`, etc.) with `400` +- Server updates `project_workflow_columns.name` only WHERE `project_id = ? AND key = ?` +- Refresh: prefer **existing** `invalidateBoard` / `loadBoardBySlug` and clearing `resolvedBoardBySlug`; use existing SSE/realtime that already triggers board refetch. Add a **new** event type only if that is insufficient. + +**Explicit invariant**: key remains immutable in both API and UI; no hidden slug/key regeneration from name changes. + +**Required code changes**: + +| Layer | File | Change | +|-------|------|--------| +| Store | `internal/store/workflows.go` | e.g. `UpdateWorkflowColumnName(ctx, projectID int64, key, newName string) error` | +| HTTP | `internal/httpapi/routing.go` | `PATCH .../workflow/{key}` + maintainer check | +| Frontend | `web/modules/dialogs/settings.ts` | Workflow UI: label rename | +| Frontend | `web/modules/views/board.ts`, `projects.ts` | Wire success to existing board refresh / prefetch invalidation | + +**Risks**: Minimal. Label-only; keys and `is_done` unchanged. + +**Test plan**: +- `TestRenameLane_UpdatesNamePreservesKey` (store test) +- `TestRenameLane_RequiresMaintainer` (HTTP integration test) +- `TestRenameLane_NonexistentKeyReturns404` (HTTP integration test) +- `TestRenameLane_BoardAPIReflectsNewName` (HTTP integration test) +- `TestRenameLane_BurndownUnaffected` (store test) +- Manual QA: label, header, DnD, todo dialog + +**Stats/charts impacted**: No. + +--- + +### Phase 2: Dashboard hardening (custom done / WIP) + +**Goal**: Fix **pre-existing** dashboard hardcoding (`'done'`, `'doing'`, `'testing'`). Same technical work as the old "Phase 0"; **not** blocking Phase 1. + +**Scope**: Refactor `GetDashboardSummary` (`internal/store/dashboard.go`): per-project done key, parameterized WIP; batch helpers in `internal/store/workflows.go` (JOIN or `map[projectID]doneKey`). Details: ~12 SQL sites (lines 124, 191, 205, 250, 255, 274, 283, 298, 323, 361, 388, 428, 443, 462). + +**Risks**: Low — internal SQL only. + +**Test plan**: `TestDashboardSummary_CustomDoneKey`, `TestDashboardSummary_CustomWIPKeys`; existing `TestBurndown_*`. + +**Stats/charts impacted**: Yes — dashboard correctness. + +--- + +### Phase 3: Add lane + +**Goal**: Allow adding a new non-done lane with **insert-before-done** behavior for v1. **Dedicated lane reorder** (e.g. `PUT .../workflow/order`) remains *optional / future*. + +**Scope**: +- `POST /api/board/{slug}/workflow` +- Body: `{ "key": "string", "name": "string", "color": "#hexhex" }` +- Server inserts new row immediately before the current done lane, then normalizes positions +- After mutation: prefer **existing** board invalidation / refetch (same as Phase 1) +- Frontend: "Add lane"; key from `keyFromLaneName` / `makeUniqueLaneKey` (`projects.ts` lines 56–82) + +**Required code changes**: + +| Layer | File | Change | +|-------|------|--------| +| Store | `internal/store/workflows.go` | New `AddWorkflowColumn(ctx, projectID int64, col WorkflowColumn) error` with key validation, uniqueness, max-lane-count, and insert-before-done placement | +| HTTP | `internal/httpapi/routing.go` | New handler: `POST .../workflow` with maintainer check | +| Frontend | `web/modules/dialogs/settings.ts` | "Add lane" button → key derived from name; no position selector in v1 | + +**Constraints**: +- New lane `is_done` must be `false` +- Max lane count: 12 (prevent abuse) +- Key auto-generation: reuse `keyFromLaneName` / `makeUniqueLaneKey` pattern from `projects.ts` on server side +- Placement in v1: always insert immediately before the done lane; no arbitrary `position` input + +**Risks**: Low. New lane has no todos, so stats/board/DnD pick it up automatically. + +**Test plan**: +- `TestAddLane_InsertsBeforeDone` (store test) +- `TestAddLane_DuplicateKeyRejected` (store test) +- `TestAddLane_InvalidKeyRejected` (store test) +- `TestAddLane_MaxLanesEnforced` (store test) +- `TestAddLane_IsDoneFalseEnforced` (store test: reject `isDone: true`) +- `TestAddLane_BoardShowsNewLane` (HTTP integration test) +- `TestAddLane_DnDWorksWithNewLane` (manual QA: add lane, drag todo into it) +- `TestAddLane_BurndownUnchanged` (store test) + +**Stats/charts impacted**: No (empty lane, not done-flagged). + +--- + +### Phase 4: Delete empty non-done lane + +**Goal**: Delete a lane with **no todos** that is **not** the done lane; retain ≥ 2 lanes. + +**Scope**: +- `DELETE /api/board/{slug}/workflow/{key}` with validations above +- Reassign positions; prefer **existing** board refresh plumbing after success +- Frontend: "Delete" per lane (disabled for done lane / non-empty lanes) + +**Required code changes**: + +| Layer | File | Change | +|-------|------|--------| +| Store | `internal/store/workflows.go` | New `DeleteWorkflowColumn(ctx, projectID int64, key string) error` with todo-count check, is-done check, min-lanes check | +| HTTP | `internal/httpapi/routing.go` | New handler: `DELETE .../workflow/{key}` with maintainer check | +| Frontend | `web/modules/dialogs/settings.ts` | Delete button per lane; disabled when lane has todos or is done; confirmation dialog | + +**Risks**: +- **Dashboard WIP**: if Phase 2 is not shipped, deleting `"doing"`/`"testing"` can still yield wrong WIP — prefer **Phase 2 before or with** Phase 4, or accept edge cases until then. +- **Board CSS**: `card--doing` / `card--testing` unused — harmless. + +**Test plan**: +- `TestDeleteLane_EmptyNonDoneLaneRemoved` (store test) +- `TestDeleteLane_NonEmptyLaneRejected` (store test) +- `TestDeleteLane_DoneLaneRejected` (store test) +- `TestDeleteLane_LastTwoLanesCannotDelete` (store test: must retain ≥ 2) +- `TestDeleteLane_PositionsReindexed` (store test) +- `TestDeleteLane_BoardNoLongerShowsLane` (HTTP integration test) +- `TestDeleteLane_ImportWithDeletedKeyStillWorks` (import test: export from old state, delete lane, import should not choke if no todos reference deleted key) +- Manual QA: delete a lane, verify board layout, verify DnD, verify stats unchanged + +**Stats/charts impacted**: No for lane data itself (lane is empty), but custom-workflow dashboard correctness still depends on **Phase 2**. Do not interpret Phase 4 as independently safe to ship before Phase 2 in custom-workflow environments. + +--- + +### Deferred: Change done designation (not near-term) + +**Do not ship** reassignment of which lane has `is_done` in this incremental rollout. Risks: §1.3–1.4, burndown historical replay vs `column_key`/`done_at`. + +If revisited (not scheduled): atomic `is_done` swap, `validateExactlyOneDoneColumn`, todo `done_at`/`column_key` rules, board refresh via **existing** plumbing unless a new event is truly required. + +**Historical data — documented options (none without strategy)**: + +1. **Option A:** Warn + accept semantic break — **insufficient alone** to justify shipping. +2. **Option B:** Migrate affected todos' `column_key` (and possibly timing) — needs spec + consent. +3. **Option C:** Store done-key-at-time / transition history — schema + burndown depth. + +**No near-term ship:** defer until a historical-data strategy exists; Options A–C are not adoption candidates without that design. + +**Tests:** deferred — `TestChangeDone_*` when/if strategy exists (§1.8). + +**Stats/charts:** high blast radius if shipped naively; **Phase 2** helps *current* dashboard reads, not burndown historical replay correctness. + +--- + +## 4. Dashboard Hardening and Optional Refactors + +Detail for **rollout Phase 2** (not blocking Phase 1 rename-label). + +### 4.1 Dashboard done-key / WIP parameterization (**not** blocking Phase 1) + +**Why fix early:** custom-workflow projects already see wrong dashboard metrics — **pre-existing** bug. Phase 1 rename does not change keys or SQL filters. + +**Scope**: Refactor `GetDashboardSummary` to resolve per-project done column keys and parameterize all SQL. + +**Approach**: + +```go +// In GetDashboardSummary, after loading projectIDs: +doneKeys, err := s.GetDoneColumnKeysForProjects(ctx, projectIDs) +// Returns map[int64]string (projectID → done column key) +``` + +For cross-project queries (throughput, completion): GROUP BY or JOIN with `project_workflow_columns`. + +For WIP: replace hardcoded `'doing'`/`'testing'` with a NOT IN (done_key, backlog_key) approach, or better: query all non-done, non-first-position lanes. + +**Files**: `internal/store/dashboard.go`, `internal/store/workflows.go` (new batch helper). + +### 4.2 Frontend fallback cleanup (optional) + +**Scope**: Remove or guard `columnsSpec()` hardcoded fallback in `drag-drop.ts`. Since `columnOrder` is always populated when the board API succeeds, the fallback is dead code. Either: +- Remove it and fail explicitly if `columnOrder` is missing +- Log a warning if the fallback is ever hit + +**Files**: `internal/httpapi/web/modules/features/drag-drop.ts`, `internal/httpapi/web/modules/state/state.ts`, `internal/httpapi/web/modules/state/selectors.ts`. + +--- + +## 5. Concrete Implementation Checklist + +### Phase 1: Rename label only + +- [ ] `internal/store/workflows.go`: `UpdateWorkflowColumnName(ctx, projectID int64, key, newName string) error` (name-only; **optional later:** extend for color if isolated) +- [ ] Strict PATCH validation in handler: reject unknown/mutating fields with `400` +- [ ] Preserve key immutability in API + UI; no key regeneration from name edits +- [ ] `internal/httpapi/routing.go`: `PATCH /api/board/{slug}/workflow/{key}` + maintainer check +- [ ] Reuse existing board refresh: `invalidateBoard` / `loadBoardBySlug`, clear `resolvedBoardBySlug`; **only if needed:** new SSE event in `sse.go` +- [ ] `internal/httpapi/web/modules/dialogs/settings.ts`: workflow label rename UI +- [ ] Test: `TestRenameLane_*` (no color-invalid tests in this increment) +- [ ] Manual QA: label, DnD, todo dialog, stats unchanged + +### Phase 2: Dashboard hardening + +- [ ] `internal/store/workflows.go`: `GetDoneColumnKeysForProjects` (or equivalent batch) +- [ ] `internal/store/dashboard.go`: parameterize `'done'`, `'doing'`, `'testing'` usage per §4.1 +- [ ] Tests: `TestDashboardSummary_CustomDoneKey`, `TestDashboardSummary_CustomWIPKeys` + +### Phase 3: Add lane + +- [ ] `internal/store/workflows.go`: `AddWorkflowColumn(...)` + insert-before-done placement (no arbitrary position input in v1) +- [ ] `internal/httpapi/routing.go`: `POST /api/board/{slug}/workflow` +- [ ] `internal/httpapi/web/modules/dialogs/settings.ts`: add-lane UI +- [ ] Test: `TestAddLane_*` +- [ ] *(Optional / future: dedicated reorder endpoint if product requires it)* + +### Phase 4: Delete empty non-done lane + +- [ ] `internal/store/workflows.go`: `DeleteWorkflowColumn(...)` with guards +- [ ] `internal/httpapi/routing.go`: `DELETE /api/board/{slug}/workflow/{key}` +- [ ] `internal/httpapi/web/modules/dialogs/settings.ts`: delete controls +- [ ] Test: `TestDeleteLane_*` + +### Deferred: Change done designation + +- [ ] **Do not schedule** until historical-data strategy (§3 deferred) is defined; then design + `TestChangeDone_*` + +### Cross-cutting (all phases) + +- [ ] `internal/httpapi/web/modules/features/drag-drop.ts`: optional `columnsSpec()` / `LANE_CARD_CLASSES` cleanup +- [ ] `internal/httpapi/web/modules/state/state.ts`: optional `boardLaneMeta` fallback cleanup +- [ ] **MCP:** out of scope for this rollout — no checklist items +- [ ] `README.md` / docs for workflow editing (as shipped) +- [ ] Optional: `workflowColumns` on public export (`projectExportJSON`) diff --git a/docs/draft/mcp_adapter_implementation_plan.md b/docs/draft/mcp_adapter_implementation_plan.md new file mode 100644 index 0000000..a407d6d --- /dev/null +++ b/docs/draft/mcp_adapter_implementation_plan.md @@ -0,0 +1,1182 @@ +## Scrumboy MCP Adapter Implementation Plan + +Builds on the prior audit: [mcp_readiness_audit_e58d38a8.plan.md](C:/Users/okayt/.cursor/plans/mcp_readiness_audit_e58d38a8.plan.md). + +This plan assumes: + +- The current frontend-facing REST surface must remain unchanged. +- MCP support should be added as a new adapter layer, not by normalizing existing REST handlers first. +- The adapter should be thin, incremental, and deterministic for machine clients. +- Canonical MCP todo identity is `projectSlug + localId` unless later code evidence proves that impossible. + +## 1. Where The MCP Layer Should Live + +### Recommended structure + +Use a new top-level package at [`internal/mcp/`](internal/mcp/) for the adapter core, and mount it from the existing HTTP server in [`internal/httpapi/server.go`](internal/httpapi/server.go). + +Recommended initial file layout: + +- [`internal/mcp/adapter.go`](internal/mcp/adapter.go) + - adapter struct, shared dependencies, mode/auth helpers +- [`internal/mcp/http_handler.go`](internal/mcp/http_handler.go) + - HTTP-facing MCP transport entrypoint mounted at `/mcp` +- [`internal/mcp/registry.go`](internal/mcp/registry.go) + - tool registry and dispatch +- [`internal/mcp/types.go`](internal/mcp/types.go) + - canonical MCP result/error/capabilities structs +- [`internal/mcp/errors.go`](internal/mcp/errors.go) + - adapter error codes and backend-to-MCP mapping +- [`internal/mcp/normalize.go`](internal/mcp/normalize.go) + - input camelCase normalization, optional/null handling, pagination translation +- [`internal/mcp/system_tools.go`](internal/mcp/system_tools.go) +- [`internal/mcp/projects_tools.go`](internal/mcp/projects_tools.go) +- [`internal/mcp/board_tools.go`](internal/mcp/board_tools.go) +- [`internal/mcp/todos_tools.go`](internal/mcp/todos_tools.go) +- [`internal/mcp/testutil_test.go`](internal/mcp/testutil_test.go) + - helper setup using the same store/test patterns already used in [`internal/httpapi/server_test.go`](internal/httpapi/server_test.go) + +Likely touched existing files: + +- [`cmd/scrumboy/main.go`](cmd/scrumboy/main.go) + - wire MCP adapter into the current process +- [`internal/config/config.go`](internal/config/config.go) + - possible touch point if mount/exposure is controlled via config +- [`internal/httpapi/server.go`](internal/httpapi/server.go) + - mount `/mcp` before SPA fallback + +### Why this structure fits this repo + +- [`cmd/scrumboy/main.go`](cmd/scrumboy/main.go) already builds one process around one `*store.Store` and one `http.Server`. Reusing that process is the lowest-risk path. +- [`internal/httpapi/`](internal/httpapi/) is heavily tied to browser concerns: cookies, SPA routing, SSE, HTML assets, CSRF header rules, and frontend JSON contracts. That makes it the wrong home for MCP tool logic. +- [`internal/store/`](internal/store/) already contains the real business and permission logic. The MCP layer should sit just above it, not below it and not inside the browser API package. +- Mounting `/mcp` from the existing server keeps deployment simple and avoids inventing a second daemon, second database wiring path, or separate lifecycle. + +### Chosen transport + +Initial transport should be HTTP-mounted MCP on the existing server, at `/mcp`. + +Why: + +- The repo already has only one server entrypoint and no CLI/stdio transport infrastructure. +- [`internal/httpapi/server.go`](internal/httpapi/server.go) already owns request dispatch, so adding one more top-level route is a very small change. +- Keeping MCP off `/api/*` avoids the existing `X-Scrumboy` mutation header rule and avoids accidental coupling to frontend REST conventions. + +### Decision point: MCP mount and exposure control + +This plan keeps the transport recommendation of `/mcp` on the existing server, but does not assume upfront how exposure is controlled. + +#### Option A: add an `MCPEnabled` config flag, default `false` + +Pros: + +- lowest operational surprise +- explicit rollout switch for dev/staging/local testing +- easy to keep dormant while package skeleton lands + +Cons: + +- adds new config surface and startup branching +- may be unnecessary if `/mcp` can always exist safely behind tool-level checks +- slightly increases config/test matrix + +Likely code touch points: + +- [`internal/config/config.go`](internal/config/config.go) +- [`cmd/scrumboy/main.go`](cmd/scrumboy/main.go) +- [`internal/httpapi/server.go`](internal/httpapi/server.go) + +Rollout implications: + +- safest for staged rollout +- requires deployment/config coordination before MCP is visible anywhere + +#### Option B: always mount `/mcp` and rely on tool-level auth/capability checks + +Pros: + +- simplest routing model +- fewer startup branches +- capability discovery can always explain current availability + +Cons: + +- exposes the surface everywhere immediately +- may be undesirable if the team wants MCP hidden until later phases +- needs especially clear capability and auth responses from day one + +Likely code touch points: + +- [`cmd/scrumboy/main.go`](cmd/scrumboy/main.go) +- [`internal/httpapi/server.go`](internal/httpapi/server.go) + +Rollout implications: + +- easiest implementation path +- requires confidence that an always-mounted but mostly inert `/mcp` route is acceptable operationally + +#### Option C: mount `/mcp` only in selected environments/builds if a natural pattern already exists + +Pros: + +- could align with existing repo/build behavior if such a pattern already exists +- avoids adding brand-new config if there is already a stronger precedent + +Cons: + +- current codebase does not yet show an obvious environment-specific server composition pattern +- risks adding build/runtime branching that is harder to reason about than a small config flag + +Likely code touch points: + +- [`cmd/scrumboy/main.go`](cmd/scrumboy/main.go) +- possible build or startup wiring around the main server path + +Rollout implications: + +- only attractive if a natural existing repo pattern is confirmed during implementation +- should not be invented from scratch just for MCP + +Current plan status: + +- Settled: MCP should live at `/mcp` on the existing server when enabled/mounted. +- Not yet settled: whether `/mcp` is always mounted or conditionally exposed. +- Earliest implementation work should preserve this decision point until a brief code inspection confirms whether there is already a natural exposure pattern. + +### Decision point: MCP auth strategy + +Auth is a first-order design decision for this adapter. It should be resolved early because it affects transport behavior, capability reporting, and which tools are safe to ship in the first cut. + +#### Option A: reuse existing session cookie behavior + +What it means technically: + +- MCP requests are authenticated the same way REST requests currently are, by resolving the existing session cookie into request context. +- The adapter reuses or lightly extracts the auth-context logic currently centered in [`internal/httpapi/server.go`](internal/httpapi/server.go). + +Likely file/package touch points: + +- [`internal/httpapi/server.go`](internal/httpapi/server.go) +- [`internal/mcp/adapter.go`](internal/mcp/adapter.go) +- [`internal/mcp/http_handler.go`](internal/mcp/http_handler.go) + +Main risks: + +- MCP clients may not reliably send browser-style cookies +- could accidentally pull browser assumptions into the adapter if not isolated cleanly +- may work well only for locally co-hosted clients and not for other MCP runtimes + +Effect on near-term rollout: + +- safest if the initial MCP client is local and can share the existing authenticated session +- makes `system.getCapabilities` and `projects.list` straightforward +- still leaves auth portability unresolved for later phases + +#### Option B: add a separate MCP-specific token or bearer mechanism + +What it means technically: + +- `/mcp` accepts a dedicated token/header flow instead of or in addition to the session cookie +- adapter auth context is derived separately from current browser session handling + +Likely file/package touch points: + +- [`internal/mcp/http_handler.go`](internal/mcp/http_handler.go) +- [`internal/mcp/adapter.go`](internal/mcp/adapter.go) +- likely config and token-management touch points not yet present in current code + +Main risks: + +- larger scope than the current thin-adapter goal +- introduces new secret lifecycle and probably new persistence/config needs +- can easily drag Phase 0 into auth-product work + +Effect on near-term rollout: + +- may be the right long-term answer +- is not the lowest-risk path for the first slice unless a target MCP runtime makes cookie reuse impossible + +#### Option C: keep early MCP phases limited until auth is resolved + +What it means technically: + +- land the adapter skeleton and capability surface first +- optionally ship only tools whose auth story is clear +- defer broader authenticated tool rollout until one auth path is chosen + +Likely file/package touch points: + +- [`internal/mcp/adapter.go`](internal/mcp/adapter.go) +- [`internal/mcp/system_tools.go`](internal/mcp/system_tools.go) +- [`internal/mcp/projects_tools.go`](internal/mcp/projects_tools.go) + +Main risks: + +- slows down functional breadth +- may create temporary capability/tool gating that later needs tightening + +Effect on near-term rollout: + +- best match if auth cannot be settled quickly +- supports the safer first cut of skeleton plus `system.getCapabilities` and, if viable, `projects.list` + +Current plan status: + +- Settled: auth must be surfaced explicitly in capabilities and error contracts. +- Not yet settled: whether the initial implementation uses session cookies, a new token flow, or a deliberately limited early rollout. +- The first coding cut should avoid committing the rest of the adapter to one auth mechanism beyond what is necessary for `system.getCapabilities` and possibly `projects.list`. + +## 2. What The MCP Layer Should Call + +### Recommended approach + +Use a hybrid approach with a strong bias toward direct store calls: + +- MCP transport/auth context setup should reuse existing server request context behavior where possible. +- MCP tool implementations should call store-layer functions directly. +- MCP should not call existing REST handlers indirectly. +- Shared logic should be extracted only when duplication is non-trivial and clearly safer than copying a few lines. + +### Why direct store calls are safest here + +- Existing REST handlers in [`internal/httpapi/routing.go`](internal/httpapi/routing.go), [`internal/httpapi/dashboard.go`](internal/httpapi/dashboard.go), and [`internal/httpapi/auth_2fa.go`](internal/httpapi/auth_2fa.go) are tightly coupled to HTTP parsing, status codes, HTML responses, and current frontend response shapes. +- The store already enforces most of the important business rules and access checks: + - project read/write access in [`internal/store/projects.go`](internal/store/projects.go) + - todo mutation and edit-scope logic in [`internal/store/todos.go`](internal/store/todos.go) + - link behavior in [`internal/store/links.go`](internal/store/links.go) + - system role checks in [`internal/store/system_roles.go`](internal/store/system_roles.go) +- Calling handlers would force the MCP layer to translate already-inconsistent REST responses back into normalized tool results. That adds complexity without gaining safety. + +### What can be reused safely + +- Request user resolution from [`internal/httpapi/server.go`](internal/httpapi/server.go) should be reused conceptually and, if practical, via a small extracted helper so MCP and REST derive auth context the same way. +- Store methods should be reused directly for all first-slice tools. +- Existing JSON shaping in [`internal/httpapi/json.go`](internal/httpapi/json.go) should not be reused by default. Those structs were built to preserve UI contracts, not MCP contracts. + +### When extraction is worth it + +Worth extracting: + +- a small auth-context helper if MCP and REST both need identical cookie-to-context behavior +- a small mode/capabilities helper if both REST and MCP need to expose `full` vs `anonymous` and bootstrap status + +Not worth extracting in Phase 0 or 1: + +- generic projection/model layers +- a shared “application service” package spanning every domain +- current REST JSON DTOs into a shared contract package + +## 3. Canonical MCP Contract + +### Canonical conventions + +#### Resource identity + +- Project identity: `projectSlug` +- Todo identity: `projectSlug + localId` +- Sprint identity: `projectSlug + sprintId` +- Tag identity: + - project-scoped mutations prefer `tagId` when known + - name-based fallback stays adapter-internal only where backend behavior requires it + +The MCP layer must never expose global todo ID routes as canonical. + +#### Input naming + +- All MCP inputs use camelCase. +- Adapter accepts only canonical camelCase at the MCP surface. +- Adapter internally translates to legacy backend fields only when store calls or transitional handler reuse require it. + +#### Output envelope + +Every successful tool returns: + +```json +{ + "ok": true, + "data": {}, + "meta": {} +} +``` + +Rules: + +- `data` is always present on success. +- Collections are wrapped as `data.items`, never returned as raw arrays. +- Entity fetches return a named object inside `data`, not a raw object at the top level. +- `meta` is optional in meaning, but present as an object for consistency. + +#### Output field stability + +- MCP output field names are part of the adapter contract and should be treated as compatibility promises once exposed. +- Field names should be semantic and stable, even when backend/store/REST naming is inconsistent today. +- Backend quirks should be translated behind the adapter rather than leaked into MCP because they already exist internally. +- Avoid exposing transitional or shaky field names that are likely to be renamed in the next phase. + +#### Pagination model + +Default canonical pagination for standard list/search tools: + +```json +{ + "limit": 20, + "cursor": "opaque-string" +} +``` + +Default canonical paged output: + +```json +{ + "ok": true, + "data": { + "items": [] + }, + "meta": { + "nextCursor": "opaque-string-or-null", + "hasMore": true + } +} +``` + +#### Board pagination special case + +`board.get` is not a standard single-list pagination problem. The current backend board surface already carries per-column metadata, and forcing that into the same `limit/cursor` output shape as normal list/search tools would hide useful structure or create awkward abstraction. + +Board-specific rule: + +- `limit` may still be used as an input concept for per-column window size +- board outputs may use specialized metadata such as `nextCursorByColumn` and `hasMoreByColumn` +- this is an explicit adapter special case, not a failure of the broader pagination model + +Why this is acceptable: + +- normal list/search tools still benefit from one simple default contract +- board views are structurally multi-column and deserve a specialized response shape +- keeping the exception explicit is less risky than pretending a universal pagination envelope fits both cases cleanly + +#### Error shape + +The adapter should normalize internal/backend failures to one MCP-side error payload shape: + +```json +{ + "ok": false, + "error": { + "code": "AUTH_REQUIRED", + "message": "Sign-in required for this tool", + "details": { + "backendCode": "UNAUTHORIZED" + } + } +} +``` + +Canonical adapter codes should include: + +- `AUTH_REQUIRED` +- `FORBIDDEN` +- `NOT_FOUND` +- `VALIDATION_ERROR` +- `CONFLICT` +- `RATE_LIMITED` +- `MODE_UNAVAILABLE` +- `CAPABILITY_UNAVAILABLE` +- `INTERNAL` + +#### Auth and capability discovery + +The adapter should expose one explicit introspection tool before domain tools: + +- `system.getCapabilities` + +This tool should report: + +- current server mode: `full` or `anonymous` +- which auth mechanism is configured or unresolved for MCP +- whether authenticated tools are usable under the current configuration +- whether bootstrap is available +- canonical identity rules +- implemented tools +- planned tools only if the adapter intentionally wants to expose roadmap-like visibility, and only when clearly labeled as non-implemented +- major adapter constraints such as legacy route suppression and current pagination contract + +Truthfulness rule: + +- `system.getCapabilities` must describe what the adapter can actually do in the current implementation. +- The tool list must not advertise future tools as if they already exist. +- If roadmap visibility is useful, expose it separately as something like `plannedTools`, not mixed into `implementedTools`. + +### Example MCP tool shapes + +#### `system.getCapabilities` + +Input: + +```json +{} +``` + +Output: + +```json +{ + "ok": true, + "data": { + "serverMode": "full", + "auth": { + "mode": "sessionCookie|bearerToken|limitedEarlyPhase", + "authenticated": true, + "bootstrapAvailable": false + }, + "identity": { + "project": "projectSlug", + "todo": ["projectSlug", "localId"], + "sprint": ["projectSlug", "sprintId"] + }, + "pagination": { + "defaultInput": ["limit", "cursor"], + "defaultOutput": ["nextCursor", "hasMore"], + "specialCases": ["board.get"] + }, + "implementedTools": [ + "system.getCapabilities", + "projects.list" + ], + "plannedTools": [ + "board.get", + "todos.create" + ] + }, + "meta": { + "adapterVersion": 1 + } +} +``` + +#### `projects.list` + +Input: + +```json +{} +``` + +Output: + +```json +{ + "ok": true, + "data": { + "items": [ + { + "projectSlug": "demo-board", + "projectId": 12, + "name": "Demo Board", + "image": null, + "dominantColor": "#64748b", + "defaultSprintWeeks": 2, + "expiresAt": null, + "createdAt": "2026-03-31T12:00:00Z", + "updatedAt": "2026-03-31T12:00:00Z", + "role": "maintainer" + } + ] + }, + "meta": {} +} +``` + +#### `board.get` + +Input: + +```json +{ + "projectSlug": "demo-board", + "tag": "bug", + "search": "login", + "sprintId": "active", + "limit": 50 +} +``` + +Output: + +```json +{ + "ok": true, + "data": { + "project": { + "projectSlug": "demo-board", + "projectId": 12, + "name": "Demo Board", + "role": "maintainer" + }, + "columns": [ + { + "key": "backlog", + "name": "Backlog", + "isDone": false, + "items": [] + } + ], + "tags": [ + { + "tagId": 3, + "name": "bug", + "count": 4, + "color": "#ef4444", + "canDelete": true + } + ] + }, + "meta": { + "nextCursorByColumn": { + "backlog": null + }, + "hasMoreByColumn": { + "backlog": false + } + } +} +``` + +#### `todos.create` + +Input: + +```json +{ + "projectSlug": "demo-board", + "title": "Add MCP adapter", + "body": "Thin layer only", + "tags": ["mcp"], + "columnKey": "backlog", + "estimationPoints": 3, + "sprintId": null, + "assigneeUserId": null, + "position": { + "afterLocalId": null, + "beforeLocalId": null + } +} +``` + +Output: + +```json +{ + "ok": true, + "data": { + "todo": { + "projectSlug": "demo-board", + "id": 101, + "localId": 27, + "title": "Add MCP adapter", + "body": "Thin layer only", + "columnKey": "backlog", + "status": "BACKLOG", + "tags": ["mcp"], + "estimationPoints": 3, + "assigneeUserId": null, + "sprintId": null, + "createdAt": "2026-03-31T12:00:00Z", + "updatedAt": "2026-03-31T12:00:00Z" + } + }, + "meta": {} +} +``` + +#### `todos.update` + +Input: + +```json +{ + "projectSlug": "demo-board", + "localId": 27, + "patch": { + "title": "Add initial MCP adapter", + "body": "Phase 0 and 1 only", + "tags": ["mcp", "backend"], + "estimationPoints": 5, + "assigneeUserId": null, + "sprintId": null + } +} +``` + +Output: + +```json +{ + "ok": true, + "data": { + "todo": { + "projectSlug": "demo-board", + "localId": 27, + "title": "Add initial MCP adapter" + } + }, + "meta": {} +} +``` + +#### `todos.move` + +Input: + +```json +{ + "projectSlug": "demo-board", + "localId": 27, + "toColumnKey": "in-progress", + "afterLocalId": 22, + "beforeLocalId": null +} +``` + +Output: + +```json +{ + "ok": true, + "data": { + "todo": { + "projectSlug": "demo-board", + "localId": 27, + "columnKey": "in-progress", + "status": "IN-PROGRESS" + } + }, + "meta": {} +} +``` + +#### `todos.search` + +Input: + +```json +{ + "projectSlug": "demo-board", + "query": "adapter", + "limit": 20, + "excludeLocalIds": [27] +} +``` + +Output: + +```json +{ + "ok": true, + "data": { + "items": [ + { + "projectSlug": "demo-board", + "localId": 31, + "title": "Document adapter behavior" + } + ] + }, + "meta": { + "nextCursor": null, + "hasMore": false + } +} +``` + +## 4. First Implementation Slice + +### Recommended first slice + +Implement the safest smallest vertical slice that proves the adapter shape and capability contract without forcing early commitment on board projection or auth breadth: + +- `system.getCapabilities` +- `projects.list` + +Why this slice: + +- It exercises mode/auth discovery before any mutation. +- It establishes the adapter skeleton, canonical envelope, and capability contract first. +- It gives one simple authenticated read tool that is much less entangled with frontend DTO shaping than `board.get`. +- It preserves room to resolve auth and board-projection decisions before mutation tools arrive. + +### Immediate next-step decision point: `board.get` + +`board.get` is still likely one of the earliest useful MCP tools, but it is a higher-risk read tool than `projects.list`. + +Why it is higher risk in this codebase: + +- the current board response shape in [`internal/httpapi/json.go`](internal/httpapi/json.go) is closely aligned to frontend board rendering +- it already carries board-specific multi-column pagination metadata +- it is the easiest place for frontend-shaped DTO complexity to leak into MCP too early + +Decision point: + +- Option A: keep `board.get` in Phase 1 if code inspection shows the projection can stay clean, small, and clearly MCP-specific +- Option B: move `board.get` to the immediate follow-on phase after Phase 1 if projection is too entangled with frontend concerns + +Current plan status: + +- Settled: `projects.list` is the safest first read tool. +- Not yet settled: whether `board.get` belongs in Phase 1 or immediately after it. +- First implementation prompt should not require `board.get`. + +### Files likely to be added + +- [`internal/mcp/adapter.go`](internal/mcp/adapter.go) +- [`internal/mcp/http_handler.go`](internal/mcp/http_handler.go) +- [`internal/mcp/registry.go`](internal/mcp/registry.go) +- [`internal/mcp/types.go`](internal/mcp/types.go) +- [`internal/mcp/errors.go`](internal/mcp/errors.go) +- [`internal/mcp/normalize.go`](internal/mcp/normalize.go) +- [`internal/mcp/system_tools.go`](internal/mcp/system_tools.go) +- [`internal/mcp/projects_tools.go`](internal/mcp/projects_tools.go) +- [`internal/mcp/adapter_test.go`](internal/mcp/adapter_test.go) +- [`internal/mcp/normalize_test.go`](internal/mcp/normalize_test.go) +- [`internal/mcp/http_handler_test.go`](internal/mcp/http_handler_test.go) + +### Files likely to be modified + +- [`cmd/scrumboy/main.go`](cmd/scrumboy/main.go) +- [`internal/config/config.go`](internal/config/config.go) if the mount decision selects config-gated exposure +- [`internal/httpapi/server.go`](internal/httpapi/server.go) + +### Existing code dependencies + +- mode/auth context from [`internal/httpapi/server.go`](internal/httpapi/server.go) +- project visibility and role checks from [`internal/store/projects.go`](internal/store/projects.go) + +### Risk level + +Low for the first cut if limited to skeleton plus `system.getCapabilities` and `projects.list`. + +Main risks: + +- auth context reuse may be awkward if `requestContext` stays private to `httpapi.Server` +- MCP client auth expectations may not match cookie-based auth +- mount/exposure decision may add minor startup wiring churn depending on which option is chosen + +### What should be tested + +- capability reporting in `full` vs `anonymous` mode +- `projects.list` auth behavior in full mode and anonymous mode +- normalization of empty arrays/nulls/meta fields +- explicit non-exposure of legacy global todo IDs as MCP input identifiers in capabilities and registry contracts + +## 5. Staged Rollout Plan + +### Phase 0: Groundwork + +#### Scope + +- package skeleton +- `/mcp` mount +- adapter registry +- canonical result/error helpers +- resolve the mount/exposure decision +- resolve or deliberately constrain the early auth decision + +#### Tools included + +- none or only a no-op health dispatch until `system.getCapabilities` is wired + +#### Technical tasks + +- decide among the mount/exposure options in Section 1 before wiring startup behavior +- construct adapter in [`cmd/scrumboy/main.go`](cmd/scrumboy/main.go) from the same store and mode already used by `httpapi` +- mount `/mcp` in [`internal/httpapi/server.go`](internal/httpapi/server.go) before SPA routing +- add `internal/mcp` package skeleton and registry +- add canonical success/error helpers +- add enough auth/capability plumbing for `system.getCapabilities` to report the configured or unresolved auth mode + +#### Risks + +- route mount order accidentally interfering with SPA or `/api/*` +- overcommitting to an SDK or transport abstraction too early + +#### Exit criteria + +- chosen mount behavior is documented in code and tests +- `/mcp` exposure behavior matches the selected option +- no existing frontend/API tests regress + +### Phase 1: Capability + Safe Read Tools + +#### Scope + +- capability discovery +- authenticated, read-only project tools +- optional `board.get` only if projection stays clean after a focused code check + +#### Exact tools + +- `system.getCapabilities` +- `projects.list` +- `board.get` only if the Phase 1 decision check stays low-risk; otherwise defer to the immediate next phase + +#### Technical tasks + +- implement mode/bootstrap/auth reporting using current store + mode state +- add canonical project projection +- if `board.get` remains in Phase 1, add a board projection that is explicitly MCP-specific rather than a thin copy of the frontend DTO +- ensure auth failures become deterministic adapter errors + +#### Risks + +- board projection shape growing too close to current frontend DTOs +- unclear semantics for anonymous temporary boards unless capability output is explicit + +#### Exit criteria + +- MCP client can discover mode and list projects through canonical `projectSlug` +- output shapes are stable and wrapped +- unauthorized access is deterministic from MCP perspective + +### Phase 1B: `board.get` if deferred + +#### Scope + +- add `board.get` only after a focused projection check + +#### Exact tools + +- `board.get` + +#### Technical tasks + +- inspect board/store shaping paths closely enough to decide whether a clean MCP projection is small and durable +- define the board-specific pagination metadata separately from standard list/search pagination +- ensure the adapter does not simply mirror [`internal/httpapi/json.go`](internal/httpapi/json.go) board DTO structure unless that shape is truly stable and semantically appropriate for MCP + +#### Risks + +- early leakage of frontend board DTO assumptions +- bloating the first rollout with multi-column pagination and filtering semantics + +#### Exit criteria + +- `board.get` contract is demonstrably MCP-specific, stable, and not just a REST/UI DTO copy +- board pagination special-case metadata is documented and tested + +### Phase 2: Core Todo Mutation Tools + +#### Scope + +- create, update, move, get, delete, search + +#### Exact tools + +- `todos.create` +- `todos.get` +- `todos.update` +- `todos.move` +- `todos.delete` +- `todos.search` + +#### Technical tasks + +- translate canonical `projectSlug + localId` to current store calls +- implement patch normalization so missing fields are not confused with explicit nulls +- hide backend quirks such as required `assigneeUserId` presence on update +- normalize search input to `query`, `limit`, `excludeLocalIds` + +#### Risks + +- update semantics: current REST layer enforces presence of `assigneeUserId`; MCP patch behavior must be explicit +- move positioning needs consistent handling of `afterLocalId` vs `beforeLocalId` + +#### Exit criteria + +- all core todo operations work without exposing global todo ID routes +- canonical patch semantics are tested +- machine clients can create and modify todos using only slug/localId + +### Phase 3: Sprints, Tags, Members + +#### Scope + +- secondary project workflows and project administration + +#### Exact tools + +- `sprints.list`, `sprints.create`, `sprints.get`, `sprints.update`, `sprints.delete`, `sprints.activate`, `sprints.close` +- `tags.listProject`, `tags.listMine`, `tags.updateColor`, `tags.delete` +- `members.list`, `members.add`, `members.updateRole`, `members.remove`, `members.listAvailable` + +#### Technical tasks + +- normalize mixed 204/object behaviors from current semantics into structured results +- hide durable-vs-anonymous tag routing differences behind one adapter contract +- make maintainer-only operations explicit in capability and auth failure metadata + +#### Risks + +- tag behavior differs across durable projects vs anonymous boards +- sprint list currently has mixed empty behavior (`204` vs object); adapter must pick one stable shape + +#### Exit criteria + +- all project collaboration tools use one canonical contract +- no MCP tool requires the caller to know durable vs temp routing quirks + +### Phase 4: Optional Advanced Surface + +#### Scope + +- dashboard/admin/backup/auth mutations/streaming if still valuable + +#### Exact tools + +- `dashboard.getSummary` +- `dashboard.listTodos` +- `admin.users.*` +- `backup.*` +- optional event/stream subscriptions +- optional auth tools beyond capability discovery + +#### Technical tasks + +- add explicit auth-required metadata for dashboard tools +- decide whether admin/backup should be exposed at all in initial MCP scope +- evaluate streaming only after the read/write tool surface is stable + +#### Risks + +- backup/admin tools carry higher blast radius +- streaming increases transport complexity disproportionately +- auth mutation tools may force a more explicit MCP auth story + +#### Exit criteria + +- only tools with clear MCP value and safe semantics are exposed +- advanced tools do not weaken the simple deterministic core + +## 6. Normalization Rules + +These are implementation rules for the adapter, not general principles. + +### `204` responses or no-body operations + +- MCP never returns an empty success. +- For delete/activate/close/disable-style operations, synthesize: + +```json +{ + "ok": true, + "data": { + "status": "deleted" + }, + "meta": {} +} +``` + +- If the operation has a more specific semantic, use that instead of `"deleted"`: + - `"updated"` + - `"moved"` + - `"activated"` + - `"closed"` + - `"loggedOut"` + +### HTML or non-JSON success responses + +- MCP must never forward raw HTML. +- For endpoints whose current REST success is HTML or text, the adapter must map them to semantic JSON success. +- Example: logout should become `{ ok:true, data:{ status:"loggedOut" }, meta:{} }`. + +### Array responses + +- MCP must never return a raw array at top level. +- Wrap all arrays as `data.items`. +- Preserve item order exactly as returned by store/backend logic. + +### Mixed snake_case vs camelCase inputs + +- MCP accepts camelCase only. +- Adapter translates legacy request names internally: + - `userId` -> backend `user_id` when needed + - `newPassword` -> backend `new_password` when needed +- Adapter should reject snake_case at the MCP boundary with `VALIDATION_ERROR` rather than silently accepting multiple spellings. + +### Legacy route suppression + +- MCP must never expose: + - `/api/todos/{id}` + - `/api/projects/{id}/board` as a canonical board fetch + - any global todo ID mutation path as MCP tool input +- Legacy backend routes remain untouched for frontend compatibility only. +- If adapter implementation internally falls back to a legacy path during transition, that must remain invisible to callers and marked with a code comment as transitional. + +### Mode-dependent behavior + +- `system.getCapabilities` must always be callable. +- In `anonymous` mode: + - capability output must say auth tools are unavailable + - project-scoped read/write tools must clearly indicate anonymous-board limitations +- In pre-bootstrap `full` mode: + - capability output must say `bootstrapAvailable: true` + - tools that are blocked only because no session exists should return `AUTH_REQUIRED`, not generic internal errors +- If auth is unresolved or intentionally limited in the first cut, `projects.list` must fail deterministically with `AUTH_REQUIRED` or `CAPABILITY_UNAVAILABLE`; it must not silently disappear from routing/dispatch or degrade into an ambiguous not-found behavior. + +### Auth failure vs not-found masking + +- The REST layer intentionally sometimes maps unauthorized access to `NOT_FOUND`. +- MCP should preserve privacy but reduce ambiguity where the caller already has a canonical target shape. +- Rule: + - entrypoint-level missing auth becomes `AUTH_REQUIRED` + - project resource fetch after auth is present but membership is missing stays `NOT_FOUND` + - capability output should warn that some unauthorized project accesses are intentionally masked as not found + +### Pagination translation + +- Standard list/search tools use canonical MCP input `limit` and `cursor`. +- Translation rules: + - board lane paging: MCP `cursor` maps to backend `afterCursor` + - dashboard paging: MCP `cursor` maps to backend `cursor` + - board summary fetch with lane windows, if exposed via `board.get`, may map MCP `limit` to backend `limitPerLane` +- Standard paged outputs normalize to `meta.nextCursor` and `meta.hasMore`. +- `board.get` is an explicit special case and may expose `meta.nextCursorByColumn` and `meta.hasMoreByColumn`. + +### Optional and null field normalization + +- If a field is meaningful but unset, return `null`, not omission, unless omission has actual semantic value. +- Collections should default to `[]`, not `null`. +- `meta` should always be an object. +- Omit fields only when omission itself is part of the contract and documented, such as capability sections not available in current mode. + +## 7. Test Strategy + +### Recommended test locations + +- [`internal/mcp/normalize_test.go`](internal/mcp/normalize_test.go) +- [`internal/mcp/errors_test.go`](internal/mcp/errors_test.go) +- [`internal/mcp/registry_test.go`](internal/mcp/registry_test.go) +- [`internal/mcp/http_handler_test.go`](internal/mcp/http_handler_test.go) +- [`internal/mcp/projects_tools_test.go`](internal/mcp/projects_tools_test.go) +- [`internal/mcp/board_tools_test.go`](internal/mcp/board_tools_test.go) +- [`internal/mcp/todos_tools_test.go`](internal/mcp/todos_tools_test.go) + +Optional light-touch route smoke test: + +- extend [`internal/httpapi/server_test.go`](internal/httpapi/server_test.go) only to verify `/mcp` mount behavior does not break current routing + +### What to test + +#### Unit tests for normalization helpers + +- array wrapping +- null/default handling +- canonical error mapping +- pagination translation +- camelCase input validation and translation + +#### Tool tests + +- `system.getCapabilities` in `full`, `anonymous`, and pre-bootstrap cases +- `projects.list` with auth and without auth +- `board.get` by slug with filters only when that tool is included in the selected phase + +#### Auth and mode behavior tests + +- anonymous mode still exposes capabilities but not auth-only tools +- full mode without session returns deterministic `AUTH_REQUIRED` +- membership denial remains `NOT_FOUND` where masking is intended + +#### Canonical identity regression tests + +- todo tools require `projectSlug + localId` +- no MCP tool accepts or emits global todo ID as the primary identifier +- board and todo outputs include enough canonical identity fields for follow-up calls + +#### Fixture-based tests + +- Add a very small set of fixture-style JSON request/response examples only for normalization-heavy tools. +- Do not build a large golden-fixture suite in Phase 0 or 1. + +### What not to over-test initially + +- full transport protocol edge cases beyond the supported `/mcp` path +- every domain before the tool exists +- duplicated coverage already exercised by store tests +- browser-specific behavior, since MCP should stay transport- and client-neutral + +## 8. Sharp Edges / Do Not Do Yet + +## Do Not Do Yet + +- Do not rewrite existing REST routes in [`internal/httpapi/routing.go`](internal/httpapi/routing.go). +- Do not rename backend fields everywhere just to make REST cleaner. +- Do not replace the frontend’s current use of legacy or duplicate REST routes. +- Do not refactor store authorization into a new generic auth platform layer. +- Do not move current frontend JSON DTOs out of [`internal/httpapi/json.go`](internal/httpapi/json.go) unless a specific duplication proves painful. +- Do not introduce a second binary or stdio-only process before the HTTP-mounted adapter proves useful. +- Do not expose admin or backup tools in the first slice. +- Do not over-abstract a plugin/service architecture before at least Phase 2 exists. +- Do not promise generic bidirectional MCP streaming in the first implementation. + +## 9. Implementation Order Recommendation + +Recommended phase order: + +1. Phase 0: Groundwork +2. Phase 1: Capability + Safe Read Tools +3. Phase 2: Core Todo Mutation Tools +4. Phase 3: Sprints, Tags, Members +5. Phase 4: Optional Advanced Surface + +### Exact first coding task + +Add the MCP skeleton: + +- resolve the minimal mount/exposure approach from the options in Section 1 +- wire an `internal/mcp` adapter from [`cmd/scrumboy/main.go`](cmd/scrumboy/main.go) +- mount `/mcp` in [`internal/httpapi/server.go`](internal/httpapi/server.go) +- add an empty registry and canonical result/error types + +### Exact second coding task + +Implement `system.getCapabilities` end to end: + +- mode reporting +- auth/bootstrap reporting +- tool list +- canonical identity and pagination declarations + +### Exact third coding task + +Implement the safest Phase 1 read tool: + +1. `projects.list` + +Then perform a focused projection check for `board.get` before deciding whether it belongs in Phase 1 or Phase 1B. + +## Recommended next implementation prompt + +Implement only the safest first cut from `mcp_adapter_implementation_plan.md`. + +Constraints: + +- Do not modify existing REST route behavior used by the frontend. +- Add MCP as a thin new layer mounted at `/mcp`. +- Use `internal/mcp/` as the main package for adapter logic. +- Reuse current store/business logic; do not route MCP calls through existing REST handlers. +- Before choosing how `/mcp` is exposed, do a brief code inspection against the mount/exposure decision point in the plan and pick the smallest repo-consistent option. +- Before committing to an auth mechanism beyond what is needed for this cut, do a brief code inspection against the auth decision point in the plan and keep the implementation as non-committal as practical. +- Implement only these MCP tools: + - `system.getCapabilities` + - `projects.list` +- Use canonical MCP conventions from the plan: + - project identity is `projectSlug` + - todo identity strategy should be declared in capabilities but no todo tools yet + - success shape is `{ ok, data, meta }` + - deterministic normalized error shape + - arrays wrapped in `data.items` +- Add focused tests under `internal/mcp/` plus only minimal route-mount coverage in existing server tests if needed. +- Leave `board.get` for the next prompt unless code inspection shows it is genuinely low-risk and can be projected cleanly without importing frontend DTO complexity. +- Do not add admin, backup, auth mutation, tag, sprint, or todo mutation tools yet. + +Before editing, quickly verify the best way to reuse current request auth context from `internal/httpapi/server.go`. If extracting a tiny shared helper is safer than duplicating cookie parsing, do that; otherwise keep the change minimal and local. diff --git a/internal/httpapi/routing.go b/internal/httpapi/routing.go index 5010dee..c174d67 100644 --- a/internal/httpapi/routing.go +++ b/internal/httpapi/routing.go @@ -1069,6 +1069,122 @@ func (s *Server) handleBoard(w http.ResponseWriter, r *http.Request, rest []stri return } + // POST /api/board/{slug}/workflow - add a new non-done lane before done. + if len(rest) == 2 && rest[1] == "workflow" && r.Method == http.MethodPost { + ctx := s.requestContext(r) + userID, ok := store.UserIDFromContext(ctx) + if !ok { + writeError(w, http.StatusUnauthorized, "UNAUTHORIZED", "unauthorized", nil) + return + } + role, err := s.store.GetProjectRole(ctx, project.ID, userID) + if err != nil || !role.HasMinimumRole(store.RoleMaintainer) { + writeError(w, http.StatusForbidden, "FORBIDDEN", "maintainer or higher required", nil) + return + } + + var in struct { + Name string `json:"name"` + } + if err := readJSON(w, r, s.maxBody, &in); err != nil { + return + } + in.Name = strings.TrimSpace(in.Name) + if in.Name == "" { + writeError(w, http.StatusBadRequest, "VALIDATION_ERROR", "name required", map[string]any{"field": "name"}) + return + } + if len(in.Name) > 200 { + writeError(w, http.StatusBadRequest, "VALIDATION_ERROR", "invalid workflow column name", map[string]any{"field": "name"}) + return + } + + col, err := s.store.AddWorkflowColumn(ctx, project.ID, in.Name) + if err != nil { + writeStoreErr(w, err, true) + return + } + s.emitRefreshNeeded(project.ID, "workflow_column_added") + writeJSON(w, http.StatusCreated, workflowColumnJSON{ + Key: col.Key, + Name: col.Name, + Color: col.Color, + IsDone: col.IsDone, + Position: col.Position, + }) + return + } + + // PATCH /api/board/{slug}/workflow/{key} - rename workflow lane label only. + if len(rest) == 3 && rest[1] == "workflow" && r.Method == http.MethodPatch { + ctx := s.requestContext(r) + userID, ok := store.UserIDFromContext(ctx) + if !ok { + writeError(w, http.StatusUnauthorized, "UNAUTHORIZED", "unauthorized", nil) + return + } + role, err := s.store.GetProjectRole(ctx, project.ID, userID) + if err != nil || !role.HasMinimumRole(store.RoleMaintainer) { + writeError(w, http.StatusForbidden, "FORBIDDEN", "maintainer or higher required", nil) + return + } + columnKey := strings.TrimSpace(rest[2]) + if columnKey == "" { + writeError(w, http.StatusBadRequest, "VALIDATION_ERROR", "invalid workflow key", map[string]any{"field": "key"}) + return + } + + var in struct { + Name string `json:"name"` + } + if err := readJSON(w, r, s.maxBody, &in); err != nil { + return + } + in.Name = strings.TrimSpace(in.Name) + if in.Name == "" { + writeError(w, http.StatusBadRequest, "VALIDATION_ERROR", "name required", map[string]any{"field": "name"}) + return + } + if len(in.Name) > 200 { + writeError(w, http.StatusBadRequest, "VALIDATION_ERROR", "invalid workflow column name", map[string]any{"field": "name"}) + return + } + if err := s.store.UpdateWorkflowColumnName(ctx, project.ID, columnKey, in.Name); err != nil { + writeStoreErr(w, err, true) + return + } + s.emitRefreshNeeded(project.ID, "workflow_column_renamed") + w.WriteHeader(http.StatusNoContent) + return + } + + // DELETE /api/board/{slug}/workflow/{key} - delete an empty non-done lane. + if len(rest) == 3 && rest[1] == "workflow" && r.Method == http.MethodDelete { + ctx := s.requestContext(r) + userID, ok := store.UserIDFromContext(ctx) + if !ok { + writeError(w, http.StatusUnauthorized, "UNAUTHORIZED", "unauthorized", nil) + return + } + role, err := s.store.GetProjectRole(ctx, project.ID, userID) + if err != nil || !role.HasMinimumRole(store.RoleMaintainer) { + writeError(w, http.StatusForbidden, "FORBIDDEN", "maintainer or higher required", nil) + return + } + columnKey := strings.TrimSpace(rest[2]) + if columnKey == "" { + writeError(w, http.StatusBadRequest, "VALIDATION_ERROR", "invalid workflow key", map[string]any{"field": "key"}) + return + } + if err := s.store.DeleteWorkflowColumn(ctx, project.ID, columnKey); err != nil { + writeStoreErr(w, err, true) + return + } + s.emitRefreshNeeded(project.ID, "workflow_column_deleted") + w.WriteHeader(http.StatusNoContent) + return + } + // GET /api/board/{slug}/lanes/{status} if len(rest) == 3 && rest[1] == "lanes" && r.Method == http.MethodGet { columnKey := normalizeLaneKey(rest[2]) @@ -1444,8 +1560,8 @@ func (s *Server) handleBoard(w http.ResponseWriter, r *http.Request, rest []stri var in struct { ToColumnKey string `json:"toColumnKey"` ToStatus string `json:"toStatus"` - AfterID *int64 `json:"afterId"` - BeforeID *int64 `json:"beforeId"` + AfterID *int64 `json:"afterId"` + BeforeID *int64 `json:"beforeId"` } if err := readJSON(w, r, s.maxBody, &in); err != nil { return @@ -1959,8 +2075,8 @@ func (s *Server) handleTodos(w http.ResponseWriter, r *http.Request, rest []stri var in struct { ToColumnKey string `json:"toColumnKey"` ToStatus string `json:"toStatus"` - AfterID *int64 `json:"afterId"` - BeforeID *int64 `json:"beforeId"` + AfterID *int64 `json:"afterId"` + BeforeID *int64 `json:"beforeId"` } if err := readJSON(w, r, s.maxBody, &in); err != nil { return diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index b12c423..c90e798 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -79,6 +79,9 @@ type storeAPI interface { UpdateProjectImage(ctx context.Context, projectID int64, userID int64, image *string, dominantColor string) error UpdateProjectName(ctx context.Context, projectID int64, userID int64, name string) error UpdateProjectDefaultSprintWeeks(ctx context.Context, projectID int64, userID int64, weeks int) error + AddWorkflowColumn(ctx context.Context, projectID int64, name string) (store.WorkflowColumn, error) + DeleteWorkflowColumn(ctx context.Context, projectID int64, key string) error + UpdateWorkflowColumnName(ctx context.Context, projectID int64, key, newName string) error GetProjectRole(ctx context.Context, projectID int64, userID int64) (store.ProjectRole, error) CheckProjectRole(ctx context.Context, projectID int64, userID int64, requiredRole store.ProjectRole) error ListProjectMembers(ctx context.Context, projectID int64, userID int64) ([]store.ProjectMember, error) @@ -227,20 +230,20 @@ func NewServer(st storeAPI, opts Options) *Server { } return &Server{ - store: st, - logger: logger, - maxBody: maxBody, - mode: mode, - hub: hub, - sink: hub, + store: st, + logger: logger, + maxBody: maxBody, + mode: mode, + hub: hub, + sink: hub, authRateLimit: authRateLimit, encryptionKey: encKey, - passwordResetAdminLimiter: passwordResetAdminLimiter, - webFS: webFS, - fileSrv: http.FileServer(http.FS(webFS)), - indexHTML: indexHTML, - landingHTML: landingHTML, - swJS: swJS, + passwordResetAdminLimiter: passwordResetAdminLimiter, + webFS: webFS, + fileSrv: http.FileServer(http.FS(webFS)), + indexHTML: indexHTML, + landingHTML: landingHTML, + swJS: swJS, } } diff --git a/internal/httpapi/server_test.go b/internal/httpapi/server_test.go index 67177a2..6f865ed 100644 --- a/internal/httpapi/server_test.go +++ b/internal/httpapi/server_test.go @@ -89,6 +89,40 @@ func doJSON(t *testing.T, client *http.Client, method, url string, body any, out return resp, b } +func newCookieClient(t *testing.T) *http.Client { + t.Helper() + jar, err := cookiejar.New(nil) + if err != nil { + t.Fatalf("cookie jar: %v", err) + } + return &http.Client{Jar: jar} +} + +func bootstrapUserClient(t *testing.T, client *http.Client, baseURL, name, email, password string) map[string]any { + t.Helper() + var user map[string]any + resp, body := doJSON(t, client, http.MethodPost, baseURL+"/api/auth/bootstrap", map[string]any{ + "name": name, + "email": email, + "password": password, + }, &user) + if resp.StatusCode != http.StatusCreated { + t.Fatalf("bootstrap status=%d body=%s", resp.StatusCode, string(body)) + } + return user +} + +func loginUserClient(t *testing.T, client *http.Client, baseURL, email, password string) { + t.Helper() + resp, body := doJSON(t, client, http.MethodPost, baseURL+"/api/auth/login", map[string]any{ + "email": email, + "password": password, + }, nil) + if resp.StatusCode != http.StatusOK { + t.Fatalf("login status=%d body=%s", resp.StatusCode, string(body)) + } +} + func TestAPI_CreateMoveAndFetchBoard(t *testing.T) { ts, sqlDB, cleanup := newTestHTTPServer(t, "full") defer cleanup() @@ -163,6 +197,428 @@ func TestAPI_CreateMoveAndFetchBoard(t *testing.T) { } } +func TestRenameLane_RequiresMaintainer(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + ctxOwner := store.WithUserID(context.Background(), ownerID) + project, err := st.CreateProject(ctxOwner, "rename-lane-auth") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + contributor, err := st.CreateUser(context.Background(), "contrib@example.com", "password123", "Contributor") + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := st.AddProjectMember(ctxOwner, ownerID, project.ID, contributor.ID, store.RoleContributor); err != nil { + t.Fatalf("AddProjectMember: %v", err) + } + + contributorClient := newCookieClient(t) + loginUserClient(t, contributorClient, ts.URL, "contrib@example.com", "password123") + + resp, body := doJSON(t, contributorClient, http.MethodPatch, ts.URL+"/api/board/"+project.Slug+"/workflow/"+store.DefaultColumnDoing, map[string]any{ + "name": "Working", + }, nil) + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("expected 403, got %d body=%s", resp.StatusCode, string(body)) + } +} + +func TestRenameLane_NonexistentKeyReturns404(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + project, err := st.CreateProject(store.WithUserID(context.Background(), ownerID), "rename-lane-404") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + resp, body := doJSON(t, ownerClient, http.MethodPatch, ts.URL+"/api/board/"+project.Slug+"/workflow/not_a_lane", map[string]any{ + "name": "Working", + }, nil) + if resp.StatusCode != http.StatusNotFound { + t.Fatalf("expected 404, got %d body=%s", resp.StatusCode, string(body)) + } +} + +func TestRenameLane_EmptyNameRejected(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + project, err := st.CreateProject(store.WithUserID(context.Background(), ownerID), "rename-lane-400") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + tests := []struct { + name string + body map[string]any + }{ + { + name: "WhitespaceOnly", + body: map[string]any{"name": " "}, + }, + { + name: "RejectsColor", + body: map[string]any{"name": "Working", "color": "#123456"}, + }, + { + name: "RejectsKey", + body: map[string]any{"name": "Working", "key": "other"}, + }, + { + name: "RejectsIsDone", + body: map[string]any{"name": "Working", "isDone": true}, + }, + { + name: "RejectsPosition", + body: map[string]any{"name": "Working", "position": 1}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp, body := doJSON(t, ownerClient, http.MethodPatch, ts.URL+"/api/board/"+project.Slug+"/workflow/"+store.DefaultColumnDoing, tc.body, nil) + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("expected 400, got %d body=%s", resp.StatusCode, string(body)) + } + }) + } +} + +func TestRenameLane_BoardAPIReflectsNewName(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + project, err := st.CreateProject(store.WithUserID(context.Background(), ownerID), "rename-lane-board") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + resp, body := doJSON(t, ownerClient, http.MethodPatch, ts.URL+"/api/board/"+project.Slug+"/workflow/"+store.DefaultColumnDoing, map[string]any{ + "name": "Working", + }, nil) + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("rename lane status=%d body=%s", resp.StatusCode, string(body)) + } + + var board struct { + ColumnOrder []struct { + Key string `json:"key"` + Name string `json:"name"` + } `json:"columnOrder"` + } + resp, body = doJSON(t, ownerClient, http.MethodGet, ts.URL+"/api/board/"+project.Slug, nil, &board) + if resp.StatusCode != http.StatusOK { + t.Fatalf("get board status=%d body=%s", resp.StatusCode, string(body)) + } + for _, lane := range board.ColumnOrder { + if lane.Key == store.DefaultColumnDoing { + if lane.Name != "Working" { + t.Fatalf("expected lane name %q, got %q", "Working", lane.Name) + } + return + } + } + t.Fatalf("expected lane %q in board response", store.DefaultColumnDoing) +} + +func TestAddLane_RequiresMaintainer(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + ctxOwner := store.WithUserID(context.Background(), ownerID) + project, err := st.CreateProject(ctxOwner, "add-lane-auth") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + contributor, err := st.CreateUser(context.Background(), "addlane-contrib@example.com", "password123", "Contributor") + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := st.AddProjectMember(ctxOwner, ownerID, project.ID, contributor.ID, store.RoleContributor); err != nil { + t.Fatalf("AddProjectMember: %v", err) + } + + contributorClient := newCookieClient(t) + loginUserClient(t, contributorClient, ts.URL, "addlane-contrib@example.com", "password123") + + resp, body := doJSON(t, contributorClient, http.MethodPost, ts.URL+"/api/board/"+project.Slug+"/workflow", map[string]any{ + "name": "Review", + }, nil) + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("expected 403, got %d body=%s", resp.StatusCode, string(body)) + } +} + +func TestAddLane_InvalidNameRejected(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + project, err := st.CreateProject(store.WithUserID(context.Background(), ownerID), "add-lane-400") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + tests := []struct { + name string + body map[string]any + }{ + {name: "WhitespaceOnly", body: map[string]any{"name": " "}}, + {name: "RejectsKey", body: map[string]any{"name": "Review", "key": "review"}}, + {name: "RejectsIsDone", body: map[string]any{"name": "Review", "isDone": true}}, + {name: "RejectsPosition", body: map[string]any{"name": "Review", "position": 1}}, + {name: "RejectsColor", body: map[string]any{"name": "Review", "color": "#123456"}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp, body := doJSON(t, ownerClient, http.MethodPost, ts.URL+"/api/board/"+project.Slug+"/workflow", tc.body, nil) + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("expected 400, got %d body=%s", resp.StatusCode, string(body)) + } + }) + } +} + +func TestAddLane_BoardShowsNewLane(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + project, err := st.CreateProject(store.WithUserID(context.Background(), ownerID), "add-lane-board") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + var created struct { + Key string `json:"key"` + Name string `json:"name"` + IsDone bool `json:"isDone"` + Position int `json:"position"` + } + resp, body := doJSON(t, ownerClient, http.MethodPost, ts.URL+"/api/board/"+project.Slug+"/workflow", map[string]any{ + "name": "Review", + }, &created) + if resp.StatusCode != http.StatusCreated { + t.Fatalf("add lane status=%d body=%s", resp.StatusCode, string(body)) + } + if created.Key != "review" { + t.Fatalf("expected created key %q, got %+v", "review", created) + } + if created.IsDone { + t.Fatalf("expected created lane to be non-done, got %+v", created) + } + + var board struct { + ColumnOrder []struct { + Key string `json:"key"` + Name string `json:"name"` + IsDone bool `json:"isDone"` + } `json:"columnOrder"` + } + resp, body = doJSON(t, ownerClient, http.MethodGet, ts.URL+"/api/board/"+project.Slug, nil, &board) + if resp.StatusCode != http.StatusOK { + t.Fatalf("get board status=%d body=%s", resp.StatusCode, string(body)) + } + + reviewIdx := -1 + doneIdx := -1 + doneCount := 0 + for i, lane := range board.ColumnOrder { + if lane.Key == created.Key { + reviewIdx = i + if lane.Name != "Review" { + t.Fatalf("expected created lane name %q, got %q", "Review", lane.Name) + } + } + if lane.IsDone { + doneIdx = i + doneCount++ + } + } + if reviewIdx < 0 { + t.Fatalf("expected created lane %q in board response", created.Key) + } + if doneCount != 1 { + t.Fatalf("expected exactly one done lane, got %d", doneCount) + } + if doneIdx < 0 || reviewIdx != doneIdx-1 { + t.Fatalf("expected created lane immediately before done, reviewIdx=%d doneIdx=%d board=%+v", reviewIdx, doneIdx, board.ColumnOrder) + } +} + +func TestAddLane_ResponseAndBoardReflectTrimmedName(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + project, err := st.CreateProject(store.WithUserID(context.Background(), ownerID), "add-lane-trim") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + var created struct { + Key string `json:"key"` + Name string `json:"name"` + } + resp, body := doJSON(t, ownerClient, http.MethodPost, ts.URL+"/api/board/"+project.Slug+"/workflow", map[string]any{ + "name": " QA Gate ", + }, &created) + if resp.StatusCode != http.StatusCreated { + t.Fatalf("add lane status=%d body=%s", resp.StatusCode, string(body)) + } + if created.Key != "qa_gate" { + t.Fatalf("expected key %q, got %+v", "qa_gate", created) + } + if created.Name != "QA Gate" { + t.Fatalf("expected trimmed name %q, got %q", "QA Gate", created.Name) + } + + var board struct { + ColumnOrder []struct { + Key string `json:"key"` + Name string `json:"name"` + } `json:"columnOrder"` + } + resp, body = doJSON(t, ownerClient, http.MethodGet, ts.URL+"/api/board/"+project.Slug, nil, &board) + if resp.StatusCode != http.StatusOK { + t.Fatalf("get board status=%d body=%s", resp.StatusCode, string(body)) + } + for _, lane := range board.ColumnOrder { + if lane.Key == created.Key { + if lane.Name != "QA Gate" { + t.Fatalf("board lane name: want %q, got %q", "QA Gate", lane.Name) + } + return + } + } + t.Fatalf("lane %q missing from board", created.Key) +} + +func TestDeleteLane_RequiresMaintainer(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + ctxOwner := store.WithUserID(context.Background(), ownerID) + project, err := st.CreateProject(ctxOwner, "delete-lane-auth") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + added, err := st.AddWorkflowColumn(ctxOwner, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn: %v", err) + } + + contributor, err := st.CreateUser(context.Background(), "deletelane-contrib@example.com", "password123", "Contributor") + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := st.AddProjectMember(ctxOwner, ownerID, project.ID, contributor.ID, store.RoleContributor); err != nil { + t.Fatalf("AddProjectMember: %v", err) + } + + contributorClient := newCookieClient(t) + loginUserClient(t, contributorClient, ts.URL, "deletelane-contrib@example.com", "password123") + + resp, body := doJSON(t, contributorClient, http.MethodDelete, ts.URL+"/api/board/"+project.Slug+"/workflow/"+added.Key, nil, nil) + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("expected 403, got %d body=%s", resp.StatusCode, string(body)) + } +} + +func TestDeleteLane_BoardNoLongerShowsLane(t *testing.T) { + ts, sqlDB, cleanup := newTestHTTPServer(t, "full") + defer cleanup() + + ownerClient := newCookieClient(t) + owner := bootstrapUserClient(t, ownerClient, ts.URL, "Owner", "owner@example.com", "password123") + ownerID := int64(owner["id"].(float64)) + + st := store.New(sqlDB, nil) + project, err := st.CreateProject(store.WithUserID(context.Background(), ownerID), "delete-lane-board") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + added, err := st.AddWorkflowColumn(store.WithUserID(context.Background(), ownerID), project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn: %v", err) + } + + resp, body := doJSON(t, ownerClient, http.MethodDelete, ts.URL+"/api/board/"+project.Slug+"/workflow/"+added.Key, nil, nil) + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("delete lane status=%d body=%s", resp.StatusCode, string(body)) + } + + var board struct { + ColumnOrder []struct { + Key string `json:"key"` + IsDone bool `json:"isDone"` + } `json:"columnOrder"` + } + resp, body = doJSON(t, ownerClient, http.MethodGet, ts.URL+"/api/board/"+project.Slug, nil, &board) + if resp.StatusCode != http.StatusOK { + t.Fatalf("get board status=%d body=%s", resp.StatusCode, string(body)) + } + doneCount := 0 + for _, lane := range board.ColumnOrder { + if lane.Key == added.Key { + t.Fatalf("expected lane %q to be removed from board", added.Key) + } + if lane.IsDone { + doneCount++ + } + } + if doneCount != 1 { + t.Fatalf("expected exactly one done lane, got %d", doneCount) + } +} + func TestFullMode_MultiProjectBehavior(t *testing.T) { ts, sqlDB, cleanup := newTestHTTPServer(t, "full") defer cleanup() @@ -1721,9 +2177,9 @@ func TestAPI_BoardPagedAndLaneEndpoint(t *testing.T) { // GET /api/board/{slug}?limitPerLane=10 returns columnsMeta var board struct { - Project map[string]any `json:"project"` - Columns map[string][]any `json:"columns"` - ColumnsMeta map[string]map[string]any `json:"columnsMeta"` + Project map[string]any `json:"project"` + Columns map[string][]any `json:"columns"` + ColumnsMeta map[string]map[string]any `json:"columnsMeta"` } resp, _ = doJSON(t, client, http.MethodGet, ts.URL+"/api/board/"+p.Slug+"?limitPerLane=10", nil, &board) if resp.StatusCode != http.StatusOK { diff --git a/internal/httpapi/web/dist/dialogs/settings.js b/internal/httpapi/web/dist/dialogs/settings.js index 248289f..84971d8 100644 --- a/internal/httpapi/web/dist/dialogs/settings.js +++ b/internal/httpapi/web/dist/dialogs/settings.js @@ -575,6 +575,137 @@ function msToDateTimeLocalStr(ms) { const mm = String(d.getMinutes()).padStart(2, "0"); return `${y}-${m}-${day}T${hh}:${mm}`; } +function renderWorkflowTabContent() { + const board = getBoard(); + const workflow = board?.columnOrder ?? []; + const canDeleteAnyLane = workflow.length > 2; + if (!getSlug()) { + return `
No project in context.
`; + } + if (workflow.length === 0) { + return `
Workflow lanes are unavailable.
`; + } + return ` +
+
Workflow
+
Rename lane labels or add a non-done lane inserted immediately before the done lane (whatever its label). Keys stay immutable.
+
+ + +
+
+ ${workflow.map((lane) => ` +
+
${escapeHTML(lane.key)}
+ + + ${lane.isDone ? `` : ``} +
+ `).join("")} +
+
+ `; +} +async function addWorkflowLane(name) { + const slug = getSlug(); + if (!slug) { + showToast("No project available"); + return; + } + const trimmed = name.trim(); + if (!trimmed) { + showToast("Lane name is required"); + return; + } + try { + recordLocalMutation(); + await apiFetch(`/api/board/${slug}/workflow`, { + method: "POST", + body: JSON.stringify({ name: trimmed }), + }); + await invalidateBoard(slug, getTag(), getSearch(), getSprintIdFromUrl()); + await renderSettingsModal(); + showToast("Lane added"); + } + catch (err) { + showToast(err.message || "Failed to add lane"); + } +} +async function renameWorkflowLaneLabel(key, name) { + const slug = getSlug(); + if (!slug) { + showToast("No project available"); + return; + } + const trimmed = name.trim(); + if (!trimmed) { + showToast("Lane name is required"); + return; + } + const currentName = getBoard()?.columnOrder?.find((lane) => lane.key === key)?.name?.trim(); + if (currentName === trimmed) + return; + try { + recordLocalMutation(); + await apiFetch(`/api/board/${slug}/workflow/${encodeURIComponent(key)}`, { + method: "PATCH", + body: JSON.stringify({ name: trimmed }), + }); + await invalidateBoard(slug, getTag(), getSearch(), getSprintIdFromUrl()); + await renderSettingsModal(); + showToast("Lane label updated"); + } + catch (err) { + showToast(err.message || "Failed to update lane label"); + } +} +async function deleteWorkflowLane(key) { + const slug = getSlug(); + if (!slug) { + showToast("No project available"); + return; + } + const lane = getBoard()?.columnOrder?.find((item) => item.key === key); + if (!lane) { + showToast("Lane not found"); + return; + } + if (lane.isDone) { + showToast("Done lane cannot be deleted"); + return; + } + const confirmed = await showConfirmDialog(`Delete lane "${lane.name}"? Only empty non-done lanes can be deleted.`, "Delete lane?", "Delete"); + if (!confirmed) + return; + try { + recordLocalMutation(); + await apiFetch(`/api/board/${slug}/workflow/${encodeURIComponent(key)}`, { + method: "DELETE", + }); + await invalidateBoard(slug, getTag(), getSearch(), getSprintIdFromUrl()); + await renderSettingsModal(); + showToast("Lane deleted"); + } + catch (err) { + showToast(err.message || "Failed to delete lane"); + } +} function computeDefaultSprintStart(now) { const daysToMonday = (now.getDay() + 6) % 7; // 0=Sun, 1=Mon, ..., 6=Sat const monday = new Date(now.getTime()); @@ -759,6 +890,7 @@ export async function renderSettingsModal(options) { } const myMember = currentUser ? boardMembers.find((m) => m.userId === currentUser.id) : null; const showSprintsTab = !!slug && hasProjectAccess && myMember?.role === "maintainer"; + const showWorkflowTab = !!slug && hasProjectAccess && myMember?.role === "maintainer"; // Charts tab only applies in durable project board view (not Dashboard/Projects/Temporary Boards, not anonymous mode, not temporary boards) const board = getBoard(); const isTemporaryBoard = !!(board?.project?.expiresAt); @@ -784,6 +916,9 @@ export async function renderSettingsModal(options) { else if (!showChartsTab && getSettingsActiveTab() === "charts") { setSettingsActiveTab(hasProjectAccess ? "tag-colors" : "customization"); } + else if (!showWorkflowTab && getSettingsActiveTab() === "workflow") { + setSettingsActiveTab(hasProjectAccess ? "tag-colors" : "customization"); + } // Fetch full user profile (including avatar) when Profile tab is shown (skip when re-rendering after avatar change) if (showProfileTab && getUser() && !options?.skipProfileRefetch) { const profileRefetchVersion = ++settingsProfileRefetchVersion; @@ -1090,19 +1225,23 @@ export async function renderSettingsModal(options) { if (showSprintsTab && getSettingsActiveTab() === "sprints") { sprintsHTML = await renderSprintsTabContent(); } + const workflowHTML = showWorkflowTab && getSettingsActiveTab() === "workflow" + ? renderWorkflowTabContent() + : ""; destroyBurndownChart(); contentEl.innerHTML = `
${showProfileTab ? `` : ``} ${showUsersTab ? `` : ``} ${showSprintsTab ? `` : ``} + ${showWorkflowTab ? `` : ``} ${showChartsTab ? `` : ``}
- ${getSettingsActiveTab() === "profile" ? profileHTML : getSettingsActiveTab() === "users" ? usersHTML : getSettingsActiveTab() === "sprints" ? sprintsHTML : getSettingsActiveTab() === "customization" ? customizationHTML : getSettingsActiveTab() === "tag-colors" ? tagColorsContent : getSettingsActiveTab() === "charts" ? chartsContent : getSettingsActiveTab() === "backup" ? renderBackupTabHTML() : ""} + ${getSettingsActiveTab() === "profile" ? profileHTML : getSettingsActiveTab() === "users" ? usersHTML : getSettingsActiveTab() === "sprints" ? sprintsHTML : getSettingsActiveTab() === "workflow" ? workflowHTML : getSettingsActiveTab() === "customization" ? customizationHTML : getSettingsActiveTab() === "tag-colors" ? tagColorsContent : getSettingsActiveTab() === "charts" ? chartsContent : getSettingsActiveTab() === "backup" ? renderBackupTabHTML() : ""}
`; // Abort previous listeners before attaching new ones @@ -1183,6 +1322,59 @@ export async function renderSettingsModal(options) { setupBackupTab(signal); }, 0); } + if (getSettingsActiveTab() === "workflow") { + const addInput = document.querySelector("[data-workflow-new-name]"); + const addLane = () => { + if (!addInput) + return; + addWorkflowLane(addInput.value); + }; + const addBtn = document.querySelector("[data-workflow-add]"); + if (addBtn) { + addBtn.addEventListener("click", addLane, { signal }); + } + if (addInput) { + addInput.addEventListener("keydown", (e) => { + if (e.key !== "Enter") + return; + e.preventDefault(); + addLane(); + }, { signal }); + } + const bindRename = (key) => { + const input = document.querySelector(`[data-workflow-name="${key}"]`); + if (!input) + return; + renameWorkflowLaneLabel(key, input.value); + }; + document.querySelectorAll("[data-workflow-save]").forEach((btn) => { + btn.addEventListener("click", () => { + const key = btn.getAttribute("data-workflow-save"); + if (!key) + return; + bindRename(key); + }, { signal }); + }); + document.querySelectorAll("[data-workflow-name]").forEach((inputEl) => { + inputEl.addEventListener("keydown", (e) => { + if (e.key !== "Enter") + return; + e.preventDefault(); + const key = inputEl.getAttribute("data-workflow-name"); + if (!key) + return; + bindRename(key); + }, { signal }); + }); + document.querySelectorAll("[data-workflow-delete]").forEach((btn) => { + btn.addEventListener("click", () => { + const key = btn.getAttribute("data-workflow-delete"); + if (!key) + return; + deleteWorkflowLane(key); + }, { signal }); + }); + } // Setup logout button — use form POST so browser processes Set-Cookie from document response // (fetch/XHR responses don't always clear cookies reliably across browsers) const logoutBtn = document.getElementById("logoutBtn"); diff --git a/internal/httpapi/web/dist/views/dashboard.js b/internal/httpapi/web/dist/views/dashboard.js index 1be7136..231bb1f 100644 --- a/internal/httpapi/web/dist/views/dashboard.js +++ b/internal/httpapi/web/dist/views/dashboard.js @@ -155,9 +155,10 @@ function renderDashboardContent() { Total assigned ${totalStoryPoints} pts `; + const showLegacyWipSplit = wipInProgressCount > 0 || wipTestingCount > 0; const wipRow = `
WIP - ${typeof wipInProgressCount === 'number' && typeof wipTestingCount === 'number' ? `In progress: ${wipInProgressCount} · Testing: ${wipTestingCount}` : wipCount} + ${showLegacyWipSplit ? `In progress: ${wipInProgressCount} · Testing: ${wipTestingCount}` : wipCount}
`; const oldestWipRow = oldestWip ? `
diff --git a/internal/httpapi/web/modules/dialogs/settings.ts b/internal/httpapi/web/modules/dialogs/settings.ts index b7601d2..d254886 100644 --- a/internal/httpapi/web/modules/dialogs/settings.ts +++ b/internal/httpapi/web/modules/dialogs/settings.ts @@ -656,6 +656,140 @@ function msToDateTimeLocalStr(ms: number): string { return `${y}-${m}-${day}T${hh}:${mm}`; } +function renderWorkflowTabContent(): string { + const board = getBoard(); + const workflow = board?.columnOrder ?? []; + const canDeleteAnyLane = workflow.length > 2; + if (!getSlug()) { + return `
No project in context.
`; + } + if (workflow.length === 0) { + return `
Workflow lanes are unavailable.
`; + } + return ` +
+
Workflow
+
Rename lane labels or add a non-done lane inserted immediately before the done lane (whatever its label). Keys stay immutable.
+
+ + +
+
+ ${workflow.map((lane) => ` +
+
${escapeHTML(lane.key)}
+ + + ${lane.isDone ? `` : ``} +
+ `).join("")} +
+
+ `; +} + +async function addWorkflowLane(name: string): Promise { + const slug = getSlug(); + if (!slug) { + showToast("No project available"); + return; + } + const trimmed = name.trim(); + if (!trimmed) { + showToast("Lane name is required"); + return; + } + try { + recordLocalMutation(); + await apiFetch(`/api/board/${slug}/workflow`, { + method: "POST", + body: JSON.stringify({ name: trimmed }), + }); + await invalidateBoard(slug, getTag(), getSearch(), getSprintIdFromUrl()); + await renderSettingsModal(); + showToast("Lane added"); + } catch (err: any) { + showToast(err.message || "Failed to add lane"); + } +} + +async function renameWorkflowLaneLabel(key: string, name: string): Promise { + const slug = getSlug(); + if (!slug) { + showToast("No project available"); + return; + } + const trimmed = name.trim(); + if (!trimmed) { + showToast("Lane name is required"); + return; + } + const currentName = getBoard()?.columnOrder?.find((lane) => lane.key === key)?.name?.trim(); + if (currentName === trimmed) return; + try { + recordLocalMutation(); + await apiFetch(`/api/board/${slug}/workflow/${encodeURIComponent(key)}`, { + method: "PATCH", + body: JSON.stringify({ name: trimmed }), + }); + await invalidateBoard(slug, getTag(), getSearch(), getSprintIdFromUrl()); + await renderSettingsModal(); + showToast("Lane label updated"); + } catch (err: any) { + showToast(err.message || "Failed to update lane label"); + } +} + +async function deleteWorkflowLane(key: string): Promise { + const slug = getSlug(); + if (!slug) { + showToast("No project available"); + return; + } + const lane = getBoard()?.columnOrder?.find((item) => item.key === key); + if (!lane) { + showToast("Lane not found"); + return; + } + if (lane.isDone) { + showToast("Done lane cannot be deleted"); + return; + } + const confirmed = await showConfirmDialog( + `Delete lane "${lane.name}"? Only empty non-done lanes can be deleted.`, + "Delete lane?", + "Delete" + ); + if (!confirmed) return; + try { + recordLocalMutation(); + await apiFetch(`/api/board/${slug}/workflow/${encodeURIComponent(key)}`, { + method: "DELETE", + }); + await invalidateBoard(slug, getTag(), getSearch(), getSprintIdFromUrl()); + await renderSettingsModal(); + showToast("Lane deleted"); + } catch (err: any) { + showToast(err.message || "Failed to delete lane"); + } +} + function computeDefaultSprintStart(now: Date): Date { const daysToMonday = (now.getDay() + 6) % 7; // 0=Sun, 1=Mon, ..., 6=Sat const monday = new Date(now.getTime()); @@ -844,6 +978,7 @@ export async function renderSettingsModal(options?: { skipProfileRefetch?: boole } const myMember = currentUser ? boardMembers.find((m: any) => m.userId === currentUser.id) : null; const showSprintsTab = !!slug && hasProjectAccess && myMember?.role === "maintainer"; + const showWorkflowTab = !!slug && hasProjectAccess && myMember?.role === "maintainer"; // Charts tab only applies in durable project board view (not Dashboard/Projects/Temporary Boards, not anonymous mode, not temporary boards) const board = getBoard(); @@ -867,6 +1002,8 @@ export async function renderSettingsModal(options?: { skipProfileRefetch?: boole setSettingsActiveTab(hasProjectAccess ? "tag-colors" : "customization"); } else if (!showChartsTab && getSettingsActiveTab() === "charts") { setSettingsActiveTab(hasProjectAccess ? "tag-colors" : "customization"); + } else if (!showWorkflowTab && getSettingsActiveTab() === "workflow") { + setSettingsActiveTab(hasProjectAccess ? "tag-colors" : "customization"); } // Fetch full user profile (including avatar) when Profile tab is shown (skip when re-rendering after avatar change) @@ -1180,6 +1317,9 @@ export async function renderSettingsModal(options?: { skipProfileRefetch?: boole if (showSprintsTab && getSettingsActiveTab() === "sprints") { sprintsHTML = await renderSprintsTabContent(); } + const workflowHTML = showWorkflowTab && getSettingsActiveTab() === "workflow" + ? renderWorkflowTabContent() + : ""; destroyBurndownChart(); contentEl.innerHTML = ` @@ -1187,13 +1327,14 @@ export async function renderSettingsModal(options?: { skipProfileRefetch?: boole ${showProfileTab ? `` : ``} ${showUsersTab ? `` : ``} ${showSprintsTab ? `` : ``} + ${showWorkflowTab ? `` : ``} ${showChartsTab ? `` : ``}
- ${getSettingsActiveTab() === "profile" ? profileHTML : getSettingsActiveTab() === "users" ? usersHTML : getSettingsActiveTab() === "sprints" ? sprintsHTML : getSettingsActiveTab() === "customization" ? customizationHTML : getSettingsActiveTab() === "tag-colors" ? tagColorsContent : getSettingsActiveTab() === "charts" ? chartsContent : getSettingsActiveTab() === "backup" ? renderBackupTabHTML() : ""} + ${getSettingsActiveTab() === "profile" ? profileHTML : getSettingsActiveTab() === "users" ? usersHTML : getSettingsActiveTab() === "sprints" ? sprintsHTML : getSettingsActiveTab() === "workflow" ? workflowHTML : getSettingsActiveTab() === "customization" ? customizationHTML : getSettingsActiveTab() === "tag-colors" ? tagColorsContent : getSettingsActiveTab() === "charts" ? chartsContent : getSettingsActiveTab() === "backup" ? renderBackupTabHTML() : ""}
`; @@ -1282,6 +1423,53 @@ export async function renderSettingsModal(options?: { skipProfileRefetch?: boole }, 0); } + if (getSettingsActiveTab() === "workflow") { + const addInput = document.querySelector("[data-workflow-new-name]") as HTMLInputElement | null; + const addLane = () => { + if (!addInput) return; + addWorkflowLane(addInput.value); + }; + const addBtn = document.querySelector("[data-workflow-add]"); + if (addBtn) { + addBtn.addEventListener("click", addLane, { signal }); + } + if (addInput) { + addInput.addEventListener("keydown", (e) => { + if ((e as KeyboardEvent).key !== "Enter") return; + e.preventDefault(); + addLane(); + }, { signal }); + } + const bindRename = (key: string) => { + const input = document.querySelector(`[data-workflow-name="${key}"]`) as HTMLInputElement | null; + if (!input) return; + renameWorkflowLaneLabel(key, input.value); + }; + document.querySelectorAll("[data-workflow-save]").forEach((btn) => { + btn.addEventListener("click", () => { + const key = (btn as HTMLElement).getAttribute("data-workflow-save"); + if (!key) return; + bindRename(key); + }, { signal }); + }); + document.querySelectorAll("[data-workflow-name]").forEach((inputEl) => { + inputEl.addEventListener("keydown", (e) => { + if ((e as KeyboardEvent).key !== "Enter") return; + e.preventDefault(); + const key = (inputEl as HTMLElement).getAttribute("data-workflow-name"); + if (!key) return; + bindRename(key); + }, { signal }); + }); + document.querySelectorAll("[data-workflow-delete]").forEach((btn) => { + btn.addEventListener("click", () => { + const key = (btn as HTMLElement).getAttribute("data-workflow-delete"); + if (!key) return; + deleteWorkflowLane(key); + }, { signal }); + }); + } + // Setup logout button — use form POST so browser processes Set-Cookie from document response // (fetch/XHR responses don't always clear cookies reliably across browsers) const logoutBtn = document.getElementById("logoutBtn"); diff --git a/internal/httpapi/web/modules/views/dashboard.ts b/internal/httpapi/web/modules/views/dashboard.ts index bf95064..285679e 100644 --- a/internal/httpapi/web/modules/views/dashboard.ts +++ b/internal/httpapi/web/modules/views/dashboard.ts @@ -184,9 +184,10 @@ function renderDashboardContent(): void { ${totalStoryPoints} pts `; + const showLegacyWipSplit = wipInProgressCount > 0 || wipTestingCount > 0; const wipRow = `
WIP - ${typeof wipInProgressCount === 'number' && typeof wipTestingCount === 'number' ? `In progress: ${wipInProgressCount} · Testing: ${wipTestingCount}` : wipCount} + ${showLegacyWipSplit ? `In progress: ${wipInProgressCount} · Testing: ${wipTestingCount}` : wipCount}
`; const oldestWipRow = oldestWip diff --git a/internal/httpapi/web/styles.css b/internal/httpapi/web/styles.css index 757a2eb..8a6c19e 100644 --- a/internal/httpapi/web/styles.css +++ b/internal/httpapi/web/styles.css @@ -1073,7 +1073,7 @@ body { } @media (min-width: 768px) { #settingsDialog.dialog { - width: min(800px, calc(100vw - 48px)); + width: min(960px, calc(100vw - 48px)); } #settingsDialog .dialog__content { overflow-x: hidden; diff --git a/internal/store/dashboard.go b/internal/store/dashboard.go index aef1195..00ff20b 100644 --- a/internal/store/dashboard.go +++ b/internal/store/dashboard.go @@ -3,7 +3,6 @@ package store import ( "context" "database/sql" - "errors" "fmt" "strconv" "strings" @@ -95,6 +94,28 @@ type DashboardSummary struct { OldestWip *OldestWip } +type dashboardWorkflowSemantics struct { + DoneKey string + InProgressKey string + TestingKey string +} + +func buildDashboardWorkflowSemantics(cols []WorkflowColumn) dashboardWorkflowSemantics { + var sem dashboardWorkflowSemantics + for _, col := range cols { + if col.IsDone { + sem.DoneKey = col.Key + } + if !col.IsDone && col.Key == DefaultColumnDoing { + sem.InProgressKey = col.Key + } + if !col.IsDone && col.Key == DefaultColumnTesting { + sem.TestingKey = col.Key + } + } + return sem +} + func (s *Store) GetDashboardSummary(ctx context.Context, userID int64, timezone string) (DashboardSummary, error) { // Assigned count and total points: per project, use sprint filter when ACTIVE sprint exists // We'll compute after we have project list and active sprints @@ -121,9 +142,10 @@ func (s *Store) GetDashboardSummary(ctx context.Context, userID int64, timezone SELECT DISTINCT p.id, p.name, p.slug FROM todos t JOIN projects p ON p.id = t.project_id -WHERE t.assignee_user_id = ? AND t.column_key != ? +JOIN project_workflow_columns wc ON wc.project_id = t.project_id AND wc.key = t.column_key +WHERE t.assignee_user_id = ? AND wc.is_done = 0 ORDER BY p.name -`, userID, DefaultColumnDone) +`, userID) if err != nil { return DashboardSummary{}, fmt.Errorf("list dashboard projects: %w", err) } @@ -147,6 +169,15 @@ ORDER BY p.name return DashboardSummary{}, fmt.Errorf("rows dashboard projects: %w", err) } + workflowByProject, err := s.GetProjectWorkflows(ctx, projectIDs) + if err != nil { + return DashboardSummary{}, fmt.Errorf("get project workflows: %w", err) + } + workflowSemantics := make(map[int64]dashboardWorkflowSemantics, len(workflowByProject)) + for _, projectID := range projectIDs { + workflowSemantics[projectID] = buildDashboardWorkflowSemantics(workflowByProject[projectID]) + } + // Batched active sprints per project (MUST be null when no ACTIVE sprint) activeSprints, err := s.GetActiveSprintsByProjectIDs(ctx, projectIDs) if err != nil { @@ -188,7 +219,7 @@ ORDER BY p.name if len(activeSprintIDs) > 0 { ph := makePlaceholders(len(activeSprintIDs)) - args := []any{userID, DefaultColumnDone} + args := []any{userID} for i := 0; i < 4; i++ { for _, id := range activeSprintIDs { args = append(args, id) @@ -201,8 +232,9 @@ SELECT COALESCE(SUM(CASE WHEN sprint_id IN `+ph+` THEN estimation_points ELSE 0 END), 0), COALESCE(SUM(CASE WHEN sprint_id IS NULL OR sprint_id NOT IN `+ph+` THEN 1 ELSE 0 END), 0), COALESCE(SUM(CASE WHEN sprint_id IS NULL OR sprint_id NOT IN `+ph+` THEN estimation_points ELSE 0 END), 0) -FROM todos -WHERE assignee_user_id = ? AND column_key != ? +FROM todos t +JOIN project_workflow_columns wc ON wc.project_id = t.project_id AND wc.key = t.column_key +WHERE assignee_user_id = ? AND wc.is_done = 0 `, args...).Scan(&sprintCount, &spPts, &backlogCount, &blPts); err != nil { return DashboardSummary{}, fmt.Errorf("assigned split: %w", err) } @@ -217,8 +249,9 @@ WHERE assignee_user_id = ? AND column_key != ? var pUnsched sql.NullInt64 if err := s.db.QueryRowContext(ctx, ` SELECT COUNT(*), COALESCE(SUM(estimation_points), 0) -FROM todos -WHERE assignee_user_id = ? AND column_key != ?`, userID, DefaultColumnDone).Scan(&backlogCount, &pUnsched); err != nil { +FROM todos t +JOIN project_workflow_columns wc ON wc.project_id = t.project_id AND wc.key = t.column_key +WHERE assignee_user_id = ? AND wc.is_done = 0`, userID).Scan(&backlogCount, &pUnsched); err != nil { return DashboardSummary{}, fmt.Errorf("count assigned: %w", err) } if pUnsched.Valid { @@ -247,12 +280,13 @@ WHERE assignee_user_id = ? AND column_key != ?`, userID, DefaultColumnDone).Scan for _, id := range activeSprintIDs { args = append(args, id) } - args = append(args, userID, DefaultColumnDone) + args = append(args, userID) if err := s.db.QueryRowContext(ctx, ` SELECT COALESCE(SUM(t.estimation_points), 0), COUNT(*) FROM todos t +JOIN project_workflow_columns done_wc ON done_wc.project_id = t.project_id AND done_wc.key = t.column_key AND done_wc.is_done = 1 JOIN sprints s ON s.id = t.sprint_id AND s.state = ? AND s.id IN `+ph+` -WHERE t.assignee_user_id = ? AND t.column_key = ? +WHERE t.assignee_user_id = ? AND s.started_at IS NOT NULL AND t.done_at >= s.started_at AND t.done_at < COALESCE(s.closed_at, s.planned_end_at) `, args...).Scan(&pointsCompleted, &storiesCompleted); err != nil { @@ -271,7 +305,7 @@ WHERE t.assignee_user_id = ? AND t.column_key = ? } if len(noSprintIDs) > 0 { placeholders := makePlaceholders(len(noSprintIDs)) - args := []any{userID, DefaultColumnDone, weekStartMs, weekEndMs} + args := []any{userID, weekStartMs, weekEndMs} for _, pid := range noSprintIDs { args = append(args, pid) } @@ -279,9 +313,10 @@ WHERE t.assignee_user_id = ? AND t.column_key = ? var c int if err := s.db.QueryRowContext(ctx, ` SELECT COALESCE(SUM(estimation_points), 0), COUNT(*) -FROM todos -WHERE assignee_user_id = ? AND column_key = ? AND done_at >= ? AND done_at < ? - AND project_id IN `+placeholders, +FROM todos t +JOIN project_workflow_columns done_wc ON done_wc.project_id = t.project_id AND done_wc.key = t.column_key AND done_wc.is_done = 1 +WHERE assignee_user_id = ? AND done_at >= ? AND done_at < ? + AND t.project_id IN `+placeholders, args...).Scan(&p, &c); err != nil { return DashboardSummary{}, fmt.Errorf("points completed this week (no sprint): %w", err) } @@ -294,9 +329,10 @@ WHERE assignee_user_id = ? AND column_key = ? AND done_at >= ? AND done_at < ? if len(projectIDsWithActive) == 0 { if err := s.db.QueryRowContext(ctx, ` SELECT COALESCE(SUM(estimation_points), 0), COUNT(*) -FROM todos -WHERE assignee_user_id = ? AND column_key = ? AND done_at >= ? AND done_at < ? -`, userID, DefaultColumnDone, weekStartMs, weekEndMs).Scan(&pointsCompleted, &storiesCompleted); err != nil { +FROM todos t +JOIN project_workflow_columns done_wc ON done_wc.project_id = t.project_id AND done_wc.key = t.column_key AND done_wc.is_done = 1 +WHERE assignee_user_id = ? AND done_at >= ? AND done_at < ? +`, userID, weekStartMs, weekEndMs).Scan(&pointsCompleted, &storiesCompleted); err != nil { return DashboardSummary{}, fmt.Errorf("points completed this week: %w", err) } } @@ -320,7 +356,7 @@ WHERE assignee_user_id = ? AND column_key = ? AND done_at >= ? AND done_at < ? var totalPoints, donePoints int64 var totalAll, doneCountAll int var totalPointsAll, donePointsAll int64 - doneCond := `t.column_key = 'done' AND s.started_at IS NOT NULL AND t.done_at >= s.started_at AND t.done_at < COALESCE(s.closed_at, s.planned_end_at)` + doneCond := `done_wc.id IS NOT NULL AND s.started_at IS NOT NULL AND t.done_at >= s.started_at AND t.done_at < COALESCE(s.closed_at, s.planned_end_at)` if err := s.db.QueryRowContext(ctx, ` SELECT COALESCE(SUM(CASE WHEN t.assignee_user_id = ? THEN 1 ELSE 0 END), 0), @@ -332,6 +368,7 @@ SELECT COALESCE(SUM(CASE WHEN `+doneCond+` THEN 1 ELSE 0 END), 0), COALESCE(SUM(CASE WHEN `+doneCond+` THEN t.estimation_points ELSE 0 END), 0) FROM todos t +LEFT JOIN project_workflow_columns done_wc ON done_wc.project_id = t.project_id AND done_wc.key = t.column_key AND done_wc.is_done = 1 JOIN sprints s ON s.id = t.sprint_id AND s.state = ? AND s.id IN `+ph+` WHERE t.sprint_id IN `+ph, args...).Scan(&total, &totalPoints, &doneCount, &donePoints, &totalAll, &totalPointsAll, &doneCountAll, &donePointsAll); err != nil { @@ -351,17 +388,61 @@ WHERE t.sprint_id IN `+ph, } } - var wipInProgressCount, wipTestingCount int - if err := s.db.QueryRowContext(ctx, ` -SELECT - COALESCE(SUM(CASE WHEN column_key = ? THEN 1 ELSE 0 END), 0), - COALESCE(SUM(CASE WHEN column_key = ? THEN 1 ELSE 0 END), 0) -FROM todos -WHERE assignee_user_id = ? AND column_key IN (?, ?) -`, DefaultColumnDoing, DefaultColumnTesting, userID, DefaultColumnDoing, DefaultColumnTesting).Scan(&wipInProgressCount, &wipTestingCount); err != nil { - return DashboardSummary{}, fmt.Errorf("wip split: %w", err) + var wipCount, wipInProgressCount, wipTestingCount int + wipRows, err := s.db.QueryContext(ctx, ` +SELECT t.project_id, t.column_key, t.local_id, t.title, t.updated_at, p.name, p.slug +FROM todos t +JOIN projects p ON p.id = t.project_id +JOIN project_workflow_columns wc ON wc.project_id = t.project_id AND wc.key = t.column_key +WHERE t.assignee_user_id = ? AND wc.is_done = 0 +`, userID) + if err != nil { + return DashboardSummary{}, fmt.Errorf("wip rows: %w", err) + } + defer wipRows.Close() + var oldestWip *OldestWip + var oldestUpdatedAtMs int64 + nowMs := time.Now().UTC().UnixMilli() + for wipRows.Next() { + var projectID int64 + var columnKey string + var localID int64 + var title string + var updatedAtMs int64 + var projectName string + var projectSlug string + var projectSlugNull sql.NullString + if err := wipRows.Scan(&projectID, &columnKey, &localID, &title, &updatedAtMs, &projectName, &projectSlugNull); err != nil { + return DashboardSummary{}, fmt.Errorf("scan wip row: %w", err) + } + if projectSlugNull.Valid { + projectSlug = projectSlugNull.String + } + wipCount++ + sem := workflowSemantics[projectID] + isInProgress := sem.InProgressKey != "" && columnKey == sem.InProgressKey + isTesting := sem.TestingKey != "" && columnKey == sem.TestingKey + if isInProgress { + wipInProgressCount++ + } + if isTesting { + wipTestingCount++ + } + if oldestWip == nil || updatedAtMs < oldestUpdatedAtMs { + oldestUpdatedAtMs = updatedAtMs + ageDays := int((nowMs - updatedAtMs) / 86400000) + oldestWip = &OldestWip{ + LocalID: localID, + Title: title, + AgeDays: ageDays, + ProjectName: projectName, + ProjectSlug: projectSlug, + } + } + } + if err := wipRows.Err(); err != nil { + return DashboardSummary{}, fmt.Errorf("rows wip: %w", err) } - wipCount := wipInProgressCount + wipTestingCount // Weekly throughput: last 4 full weeks + current partial week (5 buckets). // Query once for the full range and bucket by week index to minimize DB round-trips. @@ -384,8 +465,9 @@ SELECT CAST((done_at - ?) / ? AS INTEGER) AS week_bucket, COUNT(*) AS stories, COALESCE(SUM(estimation_points), 0) AS points -FROM todos -WHERE assignee_user_id = ? AND column_key = 'done' +FROM todos t +JOIN project_workflow_columns done_wc ON done_wc.project_id = t.project_id AND done_wc.key = t.column_key AND done_wc.is_done = 1 +WHERE assignee_user_id = ? AND done_at >= ? AND done_at < ? GROUP BY week_bucket ORDER BY week_bucket ASC @@ -424,8 +506,9 @@ ORDER BY week_bucket ASC if err := s.db.QueryRowContext(ctx, ` SELECT AVG((t.done_at - t.created_at) / 86400000.0) FROM todos t +JOIN project_workflow_columns done_wc ON done_wc.project_id = t.project_id AND done_wc.key = t.column_key AND done_wc.is_done = 1 JOIN sprints s ON s.id = t.sprint_id AND s.state = ? AND s.id IN `+ph+` -WHERE t.assignee_user_id = ? AND t.column_key = 'done' +WHERE t.assignee_user_id = ? AND s.started_at IS NOT NULL AND t.done_at >= s.started_at AND t.done_at < COALESCE(s.closed_at, s.planned_end_at) `, args...).Scan(&avg); err != nil { @@ -439,8 +522,9 @@ WHERE t.assignee_user_id = ? AND t.column_key = 'done' var avg sql.NullFloat64 if err := s.db.QueryRowContext(ctx, ` SELECT AVG((done_at - created_at) / 86400000.0) -FROM todos -WHERE assignee_user_id = ? AND column_key = 'done' AND done_at >= ? +FROM todos t +JOIN project_workflow_columns done_wc ON done_wc.project_id = t.project_id AND done_wc.key = t.column_key AND done_wc.is_done = 1 +WHERE assignee_user_id = ? AND done_at >= ? `, userID, thirtyDaysAgo).Scan(&avg); err != nil { return DashboardSummary{}, fmt.Errorf("avg lead time fallback: %w", err) } @@ -449,39 +533,6 @@ WHERE assignee_user_id = ? AND column_key = 'done' AND done_at >= ? } } - // Aging WIP: fetch oldest updated WIP directly with ORDER BY/LIMIT. - var oldestWip *OldestWip - var localID sql.NullInt64 - var title, projectName, projectSlug string - var projectSlugNull sql.NullString - var updatedAtMs int64 - if err := s.db.QueryRowContext(ctx, ` -SELECT t.local_id, t.title, t.updated_at, p.name, p.slug -FROM todos t -JOIN projects p ON p.id = t.project_id -WHERE t.assignee_user_id = ? AND t.column_key IN ('doing', 'testing') -ORDER BY t.updated_at ASC -LIMIT 1 -`, userID).Scan(&localID, &title, &updatedAtMs, &projectName, &projectSlugNull); err == nil { - if projectSlugNull.Valid { - projectSlug = projectSlugNull.String - } - ageDays := int((time.Now().UTC().UnixMilli() - updatedAtMs) / 86400000) - locID := int64(0) - if localID.Valid { - locID = localID.Int64 - } - oldestWip = &OldestWip{ - LocalID: locID, - Title: title, - AgeDays: ageDays, - ProjectName: projectName, - ProjectSlug: projectSlug, - } - } else if !errors.Is(err, sql.ErrNoRows) { - return DashboardSummary{}, fmt.Errorf("oldest wip: %w", err) - } - return DashboardSummary{ AssignedCount: count, TotalAssignedStoryPoints: points, @@ -588,12 +639,12 @@ SELECT t.id, t.local_id, t.title, t.project_id, t.column_key, t.updated_at, t.es wc.name AS status_name, wc.color AS status_color FROM todos t JOIN projects p ON p.id = t.project_id -LEFT JOIN project_workflow_columns wc ON wc.project_id = t.project_id AND wc.key = t.column_key -WHERE t.assignee_user_id = ? AND t.column_key != ? +JOIN project_workflow_columns wc ON wc.project_id = t.project_id AND wc.key = t.column_key +WHERE t.assignee_user_id = ? AND wc.is_done = 0 AND (t.updated_at < ? OR (t.updated_at = ? AND t.id < ?)) ORDER BY t.updated_at DESC, t.id DESC LIMIT ? -`, userID, DefaultColumnDone, updatedAtCursor, updatedAtCursor, idCursor, limit+1) +`, userID, updatedAtCursor, updatedAtCursor, idCursor, limit+1) } else { rows, err = s.db.QueryContext(ctx, ` SELECT t.id, t.local_id, t.title, t.project_id, t.column_key, t.updated_at, t.estimation_points, t.sprint_id, @@ -601,11 +652,11 @@ SELECT t.id, t.local_id, t.title, t.project_id, t.column_key, t.updated_at, t.es wc.name AS status_name, wc.color AS status_color FROM todos t JOIN projects p ON p.id = t.project_id -LEFT JOIN project_workflow_columns wc ON wc.project_id = t.project_id AND wc.key = t.column_key -WHERE t.assignee_user_id = ? AND t.column_key != ? +JOIN project_workflow_columns wc ON wc.project_id = t.project_id AND wc.key = t.column_key +WHERE t.assignee_user_id = ? AND wc.is_done = 0 ORDER BY t.updated_at DESC, t.id DESC LIMIT ? -`, userID, DefaultColumnDone, limit+1) +`, userID, limit+1) } if err != nil { return nil, nil, fmt.Errorf("list dashboard todos: %w", err) diff --git a/internal/store/dashboard_test.go b/internal/store/dashboard_test.go new file mode 100644 index 0000000..11034a8 --- /dev/null +++ b/internal/store/dashboard_test.go @@ -0,0 +1,278 @@ +package store + +import ( + "context" + "testing" + "time" +) + +func dashboardTestContext(t *testing.T, st *Store) (context.Context, User) { + t.Helper() + ctx := context.Background() + user, err := st.BootstrapUser(ctx, "dashboard@example.com", "password123", "Dashboard User") + if err != nil { + t.Fatalf("BootstrapUser: %v", err) + } + return WithUserID(ctx, user.ID), user +} + +func setTodoTimes(t *testing.T, st *Store, todoID int64, createdAt, updatedAt time.Time) { + t.Helper() + if _, err := st.db.Exec(` +UPDATE todos +SET created_at = ?, updated_at = ? +WHERE id = ?`, createdAt.UnixMilli(), updatedAt.UnixMilli(), todoID); err != nil { + t.Fatalf("set todo times: %v", err) + } +} + +func TestDashboardSummary_CustomDoneKey(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx, user := dashboardTestContext(t, st) + + customProject, err := st.CreateProjectWithWorkflow(ctx, "Custom Done", []WorkflowColumn{ + {Key: "backlog_custom", Name: "Backlog", Color: "#9CA3AF", Position: 0}, + {Key: "build_custom", Name: "Build", Color: "#10B981", Position: 1}, + {Key: "verify_custom", Name: "Verify", Color: "#3B82F6", Position: 2}, + {Key: "shipped_custom", Name: "Shipped", Color: "#EF4444", Position: 3, IsDone: true}, + }) + if err != nil { + t.Fatalf("CreateProjectWithWorkflow custom: %v", err) + } + defaultProject, err := st.CreateProject(ctx, "Default Done") + if err != nil { + t.Fatalf("CreateProject default: %v", err) + } + + pointsCustom := int64(3) + customTodo, err := st.CreateTodo(ctx, customProject.ID, CreateTodoInput{ + Title: "Custom done todo", + ColumnKey: "build_custom", + AssigneeUserID: &user.ID, + EstimationPoints: &pointsCustom, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo custom: %v", err) + } + setTodoTimes(t, st, customTodo.ID, time.Now().UTC().Add(-48*time.Hour), time.Now().UTC().Add(-48*time.Hour)) + if _, err := st.MoveTodo(ctx, customTodo.ID, "shipped_custom", nil, nil, ModeFull); err != nil { + t.Fatalf("MoveTodo custom done: %v", err) + } + + pointsDefault := int64(5) + defaultTodo, err := st.CreateTodo(ctx, defaultProject.ID, CreateTodoInput{ + Title: "Default done todo", + ColumnKey: DefaultColumnDoing, + AssigneeUserID: &user.ID, + EstimationPoints: &pointsDefault, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo default: %v", err) + } + setTodoTimes(t, st, defaultTodo.ID, time.Now().UTC().Add(-96*time.Hour), time.Now().UTC().Add(-96*time.Hour)) + if _, err := st.MoveTodo(ctx, defaultTodo.ID, DefaultColumnDone, nil, nil, ModeFull); err != nil { + t.Fatalf("MoveTodo default done: %v", err) + } + + summary, err := st.GetDashboardSummary(ctx, user.ID, "UTC") + if err != nil { + t.Fatalf("GetDashboardSummary: %v", err) + } + + if summary.StoriesCompletedThisWeek != 2 { + t.Fatalf("expected 2 stories completed this week, got %d", summary.StoriesCompletedThisWeek) + } + if summary.PointsCompletedThisWeek != 8 { + t.Fatalf("expected 8 points completed this week, got %d", summary.PointsCompletedThisWeek) + } + if len(summary.WeeklyThroughput) != 5 { + t.Fatalf("expected 5 weekly throughput buckets, got %d", len(summary.WeeklyThroughput)) + } + lastWeek := summary.WeeklyThroughput[len(summary.WeeklyThroughput)-1] + if lastWeek.Stories != 2 || lastWeek.Points != 8 { + t.Fatalf("expected current throughput bucket stories=2 points=8, got %+v", lastWeek) + } + if summary.AvgLeadTimeDays == nil { + t.Fatal("expected avg lead time for completed work") + } + if *summary.AvgLeadTimeDays < 2.5 || *summary.AvgLeadTimeDays > 3.5 { + t.Fatalf("expected avg lead time near 3 days, got %.2f", *summary.AvgLeadTimeDays) + } +} + +func TestDashboardSummary_CustomWIPKeys_AllNonDoneCountAsWipWithoutLegacySplit(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx, user := dashboardTestContext(t, st) + + project, err := st.CreateProjectWithWorkflow(ctx, "Custom WIP", []WorkflowColumn{ + {Key: "intake_custom", Name: "Intake", Color: "#9CA3AF", Position: 0}, + {Key: "build_custom", Name: "Build", Color: "#10B981", Position: 1}, + {Key: "verify_custom", Name: "Verify", Color: "#3B82F6", Position: 2}, + {Key: "shipped_custom", Name: "Shipped", Color: "#EF4444", Position: 3, IsDone: true}, + }) + if err != nil { + t.Fatalf("CreateProjectWithWorkflow: %v", err) + } + + intakeTodo, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Intake todo", + ColumnKey: "intake_custom", + AssigneeUserID: &user.ID, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo intake: %v", err) + } + buildTodo, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Build todo", + ColumnKey: "build_custom", + AssigneeUserID: &user.ID, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo build: %v", err) + } + verifyTodo, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Verify todo", + ColumnKey: "verify_custom", + AssigneeUserID: &user.ID, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo verify: %v", err) + } + + now := time.Now().UTC() + setTodoTimes(t, st, intakeTodo.ID, now.Add(-10*24*time.Hour), now.Add(-10*24*time.Hour)) + setTodoTimes(t, st, buildTodo.ID, now.Add(-5*24*time.Hour), now.Add(-3*24*time.Hour)) + setTodoTimes(t, st, verifyTodo.ID, now.Add(-4*24*time.Hour), now.Add(-2*24*time.Hour)) + + summary, err := st.GetDashboardSummary(ctx, user.ID, "UTC") + if err != nil { + t.Fatalf("GetDashboardSummary: %v", err) + } + + if summary.WipCount != 3 { + t.Fatalf("expected WIP count 3 for all non-done assigned work, got %d", summary.WipCount) + } + if summary.WipInProgressCount != 0 { + t.Fatalf("expected in-progress WIP split 0 without explicit %q lane, got %d", DefaultColumnDoing, summary.WipInProgressCount) + } + if summary.WipTestingCount != 0 { + t.Fatalf("expected testing WIP split 0 without explicit %q lane, got %d", DefaultColumnTesting, summary.WipTestingCount) + } + if summary.OldestWip == nil { + t.Fatal("expected oldest WIP") + } + if summary.OldestWip.LocalID != intakeTodo.LocalID { + t.Fatalf("expected oldest WIP local ID %d, got %+v", intakeTodo.LocalID, summary.OldestWip) + } + if summary.OldestWip.Title != intakeTodo.Title { + t.Fatalf("expected oldest WIP title %q, got %+v", intakeTodo.Title, summary.OldestWip) + } +} + +func TestDashboardSummary_DefaultWIPKeys_PreserveLegacySplit(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx, user := dashboardTestContext(t, st) + + project, err := st.CreateProject(ctx, "Default WIP") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + doingTodo, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Doing todo", + ColumnKey: DefaultColumnDoing, + AssigneeUserID: &user.ID, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo doing: %v", err) + } + testingTodo, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Testing todo", + ColumnKey: DefaultColumnTesting, + AssigneeUserID: &user.ID, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo testing: %v", err) + } + + now := time.Now().UTC() + setTodoTimes(t, st, doingTodo.ID, now.Add(-5*24*time.Hour), now.Add(-5*24*time.Hour)) + setTodoTimes(t, st, testingTodo.ID, now.Add(-2*24*time.Hour), now.Add(-2*24*time.Hour)) + + summary, err := st.GetDashboardSummary(ctx, user.ID, "UTC") + if err != nil { + t.Fatalf("GetDashboardSummary: %v", err) + } + + if summary.WipCount != 2 { + t.Fatalf("expected WIP count 2, got %d", summary.WipCount) + } + if summary.WipInProgressCount != 1 { + t.Fatalf("expected in-progress WIP count 1, got %d", summary.WipInProgressCount) + } + if summary.WipTestingCount != 1 { + t.Fatalf("expected testing WIP count 1, got %d", summary.WipTestingCount) + } + if summary.OldestWip == nil { + t.Fatal("expected oldest WIP") + } + if summary.OldestWip.LocalID != doingTodo.LocalID { + t.Fatalf("expected oldest WIP local ID %d, got %+v", doingTodo.LocalID, summary.OldestWip) + } +} + +func TestListDashboardTodos_CustomDoneKeyExcludesDone(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx, user := dashboardTestContext(t, st) + + project, err := st.CreateProjectWithWorkflow(ctx, "Dashboard Todos", []WorkflowColumn{ + {Key: "backlog_custom", Name: "Backlog", Color: "#9CA3AF", Position: 0}, + {Key: "build_custom", Name: "Build", Color: "#10B981", Position: 1}, + {Key: "shipped_custom", Name: "Shipped", Color: "#EF4444", Position: 2, IsDone: true}, + }) + if err != nil { + t.Fatalf("CreateProjectWithWorkflow: %v", err) + } + + openTodo, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Open todo", + ColumnKey: "build_custom", + AssigneeUserID: &user.ID, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo open: %v", err) + } + doneTodo, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Done todo", + ColumnKey: "build_custom", + AssigneeUserID: &user.ID, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo done: %v", err) + } + if _, err := st.MoveTodo(ctx, doneTodo.ID, "shipped_custom", nil, nil, ModeFull); err != nil { + t.Fatalf("MoveTodo done: %v", err) + } + + items, _, err := st.ListDashboardTodos(ctx, user.ID, 10, nil) + if err != nil { + t.Fatalf("ListDashboardTodos: %v", err) + } + if len(items) != 1 { + t.Fatalf("expected 1 dashboard todo, got %d", len(items)) + } + if items[0].ID != openTodo.ID { + t.Fatalf("expected open todo ID %d, got %+v", openTodo.ID, items) + } + if items[0].StatusName != "Build" { + t.Fatalf("expected workflow status name Build, got %+v", items[0]) + } +} diff --git a/internal/store/workflow_add_test.go b/internal/store/workflow_add_test.go new file mode 100644 index 0000000..1bf6686 --- /dev/null +++ b/internal/store/workflow_add_test.go @@ -0,0 +1,243 @@ +package store + +import ( + "context" + "errors" + "fmt" + "testing" +) + +func TestAddLane_InsertsBeforeDone(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-add") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + added, err := st.AddWorkflowColumn(ctx, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn: %v", err) + } + + workflow, err := st.GetProjectWorkflow(ctx, project.ID) + if err != nil { + t.Fatalf("GetProjectWorkflow: %v", err) + } + want := []string{ + DefaultColumnBacklog, + DefaultColumnNotStarted, + DefaultColumnDoing, + DefaultColumnTesting, + "review", + DefaultColumnDone, + } + if len(workflow) != len(want) { + t.Fatalf("expected %d workflow columns, got %d", len(want), len(workflow)) + } + for i, col := range workflow { + if col.Key != want[i] { + t.Fatalf("expected lane %d key %q, got %q", i, want[i], col.Key) + } + if col.Position != i { + t.Fatalf("expected lane %q position %d, got %d", col.Key, i, col.Position) + } + } + if added.Key != "review" { + t.Fatalf("expected generated key %q, got %q", "review", added.Key) + } + if added.Position != len(workflow)-2 { + t.Fatalf("expected inserted position %d, got %d", len(workflow)-2, added.Position) + } +} + +func TestAddLane_StoredNameIsTrimmed(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-add-trim") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + raw := "\t Ship Ready \n" + added, err := st.AddWorkflowColumn(ctx, project.ID, raw) + if err != nil { + t.Fatalf("AddWorkflowColumn: %v", err) + } + wantName := "Ship Ready" + if added.Name != wantName { + t.Fatalf("returned name: want %q, got %q", wantName, added.Name) + } + + workflow, err := st.GetProjectWorkflow(ctx, project.ID) + if err != nil { + t.Fatalf("GetProjectWorkflow: %v", err) + } + var got string + for _, col := range workflow { + if col.Key == added.Key { + got = col.Name + break + } + } + if got != wantName { + t.Fatalf("stored name: want %q, got %q", wantName, got) + } +} + +func TestAddLane_ServerGeneratedKeyUniqueWhenNamesCollide(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-add-unique") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + first, err := st.AddWorkflowColumn(ctx, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn first: %v", err) + } + second, err := st.AddWorkflowColumn(ctx, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn second: %v", err) + } + + if first.Key != "review" { + t.Fatalf("expected first key %q, got %q", "review", first.Key) + } + if second.Key != "review_2" { + t.Fatalf("expected second key %q, got %q", "review_2", second.Key) + } + if first.Key == second.Key { + t.Fatalf("expected unique generated keys, both were %q", first.Key) + } +} + +func TestAddLane_InvalidNameRejected(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-add-invalid") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + tests := []struct { + name string + laneName string + }{ + {name: "WhitespaceOnly", laneName: " "}, + {name: "TooLong", laneName: string(make([]byte, 201))}, + } + tests[1].laneName = "" + for i := 0; i < 201; i++ { + tests[1].laneName += "a" + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := st.AddWorkflowColumn(ctx, project.ID, tc.laneName) + if !errors.Is(err, ErrValidation) { + t.Fatalf("expected ErrValidation, got %v", err) + } + }) + } +} + +func TestAddLane_MaxLanesEnforced(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-add-max") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + for i := 0; i < maxWorkflowColumns-5; i++ { + if _, err := st.AddWorkflowColumn(ctx, project.ID, fmt.Sprintf("Lane %d", i+1)); err != nil { + t.Fatalf("AddWorkflowColumn %d: %v", i+1, err) + } + } + + if _, err := st.AddWorkflowColumn(ctx, project.ID, "Overflow"); !errors.Is(err, ErrValidation) { + t.Fatalf("expected ErrValidation when exceeding max lanes, got %v", err) + } +} + +func TestAddLane_IsDoneFalseEnforced(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-add-isdone") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + added, err := st.AddWorkflowColumn(ctx, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn: %v", err) + } + if added.IsDone { + t.Fatalf("expected added lane to be non-done") + } + + workflow, err := st.GetProjectWorkflow(ctx, project.ID) + if err != nil { + t.Fatalf("GetProjectWorkflow: %v", err) + } + for _, col := range workflow { + if col.Key == added.Key && col.IsDone { + t.Fatalf("expected added lane %q to remain non-done", added.Key) + } + } +} + +func TestAddLane_PreservesSingleDoneLane(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProjectWithWorkflow(ctx, "workflow-add-custom-done", []WorkflowColumn{ + {Key: "backlog_custom", Name: "Backlog", Color: "#9CA3AF", Position: 0}, + {Key: "build_custom", Name: "Build", Color: "#10B981", Position: 1}, + {Key: "shipped_custom", Name: "Shipped", Color: "#EF4444", Position: 2, IsDone: true}, + }) + if err != nil { + t.Fatalf("CreateProjectWithWorkflow: %v", err) + } + + added, err := st.AddWorkflowColumn(ctx, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn: %v", err) + } + + workflow, err := st.GetProjectWorkflow(ctx, project.ID) + if err != nil { + t.Fatalf("GetProjectWorkflow: %v", err) + } + + doneCount := 0 + for _, col := range workflow { + if col.IsDone { + doneCount++ + if col.Key != "shipped_custom" { + t.Fatalf("expected done lane key to remain %q, got %q", "shipped_custom", col.Key) + } + } + } + if doneCount != 1 { + t.Fatalf("expected exactly one done lane, got %d", doneCount) + } + if workflow[len(workflow)-2].Key != added.Key || workflow[len(workflow)-1].Key != "shipped_custom" { + t.Fatalf("expected added lane before done, got workflow=%+v", workflow) + } +} diff --git a/internal/store/workflow_delete_test.go b/internal/store/workflow_delete_test.go new file mode 100644 index 0000000..2e9a96d --- /dev/null +++ b/internal/store/workflow_delete_test.go @@ -0,0 +1,180 @@ +package store + +import ( + "context" + "errors" + "testing" +) + +func TestDeleteLane_EmptyNonDoneLaneRemoved(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-delete-empty") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + added, err := st.AddWorkflowColumn(ctx, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn: %v", err) + } + + if err := st.DeleteWorkflowColumn(ctx, project.ID, added.Key); err != nil { + t.Fatalf("DeleteWorkflowColumn: %v", err) + } + + workflow, err := st.GetProjectWorkflow(ctx, project.ID) + if err != nil { + t.Fatalf("GetProjectWorkflow: %v", err) + } + for _, col := range workflow { + if col.Key == added.Key { + t.Fatalf("expected lane %q to be deleted", added.Key) + } + } +} + +func TestDeleteLane_NonEmptyLaneRejected(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-delete-nonempty") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + added, err := st.AddWorkflowColumn(ctx, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn: %v", err) + } + if _, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Review me", + ColumnKey: added.Key, + }, ModeAnonymous); err != nil { + t.Fatalf("CreateTodo: %v", err) + } + + if err := st.DeleteWorkflowColumn(ctx, project.ID, added.Key); !errors.Is(err, ErrConflict) { + t.Fatalf("expected ErrConflict, got %v", err) + } +} + +func TestDeleteLane_DoneLaneRejected(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-delete-done") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + + if err := st.DeleteWorkflowColumn(ctx, project.ID, DefaultColumnDone); !errors.Is(err, ErrValidation) { + t.Fatalf("expected ErrValidation, got %v", err) + } +} + +func TestDeleteLane_MinLanesEnforced(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProjectWithWorkflow(ctx, "workflow-delete-min", []WorkflowColumn{ + {Key: "todo_custom", Name: "Todo", Color: "#9CA3AF", Position: 0}, + {Key: "done_custom", Name: "Done", Color: "#EF4444", Position: 1, IsDone: true}, + }) + if err != nil { + t.Fatalf("CreateProjectWithWorkflow: %v", err) + } + + if err := st.DeleteWorkflowColumn(ctx, project.ID, "todo_custom"); !errors.Is(err, ErrValidation) { + t.Fatalf("expected ErrValidation, got %v", err) + } +} + +func TestDeleteLane_PositionsReindexed(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-delete-positions") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + first, err := st.AddWorkflowColumn(ctx, project.ID, "Review") + if err != nil { + t.Fatalf("AddWorkflowColumn first: %v", err) + } + second, err := st.AddWorkflowColumn(ctx, project.ID, "QA") + if err != nil { + t.Fatalf("AddWorkflowColumn second: %v", err) + } + + if err := st.DeleteWorkflowColumn(ctx, project.ID, first.Key); err != nil { + t.Fatalf("DeleteWorkflowColumn: %v", err) + } + + workflow, err := st.GetProjectWorkflow(ctx, project.ID) + if err != nil { + t.Fatalf("GetProjectWorkflow: %v", err) + } + want := []string{ + DefaultColumnBacklog, + DefaultColumnNotStarted, + DefaultColumnDoing, + DefaultColumnTesting, + second.Key, + DefaultColumnDone, + } + if len(workflow) != len(want) { + t.Fatalf("expected %d workflow columns, got %d", len(want), len(workflow)) + } + for i, col := range workflow { + if col.Key != want[i] { + t.Fatalf("expected lane %d key %q, got %q", i, want[i], col.Key) + } + if col.Position != i { + t.Fatalf("expected lane %q position %d, got %d", col.Key, i, col.Position) + } + } +} + +func TestDeleteLane_PreservesSingleDoneLane(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProjectWithWorkflow(ctx, "workflow-delete-custom-done", []WorkflowColumn{ + {Key: "backlog_custom", Name: "Backlog", Color: "#9CA3AF", Position: 0}, + {Key: "review_custom", Name: "Review", Color: "#10B981", Position: 1}, + {Key: "shipped_custom", Name: "Shipped", Color: "#EF4444", Position: 2, IsDone: true}, + }) + if err != nil { + t.Fatalf("CreateProjectWithWorkflow: %v", err) + } + + if err := st.DeleteWorkflowColumn(ctx, project.ID, "review_custom"); err != nil { + t.Fatalf("DeleteWorkflowColumn: %v", err) + } + + workflow, err := st.GetProjectWorkflow(ctx, project.ID) + if err != nil { + t.Fatalf("GetProjectWorkflow: %v", err) + } + doneCount := 0 + for i, col := range workflow { + if col.Position != i { + t.Fatalf("expected lane %q position %d, got %d", col.Key, i, col.Position) + } + if col.IsDone { + doneCount++ + if col.Key != "shipped_custom" { + t.Fatalf("expected done lane key %q, got %q", "shipped_custom", col.Key) + } + } + } + if doneCount != 1 { + t.Fatalf("expected exactly one done lane, got %d", doneCount) + } +} diff --git a/internal/store/workflow_rename_test.go b/internal/store/workflow_rename_test.go new file mode 100644 index 0000000..d8d0480 --- /dev/null +++ b/internal/store/workflow_rename_test.go @@ -0,0 +1,104 @@ +package store + +import ( + "context" + "reflect" + "testing" +) + +func TestRenameLane_UpdatesNamePreservesKey(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-rename") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + todo, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Keep key stable", + ColumnKey: DefaultColumnDoing, + }, ModeFull) + if err != nil { + t.Fatalf("CreateTodo: %v", err) + } + + if err := st.UpdateWorkflowColumnName(ctx, project.ID, DefaultColumnDoing, "Working"); err != nil { + t.Fatalf("UpdateWorkflowColumnName: %v", err) + } + + workflow, err := st.GetProjectWorkflow(ctx, project.ID) + if err != nil { + t.Fatalf("GetProjectWorkflow: %v", err) + } + var renamed *WorkflowColumn + for i := range workflow { + if workflow[i].Key == DefaultColumnDoing { + renamed = &workflow[i] + break + } + } + if renamed == nil { + t.Fatalf("expected workflow column %q", DefaultColumnDoing) + } + if renamed.Name != "Working" { + t.Fatalf("expected renamed lane label %q, got %q", "Working", renamed.Name) + } + if renamed.Key != DefaultColumnDoing { + t.Fatalf("expected lane key to remain %q, got %q", DefaultColumnDoing, renamed.Key) + } + if todo.ColumnKey != DefaultColumnDoing { + t.Fatalf("expected todo column key to remain %q, got %q", DefaultColumnDoing, todo.ColumnKey) + } + + pc, err := st.GetProjectContextForRead(ctx, project.ID, ModeFull) + if err != nil { + t.Fatalf("GetProjectContextForRead: %v", err) + } + _, _, _, cols, err := st.GetBoard(ctx, &pc, "", "", SprintFilter{Mode: "none"}) + if err != nil { + t.Fatalf("GetBoard: %v", err) + } + doing := cols[DefaultColumnDoing] + if len(doing) != 1 || doing[0].ID != todo.ID { + t.Fatalf("expected todo %d to remain in %q, got %+v", todo.ID, DefaultColumnDoing, doing) + } +} + +func TestRenameLane_BurndownUnaffected(t *testing.T) { + st, cleanup := newTestStore(t) + defer cleanup() + + ctx := context.Background() + project, err := st.CreateProject(ctx, "workflow-burndown") + if err != nil { + t.Fatalf("CreateProject: %v", err) + } + if _, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Incomplete", + ColumnKey: DefaultColumnDoing, + }, ModeFull); err != nil { + t.Fatalf("CreateTodo incomplete: %v", err) + } + if _, err := st.CreateTodo(ctx, project.ID, CreateTodoInput{ + Title: "Completed", + ColumnKey: DefaultColumnDone, + }, ModeFull); err != nil { + t.Fatalf("CreateTodo completed: %v", err) + } + + before, err := st.GetBacklogSize(ctx, project.ID, ModeFull) + if err != nil { + t.Fatalf("GetBacklogSize before rename: %v", err) + } + if err := st.UpdateWorkflowColumnName(ctx, project.ID, DefaultColumnDone, "Shipped"); err != nil { + t.Fatalf("UpdateWorkflowColumnName: %v", err) + } + after, err := st.GetBacklogSize(ctx, project.ID, ModeFull) + if err != nil { + t.Fatalf("GetBacklogSize after rename: %v", err) + } + if !reflect.DeepEqual(before, after) { + t.Fatalf("expected burndown to be unchanged after lane rename\nbefore=%+v\nafter=%+v", before, after) + } +} diff --git a/internal/store/workflows.go b/internal/store/workflows.go index d8b0e66..ae34145 100644 --- a/internal/store/workflows.go +++ b/internal/store/workflows.go @@ -11,6 +11,13 @@ import ( var columnKeyRe = regexp.MustCompile(`^[a-z0-9](?:[a-z0-9_]*[a-z0-9])?$`) var colorHexRe = regexp.MustCompile(`^#[0-9a-fA-F]{6}$`) +const ( + maxWorkflowColumns = 12 + defaultWorkflowColor = "#64748b" + maxWorkflowNameLength = 200 + maxWorkflowKeyAttempts = 1000 +) + type sqlExecer interface { ExecContext(ctx context.Context, query string, args ...any) (sql.Result, error) } @@ -39,6 +46,67 @@ func HumanizeColumnKey(key string) string { return strings.Join(parts, " ") } +func workflowKeyFromName(name string) string { + key := strings.ToLower(strings.TrimSpace(name)) + key = strings.Join(strings.Fields(key), "_") + var b strings.Builder + b.Grow(len(key)) + lastUnderscore := false + for _, r := range key { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + b.WriteRune(r) + lastUnderscore = false + case r == '_': + if !lastUnderscore { + b.WriteByte('_') + lastUnderscore = true + } + default: + if !lastUnderscore { + b.WriteByte('_') + lastUnderscore = true + } + } + } + key = strings.Trim(b.String(), "_") + if len(key) > maxSlugLen { + key = strings.Trim(key[:maxSlugLen], "_") + } + if key == "" { + return "lane" + } + return key +} + +func uniqueWorkflowKey(baseKey string, used map[string]struct{}) (string, error) { + if _, exists := used[baseKey]; !exists && isValidColumnKey(baseKey) { + return baseKey, nil + } + for i := 2; i <= maxWorkflowKeyAttempts; i++ { + suffix := fmt.Sprintf("_%d", i) + candidate := baseKey + if len(candidate)+len(suffix) > maxSlugLen { + candidate = strings.Trim(candidate[:maxSlugLen-len(suffix)], "_") + } + if candidate == "" { + candidate = "lane" + if len(candidate)+len(suffix) > maxSlugLen { + candidate = candidate[:maxSlugLen-len(suffix)] + } + } + candidate += suffix + if !isValidColumnKey(candidate) { + continue + } + if _, exists := used[candidate]; exists { + continue + } + return candidate, nil + } + return "", fmt.Errorf("%w: could not generate unique workflow column key", ErrConflict) +} + func defaultWorkflowColumns() []WorkflowColumn { return []WorkflowColumn{ {Key: DefaultColumnBacklog, Name: "Backlog", Color: "#9CA3AF", Position: 0, IsDone: false, System: true}, @@ -179,6 +247,46 @@ ORDER BY position ASC, id ASC`, projectID) return out, nil } +func (s *Store) GetProjectWorkflows(ctx context.Context, projectIDs []int64) (map[int64][]WorkflowColumn, error) { + out := make(map[int64][]WorkflowColumn, len(projectIDs)) + if len(projectIDs) == 0 { + return out, nil + } + args := make([]any, 0, len(projectIDs)) + seen := make(map[int64]struct{}, len(projectIDs)) + for _, projectID := range projectIDs { + if _, ok := seen[projectID]; ok { + continue + } + seen[projectID] = struct{}{} + args = append(args, projectID) + out[projectID] = []WorkflowColumn{} + } + rows, err := s.db.QueryContext(ctx, ` +SELECT id, project_id, key, name, color, position, is_done, system +FROM project_workflow_columns +WHERE project_id IN `+makePlaceholders(len(args))+` +ORDER BY project_id ASC, position ASC, id ASC`, args...) + if err != nil { + return nil, fmt.Errorf("list workflow columns for projects: %w", err) + } + defer rows.Close() + for rows.Next() { + var col WorkflowColumn + var isDone, isSystem int + if err := rows.Scan(&col.ID, &col.ProjectID, &col.Key, &col.Name, &col.Color, &col.Position, &isDone, &isSystem); err != nil { + return nil, fmt.Errorf("scan workflow column for project: %w", err) + } + col.IsDone = isDone == 1 + col.System = isSystem == 1 + out[col.ProjectID] = append(out[col.ProjectID], col) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("rows workflow columns for projects: %w", err) + } + return out, nil +} + func (s *Store) GetWorkflowDoneColumnKey(ctx context.Context, projectID int64) (string, error) { var key string if err := s.db.QueryRowContext(ctx, ` @@ -193,6 +301,262 @@ LIMIT 1`, projectID).Scan(&key); err != nil { return key, nil } +func (s *Store) UpdateWorkflowColumnName(ctx context.Context, projectID int64, key, newName string) error { + key = strings.TrimSpace(key) + newName = strings.TrimSpace(newName) + if newName == "" || len(newName) > maxWorkflowNameLength { + return fmt.Errorf("%w: invalid workflow column name", ErrValidation) + } + res, err := s.db.ExecContext(ctx, ` +UPDATE project_workflow_columns +SET name = ? +WHERE project_id = ? AND key = ?`, newName, projectID, key) + if err != nil { + return fmt.Errorf("update workflow column name: %w", err) + } + rowsAffected, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("rows affected update workflow column name: %w", err) + } + if rowsAffected == 0 { + return ErrNotFound + } + return nil +} + +func (s *Store) AddWorkflowColumn(ctx context.Context, projectID int64, name string) (WorkflowColumn, error) { + name = strings.TrimSpace(name) + if name == "" || len(name) > maxWorkflowNameLength { + return WorkflowColumn{}, fmt.Errorf("%w: invalid workflow column name", ErrValidation) + } + + tx, err := s.db.BeginTx(ctx, &sql.TxOptions{}) + if err != nil { + return WorkflowColumn{}, fmt.Errorf("begin add workflow column tx: %w", err) + } + defer func() { _ = tx.Rollback() }() + + loadWorkflow := func() ([]WorkflowColumn, error) { + rows, err := tx.QueryContext(ctx, ` +SELECT id, project_id, key, name, color, position, is_done, system +FROM project_workflow_columns +WHERE project_id = ? +ORDER BY position ASC, id ASC`, projectID) + if err != nil { + return nil, fmt.Errorf("list workflow columns for add: %w", err) + } + defer rows.Close() + out := make([]WorkflowColumn, 0, 8) + for rows.Next() { + var col WorkflowColumn + var isDone, isSystem int + if err := rows.Scan(&col.ID, &col.ProjectID, &col.Key, &col.Name, &col.Color, &col.Position, &isDone, &isSystem); err != nil { + return nil, fmt.Errorf("scan workflow column for add: %w", err) + } + col.IsDone = isDone == 1 + col.System = isSystem == 1 + out = append(out, col) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("rows workflow columns for add: %w", err) + } + return out, nil + } + + workflow, err := loadWorkflow() + if err != nil { + return WorkflowColumn{}, err + } + if len(workflow) == 0 { + if err := s.ensureDefaultWorkflowColumnsExec(ctx, tx, tx, projectID); err != nil { + return WorkflowColumn{}, err + } + workflow, err = loadWorkflow() + if err != nil { + return WorkflowColumn{}, err + } + } + if len(workflow) >= maxWorkflowColumns { + return WorkflowColumn{}, fmt.Errorf("%w: workflow may have at most %d columns", ErrValidation, maxWorkflowColumns) + } + + doneIdx := -1 + doneCount := 0 + usedKeys := make(map[string]struct{}, len(workflow)) + for i, col := range workflow { + usedKeys[col.Key] = struct{}{} + if col.IsDone { + doneIdx = i + doneCount++ + } + } + if doneCount != 1 || doneIdx < 0 { + return WorkflowColumn{}, fmt.Errorf("%w: project workflow must have exactly one done column", ErrValidation) + } + + baseKey := workflowKeyFromName(name) + key, err := uniqueWorkflowKey(baseKey, usedKeys) + if err != nil { + return WorkflowColumn{}, err + } + + insertPos := doneIdx + for i := range workflow { + nextPos := i + if i >= doneIdx { + nextPos = i + 1 + } + if _, err := tx.ExecContext(ctx, ` +UPDATE project_workflow_columns +SET position = ? +WHERE id = ?`, nextPos, workflow[i].ID); err != nil { + return WorkflowColumn{}, fmt.Errorf("shift workflow column positions: %w", err) + } + } + + res, err := tx.ExecContext(ctx, ` +INSERT INTO project_workflow_columns(project_id, key, name, color, position, is_done, system) +VALUES (?, ?, ?, ?, ?, 0, 0)`, projectID, key, name, defaultWorkflowColor, insertPos) + if err != nil { + if strings.Contains(err.Error(), "UNIQUE constraint failed") { + return WorkflowColumn{}, fmt.Errorf("%w: workflow column key already exists", ErrConflict) + } + return WorkflowColumn{}, fmt.Errorf("insert workflow column: %w", err) + } + id, err := res.LastInsertId() + if err != nil { + return WorkflowColumn{}, fmt.Errorf("last insert id workflow column: %w", err) + } + + if err := validateExactlyOneDoneColumn(ctx, tx, projectID); err != nil { + return WorkflowColumn{}, err + } + if err := tx.Commit(); err != nil { + return WorkflowColumn{}, fmt.Errorf("commit add workflow column tx: %w", err) + } + + return WorkflowColumn{ + ID: id, + ProjectID: projectID, + Key: key, + Name: name, + Color: defaultWorkflowColor, + Position: insertPos, + IsDone: false, + System: false, + }, nil +} + +func (s *Store) DeleteWorkflowColumn(ctx context.Context, projectID int64, key string) error { + key = strings.TrimSpace(key) + if key == "" { + return fmt.Errorf("%w: invalid workflow column key", ErrValidation) + } + + tx, err := s.db.BeginTx(ctx, &sql.TxOptions{}) + if err != nil { + return fmt.Errorf("begin delete workflow column tx: %w", err) + } + defer func() { _ = tx.Rollback() }() + + loadWorkflow := func() ([]WorkflowColumn, error) { + rows, err := tx.QueryContext(ctx, ` +SELECT id, project_id, key, name, color, position, is_done, system +FROM project_workflow_columns +WHERE project_id = ? +ORDER BY position ASC, id ASC`, projectID) + if err != nil { + return nil, fmt.Errorf("list workflow columns for delete: %w", err) + } + defer rows.Close() + out := make([]WorkflowColumn, 0, 8) + for rows.Next() { + var col WorkflowColumn + var isDone, isSystem int + if err := rows.Scan(&col.ID, &col.ProjectID, &col.Key, &col.Name, &col.Color, &col.Position, &isDone, &isSystem); err != nil { + return nil, fmt.Errorf("scan workflow column for delete: %w", err) + } + col.IsDone = isDone == 1 + col.System = isSystem == 1 + out = append(out, col) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("rows workflow columns for delete: %w", err) + } + return out, nil + } + + workflow, err := loadWorkflow() + if err != nil { + return err + } + if len(workflow) == 0 { + if err := s.ensureDefaultWorkflowColumnsExec(ctx, tx, tx, projectID); err != nil { + return err + } + workflow, err = loadWorkflow() + if err != nil { + return err + } + } + + targetIdx := -1 + for i, col := range workflow { + if col.Key == key { + targetIdx = i + break + } + } + if targetIdx < 0 { + return ErrNotFound + } + target := workflow[targetIdx] + if target.IsDone { + return fmt.Errorf("%w: cannot delete done workflow column", ErrValidation) + } + if len(workflow) <= 2 { + return fmt.Errorf("%w: project workflow must have at least 2 columns", ErrValidation) + } + + var todoCount int + if err := tx.QueryRowContext(ctx, ` +SELECT COUNT(*) FROM todos +WHERE project_id = ? AND column_key = ?`, projectID, key).Scan(&todoCount); err != nil { + return fmt.Errorf("count todos for workflow column delete: %w", err) + } + if todoCount > 0 { + return fmt.Errorf("%w: workflow column is not empty", ErrConflict) + } + + if _, err := tx.ExecContext(ctx, ` +DELETE FROM project_workflow_columns +WHERE id = ?`, target.ID); err != nil { + return fmt.Errorf("delete workflow column: %w", err) + } + + nextPos := 0 + for i, col := range workflow { + if i == targetIdx { + continue + } + if _, err := tx.ExecContext(ctx, ` +UPDATE project_workflow_columns +SET position = ? +WHERE id = ?`, nextPos, col.ID); err != nil { + return fmt.Errorf("resequence workflow columns after delete: %w", err) + } + nextPos++ + } + + if err := validateExactlyOneDoneColumn(ctx, tx, projectID); err != nil { + return err + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("commit delete workflow column tx: %w", err) + } + return nil +} + func (s *Store) ValidateProjectColumnKey(ctx context.Context, projectID int64, columnKey string) (WorkflowColumn, error) { return validateProjectColumnKeyQueryer(ctx, s.db, projectID, columnKey) }