Merge pull request cli#3737 from cli/requested-reviewers-slug

Fix how teams are displayed in requested reviewers
areese · Jun 2, 2021 · 4960935 · 4960935
2 parents e65d956 + 1a980e7
commit 4960935
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 30 deletions.
diff --git a/api/export_pr.go b/api/export_pr.go
@@ -80,10 +80,20 @@ func (pr *PullRequest) ExportData(fields []string) *map[string]interface{} {
 		case "reviewRequests":
 			requests := make([]interface{}, 0, len(pr.ReviewRequests.Nodes))
 			for _, req := range pr.ReviewRequests.Nodes {
-				if req.RequestedReviewer.TypeName == "" {
-					continue
+				r := req.RequestedReviewer
+				switch r.TypeName {
+				case "User":
+					requests = append(requests, map[string]string{
+						"__typename": r.TypeName,
+						"login":      r.Login,
+					})
+				case "Team":
+					requests = append(requests, map[string]string{
+						"__typename": r.TypeName,
+						"name":       r.Name,
+						"slug":       r.LoginOrSlug(),
+					})
 				}
-				requests = append(requests, req.RequestedReviewer)
 			}
 			data[f] = &requests
 		default:

diff --git a/api/queries_pr.go b/api/queries_pr.go
@@ -150,28 +150,33 @@ type PullRequestFile struct {
 
 type ReviewRequests struct {
 	Nodes []struct {
-		RequestedReviewer struct {
-			TypeName     string `json:"__typename"`
-			Login        string `json:"login"`
-			Name         string `json:"name"`
-			Slug         string `json:"slug"`
-			Organization struct {
-				Login string `json:"login"`
-			}
-		}
+		RequestedReviewer RequestedReviewer
+	}
+}
+
+type RequestedReviewer struct {
+	TypeName     string `json:"__typename"`
+	Login        string `json:"login"`
+	Name         string `json:"name"`
+	Slug         string `json:"slug"`
+	Organization struct {
+		Login string `json:"login"`
+	} `json:"organization"`
+}
+
+func (r RequestedReviewer) LoginOrSlug() string {
+	if r.TypeName == teamTypeName {
+		return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug)
 	}
+	return r.Login
 }
 
 const teamTypeName = "Team"
 
 func (r ReviewRequests) Logins() []string {
 	logins := make([]string, len(r.Nodes))
-	for i, a := range r.Nodes {
-		if a.RequestedReviewer.TypeName == teamTypeName {
-			logins[i] = fmt.Sprintf("%s/%s", a.RequestedReviewer.Organization.Login, a.RequestedReviewer.Slug)
-		} else {
-			logins[i] = a.RequestedReviewer.Login
-		}
+	for i, r := range r.Nodes {
+		logins[i] = r.RequestedReviewer.LoginOrSlug()
 	}
 	return logins
 }

diff --git a/pkg/cmd/pr/view/fixtures/prViewPreviewWithReviewersByNumber.json b/pkg/cmd/pr/view/fixtures/prViewPreviewWithReviewersByNumber.json
@@ -21,10 +21,12 @@
               }
             },
             {
-                "requestedReviewer": {
-                    "__typename": "Team",
-                    "name": "Team 1"
-                }
+              "requestedReviewer": {
+                "__typename": "Team",
+                "name": "Team 1",
+                "slug": "team-1",
+                "organization": {"login": "my-org"}
+              }
             },
             {
               "requestedReviewer": {

diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go
@@ -294,8 +294,6 @@ func prReviewerList(pr api.PullRequest, cs *iostreams.ColorScheme) string {
 	return reviewerList
 }
 
-const teamTypeName = "Team"
-
 const ghostName = "ghost"
 
 // parseReviewers parses given Reviews and ReviewRequests
@@ -317,10 +315,7 @@ func parseReviewers(pr api.PullRequest) []*reviewerState {
 
 	// Overwrite reviewer's state if a review request for the same reviewer exists.
 	for _, reviewRequest := range pr.ReviewRequests.Nodes {
-		name := reviewRequest.RequestedReviewer.Login
-		if reviewRequest.RequestedReviewer.TypeName == teamTypeName {
-			name = reviewRequest.RequestedReviewer.Name
-		}
+		name := reviewRequest.RequestedReviewer.LoginOrSlug()
 		reviewerStates[name] = &reviewerState{
 			Name:  name,
 			State: requestedReviewState,

diff --git a/pkg/cmd/pr/view/view_test.go b/pkg/cmd/pr/view/view_test.go
@@ -250,7 +250,7 @@ func TestPRView_Preview_nontty(t *testing.T) {
 				`milestone:\t\n`,
 				`additions:\t100\n`,
 				`deletions:\t10\n`,
-				`reviewers:\tDEF \(Commented\), def \(Changes requested\), ghost \(Approved\), hubot \(Commented\), xyz \(Approved\), 123 \(Requested\), Team 1 \(Requested\), abc \(Requested\)\n`,
+				`reviewers:\tDEF \(Commented\), def \(Changes requested\), ghost \(Approved\), hubot \(Commented\), xyz \(Approved\), 123 \(Requested\), abc \(Requested\), my-org\/team-1 \(Requested\)\n`,
 				`\*\*blueberries taste good\*\*`,
 			},
 		},
@@ -383,7 +383,7 @@ func TestPRView_Preview(t *testing.T) {
 			},
 			expectedOutputs: []string{
 				`Blueberries are from a fork`,
-				`Reviewers:.*DEF \(.*Commented.*\), def \(.*Changes requested.*\), ghost \(.*Approved.*\), hubot \(Commented\), xyz \(.*Approved.*\), 123 \(.*Requested.*\), Team 1 \(.*Requested.*\), abc \(.*Requested.*\)\n`,
+				`Reviewers: DEF \(Commented\), def \(Changes requested\), ghost \(Approved\), hubot \(Commented\), xyz \(Approved\), 123 \(Requested\), abc \(Requested\), my-org\/team-1 \(Requested\)`,
 				`blueberries taste good`,
 				`View this pull request on GitHub: https://github.com/OWNER/REPO/pull/12`,
 			},