From 17fe8a4201adc05e487abb4134b81c439da3ef39 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 11 Aug 2026 12:20:24 +0000 Subject: [PATCH] Clarify web UI terminology, copy, and the entrypoint URL (closes #57) Terminology: the nav labelled its link Sources while every heading said Webhooks. Both nav links and the sources list page title now say Webhooks, matching the product name and the database.Webhook model. The /sources and /source/{id} routes are deliberately unchanged; renaming them would break existing bookmarks for no gain, since the URL is not what a user reads. Profile: the "Settings" column held only placeholder copy promising settings that would appear later. Password change is the only real account setting and it already has its own card below, so the column is removed and the two-column grid collapses to the single remaining one. Retention copy on the create and edit forms now describes what the code does rather than what the field implies. The reaper permanently deletes events past the cutoff along with their deliveries and delivery results, so the hint says so instead of "how long to keep event data". Both forms state that an empty field is not a way to ask for forever: the create path falls back to DefaultRetentionDays and the edit path leaves the stored policy alone, while 0 is what BeforeSave rewrites to the retain-forever sentinel. Entrypoint URL copy button as progressive enhancement. The button is rendered with the hidden attribute and a data-copy-target naming the element that holds the URL. app.js reveals it only after confirming both a resolvable target and a usable Clipboard API, so a browser without either shows no dead control, and the URL is plain selectable text in every case. The script uses const throughout, per the JS styleguide the repo policies bind this repo to; nothing in the repo's checks covers JavaScript, so that is enforced by reading rather than by the gate. Template rendering assertions cover the nav labels, the absence of any remaining user-visible "Sources", the button's hidden-by-default markup, and both forms' retention copy including the forever label, so the copy cannot drift back silently. The edit-page assertions pass the webhook as a pointer because RetentionLabel is a pointer method and a map element is not addressable. --- TODO.md | 4 + internal/handlers/ui_copy_test.go | 299 ++++++++++++++++++++++++++++++ static/js/app.js | 58 ++++++ templates/navbar.html | 4 +- templates/profile.html | 30 ++- templates/source_detail.html | 7 +- templates/source_edit.html | 2 +- templates/sources_list.html | 2 +- templates/sources_new.html | 2 +- 9 files changed, 384 insertions(+), 24 deletions(-) create mode 100644 internal/handlers/ui_copy_test.go diff --git a/TODO.md b/TODO.md index caf163c..7900d62 100644 --- a/TODO.md +++ b/TODO.md @@ -26,6 +26,10 @@ capability in the README rationale). # Completed Steps +- 2026-08-11 Web UI cleanup: nav terminology unified on Webhooks, the + Profile settings placeholder removed, a progressive-enhancement copy + button for the entrypoint URL, and retention form copy that states the + actual policy (deletion by the reaper, 0 retains forever) (#57) - 2026-08-09 Inactivity-based session timeout: sliding idle expiry (`SESSION_IDLE_TIMEOUT`, default `24h`) refreshed on authenticated requests, with the 7-day absolute cap kept as an independent diff --git a/internal/handlers/ui_copy_test.go b/internal/handlers/ui_copy_test.go new file mode 100644 index 0000000..23652f6 --- /dev/null +++ b/internal/handlers/ui_copy_test.go @@ -0,0 +1,299 @@ +package handlers_test + +import ( + "context" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" + "sneak.berlin/go/webhooker/internal/handlers" + "sneak.berlin/go/webhooker/internal/session" +) + +// Template data keys the page templates read. The handlers package has +// its own unexported constants for these; this is the external test +// package, so it needs its own. +const ( + dataKeyWebhook = "Webhook" + dataKeyError = "Error" +) + +// testWebhookID is the identifier given to the webhook under test on +// pages that render one. +const testWebhookID = "wh-1" + +// renderPage renders a page template through the real template set as +// an authenticated user and returns the resulting HTML. +func renderPage( + t *testing.T, + h *handlers.Handlers, + sess *session.Session, + page string, + data map[string]any, +) string { + t.Helper() + + cookies := authenticatedCookies(t, sess, "test-user-id", "testuser") + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/", nil, + ) + for _, c := range cookies { + req.AddCookie(c) + } + + w := httptest.NewRecorder() + h.RenderTemplateForTest(w, req, page, data) + + return w.Body.String() +} + +// TestNavbarUsesWebhookTerminology pins the user-visible navigation +// label to "Webhooks". The /sources route is deliberately unchanged, so +// the assertion targets the link text rather than the href. +func TestNavbarUsesWebhookTerminology(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + // One item, so the list body renders too: it calls + // WebhookListItem.RetentionLabel, promoted from the embedded + // Webhook and therefore a pointer method. An empty list would + // skip that call and hide a template error behind the + // navigation assertions below. + item := handlers.WebhookListItem{} + item.Name = "wh" + item.ID = testWebhookID + item.RetentionDays = 14 + + body := renderPage(t, h, sess, "sources_list.html", map[string]any{ + "Webhooks": []handlers.WebhookListItem{item}, + }) + + assert.Contains(t, body, "Retention: 14 days") + assert.Contains(t, body, `class="btn-text">Webhooks`) + assert.Contains( + t, body, `class="btn-text w-full text-left">Webhooks`, + ) + assert.Contains( + t, body, + `

Webhooks

`, + ) + assert.NotContains( + t, body, ">Sources<", + "no user-visible element may still be labelled Sources", + ) + assert.Contains( + t, body, `href="/sources"`, + "the /sources route itself must not change", + ) +} + +// TestEditPageUsesWebhookTerminology pins the edit page's heading and +// its back link. The link's href still points at /source/{id}, which is +// intentional: only user-visible copy changes. +func TestEditPageUsesWebhookTerminology(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + // The webhook goes in as a pointer because source_edit.html calls + // Webhook.RetentionLabel, a pointer method: a map element is not + // addressable, so a value here renders an error instead of the + // page. + webhook := &database.Webhook{Name: "wh", RetentionDays: 14} + webhook.ID = testWebhookID + + body := renderPage(t, h, sess, "source_edit.html", map[string]any{ + dataKeyWebhook: webhook, + dataKeyError: "", + }) + + assert.Contains(t, body, "Edit Webhook") + assert.NotContains(t, body, ">Sources<") + assert.Contains(t, body, `href="/source/wh-1"`) +} + +// TestCreateFormRetentionCopyMatchesBehaviour pins the create form's +// retention copy to what the code does: the reaper permanently deletes +// events past the cutoff, an empty field falls back to +// DefaultRetentionDays, and 0 is rewritten to the retain-forever +// sentinel by Webhook.BeforeSave. +func TestCreateFormRetentionCopyMatchesBehaviour(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + body := renderPage(t, h, sess, "sources_new.html", map[string]any{ + "Name": "", + "Description": "", + "DefaultRetentionDays": database.DefaultRetentionDays, + dataKeyError: "", + }) + + assert.Contains( + t, body, + "permanently deletes events older than this", + "the form must say retention is enforced by deletion", + ) + assert.Contains(t, body, "Enter 0 to retain events forever") + assert.Contains( + t, body, + "leave blank to use the default of "+ + strconv.Itoa(database.DefaultRetentionDays)+" days", + "blank means the default, not forever", + ) +} + +// TestEditFormRetentionCopyMatchesBehaviour pins the edit form's +// retention copy, including that it states the stored policy via +// RetentionLabel and that an empty field leaves that policy unchanged +// rather than meaning forever. +func TestEditFormRetentionCopyMatchesBehaviour(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + finite := &database.Webhook{Name: "wh", RetentionDays: 14} + finite.ID = testWebhookID + + body := renderPage(t, h, sess, "source_edit.html", map[string]any{ + dataKeyWebhook: finite, + dataKeyError: "", + }) + + assert.Contains(t, body, "Currently 14 days.") + assert.Contains( + t, body, + "permanently deletes events older than this", + ) + assert.Contains(t, body, "Enter 0 to retain events forever") + assert.Contains( + t, body, + "leave blank to keep the current setting", + "blank means unchanged, not forever", + ) + + forever := &database.Webhook{ + Name: "wh", + RetentionDays: database.RetentionForeverDays, + } + forever.ID = "wh-2" + + foreverBody := renderPage( + t, h, sess, "source_edit.html", map[string]any{ + dataKeyWebhook: forever, + dataKeyError: "", + }, + ) + + assert.Contains( + t, foreverBody, "Currently forever.", + "a retain-forever webhook must not read as a day count", + ) + assert.Contains( + t, foreverBody, + "No events are deleted while retention is set to forever", + ) + assert.NotContains( + t, foreverBody, + "permanently deletes events older than this", + "the reaper skips retain-forever webhooks, so the form "+ + "must not claim it deletes their events", + ) +} + +// TestEntrypointCopyButtonIsProgressiveEnhancement proves the copy +// affordance degrades: the button ships with the hidden attribute, so a +// browser that never runs app.js shows no dead control, and the URL is +// rendered as ordinary selectable text either way. +func TestEntrypointCopyButtonIsProgressiveEnhancement(t *testing.T) { + t.Parallel() + + var h *handlers.Handlers + + var sess *session.Session + + app := newTestApp(t, &h, &sess) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + entrypoint := database.Entrypoint{Path: "abc123"} + entrypoint.ID = "ep-1" + + // The webhook goes in as a pointer because source_detail.html + // calls Webhook.RetentionLabel, a pointer method: a map element + // is not addressable, so a value here aborts execution partway + // down the page, after the copy button has already been flushed + // to the response. + webhook := &database.Webhook{Name: "wh", RetentionDays: 14} + webhook.ID = testWebhookID + webhook.CreatedAt = time.Date( + 2026, time.January, 2, 3, 4, 5, 0, time.UTC, + ) + + body := renderPage(t, h, sess, "source_detail.html", map[string]any{ + dataKeyWebhook: webhook, + "Entrypoints": []database.Entrypoint{entrypoint}, + // The handler passes delivery.NewTargetViews(targets), never + // raw targets, so the test data has to have that same shape. + "Targets": delivery.NewTargetViews(nil), + "Events": []database.Event{}, + "BaseURL": "https://hooks.example.com", + }) + + assert.Contains( + t, body, + `