feat: combine review requests comments

- Combine review requests comments similairy how labels comments are
combined. If review requests comments were made within 60 seconds of
each other they will be grouped.
- Integration and unit test added.
- Resolves #2774
This commit is contained in:
Gusted 2024-10-25 09:24:36 +02:00
parent aa86e94853
commit 8fdc0a7a6c
9 changed files with 685 additions and 65 deletions

View file

@ -222,6 +222,12 @@ func (r RoleInRepo) LocaleHelper(lang translation.Locale) string {
return lang.TrString("repo.issues.role." + string(r) + "_helper")
}
type RequestReviewTarget interface {
ID() int64
Name() string
Type() string
}
// Comment represents a comment in commit and issue page.
type Comment struct {
ID int64 `xorm:"pk autoincr"`
@ -236,6 +242,8 @@ type Comment struct {
Label *Label `xorm:"-"`
AddedLabels []*Label `xorm:"-"`
RemovedLabels []*Label `xorm:"-"`
AddedRequestReview []RequestReviewTarget `xorm:"-"`
RemovedRequestReview []RequestReviewTarget `xorm:"-"`
OldProjectID int64
ProjectID int64
OldProject *project_model.Project `xorm:"-"`

View file

@ -182,6 +182,7 @@ func NewFuncMap() template.FuncMap {
"RenderMarkdownToHtml": RenderMarkdownToHtml,
"RenderLabel": RenderLabel,
"RenderLabels": RenderLabels,
"RenderReviewRequest": RenderReviewRequest,
// -----------------------------------------------------------------
// misc

View file

@ -262,3 +262,15 @@ func RenderLabels(ctx context.Context, locale translation.Locale, labels []*issu
htmlCode += "</span>"
return template.HTML(htmlCode)
}
func RenderReviewRequest(users []issues_model.RequestReviewTarget) template.HTML {
usernames := make([]string, 0, len(users))
for _, user := range users {
usernames = append(usernames, template.HTMLEscapeString(user.Name()))
}
htmlCode := `<span class="review-request-list">`
htmlCode += strings.Join(usernames, ", ")
htmlCode += "</span>"
return template.HTML(htmlCode)
}

View file

@ -1791,9 +1791,12 @@ issues.review.left_comment = left a comment
issues.review.content.empty = You need to leave a comment indicating the requested change(s).
issues.review.reject = requested changes %s
issues.review.wait = was requested for review %s
issues.review.add_review_request = requested review from %s %s
issues.review.remove_review_request = removed review request for %s %s
issues.review.remove_review_request_self = refused to review %s
issues.review.add_review_request = requested review from %[1]s %[2]s
issues.review.add_review_requests = requested reviews from %[1]s %[2]s
issues.review.remove_review_request = removed review request for %[1]s %[2]s
issues.review.remove_review_requests = removed review requests for %[1]s %[2]s
issues.review.remove_review_request_self = refused to review %[1]s %[2]s
issues.review.add_remove_review_requests = requested reviews from %[1]s and removed review requests for %[2]s %[3]s
issues.review.pending = Pending
issues.review.pending.tooltip = This comment is not currently visible to other users. To submit your pending comments, select "%s" -> "%s/%s/%s" at the top of the page.
issues.review.review = Review

View file

@ -1825,6 +1825,7 @@ func ViewIssue(ctx *context.Context) {
// Combine multiple label assignments into a single comment
combineLabelComments(issue)
combineRequestReviewComments(issue)
getBranchData(ctx, issue)
if issue.IsPull {
@ -3665,6 +3666,127 @@ func attachmentsHTML(ctx *context.Context, attachments []*repo_model.Attachment,
return attachHTML
}
type RequestReviewTarget struct {
user *user_model.User
team *organization.Team
}
func (t *RequestReviewTarget) ID() int64 {
if t.user != nil {
return t.user.ID
}
return t.team.ID
}
func (t *RequestReviewTarget) Name() string {
if t.user != nil {
return t.user.GetDisplayName()
}
return t.team.Name
}
func (t *RequestReviewTarget) Type() string {
if t.user != nil {
return "user"
}
return "team"
}
// combineRequestReviewComments combine the nearby request review comments as one.
func combineRequestReviewComments(issue *issues_model.Issue) {
var prev, cur *issues_model.Comment
for i := 0; i < len(issue.Comments); i++ {
cur = issue.Comments[i]
if i > 0 {
prev = issue.Comments[i-1]
}
if i == 0 || cur.Type != issues_model.CommentTypeReviewRequest ||
(prev != nil && prev.PosterID != cur.PosterID) ||
(prev != nil && cur.CreatedUnix-prev.CreatedUnix >= 60) {
if cur.Type == issues_model.CommentTypeReviewRequest && (cur.Assignee != nil || cur.AssigneeTeam != nil) {
if cur.RemovedAssignee {
if cur.AssigneeTeam != nil {
cur.RemovedRequestReview = append(cur.RemovedRequestReview, &RequestReviewTarget{team: cur.AssigneeTeam})
} else {
cur.RemovedRequestReview = append(cur.RemovedRequestReview, &RequestReviewTarget{user: cur.Assignee})
}
} else {
if cur.AssigneeTeam != nil {
cur.AddedRequestReview = append(cur.AddedRequestReview, &RequestReviewTarget{team: cur.AssigneeTeam})
} else {
cur.AddedRequestReview = append(cur.AddedRequestReview, &RequestReviewTarget{user: cur.Assignee})
}
}
}
continue
}
// Previous comment is not a review request, so cannot group. Start a new group.
if prev.Type != issues_model.CommentTypeReviewRequest {
if cur.RemovedAssignee {
if cur.AssigneeTeam != nil {
cur.RemovedRequestReview = append(cur.RemovedRequestReview, &RequestReviewTarget{team: cur.AssigneeTeam})
} else {
cur.RemovedRequestReview = append(cur.RemovedRequestReview, &RequestReviewTarget{user: cur.Assignee})
}
} else {
if cur.AssigneeTeam != nil {
cur.AddedRequestReview = append(cur.AddedRequestReview, &RequestReviewTarget{team: cur.AssigneeTeam})
} else {
cur.AddedRequestReview = append(cur.AddedRequestReview, &RequestReviewTarget{user: cur.Assignee})
}
}
continue
}
// Start grouping.
if cur.RemovedAssignee {
addedIndex := slices.IndexFunc(prev.AddedRequestReview, func(t issues_model.RequestReviewTarget) bool {
if cur.AssigneeTeam != nil {
return cur.AssigneeTeam.ID == t.ID() && t.Type() == "team"
}
return cur.Assignee.ID == t.ID() && t.Type() == "user"
})
// If for this target a AddedRequestReview, then we remove that entry. If it's not found, then add it to the RemovedRequestReview.
if addedIndex == -1 {
if cur.AssigneeTeam != nil {
prev.RemovedRequestReview = append(prev.RemovedRequestReview, &RequestReviewTarget{team: cur.AssigneeTeam})
} else {
prev.RemovedRequestReview = append(prev.RemovedRequestReview, &RequestReviewTarget{user: cur.Assignee})
}
} else {
prev.AddedRequestReview = slices.Delete(prev.AddedRequestReview, addedIndex, addedIndex+1)
}
} else {
removedIndex := slices.IndexFunc(prev.RemovedRequestReview, func(t issues_model.RequestReviewTarget) bool {
if cur.AssigneeTeam != nil {
return cur.AssigneeTeam.ID == t.ID() && t.Type() == "team"
}
return cur.Assignee.ID == t.ID() && t.Type() == "user"
})
// If for this target a RemovedRequestReview, then we remove that entry. If it's not found, then add it to the AddedRequestReview.
if removedIndex == -1 {
if cur.AssigneeTeam != nil {
prev.AddedRequestReview = append(prev.AddedRequestReview, &RequestReviewTarget{team: cur.AssigneeTeam})
} else {
prev.AddedRequestReview = append(prev.AddedRequestReview, &RequestReviewTarget{user: cur.Assignee})
}
} else {
prev.RemovedRequestReview = slices.Delete(prev.RemovedRequestReview, removedIndex, removedIndex+1)
}
}
// Propoagate creation time.
prev.CreatedUnix = cur.CreatedUnix
// Remove the current comment since it has been combined to prev comment
issue.Comments = append(issue.Comments[:i], issue.Comments[i+1:]...)
i--
}
}
// combineLabelComments combine the nearby label comments as one.
func combineLabelComments(issue *issues_model.Issue) {
var prev, cur *issues_model.Comment

View file

@ -7,6 +7,8 @@ import (
"testing"
issues_model "code.gitea.io/gitea/models/issues"
org_model "code.gitea.io/gitea/models/organization"
user_model "code.gitea.io/gitea/models/user"
"github.com/stretchr/testify/assert"
)
@ -373,3 +375,432 @@ func TestCombineLabelComments(t *testing.T) {
})
}
}
func TestCombineReviewRequests(t *testing.T) {
testCases := []struct {
name string
beforeCombined []*issues_model.Comment
afterCombined []*issues_model.Comment
}{
{
name: "case 1",
beforeCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
RemovedAssignee: true,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeComment,
PosterID: 1,
Content: "test",
CreatedUnix: 0,
},
},
afterCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
AddedRequestReview: []issues_model.RequestReviewTarget{},
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
{
Type: issues_model.CommentTypeComment,
PosterID: 1,
Content: "test",
CreatedUnix: 0,
},
},
},
{
name: "case 2",
beforeCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
Assignee: &user_model.User{
ID: 2,
Name: "Ghost 2",
},
CreatedUnix: 0,
},
},
afterCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
AddedRequestReview: []issues_model.RequestReviewTarget{
&RequestReviewTarget{
user: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
&RequestReviewTarget{
user: &user_model.User{
ID: 2,
Name: "Ghost 2",
},
},
},
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
},
},
{
name: "case 3",
beforeCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
RemovedAssignee: true,
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
CreatedUnix: 0,
},
},
afterCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
AddedRequestReview: []issues_model.RequestReviewTarget{
&RequestReviewTarget{
user: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
},
RemovedRequestReview: []issues_model.RequestReviewTarget{
&RequestReviewTarget{
team: &org_model.Team{
ID: 1,
Name: "Team 1",
},
},
},
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
},
},
{
name: "case 4",
beforeCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
RemovedAssignee: true,
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
CreatedUnix: 0,
},
},
afterCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
AddedRequestReview: []issues_model.RequestReviewTarget{
&RequestReviewTarget{
user: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
},
RemovedRequestReview: []issues_model.RequestReviewTarget{},
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
},
},
{
name: "case 5",
beforeCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
RemovedAssignee: true,
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
RemovedAssignee: true,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
},
afterCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
AddedRequestReview: []issues_model.RequestReviewTarget{},
RemovedRequestReview: []issues_model.RequestReviewTarget{},
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
},
},
{
name: "case 6",
beforeCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
RemovedAssignee: true,
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeComment,
PosterID: 1,
Content: "test",
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
RemovedAssignee: true,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
},
afterCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
RemovedRequestReview: []issues_model.RequestReviewTarget{&RequestReviewTarget{
team: &org_model.Team{
ID: 1,
Name: "Team 1",
},
}},
AddedRequestReview: []issues_model.RequestReviewTarget{&RequestReviewTarget{
user: &user_model.User{
ID: 1,
Name: "Ghost",
},
}},
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
{
Type: issues_model.CommentTypeComment,
PosterID: 1,
Content: "test",
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
AddedRequestReview: []issues_model.RequestReviewTarget{&RequestReviewTarget{
team: &org_model.Team{
ID: 1,
Name: "Team 1",
},
}},
RemovedRequestReview: []issues_model.RequestReviewTarget{&RequestReviewTarget{
user: &user_model.User{
ID: 1,
Name: "Ghost",
},
}},
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
},
},
},
{
name: "case 7",
beforeCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
CreatedUnix: 0,
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
CreatedUnix: 61,
},
},
afterCombined: []*issues_model.Comment{
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
AddedRequestReview: []issues_model.RequestReviewTarget{&RequestReviewTarget{
user: &user_model.User{
ID: 1,
Name: "Ghost",
},
}},
Assignee: &user_model.User{
ID: 1,
Name: "Ghost",
},
},
{
Type: issues_model.CommentTypeReviewRequest,
PosterID: 1,
CreatedUnix: 0,
RemovedRequestReview: []issues_model.RequestReviewTarget{&RequestReviewTarget{
team: &org_model.Team{
ID: 1,
Name: "Team 1",
},
}},
AssigneeTeam: &org_model.Team{
ID: 1,
Name: "Team 1",
},
},
},
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
issue := issues_model.Issue{
Comments: testCase.beforeCombined,
}
combineRequestReviewComments(&issue)
assert.EqualValues(t, testCase.afterCombined[0], issue.Comments[0])
})
}
}

View file

@ -522,35 +522,23 @@
</div>
</div>
{{else if eq .Type 27}}
{{if or .AddedRequestReview .RemovedRequestReview}}
<div class="timeline-item event" id="{{.HashTag}}">
<span class="badge">{{svg "octicon-eye"}}</span>
<span class="badge">{{svg "octicon-tag"}}</span>
{{template "shared/user/avatarlink" dict "user" .Poster}}
<span class="text grey muted-links">
{{template "shared/user/authorlink" .Poster}}
{{if (gt .AssigneeID 0)}}
{{if .RemovedAssignee}}
{{if eq .PosterID .AssigneeID}}
{{ctx.Locale.Tr "repo.issues.review.remove_review_request_self" $createdStr}}
{{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}}
{{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}}
{{else}}
{{ctx.Locale.Tr "repo.issues.review.remove_review_request" .Assignee.GetDisplayName $createdStr}}
{{end}}
{{else}}
{{ctx.Locale.Tr "repo.issues.review.add_review_request" .Assignee.GetDisplayName $createdStr}}
{{end}}
{{else}}
<!-- 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}}
{{ctx.Locale.Tr "repo.issues.review.add_remove_review_requests" (RenderReviewRequest .AddedRequestReview) (RenderReviewRequest .RemovedRequestReview) $createdStr}}
{{end}}
</span>
</div>
{{end}}
{{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. //-->
{{if and .Issue.IsClosed (gt .ID $.LatestCloseCommentID)}}

View file

@ -0,0 +1,21 @@
-
id: 1001
type: 4
reviewer_id: 2
issue_id: 3
official: true
stale: false
original_author_id: 0
updated_unix: 1000000000
created_unix: 1000000000
-
id: 1002
type: 4
reviewer_id: 11
issue_id: 3
official: true
stale: false
original_author_id: 0
updated_unix: 1000000000
created_unix: 1000000000

View file

@ -65,3 +65,37 @@ func TestPullManuallyMergeWarning(t *testing.T) {
assert.NotContains(t, mergeInstructions, warningMessage)
})
}
func TestPullCombinedReviewRequest(t *testing.T) {
defer tests.AddFixtures("tests/integration/fixtures/TestPullCombinedReviewRequest/")()
defer tests.PrepareTestEnv(t)()
session := loginUser(t, "user2")
helper := func(t *testing.T, action, userID, expectedText string) {
t.Helper()
req := NewRequestWithValues(t, "POST", "/user2/repo1/pulls/request_review", map[string]string{
"_csrf": GetCSRF(t, session, "/user2/repo1/pulls/3"),
"issue_ids": "3",
"action": action,
"id": userID,
})
session.MakeRequest(t, req, http.StatusOK)
req = NewRequest(t, "GET", "/user2/repo1/pulls/3")
resp := session.MakeRequest(t, req, http.StatusOK)
htmlDoc := NewHTMLParser(t, resp.Body)
assert.Contains(t, htmlDoc.Find(".timeline-item:has(.review-request-list)").Last().Text(), expectedText)
}
helper(t, "detach", "2", "refused to review")
helper(t, "attach", "4", "requested reviews from user4 and removed review requests for user2")
helper(t, "attach", "9", "requested reviews from user4, user9 and removed review requests for user2")
helper(t, "attach", "2", "requested reviews from user4, user9")
helper(t, "detach", "4", "requested review from user9")
helper(t, "detach", "11", "requested reviews from user9 and removed review requests for user11")
helper(t, "detach", "9", "removed review request for user11")
helper(t, "detach", "2", "removed review requests for user11, user2")
}