fix: represent a deleted assignee team as a Ghost team (#38413)

Fixes #35472.

`Comment.LoadAssigneeUserAndTeam` already has a Ghost user fallback
for a deleted assignee user, but the parallel branch for a deleted
assignee team just swallowed the not-found error and left
`AssigneeTeam` as `nil`. This is inconsistent (the reporter's example
shows `assignee` becoming a Ghost user while `assignee_team` becomes
`null`), and it's also a latent nil pointer bug: other code that
assumes `AssigneeTeam` is set once this function returns without
error will panic.

Added `organization.NewGhostTeam()` / `Team.IsGhost()`, mirroring the
existing `user_model.NewGhostUser()` / `User.IsGhost()` pattern, and
used it in the same fallback branch.

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

models/issues/comment.go

@@ -27,6 +27,7 @@ import (
 	"gitea.dev/modules/markup"
 	"gitea.dev/modules/optional"
 	"gitea.dev/modules/references"
+	"gitea.dev/modules/setting"
 	"gitea.dev/modules/structs"
 	"gitea.dev/modules/timeutil"
 	"gitea.dev/modules/translation"
@@ -643,36 +644,18 @@ func UpdateCommentAttachments(ctx context.Context, c *Comment, uuids []string) e
 }
 
 // LoadAssigneeUserAndTeam if comment.Type is CommentTypeAssignees, then load assignees
-func (c *Comment) LoadAssigneeUserAndTeam(ctx context.Context) error {
+func (c *Comment) LoadAssigneeUserAndTeam(ctx context.Context) (err error) {
-	var err error
-
 	if c.AssigneeID > 0 && c.Assignee == nil {
-		c.Assignee, err = user_model.GetUserByID(ctx, c.AssigneeID)
+		_, c.Assignee, err = user_model.GetPossibleUserByID(ctx, c.AssigneeID)
 		if err != nil {
-			if !user_model.IsErrUserNotExist(err) {
-				return err
-			}
-			c.Assignee = user_model.NewGhostUser()
-		}
-	} else if c.AssigneeTeamID > 0 && c.AssigneeTeam == nil {
-		if err = c.LoadIssue(ctx); err != nil {
 			return err
 		}
-
+	}
-		if err = c.Issue.LoadRepo(ctx); err != nil {
+	if c.AssigneeTeamID > 0 && c.AssigneeTeam == nil {
+		_, c.AssigneeTeam, err = organization.GetPossibleTeamByID(ctx, c.AssigneeTeamID)
+		if err != nil {
 			return err
 		}
-
-		if err = c.Issue.Repo.LoadOwner(ctx); err != nil {
-			return err
-		}
-
-		if c.Issue.Repo.Owner.IsOrganization() {
-			c.AssigneeTeam, err = organization.GetTeamByID(ctx, c.AssigneeTeamID)
-			if err != nil && !organization.IsErrTeamNotExist(err) {
-				return err
-			}
-		}
 	}
 	return nil
 }
@@ -795,8 +778,7 @@ func (c *Comment) MetaSpecialDoerTr(locale translation.Locale) template.HTML {
 }
 
 func (c *Comment) TimelineRequestedReviewTr(locale translation.Locale, createdStr template.HTML) template.HTML {
-	if c.AssigneeID > 0 {
+	if c.Assignee != nil {
-		// 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)
@@ -805,14 +787,20 @@ func (c *Comment) TimelineRequestedReviewTr(locale translation.Locale, createdSt
 		}
 		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", c.AssigneeTeam.Name, createdStr)
+		}
+		return locale.Tr("repo.issues.review.add_review_request", c.AssigneeTeam.Name, createdStr)
 	}
+
+	// impossible fallback
+	assigneePrompt := fmt.Sprintf("(AssigneeID=%d, AssigneeTeamID=%d)", c.AssigneeID, c.AssigneeTeam.ID)
+	setting.PanicInDevOrTesting("unknown timeline pull request review event comment: id=%d, %s", c.ID, assigneePrompt)
 	if c.RemovedAssignee {
-		return locale.Tr("repo.issues.review.remove_review_request", teamName, createdStr)
+		return locale.Tr("repo.issues.review.remove_review_request", assigneePrompt, createdStr)
 	}
-	return locale.Tr("repo.issues.review.add_review_request", teamName, createdStr)
+	return locale.Tr("repo.issues.review.add_review_request", assigneePrompt, createdStr)
 }
 
 // CreateComment creates comment with context

models/issues/comment_test.go

@@ -45,6 +45,19 @@ func TestCreateComment(t *testing.T) {
 	unittest.AssertInt64InRange(t, now, then, int64(updatedIssue.UpdatedUnix))
 }
 
+func TestLoadAssigneeUserAndTeam_DeletedTeamBecomesGhostTeam(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 15})
+	comment := &issues_model.Comment{
+		Type:           issues_model.CommentTypeAssignees,
+		IssueID:        issue.ID,
+		AssigneeTeamID: 999999, // non-existing team ID
+	}
+	assert.NoError(t, comment.LoadAssigneeUserAndTeam(t.Context()))
+	assert.NotNil(t, comment.AssigneeTeam)
+	assert.EqualValues(t, -1, comment.AssigneeTeam.ID)
+}
+
 func Test_UpdateCommentAttachment(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 

models/organization/team.go

@@ -6,6 +6,7 @@ package organization
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -92,6 +93,15 @@ func (t *Team) IsPublic() bool  { return t.Visibility.IsPublic() }
 func (t *Team) IsLimited() bool { return t.Visibility.IsLimited() }
 func (t *Team) IsPrivate() bool { return t.Visibility.IsPrivate() }
 
+const (
+	ghostTeamID   = -1
+	ghostTeamName = "(deleted team)"
+)
+
+func newGhostTeam() *Team {
+	return &Team{ID: ghostTeamID, Name: ghostTeamName, LowerName: ghostTeamName}
+}
+
 // CanNonMemberReadMeta reports whether a non-member, non-owner doer may read
 // the team's metadata, based on the team's visibility tier and the parent org's
 // visibility. Privileged callers (site admins, org owners, team members) are
@@ -270,6 +280,17 @@ func GetTeamByID(ctx context.Context, teamID int64) (*Team, error) {
 	return t, nil
 }
 
+func GetPossibleTeamByID(ctx context.Context, teamID int64) (int64, *Team, error) {
+	t, err := GetTeamByID(ctx, teamID)
+	if errors.Is(err, util.ErrNotExist) {
+		t = newGhostTeam()
+		return t.ID, t, nil
+	} else if err != nil {
+		return 0, nil, err
+	}
+	return t.ID, t, nil
+}
+
 // IncrTeamRepoNum increases the number of repos for the given team by 1
 func IncrTeamRepoNum(ctx context.Context, teamID int64) error {
 	_, err := db.GetEngine(ctx).Incr("num_repos").ID(teamID).Update(new(Team))