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

githubapi: remove NewService parameter notifications.ExternalService

The responsibility of marking changes as read is being moved from
the service level to the changes application. This approach scales
better.

It makes it possible to use the changes service for other internal
purposes. For example, it's being used internally as part of
implementing a Gerrit notification v2 service.

It also means the changes application can decide to mark a thread as
read when the user scrolls down and sees the new content, instead of
right away on page load.

In general, it's easier for the changes application to know (with more
certainty) when a change has been read by a user.
dmitshur committed 5 years ago commit 09fc1cdc80ff92256db38ada7ae5bdecad9db138
Collapse all
githubapi/githubapi.go
@@ -4,49 +4,42 @@ package githubapi

import (
	"context"
	"encoding/base64"
	"fmt"
	"log"
	"sort"
	"strings"

	"dmitri.shuralyov.com/route/github"
	"dmitri.shuralyov.com/service/change"
	githubv3 "github.com/google/go-github/github"
	"github.com/shurcooL/githubv4"
	"github.com/shurcooL/issues"
	"github.com/shurcooL/notifications"
	"github.com/shurcooL/reactions"
	"github.com/shurcooL/users"
)

// 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 GitHub clients (their authentication info), and cannot be used to serve multiple users.
// Both GitHub clients must use same authentication info.
// At this time it infers the current user from GitHub clients (their authentication info),
// and cannot be used to serve multiple users. Both GitHub clients must use same authentication info.
//
// If router is nil, github.DotCom router is used, which links to subjects on github.com.
func NewService(clientV3 *githubv3.Client, clientV4 *githubv4.Client, notifications notifications.ExternalService, router github.Router) change.Service {
func NewService(clientV3 *githubv3.Client, clientV4 *githubv4.Client, router github.Router) change.Service {
	if router == nil {
		router = github.DotCom{}
	}
	return service{
		clV3:          clientV3,
		clV4:          clientV4,
		rtr:           router,
		notifications: notifications,
		clV3: clientV3,
		clV4: clientV4,
		rtr:  router,
	}
}

type service struct {
	clV3 *githubv3.Client // GitHub REST API v3 client.
	clV4 *githubv4.Client // GitHub GraphQL API v4 client.
	rtr  github.Router

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

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

@@ -187,16 +180,10 @@ 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
	}

	// 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{
		ID:           pr.Number,
		State:        ghPRState(pr.State),
@@ -1018,14 +1005,5 @@ func externalizeReaction(reaction reactions.EmojiID) (githubv4.ReactionContent,
// githubPRThreadType is the notification thread type for GitHub Pull Requests.
const githubPRThreadType = "PullRequest"

// ThreadType returns the notification thread type for this service.
func (service) ThreadType(repo string) string { return githubPRThreadType }

// markRead marks the specified issue as read for current user.
func (s service) markRead(ctx context.Context, repo string, id uint64) error {
	if s.notifications == nil {
		return nil
	}

	return s.notifications.MarkRead(ctx, notifications.RepoSpec{URI: repo}, threadType, id)
}