diff --git a/packages/junior-dashboard/src/client/review-desk/ReviewItemActions.tsx b/packages/junior-dashboard/src/client/review-desk/ReviewItemActions.tsx index ddd1282b09..56d76dc117 100644 --- a/packages/junior-dashboard/src/client/review-desk/ReviewItemActions.tsx +++ b/packages/junior-dashboard/src/client/review-desk/ReviewItemActions.tsx @@ -102,7 +102,7 @@ export function ReviewItemActions(props: { item: ReviewItemPayload }) { } } - // Take and Dismiss are quiet, so the answer or Done stays the main action. + // Take and Dismiss are quiet, so the answer stays the main action. const secondaryActions = ( <> {item.canTake ? ( @@ -149,15 +149,6 @@ export function ReviewItemActions(props: { item: ReviewItemPayload }) { ) : (
- {item.kind === "notice" ? ( - - ) : null}
{secondaryActions}
diff --git a/packages/junior-dashboard/src/client/review-desk/queries.ts b/packages/junior-dashboard/src/client/review-desk/queries.ts index 315d43a8e9..fa0c7f4d09 100644 --- a/packages/junior-dashboard/src/client/review-desk/queries.ts +++ b/packages/junior-dashboard/src/client/review-desk/queries.ts @@ -137,7 +137,7 @@ export function useBlockedAutomationsData(enabled: boolean) { }); } -/** Take, submit, complete, or dismiss one Review item. */ +/** Take, submit, or dismiss one Review item. */ export function useReviewItemAction() { const queryClient = useQueryClient(); return useMutation({ diff --git a/packages/junior-github/src/webhooks/pull-request-ready.ts b/packages/junior-github/src/webhooks/pull-request-ready.ts index 560442728d..38490b62ed 100644 --- a/packages/junior-github/src/webhooks/pull-request-ready.ts +++ b/packages/junior-github/src/webhooks/pull-request-ready.ts @@ -137,7 +137,7 @@ async function syncOne( if (ready) { await items.upsert({ assigneeIdentityIds: [opener.identityId], - body: `**${state.title}** is approved, and GitHub says that it can merge. Merge it, or say why not: ${state.url}`, + body: `**${state.title}** is approved, and GitHub says that it can merge: ${state.url}`, dedupeKey, subject: { id: state.url, title: name, url: state.url }, title: `Ready to merge: ${name}`, diff --git a/packages/junior-github/tests/pull-request-ready.test.ts b/packages/junior-github/tests/pull-request-ready.test.ts index 4f1aba01d3..0450a0d20f 100644 --- a/packages/junior-github/tests/pull-request-ready.test.ts +++ b/packages/junior-github/tests/pull-request-ready.test.ts @@ -300,7 +300,7 @@ describe("ready to merge notice", () => { kind: "upsert", notice: { assigneeIdentityIds: ["identity-requester"], - body: `**Add the review desk notice** is approved, and GitHub says that it can merge. Merge it, or say why not: ${PULL_URL}`, + body: `**Add the review desk notice** is approved, and GitHub says that it can merge: ${PULL_URL}`, dedupeKey: DEDUPE_KEY, subject: { id: PULL_URL, diff --git a/packages/junior/src/api/schema/review-item.ts b/packages/junior/src/api/schema/review-item.ts index 65e782dab5..8dac4531ec 100644 --- a/packages/junior/src/api/schema/review-item.ts +++ b/packages/junior/src/api/schema/review-item.ts @@ -28,7 +28,7 @@ export const reviewItemSchema = z assignees: z.array(personSchema), automationId: z.string().min(1).optional(), body: z.string(), - /** True when the viewer can take, submit, complete, or dismiss it. */ + /** True when the viewer can take, submit, or dismiss it. */ canAct: z.boolean(), /** True when the viewer can act and is not already the only assignee. */ canTake: z.boolean(), @@ -114,7 +114,6 @@ export const reviewItemParamsSchema = z /** One person action on a Review item. */ export const reviewItemActionSchema = z.discriminatedUnion("action", [ z.object({ action: z.literal("take") }).strict(), - z.object({ action: z.literal("complete") }).strict(), z.object({ action: z.literal("dismiss") }).strict(), z .object({ diff --git a/packages/junior/src/chat/ingress/slack-review-item.ts b/packages/junior/src/chat/ingress/slack-review-item.ts index 5c821aec08..afc2f1abbc 100644 --- a/packages/junior/src/chat/ingress/slack-review-item.ts +++ b/packages/junior/src/chat/ingress/slack-review-item.ts @@ -283,10 +283,6 @@ export async function handleSlackReviewItemClick(args: { services, target, }; - if (click.kind === "done") { - await act({ ...common, action: "complete" }); - return; - } if (click.kind === "dismiss") { await act({ ...common, action: "dismiss" }); return; diff --git a/packages/junior/src/chat/review-items/actions.ts b/packages/junior/src/chat/review-items/actions.ts index ff014f124a..f5aefd2e9b 100644 --- a/packages/junior/src/chat/review-items/actions.ts +++ b/packages/junior/src/chat/review-items/actions.ts @@ -25,7 +25,7 @@ import { } from "./store"; /** What a person can do with a Review item. */ -export type ReviewItemAction = "take" | "submit" | "complete" | "dismiss"; +export type ReviewItemAction = "take" | "submit" | "dismiss"; /** An expected failure of a person action, for the API to report. */ export class ReviewItemActionError extends Error { @@ -61,7 +61,6 @@ export function reviewItemChange( ): ReviewItemChange { if (action === "take") return { kind: "taken", user }; if (action === "submit") return { kind: "submitted", user }; - if (action === "complete") return { kind: "completed", user }; return { kind: "dismissed", user }; } @@ -105,7 +104,7 @@ function submittedAnswer( /** * Do one person action on a Review item and tell its linked Conversations. - * The first submit, complete, or dismiss closes the item for every + * The first submit or dismiss closes the item for every * assignee. A retry of the same close sends any missing messages again and * succeeds, so a client can retry after a failure. With `firstCloseOnly`, * only the call that closes the item succeeds, and every other call gets a @@ -169,12 +168,6 @@ export async function actOnReviewItem(args: { "Only an assignee can act on this item.", ); } - if (action === "complete" && item.kind === "action") { - throw new ReviewItemActionError( - "invalid", - "An action item needs an answer. Submit it or dismiss it.", - ); - } const answer = action === "submit" ? submittedAnswer(item, args.answer) : undefined; closedNow = await closeReviewItem(db, { diff --git a/packages/junior/src/chat/review-items/messages.ts b/packages/junior/src/chat/review-items/messages.ts index 191b06f5f0..457c32b0e5 100644 --- a/packages/junior/src/chat/review-items/messages.ts +++ b/packages/junior/src/chat/review-items/messages.ts @@ -22,7 +22,6 @@ import type { ReviewItem } from "./store"; /** One change to a Review item that linked Conversations learn about. */ export type ReviewItemChange = | { kind: "submitted"; user: User } - | { kind: "completed"; user: User } | { kind: "dismissed"; user: User } | { kind: "taken"; user: User } | { kind: "replied"; who: string } @@ -52,11 +51,6 @@ export function renderReviewItemMessage( ? formatUserInputAnswer(item.request, item.answer) : "(no answer)", ].join("\n"); - case "completed": - return [ - `${personName(change.user)} marked ${label} done.`, - ...question, - ].join("\n"); case "dismissed": return [ `${personName(change.user)} dismissed ${label}.`, diff --git a/packages/junior/src/chat/slack/review-item-card.ts b/packages/junior/src/chat/slack/review-item-card.ts index d49a5473f8..32d62e3451 100644 --- a/packages/junior/src/chat/slack/review-item-card.ts +++ b/packages/junior/src/chat/slack/review-item-card.ts @@ -22,7 +22,6 @@ export const SLACK_REVIEW_ITEM_VIEW_ID = "junior_review_item"; const ACTION_ID_PREFIX = "junior_review_item:"; const CHOICE_ACTION_ID_PREFIX = `${ACTION_ID_PREFIX}choice:`; const ANSWER_ACTION_ID = `${ACTION_ID_PREFIX}answer`; -const DONE_ACTION_ID = `${ACTION_ID_PREFIX}done`; const DISMISS_ACTION_ID = `${ACTION_ID_PREFIX}dismiss`; // Slack allows 12,000 characters in all markdown blocks of one message. const MAX_BODY_CHARS = 3_000; @@ -67,18 +66,9 @@ function controls(item: ReviewItem): Button[] { text: plainText("Dismiss"), value: item.id, }; - if (item.kind === "notice" || !item.request) { - return [ - { - type: "button", - action_id: DONE_ACTION_ID, - style: "primary", - text: plainText("Done"), - value: item.id, - }, - dismiss, - ]; - } + // A notice has nothing to answer, and it can close by itself, so a person + // can only dismiss it. + if (item.kind === "notice" || !item.request) return [dismiss]; const buttonField = buttonUserInputField(item.request); if (!buttonField) { return [ @@ -142,8 +132,8 @@ function closedNote(item: ReviewItem): string { /** * Render a Review item as one Slack message. An open item shows its * controls: one button per choice when one click can answer it, an Answer - * button that opens a form otherwise, and Done for a notice. A closed item - * shows a note instead, so nobody can act on it again. + * button that opens a form otherwise, and only Dismiss for a notice. A + * closed item shows a note instead, so nobody can act on it again. */ export function buildSlackReviewItemCard(item: ReviewItem): { blocks: KnownBlock[]; @@ -191,7 +181,6 @@ export function buildHiddenSlackReviewItemCard(item: ReviewItem): { export type SlackReviewItemClick = | { kind: "choice"; itemId: string; label: string } | { kind: "answer"; itemId: string } - | { kind: "done"; itemId: string } | { kind: "dismiss"; itemId: string }; const choiceValueSchema = z.object({ @@ -226,9 +215,6 @@ export function parseSlackReviewItemClick(action: { if (action.action_id === ANSWER_ACTION_ID) { return { kind: "answer", itemId: value }; } - if (action.action_id === DONE_ACTION_ID) { - return { kind: "done", itemId: value }; - } if (action.action_id === DISMISS_ACTION_ID) { return { kind: "dismiss", itemId: value }; } diff --git a/packages/junior/tests/integration/api/review-items/actions.test.ts b/packages/junior/tests/integration/api/review-items/actions.test.ts index 5375f33f34..f1605970a6 100644 --- a/packages/junior/tests/integration/api/review-items/actions.test.ts +++ b/packages/junior/tests/integration/api/review-items/actions.test.ts @@ -151,11 +151,8 @@ describe("review item actions API", () => { [], ); expect( - (await api(fixture, cal).act(id, { action: "complete" })).status, + (await api(fixture, cal).act(id, { action: "dismiss" })).status, ).toBe(403); - expect( - (await api(fixture, ana).act(id, { action: "complete" })).status, - ).toBe(400); const submitted = await api(fixture, ana).act(id, { action: "submit", diff --git a/packages/junior/tests/integration/slack/review-item-card-webhook.test.ts b/packages/junior/tests/integration/slack/review-item-card-webhook.test.ts index e0d92e2a5a..547cdf5527 100644 --- a/packages/junior/tests/integration/slack/review-item-card-webhook.test.ts +++ b/packages/junior/tests/integration/slack/review-item-card-webhook.test.ts @@ -578,13 +578,18 @@ describe("Slack webhook: Review item cards", () => { dedupe_key: "deploy-key", }); + const posts = slackApiOutbox.messages(); + expect(posts.map((post) => post.params.channel)).toEqual(["C123"]); + // A notice has nothing to answer, so its only control is Dismiss. expect( - slackApiOutbox.messages().map((post) => post.params.channel), - ).toEqual(["C123"]); + blocksOf(posts[0]) + .find((block) => block.type === "actions") + ?.elements?.map((element) => element.action_id), + ).toEqual(["junior_review_item:dismiss"]); // Only an assignee can act. Ben gets an ephemeral note. await click({ - actionId: "junior_review_item:done", + actionId: "junior_review_item:dismiss", actionTs: "1712345.0003", itemId: item.id, userId: "U456", @@ -594,7 +599,7 @@ describe("Slack webhook: Review item cards", () => { ); await click({ - actionId: "junior_review_item:done", + actionId: "junior_review_item:dismiss", actionTs: "1712345.0004", itemId: item.id, userId: "U123", @@ -602,7 +607,7 @@ describe("Slack webhook: Review item cards", () => { const updates = slackApiOutbox.calls("chat.update"); expect(updates).toHaveLength(1); expect(JSON.stringify(blocksOf(updates[0]))).toContain( - "ana@example.com marked this done", + "ana@example.com dismissed this", ); });