diff --git a/cmd/mavend/actions_money.go b/cmd/mavend/actions_money.go index 9bca68d..ec65ba8 100644 --- a/cmd/mavend/actions_money.go +++ b/cmd/mavend/actions_money.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "log" "github.com/kami/maven/internal/ipc" @@ -28,12 +29,18 @@ import ( // tracking is not connected". It never computes, estimates or rounds a total of // its own — an invented number about his money is the worst thing this could do. func (h *reactiveHandler) queryMoney(ctx context.Context, t *queryTurn) (string, bool) { - window, ok := router.ParseMoneyQuery(t.dec.Utterance) + q, ok := router.ParseMoneyQuery(t.dec.Utterance) if !ok { return "", false } + if q.Window == router.MoneyUnsupported { + // Two windows are stored and no others. Answering "сколько я потратил + // вчера?" with the month-to-date total answers a different question + // with a real number, which is the shape of a lie he cannot spot. + return "я храню только сегодняшние траты и за этот месяц.", true + } key, phrase := zenmoney.KeySpentMonth, "в этом месяце" - if window == router.MoneyToday { + if q.Window == router.MoneyToday { key, phrase = zenmoney.KeySpentToday, "сегодня" } fact, err := h.api.LatestFactBySource(ctx, key, zenmoney.Source) @@ -51,28 +58,36 @@ func (h *reactiveHandler) queryMoney(ctx context.Context, t *queryTurn) (string, log.Printf("voice: money fact: decode: %v", err) return "не получилось прочитать траты.", true } + now := h.now() + if q.Window == router.MoneyToday && !val.CoversDay(now) { + // The day window rolled over and the poller had nothing to write, + // because he has not spent anything yet today. The fact is fresh by ts + // and covers yesterday, so no staleness check can catch it — only the + // window stamp inside the value can. + return "сегодня пока ничего не вижу.", true + } reply := val.FormatRU(phrase) + if q.Income { + reply = val.FormatIncomeRU(phrase) + } if reply == "" { return "по тратам пока нечего сказать.", true } // A stale fact is reported as stale rather than spoken as today's number. - if h.now().Sub(fact.Ts) > zenmoney.StaleAfter { - return "данные от " + fact.Ts.Local().Format("02.01") + ": " + reply, true + // The age is measured from when the figure was last READ, not from when it + // last changed: a month with no spending in it does not go stale. + asOf := val.AsOf + if asOf.IsZero() { + asOf = fact.Ts + } + if now.Sub(asOf) > zenmoney.StaleAfter { + return "данные от " + asOf.Local().Format("02.01") + ": " + reply, true } return reply, true } -// isNoFactErr — ErrNoFact survives the wire wrapped, so unwrap for it. +// isNoFactErr — ErrNoFact survives the wire wrapped, so unwrap for it. The +// hand-rolled loop this replaces missed any error implementing Is(error) bool. func isNoFactErr(err error) bool { - for e := err; e != nil; { - if e == ipc.ErrNoFact { - return true - } - u, ok := e.(interface{ Unwrap() error }) - if !ok { - return false - } - e = u.Unwrap() - } - return false + return errors.Is(err, ipc.ErrNoFact) } diff --git a/cmd/mavend/actions_money_test.go b/cmd/mavend/actions_money_test.go index 4359f84..5f5657f 100644 --- a/cmd/mavend/actions_money_test.go +++ b/cmd/mavend/actions_money_test.go @@ -137,3 +137,92 @@ func TestQuerySourcesOrderMoneyBeforeRecall(t *testing.T) { t.Errorf("money source at %d, after notes at %d", moneyAt, notesAt) } } + +// The day window rolls over at midnight and the poller writes nothing until the +// first spend of the new day, so the last money_today fact is fresh by ts and +// covers yesterday. No staleness check can catch that. +func TestQueryMoneyRefusesYesterdaysDayTotal(t *testing.T) { + yesterday, _ := zenmoney.DayWindow(moneyNow().AddDate(0, 0, -1)) + sum := zenmoney.Summary{From: yesterday, Spent: []zenmoney.Money{{Currency: "RUB", Amount: 1749.5}}, Count: 3} + val, ok := sum.Value(moneyNow().AddDate(0, 0, -1).Add(2 * time.Hour)) + if !ok { + t.Fatal("want a fact value") + } + api := &moneyAPI{fact: ipc.Fact{ + Kind: "env", Key: zenmoney.KeySpentToday, Value: val, + Source: zenmoney.Source, Ts: moneyNow().Add(-11 * time.Hour), + }} + h := &reactiveHandler{api: api, now: moneyNow} + reply, claimed := h.queryMoney(context.Background(), &queryTurn{ + dec: router.Decision{Utterance: "сколько я потратил сегодня?"}, + }) + if !claimed { + t.Fatal("expected the source to claim it") + } + if strings.Contains(reply, "1749.5") { + t.Errorf("reply = %q — that is yesterday's spending spoken as today's", reply) + } +} + +// Ts advances only when the number moves, so a quiet month used to be reported +// as stale while being current. The read stamp inside the value is what the +// staleness check means. +func TestQueryMoneyMeasuresStalenessFromTheRead(t *testing.T) { + from, _ := zenmoney.MonthWindow(moneyNow()) + sum := zenmoney.Summary{From: from, Spent: []zenmoney.Money{{Currency: "RUB", Amount: 100}}, Count: 1} + val, _ := sum.Value(moneyNow().Add(-time.Hour)) + // The fact itself last CHANGED three days ago: nothing was spent since. + api := &moneyAPI{fact: ipc.Fact{ + Kind: "env", Key: zenmoney.KeySpentMonth, Value: val, + Source: zenmoney.Source, Ts: moneyNow().Add(-72 * time.Hour), + }} + h := &reactiveHandler{api: api, now: moneyNow} + reply, _ := h.queryMoney(context.Background(), &queryTurn{ + dec: router.Decision{Utterance: "сколько я потратил в этом месяце?"}, + }) + if strings.Contains(reply, "данные от") { + t.Errorf("reply = %q — the figure was read an hour ago and is current", reply) + } +} + +// Two windows are stored and no others. Answering "вчера" with the +// month-to-date total answers a different question with a real number. +func TestQueryMoneyRefusesWindowsItDoesNotKeep(t *testing.T) { + api := &moneyAPI{} + h := &reactiveHandler{api: api, now: moneyNow} + reply, ok := h.queryMoney(context.Background(), &queryTurn{ + dec: router.Decision{Utterance: "сколько я потратил вчера?"}, + }) + if !ok { + t.Fatal("a money question must be claimed, not passed to recall") + } + if !strings.Contains(reply, "только") { + t.Errorf("reply = %q, want her to say which windows she keeps", reply) + } + if api.callCnt != 0 { + t.Error("a window she does not keep must not read a fact") + } +} + +// "сколько я заработал" reads the same fact and must lead with the income. +func TestQueryMoneyLeadsWithIncomeWhenAsked(t *testing.T) { + from, _ := zenmoney.MonthWindow(moneyNow()) + sum := zenmoney.Summary{ + From: from, + Spent: []zenmoney.Money{{Currency: "RUB", Amount: 100}}, + Earned: []zenmoney.Money{{Currency: "RUB", Amount: 3000}}, + Count: 2, + } + val, _ := sum.Value(moneyNow()) + api := &moneyAPI{fact: ipc.Fact{ + Kind: "env", Key: zenmoney.KeySpentMonth, Value: val, + Source: zenmoney.Source, Ts: moneyNow(), + }} + h := &reactiveHandler{api: api, now: moneyNow} + reply, _ := h.queryMoney(context.Background(), &queryTurn{ + dec: router.Decision{Utterance: "сколько я заработал в этом месяце?"}, + }) + if strings.Index(reply, "3000") > strings.Index(reply, "100") { + t.Errorf("reply = %q, want the income he asked about first", reply) + } +} diff --git a/cmd/mavend/actions_task.go b/cmd/mavend/actions_task.go index 7b6d60e..99187f0 100644 --- a/cmd/mavend/actions_task.go +++ b/cmd/mavend/actions_task.go @@ -42,6 +42,12 @@ func (h *reactiveHandler) captureTaskFromNote(ctx context.Context, dec router.De log.Printf("voice: capture task: %v", err) return "не получилось записать задачу.", true } + if resp.Promoted { + // It was a candidate Maven derived from something she read, and he has + // now said it himself. Saying "уже в списке" here would be answering a + // confirmation with a shrug. + return "поняла, беру в работу: " + cap.Text, true + } if !resp.Created { return "это уже в списке.", true } diff --git a/cmd/mavend/actions_task_test.go b/cmd/mavend/actions_task_test.go index 252c41e..628b30d 100644 --- a/cmd/mavend/actions_task_test.go +++ b/cmd/mavend/actions_task_test.go @@ -19,6 +19,7 @@ type taskAPI struct { captured []ipc.CaptureTaskReq created bool + promoted bool capErr error tasks []ipc.Task @@ -31,7 +32,7 @@ func (a *taskAPI) CaptureTask(_ context.Context, req ipc.CaptureTaskReq) (ipc.Ca if a.capErr != nil { return ipc.CaptureTaskResp{}, a.capErr } - return ipc.CaptureTaskResp{ID: 1, Created: a.created}, nil + return ipc.CaptureTaskResp{ID: 1, Created: a.created, Promoted: a.promoted}, nil } func (a *taskAPI) ListTasks(_ context.Context, status string) ([]ipc.Task, error) { @@ -225,3 +226,29 @@ func TestQuerySourcesOrderTasksBeforeRecall(t *testing.T) { t.Errorf("tasks source at %d, after notes at %d", tasksAt, notesAt) } } + +// Saying a task out loud that Maven had only proposed is a confirmation. She +// used to answer "это уже в списке" and then read it back, in the same +// conversation, as something he had not confirmed. +func TestCaptureTaskFromNoteAcknowledgesAPromotion(t *testing.T) { + api := &taskAPI{promoted: true} + h := taskHandler(api) + reply, ok := h.captureTaskFromNote(context.Background(), router.Decision{ + Intent: router.IntentNote, Utterance: "добавь в задачи продлить страховку", + }) + if !ok { + t.Fatal("an explicit capture must claim the turn") + } + if strings.Contains(reply, "уже в списке") { + t.Errorf("reply = %q — he just confirmed it, that is not a duplicate", reply) + } + if !strings.Contains(reply, "продлить страховку") { + t.Errorf("reply = %q, want the task named back", reply) + } + // Persona: feminine, informal. + for _, bad := range []string{"рад ", "вы ", "ваш"} { + if strings.Contains(reply, bad) { + t.Errorf("reply %q contains %q", reply, bad) + } + } +} diff --git a/cmd/mavend/mail.go b/cmd/mavend/mail.go index 5167e71..d1d29d4 100644 --- a/cmd/mavend/mail.go +++ b/cmd/mavend/mail.go @@ -82,9 +82,11 @@ func newMailIntake(st *store.Store, phr phraser.Phraser, cfg *config.Config, bus // reader's header filter is what keeps the resident model off newsletters. // // Every candidate is captured with Status "candidate", Source "email:" -// and the subject as Evidence. CaptureTask dedupes on normalised text among -// live rows, so a mailbox re-read after a restart produces Created=0 rather -// than a second copy of every task. +// and the subject as Evidence, under an ExternalID naming the message and the +// span it was extracted from. That key is unique over every row whatever its +// status, so a mailbox re-read after a restart produces Created=0 — and, more +// to the point, a task he already marked done is not re-proposed the next time +// the same unread message is read again. func (m *mailIntake) ingest(ctx context.Context, req ipc.IngestMailReq) (ipc.IngestMailResp, error) { msg := email.Message{ UID: req.UID, @@ -124,15 +126,16 @@ func (m *mailIntake) ingest(ctx context.Context, req ipc.IngestMailReq) (ipc.Ing // something she read is a suggestion until he confirms it on /tasks. Status: store.TaskCandidate, } + t.ExternalID = mailExternalID(source, req.UID, c.Text) if due, ok := email.ParseDue(c.Due); ok { t.Due = &due } - id, created, err := m.st.CaptureTask(ctx, t) + res, err := m.st.CaptureTask(ctx, t) if err != nil { return resp, fmt.Errorf("mail intake: capture: %w", err) } - resp.TaskIDs = append(resp.TaskIDs, id) - if created { + resp.TaskIDs = append(resp.TaskIDs, res.ID) + if res.Created { resp.Created++ // Only a row that was actually created. CaptureTask dedupes on // normalised text among live rows, so a mailbox re-read after a @@ -165,3 +168,14 @@ func truncateRunes(s string, n int) string { } return string(r[:n]) + "…" } + +// mailExternalID names the message and the span a candidate was extracted +// from. The mailbox and UID identify the message; the normalised text +// identifies which of the candidates in it this is, so a message yielding two +// tasks gets two keys and a re-read of it gets neither twice. +// +// UIDs are stable per mailbox, and a mailbox that renumbers (UIDVALIDITY +// changing) re-proposes its tasks once, which is the safe direction. +func mailExternalID(source string, uid uint32, text string) string { + return fmt.Sprintf("%s#%d:%s", source, uid, store.NormalizeTaskText(text)) +} diff --git a/cmd/mavpoll/main.go b/cmd/mavpoll/main.go index 83fd748..69c9eb4 100644 --- a/cmd/mavpoll/main.go +++ b/cmd/mavpoll/main.go @@ -28,6 +28,7 @@ package main import ( "context" "encoding/json" + "errors" "flag" "fmt" "io" @@ -193,6 +194,14 @@ func (p *poller) pollOnce(ctx context.Context) { // // Both windows are read from one diff call each. Two calls an hour against an // API whose whole job is this is not worth caching. +// +// The write is UNCONDITIONAL, unlike every other poll in this file. The +// value-dedupe in writeIfChangedRaw only advances ts when the number moves, and +// for money that made ts mean "last changed" while the reader was asking it "as +// of when". A quiet 27 hours had core prefixing "данные от 30.07" to a figure +// that was current. The value now carries its own read stamp, so it differs +// every poll anyway and there is nothing left for the dedupe to catch. + // moneyWindow — one fact key and the period it covers. type moneyWindow struct { key string @@ -215,12 +224,14 @@ func (p *poller) pollZenmoney(ctx context.Context, now time.Time) error { } continue } - val, ok := sum.Value() + val, ok := sum.Value(now) if !ok { - // Nothing read. Silence, not a zero. + // Nothing read. Silence, not a zero. The last good fact stays, and + // the window stamp inside it is what stops core reciting yesterday's + // day total as today's after midnight. continue } - if err := p.writeIfChangedRaw(ctx, w.key, zenmoney.Source, val, now); err != nil && firstErr == nil { + if err := p.writeMoneyFact(ctx, w.key, val, now); err != nil && firstErr == nil { firstErr = err } } @@ -422,20 +433,27 @@ func (p *poller) writeIfChangedRaw(ctx context.Context, key, source, jsonVal str return nil } -// isNoFact — ErrNoFact rehydrated over the wire is wrapped (fmt.Errorf %w), so -// errors.Is is the right check; keep a helper so the switch above reads clean. -func isNoFact(err error) bool { - for e := err; e != nil; { - if e == ipc.ErrNoFact { - return true - } - u, ok := e.(interface{ Unwrap() error }) - if !ok { - return false - } - e = u.Unwrap() +// writeMoneyFact writes a money fact every poll, with no value comparison. See +// the comment above pollZenmoney for why this one does not go through +// writeIfChangedRaw. +// +// The log line names the key only, never the figures: mavpoll's log is not the +// place his spending ends up. +func (p *poller) writeMoneyFact(ctx context.Context, key, jsonVal string, now time.Time) error { + if _, err := p.core.WriteFact(ctx, ipc.WriteFactReq{ + Ts: now, Kind: "env", Key: key, Value: jsonVal, + Source: zenmoney.Source, Confidence: 1.0, + }); err != nil { + return fmt.Errorf("write %s: %w", key, err) } - return false + log.Printf("mavpoll: %s read (%s)", key, zenmoney.Source) + return nil +} + +// isNoFact — ErrNoFact rehydrated over the wire is wrapped (fmt.Errorf %w), so +// errors.Is is the right check. +func isNoFact(err error) bool { + return errors.Is(err, ipc.ErrNoFact) } func (p *poller) get(ctx context.Context, url, basicUser string) ([]byte, error) { diff --git a/cmd/mavweb/main.go b/cmd/mavweb/main.go index 5023008..45f7b82 100644 --- a/cmd/mavweb/main.go +++ b/cmd/mavweb/main.go @@ -18,6 +18,7 @@ import ( "net/url" "os" "os/signal" + "strconv" "strings" "time" @@ -904,17 +905,29 @@ func handleReminders(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { } } +// now — the wall clock, indirected so the task page can be rendered at a fixed +// instant in a test. internal/tasks is pure and the daemon path already ranks +// through a clock it is handed; the page had no reason to be the one surface +// that could only be tested at whatever time it happened to run. +var now = time.Now + +// resolvedShown — how many finished tasks the page renders. The list is +// history, it only grows, and the rows below the first screen are read by +// nobody. +const resolvedShown = 50 + // taskRow is one line on /tasks, with every timestamp already formatted so the // template holds no date logic. type taskRow struct { - ID int64 - Text string - Source string - Evidence string - Status string - Due string - Created string - Resolved string + ID int64 + Text string + Source string + Evidence string + Status string + Due string + Created string + Resolved string + ResolvedBy string // Why — the ranker's reason for this row's position (Vikunja #129), in // Russian, empty when nothing distinguished the task. Blank is the honest // rendering: he never said this one mattered more. @@ -962,6 +975,7 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { // rows keep store order (newest first) — ranking finished work is pointless. var live []tasks.Item var resolved []taskRow + resolvedTotal := 0 for _, t := range all { switch t.Status { case "candidate", "open": @@ -970,10 +984,18 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { Created: t.CreatedTs, Due: t.Due, Weight: t.Weight, }) default: + resolvedTotal++ + // Finished work is history, and the history only grows. The page + // showed every row that ever existed, which is a page that gets + // slower every month for a section nobody reads past the top of. + if len(resolved) >= resolvedShown { + continue + } resolved = append(resolved, taskRow{ ID: t.ID, Text: t.Text, Source: t.Source, Evidence: t.Evidence, Status: t.Status, Created: fmtTaskTime(&t.CreatedTs), Due: fmtTaskDate(t.Due), Resolved: fmtTaskTime(t.Resolved), + ResolvedBy: t.ResolvedBy, }) } } @@ -982,7 +1004,7 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { byID[t.ID] = t } var cands, open []taskRow - for _, r := range tasks.Rank(live, time.Now()) { + for _, r := range tasks.Rank(live, now()) { t := byID[r.ID] row := taskRow{ ID: t.ID, Text: t.Text, Source: t.Source, Evidence: t.Evidence, @@ -1001,11 +1023,12 @@ func handleTasks(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI) { } w.Header().Set("Content-Type", "text/html; charset=utf-8") if err := tasksTmpl.Execute(w, struct { - Msg, Err string - Candidates []taskRow - Open []taskRow - Resolved []taskRow - }{msg, errMsg, cands, open, resolved}); err != nil { + Msg, Err string + Candidates []taskRow + Open []taskRow + Resolved []taskRow + ResolvedMore bool + }{msg, errMsg, cands, open, resolved, resolvedTotal > len(resolved)}); err != nil { log.Printf("tasks render: %v", err) } } @@ -1020,12 +1043,14 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri if text == "" { return "", errors.New("empty task text") } - req := ipc.CaptureTaskReq{Text: text, Source: "tap:web", Status: "open", Ts: time.Now()} + req := ipc.CaptureTaskReq{Text: text, Source: "tap:web", Status: "open", Ts: now()} // Importance is his, stated on the form. Out-of-range values are // clamped rather than rejected — a bad select is not worth a 400. if v := r.FormValue("weight"); v != "" { - var wgt int - if n, _ := fmt.Sscanf(v, "%d", &wgt); n != 1 || wgt < 0 { + // strconv, not Sscanf: Sscanf("3junk", "%d") succeeds with 3, and a + // form value is not a place to accept trailing garbage. + wgt, err := strconv.Atoi(v) + if err != nil || wgt < 0 { return "", fmt.Errorf("bad weight %q", v) } if wgt > tasks.MaxWeight { @@ -1034,7 +1059,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri req.Weight = wgt } if d := r.FormValue("due"); d != "" { - due, err := time.ParseInLocation("2006-01-02", d, time.Local) + due, err := time.ParseInLocation("2006-01-02", d, now().Location()) if err != nil { return "", fmt.Errorf("bad due date %q", d) } @@ -1044,14 +1069,17 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri if err != nil { return "", err } + if resp.Promoted { + return "confirmed a candidate maven had found", nil + } if !resp.Created { return "already on the list", nil } return "added task", nil } - var id int64 - if n, _ := fmt.Sscanf(r.FormValue("id"), "%d", &id); n != 1 { + id, err := strconv.ParseInt(r.FormValue("id"), 10, 64) + if err != nil { return "", errors.New("invalid id") } var status, msg string @@ -1065,7 +1093,7 @@ func applyTaskPost(ctx context.Context, core ipc.CoreAPI, r *http.Request) (stri default: return "", fmt.Errorf("unknown action %q", action) } - if err := core.SetTaskStatus(ctx, id, status, time.Now()); err != nil { + if err := core.SetTaskStatus(ctx, id, status, now(), "tap:web"); err != nil { return "", err } return msg, nil diff --git a/cmd/mavweb/tasks.html b/cmd/mavweb/tasks.html index 8194506..70f6797 100644 --- a/cmd/mavweb/tasks.html +++ b/cmd/mavweb/tasks.html @@ -9,6 +9,9 @@ +