Fix duplicate Reviewed-by trailers (#24796)

Enable deduplication of unofficial reviews. When pull requests are
configured to include all approvers, not just official ones, in the
default merge messages it was possible to generate duplicated
Reviewed-by lines for a single person. Add an option to find only
distinct reviews for a given query.

fixes #24795

---------

Signed-off-by: Cory Todd <[email protected]>
Co-authored-by: Giteabot <[email protected]>

Author: corytodd

models/fixtures/review.yml

@@ -105,3 +105,30 @@
   official: true
   updated_unix: 1603196749
   created_unix: 1603196749
+
+-
+  id: 13
+  type: 1
+  reviewer_id: 5
+  issue_id: 11
+  content: "old review from user5"
+  updated_unix: 946684820
+  created_unix: 946684820
+
+-
+  id: 14
+  type: 1
+  reviewer_id: 5
+  issue_id: 11
+  content: "duplicate review from user5 (latest)"
+  updated_unix: 946684830
+  created_unix: 946684830
+
+-
+  id: 15
+  type: 1
+  reviewer_id: 6
+  issue_id: 11
+  content: "singular review from user6 and final review for this pr"
+  updated_unix: 946684831
+  created_unix: 946684831

models/issues/pull.go

@@ -404,7 +404,7 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error {
 	defer committer.Close()
 
 	// Note: This doesn't page as we only expect a very limited number of reviews
-	reviews, err := FindReviews(ctx, FindReviewOptions{
+	reviews, err := FindLatestReviews(ctx, FindReviewOptions{
 		Type:         ReviewTypeApprove,
 		IssueID:      pr.IssueID,
 		OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly,

models/issues/pull_test.go

@@ -10,6 +10,7 @@ import (
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/setting"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -325,3 +326,14 @@ func TestParseCodeOwnersLine(t *testing.T) {
 		assert.Equal(t, g.Tokens, tokens, "Codeowners tokenizer failed")
 	}
 }
+
+func TestGetApprovers(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 5})
+	// Official reviews are already deduplicated. Allow unofficial reviews
+	// to assert that there are no duplicated approvers.
+	setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly = false
+	approvers := pr.GetApprovers()
+	expected := "Reviewed-by: User Five <[email protected]>\nReviewed-by: User Six <[email protected]>\n"
+	assert.EqualValues(t, expected, approvers)
+}

models/issues/review.go

@@ -275,6 +275,27 @@ func FindReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error)
 		Find(&reviews)
 }
 
+// FindLatestReviews returns only latest reviews per user, passing FindReviewOptions
+func FindLatestReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error) {
+	reviews := make([]*Review, 0, 10)
+	cond := opts.toCond()
+	sess := db.GetEngine(ctx).Where(cond)
+	if opts.Page > 0 {
+		sess = db.SetSessionPagination(sess, &opts)
+	}
+
+	sess.In("id", builder.
+		Select("max ( id ) ").
+		From("review").
+		Where(cond).
+		GroupBy("reviewer_id"))
+
+	return reviews, sess.
+		Asc("created_unix").
+		Asc("id").
+		Find(&reviews)
+}
+
 // CountReviews returns count of reviews passing FindReviewOptions
 func CountReviews(opts FindReviewOptions) (int64, error) {
 	return db.GetEngine(db.DefaultContext).Where(opts.toCond()).Count(&Review{})

models/issues/review_test.go

@@ -71,6 +71,18 @@ func TestFindReviews(t *testing.T) {
 	assert.Equal(t, "Demo Review", reviews[0].Content)
 }
 
+func TestFindLatestReviews(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	reviews, err := issues_model.FindLatestReviews(db.DefaultContext, issues_model.FindReviewOptions{
+		Type:    issues_model.ReviewTypeApprove,
+		IssueID: 11,
+	})
+	assert.NoError(t, err)
+	assert.Len(t, reviews, 2)
+	assert.Equal(t, "duplicate review from user5 (latest)", reviews[0].Content)
+	assert.Equal(t, "singular review from user6 and final review for this pr", reviews[1].Content)
+}
+
 func TestGetCurrentReview(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})