MM-68539: SLA digest fixes

server/plugin/graphql/digest_query.go

@@ -0,0 +1,137 @@
+// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package graphql
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	"github.com/pkg/errors"
+	"github.com/shurcooL/githubv4"
+)
+
+// DigestPR is a single open non-draft PR returned by GetOpenPRsWithRequestedReviewers,
+// flattened from the GraphQL search response so callers don't need to know about githubv4.
+type DigestPR struct {
+	Owner          string
+	Repo           string
+	Number         int
+	Title          string
+	URL            string
+	CreatedAt      time.Time
+	RequestedUsers []string
+	RequestedTeams []DigestTeamRef
+}
+
+// DigestTeamRef identifies a team review request that the caller can expand to member logins.
+type DigestTeamRef struct {
+	Org  string // organization login
+	Slug string // team slug
+}
+
+type digestRequestedReviewer struct {
+	Type githubv4.String `graphql:"__typename"`
+	User struct {
+		Login githubv4.String
+	} `graphql:"... on User"`
+	Team struct {
+		Slug         githubv4.String
+		Organization struct {
+			Login githubv4.String
+		}
+	} `graphql:"... on Team"`
+}
+
+type digestPRSearchNode struct {
+	PullRequest struct {
+		Number     githubv4.Int
+		Title      githubv4.String
+		URL        githubv4.URI
+		CreatedAt  githubv4.DateTime
+		Repository struct {
+			Name  githubv4.String
+			Owner struct {
+				Login githubv4.String
+			}
+		}
+		ReviewRequests struct {
+			Nodes []struct {
+				RequestedReviewer digestRequestedReviewer
+			}
+		} `graphql:"reviewRequests(first:100)"`
+	} `graphql:"... on PullRequest"`
+}
+
+// orgOpenPRsSearchQuery is the response shape for GetOpenPRsWithRequestedReviewers. Defined
+// at package scope so the graphql library can reflect on its tags, but instantiated locally
+// per call so concurrent callers (e.g. the digest scheduler running alongside an LHS fetch)
+// don't share mutable state.
+type orgOpenPRsSearchQuery struct {
+	Search struct {
+		Nodes    []digestPRSearchNode
+		PageInfo struct {
+			EndCursor   githubv4.String
+			HasNextPage bool
+		}
+	} `graphql:"search(first:100, after:$cursor, query:$query, type:ISSUE)"`
+}
+
+// GetOpenPRsWithRequestedReviewers returns every open non-draft PR in org along with the
+// users and teams currently requested for review on each one. Pages through the GitHub
+// search API at 100 PRs per call.
+func (c *Client) GetOpenPRsWithRequestedReviewers(ctx context.Context, org string) ([]DigestPR, error) {
+	if org == "" {
+		return nil, errors.New("org is required for org-wide PR search")
+	}
+
+	query := fmt.Sprintf("is:pr is:open archived:false draft:false org:%s", org)
+	params := map[string]any{
+		"query":  githubv4.String(query),
+		"cursor": (*githubv4.String)(nil),
+	}
+
+	var orgOpenPRsQuery orgOpenPRsSearchQuery
+	var out []DigestPR
+	for {
+		if err := c.executeQuery(ctx, &orgOpenPRsQuery, params); err != nil {
+			return nil, errors.Wrapf(err, "org-wide PR search failed for org %q", org)
+		}
+
+		for _, node := range orgOpenPRsQuery.Search.Nodes {
+			pr := DigestPR{
+				Owner:     string(node.PullRequest.Repository.Owner.Login),
+				Repo:      string(node.PullRequest.Repository.Name),
+				Number:    int(node.PullRequest.Number),
+				Title:     string(node.PullRequest.Title),
+				URL:       node.PullRequest.URL.String(),
+				CreatedAt: node.PullRequest.CreatedAt.Time,
+			}
+			for _, rr := range node.PullRequest.ReviewRequests.Nodes {
+				switch string(rr.RequestedReviewer.Type) {
+				case "User":
+					if login := string(rr.RequestedReviewer.User.Login); login != "" {
+						pr.RequestedUsers = append(pr.RequestedUsers, login)
+					}
+				case "Team":
+					orgLogin := string(rr.RequestedReviewer.Team.Organization.Login)
+					slug := string(rr.RequestedReviewer.Team.Slug)
+					if orgLogin == "" {
+						orgLogin = org
+					}
+					if slug != "" {
+						pr.RequestedTeams = append(pr.RequestedTeams, DigestTeamRef{Org: orgLogin, Slug: slug})
+					}
+				}
+			}
+			out = append(out, pr)
+		}
+
+		if !orgOpenPRsQuery.Search.PageInfo.HasNextPage {
+			break
+		}
+		params["cursor"] = githubv4.NewString(orgOpenPRsQuery.Search.PageInfo.EndCursor)
+	}
+	return out, nil
+}

server/plugin/plugin.go

@@ -1137,7 +1137,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient
 
 		for _, pr := range issueResults.Issues {
 			line := strings.TrimSuffix(getToDoDisplayText(baseURL, pr.GetTitle(), pr.GetHTMLURL(), "", nil), "\n")
-			slaStart := p.effectiveReviewSLAStart(pr, baseURL, info.GitHubUsername)
+			slaStart := p.effectiveReviewSLAStart(prRefFromIssue(pr, baseURL), info.GitHubUsername)
 			if suffix, _ := reviewSLAMarkdown(slaStart, targetDays, now); suffix != "" {
 				line += suffix
 			}

server/plugin/review_sla.go

@@ -4,8 +4,10 @@
 package plugin
 
 import (
+	"context"
 	"crypto/sha256"
 	"encoding/hex"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -17,6 +19,29 @@ import (
 
 const slaReviewReqKeyPrefix = "slarr_v1_"
 
+// prRef is the minimal PR identity the SLA code needs: stable enough to derive the KV key,
+// rich enough to fall back to PR open time when no review-request record exists. Centralizing
+// this avoids forging *github.Issue values whenever a non-search caller (e.g. the digest)
+// needs to ask SLA questions.
+type prRef struct {
+	Owner     string
+	Repo      string
+	Number    int
+	CreatedAt github.Timestamp
+}
+
+// prRefFromIssue extracts a prRef from a search-result issue. Owner/repo come from the API
+// response when present, otherwise from the HTML URL (mirrors issueOwnerRepo's behavior).
+func prRefFromIssue(pr *github.Issue, baseURL string) prRef {
+	owner, repo := issueOwnerRepo(pr, baseURL)
+	return prRef{
+		Owner:     owner,
+		Repo:      repo,
+		Number:    pr.GetNumber(),
+		CreatedAt: pr.GetCreatedAt(),
+	}
+}
+
 // reviewSLAStartKey returns a stable KV key for (repo, PR, requested reviewer login).
 func reviewSLAStartKey(owner, repo string, prNumber int, githubLogin string) string {
 	normalized := strings.ToLower(strings.TrimSpace(owner)) + "/" + strings.ToLower(strings.TrimSpace(repo)) +
@@ -37,9 +62,14 @@ func (p *Plugin) recordReviewRequestSLAStart(event *github.PullRequestEvent, req
 	if owner == "" || repo == "" || num == 0 || requestedGitHubLogin == "" {
 		return
 	}
-	key := reviewSLAStartKey(owner, repo, num, requestedGitHubLogin)
-	at := time.Now().UTC()
-	val := []byte(at.Format(time.RFC3339Nano))
+	p.storeReviewSLAStart(owner, repo, num, requestedGitHubLogin, time.Now().UTC())
+}
+
+// storeReviewSLAStart writes a review-request timestamp to KV under the canonical key. Used by
+// both the live webhook path and the digest's timeline backfill so the wire format matches.
+func (p *Plugin) storeReviewSLAStart(owner, repo string, prNumber int, githubLogin string, t time.Time) {
+	key := reviewSLAStartKey(owner, repo, prNumber, githubLogin)
+	val := []byte(t.UTC().Format(time.RFC3339Nano))
 	if _, err := p.store.Set(key, val); err != nil {
 		p.client.Log.Warn("Failed to store review SLA start time", "key", key, "error", err.Error())
 	}
@@ -101,18 +131,147 @@ func issueOwnerRepo(pr *github.Issue, baseURL string) (owner, repo string) {
 	return parseOwnerAndRepo(pr.GetHTMLURL(), baseURL)
 }
 
-// effectiveReviewSLAStart returns the timestamp used for SLA: when we recorded a review_request webhook
-// for this reviewer on this PR, else the PR created time.
-func (p *Plugin) effectiveReviewSLAStart(pr *github.Issue, baseURL, reviewerGitHubLogin string) github.Timestamp {
-	owner, repo := issueOwnerRepo(pr, baseURL)
-	num := pr.GetNumber()
-	if owner == "" || repo == "" || num == 0 {
-		return pr.GetCreatedAt()
+// effectiveReviewSLAStart returns the timestamp used for SLA: when we recorded a review_request
+// webhook for this reviewer on this PR, else the PR created time. Read-only — callers serving
+// user-facing requests (/github todo, RHS) can call freely without network I/O.
+func (p *Plugin) effectiveReviewSLAStart(pr prRef, reviewerGitHubLogin string) github.Timestamp {
+	if pr.Owner == "" || pr.Repo == "" || pr.Number == 0 {
+		return pr.CreatedAt
 	}
-	if t := p.getReviewSLAStartTime(owner, repo, num, reviewerGitHubLogin); !t.IsZero() {
+	if t := p.getReviewSLAStartTime(pr.Owner, pr.Repo, pr.Number, reviewerGitHubLogin); !t.IsZero() {
 		return github.Timestamp{Time: t}
 	}
-	return pr.GetCreatedAt()
+	return pr.CreatedAt
+}
+
+// findMostRecentReviewRequestTime walks PR timeline events chronologically and returns the
+// timestamp of the most recent surviving review_requested event for githubLogin. Returns the
+// zero time if the user has no current pending request (e.g. the request was later removed
+// without being re-requested).
+func findMostRecentReviewRequestTime(events []*github.Timeline, githubLogin string) time.Time {
+	target := strings.ToLower(strings.TrimSpace(githubLogin))
+	if target == "" {
+		return time.Time{}
+	}
+
+	// Defensive sort by CreatedAt ascending; GitHub typically returns events in chronological
+	// order already, but pagination joins are not guaranteed to be ordered across pages.
+	sorted := make([]*github.Timeline, 0, len(events))
+	for _, e := range events {
+		if e == nil || e.CreatedAt == nil {
+			continue
+		}
+		sorted = append(sorted, e)
+	}
+	sort.SliceStable(sorted, func(i, j int) bool {
+		return sorted[i].CreatedAt.Before(sorted[j].CreatedAt.Time)
+	})
+
+	var current time.Time
+	for _, e := range sorted {
+		if e.Reviewer == nil {
+			continue
+		}
+		if strings.ToLower(e.Reviewer.GetLogin()) != target {
+			continue
+		}
+		switch e.GetEvent() {
+		case "review_requested":
+			current = e.CreatedAt.Time
+		case "review_request_removed":
+			current = time.Time{}
+		}
+	}
+	return current
+}
+
+// findEarliestSurvivingTeamRequestTime walks PR timeline events and returns the earliest
+// surviving review_requested event time across any of the given teams. "Surviving" means
+// not later cancelled by a matching review_request_removed event for the same team. Returns
+// the zero time when no team has a still-active request (or when teams is empty).
+//
+// Used as a fallback for reviewers added solely via team membership: their user-scoped
+// timeline doesn't contain a review_requested event, so without this they'd fall all the way
+// back to the PR's created_at and overstate days-overdue. The earliest still-active team
+// request is the right anchor: if a user is in two requested teams, they have been on the
+// hook since the first ask, and a later team request doesn't reset that clock.
+//
+// Match is on team slug only. A PR's timeline lives within a single GitHub org, and team
+// slugs are unique within an org, so cross-org slug collision isn't possible here.
+func findEarliestSurvivingTeamRequestTime(events []*github.Timeline, teams []graphql.DigestTeamRef) time.Time {
+	if len(teams) == 0 {
+		return time.Time{}
+	}
+	wantedSlugs := make(map[string]bool, len(teams))
+	for _, t := range teams {
+		slug := strings.ToLower(strings.TrimSpace(t.Slug))
+		if slug != "" {
+			wantedSlugs[slug] = true
+		}
+	}
+	if len(wantedSlugs) == 0 {
+		return time.Time{}
+	}
+
+	sorted := make([]*github.Timeline, 0, len(events))
+	for _, e := range events {
+		if e == nil || e.CreatedAt == nil {
+			continue
+		}
+		sorted = append(sorted, e)
+	}
+	sort.SliceStable(sorted, func(i, j int) bool {
+		return sorted[i].CreatedAt.Before(sorted[j].CreatedAt.Time)
+	})
+
+	surviving := make(map[string]time.Time)
+	for _, e := range sorted {
+		if e.RequestedTeam == nil {
+			continue
+		}
+		slug := strings.ToLower(e.RequestedTeam.GetSlug())
+		if !wantedSlugs[slug] {
+			continue
+		}
+		switch e.GetEvent() {
+		case "review_requested":
+			surviving[slug] = e.CreatedAt.Time
+		case "review_request_removed":
+			delete(surviving, slug)
+		}
+	}
+
+	var earliest time.Time
+	for _, t := range surviving {
+		if earliest.IsZero() || t.Before(earliest) {
+			earliest = t
+		}
+	}
+	return earliest
+}
+
+// fetchPRTimeline returns every timeline event for (owner, repo, prNumber), paging until done.
+// Pulled out of the digest's backfill so it can be cached at a higher level (one fetch per PR
+// even when many reviewers on the same PR need backfilling).
+func fetchPRTimeline(ctx context.Context, gh *github.Client, owner, repo string, prNumber int) ([]*github.Timeline, error) {
+	if gh == nil || owner == "" || repo == "" || prNumber == 0 {
+		return nil, nil
+	}
+
+	var events []*github.Timeline
+	opts := &github.ListOptions{PerPage: 100}
+	for {
+		page, resp, err := gh.Issues.ListIssueTimeline(ctx, owner, repo, prNumber, opts)
+		if err != nil {
+			return nil, err
+		}
+		events = append(events, page...)
+		if resp == nil || resp.NextPage == 0 {
+			break
+		}
+		opts.Page = resp.NextPage
+	}
+	return events, nil
 }
 
 // enrichReviewsWithSLAStart sets review_sla_start on LHS review items so the webapp can match server SLA logic.
@@ -126,7 +285,7 @@ func (p *Plugin) enrichReviewsWithSLAStart(reviews []*graphql.GithubPRDetails, r
 		if d == nil || d.Issue == nil {
 			continue
 		}
-		eff := p.effectiveReviewSLAStart(d.Issue, baseURL, reviewerLogin)
+		eff := p.effectiveReviewSLAStart(prRefFromIssue(d.Issue, baseURL), reviewerLogin)
 		if eff.IsZero() {
 			continue
 		}