Public: anyone can read this; the crew can change it.
Every number on the forge counts a list its reader can open: close the remain… #12214
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/12213-every-number-on-the-forge-counts-a-list → main 3 commits fork 2 threads · 2 unresolved
3 files
changed +54
−4
src/core/pulls.ts +17 −3
| @@ -674,19 +674,33 @@ export function mergedPullsForRepos( | ||
| 674 | 674 | * Closed is not shipped. A pull closed without a merge has no `merged_at`, |
| 675 | 675 | * and `NULL >= anything` is `NULL` — never true, so it never counts; the |
| 676 | 676 | * `IS NOT NULL` says so to the reader as well as the planner. |
| 677 | * | |
| 678 | * A pull on a sealed report is the report's: `scope` (`sealedScope`) says | |
| 679 | * which sealed pulls this reader may be told landed. The default counts | |
| 680 | * them all, for the forge's own questions (has this app a past). | |
| 677 | 681 | */ |
| 678 | 682 | export function mergedPullCount( |
| 679 | 683 | db: Database, |
| 680 | 684 | repoId: string, |
| 681 | 685 | since: string, |
| 686 | scope: { sealed?: true; sealedBy?: string } = { sealed: true }, | |
| 682 | 687 | ): number { |
| 688 | const params: string[] = [repoId, since]; | |
| 689 | let seen = ""; | |
| 690 | if (scope.sealed !== true) { | |
| 691 | if (scope.sealedBy === undefined) seen = " AND issue.sealed = 0"; | |
| 692 | else { | |
| 693 | params.push(scope.sealedBy); | |
| 694 | seen = " AND (issue.sealed = 0 OR issue.author_id = ?3)"; | |
| 695 | } | |
| 696 | } | |
| 683 | 697 | const row = db |
| 684 | .query<{ n: number }, [string, string]>( | |
| 698 | .query<{ n: number }, string[]>( | |
| 685 | 699 | `SELECT COUNT(*) AS n FROM issue JOIN pull ON pull.issue_id = issue.id |
| 686 | 700 | WHERE issue.repo_id = ?1 |
| 687 | AND pull.merged_at IS NOT NULL AND pull.merged_at >= ?2`, | |
| 701 | AND pull.merged_at IS NOT NULL AND pull.merged_at >= ?2${seen}`, | |
| 688 | 702 | ) |
| 689 | .get(repoId, since); | |
| 703 | .get(...params); | |
| 690 | 704 | return row?.n ?? 0; |
| 691 | 705 | } |
| 692 | 706 |
src/web/studio/app.ts +3 −1
| @@ -294,7 +294,9 @@ export async function studioApp( | ||
| 294 | 294 | // cost per shipped change would read high off the short count. |
| 295 | 295 | const shipped = fresh |
| 296 | 296 | ? 0 |
| 297 | : timePhase(req, "shipped", () => mergedPullCount(db, repo.id, week)); | |
| 297 | : timePhase(req, "shipped", () => | |
| 298 | mergedPullCount(db, repo.id, week, sealedScope(db, repo, ctx.auth.user)), | |
| 299 | ); | |
| 298 | 300 | // The rest is spend, worked out only for a viewer who is going to see it — |
| 299 | 301 | // counted by the database over the whole week, not off a page of rows. |
| 300 | 302 | const weekTotals = ownerFacts && !fresh |
test/studio-shipping.test.ts +34
| @@ -2024,6 +2024,40 @@ describe("this week's shipping is counted by the database, not read off a page o | ||
| 2024 | 2024 | expect(mergedPullCount(app.db, "no-such-repo", week)).toBe(0); |
| 2025 | 2025 | }); |
| 2026 | 2026 | |
| 2027 | test("a merged pull on a sealed report is counted only for a reader who may see it", async () => { | |
outdated
This only exercises the SQL helper. It does not prove the studio page applies the viewer's scope, which is the behaviour the issue asked for. |
||
| 2028 | const week = new Date(Date.now() - 7 * 24 * 60 * 60_000).toISOString(); | |
| 2029 | const row = app.db | |
| 2030 | .query<{ id: string; author_id: string }, [string, string]>( | |
| 2031 | `SELECT issue.id, issue.author_id FROM issue JOIN pull ON pull.issue_id = issue.id | |
| 2032 | WHERE issue.repo_id = ?1 AND pull.merged_at >= ?2 LIMIT 1`, | |
| 2033 | ) | |
| 2034 | .get(busy.id, week)!; | |
| 2035 | app.db.query("UPDATE issue SET sealed = 1 WHERE id = ?1").run(row.id); | |
| 2036 | try { | |
| 2037 | expect(mergedPullCount(app.db, busy.id, week)).toBe(SHIPPED); | |
| 2038 | expect(mergedPullCount(app.db, busy.id, week, { sealed: true })).toBe(SHIPPED); | |
| 2039 | expect(mergedPullCount(app.db, busy.id, week, {})).toBe(SHIPPED - 1); | |
| 2040 | expect(mergedPullCount(app.db, busy.id, week, { sealedBy: "someone-else" })).toBe(SHIPPED - 1); | |
| 2041 | expect(mergedPullCount(app.db, busy.id, week, { sealedBy: row.author_id })).toBe(SHIPPED); | |
| 2042 | ||
| 2043 | // The page applies the viewer's scope: a stranger is not told the | |
| 2044 | // sealed pull landed, the writer is. | |
| 2045 | app.db.query("UPDATE repo SET visibility = 'public' WHERE id = ?1").run(busy.id); | |
| 2046 | const out = await ( | |
| 2047 | await fetch(`${base}/alice/busy`, { | |
| 2048 | headers: { cookie: "forge_mode=studio" }, | |
| 2049 | redirect: "manual", | |
| 2050 | }) | |
| 2051 | ).text(); | |
| 2052 | expect(out).toMatch(/Shipped<\/dt>\s*<dd>149 pull requests<\/dd>/); | |
| 2053 | const own = await (await get("/alice/busy")).text(); | |
| 2054 | expect(own).toMatch(/Shipped<\/dt>\s*<dd>150 pull requests<\/dd>/); | |
| 2055 | } finally { | |
| 2056 | app.db.query("UPDATE repo SET visibility = 'private' WHERE id = ?1").run(busy.id); | |
| 2057 | app.db.query("UPDATE issue SET sealed = 0 WHERE id = ?1").run(row.id); | |
| 2058 | } | |
| 2059 | }); | |
| 2060 | ||
| 2027 | 2061 | test("the app page says 150 changes shipped, not the 100 a page of closed pulls holds", async () => { |
| 2028 | 2062 | const page = await (await get("/alice/busy")).text(); |
| 2029 | 2063 | expect(page).toMatch(/Shipped<\/dt>\s*<dd>150 pull requests<\/dd>/); |
new above
offline
This argument is the fix and no test would fail without it. Add a page-level test (signed out reads 149, writer reads 150). Also wrap this line; it is far past the width of the lines around it.