diff --git a/cmd/mavweb/main.go b/cmd/mavweb/main.go index 510faca..dfc94bf 100644 --- a/cmd/mavweb/main.go +++ b/cmd/mavweb/main.go @@ -1050,6 +1050,12 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri return "", fmt.Errorf("unknown action %q", action) } if err := core.SetTaskStatus(ctx, id, status, now(), "tap:web"); err != nil { + if errors.Is(err, ipc.ErrTaskNoDoneWhen) { + // The refusal has to name what is missing, or the button looks + // broken. The field it asks for arrives with the intake form + // (Vikunja #511). + return "", errors.New("write a definition of done before confirming this candidate") + } return "", err } return msg, nil diff --git a/internal/ipc/api.go b/internal/ipc/api.go index 36ad4ac..d2e597e 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -142,6 +142,11 @@ type Task struct { Weight int `json:"weight,omitempty"` Resolved *time.Time `json:"resolved,omitempty"` ResolvedBy string `json:"resolved_by,omitempty"` + // DoneWhen — the acceptance criterion. Empty until he writes one, and a + // candidate with no criterion cannot be promoted to open (Vikunja #510). + DoneWhen string `json:"done_when,omitempty"` + // BlockedOn — a canonical Nexus entity id, never a name. + BlockedOn string `json:"blocked_on,omitempty"` } // CaptureTaskReq — THE INTAKE SEAM. Everything that captures a task goes @@ -170,6 +175,11 @@ type CaptureTaskReq struct { Due *time.Time `json:"due,omitempty"` Weight int `json:"weight,omitempty"` Ts time.Time `json:"ts"` + // DoneWhen and BlockedOn are optional at intake. A derived source leaves + // both empty: mail says what to do, not what finishing means, and guessing + // a criterion would put Maven's reading in the field he is meant to write. + DoneWhen string `json:"done_when,omitempty"` + BlockedOn string `json:"blocked_on,omitempty"` } // CaptureTaskResp — Created is false when the same live task already existed, @@ -539,6 +549,16 @@ type setTaskStatusReq struct { By string `json:"by,omitempty"` } +// setTaskFieldsReq — the write for the two board columns. Both are sent every +// time and both may be empty: clearing a blocker is as ordinary as setting one, +// so an omitted field cannot mean "leave it alone" without a second way to say +// "make it empty". +type setTaskFieldsReq struct { + ID int64 `json:"id"` + DoneWhen string `json:"done_when,omitempty"` + BlockedOn string `json:"blocked_on,omitempty"` +} + // idReq — methods keyed by a single id. type idReq struct { ID int64 `json:"id"` @@ -847,6 +867,11 @@ type unlockReq struct { // wire round-tripping via errors.Is). var ErrToolNotFound = errors.New("ipc: tool not found") +// ErrTaskNoDoneWhen — a candidate cannot be promoted to open with no +// definition of done (Vikunja #510). Carried across the wire so the /tasks +// form can say which refusal it hit rather than "не найдено". +var ErrTaskNoDoneWhen = errors.New("ipc: task has no definition of done") + // callerKey — context key for the authenticated caller. Server sets it from // SO_PEERCRED before dispatch; in-process callers omit it (the adapter treats // a missing Caller as "trusted same-process", the equivalent of the socket's diff --git a/internal/ipc/client.go b/internal/ipc/client.go index 1b813ad..591a382 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -515,6 +515,10 @@ func (c *Client) SetTaskStatus(ctx context.Context, id int64, status string, ts return c.call(ctx, MethodSetTaskStatus, setTaskStatusReq{ID: id, Status: status, Ts: ts, By: by}, nil) } +func (c *Client) SetTaskFields(ctx context.Context, id int64, doneWhen, blockedOn string) error { + return c.call(ctx, MethodSetTaskFields, setTaskFieldsReq{ID: id, DoneWhen: doneWhen, BlockedOn: blockedOn}, nil) +} + // IngestMail hands one fetched message to core for extraction. ErrUnknownMethod // means core has no email block configured — the caller should stop asking, not // retry. diff --git a/internal/ipc/coreapi.go b/internal/ipc/coreapi.go index 1295fc8..b4aa538 100644 --- a/internal/ipc/coreapi.go +++ b/internal/ipc/coreapi.go @@ -142,6 +142,11 @@ type TaskAPI interface { // SetTaskStatus moves a task forward once: candidate→open|dropped, // open→done|dropped. Any other move is refused. SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string) error + // SetTaskFields writes the definition of done and the blocker. Not a + // status move, so it is not one-way: he may sharpen a criterion, and a + // blocker clears when the person answers. blockedOn is a canonical Nexus + // entity id or empty, never a name the caller had lying around. + SetTaskFields(ctx context.Context, id int64, doneWhen, blockedOn string) error } // SystemAPI — what the daemon knows about itself, plus the one method that diff --git a/internal/ipc/maperr_test.go b/internal/ipc/maperr_test.go index 605e493..4857368 100644 --- a/internal/ipc/maperr_test.go +++ b/internal/ipc/maperr_test.go @@ -31,6 +31,7 @@ var mapErrPairs = []struct { {"ErrReminderNotFound", store.ErrReminderNotFound, ErrReminderNotFound}, {"ErrReminderState", store.ErrReminderState, ErrReminderState}, {"ErrToolNotFound", store.ErrToolNotFound, ErrToolNotFound}, + {"ErrTaskNoDoneWhen", store.ErrTaskNoDoneWhen, ErrTaskNoDoneWhen}, } // unmappedStoreErrors — store sentinels that deliberately have no wire twin, diff --git a/internal/ipc/server.go b/internal/ipc/server.go index eb63b59..1e5b8fc 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -540,6 +540,9 @@ var methodTable = map[Method]handlerFunc{ MethodSetTaskStatus: withParamsVoid(func(ctx context.Context, api CoreAPI, p setTaskStatusReq) error { return api.SetTaskStatus(ctx, p.ID, p.Status, p.Ts, p.By) }), + MethodSetTaskFields: withParamsVoid(func(ctx context.Context, api CoreAPI, p setTaskFieldsReq) error { + return api.SetTaskFields(ctx, p.ID, p.DoneWhen, p.BlockedOn) + }), MethodListProposedRoutines: withoutParams(func(ctx context.Context, api CoreAPI) (listProposedRoutinesResp, error) { out, err := api.ListProposedRoutines(ctx) if err != nil { diff --git a/internal/ipc/storeapi.go b/internal/ipc/storeapi.go index d44f126..656efb3 100644 --- a/internal/ipc/storeapi.go +++ b/internal/ipc/storeapi.go @@ -317,6 +317,8 @@ func (a *storeAPI) CaptureTask(ctx context.Context, req CaptureTaskReq) (Capture Status: req.Status, Due: req.Due, Weight: req.Weight, + DoneWhen: req.DoneWhen, + BlockedOn: req.BlockedOn, }) if err != nil { return CaptureTaskResp{}, mapErr(err) @@ -343,6 +345,8 @@ func (a *storeAPI) ListTasks(ctx context.Context, status string) ([]Task, error) Weight: t.Weight, Resolved: t.ResolvedTs, ResolvedBy: t.ResolvedBy, + DoneWhen: t.DoneWhen, + BlockedOn: t.BlockedOn, } } return out, nil @@ -352,6 +356,10 @@ func (a *storeAPI) SetTaskStatus(ctx context.Context, id int64, status string, t return mapErr(a.s.SetTaskStatus(ctx, id, status, ts, by)) } +func (a *storeAPI) SetTaskFields(ctx context.Context, id int64, doneWhen, blockedOn string) error { + return mapErr(a.s.SetTaskFields(ctx, id, doneWhen, blockedOn)) +} + func (a *storeAPI) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine, error) { rs, err := a.s.ListProposedRoutines(ctx) if err != nil { @@ -458,6 +466,8 @@ func mapErr(err error) error { return ErrReminderState case errors.Is(err, store.ErrToolNotFound): return ErrToolNotFound + case errors.Is(err, store.ErrTaskNoDoneWhen): + return ErrTaskNoDoneWhen } return err } diff --git a/internal/ipc/unimplemented.go b/internal/ipc/unimplemented.go index 8d4c1d9..86b83a2 100644 --- a/internal/ipc/unimplemented.go +++ b/internal/ipc/unimplemented.go @@ -114,6 +114,9 @@ func (UnimplementedCoreAPI) ListTasks(ctx context.Context, status string) ([]Tas func (UnimplementedCoreAPI) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string) error { return ErrNotImplemented } +func (UnimplementedCoreAPI) SetTaskFields(ctx context.Context, id int64, doneWhen, blockedOn string) error { + return ErrNotImplemented +} func (UnimplementedCoreAPI) ListProposedRoutines(ctx context.Context) ([]ProposedRoutine, error) { return nil, ErrNotImplemented } diff --git a/internal/ipc/wire.go b/internal/ipc/wire.go index 2e12df7..68ee161 100644 --- a/internal/ipc/wire.go +++ b/internal/ipc/wire.go @@ -55,6 +55,7 @@ const ( MethodCaptureTask Method = "capture_task" MethodListTasks Method = "list_tasks" MethodSetTaskStatus Method = "set_task_status" + MethodSetTaskFields Method = "set_task_fields" MethodIngestMail Method = "ingest_mail" MethodSwapModel Method = "swap_model" MethodModelStatus Method = "model_status" diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 642075d..77b1f3e 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -290,6 +290,16 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 ALTER TABLE nudges_new RENAME TO nudges; CREATE INDEX IF NOT EXISTS idx_nudges_rule_ts ON nudges (rule, ts DESC); CREATE INDEX IF NOT EXISTS idx_nudges_outcome ON nudges (outcome);`, + + // #22 — the two columns that make tasks a work board rather than a to-do + // list (Vikunja #510). done_when is the acceptance criterion, and blocked_on + // is a canonical Nexus entity id: it names a person, identity lives in + // Nexus, and a local free-text name would be a second answer to a question + // Nexus already owns. Both default to empty rather than NULL, because "he + // has not written one" and "there is nothing to write" are the same state + // here and no caller has to tell them apart. + `ALTER TABLE tasks ADD COLUMN done_when TEXT NOT NULL DEFAULT ''; + ALTER TABLE tasks ADD COLUMN blocked_on TEXT NOT NULL DEFAULT '';`, } // migrate applies every migration with a number greater than the DB's current diff --git a/internal/store/tasks.go b/internal/store/tasks.go index 9552554..16f4f9d 100644 --- a/internal/store/tasks.go +++ b/internal/store/tasks.go @@ -66,6 +66,16 @@ type Task struct { Weight int ResolvedTs *time.Time ResolvedBy string + + // DoneWhen — the acceptance criterion, in his words. It must be able to + // close on either outcome: "it already works" counts as complete, and a + // criterion only one result satisfies is a wish rather than a definition + // (Vikunja #510). Empty until he writes one. + DoneWhen string + // BlockedOn — a canonical Nexus entity id, never a name. Identity lives in + // Nexus, so storing "Саша" here would be a second answer to a question + // Nexus already owns. Empty when nothing blocks the task. + BlockedOn string } // CaptureResult — what CaptureTask did. Created is a new row. Promoted is an @@ -82,6 +92,12 @@ var ( ErrTaskNotFound = errors.New("store: task not found") ErrTaskEmpty = errors.New("store: task text is empty") ErrTaskStatus = errors.New("store: invalid task status") + // ErrTaskNoDoneWhen — a candidate cannot be promoted to open without a + // definition of done (Vikunja #510). Same refusal ParseTaskCapture makes + // for a capture marker with nothing after it: confirming work whose + // finish line nobody wrote is how a board fills with rows that can never + // leave it. Dropping such a candidate stays legal. + ErrTaskNoDoneWhen = errors.New("store: task has no definition of done") ) // liveTaskStatuses — the two statuses that count as outstanding work. @@ -160,10 +176,10 @@ func (s *Store) CaptureTask(ctx context.Context, t Task) (CaptureResult, error) // Untargeted DO NOTHING: either unique index may be the one that fires, and // the lookup below sorts out which. res, err := s.db.ExecContext(ctx, - `INSERT INTO tasks (created_ts, text, norm, source, evidence, ext_id, status, due_ts, weight) - VALUES (?,?,?,?,?,?,?,?,?) + `INSERT INTO tasks (created_ts, text, norm, source, evidence, ext_id, status, due_ts, weight, done_when, blocked_on) + VALUES (?,?,?,?,?,?,?,?,?,?,?) ON CONFLICT DO NOTHING`, - created2.UnixMilli(), text, norm, t.Source, t.Evidence, ext, status, due, t.Weight) + created2.UnixMilli(), text, norm, t.Source, t.Evidence, ext, status, due, t.Weight, t.DoneWhen, t.BlockedOn) if err != nil { return CaptureResult{}, fmt.Errorf("capture task: %w", err) } @@ -194,7 +210,7 @@ func (s *Store) CaptureTask(ctx context.Context, t Task) (CaptureResult, error) } } if status == TaskOpen && existing.Status == TaskCandidate { - if err := s.SetTaskStatus(ctx, existing.ID, TaskOpen, created2, t.Source); err != nil { + if err := s.setTaskStatus(ctx, existing.ID, TaskOpen, created2, t.Source, false); err != nil { return CaptureResult{}, fmt.Errorf("capture task: promote candidate: %w", err) } return CaptureResult{ID: existing.ID, Promoted: true}, nil @@ -231,7 +247,7 @@ func (s *Store) lookupLiveTaskByNorm(ctx context.Context, norm string) (Task, er return t, nil } -const taskSelect = `SELECT id, created_ts, text, source, evidence, COALESCE(ext_id,''), status, due_ts, weight, resolved_ts, resolved_by FROM tasks` +const taskSelect = `SELECT id, created_ts, text, source, evidence, COALESCE(ext_id,''), status, due_ts, weight, resolved_ts, resolved_by, done_when, blocked_on FROM tasks` // LookupTask returns one task by id. func (s *Store) LookupTask(ctx context.Context, id int64) (Task, error) { @@ -304,6 +320,16 @@ func (s *Store) ListTasks(ctx context.Context, status string) ([]Task, error) { // "tap:voice"). It is recorded on the row, so a task that turns up resolved // says what resolved it. func (s *Store) SetTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string) error { + return s.setTaskStatus(ctx, id, status, ts, by, true) +} + +// setTaskStatus — the move, with the promotion gate optional. +// +// It is optional for exactly one caller: CaptureTask promoting a candidate he +// stated out loud (Vikunja #510). Refusing there would deny intake rather than +// ask for a criterion, and a direct open capture never had one either — the gate +// belongs to the deliberate promotion on /tasks, where there is a form to fill. +func (s *Store) setTaskStatus(ctx context.Context, id int64, status string, ts time.Time, by string, gateDoneWhen bool) error { var from []string switch status { case TaskOpen: @@ -329,6 +355,13 @@ func (s *Store) SetTaskStatus(ctx context.Context, id int64, status string, ts t for _, f := range from { args = append(args, f) } + if status == TaskOpen && gateDoneWhen { + // Promotion needs an acceptance criterion. Checked in the same + // statement rather than read-then-write, so two callers confirming one + // candidate cannot race past it; the row is read afterwards only to say + // WHICH refusal this was. + q += ` AND done_when <> ''` + } res, err := s.db.ExecContext(ctx, q, args...) if err != nil { return fmt.Errorf("set task status: %w", err) @@ -338,11 +371,40 @@ func (s *Store) SetTaskStatus(ctx context.Context, id int64, status string, ts t return fmt.Errorf("set task status: rows affected: %w", err) } if n == 0 { + if status == TaskOpen && gateDoneWhen { + if t, lookErr := s.LookupTask(ctx, id); lookErr == nil && t.Status == TaskCandidate && t.DoneWhen == "" { + return fmt.Errorf("%w: id=%d", ErrTaskNoDoneWhen, id) + } + } return fmt.Errorf("%w: id=%d not in %v", ErrTaskNotFound, id, from) } return nil } +// SetTaskFields writes the two board columns. Separate from SetTaskStatus +// because a status move is one-way and these are not: he may sharpen a +// definition of done, and a blocker clears when the person answers. +// +// blockedOn is a canonical Nexus entity id or empty. Free text does not belong +// here — identity lives in Nexus, and a local name would be a second answer to +// a question Nexus already owns. The caller resolves before it writes. +func (s *Store) SetTaskFields(ctx context.Context, id int64, doneWhen, blockedOn string) error { + res, err := s.db.ExecContext(ctx, + `UPDATE tasks SET done_when = ?, blocked_on = ? WHERE id = ?`, + strings.TrimSpace(doneWhen), strings.TrimSpace(blockedOn), id) + if err != nil { + return fmt.Errorf("set task fields: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("set task fields: rows affected: %w", err) + } + if n == 0 { + return fmt.Errorf("%w: id=%d", ErrTaskNotFound, id) + } + return nil +} + // NormalizeTaskText is the dedupe key: lowercased, punctuation dropped, // whitespace collapsed. Exported because the intake seam (and its tests) needs // to reason about what will and will not be treated as the same task. @@ -372,7 +434,7 @@ func scanTask(sc scanner) (Task, error) { var t Task var created int64 var due, resolved sql.NullInt64 - if err := sc.Scan(&t.ID, &created, &t.Text, &t.Source, &t.Evidence, &t.ExternalID, &t.Status, &due, &t.Weight, &resolved, &t.ResolvedBy); err != nil { + if err := sc.Scan(&t.ID, &created, &t.Text, &t.Source, &t.Evidence, &t.ExternalID, &t.Status, &due, &t.Weight, &resolved, &t.ResolvedBy, &t.DoneWhen, &t.BlockedOn); err != nil { return Task{}, err } t.CreatedTs = time.UnixMilli(created).UTC() diff --git a/internal/store/tasks_test.go b/internal/store/tasks_test.go index dadc48e..e525b01 100644 --- a/internal/store/tasks_test.go +++ b/internal/store/tasks_test.go @@ -126,6 +126,11 @@ func TestSetTaskStatusMovesOnceForwardOnly(t *testing.T) { if err := st.SetTaskStatus(ctx, cand, TaskDone, now, "tap:web"); !errors.Is(err, ErrTaskNotFound) { t.Errorf("candidate→done err = %v, want ErrTaskNotFound", err) } + // Promotion needs a definition of done; see + // TestPromotingACandidateNeedsADefinitionOfDone for that refusal. + if err := st.SetTaskFields(ctx, cand, "запись есть", ""); err != nil { + t.Fatal(err) + } if err := st.SetTaskStatus(ctx, cand, TaskOpen, now, "tap:web"); err != nil { t.Fatal(err) } @@ -245,7 +250,10 @@ func TestCaptureTaskExternalIDSurvivesResolution(t *testing.T) { if !first.Created { t.Fatal("first capture must create a row") } - // He confirms it and does it. + // He confirms it and does it. Confirming needs a criterion (Vikunja #510). + if err := st.SetTaskFields(ctx, first.ID, "страховка продлена", ""); err != nil { + t.Fatal(err) + } if err := st.SetTaskStatus(ctx, first.ID, TaskOpen, now.Add(time.Hour), "tap:web"); err != nil { t.Fatal(err) } @@ -374,3 +382,86 @@ func TestListTasksIsBounded(t *testing.T) { t.Errorf("first = %q, want the newest", all[0].Text) } } + +func TestTaskBoardColumnsRoundTrip(t *testing.T) { + ctx := context.Background() + st := newTestStore(t) + now := time.Date(2026, 8, 5, 9, 0, 0, 0, time.UTC) + + res, err := st.CaptureTask(ctx, Task{ + Text: "оплатить интернет", Source: "tap:voice", CreatedTs: now, + DoneWhen: "квитанция оплачена", BlockedOn: "ent_kate", + }) + if err != nil { + t.Fatal(err) + } + got, err := st.LookupTask(ctx, res.ID) + if err != nil { + t.Fatal(err) + } + if got.DoneWhen != "квитанция оплачена" || got.BlockedOn != "ent_kate" { + t.Fatalf("task = %+v, want both board columns back", got) + } + + // Not one-way, unlike a status move: he sharpens the criterion, and the + // blocker clears when the person answers. + if err := st.SetTaskFields(ctx, res.ID, " пришло подтверждение ", ""); err != nil { + t.Fatal(err) + } + got, err = st.LookupTask(ctx, res.ID) + if err != nil { + t.Fatal(err) + } + if got.DoneWhen != "пришло подтверждение" { + t.Errorf("done_when = %q, want the trimmed rewrite", got.DoneWhen) + } + if got.BlockedOn != "" { + t.Errorf("blocked_on = %q, want it cleared", got.BlockedOn) + } + if err := st.SetTaskFields(ctx, 9999, "x", ""); !errors.Is(err, ErrTaskNotFound) { + t.Errorf("SetTaskFields on a missing row = %v, want ErrTaskNotFound", err) + } +} + +func TestPromotingACandidateNeedsADefinitionOfDone(t *testing.T) { + ctx := context.Background() + st := newTestStore(t) + now := time.Date(2026, 8, 5, 9, 0, 0, 0, time.UTC) + + res, err := st.CaptureTask(ctx, Task{ + Text: "продлить домен", Source: "email:main", Status: TaskCandidate, + ExternalID: "msg-1:0", Evidence: "Domain expiring", CreatedTs: now, + }) + if err != nil { + t.Fatal(err) + } + // A board row whose finish line nobody wrote can never leave the board. + if err := st.SetTaskStatus(ctx, res.ID, TaskOpen, now, "tap:web"); !errors.Is(err, ErrTaskNoDoneWhen) { + t.Fatalf("promotion with no criterion = %v, want ErrTaskNoDoneWhen", err) + } + // Dropping it stays legal — declining work does not need one. + dropped, err := st.CaptureTask(ctx, Task{ + Text: "перезвонить в банк", Source: "email:main", Status: TaskCandidate, + ExternalID: "msg-2:0", CreatedTs: now, + }) + if err != nil { + t.Fatal(err) + } + if err := st.SetTaskStatus(ctx, dropped.ID, TaskDropped, now, "tap:web"); err != nil { + t.Fatalf("dropping a candidate with no criterion: %v", err) + } + + if err := st.SetTaskFields(ctx, res.ID, "домен продлён до 2027", ""); err != nil { + t.Fatal(err) + } + if err := st.SetTaskStatus(ctx, res.ID, TaskOpen, now, "tap:web"); err != nil { + t.Fatalf("promotion after writing a criterion: %v", err) + } + got, err := st.LookupTask(ctx, res.ID) + if err != nil { + t.Fatal(err) + } + if got.Status != TaskOpen { + t.Errorf("status = %q, want open", got.Status) + } +}