diff --git a/modules/messenger/cmd/messenger/answers.go b/modules/messenger/cmd/messenger/answers.go index 4c81b708..76a9cbcb 100644 --- a/modules/messenger/cmd/messenger/answers.go +++ b/modules/messenger/cmd/messenger/answers.go @@ -15,9 +15,14 @@ package main // TokenFor, used once; refused for a record cleared or silenced, used, expired, from another kind or // unknown — and the refusal is shown where the answer came from. One answer is acted on once, // whatever its redelivery. +// - **Every message still shown keeps its Details** (issue 371): a newer message about the same problem +// makes a ticket of its own and leaves the older ones standing, so Details works on each message once. +// A record keeps at most MostDetails of them on a kind, and none older than TokenFor; a ticket dropped +// by that bound is refused in words that say to tap Details on the newest message. import ( "encoding/json" + "sort" "strings" "time" @@ -48,10 +53,18 @@ const ( MostActions = 4 // TokenFor is how long a Details ticket is good for. TokenFor = 24 * time.Hour + // MostDetails bounds the Details tickets one record keeps on one channel kind: one per message about it + // still shown there; beyond it the oldest is forgotten (issue 371). + MostDetails = 8 // KeptEvents is how many answers are remembered, so a redelivered one is acted on once. KeptEvents = 200 ) +// NoOpenMessage is the refusal of a ticket no open message holds: its problem ended, or so many newer +// messages about it were sent that this older one's Details was forgotten. +const NoOpenMessage = "it belongs to no open message; the problem may have ended, or this message is older " + + "than the last few about it: tap Details on the newest one" + func sameActions(a, b []Action) bool { x, _ := json.Marshal(a) y, _ := json.Marshal(b) @@ -65,7 +78,9 @@ func needsNothing(r *Record) bool { } // choices are the answers a record's next message offers on one kind: the open ask's options offered -// there, and Details for a condition, with a fresh ticket replacing that kind's earlier one. +// there, and Details for a condition, with a ticket of its own. The tickets of the record's earlier messages +// stand (issue 371): each message keeps a Details of its own, used once, until it is TokenFor old or +// MostDetails newer ones on that kind push it out. func (h *Holder) choices(r *Record, kind string) []Choice { var out []Choice if rec := h.openAsk(r); rec != nil { @@ -80,16 +95,38 @@ func (h *Holder) choices(r *Record, kind string) []Choice { if r.Tokens == nil { r.Tokens = map[string]Grant{} } - for t, g := range r.Tokens { - if g.Kind == kind { - delete(r.Tokens, t) - } - } + pruneGrants(r.Tokens, kind, now, MostDetails-1) t := ticket() r.Tokens[t] = Grant{Index: -1, Kind: kind, Issued: now} return append(out, Choice{Token: t, Label: DetailsLabel}) } +// pruneGrants forgets every ticket older than TokenFor, and keeps at most keep of kind's, the newest. +func pruneGrants(grants map[string]Grant, kind string, now time.Time, keep int) { + var mine []string + for t, g := range grants { + switch { + case now.Sub(g.Issued) > TokenFor: + delete(grants, t) + case g.Kind == kind: + mine = append(mine, t) + } + } + if len(mine) <= keep { + return + } + sort.Slice(mine, func(i, j int) bool { + a, b := grants[mine[i]], grants[mine[j]] + if !a.Issued.Equal(b.Issued) { + return a.Issued.Before(b.Issued) + } + return mine[i] < mine[j] + }) + for _, t := range mine[:len(mine)-keep] { + delete(grants, t) + } +} + // Chosen takes an answer a channel of a kind said it was given: an ask's option is judged and, when it // stands, closes the ask with a warrant for its asker (asks.go); Details are shown where they were asked. // Anything else is refused, and said there. The same answer is acted on once. @@ -139,7 +176,7 @@ func (h *Holder) Chosen(kind string, c asks.Choice) (string, error) { } switch { case r == nil: - return refuse("it belongs to no open message; the problem may have ended, or a newer message replaced it") + return refuse(NoOpenMessage) case !r.Cleared.IsZero(): return refuse("the problem it answered has ended") case r.silenced(now): diff --git a/modules/messenger/cmd/messenger/details_test.go b/modules/messenger/cmd/messenger/details_test.go new file mode 100644 index 00000000..8dba0d35 --- /dev/null +++ b/modules/messenger/cmd/messenger/details_test.go @@ -0,0 +1,166 @@ +package main + +import ( + "fmt" + "strings" + "testing" + "time" +) + +// Issue 371: tapping Details on any but the newest message about a problem was refused "it belongs to no +// open message", because each new message replaced the ticket of the one before it. Every message still +// shown keeps a Details of its own, used once, and Details never acts. + +func detailsOf(t *testing.T, m Message) string { + t.Helper() + tok := ticketFor(m, DetailsLabel) + if tok == "" { + t.Fatalf("no Details on %q: %+v", m.Title, m.Actions) + } + return tok +} + +// The acceptance of issue 371: three warnings in a row, Details works on each of them, once each. +func TestDetailsWorksOnEachOfThreeWarningsOnceEach(t *testing.T) { + for _, same := range []bool{true, false} { + t.Run(fmt.Sprintf("same problem %v", same), func(t *testing.T) { + h, tg, _, c, _ := newHolder(t) + h.Desk = nil + var toks []string + if same { + // One problem said three times: raised, escalated, and reminded. + k := cond("machine.novox.silent", Warning, "the anchor is silent") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + k.Severity = Urgent + h.Condition(EventChanged, k) + settle(h, c) + c.pass(RemindUrgent + time.Minute) + h.Tick() + settle(h, c) + } else { + for _, key := range []string{"machine.ace.silent", "machine.shanks.units", "plan.41.stalled"} { + k := cond(key, Warning, key+" needs a look") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + } + } + if len(tg.sends) != 3 { + t.Fatalf("%d messages, want 3", len(tg.sends)) + } + for _, m := range tg.sends { + toks = append(toks, detailsOf(t, m)) + } + if toks[0] == toks[1] || toks[1] == toks[2] || toks[0] == toks[2] { + t.Fatalf("a ticket is shared: %v", toks) + } + // The oldest first, as the operator scrolls back. + for i, tok := range toks { + before := len(tg.sends) + out, err := h.Chosen("telegram", choiceOf(tok, "42", c.now())) + if err != nil || out != "shown" || len(tg.sends) != before+1 || + !strings.HasPrefix(tg.sends[len(tg.sends)-1].Title, "Details: ") { + t.Fatalf("message %d: Details %q %v", i+1, out, err) + } + } + // Once each: a second tap (a new answer, not a redelivery) is refused in words, and shows nothing. + for i, tok := range toks { + c2 := choiceOf(tok, "42", c.now()) + c2.ID += "-again" + before := len(tg.sends) + out, err := h.Chosen("telegram", c2) + if err == nil || !strings.Contains(err.Error(), "already chosen once") || + strings.HasPrefix(tg.sends[len(tg.sends)-1].Title, "Details: ") || len(tg.sends) > before+1 { + t.Errorf("message %d: replayed Details %q %v", i+1, out, err) + } + } + }) + } +} + +// Tickets are bounded: at most MostDetails per record and kind, none older than TokenFor; one pushed out +// is refused in words that send the operator to the newest message. +func TestDetailsTicketsAreBoundedInNumberAndTime(t *testing.T) { + h, tg, _, c, _ := newHolder(t) + h.Desk = nil + k := cond("machine.ace.silent", Warning, "the home server is silent") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + r := h.open[k.Key] + first := detailsOf(t, tg.sends[0]) + var toks []string + for i := 0; i < MostDetails; i++ { + c.pass(time.Second) + toks = append(toks, ticketFor(Message{Actions: h.choices(r, "telegram")}, DetailsLabel)) + } + if n := len(r.Tokens); n != MostDetails { + t.Fatalf("%d tickets kept, want %d", n, MostDetails) + } + if _, err := h.Chosen("telegram", choiceOf(first, "42", c.now())); err == nil || + !strings.Contains(err.Error(), "tap Details on the newest one") { + t.Errorf("the pushed-out ticket: %v", err) + } + if out, err := h.Chosen("telegram", choiceOf(toks[0], "42", c.now())); err != nil || out != "shown" { + t.Errorf("the oldest kept ticket: %q %v", out, err) + } + // A day on, the rest are refused as old, and forgotten at the next message. + c.pass(TokenFor + time.Minute) + if _, err := h.Chosen("telegram", choiceOf(toks[1], "42", c.now())); err == nil || + !strings.Contains(err.Error(), "more than a day old") { + t.Errorf("an expired ticket: %v", err) + } + h.choices(r, "desktop") + if n := len(r.Tokens); n != 1 { + t.Errorf("%d tickets kept after a day, want the one just made", n) + } +} + +// A change of the condition's actions leaves every shown message its Details: Details names no action. +func TestDetailsOutlivesAChangeOfTheActions(t *testing.T) { + h, tg, _, c, _ := newHolder(t) + h.Desk = nil + k := cond("machine.ace.silent", Warning, "the home server is silent") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + tok := detailsOf(t, tg.sends[0]) + k.Actions = []Action{{Label: "Restart", Verb: "node-service-manager.restart"}} + h.Condition(EventChanged, k) + if out, err := h.Chosen("telegram", choiceOf(tok, "42", c.now())); err != nil || out != "shown" { + t.Errorf("Details after the actions changed: %q %v", out, err) + } +} + +// Details is never an approval: chosen on a message whose ask is open, it shows and performs nothing, and +// the ask stays open with no warrant; an older message's Details does the same. +func TestDetailsOnAMessageWithAnOpenAskNeverAnswersIt(t *testing.T) { + h, _, dt, c, store, w := newRouter(t) + link(h, store, c, "42", c.now().Add(-2*time.Hour)) + k := cond("delivery.hq.held", Warning, "the delivery of hq is held") + k.Raised = c.now() + h.Condition(EventRaised, k) + settle(h, c) + older := detailsOf(t, dt.sends[0]) + a := ackAsk(c, "d-1") + a.About = "delivery.hq.held" + submit(t, h, "mesh-controller", a) + settle(h, c) + var shown Message + for _, m := range dt.edits { + shown = m + } + for _, tok := range []string{older, detailsOf(t, shown)} { + if out, err := h.Chosen("desktop", choiceOf(tok, "g14", c.now())); err != nil || out != "shown" { + t.Fatalf("Details: %q %v", out, err) + } + } + if got := w.of("mesh-controller"); len(got) != 0 { + t.Errorf("Details issued a warrant: %+v", got) + } + if st := store.asks["mesh-controller.d-1"].State; st != StateOpen { + t.Errorf("Details ended the ask: %s", st) + } +} diff --git a/modules/messenger/cmd/messenger/holder.go b/modules/messenger/cmd/messenger/holder.go index 9351544b..f72279b3 100644 --- a/modules/messenger/cmd/messenger/holder.go +++ b/modules/messenger/cmd/messenger/holder.go @@ -186,7 +186,14 @@ func recordOf(c Condition) Record { func (r *Record) takeWords(from Record) { r.Headline, r.Explanation, r.Resolved, r.Needs = from.Headline, from.Explanation, from.Resolved, from.Needs if !sameActions(r.Actions, from.Actions) { - r.Actions, r.Tokens = from.Actions, nil + // Details performs nothing and names no action, so its tickets outlive a change of the actions: + // every message still shown keeps its Details (issue 371). Any other ticket dies with its action. + r.Actions = from.Actions + for t, g := range r.Tokens { + if g.Index != -1 { + delete(r.Tokens, t) + } + } } }