Fix issue where commenting with a status change could partially fail.

options/locale/locale_en-US.ini

@@ -1629,6 +1629,11 @@ issues.reopen_issue = Reopen
 issues.reopen_comment_issue = Reopen with Comment
 issues.create_comment = Comment
 issues.comment.blocked_user = Cannot create or edit comment because you are blocked by the poster or repository owner.
+issues.not_closed = The issue is not closed.
+issues.reopen_not_allowed = No permission to reopen this issue.
+issues.reopen_not_allowed_merged = A pull request cannot be reopened after it has been merged.
+issues.comment.empty_content = The comment content cannot be empty.
+issues.already_closed = The issue is already closed.
 issues.closed_at = `closed this issue <a id="%[1]s" href="#%[1]s">%[2]s</a>`
 issues.reopened_at = `reopened this issue <a id="%[1]s" href="#%[1]s">%[2]s</a>`
 issues.commit_ref_at = `referenced this issue from a commit <a id="%[1]s" href="#%[1]s">%[2]s</a>`
@@ -1949,7 +1954,7 @@ pulls.has_merged = Failed: The pull request has been merged. You cannot merge ag
 pulls.push_rejected = Push Failed: The push was rejected. Review the Git Hooks for this repository.
 pulls.push_rejected_summary = Full Rejection Message
 pulls.push_rejected_no_message = Push Failed: The push was rejected but there was no remote message. Review the Git Hooks for this repository.
-pulls.open_unmerged_pull_exists = `You cannot perform a reopen operation because there is a pending pull request (#%d) with identical properties.`
+pulls.open_unmerged_pull_exists = You cannot perform a reopen operation because there is a pending pull request (#%d) with identical properties.
 pulls.status_checking = Some checks are pending
 pulls.status_checks_success = All checks were successful
 pulls.status_checks_warning = Some checks reported warnings

routers/api/v1/repo/issue.go

@@ -733,7 +733,7 @@ func CreateIssue(ctx *context.APIContext) {
 	}
 
 	if form.Closed {
-		if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
+		if _, err := issue_service.CloseIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
 			if issues_model.IsErrDependenciesLeft(err) {
 				ctx.APIError(http.StatusPreconditionFailed, "cannot close this issue because it still has open dependencies")
 				return
@@ -1056,7 +1056,7 @@ func closeOrReopenIssue(ctx *context.APIContext, issue *issues_model.Issue, stat
 	}
 
 	if state == api.StateClosed && !issue.IsClosed {
-		if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
+		if _, err := issue_service.CloseIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
 			if issues_model.IsErrDependenciesLeft(err) {
 				ctx.APIError(http.StatusPreconditionFailed, "cannot close this issue or pull request because it still has open dependencies")
 				return
@@ -1065,7 +1065,7 @@ func closeOrReopenIssue(ctx *context.APIContext, issue *issues_model.Issue, stat
 			return
 		}
 	} else if state == api.StateOpen && issue.IsClosed {
-		if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
+		if _, err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
 			ctx.APIErrorInternal(err)
 			return
 		}

routers/web/repo/issue_comment.go

@@ -74,132 +74,131 @@ func NewComment(ctx *context.Context) {
 		return
 	}
 
-	var comment *issues_model.Comment
-	defer func() {
-		// Check if issue admin/poster changes the status of issue.
-		if (ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) || (ctx.IsSigned && issue.IsPoster(ctx.Doer.ID))) &&
-			(form.Status == "reopen" || form.Status == "close") &&
-			!(issue.IsPull && issue.PullRequest.HasMerged) {
-			// Duplication and conflict check should apply to reopen pull request.
-			var pr *issues_model.PullRequest
-
-			if form.Status == "reopen" && issue.IsPull {
-				pull := issue.PullRequest
-				var err error
-				pr, err = issues_model.GetUnmergedPullRequest(ctx, pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch, pull.Flow)
-				if err != nil {
-					if !issues_model.IsErrPullRequestNotExist(err) {
-						ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
-						return
-					}
-				}
+	var createdComment *issues_model.Comment
+	var err error
 
-				// Regenerate patch and test conflict.
-				if pr == nil {
-					issue.PullRequest.HeadCommitID = ""
-					pull_service.StartPullRequestCheckImmediately(ctx, issue.PullRequest)
-				}
+	switch form.Status {
+	case "reopen":
+		if !issue.IsClosed {
+			ctx.JSONError(ctx.Tr("repo.issues.not_closed"))
+			return
+		}
 
-				// check whether the ref of PR <refs/pulls/pr_index/head> in base repo is consistent with the head commit of head branch in the head repo
-				// get head commit of PR
-				if pull.Flow == issues_model.PullRequestFlowGithub {
-					prHeadRef := pull.GetGitHeadRefName()
-					if err := pull.LoadBaseRepo(ctx); err != nil {
-						ctx.ServerError("Unable to load base repo", err)
-						return
-					}
-					prHeadCommitID, err := git.GetFullCommitID(ctx, pull.BaseRepo.RepoPath(), prHeadRef)
-					if err != nil {
-						ctx.ServerError("Get head commit Id of pr fail", err)
-						return
-					}
-
-					// get head commit of branch in the head repo
-					if err := pull.LoadHeadRepo(ctx); err != nil {
-						ctx.ServerError("Unable to load head repo", err)
-						return
-					}
-					if ok := gitrepo.IsBranchExist(ctx, pull.HeadRepo, pull.BaseBranch); !ok {
-						// todo localize
-						ctx.JSONError("The origin branch is delete, cannot reopen.")
-						return
-					}
-					headBranchRef := pull.GetGitHeadBranchRefName()
-					headBranchCommitID, err := git.GetFullCommitID(ctx, pull.HeadRepo.RepoPath(), headBranchRef)
-					if err != nil {
-						ctx.ServerError("Get head commit Id of head branch fail", err)
-						return
-					}
-
-					err = pull.LoadIssue(ctx)
-					if err != nil {
-						ctx.ServerError("load the issue of pull request error", err)
-						return
-					}
-
-					if prHeadCommitID != headBranchCommitID {
-						// force push to base repo
-						err := git.Push(ctx, pull.HeadRepo.RepoPath(), git.PushOptions{
-							Remote: pull.BaseRepo.RepoPath(),
-							Branch: pull.HeadBranch + ":" + prHeadRef,
-							Force:  true,
-							Env:    repo_module.InternalPushingEnvironment(pull.Issue.Poster, pull.BaseRepo),
-						})
-						if err != nil {
-							ctx.ServerError("force push error", err)
-							return
-						}
-					}
-				}
+		if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) &&
+			!issue.IsPoster(ctx.Doer.ID) &&
+			!ctx.Doer.IsAdmin {
+			ctx.JSONError(ctx.Tr("repo.issues.reopen_not_allowed"))
+			return
+		}
+
+		if issue.IsPull && issue.PullRequest.HasMerged {
+			ctx.JSONError(ctx.Tr("repo.issues.reopen_not_allowed_merged"))
+			return
+		}
+
+		// check if an opened pull request exists with the same head branch and base branch
+		pull := issue.PullRequest
+		var err error
+		pr, err := issues_model.GetUnmergedPullRequest(ctx, pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch, pull.Flow)
+		if err != nil {
+			if !issues_model.IsErrPullRequestNotExist(err) {
+				ctx.JSONError(err.Error())
+				return
 			}
+		}
+		if pr != nil {
+			ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
+			return
+		}
 
-			if pr != nil {
-				ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
+		createdComment, err = issue_service.ReopenIssue(ctx, issue, ctx.Doer, "", form.Content, attachments)
+		if err != nil {
+			if errors.Is(err, user_model.ErrBlockedUser) {
+				ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user"))
 			} else {
-				if form.Status == "close" && !issue.IsClosed {
-					if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
-						log.Error("CloseIssue: %v", err)
-						if issues_model.IsErrDependenciesLeft(err) {
-							if issue.IsPull {
-								ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
-							} else {
-								ctx.JSONError(ctx.Tr("repo.issues.dependency.issue_close_blocked"))
-							}
-							return
-						}
-					} else {
-						if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil {
-							ctx.ServerError("stopTimerIfAvailable", err)
-							return
-						}
-						log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
-					}
-				} else if form.Status == "reopen" && issue.IsClosed {
-					if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
-						log.Error("ReopenIssue: %v", err)
-					}
+				ctx.ServerError("ReopenIssue", err)
+			}
+			return
+		}
+
+		// check whether the ref of PR <refs/pulls/pr_index/head> in base repo is consistent with the head commit of head branch in the head repo
+		// get head commit of PR
+		if pull.Flow == issues_model.PullRequestFlowGithub {
+			prHeadRef := pull.GetGitHeadRefName()
+			if err := pull.LoadBaseRepo(ctx); err != nil {
+				ctx.ServerError("Unable to load base repo", err)
+				return
+			}
+			prHeadCommitID, err := git.GetFullCommitID(ctx, pull.BaseRepo.RepoPath(), prHeadRef)
+			if err != nil {
+				ctx.ServerError("Get head commit Id of pr fail", err)
+				return
+			}
+
+			// get head commit of branch in the head repo
+			if err := pull.LoadHeadRepo(ctx); err != nil {
+				ctx.ServerError("Unable to load head repo", err)
+				return
+			}
+			if ok := gitrepo.IsBranchExist(ctx, pull.HeadRepo, pull.BaseBranch); !ok {
+				// todo localize
+				ctx.JSONError("The origin branch is delete, cannot reopen.")
+				return
+			}
+			headBranchRef := pull.GetGitHeadBranchRefName()
+			headBranchCommitID, err := git.GetFullCommitID(ctx, pull.HeadRepo.RepoPath(), headBranchRef)
+			if err != nil {
+				ctx.ServerError("Get head commit Id of head branch fail", err)
+				return
+			}
+
+			err = pull.LoadIssue(ctx)
+			if err != nil {
+				ctx.ServerError("load the issue of pull request error", err)
+				return
+			}
+
+			if prHeadCommitID != headBranchCommitID {
+				// force push to base repo
+				err := git.Push(ctx, pull.HeadRepo.RepoPath(), git.PushOptions{
+					Remote: pull.BaseRepo.RepoPath(),
+					Branch: pull.HeadBranch + ":" + prHeadRef,
+					Force:  true,
+					Env:    repo_module.InternalPushingEnvironment(pull.Issue.Poster, pull.BaseRepo),
+				})
+				if err != nil {
+					ctx.ServerError("force push error", err)
+					return
 				}
 			}
 		}
 
-		// Redirect to comment hashtag if there is any actual content.
-		typeName := "issues"
-		if issue.IsPull {
-			typeName = "pulls"
+		// Regenerate patch and test conflict.
+		pull.HeadCommitID = ""
+		pull_service.StartPullRequestCheckImmediately(ctx, pull)
+	case "close":
+		if issue.IsClosed {
+			ctx.JSONError(ctx.Tr("repo.issues.already_closed"))
+			return
 		}
-		if comment != nil {
-			ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d#%s", ctx.Repo.RepoLink, typeName, issue.Index, comment.HashTag()))
-		} else {
-			ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, typeName, issue.Index))
+
+		if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) &&
+			!issue.IsPoster(ctx.Doer.ID) &&
+			!ctx.Doer.IsAdmin {
+			ctx.JSONError(ctx.Tr("repo.issues.close_not_allowed"))
+			return
 		}
-	}()
 
-	// Fix #321: Allow empty comments, as long as we have attachments.
-	if len(form.Content) == 0 && len(attachments) == 0 {
-		return
+		createdComment, err = issue_service.CloseIssue(ctx, issue, ctx.Doer, "", form.Content, attachments)
+	default:
+		if len(form.Content) == 0 && len(attachments) == 0 {
+			ctx.JSONError(ctx.Tr("repo.issues.comment.empty_content"))
+			return
+		}
+
+		createdComment, err = issue_service.CreateIssueComment(ctx, ctx.Doer, ctx.Repo.Repository, issue, form.Content, attachments)
 	}
 
-	comment, err := issue_service.CreateIssueComment(ctx, ctx.Doer, ctx.Repo.Repository, issue, form.Content, attachments)
 	if err != nil {
 		if errors.Is(err, user_model.ErrBlockedUser) {
 			ctx.JSONError(ctx.Tr("repo.issues.comment.blocked_user"))
@@ -209,7 +208,17 @@ func NewComment(ctx *context.Context) {
 		return
 	}
 
-	log.Trace("Comment created: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, comment.ID)
+	// Redirect to comment hashtag if there is any actual content.
+	typeName := "issues"
+	if issue.IsPull {
+		typeName = "pulls"
+	}
+	if createdComment != nil {
+		log.Trace("Comment created: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, createdComment.ID)
+		ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d#%s", ctx.Repo.RepoLink, typeName, issue.Index, createdComment.HashTag()))
+	} else {
+		ctx.JSONRedirect(fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, typeName, issue.Index))
+	}
 }
 
 // UpdateCommentContent change comment of issue's content

routers/web/repo/issue_list.go

@@ -434,7 +434,7 @@ func UpdateIssueStatus(ctx *context.Context) {
 			continue
 		}
 		if action == "close" && !issue.IsClosed {
-			if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
+			if _, err := issue_service.CloseIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
 				if issues_model.IsErrDependenciesLeft(err) {
 					ctx.JSON(http.StatusPreconditionFailed, map[string]any{
 						"error": ctx.Tr("repo.issues.dependency.issue_batch_close_blocked", issue.Index),
@@ -445,7 +445,7 @@ func UpdateIssueStatus(ctx *context.Context) {
 				return
 			}
 		} else if action == "open" && issue.IsClosed {
-			if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
+			if _, err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, "", "", nil); err != nil {
 				ctx.ServerError("ReopenIssue", err)
 				return
 			}

services/issue/comments.go

@@ -15,6 +15,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/json"
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/timeutil"
 	git_service "code.gitea.io/gitea/services/git"
 	notify_service "code.gitea.io/gitea/services/notify"
@@ -55,6 +56,22 @@ func CreateRefComment(ctx context.Context, doer *user_model.User, repo *repo_mod
 	return err
 }
 
+func notifyCommentCreated(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, comment *issues_model.Comment) error {
+	mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, doer, comment.Content)
+	if err != nil {
+		return err
+	}
+
+	// reload issue to ensure it has the latest data, especially the number of comments
+	issue, err = issues_model.GetIssueByID(ctx, issue.ID)
+	if err != nil {
+		return err
+	}
+
+	notify_service.CreateIssueComment(ctx, doer, repo, issue, comment, mentions)
+	return nil
+}
+
 // CreateIssueComment creates a plain issue comment.
 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) {
 	if user_model.IsUserBlockedBy(ctx, doer, issue.PosterID, repo.OwnerID) {
@@ -75,19 +92,11 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m
 		return nil, err
 	}
 
-	mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, doer, comment.Content)
-	if err != nil {
-		return nil, err
+	if err := notifyCommentCreated(ctx, doer, repo, issue, comment); err != nil {
+		// If notification fails, we still return the comment but log the error.
+		log.Error("Failed to notify comment creation: %v", err)
 	}
 
-	// reload issue to ensure it has the latest data, especially the number of comments
-	issue, err = issues_model.GetIssueByID(ctx, issue.ID)
-	if err != nil {
-		return nil, err
-	}
-
-	notify_service.CreateIssueComment(ctx, doer, repo, issue, comment, mentions)
-
 	return comment, nil
 }
 

services/issue/commit.go

@@ -196,11 +196,11 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m
 						return err
 					}
 				}
-				if err := CloseIssue(ctx, refIssue, doer, c.Sha1); err != nil {
+				if _, err := CloseIssue(ctx, refIssue, doer, c.Sha1, "", nil); err != nil {
 					return err
 				}
 			} else if ref.Action == references.XRefActionReopens && refIssue.IsClosed {
-				if err := ReopenIssue(ctx, refIssue, doer, c.Sha1); err != nil {
+				if _, err := ReopenIssue(ctx, refIssue, doer, c.Sha1, "", nil); err != nil {
 					return err
 				}
 			}