Repository: palantir/policy-bot
Pull request #457 (2022-08-09 22:59:23)

option: allow non author contributor
Closes: https://github.com/palantir/policy-bot/issues/133

Similar to @ajlake, we ran into this today when trying to define a particular rule that could be approved by someone who had worked (by commits not suggested commits) with an individual on a PR but was not the PR author. After trying the combination below, I was pretty surprised to find in the documentation that a user could approve their own PR even if `allow_author: false`. This PR attempts to correct that.

```
    options:
      allow_contributor: true
      allow_author: false
```

Maybe this is actually desirable behavior for people using a comment driven workflow, but it differs greatly from a review driven workflow where an author isn't allowed to review their own PR anyway. So I think for the folks that prefer to use github review approval, we need a way to say allow contributors that doesn't include the author. Although the existing option would work, what if I decided later I want to use both github reviews and comment patterns then I'd get surprising behavior.

Adding this new option, allows us to not break the existing behavior for anyone who was currently using it this way while adding additional functionality for comment driven workflows. I tried to add every test case I could think of to make sure I got the logic right. Let me know if I missed something or if there's a better name for the option.


Patch:
diff --git a/README.md b/README.md
index 03a8dda..bbc1028 100644
--- a/README.md
+++ b/README.md
@@ -295,6 +295,13 @@ options:
   # always wins. False by default.
   allow_contributor: false
 
+  # If true, the approvals of someone who has committed to the pull request are
+  # considered when calculating the status. In this case, pull request author is NOT
+  # considered a contributor. If combined with any combination of allow_author: true
+  # or allow_contributors: true, then the pull request author IS considered when
+  # calculating approval. False by default.
+  allow_non_author_contributor: false
+
   # If true, pushing new commits to a pull request will invalidate existing
   # approvals for this rule. False by default.
   invalidate_on_push: false
diff --git a/policy/approval/approve.go b/policy/approval/approve.go
index 11bbce0..ec42ad3 100644
--- a/policy/approval/approve.go
+++ b/policy/approval/approve.go
@@ -37,9 +37,10 @@ type Rule struct {
 }
 
 type Options struct {
-	AllowAuthor      bool `yaml:"allow_author"`
-	AllowContributor bool `yaml:"allow_contributor"`
-	InvalidateOnPush bool `yaml:"invalidate_on_push"`
+	AllowAuthor               bool `yaml:"allow_author"`
+	AllowContributor          bool `yaml:"allow_contributor"`
+	AllowNonAuthorContributor bool `yaml:"allow_non_author_contributor"`
+	InvalidateOnPush          bool `yaml:"invalidate_on_push"`
 
 	IgnoreEditedComments bool          `yaml:"ignore_edited_comments"`
 	IgnoreUpdateMerges   bool          `yaml:"ignore_update_merges"`
@@ -189,12 +190,13 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string
 	// "author" is the user who opened the PR
 	// if contributors are allowed, the author counts as a contributor
 	author := prctx.Author()
+
 	if !r.Options.AllowAuthor && !r.Options.AllowContributor {
 		banned[author] = true
 	}
 
 	// "contributor" is any user who added a commit to the PR
-	if !r.Options.AllowContributor {
+	if !r.Options.AllowContributor && !r.Options.AllowNonAuthorContributor {
 		commits, err := r.filteredCommits(ctx, prctx)
 		if err != nil {
 			return false, "", err


Test patch:
diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go
index 311b4cd..a62ed60 100644
--- a/policy/approval/approve_test.go
+++ b/policy/approval/approve_test.go
@@ -184,6 +184,22 @@ func TestIsApproved(t *testing.T) {
 		assertApproved(t, prctx, r, "Approved by comment-approver, review-approver")
 	})
 
+	t.Run("authorCanApprove", func(t *testing.T) {
+		prctx := basePullContext()
+		r := &Rule{
+			Options: Options{
+				AllowAuthor: true,
+			},
+			Requires: Requires{
+				Count: 1,
+				Actors: common.Actors{
+					Organizations: []string{"everyone"},
+				},
+			},
+		}
+		assertApproved(t, prctx, r, "Approved by comment-approver, mhaypenny, review-approver")
+	})
+
 	t.Run("contributorsCannotApprove", func(t *testing.T) {
 		prctx := basePullContext()
 		r := &Rule{
@@ -200,11 +216,62 @@ func TestIsApproved(t *testing.T) {
 		assertApproved(t, prctx, r, "Approved by comment-approver, review-approver")
 	})
 
-	t.Run("contributorsCanApprove", func(t *testing.T) {
+	t.Run("contributorsIncludingAuthorCanApprove", func(t *testing.T) {
 		prctx := basePullContext()
 		r := &Rule{
 			Options: Options{
 				AllowContributor: true,
+				AllowAuthor:      false,
+			},
+			Requires: Requires{
+				Count: 1,
+				Actors: common.Actors{
+					Organizations: []string{"everyone"},
+				},
+			},
+		}
+		assertApproved(t, prctx, r, "Approved by comment-approver, mhaypenny, contributor-author, contributor-committer, review-approver")
+	})
+
+	t.Run("contributorsExcludingAuthorCanApprove", func(t *testing.T) {
+		prctx := basePullContext()
+		r := &Rule{
+			Options: Options{
+				AllowNonAuthorContributor: true,
+			},
+			Requires: Requires{
+				Count: 1,
+				Actors: common.Actors{
+					Organizations: []string{"everyone"},
+				},
+			},
+		}
+		assertApproved(t, prctx, r, "Approved by comment-approver, contributor-author, contributor-committer, review-approver")
+	})
+
+	t.Run("nonAuthorContributorsAndAuthorCanApprove", func(t *testing.T) {
+		prctx := basePullContext()
+		r := &Rule{
+			Options: Options{
+				AllowNonAuthorContributor: true,
+				AllowAuthor:               true,
+			},
+			Requires: Requires{
+				Count: 1,
+				Actors: common.Actors{
+					Organizations: []string{"everyone"},
+				},
+			},
+		}
+		assertApproved(t, prctx, r, "Approved by comment-approver, mhaypenny, contributor-author, contributor-committer, review-approver")
+	})
+
+	t.Run("contributorsAndAuthorCanApprove", func(t *testing.T) {
+		prctx := basePullContext()
+		r := &Rule{
+			Options: Options{
+				AllowNonAuthorContributor: true,
+				AllowContributor:          true,
 			},
 			Requires: Requires{
 				Count: 1,