Merge pull request '[Port] gitea#29842: Notify reviewers added via CODEOWNERS' (#2855) from gusted/forgejo-port-29842 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2855
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

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

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

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
 }

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
+}

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)

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})
+		})
+	})
+}