fix: don't display pending reviews as participants (#10528)

Fixes #10155

When participants are displayed, don't include those that only have made a pending review. Those should not yet be revealed as participants.

Apart from adding automated tests, this is the manual verification process I've followed:
1. Set up three users
2. User 1 creates a repository, then creates a pull request adding a new file
3. User 2 creates a new code comment but doesn't not publish the review, shows as pending.
4. User 3 creates a new code comment and publishes the review.
5. From everyone's perspective the number of participants is: 2. And, the participants displayed in the list are 1 and 3. User 2, which hasn't yet published the review is not displayed.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10528
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: luisadame <luisadame@noreply.codeberg.org>
Co-committed-by: luisadame <luisadame@noreply.codeberg.org>

models/issues/TestGetParticipantIDsByIssue/comment.yml

@@ -0,0 +1,10 @@
+- id: 1001
+  type: 21 # code comment
+  poster_id: 10
+  issue_id: 1
+  review_id: 1001
+  content: "Some code comment that is pending to be published"
+  line: -4
+  tree_path: "README.md"
+  created_unix: 946684812
+  invalidated: false

models/issues/TestGetParticipantIDsByIssue/review.yml

@@ -0,0 +1,7 @@
+- id: 1001
+  type: 0 # Pending review
+  reviewer_id: 10
+  issue_id: 1
+  content: "Pending review for issue 1"
+  updated_unix: 946684810
+  created_unix: 946684810

models/issues/comment_list.go

@@ -417,7 +417,7 @@ func (comments CommentList) getReviewIDs() []int64 {
 	})
 }
 
-func (comments CommentList) loadReviews(ctx context.Context) error {
+func (comments CommentList) LoadReviews(ctx context.Context) error {
 	if len(comments) == 0 {
 		return nil
 	}
@@ -476,7 +476,7 @@ func (comments CommentList) LoadAttributes(ctx context.Context) (err error) {
 		return err
 	}
 
-	if err = comments.loadReviews(ctx); err != nil {
+	if err = comments.LoadReviews(ctx); err != nil {
 		return err
 	}
 

models/issues/issue.go

@@ -591,10 +591,12 @@ func GetParticipantsIDsByIssueID(ctx context.Context, issueID int64) ([]int64, e
 	userIDs := make([]int64, 0, 5)
 	return userIDs, db.GetEngine(ctx).
 		Table("comment").
-		Cols("poster_id").
-		Where("issue_id = ?", issueID).
-		And("type in (?,?,?)", CommentTypeComment, CommentTypeCode, CommentTypeReview).
-		Distinct("poster_id").
+		Cols("`comment`.poster_id").
+		Where("`comment`.issue_id = ?", issueID).
+		And("`comment`.type in (?,?,?)", CommentTypeComment, CommentTypeCode, CommentTypeReview).
+		And("`review`.type is null or `review`.type != ?", ReviewTypePending).
+		Join("LEFT", "`review`", "`review`.id = `comment`.review_id").
+		Distinct("`comment`.poster_id").
 		Find(&userIDs)
 }
 
@@ -623,9 +625,11 @@ func (issue *Issue) GetParticipantIDsByIssue(ctx context.Context) ([]int64, erro
 	if err := db.GetEngine(ctx).Table("comment").Cols("poster_id").
 		Where("`comment`.issue_id = ?", issue.ID).
 		And("`comment`.type in (?,?,?)", CommentTypeComment, CommentTypeCode, CommentTypeReview).
+		And("`review`.type != ?", ReviewTypePending).
 		And("`user`.is_active = ?", true).
 		And("`user`.prohibit_login = ?", false).
 		Join("INNER", "`user`", "`user`.id = `comment`.poster_id").
+		Join("INNER", "`review`", "`review`.reviewer_id = `user`.id").
 		Distinct("poster_id").
 		Find(&userIDs); err != nil {
 		return nil, fmt.Errorf("get poster IDs: %w", err)

models/issues/issue_test.go

@@ -85,6 +85,7 @@ func TestGetIssuesByIDs(t *testing.T) {
 }
 
 func TestGetParticipantIDsByIssue(t *testing.T) {
+	defer unittest.OverrideFixtures("models/issues/TestGetParticipantIDsByIssue")()
 	require.NoError(t, unittest.PrepareTestDatabase())
 
 	checkParticipants := func(issueID int64, userIDs []int) {
@@ -107,6 +108,7 @@ func TestGetParticipantIDsByIssue(t *testing.T) {
 	// User 2 only labeled issue1 (see fixtures/comment.yml)
 	// Users 3 and 5 made actual comments (see fixtures/comment.yml)
 	// User 3 is inactive, thus not active participant
+	// User 10 has a pending review, thus not an active participant, yet (see TestGetParticipantIDsByIssue/comment.yml)
 	checkParticipants(1, []int{1, 5})
 }
 

routers/web/repo/issue.go

@@ -1654,17 +1654,27 @@ func ViewIssue(ctx *context.Context) {
 		ctx.ServerError("LoadAttachmentsByIssue", err)
 		return
 	}
+
 	if err := issue.Comments.LoadPosters(ctx); err != nil {
 		ctx.ServerError("LoadPosters", err)
 		return
 	}
 
+	if err := issue.Comments.LoadReviews(ctx); err != nil {
+		ctx.ServerError("LoadReviews", err)
+		return
+	}
+
 	for commentIdx, comment = range issue.Comments {
 		comment.Issue = issue
 		metas := ctx.Repo.Repository.ComposeMetas(ctx)
 		metas["scope"] = fmt.Sprintf("comment-%d", commentIdx)
 
-		if comment.Type == issues_model.CommentTypeComment || comment.Type == issues_model.CommentTypeReview {
+		if comment.Review != nil && comment.Review.Type == issues_model.ReviewTypePending {
+			continue
+		}
+
+		if comment.Type == issues_model.CommentTypeComment {
 			comment.RenderedContent, err = markdown.RenderString(&markup.RenderContext{
 				Links: markup.Links{
 					Base: ctx.Repo.RepoLink,

services/mailer/fixtures/TestCloseIssue/comment.yml

@@ -0,0 +1,10 @@
+- id: 1001
+  type: 21 # code comment
+  poster_id: 10
+  issue_id: 1
+  review_id: 1001
+  content: "Some code comment that is pending to be published"
+  line: -4
+  tree_path: "README.md"
+  created_unix: 946684812
+  invalidated: false

services/mailer/fixtures/TestCloseIssue/review.yml

@@ -0,0 +1,7 @@
+- id: 1001
+  type: 0 # Pending review
+  reviewer_id: 10
+  issue_id: 1
+  content: "Pending review for issue 1"
+  updated_unix: 946684810
+  created_unix: 946684810

services/mailer/mail_issue_test.go

@@ -18,6 +18,7 @@ import (
 )
 
 func TestCloseIssue(t *testing.T) {
+	defer unittest.OverrideFixtures("services/mailer/fixtures/TestCloseIssue")()
 	defer require.NoError(t, unittest.PrepareTestDatabase())
 
 	called := false

tests/integration/fixtures/TestPullRequestParticipants/comment.yml

@@ -0,0 +1,10 @@
+- id: 1001
+  type: 21 # code comment
+  poster_id: 10
+  issue_id: 2
+  review_id: 1001
+  content: "Some code comment that is pending to be published"
+  line: -4
+  tree_path: "README.md"
+  created_unix: 946684812
+  invalidated: false

tests/integration/fixtures/TestPullRequestParticipants/review.yml

@@ -0,0 +1,7 @@
+- id: 1001
+  type: 0 # Pending review
+  reviewer_id: 10
+  issue_id: 2
+  content: "Pending review for issue 2"
+  updated_unix: 946684810
+  created_unix: 946684810

tests/integration/pull_review_test.go

@@ -64,6 +64,20 @@ func TestPullView_ReviewerMissed(t *testing.T) {
 	assert.True(t, test.IsNormalPageCompleted(resp.Body.String()))
 }
 
+func TestPullRequestParticipants(t *testing.T) {
+	defer unittest.OverrideFixtures("tests/integration/fixtures/TestPullRequestParticipants")()
+	defer tests.PrepareTestEnv(t)()
+	session := loginUser(t, "user1")
+
+	req := NewRequest(t, "GET", "/user2/repo1/pulls/2")
+	resp := session.MakeRequest(t, req, http.StatusOK)
+	assert.Contains(t, resp.Body.String(), "2 participants")
+	assert.Contains(t, resp.Body.String(), `<a href="/user1" data-tooltip-content="user1">`)
+	assert.Contains(t, resp.Body.String(), `<a href="/user2" data-tooltip-content="user2">`)
+	// does not contain user10 which has a pending review for this issue
+	assert.NotContains(t, resp.Body.String(), `<a href="/user10" data-tooltip-content="user10">`)
+}
+
 func loadComment(t *testing.T, commentID string) *issues_model.Comment {
 	t.Helper()
 	id, err := strconv.ParseInt(commentID, 10, 64)