From 9a94019db4f5604a1923baa9b874a22adc4e7c5e Mon Sep 17 00:00:00 2001 From: oliverpool Date: Tue, 9 Apr 2024 11:59:10 +0200 Subject: [PATCH] webhook: add admin-hooks tests --- models/webhook/webhook_system.go | 20 ++++++++-------- routers/api/v1/admin/hooks.go | 3 +-- routers/web/admin/hooks.go | 3 +-- services/webhook/webhook.go | 2 +- tests/integration/repo_webhook_test.go | 32 +++++++++++++++++++++----- 5 files changed, 40 insertions(+), 20 deletions(-) diff --git a/models/webhook/webhook_system.go b/models/webhook/webhook_system.go index a2a9ee321..62e828620 100644 --- a/models/webhook/webhook_system.go +++ b/models/webhook/webhook_system.go @@ -8,15 +8,11 @@ import ( "fmt" "code.gitea.io/gitea/models/db" - "code.gitea.io/gitea/modules/optional" ) // GetDefaultWebhooks returns all admin-default webhooks. func GetDefaultWebhooks(ctx context.Context) ([]*Webhook, error) { - webhooks := make([]*Webhook, 0, 5) - return webhooks, db.GetEngine(ctx). - Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, false). - Find(&webhooks) + return getAdminWebhooks(ctx, false) } // GetSystemOrDefaultWebhook returns admin system or default webhook by given ID. @@ -34,15 +30,21 @@ func GetSystemOrDefaultWebhook(ctx context.Context, id int64) (*Webhook, error) } // GetSystemWebhooks returns all admin system webhooks. -func GetSystemWebhooks(ctx context.Context, isActive optional.Option[bool]) ([]*Webhook, error) { +func GetSystemWebhooks(ctx context.Context, onlyActive bool) ([]*Webhook, error) { + return getAdminWebhooks(ctx, true, onlyActive) +} + +func getAdminWebhooks(ctx context.Context, systemWebhooks bool, onlyActive ...bool) ([]*Webhook, error) { webhooks := make([]*Webhook, 0, 5) - if !isActive.Has() { + if len(onlyActive) > 0 && onlyActive[0] { return webhooks, db.GetEngine(ctx). - Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, true). + Where("repo_id=? AND owner_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, systemWebhooks, true). + OrderBy("id"). Find(&webhooks) } return webhooks, db.GetEngine(ctx). - Where("repo_id=? AND owner_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, true, isActive.Value()). + Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, systemWebhooks). + OrderBy("id"). Find(&webhooks) } diff --git a/routers/api/v1/admin/hooks.go b/routers/api/v1/admin/hooks.go index 4c168b55b..b246cb61b 100644 --- a/routers/api/v1/admin/hooks.go +++ b/routers/api/v1/admin/hooks.go @@ -8,7 +8,6 @@ import ( "net/http" "code.gitea.io/gitea/models/webhook" - "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/util" @@ -38,7 +37,7 @@ func ListHooks(ctx *context.APIContext) { // "200": // "$ref": "#/responses/HookList" - sysHooks, err := webhook.GetSystemWebhooks(ctx, optional.None[bool]()) + sysHooks, err := webhook.GetSystemWebhooks(ctx, false) if err != nil { ctx.Error(http.StatusInternalServerError, "GetSystemWebhooks", err) return diff --git a/routers/web/admin/hooks.go b/routers/web/admin/hooks.go index 91857d275..c1f42c006 100644 --- a/routers/web/admin/hooks.go +++ b/routers/web/admin/hooks.go @@ -8,7 +8,6 @@ import ( "code.gitea.io/gitea/models/webhook" "code.gitea.io/gitea/modules/base" - "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/services/context" webhook_service "code.gitea.io/gitea/services/webhook" @@ -36,7 +35,7 @@ func DefaultOrSystemWebhooks(ctx *context.Context) { sys["Title"] = ctx.Tr("admin.systemhooks") sys["Description"] = ctx.Tr("admin.systemhooks.desc") - sys["Webhooks"], err = webhook.GetSystemWebhooks(ctx, optional.None[bool]()) + sys["Webhooks"], err = webhook.GetSystemWebhooks(ctx, false) sys["BaseLink"] = setting.AppSubURL + "/admin/hooks" sys["BaseLinkNew"] = setting.AppSubURL + "/admin/system-hooks" sys["WebhookList"] = webhook_service.List() diff --git a/services/webhook/webhook.go b/services/webhook/webhook.go index dc68cae84..cf4f2fdfd 100644 --- a/services/webhook/webhook.go +++ b/services/webhook/webhook.go @@ -243,7 +243,7 @@ func PrepareWebhooks(ctx context.Context, source EventSource, event webhook_modu } // Add any admin-defined system webhooks - systemHooks, err := webhook_model.GetSystemWebhooks(ctx, optional.Some(true)) + systemHooks, err := webhook_model.GetSystemWebhooks(ctx, true) if err != nil { return fmt.Errorf("GetSystemWebhooks: %w", err) } diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index 4e8788f0b..9a278e706 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -87,7 +87,7 @@ func TestNewWebHookLink(t *testing.T) { func TestWebhookForms(t *testing.T) { defer tests.PrepareTestEnv(t)() - session := loginUser(t, "user2") + session := loginUser(t, "user1") t.Run("forgejo/required", testWebhookForms("forgejo", session, map[string]string{ "payload_url": "https://forgejo.example.com", @@ -299,7 +299,9 @@ func assertInput(t testing.TB, form *goquery.Selection, name string) string { t.Helper() input := form.Find(`input[name="` + name + `"]`) if input.Length() != 1 { - t.Log(form.Html()) + form.Find("input").Each(func(i int, s *goquery.Selection) { + t.Logf("found ", s.AttrOr("name", "")) + }) t.Errorf("field found %d times, expected once", name, input.Length()) } switch input.AttrOr("type", "") { @@ -321,6 +323,12 @@ func testWebhookForms(name string, session *TestSession, validFields map[string] t.Run("org3", func(t *testing.T) { testWebhookFormsShared(t, "/org/org3/settings/hooks", name, session, validFields, invalidPatches...) }) + t.Run("system", func(t *testing.T) { + testWebhookFormsShared(t, "/admin/system-hooks", name, session, validFields, invalidPatches...) + }) + t.Run("default", func(t *testing.T) { + testWebhookFormsShared(t, "/admin/default-hooks", name, session, validFields, invalidPatches...) + }) } } @@ -345,17 +353,29 @@ func testWebhookFormsShared(t *testing.T, endpoint, name string, session *TestSe // create the webhook (this redirects back to the hook list) resp = session.MakeRequest(t, NewRequestWithValues(t, "POST", endpoint+"/"+name+"/new", payload), http.StatusSeeOther) assertHasFlashMessages(t, resp, "success") + listEndpoint := resp.Header().Get("Location") + updateEndpoint := endpoint + "/" + if endpoint == "/admin/system-hooks" || endpoint == "/admin/default-hooks" { + updateEndpoint = "/admin/hooks/" + } // find last created hook in the hook list // (a bit hacky, but the list should be sorted) - resp = session.MakeRequest(t, NewRequest(t, "GET", endpoint), http.StatusOK) + resp = session.MakeRequest(t, NewRequest(t, "GET", listEndpoint), http.StatusOK) htmlDoc := NewHTMLParser(t, resp.Body) - editFormURL := htmlDoc.Find(`a[href^="`+endpoint+`/"]`).Last().AttrOr("href", "") + selector := `a[href^="` + updateEndpoint + `"]` + if endpoint == "/admin/system-hooks" { + // system-hooks and default-hooks are listed on the same page + // add a specifier to select the latest system-hooks + // (the default-hooks are at the end, so no further specifier needed) + selector = `.admin-setting-content > div:first-of-type ` + selector + } + editFormURL := htmlDoc.Find(selector).Last().AttrOr("href", "") assert.NotEmpty(t, editFormURL) // edit webhook form resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK) - htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + endpoint + `/"]`) + htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + updateEndpoint + `"]`) editPostURL := htmlForm.AttrOr("action", "") assert.NotEmpty(t, editPostURL) @@ -375,7 +395,7 @@ func testWebhookFormsShared(t *testing.T, endpoint, name string, session *TestSe // check the updated webhook resp = session.MakeRequest(t, NewRequest(t, "GET", editFormURL), http.StatusOK) - htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + endpoint + `/"]`) + htmlForm = NewHTMLParser(t, resp.Body).Find(`form[action^="` + updateEndpoint + `"]`) for k, v := range validFields { assert.Equal(t, v, assertInput(t, htmlForm, k), "input %q did not contain value %q", k, v) }