Merge pull request '[v7.0/forgejo] [BUG] fix webhook creation payload ref' (go-gitea#3070) from bp-v7.0/forgejo-2c85a14-9d29192 into v7.0/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3070
Reviewed-by: oliverpool <[email protected]>

Author: Earl Warren

models/webhook/hooktask.go

@@ -106,7 +106,7 @@ func (t *HookTask) simpleMarshalJSON(v any) string {
 	return string(p)
 }
 
-// HookTasks returns a list of hook tasks by given conditions.
+// HookTasks returns a list of hook tasks by given conditions, order by ID desc.
 func HookTasks(ctx context.Context, hookID int64, page int) ([]*HookTask, error) {
 	tasks := make([]*HookTask, 0, setting.Webhook.PagingNum)
 	return tasks, db.GetEngine(ctx).

services/webhook/notifier.go

@@ -748,10 +748,9 @@ func (m *webhookNotifier) PullRequestReviewRequest(ctx context.Context, doer *us
 func (m *webhookNotifier) CreateRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, refFullName git.RefName, refID string) {
 	apiPusher := convert.ToUser(ctx, pusher, nil)
 	apiRepo := convert.ToRepo(ctx, repo, access_model.Permission{AccessMode: perm.AccessModeNone})
-	refName := refFullName.ShortName()
 
 	if err := PrepareWebhooks(ctx, EventSource{Repository: repo}, webhook_module.HookEventCreate, &api.CreatePayload{
-		Ref:     refName, // FIXME: should it be a full ref name?
+		Ref:     refFullName.String(),
 		Sha:     refID,
 		RefType: refFullName.RefType(),
 		Repo:    apiRepo,
@@ -785,10 +784,9 @@ func (m *webhookNotifier) PullRequestSynchronized(ctx context.Context, doer *use
 func (m *webhookNotifier) DeleteRef(ctx context.Context, pusher *user_model.User, repo *repo_model.Repository, refFullName git.RefName) {
 	apiPusher := convert.ToUser(ctx, pusher, nil)
 	apiRepo := convert.ToRepo(ctx, repo, access_model.Permission{AccessMode: perm.AccessModeOwner})
-	refName := refFullName.ShortName()
 
 	if err := PrepareWebhooks(ctx, EventSource{Repository: repo}, webhook_module.HookEventDelete, &api.DeletePayload{
-		Ref:        refName, // FIXME: should it be a full ref name?
+		Ref:        refFullName.String(),
 		RefType:    refFullName.RefType(),
 		PusherType: api.PusherTypeUser,
 		Repo:       apiRepo,

tests/integration/pull_merge_test.go

@@ -83,6 +83,7 @@ func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum str
 	return resp
 }
 
+// returns the hook tasks, order by ID desc.
 func retrieveHookTasks(t *testing.T, hookID int64, activateWebhook bool) []*webhook.HookTask {
 	t.Helper()
 	if activateWebhook {

tests/integration/webhook_test.go

@@ -0,0 +1,72 @@
+// Copyright 2024 The Forgejo Authors c/o Codeberg e.V.. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package integration
+
+import (
+	"net/http"
+	"net/url"
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/models/unittest"
+	webhook_model "code.gitea.io/gitea/models/webhook"
+	"code.gitea.io/gitea/modules/json"
+	webhook_module "code.gitea.io/gitea/modules/webhook"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestWebhookPayloadRef(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		w := unittest.AssertExistsAndLoadBean(t, &webhook_model.Webhook{ID: 1})
+		w.HookEvent = &webhook_module.HookEvent{
+			SendEverything: true,
+		}
+		assert.NoError(t, w.UpdateEvent())
+		assert.NoError(t, webhook_model.UpdateWebhook(db.DefaultContext, w))
+
+		hookTasks := retrieveHookTasks(t, w.ID, true)
+		hookTasksLenBefore := len(hookTasks)
+
+		session := loginUser(t, "user2")
+		// create new branch
+		csrf := GetCSRF(t, session, "user2/repo1")
+		req := NewRequestWithValues(t, "POST", "user2/repo1/branches/_new/branch/master",
+			map[string]string{
+				"_csrf":           csrf,
+				"new_branch_name": "arbre",
+				"create_tag":      "false",
+			},
+		)
+		session.MakeRequest(t, req, http.StatusSeeOther)
+		// delete the created branch
+		req = NewRequestWithValues(t, "POST", "user2/repo1/branches/delete?name=arbre",
+			map[string]string{
+				"_csrf": csrf,
+			},
+		)
+		session.MakeRequest(t, req, http.StatusOK)
+
+		// check the newly created hooktasks
+		hookTasks = retrieveHookTasks(t, w.ID, false)
+		expected := map[webhook_module.HookEventType]bool{
+			webhook_module.HookEventCreate: true,
+			webhook_module.HookEventPush:   true, // the branch creation also creates a push event
+			webhook_module.HookEventDelete: true,
+		}
+		for _, hookTask := range hookTasks[:len(hookTasks)-hookTasksLenBefore] {
+			if !expected[hookTask.EventType] {
+				t.Errorf("unexpected (or duplicated) event %q", hookTask.EventType)
+			}
+
+			var payload struct {
+				Ref string `json:"ref"`
+			}
+			assert.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payload))
+			assert.Equal(t, "refs/heads/arbre", payload.Ref, "unexpected ref for %q event", hookTask.EventType)
+			delete(expected, hookTask.EventType)
+		}
+		assert.Empty(t, expected)
+	})
+}