Merge pull request 'messenger: every message still shown keeps its own Details (issue 371)' (#174) from fix/messenger-details-per-message into main

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):

modules/messenger/cmd/messenger/details_test.go

@@ -0,0 +1,199 @@
+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)
+	}
+}
+
+// A channel that stays unready makes no ticket at its tries: composing a message makes its Details ticket,
+// and tickets made at every try would push out those of the messages already shown.
+func TestAnUnreadyChannelPushesOutNoShownDetails(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.Severity = Urgent
+	h.Condition(EventChanged, k)
+	tg.notReady = fmt.Errorf("telegram is away")
+	for i := 0; i < 3*MostDetails; i++ {
+		c.pass(2 * time.Minute)
+		h.Flush()
+	}
+	if n := len(h.open[k.Key].Tokens); n != 1 {
+		t.Errorf("%d tickets after the tries, want the one shown", n)
+	}
+	tg.notReady = nil
+	c.pass(2 * time.Minute)
+	h.Flush()
+	if len(tg.sends) != 2 {
+		t.Fatalf("%d messages, want 2", len(tg.sends))
+	}
+	for _, tk := range []string{tok, detailsOf(t, tg.sends[1])} {
+		if out, err := h.Chosen("telegram", choiceOf(tk, "42", c.now())); err != nil || out != "shown" {
+			t.Errorf("Details: %q %v", out, err)
+		}
+	}
+}

modules/messenger/cmd/messenger/holder.go

@@ -142,7 +142,7 @@ type Record struct {
 	Explanation string `json:"explanation,omitempty"`
 	Resolved    string `json:"resolved,omitempty"`
 	// Needs is what the operator does, empty for nothing; Actions the answers offered, each by a token
-	// (Tokens: token -> the action's index, or -1 for the details) a channel hands back when chosen.
+	// (Tokens: token -> its grant; Index -1 is Details, one per message shown) a channel hands back when chosen.
 	Needs   string   `json:"needs,omitempty"`
 	Actions []Action `json:"actions,omitempty"`
 	// Kept as "grants": the build before ADR 0258 kept "tokens" as token -> index, which this form cannot
@@ -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)
+			}
+		}
 	}
 }
 

modules/messenger/cmd/messenger/outbox.go

@@ -243,6 +243,14 @@ func (h *Holder) flushChannel(ch Channel, now time.Time, force bool) {
 	if len(list) == 0 || (!force && !h.due(ch, list, now)) {
 		return
 	}
+	taken := len(list)
+	// Asked before the messages are composed: composing one makes its Details ticket, and a channel that
+	// stays unready would make one at every try, pushing out the tickets of messages already shown (issue 371).
+	if err := ch.Ready(); err != nil {
+		h.noteChannel(name, err)
+		h.failed(ch, list, taken, now, err)
+		return
+	}
 	var entries []entry
 	for _, it := range list {
 		r, m := h.resolve(it, now, name)
@@ -251,16 +259,10 @@ func (h *Holder) flushChannel(ch Channel, now time.Time, force bool) {
 		}
 		entries = append(entries, entry{it, r, *m})
 	}
-	taken := len(list)
 	if len(entries) == 0 {
 		h.drop(name, taken)
 		return
 	}
-	if err := ch.Ready(); err != nil {
-		h.noteChannel(name, err)
-		h.failed(ch, list, taken, now, err)
-		return
-	}
 	if !h.allow(name) {
 		// The last line of defence: said once, in a message of its own, and the rest held.
 		h.mu.Lock()