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 2 years ago commit c4416e7845a6acd15a4c3ee4c0b71bd2b390ec42
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{})