From 078d120ac5e687802538991f1f1c90023e14c6f8 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Fri, 26 Apr 2024 16:47:06 +0800 Subject: [PATCH 01/10] call notifier after merging a pr --- models/issues/dependency_test.go | 2 +- models/issues/issue_update.go | 4 ++-- models/issues/issue_xref_test.go | 2 +- models/issues/pull.go | 4 ---- routers/api/v1/repo/issue.go | 2 +- routers/web/repo/issue.go | 7 ++++--- services/issue/commit.go | 2 +- services/issue/status.go | 4 ++-- services/pull/check.go | 5 +++++ services/pull/merge.go | 12 ++++++++++-- services/pull/pull.go | 4 ++-- 11 files changed, 29 insertions(+), 19 deletions(-) diff --git a/models/issues/dependency_test.go b/models/issues/dependency_test.go index 6eed483cc9be7..4c40dc2904463 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.ChangeIssueStatus(db.DefaultContext, issue2, user1, true, false) 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 ef96e1ee50eaf..0b669fc969859 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -119,7 +119,7 @@ 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) { +func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed, isMergePull bool) (*Comment, error) { if err := issue.LoadRepo(ctx); err != nil { return nil, err } @@ -127,7 +127,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, isClosed, isMergePull) } // 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 5bcaf75518657..9e9d6f1dc86c6 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.ChangeIssueStatus(db.DefaultContext, i3, d, true, false) assert.NoError(t, err) pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index)) diff --git a/models/issues/pull.go b/models/issues/pull.go index dc1b1b956aab8..bbf958b137f16 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -499,10 +499,6 @@ func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) { return false, err } - if _, err := changeIssueStatus(ctx, pr.Issue, pr.Merger, true, true); err != nil { - return false, fmt.Errorf("Issue.changeStatus: %w", err) - } - // reset the conflicted files as there cannot be any if we're merged pr.ConflictedFiles = []string{} diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index dfe6d31f74ec0..57649611e35b2 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -720,7 +720,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.ChangeStatus(ctx, issue, ctx.Doer, "", true, false); err != nil { if issues_model.IsErrDependenciesLeft(err) { ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") return diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 95f0cf3d71cff..8523aadcb1990 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -242,7 +242,8 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt } var isShowClosed optional.Option[bool] - switch ctx.FormString("state") { + stateVal := ctx.FormString("state") + switch stateVal { case "closed": isShowClosed = optional.Some(true) case "all": @@ -2924,7 +2925,7 @@ func UpdateIssueStatus(ctx *context.Context) { continue } if issue.IsClosed != isClosed { - if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil { + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed, false); 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), @@ -3068,7 +3069,7 @@ func NewComment(ctx *context.Context) { 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 { + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed, false); err != nil { log.Error("ChangeStatus: %v", err) if issues_model.IsErrDependenciesLeft(err) { diff --git a/services/issue/commit.go b/services/issue/commit.go index 0579e0f5c53e6..8ed0191fe097b 100644 --- a/services/issue/commit.go +++ b/services/issue/commit.go @@ -196,7 +196,7 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m } if isClosed != refIssue.IsClosed { refIssue.Repo = refRepo - if err := ChangeStatus(ctx, refIssue, doer, c.Sha1, isClosed); err != nil { + if err := ChangeStatus(ctx, refIssue, doer, c.Sha1, isClosed, false); err != nil { return err } } diff --git a/services/issue/status.go b/services/issue/status.go index 9b6c683f4f9cc..e2e411fc377c2 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -13,8 +13,8 @@ import ( ) // ChangeStatus changes issue status to open or closed. -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) +func ChangeStatus(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string, closed, isMergePull bool) error { + comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, closed, isMergePull) if err != nil { if issues_model.IsErrDependenciesLeft(err) && closed { if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { diff --git a/services/pull/check.go b/services/pull/check.go index f4dd332b1440a..83c82e18d57c9 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -27,6 +27,7 @@ import ( "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/timeutil" asymkey_service "code.gitea.io/gitea/services/asymkey" + issue_service "code.gitea.io/gitea/services/issue" notify_service "code.gitea.io/gitea/services/notify" ) @@ -296,6 +297,10 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { return false } else if !merged { return false + } else { + if err = issue_service.ChangeStatus(ctx, pr.Issue, pr.Merger, pr.MergedCommitID, true, true); err != nil { + log.Error("ChangeStatus %-v: %v", pr, err) + } } notify_service.MergePullRequest(ctx, merger, pr) diff --git a/services/pull/merge.go b/services/pull/merge.go index 00f23e1e3ae23..a542c6646cfa7 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -193,8 +193,12 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U pr.Merger = doer pr.MergerID = doer.ID - if _, err := pr.SetMerged(ctx); err != nil { + if merged, err := pr.SetMerged(ctx); err != nil { log.Error("SetMerged %-v: %v", pr, err) + } else if merged { + if err = issue_service.ChangeStatus(ctx, pr.Issue, pr.Merger, pr.MergedCommitID, true, true); err != nil { + log.Error("ChangeStatus %-v: %v", pr, err) + } } if err := pr.LoadIssue(ctx); err != nil { @@ -233,7 +237,7 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } 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 err = issue_service.ChangeStatus(ctx, ref.Issue, doer, pr.MergedCommitID, isClosed, false); err != nil { // Allow ErrDependenciesLeft if !issues_model.IsErrDependenciesLeft(err) { return err @@ -530,6 +534,10 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use return err } else if !merged { return fmt.Errorf("SetMerged failed") + } else { + if err = issue_service.ChangeStatus(ctx, pr.Issue, pr.Merger, pr.MergedCommitID, true, true); err != nil { + log.Error("ChangeStatus %-v: %v", pr, err) + } } return nil }); err != nil { diff --git a/services/pull/pull.go b/services/pull/pull.go index 764be5c6e39cc..b113508e42281 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -633,7 +633,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.ChangeStatus(ctx, pr.Issue, doer, "", true, false); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) } } @@ -667,7 +667,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.ChangeStatus(ctx, pr.Issue, doer, "", true, false); err != nil && !issues_model.IsErrPullWasClosed(err) { errs = append(errs, err) } } From 802efa342dbdf51543c8583380837e296033da2e Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Fri, 26 Apr 2024 17:07:09 +0800 Subject: [PATCH 02/10] remove stopTimerIfAvailable --- routers/web/repo/issue.go | 7 ------- routers/web/repo/pull.go | 17 ----------------- 2 files changed, 24 deletions(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 8523aadcb1990..061a155fbf108 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3080,13 +3080,6 @@ func NewComment(ctx *context.Context) { } return } - } else { - if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil { - ctx.ServerError("CreateOrStopIssueStopwatch", err) - return - } - - log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed) } } } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index acdba4bcdc021..d3ff5522d5190 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1153,13 +1153,6 @@ func MergePullRequest(ctx *context.Context) { } log.Trace("Pull request merged: %d", pr.ID) - if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil { - ctx.ServerError("stopTimerIfAvailable", err) - return - } - - log.Trace("Pull request merged: %d", pr.ID) - if form.DeleteBranchAfterMerge { // Don't cleanup when other pr use this branch as head branch exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) @@ -1209,16 +1202,6 @@ func CancelAutoMergePullRequest(ctx *context.Context) { ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) } -func stopTimerIfAvailable(ctx *context.Context, user *user_model.User, issue *issues_model.Issue) error { - if issues_model.StopwatchExists(ctx, user.ID, issue.ID) { - if err := issues_model.CreateOrStopIssueStopwatch(ctx, user, issue); err != nil { - return err - } - } - - return nil -} - // CompareAndPullRequestPost response for creating pull request func CompareAndPullRequestPost(ctx *context.Context) { form := web.GetForm(ctx).(*forms.CreateIssueForm) From a02791ffe2f0fec5f25bb1058289e9678a0d2067 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Sun, 28 Apr 2024 13:54:24 +0800 Subject: [PATCH 03/10] lint --- services/pull/check.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/services/pull/check.go b/services/pull/check.go index 83c82e18d57c9..f3f30fd147a90 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -297,10 +297,10 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { return false } else if !merged { return false - } else { - if err = issue_service.ChangeStatus(ctx, pr.Issue, pr.Merger, pr.MergedCommitID, true, true); err != nil { - log.Error("ChangeStatus %-v: %v", pr, err) - } + } + + if err = issue_service.ChangeStatus(ctx, pr.Issue, pr.Merger, pr.MergedCommitID, true, true); err != nil { + log.Error("ChangeStatus %-v: %v", pr, err) } notify_service.MergePullRequest(ctx, merger, pr) From 6bf388bdd7c13560a5a12a23540777c6a9fffbef Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Sun, 28 Apr 2024 14:26:25 +0800 Subject: [PATCH 04/10] Revert --- models/issues/dependency_test.go | 2 +- models/issues/issue_update.go | 4 ++-- models/issues/issue_xref_test.go | 2 +- models/issues/pull.go | 4 ++++ routers/api/v1/repo/issue.go | 2 +- routers/web/repo/issue.go | 14 ++++++++++---- routers/web/repo/pull.go | 17 +++++++++++++++++ services/issue/commit.go | 2 +- services/issue/status.go | 4 ++-- services/pull/check.go | 5 ----- services/pull/merge.go | 12 ++---------- services/pull/pull.go | 4 ++-- 12 files changed, 43 insertions(+), 29 deletions(-) diff --git a/models/issues/dependency_test.go b/models/issues/dependency_test.go index 4c40dc2904463..6eed483cc9be7 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, false) + _, err = issues_model.ChangeIssueStatus(db.DefaultContext, issue2, user1, true) 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 0b669fc969859..ef96e1ee50eaf 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -119,7 +119,7 @@ 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, isMergePull bool) (*Comment, error) { +func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed bool) (*Comment, error) { if err := issue.LoadRepo(ctx); err != nil { return nil, err } @@ -127,7 +127,7 @@ func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, return nil, err } - return changeIssueStatus(ctx, issue, doer, isClosed, isMergePull) + return changeIssueStatus(ctx, issue, doer, isClosed, 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 9e9d6f1dc86c6..5bcaf75518657 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, false) + _, err := issues_model.ChangeIssueStatus(db.DefaultContext, i3, d, true) assert.NoError(t, err) pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index)) diff --git a/models/issues/pull.go b/models/issues/pull.go index bbf958b137f16..dc1b1b956aab8 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -499,6 +499,10 @@ func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) { return false, err } + if _, err := changeIssueStatus(ctx, pr.Issue, pr.Merger, true, true); err != nil { + return false, fmt.Errorf("Issue.changeStatus: %w", err) + } + // reset the conflicted files as there cannot be any if we're merged pr.ConflictedFiles = []string{} diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 57649611e35b2..dfe6d31f74ec0 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -720,7 +720,7 @@ func CreateIssue(ctx *context.APIContext) { } if form.Closed { - if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", true, false); err != nil { + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", true); err != nil { if issues_model.IsErrDependenciesLeft(err) { ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") return diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 061a155fbf108..95f0cf3d71cff 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -242,8 +242,7 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt } var isShowClosed optional.Option[bool] - stateVal := ctx.FormString("state") - switch stateVal { + switch ctx.FormString("state") { case "closed": isShowClosed = optional.Some(true) case "all": @@ -2925,7 +2924,7 @@ func UpdateIssueStatus(ctx *context.Context) { continue } if issue.IsClosed != isClosed { - if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed, false); err != nil { + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); 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), @@ -3069,7 +3068,7 @@ func NewComment(ctx *context.Context) { 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, false); err != nil { + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", isClosed); err != nil { log.Error("ChangeStatus: %v", err) if issues_model.IsErrDependenciesLeft(err) { @@ -3080,6 +3079,13 @@ func NewComment(ctx *context.Context) { } return } + } else { + if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil { + ctx.ServerError("CreateOrStopIssueStopwatch", err) + return + } + + log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed) } } } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index d3ff5522d5190..acdba4bcdc021 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1153,6 +1153,13 @@ func MergePullRequest(ctx *context.Context) { } log.Trace("Pull request merged: %d", pr.ID) + if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil { + ctx.ServerError("stopTimerIfAvailable", err) + return + } + + log.Trace("Pull request merged: %d", pr.ID) + if form.DeleteBranchAfterMerge { // Don't cleanup when other pr use this branch as head branch exist, err := issues_model.HasUnmergedPullRequestsByHeadInfo(ctx, pr.HeadRepoID, pr.HeadBranch) @@ -1202,6 +1209,16 @@ func CancelAutoMergePullRequest(ctx *context.Context) { ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) } +func stopTimerIfAvailable(ctx *context.Context, user *user_model.User, issue *issues_model.Issue) error { + if issues_model.StopwatchExists(ctx, user.ID, issue.ID) { + if err := issues_model.CreateOrStopIssueStopwatch(ctx, user, issue); err != nil { + return err + } + } + + return nil +} + // CompareAndPullRequestPost response for creating pull request func CompareAndPullRequestPost(ctx *context.Context) { form := web.GetForm(ctx).(*forms.CreateIssueForm) diff --git a/services/issue/commit.go b/services/issue/commit.go index 8ed0191fe097b..0579e0f5c53e6 100644 --- a/services/issue/commit.go +++ b/services/issue/commit.go @@ -196,7 +196,7 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m } if isClosed != refIssue.IsClosed { refIssue.Repo = refRepo - if err := ChangeStatus(ctx, refIssue, doer, c.Sha1, isClosed, false); err != nil { + if err := ChangeStatus(ctx, refIssue, doer, c.Sha1, isClosed); err != nil { return err } } diff --git a/services/issue/status.go b/services/issue/status.go index e2e411fc377c2..9b6c683f4f9cc 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -13,8 +13,8 @@ import ( ) // ChangeStatus changes issue status to open or closed. -func ChangeStatus(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string, closed, isMergePull bool) error { - comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, closed, isMergePull) +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) if err != nil { if issues_model.IsErrDependenciesLeft(err) && closed { if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil { diff --git a/services/pull/check.go b/services/pull/check.go index f3f30fd147a90..f4dd332b1440a 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -27,7 +27,6 @@ import ( "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/timeutil" asymkey_service "code.gitea.io/gitea/services/asymkey" - issue_service "code.gitea.io/gitea/services/issue" notify_service "code.gitea.io/gitea/services/notify" ) @@ -299,10 +298,6 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { return false } - if err = issue_service.ChangeStatus(ctx, pr.Issue, pr.Merger, pr.MergedCommitID, true, true); err != nil { - log.Error("ChangeStatus %-v: %v", pr, err) - } - notify_service.MergePullRequest(ctx, merger, pr) log.Info("manuallyMerged[%-v]: Marked as manually merged into %s/%s by commit id: %s", pr, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String()) diff --git a/services/pull/merge.go b/services/pull/merge.go index a542c6646cfa7..00f23e1e3ae23 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -193,12 +193,8 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U pr.Merger = doer pr.MergerID = doer.ID - if merged, err := pr.SetMerged(ctx); err != nil { + if _, err := pr.SetMerged(ctx); err != nil { log.Error("SetMerged %-v: %v", pr, err) - } else if merged { - if err = issue_service.ChangeStatus(ctx, pr.Issue, pr.Merger, pr.MergedCommitID, true, true); err != nil { - log.Error("ChangeStatus %-v: %v", pr, err) - } } if err := pr.LoadIssue(ctx); err != nil { @@ -237,7 +233,7 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } isClosed := ref.RefAction == references.XRefActionCloses if isClosed != ref.Issue.IsClosed { - if err = issue_service.ChangeStatus(ctx, ref.Issue, doer, pr.MergedCommitID, isClosed, false); err != nil { + if err = issue_service.ChangeStatus(ctx, ref.Issue, doer, pr.MergedCommitID, isClosed); err != nil { // Allow ErrDependenciesLeft if !issues_model.IsErrDependenciesLeft(err) { return err @@ -534,10 +530,6 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use return err } else if !merged { return fmt.Errorf("SetMerged failed") - } else { - if err = issue_service.ChangeStatus(ctx, pr.Issue, pr.Merger, pr.MergedCommitID, true, true); err != nil { - log.Error("ChangeStatus %-v: %v", pr, err) - } } return nil }); err != nil { diff --git a/services/pull/pull.go b/services/pull/pull.go index b113508e42281..764be5c6e39cc 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -633,7 +633,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, false); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { + if err = issue_service.ChangeStatus(ctx, pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) { errs = append(errs, err) } } @@ -667,7 +667,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, false); err != nil && !issues_model.IsErrPullWasClosed(err) { + if err = issue_service.ChangeStatus(ctx, pr.Issue, doer, "", true); err != nil && !issues_model.IsErrPullWasClosed(err) { errs = append(errs, err) } } From 7b70192356048ecbfe042a65faebb86cb465b4c3 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Sun, 28 Apr 2024 14:55:53 +0800 Subject: [PATCH 05/10] update indexer notifier --- services/indexer/notify.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/services/indexer/notify.go b/services/indexer/notify.go index f1e21a2d40ed1..e2cfe477d3d29 100644 --- a/services/indexer/notify.go +++ b/services/indexer/notify.go @@ -152,3 +152,19 @@ func (r *indexerNotifier) IssueChangeLabels(ctx context.Context, doer *user_mode func (r *indexerNotifier) IssueClearLabels(ctx context.Context, doer *user_model.User, issue *issues_model.Issue) { issue_indexer.UpdateIssueIndexer(ctx, issue.ID) } + +func (r *indexerNotifier) MergePullRequest(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest) { + if err := pr.LoadIssue(ctx); err != nil { + log.Error("LoadIssue: %v", err) + return + } + issue_indexer.UpdateIssueIndexer(ctx, pr.Issue.ID) +} + +func (r *indexerNotifier) AutoMergePullRequest(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest) { + if err := pr.LoadIssue(ctx); err != nil { + log.Error("LoadIssue: %v", err) + return + } + issue_indexer.UpdateIssueIndexer(ctx, pr.Issue.ID) +} From 214d2412cab1f7cc7e4173e88886fd28cdab81b6 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Sun, 28 Apr 2024 18:00:03 +0800 Subject: [PATCH 06/10] add TestPullMergeIndexerNotifier --- tests/integration/pull_merge_test.go | 60 ++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index daf411f452a44..171602c7cf064 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -26,6 +26,7 @@ import ( "code.gitea.io/gitea/models/webhook" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/queue" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" @@ -587,3 +588,62 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) { assert.EqualValues(t, "Closed", prStatus) }) } + +func TestPullMergeIndexerNotifier(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + hookTasks, err := webhook.HookTasks(db.DefaultContext, 1, 1) // Retrieve previous hook number + assert.NoError(t, err) + hookTasksLenBefore := len(hookTasks) + + // create a pull request + session := loginUser(t, "user1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") + testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") + createPullResp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "Indexer notifier test pull") + + queue.GetManager().FlushAll(context.Background(), 5*time.Second) + + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ + OwnerName: "user2", + Name: "repo1", + }) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ + RepoID: repo1.ID, + Title: "Indexer notifier test pull", + IsPull: true, + IsClosed: false, + }) + + // search issues + link, _ := url.Parse("/api/v1/repos/issues/search") + query := url.Values{} + query.Set("state", "open") + query.Set("type", "pulls") + query.Set("q", "notifier") + link.RawQuery = query.Encode() + req := NewRequest(t, "GET", link.String()) + searchIssuesResp := session.MakeRequest(t, req, http.StatusOK) + var apiIssuesBefore []*api.Issue + DecodeJSON(t, searchIssuesResp, &apiIssuesBefore) + if assert.Len(t, apiIssuesBefore, 1) { + assert.Equal(t, issue.ID, apiIssuesBefore[0].ID) + } + + // merge the pull request + elem := strings.Split(test.RedirectURL(createPullResp), "/") + assert.EqualValues(t, "pulls", elem[3]) + testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false) + + queue.GetManager().FlushAll(context.Background(), 5*time.Second) + + // search issues again + searchIssuesResp = session.MakeRequest(t, req, http.StatusOK) + var apiIssuesAfter []*api.Issue + DecodeJSON(t, searchIssuesResp, &apiIssuesAfter) + assert.Len(t, apiIssuesAfter, 0) + + hookTasks, err = webhook.HookTasks(db.DefaultContext, 1, 1) + assert.NoError(t, err) + assert.Len(t, hookTasks, hookTasksLenBefore+1) + }) +} From cad8ea713d4149cf581666a2003e5c21254bc052 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 29 Apr 2024 09:43:48 +0800 Subject: [PATCH 07/10] update test --- tests/integration/pull_merge_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 171602c7cf064..6dadc62144002 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -601,7 +601,7 @@ func TestPullMergeIndexerNotifier(t *testing.T) { testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") createPullResp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "Indexer notifier test pull") - queue.GetManager().FlushAll(context.Background(), 5*time.Second) + queue.GetManager().FlushAll(context.Background(), 0) repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ OwnerName: "user2", @@ -621,8 +621,8 @@ func TestPullMergeIndexerNotifier(t *testing.T) { query.Set("type", "pulls") query.Set("q", "notifier") link.RawQuery = query.Encode() - req := NewRequest(t, "GET", link.String()) - searchIssuesResp := session.MakeRequest(t, req, http.StatusOK) + searchIssueReq := NewRequest(t, "GET", link.String()) + searchIssuesResp := session.MakeRequest(t, searchIssueReq, http.StatusOK) var apiIssuesBefore []*api.Issue DecodeJSON(t, searchIssuesResp, &apiIssuesBefore) if assert.Len(t, apiIssuesBefore, 1) { @@ -634,10 +634,10 @@ func TestPullMergeIndexerNotifier(t *testing.T) { assert.EqualValues(t, "pulls", elem[3]) testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false) - queue.GetManager().FlushAll(context.Background(), 5*time.Second) + queue.GetManager().FlushAll(context.Background(), 0) // search issues again - searchIssuesResp = session.MakeRequest(t, req, http.StatusOK) + searchIssuesResp = session.MakeRequest(t, searchIssueReq, http.StatusOK) var apiIssuesAfter []*api.Issue DecodeJSON(t, searchIssuesResp, &apiIssuesAfter) assert.Len(t, apiIssuesAfter, 0) From e9436f0b10aef4e01c0ebdb197a8746f9d80048d Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 29 Apr 2024 14:28:47 +0800 Subject: [PATCH 08/10] check if the issue is closed --- tests/integration/pull_merge_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 6dadc62144002..986b400958e78 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -634,6 +634,12 @@ func TestPullMergeIndexerNotifier(t *testing.T) { assert.EqualValues(t, "pulls", elem[3]) testPullMerge(t, session, elem[1], elem[2], elem[4], repo_model.MergeStyleMerge, false) + // check if the issue is closed + issue = unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ + ID: issue.ID, + }) + assert.True(t, issue.IsClosed) + queue.GetManager().FlushAll(context.Background(), 0) // search issues again From 0538400d22fc2dd4cc59b2e29cc6cac92b5bf67a Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Tue, 30 Apr 2024 15:08:41 +0800 Subject: [PATCH 09/10] test with closed state --- tests/integration/pull_merge_test.go | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 986b400958e78..db91ac4a80119 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -591,10 +591,6 @@ func TestPullDontRetargetChildOnWrongRepo(t *testing.T) { func TestPullMergeIndexerNotifier(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - hookTasks, err := webhook.HookTasks(db.DefaultContext, 1, 1) // Retrieve previous hook number - assert.NoError(t, err) - hookTasksLenBefore := len(hookTasks) - // create a pull request session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") @@ -617,17 +613,14 @@ func TestPullMergeIndexerNotifier(t *testing.T) { // search issues link, _ := url.Parse("/api/v1/repos/issues/search") query := url.Values{} - query.Set("state", "open") - query.Set("type", "pulls") - query.Set("q", "notifier") + query.Add("state", "closed") + query.Add("type", "pulls") + query.Add("q", "notifier") link.RawQuery = query.Encode() - searchIssueReq := NewRequest(t, "GET", link.String()) - searchIssuesResp := session.MakeRequest(t, searchIssueReq, http.StatusOK) + searchIssuesResp := session.MakeRequest(t, NewRequest(t, "GET", link.String()), http.StatusOK) var apiIssuesBefore []*api.Issue DecodeJSON(t, searchIssuesResp, &apiIssuesBefore) - if assert.Len(t, apiIssuesBefore, 1) { - assert.Equal(t, issue.ID, apiIssuesBefore[0].ID) - } + assert.Len(t, apiIssuesBefore, 0) // merge the pull request elem := strings.Split(test.RedirectURL(createPullResp), "/") @@ -643,13 +636,11 @@ func TestPullMergeIndexerNotifier(t *testing.T) { queue.GetManager().FlushAll(context.Background(), 0) // search issues again - searchIssuesResp = session.MakeRequest(t, searchIssueReq, http.StatusOK) + searchIssuesResp = session.MakeRequest(t, NewRequest(t, "GET", link.String()), http.StatusOK) var apiIssuesAfter []*api.Issue DecodeJSON(t, searchIssuesResp, &apiIssuesAfter) - assert.Len(t, apiIssuesAfter, 0) - - hookTasks, err = webhook.HookTasks(db.DefaultContext, 1, 1) - assert.NoError(t, err) - assert.Len(t, hookTasks, hookTasksLenBefore+1) + if assert.Len(t, apiIssuesAfter, 1) { + assert.Equal(t, issue.ID, apiIssuesAfter[0].ID) + } }) } From 17651f9807379a045ada4eb93575f0ec5953394d Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Wed, 8 May 2024 15:58:12 +0800 Subject: [PATCH 10/10] sleep 1s after flushing queues --- tests/integration/pull_merge_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index db91ac4a80119..826568caf2b4f 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -597,7 +597,8 @@ func TestPullMergeIndexerNotifier(t *testing.T) { testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") createPullResp := testPullCreate(t, session, "user1", "repo1", false, "master", "master", "Indexer notifier test pull") - queue.GetManager().FlushAll(context.Background(), 0) + assert.NoError(t, queue.GetManager().FlushAll(context.Background(), 0)) + time.Sleep(time.Second) repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ OwnerName: "user2", @@ -610,13 +611,15 @@ func TestPullMergeIndexerNotifier(t *testing.T) { IsClosed: false, }) - // search issues + // build the request for searching issues link, _ := url.Parse("/api/v1/repos/issues/search") query := url.Values{} query.Add("state", "closed") query.Add("type", "pulls") query.Add("q", "notifier") link.RawQuery = query.Encode() + + // search issues searchIssuesResp := session.MakeRequest(t, NewRequest(t, "GET", link.String()), http.StatusOK) var apiIssuesBefore []*api.Issue DecodeJSON(t, searchIssuesResp, &apiIssuesBefore) @@ -633,7 +636,8 @@ func TestPullMergeIndexerNotifier(t *testing.T) { }) assert.True(t, issue.IsClosed) - queue.GetManager().FlushAll(context.Background(), 0) + assert.NoError(t, queue.GetManager().FlushAll(context.Background(), 0)) + time.Sleep(time.Second) // search issues again searchIssuesResp = session.MakeRequest(t, NewRequest(t, "GET", link.String()), http.StatusOK)