Adding approval metadata to project command context [Pulling from ups…

…tream] (#116)

* Pulling [#1827-Updating client interface and adding ApprovalStatus model] from master

* Fix failing test

server/controllers/events/events_controller_e2e_test.go

@@ -591,7 +591,9 @@ func TestGitHubWorkflowWithPolicyCheck(t *testing.T) {
 			// Setup test dependencies.
 			w := httptest.NewRecorder()
 			When(githubClient.PullIsSQMergeable(AnyRepo(), matchers.AnyModelsPullRequest(), AnyStatus())).ThenReturn(true, nil)
-			When(githubClient.PullIsApproved(AnyRepo(), matchers.AnyModelsPullRequest())).ThenReturn(true, nil)
+			When(githubClient.PullIsApproved(AnyRepo(), matchers.AnyModelsPullRequest())).ThenReturn(models.ApprovalStatus{
+				IsApproved: true,
+			}, nil)
 			When(githubGetter.GetPullRequest(AnyRepo(), AnyInt())).ThenReturn(GitHubPullRequestParsed(headSHA), nil)
 			When(vcsClient.GetModifiedFiles(AnyRepo(), matchers.AnyModelsPullRequest())).ThenReturn(c.ModifiedFiles, nil)
 

server/events/apply_requirement_handler.go

@@ -19,7 +19,7 @@ func (a *AggregateApplyRequirements) ValidateProject(repoDir string, ctx models.
 	for _, req := range ctx.ApplyRequirements {
 		switch req {
 		case raw.ApprovedApplyRequirement:
-			if !ctx.PullReqStatus.Approved {
+			if !ctx.PullReqStatus.Approved.IsApproved {
 				return "Pull request must be approved by at least one person other than the author before running apply.", nil
 			}
 		// this should come before mergeability check since mergeability is a superset of this check.

server/events/models/models.go

@@ -38,12 +38,6 @@ const (
 	LogStreamingClearMsg = "\n-----Starting New Process-----"
 )
 
-type PullReqStatus struct {
-	Approved  bool
-	Mergeable bool
-	SQLocked  bool
-}
-
 // Repo is a VCS repository.
 type Repo struct {
 	// FullName is the owner and repo name separated
@@ -151,6 +145,18 @@ func NewRepo(vcsHostType VCSHostType, repoFullName string, cloneURL string, vcsU
 	}, nil
 }
 
+type PullReqStatus struct {
+	Approved  ApprovalStatus
+	Mergeable bool
+	SQLocked  bool
+}
+
+type ApprovalStatus struct {
+	IsApproved bool
+	ApprovedBy string
+	Date       time.Time
+}
+
 // PullRequest is a VCS pull request.
 // GitLab calls these Merge Requests.
 type PullRequest struct {

server/events/vcs/azuredevops_client.go

@@ -136,15 +136,15 @@ func (g *AzureDevopsClient) HidePrevCommandComments(repo models.Repo, pullNum in
 
 // PullIsApproved returns true if the merge request was approved by another reviewer.
 // https://docs.microsoft.com/en-us/azure/devops/repos/git/branch-policies?view=azure-devops#require-a-minimum-number-of-reviewers
-func (g *AzureDevopsClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error) {
+func (g *AzureDevopsClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (approvalStatus models.ApprovalStatus, err error) {
 	owner, project, repoName := SplitAzureDevopsRepoFullName(repo.FullName)
 
 	opts := azuredevops.PullRequestGetOptions{
 		IncludeWorkItemRefs: true,
 	}
 	adPull, _, err := g.Client.PullRequests.GetWithRepo(g.ctx, owner, project, repoName, pull.Num, &opts)
 	if err != nil {
-		return false, errors.Wrap(err, "getting pull request")
+		return approvalStatus, errors.Wrap(err, "getting pull request")
 	}
 
 	for _, review := range adPull.Reviewers {
@@ -157,11 +157,13 @@ func (g *AzureDevopsClient) PullIsApproved(repo models.Repo, pull models.PullReq
 		}
 
 		if review.GetVote() == azuredevops.VoteApproved || review.GetVote() == azuredevops.VoteApprovedWithSuggestions {
-			return true, nil
+			return models.ApprovalStatus{
+				IsApproved: true,
+			}, nil
 		}
 	}
 
-	return false, nil
+	return approvalStatus, nil
 }
 
 // PullIsMergeable returns true if the merge request can be merged.

server/events/vcs/azuredevops_client_test.go

@@ -495,7 +495,7 @@ func TestAzureDevopsClient_PullIsApproved(t *testing.T) {
 
 			defer disableSSLVerification()()
 
-			actApproved, err := client.PullIsApproved(models.Repo{
+			approvalStatus, err := client.PullIsApproved(models.Repo{
 				FullName:          "owner/project/repo",
 				Owner:             "owner",
 				Name:              "repo",
@@ -509,7 +509,7 @@ func TestAzureDevopsClient_PullIsApproved(t *testing.T) {
 				Num: 1,
 			})
 			Ok(t, err)
-			Equals(t, c.expApproved, actApproved)
+			Equals(t, c.expApproved, approvalStatus.IsApproved)
 		})
 	}
 }

server/events/vcs/bitbucketcloud/client.go

@@ -105,28 +105,30 @@ func (b *Client) HidePrevCommandComments(repo models.Repo, pullNum int, command
 }
 
 // PullIsApproved returns true if the merge request was approved.
-func (b *Client) PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error) {
+func (b *Client) PullIsApproved(repo models.Repo, pull models.PullRequest) (approvalStatus models.ApprovalStatus, err error) {
 	path := fmt.Sprintf("%s/2.0/repositories/%s/pullrequests/%d", b.BaseURL, repo.FullName, pull.Num)
 	resp, err := b.makeRequest("GET", path, nil)
 	if err != nil {
-		return false, err
+		return approvalStatus, err
 	}
 	var pullResp PullRequest
 	if err := json.Unmarshal(resp, &pullResp); err != nil {
-		return false, errors.Wrapf(err, "Could not parse response %q", string(resp))
+		return approvalStatus, errors.Wrapf(err, "Could not parse response %q", string(resp))
 	}
 	if err := validator.New().Struct(pullResp); err != nil {
-		return false, errors.Wrapf(err, "API response %q was missing fields", string(resp))
+		return approvalStatus, errors.Wrapf(err, "API response %q was missing fields", string(resp))
 	}
 	authorUUID := *pullResp.Author.UUID
 	for _, participant := range pullResp.Participants {
 		// Bitbucket allows the author to approve their own pull request. This
 		// defeats the purpose of approvals so we don't count that approval.
 		if *participant.Approved && *participant.User.UUID != authorUUID {
-			return true, nil
+			return models.ApprovalStatus{
+				IsApproved: true,
+			}, nil
 		}
 	}
-	return false, nil
+	return approvalStatus, nil
 }
 
 // PullIsMergeable returns true if the merge request has no conflicts and can be merged.

server/events/vcs/bitbucketcloud/client_test.go

@@ -202,14 +202,14 @@ func TestClient_PullIsApproved(t *testing.T) {
 
 			repo, err := models.NewRepo(models.BitbucketServer, "owner/repo", "https://bitbucket.org/owner/repo.git", "user", "token")
 			Ok(t, err)
-			approved, err := client.PullIsApproved(repo, models.PullRequest{
+			approvalStatus, err := client.PullIsApproved(repo, models.PullRequest{
 				Num:        1,
 				HeadBranch: "branch",
 				Author:     "author",
 				BaseRepo:   repo,
 			})
 			Ok(t, err)
-			Equals(t, c.exp, approved)
+			Equals(t, c.exp, approvalStatus.IsApproved)
 		})
 	}
 }

server/events/vcs/bitbucketserver/client.go

@@ -161,29 +161,31 @@ func (b *Client) postComment(repo models.Repo, pullNum int, comment string) erro
 }
 
 // PullIsApproved returns true if the merge request was approved.
-func (b *Client) PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error) {
+func (b *Client) PullIsApproved(repo models.Repo, pull models.PullRequest) (approvalStatus models.ApprovalStatus, err error) {
 	projectKey, err := b.GetProjectKey(repo.Name, repo.SanitizedCloneURL)
 	if err != nil {
-		return false, err
+		return approvalStatus, err
 	}
 	path := fmt.Sprintf("%s/rest/api/1.0/projects/%s/repos/%s/pull-requests/%d", b.BaseURL, projectKey, repo.Name, pull.Num)
 	resp, err := b.makeRequest("GET", path, nil)
 	if err != nil {
-		return false, err
+		return approvalStatus, err
 	}
 	var pullResp PullRequest
 	if err := json.Unmarshal(resp, &pullResp); err != nil {
-		return false, errors.Wrapf(err, "Could not parse response %q", string(resp))
+		return approvalStatus, errors.Wrapf(err, "Could not parse response %q", string(resp))
 	}
 	if err := validator.New().Struct(pullResp); err != nil {
-		return false, errors.Wrapf(err, "API response %q was missing fields", string(resp))
+		return approvalStatus, errors.Wrapf(err, "API response %q was missing fields", string(resp))
 	}
 	for _, reviewer := range pullResp.Reviewers {
 		if *reviewer.Approved {
-			return true, nil
+			return models.ApprovalStatus{
+				IsApproved: true,
+			}, nil
 		}
 	}
-	return false, nil
+	return approvalStatus, nil
 }
 
 // PullIsMergeable returns true if the merge request has no conflicts and can be merged.

server/events/vcs/client.go

@@ -26,7 +26,7 @@ type Client interface {
 	GetModifiedFiles(repo models.Repo, pull models.PullRequest) ([]string, error)
 	CreateComment(repo models.Repo, pullNum int, comment string, command string) error
 	HidePrevCommandComments(repo models.Repo, pullNum int, command string) error
-	PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error)
+	PullIsApproved(repo models.Repo, pull models.PullRequest) (models.ApprovalStatus, error)
 	PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error)
 	// UpdateStatus updates the commit status to state for pull. src is the
 	// source of this status. This should be relatively static across runs,

server/events/vcs/github_client.go

@@ -254,7 +254,7 @@ func (g *GithubClient) HidePrevCommandComments(repo models.Repo, pullNum int, co
 }
 
 // PullIsApproved returns true if the pull request was approved.
-func (g *GithubClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error) {
+func (g *GithubClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (approvalStatus models.ApprovalStatus, err error) {
 	nextPage := 0
 	for {
 		opts := github.ListOptions{
@@ -266,19 +266,23 @@ func (g *GithubClient) PullIsApproved(repo models.Repo, pull models.PullRequest)
 		g.logger.Debug("GET /repos/%v/%v/pulls/%d/reviews", repo.Owner, repo.Name, pull.Num)
 		pageReviews, resp, err := g.client.PullRequests.ListReviews(g.ctx, repo.Owner, repo.Name, pull.Num, &opts)
 		if err != nil {
-			return false, errors.Wrap(err, "getting reviews")
+			return approvalStatus, errors.Wrap(err, "getting reviews")
 		}
 		for _, review := range pageReviews {
 			if review != nil && review.GetState() == "APPROVED" {
-				return true, nil
+				return models.ApprovalStatus{
+					IsApproved: true,
+					ApprovedBy: *review.User.Login,
+					Date:       *review.SubmittedAt,
+				}, nil
 			}
 		}
 		if resp.NextPage == 0 {
 			break
 		}
 		nextPage = resp.NextPage
 	}
-	return false, nil
+	return approvalStatus, nil
 }
 
 // PullIsMergeable returns true if the pull request is mergeable.

server/events/vcs/github_client_test.go

@@ -10,6 +10,7 @@ import (
 	"net/url"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/google/go-github/v31/github"
 	"github.com/runatlantis/atlantis/server/events/models"
@@ -415,7 +416,7 @@ func TestGithubClient_PullIsApproved(t *testing.T) {
 			},
 			"body": "Here is the body for the review.",
 			"commit_id": "ecdd80bb57125d7ba9641ffaa4d7d2c19d3f3091",
-			"state": "CHANGES_REQUESTED",
+			"state": "APPROVED",
 			"html_url": "https://github.com/octocat/Hello-World/pull/12#pullrequestreview-%d",
 			"pull_request_url": "https://api.github.com/repos/octocat/Hello-World/pulls/12",
 			"_links": {
@@ -425,7 +426,8 @@ func TestGithubClient_PullIsApproved(t *testing.T) {
 			  "pull_request": {
 				"href": "https://api.github.com/repos/octocat/Hello-World/pulls/12"
 			  }
-			}
+			},
+			"submitted_at": "2019-11-17T17:43:43Z"
 		  }
 ]`
 	firstResp := fmt.Sprintf(respTemplate, 80, 80, 80)
@@ -456,7 +458,7 @@ func TestGithubClient_PullIsApproved(t *testing.T) {
 	Ok(t, err)
 	defer disableSSLVerification()()
 
-	approved, err := client.PullIsApproved(models.Repo{
+	approvalStatus, err := client.PullIsApproved(models.Repo{
 		FullName:          "owner/repo",
 		Owner:             "owner",
 		Name:              "repo",
@@ -470,7 +472,16 @@ func TestGithubClient_PullIsApproved(t *testing.T) {
 		Num: 1,
 	})
 	Ok(t, err)
-	Equals(t, false, approved)
+
+	timeOfApproval, err := time.Parse("2006-01-02T15:04:05Z", "2019-11-17T17:43:43Z")
+	Ok(t, err)
+
+	expApprovalStatus := models.ApprovalStatus{
+		IsApproved: true,
+		ApprovedBy: "octocat",
+		Date:       timeOfApproval,
+	}
+	Equals(t, expApprovalStatus, approvalStatus)
 }
 
 func TestGithubClient_PullIsMergeable(t *testing.T) {

server/events/vcs/gitlab_client.go

@@ -153,15 +153,17 @@ func (g *GitlabClient) HidePrevCommandComments(repo models.Repo, pullNum int, co
 }
 
 // PullIsApproved returns true if the merge request was approved.
-func (g *GitlabClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error) {
+func (g *GitlabClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (approvalStatus models.ApprovalStatus, err error) {
 	approvals, _, err := g.Client.MergeRequests.GetMergeRequestApprovals(repo.FullName, pull.Num)
 	if err != nil {
-		return false, err
+		return approvalStatus, err
 	}
 	if approvals.ApprovalsLeft > 0 {
-		return false, nil
+		return approvalStatus, nil
 	}
-	return true, nil
+	return models.ApprovalStatus{
+		IsApproved: true,
+	}, nil
 }
 
 // PullIsMergeable returns true if the merge request can be merged.

server/events/vcs/instrumented_client.go

@@ -251,7 +251,7 @@ func (c *InstrumentedClient) HidePrevCommandComments(repo models.Repo, pullNum i
 	return nil
 
 }
-func (c *InstrumentedClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (bool, error) {
+func (c *InstrumentedClient) PullIsApproved(repo models.Repo, pull models.PullRequest) (models.ApprovalStatus, error) {
 	scope := c.StatsScope.Scope("pull_is_approved")
 	logger := c.Logger.WithHistory(fmtLogSrc(repo, pull.Num)...)
 
@@ -261,7 +261,7 @@ func (c *InstrumentedClient) PullIsApproved(repo models.Repo, pull models.PullRe
 	executionSuccess := scope.NewCounter(metrics.ExecutionSuccessMetric)
 	executionError := scope.NewCounter(metrics.ExecutionErrorMetric)
 
-	approved, err := c.Client.PullIsApproved(repo, pull)
+	approvalStatus, err := c.Client.PullIsApproved(repo, pull)
 
 	if err != nil {
 		executionError.Inc()
@@ -270,7 +270,7 @@ func (c *InstrumentedClient) PullIsApproved(repo models.Repo, pull models.PullRe
 		executionSuccess.Inc()
 	}
 
-	return approved, err
+	return approvalStatus, err
 
 }
 func (c *InstrumentedClient) PullIsMergeable(repo models.Repo, pull models.PullRequest) (bool, error) {