Merge pull request cli#3701 from mercimat/fix-pr-team-reviewrequests

samcoe · web-flow · commit 35e5c758b5ee · 2021-05-27T09:09:58.000-04:00
fix adding/removing team reviewers with `gh pr edit`
diff --git 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] = fmt.Sprintf("%s/%s", a.RequestedReviewer.Organization.Login, a.RequestedReviewer.Slug)
+		} else {
+			logins[i] = a.RequestedReviewer.Login
+		}
 	}
 	return logins
 }
@@ -393,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
@@ -410,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
@@ -1,12 +1,13 @@
 package api
 
 import (
-	"reflect"
+	"encoding/json"
 	"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) {
@@ -148,13 +149,94 @@ 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)
+		})
+	}
+}
+
+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)
+			assert.NoError(t, err, "Failed to unmarshal json string as ReviewRequests")
+			logins := rr.Logins()
+			assert.Equal(t, tt.want, logins)
 		})
 	}
 }
diff --git 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
+				}
 			}
 		}
 	}

Original file line number	Diff line number	Diff line change
@@ -41,7 +41,11 @@ var prReviewRequests = shortenQuery(`
`41`	`41`	`requestedReviewer {`
`42`	`42`	`__typename,`
`43`	`43`	`...on User{login},`
`44`		`- ...on Team{name}`
	`44`	`+ ...on Team{`
	`45`	`+ organization{login}`
	`46`	`+ name,`
	`47`	`+ slug`
	`48`	`+ }`
`45`	`49`	`}`
`46`	`50`	`}`
`47`	`51`	`}`