Add the scheduled ReviewSLA job: comments, reminders, digest, dashboard

AssignReviewers/assign.js

@@ -11,6 +11,7 @@
 // - A PR that already has a domain approver and a second assignee only gets the review requests
 //   topped up, so reruns are harmless and hand-picked assignees are kept.
 
+const { pickLeastLoaded, teamMembers } = require('../ReviewConfig/pick.js');
 const { awaySoon } = require('../ReviewConfig/pto.js');
 
 function isBot(user) {
@@ -49,64 +50,6 @@ async function inheritedAssignees(github, owner, repo, pr, author, { maxDepth, p
   return null;
 }
 
-async function teamMembers(github, org, team) {
-  const members = await github.paginate(github.rest.teams.listMembersInOrg, { org, team_slug: team, per_page: 100 });
-  return members.map((m) => m.login);
-}
-
-// For each login: how many open, ready PRs in the org already have them as assignee.
-async function reviewerLoad(github, org, logins, core) {
-  if (logins.length === 0) return {};
-  try {
-    return await queryLoad(github, org, logins);
-  } catch (error) {
-    core.warning(`Couldn't read reviewer load (${error.message}); picking without it.`);
-    return Object.fromEntries(logins.map((login) => [login, 0]));
-  }
-}
-
-// Load is counted from live PR data rather than GitHub search, whose index lags new assignments
-// by minutes: several PRs opened close together would otherwise all see stale counts. The query
-// covers the org's most recently pushed repositories, which is where open PRs live.
-async function queryLoad(github, org, logins) {
-  const prFields = 'pageInfo { hasNextPage endCursor } nodes { isDraft assignees(first: 10) { nodes { login } } }';
-  const data = await github.graphql(
-    `query($org: String!) {
-      organization(login: $org) {
-        repositories(first: 50, orderBy: { field: PUSHED_AT, direction: DESC }) {
-          nodes { name pullRequests(states: OPEN, first: 100) { ${prFields} } }
-        }
-      }
-    }`,
-    { org },
-  );
-  const load = Object.fromEntries(logins.map((login) => [login, 0]));
-  const count = (pullRequests) => {
-    for (const pullRequest of pullRequests?.nodes ?? []) {
-      if (pullRequest.isDraft) continue;
-      for (const assignee of pullRequest.assignees?.nodes ?? []) {
-        if (assignee.login in load) load[assignee.login]++;
-      }
-    }
-  };
-  for (const repository of data.organization?.repositories?.nodes ?? []) {
-    let page = repository.pullRequests;
-    count(page);
-    // Busy repos (StaflLib) have more than one page of open PRs.
-    while (page?.pageInfo?.hasNextPage) {
-      const next = await github.graphql(
-        `query($org: String!, $name: String!, $after: String!) {
-          repository(owner: $org, name: $name) { pullRequests(states: OPEN, first: 100, after: $after) { ${prFields} } }
-        }`,
-        { org, name: repository.name, after: page.pageInfo.endCursor },
-      );
-      page = next.repository?.pullRequests;
-      count(page);
-    }
-  }
-  return load;
-}
-
 module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Promise((r) => setTimeout(r, ms)) }) => {
   const { owner, repo } = context.repo;
   const pr = context.payload.pull_request;
@@ -146,18 +89,12 @@ module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Pro
     // 2. Fill the domain approver, then the rotation reviewer, least-loaded first.
     let away;
     const pick = async (team, members) => {
-      const candidates = members.filter((login) => login !== author && !named.has(login)).sort();
+      const candidates = members.filter((login) => login !== author && !named.has(login));
       if (candidates.length === 0) return null;
       away ??= await awaySoon({ url: inputs.ptoCalendarUrl, people: inputs.people, core, ...pto });
-      const load = await reviewerLoad(github, owner, candidates, core);
-      const available = candidates.filter((login) => !away.has(login));
-      const pool = available.length > 0 ? available : candidates;
-      // Least loaded wins; ties rotate by PR number so they don't always go to the same person.
-      const least = Math.min(...pool.map((login) => load[login]));
-      const tied = pool.filter((login) => load[login] === least);
-      const chosen = tied[pr.number % tied.length];
-      notes.push(`picked ${chosen} from ${team} (${least} open assigned PRs)`);
-      return chosen;
+      const chosen = await pickLeastLoaded({ github, org: owner, core, candidates, away, seed: pr.number });
+      notes.push(`picked ${chosen.login} from ${team} (${chosen.load} open assigned PRs)`);
+      return chosen.login;
     };
     if (!hasDomainApprover()) {
       const chosen = await pick(domainTeam, domainMembers);

ReviewConfig/README.md

@@ -27,6 +27,7 @@ Whoever adds or removes someone from the review teams updates it at the same tim
 | `holidays.json` | Company paid holidays, by year | By hand, each year, from the holiday calendar People Operations publishes. Add next year's before Jan 1 |
 | `pto.js` | Reads the Rippling PTO calendar feed | — |
 | `hours.js` | Business hours: how many lie between two times, and when a target falls due | — |
+| `pick.js` | The least-loaded pick of a reviewer from a team | — |
 
 ## Who is out
 

ReviewConfig/hours.js

@@ -60,4 +60,4 @@ function addBusinessHours(start, hours, holidays) {
   }
 }
 
-module.exports = { addBusinessHours, businessHoursBetween, isBusinessDay, pacificTime };
+module.exports = { addBusinessHours, businessHoursBetween, isBusinessDay, pacificOffset, pacificTime };

ReviewConfig/pick.js

@@ -0,0 +1,76 @@
+// Picking a reviewer: the least-loaded available member of a team. Shared by AssignReviewers,
+// which picks a new PR's assignees, and ReviewSLA, which replaces an assignee who is out.
+
+async function teamMembers(github, org, team) {
+  const members = await github.paginate(github.rest.teams.listMembersInOrg, { org, team_slug: team, per_page: 100 });
+  return members.map((m) => m.login);
+}
+
+// For each login: how many open, ready PRs in the org already have them as assignee.
+async function reviewerLoad(github, org, logins, core) {
+  if (logins.length === 0) return {};
+  try {
+    return await queryLoad(github, org, logins);
+  } catch (error) {
+    core.warning(`Couldn't read reviewer load (${error.message}); picking without it.`);
+    return Object.fromEntries(logins.map((login) => [login, 0]));
+  }
+}
+
+// Load is counted from live PR data rather than GitHub search, whose index lags new assignments
+// by minutes: several PRs opened close together would otherwise all see stale counts. The query
+// covers the org's most recently pushed repositories, which is where open PRs live.
+async function queryLoad(github, org, logins) {
+  const prFields = 'pageInfo { hasNextPage endCursor } nodes { isDraft assignees(first: 10) { nodes { login } } }';
+  const data = await github.graphql(
+    `query($org: String!) {
+      organization(login: $org) {
+        repositories(first: 50, orderBy: { field: PUSHED_AT, direction: DESC }) {
+          nodes { name pullRequests(states: OPEN, first: 100) { ${prFields} } }
+        }
+      }
+    }`,
+    { org },
+  );
+  const load = Object.fromEntries(logins.map((login) => [login, 0]));
+  const count = (pullRequests) => {
+    for (const pullRequest of pullRequests?.nodes ?? []) {
+      if (pullRequest.isDraft) continue;
+      for (const assignee of pullRequest.assignees?.nodes ?? []) {
+        if (assignee.login in load) load[assignee.login]++;
+      }
+    }
+  };
+  for (const repository of data.organization?.repositories?.nodes ?? []) {
+    let page = repository.pullRequests;
+    count(page);
+    // Busy repos (StaflLib) have more than one page of open PRs.
+    while (page?.pageInfo?.hasNextPage) {
+      const next = await github.graphql(
+        `query($org: String!, $name: String!, $after: String!) {
+          repository(owner: $org, name: $name) { pullRequests(states: OPEN, first: 100, after: $after) { ${prFields} } }
+        }`,
+        { org, name: repository.name, after: page.pageInfo.endCursor },
+      );
+      page = next.repository?.pullRequests;
+      count(page);
+    }
+  }
+  return load;
+}
+
+// The least-loaded candidate who isn't away, or the least-loaded of everyone if they all are.
+// Ties rotate by `seed` (the PR number) so they don't always go to the same person.
+// Returns { login, load } or null when there are no candidates.
+async function pickLeastLoaded({ github, org, core, candidates, away, seed }) {
+  if (candidates.length === 0) return null;
+  const sorted = [...candidates].sort();
+  const load = await reviewerLoad(github, org, sorted, core);
+  const available = sorted.filter((login) => !away.has(login));
+  const pool = available.length > 0 ? available : sorted;
+  const least = Math.min(...pool.map((login) => load[login]));
+  const tied = pool.filter((login) => load[login] === least);
+  return { login: tied[seed % tied.length], load: least };
+}
+
+module.exports = { pickLeastLoaded, reviewerLoad, teamMembers };

ReviewConfig/pto.js

@@ -125,46 +125,35 @@ function outDates(icsText, people) {
   return out;
 }
 
-// The logins who are out today or on the next business day (Pacific): a review assigned now is
-// due within about one business day, so someone out tomorrow shouldn't get it today.
+// For each login in the people map, the Pacific dates the PTO calendar has them out, or null with a
+// warning when the feed or the map can't be read. With no URL configured, it's null with a note.
 //
 // `people` is the people map, as an object or as the JSON text of the REVIEW_PEOPLE org variable:
 // { "<github login>": { "name": "<name as Rippling shows it>", "aliases": ["<other name>"],
 //                        "slack": "<Slack member ID>" } }, with aliases optional.
-//
-// Returns an empty set, with a warning, when the feed or the map can't be read: picking someone who
-// is out is better than not assigning at all. With no URL configured, it does nothing.
-async function awaySoon({
-  url,
-  core,
-  people,
-  now = new Date(),
-  fetch = globalThis.fetch,
-  holidays = readHolidays(),
-}) {
+async function readOutDates({ url, core, people, now = new Date(), fetch = globalThis.fetch, holidays = readHolidays() }) {
   if (!url) {
     core.info('No PTO calendar configured; not checking who is out.');
-    return new Set();
+    return null;
   }
   try {
     people = typeof people === 'string' ? JSON.parse(people || '{}') : people || {};
   } catch {
     core.warning("The REVIEW_PEOPLE org variable isn't valid JSON; not checking who is out.");
-    return new Set();
+    return null;
   }
   if (Object.keys(people).length === 0) {
     core.warning('The REVIEW_PEOPLE org variable is empty or not passed in; not checking who is out.');
-    return new Set();
+    return null;
   }
   const unnamed = Object.entries(people).filter(([, person]) => namesOf(person).length === 0).map(([login]) => login);
   if (unnamed.length > 0) {
     core.warning(`No Rippling name in REVIEW_PEOPLE for ${unnamed.join(', ')}, so their PTO isn't checked.`);
   }
-  const today = pacificDate(now);
-  if (!holidaysFor(holidays, today.slice(0, 4))) {
-    core.warning(`ReviewConfig/holidays.json has no holidays for ${today.slice(0, 4)}; every weekday counts as a business day.`);
+  const year = pacificDate(now).slice(0, 4);
+  if (!holidaysFor(holidays, year)) {
+    core.warning(`ReviewConfig/holidays.json has no holidays for ${year}; every weekday counts as a business day.`);
   }
-  const window = [today, nextBusinessDay(today, holidays)];
   let text;
   try {
     // The URL carries a token, so neither it nor an error that might quote it is logged.
@@ -173,11 +162,24 @@ async function awaySoon({
     text = await response.text();
   } catch (error) {
     core.warning(`Couldn't read the PTO calendar (${/^HTTP \d+$/.test(error.message) ? error.message : error.name}); not checking who is out.`);
-    return new Set();
+    return null;
   }
-  const dates = outDates(text, people);
+  return outDates(text, people);
+}
+
+// The logins who are out today or on the next business day (Pacific): a review assigned now is
+// due within about one business day, so someone out tomorrow shouldn't get it today.
+//
+// Returns an empty set when the feed or the map can't be read: picking someone who is out is
+// better than not assigning at all.
+async function awaySoon(options) {
+  const { now = new Date(), holidays = readHolidays() } = options;
+  const dates = await readOutDates({ ...options, now, holidays });
+  if (!dates) return new Set();
+  const today = pacificDate(now);
+  const window = [today, nextBusinessDay(today, holidays)];
   const away = new Set(Object.keys(dates).filter((login) => window.some((d) => dates[login].has(d))));
-  core.info(`Out ${window.join(' or ')}: ${[...away].join(', ') || 'nobody'}.`);
+  options.core.info(`Out ${window.join(' or ')}: ${[...away].join(', ') || 'nobody'}.`);
   return away;
 }
 
@@ -192,4 +194,5 @@ module.exports = {
   pacificDate,
   parseIcs,
   readHolidays,
+  readOutDates,
 };