diff --git a/src/slack/commands.ts b/src/slack/commands.ts index 26a6974..b7657d3 100644 --- a/src/slack/commands.ts +++ b/src/slack/commands.ts @@ -24,6 +24,7 @@ import { screeningStatus } from "../domain/rules/screening.js"; import { log } from "../logger.js"; import { isSettingKey, + parseHandles, SETTING_KEYS, SETTINGS, setting, @@ -583,7 +584,7 @@ async function configText( ): Promise { const [verb, key, ...valueWords] = rest; - if (!verb) return configListing(); + if (!verb) return configListing(client); if (verb !== "set") { return ( @@ -615,11 +616,14 @@ async function configText( actorName: caller.name, }); + const now = await describeValue(client, key, cleaned.value); + const was = before.value + ? await describeValue(client, key, before.value) + : null; + const lines = [ - `*${SETTINGS[key].label}* is now \`${cleaned.value}\`` + - (before.value - ? ` (was \`${before.value}\`, from ${before.source})` - : "") + + `*${SETTINGS[key].label}* is now ${now}` + + (was ? ` (was ${was}, from ${before.source})` : "") + ".", ]; @@ -636,17 +640,49 @@ async function configText( return lines.join("\n"); } -function configListing(): string { - const rows = SETTING_KEYS.map((key) => { - const { value, source } = setting(key); - const where = - source === "slack" - ? "set here" - : source === "env" - ? `from ${SETTINGS[key].env}` - : "*not set*"; - return `• \`${key}\` — ${value ?? "—"} _(${where})_`; - }); +/** + * Renders a stored value the way a person wrote it. + * + * Channels are stored by id, deliberately — an id survives the channel being + * renamed, and a stored `#name` would quietly stop resolving the day somebody + * tidied it up. But `C0BPAV78LKZ` tells a reader nothing, so the id is what is + * kept and the name is what is shown. Falls back to the raw value if Slack + * cannot be asked: a settings listing that throws is worse than one that is + * briefly ugly. + */ +async function describeValue( + client: WebClient, + key: SettingKey, + value: string +): Promise { + if (SETTINGS[key].kind === "channel") { + try { + const info = await client.conversations.info({ channel: value }); + return info.channel?.name ? `#${info.channel.name}` : `\`${value}\``; + } catch { + return `\`${value}\``; + } + } + const handles = parseHandles(value); + return handles.length + ? handles.map((h) => `@${h}`).join(", ") + : `\`${value}\``; +} + +async function configListing(client: WebClient): Promise { + const rows = await Promise.all( + SETTING_KEYS.map(async (key) => { + const { value, source } = setting(key); + const where = + source === "slack" + ? "set here" + : source === "env" + ? `from ${SETTINGS[key].env}` + : "*not set*"; + const shown = value ? await describeValue(client, key, value) : "—"; + return `• \`${key}\` — ${shown} _(${where})_`; + }) + ); const unset = SETTING_KEYS.filter((k) => setting(k).source === "unset"); diff --git a/src/slack/userGroups.ts b/src/slack/userGroups.ts index 0a94367..f1a6eb4 100644 --- a/src/slack/userGroups.ts +++ b/src/slack/userGroups.ts @@ -8,16 +8,37 @@ export type ResolvedGroup = { members: Set; }; -/** Slack ids for user groups are `S` followed by uppercase alphanumerics. */ -const GROUP_ID = /^S[A-Z0-9]{4,}$/; - -export function isGroupId(ref: string): boolean { - return GROUP_ID.test(ref.toUpperCase()); +/** + * Whether a group is the one being asked for, by handle or by id. + * + * Matches on *either*, deliberately, rather than deciding up front which kind + * of reference it was handed. A previous version guessed with a pattern for + * Slack ids — `S` followed by alphanumerics — and `students` matches it: + * uppercased it is `STUDENTS`, which is an `S` and seven more characters. So + * the most important handle in this entire project was read as an opaque id, + * matched against nothing, and reported as a group that does not exist. The + * role sync said the same thing, which meant no student was being rostered at + * all. + * + * Handles and ids cannot realistically collide — a handle would have to be + * spelled exactly like some other group's id — so there is nothing to gain by + * telling them apart, and this cannot be wrong in the way guessing was. + */ +export function matchesGroup( + group: { id?: string; handle?: string }, + ref: string +): boolean { + const raw = ref.trim().replace(/^@/, ""); + if (!raw) return false; + return ( + (group.handle ?? "").toLowerCase() === raw.toLowerCase() || + (group.id ?? "") === raw.toUpperCase() + ); } /** * Resolves a user group by its @handle or its id. Handles are what people - * actually type and see, so they are what the config names; ids are opaque — + * actually type and see, so they are what the settings name; ids are opaque — * but an id is what Slack sends when a slash command has link escaping on and * somebody types `@students`, which arrives as ``. * @@ -28,15 +49,16 @@ export async function resolveGroup( client: WebClient, ref: string ): Promise { - const raw = ref.replace(/^@/, ""); - const wanted = raw.toLowerCase(); - const byId = isGroupId(raw) ? raw.toUpperCase() : null; + const wanted = ref.trim().replace(/^@/, ""); const list = await client.usergroups.list({ include_disabled: false }); - const group = (list.usergroups ?? []).find((g) => - byId ? g.id === byId : (g.handle ?? "").toLowerCase() === wanted - ); + const group = (list.usergroups ?? []).find((g) => matchesGroup(g, wanted)); if (!group?.id) { - log.warn("user group not found", { ref }); + // Naming what does exist turns "it says my group is missing" into a + // one-glance answer, which is how this bug should have been found. + log.warn("user group not found", { + ref, + available: (list.usergroups ?? []).map((g) => g.handle).filter(Boolean), + }); return null; } @@ -106,3 +128,12 @@ export async function setGroupMembership( users: userIds.join(","), }); } + +/** Every user group handle in the workspace, for "did you mean" messages. */ +export async function listGroupHandles(client: WebClient): Promise { + const list = await client.usergroups.list({ include_disabled: false }); + return (list.usergroups ?? []) + .map((g) => g.handle) + .filter((h): h is string => Boolean(h)) + .sort(); +} diff --git a/test/userGroups.test.ts b/test/userGroups.test.ts new file mode 100644 index 0000000..9532087 --- /dev/null +++ b/test/userGroups.test.ts @@ -0,0 +1,66 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { matchesGroup } from "../src/slack/userGroups.js"; + +const students = { id: "S0614TY5A", handle: "students" }; +const mentors = { id: "S07QQ2M1B", handle: "mentors" }; + +describe("finding a user group by handle or id", () => { + it("matches a plain handle", () => { + assert.equal(matchesGroup(students, "students"), true); + }); + + it("matches a handle written with @", () => { + assert.equal(matchesGroup(students, "@students"), true); + }); + + it("matches regardless of case", () => { + assert.equal(matchesGroup(students, "@Students"), true); + }); + + it("matches the id Slack sends in an escaped mention", () => { + assert.equal(matchesGroup(students, "S0614TY5A"), true); + }); + + it("does not match a different group", () => { + assert.equal(matchesGroup(mentors, "students"), false); + assert.equal(matchesGroup(students, "S07QQ2M1B"), false); + }); + + it("ignores surrounding whitespace", () => { + assert.equal(matchesGroup(students, " @students "), true); + }); + + it("does not match nothing", () => { + assert.equal(matchesGroup(students, ""), false); + assert.equal(matchesGroup(students, " "), false); + assert.equal(matchesGroup(students, "@"), false); + }); + + it("copes with a group Slack returned without a handle", () => { + assert.equal(matchesGroup({ id: "S1234ABCD" }, "students"), false); + assert.equal(matchesGroup({ handle: "students" }, "S1234ABCD"), false); + }); + + /** + * The regression, and it was as bad as it looks. Resolution used to decide up + * front whether a reference was a handle or an id, using a pattern for Slack + * ids: `S` followed by alphanumerics. `students` uppercased is `STUDENTS` — + * an `S` and seven more characters — so the single most important handle in + * this project was read as an opaque id, matched against no group, and + * reported as missing. + * + * That broke `/hawkmod config`, `/hawkmod group`, and the role sync, which + * meant no student was rostered and so none was monitored. Matching on either + * handle or id removes the guess that made it possible. + */ + it("matches handles that look like a Slack id", () => { + for (const handle of ["students", "staff", "seniors", "scouting"]) { + assert.equal( + matchesGroup({ id: "S0614TY5A", handle }, handle), + true, + `@${handle} must resolve by handle` + ); + } + }); +});