feat(ui): add links to review request targets in issue comments (#8239)

- Add links to review request targets in issue comments
- Fix links to ghost users/orgs/teams to be empty

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8239
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Robert Wolff <mahlzahn@posteo.de>
Co-committed-by: Robert Wolff <mahlzahn@posteo.de>

models/issues/action_aggregator.go

@@ -4,6 +4,7 @@
 package issues
 
 import (
+	"context"
 	"slices"
 
 	"forgejo.org/models/organization"
@@ -374,3 +375,10 @@ func (t *RequestReviewTarget) Type() string {
 	}
 	return "team"
 }
+
+func (t *RequestReviewTarget) Link(ctx context.Context) string {
+	if t.User != nil {
+		return t.User.HomeLink()
+	}
+	return t.Team.Link(ctx)
+}

models/issues/action_aggregator_test.go

@@ -0,0 +1,37 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package issues
+
+import (
+	"testing"
+
+	"forgejo.org/models/db"
+	org_model "forgejo.org/models/organization"
+	"forgejo.org/models/unittest"
+	user_model "forgejo.org/models/user"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRequestReviewTarget(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	target := RequestReviewTarget{User: &user_model.User{ID: 1, Name: "user1"}}
+	assert.Equal(t, int64(1), target.ID())
+	assert.Equal(t, "user1", target.Name())
+	assert.Equal(t, "user", target.Type())
+	assert.Equal(t, "/user1", target.Link(db.DefaultContext))
+
+	target = RequestReviewTarget{Team: &org_model.Team{ID: 2, Name: "Collaborators", OrgID: 3}}
+	assert.Equal(t, int64(2), target.ID())
+	assert.Equal(t, "Collaborators", target.Name())
+	assert.Equal(t, "team", target.Type())
+	assert.Equal(t, "/org/org3/teams/Collaborators", target.Link(db.DefaultContext))
+
+	target = RequestReviewTarget{Team: org_model.NewGhostTeam()}
+	assert.Equal(t, int64(-1), target.ID())
+	assert.Equal(t, "Ghost team", target.Name())
+	assert.Equal(t, "team", target.Type())
+	assert.Empty(t, target.Link(db.DefaultContext))
+}

models/organization/org.go

@@ -186,6 +186,11 @@ func (org *Organization) CanCreateRepo() bool {
 	return org.AsUser().CanCreateRepo()
 }
 
+// IsGhost returns if the organization is a ghost
+func (org *Organization) IsGhost() bool {
+	return org.AsUser().IsGhost()
+}
+
 // FindOrgMembersOpts represensts find org members conditions
 type FindOrgMembersOpts struct {
 	db.ListOptions

models/organization/team.go

@@ -1,5 +1,6 @@
-// Copyright 2018 The Gitea Authors. All rights reserved.
 // Copyright 2016 The Gogs Authors. All rights reserved.
+// Copyright 2018 The Gitea Authors. All rights reserved.
+// Copyright 2025 The Forgejo Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package organization
@@ -7,6 +8,7 @@ package organization
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"strings"
 
 	"forgejo.org/models/db"
@@ -20,13 +22,6 @@ import (
 	"xorm.io/builder"
 )
 
-// ___________
-// \__    ___/___ _____    _____
-//   |    |_/ __ \\__  \  /     \
-//   |    |\  ___/ / __ \|  Y Y  \
-//   |____| \___  >____  /__|_|  /
-//              \/     \/      \/
-
 // ErrTeamAlreadyExist represents a "TeamAlreadyExist" kind of error.
 type ErrTeamAlreadyExist struct {
 	OrgID int64
@@ -193,6 +188,27 @@ func (t *Team) UnitAccessMode(ctx context.Context, tp unit.Type) perm.AccessMode
 	return perm.AccessModeNone
 }
 
+// GetOrg returns the team's organization
+func (t *Team) GetOrg(ctx context.Context) *Organization {
+	org, err := GetOrgByID(ctx, t.OrgID)
+	if err != nil {
+		return OrgFromUser(user_model.NewGhostUser())
+	}
+	return org
+}
+
+// Link returns the team's page link
+func (t *Team) Link(ctx context.Context) string {
+	if t.IsGhost() {
+		return ""
+	}
+	org := t.GetOrg(ctx)
+	if org.IsGhost() {
+		return ""
+	}
+	return org.OrganisationLink() + "/teams/" + url.PathEscape(t.Name)
+}
+
 // IsUsableTeamName tests if a name could be as team name
 func IsUsableTeamName(name string) error {
 	switch name {
@@ -293,10 +309,22 @@ func FixInconsistentOwnerTeams(ctx context.Context) (int64, error) {
 	return int64(len(teamIDs)), nil
 }
 
+const (
+	GhostTeamID        = -1
+	GhostTeamName      = "Ghost team"
+	GhostTeamLowerName = "ghost team"
+)
+
+// NewGhostTeam creates ghost team (for deleted team)
 func NewGhostTeam() *Team {
 	return &Team{
-		ID:        -1,
+		ID:        GhostTeamID,
-		Name:      "Ghost team",
+		Name:      GhostTeamName,
-		LowerName: "ghost team",
+		LowerName: GhostTeamLowerName,
 	}
 }
+
+// IsGhost returns if a team is a ghost team
+func (t *Team) IsGhost() bool {
+	return t.ID == GhostTeamID
+}

models/organization/team_test.go

@@ -1,4 +1,5 @@
 // Copyright 2017 The Gitea Authors. All rights reserved.
+// Copyright 2025 The Forgejo Authors. All rights reserved.
 // SPDX-License-Identifier: MIT
 
 package organization_test
@@ -15,14 +16,33 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestTeam_IsOwnerTeam(t *testing.T) {
+func TestTeam(t *testing.T) {
 	require.NoError(t, unittest.PrepareTestDatabase())
 
-	team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 1})
+	owners := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 1})
-	assert.True(t, team.IsOwnerTeam())
+	assert.Equal(t, int64(3), owners.GetOrg(db.DefaultContext).ID)
+	assert.Equal(t, "/org/org3/teams/Owners", owners.Link(db.DefaultContext))
+	assert.False(t, owners.IsGhost())
+	assert.True(t, owners.IsOwnerTeam())
 
-	team = unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2})
+	team1 := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2})
-	assert.False(t, team.IsOwnerTeam())
+	assert.Equal(t, int64(3), team1.GetOrg(db.DefaultContext).ID)
+	assert.Equal(t, "/org/org3/teams/team1", team1.Link(db.DefaultContext))
+	assert.False(t, team1.IsGhost())
+	assert.False(t, team1.IsOwnerTeam())
+
+	ghost := organization.NewGhostTeam()
+	assert.Equal(t, int64(-1), ghost.ID)
+	assert.Equal(t, int64(-1), ghost.GetOrg(db.DefaultContext).ID)
+	assert.Empty(t, ghost.Link(db.DefaultContext))
+	assert.True(t, ghost.IsGhost())
+	assert.False(t, ghost.IsOwnerTeam())
+
+	ghosted := organization.Team{ID: 10, Name: "Ghosted"}
+	assert.Equal(t, int64(-1), ghosted.GetOrg(db.DefaultContext).ID)
+	assert.Empty(t, ghosted.Link(db.DefaultContext))
+	assert.False(t, ghosted.IsGhost())
+	assert.False(t, ghosted.IsOwnerTeam())
 }
 
 func TestTeam_IsMember(t *testing.T) {

models/user/user.go

@@ -296,6 +296,9 @@ func (u *User) CanImportLocal() bool {
 
 // DashboardLink returns the user dashboard page link.
 func (u *User) DashboardLink() string {
+	if u.IsGhost() {
+		return ""
+	}
 	if u.IsOrganization() {
 		return u.OrganisationLink() + "/dashboard"
 	}
@@ -304,16 +307,25 @@ func (u *User) DashboardLink() string {
 
 // HomeLink returns the user or organization home page link.
 func (u *User) HomeLink() string {
+	if u.IsGhost() {
+		return ""
+	}
 	return setting.AppSubURL + "/" + url.PathEscape(u.Name)
 }
 
 // HTMLURL returns the user or organization's full link.
 func (u *User) HTMLURL() string {
+	if u.IsGhost() {
+		return ""
+	}
 	return setting.AppURL + url.PathEscape(u.Name)
 }
 
 // OrganisationLink returns the organization sub page link.
 func (u *User) OrganisationLink() string {
+	if u.IsGhost() || !u.IsOrganization() {
+		return ""
+	}
 	return setting.AppSubURL + "/org/" + url.PathEscape(u.Name)
 }
 

models/user/user_test.go

@@ -46,6 +46,28 @@ func TestIsValidUserID(t *testing.T) {
 	assert.True(t, user_model.IsValidUserID(200))
 }
 
+func TestUserLinks(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+	assert.Equal(t, "/", user1.DashboardLink())
+	assert.Equal(t, "/user1", user1.HomeLink())
+	assert.Equal(t, "https://try.gitea.io/user1", user1.HTMLURL())
+	assert.Empty(t, user1.OrganisationLink())
+
+	org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3})
+	assert.Equal(t, "/org/org3/dashboard", org3.DashboardLink())
+	assert.Equal(t, "/org3", org3.HomeLink())
+	assert.Equal(t, "https://try.gitea.io/org3", org3.HTMLURL())
+	assert.Equal(t, "/org/org3", org3.OrganisationLink())
+
+	ghost := user_model.NewGhostUser()
+	assert.Empty(t, ghost.DashboardLink())
+	assert.Empty(t, ghost.HomeLink())
+	assert.Empty(t, ghost.HTMLURL())
+	assert.Empty(t, ghost.OrganisationLink())
+}
+
 func TestGetUserFromMap(t *testing.T) {
 	id := int64(200)
 	idMap := map[int64]*user_model.User{

modules/templates/util_render.go

@@ -275,10 +275,18 @@ func RenderUser(ctx context.Context, user user_model.User) template.HTML {
 		html.EscapeString(user.GetDisplayName())))
 }
 
-func RenderReviewRequest(users []issues_model.RequestReviewTarget) template.HTML {
+func RenderReviewRequest(ctx context.Context, users []issues_model.RequestReviewTarget) template.HTML {
 	usernames := make([]string, 0, len(users))
 	for _, user := range users {
-		usernames = append(usernames, html.EscapeString(user.Name()))
+		if user.ID() > 0 {
+			usernames = append(usernames, fmt.Sprintf(
+				"<a href='%s' rel='nofollow'><strong>%s</strong></a>",
+				user.Link(ctx), html.EscapeString(user.Name())))
+		} else {
+			usernames = append(usernames, fmt.Sprintf(
+				"<strong>%s</strong>",
+				html.EscapeString(user.Name())))
+		}
 	}
 
 	htmlCode := `<span class="review-request-list">`

modules/templates/util_render_test.go

@@ -10,6 +10,7 @@ import (
 
 	"forgejo.org/models/db"
 	issues_model "forgejo.org/models/issues"
+	org_model "forgejo.org/models/organization"
 	"forgejo.org/models/unittest"
 	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/setting"
@@ -266,3 +267,19 @@ func TestRenderUser(t *testing.T) {
 	assert.Contains(t, RenderUser(db.DefaultContext, *ghost),
 		"<strong>Ghost</strong>")
 }
+
+func TestRenderReviewRequest(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	target1 := issues_model.RequestReviewTarget{User: &user_model.User{ID: 1, Name: "user1", FullName: "User <One>"}}
+	target2 := issues_model.RequestReviewTarget{Team: &org_model.Team{ID: 2, Name: "Team2", OrgID: 3}}
+	target3 := issues_model.RequestReviewTarget{Team: org_model.NewGhostTeam()}
+	assert.Contains(t, RenderReviewRequest(db.DefaultContext, []issues_model.RequestReviewTarget{target1, target2, target3}),
+		"<a href='/user1' rel='nofollow'><strong>user1</strong></a>, "+
+			"<a href='/org/org3/teams/Team2' rel='nofollow'><strong>Team2</strong></a>, "+
+			"<strong>Ghost team</strong>")
+
+	defer test.MockVariableValue(&setting.UI.DefaultShowFullName, true)()
+	assert.Contains(t, RenderReviewRequest(db.DefaultContext, []issues_model.RequestReviewTarget{target1}),
+		"<a href='/user1' rel='nofollow'><strong>User &lt;One&gt;</strong></a>")
+}

templates/repo/issue/view_content/comments.tmpl

@@ -548,14 +548,15 @@
 					<span class="badge">{{svg "octicon-eye"}}</span>
 					{{template "shared/user/avatarlink" dict "user" .Poster}}
 					<span class="text grey muted-links">
+						{{template "shared/user/authorlink" .Poster}}
 						{{if and (eq (len .RemovedRequestReview) 1) (eq (len .AddedRequestReview) 0) (eq ((index .RemovedRequestReview 0).ID) .PosterID) (eq ((index .RemovedRequestReview 0).Type) "user")}}
 							<span class="review-request-list">{{ctx.Locale.Tr "repo.issues.review.remove_review_request_self" $createdStr}}</span>
 						{{else if and .AddedRequestReview (not .RemovedRequestReview)}}
-							{{ctx.Locale.TrN (len .AddedRequestReview) "repo.issues.review.add_review_request" "repo.issues.review.add_review_requests" (RenderReviewRequest .AddedRequestReview) $createdStr}}
+							{{ctx.Locale.TrN (len .AddedRequestReview) "repo.issues.review.add_review_request" "repo.issues.review.add_review_requests" (RenderReviewRequest $.Context .AddedRequestReview) $createdStr}}
 						{{else if and (not .AddedRequestReview) .RemovedRequestReview}}
-							{{ctx.Locale.TrN (len .RemovedRequestReview) "repo.issues.review.remove_review_request" "repo.issues.review.remove_review_requests" (RenderReviewRequest .RemovedRequestReview) $createdStr}}
+							{{ctx.Locale.TrN (len .RemovedRequestReview) "repo.issues.review.remove_review_request" "repo.issues.review.remove_review_requests" (RenderReviewRequest $.Context .RemovedRequestReview) $createdStr}}
 						{{else}}
-							{{ctx.Locale.Tr "repo.issues.review.add_remove_review_requests" (RenderReviewRequest .AddedRequestReview) (RenderReviewRequest .RemovedRequestReview) $createdStr}}
+							{{ctx.Locale.Tr "repo.issues.review.add_remove_review_requests" (RenderReviewRequest $.Context .AddedRequestReview) (RenderReviewRequest $.Context .RemovedRequestReview) $createdStr}}
 						{{end}}
 					</span>
 				</div>

tests/integration/issue_comment_test.go

@@ -5,13 +5,20 @@ package integration
 
 import (
 	"net/http"
+	"strconv"
 	"strings"
 	"testing"
 
+	"forgejo.org/models/db"
+	issues_model "forgejo.org/models/issues"
+	org_model "forgejo.org/models/organization"
+	"forgejo.org/models/unittest"
+	user_model "forgejo.org/models/user"
 	"forgejo.org/tests"
 
 	"github.com/PuerkitoBio/goquery"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func testIssueCommentChangeEvent(t *testing.T, htmlDoc *HTMLDoc, commentID, badgeOcticon, avatarTitle, avatarLink string, texts, links []string) {
@@ -238,6 +245,76 @@ func TestIssueCommentChangeAssignee(t *testing.T) {
 		[]string{"/user2"})
 }
 
+func TestIssueCommentChangeReviewRequest(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 6})
+	require.NoError(t, pull.LoadIssue(db.DefaultContext))
+	issue := pull.Issue
+	require.NoError(t, issue.LoadRepo(db.DefaultContext))
+
+	user1, err := user_model.GetUserByID(db.DefaultContext, 1)
+	require.NoError(t, err)
+	user2, err := user_model.GetUserByID(db.DefaultContext, 2)
+	require.NoError(t, err)
+	team1, err := org_model.GetTeamByID(db.DefaultContext, 2)
+	require.NoError(t, err)
+	assert.NotNil(t, team1)
+
+	// Request from other
+	comment1, err := issues_model.AddReviewRequest(db.DefaultContext, issue, user2, user1)
+	require.NoError(t, err)
+
+	// Refuse review
+	comment2, err := issues_model.RemoveReviewRequest(db.DefaultContext, issue, user2, user2)
+	require.NoError(t, err)
+
+	// Request from other
+	comment3, err := issues_model.AddReviewRequest(db.DefaultContext, issue, user2, user1)
+	require.NoError(t, err)
+	// Request from team
+	comment4, err := issues_model.AddTeamReviewRequest(db.DefaultContext, issue, team1, user1)
+	require.NoError(t, err)
+
+	// Remove request from team
+	comment5, err := issues_model.RemoveTeamReviewRequest(db.DefaultContext, issue, team1, user2)
+	require.NoError(t, err)
+	// Request from other
+	comment6, err := issues_model.AddReviewRequest(db.DefaultContext, issue, user1, user2)
+	require.NoError(t, err)
+
+	session := loginUser(t, "user2")
+	req := NewRequest(t, "GET", "/org3/repo3/pulls/2")
+	resp := session.MakeRequest(t, req, http.StatusOK)
+	htmlDoc := NewHTMLParser(t, resp.Body)
+
+	// Request from other
+	testIssueCommentChangeEvent(t, htmlDoc, strconv.FormatInt(comment1.ID, 10),
+		"octicon-eye", "User One", "/user1",
+		[]string{"user1 requested review from user2"},
+		[]string{"/user1", "/user2"})
+
+	// Refuse review
+	testIssueCommentChangeEvent(t, htmlDoc, strconv.FormatInt(comment2.ID, 10),
+		"octicon-eye", "< U<se>r Tw<o > ><", "/user2",
+		[]string{"user2 refused to review"},
+		[]string{"/user2"})
+
+	// Request review from other and from team
+	testIssueCommentChangeEvent(t, htmlDoc, strconv.FormatInt(comment3.ID, 10),
+		"octicon-eye", "User One", "/user1",
+		[]string{"user1 requested reviews from user2, team1"},
+		[]string{"/user1", "/user2", "/org/org3/teams/team1"})
+	assert.Empty(t, htmlDoc.Find("#issuecomment-"+strconv.FormatInt(comment4.ID, 10)+" .text").Text())
+
+	// Remove and add request
+	testIssueCommentChangeEvent(t, htmlDoc, strconv.FormatInt(comment5.ID, 10),
+		"octicon-eye", "< U<se>r Tw<o > ><", "/user2",
+		[]string{"user2 requested reviews from user1 and removed review requests for team1"},
+		[]string{"/user2", "/user1", "/org/org3/teams/team1"})
+	assert.Empty(t, htmlDoc.Find("#issuecomment-"+strconv.FormatInt(comment6.ID, 10)+" .text").Text())
+}
+
 func TestIssueCommentChangeLock(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()