Complete push webhooks (#2530)

* implemented missing 'delete' push webhooks

moreover created ActionDeleteBranch and ActionDeleteTag

* add CommitRepoAction tests for tag/branch creation/deletion

* fixed error where push webhook not called if is new branch or tag
removed unnecessary code

* moved prepare unit test environment into separate method to be used across unit tests

* add missing if clause in pushUpdate

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

Author: daviian

Diff for: integrations/gitea-repositories-meta/user2/repo1.git/refs/tags/v1.1

@@ -0,0 +1 @@
+65f1bf27bc3bf70f64657658635e66094edbcb4d

Diff for: models/action.go

@@ -46,6 +46,8 @@ const (
 	ActionReopenIssue                             // 13
 	ActionClosePullRequest                        // 14
 	ActionReopenPullRequest                       // 15
+	ActionDeleteTag                               // 16
+	ActionDeleteBranch                            // 17
 )
 
 var (
@@ -554,6 +556,12 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
 	// Check it's tag push or branch.
 	if strings.HasPrefix(opts.RefFullName, git.TagPrefix) {
 		opType = ActionPushTag
+		if opts.NewCommitID == git.EmptySHA {
+			opType = ActionDeleteTag
+		}
+		opts.Commits = &PushCommits{}
+	} else if opts.NewCommitID == git.EmptySHA {
+		opType = ActionDeleteBranch
 		opts.Commits = &PushCommits{}
 	} else {
 		// if not the first commit, set the compare URL.
@@ -599,40 +607,38 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
 	apiRepo := repo.APIFormat(AccessModeNone)
 
 	var shaSum string
+	var isHookEventPush = false
 	switch opType {
 	case ActionCommitRepo: // Push
-		if err = PrepareWebhooks(repo, HookEventPush, &api.PushPayload{
-			Ref:        opts.RefFullName,
-			Before:     opts.OldCommitID,
-			After:      opts.NewCommitID,
-			CompareURL: setting.AppURL + opts.Commits.CompareURL,
-			Commits:    opts.Commits.ToAPIPayloadCommits(repo.HTMLURL()),
-			Repo:       apiRepo,
-			Pusher:     apiPusher,
-			Sender:     apiPusher,
-		}); err != nil {
-			return fmt.Errorf("PrepareWebhooks: %v", err)
-		}
+		isHookEventPush = true
 
 		if isNewBranch {
 			gitRepo, err := git.OpenRepository(repo.RepoPath())
 			if err != nil {
 				log.Error(4, "OpenRepository[%s]: %v", repo.RepoPath(), err)
 			}
+
 			shaSum, err = gitRepo.GetBranchCommitID(refName)
 			if err != nil {
 				log.Error(4, "GetBranchCommitID[%s]: %v", opts.RefFullName, err)
 			}
-			return PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
+			if err = PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
 				Ref:     refName,
 				Sha:     shaSum,
 				RefType: "branch",
 				Repo:    apiRepo,
 				Sender:  apiPusher,
-			})
+			}); err != nil {
+				return fmt.Errorf("PrepareWebhooks: %v", err)
+			}
 		}
 
+	case ActionDeleteBranch: // Delete Branch
+		isHookEventPush = true
+
 	case ActionPushTag: // Create
+		isHookEventPush = true
+
 		gitRepo, err := git.OpenRepository(repo.RepoPath())
 		if err != nil {
 			log.Error(4, "OpenRepository[%s]: %v", repo.RepoPath(), err)
@@ -641,13 +647,32 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
 		if err != nil {
 			log.Error(4, "GetTagCommitID[%s]: %v", opts.RefFullName, err)
 		}
-		return PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
+		if err = PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{
 			Ref:     refName,
 			Sha:     shaSum,
 			RefType: "tag",
 			Repo:    apiRepo,
 			Sender:  apiPusher,
-		})
+		}); err != nil {
+			return fmt.Errorf("PrepareWebhooks: %v", err)
+		}
+	case ActionDeleteTag: // Delete Tag
+		isHookEventPush = true
+	}
+
+	if isHookEventPush {
+		if err = PrepareWebhooks(repo, HookEventPush, &api.PushPayload{
+			Ref:        opts.RefFullName,
+			Before:     opts.OldCommitID,
+			After:      opts.NewCommitID,
+			CompareURL: setting.AppURL + opts.Commits.CompareURL,
+			Commits:    opts.Commits.ToAPIPayloadCommits(repo.HTMLURL()),
+			Repo:       apiRepo,
+			Pusher:     apiPusher,
+			Sender:     apiPusher,
+		}); err != nil {
+			return fmt.Errorf("PrepareWebhooks: %v", err)
+		}
 	}
 
 	return nil

Diff for: models/action_test.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 	"testing"
 
+	"code.gitea.io/git"
 	"code.gitea.io/gitea/modules/setting"
 
 	"github.com/stretchr/testify/assert"
@@ -202,55 +203,116 @@ func TestUpdateIssuesCommit(t *testing.T) {
 	CheckConsistencyFor(t, &Action{})
 }
 
-func TestCommitRepoAction(t *testing.T) {
-	assert.NoError(t, PrepareTestDatabase())
-
-	user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
-	repo := AssertExistsAndLoadBean(t, &Repository{ID: 2, OwnerID: user.ID}).(*Repository)
-	repo.Owner = user
+func testCorrectRepoAction(t *testing.T, opts CommitRepoActionOptions, actionBean *Action) {
+	AssertNotExistsBean(t, actionBean)
+	assert.NoError(t, CommitRepoAction(opts))
+	AssertExistsAndLoadBean(t, actionBean)
+	CheckConsistencyFor(t, &Action{})
+}
 
-	pushCommits := NewPushCommits()
-	pushCommits.Commits = []*PushCommit{
+func TestCommitRepoAction(t *testing.T) {
+	samples := []struct {
+		userID                  int64
+		repositoryID            int64
+		commitRepoActionOptions CommitRepoActionOptions
+		action                  Action
+	}{
 		{
-			Sha1:           "abcdef1",
-			CommitterEmail: "user2@example.com",
-			CommitterName:  "User Two",
-			AuthorEmail:    "user4@example.com",
-			AuthorName:     "User Four",
-			Message:        "message1",
+			userID:       2,
+			repositoryID: 2,
+			commitRepoActionOptions: CommitRepoActionOptions{
+				RefFullName: "refName",
+				OldCommitID: "oldCommitID",
+				NewCommitID: "newCommitID",
+				Commits: &PushCommits{
+					avatars: make(map[string]string),
+					Commits: []*PushCommit{
+						{
+							Sha1:           "abcdef1",
+							CommitterEmail: "user2@example.com",
+							CommitterName:  "User Two",
+							AuthorEmail:    "user4@example.com",
+							AuthorName:     "User Four",
+							Message:        "message1",
+						},
+						{
+							Sha1:           "abcdef2",
+							CommitterEmail: "user2@example.com",
+							CommitterName:  "User Two",
+							AuthorEmail:    "user2@example.com",
+							AuthorName:     "User Two",
+							Message:        "message2",
+						},
+					},
+					Len: 2,
+				},
+			},
+			action: Action{
+				OpType:  ActionCommitRepo,
+				RefName: "refName",
+			},
 		},
 		{
-			Sha1:           "abcdef2",
-			CommitterEmail: "user2@example.com",
-			CommitterName:  "User Two",
-			AuthorEmail:    "user2@example.com",
-			AuthorName:     "User Two",
-			Message:        "message2",
+			userID:       2,
+			repositoryID: 1,
+			commitRepoActionOptions: CommitRepoActionOptions{
+				RefFullName: git.TagPrefix + "v1.1",
+				OldCommitID: git.EmptySHA,
+				NewCommitID: "newCommitID",
+				Commits:     &PushCommits{},
+			},
+			action: Action{
+				OpType:  ActionPushTag,
+				RefName: "v1.1",
+			},
+		},
+		{
+			userID:       2,
+			repositoryID: 1,
+			commitRepoActionOptions: CommitRepoActionOptions{
+				RefFullName: git.TagPrefix + "v1.1",
+				OldCommitID: "oldCommitID",
+				NewCommitID: git.EmptySHA,
+				Commits:     &PushCommits{},
+			},
+			action: Action{
+				OpType:  ActionDeleteTag,
+				RefName: "v1.1",
+			},
+		},
+		{
+			userID:       2,
+			repositoryID: 1,
+			commitRepoActionOptions: CommitRepoActionOptions{
+				RefFullName: git.BranchPrefix + "feature/1",
+				OldCommitID: "oldCommitID",
+				NewCommitID: git.EmptySHA,
+				Commits:     &PushCommits{},
+			},
+			action: Action{
+				OpType:  ActionDeleteBranch,
+				RefName: "feature/1",
+			},
 		},
 	}
-	pushCommits.Len = len(pushCommits.Commits)
 
-	actionBean := &Action{
-		OpType:    ActionCommitRepo,
-		ActUserID: user.ID,
-		ActUser:   user,
-		RepoID:    repo.ID,
-		Repo:      repo,
-		RefName:   "refName",
-		IsPrivate: repo.IsPrivate,
+	for _, s := range samples {
+		prepareTestEnv(t)
+
+		user := AssertExistsAndLoadBean(t, &User{ID: s.userID}).(*User)
+		repo := AssertExistsAndLoadBean(t, &Repository{ID: s.repositoryID, OwnerID: user.ID}).(*Repository)
+		repo.Owner = user
+
+		s.commitRepoActionOptions.PusherName = user.Name
+		s.commitRepoActionOptions.RepoOwnerID = user.ID
+		s.commitRepoActionOptions.RepoName = repo.Name
+
+		s.action.ActUserID = user.ID
+		s.action.RepoID = repo.ID
+		s.action.IsPrivate = repo.IsPrivate
+
+		testCorrectRepoAction(t, s.commitRepoActionOptions, &s.action)
 	}
-	AssertNotExistsBean(t, actionBean)
-	assert.NoError(t, CommitRepoAction(CommitRepoActionOptions{
-		PusherName:  user.Name,
-		RepoOwnerID: user.ID,
-		RepoName:    repo.Name,
-		RefFullName: "refName",
-		OldCommitID: "oldCommitID",
-		NewCommitID: "newCommitID",
-		Commits:     pushCommits,
-	}))
-	AssertExistsAndLoadBean(t, actionBean)
-	CheckConsistencyFor(t, &Action{})
 }
 
 func TestTransferRepoAction(t *testing.T) {

Diff for: models/unit_tests.go

@@ -5,8 +5,12 @@
 package models
 
 import (
+	"os"
 	"testing"
 
+	"code.gitea.io/gitea/modules/setting"
+
+	"github.com/Unknwon/com"
 	"github.com/go-xorm/core"
 	"github.com/go-xorm/xorm"
 	"github.com/stretchr/testify/assert"
@@ -38,6 +42,12 @@ func PrepareTestDatabase() error {
 	return LoadFixtures()
 }
 
+func prepareTestEnv(t testing.TB) {
+	assert.NoError(t, PrepareTestDatabase())
+	assert.NoError(t, os.RemoveAll(setting.RepoRootPath))
+	assert.NoError(t, com.CopyDir("../integrations/gitea-repositories-meta", setting.RepoRootPath))
+}
+
 type testCond struct {
 	query interface{}
 	args  []interface{}