Skip to content

Commit 5feb1a6

Browse files
lunnyZettat123
lunny
and
Zettat123
authored
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 <[email protected]>
1 parent f44712f commit 5feb1a6

File tree

11 files changed

+167
-95
lines changed

11 files changed

+167
-95
lines changed

‎models/issues/dependency_test.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,25 @@ func TestCreateIssueDependency(t *testing.T) {
4949
assert.False(t, left)
5050

5151
// Close #2 and check again
52-
_, err = issues_model.ChangeIssueStatus(db.DefaultContext, issue2, user1, true)
52+
_, err = issues_model.CloseIssue(db.DefaultContext, issue2, user1)
5353
assert.NoError(t, err)
5454

55+
issue2Closed, err := issues_model.GetIssueByID(db.DefaultContext, 2)
56+
assert.NoError(t, err)
57+
assert.True(t, issue2Closed.IsClosed)
58+
5559
left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1)
5660
assert.NoError(t, err)
5761
assert.True(t, left)
5862

5963
// Test removing the dependency
6064
err = issues_model.RemoveIssueDependency(db.DefaultContext, user1, issue1, issue2, issues_model.DependencyTypeBlockedBy)
6165
assert.NoError(t, err)
66+
67+
_, err = issues_model.ReopenIssue(db.DefaultContext, issue2, user1)
68+
assert.NoError(t, err)
69+
70+
issue2Reopened, err := issues_model.GetIssueByID(db.DefaultContext, 2)
71+
assert.NoError(t, err)
72+
assert.False(t, issue2Reopened.IsClosed)
6273
}

‎models/issues/issue_update.go

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,16 +119,54 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
119119
})
120120
}
121121

122-
// ChangeIssueStatus changes issue status to open or closed.
123-
func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed bool) (*Comment, error) {
122+
// CloseIssue changes issue status to closed.
123+
func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) {
124124
if err := issue.LoadRepo(ctx); err != nil {
125125
return nil, err
126126
}
127127
if err := issue.LoadPoster(ctx); err != nil {
128128
return nil, err
129129
}
130130

131-
return changeIssueStatus(ctx, issue, doer, isClosed, false)
131+
ctx, committer, err := db.TxContext(ctx)
132+
if err != nil {
133+
return nil, err
134+
}
135+
defer committer.Close()
136+
137+
comment, err := changeIssueStatus(ctx, issue, doer, true, false)
138+
if err != nil {
139+
return nil, err
140+
}
141+
if err := committer.Commit(); err != nil {
142+
return nil, err
143+
}
144+
return comment, nil
145+
}
146+
147+
// ReopenIssue changes issue status to open.
148+
func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) {
149+
if err := issue.LoadRepo(ctx); err != nil {
150+
return nil, err
151+
}
152+
if err := issue.LoadPoster(ctx); err != nil {
153+
return nil, err
154+
}
155+
156+
ctx, committer, err := db.TxContext(ctx)
157+
if err != nil {
158+
return nil, err
159+
}
160+
defer committer.Close()
161+
162+
comment, err := changeIssueStatus(ctx, issue, doer, false, false)
163+
if err != nil {
164+
return nil, err
165+
}
166+
if err := committer.Commit(); err != nil {
167+
return nil, err
168+
}
169+
return comment, nil
132170
}
133171

134172
// ChangeIssueTitle changes the title of this issue, as the given user.

‎models/issues/issue_xref_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func TestXRef_ResolveCrossReferences(t *testing.T) {
9898
i1 := testCreateIssue(t, 1, 2, "title1", "content1", false)
9999
i2 := testCreateIssue(t, 1, 2, "title2", "content2", false)
100100
i3 := testCreateIssue(t, 1, 2, "title3", "content3", false)
101-
_, err := issues_model.ChangeIssueStatus(db.DefaultContext, i3, d, true)
101+
_, err := issues_model.CloseIssue(db.DefaultContext, i3, d)
102102
assert.NoError(t, err)
103103

104104
pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index))

‎routers/api/v1/repo/issue.go

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,7 @@ func CreateIssue(ctx *context.APIContext) {
733733
}
734734

735735
if form.Closed {
736-
if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", true); err != nil {
736+
if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
737737
if issues_model.IsErrDependenciesLeft(err) {
738738
ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies")
739739
return
@@ -912,27 +912,11 @@ func EditIssue(ctx *context.APIContext) {
912912
}
913913
}
914914

915-
var isClosed bool
916-
switch state := api.StateType(*form.State); state {
917-
case api.StateOpen:
918-
isClosed = false
919-
case api.StateClosed:
920-
isClosed = true
921-
default:
922-
ctx.Error(http.StatusPreconditionFailed, "UnknownIssueStateError", fmt.Sprintf("unknown state: %s", state))
915+
state := api.StateType(*form.State)
916+
closeOrReopenIssue(ctx, issue, state)
917+
if ctx.Written() {
923918
return
924919
}
925-
926-
if issue.IsClosed != isClosed {
927-
if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil {
928-
if issues_model.IsErrDependenciesLeft(err) {
929-
ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies")
930-
return
931-
}
932-
ctx.Error(http.StatusInternalServerError, "ChangeStatus", err)
933-
return
934-
}
935-
}
936920
}
937921

938922
// Refetch from database to assign some automatic values
@@ -1055,3 +1039,26 @@ func UpdateIssueDeadline(ctx *context.APIContext) {
10551039

10561040
ctx.JSON(http.StatusCreated, api.IssueDeadline{Deadline: deadlineUnix.AsTimePtr()})
10571041
}
1042+
1043+
func closeOrReopenIssue(ctx *context.APIContext, issue *issues_model.Issue, state api.StateType) {
1044+
if state != api.StateOpen && state != api.StateClosed {
1045+
ctx.Error(http.StatusPreconditionFailed, "UnknownIssueStateError", fmt.Sprintf("unknown state: %s", state))
1046+
return
1047+
}
1048+
1049+
if state == api.StateClosed && !issue.IsClosed {
1050+
if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
1051+
if issues_model.IsErrDependenciesLeft(err) {
1052+
ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue or pull request because it still has open dependencies")
1053+
return
1054+
}
1055+
ctx.Error(http.StatusInternalServerError, "CloseIssue", err)
1056+
return
1057+
}
1058+
} else if state == api.StateOpen && issue.IsClosed {
1059+
if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
1060+
ctx.Error(http.StatusInternalServerError, "ReopenIssue", err)
1061+
return
1062+
}
1063+
}
1064+
}

‎routers/api/v1/repo/pull.go

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -728,27 +728,11 @@ func EditPullRequest(ctx *context.APIContext) {
728728
return
729729
}
730730

731-
var isClosed bool
732-
switch state := api.StateType(*form.State); state {
733-
case api.StateOpen:
734-
isClosed = false
735-
case api.StateClosed:
736-
isClosed = true
737-
default:
738-
ctx.Error(http.StatusPreconditionFailed, "UnknownPRStateError", fmt.Sprintf("unknown state: %s", state))
731+
state := api.StateType(*form.State)
732+
closeOrReopenIssue(ctx, issue, state)
733+
if ctx.Written() {
739734
return
740735
}
741-
742-
if issue.IsClosed != isClosed {
743-
if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil {
744-
if issues_model.IsErrDependenciesLeft(err) {
745-
ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies")
746-
return
747-
}
748-
ctx.Error(http.StatusInternalServerError, "ChangeStatus", err)
749-
return
750-
}
751-
}
752736
}
753737

754738
// change pull target branch

‎routers/web/repo/issue_comment.go

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -154,25 +154,28 @@ func NewComment(ctx *context.Context) {
154154
if pr != nil {
155155
ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
156156
} else {
157-
isClosed := form.Status == "close"
158-
if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil {
159-
log.Error("ChangeStatus: %v", err)
160-
161-
if issues_model.IsErrDependenciesLeft(err) {
162-
if issue.IsPull {
163-
ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
164-
} else {
165-
ctx.JSONError(ctx.Tr("repo.issues.dependency.issue_close_blocked"))
157+
if form.Status == "close" && !issue.IsClosed {
158+
if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
159+
log.Error("CloseIssue: %v", err)
160+
if issues_model.IsErrDependenciesLeft(err) {
161+
if issue.IsPull {
162+
ctx.JSONError(ctx.Tr("repo.issues.dependency.pr_close_blocked"))
163+
} else {
164+
ctx.JSONError(ctx.Tr("repo.issues.dependency.issue_close_blocked"))
165+
}
166+
return
166167
}
167-
return
168+
} else {
169+
if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil {
170+
ctx.ServerError("stopTimerIfAvailable", err)
171+
return
172+
}
173+
log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
168174
}
169-
} else {
170-
if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil {
171-
ctx.ServerError("CreateOrStopIssueStopwatch", err)
172-
return
175+
} else if form.Status == "reopen" && issue.IsClosed {
176+
if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
177+
log.Error("ReopenIssue: %v", err)
173178
}
174-
175-
log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
176179
}
177180
}
178181
}

‎routers/web/repo/issue_list.go

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -418,14 +418,11 @@ func UpdateIssueStatus(ctx *context.Context) {
418418
return
419419
}
420420

421-
var isClosed bool
422-
switch action := ctx.FormString("action"); action {
423-
case "open":
424-
isClosed = false
425-
case "close":
426-
isClosed = true
427-
default:
421+
action := ctx.FormString("action")
422+
if action != "open" && action != "close" {
428423
log.Warn("Unrecognized action: %s", action)
424+
ctx.JSONOK()
425+
return
429426
}
430427

431428
if _, err := issues.LoadRepositories(ctx); err != nil {
@@ -441,15 +438,20 @@ func UpdateIssueStatus(ctx *context.Context) {
441438
if issue.IsPull && issue.PullRequest.HasMerged {
442439
continue
443440
}
444-
if issue.IsClosed != isClosed {
445-
if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil {
441+
if action == "close" && !issue.IsClosed {
442+
if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
446443
if issues_model.IsErrDependenciesLeft(err) {
447444
ctx.JSON(http.StatusPreconditionFailed, map[string]any{
448445
"error": ctx.Tr("repo.issues.dependency.issue_batch_close_blocked", issue.Index),
449446
})
450447
return
451448
}
452-
ctx.ServerError("ChangeStatus", err)
449+
ctx.ServerError("CloseIssue", err)
450+
return
451+
}
452+
} else if action == "open" && issue.IsClosed {
453+
if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
454+
ctx.ServerError("ReopenIssue", err)
453455
return
454456
}
455457
}

‎services/issue/commit.go

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -188,15 +188,19 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m
188188
continue
189189
}
190190
}
191-
isClosed := ref.Action == references.XRefActionCloses
192-
if isClosed && len(ref.TimeLog) > 0 {
193-
if err := issueAddTime(ctx, refIssue, doer, c.Timestamp, ref.TimeLog); err != nil {
191+
192+
refIssue.Repo = refRepo
193+
if ref.Action == references.XRefActionCloses && !refIssue.IsClosed {
194+
if len(ref.TimeLog) > 0 {
195+
if err := issueAddTime(ctx, refIssue, doer, c.Timestamp, ref.TimeLog); err != nil {
196+
return err
197+
}
198+
}
199+
if err := CloseIssue(ctx, refIssue, doer, c.Sha1); err != nil {
194200
return err
195201
}
196-
}
197-
if isClosed != refIssue.IsClosed {
198-
refIssue.Repo = refRepo
199-
if err := ChangeStatus(ctx, refIssue, doer, c.Sha1, isClosed); err != nil {
202+
} else if ref.Action == references.XRefActionReopens && refIssue.IsClosed {
203+
if err := ReopenIssue(ctx, refIssue, doer, c.Sha1); err != nil {
200204
return err
201205
}
202206
}

‎services/issue/status.go

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,34 +6,54 @@ package issue
66
import (
77
"context"
88

9+
"code.gitea.io/gitea/models/db"
910
issues_model "code.gitea.io/gitea/models/issues"
1011
user_model "code.gitea.io/gitea/models/user"
1112
"code.gitea.io/gitea/modules/log"
1213
notify_service "code.gitea.io/gitea/services/notify"
1314
)
1415

15-
// ChangeStatus changes issue status to open or closed.
16-
// closed means the target status
17-
// Fix me: you should check whether the current issue status is same to the target status before call this function
18-
// as in function changeIssueStatus we will return WasClosedError, even the issue status and target status are both open
19-
func ChangeStatus(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string, closed bool) error {
20-
comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, closed)
16+
// CloseIssue close an issue.
17+
func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error {
18+
dbCtx, committer, err := db.TxContext(ctx)
2119
if err != nil {
22-
if issues_model.IsErrDependenciesLeft(err) && closed {
23-
if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil {
20+
return err
21+
}
22+
defer committer.Close()
23+
24+
comment, err := issues_model.CloseIssue(dbCtx, issue, doer)
25+
if err != nil {
26+
if issues_model.IsErrDependenciesLeft(err) {
27+
if err := issues_model.FinishIssueStopwatchIfPossible(dbCtx, doer, issue); err != nil {
2428
log.Error("Unable to stop stopwatch for issue[%d]#%d: %v", issue.ID, issue.Index, err)
2529
}
2630
}
2731
return err
2832
}
2933

30-
if closed {
31-
if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil {
32-
return err
33-
}
34+
if err := issues_model.FinishIssueStopwatchIfPossible(dbCtx, doer, issue); err != nil {
35+
return err
36+
}
37+
38+
if err := committer.Commit(); err != nil {
39+
return err
40+
}
41+
committer.Close()
42+
43+
notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, true)
44+
45+
return nil
46+
}
47+
48+
// ReopenIssue reopen an issue.
49+
// FIXME: If some issues dependent this one are closed, should we also reopen them?
50+
func ReopenIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error {
51+
comment, err := issues_model.ReopenIssue(ctx, issue, doer)
52+
if err != nil {
53+
return err
3454
}
3555

36-
notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, closed)
56+
notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, false)
3757

3858
return nil
3959
}

‎services/pull/merge.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -263,14 +263,17 @@ func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullReques
263263
if err = ref.Issue.LoadRepo(ctx); err != nil {
264264
return err
265265
}
266-
isClosed := ref.RefAction == references.XRefActionCloses
267-
if isClosed != ref.Issue.IsClosed {
268-
if err = issue_service.ChangeStatus(ctx, ref.Issue, doer, pr.MergedCommitID, isClosed); err != nil {
266+
if ref.RefAction == references.XRefActionCloses && !ref.Issue.IsClosed {
267+
if err = issue_service.CloseIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil {
269268
// Allow ErrDependenciesLeft
270269
if !issues_model.IsErrDependenciesLeft(err) {
271270
return err
272271
}
273272
}
273+
} else if ref.RefAction == references.XRefActionReopens && ref.Issue.IsClosed {
274+
if err = issue_service.ReopenIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil {
275+
return err
276+
}
274277
}
275278
}
276279
return nil

0 commit comments

Comments
 (0)