From dc840a393fd78ab6558fa4c1862ef77ee8e7bf4e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 5 Dec 2019 20:44:14 +0800 Subject: [PATCH 1/2] Move comment action out of models package --- models/action.go | 15 +++++++- models/issue.go | 50 ++++++++++++------------- models/issue_comment.go | 2 +- models/issue_dependency_test.go | 2 +- models/issue_xref_test.go | 3 +- models/pull.go | 17 +++++++-- modules/notification/action/action.go | 33 ++++++++++++++++ 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/notification/webhook/webhook.go | 2 +- services/issue/status.go | 4 +- services/pull/merge.go | 2 +- 15 files changed, 98 insertions(+), 44 deletions(-) diff --git a/models/action.go b/models/action.go index 8673dd2946cdf..e89d22c618470 100644 --- a/models/action.go +++ b/models/action.go @@ -436,13 +436,26 @@ func changeIssueStatus(repo *Repository, issue *Issue, doer *User, status bool) } issue.Repo = repo - if err := issue.ChangeStatus(doer, status); err != nil { + comment, err := issue.ChangeStatus(doer, status) + if err != nil { // Don't return an error when dependencies are open as this would let the push fail if IsErrDependenciesLeft(err) { return stopTimerIfAvailable(doer, issue) } return err } + var tp = CommentTypeClose + if !status { + tp = CommentTypeReopen + } + if err := sendCreateCommentAction(x, &CreateCommentOptions{ + Type: tp, + Doer: doer, + Repo: issue.Repo, + Issue: issue, + }, comment); err != nil { + return fmt.Errorf("sendCreateCommentAction: %v", err) + } return stopTimerIfAvailable(doer, issue) } diff --git a/models/issue.go b/models/issue.go index 0a08a97fdd35b..48d430df917c6 100644 --- a/models/issue.go +++ b/models/issue.go @@ -600,16 +600,16 @@ func updateIssueCols(e Engine, issue *Issue, cols ...string) error { return nil } -func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (err error) { +func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (*Comment, error) { // Reload the issue currentIssue, err := getIssueByID(e, issue.ID) if err != nil { - return err + return nil, err } // Nothing should be performed if current status is same as target status if currentIssue.IsClosed == isClosed { - return nil + return nil, nil } // Check for open dependencies @@ -617,11 +617,11 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er // only check if dependencies are enabled and we're about to close an issue, otherwise reopening an issue would fail when there are unsatisfied dependencies noDeps, err := issueNoDependenciesLeft(e, issue) if err != nil { - return err + return nil, err } if !noDeps { - return ErrDependenciesLeft{issue.ID} + return nil, ErrDependenciesLeft{issue.ID} } } @@ -633,22 +633,22 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er } if err = updateIssueCols(e, issue, "is_closed", "closed_unix"); err != nil { - return err + return nil, err } // Update issue count of labels if err = issue.getLabels(e); err != nil { - return err + return nil, err } for idx := range issue.Labels { if err = updateLabel(e, issue.Labels[idx]); err != nil { - return err + return nil, err } } // Update issue count of milestone if err := updateMilestoneClosedNum(e, issue.MilestoneID); err != nil { - return err + return nil, err } // New action comment @@ -657,43 +657,39 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er cmtType = CommentTypeReopen } - var opts = &CreateCommentOptions{ + return createCommentWithNoAction(e, &CreateCommentOptions{ Type: cmtType, Doer: doer, Repo: issue.Repo, Issue: issue, - } - comment, err := createCommentWithNoAction(e, opts) - if err != nil { - return err - } - return sendCreateCommentAction(e, opts, comment) + }) } // ChangeStatus changes issue status to open or closed. -func (issue *Issue) ChangeStatus(doer *User, isClosed bool) (err error) { +func (issue *Issue) ChangeStatus(doer *User, isClosed bool) (*Comment, error) { sess := x.NewSession() defer sess.Close() - if err = sess.Begin(); err != nil { - return err + if err := sess.Begin(); err != nil { + return nil, err } - if err = issue.loadRepo(sess); err != nil { - return err + if err := issue.loadRepo(sess); err != nil { + return nil, err } - if err = issue.loadPoster(sess); err != nil { - return err + if err := issue.loadPoster(sess); err != nil { + return nil, err } - if err = issue.changeStatus(sess, doer, isClosed); err != nil { - return err + comment, err := issue.changeStatus(sess, doer, isClosed) + if err != nil { + return nil, err } if err = sess.Commit(); err != nil { - return fmt.Errorf("Commit: %v", err) + return nil, fmt.Errorf("Commit: %v", err) } - return nil + return comment, nil } // ChangeTitle changes the title of this issue, as the given user. diff --git a/models/issue_comment.go b/models/issue_comment.go index 5843689f1bbb4..d12cce3095343 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -596,7 +596,7 @@ func updateCommentInfos(e *xorm.Session, opts *CreateCommentOptions, comment *Co return updateIssueCols(e, opts.Issue, "updated_unix") } -func sendCreateCommentAction(e *xorm.Session, opts *CreateCommentOptions, comment *Comment) (err error) { +func sendCreateCommentAction(e Engine, opts *CreateCommentOptions, comment *Comment) (err error) { // 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 := &Action{ diff --git a/models/issue_dependency_test.go b/models/issue_dependency_test.go index ede9e008eb8cc..bf323abb98bf8 100644 --- a/models/issue_dependency_test.go +++ b/models/issue_dependency_test.go @@ -45,7 +45,7 @@ func TestCreateIssueDependency(t *testing.T) { assert.False(t, left) // Close #2 and check again - err = issue2.ChangeStatus(user1, true) + _, err = issue2.ChangeStatus(user1, true) assert.NoError(t, err) left, err = IssueNoDependenciesLeft(issue1) diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go index c13577e905aeb..31805e9059c3e 100644 --- a/models/issue_xref_test.go +++ b/models/issue_xref_test.go @@ -94,7 +94,8 @@ func TestXRef_ResolveCrossReferences(t *testing.T) { i1 := testCreateIssue(t, 1, 2, "title1", "content1", false) i2 := testCreateIssue(t, 1, 2, "title2", "content2", false) i3 := testCreateIssue(t, 1, 2, "title3", "content3", false) - assert.NoError(t, i3.ChangeStatus(d, true)) + _, err := i3.ChangeStatus(d, true) + assert.NoError(t, err) pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index)) rp := AssertExistsAndLoadBean(t, &Comment{IssueID: i1.ID, RefIssueID: pr.Issue.ID, RefCommentID: 0}).(*Comment) diff --git a/models/pull.go b/models/pull.go index 27824c546a1a8..e6b733ee34461 100644 --- a/models/pull.go +++ b/models/pull.go @@ -445,7 +445,7 @@ func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) { } // SetMerged sets a pull request to merged and closes the corresponding issue -func (pr *PullRequest) SetMerged() (err error) { +func (pr *PullRequest) SetMerged(doer *User) (err error) { if pr.HasMerged { return fmt.Errorf("PullRequest[%d] already merged", pr.Index) } @@ -472,9 +472,20 @@ func (pr *PullRequest) SetMerged() (err error) { return err } - if err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil { + comment, err := pr.Issue.changeStatus(sess, pr.Merger, true) + if err != nil { return fmt.Errorf("Issue.changeStatus: %v", err) } + + if err := sendCreateCommentAction(sess, &CreateCommentOptions{ + Type: CommentTypeClose, + Doer: doer, + Repo: pr.Issue.Repo, + Issue: pr.Issue, + }, comment); err != nil { + return fmt.Errorf("sendCreateCommentAction: %v", err) + } + if _, err = sess.ID(pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil { return fmt.Errorf("update pull request: %v", err) } @@ -512,7 +523,7 @@ func (pr *PullRequest) manuallyMerged() bool { pr.Merger = merger pr.MergerID = merger.ID - if err = pr.SetMerged(); err != nil { + if err = pr.SetMerged(merger); err != nil { log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) return false } diff --git a/modules/notification/action/action.go b/modules/notification/action/action.go index 9caeb5aac0936..82834c7bce0ab 100644 --- a/modules/notification/action/action.go +++ b/modules/notification/action/action.go @@ -53,6 +53,39 @@ func (a *actionNotifier) NotifyNewIssue(issue *models.Issue) { } } +// NotifyIssueChangeStatus notifies close or reopen issue to notifiers +func (a *actionNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.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 := &models.Action{ + ActUserID: doer.ID, + ActUser: doer, + Content: fmt.Sprintf("%d|%s", issue.Index, ""), + RepoID: issue.Repo.ID, + Repo: issue.Repo, + Comment: actionComment, + CommentID: actionComment.ID, + IsPrivate: issue.Repo.IsPrivate, + } + // Check comment type. + if closeOrReopen { + act.OpType = models.ActionCloseIssue + if issue.IsPull { + act.OpType = models.ActionClosePullRequest + } + } else { + act.OpType = models.ActionReopenIssue + if issue.IsPull { + act.OpType = models.ActionReopenPullRequest + } + } + + // Notify watchers for whatever action comes in, ignore if no action type. + if err := models.NotifyWatchers(act); err != nil { + log.Error("NotifyWatchers: %v", err) + } +} + func (a *actionNotifier) NotifyNewPullRequest(pull *models.PullRequest) { if err := pull.LoadIssue(); err != nil { log.Error("pull.LoadIssue: %v", err) diff --git a/modules/notification/base/notifier.go b/modules/notification/base/notifier.go index c8c89d22bd0d1..934ee80aa784e 100644 --- a/modules/notification/base/notifier.go +++ b/modules/notification/base/notifier.go @@ -21,7 +21,7 @@ type Notifier interface { NotifyTransferRepository(doer *models.User, repo *models.Repository, oldOwnerName string) NotifyNewIssue(*models.Issue) - NotifyIssueChangeStatus(*models.User, *models.Issue, bool) + NotifyIssueChangeStatus(*models.User, *models.Issue, *models.Comment, bool) NotifyIssueChangeMilestone(doer *models.User, issue *models.Issue, oldMilestoneID int64) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment) NotifyIssueChangeContent(doer *models.User, issue *models.Issue, oldContent string) diff --git a/modules/notification/base/null.go b/modules/notification/base/null.go index 79e9f7f7fc8aa..f807fcb369949 100644 --- a/modules/notification/base/null.go +++ b/modules/notification/base/null.go @@ -31,7 +31,7 @@ func (*NullNotifier) NotifyNewIssue(issue *models.Issue) { } // NotifyIssueChangeStatus places a place holder function -func (*NullNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) { +func (*NullNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, comment *models.Comment, isClosed bool) { } // NotifyNewPullRequest places a place holder function diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index 558c9a6243bb5..43c45e25bcbfa 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -51,7 +51,7 @@ func (m *mailNotifier) NotifyNewIssue(issue *models.Issue) { } } -func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) { +func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, comment *models.Comment, isClosed bool) { var actionType models.ActionType if issue.IsPull { if isClosed { diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 71d6e79e6da52..37143cc2509aa 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -53,9 +53,9 @@ func NotifyNewIssue(issue *models.Issue) { } // NotifyIssueChangeStatus notifies close or reopen issue to notifiers -func NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, closeOrReopen bool) { +func NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, comment *models.Comment, closeOrReopen bool) { for _, notifier := range notifiers { - notifier.NotifyIssueChangeStatus(doer, issue, closeOrReopen) + notifier.NotifyIssueChangeStatus(doer, issue, comment, closeOrReopen) } } diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go index bfe497f866d46..5c9b35010a493 100644 --- a/modules/notification/ui/ui.go +++ b/modules/notification/ui/ui.go @@ -62,7 +62,7 @@ func (ns *notificationService) NotifyNewIssue(issue *models.Issue) { } } -func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) { +func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, comment *models.Comment, isClosed bool) { ns.issueQueue <- issueNotificationOpts{ issueID: issue.ID, notificationAuthorID: doer.ID, diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go index 4ef60fef845b4..6d85b37a72fb5 100644 --- a/modules/notification/webhook/webhook.go +++ b/modules/notification/webhook/webhook.go @@ -211,7 +211,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *model } } -func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) { +func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, comment *models.Comment, isClosed bool) { mode, _ := models.AccessLevel(issue.Poster, issue.Repo) var err error if issue.IsPull { diff --git a/services/issue/status.go b/services/issue/status.go index 0df08eafd1e58..b01ce4bbb86ed 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -11,11 +11,11 @@ import ( // ChangeStatus changes issue status to open or closed. func ChangeStatus(issue *models.Issue, doer *models.User, isClosed bool) (err error) { - err = issue.ChangeStatus(doer, isClosed) + comment, err := issue.ChangeStatus(doer, isClosed) if err != nil { return } - notification.NotifyIssueChangeStatus(doer, issue, isClosed) + notification.NotifyIssueChangeStatus(doer, issue, comment, isClosed) return nil } diff --git a/services/pull/merge.go b/services/pull/merge.go index e5563a89b9e98..32dadf1e45d11 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -422,7 +422,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor pr.Merger = doer pr.MergerID = doer.ID - if err = pr.SetMerged(); err != nil { + if err = pr.SetMerged(doer); err != nil { log.Error("setMerged [%d]: %v", pr.ID, err) } From f582fd28e5955b280202081b8432bc3e2c2e3ead Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 5 Dec 2019 21:29:36 +0800 Subject: [PATCH 2/2] Return error if close a closed issue/pull --- models/action.go | 1 + models/error.go | 32 ++++++++++++++++++++++++++++++++ models/issue.go | 9 ++++++++- 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/models/action.go b/models/action.go index e89d22c618470..900cccbf0431e 100644 --- a/models/action.go +++ b/models/action.go @@ -448,6 +448,7 @@ func changeIssueStatus(repo *Repository, issue *Issue, doer *User, status bool) if !status { tp = CommentTypeReopen } + if err := sendCreateCommentAction(x, &CreateCommentOptions{ Type: tp, Doer: doer, diff --git a/models/error.go b/models/error.go index 313c36354df30..bd95b7a0ebce4 100644 --- a/models/error.go +++ b/models/error.go @@ -1121,6 +1121,38 @@ func (err ErrNewIssueInsert) Error() string { return err.OriginalError.Error() } +// ErrIssueWasClosed is used when close a closed issue +type ErrIssueWasClosed struct { + ID int64 + Index int64 +} + +// IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed. +func IsErrIssueWasClosed(err error) bool { + _, ok := err.(ErrIssueWasClosed) + return ok +} + +func (err ErrIssueWasClosed) Error() string { + return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index) +} + +// ErrPullWasClosed is used close a closed pull request +type ErrPullWasClosed struct { + ID int64 + Index int64 +} + +// IsErrPullWasClosed checks if an error is a ErrErrPullWasClosed. +func IsErrPullWasClosed(err error) bool { + _, ok := err.(ErrPullWasClosed) + return ok +} + +func (err ErrPullWasClosed) Error() string { + return fmt.Sprintf("Pull request [%d] %d was already closed", err.ID, err.Index) +} + // __________ .__ .__ __________ __ // \______ \__ __| | | |\______ \ ____ ________ __ ____ _______/ |_ // | ___/ | \ | | | | _// __ \/ ____/ | \_/ __ \ / ___/\ __\ diff --git a/models/issue.go b/models/issue.go index 48d430df917c6..e97c61d8e56ae 100644 --- a/models/issue.go +++ b/models/issue.go @@ -609,7 +609,14 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (*C // Nothing should be performed if current status is same as target status if currentIssue.IsClosed == isClosed { - return nil, nil + if isClosed { + return nil, ErrIssueWasClosed{ + ID: issue.ID, + } + } + return nil, ErrPullWasClosed{ + ID: issue.ID, + } } // Check for open dependencies