fix(review-items): Let a person only dismiss a notice

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 }) {
         </div>
       ) : (
         <div className="flex flex-wrap items-center gap-2">
-          {item.kind === "notice" ? (
-            <Button
-              disabled={action.isPending}
-              onClick={() => void run({ action: "complete" }).catch(() => {})}
-              variant="primary"
-            >
-              Done
-            </Button>
-          ) : null}
           <div className="flex flex-wrap gap-1 sm:ml-auto">
             {secondaryActions}
           </div>

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({

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}`,

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,

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({

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;

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, {

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}.`,

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 };
   }

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",

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,15 +599,15 @@ 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",
     });
     const updates = slackApiOutbox.calls("chat.update");
     expect(updates).toHaveLength(1);
     expect(JSON.stringify(blocksOf(updates[0]))).toContain(
-      "[email protected] marked this done",
+      "[email protected] dismissed this",
     );
   });