From 003881b06ddf59430b603b9b06854f2f710d1351 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Tue, 19 Mar 2024 06:28:43 +0100 Subject: [PATCH] [Port] gitea#29842: Notify reviewers added via CODEOWNERS Fixes https://github.com/go-gitea/gitea/issues/28297 This PR also fixed a problem that it needs a database transaction when removing the WIP title. --- Resolves #2771 Also partially ports gitea#29783 (cherry picked from commit 17d7ab5ad4ce3d0fbc1251572c22687c237a30b1) --- models/issues/pull.go | 87 ---------------- services/issue/assignee.go | 23 ++++- services/issue/issue.go | 24 +++-- services/issue/pull.go | 143 ++++++++++++++++++++++++++ services/pull/pull.go | 7 +- tests/integration/pull_review_test.go | 130 ++++++++++++++++++++++- 6 files changed, 310 insertions(+), 104 deletions(-) create mode 100644 services/issue/pull.go diff --git a/models/issues/pull.go b/models/issues/pull.go index f1baa9b5e5..dc1b1b956a 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -19,7 +19,6 @@ import ( repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" @@ -884,92 +883,6 @@ func MergeBlockedByOutdatedBranch(protectBranch *git_model.ProtectedBranch, pr * return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0 } -func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullRequest) error { - files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} - - if pr.IsWorkInProgress(ctx) { - return nil - } - - if err := pull.LoadRepo(ctx); err != nil { - return err - } - - if pull.Repo.IsFork { - return nil - } - - if err := pr.LoadBaseRepo(ctx); err != nil { - return err - } - - repo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - return err - } - defer repo.Close() - - commit, err := repo.GetBranchCommit(pr.BaseRepo.DefaultBranch) - if err != nil { - return err - } - - var data string - for _, file := range files { - if blob, err := commit.GetBlobByPath(file); err == nil { - data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) - if err == nil { - break - } - } - } - - rules, _ := GetCodeOwnersFromContent(ctx, data) - - prInfo, err := repo.GetCompareInfo(repo.Path, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) - if err != nil { - return err - } - // Use the merge base as the base instead of the main branch to avoid problems - // if the pull request is out of date with the base branch. - changedFiles, err := repo.GetFilesChangedBetween(prInfo.MergeBase, prInfo.HeadCommitID) - if err != nil { - return err - } - - uniqUsers := make(map[int64]*user_model.User) - uniqTeams := make(map[string]*org_model.Team) - for _, rule := range rules { - for _, f := range changedFiles { - if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) { - for _, u := range rule.Users { - uniqUsers[u.ID] = u - } - for _, t := range rule.Teams { - uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t - } - } - } - } - - for _, u := range uniqUsers { - if u.ID != pull.Poster.ID { - if _, err := AddReviewRequest(ctx, pull, u, pull.Poster); 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 err - } - } - } - for _, t := range uniqTeams { - if _, err := AddTeamReviewRequest(ctx, pull, t, pull.Poster); 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 err - } - } - - return nil -} - // GetCodeOwnersFromContent returns the code owners configuration // Return empty slice if files missing // Return warning messages on parsing errors diff --git a/services/issue/assignee.go b/services/issue/assignee.go index b5f472ba53..8740a6664a 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -226,16 +226,33 @@ func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *use return nil, nil } + return comment, teamReviewRequestNotify(ctx, issue, doer, reviewer, isAdd, comment) +} + +func ReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewNotifers []*ReviewRequestNotifier) { + for _, reviewNotifer := range reviewNotifers { + if reviewNotifer.Reviwer != nil { + notify_service.PullRequestReviewRequest(ctx, issue.Poster, issue, reviewNotifer.Reviwer, reviewNotifer.IsAdd, reviewNotifer.Comment) + } else if reviewNotifer.ReviewTeam != nil { + if err := teamReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifer.ReviewTeam, reviewNotifer.IsAdd, reviewNotifer.Comment); err != nil { + log.Error("teamReviewRequestNotify: %v", err) + } + } + } +} + +// teamReviewRequestNotify notify all user in this team +func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool, comment *issues_model.Comment) error { // notify all user in this team if err := comment.LoadIssue(ctx); err != nil { - return nil, err + return err } members, err := organization.GetTeamMembers(ctx, &organization.SearchMembersOptions{ TeamID: reviewer.ID, }) if err != nil { - return nil, err + return err } for _, member := range members { @@ -246,7 +263,7 @@ func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *use notify_service.PullRequestReviewRequest(ctx, doer, issue, member, isAdd, comment) } - return comment, err + return err } // CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR diff --git a/services/issue/issue.go b/services/issue/issue.go index 627c6d4bce..995015da25 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -64,17 +64,27 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode return nil } - if err := issues_model.ChangeIssueTitle(ctx, issue, doer, oldTitle); err != nil { + 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 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 + } + } + return nil + }); err != nil { return err } - if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) { - if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest); err != nil { - return err - } - } - notify_service.IssueChangeTitle(ctx, doer, issue, oldTitle) + ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifers) return nil } diff --git a/services/issue/pull.go b/services/issue/pull.go new file mode 100644 index 0000000000..8e85c11e9b --- /dev/null +++ b/services/issue/pull.go @@ -0,0 +1,143 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package issue + +import ( + "context" + "fmt" + "time" + + issues_model "code.gitea.io/gitea/models/issues" + org_model "code.gitea.io/gitea/models/organization" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" +) + +func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) { + // Add a temporary remote + tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano()) + if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil { + return "", fmt.Errorf("AddRemote: %w", err) + } + defer func() { + if err := repo.RemoveRemote(tmpRemote); err != nil { + log.Error("getMergeBase: RemoveRemote: %v", err) + } + }() + + mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch) + return mergeBase, err +} + +type ReviewRequestNotifier struct { + Comment *issues_model.Comment + IsAdd bool + Reviwer *user_model.User + ReviewTeam *org_model.Team +} + +func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) { + files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"} + + if pr.IsWorkInProgress(ctx) { + return nil, nil + } + + if err := pr.LoadHeadRepo(ctx); err != nil { + return nil, err + } + + if pr.HeadRepo.IsFork { + return nil, nil + } + + if err := pr.LoadBaseRepo(ctx); err != nil { + return nil, err + } + + repo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo) + if err != nil { + return nil, err + } + defer repo.Close() + + commit, err := repo.GetBranchCommit(pr.BaseRepo.DefaultBranch) + if err != nil { + return nil, err + } + + var data string + for _, file := range files { + if blob, err := commit.GetBlobByPath(file); err == nil { + data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize) + if err == nil { + break + } + } + } + + rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data) + + // get the mergebase + mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName()) + if err != nil { + return nil, err + } + + // 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) + if err != nil { + return nil, err + } + + uniqUsers := make(map[int64]*user_model.User) + uniqTeams := make(map[string]*org_model.Team) + for _, rule := range rules { + for _, f := range changedFiles { + if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) { + for _, u := range rule.Users { + uniqUsers[u.ID] = u + } + for _, t := range rule.Teams { + uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t + } + } + } + } + + notifiers := make([]*ReviewRequestNotifier, 0, len(uniqUsers)+len(uniqTeams)) + + for _, u := range uniqUsers { + if u.ID != pull.Poster.ID { + comment, err := issues_model.AddReviewRequest(ctx, pull, u, pull.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 + } + notifiers = append(notifiers, &ReviewRequestNotifier{ + Comment: comment, + IsAdd: true, + Reviwer: pull.Poster, + }) + } + } + for _, t := range uniqTeams { + comment, err := issues_model.AddTeamReviewRequest(ctx, pull, t, pull.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 + } + notifiers = append(notifiers, &ReviewRequestNotifier{ + Comment: comment, + IsAdd: true, + ReviewTeam: t, + }) + } + + return notifiers, nil +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 5d597b7a77..fb4b88309a 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -74,6 +74,7 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss } defer baseGitRepo.Close() + var reviewNotifers []*issue_service.ReviewRequestNotifier if err := db.WithTx(ctx, func(ctx context.Context) error { if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil { return err @@ -133,7 +134,8 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss } if !pr.IsWorkInProgress(ctx) { - if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil { + reviewNotifers, err = issue_service.PullRequestCodeOwnersReview(ctx, issue, pr) + if err != nil { return err } } @@ -147,11 +149,12 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss } baseGitRepo.Close() // close immediately to avoid notifications will open the repository again + issue_service.ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifers) + mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) if err != nil { return err } - notify_service.NewPullRequest(ctx, pr, mentions) if len(issue.Labels) > 0 { notify_service.IssueChangeLabels(ctx, issue.Poster, issue, issue.Labels, nil) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 2daba05ddf..1400ad1d73 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -6,11 +6,18 @@ package integration import ( "context" "net/http" + "net/url" "strconv" + "strings" "testing" - "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + repo_service "code.gitea.io/gitea/services/repository" + files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" "github.com/PuerkitoBio/goquery" @@ -28,11 +35,11 @@ func TestPullView_ReviewerMissed(t *testing.T) { session.MakeRequest(t, req, http.StatusOK) } -func loadComment(t *testing.T, commentID string) *issues.Comment { +func loadComment(t *testing.T, commentID string) *issues_model.Comment { t.Helper() id, err := strconv.ParseInt(commentID, 10, 64) assert.NoError(t, err) - return unittest.AssertExistsAndLoadBean(t, &issues.Comment{ID: id}) + return unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: id}) } func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { @@ -81,7 +88,7 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { // (to invalidate it properly, one should push a commit which should trigger this logic, // in the meantime, use this quick-and-dirty trick) comment := loadComment(t, commentID) - assert.NoError(t, issues.UpdateCommentInvalidate(context.Background(), &issues.Comment{ + assert.NoError(t, issues_model.UpdateCommentInvalidate(context.Background(), &issues_model.Comment{ ID: comment.ID, Invalidated: true, })) @@ -143,7 +150,7 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { // (to invalidate it properly, one should push a commit which should trigger this logic, // in the meantime, use this quick-and-dirty trick) comment := loadComment(t, commentID) - assert.NoError(t, issues.UpdateCommentInvalidate(context.Background(), &issues.Comment{ + assert.NoError(t, issues_model.UpdateCommentInvalidate(context.Background(), &issues_model.Comment{ ID: comment.ID, Invalidated: true, })) @@ -250,3 +257,116 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { assert.Len(t, comments.Nodes, 3) // 1 comment on line 1 + 2 comments on line 3 }) } + +func TestPullView_CodeOwner(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + // Create the repo. + repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{ + Name: "test_codeowner", + Readme: "Default", + AutoInit: true, + ObjectFormatName: git.Sha1ObjectFormat.Name(), + DefaultBranch: "master", + }) + assert.NoError(t, err) + + // add CODEOWNERS to default branch + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + OldBranch: repo.DefaultBranch, + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "CODEOWNERS", + ContentReader: strings.NewReader("README.md @user5\n"), + }, + }, + }) + assert.NoError(t, err) + + t.Run("First Pull Request", func(t *testing.T) { + // create a new branch to prepare for pull request + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + NewBranch: "codeowner-basebranch", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("# This is a new project\n"), + }, + }, + }) + assert.NoError(t, err) + + // Create a pull request. + 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"}) + unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5}) + }) + + // change the default branch CODEOWNERS file to change README.md's codeowner + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "CODEOWNERS", + ContentReader: strings.NewReader("README.md @user8\n"), + }, + }, + }) + assert.NoError(t, err) + + t.Run("Second Pull Request", func(t *testing.T) { + // create a new branch to prepare for pull request + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + NewBranch: "codeowner-basebranch2", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("# This is a new project2\n"), + }, + }, + }) + assert.NoError(t, err) + + // Create a pull request. + session := loginUser(t, "user2") + testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch2", "Test Pull Request2") + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"}) + unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + }) + + t.Run("Forked Repo Pull Request", func(t *testing.T) { + user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + forkedRepo, err := repo_service.ForkRepository(db.DefaultContext, user2, user5, repo_service.ForkRepoOptions{ + BaseRepo: repo, + Name: "test_codeowner_fork", + }) + assert.NoError(t, err) + + // create a new branch to prepare for pull request + _, err = files_service.ChangeRepoFiles(db.DefaultContext, forkedRepo, user5, &files_service.ChangeRepoFilesOptions{ + NewBranch: "codeowner-basebranch-forked", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("# This is a new forked project\n"), + }, + }, + }) + assert.NoError(t, err) + + session := loginUser(t, "user5") + testPullCreate(t, session, "user5", "test_codeowner_fork", false, forkedRepo.DefaultBranch, "codeowner-basebranch-forked", "Test Pull Request2") + + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch-forked"}) + unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) + }) + }) +}