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

githubapi: Fetch current user per request, rather than upfront.

This is more robust.

It also removes the need to talk to real GitHub API while creating a
new service, which is undesirable during testing.

Analogous to https://github.com/shurcooL/issues/commit/adf13e46e3d966ef14ee08722b56b823cf5c3f3b.
dmitshur committed 6 years ago commit c4416e7845a6acd15a4c3ee4c0b71bd2b390ec42
Collapse all
githubapi/githubapi.go
@@ -14,41 +14,30 @@ import (
	"github.com/shurcooL/githubql"
	"github.com/shurcooL/issues"
	"github.com/shurcooL/notifications"
	"github.com/shurcooL/reactions"
	"github.com/shurcooL/users"
	ghusers "github.com/shurcooL/users/githubapi"
)

// NewService creates a GitHub-backed change.Service using given GitHub clients.
// It uses notifications service, if not nil. At this time it infers the current user
// from the client (its authentication info), and cannot be used to serve multiple users.
func NewService(clientV3 *github.Client, clientV4 *githubql.Client, notifications notifications.ExternalService) (change.Service, error) {
	users, err := ghusers.NewService(clientV3)
	if err != nil {
		return nil, err
	}
	currentUser, err := users.GetAuthenticated(context.Background())
	if err != nil {
		return nil, err
	}
// from GitHub clients (their authentication info), and cannot be used to serve multiple users.
// Both GitHub clients must use same authentication info.
func NewService(clientV3 *github.Client, clientV4 *githubql.Client, notifications notifications.ExternalService) change.Service {
	return service{
		clV3:          clientV3,
		clV4:          clientV4,
		notifications: notifications,
		currentUser:   currentUser,
	}, nil
	}
}

type service struct {
	clV3 *github.Client   // GitHub REST API v3 client.
	clV4 *githubql.Client // GitHub GraphQL API v4 client.

	// notifications may be nil if there's no notifications service.
	notifications notifications.ExternalService

	currentUser users.User
}

// We use 0 as a special ID for the comment that is the PR description. This comment is edited differently.
const prDescriptionCommentID string = "0"

@@ -189,16 +178,14 @@ func (s service) Get(ctx context.Context, rs string, id uint64) (change.Change,
	err = s.clV4.Query(ctx, &q, variables)
	if err != nil {
		return change.Change{}, err
	}

	if s.currentUser.ID != 0 {
		// Mark as read.
		err = s.markRead(ctx, rs, id)
		if err != nil {
			log.Println("service.Get: failed to markRead:", err)
		}
	// Mark as read. (We know there's an authenticated user since we're using GitHub GraphQL API v4 above.)
	err = s.markRead(ctx, rs, id)
	if err != nil {
		log.Println("service.Get: failed to markRead:", err)
	}

	// TODO: Eliminate comment body properties from issues.Issue. It's missing increasingly more fields, like Edited, etc.
	pr := q.Repository.PullRequest
	return change.Change{
@@ -387,10 +374,11 @@ func (s service) ListTimeline(ctx context.Context, rs string, id uint64, opt *ch
						} `graphql:"comments(first:100)"` // TODO: Pagination... Figure out how to make pagination across 2 resource types work...
					}
				} `graphql:"reviews(first:100)"` // TODO: Pagination... Figure out how to make pagination across 2 resource types work...
			} `graphql:"pullRequest(number:$prNumber)"`
		} `graphql:"repository(owner:$repositoryOwner,name:$repositoryName)"`
		Viewer githubqlUser
	}
	variables := map[string]interface{}{
		"repositoryOwner": githubql.String(repo.Owner),
		"repositoryName":  githubql.String(repo.Repo),
		"prNumber":        githubql.Int(id),
@@ -413,11 +401,11 @@ func (s service) ListTimeline(ctx context.Context, rs string, id uint64, opt *ch
			ID:        prDescriptionCommentID,
			User:      ghActor(pr.Author),
			CreatedAt: pr.PublishedAt.Time,
			Edited:    edited,
			Body:      string(pr.Body),
			Reactions: s.reactions(pr.ReactionGroups),
			Reactions: ghReactions(pr.ReactionGroups, ghUser(&q.Viewer)),
			Editable:  pr.ViewerCanUpdate,
		})
	}
	for _, node := range q.Repository.PullRequest.Timeline.Nodes {
		if node.Typename != "IssueComment" {
@@ -435,11 +423,11 @@ func (s service) ListTimeline(ctx context.Context, rs string, id uint64, opt *ch
			ID:        fmt.Sprintf("c%d", comment.DatabaseID),
			User:      ghActor(comment.Author),
			CreatedAt: comment.PublishedAt.Time,
			Edited:    edited,
			Body:      comment.Body,
			Reactions: s.reactions(comment.ReactionGroups),
			Reactions: ghReactions(comment.ReactionGroups, ghUser(&q.Viewer)),
			Editable:  comment.ViewerCanUpdate,
		})
	}
	for _, review := range q.Repository.PullRequest.Reviews.Nodes {
		state, ok := ghPRReviewState(review.State)
@@ -458,11 +446,11 @@ func (s service) ListTimeline(ctx context.Context, rs string, id uint64, opt *ch
			cs = append(cs, change.InlineComment{
				ID:        fmt.Sprintf("rc%d", comment.DatabaseID),
				File:      comment.Path,
				Line:      comment.OriginalPosition,
				Body:      comment.Body,
				Reactions: s.reactions(comment.ReactionGroups),
				Reactions: ghReactions(comment.ReactionGroups, ghUser(&q.Viewer)),
			})
		}
		sort.Slice(cs, func(i, j int) bool {
			if cs[i].File == cs[j].File {
				return cs[i].Line < cs[j].Line
@@ -610,10 +598,11 @@ func (s service) EditComment(ctx context.Context, rs string, id uint64, cr chang
		// See if user has already reacted with that reaction.
		// If not, add it. Otherwise, remove it.
		var (
			subjectID        githubql.ID
			viewerHasReacted bool
			viewer           users.User
		)
		switch {
		case cr.ID == prDescriptionCommentID:
			var q struct {
				Repository struct {
@@ -622,10 +611,11 @@ func (s service) EditComment(ctx context.Context, rs string, id uint64, cr chang
						Reactions struct {
							ViewerHasReacted bool
						} `graphql:"reactions(content:$reactionContent)"`
					} `graphql:"pullRequest(number:$prNumber)"`
				} `graphql:"repository(owner:$repositoryOwner,name:$repositoryName)"`
				Viewer githubqlUser
			}
			variables := map[string]interface{}{
				"repositoryOwner": githubql.String(repo.Owner),
				"repositoryName":  githubql.String(repo.Repo),
				"prNumber":        githubql.Int(id),
@@ -635,10 +625,11 @@ func (s service) EditComment(ctx context.Context, rs string, id uint64, cr chang
			if err != nil {
				return change.Comment{}, err
			}
			subjectID = q.Repository.PullRequest.ID
			viewerHasReacted = q.Repository.PullRequest.Reactions.ViewerHasReacted
			viewer = ghUser(&q.Viewer)
		case strings.HasPrefix(cr.ID, "c"):
			commentID := "012:IssueComment" + cr.ID[len("c"):]
			var q struct {
				Node struct {
					IssueComment struct {
@@ -646,10 +637,11 @@ func (s service) EditComment(ctx context.Context, rs string, id uint64, cr chang
						Reactions struct {
							ViewerHasReacted bool
						} `graphql:"reactions(content:$reactionContent)"`
					} `graphql:"...on IssueComment"`
				} `graphql:"node(id:$commentID)"`
				Viewer githubqlUser
			}
			variables := map[string]interface{}{
				"commentID":       githubql.ID(base64.StdEncoding.EncodeToString([]byte(commentID))), // HACK, TODO: Confirm StdEncoding vs URLEncoding.
				"reactionContent": reactionContent,
			}
@@ -657,10 +649,11 @@ func (s service) EditComment(ctx context.Context, rs string, id uint64, cr chang
			if err != nil {
				return change.Comment{}, err
			}
			subjectID = q.Node.IssueComment.ID
			viewerHasReacted = q.Node.IssueComment.Reactions.ViewerHasReacted
			viewer = ghUser(&q.Viewer)
		case strings.HasPrefix(cr.ID, "rc"):
			commentID := "024:PullRequestReviewComment" + cr.ID[len("rc"):]
			var q struct {
				Node struct {
					PullRequestReviewComment struct {
@@ -668,10 +661,11 @@ func (s service) EditComment(ctx context.Context, rs string, id uint64, cr chang
						Reactions struct {
							ViewerHasReacted bool
						} `graphql:"reactions(content:$reactionContent)"`
					} `graphql:"...on PullRequestReviewComment"`
				} `graphql:"node(id:$commentID)"`
				Viewer githubqlUser
			}
			variables := map[string]interface{}{
				"commentID":       githubql.ID(base64.StdEncoding.EncodeToString([]byte(commentID))), // HACK, TODO: Confirm StdEncoding vs URLEncoding.
				"reactionContent": reactionContent,
			}
@@ -679,10 +673,11 @@ func (s service) EditComment(ctx context.Context, rs string, id uint64, cr chang
			if err != nil {
				return change.Comment{}, err
			}
			subjectID = q.Node.PullRequestReviewComment.ID
			viewerHasReacted = q.Node.PullRequestReviewComment.Reactions.ViewerHasReacted
			viewer = ghUser(&q.Viewer)
		default:
			return change.Comment{}, fmt.Errorf("EditComment: unrecognized kind of comment ID: %q", cr.ID)
		}

		var rgs reactionGroups
@@ -723,11 +718,11 @@ func (s service) EditComment(ctx context.Context, rs string, id uint64, cr chang
			}
			rgs = m.RemoveReaction.Subject.ReactionGroups
		}
		// TODO: Consider setting other fields? Now that using GraphQL, not that expensive (same API call).
		//       But not needed for app yet...
		comment.Reactions = s.reactions(rgs)
		comment.Reactions = ghReactions(rgs, viewer)
	}

	return comment, nil
}

@@ -883,32 +878,32 @@ type reactionGroups []struct {
		TotalCount int
	} `graphql:"users(first:10)"`
	ViewerHasReacted bool
}

// reactions converts []githubql.ReactionGroup to []reactions.Reaction.
func (s service) reactions(rgs reactionGroups) []reactions.Reaction {
// ghReactions converts []githubql.ReactionGroup to []reactions.Reaction.
func ghReactions(rgs reactionGroups, viewer users.User) []reactions.Reaction {
	var rs []reactions.Reaction
	for _, rg := range rgs {
		if rg.Users.TotalCount == 0 {
			continue
		}

		// Only return the details of first few users and authed user.
		// Only return the details of first few users and viewer.
		var us []users.User
		addedAuthedUser := false
		addedViewer := false
		for i := 0; i < rg.Users.TotalCount; i++ {
			if i < len(rg.Users.Nodes) {
				user := ghUser(rg.Users.Nodes[i])
				us = append(us, user)
				if s.currentUser.ID != 0 && user.UserSpec == s.currentUser.UserSpec {
					addedAuthedUser = true
				if user.UserSpec == viewer.UserSpec {
					addedViewer = true
				}
			} else if i == len(rg.Users.Nodes) {
				// Add authed user last if they've reacted, but haven't been added already.
				if rg.ViewerHasReacted && !addedAuthedUser {
					us = append(us, s.currentUser)
				// Add viewer last if they've reacted, but haven't been added already.
				if rg.ViewerHasReacted && !addedViewer {
					us = append(us, viewer)
				} else {
					us = append(us, users.User{})
				}
			} else {
				us = append(us, users.User{})