From 5feb1a6bff6b6931ebe197258032557f55e32c6c Mon Sep 17 00:00:00 2001
From: Lunny Xiao <xiaolunwen@gmail.com>
Date: Tue, 24 Dec 2024 23:38:30 -0800
Subject: [PATCH] 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>
---
models/issues/dependency_test.go | 13 ++++++++-
models/issues/issue_update.go | 44 +++++++++++++++++++++++++++--
models/issues/issue_xref_test.go | 2 +-
routers/api/v1/repo/issue.go | 47 ++++++++++++++++++-------------
routers/api/v1/repo/pull.go | 22 ++-------------
routers/web/repo/issue_comment.go | 35 ++++++++++++-----------
routers/web/repo/issue_list.go | 22 ++++++++-------
services/issue/commit.go | 18 +++++++-----
services/issue/status.go | 46 +++++++++++++++++++++---------
services/pull/merge.go | 9 ++++--
services/pull/pull.go | 4 +--
11 files changed, 167 insertions(+), 95 deletions(-)
diff --git a/models/issues/dependency_test.go b/models/issues/dependency_test.go
index 6eed483cc9..67418039de 100644
--- a/models/issues/dependency_test.go
+++ b/models/issues/dependency_test.go
@@ -49,9 +49,13 @@ func TestCreateIssueDependency(t *testing.T) {
assert.False(t, left)
// Close #2 and check again
- _, err = issues_model.ChangeIssueStatus(db.DefaultContext, issue2, user1, true)
+ _, err = issues_model.CloseIssue(db.DefaultContext, issue2, user1)
assert.NoError(t, err)
+ issue2Closed, err := issues_model.GetIssueByID(db.DefaultContext, 2)
+ assert.NoError(t, err)
+ assert.True(t, issue2Closed.IsClosed)
+
left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1)
assert.NoError(t, err)
assert.True(t, left)
@@ -59,4 +63,11 @@ func TestCreateIssueDependency(t *testing.T) {
// Test removing the dependency
err = issues_model.RemoveIssueDependency(db.DefaultContext, user1, issue1, issue2, issues_model.DependencyTypeBlockedBy)
assert.NoError(t, err)
+
+ _, err = issues_model.ReopenIssue(db.DefaultContext, issue2, user1)
+ assert.NoError(t, err)
+
+ issue2Reopened, err := issues_model.GetIssueByID(db.DefaultContext, 2)
+ assert.NoError(t, err)
+ assert.False(t, issue2Reopened.IsClosed)
}
diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go
index 5b929c9115..ceb4a4027e 100644
--- a/models/issues/issue_update.go
+++ b/models/issues/issue_update.go
@@ -119,8 +119,8 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
})
}
-// ChangeIssueStatus changes issue status to open or closed.
-func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed bool) (*Comment, error) {
+// CloseIssue changes issue status to closed.
+func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) {
if err := issue.LoadRepo(ctx); err != nil {
return nil, err
}
@@ -128,7 +128,45 @@ func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User,
return nil, err
}
- return changeIssueStatus(ctx, issue, doer, isClosed, false)
+ ctx, committer, err := db.TxContext(ctx)
+ if err != nil {
+ return nil, err
+ }
+ defer committer.Close()
+
+ comment, err := changeIssueStatus(ctx, issue, doer, true, false)
+ if err != nil {
+ return nil, err
+ }
+ if err := committer.Commit(); err != nil {
+ return nil, err
+ }
+ return comment, nil
+}
+
+// ReopenIssue changes issue status to open.
+func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) {
+ if err := issue.LoadRepo(ctx); err != nil {
+ return nil, err
+ }
+ if err := issue.LoadPoster(ctx); err != nil {
+ return nil, err
+ }
+
+ ctx, committer, err := db.TxContext(ctx)
+ if err != nil {
+ return nil, err
+ }
+ defer committer.Close()
+
+ comment, err := changeIssueStatus(ctx, issue, doer, false, false)
+ if err != nil {
+ return nil, err
+ }
+ if err := committer.Commit(); err != nil {
+ return nil, err
+ }
+ return comment, nil
}
// ChangeIssueTitle changes the title of this issue, as the given user.
diff --git a/models/issues/issue_xref_test.go b/models/issues/issue_xref_test.go
index f1b1bb2a6b..7f257330b7 100644
--- a/models/issues/issue_xref_test.go
+++ b/models/issues/issue_xref_test.go
@@ -98,7 +98,7 @@ func TestXRef_ResolveCrossReferences(t *testing.T) {
i1 := testCreateIssue(t, 1, 2, "title1", "content1", false)
i2 := testCreateIssue(t, 1, 2, "title2", "content2", false)
i3 := testCreateIssue(t, 1, 2, "title3", "content3", false)
- _, err := issues_model.ChangeIssueStatus(db.DefaultContext, i3, d, true)
+ _, err := issues_model.CloseIssue(db.DefaultContext, i3, d)
assert.NoError(t, err)
pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index))
diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go
index 6f45fce226..86dbcee5f7 100644
--- a/routers/api/v1/repo/issue.go
+++ b/routers/api/v1/repo/issue.go
@@ -733,7 +733,7 @@ func CreateIssue(ctx *context.APIContext) {
}
if form.Closed {
- if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", true); err != nil {
+ if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
if issues_model.IsErrDependenciesLeft(err) {
ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies")
return
@@ -912,27 +912,11 @@ func EditIssue(ctx *context.APIContext) {
}
}
- var isClosed bool
- switch state := api.StateType(*form.State); state {
- case api.StateOpen:
- isClosed = false
- case api.StateClosed:
- isClosed = true
- default:
- ctx.Error(http.StatusPreconditionFailed, "UnknownIssueStateError", fmt.Sprintf("unknown state: %s", state))
+ state := api.StateType(*form.State)
+ closeOrReopenIssue(ctx, issue, state)
+ if ctx.Written() {
return
}
-
- if issue.IsClosed != isClosed {
- if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil {
- if issues_model.IsErrDependenciesLeft(err) {
- ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies")
- return
- }
- ctx.Error(http.StatusInternalServerError, "ChangeStatus", err)
- return
- }
- }
}
// Refetch from database to assign some automatic values
@@ -1055,3 +1039,26 @@ func UpdateIssueDeadline(ctx *context.APIContext) {
ctx.JSON(http.StatusCreated, api.IssueDeadline{Deadline: deadlineUnix.AsTimePtr()})
}
+
+func closeOrReopenIssue(ctx *context.APIContext, issue *issues_model.Issue, state api.StateType) {
+ if state != api.StateOpen && state != api.StateClosed {
+ ctx.Error(http.StatusPreconditionFailed, "UnknownIssueStateError", fmt.Sprintf("unknown state: %s", state))
+ return
+ }
+
+ if state == api.StateClosed && !issue.IsClosed {
+ if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
+ if issues_model.IsErrDependenciesLeft(err) {
+ ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue or pull request because it still has open dependencies")
+ return
+ }
+ ctx.Error(http.StatusInternalServerError, "CloseIssue", err)
+ return
+ }
+ } else if state == api.StateOpen && issue.IsClosed {
+ if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
+ ctx.Error(http.StatusInternalServerError, "ReopenIssue", err)
+ return
+ }
+ }
+}
diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index ca5120eef5..d0c3459b63 100644
--- a/routers/api/v1/repo/pull.go
+++ b/routers/api/v1/repo/pull.go
@@ -728,27 +728,11 @@ func EditPullRequest(ctx *context.APIContext) {
return
}
- var isClosed bool
- switch state := api.StateType(*form.State); state {
- case api.StateOpen:
- isClosed = false
- case api.StateClosed:
- isClosed = true
- default:
- ctx.Error(http.StatusPreconditionFailed, "UnknownPRStateError", fmt.Sprintf("unknown state: %s", state))
+ state := api.StateType(*form.State)
+ closeOrReopenIssue(ctx, issue, state)
+ if ctx.Written() {
return
}
-
- if issue.IsClosed != isClosed {
- if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil {
- if issues_model.IsErrDependenciesLeft(err) {
- ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies")
- return
- }
- ctx.Error(http.StatusInternalServerError, "ChangeStatus", err)
- return
- }
- }
}
// change pull target branch
diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go
index 438822431d..8564c613de 100644
--- a/routers/web/repo/issue_comment.go
+++ b/routers/web/repo/issue_comment.go
@@ -154,25 +154,28 @@ func NewComment(ctx *context.Context) {
if pr != nil {
ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
} else {
- isClosed := form.Status == "close"
- if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil {
- log.Error("ChangeStatus: %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"))
+ 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
}
- 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 err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil {
- ctx.ServerError("CreateOrStopIssueStopwatch", err)
- return
+ } else if form.Status == "reopen" && issue.IsClosed {
+ if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
+ log.Error("ReopenIssue: %v", err)
}
-
- log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
}
}
}
diff --git a/routers/web/repo/issue_list.go b/routers/web/repo/issue_list.go
index 2c73a24632..2f615a100e 100644
--- a/routers/web/repo/issue_list.go
+++ b/routers/web/repo/issue_list.go
@@ -418,14 +418,11 @@ func UpdateIssueStatus(ctx *context.Context) {
return
}
- var isClosed bool
- switch action := ctx.FormString("action"); action {
- case "open":
- isClosed = false
- case "close":
- isClosed = true
- default:
+ action := ctx.FormString("action")
+ if action != "open" && action != "close" {
log.Warn("Unrecognized action: %s", action)
+ ctx.JSONOK()
+ return
}
if _, err := issues.LoadRepositories(ctx); err != nil {
@@ -441,15 +438,20 @@ func UpdateIssueStatus(ctx *context.Context) {
if issue.IsPull && issue.PullRequest.HasMerged {
continue
}
- if issue.IsClosed != isClosed {
- if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil {
+ if action == "close" && !issue.IsClosed {
+ if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); 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),
})
return
}
- ctx.ServerError("ChangeStatus", err)
+ ctx.ServerError("CloseIssue", err)
+ return
+ }
+ } else if action == "open" && issue.IsClosed {
+ if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
+ ctx.ServerError("ReopenIssue", err)
return
}
}
diff --git a/services/issue/commit.go b/services/issue/commit.go
index 0579e0f5c5..963d0359fd 100644
--- a/services/issue/commit.go
+++ b/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
}
}
diff --git a/services/issue/status.go b/services/issue/status.go
index 967c29bd22..e18b891175 100644
--- a/services/issue/status.go
+++ b/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
}
diff --git a/services/pull/merge.go b/services/pull/merge.go
index fba85f1e51..75011a697c 100644
--- a/services/pull/merge.go
+++ b/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
diff --git a/services/pull/pull.go b/services/pull/pull.go
index 0256c2c3f6..ac91fd6409 100644
--- a/services/pull/pull.go
+++ b/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)
}
}