From 406884a6f48e412b3392d45cfb33d93d897fba31 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 10 Nov 2024 13:12:13 -0800 Subject: [PATCH 1/9] Use CloseIssue and ReopenIssue instead of ChagneStatus for issues --- routers/api/v1/repo/issue.go | 19 ++++++++----- routers/api/v1/repo/pull.go | 17 ++++++++---- routers/web/repo/issue.go | 53 ++++++++++++++++++++++-------------- services/issue/commit.go | 4 +-- services/issue/status.go | 36 ++++++++++++++++-------- services/pull/merge.go | 4 +-- services/pull/pull.go | 4 +-- 7 files changed, 85 insertions(+), 52 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index cbe709c030db4..9fa9b6220a5e0 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,24 +912,29 @@ func EditIssue(ctx *context.APIContext) { } } - var isClosed bool + var closeOrReopen bool switch state := api.StateType(*form.State); state { case api.StateOpen: - isClosed = false + closeOrReopen = false case api.StateClosed: - isClosed = true + closeOrReopen = true default: ctx.Error(http.StatusPreconditionFailed, "UnknownIssueStateError", fmt.Sprintf("unknown state: %s", state)) return } - if issue.IsClosed != isClosed { - if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil { + if closeOrReopen && !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 because it still has open dependencies") return } - ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) + ctx.Error(http.StatusInternalServerError, "CloseIssue", err) + return + } + } else if !closeOrReopen && 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 28d7379f07b8a..100f4dd8cf99a 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -753,24 +753,29 @@ func EditPullRequest(ctx *context.APIContext) { return } - var isClosed bool + var closeOrReopen bool switch state := api.StateType(*form.State); state { case api.StateOpen: - isClosed = false + closeOrReopen = false case api.StateClosed: - isClosed = true + closeOrReopen = true default: ctx.Error(http.StatusPreconditionFailed, "UnknownPRStateError", fmt.Sprintf("unknown state: %s", state)) return } - if issue.IsClosed != isClosed { - if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil { + if closeOrReopen && !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 pull request because it still has open dependencies") return } - ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) + ctx.Error(http.StatusInternalServerError, "CloseIssue", err) + return + } + } else if !closeOrReopen && issue.IsClosed { + if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil { + ctx.Error(http.StatusInternalServerError, "ReopenIssue", err) return } } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 72f89bd27d0a0..deda5d1d13885 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2991,14 +2991,16 @@ func UpdateIssueStatus(ctx *context.Context) { return } - var isClosed bool + var closeOrReopen bool // true: close, false: reopen switch action := ctx.FormString("action"); action { case "open": - isClosed = false + closeOrReopen = false case "close": - isClosed = true + closeOrReopen = true default: log.Warn("Unrecognized action: %s", action) + ctx.JSONOK() + return } if _, err := issues.LoadRepositories(ctx); err != nil { @@ -3014,15 +3016,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 closeOrReopen && !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 !closeOrReopen && issue.IsClosed { + if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil { + ctx.ServerError("ReopenIssue", err) return } } @@ -3158,25 +3165,29 @@ 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")) + closeOrReopen := form.Status == "close" + if closeOrReopen { + 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 + 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/services/issue/commit.go b/services/issue/commit.go index 0579e0f5c53e6..cd64be44105cb 100644 --- a/services/issue/commit.go +++ b/services/issue/commit.go @@ -194,9 +194,9 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m return err } } - if isClosed != refIssue.IsClosed { + if isClosed && !refIssue.IsClosed { refIssue.Repo = refRepo - if err := ChangeStatus(ctx, refIssue, doer, c.Sha1, isClosed); err != nil { + if err := CloseIssue(ctx, refIssue, doer, c.Sha1); err != nil { return err } } diff --git a/services/issue/status.go b/services/issue/status.go index 967c29bd22230..b6ee54082ffec 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -12,14 +12,11 @@ import ( 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 and issue. +func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error { + comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, true) if err != nil { - if issues_model.IsErrDependenciesLeft(err) && closed { + if issues_model.IsErrDependenciesLeft(err) { if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { log.Error("Unable to stop stopwatch for issue[%d]#%d: %v", issue.ID, issue.Index, err) } @@ -27,13 +24,28 @@ func ChangeStatus(ctx context.Context, issue *issues_model.Issue, doer *user_mod return err } - if closed { - if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { - return err - } + if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { + return err + } + + 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.ChangeIssueStatus(ctx, issue, doer, false) + if err != nil { + return err + } + + if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { + return err } - notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, closed) + notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, false) return nil } diff --git a/services/pull/merge.go b/services/pull/merge.go index a3fbe4f627b00..5c68e6ef2c99d 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -243,8 +243,8 @@ func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullReques 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 isClosed && !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 diff --git a/services/pull/pull.go b/services/pull/pull.go index 3362cb97ff70f..2ec001b92a2b2 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -685,7 +685,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) } } @@ -719,7 +719,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) } } From ed261f1403b24eb9e2a507e8a2f8ad7b70e9cf5c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 10 Nov 2024 13:29:12 -0800 Subject: [PATCH 2/9] More improvements --- models/issues/dependency_test.go | 2 +- models/issues/issue_update.go | 16 ++++++++++++++-- models/issues/issue_xref_test.go | 2 +- routers/web/repo/issue.go | 4 ++-- services/issue/commit.go | 19 ++++++++++++------- services/issue/status.go | 8 ++------ services/pull/merge.go | 8 ++++++-- 7 files changed, 38 insertions(+), 21 deletions(-) diff --git a/models/issues/dependency_test.go b/models/issues/dependency_test.go index 6eed483cc9be7..b8c21f3d10c31 100644 --- a/models/issues/dependency_test.go +++ b/models/issues/dependency_test.go @@ -49,7 +49,7 @@ 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) left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 31d76be5e0aea..21458e458ca5e 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -118,8 +118,20 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use }) } +// 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 + } + if err := issue.LoadPoster(ctx); err != nil { + return nil, err + } + + return changeIssueStatus(ctx, issue, doer, true, false) +} + // ChangeIssueStatus changes issue status to open or closed. -func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed bool) (*Comment, error) { +func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) { if err := issue.LoadRepo(ctx); err != nil { return nil, err } @@ -127,7 +139,7 @@ func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, return nil, err } - return changeIssueStatus(ctx, issue, doer, isClosed, false) + return changeIssueStatus(ctx, issue, doer, false, false) } // 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 f1b1bb2a6b1b3..7f257330b769e 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/web/repo/issue.go b/routers/web/repo/issue.go index deda5d1d13885..94165b1518871 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3166,7 +3166,7 @@ func NewComment(ctx *context.Context) { ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index)) } else { closeOrReopen := form.Status == "close" - if closeOrReopen { + if closeOrReopen && !issue.IsClosed { if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil { log.Error("CloseIssue: %v", err) if issues_model.IsErrDependenciesLeft(err) { @@ -3184,7 +3184,7 @@ func NewComment(ctx *context.Context) { } log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed) } - } else { + } else if !closeOrReopen && issue.IsClosed { if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil { log.Error("ReopenIssue: %v", err) } diff --git a/services/issue/commit.go b/services/issue/commit.go index cd64be44105cb..805cdd8672d86 100644 --- a/services/issue/commit.go +++ b/services/issue/commit.go @@ -188,17 +188,22 @@ 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 { - return err + + closeOrReopen := ref.Action == references.XRefActionCloses + refIssue.Repo = refRepo + if closeOrReopen && !refIssue.IsClosed { + if len(ref.TimeLog) > 0 { + if err := issueAddTime(ctx, refIssue, doer, c.Timestamp, ref.TimeLog); err != nil { + return err + } } - } - if isClosed && !refIssue.IsClosed { - refIssue.Repo = refRepo if err := CloseIssue(ctx, refIssue, doer, c.Sha1); err != nil { return err } + } else if !closeOrReopen && 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 b6ee54082ffec..a7f3ce8f7b569 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -14,7 +14,7 @@ import ( // CloseIssue close and issue. func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error { - comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, true) + comment, err := issues_model.CloseIssue(ctx, issue, doer) if err != nil { if issues_model.IsErrDependenciesLeft(err) { if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { @@ -36,15 +36,11 @@ func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model // 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.ChangeIssueStatus(ctx, issue, doer, false) + comment, err := issues_model.ReopenIssue(ctx, issue, doer) if err != nil { return err } - if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); 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 5c68e6ef2c99d..14dfe235ab2fb 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -242,14 +242,18 @@ 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 { + closeOrReopen := ref.RefAction == references.XRefActionCloses + if closeOrReopen && !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 !closeOrReopen && ref.Issue.IsClosed { + if err = issue_service.ReopenIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil { + return err + } } } return nil From fdff19948a14a0a4a403f33420173704045ebd68 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 10 Nov 2024 13:42:16 -0800 Subject: [PATCH 3/9] Add transaction for ReopenIssue --- models/issues/issue_update.go | 15 ++++++++++++++- services/issue/status.go | 18 +++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 21458e458ca5e..849bc8125e52b 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -139,7 +139,20 @@ func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Com return nil, err } - return changeIssueStatus(ctx, issue, doer, false, false) + 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/services/issue/status.go b/services/issue/status.go index a7f3ce8f7b569..ecbf87d533f07 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -6,6 +6,7 @@ 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" @@ -14,19 +15,30 @@ import ( // CloseIssue close and issue. func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error { - comment, err := issues_model.CloseIssue(ctx, issue, doer) + dbCtx, committer, err := db.TxContext(ctx) + if 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(ctx, doer, issue); err != nil { + 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 err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { + if err := issues_model.FinishIssueStopwatchIfPossible(dbCtx, doer, issue); err != nil { + return err + } + + if err := committer.Commit(); err != nil { return err } + committer.Close() notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, true) From 6fe35175a7fcab7179c8f4f45d1e7ad10a1eeecb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 10 Nov 2024 13:45:31 -0800 Subject: [PATCH 4/9] Add transaction for CloseIssue --- models/issues/issue_update.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 849bc8125e52b..6766e7bbc52fc 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -127,7 +127,20 @@ func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comm return nil, err } - return changeIssueStatus(ctx, issue, doer, true, 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 } // ChangeIssueStatus changes issue status to open or closed. From e26a30105fcfef953564652c81472c0757f4f889 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Nov 2024 05:46:29 +0800 Subject: [PATCH 5/9] Update routers/web/repo/issue_list.go Co-authored-by: Zettat123 --- routers/web/repo/issue_list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/issue_list.go b/routers/web/repo/issue_list.go index f969c45093931..201cba38214d4 100644 --- a/routers/web/repo/issue_list.go +++ b/routers/web/repo/issue_list.go @@ -468,7 +468,7 @@ func UpdateIssueStatus(ctx *context.Context) { }) return } - ctx.ServerError("ChangeStatus", err) + ctx.ServerError("CloseIssue", err) return } } else if !closeOrReopen && issue.IsClosed { From 6e787dfcf80581ef87329e17de4e2f798e5ebbd8 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 27 Nov 2024 14:35:08 +0800 Subject: [PATCH 6/9] improve --- routers/api/v1/repo/issue.go | 50 ++++++++++++++++--------------- routers/api/v1/repo/pull.go | 27 ++--------------- routers/web/repo/issue_comment.go | 5 ++-- routers/web/repo/issue_list.go | 13 +++----- services/issue/commit.go | 5 ++-- services/pull/merge.go | 5 ++-- 6 files changed, 39 insertions(+), 66 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 9fa9b6220a5e0..37bd248c973d5 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -912,32 +912,11 @@ func EditIssue(ctx *context.APIContext) { } } - var closeOrReopen bool - switch state := api.StateType(*form.State); state { - case api.StateOpen: - closeOrReopen = false - case api.StateClosed: - closeOrReopen = 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 closeOrReopen && !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 because it still has open dependencies") - return - } - ctx.Error(http.StatusInternalServerError, "CloseIssue", err) - return - } - } else if !closeOrReopen && issue.IsClosed { - if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil { - ctx.Error(http.StatusInternalServerError, "ReopenIssue", err) - return - } - } } // Refetch from database to assign some automatic values @@ -1060,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 100f4dd8cf99a..1de937ef0457b 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -753,32 +753,11 @@ func EditPullRequest(ctx *context.APIContext) { return } - var closeOrReopen bool - switch state := api.StateType(*form.State); state { - case api.StateOpen: - closeOrReopen = false - case api.StateClosed: - closeOrReopen = 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 closeOrReopen && !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 pull request because it still has open dependencies") - return - } - ctx.Error(http.StatusInternalServerError, "CloseIssue", err) - return - } - } else if !closeOrReopen && issue.IsClosed { - if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil { - ctx.Error(http.StatusInternalServerError, "ReopenIssue", err) - return - } - } } // change pull target branch diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 3c6dae584eeed..2bb47f2588247 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -154,8 +154,7 @@ func NewComment(ctx *context.Context) { if pr != nil { ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index)) } else { - closeOrReopen := form.Status == "close" - if closeOrReopen && !issue.IsClosed { + 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) { @@ -173,7 +172,7 @@ func NewComment(ctx *context.Context) { } log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed) } - } else if !closeOrReopen && 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) } diff --git a/routers/web/repo/issue_list.go b/routers/web/repo/issue_list.go index 201cba38214d4..cdb769999db21 100644 --- a/routers/web/repo/issue_list.go +++ b/routers/web/repo/issue_list.go @@ -435,13 +435,8 @@ func UpdateIssueStatus(ctx *context.Context) { return } - var closeOrReopen bool // true: close, false: reopen - switch action := ctx.FormString("action"); action { - case "open": - closeOrReopen = false - case "close": - closeOrReopen = true - default: + action := ctx.FormString("action") + if action != "open" && action != "close" { log.Warn("Unrecognized action: %s", action) ctx.JSONOK() return @@ -460,7 +455,7 @@ func UpdateIssueStatus(ctx *context.Context) { if issue.IsPull && issue.PullRequest.HasMerged { continue } - if closeOrReopen && !issue.IsClosed { + 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{ @@ -471,7 +466,7 @@ func UpdateIssueStatus(ctx *context.Context) { ctx.ServerError("CloseIssue", err) return } - } else if !closeOrReopen && issue.IsClosed { + } 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 805cdd8672d86..963d0359fd35d 100644 --- a/services/issue/commit.go +++ b/services/issue/commit.go @@ -189,9 +189,8 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m } } - closeOrReopen := ref.Action == references.XRefActionCloses refIssue.Repo = refRepo - if closeOrReopen && !refIssue.IsClosed { + 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 @@ -200,7 +199,7 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m if err := CloseIssue(ctx, refIssue, doer, c.Sha1); err != nil { return err } - } else if !closeOrReopen && refIssue.IsClosed { + } else if ref.Action == references.XRefActionReopens && refIssue.IsClosed { if err := ReopenIssue(ctx, refIssue, doer, c.Sha1); err != nil { return err } diff --git a/services/pull/merge.go b/services/pull/merge.go index 14dfe235ab2fb..84b16a160ecbe 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -242,15 +242,14 @@ func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullReques if err = ref.Issue.LoadRepo(ctx); err != nil { return err } - closeOrReopen := ref.RefAction == references.XRefActionCloses - if closeOrReopen && !ref.Issue.IsClosed { + 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 !closeOrReopen && ref.Issue.IsClosed { + } else if ref.RefAction == references.XRefActionReopens && ref.Issue.IsClosed { if err = issue_service.ReopenIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil { return err } From 542eba6436cb6a7a832314ad3809359fdc864f7c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 18 Dec 2024 14:38:03 +0800 Subject: [PATCH 7/9] Apply suggestions from code review Co-authored-by: Zettat123 --- models/issues/issue_update.go | 2 +- services/issue/status.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index e07a5a74ae011..ceb4a4027eabe 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -144,7 +144,7 @@ func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comm return comment, nil } -// ChangeIssueStatus changes issue status to open or closed. +// 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 diff --git a/services/issue/status.go b/services/issue/status.go index ecbf87d533f07..e18b891175a0a 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -13,7 +13,7 @@ import ( notify_service "code.gitea.io/gitea/services/notify" ) -// CloseIssue close and issue. +// 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 { From a869ae7088a362886f39216aed19fc0ec8e36961 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 17 Dec 2024 22:58:35 -0800 Subject: [PATCH 8/9] Add reopen test --- models/issues/dependency_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/models/issues/dependency_test.go b/models/issues/dependency_test.go index b8c21f3d10c31..16d18036a6026 100644 --- a/models/issues/dependency_test.go +++ b/models/issues/dependency_test.go @@ -52,6 +52,10 @@ func TestCreateIssueDependency(t *testing.T) { _, err = issues_model.CloseIssue(db.DefaultContext, issue2, user1) assert.NoError(t, err) + issue2_closed, err := issues_model.GetIssueByID(db.DefaultContext, 2) + assert.NoError(t, err) + assert.True(t, issue2_closed.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) + + issue2_reopened, err := issues_model.GetIssueByID(db.DefaultContext, 2) + assert.NoError(t, err) + assert.False(t, issue2_reopened.IsClosed) } From f508a32e939a9f1503086941c2ad979b217097ea Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 17 Dec 2024 23:15:40 -0800 Subject: [PATCH 9/9] Fix test --- models/issues/dependency_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/models/issues/dependency_test.go b/models/issues/dependency_test.go index 16d18036a6026..67418039ded5e 100644 --- a/models/issues/dependency_test.go +++ b/models/issues/dependency_test.go @@ -52,9 +52,9 @@ func TestCreateIssueDependency(t *testing.T) { _, err = issues_model.CloseIssue(db.DefaultContext, issue2, user1) assert.NoError(t, err) - issue2_closed, err := issues_model.GetIssueByID(db.DefaultContext, 2) + issue2Closed, err := issues_model.GetIssueByID(db.DefaultContext, 2) assert.NoError(t, err) - assert.True(t, issue2_closed.IsClosed) + assert.True(t, issue2Closed.IsClosed) left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1) assert.NoError(t, err) @@ -67,7 +67,7 @@ func TestCreateIssueDependency(t *testing.T) { _, err = issues_model.ReopenIssue(db.DefaultContext, issue2, user1) assert.NoError(t, err) - issue2_reopened, err := issues_model.GetIssueByID(db.DefaultContext, 2) + issue2Reopened, err := issues_model.GetIssueByID(db.DefaultContext, 2) assert.NoError(t, err) - assert.False(t, issue2_reopened.IsClosed) + assert.False(t, issue2Reopened.IsClosed) }