Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Commit

Permalink
fix: filter pull requests on the server, not client
Browse files Browse the repository at this point in the history
Ensure that the GraphQL query requests PRs matching the desired states,
rather than requesting all PRs and filtering on the client.

Fixes #233
  • Loading branch information
jhosteny committed Nov 2, 2020
1 parent a98122f commit f119d22
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 22 deletions.
25 changes: 7 additions & 18 deletions check.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ import (
func Check(request CheckRequest, manager Github) (CheckResponse, error) {
var response CheckResponse

pulls, err := manager.ListOpenPullRequests()
// Filter out pull request if it does not have a filtered state
filterStates := []githubv4.PullRequestState{githubv4.PullRequestStateOpen}
if len(request.Source.States) > 0 {
filterStates = request.Source.States
}

pulls, err := manager.ListOpenPullRequests(filterStates)
if err != nil {
return nil, fmt.Errorf("failed to get last commits: %s", err)
}
Expand All @@ -38,23 +44,6 @@ Loop:
continue
}

// Filter out pull request if it does not have a filtered state
filterStates := []githubv4.PullRequestState{githubv4.PullRequestStateOpen}
if len(request.Source.States) > 0 {
filterStates = request.Source.States
}

stateFound := false
for _, state := range filterStates {
if p.State == state {
stateFound = true
break
}
}
if !stateFound {
continue
}

// Filter out commits that are too old.
if !p.UpdatedDate().Time.After(request.Version.CommittedDate) {
continue
Expand Down
15 changes: 14 additions & 1 deletion check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,20 @@ func TestCheck(t *testing.T) {
for _, tc := range tests {
t.Run(tc.description, func(t *testing.T) {
github := new(fakes.FakeGithub)
github.ListOpenPullRequestsReturns(tc.pullRequests, nil)
pullRequests := []*resource.PullRequest{}
filterStates := []githubv4.PullRequestState{githubv4.PullRequestStateOpen}
if len(tc.source.States) > 0 {
filterStates = tc.source.States
}
for i := range tc.pullRequests {
for j := range filterStates {
if filterStates[j] == tc.pullRequests[i].PullRequestObject.State {
pullRequests = append(pullRequests, tc.pullRequests[i])
break
}
}
}
github.ListOpenPullRequestsReturns(pullRequests, nil)

for i, file := range tc.files {
github.ListModifiedFilesReturnsOnCall(i, file, nil)
Expand Down
6 changes: 3 additions & 3 deletions github.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
// Github for testing purposes.
//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o fakes/fake_github.go . Github
type Github interface {
ListOpenPullRequests() ([]*PullRequest, error)
ListOpenPullRequests([]githubv4.PullRequestState) ([]*PullRequest, error)
ListModifiedFiles(int) ([]string, error)
PostComment(string, string) error
GetPullRequest(string, string) (*PullRequest, error)
Expand Down Expand Up @@ -98,7 +98,7 @@ func NewGithubClient(s *Source) (*GithubClient, error) {
}

// ListOpenPullRequests gets the last commit on all open pull requests.
func (m *GithubClient) ListOpenPullRequests() ([]*PullRequest, error) {
func (m *GithubClient) ListOpenPullRequests(prStates []githubv4.PullRequestState) ([]*PullRequest, error) {
var query struct {
Repository struct {
PullRequests struct {
Expand Down Expand Up @@ -136,7 +136,7 @@ func (m *GithubClient) ListOpenPullRequests() ([]*PullRequest, error) {
"repositoryOwner": githubv4.String(m.Owner),
"repositoryName": githubv4.String(m.Repository),
"prFirst": githubv4.Int(100),
"prStates": []githubv4.PullRequestState{githubv4.PullRequestStateOpen, githubv4.PullRequestStateClosed, githubv4.PullRequestStateMerged},
"prStates": prStates,
"prCursor": (*githubv4.String)(nil),
"commitsLast": githubv4.Int(1),
"prReviewStates": []githubv4.PullRequestReviewState{githubv4.PullRequestReviewStateApproved},
Expand Down

0 comments on commit f119d22

Please sign in to comment.