Webhooks: for issue close/reopen action, add commit ID that caused it if any

brechtvl · techknowlogick · commit f437ec669da9 · 2023-01-24T22:39:03.000-05:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ func (a actionNotifier) NotifyNewIssue(ctx context.Context, issue issues_model`
`56`	`56`	`}`
`57`	`57`
`58`	`58`	`// NotifyIssueChangeStatus notifies close or reopen issue to notifiers`
`59`		`-func (a actionNotifier) NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, issue issues_model.Issue, actionComment issues_model.Comment, closeOrReopen bool) {`
	`59`	`+func (a actionNotifier) NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, commitID string, issue issues_model.Issue, actionComment issues_model.Comment, closeOrReopen bool) {`
`60`	`60`	`// Compose comment action, could be plain comment, close or reopen issue/pull request.`
`61`	`61`	`// This object will be used to notify watchers in the end of function.`
`62`	`62`	`act := &activities_model.Action{`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ func (NullNotifier) NotifyNewIssue(ctx context.Context, issue issues_model.Iss`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`// NotifyIssueChangeStatus places a place holder function`
`35`		`-func (NullNotifier) NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, issue issues_model.Issue, actionComment issues_model.Comment, isClosed bool) {`
	`35`	`+func (NullNotifier) NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, commitID string, issue issues_model.Issue, actionComment issues_model.Comment, isClosed bool) {`
`36`	`36`	`}`
`37`	`37`
`38`	`38`	`// NotifyDeleteIssue notify when some issue deleted`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ func (m mailNotifier) NotifyNewIssue(ctx context.Context, issue issues_model.I`
`54`	`54`	`}`
`55`	`55`	`}`
`56`	`56`
`57`		`-func (m mailNotifier) NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, issue issues_model.Issue, actionComment issues_model.Comment, isClosed bool) {`
	`57`	`+func (m mailNotifier) NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, commitID string, issue issues_model.Issue, actionComment issues_model.Comment, isClosed bool) {`
`58`	`58`	`var actionType activities_model.ActionType`
`59`	`59`	`if issue.IsPull {`
`60`	`60`	`if isClosed {`
Original file line number	Diff line number	Diff line change
`@@ -77,9 +77,9 @@ func NotifyNewIssue(ctx context.Context, issue issues_model.Issue, mentions []`
`77`	`77`	`}`
`78`	`78`
`79`	`79`	`// NotifyIssueChangeStatus notifies close or reopen issue to notifiers`
`80`		`-func NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, issue issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) {`
	`80`	`+func NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, commitID string, issue issues_model.Issue, actionComment *issues_model.Comment, closeOrReopen bool) {`
`81`	`81`	`for _, notifier := range notifiers {`
`82`		`- notifier.NotifyIssueChangeStatus(ctx, doer, issue, actionComment, closeOrReopen)`
	`82`	`+ notifier.NotifyIssueChangeStatus(ctx, doer, commitID, issue, actionComment, closeOrReopen)`
`83`	`83`	`}`
`84`	`84`	`}`
`85`	`85`
Original file line number	Diff line number	Diff line change
`@@ -93,7 +93,7 @@ func (ns notificationService) NotifyNewIssue(ctx context.Context, issue issues`
`93`	`93`	`}`
`94`	`94`	`}`
`95`	`95`
`96`		`-func (ns notificationService) NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, issue issues_model.Issue, actionComment issues_model.Comment, isClosed bool) {`
	`96`	`+func (ns notificationService) NotifyIssueChangeStatus(ctx context.Context, doer user_model.User, commitID string, issue issues_model.Issue, actionComment issues_model.Comment, isClosed bool) {`
`97`	`97`	`_ = ns.issueQueue.Push(issueNotificationOpts{`
`98`	`98`	`IssueID: issue.ID,`
`99`	`99`	`NotificationAuthorID: doer.ID,`
Original file line number	Diff line number	Diff line change
`@@ -352,6 +352,7 @@ type IssuePayload struct {`
`352`	`352`	Issue *Issue `json:"issue"`
`353`	`353`	Repository *Repository `json:"repository"`
`354`	`354`	Sender *User `json:"sender"`
	`355`	+ CommitID string `json:"commit_id"`
`355`	`356`	`}`
`356`	`357`
`357`	`358`	`// JSONPayload encodes the IssuePayload to JSON, with an indentation of two spaces.`
`@@ -386,6 +387,7 @@ type PullRequestPayload struct {`
`386`	`387`	PullRequest *PullRequest `json:"pull_request"`
`387`	`388`	Repository *Repository `json:"repository"`
`388`	`389`	Sender *User `json:"sender"`
	`390`	+ CommitID string `json:"commit_id"`
`389`	`391`	Review *ReviewPayload `json:"review"`
`390`	`392`	`}`
`391`	`393`
Original file line number	Diff line number	Diff line change
`@@ -654,7 +654,7 @@ func CreateIssue(ctx *context.APIContext) {`
`654`	`654`	`}`
`655`	`655`
`656`	`656`	`if form.Closed {`
`657`		`- if err := issue_service.ChangeStatus(issue, ctx.Doer, true); err != nil {`
	`657`	`+ if err := issue_service.ChangeStatus(issue, ctx.Doer, "", true); err != nil {`
`658`	`658`	`if issues_model.IsErrDependenciesLeft(err) {`
`659`	`659`	`ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies")`
`660`	`660`	`return`
`@@ -826,7 +826,7 @@ func EditIssue(ctx *context.APIContext) {`
`826`	`826`	`}`
`827`	`827`
`828`	`828`	`if statusChangeComment != nil {`
`829`		`- notification.NotifyIssueChangeStatus(ctx, ctx.Doer, issue, statusChangeComment, issue.IsClosed)`
	`829`	`+ notification.NotifyIssueChangeStatus(ctx, ctx.Doer, "", issue, statusChangeComment, issue.IsClosed)`
`830`	`830`	`}`
`831`	`831`
`832`	`832`	`// Refetch from database to assign some automatic values`
Original file line number	Diff line number	Diff line change
`@@ -600,7 +600,7 @@ func EditPullRequest(ctx *context.APIContext) {`
`600`	`600`	`}`
`601`	`601`
`602`	`602`	`if statusChangeComment != nil {`
`603`		`- notification.NotifyIssueChangeStatus(ctx, ctx.Doer, issue, statusChangeComment, issue.IsClosed)`
	`603`	`+ notification.NotifyIssueChangeStatus(ctx, ctx.Doer, "", issue, statusChangeComment, issue.IsClosed)`
`604`	`604`	`}`
`605`	`605`
`606`	`606`	`// change pull target branch`
Original file line number	Diff line number	Diff line change
`@@ -193,7 +193,7 @@ func UpdateIssuesCommit(doer user_model.User, repo repo_model.Repository, comm`
`193`	`193`	`}`
`194`	`194`	`if close != refIssue.IsClosed {`
`195`	`195`	`refIssue.Repo = refRepo`
`196`		`- if err := ChangeStatus(refIssue, doer, close); err != nil {`
	`196`	`+ if err := ChangeStatus(refIssue, doer, c.Sha1, close); err != nil {`
`197`	`197`	`return err`
`198`	`198`	`}`
`199`	`199`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,13 +14,13 @@ import (`
`14`	`14`	`)`
`15`	`15`
`16`	`16`	`// ChangeStatus changes issue status to open or closed.`
`17`		`-func ChangeStatus(issue issues_model.Issue, doer user_model.User, closed bool) error {`
`18`		`- return changeStatusCtx(db.DefaultContext, issue, doer, closed)`
	`17`	`+func ChangeStatus(issue issues_model.Issue, doer user_model.User, commitID string, closed bool) error {`
	`18`	`+ return changeStatusCtx(db.DefaultContext, issue, doer, commitID, closed)`
`19`	`19`	`}`
`20`	`20`
`21`	`21`	`// changeStatusCtx changes issue status to open or closed.`
`22`	`22`	`// TODO: if context is not db.DefaultContext we get a deadlock!!!`
`23`		`-func changeStatusCtx(ctx context.Context, issue issues_model.Issue, doer user_model.User, closed bool) error {`
	`23`	`+func changeStatusCtx(ctx context.Context, issue issues_model.Issue, doer user_model.User, commitID string, closed bool) error {`
`24`	`24`	`comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, closed)`
`25`	`25`	`if err != nil {`
`26`	`26`	`if issues_model.IsErrDependenciesLeft(err) && closed {`
`@@ -37,7 +37,7 @@ func changeStatusCtx(ctx context.Context, issue issues_model.Issue, doer user_`
`37`	`37`	`}`
`38`	`38`	`}`
`39`	`39`
`40`		`- notification.NotifyIssueChangeStatus(ctx, doer, issue, comment, closed)`
	`40`	`+ notification.NotifyIssueChangeStatus(ctx, doer, commitID, issue, comment, closed)`
`41`	`41`
`42`	`42`	`return nil`
`43`	`43`	`}`