Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Webhooks: for issue close/reopen action, add commit ID that caused it #22583

Merged
merged 1 commit into from
Jan 25, 2023
Merged

Webhooks: for issue close/reopen action, add commit ID that caused it #22583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/notification/action/action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
2 changes: 1 addition & 1 deletion modules/notification/base/notifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, if we sort parameters based on importance, wouldn't it make more sense to add it before or after closeOrReopen?

Copy link
Contributor Author

@brechtvl brechtvl Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My rationale was sort of following the order of operations, user X does commit Y which then affects issue Z. But I can of course change it to whatever order is preferred.

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)
Expand Down
2 changes: 1 addition & 1 deletion modules/notification/base/null.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion modules/notification/mail/mail.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions modules/notification/notification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion modules/notification/ui/ui.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions modules/structs/hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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"`
}

Expand Down
4 changes: 2 additions & 2 deletions routers/api/v1/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions routers/web/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion services/issue/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down
8 changes: 4 additions & 4 deletions services/issue/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
2 changes: 1 addition & 1 deletion services/pull/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions services/pull/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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)
}
}
Expand Down
4 changes: 3 additions & 1 deletion services/webhook/notifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand Down