From 1eedd983ea0f387b6e72264b1cd195a4d3ea1253 Mon Sep 17 00:00:00 2001 From: David Schneiderbauer Date: Thu, 21 Sep 2017 09:43:26 +0200 Subject: [PATCH] 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 --- .../user2/repo1.git/refs/tags/v1.1 | 1 + models/action.go | 95 +++++++---- models/action_test.go | 154 ++++++++++++------ models/unit_tests.go | 10 ++ models/update.go | 77 ++++----- modules/templates/helper.go | 2 +- options/locale/locale_en-US.ini | 2 + templates/user/dashboard/feeds.tmpl | 6 + 8 files changed, 221 insertions(+), 126 deletions(-) create mode 100644 integrations/gitea-repositories-meta/user2/repo1.git/refs/tags/v1.1 diff --git a/integrations/gitea-repositories-meta/user2/repo1.git/refs/tags/v1.1 b/integrations/gitea-repositories-meta/user2/repo1.git/refs/tags/v1.1 new file mode 100644 index 0000000000..f98a263be6 --- /dev/null +++ b/integrations/gitea-repositories-meta/user2/repo1.git/refs/tags/v1.1 @@ -0,0 +1 @@ +65f1bf27bc3bf70f64657658635e66094edbcb4d diff --git a/models/action.go b/models/action.go index e687336229..27cfdc865c 100644 --- a/models/action.go +++ b/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,8 +607,60 @@ func CommitRepoAction(opts CommitRepoActionOptions) error { apiRepo := repo.APIFormat(AccessModeNone) var shaSum string + var isHookEventPush = false switch opType { case ActionCommitRepo: // Push + 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) + } + 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) + } + shaSum, err = gitRepo.GetTagCommitID(refName) + if err != nil { + log.Error(4, "GetTagCommitID[%s]: %v", opts.RefFullName, err) + } + 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, @@ -613,41 +673,6 @@ func CommitRepoAction(opts CommitRepoActionOptions) error { }); 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{ - Ref: refName, - Sha: shaSum, - RefType: "branch", - Repo: apiRepo, - Sender: apiPusher, - }) - } - - case ActionPushTag: // Create - gitRepo, err := git.OpenRepository(repo.RepoPath()) - if err != nil { - log.Error(4, "OpenRepository[%s]: %v", repo.RepoPath(), err) - } - shaSum, err = gitRepo.GetTagCommitID(refName) - if err != nil { - log.Error(4, "GetTagCommitID[%s]: %v", opts.RefFullName, err) - } - return PrepareWebhooks(repo, HookEventCreate, &api.CreatePayload{ - Ref: refName, - Sha: shaSum, - RefType: "tag", - Repo: apiRepo, - Sender: apiPusher, - }) } return nil diff --git a/models/action_test.go b/models/action_test.go index cecc83f86b..66400cd50c 100644 --- a/models/action_test.go +++ b/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,57 +203,118 @@ 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 - - pushCommits := NewPushCommits() - pushCommits.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", - }, - } - pushCommits.Len = len(pushCommits.Commits) - - actionBean := &Action{ - OpType: ActionCommitRepo, - ActUserID: user.ID, - ActUser: user, - RepoID: repo.ID, - Repo: repo, - RefName: "refName", - IsPrivate: repo.IsPrivate, - } +func testCorrectRepoAction(t *testing.T, opts CommitRepoActionOptions, actionBean *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, - })) + assert.NoError(t, CommitRepoAction(opts)) AssertExistsAndLoadBean(t, actionBean) CheckConsistencyFor(t, &Action{}) } +func TestCommitRepoAction(t *testing.T) { + samples := []struct { + userID int64 + repositoryID int64 + commitRepoActionOptions CommitRepoActionOptions + action Action + }{ + { + 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", + }, + }, + { + 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", + }, + }, + } + + 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) + } +} + func TestTransferRepoAction(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) diff --git a/models/unit_tests.go b/models/unit_tests.go index ce328b579d..f7a06bf0ef 100644 --- a/models/unit_tests.go +++ b/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{} diff --git a/models/update.go b/models/update.go index 56a227b24a..474b989bbd 100644 --- a/models/update.go +++ b/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 err := CommitRepoAction(CommitRepoActionOptions{ - PusherName: opts.PusherName, - RepoOwnerID: owner.ID, - RepoName: repo.Name, - RefFullName: opts.RefFullName, - OldCommitID: opts.OldCommitID, - NewCommitID: opts.NewCommitID, - Commits: &PushCommits{}, - }); err != nil { - return nil, fmt.Errorf("CommitRepoAction (tag): %v", err) + // If is tag reference + if isDelRef { + err = pushUpdateDeleteTag(repo, gitRepo, opts.RefFullName[len(git.TagPrefix):]) + if err != nil { + return nil, fmt.Errorf("pushUpdateDeleteTag: %v", err) + } + } else { + err = pushUpdateAddTag(repo, gitRepo, opts.RefFullName[len(git.TagPrefix):]) + if err != nil { + return nil, fmt.Errorf("pushUpdateAddTag: %v", err) + } } - return repo, nil - } - - 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) + } else if !isDelRef { + // If is branch reference + newCommit, err := gitRepo.GetCommit(opts.NewCommitID) 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) - if err != nil { - return nil, fmt.Errorf("newCommit.CommitsBeforeUntil: %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) + } + } 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 } diff --git a/modules/templates/helper.go b/modules/templates/helper.go index e9613b5445..0d307f4e9d 100644 --- a/modules/templates/helper.go +++ b/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" diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 6d4c79e8f5..f39ff0fa60 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1436,6 +1436,8 @@ comment_issue = `commented on issue %s#%[2]s` merge_pull_request = `merged pull request %s#%[2]s` transfer_repo = transferred repository %s to %s push_tag = pushed tag %[2]s to %[3]s +delete_tag = deleted tag %[2]s from %[3]s +delete_branch = deleted branch %[2]s from %[3]s compare_commits = Compare %d commits [tool] diff --git a/templates/user/dashboard/feeds.tmpl b/templates/user/dashboard/feeds.tmpl index 855a79786a..e94ef4d6b8 100644 --- a/templates/user/dashboard/feeds.tmpl +++ b/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}}

{{if eq .GetOpType 5}}