diff --git a/api/queries_pr.go b/api/queries_pr.go index ad29f163f..2a38bf909 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" ) @@ -76,9 +75,7 @@ type PullRequest struct { Author struct { Login string } - State string - CreatedAt time.Time - PublishedAt time.Time + State string } } Assignees struct { @@ -392,8 +389,6 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu login } state - createdAt - publishedAt } totalCount } @@ -497,8 +492,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 1aeaa77e2..c008b1292 100644 --- a/command/pr.go +++ b/command/pr.go @@ -4,6 +4,7 @@ import ( "fmt" "io" "regexp" + "sort" "strconv" "strings" @@ -411,17 +412,20 @@ func prReviewerList(pr api.PullRequest) string { reviewerStates := parseReviewers(pr) reviewers := make([]string, 0, len(reviewerStates)) + sortReviewerStates(reviewerStates) + for _, reviewer := range reviewerStates { stateColorFunc := colorFuncForReviewerState(reviewer.State) reviewers = append(reviewers, fmt.Sprintf("%s (%s)", reviewer.Name, stateColorFunc(strings.ReplaceAll(strings.Title(strings.ToLower(reviewer.State)), "_", " ")))) } + reviewerList := strings.Join(reviewers, ", ") return reviewerList } // parseReviewers parses given Reviews and ReviewRequests -func parseReviewers(pr api.PullRequest) map[string]*reviewerState { +func parseReviewers(pr api.PullRequest) []*reviewerState { var reviewerStates = map[string]*reviewerState{} for _, review := range pr.Reviews.Nodes { @@ -441,7 +445,29 @@ func parseReviewers(pr api.PullRequest) map[string]*reviewerState { } } - return reviewerStates + // Convert map to slice for ease of sort + result := []*reviewerState{} + for _, reviewer := range reviewerStates { + result = append(result, reviewer) + } + + return result +} + +// sortReviewerStates puts completed reviews before review requests and sort 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 { diff --git a/command/pr_test.go b/command/pr_test.go index 00cabcbb9..ed0e754d1 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\)\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\), xyz \(Approved\), 123 \(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 24927cb19..7c82f438a 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..fd293c1c7 --- /dev/null +++ b/test/fixtures/prViewPreviewWithReviewersByNumber.json @@ -0,0 +1,98 @@ +{ + "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": "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" + } + ] + }, + "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 + } + } + } +}