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

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

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 {

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