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 <cory.todd@canonical.com>
Co-authored-by: Giteabot <teabot@gitea.io>
This commit is contained in:
Cory Todd 2023-06-09 07:34:49 -07:00 committed by GitHub
parent 81211db077
commit 179704695c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 73 additions and 1 deletions

View file

@ -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

View file

@ -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,

View file

@ -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)
}

View file

@ -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{})

View file

@ -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})