From 5b58c5b6bfd6c5e58a7d8b450ef671b33ab7ffd6 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Fri, 19 Jun 2026 20:59:03 -0700 Subject: [PATCH 1/3] MM-69257: Add configurable digest service user for SLA channel digests Org-wide digest GraphQL requires a stable connected GitHub account; admins can now set the Mattermost username instead of relying on lexicographic KV order. --- plugin.json | 8 ++++++- server/plugin/configuration.go | 4 ++++ server/plugin/sla_digest.go | 37 +++++++++++++++++++++++++---- server/plugin/sla_digest_test.go | 40 +++++++++++++++++++++++++++++++- 4 files changed, 82 insertions(+), 7 deletions(-) diff --git a/plugin.json b/plugin.json index 0ef7dcb79..20b30fe86 100644 --- a/plugin.json +++ b/plugin.json @@ -146,7 +146,13 @@ "key": "OverdueReviewsChannelID", "display_name": "Post overdue review alerts to channel (ID):", "type": "text", - "help_text": "Optional. Paste a channel ID (Channel menu > View Info). When set together with PR review target (days) and GitHub Organizations, the plugin posts one aggregated overdue-review digest for those orgs to this channel each day shortly after midnight in the server local timezone. Requires at least one user connected via /github connect. The GitHub bot must be a member of the channel." + "help_text": "Optional. Paste a channel ID (Channel menu > View Info). When set together with PR review target (days) and GitHub Organizations, the plugin posts one aggregated overdue-review digest for those orgs to this channel each day shortly after midnight in the server local timezone. Requires digest service user (below) to be connected via /github connect. The GitHub bot must be a member of the channel." + }, + { + "key": "DigestServiceUsername", + "display_name": "Digest service user (Mattermost username):", + "type": "text", + "help_text": "Optional. Mattermost @username whose GitHub connection runs the overdue review digest (org-wide GraphQL and team lookups). Use a stable account that will stay connected to GitHub, e.g. a team lead or bot user. When empty, the plugin uses the lexicographically-first connected user, which can be hard to predict." } ], "footer": "* To report an issue, make a suggestion or a contribution, [check the repository](https://github.com/mattermost/mattermost-plugin-github)." diff --git a/server/plugin/configuration.go b/server/plugin/configuration.go index 9c2d31c57..41fe7965d 100644 --- a/server/plugin/configuration.go +++ b/server/plugin/configuration.go @@ -46,6 +46,9 @@ type Configuration struct { ReviewTargetDays int `json:"reviewtargetdays"` // OverdueReviewsChannelID is an optional channel ID for daily alerts when users have overdue review requests. OverdueReviewsChannelID string `json:"overduereviewschannelid"` + // DigestServiceUsername is the Mattermost username whose GitHub connection runs the overdue review digest. + // When empty, the plugin falls back to the lexicographically-first connected user. + DigestServiceUsername string `json:"digestserviceusername"` } func (c *Configuration) ToMap() (map[string]any, error) { @@ -105,6 +108,7 @@ func (c *Configuration) sanitize() { c.EnterpriseBaseURL = strings.TrimRight(c.EnterpriseBaseURL, "/") c.EnterpriseUploadURL = strings.TrimRight(c.EnterpriseUploadURL, "/") c.OverdueReviewsChannelID = strings.TrimSpace(c.OverdueReviewsChannelID) + c.DigestServiceUsername = strings.TrimSpace(c.DigestServiceUsername) if c.ReviewTargetDays < 0 { c.ReviewTargetDays = 0 } diff --git a/server/plugin/sla_digest.go b/server/plugin/sla_digest.go index a14a1d330..9988ab9db 100644 --- a/server/plugin/sla_digest.go +++ b/server/plugin/sla_digest.go @@ -114,13 +114,40 @@ func (p *Plugin) resolveReviewerDisplayName(githubLogin string) string { return fmt.Sprintf("(not connected) - %s", githubLogin) } -// pickServiceGitHubUser returns the first connected user the digest can use as a service -// caller for org-wide GitHub queries. Keys are sorted before iteration so the choice is -// deterministic across runs (and across cluster nodes) — otherwise the digest's visibility -// into private repos/teams could silently shift run-to-run with KV iteration order. +// pickServiceGitHubUser returns the connected user the digest uses as a service caller for +// org-wide GitHub queries. When DigestServiceUsername is set, only that Mattermost user is +// considered; otherwise keys are sorted before iteration so the choice is deterministic +// across runs (and across cluster nodes). // -// Returns nil when no connected user is available; the digest cannot run in that case. +// Returns nil when no usable connected user is available; the digest cannot run in that case. func (p *Plugin) pickServiceGitHubUser(ctx context.Context) *GitHubUserInfo { + if configured := strings.TrimSpace(p.getConfiguration().DigestServiceUsername); configured != "" { + if ctx.Err() != nil { + return nil + } + user, err := p.client.User.GetByUsername(configured) + if err != nil { + p.client.Log.Warn("SLA digest configured service user not found", + "username", configured, "error", err.Error()) + return nil + } + if user == nil { + p.client.Log.Warn("SLA digest configured service user not found", "username", configured) + return nil + } + ghInfo, apiErr := p.getGitHubUserInfo(user.Id) + if apiErr != nil || ghInfo == nil { + p.client.Log.Warn("SLA digest configured service user is not connected to GitHub", + "username", configured, "user_id", user.Id) + return nil + } + p.client.Log.Info("SLA digest using configured service user", + "mattermost_username", configured, + "github_username", ghInfo.GitHubUsername, + "user_id", user.Id) + return ghInfo + } + checker := func(key string) (bool, error) { return strings.HasSuffix(key, githubTokenKey), nil } diff --git a/server/plugin/sla_digest_test.go b/server/plugin/sla_digest_test.go index 196ea5850..96e82ee41 100644 --- a/server/plugin/sla_digest_test.go +++ b/server/plugin/sla_digest_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/oauth2" + "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/plugin/plugintest" "github.com/mattermost/mattermost/server/public/pluginapi" @@ -428,6 +429,42 @@ func TestPickServiceGitHubUser_NoConnectedUsers(t *testing.T) { assert.Nil(t, p.pickServiceGitHubUser(context.Background())) } +func TestPickServiceGitHubUser_ConfiguredMattermostUsername(t *testing.T) { + p, mockKvStore, ctrl := setupServiceUserPickTest(t) + defer ctrl.Finish() + + p.setConfiguration(&Configuration{ + EncryptionKey: "dummyEncryptKey1", + DigestServiceUsername: "it33", + }) + + api, ok := p.API.(*plugintest.API) + require.True(t, ok, "expected plugintest.API") + api.On("GetUserByUsername", "it33").Return(&model.User{Id: "ceo-user-id", Username: "it33"}, nil) + api.On("LogInfo", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + + encryptedToken, err := encrypt([]byte("dummyEncryptKey1"), MockAccessToken) + require.NoError(t, err) + storedInfo := GitHubUserInfo{ + UserID: "ceo-user-id", + GitHubUsername: "ceo-gh", + Token: &oauth2.Token{AccessToken: encryptedToken}, + Settings: &UserSettings{}, + } + infoBytes, err := json.Marshal(&storedInfo) + require.NoError(t, err) + mockKvStore.EXPECT().Get("ceo-user-id"+githubTokenKey, gomock.Any()).DoAndReturn( + func(_ string, out any) error { + return json.Unmarshal(infoBytes, out) + }, + ) + + picked := p.pickServiceGitHubUser(context.Background()) + require.NotNil(t, picked) + assert.Equal(t, "ceo-user-id", picked.UserID) + assert.Equal(t, "ceo-gh", picked.GitHubUsername) +} + func TestNewDigestSLAStartResolver_LogsAccurateCounters(t *testing.T) { // The summary log must (a) not fire until summarize() is called and (b) reflect the // resolver's actual usage when it does. Locks in the fix for a prior bug where the log @@ -533,7 +570,8 @@ func setupServiceUserPickTest(t *testing.T) (*Plugin, *mocks.MockKvStore, *gomoc mockKvStore := mocks.NewMockKvStore(ctrl) api := &plugintest.API{} - api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() api.On("LogDebug", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Maybe() p := NewPlugin() From f4091c2999c57adb73d1c0a3cecdbb1194eb95f4 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Fri, 19 Jun 2026 20:59:32 -0700 Subject: [PATCH 2/3] MM-69257: Clip SLA digest channel posts at 64K runes Large org digests can exceed Mattermost post limits and fail silently; cap the message and append a clipped notice so the digest still posts. --- server/plugin/plugin.go | 13 +------------ server/plugin/sla_digest.go | 9 ++++++++- server/plugin/sla_digest_test.go | 22 ++++++++++++++++++++++ server/plugin/utils.go | 22 ++++++++++++++++++++++ 4 files changed, 53 insertions(+), 13 deletions(-) diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index a57005db4..96a723da5 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -14,7 +14,6 @@ import ( "strings" "sync" "time" - "unicode/utf8" "github.com/google/go-github/v54/github" "github.com/gorilla/mux" @@ -968,17 +967,7 @@ func (p *Plugin) CreateBotDMPost(userID, message, postType string) { func truncatePostMessage(message string) string { const truncationMarker = "\n\n_… message truncated_" - - if utf8.RuneCountInString(message) <= model.PostMessageMaxRunesV2 { - return message - } - - keep := model.PostMessageMaxRunesV2 - utf8.RuneCountInString(truncationMarker) - if keep <= 0 { - return string([]rune(truncationMarker)[:model.PostMessageMaxRunesV2]) - } - - return string([]rune(message)[:keep]) + truncationMarker + return truncateMessageAtRunes(message, model.PostMessageMaxRunesV2, truncationMarker) } func (p *Plugin) CheckIfDuplicateDailySummary(userID, text string) (bool, error) { diff --git a/server/plugin/sla_digest.go b/server/plugin/sla_digest.go index 9988ab9db..3ece31fde 100644 --- a/server/plugin/sla_digest.go +++ b/server/plugin/sla_digest.go @@ -24,6 +24,9 @@ const ( // slaDigestDayKVKey stores the last local calendar day (server timezone) we posted or skipped the digest. slaDigestDayKVKey = "github_sla_digest_local_day" slaDigestMutexKey = "github_sla_digest_mutex" + // slaDigestMaxMessageRunes caps the channel digest below Mattermost post limits (64K runes). + slaDigestMaxMessageRunes = 64000 + slaDigestClippedMarker = "\n\n_This digest was clipped due to message size limits._" ) // slaDigestEntry is one (reviewer, PR) pair past SLA. Stored as separate fields rather than @@ -84,7 +87,7 @@ func (p *Plugin) maybePostDailyOverdueSLADigest(ctx context.Context) { return } - msg := buildSLADigestMessage(entries, cfg.ReviewTargetDays) + msg := clipSLADigestMessage(buildSLADigestMessage(entries, cfg.ReviewTargetDays)) post := &model.Post{ ChannelId: cfg.OverdueReviewsChannelID, UserId: p.BotUserID, @@ -574,3 +577,7 @@ func buildSLADigestMessage(entries []slaDigestEntry, targetDays int) string { } return strings.TrimSpace(b.String()) } + +func clipSLADigestMessage(message string) string { + return truncateMessageAtRunes(message, slaDigestMaxMessageRunes, slaDigestClippedMarker) +} diff --git a/server/plugin/sla_digest_test.go b/server/plugin/sla_digest_test.go index 96e82ee41..7264e86b4 100644 --- a/server/plugin/sla_digest_test.go +++ b/server/plugin/sla_digest_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" "time" + "unicode/utf8" "github.com/golang/mock/gomock" "github.com/google/go-github/v54/github" @@ -236,6 +237,27 @@ func TestBuildSLADigestMessage(t *testing.T) { }) } +func TestClipSLADigestMessage(t *testing.T) { + t.Run("short message is unchanged", func(t *testing.T) { + msg := "hello" + assert.Equal(t, msg, clipSLADigestMessage(msg)) + }) + + t.Run("oversized message is clipped with marker", func(t *testing.T) { + msg := strings.Repeat("a", slaDigestMaxMessageRunes+1000) + out := clipSLADigestMessage(msg) + assert.LessOrEqual(t, utf8.RuneCountInString(out), slaDigestMaxMessageRunes) + assert.True(t, strings.HasSuffix(out, slaDigestClippedMarker)) + }) + + t.Run("multibyte runes are not split", func(t *testing.T) { + msg := strings.Repeat("✓", slaDigestMaxMessageRunes+500) + out := clipSLADigestMessage(msg) + assert.LessOrEqual(t, utf8.RuneCountInString(out), slaDigestMaxMessageRunes) + assert.True(t, utf8.ValidString(out)) + }) +} + func TestGatherReviewersForPR(t *testing.T) { logins := func(rrs []reviewerRequest) []string { out := make([]string, 0, len(rrs)) diff --git a/server/plugin/utils.go b/server/plugin/utils.go index 4f590aa6f..f1b3fd097 100644 --- a/server/plugin/utils.go +++ b/server/plugin/utils.go @@ -18,6 +18,7 @@ import ( "strings" "time" "unicode" + "unicode/utf8" "github.com/mattermost/mattermost/server/public/pluginapi" @@ -404,6 +405,27 @@ func formatChannelOverduePRBody(title, htmlURL, baseURL string) string { return fmt.Sprintf("%s - %s", repoDisplay, titleDisplay) } +// truncateMessageAtRunes shortens message to at most maxRunes runes, appending marker when clipped. +// Truncation is rune-aware so multibyte UTF-8 sequences are not split. +func truncateMessageAtRunes(message string, maxRunes int, marker string) string { + if maxRunes <= 0 { + return "" + } + if utf8.RuneCountInString(message) <= maxRunes { + return message + } + markerRunes := utf8.RuneCountInString(marker) + keep := maxRunes - markerRunes + if keep <= 0 { + runes := []rune(marker) + if len(runes) > maxRunes { + return string(runes[:maxRunes]) + } + return marker + } + return string([]rune(message)[:keep]) + marker +} + // escapeMarkdownLinkText escapes characters that would break a markdown link's display text. // We only escape `]` and `\` so that titles containing brackets (common in JIRA-prefixed PRs) // do not terminate the link early. From 4089bf7d55824b7cb744ad3a4fdafe0a92b31b3e Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Sat, 20 Jun 2026 11:10:47 -0700 Subject: [PATCH 3/3] MM-69257: Use human-readable SLA digest overdue buckets Replace day-range headers with week- and month-based labels so channel digests are easier to scan at a glance. --- server/plugin/sla_digest.go | 15 ++++----- server/plugin/sla_digest_test.go | 54 +++++++++++++++++--------------- 2 files changed, 36 insertions(+), 33 deletions(-) diff --git a/server/plugin/sla_digest.go b/server/plugin/sla_digest.go index 3ece31fde..9b52783fd 100644 --- a/server/plugin/sla_digest.go +++ b/server/plugin/sla_digest.go @@ -480,13 +480,14 @@ var slaBuckets = []struct { min int // inclusive; -1 for open-ended upper bucket max int // inclusive; -1 for open-ended upper bucket }{ - {label: "More than 1 year overdue", min: 366, max: -1}, - {label: "91-365 days overdue", min: 91, max: 365}, - {label: "31-90 days overdue", min: 31, max: 90}, - {label: "15-30 days overdue", min: 15, max: 30}, - {label: "8-14 days overdue", min: 8, max: 14}, - {label: "4-7 days overdue", min: 4, max: 7}, - {label: "1-3 days overdue", min: 1, max: 3}, + {label: "Over a year overdue", min: 365, max: -1}, + {label: "Overdue by 6 months", min: 180, max: 364}, + {label: "Overdue by 3 months", min: 90, max: 179}, + {label: "Overdue by 1 month", min: 28, max: 89}, + {label: "Overdue by 3 weeks", min: 21, max: 27}, + {label: "Overdue by 2 weeks", min: 14, max: 20}, + {label: "Overdue by 1 week", min: 7, max: 13}, + {label: "Overdue", min: 1, max: 6}, } // slaBucketIndex returns the index into slaBuckets for the given days-overdue value, or -1 if not overdue. diff --git a/server/plugin/sla_digest_test.go b/server/plugin/sla_digest_test.go index 7264e86b4..c45365a52 100644 --- a/server/plugin/sla_digest_test.go +++ b/server/plugin/sla_digest_test.go @@ -86,20 +86,22 @@ func TestSLABucketIndex(t *testing.T) { }{ {0, ""}, {-3, ""}, - {1, "1-3 days overdue"}, - {3, "1-3 days overdue"}, - {4, "4-7 days overdue"}, - {7, "4-7 days overdue"}, - {8, "8-14 days overdue"}, - {14, "8-14 days overdue"}, - {15, "15-30 days overdue"}, - {30, "15-30 days overdue"}, - {31, "31-90 days overdue"}, - {90, "31-90 days overdue"}, - {91, "91-365 days overdue"}, - {365, "91-365 days overdue"}, - {366, "More than 1 year overdue"}, - {1000, "More than 1 year overdue"}, + {1, "Overdue"}, + {6, "Overdue"}, + {7, "Overdue by 1 week"}, + {13, "Overdue by 1 week"}, + {14, "Overdue by 2 weeks"}, + {20, "Overdue by 2 weeks"}, + {21, "Overdue by 3 weeks"}, + {27, "Overdue by 3 weeks"}, + {28, "Overdue by 1 month"}, + {89, "Overdue by 1 month"}, + {90, "Overdue by 3 months"}, + {179, "Overdue by 3 months"}, + {180, "Overdue by 6 months"}, + {364, "Overdue by 6 months"}, + {365, "Over a year overdue"}, + {1000, "Over a year overdue"}, } for _, tc := range cases { @@ -133,15 +135,15 @@ func TestBuildSLADigestMessage(t *testing.T) { assert.True(t, strings.HasPrefix(msg, "### Pull request reviews past SLA (target: 3 days from most recent review request)")) // Verify bucket order in message: most overdue first. - idxYear := strings.Index(msg, "#### More than 1 year overdue") - idx91 := strings.Index(msg, "#### 91-365 days overdue") - idx8 := strings.Index(msg, "#### 8-14 days overdue") - idx4 := strings.Index(msg, "#### 4-7 days overdue") - idx1 := strings.Index(msg, "#### 1-3 days overdue") - assert.True(t, idxYear >= 0 && idx91 > idxYear && idx8 > idx91 && idx4 > idx8 && idx1 > idx4, "buckets should appear in most-overdue-first order") - - assert.NotContains(t, msg, "#### 31-90 days overdue", "empty bucket should be omitted") - assert.NotContains(t, msg, "#### 15-30 days overdue", "empty bucket should be omitted") + idxYear := strings.Index(msg, "#### Over a year overdue") + idx3mo := strings.Index(msg, "#### Overdue by 3 months") + idx1wk := strings.Index(msg, "#### Overdue by 1 week") + idxOverdue := strings.Index(msg, "#### Overdue\n") + assert.True(t, idxYear >= 0 && idx3mo > idxYear && idx1wk > idx3mo && idxOverdue > idx1wk, + "buckets should appear in most-overdue-first order") + + assert.NotContains(t, msg, "#### Overdue by 6 months", "empty bucket should be omitted") + assert.NotContains(t, msg, "#### Overdue by 1 month", "empty bucket should be omitted") }) t.Run("non-overdue entries are dropped", func(t *testing.T) { @@ -190,8 +192,8 @@ func TestBuildSLADigestMessage(t *testing.T) { // The reviewer header must appear EXACTLY once in this bucket — that's the whole // point of grouping; otherwise the digest still @-spams the reviewer per-PR. - bucketStart := strings.Index(msg, "#### 1-3 days overdue") - require.True(t, bucketStart >= 0, "expected the 1-3 days bucket header") + bucketStart := strings.Index(msg, "#### Overdue\n") + require.True(t, bucketStart >= 0, "expected the Overdue bucket header") bucketSlice := msg[bucketStart:] assert.Equal(t, 1, strings.Count(bucketSlice, "- "+reviewer+"\n"), "reviewer should appear once per bucket as the outer bullet") @@ -211,7 +213,7 @@ func TestBuildSLADigestMessage(t *testing.T) { entry(2, "@bob (bob-gh)", "o/r - [c-pr](url)"), } msg := buildSLADigestMessage(entries, 3) - bucketStart := strings.Index(msg, "#### 1-3 days overdue") + bucketStart := strings.Index(msg, "#### Overdue\n") require.True(t, bucketStart >= 0) bucket := msg[bucketStart:]