From a9abd4d75b335c80d7b4318b20dda98461879e24 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Tue, 4 Aug 2026 12:46:29 -0700 Subject: [PATCH 1/2] MM-70112: add business-days option for PR review SLA due dates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow admins to count the review target in Mon–Fri business days so short SLAs requested on Friday are not overdue by Monday. Overdue age and digest buckets remain calendar days; calendar mode stays the default. --- plugin.json | 19 ++++++- server/plugin/configuration.go | 15 ++++-- server/plugin/plugin.go | 3 +- server/plugin/sla_digest.go | 18 +++++-- server/plugin/sla_digest_test.go | 19 ++++--- server/plugin/utils.go | 41 ++++++++++++-- server/plugin/utils_test.go | 38 +++++++++++-- .../src/components/sidebar_buttons/index.js | 1 + .../sidebar_buttons/sidebar_buttons.jsx | 8 +-- .../sidebar_buttons/sidebar_buttons.test.jsx | 1 + .../components/sidebar_right/github_items.tsx | 6 +-- webapp/src/components/sidebar_right/index.jsx | 3 +- .../sidebar_right/sidebar_right.jsx | 2 + webapp/src/reducers/index.ts | 1 + webapp/src/selectors.ts | 2 + webapp/src/types/github_types.ts | 5 ++ webapp/src/utils/sla.test.ts | 45 ++++++++++++++++ webapp/src/utils/sla.ts | 54 +++++++++++++++---- 18 files changed, 237 insertions(+), 44 deletions(-) create mode 100644 webapp/src/utils/sla.test.ts diff --git a/plugin.json b/plugin.json index 20b30fe86..8fe766faf 100644 --- a/plugin.json +++ b/plugin.json @@ -140,7 +140,24 @@ "display_name": "PR review target (days):", "type": "number", "default": "0", - "help_text": "Optional. Number of calendar days until a review counts as due. The start time is when you were requested as a reviewer (recorded from GitHub pull_request review_requested webhooks to this server); if unknown, the PR open date is used. When greater than zero, /github todo shows due/overdue text and the sidebar review counter can be color-coded. Set to 0 to disable." + "help_text": "Optional. Number of days until a review counts as due (see PR review target day type). The start time is when you were requested as a reviewer (recorded from GitHub pull_request review_requested webhooks to this server); if unknown, the PR open date is used. When greater than zero, /github todo shows due/overdue text and the sidebar review counter can be color-coded. Set to 0 to disable." + }, + { + "key": "ReviewTargetDayType", + "display_name": "PR review target day type:", + "type": "dropdown", + "default": "calendar", + "help_text": "How PR review target (days) is counted when computing the due date. Calendar days count every day including weekends. Business days count Monday–Friday only (e.g. Friday + 2 business days = Tuesday). Overdue age shown in /github todo, sidebar badges, and digest buckets remains in calendar days either way.", + "options": [ + { + "display_name": "Calendar days", + "value": "calendar" + }, + { + "display_name": "Business days (Mon–Fri)", + "value": "business" + } + ] }, { "key": "OverdueReviewsChannelID", diff --git a/server/plugin/configuration.go b/server/plugin/configuration.go index 41fe7965d..ffcf57437 100644 --- a/server/plugin/configuration.go +++ b/server/plugin/configuration.go @@ -42,8 +42,11 @@ type Configuration struct { UsePreregisteredApplication bool `json:"usepreregisteredapplication"` ShowAuthorInCommitNotification bool `json:"showauthorincommitnotification"` GetNotificationForDraftPRs bool `json:"getnotificationfordraftprs"` - // ReviewTargetDays is the number of calendar days from PR open until a review is "due" (0 = SLA disabled). + // ReviewTargetDays is the number of days from the review-request start until a review is "due" (0 = SLA disabled). + // Day counting follows ReviewTargetDayType. ReviewTargetDays int `json:"reviewtargetdays"` + // ReviewTargetDayType is "calendar" (default) or "business" (Mon–Fri) for computing the due date. + ReviewTargetDayType string `json:"reviewtargetdaytype"` // 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. @@ -112,6 +115,7 @@ func (c *Configuration) sanitize() { if c.ReviewTargetDays < 0 { c.ReviewTargetDays = 0 } + c.ReviewTargetDayType = normalizeSLADayType(c.ReviewTargetDayType) // Trim spaces around org and OAuth credentials c.GitHubOrg = strings.TrimSpace(c.GitHubOrg) @@ -131,11 +135,16 @@ func (c *Configuration) IsSASS() bool { func (c *Configuration) ClientConfiguration() map[string]any { return map[string]any{ - "left_sidebar_enabled": c.EnableLeftSidebar, - "review_target_days": c.ReviewTargetDays, + "left_sidebar_enabled": c.EnableLeftSidebar, + "review_target_days": c.ReviewTargetDays, + "review_target_day_type": c.reviewTargetDayType(), } } +func (c *Configuration) reviewTargetDayType() string { + return normalizeSLADayType(c.ReviewTargetDayType) +} + // Clone shallow copies the configuration. Your implementation may require a deep copy if // your configuration has reference types. func (c *Configuration) Clone() *Configuration { diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 4b3e78bf7..079bfd172 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -1131,6 +1131,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient text.WriteString("##### Review Requests\n") targetDays := config.ReviewTargetDays + dayType := config.reviewTargetDayType() now := time.Now() if issueResults.GetTotal() == 0 { @@ -1141,7 +1142,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(prRefFromIssue(pr, baseURL), info.GitHubUsername) - if suffix, _ := reviewSLAMarkdown(slaStart, targetDays, now); suffix != "" { + if suffix, _ := reviewSLAMarkdown(slaStart, targetDays, now, dayType); suffix != "" { line += suffix } text.WriteString(line + "\n") diff --git a/server/plugin/sla_digest.go b/server/plugin/sla_digest.go index 9b52783fd..e80bc93c8 100644 --- a/server/plugin/sla_digest.go +++ b/server/plugin/sla_digest.go @@ -87,7 +87,7 @@ func (p *Plugin) maybePostDailyOverdueSLADigest(ctx context.Context) { return } - msg := clipSLADigestMessage(buildSLADigestMessage(entries, cfg.ReviewTargetDays)) + msg := clipSLADigestMessage(buildSLADigestMessage(entries, cfg.ReviewTargetDays, cfg.reviewTargetDayType())) post := &model.Post{ ChannelId: cfg.OverdueReviewsChannelID, UserId: p.BotUserID, @@ -195,6 +195,7 @@ func (p *Plugin) pickServiceGitHubUser(ctx context.Context) *GitHubUserInfo { func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context) ([]slaDigestEntry, bool) { config := p.getConfiguration() targetDays := config.ReviewTargetDays + dayType := config.reviewTargetDayType() orgList := config.getOrganizations() now := time.Now() @@ -242,7 +243,7 @@ func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context) ([]slaDigestEntr CreatedAt: github.Timestamp{Time: pr.CreatedAt}, } for _, rr := range gatherReviewersForPR(pr, resolveTeam) { - entry := p.evaluateOverdueForReviewer(ref, pr, rr, targetDays, now, seen, resolveSLAStart) + entry := p.evaluateOverdueForReviewer(ref, pr, rr, targetDays, dayType, now, seen, resolveSLAStart) if entry != nil { out = append(out, *entry) } @@ -449,6 +450,7 @@ func (p *Plugin) evaluateOverdueForReviewer( pr graphql.DigestPR, rr reviewerRequest, targetDays int, + dayType string, now time.Time, seen map[string]bool, resolveSLAStart func(prRef, reviewerRequest) github.Timestamp, @@ -463,7 +465,7 @@ func (p *Plugin) evaluateOverdueForReviewer( seen[dedupeKey] = true slaStart := resolveSLAStart(ref, rr) - diff := slaCalendarDiffDays(slaStart, targetDays, now) + diff := slaDiffDays(slaStart, targetDays, now, dayType) if diff >= 0 { return nil } @@ -542,7 +544,7 @@ func groupBucketEntriesByReviewer(entries []slaDigestEntry) []reviewerBucketGrou return out } -func buildSLADigestMessage(entries []slaDigestEntry, targetDays int) string { +func buildSLADigestMessage(entries []slaDigestEntry, targetDays int, dayType string) string { bucketEntries := make([][]slaDigestEntry, len(slaBuckets)) for _, e := range entries { idx := slaBucketIndex(e.DaysOverdue) @@ -555,7 +557,13 @@ func buildSLADigestMessage(entries []slaDigestEntry, targetDays int) string { var b strings.Builder if targetDays > 0 { unit := "days" - if targetDays == 1 { + if normalizeSLADayType(dayType) == slaDayTypeBusiness { + if targetDays == 1 { + unit = "business day" + } else { + unit = "business days" + } + } else if targetDays == 1 { unit = "day" } fmt.Fprintf(&b, "### Pull request reviews past SLA (target: %d %s from most recent review request)\n\n", targetDays, unit) diff --git a/server/plugin/sla_digest_test.go b/server/plugin/sla_digest_test.go index c45365a52..50c43cc3d 100644 --- a/server/plugin/sla_digest_test.go +++ b/server/plugin/sla_digest_test.go @@ -130,7 +130,7 @@ func TestBuildSLADigestMessage(t *testing.T) { entry(400, "@e (e-gh)", "owner/repo - [E](url)"), } - msg := buildSLADigestMessage(entries, 3) + msg := buildSLADigestMessage(entries, 3, slaDayTypeCalendar) assert.True(t, strings.HasPrefix(msg, "### Pull request reviews past SLA (target: 3 days from most recent review request)")) @@ -152,19 +152,24 @@ func TestBuildSLADigestMessage(t *testing.T) { entry(-2, "@skip2 (skip2-gh)", "owner/repo - [skipped-too](url)"), entry(1, "@keep (keep-gh)", "owner/repo - [kept](url)"), } - msg := buildSLADigestMessage(entries, 3) + msg := buildSLADigestMessage(entries, 3, slaDayTypeCalendar) assert.Contains(t, msg, "[kept]") assert.NotContains(t, msg, "[skipped]") assert.NotContains(t, msg, "[skipped-too]") }) t.Run("singular target days uses 'day'", func(t *testing.T) { - msg := buildSLADigestMessage([]slaDigestEntry{entry(1, "@x (x-gh)", "owner/repo - [X](url)")}, 1) + msg := buildSLADigestMessage([]slaDigestEntry{entry(1, "@x (x-gh)", "owner/repo - [X](url)")}, 1, slaDayTypeCalendar) assert.Contains(t, msg, "target: 1 day from") }) + t.Run("business day type uses business day wording", func(t *testing.T) { + msg := buildSLADigestMessage([]slaDigestEntry{entry(1, "@x (x-gh)", "owner/repo - [X](url)")}, 2, slaDayTypeBusiness) + assert.Contains(t, msg, "target: 2 business days from") + }) + t.Run("zero target days falls back to plain header", func(t *testing.T) { - msg := buildSLADigestMessage([]slaDigestEntry{entry(1, "@x (x-gh)", "owner/repo - [X](url)")}, 0) + msg := buildSLADigestMessage([]slaDigestEntry{entry(1, "@x (x-gh)", "owner/repo - [X](url)")}, 0, slaDayTypeCalendar) assert.True(t, strings.HasPrefix(msg, "### Pull request reviews past SLA\n")) }) @@ -174,7 +179,7 @@ func TestBuildSLADigestMessage(t *testing.T) { entry(2, "@alpha (alpha-gh)", "owner/repo - [PR-a](url)"), entry(2, "@Mu (mu-gh)", "owner/repo - [PR-m](url)"), } - msg := buildSLADigestMessage(entries, 3) + msg := buildSLADigestMessage(entries, 3, slaDayTypeCalendar) ai := strings.Index(msg, "@alpha") mi := strings.Index(msg, "@Mu") zi := strings.Index(msg, "@Zeta") @@ -188,7 +193,7 @@ func TestBuildSLADigestMessage(t *testing.T) { entry(2, reviewer, "owner/repo - [alpha-pr](https://example/pr/1)"), entry(2, reviewer, "owner/repo - [mu-pr](https://example/pr/2)"), } - msg := buildSLADigestMessage(entries, 3) + msg := buildSLADigestMessage(entries, 3, slaDayTypeCalendar) // 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. @@ -212,7 +217,7 @@ func TestBuildSLADigestMessage(t *testing.T) { entry(2, "@alice (alice-gh)", "o/r - [b-pr](url)"), entry(2, "@bob (bob-gh)", "o/r - [c-pr](url)"), } - msg := buildSLADigestMessage(entries, 3) + msg := buildSLADigestMessage(entries, 3, slaDayTypeCalendar) bucketStart := strings.Index(msg, "#### Overdue\n") require.True(t, bucketStart >= 0) bucket := msg[bucketStart:] diff --git a/server/plugin/utils.go b/server/plugin/utils.go index f1b3fd097..a6f6f01fb 100644 --- a/server/plugin/utils.go +++ b/server/plugin/utils.go @@ -367,15 +367,46 @@ func getToDoDisplayText(baseURL, title, url, notifType string, repository *githu return fmt.Sprintf("* %s %s %s\n", repoPart, notifType, titlePart) } -// slaCalendarDiffDays returns dueDate minus today in calendar days (negative when the review is overdue). -func slaCalendarDiffDays(createdAt github.Timestamp, targetDays int, now time.Time) int { +const ( + slaDayTypeCalendar = "calendar" + slaDayTypeBusiness = "business" +) + +func normalizeSLADayType(dayType string) string { + if strings.EqualFold(strings.TrimSpace(dayType), slaDayTypeBusiness) { + return slaDayTypeBusiness + } + return slaDayTypeCalendar +} + +// addBusinessDays returns start advanced by n weekdays (Mon–Fri). Weekends are skipped. +func addBusinessDays(start time.Time, n int) time.Time { + d := start + for remaining := n; remaining > 0; { + d = d.AddDate(0, 0, 1) + wd := d.Weekday() + if wd != time.Saturday && wd != time.Sunday { + remaining-- + } + } + return d +} + +// slaDiffDays returns dueDate minus today in calendar days (negative when the review is overdue). +// dayType controls how the due date is computed from targetDays; the returned difference is always calendar days. +func slaDiffDays(createdAt github.Timestamp, targetDays int, now time.Time, dayType string) int { if targetDays <= 0 || createdAt.IsZero() { return 0 } c := createdAt.UTC() createdDay := time.Date(c.Year(), c.Month(), c.Day(), 0, 0, 0, 0, time.UTC) - dueDay := createdDay.AddDate(0, 0, targetDays) + var dueDay time.Time + if normalizeSLADayType(dayType) == slaDayTypeBusiness { + dueDay = addBusinessDays(createdDay, targetDays) + } else { + dueDay = createdDay.AddDate(0, 0, targetDays) + } n := now.UTC() todayDay := time.Date(n.Year(), n.Month(), n.Day(), 0, 0, 0, 0, time.UTC) @@ -438,12 +469,12 @@ func escapeMarkdownLinkText(s string) string { } // reviewSLAMarkdown returns a Markdown SLA suffix for Mattermost posts and whether the review is overdue. -func reviewSLAMarkdown(createdAt github.Timestamp, targetDays int, now time.Time) (suffix string, overdue bool) { +func reviewSLAMarkdown(createdAt github.Timestamp, targetDays int, now time.Time, dayType string) (suffix string, overdue bool) { if targetDays <= 0 || createdAt.IsZero() { return "", false } - diffDays := slaCalendarDiffDays(createdAt, targetDays, now) + diffDays := slaDiffDays(createdAt, targetDays, now, dayType) if diffDays < 0 { overdueCount := -diffDays diff --git a/server/plugin/utils_test.go b/server/plugin/utils_test.go index c52a1f975..58b916c68 100644 --- a/server/plugin/utils_test.go +++ b/server/plugin/utils_test.go @@ -213,7 +213,7 @@ func TestReviewSLAMarkdown(t *testing.T) { ts := github.Timestamp{Time: created} t.Run("disabled when target is zero", func(t *testing.T) { - s, overdue := reviewSLAMarkdown(ts, 0, time.Date(2025, 3, 20, 0, 0, 0, 0, time.UTC)) + s, overdue := reviewSLAMarkdown(ts, 0, time.Date(2025, 3, 20, 0, 0, 0, 0, time.UTC), slaDayTypeCalendar) assert.Empty(t, s) assert.False(t, overdue) }) @@ -221,7 +221,7 @@ func TestReviewSLAMarkdown(t *testing.T) { t.Run("overdue", func(t *testing.T) { // Due March 15 (10th + 5), today March 19 -> 4 days overdue now := time.Date(2025, 3, 19, 12, 0, 0, 0, time.UTC) - s, overdue := reviewSLAMarkdown(ts, 5, now) + s, overdue := reviewSLAMarkdown(ts, 5, now, slaDayTypeCalendar) assert.True(t, overdue) assert.Contains(t, s, "4 days overdue") }) @@ -229,19 +229,49 @@ func TestReviewSLAMarkdown(t *testing.T) { t.Run("due in future", func(t *testing.T) { // Due March 20 (10th + 10), today March 18 -> 2 days now := time.Date(2025, 3, 18, 12, 0, 0, 0, time.UTC) - s, overdue := reviewSLAMarkdown(ts, 10, now) + s, overdue := reviewSLAMarkdown(ts, 10, now, slaDayTypeCalendar) assert.False(t, overdue) assert.Contains(t, s, "Due in 2 days") }) t.Run("due today", func(t *testing.T) { now := time.Date(2025, 3, 20, 23, 59, 0, 0, time.UTC) - s, overdue := reviewSLAMarkdown(ts, 10, now) + s, overdue := reviewSLAMarkdown(ts, 10, now, slaDayTypeCalendar) assert.False(t, overdue) assert.Contains(t, s, "Due today") }) } +func TestAddBusinessDays(t *testing.T) { + // Friday 2025-03-14 + fri := time.Date(2025, 3, 14, 0, 0, 0, 0, time.UTC) + assert.Equal(t, time.Date(2025, 3, 17, 0, 0, 0, 0, time.UTC), addBusinessDays(fri, 1)) // Mon + assert.Equal(t, time.Date(2025, 3, 18, 0, 0, 0, 0, time.UTC), addBusinessDays(fri, 2)) // Tue + assert.Equal(t, time.Date(2025, 3, 21, 0, 0, 0, 0, time.UTC), addBusinessDays(fri, 5)) // next Fri + + sat := time.Date(2025, 3, 15, 0, 0, 0, 0, time.UTC) + assert.Equal(t, time.Date(2025, 3, 17, 0, 0, 0, 0, time.UTC), addBusinessDays(sat, 1)) // Mon + + sun := time.Date(2025, 3, 16, 0, 0, 0, 0, time.UTC) + assert.Equal(t, time.Date(2025, 3, 17, 0, 0, 0, 0, time.UTC), addBusinessDays(sun, 1)) // Mon +} + +func TestSLADiffDaysBusiness(t *testing.T) { + // Friday evening request, 2 business-day target → due Tuesday. + fri := github.Timestamp{Time: time.Date(2025, 3, 14, 18, 0, 0, 0, time.UTC)} + mon := time.Date(2025, 3, 17, 12, 0, 0, 0, time.UTC) + assert.Equal(t, 1, slaDiffDays(fri, 2, mon, slaDayTypeBusiness), "Mon should be due in 1 calendar day") + + tue := time.Date(2025, 3, 18, 12, 0, 0, 0, time.UTC) + assert.Equal(t, 0, slaDiffDays(fri, 2, tue, slaDayTypeBusiness), "Tue should be due today") + + wed := time.Date(2025, 3, 19, 12, 0, 0, 0, time.UTC) + assert.Equal(t, -1, slaDiffDays(fri, 2, wed, slaDayTypeBusiness), "Wed should be 1 calendar day overdue") + + // Same start/target in calendar mode: Fri+2 = Sun → Mon is already overdue. + assert.Equal(t, -1, slaDiffDays(fri, 2, mon, slaDayTypeCalendar)) +} + func TestGetToDoDisplayText(t *testing.T) { type input struct { title string diff --git a/webapp/src/components/sidebar_buttons/index.js b/webapp/src/components/sidebar_buttons/index.js index fae00bb0b..bc607b3d7 100644 --- a/webapp/src/components/sidebar_buttons/index.js +++ b/webapp/src/components/sidebar_buttons/index.js @@ -22,6 +22,7 @@ function mapStateToProps(state) { enterpriseURL: state[`plugins-${pluginId}`].enterpriseURL, showRHSPlugin: state[`plugins-${pluginId}`].rhsPluginAction, reviewTargetDays: state[`plugins-${pluginId}`].configuration.review_target_days || 0, + reviewTargetDayType: state[`plugins-${pluginId}`].configuration.review_target_day_type || 'calendar', }; } diff --git a/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx b/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx index 3be8eed73..957199f02 100644 --- a/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx +++ b/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx @@ -17,6 +17,7 @@ export default class SidebarButtons extends React.PureComponent { enterpriseURL: PropTypes.string, reviews: PropTypes.arrayOf(PropTypes.object), reviewTargetDays: PropTypes.number, + reviewTargetDayType: PropTypes.string, unreads: PropTypes.arrayOf(PropTypes.object), yourPrs: PropTypes.arrayOf(PropTypes.object), yourAssignments: PropTypes.arrayOf(PropTypes.object), @@ -137,6 +138,7 @@ export default class SidebarButtons extends React.PureComponent { const reviews = this.props.reviews || []; const reviewTargetDays = this.props.reviewTargetDays || 0; + const reviewTargetDayType = this.props.reviewTargetDayType || 'calendar'; const yourPrs = this.props.yourPrs || []; const unreads = this.props.unreads || []; const yourAssignments = this.props.yourAssignments || []; @@ -178,7 +180,7 @@ export default class SidebarButtons extends React.PureComponent { > this.openRHS(RHSStates.REVIEWS)} - style={reviewButtonStyle(button, reviews, reviewTargetDays)} + style={reviewButtonStyle(button, reviews, reviewTargetDays, reviewTargetDayType)} > {' ' + reviews.length} @@ -228,7 +230,7 @@ export default class SidebarButtons extends React.PureComponent { } } -function reviewButtonStyle(base, reviews, targetDays) { +function reviewButtonStyle(base, reviews, targetDays, dayType) { // Match getReviewSLAStatus / reviewsHaveOverdue: a non-positive target means SLA // is not configured. !targetDays alone would let a negative value through and // produce a misleading green indicator. @@ -239,7 +241,7 @@ function reviewButtonStyle(base, reviews, targetDays) { if (list.length === 0) { return base; } - if (reviewsHaveOverdue(list, targetDays)) { + if (reviewsHaveOverdue(list, targetDays, dayType || 'calendar')) { return {...base, color: 'var(--dnd-indicator)'}; } return {...base, color: 'var(--online-indicator)'}; diff --git a/webapp/src/components/sidebar_buttons/sidebar_buttons.test.jsx b/webapp/src/components/sidebar_buttons/sidebar_buttons.test.jsx index 1ce0e746b..aab097ddd 100644 --- a/webapp/src/components/sidebar_buttons/sidebar_buttons.test.jsx +++ b/webapp/src/components/sidebar_buttons/sidebar_buttons.test.jsx @@ -25,6 +25,7 @@ const baseProps = { enterpriseURL: '', isTeamSidebar: false, reviewTargetDays: 0, + reviewTargetDayType: 'calendar', reviews: [], theme: { centerChannelBg: '#ffffff', diff --git a/webapp/src/components/sidebar_right/github_items.tsx b/webapp/src/components/sidebar_right/github_items.tsx index e38d33138..4f4e79184 100644 --- a/webapp/src/components/sidebar_right/github_items.tsx +++ b/webapp/src/components/sidebar_right/github_items.tsx @@ -141,7 +141,7 @@ function GithubItems(props: GithubItemsProps) { let slaBadge: JSX.Element | null = null; if (showReviewSLA) { - slaBadge = renderReviewSLABadge(item, props.reviewTargetDays || 0, style); + slaBadge = renderReviewSLABadge(item, props.reviewTargetDays || 0, props.reviewTargetDayType || 'calendar', style); } // Status images pasted directly from GitHub. Change to our own version when styles are decided. @@ -409,8 +409,8 @@ const getStyle = makeStyleFromTheme((theme) => { }; }); -function renderReviewSLABadge(item: GithubItem, targetDays: number, style: any): JSX.Element | null { - const status = getReviewSLAStatus(item, targetDays); +function renderReviewSLABadge(item: GithubItem, targetDays: number, dayType: 'calendar' | 'business', style: any): JSX.Element | null { + const status = getReviewSLAStatus(item, targetDays, dayType); if (!status) { return null; } diff --git a/webapp/src/components/sidebar_right/index.jsx b/webapp/src/components/sidebar_right/index.jsx index 7653aa5a7..481f0fdc5 100644 --- a/webapp/src/components/sidebar_right/index.jsx +++ b/webapp/src/components/sidebar_right/index.jsx @@ -11,7 +11,7 @@ import {getSidebarData} from 'src/selectors'; import SidebarRight from './sidebar_right.jsx'; function mapStateToProps(state) { - const {username, reviews, yourPrs, yourAssignments, unreads, enterpriseURL, orgs, rhsState, reviewTargetDays} = getSidebarData(state); + const {username, reviews, yourPrs, yourAssignments, unreads, enterpriseURL, orgs, rhsState, reviewTargetDays, reviewTargetDayType} = getSidebarData(state); return { username, reviews, @@ -22,6 +22,7 @@ function mapStateToProps(state) { orgs, rhsState, reviewTargetDays, + reviewTargetDayType, }; } diff --git a/webapp/src/components/sidebar_right/sidebar_right.jsx b/webapp/src/components/sidebar_right/sidebar_right.jsx index 0b2c0e632..8d2735fa9 100644 --- a/webapp/src/components/sidebar_right/sidebar_right.jsx +++ b/webapp/src/components/sidebar_right/sidebar_right.jsx @@ -98,6 +98,7 @@ export default class SidebarRight extends React.PureComponent { yourAssignments: PropTypes.arrayOf(PropTypes.object), rhsState: PropTypes.string, reviewTargetDays: PropTypes.number, + reviewTargetDayType: PropTypes.string, theme: PropTypes.object.isRequired, actions: PropTypes.shape({ getYourPrsDetails: PropTypes.func.isRequired, @@ -238,6 +239,7 @@ export default class SidebarRight extends React.PureComponent { theme={this.props.theme} showReviewSLA={rhsState === RHSStates.REVIEWS} reviewTargetDays={this.props.reviewTargetDays || 0} + reviewTargetDayType={this.props.reviewTargetDayType || 'calendar'} /> diff --git a/webapp/src/reducers/index.ts b/webapp/src/reducers/index.ts index 6cb4adb22..47326ed16 100644 --- a/webapp/src/reducers/index.ts +++ b/webapp/src/reducers/index.ts @@ -66,6 +66,7 @@ function userSettings(state = { function configuration(state: ConfigurationData = { left_sidebar_enabled: true, review_target_days: 0, + review_target_day_type: 'calendar', }, action: {type: string, data: ConnectedData | ConfigurationData}): ConfigurationData { switch (action.type) { case ActionTypes.RECEIVED_CONNECTED: diff --git a/webapp/src/selectors.ts b/webapp/src/selectors.ts index 2ecd0d3c6..55e66776a 100644 --- a/webapp/src/selectors.ts +++ b/webapp/src/selectors.ts @@ -7,6 +7,7 @@ import {createSelector} from 'reselect'; import {GlobalState, PluginState} from './types/store'; import {GithubIssueData, SidebarData, PrsDetailsData, UnreadsData} from './types/github_types'; +import {normalizeReviewTargetDayType} from './utils/sla'; const emptyArray: GithubIssueData[] | UnreadsData[] = []; @@ -64,6 +65,7 @@ export const getSidebarData = createSelector( orgs: organizations, rhsState, reviewTargetDays: pluginConfig.review_target_days || 0, + reviewTargetDayType: normalizeReviewTargetDayType(pluginConfig.review_target_day_type as string | undefined), }; }, ); diff --git a/webapp/src/types/github_types.ts b/webapp/src/types/github_types.ts index 31a874557..3ddd2eaa4 100644 --- a/webapp/src/types/github_types.ts +++ b/webapp/src/types/github_types.ts @@ -62,6 +62,9 @@ export type GithubItemsProps = { /** SLA target in days, used to compute the badge. Falsy disables the badge regardless of showReviewSLA. */ reviewTargetDays?: number; + + /** calendar (default) or business — how reviewTargetDays advances the due date. */ + reviewTargetDayType?: 'calendar' | 'business'; } export type UserSettingsData = { @@ -83,6 +86,7 @@ export type ConnectedData = { export type ConfigurationData = { left_sidebar_enabled: boolean; review_target_days?: number; + review_target_day_type?: 'calendar' | 'business'; } export type PrsDetailsData = { @@ -179,6 +183,7 @@ export type SidebarData = { orgs: string[], rhsState?: string | null, reviewTargetDays: number, + reviewTargetDayType: 'calendar' | 'business', } export type Organization = { diff --git a/webapp/src/utils/sla.test.ts b/webapp/src/utils/sla.test.ts new file mode 100644 index 000000000..9236d03a3 --- /dev/null +++ b/webapp/src/utils/sla.test.ts @@ -0,0 +1,45 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {addBusinessDaysUTC, getReviewSLAStatus, normalizeReviewTargetDayType} from './sla'; + +describe('normalizeReviewTargetDayType', () => { + it('defaults to calendar', () => { + expect(normalizeReviewTargetDayType()).toBe('calendar'); + expect(normalizeReviewTargetDayType('')).toBe('calendar'); + expect(normalizeReviewTargetDayType('other')).toBe('calendar'); + }); + + it('accepts business case-insensitively', () => { + expect(normalizeReviewTargetDayType('business')).toBe('business'); + expect(normalizeReviewTargetDayType('Business')).toBe('business'); + }); +}); + +describe('addBusinessDaysUTC', () => { + it('skips weekends from Friday', () => { + // 2025-03-14 is Friday + expect(addBusinessDaysUTC(2025, 2, 14, 1)).toBe(Date.UTC(2025, 2, 17)); // Mon + expect(addBusinessDaysUTC(2025, 2, 14, 2)).toBe(Date.UTC(2025, 2, 18)); // Tue + expect(addBusinessDaysUTC(2025, 2, 14, 5)).toBe(Date.UTC(2025, 2, 21)); // next Fri + }); + + it('starts from weekend and lands on Monday for +1', () => { + expect(addBusinessDaysUTC(2025, 2, 15, 1)).toBe(Date.UTC(2025, 2, 17)); // Sat + 1 + expect(addBusinessDaysUTC(2025, 2, 16, 1)).toBe(Date.UTC(2025, 2, 17)); // Sun + 1 + }); +}); + +describe('getReviewSLAStatus business days', () => { + const friItem = {created_at: '2025-03-14T18:00:00.000Z'}; + + it('Friday + 2 business days is still due later on Monday', () => { + const status = getReviewSLAStatus(friItem, 2, 'business', new Date('2025-03-17T12:00:00.000Z')); + expect(status).toEqual({daysFromDue: 1, overdue: false}); + }); + + it('Friday + 2 calendar days is overdue on Monday', () => { + const status = getReviewSLAStatus(friItem, 2, 'calendar', new Date('2025-03-17T12:00:00.000Z')); + expect(status).toEqual({daysFromDue: -1, overdue: true}); + }); +}); diff --git a/webapp/src/utils/sla.ts b/webapp/src/utils/sla.ts index 0e0fe322b..b5cf314d0 100644 --- a/webapp/src/utils/sla.ts +++ b/webapp/src/utils/sla.ts @@ -3,6 +3,8 @@ const MS_PER_DAY = 24 * 60 * 60 * 1000; +export type ReviewTargetDayType = 'calendar' | 'business'; + // daysFromDue is negative when overdue, 0 when due today, positive when in the future. export type ReviewSLAStatus = { daysFromDue: number; @@ -24,15 +26,43 @@ export function getReviewSLAStartIso(item: {review_sla_start?: string | null; cr return null; } +export function normalizeReviewTargetDayType(dayType?: string | null): ReviewTargetDayType { + if (typeof dayType === 'string' && dayType.trim().toLowerCase() === 'business') { + return 'business'; + } + return 'calendar'; +} + +/** Advance a UTC Y/M/D by n weekdays (Mon–Fri). Returns UTC midnight ms of the due day. */ +export function addBusinessDaysUTC(year: number, month: number, date: number, n: number): number { + let y = year; + let m = month; + let d = date; + let remaining = n; + while (remaining > 0) { + const next = new Date(Date.UTC(y, m, d + 1)); + y = next.getUTCFullYear(); + m = next.getUTCMonth(); + d = next.getUTCDate(); + const wd = next.getUTCDay(); // 0=Sun … 6=Sat + if (wd !== 0 && wd !== 6) { + remaining -= 1; + } + } + return Date.UTC(y, m, d); +} + /** * Computes the SLA status for a review item, or null when no useful answer is - * possible (no target configured, no start date, unparsable date). The "days" - * are calendar days computed against today's UTC date, matching the server's - * digest math. + * possible (no target configured, no start date, unparsable date). Due date + * uses calendar or business days per dayType; daysFromDue is always calendar + * days against today's UTC date, matching the server's digest math. */ export function getReviewSLAStatus( item: {review_sla_start?: string | null; created_at?: string | null}, targetDays: number, + dayType: ReviewTargetDayType = 'calendar', + now: Date = new Date(), ): ReviewSLAStatus | null { if (!targetDays || targetDays <= 0) { return null; @@ -48,13 +78,14 @@ export function getReviewSLAStatus( return null; } - const dueUTC = Date.UTC( - start.getUTCFullYear(), - start.getUTCMonth(), - start.getUTCDate() + targetDays, - ); - const today = new Date(); - const todayUTC = Date.UTC(today.getUTCFullYear(), today.getUTCMonth(), today.getUTCDate()); + const y = start.getUTCFullYear(); + const m = start.getUTCMonth(); + const d = start.getUTCDate(); + const dueUTC = normalizeReviewTargetDayType(dayType) === 'business' ? + addBusinessDaysUTC(y, m, d, targetDays) : + Date.UTC(y, m, d + targetDays); + + const todayUTC = Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate()); const daysFromDue = Math.round((dueUTC - todayUTC) / MS_PER_DAY); return { @@ -70,12 +101,13 @@ export function getReviewSLAStatus( export function reviewsHaveOverdue( reviews: Array<{review_sla_start?: string | null; created_at?: string | null}> | null | undefined, targetDays: number, + dayType: ReviewTargetDayType = 'calendar', ): boolean { if (!targetDays || !reviews || reviews.length === 0) { return false; } for (const pr of reviews) { - const status = getReviewSLAStatus(pr, targetDays); + const status = getReviewSLAStatus(pr, targetDays, dayType); if (status && status.overdue) { return true; } From 3b2a20ae1e40533d442c3363c55e34aabf1d20bf Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Tue, 4 Aug 2026 14:18:14 -0700 Subject: [PATCH 2/2] MM-70112: snapshot SLA day type for digest scan and header Reuse one config snapshot so overdue evaluation and the digest header cannot diverge if settings change mid-scan. --- server/plugin/sla_digest.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/plugin/sla_digest.go b/server/plugin/sla_digest.go index e80bc93c8..d6fde0eaa 100644 --- a/server/plugin/sla_digest.go +++ b/server/plugin/sla_digest.go @@ -45,6 +45,8 @@ func (p *Plugin) maybePostDailyOverdueSLADigest(ctx context.Context) { if cfg.OverdueReviewsChannelID == "" || cfg.ReviewTargetDays <= 0 { return } + targetDays := cfg.ReviewTargetDays + dayType := cfg.reviewTargetDayType() day := time.Now().In(time.Local).Format("2006-01-02") var marker []byte @@ -72,7 +74,7 @@ func (p *Plugin) maybePostDailyOverdueSLADigest(ctx context.Context) { return } - entries, ok := p.collectAllOverdueSLAItems(ctx) + entries, ok := p.collectAllOverdueSLAItems(ctx, targetDays, dayType) if !ok { // Distinguishes "digest could not complete a real scan" (config issue, no service user, // or every configured org's GraphQL fetch failed) from "scan ran and found nothing @@ -87,7 +89,7 @@ func (p *Plugin) maybePostDailyOverdueSLADigest(ctx context.Context) { return } - msg := clipSLADigestMessage(buildSLADigestMessage(entries, cfg.ReviewTargetDays, cfg.reviewTargetDayType())) + msg := clipSLADigestMessage(buildSLADigestMessage(entries, targetDays, dayType)) post := &model.Post{ ChannelId: cfg.OverdueReviewsChannelID, UserId: p.BotUserID, @@ -192,10 +194,8 @@ func (p *Plugin) pickServiceGitHubUser(ctx context.Context) *GitHubUserInfo { // configured org's GraphQL fetch failed); the caller should retry on the next scheduler tick // rather than treat that as "ran successfully and found nothing." A successful scan returns // ok=true even when entries is empty. -func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context) ([]slaDigestEntry, bool) { +func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context, targetDays int, dayType string) ([]slaDigestEntry, bool) { config := p.getConfiguration() - targetDays := config.ReviewTargetDays - dayType := config.reviewTargetDayType() orgList := config.getOrganizations() now := time.Now()