Add Reviewers to pr view in CLI by doi-t · Pull Request #762 · cli/cli

doi-t · 2020-04-07T15:09:18Z

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 for Reviewers is in #748.

~~The design must be reviewed~~ The design has been reviewed #663 (comment). The author of a PR won't be included in the Reviewers list as GitHub.com does not include it (this is the reference implementation).

When there is a requested reviewer:

When there are multiple reviews:

When there is no reviews and requested reviewer (No metadata too):

Todos

Implement a parser for Reviews and ReviewRequests
sort reviewer list
Write unit tests

… alphabetically

doi-t · 2020-04-09T16:24:41Z

@ampinsk
I've added colors to the reviewer's state following #663 (comment).

This is a random pick example that includes multiple reviewer's states. I put completed reviews before review requests and sort the reviewer's name alphabetically. As far as I can observe, GitHub.com does further sorting (top > Changes requested > Commented > Approved > Requested > bottom & alphabetical sort of reviewer's name???) but I guess this can be a starting point of gh. 🤔

This is an example of Commened state.

@mislav @vilmibm
This PR is also ready for code review once #748 gets merged!

ampinsk · 2020-04-09T17:51:34Z

@doi-t yes this is totally great for now! I definitely don't think we need to over optimize right now, this is exactly what we need to start :)

doi-t · 2020-04-13T09:27:24Z

Solved the conflict and made additional refactoring. Now it's ready for code review.

tierninho · 2020-04-13T21:47:20Z

Tested this change and it functions well. 👍

I like that that reviewers and the type of review are both in alpha order, however I noticed that there are actually four ways we can organize this info. I am not sure of the importance of this, but wanted to point it out in case we missed it.

(Current version) Alpha order by username and then by review-type, like so:
Chronologically beginning with the first request for a review. E.g., Tom was requested first, Jerry was second. See sidebar in pic below.
Chronologically beginning with an actual review. E.g., Jerry beat Tom to reviewing the PR. See timeline in pic below.
We set an alternative particular order, tbd.

doi-t · 2020-04-14T14:34:24Z

Thank you for the double-check!

Chronologically beginning with the first request for a review. E.g., Tom was requested first, Jerry was second. See sidebar in pic below.

Chronologically beginning with an actual review. E.g., Jerry beat Tom to reviewing the PR. See timeline in pic below.

Since an actual review can happen without a review request and "reviews" and "review requests" are managed differently by GitHub, I guess we need to combine them if we go with this idea (e.g. the first review (Approved), the second review (Changes requested), the first review request (Requested), the second review request (Requested)). I'm not sure if we can get the timestamp for "review requests". 🤔

ampinsk · 2020-04-14T18:54:26Z

I'm not too worried about the ordering for this first version! I think whatever's simplest is great, and we can always iterate on this detail in the future if we find people are getting confused by this or are needing more nuanced ordering 😄

mislav

This looks great to me! Thank you for the hard work.

mislav · 2020-04-15T08:05:08Z

test/fixtures/prViewPreviewWithMetadataByNumber.json

+            "nodes": [
+                {
+                    "requestedReviewer": {
+                        "__typename": "user",


I think it might be valuable to also display teams among requested reviewers. But I also think that this is something that could just as well be a follow-up.

~~Let me create another follow-up PR for team! Since the key is different from user, I guess I need to tweak the sorting logic along with a query update.~~

@mislav
I've added the team reviewer support d70358e to this PR.

The following test results are made by below permissions (read:org works 👍 ).

- q.Set("scope", "repo") + q.Set("scope", "repo read:org")

It seems that the requested team reviewer will be gone once one of the team members takes an action. The requested team will not be shown in the review results too because a result (Comment/Approved/Request changes) is always recorded with user account as far as I can see.

I also realized that ghost account does not have Login value. I added a workaround to show ghost's review results 67907c8.

I think this edge case is happening in other commands which display a user account. 🤔

doi-t · 2020-04-29T15:46:11Z

I've merged master into this branch to include #786 and confirmed the work.

This PR is now ready for re-review and merge. #762 (comment) is additional changes since the last code review.

By the way, gh asked me to do auth again as I expected. However, it didn't work so that I needed to remove ~/.config/gh/config.yml.

$ gh pr view 1 --repo cli-test-org/cli-test
Notice: additional authorization required
Press Enter to open github.com in your browser...
Authentication complete. Press Enter to continue...

graphql error: 'Your token has not been granted the required scopes to execute this query. The 'name' field requires one of the following scopes: ['read:org', 'read:discussion'], but your token has only been granted the: ['repo'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.'

mislav

Thank you for the update!

By the way, gh asked me to do auth again as I expected. However, it didn't work so that I needed to remove ~/.config/gh/config.yml.

Thanks for reporting that. I think that something in #786 with refreshing the token for the current process is off. 🤔 I will take a look.

vilmibm

thank you!

This was referenced Apr 7, 2020

Add relevant metadata to issue view and pr view #663

Closed

Add relevant metadata to pr view in CLI #748

Merged

doi-t added 3 commits April 8, 2020 16:27

Update unit tests for PR metadata

cef525a

Parses Reviews and ReviewRequests to show reviewers and their state

bc42228

Give a color to reviewer states

5c261e2

doi-t force-pushed the reviewers-in-pr-view branch 2 times, most recently from b18b1b8 to 5bc426b Compare April 9, 2020 15:53

Puts completed reviews before review requests and sort reviewer names…

6223a2c

… alphabetically

doi-t force-pushed the reviewers-in-pr-view branch from 5bc426b to 6223a2c Compare April 9, 2020 16:16

doi-t marked this pull request as ready for review April 9, 2020 16:24

doi-t added 4 commits April 10, 2020 04:46

Merge branch 'master' into reviewers-in-pr-view

2df5281

Add comments on review state values

fbff722

Fix slice initialization with provided capacity

fe27c84

Extract the reviewerstate formatter into a function

502f670

billygriffin requested review from mislav and vilmibm April 13, 2020 14:59

mislav approved these changes Apr 15, 2020

View reviewed changes

doi-t mentioned this pull request Apr 22, 2020

Support GitHub Teams in requested reviewers metada #812

Closed

doi-t added 3 commits April 22, 2020 21:58

Support GitHub Team in requested reviewers

d70358e

Support ghost reviews in pr view

67907c8

Merge branch 'master' into reviewers-in-pr-view

1ce09ff

doi-t force-pushed the reviewers-in-pr-view branch from e046e12 to 1ce09ff Compare April 22, 2020 13:00

doi-t requested a review from mislav April 22, 2020 13:24

Merge branch 'master' into reviewers-in-pr-view

d2d0b47

mislav approved these changes May 5, 2020

View reviewed changes

vilmibm approved these changes May 8, 2020

View reviewed changes

vilmibm merged commit 5ba3baa into cli:master May 8, 2020

doi-t deleted the reviewers-in-pr-view branch May 9, 2020 04:04

This was referenced May 13, 2020

Updated config is not correctly picked up after re-auth #914

Closed

invalid memory address or nil pointer dereference in formattedReviewerState #934

Closed

Avoid crash in pr view with reviewers #975

Merged

Conversation

doi-t commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todos

Uh oh!

doi-t commented Apr 9, 2020

Uh oh!

ampinsk commented Apr 9, 2020

Uh oh!

doi-t commented Apr 13, 2020

Uh oh!

tierninho commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doi-t commented Apr 14, 2020

Uh oh!

ampinsk commented Apr 14, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

mislav Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

doi-t Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doi-t Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doi-t commented Apr 29, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

vilmibm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

doi-t commented Apr 7, 2020 •

edited

Loading

tierninho commented Apr 13, 2020 •

edited

Loading

doi-t Apr 15, 2020 •

edited

Loading

doi-t Apr 22, 2020 •

edited

Loading