MM-69257: Address issues with github digest on Mattermost

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)."

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
 	}

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) {

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,
@@ -114,13 +117,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
 	}
@@ -450,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.
@@ -547,3 +578,7 @@ func buildSLADigestMessage(entries []slaDigestEntry, targetDays int) string {
 	}
 	return strings.TrimSpace(b.String())
 }
+
+func clipSLADigestMessage(message string) string {
+	return truncateMessageAtRunes(message, slaDigestMaxMessageRunes, slaDigestClippedMarker)
+}

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"
@@ -18,6 +19,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"
 
@@ -84,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 {
@@ -131,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) {
@@ -188,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")
@@ -209,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:]
 
@@ -235,6 +239,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))
@@ -428,6 +453,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 +594,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()

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.