Name the person by id in a forum-link error, and date billing by Stripe's events

Two fixes ride along with #163, each listed in its change. When the forum reports an account created and the lookup that follows finds nothing, confirmCreated's error named the person by their email address, which reached the logs and the targeted reconcile's activity failure in Temporal. It now names the person by id; the rest of its text is unchanged. The other copies of personal data in Discourse errors are #250 (logs) and #162 (Temporal). The operator billing views are projected from Stripe's webhook events, but their "Last updated" stamp took the newest processed event of any provider, so a processed forum event moved it forward while Stripe's stream could have stopped. The stamp now reads Stripe's events only (latestProcessedStripeEventAt), and the operator-billing-views spec says so. Deliberate breaks, scripts/mutation-check -side new, all CAUGHT: the confirmation error carries the email again the confirmation error drops the person's id the stamp's provider condition removed (both subtests)
2026-10-22 19:32:15 -04:00
parent 5219a45a4b
commit 8e1127edd8
7 changed files with 101 additions and 21 deletions
@@ -15,7 +15,7 @@
     already names Billing / Invoices (maintainer 2026-08-31). .ActiveSection
     drives both the active pill and the page's IAPosition
     (operator_pages.go). The recency stamp renders only when
-     provider events exist; a quiet stream says nothing here (maintainer,
+     Stripe events exist; a quiet stream says nothing here (maintainer,
     2026-08-30: the zero-events banner conflated the provider stream with
     the records below it; stream health belongs to the Stripe integration
     settings page). Delegates to .InnerTemplate / .InnerData via
@@ -259,7 +259,7 @@ func (l *Linker) confirmCreated(ctx context.Context, ident dcmod.GetPersonIdenti
 		return nil, fmt.Errorf("confirm created forum user: %w", err)
 	}
 	if u == nil {
-		return nil, fmt.Errorf("forum reported user created for %q but post-create lookup found nothing (duplicate email swallowed by enumeration protection?)", ident.PrimaryEmail)
+		return nil, fmt.Errorf("forum reported user created for person %s but post-create lookup found nothing (duplicate email swallowed by enumeration protection?)", ident.PersonID)
 	}
 	return u, nil
 }
@@ -9,6 +9,7 @@ import (
 	"errors"
 	"log/slog"
 	"os"
+	"strings"
 	"testing"

 	"git.coopcloud.tech/wiki-cafe/member-console/internal/identity"
@@ -196,6 +197,36 @@ func TestAutoCreateOIDC(t *testing.T) {
 	}
 }

+// Scenario: auto-create in oidc mode where the forum swallows the create as
+// a duplicate email → the confirmation finds nobody, and its error names the
+// person by id, never by email address (the error reaches logs and Temporal).
+func TestAutoCreateUnconfirmedErrorNamesPersonByID(t *testing.T) {
+	f, personID := newFixture(t)
+	// No external id, and a username other than the seed usernameBase derives
+	// from the email, so the lookup by subject finds nobody and the create is
+	// swallowed as a duplicate email.
+	f.fake.AddUser(discoursetest.FakeUser{
+		Username: "someone-else", Email: f.email, Active: true,
+	})
+
+	outcome, err := f.linker(linkage.ModeOIDC, true).EnsureLink(f.ctx, f.q, personID)
+	if err == nil {
+		t.Fatalf("EnsureLink = (%q, nil), want the confirmation error", outcome)
+	}
+	if !strings.Contains(err.Error(), personID) {
+		t.Errorf("error %q does not contain the person's id %q", err, personID)
+	}
+	if !strings.Contains(err.Error(), "post-create lookup found nothing") {
+		t.Errorf("error %q is not the confirmation path's error", err)
+	}
+	if strings.Contains(err.Error(), f.email) {
+		t.Errorf("error %q contains the person's email address", err)
+	}
+	if _, err := f.q.GetUserLinkByPersonID(f.ctx, personID); !errors.Is(err, sql.ErrNoRows) {
+		t.Errorf("expected no link, got err=%v", err)
+	}
+}
+
 // Scenario: the resolved forum user is already linked to a different person
 // → existing link marked conflict, no reassignment, this person unlinked.
 func TestConflictNeverReassigns(t *testing.T) {
@@ -246,20 +246,27 @@ type BillingAccountsData struct {
 	Nav ListNav
 }

-// latestProcessedWebhookEventAt returns the most recent time any provider's
+// rowQuerier is the one-row read both *sql.DB and *sql.Tx provide.
+type rowQuerier interface {
+	QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row
+}
+
+// latestProcessedStripeEventAt returns the most recent time a Stripe
 // webhook event finished processing (processed_at IS NOT NULL): the four
-// operator billing views are projections from these events, so this is the
-// recency stamp the honest-surfaces delta requires (design.md Decision 3;
-// spec: operator-billing-views) — "data as of ..." when at least one event
-// has ever finished processing, ok=false when none has (a fresh deployment,
-// or Stripe never configured), so the caller can state that plainly instead
-// of rendering an empty or stale-looking table as if it were current.
-func (h *OperatorPartialsHandler) latestProcessedWebhookEventAt(ctx context.Context) (time.Time, bool) {
+// operator billing views are projections from Stripe's events, so this is
+// the recency stamp the honest-surfaces delta requires (design.md Decision 3;
+// spec: operator-billing-views) — "data as of ..." when at least one such
+// event has ever finished processing, ok=false when none has (a fresh
+// deployment, or Stripe never configured), so the caller can state that
+// plainly instead of rendering an empty or stale-looking table as if it were
+// current. Other providers' events share the table and project nothing into
+// the billing views, so they do not move the stamp.
+func latestProcessedStripeEventAt(ctx context.Context, db rowQuerier, logger *slog.Logger) (time.Time, bool) {
 	var latest sql.NullTime
-	if err := h.Database.QueryRowContext(ctx,
-		`SELECT MAX(processed_at) FROM core.webhook_events WHERE processed_at IS NOT NULL`,
+	if err := db.QueryRowContext(ctx,
+		`SELECT MAX(processed_at) FROM core.webhook_events WHERE provider = 'stripe' AND processed_at IS NOT NULL`,
 	).Scan(&latest); err != nil {
-		h.Logger.Error("failed to load latest processed webhook event", "error", err)
+		logger.Error("failed to load latest processed Stripe webhook event", "error", err)
 		return time.Time{}, false
 	}
 	if !latest.Valid {
@@ -232,3 +232,45 @@ func TestOperatorBillingRecencyStamp(t *testing.T) {
 		}
 	})
 }
+
+// TestLatestProcessedStripeEventAt covers the recency stamp's provider scope
+// (operator-billing-views, "Billing views disclose projection recency"): a
+// processed Discourse event after the last Stripe event does not move the
+// stamp, and with no processed Stripe event there is no stamp. The shared
+// table may hold committed rows from other tests, so each subtest clears the
+// Stripe rows inside its own rolled-back transaction.
+func TestLatestProcessedStripeEventAt(t *testing.T) {
+	ctx := context.Background()
+	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
+	database := dbtest.Open(t)
+
+	fixture := func(t *testing.T) (*dbtest.Q, func(provider, processedAt string)) {
+		t.Helper()
+		q := dbtest.Tx(t, database)
+		q.Exec(`DELETE FROM core.webhook_events WHERE provider = 'stripe'`)
+		return q, func(provider, processedAt string) {
+			t.Helper()
+			q.Exec(`INSERT INTO core.webhook_events (provider, provider_event_id, event_type, status, processed_at)
+				VALUES ($1, $2, 'x', 'completed', $3::timestamptz)`,
+				provider, dbtest.Name("evt"), processedAt)
+		}
+	}
+
+	t.Run("a Discourse event alone gives no stamp", func(t *testing.T) {
+		q, insert := fixture(t)
+		insert("discourse", "2026-03-05 10:00:00+00")
+		if got, ok := latestProcessedStripeEventAt(ctx, q.Tx(), logger); ok {
+			t.Errorf("only a processed Discourse event: stamp = %s, want none", got)
+		}
+	})
+
+	t.Run("a later Discourse event does not move the Stripe stamp", func(t *testing.T) {
+		q, insert := fixture(t)
+		insert("stripe", "2026-03-01 10:00:00+00")
+		insert("discourse", "2026-03-05 10:00:00+00")
+		got, ok := latestProcessedStripeEventAt(ctx, q.Tx(), logger)
+		if want := "2026-03-01 10:00:00"; !ok || got.UTC().Format("2006-01-02 15:04:05") != want {
+			t.Errorf("stamp = (%s, %v), want the Stripe event's time %s", got, ok, want)
+		}
+	})
+}
@@ -450,7 +450,7 @@ type OperatorBillingWrapperData struct {
 	// DataAsOf carries the webhook-projection recency stamp (design.md
 	// Decision 3; spec: operator-billing-views) shared by all four billing
 	// sub-pages: renderBillingPage computes it once here. Empty when no
-	// events have been processed, in which case the wrapper says nothing
+	// Stripe events have been processed, in which case the wrapper says nothing
 	// (maintainer, 2026-08-30: the zero-events banner conflated the
 	// provider stream with the records below it).
 	DataAsOf string
@@ -500,7 +500,7 @@ func (h *OperatorPartialsHandler) renderBillingPage(w http.ResponseWriter, r *ht
 		InnerData:     innerData,
 		TestMode:      h.StripeMode == "test",
 	}
-	if processedAt, ok := h.latestProcessedWebhookEventAt(r.Context()); ok {
+	if processedAt, ok := latestProcessedStripeEventAt(r.Context(), h.Database, h.Logger); ok {
 		wrapper.DataAsOf = processedAt.Format("Jan 2, 2006 15:04 MST")
 	}
 	wrapper.Header = PageHeader{Title: "Billing"}
@@ -31,14 +31,14 @@ Sections 1 to 3 are one commit each, and their items are that commit's parts; se

 ## 2. Ride-alongs: the confirmation error names the person by id, and the billing stamp reads Stripe's events

- [ ] 2.1 In `confirmCreated` (`linkage/linkage.go:262`), the error names the person by id: `for person %s` with `ident.PersonID` in place of `for %q` with `ident.PrimaryEmail`; the rest of the text stays. The error at `:241` stays as it is (design D9).
- [ ] 2.2 Test in `linkage/linkage_test.go`, `TestAutoCreateUnconfirmedErrorNamesPersonByID`: `oidc` mode with auto-create on; the fake holds a forum user whose email is the person's and who has no external id, and whose username differs from the seed `usernameBase` derives, so the lookup by subject finds nobody, the fake swallows the create as a duplicate email, and the confirmation finds nothing. `EnsureLink` returns an error that contains the person's id and does not contain the person's email address; no link is recorded.
- [ ] 2.3 Breaks:
+- [x] 2.1 In `confirmCreated` (`linkage/linkage.go:262`), the error names the person by id: `for person %s` with `ident.PersonID` in place of `for %q` with `ident.PrimaryEmail`; the rest of the text stays. The error at `:241` stays as it is (design D9).
+- [x] 2.2 Test in `linkage/linkage_test.go`, `TestAutoCreateUnconfirmedErrorNamesPersonByID`: `oidc` mode with auto-create on; the fake holds a forum user whose email is the person's and who has no external id, and whose username differs from the seed `usernameBase` derives, so the lookup by subject finds nobody, the fake swallows the create as a duplicate email, and the confirmation finds nothing. `EnsureLink` returns an error that contains the person's id and does not contain the person's email address; no link is recorded.
+- [x] 2.3 Breaks:
  - the error carries the email again (`ident.PrimaryEmail` added beside the id): caught by the no-email assertion;
  - the error drops the id (`ident.PersonID` replaced by a fixed word): caught by the person-id assertion.
- [ ] 2.4 The billing views' recency stamp (`latestProcessedWebhookEventAt`, `internal/server/operator_billing.go`) reads only Stripe's events: its query gains `provider = 'stripe'`, and its doc comment says that the views are projected from Stripe's events and that other providers' events, which share the table, project nothing into them (spec `operator-billing-views`, "Billing views disclose projection recency").
- [ ] 2.5 Test in `internal/server`, beside the existing tests of the billing views' recency line: with a processed Stripe event and a later processed Discourse event, the stamp is the Stripe event's processing time; with only a processed Discourse event, no recency line renders. Break, run with section 2's list: the `provider = 'stripe'` condition removed, caught by both cases.
- [ ] 2.6 No golden moves.
+- [x] 2.4 The billing views' recency stamp (`latestProcessedWebhookEventAt`, `internal/server/operator_billing.go`) reads only Stripe's events: its query gains `provider = 'stripe'`, and its doc comment says that the views are projected from Stripe's events and that other providers' events, which share the table, project nothing into them (spec `operator-billing-views`, "Billing views disclose projection recency").
+- [x] 2.5 Test in `internal/server`, beside the existing tests of the billing views' recency line: with a processed Stripe event and a later processed Discourse event, the stamp is the Stripe event's processing time; with only a processed Discourse event, no recency line renders. Break, run with section 2's list: the `provider = 'stripe'` condition removed, caught by both cases.
+- [x] 2.6 No golden moves.

 ## 3. The data model's contradiction, recorded for upstream