Merge pull request 'messenger: a replaced ask's own message ends with it, and a cleared record owes nothing (hq issue 548)' (#241) from fix/548-replaced-ask-message into main

modules/messenger/README.md

@@ -148,7 +148,10 @@ new message is shown, so it rings once and no second message stands beside the f
 notification by its id; Telegram deletes the earlier message). A message said again goes alone, never as a
 line of a digest beside the message it follows; one first said inside a digest gets a message of its own the
 next time, and later words replace or edit that one. An ask's buttons live on that one message, so an ask that
-ends takes them away everywhere; its expiry is said with its day when it is not today.
+ends takes them away everywhere; its expiry is said with its day when it is not today. An ask replaced by a
+newer one about the same thing ends its own message too, which then says "Replaced by a newer question"; the
+newer ask shows a message of its own (issue 548). A message left open by an ask that already ended is ended on
+the next minute's sweep.
 
 - **Routing:** urgent to every kind that can send; a warning to the desk where it can send, otherwise to
   the channel that reaches the operator away; an ask to every kind that carries one of its answers.
@@ -187,7 +190,8 @@ The controller's `plan-moved` says each walk's start and end: `walk`, `phase` (`
   open, one line each; the ones that cleared counted. Delivered with nothing held and nothing open: one line —
   and nothing on a kind listed in the setting `only-what-needs-me`. What it carries is marked told on a kind
   only once the message was sent there. Until then each is kept owed in the state (`owed`): a message lost with a
-  restart before it was sent leaves them untold and owed, and the restarted router says each on its own.
+  restart before it was sent leaves them untold and owed, and the restarted router says each on its own. A
+  record that clears owes nothing more.
 - **The bound:** at its start plus `bound_seconds` (900 when not given), what is held and still open is said,
   with "The walk … is still running, since HH:MM", and the cleared counted; the walk holds nothing more. A walk
   whose end never comes is forgotten a day after it started. A start heard after its end opens nothing: the

modules/messenger/cmd/messenger/asks.go

@@ -691,30 +691,58 @@ func (h *Holder) showClosed(rec AskRecord) {
 	}
 	told := h.toldLine(rec) // read before the lock: it reads the ask's record
 	h.mu.Lock()
+	ownMessage := r.Origin == "ask"
 	// The first line is how it ended, which an ask's own message shows as its title (sayOn); the rest follows.
 	switch {
 	case rec.Warrant != nil && rec.Warrant.Outcome == asks.OutcomeChosen:
 		r.Answered = chosenLine(rec.Warrant, rec.Closed.In(h.zone())) + "\n" + told
 	case rec.State == string(asks.OutcomeExpired):
 		r.Answered = "No longer answerable: nobody answered in time.\n" + strings.TrimSuffix(capital(rec.Ask.OnExpiry), ".") + "."
-	case rec.State == string(asks.OutcomeReplaced):
+	case rec.State == string(asks.OutcomeReplaced) && !ownMessage:
+		// On a condition's message, which the newer ask takes over and shows in place.
 		r.Answered = ""
+	case rec.State == string(asks.OutcomeReplaced):
+		// Its own message: the newer ask looks for its message under its own key and opens one of its own, so
+		// this one ends here, on every channel that showed it (issue 548).
+		r.Answered = "Replaced by a newer question.\n" + rec.Asker + " asked again about the same thing; that question is the one to answer."
 	default:
 		r.Answered = "No longer needed."
 	}
-	ownMessage := r.Origin == "ask"
 	h.mu.Unlock()
 	if r.told() {
 		h.edit(r, "answered", false)
 	}
-	if ownMessage && rec.State != string(asks.OutcomeReplaced) {
+	if ownMessage {
 		h.mu.Lock()
 		r.Cleared = h.Now()
+		r.Owed = "" // an ended question is owed to nobody (the review of PR 238)
 		h.mu.Unlock()
 	}
 	h.persist(r)
 }
 
+// endLeftovers ends the own message of an ask that already ended and whose message stayed open: one left by a
+// router before issue 548 was fixed, whose replaced asks kept their messages for good. Its copies say how the
+// ask ended, as its end would have.
+func (h *Holder) endLeftovers() {
+	h.mu.Lock()
+	var ended []AskRecord
+	for _, r := range h.open {
+		if r.Origin != "ask" || r.Ask == "" || !r.Cleared.IsZero() {
+			continue
+		}
+		if e := h.asks[r.Ask]; e != nil && e.rec.State != StateOpen && e.rec.Record == r.Key {
+			ended = append(ended, e.rec)
+		}
+	}
+	h.mu.Unlock()
+	sort.Slice(ended, func(i, j int) bool { return ended[i].Record < ended[j].Record })
+	for _, rec := range ended {
+		h.Logf("[messenger] the message of %s stayed open after its ask ended (%s): ended now", rec.Record, rec.State)
+		h.showClosed(rec)
+	}
+}
+
 // chosenLine is how a chosen ask ended, in the operator's words: "You chose Approve at 13:30 on Telegram."
 func chosenLine(w *asks.Warrant, at time.Time) string {
 	who, where := "Somebody", ""
@@ -775,6 +803,7 @@ func (h *Holder) sweepAsks(now time.Time) {
 		}
 	}
 	h.mu.Unlock()
+	h.endLeftovers()
 	sort.Strings(expired)
 	for _, k := range expired {
 		e := h.askOf(k)

modules/messenger/cmd/messenger/holder.go

@@ -687,6 +687,7 @@ func (h *Holder) cleared(key string, quiet bool) {
 	}
 	old.Cleared = now
 	old.Pending = ""
+	old.Owed = "" // what a walk held of it is owed no more: it ended (the review of PR 238)
 	old.Folded = false
 	old.Seen = now
 	told := old.told()

modules/messenger/cmd/messenger/replaced_test.go

@@ -0,0 +1,111 @@
+package main
+
+import (
+	"strings"
+	"testing"
+	"time"
+
+	"git.novox.be/novox/mesh-sdk/go/asks"
+)
+
+// openAskRecords are the records still open that show an ask as their own message.
+func openAskRecords(h *Holder) []string {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	var out []string
+	for k, r := range h.open {
+		if r.Origin == "ask" && r.Cleared.IsZero() {
+			out = append(out, k)
+		}
+	}
+	return out
+}
+
+// Issue 548: an ask replaced by a newer one about the same thing kept its own message open for good, its
+// buttons on the phone answering nothing. Two asks with the same About from one asker leave exactly one open
+// record, and the older one's message says it was replaced, its buttons gone, on every channel that showed it.
+func TestAReplacedAsksMessageEndsWithIt(t *testing.T) {
+	h, tg, dt, c, store, _ := newRouter(t)
+	link(h, store, c, "42", c.now().Add(-2*time.Hour))
+	a := ackAsk(c, "d-1")
+	a.About = "delivery.hq"
+	submit(t, h, "mesh-delivery", a)
+	settle(h, c)
+	if len(tg.sends) != 1 || len(dt.sends) != 1 {
+		t.Fatalf("the first ask was not shown: telegram %d, desktop %d", len(tg.sends), len(dt.sends))
+	}
+	b := ackAsk(c, "d-2")
+	b.About = "delivery.hq"
+	b.Headline = "Delivery of hq held, asked again"
+	submit(t, h, "mesh-delivery", b)
+	settle(h, c)
+	if got := openAskRecords(h); len(got) != 1 || got[0] != "ask.mesh-delivery.d-2" {
+		t.Fatalf("open ask records: %v; want only the newer ask's", got)
+	}
+	for _, ch := range []*fakeChannel{tg, dt} {
+		id := map[string]string{"telegram": "1", "desktop": "g14=1"}[ch.name]
+		m, edited := ch.edits[id]
+		for _, r := range ch.replaced {
+			if r.id == id {
+				m, edited = r.m, true
+			}
+		}
+		if !edited || len(m.Actions) != 0 || !strings.Contains(m.Title, "Replaced by a newer question") {
+			t.Errorf("%s: the replaced ask's message still stands: edited %v, %q %v", ch.name, edited, m.Text(), labelsOf(m))
+		}
+	}
+	if store.asks["mesh-delivery.d-1"].State != string(asks.OutcomeReplaced) {
+		t.Errorf("the older ask was not replaced: %s", store.asks["mesh-delivery.d-1"].State)
+	}
+}
+
+// The leftover of issue 548: a record from before the fix whose ask already ended keeps its message open. The
+// next tick ends it, as the ask's end would have.
+func TestAnAsksMessageLeftOpenAfterItsAskEndedIsEndedOnTheNextTick(t *testing.T) {
+	h, tg, _, c, store, _ := newRouter(t)
+	link(h, store, c, "42", c.now().Add(-2*time.Hour))
+	a := ackAsk(c, "d-1")
+	a.About = "delivery.hq"
+	submit(t, h, "mesh-delivery", a)
+	settle(h, c)
+	// Ended as replaced the way the router did before the fix: the ask's record only, its message untouched.
+	key := "mesh-delivery.d-1"
+	rec := store.asks[key]
+	rec.State, rec.Closed, rec.Told = string(asks.OutcomeReplaced), c.now(), true
+	rec.Warrant = &asks.Warrant{Ask: "d-1", Asker: "mesh-delivery", About: a.About, Outcome: asks.OutcomeReplaced,
+		Words: "a newer ask about the same thing replaced it", At: c.now()}
+	store.asks[key] = rec
+	h.keepAsk(key, rec, 99)
+	if got := openAskRecords(h); len(got) != 1 {
+		t.Fatalf("the leftover is not set up: %v", got)
+	}
+	c.pass(time.Minute)
+	stand(h, c)
+	h.Tick()
+	h.Flush()
+	if got := openAskRecords(h); len(got) != 0 {
+		t.Fatalf("the message of an ended ask is still open: %v", got)
+	}
+	if m, ok := tg.edits["1"]; !ok || len(m.Actions) != 0 || !strings.Contains(m.Title, "Replaced by a newer question") {
+		t.Errorf("the leftover's copy was not ended: %v %q %v", ok, m.Text(), labelsOf(m))
+	}
+}
+
+// The review of PR 238: what a walk owed of a record is dropped when the record clears, so a router that
+// restarts never says a held message of something that already ended.
+func TestAClearedRecordOwesNothing(t *testing.T) {
+	h, _, _, c, st := newHolder(t)
+	k := cond("machine.shanks.units", Warning, "2 failed services on shanks")
+	k.Raised = c.now()
+	h.Condition(EventRaised, k)
+	h.mu.Lock()
+	h.open[k.Key].Owed = "raised"
+	h.mu.Unlock()
+	h.cleared(k.Key, false)
+	if r := h.open[k.Key]; r.Owed != "" {
+		t.Errorf("a cleared record still owes %q", r.Owed)
+	}
+	if r := st.recs[k.Key]; r.Owed != "" {
+		t.Errorf("a cleared record still owes %q in the state", r.Owed)
+	}
+}