From 68e27ea1d0cada6e06d41fca20feb9bc44d3f3cc Mon Sep 17 00:00:00 2001 From: harshit-exe-dev Date: Tue, 27 Oct 2026 02:05:06 +0530 Subject: [PATCH] fix: edit merged PR shoutout when the program label lands late A merged PR whose program label is added after the merge used to keep a shoutout with no count, and the board only refreshed when the cache ran out. Handle the labeled event for an already-merged PR: count it the same way a merge does, refresh the board, and edit the stored shoutout so it reads like it had the label at the merge, including the qualify line. A shoutout that already has the count, an open PR, or any other label changes nothing. Closes #59 --- src/webhook.ts | 181 +++++++++++++++++++++++++++++++++---------- test/webhook.test.ts | 117 ++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+), 41 deletions(-) diff --git a/src/webhook.ts b/src/webhook.ts index bfa1d4a..019f412 100644 --- a/src/webhook.ts +++ b/src/webhook.ts @@ -1,5 +1,5 @@ // Handles GitHub webhook events: releases, starter issues and merged PRs. -import type { Config, Env } from "./config"; +import type { Config, Env, Program } from "./config"; import { DiscordError, editMessage, sendMessage } from "./discord"; import { countMergedPrs, getIssue, type GithubIssue, type StarterIssue } from "./github"; import { cacheLeaderboard, invalidateLeaderboard, recentMerges, rememberMerges, searchCountedPrs } from "./leaderboard"; @@ -30,6 +30,8 @@ interface IssuesEvent { interface PullRequestEvent { action: string; repository: Repository; + /** The label that was added, on labeled events. */ + label?: { name: string }; pull_request: { number: number; title: string; @@ -199,12 +201,78 @@ export async function resyncStarterIssues(env: Env, cfg: Config): Promise } } +/** The shoutout a merged PR got, so a late label can edit it instead of posting again. */ +interface ShoutoutRecord { + messageId: string; + /** True when the shoutout already carries a count line. */ + counted: boolean; + discordId?: string; + linking?: boolean; + newRoles?: string[]; +} + +const shoutoutKey = (repo: string, number: number) => `merged-pr:${repo}#${number}`; +/** A late label usually lands soon after the merge, so a few days is plenty. */ +const SHOUTOUT_TTL_SECONDS = 3 * 24 * 60 * 60; + +/** + * Counts a PR for a program and caches the board, the way a merge does. + * Returns undefined when the count couldn't be worked out; the cached board + * is cleared then, so a stale one is never served. + */ +async function countForProgram( + env: Env, + cfg: Config, + program: Program, + e: PullRequestEvent, + mergedAt: Date +): Promise[1] | undefined> { + const pr = e.pull_request; + try { + // GitHub's search index can lag a few seconds behind the merge, so add + // this event's PR, and the ones merged just before it that search doesn't + // have yet either, before counting and caching the leaderboard. + const login = pr.user.login.toLowerCase(); + const searched = await searchCountedPrs(env, cfg, program); + const indexed = new Set(searched.map((p) => p.url)); + const pending = [ + ...(await recentMerges(env, program)), + { + login: pr.user.login, + mergedAt: mergedAt.toISOString(), + url: pr.html_url, + title: pr.title, + repo: e.repository.name, + number: pr.number, + }, + ].filter((p, i, all) => !indexed.has(p.url) && all.findIndex((q) => q.url === p.url) === i); + await rememberMerges(env, program, pending); + const prs = [...searched, ...pending]; + await cacheLeaderboard(env, cfg, program, prs); + const others = prs.filter( + (p) => p.login.toLowerCase() === login && p.url !== pr.html_url && Date.parse(p.mergedAt) <= mergedAt.getTime() + ); + const count = others.length + 1; + return { + count, + label: program.title, + // Said once, on the merge that takes them to the minimum. + qualified: count === program.minPrs ? { topN: program.topN, reward: program.reward, note: program.note } : undefined, + }; + } catch { + // The shoutout matters more than the count; don't serve an old board. + await invalidateLeaderboard(env, program); + return undefined; + } +} + async function onPullRequest(e: PullRequestEvent, env: Env, cfg: Config): Promise { const pr = e.pull_request; - if (e.action !== "closed" || !pr.merged || !ours(cfg, e.repository)) return "ignored pull request"; + if (!ours(cfg, e.repository)) return "ignored pull request"; if (pr.user.type === "Bot" || cfg.excludedUsers.has(pr.user.login.toLowerCase())) return "ignored pull request author"; + if (e.action === "labeled") return onLateLabel(e, env, cfg); + if (e.action !== "closed" || !pr.merged) return "ignored pull request"; - let countInWindow: Parameters[1]; const mergedAt = pr.merged_at ? new Date(pr.merged_at) : new Date(); // The program this PR counts for, if any. const program = cfg.programs.find( @@ -214,43 +282,7 @@ async function onPullRequest(e: PullRequestEvent, env: Env, cfg: Config): Promis mergedAt >= p.start && mergedAt <= p.end ); - if (program) { - try { - // GitHub's search index can lag a few seconds behind the merge, so add - // this event's PR, and the ones merged just before it that search doesn't - // have yet either, before counting and caching the leaderboard. - const login = pr.user.login.toLowerCase(); - const searched = await searchCountedPrs(env, cfg, program); - const indexed = new Set(searched.map((p) => p.url)); - const pending = [ - ...(await recentMerges(env, program)), - { - login: pr.user.login, - mergedAt: mergedAt.toISOString(), - url: pr.html_url, - title: pr.title, - repo: e.repository.name, - number: pr.number, - }, - ].filter((p, i, all) => !indexed.has(p.url) && all.findIndex((q) => q.url === p.url) === i); - await rememberMerges(env, program, pending); - const prs = [...searched, ...pending]; - await cacheLeaderboard(env, cfg, program, prs); - const others = prs.filter( - (p) => p.login.toLowerCase() === login && p.url !== pr.html_url && Date.parse(p.mergedAt) <= mergedAt.getTime() - ); - const count = others.length + 1; - countInWindow = { - count, - label: program.title, - // Said once, on the merge that takes them to the minimum. - qualified: count === program.minPrs ? { topN: program.topN, reward: program.reward, note: program.note } : undefined, - }; - } catch { - // The shoutout matters more than the count; don't serve an old board. - await invalidateLeaderboard(env, program); - } - } + const countInWindow = program ? await countForProgram(env, cfg, program, e, mergedAt) : undefined; // If the author linked their GitHub account, show who they are here and // hand out any role this merge earned. The shoutout goes out either way. @@ -265,7 +297,7 @@ async function onPullRequest(e: PullRequestEvent, env: Env, cfg: Config): Promis } } - await sendMessage( + const messageId = await sendMessage( env, env.CHANNEL_MERGED_PRS, mergedPrMessage( @@ -282,5 +314,72 @@ async function onPullRequest(e: PullRequestEvent, env: Env, cfg: Config): Promis countInWindow ) ); + // Kept so a program label added after the merge can edit this shoutout + // instead of posting a second one. It expires on its own. + await env.STATE.put( + shoutoutKey(e.repository.name, pr.number), + JSON.stringify({ + messageId, + counted: countInWindow !== undefined, + discordId: link?.discordId, + linking: linkingEnabled(env), + newRoles, + } satisfies ShoutoutRecord), + { expirationTtl: SHOUTOUT_TTL_SECONDS } + ); return "posted merged pull request"; } + +/** + * A program label added after the merge, e.g. by the accepted-label workflow + * when a maintainer missed it. The PR already has its shoutout, so this edits + * that message to the text it would have had at the merge, and refreshes the + * board. A PR whose shoutout already has the count, or a label that isn't a + * program's, changes nothing. + */ +async function onLateLabel(e: PullRequestEvent, env: Env, cfg: Config): Promise { + const pr = e.pull_request; + if (!pr.merged || !pr.merged_at) return "ignored pull request"; + const added = e.label?.name; + if (!added) return "ignored pull request"; + const mergedAt = new Date(pr.merged_at); + const program = cfg.programs.find( + (p) => p.repos.includes(e.repository.name) && p.label === added && mergedAt >= p.start && mergedAt <= p.end + ); + if (!program) return "ignored pull request"; + + const record = await env.STATE.get(shoutoutKey(e.repository.name, pr.number), "json"); + if (record?.counted) return "ignored pull request"; + + const countInWindow = await countForProgram(env, cfg, program, e, mergedAt); + if (!countInWindow) return "late label, shoutout unchanged"; + // No stored shoutout (it predates this, or its record expired): the board + // is refreshed, but there's no message to edit, and guessing at one would + // risk editing the wrong message. + if (!record) return "late label, board refreshed"; + + await editMessage( + env, + env.CHANNEL_MERGED_PRS, + record.messageId, + mergedPrMessage( + { + repo: e.repository.name, + number: pr.number, + title: pr.title, + url: pr.html_url, + login: pr.user.login, + discordId: record.discordId, + linking: record.linking, + newRoles: record.newRoles, + }, + countInWindow + ) + ); + await env.STATE.put( + shoutoutKey(e.repository.name, pr.number), + JSON.stringify({ ...record, counted: true } satisfies ShoutoutRecord), + { expirationTtl: SHOUTOUT_TTL_SECONDS } + ); + return "updated merged pull request shoutout"; +} diff --git a/test/webhook.test.ts b/test/webhook.test.ts index 9248e0f..e2e9b97 100644 --- a/test/webhook.test.ts +++ b/test/webhook.test.ts @@ -609,6 +609,123 @@ describe("merged pull requests", () => { }); }); +describe("late program label on a merged pull request", () => { + const mergedPr = (over: Record = {}) => ({ + number: 139, + title: "fix(url-encoder): show an error", + html_url: "https://github.com/codercops/toolbelt/pull/139", + repository_url: "https://api.github.com/repos/codercops/toolbelt", + merged: true, + merged_at: "2026-10-11T16:20:00Z", + user: { login: "ada", type: "User" }, + labels: [{ name: "hacktoberfest-accepted" }], + ...over, + }); + const prEvent = (action: string, overPr: Record = {}, label?: string) => ({ + action, + repository, + ...(label ? { label: { name: label } } : {}), + pull_request: mergedPr(overPr), + }); + const earlier = (n: number) => ({ + total_count: n, + items: Array.from({ length: n }, (_, i) => ({ + html_url: `https://github.com/codercops/toolbelt/pull/${i + 1}`, + number: i + 1, + title: "Fix a thing", + repository_url: "https://api.github.com/repos/codercops/toolbelt", + user: { login: "ada" }, + pull_request: { merged_at: `2026-10-0${i + 1}T00:00:00Z` }, + })), + }); + // A merge that arrived without the program label, so its shoutout has no count yet. + // No search runs for it, so the mock's search answer doesn't matter here. + const postUncountedShoutout = async () => { + await handleGithubEvent("pull_request", prEvent("closed", { labels: [] }), env, cfg); + }; + const board = () => JSON.parse(env.STATE.store.get("leaderboard:v3:2026-10") ?? "{}"); + + it("edits the shoutout to the count and qualify lines, and refreshes the board", async () => { + const fetched = mockFetch(() => earlier(4)); + await postUncountedShoutout(); + expect(fetched.discord()).toHaveLength(1); + expect(fetched.discord()[0]?.body.embeds[0].description).not.toContain("That's their"); + fetched.clear(); + + expect(await handleGithubEvent("pull_request", prEvent("labeled", {}, "hacktoberfest-accepted"), env, cfg)).toBe( + "updated merged pull request shoutout" + ); + const edits = fetched.discord(); + expect(edits).toHaveLength(1); + expect(edits[0]?.method).toBe("PATCH"); + expect(edits[0]?.url).toBe("https://discord.com/api/v10/channels/chan-merged/messages/msg-1"); + const description = edits[0]?.body.embeds[0].description as string; + expect(description).toContain("That's their 5th merged PR on the Hacktoberfest 2026 leaderboard."); + expect(description).toContain("With 5 they now qualify"); + expect(board().rows).toEqual([expect.objectContaining({ login: "ada", count: 5 })]); + }); + + it("edits in the count without the qualify line when the minimum isn't reached", async () => { + const fetched = mockFetch(() => earlier(1)); + await postUncountedShoutout(); + fetched.clear(); + await handleGithubEvent("pull_request", prEvent("labeled", {}, "hacktoberfest-accepted"), env, cfg); + const description = fetched.discord()[0]?.body.embeds[0].description as string; + expect(description).toContain("That's their 2nd merged PR"); + expect(description).not.toContain("they now qualify"); + expect(board().rows).toEqual([expect.objectContaining({ login: "ada", count: 2 })]); + }); + + it("doesn't count twice when search already includes this PR", async () => { + const fetched = mockFetch(() => ({ + total_count: 5, + items: [ + ...earlier(4).items, + { html_url: "https://github.com/codercops/toolbelt/pull/139", number: 139, title: "fix(url-encoder): show an error", repository_url: "https://api.github.com/repos/codercops/toolbelt", user: { login: "ada" }, pull_request: { merged_at: "2026-10-11T16:20:00Z" } }, + ], + })); + await postUncountedShoutout(); + fetched.clear(); + await handleGithubEvent("pull_request", prEvent("labeled", {}, "hacktoberfest-accepted"), env, cfg); + expect(fetched.discord()[0]?.body.embeds[0].description).toContain("That's their 5th merged PR"); + expect(board().rows).toEqual([expect.objectContaining({ login: "ada", count: 5 })]); + }); + + it("changes nothing for an open PR, another label, or a shoutout that already has the count", async () => { + const fetched = mockFetch(() => earlier(0)); + expect(await handleGithubEvent("pull_request", prEvent("labeled", { merged: false, merged_at: null }, "hacktoberfest-accepted"), env, cfg)).toBe("ignored pull request"); + expect(await handleGithubEvent("pull_request", prEvent("labeled", { labels: [{ name: "bug" }] }, "bug"), env, cfg)).toBe("ignored pull request"); + expect(await handleGithubEvent("pull_request", prEvent("labeled", { merged_at: "2026-11-02T00:00:00Z" }, "hacktoberfest-accepted"), env, cfg)).toBe("ignored pull request"); + expect(fetched.discord()).toHaveLength(0); + + // This merge had the label all along, so its shoutout already has the count. + await handleGithubEvent("pull_request", prEvent("closed"), env, cfg); + fetched.clear(); + expect(await handleGithubEvent("pull_request", prEvent("labeled", {}, "hacktoberfest-accepted"), env, cfg)).toBe("ignored pull request"); + expect(fetched.discord()).toHaveLength(0); + }); + + it("refreshes the board but doesn't edit when the shoutout record is gone", async () => { + const fetched = mockFetch(() => earlier(4)); + await postUncountedShoutout(); + await env.STATE.delete("merged-pr:toolbelt#139"); + fetched.clear(); + expect(await handleGithubEvent("pull_request", prEvent("labeled", {}, "hacktoberfest-accepted"), env, cfg)).toBe("late label, board refreshed"); + expect(fetched.discord()).toHaveLength(0); + expect(board().rows).toEqual([expect.objectContaining({ login: "ada", count: 5 })]); + }); + + it("leaves the shoutout alone and clears the board when search fails", async () => { + mockFetch(() => earlier(4)); + await postUncountedShoutout(); + await env.STATE.put("leaderboard:v3:2026-10", JSON.stringify({ rows: [], generatedAt: "earlier" })); + const fetched = mockFetch(() => new Response("unavailable", { status: 503 })); + expect(await handleGithubEvent("pull_request", prEvent("labeled", {}, "hacktoberfest-accepted"), env, cfg)).toBe("late label, shoutout unchanged"); + expect(fetched.discord()).toHaveLength(0); + expect(env.STATE.store.has("leaderboard:v3:2026-10")).toBe(false); + }); +}); + const rateLimit = () => new Response(JSON.stringify({ retry_after: 30 }), { status: 429 }); const onGithub = (over: Record = {}) => () => ({ ...issueEvent("edited").issue, ...over });