diff --git a/models/issues/action_aggregator.go b/models/issues/action_aggregator.go index d3643adeef..c4632fd4dd 100644 --- a/models/issues/action_aggregator.go +++ b/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) +} diff --git a/models/issues/action_aggregator_test.go b/models/issues/action_aggregator_test.go new file mode 100644 index 0000000000..1962596d2d --- /dev/null +++ b/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)) +} diff --git a/models/organization/org.go b/models/organization/org.go index ff95261051..c4df5d4fe1 100644 --- a/models/organization/org.go +++ b/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 diff --git a/models/organization/team.go b/models/organization/team.go index c78eff39fb..209471e013 100644 --- a/models/organization/team.go +++ b/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, - Name: "Ghost team", - LowerName: "ghost team", + ID: GhostTeamID, + Name: GhostTeamName, + LowerName: GhostTeamLowerName, } } + +// IsGhost returns if a team is a ghost team +func (t *Team) IsGhost() bool { + return t.ID == GhostTeamID +} diff --git a/models/organization/team_test.go b/models/organization/team_test.go index 60c500e7ec..768ccdf5be 100644 --- a/models/organization/team_test.go +++ b/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}) - assert.True(t, team.IsOwnerTeam()) + owners := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 1}) + 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}) - assert.False(t, team.IsOwnerTeam()) + team1 := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2}) + 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) { diff --git a/models/user/user.go b/models/user/user.go index 6b54776adf..e3d725677f 100644 --- a/models/user/user.go +++ b/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) } diff --git a/models/user/user_test.go b/models/user/user_test.go index f9a3aa6075..71190751da 100644 --- a/models/user/user_test.go +++ b/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{ diff --git a/modules/templates/util_render.go b/modules/templates/util_render.go index 48f4eb04a3..badff5f193 100644 --- a/modules/templates/util_render.go +++ b/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( + "%s", + user.Link(ctx), html.EscapeString(user.Name()))) + } else { + usernames = append(usernames, fmt.Sprintf( + "%s", + html.EscapeString(user.Name()))) + } } htmlCode := `` diff --git a/modules/templates/util_render_test.go b/modules/templates/util_render_test.go index a5fb18642a..8d58d7d2d4 100644 --- a/modules/templates/util_render_test.go +++ b/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), "Ghost") } + +func TestRenderReviewRequest(t *testing.T) { + unittest.PrepareTestEnv(t) + + target1 := issues_model.RequestReviewTarget{User: &user_model.User{ID: 1, Name: "user1", FullName: "User "}} + 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}), + "user1, "+ + "Team2, "+ + "Ghost team") + + defer test.MockVariableValue(&setting.UI.DefaultShowFullName, true)() + assert.Contains(t, RenderReviewRequest(db.DefaultContext, []issues_model.RequestReviewTarget{target1}), + "User <One>") +} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 4ab1fa7584..76b02f4755 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -548,14 +548,15 @@ {{svg "octicon-eye"}} {{template "shared/user/avatarlink" dict "user" .Poster}} + {{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")}} {{ctx.Locale.Tr "repo.issues.review.remove_review_request_self" $createdStr}} {{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}} diff --git a/tests/integration/issue_comment_test.go b/tests/integration/issue_comment_test.go index 6c4a514eba..eda643fa79 100644 --- a/tests/integration/issue_comment_test.go +++ b/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", "< Ur Tw ><", "/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", "< Ur Tw ><", "/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)()