diff --git a/ReviewSLA/README.md b/ReviewSLA/README.md index 2d9507e..6b04ebc 100644 --- a/ReviewSLA/README.md +++ b/ReviewSLA/README.md @@ -45,7 +45,7 @@ Bot PRs and Graphite merge-queue PRs have no clocks. and late scheduled runs don't send them twice. The comment is written before the DM, so a failed write means a missed reminder, not a repeated one. - **Reminder DMs** (`remind`): one per clock, when it's due, to the assignee's Slack ID in - `REVIEW_PEOPLE`. + `REVIEW_PEOPLE`, linking to the PR in Graphite. - **Out-of-office reassignment** (`remind`): an assignee the PTO calendar has out on any day from today to the day their review is due is replaced on every PR in the stack they're assigned to and haven't approved. Their approvals stand. @@ -54,10 +54,12 @@ Bot PRs and Graphite merge-queue PRs have no clocks. kept, and a comment on the lowest of those PRs says who took over and why. In a public repo it says only that they aren't available, since anyone can read it. - **The digest:** posted to the `REVIEW_SLA_SLACK_CHANNEL` channel by the 10:00 Pacific run - (or a manual run with **digest** ticked): overdue reviews by person, and how many more are due - today. + (or a manual run with **digest** ticked): the overdue reviews and the ones due today, by person, + each linked to the PR in Graphite, and how many more are owed but due later. - **The dashboard:** a Confluence page with a checkmark per person while they're meeting the SLA, - and their overdue reviews when they aren't. It saves a new version only when the content + and the number overdue when they aren't. Next to it, every review they owe, linked to the PR in + Graphite, with when it's due, overdue ones in bold. PR titles are left out so a busy row stays + short; the link leads to the PR. It saves a new version only when the content changes, as a minor edit, so watchers aren't notified every 30 minutes. It fails safe: an API error is a warning, and that PR, repo, channel or page is skipped this run. diff --git a/ReviewSLA/comment.js b/ReviewSLA/comment.js index 643d5cc..d337441 100644 --- a/ReviewSLA/comment.js +++ b/ReviewSLA/comment.js @@ -27,6 +27,13 @@ function formatTime(instant) { return `${parts.weekday} ${parts.month} ${parts.day}, ${parts.hour}:${parts.minute}`; } +// A PR's page in Graphite, where the team reviews stacks: github.com///pull/ becomes +// app.graphite.com/github/pr///. +function graphiteUrl(pr) { + const match = /^https:\/\/github\.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/.exec(pr.url ?? ''); + return match ? `https://app.graphite.com/github/pr/${match[1]}/${match[2]}/${match[3]}` : pr.url; +} + // One key per clock, so a reminder is sent once per clock: a new clock (a re-review, or a restart // after the PR goes back to draft) has a new start and gets its own. const reminderKey = (clock) => `${clock.login}:${clock.kind}:${clock.start.toISOString()}`; @@ -60,4 +67,4 @@ function renderComment(clocks, state, { dashboardUrl } = {}) { return lines.join('\n'); } -module.exports = { formatTime, KIND, MARKER, parseState, reminderKey, renderComment }; +module.exports = { formatTime, graphiteUrl, KIND, MARKER, parseState, reminderKey, renderComment }; diff --git a/ReviewSLA/confluence.js b/ReviewSLA/confluence.js index 63938f2..9979650 100644 --- a/ReviewSLA/confluence.js +++ b/ReviewSLA/confluence.js @@ -1,12 +1,12 @@ // The Review SLA dashboard: a Confluence page under Embedded Team Dashboard, rewritten each run. -// It shows a checkmark per person while they're meeting the SLA, and lists overdue reviews only for -// those who aren't. A new version is saved only when the content changes, as a minor edit, so +// It shows a checkmark per person while they're meeting the SLA, and the number overdue when they +// aren't, then every review each person owes, linked to Graphite, with when it's due. A new version is saved only when the content changes, as a minor edit, so // neither the page history nor watchers' notifications fill up every 30 minutes. The version // message carries a hash of the content, which is what "changed" is checked against: Confluence // rewrites storage format on save, so the page's own body can't be compared. const crypto = require('node:crypto'); -const { formatTime, KIND } = require('./comment.js'); +const { formatTime, graphiteUrl, KIND } = require('./comment.js'); const { SMALL_PR_LINES, TARGET_HOURS } = require('./clock.js'); const html = (text) => String(text).replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"'); @@ -15,19 +15,22 @@ function renderDashboard(clocks, { people }) { const nameOf = (login) => people[login]?.name || login; const logins = [...new Set([...Object.keys(people), ...clocks.map((c) => c.login)])].sort((a, b) => nameOf(a).localeCompare(nameOf(b))); const rows = logins.map((login) => { - const mine = clocks.filter((c) => c.login === login); - const overdue = mine.filter((c) => c.overdue).sort((a, b) => a.due - b.due); - const status = overdue.length === 0 ? '✅' : `${overdue.length} overdue`; - const list = overdue.length === 0 ? '' : ``; - return `${html(nameOf(login))}${status}${mine.length}${list}`; + const mine = clocks.filter((c) => c.login === login).sort((a, b) => a.due - b.due); + const overdue = mine.filter((c) => c.overdue).length; + const status = overdue === 0 ? '✅' : `${overdue} overdue`; + const item = (c) => { + const when = c.overdue ? `overdue since ${html(formatTime(c.due))}` : `due ${html(formatTime(c.due))}`; + // No title: the link leads to it, and titles make a busy row too tall to scan. + return `
  • ${html(`${c.pr.repo}#${c.pr.number}`)}: ${KIND[c.kind].toLowerCase()}, ${when}
    • `; + }; + const list = mine.length === 0 ? '' : `
      ${mine.map(item).join('')}
    `; + return `${html(nameOf(login))}${status}${list}`; }); return [ '

    Kept up to date by the Review SLA job every 30 minutes during business hours (10:00 to 17:00 Pacific, business days). ' + `An assignee owes a first response within ${TARGET_HOURS.small} business hours on a PR under ${SMALL_PR_LINES} added lines, ` + `${TARGET_HOURS.standard} otherwise, and a re-review within ${TARGET_HOURS.reReview}. Times are Pacific.

    `, - '', + '
    AssigneeStatusReviews owedOverdue
    ', ...rows, '
    AssigneeStatusReviews owed
    ', ].join(''); diff --git a/ReviewSLA/run.test.js b/ReviewSLA/run.test.js index dcadcfa..4d09ffa 100644 --- a/ReviewSLA/run.test.js +++ b/ReviewSLA/run.test.js @@ -169,7 +169,7 @@ test('remind DMs each overdue assignee once, however many runs see it', async () assert.deepStrictEqual(w.calls.dms, []); await go(w, { now: '2026-10-02T00:00:00Z' }); assert.deepStrictEqual(w.calls.dms.map((m) => m.channel).sort(), ['U000', 'U002']); - assert.match(w.calls.dms[0].text, /^Review due: \. As an assignee, your first response was due at Thu Oct 1, 17:00 PT\.$/); + assert.match(w.calls.dms[0].text, /^Review due: \. As an assignee, your first response was due at Thu Oct 1, 17:00 PT\.$/); await go(w, { now: '2026-10-02T17:30:00Z' }); assert.strictEqual(w.calls.dms.length, 2); // One comment, edited in place. @@ -248,11 +248,52 @@ test('the digest posts only on the 10:00 Pacific run', async () => { await go(w, { now: '2026-10-02T17:05:00Z', inputs: { schedule: '0 17 * * 1-5' } }); assert.strictEqual(w.calls.posts.length, 1); assert.strictEqual(w.calls.posts[0].channel, 'C123'); - assert.match(w.calls.posts[0].text, /^\*Review SLA, Fri Oct 2\*\n2 overdue reviews:\n• Person ghopper: /); + assert.strictEqual( + w.calls.posts[0].text, + [ + '*Review SLA, Fri Oct 2*', + '*Overdue (2)*', + '• Person ghopper: , first response due Thu Oct 1, 17:00', + '• Person kjohnson: , first response due Thu Oct 1, 17:00', + '', + ].join('\n'), + ); assert.ok(run.isDigestRun({ eventName: 'schedule', schedule: '0 18 * * 1-5' }, new Date('2026-12-01T18:00:00Z'))); assert.ok(run.isDigestRun({ digest: 'true', eventName: 'workflow_dispatch' }, new Date('2026-12-01T20:00:00Z'))); }); +test('the digest also lists reviews due today, and counts the ones due later', async () => { + // PR 1 opened 10:00 Thursday is due 17:00 Thursday; PR 2, opened 16:00 Thursday, isn't due until Friday. + const second = { number: 2, head: 'feature/b', assignees: ['mhamilton'], timeline: [{ __typename: 'AssignedEvent', createdAt: '2026-10-01T23:00:00Z', assignee: userNode('mhamilton') }] }; + const w = world({ prs: [onePr(), second] }); + await go(w, { now: '2026-10-01T23:30:00Z', inputs: { digest: 'true' } }); + assert.strictEqual( + w.calls.posts[0].text, + [ + '*Review SLA, Thu Oct 1*', + '*Due today (2)*', + '• Person ghopper: , first response due 17:00', + '• Person kjohnson: , first response due 17:00', + '1 more review is owed, due after today.', + '', + ].join('\n'), + ); +}); + +test('the dashboard lists every review owed, linked to Graphite, with overdue ones marked', async () => { + const second = { number: 2, head: 'feature/b', assignees: ['ghopper'], timeline: [{ __typename: 'AssignedEvent', createdAt: '2026-10-01T23:00:00Z', assignee: userNode('ghopper') }] }; + const w = world({ prs: [onePr(), second] }); + await go(w, { now: '2026-10-02T00:00:00Z' }); + const page = w.calls.puts.at(-1).body.storage.value; + assert.ok(page.includes( + 'Person ghopper1 overdue
      ' + + '
    • StaflLib#1: first response, overdue since Thu Oct 1, 17:00
      • ' + + '
    • StaflLib#2: first response, due Fri Oct 2, 16:00
      • ' + + '
    ', + ), page); + assert.ok(page.includes('Person dvaughan✅')); +}); + test('the dashboard saves a new version only when its content changes, as a minor edit', async () => { const w = world({ prs: [onePr()] }); await go(w, { now: '2026-10-01T18:00:00Z' }); diff --git a/ReviewSLA/slack.js b/ReviewSLA/slack.js index 2777de1..58434bd 100644 --- a/ReviewSLA/slack.js +++ b/ReviewSLA/slack.js @@ -2,7 +2,7 @@ // is chat:write, and the bot has to be invited to the digest channel. const { pacificDate } = require('../ReviewConfig/pto.js'); -const { formatTime, KIND } = require('./comment.js'); +const { formatTime, graphiteUrl, KIND } = require('./comment.js'); async function postMessage({ token, channel, text, fetch = globalThis.fetch }) { const response = await fetch('https://slack.com/api/chat.postMessage', { @@ -16,24 +16,32 @@ async function postMessage({ token, channel, text, fetch = globalThis.fetch }) { // Slack's mrkdwn wants &, < and > escaped in text. const escape = (text) => text.replace(/&/g, '&').replace(//g, '>'); -const prLink = (pr) => `<${pr.url}|${pr.repo}#${pr.number}: ${escape(pr.title)}>`; +const prLink = (pr) => `<${graphiteUrl(pr)}|${pr.repo}#${pr.number}: ${escape(pr.title)}>`; function reminderText(clock) { const what = clock.kind === 'first' ? 'your first response' : 'your re-review'; return `Review due: ${prLink(clock.pr)}. As an assignee, ${what} was due at ${formatTime(clock.due)} PT.`; } +// Overdue reviews and those due today, by person, each linked to Graphite, then a count of the rest. function digestText(clocks, { now, people, dashboardUrl }) { const nameOf = (login) => people[login]?.name || login; - const overdue = clocks.filter((c) => c.overdue).sort((a, b) => nameOf(a.login).localeCompare(nameOf(b.login)) || a.due - b.due); - const dueToday = clocks.filter((c) => !c.overdue && pacificDate(c.due) === pacificDate(now)); + const byPerson = (a, b) => nameOf(a.login).localeCompare(nameOf(b.login)) || a.due - b.due; + const overdue = clocks.filter((c) => c.overdue).sort(byPerson); + const dueToday = clocks.filter((c) => !c.overdue && pacificDate(c.due) === pacificDate(now)).sort(byPerson); + const later = clocks.length - overdue.length - dueToday.length; + const line = (c, when) => `• ${escape(nameOf(c.login))}: ${prLink(c.pr)}, ${KIND[c.kind].toLowerCase()} due ${when}`; const lines = [`*Review SLA, ${formatTime(now).split(',')[0]}*`]; - if (overdue.length === 0) lines.push('No overdue reviews.'); - else { - lines.push(`${overdue.length} overdue ${overdue.length === 1 ? 'review' : 'reviews'}:`); - for (const c of overdue) lines.push(`• ${escape(nameOf(c.login))}: ${prLink(c.pr)} (${KIND[c.kind].toLowerCase()}, due ${formatTime(c.due)})`); + if (overdue.length === 0 && dueToday.length === 0) lines.push('No reviews overdue or due today.'); + if (overdue.length > 0) { + lines.push(`*Overdue (${overdue.length})*`); + for (const c of overdue) lines.push(line(c, formatTime(c.due))); } - if (dueToday.length > 0) lines.push(`${dueToday.length} more due today.`); + if (dueToday.length > 0) { + lines.push(`*Due today (${dueToday.length})*`); + for (const c of dueToday) lines.push(line(c, formatTime(c.due).split(', ')[1])); + } + if (later > 0) lines.push(`${later} more ${later === 1 ? 'review is' : 'reviews are'} owed, due after today.`); if (dashboardUrl) lines.push(`<${dashboardUrl}|Dashboard>`); return lines.join('\n'); }