Add tests for FindReviews and GetApprovers

Not all SQL dialects will work with Distinct so use a subquery grouping
instead. Add 3 reviews to the test data to simulate the scenario being
fixed by this patch.

- One reviewer submitting 2 reviews. Only the latest should be returned.
- One reviewer submitting 1 review. This is the oldest review and should
  still be the first approver.

Omitting LatestOnly should preserve existing behavior as it is used by
the REST API.

Signed-off-by: Cory Todd <cory.todd@canonical.com>

Author: Cory Todd

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

@@ -408,7 +408,7 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error {
 		Type:         ReviewTypeApprove,
 		IssueID:      pr.IssueID,
 		OfficialOnly: setting.Repository.PullRequest.DefaultMergeMessageOfficialApproversOnly,
-		Distinct:     true,
+		LatestOnly:   true,
 	})
 	if err != nil {
 		log.Error("Unable to FindReviews for PR ID %d: %v", pr.ID, err)

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 <user5@example.com>\nReviewed-by: User Six <user6@example.com>\n"
+	assert.EqualValues(t, expected, approvers)
+}

models/issues/review.go

@@ -243,7 +243,7 @@ type FindReviewOptions struct {
 	IssueID      int64
 	ReviewerID   int64
 	OfficialOnly bool
-	Distinct     bool
+	LatestOnly   bool // Latest per-reviewer
 }
 
 func (opts *FindReviewOptions) toCond() builder.Cond {
@@ -270,14 +270,15 @@ func FindReviews(ctx context.Context, opts FindReviewOptions) ([]*Review, error)
 	if opts.Page > 0 {
 		sess = db.SetSessionPagination(sess, &opts)
 	}
-	distinct := ""
-	if opts.Distinct {
-		distinct = "reviewer_id"
+	if opts.LatestOnly {
+		sess.In("id", builder.
+			Select("max ( id ) ").
+			From("review").
+			GroupBy("reviewer_id"))
 	}
 	return reviews, sess.
 		Asc("created_unix").
 		Asc("id").
-		Distinct(distinct).
 		Find(&reviews)
 }
 

models/issues/review_test.go

@@ -71,6 +71,31 @@ func TestFindReviews(t *testing.T) {
 	assert.Equal(t, "Demo Review", reviews[0].Content)
 }
 
+func TestFindReviews_NotLatestOnly(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{
+		Type:       issues_model.ReviewTypeApprove,
+		IssueID:    11,
+		LatestOnly: false,
+	})
+	assert.NoError(t, err)
+	assert.Len(t, reviews, 3)
+	assert.Equal(t, "old review from user5", reviews[0].Content)
+}
+
+func TestFindReviews_LatestOnly(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	reviews, err := issues_model.FindReviews(db.DefaultContext, issues_model.FindReviewOptions{
+		Type:       issues_model.ReviewTypeApprove,
+		IssueID:    11,
+		LatestOnly: true,
+	})
+	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})