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..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)) + msg := clipSLADigestMessage(buildSLADigestMessage(entries, targetDays, dayType)) post := &model.Post{ ChannelId: cfg.OverdueReviewsChannelID, UserId: p.BotUserID, @@ -192,9 +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 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; }