Refactor comment (#9330)

* Refactor comment

* fix test

* improve code

6 files changed, 66 insertions, 34 deletions

diff --git a/models/error.go b/models/error.go
index 16be512139..a17ff5f9d0 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)
+}
+
 // ErrForbiddenIssueReaction is used when a forbidden reaction was try to created
 type ErrForbiddenIssueReaction struct {
 	Reaction string
diff --git a/models/issue.go b/models/issue.go
index 0a08a97fdd..48974b279e 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -600,16 +600,23 @@ 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 !issue.IsPull {
+			return nil, ErrIssueWasClosed{
+				ID: issue.ID,
+			}
+		}
+		return nil, ErrPullWasClosed{
+			ID: issue.ID,
+		}
 	}
 
 	// Check for open dependencies
@@ -617,11 +624,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 +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.
diff --git a/models/issue_comment.go b/models/issue_comment.go
index c68df028e0..9ca7aaeb01 100644
--- a/models/issue_comment.go
+++ b/models/issue_comment.go
@@ -750,10 +750,6 @@ func CreateComment(opts *CreateCommentOptions) (comment *Comment, err error) {
 		return nil, err
 	}
 
-	if err = sendCreateCommentAction(sess, opts, comment); err != nil {
-		return nil, err
-	}
-
 	if err = sess.Commit(); err != nil {
 		return nil, err
 	}
diff --git a/models/issue_dependency_test.go b/models/issue_dependency_test.go
index ede9e008eb..bf323abb98 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 d8defd99c6..936d1124be 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 3af51112d4..d703c9a2ee 100644
--- a/models/pull.go
+++ b/models/pull.go
@@ -460,7 +460,7 @@ func (pr *PullRequest) SetMerged() (err error) {
 		return err
 	}
 
-	if err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
+	if _, err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
 		return fmt.Errorf("Issue.changeStatus: %v", err)
 	}
 	if _, err = sess.ID(pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {