Merge pull request #72 from form3tech-oss/wp-groups

feat: Add comment on pr when user approved request but is not member …

Author: przechrzta-f3

internal/api/api_test.go

@@ -328,9 +328,9 @@ func TestWhenPRReviewedByNonTeamMemberAndAuthorsArePartOfTeam(t *testing.T) {
 	then.
 		ExpectPendingAnswerReturned().
 		ExpectStatusPendingReported().
-		ExpectNoCommentsMade().
+		ExpectInvalidCommentCharlieIgnoredAsReviewer().
 		ExpectLabelsUpdated().
-		ExpectNoReviewRequestsMade()
+		ExpectedReviewRequestsMadeForFoo()
 }
 
 func TestGitHubTeamApproverCleansUpOldIgnoredReviewsComments(t *testing.T) {
@@ -358,6 +358,56 @@ func TestGitHubTeamApproverCleansUpOldIgnoredReviewsComments(t *testing.T) {
 		ExpectedReviewRequestsMadeForFoo()
 }
 
+func TestGitHubTeamApproverCleansUpOldInvalidReviewsComments(t *testing.T) {
+	given, when, then := stages.ApiTest(t)
+
+	given.
+		EncryptionKeyExists().
+		GitHubWebHookTokenExists().
+		FakeGHRunning().
+		OrganisationWithTeamFoo().
+		RepoWithNoContributorReviewEnabledAndFooAsApprovingTeam().
+		PullRequestExists().
+		InvalidReviewCommentsExist().
+		CommitsWithAliceAsContributor().
+		CharlieApprovesPullRequest().
+		GitHubTeamApproverRunning()
+	when.
+		SendingApprovedPRReviewSubmittedEvent()
+	then.
+		ExpectPendingAnswerReturned().
+		ExpectStatusPendingReported().
+		ExpectPreviousIgnoredReviewCommentsDeleted().
+		ExpectInvalidCommentCharlieIgnoredAsReviewer().
+		ExpectLabelsUpdated().
+		ExpectedReviewRequestsMadeForFoo()
+}
+
+func TestGitHubTeamApproverCleansUpOldInvalidAndRetainsIgnoredReviewsComments(t *testing.T) {
+	given, when, then := stages.ApiTest(t)
+
+	given.
+		EncryptionKeyExists().
+		GitHubWebHookTokenExists().
+		FakeGHRunning().
+		OrganisationWithTeamFoo().
+		RepoWithNoContributorReviewEnabledAndFooAsApprovingTeam().
+		PullRequestExists().
+		InvalidAndIgnoredReviewCommentsExist().
+		CommitsWithAliceAsContributor().
+		CharlieApprovesPullRequest().
+		GitHubTeamApproverRunning()
+	when.
+		SendingApprovedPRReviewSubmittedEvent()
+	then.
+		ExpectPendingAnswerReturned().
+		ExpectStatusPendingReported().
+		ExpectPreviousAliceIgnoredReviewCommentsRetained().
+		ExpectInvalidCommentCharlieIgnoredAsReviewer().
+		ExpectLabelsUpdated().
+		ExpectedReviewRequestsMadeForFoo()
+}
+
 func TestGitHubTeamApproverReportsInvalidTeamHandlesInConfiguration(t *testing.T) {
 	given, when, then := stages.ApiTest(t)
 

internal/api/approval/approval.go

@@ -113,6 +113,7 @@ func (a *Approval) ComputeApprovalStatus(ctx context.Context, pr *PR) (*Result,
 		}
 	}
 
+	allAllowedMembers := map[string]bool{}
 	// Check if each required team has approved the pull request.
 	for _, rule := range rules {
 		matched, err := a.isRuleMatched(ctx, rule, pr)
@@ -149,11 +150,12 @@ func (a *Approval) ComputeApprovalStatus(ctx context.Context, pr *PR) (*Result,
 				return nil, err
 			}
 
+			addMembers(allAllowedMembers, members)
+
 			allowed, ignored, err := a.allowedAndIgnoreReviewers(ctx, pr, members, rule.IgnoreContributorApproval)
 			if err != nil {
 				return nil, err
 			}
-
 			// Check whether the current team has approved the PR.
 			approvalCount := countApprovalsForTeam(reviews, allowed)
 			// Need to use full team handle here, as we'll be comparing recorded handles
@@ -164,19 +166,31 @@ func (a *Approval) ComputeApprovalStatus(ctx context.Context, pr *PR) (*Result,
 		state.addMatchedRule(mr)
 	}
 
+	state.updateInvalidReviewers(allAllowedMembers)
+
 	result := state.result(a.log, teams) // state should not be consumed past this point
 
 	if result.pendingReviewsWaiting() {
-		err := a.client.ReportIgnoredReviews(
-			ctx, pr.OwnerLogin, pr.RepoName, pr.Number, result.IgnoredReviewers())
-		if err != nil {
+		if err := a.client.ReportIgnoredReviews(
+			ctx, pr.OwnerLogin, pr.RepoName, pr.Number, result.IgnoredReviewers()); err != nil {
+			return nil, err
+		}
+
+		if err := a.client.ReportInvalidReviews(
+			ctx, pr.OwnerLogin, pr.RepoName, pr.Number, result.InvalidReviewers()); err != nil {
 			return nil, err
 		}
 	}
 
 	return result, nil
 }
 
+func addMembers(allowed map[string]bool, members []*github.User) {
+	for _, m := range members {
+		allowed[m.GetLogin()] = true
+	}
+}
+
 func (a *Approval) isRuleMatched(ctx context.Context, rule configuration.Rule, pr *PR) (bool, error) {
 	// Check whether the pull request's body matches the aforementioned regex (ignoring case).
 	prBodyMatch, err := a.isRegexMatched(ctx, pr.OwnerLogin, pr.RepoName, pr.Number, rule.Regex, pr.Body)

internal/api/approval/result.go

@@ -12,6 +12,7 @@ type Result struct {
 	finalLabels      []string
 	reviewsToRequest []string
 	ignoredReviewers []string
+	invalidReviewers []string
 }
 
 func (r *Result) pendingReviewsWaiting() bool {
@@ -26,6 +27,7 @@ func (r *Result) Status() string             { return r.status }
 func (r *Result) FinalLabels() []string      { return r.finalLabels }
 func (r *Result) ReviewsToRequest() []string { return r.reviewsToRequest }
 func (r *Result) IgnoredReviewers() []string { return r.ignoredReviewers }
+func (r *Result) InvalidReviewers() []string { return r.invalidReviewers }
 
 func truncate(v string, n int) string {
 	suffix := "..."

internal/api/approval/state.go

@@ -30,6 +30,8 @@ type state struct {
 	ignoredReviewers []string
 	// Invalid team handles found in configuration
 	invalidTeamHandles []string
+	// Reviewers who have approved PR but are not member of any valid team
+	invalidReviewers []string
 }
 
 func newState() *state {
@@ -131,6 +133,7 @@ func (s *state) result(log *log.Entry, teams []*github.Team) *Result {
 	result := &Result{
 		finalLabels:      s.labels,
 		ignoredReviewers: s.ignoredReviewers,
+		invalidReviewers: s.invalidReviewers,
 	}
 
 	pendingTeamNames := s.pendingTeamNames()
@@ -171,6 +174,17 @@ func (s *state) result(log *log.Entry, teams []*github.Team) *Result {
 	return result
 }
 
+func (s *state) updateInvalidReviewers(allAllowedMembers map[string]bool) {
+	if len(s.matchedRules) == 0 {
+		return
+	}
+	for member := range s.approvingReviewers {
+		if _, ok := allAllowedMembers[member]; !ok {
+			s.invalidReviewers = appendIfMissing(s.invalidReviewers, member)
+		}
+	}
+}
+
 func computeReviewsToRequest(log *log.Entry, teams []*github.Team, pendingTeams []string) []string {
 	var reviewsToRequest []string
 

internal/api/github/client.go

@@ -35,6 +35,9 @@ const (
 	envGitHubAppId             = "GITHUB_APP_ID"
 	envGitHubAppInstallationId = "GITHUB_APP_INSTALLATION_ID"
 	envGitHubAppPrivateKeyPath = "GITHUB_APP_PRIVATE_KEY_PATH"
+
+	ignoredReviewersTitle = "Following reviewers have been ignored as they either contributed to or reopened the PR:\n"
+	invalidReviewersTitle = "Following reviewers have been ignored as they are not a member of any valid team:\n"
 )
 
 var (
@@ -364,12 +367,18 @@ func (c *Client) ReportStatus(ctx context.Context, ownerLogin, repoName, statuse
 }
 
 func (c *Client) ReportIgnoredReviews(ctx context.Context, owner, repo string, prNumber int, reviewers []string) error {
+	return c.reportIgnoredReviews(ctx, owner, repo, prNumber, reviewers, ignoredReviewersTitle)
+}
+
+func (c *Client) ReportInvalidReviews(ctx context.Context, owner, repo string, prNumber int, reviewers []string) error {
+	return c.reportIgnoredReviews(ctx, owner, repo, prNumber, reviewers, invalidReviewersTitle)
+}
+
+func (c *Client) reportIgnoredReviews(ctx context.Context, owner, repo string, prNumber int, reviewers []string, title string) error {
 	if len(reviewers) == 0 {
 		return nil
 	}
 
-	title := "Following reviewers have been ignored as they either contributed to or reopened the PR:\n"
-
 	err := c.removeOldBotComments(ctx, owner, repo, prNumber, title)
 	if err != nil {
 		return err

internal/api/handler_test.go

@@ -190,6 +190,22 @@ func Test_Handle(t *testing.T) {
 
 			expectedFinalStatus: approval.StatusEventStatusSuccess,
 		},
+		{
+			name: `PR review Submitted (requires approval from CAB - FOO, David is invalid user thus PR review isn't accepted)`,
+
+			eventType:      eventTypePullRequestReview,
+			eventBody:      readGitHubExampleFile("pull_request_review_submitted.json"),
+			eventSignature: "sha256=802a01c378001fbbbea8f59e7d5eab688550bcbd097491abc907d8850cef6e17",
+			pacts: []pacttesting.Pact{
+				"pull_request_commits_pr_7",
+				"pull_request_get_comments_pr_7",
+				"pull_request_post_comment_pr_7",
+				"pull_request_review_submitted_david_approved",
+				"issue_events_pr_7",
+			},
+
+			expectedFinalStatus: approval.StatusEventStatusPending,
+		},
 	}
 
 	for _, tt := range tests {