From 087aed70969dd122e7292be52115a819c0c3f5fe Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Mar 2024 17:26:05 +0800 Subject: [PATCH] Fix Add/Remove WIP on pull request title failure (#29999) (#30066) Fix #29997 Backport #29999 --- services/issue/issue.go | 25 ++++++++++--------------- services/issue/pull.go | 16 ++++++++++------ tests/integration/pull_review_test.go | 16 +++++++++++++++- 3 files changed, 35 insertions(+), 22 deletions(-) diff --git a/services/issue/issue.go b/services/issue/issue.go index 81fa980b6b1cb..56ef627b2b4e7 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -16,6 +16,7 @@ import ( system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/storage" notify_service "code.gitea.io/gitea/services/notify" ) @@ -70,23 +71,17 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode return err } - var reviewNotifers []*ReviewRequestNotifier - - if err := db.WithTx(ctx, func(ctx context.Context) error { - if err := issues_model.ChangeIssueTitle(ctx, issue, doer, oldTitle); err != nil { - return err - } + if err := issues_model.ChangeIssueTitle(ctx, issue, doer, oldTitle); err != nil { + return err + } - if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) { - var err error - reviewNotifers, err = PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest) - if err != nil { - return err - } + var reviewNotifers []*ReviewRequestNotifier + if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) { + var err error + reviewNotifers, err = PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest) + if err != nil { + log.Error("PullRequestCodeOwnersReview: %v", err) } - return nil - }); err != nil { - return err } notify_service.IssueChangeTitle(ctx, doer, issue, oldTitle) diff --git a/services/issue/pull.go b/services/issue/pull.go index 2bb8cbbc9ebd3..99c5a94a3f512 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -39,7 +39,7 @@ type ReviewRequestNotifier struct { ReviewTeam *org_model.Team } -func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { +func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} if pr.IsWorkInProgress(ctx) { @@ -89,7 +89,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, // https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed // between the merge base and the head commit but not the base branch and the head commit - changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.HeadCommitID) + changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitRefName()) if err != nil { return nil, err } @@ -111,9 +111,13 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, notifiers := make([]*ReviewRequestNotifier, 0, len(uniqUsers)+len(uniqTeams)) + if err := issue.LoadPoster(ctx); err != nil { + return nil, err + } + for _, u := range uniqUsers { - if u.ID != pull.Poster.ID { - comment, err := issues_model.AddReviewRequest(ctx, pull, u, pull.Poster) + if u.ID != issue.Poster.ID { + comment, err := issues_model.AddReviewRequest(ctx, issue, u, issue.Poster) if err != nil { log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err) return nil, err @@ -121,12 +125,12 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, notifiers = append(notifiers, &ReviewRequestNotifier{ Comment: comment, IsAdd: true, - Reviwer: pull.Poster, + Reviwer: u, }) } } for _, t := range uniqTeams { - comment, err := issues_model.AddTeamReviewRequest(ctx, pull, t, pull.Poster) + comment, err := issues_model.AddTeamReviewRequest(ctx, issue, t, issue.Poster) if err != nil { log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err) return nil, err diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index c52fce353e75f..64a74fb103860 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -14,6 +14,7 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/test" + issue_service "code.gitea.io/gitea/services/issue" repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" @@ -85,8 +86,21 @@ func TestPullView_CodeOwner(t *testing.T) { session := loginUser(t, "user2") createPullRequest(t, session, "user2", "test_codeowner", repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request") - pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) + assert.NoError(t, pr.LoadIssue(db.DefaultContext)) + + err := issue_service.ChangeTitle(db.DefaultContext, pr.Issue, user2, "[WIP] Test Pull Request") + assert.NoError(t, err) + prUpdated1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + assert.NoError(t, prUpdated1.LoadIssue(db.DefaultContext)) + assert.EqualValues(t, "[WIP] Test Pull Request", prUpdated1.Issue.Title) + + err = issue_service.ChangeTitle(db.DefaultContext, prUpdated1.Issue, user2, "Test Pull Request2") + assert.NoError(t, err) + prUpdated2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + assert.NoError(t, prUpdated2.LoadIssue(db.DefaultContext)) + assert.EqualValues(t, "Test Pull Request2", prUpdated2.Issue.Title) }) // change the default branch CODEOWNERS file to change README.md's codeowner