From f08f06bef47aac2c9effb4cec650d99c2deb2dd7 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Sat, 18 Jul 2026 00:14:45 +0000 Subject: Rework Tasks tab: Chains section, checklist modal, project visibility The flat Tasks-tab atom list was silently dumping every chain step (locked and unlocked) and dormant bucket-pool items in as ordinary undated cards, with no chain/project context and no protection against completing a locked step out of order. - CompleteNativeTask now rejects completing a locked chain task (ErrChainTaskLocked), mapped to 400 in both the widget and web complete-atom handlers. - Chain tasks and dormant bucket items are excluded from the flat atom list; a new "Chains" section shows one card per active/paused chain with the current step and N/M progress. - New chain checklist modal (GET /chains/{id}) lists every position in order with pause/resume/abandon -- the web view originally deferred as Android-only. - Fixed a real bug this surfaced: resuming a paused chain only flipped the status flag, never unlocking the deferred successor, so a chain paused right after a completion stayed stuck forever. SetChainStatus now catches up the deferred advancement on resume, idempotently. - Atom cards gained a project-name chip for general visibility. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01EZ7ikw2ukGJFTHE3bJS7zL --- internal/handlers/atoms.go | 16 ++- internal/handlers/atoms_test.go | 53 +++++++++ internal/handlers/chains_web.go | 109 ++++++++++++++++++ internal/handlers/chains_web_test.go | 213 +++++++++++++++++++++++++++++++++++ internal/handlers/handlers.go | 13 +++ internal/handlers/widget.go | 4 + internal/models/atom.go | 16 ++- internal/models/types.go | 11 ++ internal/store/chains.go | 111 ++++++++++++++---- internal/store/chains_test.go | 68 ++++++++++- internal/store/native_tasks.go | 26 ++++- 11 files changed, 608 insertions(+), 32 deletions(-) create mode 100644 internal/handlers/chains_web.go create mode 100644 internal/handlers/chains_web_test.go (limited to 'internal') diff --git a/internal/handlers/atoms.go b/internal/handlers/atoms.go index 9474150..16cd003 100644 --- a/internal/handlers/atoms.go +++ b/internal/handlers/atoms.go @@ -30,8 +30,22 @@ func BuildUnifiedAtomList(s *store.Store, claudomator api.ClaudomatorClient) ([] atoms := make([]models.Atom, 0, len(gTasks)+len(nativeTasks)) - // Add native doot tasks (GetNativeTasks already filters completed=0) + // Add native doot tasks (GetNativeTasks already filters completed=0). + // Chain tasks (locked or unlocked) and dormant bucket-pool items are + // deliberately excluded here. Chains get their own "Chains" section + // (see BuildChainSummaries) that owns the whole lifecycle -- viewing + // every step, completing the current one, pause/resume/abandon -- in + // one place, rather than the current step also duplicating into this + // flat list and the locked steps cluttering it with non-actionable + // cards. Dormant bucket items aren't actionable until picked, per the + // bucket design. for _, task := range nativeTasks { + if task.ChainID != "" { + continue + } + if task.BucketState == "dormant" { + continue + } atoms = append(atoms, models.NativeTaskToAtom(task)) } diff --git a/internal/handlers/atoms_test.go b/internal/handlers/atoms_test.go index 521b147..7cb94a7 100644 --- a/internal/handlers/atoms_test.go +++ b/internal/handlers/atoms_test.go @@ -44,6 +44,59 @@ func TestBuildUnifiedAtomList_WithNativeTasks(t *testing.T) { } } +func TestBuildUnifiedAtomList_ExcludesLockedChainTasks(t *testing.T) { + s, err := store.New(":memory:", "../../migrations") + if err != nil { + t.Fatalf("failed to create in-memory store: %v", err) + } + defer s.Close() + + if _, err := s.CreateChain("Track", []models.ChainTaskInput{ + {Content: "Step 1"}, {Content: "Step 2"}, {Content: "Step 3"}, + }); err != nil { + t.Fatal(err) + } + + atoms, _, err := BuildUnifiedAtomList(s, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + for _, a := range atoms { + if a.Title == "Step 1" || a.Title == "Step 2" || a.Title == "Step 3" { + t.Errorf("chain task %q should not appear in the flat atom list (chains get their own Chains section)", a.Title) + } + } +} + +func TestBuildUnifiedAtomList_ExcludesDormantBucketItems(t *testing.T) { + s, err := store.New(":memory:", "../../migrations") + if err != nil { + t.Fatalf("failed to create in-memory store: %v", err) + } + defer s.Close() + + bucket, err := s.CreateBucket("Gutters", 30, 1) + if err != nil { + t.Fatal(err) + } + if err := s.CreateNativeTask(models.Task{ID: "pool-item", Content: "Clean gutters", Priority: 1}); err != nil { + t.Fatal(err) + } + if err := s.AddBucketItem(bucket.ID, "pool-item"); err != nil { + t.Fatal(err) + } + + atoms, _, err := BuildUnifiedAtomList(s, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + for _, a := range atoms { + if a.Title == "Clean gutters" { + t.Error("dormant bucket-pool item should not appear in the flat atom list") + } + } +} + func TestBuildUnifiedAtomList_WithClaudomator(t *testing.T) { s, err := store.New(":memory:", "../../migrations") if err != nil { diff --git a/internal/handlers/chains_web.go b/internal/handlers/chains_web.go new file mode 100644 index 0000000..862bec3 --- /dev/null +++ b/internal/handlers/chains_web.go @@ -0,0 +1,109 @@ +package handlers + +import ( + "errors" + "net/http" + + "github.com/go-chi/chi/v5" + + "task-dashboard/internal/models" + "task-dashboard/internal/store" +) + +// BuildChainSummaries returns one summary per active/paused chain -- +// project name, the currently-unlocked task (nil if none, e.g. a chain +// that just completed and hasn't been re-fetched into the active/paused +// set yet), and 1-indexed position/total for a "N/M" progress display. +// Backs the Tasks tab's Chains section. +func BuildChainSummaries(s *store.Store) ([]models.ChainSummary, error) { + chains, err := s.GetChains() + if err != nil { + return nil, err + } + summaries := make([]models.ChainSummary, 0, len(chains)) + for _, chain := range chains { + project, err := s.GetProjectByID(chain.ProjectID) + if err != nil { + continue // orphaned project reference -- skip rather than fail the whole tab + } + tasks, err := s.GetChainTasks(chain.ID) + if err != nil { + return nil, err + } + summary := models.ChainSummary{Chain: chain, ProjectName: project.Name, Total: len(tasks)} + for i, t := range tasks { + if t.ChainUnlocked { + task := t + summary.CurrentTask = &task + summary.Position = i + 1 + break + } + } + summaries = append(summaries, summary) + } + return summaries, nil +} + +// HandleChainDetailView renders the full ordered checklist for a chain -- +// locked and unlocked tasks both -- for the web Tasks tab's chain modal. +// This is the "visible in the tasks list" requirement from the chains +// design spec, met on web (the spec originally scoped this to Android +// only; extended to web per user request). +func (h *Handler) HandleChainDetailView(w http.ResponseWriter, r *http.Request) { + id := chi.URLParam(r, "id") + chain, err := h.store.GetChain(id) + if err != nil { + if errors.Is(err, store.ErrNativeTaskNotFound) { + http.Error(w, "chain not found", http.StatusNotFound) + return + } + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + project, err := h.store.GetProjectByID(chain.ProjectID) + if err != nil { + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + tasks, err := h.store.GetChainTasks(id) + if err != nil { + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + data := struct { + Chain models.Chain + ProjectName string + Tasks []models.Task + }{Chain: *chain, ProjectName: project.Name, Tasks: tasks} + HTMLResponse(w, h.renderer, "chain-detail", data) +} + +// HandleChainPause, HandleChainResume, and HandleChainAbandon are the +// session-authed web equivalents of the widget API's chain-status +// endpoints, for the Tasks tab's chain modal. +func (h *Handler) HandleChainPause(w http.ResponseWriter, r *http.Request) { + h.setChainStatusWeb(w, r, "paused") +} + +func (h *Handler) HandleChainResume(w http.ResponseWriter, r *http.Request) { + h.setChainStatusWeb(w, r, "active") +} + +func (h *Handler) HandleChainAbandon(w http.ResponseWriter, r *http.Request) { + h.setChainStatusWeb(w, r, "abandoned") +} + +func (h *Handler) setChainStatusWeb(w http.ResponseWriter, r *http.Request, status string) { + id := chi.URLParam(r, "id") + if err := h.store.SetChainStatus(id, status); err != nil { + if errors.Is(err, store.ErrNativeTaskNotFound) { + http.Error(w, "chain not found", http.StatusNotFound) + return + } + http.Error(w, "failed to update chain", http.StatusInternalServerError) + return + } + w.Header().Set("HX-Reswap", "none") + w.Header().Set("HX-Trigger", "refresh-tasks") + w.WriteHeader(http.StatusOK) +} diff --git a/internal/handlers/chains_web_test.go b/internal/handlers/chains_web_test.go new file mode 100644 index 0000000..d27dab9 --- /dev/null +++ b/internal/handlers/chains_web_test.go @@ -0,0 +1,213 @@ +package handlers + +import ( + "net/http" + "net/http/httptest" + "testing" + + "task-dashboard/internal/models" +) + +func TestBuildChainSummaries_ReturnsPositionAndCurrentTask(t *testing.T) { + db, cleanup := setupTestDB(t) + defer cleanup() + + chain, err := db.CreateChain("Track", []models.ChainTaskInput{ + {Content: "Step 1"}, {Content: "Step 2"}, {Content: "Step 3"}, + }) + if err != nil { + t.Fatal(err) + } + + summaries, err := BuildChainSummaries(db) + if err != nil { + t.Fatalf("BuildChainSummaries: %v", err) + } + if len(summaries) != 1 { + t.Fatalf("len(summaries) = %d, want 1", len(summaries)) + } + s := summaries[0] + if s.Chain.ID != chain.ID || s.ProjectName != "Track" { + t.Errorf("summary = %+v", s) + } + if s.Total != 3 || s.Position != 1 { + t.Errorf("Position/Total = %d/%d, want 1/3", s.Position, s.Total) + } + if s.CurrentTask == nil || s.CurrentTask.Content != "Step 1" { + t.Errorf("CurrentTask = %+v, want Step 1", s.CurrentTask) + } +} + +func TestBuildChainSummaries_ExcludesAbandonedAndCompleted(t *testing.T) { + db, cleanup := setupTestDB(t) + defer cleanup() + + chain, err := db.CreateChain("Track", []models.ChainTaskInput{{Content: "Only step"}}) + if err != nil { + t.Fatal(err) + } + if err := db.SetChainStatus(chain.ID, "abandoned"); err != nil { + t.Fatal(err) + } + + summaries, err := BuildChainSummaries(db) + if err != nil { + t.Fatal(err) + } + if len(summaries) != 0 { + t.Errorf("summaries = %+v, want none (abandoned chain excluded)", summaries) + } +} + +func TestHandleChainDetailView_RendersFullOrderedList(t *testing.T) { + h, cleanup := setupTestHandler(t) + defer cleanup() + + chain, err := h.store.CreateChain("Track", []models.ChainTaskInput{{Content: "Step 1"}, {Content: "Step 2"}}) + if err != nil { + t.Fatal(err) + } + + req := withURLParam(httptest.NewRequest("GET", "/chains/"+chain.ID, nil), "id", chain.ID) + w := httptest.NewRecorder() + h.HandleChainDetailView(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", w.Code, w.Body.String()) + } + + mock := h.renderer.(*MockRenderer) + if len(mock.Calls) == 0 { + t.Fatal("expected renderer to be called") + } + lastCall := mock.Calls[len(mock.Calls)-1] + if lastCall.Name != "chain-detail" { + t.Errorf("template = %q, want chain-detail", lastCall.Name) + } + data, ok := lastCall.Data.(struct { + Chain models.Chain + ProjectName string + Tasks []models.Task + }) + if !ok { + t.Fatalf("unexpected data type %T", lastCall.Data) + } + if data.ProjectName != "Track" || len(data.Tasks) != 2 { + t.Errorf("data = %+v", data) + } + if data.Tasks[0].Content != "Step 1" || data.Tasks[1].Content != "Step 2" { + t.Errorf("tasks out of order: %+v", data.Tasks) + } +} + +func TestHandleChainDetailView_UnknownID_Returns404(t *testing.T) { + db, cleanup := setupTestDB(t) + defer cleanup() + h := &Handler{store: db} + + req := withURLParam(httptest.NewRequest("GET", "/chains/nope", nil), "id", "nope") + w := httptest.NewRecorder() + h.HandleChainDetailView(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", w.Code) + } +} + +func TestHandleChainPauseResumeAbandon_Web(t *testing.T) { + db, cleanup := setupTestDB(t) + defer cleanup() + h := &Handler{store: db} + + chain, err := db.CreateChain("Track", []models.ChainTaskInput{{Content: "Step 1"}, {Content: "Step 2"}}) + if err != nil { + t.Fatal(err) + } + + req := withURLParam(httptest.NewRequest("POST", "/chains/"+chain.ID+"/pause", nil), "id", chain.ID) + w := httptest.NewRecorder() + h.HandleChainPause(w, req) + if w.Code != http.StatusOK { + t.Fatalf("pause status = %d, want 200", w.Code) + } + if w.Header().Get("HX-Trigger") != "refresh-tasks" { + t.Errorf("HX-Trigger = %q, want refresh-tasks", w.Header().Get("HX-Trigger")) + } + paused, err := db.GetChain(chain.ID) + if err != nil || paused.Status != "paused" { + t.Fatalf("chain after pause = %+v, err=%v", paused, err) + } + + req = withURLParam(httptest.NewRequest("POST", "/chains/"+chain.ID+"/resume", nil), "id", chain.ID) + w = httptest.NewRecorder() + h.HandleChainResume(w, req) + if w.Code != http.StatusOK { + t.Fatalf("resume status = %d, want 200", w.Code) + } + + req = withURLParam(httptest.NewRequest("POST", "/chains/"+chain.ID+"/abandon", nil), "id", chain.ID) + w = httptest.NewRecorder() + h.HandleChainAbandon(w, req) + if w.Code != http.StatusOK { + t.Fatalf("abandon status = %d, want 200", w.Code) + } + abandoned, err := db.GetChain(chain.ID) + if err != nil || abandoned.Status != "abandoned" { + t.Fatalf("chain after abandon = %+v, err=%v", abandoned, err) + } +} + +func TestHandleChainPause_UnknownID_Returns404(t *testing.T) { + db, cleanup := setupTestDB(t) + defer cleanup() + h := &Handler{store: db} + + req := withURLParam(httptest.NewRequest("POST", "/chains/nope/pause", nil), "id", "nope") + w := httptest.NewRecorder() + h.HandleChainPause(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", w.Code) + } +} + +func TestHandleTabTasks_IncludesChainSummaries(t *testing.T) { + h, cleanup := setupTestHandler(t) + defer cleanup() + + if _, err := h.store.CreateChain("Track", []models.ChainTaskInput{{Content: "Step 1"}}); err != nil { + t.Fatal(err) + } + + req := httptest.NewRequest("GET", "/tabs/tasks", nil) + w := httptest.NewRecorder() + h.HandleTabTasks(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", w.Code, w.Body.String()) + } + + mock := h.renderer.(*MockRenderer) + lastCall := mock.Calls[len(mock.Calls)-1] + if lastCall.Name != "tasks-tab" { + t.Errorf("template = %q, want tasks-tab", lastCall.Name) + } + data, ok := lastCall.Data.(struct { + Atoms []models.Atom + FutureAtoms []models.Atom + Boards []models.Board + Chains []models.ChainSummary + Today string + }) + if !ok { + t.Fatalf("unexpected data type %T", lastCall.Data) + } + if len(data.Chains) != 1 || data.Chains[0].CurrentTask == nil || data.Chains[0].CurrentTask.Content != "Step 1" { + t.Errorf("Chains = %+v", data.Chains) + } + for _, a := range data.Atoms { + if a.Title == "Step 1" { + t.Error("chain task should not also appear in the flat Atoms list") + } + } +} diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 343d0b1..54c6a70 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -3,6 +3,7 @@ package handlers import ( "context" "crypto/rand" + "errors" "fmt" "html/template" "log" @@ -46,6 +47,7 @@ func New(s *store.Store, trello api.TrelloAPI, planToEat api.PlanToEatAPI, googl // Template functions funcMap := template.FuncMap{ "subtract": func(a, b int) int { return a - b }, + "add": func(a, b int) int { return a + b }, // multiDayLabel returns the "(starts/ends HH:MM)" suffix for a // multi-day calendar event's row on the day it's rendered, or "" // for a normal item or a "spans" (pass-through) day, which shows @@ -580,6 +582,10 @@ func (h *Handler) handleAtomToggle(w http.ResponseWriter, r *http.Request, compl } if err != nil { + if errors.Is(err, store.ErrChainTaskLocked) { + JSONError(w, http.StatusBadRequest, "Task is locked in its chain", err) + return + } action := "complete" if !complete { action = "reopen" @@ -858,6 +864,11 @@ func (h *Handler) HandleTabTasks(w http.ResponseWriter, r *http.Request) { return } + chainSummaries, err := BuildChainSummaries(h.store) + if err != nil { + log.Printf("Warning: failed to build chain summaries: %v", err) + } + SortAtomsByUrgency(atoms) currentAtoms, futureAtoms := PartitionAtomsByTime(atoms) @@ -865,11 +876,13 @@ func (h *Handler) HandleTabTasks(w http.ResponseWriter, r *http.Request) { Atoms []models.Atom FutureAtoms []models.Atom Boards []models.Board + Chains []models.ChainSummary Today string }{ Atoms: currentAtoms, FutureAtoms: futureAtoms, Boards: boards, + Chains: chainSummaries, Today: config.Now().Format("2006-01-02"), } diff --git a/internal/handlers/widget.go b/internal/handlers/widget.go index a7f7b66..915909e 100644 --- a/internal/handlers/widget.go +++ b/internal/handlers/widget.go @@ -391,6 +391,10 @@ func (h *Handler) HandleWidgetComplete(w http.ResponseWriter, r *http.Request) { http.Error(w, "task not found", http.StatusNotFound) return } + if errors.Is(err, store.ErrChainTaskLocked) { + http.Error(w, "task is locked in its chain", http.StatusBadRequest) + return + } http.Error(w, "failed to complete task", http.StatusInternalServerError) return } diff --git a/internal/models/atom.go b/internal/models/atom.go index bcf3c78..824faaf 100644 --- a/internal/models/atom.go +++ b/internal/models/atom.go @@ -36,12 +36,16 @@ type Atom struct { Priority int // Normalized: 1 (Low) to 4 (Urgent) + // ProjectName is set for doot-native tasks with a project assigned + // (empty for every other source, and for unassigned native tasks). + ProjectName string + // UI Helpers (to be populated by mappers) - SourceIcon string // e.g., "trello-icon.svg" or emoji - ColorClass string // e.g., "border-blue-500" - IsOverdue bool // True if due date is before today - IsFuture bool // True if due date is after today - HasSetTime bool // True if due time is not midnight (has specific time) + SourceIcon string // e.g., "trello-icon.svg" or emoji + ColorClass string // e.g., "border-blue-500" + IsOverdue bool // True if due date is before today + IsFuture bool // True if due date is after today + HasSetTime bool // True if due time is not midnight (has specific time) // Original Data (for write operations) Raw interface{} @@ -89,6 +93,7 @@ func NativeTaskToAtom(t Task) Atom { DueDate: t.DueDate, CreatedAt: t.CreatedAt, Priority: priority, + ProjectName: t.ProjectName, SourceIcon: "✅", ColorClass: "border-green-500", Raw: t, @@ -153,4 +158,3 @@ func GoogleTaskToAtom(t GoogleTask) Atom { Raw: t, } } - diff --git a/internal/models/types.go b/internal/models/types.go index aa65e18..846ce50 100644 --- a/internal/models/types.go +++ b/internal/models/types.go @@ -51,6 +51,17 @@ type Chain struct { CreatedAt time.Time `json:"created_at"` } +// ChainSummary is a chain plus enough denormalized context (project name, +// the currently-unlocked task, position/total) to render a single Tasks-tab +// card without the template needing its own store access. +type ChainSummary struct { + Chain Chain + ProjectName string + CurrentTask *Task // nil if the chain has no unlocked task right now + Position int // 1-indexed + Total int +} + // ChainTaskInput seeds one position of a chain at creation time. Priority // of 0 (the zero value, JSON field omitted) defaults to 1, matching // CreateNativeTask's default. diff --git a/internal/store/chains.go b/internal/store/chains.go index f3bd0d7..f854678 100644 --- a/internal/store/chains.go +++ b/internal/store/chains.go @@ -84,6 +84,31 @@ func (s *Store) GetChain(id string) (*models.Chain, error) { return &c, nil } +// GetChains returns every active or paused chain, oldest first. Completed +// and abandoned chains are excluded -- they're done, not part of "current +// work" surfaces like the Tasks tab. +func (s *Store) GetChains() ([]models.Chain, error) { + rows, err := s.db.Query(` + SELECT id, project_id, status, created_at FROM task_chains + WHERE status IN ('active', 'paused') + ORDER BY created_at ASC + `) + if err != nil { + return nil, err + } + defer func() { _ = rows.Close() }() + + var chains []models.Chain + for rows.Next() { + var c models.Chain + if err := rows.Scan(&c.ID, &c.ProjectID, &c.Status, &c.CreatedAt); err != nil { + return nil, err + } + chains = append(chains, c) + } + return chains, rows.Err() +} + // GetChainTasks returns every task in the chain, locked and unlocked both, // ordered by chain_position -- the full checklist view. func (s *Store) GetChainTasks(chainID string) ([]models.Task, error) { @@ -104,8 +129,34 @@ func (s *Store) GetChainTasks(chainID string) ([]models.Task, error) { } // SetChainStatus updates a chain's status (active/paused/abandoned/completed). -// Returns ErrNativeTaskNotFound if id doesn't match any row. +// Resuming a paused chain (status "active" from a current status of +// "paused") catches up any advancement that was deferred while paused -- +// per the design, "the chain must be explicitly resumed for the next task +// to unlock," i.e. resuming itself performs the unlock, not merely +// re-arming future completions to do so. Without this, a chain paused +// immediately after a completion (before its successor could unlock) would +// stay stuck forever: no task is ever unlocked, so no future completion +// could trigger advanceChain either. Returns ErrNativeTaskNotFound if id +// doesn't match any row. func (s *Store) SetChainStatus(id, status string) error { + if status == "active" { + current, err := s.GetChain(id) + if err != nil { + return err + } + if current.Status == "paused" { + maxCompleted, err := s.maxCompletedChainPosition(id) + if err != nil { + return err + } + if maxCompleted != nil { + if err := s.advanceChain(id, *maxCompleted); err != nil { + return err + } + } + } + } + result, err := s.db.Exec(`UPDATE task_chains SET status = ? WHERE id = ?`, status, id) if err != nil { return err @@ -113,34 +164,50 @@ func (s *Store) SetChainStatus(id, status string) error { return checkRowsAffected(result) } -// advanceChain is called from CompleteNativeTask when the just-completed -// task has a chain_id set. Per the design, a paused chain does not -// auto-advance -- it must be explicitly resumed first. Completing the last -// position marks the chain completed instead of advancing. -func (s *Store) advanceChain(chainID string, completedPosition int) error { - chain, err := s.GetChain(chainID) +// maxCompletedChainPosition returns the highest chain_position among +// completed tasks in the chain, or nil if none are completed yet. +func (s *Store) maxCompletedChainPosition(chainID string) (*int, error) { + var pos sql.NullInt64 + err := s.db.QueryRow(` + SELECT MAX(chain_position) FROM native_tasks WHERE chain_id = ? AND completed = 1 + `, chainID).Scan(&pos) if err != nil { - return err + return nil, err } - if chain.Status == "paused" { - return nil + if !pos.Valid { + return nil, nil } + p := int(pos.Int64) + return &p, nil +} - now := config.Now() - result, err := s.db.Exec(` - UPDATE native_tasks SET chain_unlocked = 1, due_date = ?, updated_at = ? - WHERE chain_id = ? AND chain_position = ? - `, now, now, chainID, completedPosition+1) - if err != nil { - return err +// advanceChain is called from CompleteNativeTask (when the just-completed +// task has a chain_id set and the chain isn't paused) and from +// SetChainStatus's resume catch-up. Idempotent: if the successor position +// is already unlocked, it's left untouched (so a resume catch-up run +// against a chain that was never actually stuck is a no-op, not a +// due_date-resetting re-unlock). Completing/catching-up-to the last +// position marks the chain completed instead of unlocking a successor. +func (s *Store) advanceChain(chainID string, completedPosition int) error { + var successorID string + var successorUnlocked bool + err := s.db.QueryRow(` + SELECT id, chain_unlocked FROM native_tasks WHERE chain_id = ? AND chain_position = ? + `, chainID, completedPosition+1).Scan(&successorID, &successorUnlocked) + if err == sql.ErrNoRows { + // No next position -- the completed task was the last in the chain. + return s.SetChainStatus(chainID, "completed") } - affected, err := result.RowsAffected() if err != nil { return err } - if affected == 0 { - // No next position -- the completed task was the last in the chain. - return s.SetChainStatus(chainID, "completed") + if successorUnlocked { + return nil } - return nil + + now := config.Now() + _, err = s.db.Exec(` + UPDATE native_tasks SET chain_unlocked = 1, due_date = ?, updated_at = ? WHERE id = ? + `, now, now, successorID) + return err } diff --git a/internal/store/chains_test.go b/internal/store/chains_test.go index 1d16508..6359d11 100644 --- a/internal/store/chains_test.go +++ b/internal/store/chains_test.go @@ -132,10 +132,19 @@ func TestCompleteNativeTask_PausedChain_DoesNotAdvance(t *testing.T) { t.Error("position 1 should still be locked while chain is paused") } - // Resuming re-enables advancement on the *next* completion. + // Resuming performs the deferred unlock itself -- position 1 becomes + // completable immediately, not only after some future completion. if err := s.SetChainStatus(chain.ID, "active"); err != nil { t.Fatal(err) } + resumed, err := s.GetChainTasks(chain.ID) + if err != nil { + t.Fatal(err) + } + if !resumed[1].ChainUnlocked { + t.Fatal("expected resume to unlock position 1 immediately (deferred advancement catch-up)") + } + if err := s.CompleteNativeTask(tasks[1].ID); err != nil { t.Fatalf("CompleteNativeTask after resume: %v", err) } @@ -148,6 +157,63 @@ func TestCompleteNativeTask_PausedChain_DoesNotAdvance(t *testing.T) { } } +func TestCompleteNativeTask_LockedChainTask_ReturnsErrChainTaskLocked(t *testing.T) { + s := newNativeTasksTestStore(t) + + chain, err := s.CreateChain("Track", chainTasks("Step 1", "Step 2")) + if err != nil { + t.Fatal(err) + } + tasks, err := s.GetChainTasks(chain.ID) + if err != nil { + t.Fatal(err) + } + + if err := s.CompleteNativeTask(tasks[1].ID); err != ErrChainTaskLocked { + t.Errorf("err = %v, want ErrChainTaskLocked", err) + } + + // Confirm nothing was mutated -- still locked, still not completed. + after, err := s.GetChainTasks(chain.ID) + if err != nil { + t.Fatal(err) + } + if after[1].Completed || after[1].ChainUnlocked { + t.Errorf("locked task should be untouched by the rejected completion attempt: %+v", after[1]) + } +} + +func TestSetChainStatus_ResumeWithNothingStuck_DoesNotResetDueDate(t *testing.T) { + s := newNativeTasksTestStore(t) + + chain, err := s.CreateChain("Track", chainTasks("Step 1", "Step 2")) + if err != nil { + t.Fatal(err) + } + tasks, err := s.GetChainTasks(chain.ID) + if err != nil { + t.Fatal(err) + } + originalDueDate := *tasks[0].DueDate + + // Pause and resume with nothing completed yet -- position 0 is already + // unlocked and should be left untouched by the resume catch-up. + if err := s.SetChainStatus(chain.ID, "paused"); err != nil { + t.Fatal(err) + } + if err := s.SetChainStatus(chain.ID, "active"); err != nil { + t.Fatal(err) + } + + after, err := s.GetChainTasks(chain.ID) + if err != nil { + t.Fatal(err) + } + if !after[0].DueDate.Equal(originalDueDate) { + t.Errorf("DueDate = %v, want unchanged %v (resume catch-up should be a no-op when nothing was stuck)", after[0].DueDate, originalDueDate) + } +} + func TestGetUndatedNativeTasks_ExcludesLockedChainTasks(t *testing.T) { s := newNativeTasksTestStore(t) diff --git a/internal/store/native_tasks.go b/internal/store/native_tasks.go index 6f8ba7a..11a9197 100644 --- a/internal/store/native_tasks.go +++ b/internal/store/native_tasks.go @@ -161,15 +161,27 @@ func (s *Store) UpdateNativeTaskDescription(id, description string) error { return err } +// ErrChainTaskLocked is returned by CompleteNativeTask when the task +// belongs to a chain but isn't the currently-unlocked position -- without +// this guard, completing a locked task directly by id (bypassing the UI, +// which never renders a checkbox for locked chain tasks) would still run +// advanceChain against the wrong position, breaking the chain's WIP-1 +// invariant. +var ErrChainTaskLocked = errors.New("task is locked in its chain") + // CompleteNativeTask marks a task as completed. If it's the latest // occurrence of a recurring series (no newer row exists yet), it also // creates the next iteration. Returns ErrNativeTaskNotFound if id doesn't -// match any row. +// match any row, or ErrChainTaskLocked if it's a locked (not yet +// actionable) chain task. func (s *Store) CompleteNativeTask(id string) error { task, err := s.GetNativeTaskByID(id) if err != nil { return err } + if task.ChainID != "" && !task.ChainUnlocked { + return ErrChainTaskLocked + } result, err := s.db.Exec(` UPDATE native_tasks SET completed = 1, updated_at = CURRENT_TIMESTAMP WHERE id = ? @@ -182,9 +194,19 @@ func (s *Store) CompleteNativeTask(id string) error { } if task.ChainID != "" { - if err := s.advanceChain(task.ChainID, task.ChainPosition); err != nil { + chain, err := s.GetChain(task.ChainID) + if err != nil { return err } + // A paused chain does not auto-advance -- completing its unlocked + // task is still allowed (it's the one actionable step), but the + // successor stays locked until the chain is explicitly resumed + // (see SetChainStatus's resume catch-up). + if chain.Status != "paused" { + if err := s.advanceChain(task.ChainID, task.ChainPosition); err != nil { + return err + } + } } if task.BucketID != "" { -- cgit v1.2.3