dmitri.shuralyov.com/app/changes

mark changes as read

The responsibility of marking changes as read is being moved from
the service level to the changes application. It has already been
removed from the change service. In this commit, it's being added
to the changes application. Use the existing Options.Notifications
field that provides a notification service and its MarkRead method
for accomplishing this.

In general, it's easier for the changes application to know (with
higher certainty) when a change has been read by a user. For now,
it continues to happen whenever the HTTP handler serves a page that
displays a change. In the future, we 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.

Also update cmd/changesdev for githubapi.NewService API change.

Follows https://dmitri.shuralyov.com/service/change/githubapi$commit/09fc1cdc80ff92256db38ada7ae5bdecad9db138

dmitshur committed 5 years ago commit 2d5cbe16520770145f8d6e7e3fa8d17a81c896a5

Showing partial commit. Full Commit

Collapse all

main.go

@@ -94,12 +94,15 @@ var RepoSpecContextKey = &contextKey{"RepoSpec"}
// The associated value will be of type string.
var BaseURIContextKey = &contextKey{"BaseURI"}

// Options for configuring changes app.
type Options struct {
	Notifications    notifications.Service // If not nil, changes containing unread notifications are highlighted.
	DisableReactions bool                  // Disable all support for displaying and toggling reactions.
	// Notifications, if not nil, is used to highlight changes containing
	// unread notifications, and to mark changes that are viewed as read.
	Notifications notifications.Service

	DisableReactions bool // Disable all support for displaying and toggling reactions.

	HeadPre, HeadPost template.HTML
	BodyPre           string // An html/template definition of "body-pre" template.

	// BodyTop provides components to include on top of <body> of page rendered for req. It can be nil.
@@ -214,11 +217,13 @@ func (h *handler) ChangesHandler(w http.ResponseWriter, req *http.Request) error
	}
	var es []component.ChangeEntry
	for _, i := range is {
		es = append(es, component.ChangeEntry{Change: i, BaseURI: state.BaseURI})
	}
	es = state.augmentUnread(req.Context(), es, h.cs, h.Notifications)
	if h.Notifications != nil {
		es = state.augmentUnread(req.Context(), es, h.Notifications, h.cs)
	}
	state.Changes = component.Changes{
		ChangesNav: component.ChangesNav{
			OpenCount:     openCount,
			ClosedCount:   closedCount,
			Path:          state.BaseURI + state.ReqPath,
@@ -255,42 +260,40 @@ func stateFilter(query url.Values) (change.StateFilter, error) {
	default:
		return "", fmt.Errorf("unsupported state filter value: %q", selectedTabName)
	}
}

func (s state) augmentUnread(ctx context.Context, es []component.ChangeEntry, service change.Service, notificationsService notifications.Service) []component.ChangeEntry {
	if notificationsService == nil {
		return es
	}

	tt, ok := service.(interface {
func (s state) augmentUnread(ctx context.Context, es []component.ChangeEntry, notificationService notifications.Service, changeService change.Service) []component.ChangeEntry {
	tt, ok := changeService.(interface {
		ThreadType(repo string) string
	})
	if !ok {
		log.Println("augmentUnread: change service doesn't implement ThreadType")
		return es
	}
	threadType := tt.ThreadType(s.RepoSpec)

	if s.CurrentUser.ID == 0 {
		// Unauthenticated user cannot have any unread changes.
		return es
	}

	// TODO: Consider starting to do this in background in parallel with is.List.
	ns, err := notificationsService.List(ctx, notifications.ListOptions{
	ns, err := notificationService.List(ctx, notifications.ListOptions{
		Repo: &notifications.RepoSpec{URI: s.RepoSpec},
		All:  false,
	})
	if err != nil {
		log.Println("augmentUnread: failed to notifications.List:", err)
		return es
	}

	unreadThreads := make(map[uint64]struct{}) // Set of unread thread IDs.
	for _, n := range ns {
		// n.RepoSpec == s.RepoSpec is guaranteed because we filtered in notifications.ListOptions,
		// so we only need to check that n.ThreadType matches.
		if n.ThreadType != tt.ThreadType(s.RepoSpec) {
		if n.ThreadType != threadType {
			continue
		}
		unreadThreads[n.ThreadID] = struct{}{}
	}

@@ -362,10 +365,16 @@ func (h *handler) ChangeHandler(w http.ResponseWriter, req *http.Request, change
	}
	ts, err := h.cs.ListTimeline(req.Context(), state.RepoSpec, state.ChangeID, nil)
	if err != nil {
		return fmt.Errorf("changes.ListTimeline: %v", err)
	}
	if h.Notifications != nil {
		err := state.markRead(req.Context(), h.Notifications, h.cs)
		if err != nil {
			log.Println("ChangeHandler: failed to markRead:", err)
		}
	}
	var timeline []timelineItem
	for _, item := range ts {
		timeline = append(timeline, timelineItem{item})
	}
	sort.Sort(byCreatedAtID(timeline))
@@ -381,10 +390,29 @@ func (h *handler) ChangeHandler(w http.ResponseWriter, req *http.Request, change
		return fmt.Errorf("t.ExecuteTemplate: %v", err)
	}
	return nil
}

func (s state) markRead(ctx context.Context, notificationService notifications.Service, changeService change.Service) error {
	tt, ok := changeService.(interface {
		ThreadType(repo string) string
	})
	if !ok {
		log.Println("markRead: change service doesn't implement ThreadType")
		return nil
	}
	threadType := tt.ThreadType(s.RepoSpec)

	if s.CurrentUser.ID == 0 {
		// Unauthenticated user cannot mark anything as read.
		return nil
	}

	err := notificationService.MarkRead(ctx, notifications.RepoSpec{URI: s.RepoSpec}, threadType, s.ChangeID)
	return err
}

func (h *handler) ChangeCommitsHandler(w http.ResponseWriter, req *http.Request, changeID uint64) error {
	if req.Method != http.MethodGet {
		return httperror.Method{Allowed: []string{http.MethodGet}}
	}
	state, err := h.state(req, changeID)