From aab448b07627e905349d2254acfe932a0556fda9 Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 11 Aug 2026 15:42:08 +0200 Subject: [PATCH] Clarify web UI terminology, copy, and the entrypoint URL (closes #57) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unifies user-visible copy on "Webhook" (routes and URLs unchanged), drops the placeholder Profile settings section, and adds a copy-to-clipboard affordance for the entrypoint URL as progressive enhancement — the button stays hidden unless both the target element and the Clipboard API resolve, so no dead control appears without JavaScript and the URL stays selectable. Retention copy now matches what the code does: deletion is permanent, 0 retains forever, and a blank field means the default on create or the current value on edit. The permanent-deletion sentence is suppressed for a retain-forever webhook, which the reaper exempts before computing a cutoff. Template tests gained a render-completed assertion. Without it, a page that aborted mid-render still satisfied assertions matching the already-flushed prefix, because renderTemplate streams to the ResponseWriter (#123). --- 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, + `