From 406884a6f48e412b3392d45cfb33d93d897fba31 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 10 Nov 2024 13:12:13 -0800 Subject: [PATCH] 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 cbe709c030d..9fa9b6220a5 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 28d7379f07b..100f4dd8cf9 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 72f89bd27d0..deda5d1d138 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 0579e0f5c53..cd64be44105 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 967c29bd222..b6ee54082ff 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, closed) + 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, false) return nil } diff --git a/services/pull/merge.go b/services/pull/merge.go index a3fbe4f627b..5c68e6ef2c9 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 3362cb97ff7..2ec001b92a2 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) } }