From 1eaf229ff01d20b10bb1a85580109e87eca3c6ea Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Mar 2024 17:23:12 +0800 Subject: [PATCH 1/5] Fix bug when remove wip title --- services/issue/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/issue/pull.go b/services/issue/pull.go index 8e85c11e9b873..6c28c1292b0ce 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -90,7 +90,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 } From 58ec62c249021547d479a3718a53f8d474b44791 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Mar 2024 18:14:54 +0800 Subject: [PATCH 2/5] Add test for change wip title --- tests/integration/pull_review_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index bdfecb32805c4..2cf42c44542f9 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -4,6 +4,7 @@ package integration import ( + "fmt" "net/http" "net/url" "strings" @@ -89,6 +90,23 @@ func TestPullView_CodeOwner(t *testing.T) { pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) + + prURL := fmt.Sprintf("/user2/test_codeowner/pulls/%d", pr.Index) + req := NewRequest(t, "GET", prURL) + resp := session.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + req = NewRequestWithValues(t, "POST", prURL+"/title", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "title": "[WIP] Test Pull Request", + }) + session.MakeRequest(t, req, http.StatusOK) + + req = NewRequestWithValues(t, "POST", prURL+"/title", map[string]string{ + "_csrf": htmlDoc.GetCSRF(), + "title": "Test Pull Request", + }) + session.MakeRequest(t, req, http.StatusOK) }) // change the default branch CODEOWNERS file to change README.md's codeowner From 6d424210ca680ee74cf44e8a96d0bc5fba1d1006 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Mar 2024 18:33:02 +0800 Subject: [PATCH 3/5] Add more tests --- tests/integration/pull_review_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 2cf42c44542f9..df85e9754c5bf 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -101,12 +101,18 @@ func TestPullView_CodeOwner(t *testing.T) { "title": "[WIP] Test Pull Request", }) session.MakeRequest(t, req, http.StatusOK) + prUpdated1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) + assert.NoError(t, prUpdated1.LoadIssue(db.DefaultContext)) + assert.EqualValues(t, "[WIP] Test Pull Request", prUpdated1.Issue.Title) req = NewRequestWithValues(t, "POST", prURL+"/title", map[string]string{ "_csrf": htmlDoc.GetCSRF(), "title": "Test Pull Request", }) session.MakeRequest(t, req, http.StatusOK) + prUpdated2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) + assert.NoError(t, prUpdated2.LoadIssue(db.DefaultContext)) + assert.EqualValues(t, "Test Pull Request", prUpdated2.Issue.Title) }) // change the default branch CODEOWNERS file to change README.md's codeowner From 0ddbab33156c7aa3783cd0469f383941647150ae Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Mar 2024 09:54:03 +0800 Subject: [PATCH 4/5] Update test --- services/issue/pull.go | 14 ++++++++----- tests/integration/pull_review_test.go | 30 +++++++++------------------ 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/services/issue/pull.go b/services/issue/pull.go index 6c28c1292b0ce..b7b63a70246a8 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -40,7 +40,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) { @@ -112,9 +112,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 @@ -122,12 +126,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 df85e9754c5bf..9a5877697c0ba 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -4,7 +4,6 @@ package integration import ( - "fmt" "net/http" "net/url" "strings" @@ -16,6 +15,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "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" @@ -88,31 +88,21 @@ func TestPullView_CodeOwner(t *testing.T) { session := loginUser(t, "user2") testPullCreate(t, session, "user2", "test_codeowner", false, 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)) - prURL := fmt.Sprintf("/user2/test_codeowner/pulls/%d", pr.Index) - req := NewRequest(t, "GET", prURL) - resp := session.MakeRequest(t, req, http.StatusOK) - htmlDoc := NewHTMLParser(t, resp.Body) - - req = NewRequestWithValues(t, "POST", prURL+"/title", map[string]string{ - "_csrf": htmlDoc.GetCSRF(), - "title": "[WIP] Test Pull Request", - }) - session.MakeRequest(t, req, http.StatusOK) - prUpdated1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) + 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) - req = NewRequestWithValues(t, "POST", prURL+"/title", map[string]string{ - "_csrf": htmlDoc.GetCSRF(), - "title": "Test Pull Request", - }) - session.MakeRequest(t, req, http.StatusOK) - prUpdated2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"}) + 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 Request", prUpdated2.Issue.Title) + assert.EqualValues(t, "Test Pull Request2", prUpdated2.Issue.Title) }) // change the default branch CODEOWNERS file to change README.md's codeowner From 02833022b2569e9970b2f6dff22682f8308ea94f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Mar 2024 15:20:53 +0800 Subject: [PATCH 5/5] request codeowner reviewer failure should not prevent change the title --- services/issue/issue.go | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/services/issue/issue.go b/services/issue/issue.go index 94b0ee6f693eb..c7fa9f3300a5e 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -17,6 +17,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" "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" ) @@ -85,23 +86,17 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode } } - 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)