Merge pull request 'Fix release published actions not triggering for releases created from existing tags' (#3220) from zotan/forgejo:forgejo into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3220
Reviewed-by: oliverpool <oliverpool@noreply.codeberg.org>
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

routers/api/v1/repo/release.go

@@ -267,7 +267,7 @@ func CreateRelease(ctx *context.APIContext) {
 		rel.Publisher = ctx.Doer
 		rel.Target = form.Target
 
-		if err = release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, nil, nil, nil); err != nil {
+		if err = release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, nil, nil, nil, true); err != nil {
 			ctx.Error(http.StatusInternalServerError, "UpdateRelease", err)
 			return
 		}
@@ -341,7 +341,7 @@ func EditRelease(ctx *context.APIContext) {
 	if form.IsPrerelease != nil {
 		rel.IsPrerelease = *form.IsPrerelease
 	}
-	if err := release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, nil, nil, nil); err != nil {
+	if err := release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, nil, nil, nil, false); err != nil {
 		ctx.Error(http.StatusInternalServerError, "UpdateRelease", err)
 		return
 	}

routers/web/repo/release.go

@@ -567,7 +567,7 @@ func NewReleasePost(ctx *context.Context) {
 		rel.PublisherID = ctx.Doer.ID
 		rel.IsTag = false
 
-		if err = releaseservice.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, attachmentUUIDs, nil, nil); err != nil {
+		if err = releaseservice.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, attachmentUUIDs, nil, nil, true); err != nil {
 			ctx.Data["Err_TagName"] = true
 			ctx.ServerError("UpdateRelease", err)
 			return
@@ -674,7 +674,7 @@ func EditReleasePost(ctx *context.Context) {
 	rel.IsDraft = len(form.Draft) > 0
 	rel.IsPrerelease = form.Prerelease
 	if err = releaseservice.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo,
-		rel, addAttachmentUUIDs, delAttachmentUUIDs, editAttachments); err != nil {
+		rel, addAttachmentUUIDs, delAttachmentUUIDs, editAttachments, false); err != nil {
 		ctx.ServerError("UpdateRelease", err)
 		return
 	}

services/release/release.go

@@ -199,7 +199,7 @@ func CreateNewTag(ctx context.Context, doer *user_model.User, repo *repo_model.R
 // delAttachmentUUIDs accept a slice of attachments' uuids which will be deleted from the release
 // editAttachments accept a map of attachment uuid to new attachment name which will be updated with attachments.
 func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, rel *repo_model.Release,
-	addAttachmentUUIDs, delAttachmentUUIDs []string, editAttachments map[string]string,
+	addAttachmentUUIDs, delAttachmentUUIDs []string, editAttachments map[string]string, createdFromTag bool,
 ) error {
 	if rel.ID == 0 {
 		return errors.New("UpdateRelease only accepts an exist release")
@@ -292,11 +292,11 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo
 	}
 
 	if !rel.IsDraft {
-		if !isCreated {
+		if createdFromTag || isCreated {
-			notify_service.UpdateRelease(gitRepo.Ctx, doer, rel)
+			notify_service.NewRelease(gitRepo.Ctx, rel)
 			return nil
 		}
-		notify_service.NewRelease(gitRepo.Ctx, rel)
+		notify_service.UpdateRelease(gitRepo.Ctx, doer, rel)
 	}
 	return nil
 }

services/release/release_test.go

@@ -159,7 +159,7 @@ func TestRelease_Update(t *testing.T) {
 	releaseCreatedUnix := release.CreatedUnix
 	time.Sleep(2 * time.Second) // sleep 2 seconds to ensure a different timestamp
 	release.Note = "Changed note"
-	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, nil))
+	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, nil, false))
 	release, err = repo_model.GetReleaseByID(db.DefaultContext, release.ID)
 	assert.NoError(t, err)
 	assert.Equal(t, int64(releaseCreatedUnix), int64(release.CreatedUnix))
@@ -183,7 +183,7 @@ func TestRelease_Update(t *testing.T) {
 	releaseCreatedUnix = release.CreatedUnix
 	time.Sleep(2 * time.Second) // sleep 2 seconds to ensure a different timestamp
 	release.Title = "Changed title"
-	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, nil))
+	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, nil, false))
 	release, err = repo_model.GetReleaseByID(db.DefaultContext, release.ID)
 	assert.NoError(t, err)
 	assert.Less(t, int64(releaseCreatedUnix), int64(release.CreatedUnix))
@@ -208,7 +208,7 @@ func TestRelease_Update(t *testing.T) {
 	time.Sleep(2 * time.Second) // sleep 2 seconds to ensure a different timestamp
 	release.Title = "Changed title"
 	release.Note = "Changed note"
-	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, nil))
+	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, nil, false))
 	release, err = repo_model.GetReleaseByID(db.DefaultContext, release.ID)
 	assert.NoError(t, err)
 	assert.Equal(t, int64(releaseCreatedUnix), int64(release.CreatedUnix))
@@ -233,7 +233,7 @@ func TestRelease_Update(t *testing.T) {
 	release.IsDraft = false
 	tagName := release.TagName
 
-	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, nil))
+	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, nil, false))
 	release, err = repo_model.GetReleaseByID(db.DefaultContext, release.ID)
 	assert.NoError(t, err)
 	assert.Equal(t, tagName, release.TagName)
@@ -247,7 +247,7 @@ func TestRelease_Update(t *testing.T) {
 	}, strings.NewReader(samplePayload), int64(len([]byte(samplePayload))))
 	assert.NoError(t, err)
 
-	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, []string{attach.UUID}, nil, nil))
+	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, []string{attach.UUID}, nil, nil, false))
 	assert.NoError(t, repo_model.GetReleaseAttachments(db.DefaultContext, release))
 	assert.Len(t, release.Attachments, 1)
 	assert.EqualValues(t, attach.UUID, release.Attachments[0].UUID)
@@ -257,7 +257,7 @@ func TestRelease_Update(t *testing.T) {
 	// update the attachment name
 	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, nil, map[string]string{
 		attach.UUID: "test2.txt",
-	}))
+	}, false))
 	release.Attachments = nil
 	assert.NoError(t, repo_model.GetReleaseAttachments(db.DefaultContext, release))
 	assert.Len(t, release.Attachments, 1)
@@ -266,7 +266,7 @@ func TestRelease_Update(t *testing.T) {
 	assert.EqualValues(t, "test2.txt", release.Attachments[0].Name)
 
 	// delete the attachment
-	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, []string{attach.UUID}, nil))
+	assert.NoError(t, UpdateRelease(db.DefaultContext, user, gitRepo, release, nil, []string{attach.UUID}, nil, false))
 	release.Attachments = nil
 	assert.NoError(t, repo_model.GetReleaseAttachments(db.DefaultContext, release))
 	assert.Empty(t, release.Attachments)

tests/integration/webhook_test.go

@@ -9,10 +9,16 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/models/db"
+	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
 	webhook_model "code.gitea.io/gitea/models/webhook"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/json"
 	webhook_module "code.gitea.io/gitea/modules/webhook"
+	"code.gitea.io/gitea/services/release"
+	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -70,3 +76,113 @@ func TestWebhookPayloadRef(t *testing.T) {
 		assert.Empty(t, expected)
 	})
 }
+
+func TestWebhookReleaseEvents(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+	w := unittest.AssertExistsAndLoadBean(t, &webhook_model.Webhook{
+		ID:     1,
+		RepoID: repo.ID,
+	})
+	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)
+
+	gitRepo, err := gitrepo.OpenRepository(git.DefaultContext, repo)
+	assert.NoError(t, err)
+	defer gitRepo.Close()
+
+	t.Run("CreateRelease", func(t *testing.T) {
+		assert.NoError(t, release.CreateRelease(gitRepo, &repo_model.Release{
+			RepoID:       repo.ID,
+			Repo:         repo,
+			PublisherID:  user.ID,
+			Publisher:    user,
+			TagName:      "v1.1.1",
+			Target:       "master",
+			Title:        "v1.1.1 is released",
+			Note:         "v1.1.1 is released",
+			IsDraft:      false,
+			IsPrerelease: false,
+			IsTag:        false,
+		}, nil, ""))
+
+		// check the newly created hooktasks
+		hookTasksLenBefore := len(hookTasks)
+		hookTasks = retrieveHookTasks(t, w.ID, false)
+
+		checkHookTasks(t, map[webhook_module.HookEventType]string{
+			webhook_module.HookEventRelease: "published",
+			webhook_module.HookEventCreate:  "", // a tag was created as well
+			webhook_module.HookEventPush:    "", // the tag creation also means a push event
+		}, hookTasks[:len(hookTasks)-hookTasksLenBefore])
+
+		t.Run("UpdateRelease", func(t *testing.T) {
+			rel := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{RepoID: repo.ID, TagName: "v1.1.1"})
+			assert.NoError(t, release.UpdateRelease(db.DefaultContext, user, gitRepo, rel, nil, nil, nil, false))
+
+			// check the newly created hooktasks
+			hookTasksLenBefore := len(hookTasks)
+			hookTasks = retrieveHookTasks(t, w.ID, false)
+
+			checkHookTasks(t, map[webhook_module.HookEventType]string{
+				webhook_module.HookEventRelease: "updated",
+			}, hookTasks[:len(hookTasks)-hookTasksLenBefore])
+		})
+	})
+
+	t.Run("CreateNewTag", func(t *testing.T) {
+		assert.NoError(t, release.CreateNewTag(db.DefaultContext,
+			user,
+			repo,
+			"master",
+			"v1.1.2",
+			"v1.1.2 is tagged",
+		))
+
+		// check the newly created hooktasks
+		hookTasksLenBefore := len(hookTasks)
+		hookTasks = retrieveHookTasks(t, w.ID, false)
+
+		checkHookTasks(t, map[webhook_module.HookEventType]string{
+			webhook_module.HookEventCreate: "", // tag was created as well
+			webhook_module.HookEventPush:   "", // the tag creation also means a push event
+		}, hookTasks[:len(hookTasks)-hookTasksLenBefore])
+
+		t.Run("UpdateRelease", func(t *testing.T) {
+			rel := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{RepoID: repo.ID, TagName: "v1.1.2"})
+			assert.NoError(t, release.UpdateRelease(db.DefaultContext, user, gitRepo, rel, nil, nil, nil, true))
+
+			// check the newly created hooktasks
+			hookTasksLenBefore := len(hookTasks)
+			hookTasks = retrieveHookTasks(t, w.ID, false)
+
+			checkHookTasks(t, map[webhook_module.HookEventType]string{
+				webhook_module.HookEventRelease: "published",
+			}, hookTasks[:len(hookTasks)-hookTasksLenBefore])
+		})
+	})
+}
+
+func checkHookTasks(t *testing.T, expectedActions map[webhook_module.HookEventType]string, hookTasks []*webhook_model.HookTask) {
+	t.Helper()
+	for _, hookTask := range hookTasks {
+		expectedAction, ok := expectedActions[hookTask.EventType]
+		if !ok {
+			t.Errorf("unexpected (or duplicated) event %q", hookTask.EventType)
+		}
+		var payload struct {
+			Action string `json:"action"`
+		}
+		assert.NoError(t, json.Unmarshal([]byte(hookTask.PayloadContent), &payload))
+		assert.Equal(t, expectedAction, payload.Action, "unexpected action for %q event", hookTask.EventType)
+		delete(expectedActions, hookTask.EventType)
+	}
+	assert.Empty(t, expectedActions)
+}