From 3489d6909ac08c259432e7beb2ae447ed5300631 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sat, 3 Oct 2026 01:16:14 +0200 Subject: [PATCH] Offer archive expiry choices on the target forms, show plain units (closes #396) A database target's archive expiry was typed by hand as never or a raw duration such as 720h, and the target list showed it back raw. Adding or editing a database target now offers the new-webhook page's list of choices (never, 1h, 12h, 24h, 30d, 90d, 365d), defined once and shared by all three forms. The edit form starts on the stored expiry, or on the submitted one after a refused save; a stored value outside the choices is listed under its own value, so saving unchanged keeps it. The target list shows the expiry in plain units: "30 days", "12 hours", "never". Model: opus-5-5 --- README.md | 7 +- internal/delivery/target_config_edit.go | 6 +- internal/delivery/target_config_view.go | 70 +++++--- internal/delivery/target_config_view_test.go | 14 +- internal/delivery/target_headers_test.go | 5 +- internal/handlers/archive_expiry.go | 58 +++++++ internal/handlers/archive_expiry_test.go | 166 +++++++++++++++++++ internal/handlers/source_detail_test.go | 2 +- internal/handlers/source_management.go | 6 + internal/handlers/target_edit.go | 7 +- internal/handlers/target_edit_test.go | 10 +- internal/server/alpine_browser_test.go | 38 +++++ templates/source_detail.html | 11 +- templates/sources_new.html | 10 +- templates/target_edit.html | 8 +- 15 files changed, 366 insertions(+), 52 deletions(-) create mode 100644 internal/handlers/archive_expiry.go create mode 100644 internal/handlers/archive_expiry_test.go diff --git a/README.md b/README.md index 39cbfa8..bc13f49 100644 --- a/README.md +++ b/README.md @@ -1721,8 +1721,11 @@ events should be forwarded. own archive database (`archive-{webhook_name}-{target_name}-{target_uuid}.db`) for long-term retention, with an optional creation-validated expiry (default: keep - forever). No external delivery and no retries; an archive write - failure fails the delivery. See the database target section under + forever). The new webhook form, the add target form and the target edit + form all offer the same expiries: never, 1h, 12h, 24h, 30d, 90d or 365d. + The target list shows the expiry in plain units, such as "30 days". No + external delivery and no retries; an archive write failure fails the + delivery. See the database target section under "Per-Webhook Event Databases" for the full semantics. - **`log`** — Write the event to the application log (stdout). Useful for debugging. diff --git a/internal/delivery/target_config_edit.go b/internal/delivery/target_config_edit.go index 42da682..3753e3c 100644 --- a/internal/delivery/target_config_edit.go +++ b/internal/delivery/target_config_edit.go @@ -86,9 +86,9 @@ func NewTargetConfigForm( } // databaseConfigForm parses an archive target's optional expiry. -// An absent or empty configuration is the keep-forever default and -// yields an empty field, so re-saving the form unchanged stores the -// same empty configuration it started with. An expiry that is set +// An absent, empty or never expiry yields an empty expiry, on which +// the edit form starts at never; saving it unchanged stores never, +// which means the same as an empty expiry. An expiry that is set // but not a valid duration is an error, not a blank field. func databaseConfigForm( configJSON string, diff --git a/internal/delivery/target_config_view.go b/internal/delivery/target_config_view.go index 1053eac..d6e3a84 100644 --- a/internal/delivery/target_config_view.go +++ b/internal/delivery/target_config_view.go @@ -1,9 +1,9 @@ package delivery import ( - "encoding/json" "fmt" "strconv" + "time" "sneak.berlin/go/webhooker/internal/database" ) @@ -197,37 +197,61 @@ func maxRetriesField(t *database.Target) ConfigField { } } -// databaseConfigFields describes an archive target. Its -// configuration is optional, and an absent or empty expiry -// means the archive is kept forever. An expiry that is set -// but not a valid duration is reported as unavailable rather -// than echoed back. +// databaseConfigFields describes an archive target by its +// expiry in plain units, such as "30 days", or "never" when +// the archive is kept forever. An expiry that is set but not +// a valid duration is reported as unavailable rather than +// echoed back. func databaseConfigFields(configJSON string) []ConfigField { - expiry := archiveExpiryNever + expiry, err := parseArchiveExpiry(configJSON) + if err != nil { + return unavailableConfigFields() + } - if configJSON != "" { - var cfg databaseTargetConfig - - err := json.Unmarshal([]byte(configJSON), &cfg) - if err != nil { - return unavailableConfigFields() - } - - if cfg.Expiry != "" { - if ValidateArchiveExpiry(cfg.Expiry) != nil { - return unavailableConfigFields() - } - - expiry = cfg.Expiry - } + value := archiveExpiryNever + if expiry > 0 { + value = plainDuration(expiry) } return []ConfigField{{ Label: "Archive Expiry", - Value: expiry, + Value: value, }} } +// plainDuration writes a positive duration as a count of the +// largest whole unit it divides into: "30 days", "12 hours", +// "1 minute". A duration with a fraction of a second is +// written as Go writes it. +func plainDuration(d time.Duration) string { + const day = 24 * time.Hour + + units := []struct { + size time.Duration + name string + }{ + {day, "day"}, + {time.Hour, "hour"}, + {time.Minute, "minute"}, + {time.Second, "second"}, + } + + for _, unit := range units { + if d%unit.size != 0 { + continue + } + + count := int64(d / unit.size) + if count == 1 { + return "1 " + unit.name + } + + return fmt.Sprintf("%d %ss", count, unit.name) + } + + return d.String() +} + // MaskedWebhookURL returns the Slack webhook URL reduced to // its scheme and host, with the path, query and any userinfo // elided. The path segments are the credential, so none of diff --git a/internal/delivery/target_config_view_test.go b/internal/delivery/target_config_view_test.go index c9e58f5..f763d86 100644 --- a/internal/delivery/target_config_view_test.go +++ b/internal/delivery/target_config_view_test.go @@ -305,14 +305,20 @@ func TestNewTargetViews_Database(t *testing.T) { }{ "empty config": {config: "", want: viewExpiryNever}, "empty expiry": {config: `{}`, want: viewExpiryNever}, - "explicit": { - config: `{"expiry":"720h"}`, - want: "720h", - }, "never literal": { config: `{"expiry":"` + viewExpiryNever + `"}`, want: viewExpiryNever, }, + "1h": {config: `{"expiry":"1h"}`, want: "1 hour"}, + "12h": {config: `{"expiry":"12h"}`, want: "12 hours"}, + "24h": {config: `{"expiry":"24h"}`, want: "1 day"}, + "720h": {config: `{"expiry":"720h"}`, want: "30 days"}, + "2160h": {config: `{"expiry":"2160h"}`, want: "90 days"}, + "8760h": {config: `{"expiry":"8760h"}`, want: "365 days"}, + "36h": {config: `{"expiry":"36h"}`, want: "36 hours"}, + "1h30m": {config: `{"expiry":"1h30m"}`, want: "90 minutes"}, + "45s": {config: `{"expiry":"45s"}`, want: "45 seconds"}, + "1.5s": {config: `{"expiry":"1.5s"}`, want: "1.5s"}, } for name, tc := range tests { diff --git a/internal/delivery/target_headers_test.go b/internal/delivery/target_headers_test.go index 3237132..ac72b86 100644 --- a/internal/delivery/target_headers_test.go +++ b/internal/delivery/target_headers_test.go @@ -216,8 +216,9 @@ func TestNewTargetConfigForm(t *testing.T) { assert.Empty(t, form.URL) } -// A keep-forever archive target must pre-fill as an empty field, so -// saving the form back unchanged stores the same empty config. +// A keep-forever archive target yields an empty expiry, so the edit +// form starts on never; saving it unchanged stores never, which means +// the same as an empty expiry. func TestNewTargetConfigForm_DatabaseNeverIsBlank(t *testing.T) { t.Parallel() diff --git a/internal/handlers/archive_expiry.go b/internal/handlers/archive_expiry.go new file mode 100644 index 0000000..060737d --- /dev/null +++ b/internal/handlers/archive_expiry.go @@ -0,0 +1,58 @@ +package handlers + +const ( + // archiveExpiryNever is the archive expiry that keeps archived + // events forever. A stored empty expiry means the same. + archiveExpiryNever = "never" + + // tmplKeyArchiveExpiryChoices is the template data key for the + // entries of a page's archive expiry select. + tmplKeyArchiveExpiryChoices = "ArchiveExpiryChoices" +) + +// archiveExpiryChoice is one entry of a database target's archive +// expiry select: the expiry stored, the label shown, and whether the +// select starts on it. +type archiveExpiryChoice struct { + Value string + Label string + Selected bool +} + +// archiveExpiryChoices lists the archive expiries offered by the new +// webhook page, the add target form and the target edit form. +func archiveExpiryChoices() []archiveExpiryChoice { + return []archiveExpiryChoice{ + {Value: archiveExpiryNever, Label: archiveExpiryNever}, + {Value: "1h", Label: "1h"}, + {Value: "12h", Label: "12h"}, + {Value: "24h", Label: "24h"}, + {Value: "720h", Label: "30d"}, + {Value: "2160h", Label: "90d"}, + {Value: "8760h", Label: "365d"}, + } +} + +// archiveExpiryOptions returns the choices with expiry selected; an +// empty expiry selects never. An expiry that is not one of the +// choices comes first as its own selected entry, so saving the form +// unchanged keeps it. +func archiveExpiryOptions(expiry string) []archiveExpiryChoice { + if expiry == "" { + expiry = archiveExpiryNever + } + + options := archiveExpiryChoices() + + for i := range options { + if options[i].Value == expiry { + options[i].Selected = true + + return options + } + } + + own := archiveExpiryChoice{Value: expiry, Label: expiry, Selected: true} + + return append([]archiveExpiryChoice{own}, options...) +} diff --git a/internal/handlers/archive_expiry_test.go b/internal/handlers/archive_expiry_test.go new file mode 100644 index 0000000..70bf476 --- /dev/null +++ b/internal/handlers/archive_expiry_test.go @@ -0,0 +1,166 @@ +package handlers_test + +import ( + "net/http" + "net/http/httptest" + "net/url" + "regexp" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" +) + +// expiryNever is the archive expiry that keeps archived events +// forever. +const expiryNever = "never" + +// matched returns what the one group of pattern matched in page, at +// each match. +func matched(pattern, page string) []string { + matches := regexp.MustCompile(pattern).FindAllStringSubmatch(page, -1) + groups := make([]string, 0, len(matches)) + + for _, m := range matches { + groups = append(groups, m[1]) + } + + return groups +} + +// expiryShown returns the archive expiries the webhook page's target +// list shows. +func expiryShown( + t *testing.T, env *sourceTestEnv, webhookID string, +) []string { + t.Helper() + + w := httptest.NewRecorder() + env.handlers.HandleSourceDetail().ServeHTTP(w, getRequest( + t, "/hook/"+webhookID, env.cookies, + map[string]string{sourceIDParam: webhookID}, + )) + require.Equal(t, http.StatusOK, w.Code) + + return matched( + `Archive Expiry:\s*([^<]*)`, w.Body.String(), + ) +} + +// expirySelected returns the target edit page and the expiries its +// select starts on. +func expirySelected( + t *testing.T, env *sourceTestEnv, webhookID, targetID string, +) (string, []string) { + t.Helper() + + w := serveTarget( + env, http.MethodGet, + "/hook/"+webhookID+"/targets/"+targetID+"/edit", nil, + ) + require.Equal(t, http.StatusOK, w.Code) + + page := w.Body.String() + + return page, matched(`