Skip to content

Commit 67b316a

Browse files
lunnytechknowlogick
lunny
authored and
techknowlogick
committed
Refactor comment (#9330)
* Refactor comment * fix test * improve code
1 parent c6b3c5b commit 67b316a

File tree

16 files changed

+144
-45
lines changed

16 files changed

+144
-45
lines changed

‎models/error.go

+32
Original file line numberDiff line numberDiff line change
@@ -1121,6 +1121,38 @@ func (err ErrNewIssueInsert) Error() string {
11211121
return err.OriginalError.Error()
11221122
}
11231123

1124+
// ErrIssueWasClosed is used when close a closed issue
1125+
type ErrIssueWasClosed struct {
1126+
ID int64
1127+
Index int64
1128+
}
1129+
1130+
// IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed.
1131+
func IsErrIssueWasClosed(err error) bool {
1132+
_, ok := err.(ErrIssueWasClosed)
1133+
return ok
1134+
}
1135+
1136+
func (err ErrIssueWasClosed) Error() string {
1137+
return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index)
1138+
}
1139+
1140+
// ErrPullWasClosed is used close a closed pull request
1141+
type ErrPullWasClosed struct {
1142+
ID int64
1143+
Index int64
1144+
}
1145+
1146+
// IsErrPullWasClosed checks if an error is a ErrErrPullWasClosed.
1147+
func IsErrPullWasClosed(err error) bool {
1148+
_, ok := err.(ErrPullWasClosed)
1149+
return ok
1150+
}
1151+
1152+
func (err ErrPullWasClosed) Error() string {
1153+
return fmt.Sprintf("Pull request [%d] %d was already closed", err.ID, err.Index)
1154+
}
1155+
11241156
// ErrForbiddenIssueReaction is used when a forbidden reaction was try to created
11251157
type ErrForbiddenIssueReaction struct {
11261158
Reaction string

‎models/issue.go

+30-27
Original file line numberDiff line numberDiff line change
@@ -600,28 +600,35 @@ func updateIssueCols(e Engine, issue *Issue, cols ...string) error {
600600
return nil
601601
}
602602

603-
func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (err error) {
603+
func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (*Comment, error) {
604604
// Reload the issue
605605
currentIssue, err := getIssueByID(e, issue.ID)
606606
if err != nil {
607-
return err
607+
return nil, err
608608
}
609609

610610
// Nothing should be performed if current status is same as target status
611611
if currentIssue.IsClosed == isClosed {
612-
return nil
612+
if !issue.IsPull {
613+
return nil, ErrIssueWasClosed{
614+
ID: issue.ID,
615+
}
616+
}
617+
return nil, ErrPullWasClosed{
618+
ID: issue.ID,
619+
}
613620
}
614621

615622
// Check for open dependencies
616623
if isClosed && issue.Repo.isDependenciesEnabled(e) {
617624
// 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
618625
noDeps, err := issueNoDependenciesLeft(e, issue)
619626
if err != nil {
620-
return err
627+
return nil, err
621628
}
622629

623630
if !noDeps {
624-
return ErrDependenciesLeft{issue.ID}
631+
return nil, ErrDependenciesLeft{issue.ID}
625632
}
626633
}
627634

@@ -633,22 +640,22 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er
633640
}
634641

635642
if err = updateIssueCols(e, issue, "is_closed", "closed_unix"); err != nil {
636-
return err
643+
return nil, err
637644
}
638645

639646
// Update issue count of labels
640647
if err = issue.getLabels(e); err != nil {
641-
return err
648+
return nil, err
642649
}
643650
for idx := range issue.Labels {
644651
if err = updateLabel(e, issue.Labels[idx]); err != nil {
645-
return err
652+
return nil, err
646653
}
647654
}
648655

649656
// Update issue count of milestone
650657
if err := updateMilestoneClosedNum(e, issue.MilestoneID); err != nil {
651-
return err
658+
return nil, err
652659
}
653660

654661
// New action comment
@@ -657,43 +664,39 @@ func (issue *Issue) changeStatus(e *xorm.Session, doer *User, isClosed bool) (er
657664
cmtType = CommentTypeReopen
658665
}
659666

660-
var opts = &CreateCommentOptions{
667+
return createCommentWithNoAction(e, &CreateCommentOptions{
661668
Type: cmtType,
662669
Doer: doer,
663670
Repo: issue.Repo,
664671
Issue: issue,
665-
}
666-
comment, err := createCommentWithNoAction(e, opts)
667-
if err != nil {
668-
return err
669-
}
670-
return sendCreateCommentAction(e, opts, comment)
672+
})
671673
}
672674

673675
// ChangeStatus changes issue status to open or closed.
674-
func (issue *Issue) ChangeStatus(doer *User, isClosed bool) (err error) {
676+
func (issue *Issue) ChangeStatus(doer *User, isClosed bool) (*Comment, error) {
675677
sess := x.NewSession()
676678
defer sess.Close()
677-
if err = sess.Begin(); err != nil {
678-
return err
679+
if err := sess.Begin(); err != nil {
680+
return nil, err
679681
}
680682

681-
if err = issue.loadRepo(sess); err != nil {
682-
return err
683+
if err := issue.loadRepo(sess); err != nil {
684+
return nil, err
683685
}
684-
if err = issue.loadPoster(sess); err != nil {
685-
return err
686+
if err := issue.loadPoster(sess); err != nil {
687+
return nil, err
686688
}
687689

688-
if err = issue.changeStatus(sess, doer, isClosed); err != nil {
689-
return err
690+
comment, err := issue.changeStatus(sess, doer, isClosed)
691+
if err != nil {
692+
return nil, err
690693
}
691694

692695
if err = sess.Commit(); err != nil {
693-
return fmt.Errorf("Commit: %v", err)
696+
return nil, fmt.Errorf("Commit: %v", err)
694697
}
695698

696-
return nil
699+
return comment, nil
697700
}
698701

699702
// ChangeTitle changes the title of this issue, as the given user.

‎models/issue_comment.go

-4
Original file line numberDiff line numberDiff line change
@@ -750,10 +750,6 @@ func CreateComment(opts *CreateCommentOptions) (comment *Comment, err error) {
750750
return nil, err
751751
}
752752

753-
if err = sendCreateCommentAction(sess, opts, comment); err != nil {
754-
return nil, err
755-
}
756-
757753
if err = sess.Commit(); err != nil {
758754
return nil, err
759755
}

‎models/issue_dependency_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func TestCreateIssueDependency(t *testing.T) {
4545
assert.False(t, left)
4646

4747
// Close #2 and check again
48-
err = issue2.ChangeStatus(user1, true)
48+
_, err = issue2.ChangeStatus(user1, true)
4949
assert.NoError(t, err)
5050

5151
left, err = IssueNoDependenciesLeft(issue1)

‎models/issue_xref_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ func TestXRef_ResolveCrossReferences(t *testing.T) {
9494
i1 := testCreateIssue(t, 1, 2, "title1", "content1", false)
9595
i2 := testCreateIssue(t, 1, 2, "title2", "content2", false)
9696
i3 := testCreateIssue(t, 1, 2, "title3", "content3", false)
97-
assert.NoError(t, i3.ChangeStatus(d, true))
97+
_, err := i3.ChangeStatus(d, true)
98+
assert.NoError(t, err)
9899

99100
pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index))
100101
rp := AssertExistsAndLoadBean(t, &Comment{IssueID: i1.ID, RefIssueID: pr.Issue.ID, RefCommentID: 0}).(*Comment)

‎models/pull.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,7 @@ func (pr *PullRequest) SetMerged() (err error) {
460460
return err
461461
}
462462

463-
if err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
463+
if _, err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
464464
return fmt.Errorf("Issue.changeStatus: %v", err)
465465
}
466466
if _, err = sess.ID(pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {

‎modules/notification/action/action.go

+54
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,60 @@ func (a *actionNotifier) NotifyNewIssue(issue *models.Issue) {
5353
}
5454
}
5555

56+
// NotifyIssueChangeStatus notifies close or reopen issue to notifiers
57+
func (a *actionNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, closeOrReopen bool) {
58+
// Compose comment action, could be plain comment, close or reopen issue/pull request.
59+
// This object will be used to notify watchers in the end of function.
60+
act := &models.Action{
61+
ActUserID: doer.ID,
62+
ActUser: doer,
63+
Content: fmt.Sprintf("%d|%s", issue.Index, ""),
64+
RepoID: issue.Repo.ID,
65+
Repo: issue.Repo,
66+
Comment: actionComment,
67+
CommentID: actionComment.ID,
68+
IsPrivate: issue.Repo.IsPrivate,
69+
}
70+
// Check comment type.
71+
if closeOrReopen {
72+
act.OpType = models.ActionCloseIssue
73+
if issue.IsPull {
74+
act.OpType = models.ActionClosePullRequest
75+
}
76+
} else {
77+
act.OpType = models.ActionReopenIssue
78+
if issue.IsPull {
79+
act.OpType = models.ActionReopenPullRequest
80+
}
81+
}
82+
83+
// Notify watchers for whatever action comes in, ignore if no action type.
84+
if err := models.NotifyWatchers(act); err != nil {
85+
log.Error("NotifyWatchers: %v", err)
86+
}
87+
}
88+
89+
// NotifyCreateIssueComment notifies comment on an issue to notifiers
90+
func (a *actionNotifier) NotifyCreateIssueComment(doer *models.User, repo *models.Repository,
91+
issue *models.Issue, comment *models.Comment) {
92+
act := &models.Action{
93+
OpType: models.ActionCommentIssue,
94+
ActUserID: doer.ID,
95+
ActUser: doer,
96+
Content: fmt.Sprintf("%d|%s", issue.Index, comment.Content),
97+
RepoID: issue.Repo.ID,
98+
Repo: issue.Repo,
99+
Comment: comment,
100+
CommentID: comment.ID,
101+
IsPrivate: issue.Repo.IsPrivate,
102+
}
103+
104+
// Notify watchers for whatever action comes in, ignore if no action type.
105+
if err := models.NotifyWatchers(act); err != nil {
106+
log.Error("NotifyWatchers: %v", err)
107+
}
108+
}
109+
56110
func (a *actionNotifier) NotifyNewPullRequest(pull *models.PullRequest) {
57111
if err := pull.LoadIssue(); err != nil {
58112
log.Error("pull.LoadIssue: %v", err)

‎modules/notification/base/notifier.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ type Notifier interface {
2121
NotifyTransferRepository(doer *models.User, repo *models.Repository, oldOwnerName string)
2222

2323
NotifyNewIssue(*models.Issue)
24-
NotifyIssueChangeStatus(*models.User, *models.Issue, bool)
24+
NotifyIssueChangeStatus(*models.User, *models.Issue, *models.Comment, bool)
2525
NotifyIssueChangeMilestone(doer *models.User, issue *models.Issue, oldMilestoneID int64)
2626
NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment)
2727
NotifyIssueChangeContent(doer *models.User, issue *models.Issue, oldContent string)

‎modules/notification/base/null.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func (*NullNotifier) NotifyNewIssue(issue *models.Issue) {
3131
}
3232

3333
// NotifyIssueChangeStatus places a place holder function
34-
func (*NullNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) {
34+
func (*NullNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
3535
}
3636

3737
// NotifyNewPullRequest places a place holder function

‎modules/notification/mail/mail.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func (m *mailNotifier) NotifyNewIssue(issue *models.Issue) {
5151
}
5252
}
5353

54-
func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) {
54+
func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
5555
var actionType models.ActionType
5656
if issue.IsPull {
5757
if isClosed {

‎modules/notification/notification.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,9 @@ func NotifyNewIssue(issue *models.Issue) {
5353
}
5454

5555
// NotifyIssueChangeStatus notifies close or reopen issue to notifiers
56-
func NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, closeOrReopen bool) {
56+
func NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, closeOrReopen bool) {
5757
for _, notifier := range notifiers {
58-
notifier.NotifyIssueChangeStatus(doer, issue, closeOrReopen)
58+
notifier.NotifyIssueChangeStatus(doer, issue, actionComment, closeOrReopen)
5959
}
6060
}
6161

‎modules/notification/ui/ui.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func (ns *notificationService) NotifyNewIssue(issue *models.Issue) {
6262
}
6363
}
6464

65-
func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) {
65+
func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
6666
ns.issueQueue <- issueNotificationOpts{
6767
issueID: issue.ID,
6868
notificationAuthorID: doer.ID,

‎modules/notification/webhook/webhook.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *model
211211
}
212212
}
213213

214-
func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) {
214+
func (m *webhookNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, actionComment *models.Comment, isClosed bool) {
215215
mode, _ := models.AccessLevel(issue.Poster, issue.Repo)
216216
var err error
217217
if issue.IsPull {

‎modules/repofiles/action.go

+5-2
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func getIssueFromRef(repo *models.Repository, index int64) (*models.Issue, error
3131
return issue, nil
3232
}
3333

34-
func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *models.User, status bool) error {
34+
func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *models.User, closed bool) error {
3535
stopTimerIfAvailable := func(doer *models.User, issue *models.Issue) error {
3636

3737
if models.StopwatchExists(doer.ID, issue.ID) {
@@ -44,14 +44,17 @@ func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *model
4444
}
4545

4646
issue.Repo = repo
47-
if err := issue.ChangeStatus(doer, status); err != nil {
47+
comment, err := issue.ChangeStatus(doer, closed)
48+
if err != nil {
4849
// Don't return an error when dependencies are open as this would let the push fail
4950
if models.IsErrDependenciesLeft(err) {
5051
return stopTimerIfAvailable(doer, issue)
5152
}
5253
return err
5354
}
5455

56+
notification.NotifyIssueChangeStatus(doer, issue, comment, closed)
57+
5558
return stopTimerIfAvailable(doer, issue)
5659
}
5760

‎services/issue/status.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,11 @@ import (
1111

1212
// ChangeStatus changes issue status to open or closed.
1313
func ChangeStatus(issue *models.Issue, doer *models.User, isClosed bool) (err error) {
14-
err = issue.ChangeStatus(doer, isClosed)
14+
comment, err := issue.ChangeStatus(doer, isClosed)
1515
if err != nil {
1616
return
1717
}
1818

19-
notification.NotifyIssueChangeStatus(doer, issue, isClosed)
19+
notification.NotifyIssueChangeStatus(doer, issue, comment, isClosed)
2020
return nil
2121
}

‎services/pull/check.go

+10
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"code.gitea.io/gitea/modules/git"
2020
"code.gitea.io/gitea/modules/graceful"
2121
"code.gitea.io/gitea/modules/log"
22+
"code.gitea.io/gitea/modules/notification"
2223
"code.gitea.io/gitea/modules/setting"
2324
"code.gitea.io/gitea/modules/sync"
2425
"code.gitea.io/gitea/modules/timeutil"
@@ -145,6 +146,15 @@ func manuallyMerged(pr *models.PullRequest) bool {
145146
log.Error("PullRequest[%d].setMerged : %v", pr.ID, err)
146147
return false
147148
}
149+
150+
baseGitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
151+
if err != nil {
152+
log.Error("OpenRepository[%s] : %v", pr.BaseRepo.RepoPath(), err)
153+
return false
154+
}
155+
156+
notification.NotifyMergePullRequest(pr, merger, baseGitRepo)
157+
148158
log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String())
149159
return true
150160
}

0 commit comments

Comments
 (0)