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, + `