From c8139c0f642a308b544d2f17e7b728ee6762a0eb Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Wed, 25 Jan 2023 05:47:53 +0100 Subject: [PATCH] Webhooks: for issue close/reopen action, add commit ID that caused it (#22583) The `commit_id` property name is the same as equivalent functionality in GitHub. If the action was not caused by a commit, an empty string is used. This can for example be used to automatically add a Resolved label to an issue fixed by a commit, or clear it when the issue is reopened. --- modules/notification/action/action.go | 2 +- modules/notification/base/notifier.go | 2 +- modules/notification/base/null.go | 2 +- modules/notification/mail/mail.go | 2 +- modules/notification/notification.go | 4 ++-- modules/notification/ui/ui.go | 2 +- modules/structs/hook.go | 2 ++ routers/api/v1/repo/issue.go | 4 ++-- routers/api/v1/repo/pull.go | 2 +- routers/web/repo/issue.go | 4 ++-- services/issue/commit.go | 2 +- services/issue/status.go | 8 ++++---- services/pull/merge.go | 2 +- services/pull/pull.go | 4 ++-- services/webhook/notifier.go | 4 +++- 15 files changed, 25 insertions(+), 21 deletions(-) diff --git a/modules/notification/action/action.go b/modules/notification/action/action.go index 2f882c2cb..c043ef62d 100644 --- a/modules/notification/action/action.go +++ b/modules/notification/action/action.go @@ -56,7 +56,7 @@ func (a *actionNotifier) NotifyNewIssue(ctx context.Context, issue *issues_model } // NotifyIssueChangeStatus notifies close or reopen issue to notifiers -func (a *actionNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) { +func (a *actionNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, commitID string, issue *issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) { // Compose comment action, could be plain comment, close or reopen issue/pull request. // This object will be used to notify watchers in the end of function. act := &activities_model.Action{ diff --git a/modules/notification/base/notifier.go b/modules/notification/base/notifier.go index dbed20ba3..4021bbe14 100644 --- a/modules/notification/base/notifier.go +++ b/modules/notification/base/notifier.go @@ -23,7 +23,7 @@ type Notifier interface { NotifyRenameRepository(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, oldRepoName string) NotifyTransferRepository(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, oldOwnerName string) NotifyNewIssue(ctx context.Context, issue *issues_model.Issue, mentions []*user_model.User) - NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) + NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, commitID string, issue *issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) NotifyDeleteIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Issue) NotifyIssueChangeMilestone(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, oldMilestoneID int64) NotifyIssueChangeAssignee(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, assignee *user_model.User, removed bool, comment *issues_model.Comment) diff --git a/modules/notification/base/null.go b/modules/notification/base/null.go index de5f072d2..161eadfbe 100644 --- a/modules/notification/base/null.go +++ b/modules/notification/base/null.go @@ -32,7 +32,7 @@ func (*NullNotifier) NotifyNewIssue(ctx context.Context, issue *issues_model.Iss } // NotifyIssueChangeStatus places a place holder function -func (*NullNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, actionComment *issues_model.Comment, isClosed bool) { +func (*NullNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, commitID string, issue *issues_model.Issue, actionComment *issues_model.Comment, isClosed bool) { } // NotifyDeleteIssue notify when some issue deleted diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index 18f7fa22a..7e54df44c 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -54,7 +54,7 @@ func (m *mailNotifier) NotifyNewIssue(ctx context.Context, issue *issues_model.I } } -func (m *mailNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, actionComment *issues_model.Comment, isClosed bool) { +func (m *mailNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, commitID string, issue *issues_model.Issue, actionComment *issues_model.Comment, isClosed bool) { var actionType activities_model.ActionType if issue.IsPull { if isClosed { diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 10581eb87..2300b68f7 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -77,9 +77,9 @@ func NotifyNewIssue(ctx context.Context, issue *issues_model.Issue, mentions []* } // NotifyIssueChangeStatus notifies close or reopen issue to notifiers -func NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) { +func NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, commitID string, issue *issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) { for _, notifier := range notifiers { - notifier.NotifyIssueChangeStatus(ctx, doer, issue, actionComment, closeOrReopen) + notifier.NotifyIssueChangeStatus(ctx, doer, commitID, issue, actionComment, closeOrReopen) } } diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go index bc66c3d5a..4b85f17b6 100644 --- a/modules/notification/ui/ui.go +++ b/modules/notification/ui/ui.go @@ -93,7 +93,7 @@ func (ns *notificationService) NotifyNewIssue(ctx context.Context, issue *issues } } -func (ns *notificationService) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, actionComment *issues_model.Comment, isClosed bool) { +func (ns *notificationService) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, commitID string, issue *issues_model.Issue, actionComment *issues_model.Comment, isClosed bool) { _ = ns.issueQueue.Push(issueNotificationOpts{ IssueID: issue.ID, NotificationAuthorID: doer.ID, diff --git a/modules/structs/hook.go b/modules/structs/hook.go index b722e32ca..df5da6790 100644 --- a/modules/structs/hook.go +++ b/modules/structs/hook.go @@ -352,6 +352,7 @@ type IssuePayload struct { Issue *Issue `json:"issue"` Repository *Repository `json:"repository"` Sender *User `json:"sender"` + CommitID string `json:"commit_id"` } // JSONPayload encodes the IssuePayload to JSON, with an indentation of two spaces. @@ -386,6 +387,7 @@ type PullRequestPayload struct { PullRequest *PullRequest `json:"pull_request"` Repository *Repository `json:"repository"` Sender *User `json:"sender"` + CommitID string `json:"commit_id"` Review *ReviewPayload `json:"review"` } diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index fecb601dd..458838b93 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -654,7 +654,7 @@ func CreateIssue(ctx *context.APIContext) { } if form.Closed { - if err := issue_service.ChangeStatus(issue, ctx.Doer, true); err != nil { + if err := issue_service.ChangeStatus(issue, ctx.Doer, "", true); err != nil { if issues_model.IsErrDependenciesLeft(err) { ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") return @@ -826,7 +826,7 @@ func EditIssue(ctx *context.APIContext) { } if statusChangeComment != nil { - notification.NotifyIssueChangeStatus(ctx, ctx.Doer, issue, statusChangeComment, issue.IsClosed) + notification.NotifyIssueChangeStatus(ctx, ctx.Doer, "", issue, statusChangeComment, issue.IsClosed) } // Refetch from database to assign some automatic values diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 8fdbec4b8..8164b3869 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -600,7 +600,7 @@ func EditPullRequest(ctx *context.APIContext) { } if statusChangeComment != nil { - notification.NotifyIssueChangeStatus(ctx, ctx.Doer, issue, statusChangeComment, issue.IsClosed) + notification.NotifyIssueChangeStatus(ctx, ctx.Doer, "", issue, statusChangeComment, issue.IsClosed) } // change pull target branch diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 44ac81f65..edd9821ac 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2599,7 +2599,7 @@ func UpdateIssueStatus(ctx *context.Context) { } for _, issue := range issues { if issue.IsClosed != isClosed { - if err := issue_service.ChangeStatus(issue, ctx.Doer, isClosed); err != nil { + if err := issue_service.ChangeStatus(issue, ctx.Doer, "", isClosed); err != nil { if issues_model.IsErrDependenciesLeft(err) { ctx.JSON(http.StatusPreconditionFailed, map[string]interface{}{ "error": "cannot close this issue because it still has open dependencies", @@ -2696,7 +2696,7 @@ func NewComment(ctx *context.Context) { ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index)) } else { isClosed := form.Status == "close" - if err := issue_service.ChangeStatus(issue, ctx.Doer, isClosed); err != nil { + if err := issue_service.ChangeStatus(issue, ctx.Doer, "", isClosed); err != nil { log.Error("ChangeStatus: %v", err) if issues_model.IsErrDependenciesLeft(err) { diff --git a/services/issue/commit.go b/services/issue/commit.go index c3d2e853b..7a8c71e60 100644 --- a/services/issue/commit.go +++ b/services/issue/commit.go @@ -193,7 +193,7 @@ func UpdateIssuesCommit(doer *user_model.User, repo *repo_model.Repository, comm } if close != refIssue.IsClosed { refIssue.Repo = refRepo - if err := ChangeStatus(refIssue, doer, close); err != nil { + if err := ChangeStatus(refIssue, doer, c.Sha1, close); err != nil { return err } } diff --git a/services/issue/status.go b/services/issue/status.go index 782ce0bd9..d4a0fce3e 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -14,13 +14,13 @@ import ( ) // ChangeStatus changes issue status to open or closed. -func ChangeStatus(issue *issues_model.Issue, doer *user_model.User, closed bool) error { - return changeStatusCtx(db.DefaultContext, issue, doer, closed) +func ChangeStatus(issue *issues_model.Issue, doer *user_model.User, commitID string, closed bool) error { + return changeStatusCtx(db.DefaultContext, issue, doer, commitID, closed) } // changeStatusCtx changes issue status to open or closed. // TODO: if context is not db.DefaultContext we get a deadlock!!! -func changeStatusCtx(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, closed bool) error { +func changeStatusCtx(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string, closed bool) error { comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, closed) if err != nil { if issues_model.IsErrDependenciesLeft(err) && closed { @@ -37,7 +37,7 @@ func changeStatusCtx(ctx context.Context, issue *issues_model.Issue, doer *user_ } } - notification.NotifyIssueChangeStatus(ctx, doer, issue, comment, closed) + notification.NotifyIssueChangeStatus(ctx, doer, commitID, issue, comment, closed) return nil } diff --git a/services/pull/merge.go b/services/pull/merge.go index bdd2cb0e8..7ffbdb78b 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -225,7 +225,7 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } close := ref.RefAction == references.XRefActionCloses if close != ref.Issue.IsClosed { - if err = issue_service.ChangeStatus(ref.Issue, doer, close); err != nil { + if err = issue_service.ChangeStatus(ref.Issue, doer, pr.MergedCommitID, close); err != nil { // Allow ErrDependenciesLeft if !issues_model.IsErrDependenciesLeft(err) { return err diff --git a/services/pull/pull.go b/services/pull/pull.go index 08f70a5e4..7f81def6d 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -532,7 +532,7 @@ func CloseBranchPulls(doer *user_model.User, repoID int64, branch string) error var errs errlist for _, pr := range prs { - if err = issue_service.ChangeStatus(pr.Issue, doer, true); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { + if err = issue_service.ChangeStatus(pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) } } @@ -566,7 +566,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re if pr.BaseRepoID == repo.ID { continue } - if err = issue_service.ChangeStatus(pr.Issue, doer, true); err != nil && !issues_model.IsErrPullWasClosed(err) { + if err = issue_service.ChangeStatus(pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) { errs = append(errs, err) } } diff --git a/services/webhook/notifier.go b/services/webhook/notifier.go index ee8076603..16d2b9581 100644 --- a/services/webhook/notifier.go +++ b/services/webhook/notifier.go @@ -229,7 +229,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(ctx context.Context, doer *user } } -func (m *webhookNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, actionComment *issues_model.Comment, isClosed bool) { +func (m *webhookNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *user_model.User, commitID string, issue *issues_model.Issue, actionComment *issues_model.Comment, isClosed bool) { mode, _ := access_model.AccessLevel(ctx, issue.Poster, issue.Repo) var err error if issue.IsPull { @@ -243,6 +243,7 @@ func (m *webhookNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *use PullRequest: convert.ToAPIPullRequest(ctx, issue.PullRequest, nil), Repository: convert.ToRepo(ctx, issue.Repo, mode), Sender: convert.ToUser(doer, nil), + CommitID: commitID, } if isClosed { apiPullRequest.Action = api.HookIssueClosed @@ -256,6 +257,7 @@ func (m *webhookNotifier) NotifyIssueChangeStatus(ctx context.Context, doer *use Issue: convert.ToAPIIssue(ctx, issue), Repository: convert.ToRepo(ctx, issue.Repo, mode), Sender: convert.ToUser(doer, nil), + CommitID: commitID, } if isClosed { apiIssue.Action = api.HookIssueClosed