Skip to content

Commit f5be627

Browse files
lunnylunny
committed
Fix bug partially failure when commenting with status change in an issue
1 parent 463016b commit f5be627

File tree

10 files changed

+128
-73
lines changed

10 files changed

+128
-73
lines changed

‎options/locale/locale_en-US.ini‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1629,6 +1629,9 @@ issues.reopen_issue = Reopen
16291629
issues.reopen_comment_issue = Reopen with Comment
16301630
issues.create_comment = Comment
16311631
issues.comment.blocked_user = Cannot create or edit comment because you are blocked by the poster or repository owner.
1632+
issues.not_closed = The issue is not closed.
1633+
issues.comment.empty_content = The comment content cannot be empty.
1634+
issues.already_closed = The issue is already closed.
16321635
issues.closed_at = `closed this issue <a id="%[1]s" href="#%[1]s">%[2]s</a>`
16331636
issues.reopened_at = `reopened this issue <a id="%[1]s" href="#%[1]s">%[2]s</a>`
16341637
issues.commit_ref_at = `referenced this issue from a commit <a id="%[1]s" href="#%[1]s">%[2]s</a>`

‎routers/api/v1/repo/issue.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,7 @@ func CreateIssue(ctx *context.APIContext) {
733733
}
734734

735735
if form.Closed {
736-
if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
736+
if _, err := issue_service.CloseIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
737737
if issues_model.IsErrDependenciesLeft(err) {
738738
ctx.APIError(http.StatusPreconditionFailed, "cannot close this issue because it still has open dependencies")
739739
return
@@ -1056,7 +1056,7 @@ func closeOrReopenIssue(ctx *context.APIContext, issue *issues_model.Issue, stat
10561056
}
10571057

10581058
if state == api.StateClosed && !issue.IsClosed {
1059-
if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
1059+
if _, err := issue_service.CloseIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
10601060
if issues_model.IsErrDependenciesLeft(err) {
10611061
ctx.APIError(http.StatusPreconditionFailed, "cannot close this issue or pull request because it still has open dependencies")
10621062
return
@@ -1065,7 +1065,7 @@ func closeOrReopenIssue(ctx *context.APIContext, issue *issues_model.Issue, stat
10651065
return
10661066
}
10671067
} else if state == api.StateOpen && issue.IsClosed {
1068-
if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
1068+
if _, err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
10691069
ctx.APIErrorInternal(err)
10701070
return
10711071
}

‎routers/web/repo/issue_comment.go‎

Lines changed: 34 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ func NewComment(ctx *context.Context) {
7474
return
7575
}
7676

77-
var comment *issues_model.Comment
7877
defer func() {
7978
// Check if issue admin/poster changes the status of issue.
8079
if (ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) || (ctx.IsSigned && issue.IsPoster(ctx.Doer.ID))) &&
@@ -155,51 +154,37 @@ func NewComment(ctx *context.Context) {
155154

156155
if pr != nil {
157156
ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
158-
} else {
159-
if form.Status == "close" && !issue.IsClosed {
160-
if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
161-
log.Error("CloseIssue: %v", err)
162-
if issues_model.IsErrDependenciesLeft(err) {
163-
if issue.IsPull {
164-
ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
165-
} else {
166-
ctx.JSONError(ctx.Tr("repo.issues.dependency.issue_close_blocked"))
167-
}
168-
return
169-
}
170-
} else {
171-
if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil {
172-
ctx.ServerError("stopTimerIfAvailable", err)
173-
return
174-
}
175-
log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
176-
}
177-
} else if form.Status == "reopen" && issue.IsClosed {
178-
if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
179-
log.Error("ReopenIssue: %v", err)
180-
}
181-
}
182157
}
183158
}
159+
}()
184160

185-
// Redirect to comment hashtag if there is any actual content.
186-
typeName := "issues"
187-
if issue.IsPull {
188-
typeName = "pulls"
161+
var createdComment *issues_model.Comment
162+
var err error
163+
164+
switch form.Status {
165+
case "reopen":
166+
if !issue.IsClosed {
167+
ctx.JSONError(ctx.Tr("repo.issues.not_closed"))
168+
return
189169
}
190-
if comment != nil {
191-
ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d#%s", ctx.Repo.RepoLink, typeName, issue.Index, comment.HashTag()))
192-
} else {
193-
ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, typeName, issue.Index))
170+
171+
createdComment, err = issue_service.ReopenIssue(ctx, issue, ctx.Doer, "", form.Content, attachments)
172+
case "close":
173+
if issue.IsClosed {
174+
ctx.JSONError(ctx.Tr("repo.issues.already_closed"))
175+
return
194176
}
195-
}()
196177

197-
// Fix #321: Allow empty comments, as long as we have attachments.
198-
if len(form.Content) == 0 && len(attachments) == 0 {
199-
return
178+
createdComment, err = issue_service.CloseIssue(ctx, issue, ctx.Doer, "", form.Content, attachments)
179+
default:
180+
if len(form.Content) == 0 && len(attachments) == 0 {
181+
ctx.JSONError(ctx.Tr("repo.issues.comment.empty_content"))
182+
return
183+
}
184+
185+
createdComment, err = issue_service.CreateIssueComment(ctx, ctx.Doer, ctx.Repo.Repository, issue, form.Content, attachments)
200186
}
201187

202-
comment, err := issue_service.CreateIssueComment(ctx, ctx.Doer, ctx.Repo.Repository, issue, form.Content, attachments)
203188
if err != nil {
204189
if errors.Is(err, user_model.ErrBlockedUser) {
205190
ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user"))
@@ -209,7 +194,17 @@ func NewComment(ctx *context.Context) {
209194
return
210195
}
211196

212-
log.Trace("Comment created: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, comment.ID)
197+
// Redirect to comment hashtag if there is any actual content.
198+
typeName := "issues"
199+
if issue.IsPull {
200+
typeName = "pulls"
201+
}
202+
if createdComment != nil {
203+
log.Trace("Comment created: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, createdComment.ID)
204+
ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d#%s", ctx.Repo.RepoLink, typeName, issue.Index, createdComment.HashTag()))
205+
} else {
206+
ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, typeName, issue.Index))
207+
}
213208
}
214209

215210
// UpdateCommentContent change comment of issue's content

‎routers/web/repo/issue_list.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,7 @@ func UpdateIssueStatus(ctx *context.Context) {
434434
continue
435435
}
436436
if action == "close" && !issue.IsClosed {
437-
if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
437+
if _, err := issue_service.CloseIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
438438
if issues_model.IsErrDependenciesLeft(err) {
439439
ctx.JSON(http.StatusPreconditionFailed, map[string]any{
440440
"error": ctx.Tr("repo.issues.dependency.issue_batch_close_blocked", issue.Index),
@@ -445,7 +445,7 @@ func UpdateIssueStatus(ctx *context.Context) {
445445
return
446446
}
447447
} else if action == "open" && issue.IsClosed {
448-
if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
448+
if _, err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
449449
ctx.ServerError("ReopenIssue", err)
450450
return
451451
}

‎services/issue/comments.go‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,22 @@ func CreateRefComment(ctx context.Context, doer *user_model.User, repo *repo_mod
5555
return err
5656
}
5757

58+
func notifyCommentCreated(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, comment *issues_model.Comment) error {
59+
mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, doer, comment.Content)
60+
if err != nil {
61+
return err
62+
}
63+
64+
// reload issue to ensure it has the latest data, especially the number of comments
65+
issue, err = issues_model.GetIssueByID(ctx, issue.ID)
66+
if err != nil {
67+
return err
68+
}
69+
70+
notify_service.CreateIssueComment(ctx, doer, repo, issue, comment, mentions)
71+
return nil
72+
}
73+
5874
// CreateIssueComment creates a plain issue comment.
5975
func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content string, attachments []string) (*issues_model.Comment, error) {
6076
if user_model.IsUserBlockedBy(ctx, doer, issue.PosterID, repo.OwnerID) {
@@ -75,19 +91,16 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m
7591
return nil, err
7692
}
7793

78-
mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, doer, comment.Content)
79-
if err != nil {
80-
return nil, err
81-
}
94+
return comment, notifyCommentCreated(ctx, doer, repo, issue, comment)
95+
}
8296

83-
// reload issue to ensure it has the latest data, especially the number of comments
84-
issue, err = issues_model.GetIssueByID(ctx, issue.ID)
97+
// CreateCommentAndChangeStatus creates a comment and changes the issue status.
98+
func CreateCommentAndChangeStatus(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content string, attachments []string) (*issues_model.Comment, error) {
99+
comment, err := CreateIssueComment(ctx, doer, repo, issue, content, attachments)
85100
if err != nil {
86101
return nil, err
87102
}
88103

89-
notify_service.CreateIssueComment(ctx, doer, repo, issue, comment, mentions)
90-
91104
return comment, nil
92105
}
93106

‎services/issue/commit.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -196,11 +196,11 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m
196196
return err
197197
}
198198
}
199-
if err := CloseIssue(ctx, refIssue, doer, c.Sha1); err != nil {
199+
if _, err := CloseIssue(ctx, refIssue, doer, c.Sha1, "", nil); err != nil {
200200
return err
201201
}
202202
} else if ref.Action == references.XRefActionReopens && refIssue.IsClosed {
203-
if err := ReopenIssue(ctx, refIssue, doer, c.Sha1); err != nil {
203+
if _, err := ReopenIssue(ctx, refIssue, doer, c.Sha1, "", nil); err != nil {
204204
return err
205205
}
206206
}

‎services/issue/status.go‎

Lines changed: 56 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,25 @@ import (
1414
)
1515

1616
// CloseIssue close an issue.
17-
func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error {
18-
var comment *issues_model.Comment
17+
func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID, commentContent string, attachments []string) (*issues_model.Comment, error) {
18+
var refComment, createdComment *issues_model.Comment
1919
if err := db.WithTx(ctx, func(ctx context.Context) error {
2020
var err error
21-
comment, err = issues_model.CloseIssue(ctx, issue, doer)
21+
if commentContent != "" || len(attachments) > 0 {
22+
createdComment, err = issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
23+
Type: issues_model.CommentTypeComment,
24+
Doer: doer,
25+
Repo: issue.Repo,
26+
Issue: issue,
27+
Content: commentContent,
28+
Attachments: attachments,
29+
})
30+
if err != nil {
31+
return err
32+
}
33+
}
34+
35+
refComment, err = issues_model.CloseIssue(ctx, issue, doer)
2236
if err != nil {
2337
if issues_model.IsErrDependenciesLeft(err) {
2438
if _, err := issues_model.FinishIssueStopwatch(ctx, doer, issue); err != nil {
@@ -31,23 +45,53 @@ func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model
3145
_, err = issues_model.FinishIssueStopwatch(ctx, doer, issue)
3246
return err
3347
}); err != nil {
34-
return err
48+
return nil, err
3549
}
3650

37-
notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, true)
51+
if createdComment != nil {
52+
if err := notifyCommentCreated(ctx, doer, issue.Repo, issue, createdComment); err != nil {
53+
log.Error("Unable to notify comment created for issue[%d]#%d: %v", issue.ID, issue.Index, err)
54+
}
55+
}
3856

39-
return nil
57+
notify_service.IssueChangeStatus(ctx, doer, commitID, issue, refComment, true)
58+
59+
return createdComment, nil
4060
}
4161

42-
// ReopenIssue reopen an issue.
62+
// ReopenIssue reopen an issue with or without a comment.
4363
// FIXME: If some issues dependent this one are closed, should we also reopen them?
44-
func ReopenIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error {
45-
comment, err := issues_model.ReopenIssue(ctx, issue, doer)
64+
func ReopenIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID, commentContent string, attachments []string) (*issues_model.Comment, error) {
65+
var createdComment *issues_model.Comment
66+
refComment, err := db.WithTx2(ctx, func(ctx context.Context) (*issues_model.Comment, error) {
67+
var err error
68+
if commentContent != "" || len(attachments) > 0 {
69+
createdComment, err = issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{
70+
Type: issues_model.CommentTypeComment,
71+
Doer: doer,
72+
Repo: issue.Repo,
73+
Issue: issue,
74+
Content: commentContent,
75+
Attachments: attachments,
76+
})
77+
if err != nil {
78+
return nil, err
79+
}
80+
}
81+
82+
return issues_model.ReopenIssue(ctx, issue, doer)
83+
})
4684
if err != nil {
47-
return err
85+
return nil, err
86+
}
87+
88+
if createdComment != nil {
89+
if err := notifyCommentCreated(ctx, doer, issue.Repo, issue, createdComment); err != nil {
90+
log.Error("Unable to notify comment created for issue[%d]#%d: %v", issue.ID, issue.Index, err)
91+
}
4892
}
4993

50-
notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, false)
94+
notify_service.IssueChangeStatus(ctx, doer, commitID, issue, refComment, false)
5195

52-
return nil
96+
return createdComment, nil
5397
}

‎services/pull/merge.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,14 +309,14 @@ func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullReques
309309
return err
310310
}
311311
if ref.RefAction == references.XRefActionCloses && !ref.Issue.IsClosed {
312-
if err = issue_service.CloseIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil {
312+
if _, err = issue_service.CloseIssue(ctx, ref.Issue, doer, pr.MergedCommitID, "", nil); err != nil {
313313
// Allow ErrDependenciesLeft
314314
if !issues_model.IsErrDependenciesLeft(err) {
315315
return err
316316
}
317317
}
318318
} else if ref.RefAction == references.XRefActionReopens && ref.Issue.IsClosed {
319-
if err = issue_service.ReopenIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil {
319+
if _, err = issue_service.ReopenIssue(ctx, ref.Issue, doer, pr.MergedCommitID, "", nil); err != nil {
320320
return err
321321
}
322322
}

‎services/pull/pull.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -700,7 +700,7 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User
700700

701701
var errs []error
702702
for _, pr := range prs {
703-
if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) && !issues_model.IsErrDependenciesLeft(err) {
703+
if _, err = issue_service.CloseIssue(ctx, pr.Issue, doer, "", "", nil); err != nil && !issues_model.IsErrIssueIsClosed(err) && !issues_model.IsErrDependenciesLeft(err) {
704704
errs = append(errs, err)
705705
}
706706
if err == nil {
@@ -740,7 +740,7 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User
740740
errs = append(errs, err)
741741
}
742742
if err == nil {
743-
if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) && !issues_model.IsErrDependenciesLeft(err) {
743+
if _, err = issue_service.CloseIssue(ctx, pr.Issue, doer, "", "", nil); err != nil && !issues_model.IsErrIssueIsClosed(err) && !issues_model.IsErrDependenciesLeft(err) {
744744
errs = append(errs, err)
745745
}
746746
}
@@ -772,7 +772,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
772772
if pr.BaseRepoID == repo.ID {
773773
continue
774774
}
775-
if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) {
775+
if _, err = issue_service.CloseIssue(ctx, pr.Issue, doer, "", "", nil); err != nil && !issues_model.IsErrIssueIsClosed(err) {
776776
errs = append(errs, err)
777777
}
778778
}

‎tests/integration/actions_trigger_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -565,12 +565,12 @@ jobs:
565565
checkCommitStatusAndInsertFakeStatus(t, repo, sha)
566566

567567
// closed
568-
err = issue_service.CloseIssue(db.DefaultContext, pullIssue, user2, "")
568+
_, err = issue_service.CloseIssue(db.DefaultContext, pullIssue, user2, "", "", nil)
569569
assert.NoError(t, err)
570570
checkCommitStatusAndInsertFakeStatus(t, repo, sha)
571571

572572
// reopened
573-
err = issue_service.ReopenIssue(db.DefaultContext, pullIssue, user2, "")
573+
_, err = issue_service.ReopenIssue(db.DefaultContext, pullIssue, user2, "", "", nil)
574574
assert.NoError(t, err)
575575
checkCommitStatusAndInsertFakeStatus(t, repo, sha)
576576

0 commit comments

Comments
 (0)