dmitri.shuralyov.com/service/change/...

githubapi: use AuthorAssociation to determine ReviewState better

This produces more accurate results and is consistent with similar
code elsewhere.

This change also improves handling of other PullRequestReviewState
values.
dmitshur committed 5 years ago commit a1ef85d7455ffbeb260718aa501ac5c3eea3707c
Collapse all
githubapi/githubapi.go
@@ -354,20 +354,21 @@ func (s service) ListTimeline(ctx context.Context, rs string, id uint64, opt *ch

				// Need to use PullRequest.Reviews rather than PullRequest.Timeline.PullRequestReview,
				// because the latter is missing single-inline-reply reviews (as of 2018-02-08).
				Reviews struct {
					Nodes []struct {
						DatabaseID      uint64
						Author          *githubV4Actor
						PublishedAt     githubv4.DateTime
						LastEditedAt    *githubv4.DateTime
						Editor          *githubV4Actor
						State           githubv4.PullRequestReviewState
						Body            string
						ReactionGroups  reactionGroups
						ViewerCanUpdate bool
						Comments        struct {
						DatabaseID        uint64
						Author            *githubV4Actor
						AuthorAssociation githubv4.CommentAuthorAssociation
						PublishedAt       githubv4.DateTime
						LastEditedAt      *githubv4.DateTime
						Editor            *githubV4Actor
						State             githubv4.PullRequestReviewState
						Body              string
						ReactionGroups    reactionGroups
						ViewerCanUpdate   bool
						Comments          struct {
							Nodes []struct {
								DatabaseID       uint64
								Path             string
								OriginalPosition int // The original line index in the diff to which the comment applies.
								Body             string
@@ -434,11 +435,11 @@ func (s service) ListTimeline(ctx context.Context, rs string, id uint64, opt *ch
				Editable:  comment.ViewerCanUpdate,
			})
		}
		if variables["firstPage"].(githubv4.Boolean) {
			for _, review := range q.Repository.PullRequest.Reviews.Nodes {
				state, ok := ghPRReviewState(review.State)
				state, ok := ghPRReviewState(review.State, review.AuthorAssociation)
				if !ok {
					continue
				}
				var edited *change.Edited
				if review.LastEditedAt != nil {
@@ -875,27 +876,36 @@ func ghPRState(state githubv4.PullRequestState) change.State {
		panic("unreachable")
	}
}

// ghPRReviewState converts a GitHub PullRequestReviewState to state.Review, if it's supported.
//
// TODO: Consider using author association or so to tell approved by someone with
//       push rights (+2) apart from approved by someone without push rights (+1), etc.
func ghPRReviewState(st githubv4.PullRequestReviewState) (_ state.Review, ok bool) {
	switch st {
	case githubv4.PullRequestReviewStateApproved:
func ghPRReviewState(st githubv4.PullRequestReviewState, aa githubv4.CommentAuthorAssociation) (_ state.Review, ok bool) {
	// TODO: This is a heuristic. Author can be a member of the organization that
	// owns the repository, but it's not known whether they have push access or not.
	// TODO: Use https://developer.github.com/v3/repos/collaborators/#review-a-users-permission-level perhaps?
	// Or wait for equivalent to be available via API v4?
	approver := aa == githubv4.CommentAuthorAssociationOwner ||
		aa == githubv4.CommentAuthorAssociationCollaborator ||
		aa == githubv4.CommentAuthorAssociationMember

	switch {
	case st == githubv4.PullRequestReviewStateApproved && approver:
		return state.ReviewPlus2, true
	case githubv4.PullRequestReviewStateCommented:
	case st == githubv4.PullRequestReviewStateApproved && !approver:
		return state.ReviewPlus1, true
	case st == githubv4.PullRequestReviewStateCommented:
		return state.ReviewNoScore, true
	case githubv4.PullRequestReviewStateChangesRequested:
	case st == githubv4.PullRequestReviewStateChangesRequested && !approver:
		return state.ReviewMinus1, true
	case st == githubv4.PullRequestReviewStateChangesRequested && approver:
		return state.ReviewMinus2, true
	case githubv4.PullRequestReviewStateDismissed:
	case st == githubv4.PullRequestReviewStateDismissed:
		// PullRequestReviewStateDismissed are reviews that have been retroactively dismissed.
		// Display them as a regular comment review for now (we can't know the original state).
		// THINK: Consider displaying these more distinctly.
		return state.ReviewNoScore, true
	case githubv4.PullRequestReviewStatePending:
	case st == githubv4.PullRequestReviewStatePending:
		// PullRequestReviewStatePending are reviews that are pending (haven't been posted yet).
		// TODO: Consider displaying pending review comments. Figure this out
		//       when adding ability to leave reviews.
		return 0, false
	default: