From 065aab8bf998f5e1e5e9c0c552d30d445120c739 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Mon, 26 Nov 2018 15:38:32 -0800 Subject: [PATCH] Consider only one review event per user (#35) Previously, if a user left multiple approval comments or left a comment and a review, their approval would count twice. Now, review candidates are deduplicated so that only the most recent for each user is considered. As a result, add the LastModified field to test data so that generated strings have a consistent order. --- policy/approval/approve_test.go | 62 +++++++++++++++++++-------------- policy/common/methods.go | 23 +++++++++++- policy/common/methods_test.go | 21 +++++++++++ 3 files changed, 79 insertions(+), 27 deletions(-) diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index 60c4bf54..7ddbde42 100644 --- a/policy/approval/approve_test.go +++ b/policy/approval/approve_test.go @@ -18,6 +18,7 @@ import ( "context" "os" "testing" + "time" "github.com/rs/zerolog" "github.com/stretchr/testify/assert" @@ -32,52 +33,61 @@ func TestIsApproved(t *testing.T) { logger := zerolog.New(os.Stdout) ctx := logger.WithContext(context.Background()) + now := time.Now() prctx := &pulltest.Context{ AuthorValue: "mhaypenny", CommentsValue: []*pull.Comment{ { - Order: 10, - Author: "other-user", - Body: "Why did you do this?", + Order: 10, + Author: "other-user", + Body: "Why did you do this?", + LastModified: now.Add(1 * time.Second), }, { - Order: 20, - Author: "comment-approver", - Body: "LGTM :+1: :shipit:", + Order: 20, + Author: "comment-approver", + Body: "LGTM :+1: :shipit:", + LastModified: now.Add(2 * time.Second), }, { - Order: 30, - Author: "disapprover", - Body: "I don't like things! :-1:", + Order: 30, + Author: "disapprover", + Body: "I don't like things! :-1:", + LastModified: now.Add(3 * time.Second), }, { - Order: 40, - Author: "mhaypenny", - Body: ":+1: my stuff is cool", + Order: 40, + Author: "mhaypenny", + Body: ":+1: my stuff is cool", + LastModified: now.Add(4 * time.Second), }, { - Order: 50, - Author: "contributor-author", - Body: ":+1: I added to this PR", + Order: 50, + Author: "contributor-author", + Body: ":+1: I added to this PR", + LastModified: now.Add(5 * time.Second), }, { - Order: 60, - Author: "contributor-committer", - Body: ":+1: I also added to this PR", + Order: 60, + Author: "contributor-committer", + Body: ":+1: I also added to this PR", + LastModified: now.Add(6 * time.Second), }, }, ReviewsValue: []*pull.Review{ { - Order: 70, - Author: "disapprover", - State: pull.ReviewChangesRequested, - Body: "I _really_ don't like things!", + Order: 70, + Author: "disapprover", + State: pull.ReviewChangesRequested, + Body: "I _really_ don't like things!", + LastModified: now.Add(7 * time.Second), }, { - Order: 80, - Author: "review-approver", - State: pull.ReviewApproved, - Body: "I LIKE THIS", + Order: 80, + Author: "review-approver", + State: pull.ReviewApproved, + Body: "I LIKE THIS", + LastModified: now.Add(8 * time.Second), }, }, CommitsValue: []*pull.Commit{ diff --git a/policy/common/methods.go b/policy/common/methods.go index 1a038086..63d8ea89 100644 --- a/policy/common/methods.go +++ b/policy/common/methods.go @@ -46,6 +46,10 @@ func (cs CandidatesByModifiedTime) Less(i, j int) bool { return cs[i].LastModified.Before(cs[j].LastModified) } +// Candidates returns a list of user candidates based on the configured +// methods. A given user will appear at most once in the list. If that user has +// taken multiple actions that match the methods, only the most recent by event +// order is included. The order of the candidates is unspecified. func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candidate, error) { var candidates []*Candidate @@ -83,7 +87,24 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid } } - return candidates, nil + return deduplicateCandidates(candidates), nil +} + +func deduplicateCandidates(all []*Candidate) []*Candidate { + users := make(map[string]*Candidate) + for _, c := range all { + last, ok := users[c.User] + if !ok || last.Order < c.Order { + users[c.User] = c + } + } + + candidates := make([]*Candidate, 0, len(users)) + for _, c := range users { + candidates = append(candidates, c) + } + + return candidates } func (m *Methods) CommentMatches(commentBody string) bool { diff --git a/policy/common/methods_test.go b/policy/common/methods_test.go index 0b0a405e..0c369ca9 100644 --- a/policy/common/methods_test.go +++ b/policy/common/methods_test.go @@ -34,28 +34,34 @@ func TestCandidates(t *testing.T) { { Body: "I like to comment!", Author: "rrandom", + Order: 0, }, { Body: "Looks good to me :+1:", Author: "mhaypenny", + Order: 2, }, { Body: ":lgtm:", Author: "ttest", + Order: 4, }, }, ReviewsValue: []*pull.Review{ { Author: "rrandom", State: pull.ReviewCommented, + Order: 1, }, { Author: "mhaypenny", State: pull.ReviewChangesRequested, + Order: 3, }, { Author: "ttest", State: pull.ReviewApproved, + Order: 5, }, }, } @@ -85,6 +91,21 @@ func TestCandidates(t *testing.T) { require.Len(t, cs, 1, "incorrect number of candidates found") assert.Equal(t, "mhaypenny", cs[0].User) }) + + t.Run("deduplicate", func(t *testing.T) { + m := &Methods{ + Comments: []string{":+1:", ":lgtm:"}, + GithubReview: true, + GithubReviewState: pull.ReviewApproved, + } + + cs, err := m.Candidates(ctx, prctx) + require.NoError(t, err) + + require.Len(t, cs, 2, "incorrect number of candidates found") + assert.Equal(t, "mhaypenny", cs[0].User) + assert.Equal(t, "ttest", cs[1].User) + }) } func TestCandidatesByLastModified(t *testing.T) {