Fix re-request loop for user reviewers (#302)

If a user was requested for review, but their review didn't change the
status of the policy for some reason, policy-bot could immediately
re-request them as a reviewer. This was most likely to happen if a user
left a "request changes" review on a repository that does not have a
disapproval policy.

To fix this, look at all reviews on the current commit and exclude their
authors from the list of requests. If the pull request is updated in the
future, the users become eligible for requesting again. Because we
already list reviews for basic policy evaluation, this shouldn't add any
new requests.

Author: bluekeyes

pull/context.go

@@ -201,9 +201,7 @@ type Review struct {
 	Author    string
 	State     ReviewState
 	Body      string
-
-	// ID is the GitHub node ID of the review, used to resolve dismissals
-	ID string
+	SHA       string
 }
 
 type ReviewerType string

pull/github.go

@@ -746,6 +746,7 @@ func (ghc *GitHubContext) loadPagedData() error {
 			switch r.State {
 			case "COMMENTED":
 				comments = append(comments, r.ToComment())
+				fallthrough
 			case "APPROVED", "CHANGES_REQUESTED":
 				reviews = append(reviews, r.ToReview())
 			}
@@ -982,6 +983,9 @@ type v4PullRequestReview struct {
 	State       string
 	Body        string
 	SubmittedAt time.Time
+	Commit      struct {
+		OID string
+	}
 }
 
 func (r *v4PullRequestReview) ToReview() *Review {
@@ -990,6 +994,7 @@ func (r *v4PullRequestReview) ToReview() *Review {
 		Author:    r.Author.GetV3Login(),
 		State:     ReviewState(strings.ToLower(r.State)),
 		Body:      r.Body,
+		SHA:       r.Commit.OID,
 	}
 }
 

pull/github_test.go

@@ -193,7 +193,7 @@ func TestReviews(t *testing.T) {
 	reviews, err := ctx.Reviews()
 	require.NoError(t, err)
 
-	require.Len(t, reviews, 2, "incorrect number of reviews")
+	require.Len(t, reviews, 3, "incorrect number of reviews")
 	assert.Equal(t, 2, dataRule.Count, "no http request was made")
 
 	expectedTime, err := time.Parse(time.RFC3339, "2018-06-27T20:33:26Z")
@@ -209,11 +209,16 @@ func TestReviews(t *testing.T) {
 	assert.Equal(t, ReviewApproved, reviews[1].State)
 	assert.Equal(t, "the body", reviews[1].Body)
 
+	assert.Equal(t, "jgiannuzzi", reviews[2].Author)
+	assert.Equal(t, expectedTime.Add(-4*time.Second).Add(5*time.Minute), reviews[2].CreatedAt)
+	assert.Equal(t, ReviewCommented, reviews[2].State)
+	assert.Equal(t, "A review comment", reviews[2].Body)
+
 	// verify that the review list is cached
 	reviews, err = ctx.Reviews()
 	require.NoError(t, err)
 
-	require.Len(t, reviews, 2, "incorrect number of reviews")
+	require.Len(t, reviews, 3, "incorrect number of reviews")
 	assert.Equal(t, 2, dataRule.Count, "cached reviews were not used")
 }
 
@@ -373,7 +378,7 @@ func TestMixedReviewCommentPaging(t *testing.T) {
 
 	assert.Equal(t, 2, dataRule.Count, "cached values were not used")
 	assert.Len(t, comments, 3, "incorrect number of comments")
-	assert.Len(t, reviews, 2, "incorrect number of reviews")
+	assert.Len(t, reviews, 3, "incorrect number of reviews")
 }
 
 func TestIsOrgMember(t *testing.T) {

server/handler/base.go

@@ -299,12 +299,32 @@ func (b *Base) requestReviews(ctx context.Context, prctx pull.Context, client *g
 	logger.Debug().Msgf("Waiting for %s to spread out reviewer processing", delay)
 	time.Sleep(delay)
 
-	// check again if someone assigned a reviewer while we were calculating users to request
+	// Get existing reviewers _after_ computing the selection and sleeping in
+	// case something assigned reviewers (or left reviews) in the meantime.
 	reviewers, err := prctx.RequestedReviewers()
 	if err != nil {
 		return err
 	}
 
+	// After a user leaves a review, GitHub removes the user from the request
+	// list. If the review didn't actually change the state of the requesting
+	// rule, policy-bot may request the same user again. To avoid this, include
+	// any reviews on the head commit in the set of existing reviewers to avoid
+	// re-requesting them until the content changes.
+	head := prctx.HeadSHA()
+	reviews, err := prctx.Reviews()
+	if err != nil {
+		return err
+	}
+	for _, r := range reviews {
+		if r.SHA == head {
+			reviewers = append(reviewers, &pull.Reviewer{
+				Type: pull.ReviewerUser,
+				Name: r.Author,
+			})
+		}
+	}
+
 	if diff := selection.Difference(reviewers); !diff.IsEmpty() {
 		req := selectionToReviewersRequest(diff)
 		logger.Debug().