Repository: cli/cli
Pull request #762 (2020-04-07 15:09:18)

Add Reviewers to pr view in CLI
This PR is a part of #663 (`pr view`).

~(This is a nested PR. #748 needs to be merged at first.)~ Merged.

[The query updates](https://github.com/doi-t/cli/compare/85d8f80...doi-t:8fca78f) for `Reviewers` is in #748.

~The design must be reviewed~ The design has been reviewed https://github.com/cli/cli/issues/663#issuecomment-610644914. The author of a PR won't be included in the `Reviewers` list as GitHub.com does not include it ([this](https://github.com/microsoft/vscode-pull-request-github/blob/7fbd5ec/src/github/pullRequestOverview.ts#L98-L153) is the reference implementation).

When there is a requested reviewer:
![Screen Shot 2020-04-08 at 0 00 25](https://user-images.githubusercontent.com/5877477/78684940-0c4b3a00-792c-11ea-8c14-78fb48ec8180.png)

When there are multiple reviews:
![Screen Shot 2020-04-07 at 23 59 59](https://user-images.githubusercontent.com/5877477/78684931-0a817680-792c-11ea-9ce8-9018bf32c31c.png)

When there is no reviews and requested reviewer (No metadata too):
![Screen Shot 2020-04-08 at 0 00 40](https://user-images.githubusercontent.com/5877477/78684938-0bb2a380-792c-11ea-928a-625d819c75b2.png)

## Todos
- [x] Implement a parser for `Reviews` and `ReviewRequests`
- [x] sort reviewer list
- [x] Write unit tests


Patch:
diff --git a/api/queries_pr.go b/api/queries_pr.go
index 0b2fd378a..f2f4179e9 100644
--- a/api/queries_pr.go
+++ b/api/queries_pr.go
@@ -3,7 +3,6 @@ package api
 import (
 	"fmt"
 	"strings"
-	"time"
 
 	"github.com/cli/cli/internal/ghrepo"
 )
@@ -67,6 +66,7 @@ type PullRequest struct {
 			RequestedReviewer struct {
 				TypeName string `json:"__typename"`
 				Login    string
+				Name     string
 			}
 		}
 		TotalCount int
@@ -76,9 +76,7 @@ type PullRequest struct {
 			Author struct {
 				Login string
 			}
-			State       string
-			CreatedAt   time.Time
-			PublishedAt time.Time
+			State string
 		}
 	}
 	Assignees struct {
@@ -376,6 +374,9 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu
 							...on User {
 								login
 							}
+							...on Team {
+								name
+							}
 						}
 					}
 					totalCount
@@ -386,8 +387,6 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu
 						  login
 						}
 						state
-						createdAt
-						publishedAt
 					}
 					totalCount
 				}
@@ -475,6 +474,9 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea
 								...on User {
 									login
 								}
+								...on Team {
+									name
+								}
 							}
 						}
 						totalCount
@@ -485,8 +487,6 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea
 							  login
 							}
 							state
-							createdAt
-							publishedAt
 						}
 						totalCount
 					}
diff --git a/command/pr.go b/command/pr.go
index 009548061..8bbfd6b9d 100644
--- a/command/pr.go
+++ b/command/pr.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -339,10 +340,13 @@ func printPrPreview(out io.Writer, pr *api.PullRequest) error {
 		pr.BaseRefName,
 		pr.HeadRefName,
 	)))
+	fmt.Fprintln(out)
 
 	// Metadata
-	// TODO: Reviewers
-	fmt.Fprintln(out)
+	if reviewers := prReviewerList(*pr); reviewers != "" {
+		fmt.Fprint(out, utils.Bold("Reviewers: "))
+		fmt.Fprintln(out, reviewers)
+	}
 	if assignees := prAssigneeList(*pr); assignees != "" {
 		fmt.Fprint(out, utils.Bold("Assignees: "))
 		fmt.Fprintln(out, assignees)
@@ -376,6 +380,116 @@ func printPrPreview(out io.Writer, pr *api.PullRequest) error {
 	return nil
 }
 
+// Ref. https://developer.github.com/v4/enum/pullrequestreviewstate/
+const (
+	requestedReviewState        = "REQUESTED" // This is our own state for review request
+	approvedReviewState         = "APPROVED"
+	changesRequestedReviewState = "CHANGES_REQUESTED"
+	commentedReviewState        = "COMMENTED"
+)
+
+type reviewerState struct {
+	Name  string
+	State string
+}
+
+// colorFuncForReviewerState returns a color function for a reviewer state
+func colorFuncForReviewerState(state string) func(string) string {
+	switch state {
+	case requestedReviewState:
+		return utils.Yellow
+	case approvedReviewState:
+		return utils.Green
+	case changesRequestedReviewState:
+		return utils.Red
+	case commentedReviewState:
+		return func(str string) string { return str } // Do nothing
+	default:
+		return nil
+	}
+}
+
+// formattedReviewerState formats a reviewerState with state color
+func formattedReviewerState(reviewer *reviewerState) string {
+	stateColorFunc := colorFuncForReviewerState(reviewer.State)
+	return fmt.Sprintf("%s (%s)", reviewer.Name, stateColorFunc(strings.ReplaceAll(strings.Title(strings.ToLower(reviewer.State)), "_", " ")))
+}
+
+// prReviewerList generates a reviewer list with their last state
+func prReviewerList(pr api.PullRequest) string {
+	reviewerStates := parseReviewers(pr)
+	reviewers := make([]string, 0, len(reviewerStates))
+
+	sortReviewerStates(reviewerStates)
+
+	for _, reviewer := range reviewerStates {
+		reviewers = append(reviewers, formattedReviewerState(reviewer))
+	}
+
+	reviewerList := strings.Join(reviewers, ", ")
+
+	return reviewerList
+}
+
+// Ref. https://developer.github.com/v4/union/requestedreviewer/
+const teamTypeName = "Team"
+
+const ghostName = "ghost"
+
+// parseReviewers parses given Reviews and ReviewRequests
+func parseReviewers(pr api.PullRequest) []*reviewerState {
+	reviewerStates := make(map[string]*reviewerState)
+
+	for _, review := range pr.Reviews.Nodes {
+		if review.Author.Login != pr.Author.Login {
+			name := review.Author.Login
+			if name == "" {
+				name = ghostName
+			}
+			reviewerStates[name] = &reviewerState{
+				Name:  name,
+				State: review.State,
+			}
+		}
+	}
+
+	// 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
+		}
+		reviewerStates[name] = &reviewerState{
+			Name:  name,
+			State: requestedReviewState,
+		}
+	}
+
+	// Convert map to slice for ease of sort
+	result := make([]*reviewerState, 0, len(reviewerStates))
+	for _, reviewer := range reviewerStates {
+		result = append(result, reviewer)
+	}
+
+	return result
+}
+
+// sortReviewerStates puts completed reviews before review requests and sorts names alphabetically
+func sortReviewerStates(reviewerStates []*reviewerState) {
+	sort.Slice(reviewerStates, func(i, j int) bool {
+		if reviewerStates[i].State == requestedReviewState &&
+			reviewerStates[j].State != requestedReviewState {
+			return false
+		}
+		if reviewerStates[j].State == requestedReviewState &&
+			reviewerStates[i].State != requestedReviewState {
+			return true
+		}
+
+		return reviewerStates[i].Name < reviewerStates[j].Name
+	})
+}
+
 func prAssigneeList(pr api.PullRequest) string {
 	if len(pr.Assignees.Nodes) == 0 {
 		return ""


Test patch:
diff --git a/command/pr_test.go b/command/pr_test.go
index ba0989645..e43d83d8f 100644
--- a/command/pr_test.go
+++ b/command/pr_test.go
@@ -427,6 +427,7 @@ func TestPRView_Preview(t *testing.T) {
 			expectedOutputs: []string{
 				`Blueberries are from a fork`,
 				`Open • nobody wants to merge 12 commits into master from blueberries`,
+				`Reviewers: 2 \(Approved\), 3 \(Commented\), 1 \(Requested\)\n`,
 				`Assignees: marseilles, monaco\n`,
 				`Labels: one, two, three, four, five\n`,
 				`Projects: Project 1 \(column A\), Project 2 \(column B\), Project 3 \(column C\), Project 4 \(Awaiting triage\)\n`,
@@ -435,6 +436,17 @@ func TestPRView_Preview(t *testing.T) {
 				`View this pull request on GitHub: https://github.com/OWNER/REPO/pull/12\n`,
 			},
 		},
+		"Open PR with reviewers by number": {
+			ownerRepo: "master",
+			args:      "pr view 12",
+			fixture:   "../test/fixtures/prViewPreviewWithReviewersByNumber.json",
+			expectedOutputs: []string{
+				`Blueberries are from a fork`,
+				`Reviewers: DEF \(Commented\), def \(Changes requested\), ghost \(Approved\), xyz \(Approved\), 123 \(Requested\), Team 1 \(Requested\), abc \(Requested\)\n`,
+				`blueberries taste good`,
+				`View this pull request on GitHub: https://github.com/OWNER/REPO/pull/12\n`,
+			},
+		},
 		"Open PR with metadata by branch": {
 			ownerRepo: "master",
 			args:      "pr view blueberries",
diff --git a/test/fixtures/prViewPreviewWithMetadataByNumber.json b/test/fixtures/prViewPreviewWithMetadataByNumber.json
index e49bcdc85..0ca6124e6 100644
--- a/test/fixtures/prViewPreviewWithMetadataByNumber.json
+++ b/test/fixtures/prViewPreviewWithMetadataByNumber.json
@@ -10,6 +10,39 @@
         "author": {
           "login": "nobody"
         },
+        "reviewRequests": {
+            "nodes": [
+                {
+                    "requestedReviewer": {
+                        "__typename": "User",
+                        "login": "1"
+                    }
+                }
+            ],
+            "totalcount": 1
+        },
+        "reviews": {
+          "nodes": [
+            {
+              "author": {
+                "login": "3"
+              },
+              "state": "COMMENTED"
+            },
+            {
+              "author": {
+                "login": "2"
+              },
+              "state": "APPROVED"
+            },
+            {
+              "author": {
+                "login": "1"
+              },
+              "state": "CHANGES_REQUESTED"
+            }
+          ]
+        },
         "assignees": {
           "nodes": [
             {
diff --git a/test/fixtures/prViewPreviewWithReviewersByNumber.json b/test/fixtures/prViewPreviewWithReviewersByNumber.json
new file mode 100644
index 000000000..791512d41
--- /dev/null
+++ b/test/fixtures/prViewPreviewWithReviewersByNumber.json
@@ -0,0 +1,110 @@
+{
+  "data": {
+    "repository": {
+      "pullRequest": {
+        "number": 12,
+        "title": "Blueberries are from a fork",
+        "state": "OPEN",
+        "body": "**blueberries taste good**",
+        "url": "https://github.com/OWNER/REPO/pull/12",
+        "author": {
+          "login": "nobody"
+        },
+        "reviewRequests": {
+          "nodes": [
+            {
+              "requestedReviewer": {
+                "__typename": "user",
+                "login": "123"
+              }
+            },
+            {
+                "requestedReviewer": {
+                    "__typename": "Team",
+                    "name": "Team 1"
+                }
+            },
+            {
+              "requestedReviewer": {
+                "__typename": "user",
+                "login": "abc"
+              }
+            }
+          ],
+          "totalcount": 1
+        },
+        "reviews": {
+          "nodes": [
+            {
+              "author": {
+                "login": "123"
+              },
+              "state": "COMMENTED"
+            },
+            {
+              "author": {
+                "login": "def"
+              },
+              "state": "CHANGES_REQUESTED"
+            },
+            {
+              "author": {
+                "login": "abc"
+              },
+              "state": "APPROVED"
+            },
+            {
+              "author": {
+                "login": "DEF"
+              },
+              "state": "COMMENTED"
+            },
+            {
+              "author": {
+                "login": "xyz"
+              },
+              "state": "APPROVED"
+            },
+            {
+              "author": {
+                "login": ""
+              },
+              "state": "APPROVED"
+            }
+          ]
+        },
+        "assignees": {
+          "nodes": [],
+          "totalcount": 0
+        },
+        "labels": {
+          "nodes": [],
+          "totalcount": 0
+        },
+        "projectcards": {
+          "nodes": [],
+          "totalcount": 0
+        },
+        "milestone": {},
+        "participants": {
+          "nodes": [
+            {
+              "login": "marseilles"
+            }
+          ],
+          "totalcount": 1
+        },
+        "commits": {
+          "totalCount": 12
+        },
+        "baseRefName": "master",
+        "headRefName": "blueberries",
+        "headRepositoryOwner": {
+          "login": "hubot"
+        },
+        "isCrossRepository": true,
+        "isDraft": false
+      }
+    }
+  }
+}