Use CloseIssue and ReopenIssue instead of ChangeStatus (#32467)

The behaviors of closing issues and reopening issues are very different.
So splitting it into two different functions makes it easier to
maintain.

- [x] Split ChangeIssueStatus into CloseIssue and ReopenIssue both at
the service layer and model layer
- [x] Rename `isClosed` to `CloseOrReopen` to make it more readable.
- [x] Add transactions for ReopenIssue and CloseIssue

---------

Co-authored-by: Zettat123 <zettat123@gmail.com>

services/issue/commit.go

@@ -188,15 +188,19 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m
 					continue
 				}
 			}
-			isClosed := ref.Action == references.XRefActionCloses
-			if isClosed && len(ref.TimeLog) > 0 {
-				if err := issueAddTime(ctx, refIssue, doer, c.Timestamp, ref.TimeLog); err != nil {
+
+			refIssue.Repo = refRepo
+			if ref.Action == references.XRefActionCloses && !refIssue.IsClosed {
+				if len(ref.TimeLog) > 0 {
+					if err := issueAddTime(ctx, refIssue, doer, c.Timestamp, ref.TimeLog); err != nil {
+						return err
+					}
+				}
+				if err := CloseIssue(ctx, refIssue, doer, c.Sha1); err != nil {
 					return err
 				}
-			}
-			if isClosed != refIssue.IsClosed {
-				refIssue.Repo = refRepo
-				if err := ChangeStatus(ctx, refIssue, doer, c.Sha1, isClosed); err != nil {
+			} else if ref.Action == references.XRefActionReopens && refIssue.IsClosed {
+				if err := ReopenIssue(ctx, refIssue, doer, c.Sha1); err != nil {
 					return err
 				}
 			}

services/issue/status.go

@@ -6,34 +6,54 @@ package issue
 import (
 	"context"
 
+	"code.gitea.io/gitea/models/db"
 	issues_model "code.gitea.io/gitea/models/issues"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/log"
 	notify_service "code.gitea.io/gitea/services/notify"
 )
 
-// ChangeStatus changes issue status to open or closed.
-// closed means the target status
-// Fix me: you should check whether the current issue status is same to the target status before call this function
-// as in function changeIssueStatus we will return WasClosedError, even the issue status and target status are both open
-func ChangeStatus(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string, closed bool) error {
-	comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, closed)
+// CloseIssue close an issue.
+func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error {
+	dbCtx, committer, err := db.TxContext(ctx)
 	if err != nil {
-		if issues_model.IsErrDependenciesLeft(err) && closed {
-			if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil {
+		return err
+	}
+	defer committer.Close()
+
+	comment, err := issues_model.CloseIssue(dbCtx, issue, doer)
+	if err != nil {
+		if issues_model.IsErrDependenciesLeft(err) {
+			if err := issues_model.FinishIssueStopwatchIfPossible(dbCtx, doer, issue); err != nil {
 				log.Error("Unable to stop stopwatch for issue[%d]#%d: %v", issue.ID, issue.Index, err)
 			}
 		}
 		return err
 	}
 
-	if closed {
-		if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil {
-			return err
-		}
+	if err := issues_model.FinishIssueStopwatchIfPossible(dbCtx, doer, issue); err != nil {
+		return err
 	}
 
-	notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, closed)
+	if err := committer.Commit(); err != nil {
+		return err
+	}
+	committer.Close()
+
+	notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, true)
+
+	return nil
+}
+
+// ReopenIssue reopen an issue.
+// FIXME: If some issues dependent this one are closed, should we also reopen them?
+func ReopenIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error {
+	comment, err := issues_model.ReopenIssue(ctx, issue, doer)
+	if err != nil {
+		return err
+	}
+
+	notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, false)
 
 	return nil
 }

services/pull/merge.go

@@ -263,14 +263,17 @@ func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullReques
 		if err = ref.Issue.LoadRepo(ctx); err != nil {
 			return err
 		}
-		isClosed := ref.RefAction == references.XRefActionCloses
-		if isClosed != ref.Issue.IsClosed {
-			if err = issue_service.ChangeStatus(ctx, ref.Issue, doer, pr.MergedCommitID, isClosed); err != nil {
+		if ref.RefAction == references.XRefActionCloses && !ref.Issue.IsClosed {
+			if err = issue_service.CloseIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil {
 				// Allow ErrDependenciesLeft
 				if !issues_model.IsErrDependenciesLeft(err) {
 					return err
 				}
 			}
+		} else if ref.RefAction == references.XRefActionReopens && ref.Issue.IsClosed {
+			if err = issue_service.ReopenIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil {
+				return err
+			}
 		}
 	}
 	return nil

services/pull/pull.go

@@ -706,7 +706,7 @@ func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64,
 
 	var errs errlist
 	for _, pr := range prs {
-		if err = issue_service.ChangeStatus(ctx, pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) {
+		if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) {
 			errs = append(errs, err)
 		}
 	}
@@ -740,7 +740,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
 			if pr.BaseRepoID == repo.ID {
 				continue
 			}
-			if err = issue_service.ChangeStatus(ctx, pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) {
+			if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) {
 				errs = append(errs, err)
 			}
 		}