Fix internal sever error when visiting a PR that bound to the deleted team

[sillyguodong]

Close: #23738

The actual cause of 500 Internal Server Error in the issue is not what is descirbed in the issue.

The actual cause is that after deleting team, if there is a PR which has requested reivew from the deleted team, the comment could not match with the deleted team by assgin_team_id. So the value of .AssigneeTeam (see below code block) is nil which cause 500 error.

gitea/templates/repo/issue/view_content/comments.tmpl

Lines 691 to 695 in 1c8bc40

	{{if .RemovedAssignee}}
	{{$.locale.Tr "repo.issues.review.remove_review_request" (.AssigneeTeam.Name|Escape) $createdStr | Safe}}
	{{else}}
	{{$.locale.Tr "repo.issues.review.add_review_request" (.AssigneeTeam.Name|Escape) $createdStr | Safe}}
	{{end}}

To fix this bug, there are the following problems to be resolved:

• 1. Stroe the name of the team in content column when inserting comment into DB in case that we cannot get the name of team after it is deleted. But for comments that already exist, just display "Unknown Team" Just display "Ghost Team" in the comment if the assgined team is deleted.
• 2. Delete the PR&team binding (the row of which review_team_id = ${team_id} in table review) when deleting team.
• 3.For already exist and undeleted binding rows in in table review, we can delete these rows when executing migrations. they do not affect the function, so won't delete them.

[wxiaoguang]

I think it's better to just show "ghost team" or "deleted team" instead of such complex logic.

For example, if you create a team "MyTeam" , then request review from it, then delete it, then create a new "MyTeam", the new team is not the old team.

[sillyguodong]

the new team is not the old team.

Agree. I'm not going to match the two teams if they have the same name, just want to preserve the original information in the comment as much as possible.

[sillyguodong]

Just displaying ”Ghost Team“ in the comment looks more concise, and there will be no misunderstanding.
I will use "Ghost Team" instead of original team name...

[delvh]

I'll accept this change for now.
As a long-term solution we need to do something similar to the ghost user and add a ghost team.

[codecov-commenter]

Codecov Report

Merging #24127 (a4de58c) into main (f521e88) will decrease coverage by 0.02%.
The diff coverage is 41.61%.

❗ Current head a4de58c differs from pull request most recent head b6376b7. Consider uploading reports for the commit b6376b7 to get more accurate results

@@            Coverage Diff             @@
##             main   #24127      +/-   ##
==========================================
- Coverage   47.14%   47.13%   -0.02%     
==========================================
  Files        1149     1161      +12     
  Lines      151446   153767    +2321     
==========================================
+ Hits        71397    72471    +1074     
- Misses      71611    72773    +1162     
- Partials     8438     8523      +85     

Impacted Files	Coverage Δ
cmd/dump.go	0.64% <0.00%> (-0.03%)	⬇️
cmd/embedded.go	0.00% <0.00%> (ø)
cmd/mailer.go	0.00% <0.00%> (ø)
cmd/manager.go	0.00% <0.00%> (ø)
cmd/manager_logging.go	0.00% <0.00%> (ø)
cmd/migrate_storage.go	5.76% <0.00%> (-0.12%)	⬇️
cmd/restore_repo.go	0.00% <0.00%> (ø)
cmd/web.go	0.00% <0.00%> (ø)
models/actions/run.go	1.63% <0.00%> (-0.10%)	⬇️
models/actions/runner.go	1.44% <ø> (ø)
... and 61 more

... and 156 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.