diff --git a/modules/messenger/README.md b/modules/messenger/README.md index bb915243..e56b15d3 100644 --- a/modules/messenger/README.md +++ b/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 diff --git a/modules/messenger/cmd/messenger/asks.go b/modules/messenger/cmd/messenger/asks.go index 6670e369..33e7459a 100644 --- a/modules/messenger/cmd/messenger/asks.go +++ b/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) diff --git a/modules/messenger/cmd/messenger/holder.go b/modules/messenger/cmd/messenger/holder.go index 2eb5676a..55bdd51d 100644 --- a/modules/messenger/cmd/messenger/holder.go +++ b/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() diff --git a/modules/messenger/cmd/messenger/replaced_test.go b/modules/messenger/cmd/messenger/replaced_test.go new file mode 100644 index 00000000..100e0b80 --- /dev/null +++ b/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) + } +}