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>

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

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

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{
+		isHookEventPush = true
-			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)
-		}
 
 		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

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) {
+func testCorrectRepoAction(t *testing.T, opts CommitRepoActionOptions, actionBean *Action) {
-	assert.NoError(t, PrepareTestDatabase())
+	AssertNotExistsBean(t, actionBean)
-
+	assert.NoError(t, CommitRepoAction(opts))
-	user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
+	AssertExistsAndLoadBean(t, actionBean)
-	repo := AssertExistsAndLoadBean(t, &Repository{ID: 2, OwnerID: user.ID}).(*Repository)
+	CheckConsistencyFor(t, &Action{})
-	repo.Owner = user
+}
 
-	pushCommits := NewPushCommits()
+func TestCommitRepoAction(t *testing.T) {
-	pushCommits.Commits = []*PushCommit{
+	samples := []struct {
+		userID                  int64
+		repositoryID            int64
+		commitRepoActionOptions CommitRepoActionOptions
+		action                  Action
+	}{
 		{
-			Sha1:           "abcdef1",
+			userID:       2,
-			CommitterEmail: "user2@example.com",
+			repositoryID: 2,
-			CommitterName:  "User Two",
+			commitRepoActionOptions: CommitRepoActionOptions{
-			AuthorEmail:    "user4@example.com",
+				RefFullName: "refName",
-			AuthorName:     "User Four",
+				OldCommitID: "oldCommitID",
-			Message:        "message1",
+				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",
+			userID:       2,
-			CommitterEmail: "user2@example.com",
+			repositoryID: 1,
-			CommitterName:  "User Two",
+			commitRepoActionOptions: CommitRepoActionOptions{
-			AuthorEmail:    "user2@example.com",
+				RefFullName: git.TagPrefix + "v1.1",
-			AuthorName:     "User Two",
+				OldCommitID: git.EmptySHA,
-			Message:        "message2",
+				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{
+	for _, s := range samples {
-		OpType:    ActionCommitRepo,
+		prepareTestEnv(t)
-		ActUserID: user.ID,
+
-		ActUser:   user,
+		user := AssertExistsAndLoadBean(t, &User{ID: s.userID}).(*User)
-		RepoID:    repo.ID,
+		repo := AssertExistsAndLoadBean(t, &Repository{ID: s.repositoryID, OwnerID: user.ID}).(*Repository)
-		Repo:      repo,
+		repo.Owner = user
-		RefName:   "refName",
+
-		IsPrivate: repo.IsPrivate,
+		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) {

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{}

models/update.go

@@ -198,57 +198,46 @@ func pushUpdate(opts PushUpdateOptions) (repo *Repository, err error) {
 		return nil, fmt.Errorf("OpenRepository: %v", err)
 	}
 
-	if isDelRef {
-		// Tag has been deleted
-		if strings.HasPrefix(opts.RefFullName, git.TagPrefix) {
-			err = pushUpdateDeleteTag(repo, gitRepo, opts.RefFullName[len(git.TagPrefix):])
-			if err != nil {
-				return nil, fmt.Errorf("pushUpdateDeleteTag: %v", err)
-			}
-		}
-		log.GitLogger.Info("Reference '%s' has been deleted from '%s/%s' by %s",
-			opts.RefFullName, opts.RepoUserName, opts.RepoName, opts.PusherName)
-		return repo, nil
-	}
-
 	if err = repo.UpdateSize(); err != nil {
 		log.Error(4, "Failed to update size for repository: %v", err)
 	}
 
-	// Push tags.
+	var commits = &PushCommits{}
 	if strings.HasPrefix(opts.RefFullName, git.TagPrefix) {
-		pushUpdateAddTag(repo, gitRepo, opts.RefFullName[len(git.TagPrefix):])
+		// If is tag reference
-		if err := CommitRepoAction(CommitRepoActionOptions{
+		if isDelRef {
-			PusherName:  opts.PusherName,
+			err = pushUpdateDeleteTag(repo, gitRepo, opts.RefFullName[len(git.TagPrefix):])
-			RepoOwnerID: owner.ID,
+			if err != nil {
-			RepoName:    repo.Name,
+				return nil, fmt.Errorf("pushUpdateDeleteTag: %v", err)
-			RefFullName: opts.RefFullName,
+			}
-			OldCommitID: opts.OldCommitID,
+		} else {
-			NewCommitID: opts.NewCommitID,
+			err = pushUpdateAddTag(repo, gitRepo, opts.RefFullName[len(git.TagPrefix):])
-			Commits:     &PushCommits{},
+			if err != nil {
-		}); err != nil {
+				return nil, fmt.Errorf("pushUpdateAddTag: %v", err)
-			return nil, fmt.Errorf("CommitRepoAction (tag): %v", err)
+			}
 		}
-		return repo, nil
+	} else if !isDelRef {
-	}
+		// If is branch reference
-
+		newCommit, err := gitRepo.GetCommit(opts.NewCommitID)
-	newCommit, err := gitRepo.GetCommit(opts.NewCommitID)
-	if err != nil {
-		return nil, fmt.Errorf("gitRepo.GetCommit: %v", err)
-	}
-
-	// Push new branch.
-	var l *list.List
-	if isNewRef {
-		l, err = newCommit.CommitsBeforeLimit(10)
 		if err != nil {
-			return nil, fmt.Errorf("newCommit.CommitsBeforeLimit: %v", err)
+			return nil, fmt.Errorf("gitRepo.GetCommit: %v", err)
 		}
-	} else {
+
-		l, err = newCommit.CommitsBeforeUntil(opts.OldCommitID)
+		// Push new branch.
-		if err != nil {
+		var l *list.List
-			return nil, fmt.Errorf("newCommit.CommitsBeforeUntil: %v", err)
+		if isNewRef {
+			l, err = newCommit.CommitsBeforeLimit(10)
+			if err != nil {
+				return nil, fmt.Errorf("newCommit.CommitsBeforeLimit: %v", err)
+			}
+		} else {
+			l, err = newCommit.CommitsBeforeUntil(opts.OldCommitID)
+			if err != nil {
+				return nil, fmt.Errorf("newCommit.CommitsBeforeUntil: %v", err)
+			}
 		}
+
+		commits = ListToPushCommits(l)
 	}
 
 	if err := CommitRepoAction(CommitRepoActionOptions{
@@ -258,9 +247,9 @@ func pushUpdate(opts PushUpdateOptions) (repo *Repository, err error) {
 		RefFullName: opts.RefFullName,
 		OldCommitID: opts.OldCommitID,
 		NewCommitID: opts.NewCommitID,
-		Commits:     ListToPushCommits(l),
+		Commits:     commits,
 	}); err != nil {
-		return nil, fmt.Errorf("CommitRepoAction (branch): %v", err)
+		return nil, fmt.Errorf("CommitRepoAction: %v", err)
 	}
 	return repo, nil
 }

modules/templates/helper.go

@@ -294,7 +294,7 @@ func ActionIcon(opType models.ActionType) string {
 	switch opType {
 	case models.ActionCreateRepo, models.ActionTransferRepo:
 		return "repo"
-	case models.ActionCommitRepo, models.ActionPushTag:
+	case models.ActionCommitRepo, models.ActionPushTag, models.ActionDeleteTag, models.ActionDeleteBranch:
 		return "git-commit"
 	case models.ActionCreateIssue:
 		return "issue-opened"

options/locale/locale_en-US.ini

@@ -1436,6 +1436,8 @@ comment_issue = `commented on issue <a href="%s/issues/%s">%s#%[2]s</a>`
 merge_pull_request = `merged pull request <a href="%s/pulls/%s">%s#%[2]s</a>`
 transfer_repo = transferred repository <code>%s</code> to <a href="%s">%s</a>
 push_tag = pushed tag <a href="%s/src/%s">%[2]s</a> to <a href="%[1]s">%[3]s</a>
+delete_tag = deleted tag %[2]s from <a href="%[1]s">%[3]s</a>
+delete_branch = deleted branch %[2]s from <a href="%[1]s">%[3]s</a>
 compare_commits = Compare %d commits
 
 [tool]

templates/user/dashboard/feeds.tmpl

@@ -43,6 +43,12 @@
 						{{else if eq .GetOpType 15}}
 							{{ $index := index .GetIssueInfos 0}}
 							{{$.i18n.Tr "action.reopen_pull_request" .GetRepoLink $index .ShortRepoPath | Str2html}}
+						{{else if eq .GetOpType 16}}
+							{{ $index := index .GetIssueInfos 0}}
+							{{$.i18n.Tr "action.delete_tag" .GetRepoLink .GetBranch .ShortRepoPath | Str2html}}
+						{{else if eq .GetOpType 17}}
+							{{ $index := index .GetIssueInfos 0}}
+							{{$.i18n.Tr "action.delete_branch" .GetRepoLink .GetBranch .ShortRepoPath | Str2html}}
 						{{end}}
 					</p>
 					{{if eq .GetOpType 5}}