From fb63efcf0520102315e3450d576224e6f71a2cdf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mislav=20Marohni=C4=87?= <mislav@github.com>
Date: Wed, 20 May 2020 14:59:40 +0200
Subject: [PATCH] Avoid crash around "DISMISSED" or "PENDING" reviewer states

---
 command/pr.go                                  | 15 +++++++++++++--
 command/pr_test.go                             |  2 +-
 .../prViewPreviewWithReviewersByNumber.json    | 18 ++++++++++++++++++
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/command/pr.go b/command/pr.go
index 83ddf7d13..3bf36b432 100644
--- a/command/pr.go
+++ b/command/pr.go
@@ -623,6 +623,8 @@ const (
 	approvedReviewState         = "APPROVED"
 	changesRequestedReviewState = "CHANGES_REQUESTED"
 	commentedReviewState        = "COMMENTED"
+	dismissedReviewState        = "DISMISSED"
+	pendingReviewState          = "PENDING"
 )
 
 type reviewerState struct {
@@ -648,8 +650,14 @@ func colorFuncForReviewerState(state string) func(string) string {
 
 // 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)), "_", " ")))
+	state := reviewer.State
+	if state == dismissedReviewState {
+		// Show "DISMISSED" review as "COMMENTED", since "dimissed" only makes
+		// sense when displayed in an events timeline but not in the final tally.
+		state = commentedReviewState
+	}
+	stateColorFunc := colorFuncForReviewerState(state)
+	return fmt.Sprintf("%s (%s)", reviewer.Name, stateColorFunc(strings.ReplaceAll(strings.Title(strings.ToLower(state)), "_", " ")))
 }
 
 // prReviewerList generates a reviewer list with their last state
@@ -705,6 +713,9 @@ func parseReviewers(pr api.PullRequest) []*reviewerState {
 	// Convert map to slice for ease of sort
 	result := make([]*reviewerState, 0, len(reviewerStates))
 	for _, reviewer := range reviewerStates {
+		if reviewer.State == pendingReviewState {
+			continue
+		}
 		result = append(result, reviewer)
 	}
 
diff --git a/command/pr_test.go b/command/pr_test.go
index 24fce9d63..22f7102f9 100644
--- a/command/pr_test.go
+++ b/command/pr_test.go
@@ -477,7 +477,7 @@ func TestPRView_Preview(t *testing.T) {
 			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`,
+				`Reviewers: DEF \(Commented\), def \(Changes requested\), ghost \(Approved\), hubot \(Commented\), 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`,
 			},
diff --git a/test/fixtures/prViewPreviewWithReviewersByNumber.json b/test/fixtures/prViewPreviewWithReviewersByNumber.json
index 791512d41..4d2bd57af 100644
--- a/test/fixtures/prViewPreviewWithReviewersByNumber.json
+++ b/test/fixtures/prViewPreviewWithReviewersByNumber.json
@@ -70,6 +70,24 @@
                 "login": ""
               },
               "state": "APPROVED"
+            },
+            {
+              "author": {
+                "login": "hubot"
+              },
+              "state": "CHANGES_REQUESTED"
+            },
+            {
+              "author": {
+                "login": "hubot"
+              },
+              "state": "DISMISSED"
+            },
+            {
+              "author": {
+                "login": "monalisa"
+              },
+              "state": "PENDING"
             }
           ]
         },