messenger: every message still shown keeps its own Details, bounded in number and age (issue 371)

A newer message about the same problem (an escalation, a reminder, a re-send) replaced the Details
ticket of the one before it on that channel kind, so tapping Details on any but the newest was refused
"it belongs to no open message". Each message now keeps a ticket of its own, used once, at most
MostDetails per record and kind and none older than TokenFor; one pushed out is refused in words that
send the operator to the newest message. A change of the condition's actions no longer drops Details,
which names no action.

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,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)
+	}
+}

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)
+			}
+		}
 	}
 }