trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns13g rEventsFlows

A row the owner ranked is marked as his and no planner or keeper re-ranks it #11909

scoutcrew opened this

Check · Review · Land · Live — Its check is queued. Next: reviewer, no review yet.

ConversationFiles
Ready to land

scout/forge:scout/11753-a-row-the-owner-ranked-is-marked-as-his → main 3 commits fork

Review

Sign in to review this change.
6 files changed +135 −5
src/actions/next.ts +1 −1
@@ -79,7 +79,7 @@ export function rankIssue(
7979 return ok(undefined);
8080 }
8181 lift(app.db, repo, issue.id);
82 writeOrder(app.db, repo, order, actor.id);
82 writeOrder(app.db, repo, order, actor.id, undefined, { owner: actor.kind === "user", moved: issue.id });
8383 return ok(undefined);
8484 }).immediate();
8585 if (!moved.ok) return moved;
src/core/migration-pins/20261112200000-issue-rank-owner.pin added +1
@@ -0,0 +1,1 @@
1 a92d3610f168e7df22524a88d60ac9c065a29b7ba8496754432b3281cd299d8e
src/core/migrations/20261112200000-issue-rank-owner.ts added +6
@@ -0,0 +1,6 @@
1 // Who ranked (#11753): 1 when a person placed the row, 0 when the planner or
2 // a keeper did. An owner's row stays where it is until a person moves it.
3 // Additive; existing rows were all placed by a person's move.
4 export default `
5 ALTER TABLE issue_rank ADD COLUMN by_owner INTEGER NOT NULL DEFAULT 1;
6 `;
src/core/rank.ts +38 −2
@@ -33,9 +33,26 @@ export function clearRank(db: Database, issueId: string): void {
3333 db.query("DELETE FROM issue_rank WHERE issue_id = ?1").run(issueId);
3434 }
3535
36 /** The open issues in one repository whose place a person chose. */
37 export function ownerRanked(db: Database, repoId: string): Set<string> {
38 return new Set(
39 db
40 .query<{ issue_id: string }, [string]>(
41 `SELECT r.issue_id FROM issue_rank r JOIN issue i ON i.id = r.issue_id
42 WHERE i.repo_id = ?1 AND i.state = 'open' AND r.by_owner = 1`,
43 )
44 .all(repoId)
45 .map((r) => r.issue_id),
46 );
47 }
48
3649 /**
3750 * Make `order` the whole ranking of a repository: places 1..n in that order,
3851 * every other rank dropped. The caller's transaction makes it one write.
52 * `by` says who is ranking: a person (`owner: true`) marks the rows it moves
53 * (`moved`, else all) as the owner's; the planner or a keeper (`owner: false`)
54 * leaves every owner-ranked row at its place and orders only the rest around
55 * them. A row's mark survives a write that does not move it.
3956 */
4057 export function writeOrder(
4158 db: Database,
@@ -43,14 +60,33 @@ export function writeOrder(
4360 order: readonly string[],
4461 actorId: string,
4562 at = new Date().toISOString(),
63 by: { readonly owner: boolean; readonly moved?: string } = { owner: true },
4664 ): void {
65 const before = db
66 .query<{ issue_id: string; pos: number; by_owner: number }, [string]>(
67 `SELECT r.issue_id, r.pos, r.by_owner FROM issue_rank r JOIN issue i ON i.id = r.issue_id WHERE i.repo_id = ?1`,
68 )
69 .all(repo.id);
70 const owned = new Map(before.filter((r) => r.by_owner === 1).map((r) => [r.issue_id, r.pos]));
71 let placed: string[];
72 if (by.owner) placed = [...order];
73 else {
74 // Pinned rows keep their place; the others fill the free places in order.
75 const free = order.filter((id) => !owned.has(id));
76 const pinned = [...owned].filter(([id]) => order.includes(id)).sort((x, y) => x[1] - y[1]);
77 placed = [...free];
78 for (const [id, pos] of pinned) placed.splice(Math.min(pos - 1, placed.length), 0, id);
79 }
4780 db.query(
4881 "DELETE FROM issue_rank WHERE issue_id IN (SELECT id FROM issue WHERE repo_id = ?1)",
4982 ).run(repo.id);
5083 const put = db.query(
51 "INSERT INTO issue_rank (issue_id, pos, actor_id, at) VALUES (?1, ?2, ?3, ?4)",
84 "INSERT INTO issue_rank (issue_id, pos, actor_id, at, by_owner) VALUES (?1, ?2, ?3, ?4, ?5)",
5285 );
53 order.forEach((id, n) => put.run(id, n + 1, actorId, at));
86 placed.forEach((id, n) => {
87 const mark = by.owner ? (by.moved === undefined || id === by.moved || owned.has(id) ? 1 : 0) : owned.has(id) ? 1 : 0;
88 put.run(id, n + 1, actorId, at, mark);
89 });
5490 }
5591
5692 /**
src/web/pages/next.ts +6 −2
@@ -6,6 +6,7 @@
66 * phone need no script; a row can also be dragged, or moved by the key shown
77 * beside each action (`j`/`k` choose the row), which post the same form.
88 */
9 import { ownerRanked } from "../../core/rank.ts";
910 import { placeOf, rankIssue, rankedWords, stepTo, type Place } from "../../actions/next.ts";
1011 import { nextUp, TOP, type NextRow } from "../../core/next_up.ts";
1112 import { openPullStages } from "../../flow/read.ts";
@@ -37,6 +38,7 @@ export const NEXT_CSS = `[data-next] { overflow-wrap: anywhere; }
3738 [data-next] .list > li.has-after > .row { flex: 1 1 14rem; }
3839 [data-next] .row-after { display: flex; flex-wrap: wrap; gap: 6px; max-width: 100%; padding: 0 14px 8px; }
3940 [data-next] summary h2 { display: inline; margin: 0; }
41 [data-next] .mine { font-weight: 600; }
4042 [data-next] .cut-line { overflow-wrap: anywhere; }`;
4143
4244 export const KEYS_JS = `
@@ -151,6 +153,7 @@ function line(
151153 cost: number | undefined = undefined,
152154 position: number | null = null,
153155 state: string | null = null,
156 mine = false,
154157 ): Html {
155158 const r = rows[at]!;
156159 const n = r.issue.number;
@@ -169,7 +172,7 @@ function line(
169172 title: r.issue.title,
170173 titleAttr: r.issue.title,
171174 lead: html`${position === null ? "·" : String(position)}`,
172 meta: html`${r.role === "blocker" ? "Blocker" : "Enabler"} · #${n}${state === null ? "" : ` · ${state} · ${r.onIt ?? "next"}`} · ${r.roleWhy}${away ? null : html` · <span class="estimate">${rangeWords(priced.find((p) => p.number === n)?.estimate ?? { known: false, samples: 0, basis: "no estimate" })}</span>`}${state !== null || r.onIt === null ? "" : ` · on it: ${r.onIt}`}${r.why === null ? "" : ` · ${r.why}`}`,
175 meta: html`${mine ? html`<span class="mine" title="Placed by a person; the planner leaves it here">Owner's pick</span> · ` : null}${r.role === "blocker" ? "Blocker" : "Enabler"} · #${n}${state === null ? "" : ` · ${state} · ${r.onIt ?? "next"}`} · ${r.roleWhy}${away ? null : html` · <span class="estimate">${rangeWords(priced.find((p) => p.number === n)?.estimate ?? { known: false, samples: 0, basis: "no estimate" })}</span>`}${state !== null || r.onIt === null ? "" : ` · on it: ${r.onIt}`}${r.why === null ? "" : ` · ${r.why}`}`,
173176 nav: true,
174177 data: { number: String(n), ...(cost === undefined ? {} : { cost: String(cost) }) },
175178 ...(controls === null ? {} : { after: controls }),
@@ -192,6 +195,7 @@ export function nextPage(ctx: Ctx): Response {
192195 const scope = sealedScope(ctx.app.db, repo, ctx.auth.user);
193196 const next = nextUp(ctx.app.db, repo, scope, (ready) => notStartable(ctx.app, repo, ready, scope));
194197 const rows = next.rows;
198 const mine = ownerRanked(ctx.app.db, repo.id);
195199 const back = `${base}/next`;
196200 const rendering = performance.now();
197201 const { priced, cut, capacity, sentence } = weekView(ctx.app.db, repo.id, rows, new Date());
@@ -207,7 +211,7 @@ export function nextPage(ctx: Ctx): Response {
207211 ${list.flatMap((_, i) => {
208212 const at = start + i;
209213 const mark = at === cut.fits.length && cut.unsure === null && id !== undefined && i > 0 ? [cutMark(cut, false)] : [];
210 return [...mark, line(base, rows, at, writer, back, false, priced, costs?.[at], at + 1, stateWord(rows[at]!, facts(rows[at]!)))];
214 return [...mark, line(base, rows, at, writer, back, false, priced, costs?.[at], at + 1, stateWord(rows[at]!, facts(rows[at]!)), mine.has(rows[at]!.issue.id))];
211215 })}
212216 </ul></details>`;
213217 const body = html`${repoHeader(ctx, repo, "issues")}
test/next-owner-rank.test.ts added +83
@@ -0,0 +1,83 @@
1 /** A row a person ranked is the owner's: marked on /next, left alone by automatic ranking (#11753). */
2 import { afterAll, beforeAll, expect, test } from "bun:test";
3 import { mkdtemp, rm } from "node:fs/promises";
4 import { tmpdir } from "node:os";
5 import { join } from "node:path";
6 import { createApp, type App } from "../src/app.ts";
7 import { openIssue } from "../src/actions/issues.ts";
8 import { rankIssue } from "../src/actions/next.ts";
9 import { createRepo } from "../src/actions/repos.ts";
10 import { ownerRanked, ranksInRepo, writeOrder } from "../src/core/rank.ts";
11 import type { Repo } from "../src/core/repos.ts";
12 import { createSession } from "../src/core/sessions.ts";
13 import { createUser, type User } from "../src/core/users.ts";
14 import { serve, type Servers } from "../src/http/server.ts";
15
16 let dir: string;
17 let app: App;
18 let servers: Servers;
19 let cookie: string;
20 let alice: User;
21 let repo: Repo;
22 const ids: string[] = [];
23 const nums: number[] = [];
24
25 beforeAll(async () => {
26 dir = await mkdtemp(join(tmpdir(), "forge-owner-rank-"));
27 app = createApp({ dataDir: dir, port: 0 });
28 servers = await serve(app);
29 const made = await createUser(app.db, { origin: "seed", trusted: true, username: "alice", email: "[email protected]", password: "a-good-passphrase", adminIfFirst: true });
30 if (!made.ok) throw new Error(made.error);
31 alice = made.value;
32 cookie = `forge_session=${createSession(app.db, alice.id).token}`;
33 const r = await createRepo(app, alice, { name: "own", visibility: "public" });
34 if (!r.ok) throw new Error(JSON.stringify(r.error));
35 repo = r.value;
36 for (let i = 1; i <= 4; i++) {
37 const o = openIssue(app, alice, repo, { title: `Issue ${i}` });
38 if (!o.ok) throw new Error(String(o.error));
39 ids.push(o.value.id);
40 nums.push(o.value.number);
41 }
42 });
43
44 afterAll(async () => {
45 await servers.stop();
46 app.close();
47 await rm(dir, { recursive: true, force: true });
48 });
49
50 const order = () => [...ranksInRepo(app.db, repo.id)].sort((a, b) => a[1] - b[1]).map(([id]) => id);
51
52 test("a person's move is stored as the owner's, and survives a planner pass", () => {
53 expect(rankIssue(app, alice, repo, nums[2]!, { to: "top" }).ok).toBe(true);
54 expect(ownerRanked(app.db, repo.id)).toEqual(new Set([ids[2]!]));
55 // A planner pass over the same issues, trying to put the owner's row last.
56 app.db.transaction(() => {
57 writeOrder(app.db, repo, [ids[0]!, ids[1]!, ids[2]!], "planner", undefined, { owner: false });
58 })();
59 expect(order()).toEqual([ids[2]!, ids[0]!, ids[1]!]);
60 expect(ownerRanked(app.db, repo.id)).toEqual(new Set([ids[2]!]));
61 });
62
63 test("a planner's own rank is still re-ranked", () => {
64 app.db.transaction(() => {
65 writeOrder(app.db, repo, [ids[1]!, ids[0]!, ids[2]!], "planner", undefined, { owner: false });
66 })();
67 expect(order()).toEqual([ids[2]!, ids[1]!, ids[0]!]);
68 expect(ownerRanked(app.db, repo.id).has(ids[1]!)).toBe(false);
69 });
70
71 test("a later person's move of another row does not make the planner's rows the owner's", () => {
72 expect(rankIssue(app, alice, repo, nums[3]!, { to: "top" }).ok).toBe(true);
73 expect(ownerRanked(app.db, repo.id)).toEqual(new Set([ids[2]!, ids[3]!]));
74 expect(order()[0]).toBe(ids[3]!);
75 });
76
77 test("only the owner's rows wear the mark", async () => {
78 const page = await (await fetch(`${servers.url}/alice/own/next`, { headers: { cookie } })).text();
79 const rows = page.split("<li").filter((c) => /data-number="\d+"/.test(c));
80 const marked = rows.filter((c) => c.includes('class="mine"')).map((c) => Number(/data-number="(\d+)"/.exec(c)![1]));
81 expect(marked.sort()).toEqual([nums[2]!, nums[3]!].sort());
82 expect(rows.length).toBeGreaterThan(marked.length);
83 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs