Fix CODEOWNERS review request attribution using comment metadata (#36348)

Fixes #36333

## Problem

When CODEOWNERS automatically assigns reviewers to a pull request, the
timeline incorrectly shows the PR author as the one who requested the
review (e.g., "PR_AUTHOR requested review from CODE_OWNER"). This is
misleading since the action was triggered automatically by CODEOWNERS
rules, not by the PR author.

## Solution

Store CODEOWNERS attribution in comment metadata instead of changing the
doer user:
- Add `SpecialDoerName` field to `CommentMetaData` struct (value:
`"CODEOWNERS"` for CODEOWNERS-triggered requests)
- Pass `isCodeOwners=true` to `AddReviewRequest` and
`AddTeamReviewRequest` functions
- Template can check this metadata to show appropriate attribution
message

---------

Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/issues/comment.go

@@ -20,6 +20,7 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/container"
+	"code.gitea.io/gitea/modules/htmlutil"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/references"
@@ -233,11 +234,17 @@ func (r RoleInRepo) LocaleHelper(lang translation.Locale) string {
 	return lang.TrString("repo.issues.role." + string(r) + "_helper")
 }
 
+type SpecialDoerNameType string
+
+const SpecialDoerNameCodeOwners SpecialDoerNameType = "CODEOWNERS"
+
 // CommentMetaData stores metadata for a comment, these data will not be changed once inserted into database
 type CommentMetaData struct {
 	ProjectColumnID    int64  `json:"project_column_id,omitempty"`
 	ProjectColumnTitle string `json:"project_column_title,omitempty"`
 	ProjectTitle       string `json:"project_title,omitempty"`
+
+	SpecialDoerName SpecialDoerNameType `json:"special_doer_name,omitempty"` // e.g. "CODEOWNERS" for CODEOWNERS-triggered review requests
 }
 
 // Comment represents a comment in commit and issue page.
@@ -764,6 +771,37 @@ func (c *Comment) CodeCommentLink(ctx context.Context) string {
 	return fmt.Sprintf("%s/files#%s", c.Issue.Link(), c.HashTag())
 }
 
+func (c *Comment) MetaSpecialDoerTr(locale translation.Locale) template.HTML {
+	if c.CommentMetaData == nil {
+		return ""
+	}
+	if c.CommentMetaData.SpecialDoerName == SpecialDoerNameCodeOwners {
+		return locale.Tr("repo.issues.review.codeowners_rules")
+	}
+	return htmlutil.HTMLFormat("%s", c.CommentMetaData.SpecialDoerName)
+}
+
+func (c *Comment) TimelineRequestedReviewTr(locale translation.Locale, createdStr template.HTML) template.HTML {
+	if c.AssigneeID > 0 {
+		// it guarantees LoadAssigneeUserAndTeam has been called, and c.Assignee is Ghost user but not nil if the user doesn't exist
+		if c.RemovedAssignee {
+			if c.PosterID == c.AssigneeID {
+				return locale.Tr("repo.issues.review.remove_review_request_self", createdStr)
+			}
+			return locale.Tr("repo.issues.review.remove_review_request", c.Assignee.GetDisplayName(), createdStr)
+		}
+		return locale.Tr("repo.issues.review.add_review_request", c.Assignee.GetDisplayName(), createdStr)
+	}
+	teamName := "Ghost Team"
+	if c.AssigneeTeam != nil {
+		teamName = c.AssigneeTeam.Name
+	}
+	if c.RemovedAssignee {
+		return locale.Tr("repo.issues.review.remove_review_request", teamName, createdStr)
+	}
+	return locale.Tr("repo.issues.review.add_review_request", teamName, createdStr)
+}
+
 // CreateComment creates comment with context
 func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment, err error) {
 	return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) {
@@ -780,6 +818,11 @@ func CreateComment(ctx context.Context, opts *CreateCommentOptions) (_ *Comment,
 				ProjectTitle:       opts.ProjectTitle,
 			}
 		}
+		if opts.SpecialDoerName != "" {
+			commentMetaData = &CommentMetaData{
+				SpecialDoerName: opts.SpecialDoerName,
+			}
+		}
 
 		comment := &Comment{
 			Type:             opts.Type,
@@ -976,6 +1019,7 @@ type CreateCommentOptions struct {
 	RefIsPull          bool
 	IsForcePush        bool
 	Invalidated        bool
+	SpecialDoerName    SpecialDoerNameType // e.g. "CODEOWNERS" for CODEOWNERS-triggered review requests
 }
 
 // GetCommentByID returns the comment by given ID.

models/issues/pull_test.go

@@ -130,7 +130,7 @@ func TestLoadRequestedReviewers(t *testing.T) {
 	user1, err := user_model.GetUserByID(t.Context(), 1)
 	assert.NoError(t, err)
 
-	comment, err := issues_model.AddReviewRequest(t.Context(), issue, user1, &user_model.User{})
+	comment, err := issues_model.AddReviewRequest(t.Context(), issue, user1, &user_model.User{}, false)
 	assert.NoError(t, err)
 	assert.NotNil(t, comment)
 

models/issues/review.go

@@ -643,7 +643,7 @@ func InsertReviews(ctx context.Context, reviews []*Review) error {
 }
 
 // AddReviewRequest add a review request from one reviewer
-func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
+func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User, isCodeOwners bool) (*Comment, error) {
 	return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) {
 		sess := db.GetEngine(ctx)
 
@@ -702,6 +702,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 			RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 			AssigneeID:      reviewer.ID, // Use AssigneeID as reviewer ID
 			ReviewID:        review.ID,
+			SpecialDoerName: util.Iif(isCodeOwners, SpecialDoerNameCodeOwners, ""),
 		})
 		if err != nil {
 			return nil, err
@@ -767,7 +768,7 @@ func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64)
 }
 
 // AddTeamReviewRequest add a review request from one team
-func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
+func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User, isCodeOwners bool) (*Comment, error) {
 	return db.WithTx2(ctx, func(ctx context.Context) (*Comment, error) {
 		review, err := GetTeamReviewerByIssueIDAndTeamID(ctx, issue.ID, reviewer.ID)
 		if err != nil && !IsErrReviewNotExist(err) {
@@ -812,6 +813,7 @@ func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organizat
 			RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 			AssigneeTeamID:  reviewer.ID, // Use AssigneeTeamID as reviewer team ID
 			ReviewID:        review.ID,
+			SpecialDoerName: util.Iif(isCodeOwners, SpecialDoerNameCodeOwners, ""),
 		})
 		if err != nil {
 			return nil, fmt.Errorf("CreateComment(): %w", err)

models/issues/review_test.go

@@ -321,14 +321,28 @@ func TestAddReviewRequest(t *testing.T) {
 	pull.HasMerged = false
 	assert.NoError(t, pull.UpdateCols(t.Context(), "has_merged"))
 	issue.IsClosed = true
-	_, err = issues_model.AddReviewRequest(t.Context(), issue, reviewer, &user_model.User{})
+	_, err = issues_model.AddReviewRequest(t.Context(), issue, reviewer, &user_model.User{}, false)
 	assert.Error(t, err)
 	assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err))
 
 	pull.HasMerged = true
 	assert.NoError(t, pull.UpdateCols(t.Context(), "has_merged"))
 	issue.IsClosed = false
-	_, err = issues_model.AddReviewRequest(t.Context(), issue, reviewer, &user_model.User{})
+	_, err = issues_model.AddReviewRequest(t.Context(), issue, reviewer, &user_model.User{}, false)
 	assert.Error(t, err)
 	assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err))
+
+	// Test CODEOWNERS review request stores metadata correctly
+	pull2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})
+	assert.NoError(t, pull2.LoadIssue(t.Context()))
+	issue2 := pull2.Issue
+	assert.NoError(t, issue2.LoadRepo(t.Context()))
+	reviewer2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 7})
+	doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	comment, err := issues_model.AddReviewRequest(t.Context(), issue2, reviewer2, doer, true)
+	assert.NoError(t, err)
+	assert.NotNil(t, comment)
+	assert.NotNil(t, comment.CommentMetaData)
+	assert.Equal(t, issues_model.SpecialDoerNameCodeOwners, comment.CommentMetaData.SpecialDoerName)
 }

options/locale/locale_en-US.json

@@ -1702,6 +1702,7 @@
   "repo.issues.review.content.empty": "You need to leave a comment indicating the requested change(s).",
   "repo.issues.review.reject": "requested changes %s",
   "repo.issues.review.wait": "was requested for review %s",
+  "repo.issues.review.codeowners_rules": "CODEOWNERS rules",
   "repo.issues.review.add_review_request": "requested review from %s %s",
   "repo.issues.review.remove_review_request": "removed review request for %s %s",
   "repo.issues.review.remove_review_request_self": "declined to review %s",

services/issue/assignee.go

@@ -70,7 +70,7 @@ func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_mo
 	}
 
 	if isAdd {
-		comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer)
+		comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer, false)
 	} else {
 		comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer)
 	}
@@ -224,7 +224,7 @@ func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *use
 		return nil, err
 	}
 	if isAdd {
-		comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer)
+		comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer, false)
 	} else {
 		comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer)
 	}

services/issue/pull.go

@@ -149,7 +149,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque
 
 	for _, u := range uniqUsers {
 		if u.ID != issue.Poster.ID && !contain(latestReivews, u) {
-			comment, err := issues_model.AddReviewRequest(ctx, issue, u, issue.Poster)
+			comment, err := issues_model.AddReviewRequest(ctx, issue, u, issue.Poster, true)
 			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
@@ -166,7 +166,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque
 	}
 
 	for _, t := range uniqTeams {
-		comment, err := issues_model.AddTeamReviewRequest(ctx, issue, t, issue.Poster)
+		comment, err := issues_model.AddTeamReviewRequest(ctx, issue, t, issue.Poster, true)
 		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

templates/repo/graph/commits.tmpl

@@ -40,14 +40,14 @@
 						{{end}}
 					</span>
 
-					<span class="author flex-text-inline">
+					<span class="flex-text-inline tw-text-12">
 						{{$userName := $commit.Commit.Author.Name}}
 						{{if $commit.User}}
 							{{if and $commit.User.FullName DefaultShowFullName}}
 								{{$userName = $commit.User.FullName}}
 							{{end}}
 							{{ctx.AvatarUtils.Avatar $commit.User 18}}
-							<a href="{{$commit.User.HomeLink}}">{{$userName}}</a>
+							<a class="muted" href="{{$commit.User.HomeLink}}">{{$userName}}</a>
 						{{else}}
 							{{ctx.AvatarUtils.AvatarByEmail $commit.Commit.Author.Email $userName 18}}
 							{{$userName}}

templates/repo/issue/view_content/comments.tmpl

@@ -514,32 +514,20 @@
 		{{else if eq .Type 27}}
 			<div class="timeline-item event" id="{{.HashTag}}">
 				<span class="badge">{{svg "octicon-eye"}}</span>
-				{{template "shared/user/avatarlink" dict "user" .Poster}}
+				{{$specialDoerHtml := .MetaSpecialDoerTr ctx.Locale}}
-				<span class="comment-text-line">
+				{{$timelineRequestedReviewHtml := .TimelineRequestedReviewTr ctx.Locale $createdStr}}
-					{{template "shared/user/authorlink" .Poster}}
+				{{if $specialDoerHtml}}
-					{{if (gt .AssigneeID 0)}}
+					<span class="comment-text-line">
-						{{if .RemovedAssignee}}
+						{{$specialDoerHtml}}
-							{{if eq .PosterID .AssigneeID}}
+						{{$timelineRequestedReviewHtml}}
-								{{ctx.Locale.Tr "repo.issues.review.remove_review_request_self" $createdStr}}
+					</span>
-							{{else}}
+				{{else}}
-								{{ctx.Locale.Tr "repo.issues.review.remove_review_request" .Assignee.GetDisplayName $createdStr}}
+					{{template "shared/user/avatarlink" dict "user" .Poster}}
-							{{end}}
+					<span class="comment-text-line">
-						{{else}}
+						{{template "shared/user/authorlink" .Poster}}
-							{{ctx.Locale.Tr "repo.issues.review.add_review_request" .Assignee.GetDisplayName $createdStr}}
+						{{$timelineRequestedReviewHtml}}
-						{{end}}
+					</span>
-					{{else}}
+				{{end}}
-						<!-- If the assigned team is deleted, just displaying "Ghost Team" in the comment -->
-						{{$teamName := "Ghost Team"}}
-						{{if .AssigneeTeam}}
-							{{$teamName = .AssigneeTeam.Name}}
-						{{end}}
-						{{if .RemovedAssignee}}
-							{{ctx.Locale.Tr "repo.issues.review.remove_review_request" $teamName $createdStr}}
-						{{else}}
-							{{ctx.Locale.Tr "repo.issues.review.add_review_request" $teamName $createdStr}}
-						{{end}}
-					{{end}}
-				</span>
 			</div>
 		{{else if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}}
 			<!-- If PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered. //-->

templates/shared/user/authorlink.tmpl

@@ -1 +1 @@
-<a class="author text black tw-font-semibold muted"{{if gt .ID 0}} href="{{.HomeLink}}"{{end}}>{{.GetDisplayName}}</a>{{if .IsTypeBot}}<span class="ui basic label tw-p-1 tw-align-baseline">bot</span>{{end}}
+<a class="muted text black tw-font-semibold"{{if gt .ID 0}} href="{{.HomeLink}}"{{end}}>{{.GetDisplayName}}</a>{{if .IsTypeBot}}<span class="ui basic label tw-p-1 tw-align-baseline">bot</span>{{end}}

templates/shared/user/avatarlink.tmpl

@@ -1 +1 @@
-<a class="avatar-with-link" {{if gt .user.ID 0}}href="{{.user.HomeLink}}"{{end}}>{{ctx.AvatarUtils.Avatar .user}}</a>
+<a class="avatar-with-link" {{if gt .user.ID 0}}href="{{.user.HomeLink}}"{{end}}>{{ctx.AvatarUtils.Avatar .user (or .size 20)}}</a>

web_src/css/base.css

@@ -469,19 +469,6 @@ a.label,
   box-shadow: 1px 1px 0 0 var(--color-secondary);
 }
 
-.ui.comments .comment .text {
-  margin: 0;
-}
-
-.ui.comments .comment .text,
-.ui.comments .comment .author {
-  color: var(--color-text);
-}
-
-.ui.comments .comment a.author:hover {
-  color: var(--color-primary);
-}
-
 .ui.comments .comment .metadata {
   color: var(--color-text-light-2);
 }
@@ -562,6 +549,7 @@ img.ui.avatar,
   color: var(--color-purple) !important;
 }
 
+/* it is different from tw-text-black: this one changes in dark theme */
 .text.black {
   color: var(--color-text) !important;
 }

web_src/css/features/gitgraph.css

@@ -46,10 +46,6 @@
   height: 20px;
 }
 
-#git-graph-container li .author {
-  color: var(--color-text-light);
-}
-
 #git-graph-container li .time {
   color: var(--color-text-light-3);
   font-size: 80%;

web_src/css/modules/comment.css

@@ -56,15 +56,6 @@
   min-width: 0;
 }
 
-.ui.comments .comment .author {
-  font-size: 1em;
-  font-weight: var(--font-weight-medium);
-}
-
-.ui.comments .comment a.author {
-  cursor: pointer;
-}
-
 .ui.comments .comment .metadata {
   display: inline-block;
   margin-left: 0.5em;