Refactor comment (#9330)

* Refactor comment

* fix test

* improve code

8 files changed, 66 insertions, 9 deletions

diff --git a/modules/notification/action/action.go b/modules/notification/action/action.go
index 9caeb5aac0..7441de638a 100644
--- a/modules/notification/action/action.go
+++ b/modules/notification/action/action.go
@@ -53,6 +53,60 @@ 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)
+	}
+}
+
+// NotifyCreateIssueComment notifies comment on an issue to notifiers
+func (a *actionNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
+	issue *models.Issue, comment *models.Comment) {
+	act := &models.Action{
+		OpType:    models.ActionCommentIssue,
+		ActUserID: doer.ID,
+		ActUser:   doer,
+		Content:   fmt.Sprintf("%d|%s", issue.Index, comment.Content),
+		RepoID:    issue.Repo.ID,
+		Repo:      issue.Repo,
+		Comment:   comment,
+		CommentID: comment.ID,
+		IsPrivate: issue.Repo.IsPrivate,
+	}
+
+	// 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 c8c89d22bd..934ee80aa7 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 79e9f7f7fc..a04d0e8caa 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, actionComment *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 558c9a6243..38146cef2e 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, actionComment *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 71d6e79e6d..ab671fa291 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, actionComment *models.Comment, closeOrReopen bool) {
 	for _, notifier := range notifiers {
-		notifier.NotifyIssueChangeStatus(doer, issue, closeOrReopen)
+		notifier.NotifyIssueChangeStatus(doer, issue, actionComment, closeOrReopen)
 	}
 }
 
diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go
index bfe497f866..f58ebce6d7 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, actionComment *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 4ef60fef84..ee91a29f02 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, actionComment *models.Comment, isClosed bool) {
 	mode, _ := models.AccessLevel(issue.Poster, issue.Repo)
 	var err error
 	if issue.IsPull {
diff --git a/modules/repofiles/action.go b/modules/repofiles/action.go
index 517c145659..a5a5e151cb 100644
--- a/modules/repofiles/action.go
+++ b/modules/repofiles/action.go
@@ -31,7 +31,7 @@ func getIssueFromRef(repo *models.Repository, index int64) (*models.Issue, error
 	return issue, nil
 }
 
-func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *models.User, status bool) error {
+func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *models.User, closed bool) error {
 	stopTimerIfAvailable := func(doer *models.User, issue *models.Issue) error {
 
 		if models.StopwatchExists(doer.ID, issue.ID) {
@@ -44,7 +44,8 @@ func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *model
 	}
 
 	issue.Repo = repo
-	if err := issue.ChangeStatus(doer, status); err != nil {
+	comment, err := issue.ChangeStatus(doer, closed)
+	if err != nil {
 		// Don't return an error when dependencies are open as this would let the push fail
 		if models.IsErrDependenciesLeft(err) {
 			return stopTimerIfAvailable(doer, issue)
@@ -52,6 +53,8 @@ func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *model
 		return err
 	}
 
+	notification.NotifyIssueChangeStatus(doer, issue, comment, closed)
+
 	return stopTimerIfAvailable(doer, issue)
 }