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/2] 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/2] 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) }) } }