amadan

carlos / platform Public

console: thread period through /fleet/economics/accounts's list controls

Fix round: two review findings, one fix.

IMPORTANT — every list control (search form, NextURL, AllURL, pos-filter
chips) silently dropped ?period=, so "Next 50" on a non-mtd period resumed
a cursor computed against a different period's row order under mtd's
numbers. Fix: econAccountQuery gains Period (set by the handler from
view.Period, the NORMALIZED value LoadEconomics actually priced rows
under, never the raw query param); a new econLinkURL builder threads it
(when != "mtd") into AllURL, every econPosLink.Href, and NextURL; the
search form gains a hidden period field when non-default.

Mirror: the period nav links dropped q=/pos= entirely, throwing away the
operator's search on every period switch. Ruling: period links preserve Q
and Pos but drop Cursor (a cursor decodes to an account-name resume point
computed against ONE period's row order — meaningless against another's,
so a period switch always lands on page one of the still-filtered view).
New econPeriodURL template func implements this.

Tests: econlistquery_test.go gains TestPageEconAccountsNextURLCarriesPeriod
(NextURL/AllURL/chip Href carry period=last, omit it for mtd/unset) and
TestEconPeriodURLPreservesQAndPosDropsCursor (q/pos preserved, cursor
never threaded, period=mtd omitted). economicspage_test.go gains
TestEconomicsAccountsPageThreadsPeriod: seeds golden usage inside LAST
calendar month specifically so ?period=last&pos=partial has a real
matching row, then asserts the hidden period field, a chip href carrying
period=last, and the Month-to-date period link preserving pos=partial
with no period=/cursor=.

go test ./internal/console/ -run Econ -count=1: all pass (23 cases).
go build ./..., go vet ./internal/console/, full go test ./... -count=1
(27 packages) all clean; gofmt clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 4f2131aab395664d7699161ec416db261c1a82a1 parent a6713a8
5 files changed, +242 −14
  • internal/console/econlistquery.go +64 −9
  • internal/console/econlistquery_test.go +78 −0
  • internal/console/economicspage_test.go +74 −0
  • internal/console/server.go +11 −0
  • internal/console/templates/economics_accounts.html +15 −5
diff --git a/internal/console/econlistquery.go b/internal/console/econlistquery.go
index 82dde571e..cc96d34c1 100644
--- a/internal/console/econlistquery.go
+++ b/internal/console/econlistquery.go
@@ -40,6 +40,18 @@ type econAccountQuery struct {
Q string // substring match on account, trimmed
Pos string // FreeTierPos filter ("" = all)
Cursor string // opaque; list resumes after the account inside it
+ // Period is the normalized period this page's rows were priced under
+ // ("mtd"/"last"/"7d" — EconomicsView.Period, NOT the raw "period" query
+ // param, which LoadEconomics's normalizeEconomicsPeriod may have
+ // silently rewritten). Set by the handler AFTER LoadEconomics, never by
+ // parseEconAccountQuery: every link this page emits (search form,
+ // pos-filter chips, cursor pager, period nav) must carry the SAME
+ // period that produced the rows on screen, or "Next 50" on a non-mtd
+ // period silently resumes a cursor computed against a different
+ // period's row order under mtd's numbers — the bug this field exists
+ // to close. "" behaves identically to "mtd" everywhere below (both are
+ // the default, so both are omitted from a built URL).
+ Period string
}
func parseEconAccountQuery(v url.Values) econAccountQuery {
@@ -86,11 +98,15 @@ func decodeEconAccountCursor(s string) (string, bool) {
}
// econPosLink is one header chip: a free-tier position, its count over ALL
-// the page's rows, and the link that filters to it — healthLink's shape.
+// the page's rows, and the link that filters to it — healthLink's shape,
+// plus a precomputed Href (unlike healthLink, whose template builds hrefs
+// inline) so period-carrying is done once, correctly, in Go rather than
+// re-derived at every call site in the template.
type econPosLink struct {
Word string
Count int
Active bool
+ Href string
}
// econAccountsPage is one screen of the accounts list: the rows to render
@@ -100,10 +116,54 @@ type econAccountsPage struct {
Counts map[string]int // free-tier position -> count over ALL rows
Total int // rows matching Q+Pos, across all pages
Links []econPosLink // non-zero positions, econPosWords order
+ AllURL string // clears Pos, keeps Q+Period — the "All" link
NextCursor string // "" on the last page
NextURL string // the whole next-page href; "" on the last page
}
+// econLinkURL builds one of this page's own-filter links (AllURL, a
+// econPosLink.Href, or NextURL before the cursor is appended): Q and
+// Period (when set and non-default) always carry forward; pos is whatever
+// the caller wants that ONE link to filter to ("" clears the filter).
+// Never carries Cursor — every caller of this helper is building a link to
+// page ONE of a (possibly still-filtered) view, not a cursor resume.
+func econLinkURL(q econAccountQuery, pos string) url.Values {
+ v := url.Values{}
+ if q.Q != "" {
+ v.Set("q", q.Q)
+ }
+ if pos != "" {
+ v.Set("pos", pos)
+ }
+ if q.Period != "" && q.Period != "mtd" {
+ v.Set("period", q.Period)
+ }
+ return v
+}
+
+// econPeriodURL builds the href for the period-switch nav
+// (economics_accounts.html): switches to period, preserves Q and Pos (an
+// operator's search/filter survives a period switch), but drops Cursor —
+// a cursor decodes to an account-name resume point computed against ONE
+// period's row order; carrying it into a different period's differently
+// filtered/ordered set is meaningless, so a period switch always lands on
+// page one of the (still-filtered) view. Registered as a template func
+// (server.go) since it takes two arguments, unlike the single-struct
+// econAccountsPage fields above.
+func econPeriodURL(q econAccountQuery, period string) string {
+ v := url.Values{}
+ if q.Q != "" {
+ v.Set("q", q.Q)
+ }
+ if q.Pos != "" {
+ v.Set("pos", q.Pos)
+ }
+ if period != "" && period != "mtd" {
+ v.Set("period", period)
+ }
+ return "?" + v.Encode()
+}
+
// pageEconAccounts sorts rows by account name ASC (a stable cursor order
// independent of EconomicsView.Accounts's own CostUSD-desc ordering — see
// economicsview.go's doc comment on that sort), applies q, and cuts one
@@ -143,20 +203,15 @@ func pageEconAccounts(rows []econAccountRow, q econAccountQuery, limit int) econ
end = len(matched)
}
p.Rows = matched[start:end]
+ p.AllURL = "?" + econLinkURL(q, "").Encode()
for _, w := range econPosWords {
if p.Counts[w] == 0 {
continue
}
- p.Links = append(p.Links, econPosLink{Word: w, Count: p.Counts[w], Active: q.Pos == w})
+ p.Links = append(p.Links, econPosLink{Word: w, Count: p.Counts[w], Active: q.Pos == w, Href: "?" + econLinkURL(q, w).Encode()})
}
if p.NextCursor != "" {
- v := url.Values{}
- if q.Q != "" {
- v.Set("q", q.Q)
- }
- if q.Pos != "" {
- v.Set("pos", q.Pos)
- }
+ v := econLinkURL(q, q.Pos)
v.Set("cursor", p.NextCursor)
p.NextURL = "?" + v.Encode()
}
diff --git a/internal/console/econlistquery_test.go b/internal/console/econlistquery_test.go
index d915decfe..5ace072e4 100644
--- a/internal/console/econlistquery_test.go
+++ b/internal/console/econlistquery_test.go
@@ -3,6 +3,7 @@ package console
import (
"fmt"
"net/url"
+ "strings"
"testing"
"github.com/carlosframework/platform/internal/economics"
@@ -111,6 +112,83 @@ func TestPageEconAccountsGarbageCursorStartsOver(t *testing.T) {
}
}
+// TestPageEconAccountsNextURLCarriesPeriod is the fix-round regression for
+// the silent-wrong-data bug: NextURL (and, by the same econLinkURL builder,
+// AllURL/Links[].Href) must carry period="last"/"7d" so a "Next 50" click
+// doesn't resume a cursor computed against one period's row order under
+// another period's numbers — but must OMIT period=mtd, mtd being the
+// implicit default every other link on this page already omits it for.
+func TestPageEconAccountsNextURLCarriesPeriod(t *testing.T) {
+ rows := econRowsFor(120)
+
+ pLast := pageEconAccounts(rows, econAccountQuery{Period: "last"}, econAccountPageSize)
+ if pLast.NextCursor == "" {
+ t.Fatalf("expected a NextCursor over 120 rows at page size %d", econAccountPageSize)
+ }
+ if !strings.Contains(pLast.NextURL, "period=last") {
+ t.Fatalf("NextURL = %q, want it to carry period=last", pLast.NextURL)
+ }
+ if !strings.Contains(pLast.AllURL, "period=last") {
+ t.Fatalf("AllURL = %q, want it to carry period=last", pLast.AllURL)
+ }
+
+ pMTD := pageEconAccounts(rows, econAccountQuery{Period: "mtd"}, econAccountPageSize)
+ if strings.Contains(pMTD.NextURL, "period=") {
+ t.Fatalf("NextURL = %q, want no period param for the default mtd", pMTD.NextURL)
+ }
+ if strings.Contains(pMTD.AllURL, "period=") {
+ t.Fatalf("AllURL = %q, want no period param for the default mtd", pMTD.AllURL)
+ }
+
+ // The zero value (Period: "") behaves identically to "mtd" — omitted.
+ pZero := pageEconAccounts(rows, econAccountQuery{}, econAccountPageSize)
+ if strings.Contains(pZero.NextURL, "period=") {
+ t.Fatalf("NextURL = %q, want no period param when Period is unset", pZero.NextURL)
+ }
+
+ // A pos-filter link (Links[].Href) carries period too — resuming the
+ // SAME bug from a different link on the page would be an incomplete fix.
+ rows[0].Econ.FreeTierPos = "inside"
+ pLastFiltered := pageEconAccounts(rows, econAccountQuery{Period: "last"}, econAccountPageSize)
+ var insideHref string
+ for _, l := range pLastFiltered.Links {
+ if l.Word == "inside" {
+ insideHref = l.Href
+ }
+ }
+ if !strings.Contains(insideHref, "period=last") {
+ t.Fatalf("inside link Href = %q, want it to carry period=last", insideHref)
+ }
+}
+
+// TestEconPeriodURLPreservesQAndPosDropsCursor is the mirror fix: the
+// period-switch nav preserves Q and Pos (an operator's search survives a
+// period switch) but never carries a cursor — econAccountQuery has no
+// Cursor parameter for econPeriodURL to accidentally thread through in the
+// first place, so this pins that shape at the signature level too.
+func TestEconPeriodURLPreservesQAndPosDropsCursor(t *testing.T) {
+ q := econAccountQuery{Q: "acct-01", Pos: "partial", Cursor: "some-cursor-that-must-never-appear"}
+ href := econPeriodURL(q, "last")
+ if !strings.Contains(href, "q=acct-01") {
+ t.Fatalf("href = %q, want it to preserve q=acct-01", href)
+ }
+ if !strings.Contains(href, "pos=partial") {
+ t.Fatalf("href = %q, want it to preserve pos=partial", href)
+ }
+ if !strings.Contains(href, "period=last") {
+ t.Fatalf("href = %q, want it to carry period=last", href)
+ }
+ if strings.Contains(href, "cursor") {
+ t.Fatalf("href = %q, must never carry a cursor across a period switch", href)
+ }
+
+ // Switching TO mtd omits period= (the default), same "omit when mtd" rule.
+ hrefMTD := econPeriodURL(q, "mtd")
+ if strings.Contains(hrefMTD, "period=") {
+ t.Fatalf("href = %q, want no period param when switching to the default mtd", hrefMTD)
+ }
+}
+
func TestParseEconAccountQuery(t *testing.T) {
q := parseEconAccountQuery(url.Values{"q": {" acct-01 "}, "pos": {"inside"}, "cursor": {"abc"}})
if q.Q != "acct-01" || q.Pos != "inside" || q.Cursor != "abc" {
diff --git a/internal/console/economicspage_test.go b/internal/console/economicspage_test.go
index 0980a29bb..5cc78d4af 100644
--- a/internal/console/economicspage_test.go
+++ b/internal/console/economicspage_test.go
@@ -305,3 +305,77 @@ func TestEconomicsAccountsPageContent(t *testing.T) {
}
}
}
+
+// TestEconomicsAccountsPageThreadsPeriod is the fix-round regression for
+// the silent-wrong-data bug: every list control on this page (search form,
+// pos-filter chips, period nav) must carry the SAME period the rows on
+// screen were priced under. Seeds the golden fixture inside LAST calendar
+// month's window specifically (economicsPeriodBounds("last", now)) —
+// same shape as seedEconomicsGolden, just dated a month back — so
+// ?period=last&pos=partial has a genuine matching row (this fixture's
+// egress is fully free-tier-covered under DefaultScenario, same reasoning
+// as TestEconomicsAccountsPageContent) and renders a real "partial" chip,
+// rather than asserting only on chrome that happens to render with zero
+// matches.
+func TestEconomicsAccountsPageThreadsPeriod(t *testing.T) {
+ fx := newConsoleFixture(t)
+
+ reg, err := registry.Open(fx.regPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := reg.Upsert(registry.Route{Host: "a.example.com", Kind: "instance",
+ Status: "ready", App: fixtureApp, Version: "v1"}); err != nil {
+ t.Fatal(err)
+ }
+
+ // A date safely inside last calendar month by the wall clock — the same
+ // "relative to real time" rule TestLoadEconomicsGolden documents.
+ lastMonth := time.Now().UTC().AddDate(0, -1, 0)
+ date := usage.DayOf(lastMonth)
+ roll := metrics.DayRollup{V: 1, Box: "box1", Date: date, Hours: map[string]metrics.HourAgg{
+ "09": {Units: map[string]map[string]metrics.Agg{
+ "carlos-app@a.example.com.service": {
+ "cpu": {Avg: 0.5, N: 60},
+ "mem_bytes": {Avg: 1e9, N: 60},
+ },
+ }},
+ }}
+ putStoreJSON(t, fx.rel, metrics.DayKey("box1", date), roll)
+ putStoreJSON(t, fx.rel, usage.EdgeDayKey("box1", date), usage.EdgeDay{
+ V: 1, Box: "box1", Date: date,
+ Hours: map[string]map[string]usage.EdgeRow{"09": {
+ "a.example.com": {Account: fixtureAcct, App: fixtureApp, Requests: 4000, BytesOut: 2e9},
+ }},
+ })
+
+ c := signInPaul(t, fx)
+ if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil {
+ t.Fatal(err)
+ }
+
+ status, _, body := get(t, c, fx.ts.URL+"/fleet/economics/accounts?period=last&pos=partial&cursor=abc123")
+ if status != 200 {
+ t.Fatalf("/fleet/economics/accounts?period=last = %d, want 200:\n%s", status, body)
+ }
+
+ // 1. The search form carries period forward as a hidden field.
+ if !strings.Contains(body, `<input type="hidden" name="period" value="last">`) {
+ t.Fatalf("missing hidden period field in the search form:\n%s", body)
+ }
+ // 2. A pos-filter chip's href carries period=last — this fixture's
+ // "partial" row makes that chip's presence deterministic rather than
+ // incidental. html/template escapes "&" to "&amp;" inside an href
+ // attribute value.
+ if !strings.Contains(body, `href="?period=last&amp;pos=partial"`) {
+ t.Fatalf("missing the partial-position chip href carrying period=last:\n%s", body)
+ }
+ // 3. The period nav preserves pos= (the operator's filter survives a
+ // period switch) but never a cursor. The "Month to date" link
+ // switches TO the default period, so period= itself is omitted (the
+ // same "omit when mtd" rule as everywhere else on this page),
+ // leaving exactly "?pos=partial" — no period=, no cursor=.
+ if !strings.Contains(body, `href="?pos=partial"`) {
+ t.Fatalf("missing the Month-to-date period link preserving pos=partial with no period/cursor:\n%s", body)
+ }
+}
diff --git a/internal/console/server.go b/internal/console/server.go
index 2f9b656a8..1b322b615 100644
--- a/internal/console/server.go
+++ b/internal/console/server.go
@@ -357,6 +357,10 @@ var templates = template.Must(template.New("").Funcs(template.FuncMap{
raw, err := json.Marshal(sc)
return string(raw), err
},
+ // econPeriodURL: /fleet/economics/accounts's period-switch nav
+ // (economics_accounts.html) — see econlistquery.go's doc comment on the
+ // func itself for why it preserves Q/Pos but drops Cursor.
+ "econPeriodURL": econPeriodURL,
}).ParseFS(tmplFS, "templates/*.html"))
const (
@@ -1736,6 +1740,13 @@ func (s *Server) handleEconomicsAccounts(w http.ResponseWriter, r *http.Request)
rows = append(rows, econAccountRow{Account: ae.Account, Meters: view.Meters[ae.Account], Econ: ae})
}
q := parseEconAccountQuery(r.URL.Query())
+ // Period is the NORMALIZED value LoadEconomics actually priced rows
+ // under (view.Period), not the raw query param — every link this page
+ // builds (search form, pos chips, cursor pager, period nav) must carry
+ // the period that produced what's on screen, or "Next 50" on a non-mtd
+ // period would resume a cursor computed against a different period's
+ // row order. See econAccountQuery.Period's doc comment.
+ q.Period = view.Period
pg := pageEconAccounts(rows, q, econAccountPageSize)
p := page{Title: "Fleet economics: accounts", Address: addr, CSRF: raw, Economics: view, EconAccounts: pg, EconQuery: q}
p.Flash, p.FlashErr = s.takeFlash(w, r)
diff --git a/internal/console/templates/economics_accounts.html b/internal/console/templates/economics_accounts.html
index 6a39b73f8..f8c693f67 100644
--- a/internal/console/templates/economics_accounts.html
+++ b/internal/console/templates/economics_accounts.html
@@ -32,24 +32,34 @@
<p class="small">Generated {{fmtTime .Economics.GeneratedAt}} &middot; scenario <strong>{{.Economics.Scenario.Name}}</strong> ({{.Economics.Scenario.Mode}}) &middot; {{.Economics.CoveredDays}}/{{.Economics.PeriodDays}} days metered</p>
+{{/* Period nav: switching period keeps the operator's search/filter
+ (Q/Pos) — econPeriodURL — but drops any cursor, since a cursor resumes
+ an account-name position computed against ONE period's row order and
+ means nothing against another period's. */}}
<nav class="small" aria-label="Period">
Period:
- <a href="?period=mtd"{{if eq .Economics.Period "mtd"}} aria-current="page"{{end}}>Month to date</a> &middot;
- <a href="?period=last"{{if eq .Economics.Period "last"}} aria-current="page"{{end}}>Last month</a> &middot;
- <a href="?period=7d"{{if eq .Economics.Period "7d"}} aria-current="page"{{end}}>Last 7 days</a>
+ <a href="{{econPeriodURL .EconQuery "mtd"}}"{{if eq .Economics.Period "mtd"}} aria-current="page"{{end}}>Month to date</a> &middot;
+ <a href="{{econPeriodURL .EconQuery "last"}}"{{if eq .Economics.Period "last"}} aria-current="page"{{end}}>Last month</a> &middot;
+ <a href="{{econPeriodURL .EconQuery "7d"}}"{{if eq .Economics.Period "7d"}} aria-current="page"{{end}}>Last 7 days</a>
</nav>
<p class="small"><a href="/fleet/economics">&larr; Back to the dashboard</a></p>
{{if or .EconAccounts.Rows .EconQuery.Q .EconQuery.Pos}}
+{{/* The hidden period field only needs to exist for a non-default period:
+ "mtd" is what an absent "period" param already means, matching
+ econLinkURL/econPeriodURL's own "omit when mtd" rule — so a search
+ submitted on the default period doesn't grow a redundant ?period=mtd
+ into every subsequent link. */}}
<form method="get" class="listctl">
<input type="search" name="q" value="{{.EconQuery.Q}}" placeholder="Filter accounts" aria-label="Filter accounts">
{{if .EconQuery.Pos}}<input type="hidden" name="pos" value="{{.EconQuery.Pos}}">{{end}}
+ {{if ne .EconQuery.Period "mtd"}}<input type="hidden" name="period" value="{{.EconQuery.Period}}">{{end}}
<button>Filter</button>
</form>
<p class="small">
- <a href="?{{if .EconQuery.Q}}q={{.EconQuery.Q}}{{end}}"{{if not .EconQuery.Pos}} aria-current="true"{{end}}>All</a>
+ <a href="{{.EconAccounts.AllURL}}"{{if not .EconQuery.Pos}} aria-current="true"{{end}}>All</a>
{{range .EconAccounts.Links}}
- &middot; <a href="?pos={{.Word}}{{if $.EconQuery.Q}}&amp;q={{$.EconQuery.Q}}{{end}}"{{if .Active}} aria-current="true"{{end}}>{{.Word}} {{.Count}}</a>
+ &middot; <a href="{{.Href}}"{{if .Active}} aria-current="true"{{end}}>{{.Word}} {{.Count}}</a>
{{end}}
</p>
{{end}}