Repository: palantir/policy-bot (go)

Issue:
Stop re-requesting reviews when all-users mode is used
We've encountered the following issues on a GitHub repository today:
- We have a PR that modifies a single file.
- That file is configured to require approval from a GitHub team, and the requesting mode is set to `all-users`.
- policy-bot requests the review from all the users of that GitHub team, as expected.
- A member reviews the PR (in this case the PR is rejected asking for changes).
- policy-bot immediately re-requests a review from the member that reviewed.

This triggers more email notifications than are necessary, but more importantly the state the PR is left in looks like it needs to be reviewed (i.e. action pending on the reviewing team) when it actually isn't.

Pull request description:
Fix re-request loop for user reviewers
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.

Fixes #263.

Patch:
diff --git a/policy/predicate/file.go b/policy/predicate/file.go
index 918ab6d..e832692 100644
--- a/policy/predicate/file.go
+++ b/policy/predicate/file.go
@@ -15,9 +15,9 @@
 package predicate
 
 import (
+	"bytes"
 	"context"
 	"fmt"
-	"regexp"
 	"strconv"
 
 	"github.com/palantir/policy-bot/policy/common"
@@ -96,35 +96,80 @@ type ModifiedLines struct {
 	Total     ComparisonExpr `yaml:"total"`
 }
 
-type ComparisonExpr string
+type CompareOp uint8
 
-var (
-	numCompRegexp = regexp.MustCompile(`^(<|>) ?(\d+)$`)
+const (
+	OpNone CompareOp = iota
+	OpLessThan
+	OpGreaterThan
 )
 
+type ComparisonExpr struct {
+	Op    CompareOp
+	Value int64
+}
+
 func (exp ComparisonExpr) IsEmpty() bool {
-	return exp == ""
+	return exp.Op == OpNone && exp.Value == 0
 }
 
-func (exp ComparisonExpr) Evaluate(n int64) (bool, error) {
-	match := numCompRegexp.FindStringSubmatch(string(exp))
-	if match == nil {
-		return false, errors.Errorf("invalid comparison expression: %q", exp)
+func (exp ComparisonExpr) Evaluate(n int64) bool {
+	switch exp.Op {
+	case OpLessThan:
+		return n < exp.Value
+	case OpGreaterThan:
+		return n > exp.Value
 	}
+	return false
+}
 
-	op := match[1]
-	v, err := strconv.ParseInt(match[2], 10, 64)
-	if err != nil {
-		return false, errors.Wrapf(err, "invalid commparison expression: %q", exp)
+func (exp ComparisonExpr) MarshalText() ([]byte, error) {
+	if exp.Op == OpNone {
+		return nil, nil
 	}
 
-	switch op {
-	case "<":
-		return n < v, nil
-	case ">":
-		return n > v, nil
+	var op string
+	switch exp.Op {
+	case OpLessThan:
+		op = "<"
+	case OpGreaterThan:
+		op = ">"
+	default:
+		return nil, errors.Errorf("unknown operation: %d", exp.Op)
 	}
-	return false, errors.Errorf("invalid comparison expression: %q", exp)
+	return []byte(fmt.Sprintf("%s %d", op, exp.Value)), nil
+}
+
+func (exp *ComparisonExpr) UnmarshalText(text []byte) error {
+	text = bytes.TrimSpace(text)
+	if len(text) == 0 {
+		*exp = ComparisonExpr{}
+		return nil
+	}
+
+	i := 0
+	var op CompareOp
+	switch text[i] {
+	case '<':
+		op = OpLessThan
+	case '>':
+		op = OpGreaterThan
+	default:
+		return errors.Errorf("invalid comparison operator: %c", text[i])
+	}
+
+	i++
+	for i < len(text) && (text[i] == ' ' || text[i] == '\t') {
+		i++
+	}
+
+	v, err := strconv.ParseInt(string(text[i:]), 10, 64)
+	if err != nil {
+		return errors.Wrapf(err, "invalid comparison value")
+	}
+
+	*exp = ComparisonExpr{Op: op, Value: v}
+	return nil
 }
 
 func (pred *ModifiedLines) Evaluate(ctx context.Context, prctx pull.Context) (bool, string, error) {
@@ -144,14 +189,8 @@ func (pred *ModifiedLines) Evaluate(ctx context.Context, prctx pull.Context) (bo
 		pred.Deletions: deletions,
 		pred.Total:     additions + deletions,
 	} {
-		if !expr.IsEmpty() {
-			ok, err := expr.Evaluate(v)
-			if err != nil {
-				return false, "", err
-			}
-			if ok {
-				return true, "", nil
-			}
+		if !expr.IsEmpty() && expr.Evaluate(v) {
+			return true, "", nil
 		}
 	}
 	return false, fmt.Sprintf("modification of (+%d, -%d) does not match any conditions", additions, deletions), nil
diff --git a/pull/context.go b/pull/context.go
index 16d164b..965a211 100644
--- a/pull/context.go
+++ b/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
diff --git a/pull/github.go b/pull/github.go
index 425c3bb..5c62c90 100644
--- a/pull/github.go
+++ b/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,
 	}
 }
 
diff --git a/server/handler/base.go b/server/handler/base.go
index f653c92..4eb029b 100644
--- a/server/handler/base.go
+++ b/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().
diff --git a/server/handler/details.go b/server/handler/details.go
index cf1f561..9c69353 100644
--- a/server/handler/details.go
+++ b/server/handler/details.go
@@ -24,6 +24,7 @@ import (
 	"github.com/alexedwards/scs"
 	"github.com/bluekeyes/templatetree"
 	"github.com/google/go-github/v32/github"
+	"github.com/palantir/go-githubapp/githubapp"
 	"github.com/palantir/policy-bot/policy/common"
 	"github.com/palantir/policy-bot/pull"
 	"github.com/pkg/errors"
@@ -44,12 +45,16 @@ func (h *Details) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 
 	number, err := strconv.Atoi(pat.Param(r, "number"))
 	if err != nil {
-		http.Error(w, fmt.Sprintf("invalid pull request number: %v", err), http.StatusBadRequest)
+		http.Error(w, fmt.Sprintf("Invalid pull request number: %v", err), http.StatusBadRequest)
 		return nil
 	}
 
 	installation, err := h.Installations.GetByOwner(ctx, owner)
 	if err != nil {
+		if _, notFound := err.(githubapp.InstallationNotFound); notFound {
+			h.render404(w, owner, repo, number)
+			return nil
+		}
 		return err
 	}
 
@@ -72,7 +77,7 @@ func (h *Details) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 	level, _, err := client.Repositories.GetPermissionLevel(ctx, owner, repo, user)
 	if err != nil {
 		if isNotFound(err) {
-			http.Error(w, fmt.Sprintf("not found: %s/%s#%d", owner, repo, number), http.StatusNotFound)
+			h.render404(w, owner, repo, number)
 			return nil
 		}
 		return errors.Wrap(err, "failed to get user permission level")
@@ -80,14 +85,14 @@ func (h *Details) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 
 	// if the user does not have permission, pretend the repo/PR doesn't exist
 	if level.GetPermission() == "none" {
-		http.Error(w, fmt.Sprintf("not found: %s/%s#%d", owner, repo, number), http.StatusNotFound)
+		h.render404(w, owner, repo, number)
 		return nil
 	}
 
 	pr, _, err := client.PullRequests.Get(ctx, owner, repo, number)
 	if err != nil {
 		if isNotFound(err) {
-			http.Error(w, fmt.Sprintf("not found: %s/%s#%d", owner, repo, number), http.StatusNotFound)
+			h.render404(w, owner, repo, number)
 			return nil
 		}
 		return errors.Wrap(err, "failed to get pull request")
@@ -156,6 +161,14 @@ func (h *Details) render(w http.ResponseWriter, data interface{}) error {
 	return h.Templates.ExecuteTemplate(w, "details.html.tmpl", data)
 }
 
+func (h *Details) render404(w http.ResponseWriter, owner, repo string, number int) {
+	msg := fmt.Sprintf(
+		"Not Found: %s/%s#%d\n\nThe repository or pull request does not exist, you do not have permission, or policy-bot is not installed.",
+		owner, repo, number,
+	)
+	http.Error(w, msg, http.StatusNotFound)
+}
+
 func getPolicyURL(pr *github.PullRequest, config FetchedConfig) string {
 	base := pr.GetBase().GetRepo().GetHTMLURL()
 	if u, _ := url.Parse(base); u != nil {


Test patch:
diff --git a/policy/predicate/file_test.go b/policy/predicate/file_test.go
index 73fd0ab..6a8477e 100644
--- a/policy/predicate/file_test.go
+++ b/policy/predicate/file_test.go
@@ -176,8 +176,8 @@ func TestOnlyChangedFiles(t *testing.T) {
 
 func TestModifiedLines(t *testing.T) {
 	p := &ModifiedLines{
-		Additions: ">100",
-		Deletions: ">10",
+		Additions: ComparisonExpr{Op: OpGreaterThan, Value: 100},
+		Deletions: ComparisonExpr{Op: OpGreaterThan, Value: 10},
 	}
 
 	runFileTests(t, p, []FileTestCase{
@@ -208,7 +208,7 @@ func TestModifiedLines(t *testing.T) {
 	})
 
 	p = &ModifiedLines{
-		Total: ">100",
+		Total: ComparisonExpr{Op: OpGreaterThan, Value: 100},
 	}
 
 	runFileTests(t, p, []FileTestCase{
@@ -230,55 +230,89 @@ func TestComparisonExpr(t *testing.T) {
 		Expr   ComparisonExpr
 		Value  int64
 		Output bool
-		Err    bool
 	}{
 		"greaterThanTrue": {
-			Expr:   ">100",
+			Expr:   ComparisonExpr{Op: OpGreaterThan, Value: 100},
 			Value:  200,
 			Output: true,
 		},
 		"greaterThanFalse": {
-			Expr:   "> 100",
+			Expr:   ComparisonExpr{Op: OpGreaterThan, Value: 100},
 			Value:  50,
 			Output: false,
 		},
 		"lessThanTrue": {
-			Expr:   "<100",
+			Expr:   ComparisonExpr{Op: OpLessThan, Value: 100},
 			Value:  50,
 			Output: true,
 		},
 		"lessThanFalse": {
-			Expr:   "< 100",
+			Expr:   ComparisonExpr{Op: OpLessThan, Value: 100},
 			Value:  200,
 			Output: false,
 		},
-		"invalidOperator": {
-			Expr: "=200",
-			Err:  true,
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			ok := test.Expr.Evaluate(test.Value)
+			assert.Equal(t, test.Output, ok, "evaluation was not correct")
+		})
+	}
+
+	t.Run("isEmpty", func(t *testing.T) {
+		assert.True(t, ComparisonExpr{}.IsEmpty(), "expression was not empty")
+		assert.False(t, ComparisonExpr{Op: OpGreaterThan, Value: 100}.IsEmpty(), "expression was empty")
+	})
+
+	parseTests := map[string]struct {
+		Input  string
+		Output ComparisonExpr
+		Err    bool
+	}{
+		"lessThan": {
+			Input:  "<100",
+			Output: ComparisonExpr{Op: OpLessThan, Value: 100},
+		},
+		"greaterThan": {
+			Input:  ">100",
+			Output: ComparisonExpr{Op: OpGreaterThan, Value: 100},
+		},
+		"innerSpaces": {
+			Input:  "<   35",
+			Output: ComparisonExpr{Op: OpLessThan, Value: 35},
+		},
+		"leadingSpaces": {
+			Input:  "   < 35",
+			Output: ComparisonExpr{Op: OpLessThan, Value: 35},
+		},
+		"trailngSpaces": {
+			Input:  "< 35   ",
+			Output: ComparisonExpr{Op: OpLessThan, Value: 35},
 		},
-		"invalidNumber": {
-			Expr: ">12ab",
-			Err:  true,
+		"invalidOp": {
+			Input: "=10",
+			Err:   true,
+		},
+		"invalidValue": {
+			Input: "< 10ab",
+			Err:   true,
 		},
 	}
 
-	for name, test := range tests {
+	for name, test := range parseTests {
 		t.Run(name, func(t *testing.T) {
-			ok, err := test.Expr.Evaluate(test.Value)
+			var expr ComparisonExpr
+			err := expr.UnmarshalText([]byte(test.Input))
 			if test.Err {
-				assert.Error(t, err, "expected error evaluating expression, but got nil")
+				assert.Error(t, err, "expected error parsing expression, but got nil")
 				return
 			}
-			if assert.NoError(t, err, "unexpected error evaluating expression") {
-				assert.Equal(t, test.Output, ok, "evaluation was not correct")
+			if assert.NoError(t, err, "unexpected error parsing expression") {
+				assert.Equal(t, test.Output, expr, "parsed expression was not correct")
 			}
 		})
 	}
-
-	t.Run("isEmpty", func(t *testing.T) {
-		assert.True(t, ComparisonExpr("").IsEmpty(), "expression was not empty")
-		assert.False(t, ComparisonExpr(">100").IsEmpty(), "expression was empty")
-	})
 }
 
 type FileTestCase struct {
diff --git a/pull/github_test.go b/pull/github_test.go
index e652444..1be17c4 100644
--- a/pull/github_test.go
+++ b/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) {