Move comment action out of models package

models/action.go

@@ -436,13 +436,27 @@ 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)
 }

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)
+}
+
 // __________      .__  .__ __________                                     __
 // \______   \__ __|  | |  |\______   \ ____  ________ __   ____   _______/  |_
 //  |     ___/  |  \  | |  | |       _// __ \/ ____/  |  \_/ __ \ /  ___/\   __\

models/issue.go

@@ -600,28 +600,35 @@ 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
+		if isClosed {
+			return nil, ErrIssueWasClosed{
+				ID: issue.ID,
+			}
+		}
+		return nil, ErrPullWasClosed{
+			ID: issue.ID,
+		}
 	}
 
 	// Check for open dependencies
 	if isClosed && issue.Repo.isDependenciesEnabled(e) {
 		// 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 +640,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 +664,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.

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{

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)

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)

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
 		}

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)

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)

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

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 {

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)
 	}
 }
 

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,

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 {

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
 }

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)
 	}