From a612f06deec8bd79085c05d55e7083da5beebce8 Mon Sep 17 00:00:00 2001 From: mercimat <11376662+mercimat@users.noreply.github.com> Date: Mon, 24 May 2021 17:00:25 +0200 Subject: [PATCH 1/3] fix pr review requests for teams --- api/queries_pr.go | 18 +++++++-- api/queries_pr_test.go | 86 ++++++++++++++++++++++++++++++++++++++++++ api/query_builder.go | 6 ++- 3 files changed, 105 insertions(+), 5 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 8a1d0e421..87c05b36f 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -151,17 +151,27 @@ type PullRequestFile struct { type ReviewRequests struct { Nodes []struct { RequestedReviewer struct { - TypeName string `json:"__typename"` - Login string `json:"login"` - Name string `json:"name"` + TypeName string `json:"__typename"` + Login string `json:"login"` + Name string `json:"name"` + Slug string `json:"slug"` + Organization struct { + Login string `json:"login"` + } } } } +const teamTypeName = "Team" + func (r ReviewRequests) Logins() []string { logins := make([]string, len(r.Nodes)) for i, a := range r.Nodes { - logins[i] = a.RequestedReviewer.Login + if a.RequestedReviewer.TypeName == teamTypeName { + logins[i] = a.RequestedReviewer.Organization.Login + "/" + a.RequestedReviewer.Slug + } else { + logins[i] = a.RequestedReviewer.Login + } } return logins } diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 886f16dd0..7d3888e83 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -1,6 +1,7 @@ package api import ( + "encoding/json" "reflect" "testing" @@ -158,3 +159,88 @@ func Test_determinePullRequestFeatures(t *testing.T) { }) } } + +func Test_Logins(t *testing.T) { + rr := ReviewRequests{} + var tests = []struct { + name string + requestedReviews string + want []string + }{ + { + name: "no requested reviewers", + requestedReviews: `{"nodes": []}`, + want: []string{}, + }, + { + name: "user", + requestedReviews: `{"nodes": [ + { + "requestedreviewer": { + "__typename": "User", "login": "testuser" + } + } + ]}`, + want: []string{"testuser"}, + }, + { + name: "team", + requestedReviews: `{"nodes": [ + { + "requestedreviewer": { + "__typename": "Team", + "name": "Test Team", + "slug": "test-team", + "organization": {"login": "myorg"} + } + } + ]}`, + want: []string{"myorg/test-team"}, + }, + { + name: "multiple users and teams", + requestedReviews: `{"nodes": [ + { + "requestedreviewer": { + "__typename": "User", "login": "user1" + } + }, + { + "requestedreviewer": { + "__typename": "User", "login": "user2" + } + }, + { + "requestedreviewer": { + "__typename": "Team", + "name": "Test Team", + "slug": "test-team", + "organization": {"login": "myorg"} + } + }, + { + "requestedreviewer": { + "__typename": "Team", + "name": "Dev Team", + "slug": "dev-team", + "organization": {"login": "myorg"} + } + } + ]}`, + want: []string{"user1", "user2", "myorg/test-team", "myorg/dev-team"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := json.Unmarshal([]byte(tt.requestedReviews), &rr) + if err != nil { + t.Fatalf("Failed to unmarshal json string as ReviewRequests: %v", tt.requestedReviews) + } + got := rr.Logins() + if !reflect.DeepEqual(got, tt.want) { + t.Fatalf("Unexpected results: expected %v but got %v", tt.want, got) + } + }) + } +} diff --git a/api/query_builder.go b/api/query_builder.go index 084e97195..3dc97b826 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -41,7 +41,11 @@ var prReviewRequests = shortenQuery(` requestedReviewer { __typename, ...on User{login}, - ...on Team{name} + ...on Team{ + organization{login} + name, + slug + } } } } From 761fa94831c7d2bbc99bcb81a3069c23c6da0155 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Thu, 27 May 2021 08:47:41 -0400 Subject: [PATCH 2/3] Small nitpicky polish --- api/queries_pr.go | 10 +++++----- api/queries_pr_test.go | 26 +++++++++++--------------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 87c05b36f..364784f9f 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -168,7 +168,7 @@ func (r ReviewRequests) Logins() []string { logins := make([]string, len(r.Nodes)) for i, a := range r.Nodes { if a.RequestedReviewer.TypeName == teamTypeName { - logins[i] = a.RequestedReviewer.Organization.Login + "/" + a.RequestedReviewer.Slug + logins[i] = fmt.Sprintf("%s/%s", a.RequestedReviewer.Organization.Login, a.RequestedReviewer.Slug) } else { logins[i] = a.RequestedReviewer.Login } @@ -403,8 +403,8 @@ func PullRequestStatus(client *Client, repo ghrepo.Interface, options StatusOpti queryPrefix := ` query PullRequestStatus($owner: String!, $repo: String!, $headRefName: String!, $viewerQuery: String!, $reviewerQuery: String!, $per_page: Int = 10) { repository(owner: $owner, name: $repo) { - defaultBranchRef { - name + defaultBranchRef { + name } pullRequests(headRefName: $headRefName, first: $per_page, orderBy: { field: CREATED_AT, direction: DESC }) { totalCount @@ -420,8 +420,8 @@ func PullRequestStatus(client *Client, repo ghrepo.Interface, options StatusOpti queryPrefix = ` query PullRequestStatus($owner: String!, $repo: String!, $number: Int!, $viewerQuery: String!, $reviewerQuery: String!, $per_page: Int = 10) { repository(owner: $owner, name: $repo) { - defaultBranchRef { - name + defaultBranchRef { + name } pullRequest(number: $number) { ...prWithReviews diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 7d3888e83..537c71268 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -2,12 +2,12 @@ package api import ( "encoding/json" - "reflect" "testing" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" + "github.com/stretchr/testify/assert" ) func TestBranchDeleteRemote(t *testing.T) { @@ -149,13 +149,13 @@ func Test_determinePullRequestFeatures(t *testing.T) { } gotPrFeatures, err := determinePullRequestFeatures(httpClient, tt.hostname) - if (err != nil) != tt.wantErr { - t.Errorf("determinePullRequestFeatures() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + assert.Error(t, err) return + } else { + assert.NoError(t, err) } - if !reflect.DeepEqual(gotPrFeatures, tt.wantPrFeatures) { - t.Errorf("determinePullRequestFeatures() = %v, want %v", gotPrFeatures, tt.wantPrFeatures) - } + assert.Equal(t, tt.wantPrFeatures, gotPrFeatures) }) } } @@ -168,9 +168,9 @@ func Test_Logins(t *testing.T) { want []string }{ { - name: "no requested reviewers", + name: "no requested reviewers", requestedReviews: `{"nodes": []}`, - want: []string{}, + want: []string{}, }, { name: "user", @@ -234,13 +234,9 @@ func Test_Logins(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := json.Unmarshal([]byte(tt.requestedReviews), &rr) - if err != nil { - t.Fatalf("Failed to unmarshal json string as ReviewRequests: %v", tt.requestedReviews) - } - got := rr.Logins() - if !reflect.DeepEqual(got, tt.want) { - t.Fatalf("Unexpected results: expected %v but got %v", tt.want, got) - } + assert.NoError(t, err, "Failed to unmarshal json string as ReviewRequests") + logins := rr.Logins() + assert.Equal(t, tt.want, logins) }) } } From e160dd3eae3925546402ed2b1def91e756c0074e Mon Sep 17 00:00:00 2001 From: Gowtham Munukutla Date: Fri, 28 May 2021 15:41:12 +0530 Subject: [PATCH 3/3] fix listing of PRs when merged ones are searched (#3730) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Mislav Marohnić --- pkg/cmd/issue/list/list.go | 7 +++++- pkg/cmd/pr/list/list.go | 7 +++++- pkg/cmd/pr/shared/params.go | 16 +++++++++++++ pkg/cmd/pr/shared/params_test.go | 41 ++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/issue/list/list.go b/pkg/cmd/issue/list/list.go index c543a84dc..f0942ab20 100644 --- a/pkg/cmd/issue/list/list.go +++ b/pkg/cmd/issue/list/list.go @@ -112,9 +112,14 @@ func listRun(opts *ListOptions) error { return err } + issueState := strings.ToLower(opts.State) + if issueState == "open" && shared.QueryHasStateClause(opts.Search) { + issueState = "" + } + filterOptions := prShared.FilterOptions{ Entity: "issue", - State: strings.ToLower(opts.State), + State: issueState, Assignee: opts.Assignee, Labels: opts.Labels, Author: opts.Author, diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index faf7a3e24..5810e8d7c 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -116,9 +116,14 @@ func listRun(opts *ListOptions) error { return err } + prState := strings.ToLower(opts.State) + if prState == "open" && shared.QueryHasStateClause(opts.Search) { + prState = "" + } + filters := shared.FilterOptions{ Entity: "pr", - State: strings.ToLower(opts.State), + State: prState, Author: opts.Author, Assignee: opts.Assignee, Labels: opts.Labels, diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index d8348ec2c..93a5f2f58 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -2,6 +2,7 @@ package shared import ( "fmt" + "github.com/google/shlex" "net/url" "strings" @@ -243,6 +244,21 @@ func SearchQueryBuild(options FilterOptions) string { return q.String() } +func QueryHasStateClause(searchQuery string) bool { + argv, err := shlex.Split(searchQuery) + if err != nil { + return false + } + + for _, arg := range argv { + if arg == "is:closed" || arg == "is:merged" || arg == "state:closed" || arg == "state:merged" || strings.HasPrefix(arg, "merged:") || strings.HasPrefix(arg, "closed:") { + return true + } + } + + return false +} + // MeReplacer resolves usages of `@me` to the handle of the currently logged in user. type MeReplacer struct { apiClient *api.Client diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 74115c5b6..314218754 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -1,6 +1,7 @@ package shared import ( + "github.com/stretchr/testify/assert" "net/http" "reflect" "testing" @@ -151,3 +152,43 @@ func TestMeReplacer_Replace(t *testing.T) { }) } } + +func Test_QueryHasStateClause(t *testing.T) { + tests := []struct { + searchQuery string + hasState bool + }{ + { + searchQuery: "is:closed is:merged", + hasState: true, + }, + { + searchQuery: "author:mislav", + hasState: false, + }, + { + searchQuery: "assignee:g14a mentions:vilmibm", + hasState: false, + }, + { + searchQuery: "merged:>2021-05-20", + hasState: true, + }, + { + searchQuery: "state:merged state:open", + hasState: true, + }, + { + searchQuery: "assignee:g14a is:closed", + hasState: true, + }, + { + searchQuery: "state:closed label:bug", + hasState: true, + }, + } + for _, tt := range tests { + gotState := QueryHasStateClause(tt.searchQuery) + assert.Equal(t, tt.hasState, gotState) + } +}