From 179704695c2c50b3577a25f16d7344eadb3b2da3 Mon Sep 17 00:00:00 2001 From: Cory Todd <2796559+corytodd@users.noreply.github.com> Date: Fri, 9 Jun 2023 07:34:49 -0700 Subject: [PATCH] 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 Co-authored-by: Giteabot --- models/fixtures/review.yml | 27 +++++++++++++++++++++++++++ models/issues/pull.go | 2 +- models/issues/pull_test.go | 12 ++++++++++++ models/issues/review.go | 21 +++++++++++++++++++++ models/issues/review_test.go | 12 ++++++++++++ 5 files changed, 73 insertions(+), 1 deletion(-) diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index d44d0cde98..cc2c7e06e7 100644 --- a/models/fixtures/review.yml +++ b/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 diff --git a/models/issues/pull.go b/models/issues/pull.go index 63bc258d2f..23b938f95a 100644 --- a/models/issues/pull.go +++ b/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, diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 1eb106047c..5856b5dc58 100644 --- a/models/issues/pull_test.go +++ b/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 \nReviewed-by: User Six \n" + assert.EqualValues(t, expected, approvers) +} diff --git a/models/issues/review.go b/models/issues/review.go index 06cf132a48..3a1ab7468a 100644 --- a/models/issues/review.go +++ b/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{}) diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 0cb621812c..de7b6b2902 100644 --- a/models/issues/review_test.go +++ b/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})