diff --git a/frontend/app/cycle/cycle.spec.ts b/frontend/app/cycle/cycle.spec.ts index 00c8589a..80e7960b 100644 --- a/frontend/app/cycle/cycle.spec.ts +++ b/frontend/app/cycle/cycle.spec.ts @@ -356,8 +356,33 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa await test.step("promote the completed batch into the trunk", async () => { // The trunk carries assets only, and promotion is a **union** against current // membership — idempotent, with no log entry when nothing changed. + // + // **This used to assert the button's label flipped to "Promoted", and that + // was the whole of the feedback.** A label flip is not a report: it could not + // say how many assets moved, it could not tell a first press from a repeat, + // and it made a second press look forbidden when it is merely a no-op. + // Promotion is not a transition either, so nothing else on the row could + // move — which is how a working call came to read as a broken button. await page.getByTestId("promote-cycle-batch").click(); - await expect(page.getByTestId("promote-cycle-batch")).toHaveText("Promoted"); + + const said = page.getByTestId("promoted-cycle-batch"); + await expect(said).toBeVisible({ timeout: 30_000 }); + // Three assets annotated in the step above, and every one of them promotable. + await expect(said).toHaveText(/Promoted 3 assets/); + // The button stays a button, so the batch can be promoted again after a + // curator removes something. + await expect(page.getByTestId("promote-cycle-batch")).toHaveText(/Promote/); + }); + + await test.step("the trunk count survives a reload, which the response cannot", async () => { + // The other half of making promotion observable: the response says what *this + // press* did and is gone on the next render, while `promoted_asset_count` is + // derived per read and is still right in a session that did no promoting. + await page.reload(); + await expect(page.getByTestId("batches-table")).toBeVisible(); + await expect(page.getByTestId("promoted-count-cycle-batch")).toHaveText( + /3 of 3 in the dataset/, + ); }); await test.step("publish a release", async () => { diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index 2e7659f7..f97a41c2 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -114,6 +114,7 @@ async function serveApi( schema_version: 3, asset_count: 2, allowed_actions: batchActions(lifecycle.batch), + promoted_asset_count: 0, progress: { unannotated: 2, annotated: 0, diff --git a/frontend/app/e2e/gallery.spec.ts b/frontend/app/e2e/gallery.spec.ts index b17efc60..0104bd5d 100644 --- a/frontend/app/e2e/gallery.spec.ts +++ b/frontend/app/e2e/gallery.spec.ts @@ -185,6 +185,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro asset_count: counts.total, progress: counts, allowed_actions: batchActions(current), + promoted_asset_count: 0, }, }); } @@ -233,6 +234,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro asset_count: counts.total, progress: counts, allowed_actions: batchActions(current), + promoted_asset_count: 0, }, }); } @@ -253,6 +255,7 @@ async function serveApi(page: Page, sent: Request[], options: Options = {}): Pro asset_count: counts.total, progress: counts, allowed_actions: batchActions(current), + promoted_asset_count: 0, }, }); } diff --git a/frontend/app/e2e/navigation.spec.ts b/frontend/app/e2e/navigation.spec.ts index ef7aaab5..da0130dd 100644 --- a/frontend/app/e2e/navigation.spec.ts +++ b/frontend/app/e2e/navigation.spec.ts @@ -101,6 +101,7 @@ async function serveApi(page: Page): Promise { name: "drive-01", state: "in_annotation", allowed_actions: batchActions("in_annotation"), + promoted_asset_count: 0, schema_version: 1, asset_count: 1, progress: { ...NO_PROGRESS, unannotated: 1, total: 1 }, diff --git a/frontend/app/e2e/viewport.spec.ts b/frontend/app/e2e/viewport.spec.ts index 6d35e483..822a55fb 100644 --- a/frontend/app/e2e/viewport.spec.ts +++ b/frontend/app/e2e/viewport.spec.ts @@ -96,6 +96,7 @@ async function serveApi(page: Page): Promise { state: "in_annotation", schema_version: 1, allowed_actions: batchActions("in_annotation"), + promoted_asset_count: 0, asset_count: 1, progress: NO_PROGRESS, }, diff --git a/frontend/app/src/routes.tsx b/frontend/app/src/routes.tsx index 4d8e3625..b7e2e728 100644 --- a/frontend/app/src/routes.tsx +++ b/frontend/app/src/routes.tsx @@ -221,6 +221,10 @@ function Gallery(): JSX.Element { // The approve dialog's SCHEMA_NOT_FOUND remedy (#291): the schema section // is a `?tab=` on the project page, and spelling that URL is this file's job. onOpenSchema={() => void navigate(`/projects/${projectId}?tab=schema`)} + // Where a promotion from this screen lands (F18). The gallery is where a + // batch is finished, and it had no way to reach the one screen that shows + // what finishing it produced. + onOpenDataset={() => void navigate(`/projects/${projectId}/dataset`)} /> ); } diff --git a/frontend/ui-core/src/annotator/viewportFloor.test.tsx b/frontend/ui-core/src/annotator/viewportFloor.test.tsx index d522af54..7b82b3da 100644 --- a/frontend/ui-core/src/annotator/viewportFloor.test.tsx +++ b/frontend/ui-core/src/annotator/viewportFloor.test.tsx @@ -68,6 +68,7 @@ beforeEach(() => { schema_version: 1, asset_count: 1, allowed_actions: batchActions("in_annotation"), + promoted_asset_count: 0, progress: { unannotated: 1, annotated: 0, diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index 677fdd3f..4da3f105 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -1802,6 +1802,8 @@ export interface components { * Format: uuid */ project_id: string; + /** Promoted Asset Count */ + promoted_asset_count: number; /** Schema Version */ schema_version: number | null; state: components["schemas"]["BatchState"]; diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index 7d7e60ed..be8f2557 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -83,7 +83,7 @@ export const checkProgressCounts: Check = /*#__PURE__*/ object({ "accepted": [true, isInteger], "annotated": [true, isInteger], "review_pending": [true, isInteger], "skipped": [true, isInteger], "total": [true, isInteger], "unannotated": [true, isInteger] } as const); export const checkBatchOut: Check = - /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkBatchAction)], "asset_count": [true, isInteger], "id": [true, isString], "name": [true, isString], "progress": [true, checkProgressCounts], "project_id": [true, isString], "schema_version": [true, either([isInteger, isNull] as const)], "state": [true, checkBatchState] } as const); + /*#__PURE__*/ object({ "allowed_actions": [true, arrayOf(checkBatchAction)], "asset_count": [true, isInteger], "id": [true, isString], "name": [true, isString], "progress": [true, checkProgressCounts], "project_id": [true, isString], "promoted_asset_count": [true, isInteger], "schema_version": [true, either([isInteger, isNull] as const)], "state": [true, checkBatchState] } as const); export const checkBatchPage: Check = /*#__PURE__*/ object({ "items": [true, arrayOf(checkBatchOut)], "total": [true, isInteger] } as const); diff --git a/frontend/ui-core/src/index.ts b/frontend/ui-core/src/index.ts index 6ca64b37..efb4ab7a 100644 --- a/frontend/ui-core/src/index.ts +++ b/frontend/ui-core/src/index.ts @@ -191,6 +191,7 @@ export { IngestScreen, type IngestScreenProps } from "./screens/IngestScreen.js" export { BatchesScreen, type BatchesScreenProps } from "./screens/BatchesScreen.js"; export { GalleryScreen, type GalleryScreenProps } from "./screens/GalleryScreen.js"; export { ApproveDialog, BatchProgressBar } from "./screens/BatchLifecycle.js"; +export { PromoteButton, promotionSummary, type PromoteButtonProps } from "./screens/PromoteButton.js"; export { batchStateLabel, segmentCounts, diff --git a/frontend/ui-core/src/screens/BatchesScreen.tsx b/frontend/ui-core/src/screens/BatchesScreen.tsx index 77f2d8af..7f128896 100644 --- a/frontend/ui-core/src/screens/BatchesScreen.tsx +++ b/frontend/ui-core/src/screens/BatchesScreen.tsx @@ -30,7 +30,7 @@ * UUIDs. A program has the SDK and the API. */ -import { ArrowUpFromLine, Layers, Play } from "lucide-react"; +import { Layers, Play } from "lucide-react"; import { useState, type JSX } from "react"; import { Async } from "../data/Async"; @@ -43,19 +43,28 @@ import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from ". import { ApproveDialog, BatchProgressBar, CompleteBatchButton } from "./BatchLifecycle"; import { BATCH_STATE_VARIANT, batchStateLabel } from "./batchState"; import { SchemaForeshadow } from "./SchemaForeshadow"; -import { useBatchTransition, useBatches, usePromoteBatch, type Batch } from "./queries"; +import { PromoteButton } from "./PromoteButton"; +import { useBatchTransition, useBatches, type Batch } from "./queries"; export interface BatchesScreenProps { readonly projectId: string; readonly onOpenBatch: (batchId: string) => void; /** Where "define your labels" goes — the schema tab, as the host spells it. */ readonly onOpenSchema?: () => void; + /** + * The dataset, so a promotion can be followed to where it landed. + * + * Promotion's entire evidence lives on that screen and nothing linked there + * from here, so a person was told something had happened and left to find it. + */ + readonly onOpenDataset?: () => void; } export function BatchesScreen({ projectId, onOpenBatch, onOpenSchema, + onOpenDataset, }: BatchesScreenProps): JSX.Element { const batches = useBatches(projectId); const [approving, setApproving] = useState(null); @@ -135,6 +144,7 @@ export function BatchesScreen({ setApproving(batch)} + {...(onOpenDataset === undefined ? {} : { onOpenDataset })} /> @@ -172,37 +182,28 @@ export function BatchesScreen({ function Lifecycle({ batch, onApprove, + onOpenDataset, }: { readonly batch: Batch & { readonly projectId?: string }; readonly onApprove: () => void; + readonly onOpenDataset?: () => void; }): JSX.Element | null { const start = useBatchTransition(batch.id, "start"); - const promote = usePromoteBatch(batch.projectId ?? ""); if (declares(batch, BATCH_ACTION.promote)) { // The last move, and the only one that is not a state transition: promotion // adds the batch's assets to the trunk. Idempotent — a **union** against // current membership, with no log entry when nothing changed — so pressing it // twice is safe and a curator's earlier removal is restored rather than - // remembered. + // remembered. Which is exactly why the control has to *say* what it did: + // "safe to press twice" and "you cannot tell whether it worked" were the + // same button until #307's successor. See `PromoteButton`. return ( -
- - {promote.isError && ( - - {refusalProse(promote.error)} - - )} -
+ ); } if (declares(batch, BATCH_ACTION.approve)) { diff --git a/frontend/ui-core/src/screens/GalleryScreen.tsx b/frontend/ui-core/src/screens/GalleryScreen.tsx index 1e930c75..6951aeb1 100644 --- a/frontend/ui-core/src/screens/GalleryScreen.tsx +++ b/frontend/ui-core/src/screens/GalleryScreen.tsx @@ -53,6 +53,7 @@ import { AssetThumbnail } from "./AssetThumbnail"; import { BackLink } from "../patterns/BackLink"; import { parentLabel } from "../patterns/parentLabel"; import { ApproveDialog, BatchProgressBar, CompleteBatchButton } from "./BatchLifecycle"; +import { PromoteButton } from "./PromoteButton"; import { ASSET_ACTION, BATCH_ACTION, @@ -144,6 +145,14 @@ export interface GalleryScreenProps { readonly onBack?: () => void; /** The project's schema tab, for the approve dialog's `SCHEMA_NOT_FOUND` remedy (#291). */ readonly onOpenSchema?: () => void; + /** + * The dataset — where a promotion from this screen lands (audit F18). + * + * The `information-architecture` skill's rule that the dataset is reachable in + * one click from anywhere it is relevant, applied to the one screen that can + * put something into it. + */ + readonly onOpenDataset?: () => void; } export function GalleryScreen({ @@ -152,6 +161,7 @@ export function GalleryScreen({ onOpenAsset, onBack, onOpenSchema, + onOpenDataset, }: GalleryScreenProps): JSX.Element { const project = useProject(projectId); const batch = useBatch(batchId); @@ -300,8 +310,10 @@ export function GalleryScreen({ setApproving(true)} {...(onOpenAsset === undefined ? {} @@ -458,13 +470,17 @@ export function GalleryScreen({ */ function BatchHeader({ batch, + projectId, assets, showsProgress, onApprove, onStartAnnotating, + onOpenDataset, }: { readonly batch: Batch | undefined; + readonly projectId: string; readonly assets: readonly BatchAsset[]; + readonly onOpenDataset?: () => void; /** False for a draft, whose counts are documented zeros rather than data. */ readonly showsProgress: boolean; readonly onApprove: () => void; @@ -577,6 +593,23 @@ function BatchHeader({ control is shared with that table rather than spelled twice, and it withholds the press — with the count — while anything is outstanding. */} + {/* + Promotion, on the screen the work is finished from (audit F18). + + It existed only on the batch table one tab away, so a person could + settle forty-eight frames here and have nowhere to put them — and the + gallery had no link to the dataset either, which is where a promotion's + evidence lives. Capability-gated and shared with that table rather than + spelled twice: `PromoteButton` owns the sentence and the reason. + */} + {batch !== undefined && ( + + )} {batch !== undefined && batch.state === "in_annotation" && ( )} diff --git a/frontend/ui-core/src/screens/ProjectScreen.tsx b/frontend/ui-core/src/screens/ProjectScreen.tsx index 775278c2..4a73cbe0 100644 --- a/frontend/ui-core/src/screens/ProjectScreen.tsx +++ b/frontend/ui-core/src/screens/ProjectScreen.tsx @@ -294,6 +294,7 @@ export function ProjectScreen({ {...(onTabChange === undefined ? {} : { onOpenSchema: () => onTabChange("schema") })} + {...(onOpenDataset === undefined ? {} : { onOpenDataset })} /> )} diff --git a/frontend/ui-core/src/screens/PromoteButton.tsx b/frontend/ui-core/src/screens/PromoteButton.tsx new file mode 100644 index 00000000..b3ae91db --- /dev/null +++ b/frontend/ui-core/src/screens/PromoteButton.tsx @@ -0,0 +1,173 @@ +/** + * Promotion, and saying what it did — audit findings F5, F17 and F18. + * + * ## The bug was that a working call looked broken + * + * Promoting a completed batch succeeds. It always did. What a person could + * observe afterwards was the word "Promoted" on the button they had just pressed + * and **nothing else** — and nothing else was structurally possible: promotion is + * not a transition, so the batch stays `completed`, and no read model recorded + * that anything had entered the trunk. The response carrying *the assets this + * press actually promoted* was discarded unread. + * + * So three outcomes were indistinguishable: + * + * 1. promoted 3 of 48 — the founder's real batch, 3 annotated and 45 skipped; + * 2. promoted nothing, because it was already done (promotion is a **union**, so + * a second press legitimately moves zero); + * 3. the press did nothing at all. + * + * A user seeing no change concludes (3), which is the only one that was never + * true. This says which it was. + * + * ## Two numbers, because neither one alone is the answer + * + * The response says what **this press** did and cannot be recovered afterwards. + * `promoted_asset_count` says what is in the trunk **now** and is the only half + * that survives a reload. A screen with only the first forgets; a screen with + * only the second cannot tell a fresh promotion from an old one. + * + * ## Why the skipped frames get a sentence + * + * `PROMOTABLE_PROGRESS` is `{annotated, accepted}` — `skipped` is deliberately + * excluded, because skipping is a decision that the asset does not belong in the + * dataset. For the batch this was reported on that is 45 of 48 frames, and "3 + * promoted" over a batch of 48 reads as a failure unless somebody says where the + * other 45 went. + * + * ## Shared, not copied + * + * The batch table and the gallery header both render this. Two spellings of "did + * my work reach the dataset" would eventually disagree in front of a user, and + * the gallery is the screen somebody is actually on when they finish a batch — + * F18 is that it had no promote control at all. + */ + +import { ArrowUpFromLine, ArrowRight } from "lucide-react"; +import type { JSX } from "react"; + +import { BATCH_ACTION, declares } from "../data/capabilities"; +import { refusalProse } from "../data/refusals"; +import { Button } from "../primitives/Button"; +import { FieldError } from "../primitives/Input"; +import { usePromoteBatch, type Batch } from "./queries"; + +export interface PromoteButtonProps { + readonly batch: Batch; + readonly projectId: string; + /** The dataset screen, so a promotion can be followed to where it landed. */ + readonly onOpenDataset?: () => void; + readonly className?: string; +} + +/** + * What a press promoted, said in one sentence. + * + * Exported and pure because it is the part with a decision in it, and the + * decision is not obvious: **zero promoted is not a failure**, and the sentence + * for it has to say why without sounding like one. + * + * `moved` is what the response carried; `settled` is how many of the batch's + * assets are in the trunk now. A press that moved nothing over a batch that is + * entirely in the trunk means "already there"; a press that moved nothing over a + * batch with nothing in the trunk means every frame was skipped. + */ +export function promotionSummary( + moved: number, + settled: number, + total: number, +): string { + if (moved > 0) { + const excluded = total - settled; + const tail = + excluded > 0 + ? ` ${excluded} skipped frame${excluded === 1 ? "" : "s"} stayed out.` + : ""; + return `Promoted ${moved} asset${moved === 1 ? "" : "s"} to the dataset.${tail}`; + } + if (settled > 0) { + // The idempotent no-op, which is the case that looked most like a bug: a + // second press, or a first press in a fresh session after an earlier one. + return `Already in the dataset — nothing new to promote.`; + } + return "Nothing here can be promoted: every frame was skipped, and a skipped frame never enters the dataset."; +} + +export function PromoteButton({ + batch, + projectId, + onOpenDataset, + className, +}: PromoteButtonProps): JSX.Element | null { + const promote = usePromoteBatch(projectId); + + if (!declares(batch, BATCH_ACTION.promote)) return null; + + const summary = promote.isSuccess + ? promotionSummary(promote.data.total, batch.promoted_asset_count, batch.asset_count) + : null; + + return ( +
+ + + {/* + What is in the trunk already, before anybody presses anything. This is + the half that survives a reload — `promoted_asset_count` is derived per + read, so it is still right in a session that did not do the promoting. + */} + {summary === null && batch.promoted_asset_count > 0 && ( + + {batch.promoted_asset_count} of {batch.asset_count} in the dataset + + )} + + {summary !== null && ( + + {summary} + + )} + + {/* + The way onward. Promotion's whole evidence lives on the dataset screen, + and until now nothing linked there from the place the work was finished — + so a person was told something had happened and left to find it. + */} + {promote.isSuccess && onOpenDataset !== undefined && ( + + )} + + {promote.isError && ( + + {refusalProse(promote.error)} + + )} +
+ ); +} diff --git a/frontend/ui-core/src/screens/batchLifecycle.test.tsx b/frontend/ui-core/src/screens/batchLifecycle.test.tsx index 59554375..ed212970 100644 --- a/frontend/ui-core/src/screens/batchLifecycle.test.tsx +++ b/frontend/ui-core/src/screens/batchLifecycle.test.tsx @@ -86,6 +86,7 @@ const DRAFT: Batch = { schema_version: null, asset_count: 48, allowed_actions: batchActions("draft"), + promoted_asset_count: 0, progress: { unannotated: 48, annotated: 0, @@ -165,6 +166,7 @@ describe("the approve dialog's refusals", () => { state: "approved", schema_version: 3, allowed_actions: batchActions("approved"), + promoted_asset_count: 0, }, }); const closed = vi.fn(); diff --git a/frontend/ui-core/src/screens/gallery.test.tsx b/frontend/ui-core/src/screens/gallery.test.tsx index dbde06b2..cf3ed14d 100644 --- a/frontend/ui-core/src/screens/gallery.test.tsx +++ b/frontend/ui-core/src/screens/gallery.test.tsx @@ -122,6 +122,7 @@ function batch(overrides: Record = {}): Record asset_count: 120, progress: { ...NO_PROGRESS, unannotated: 120, total: 120 }, allowed_actions: batchActions(state), + promoted_asset_count: 0, ...overrides, }; } @@ -1375,4 +1376,23 @@ describe("the gallery header's way into the annotator", () => { await open("annotated", "skipped"); expect(screen.getByTestId("open-asset-0").textContent).toBe("Open"); }); + + /** + * Promotion, on the screen the work is finished from — audit finding F18. + * + * It lived only on the batch table one tab away, so a person could settle + * forty-eight frames here and have nowhere to put them; and the gallery had no + * link to the dataset either, which is where a promotion's evidence lives. + */ + it("offers Promote once the batch is completed", async () => { + await openIn("completed", "annotated", "skipped"); + expect(screen.queryByTestId("promote-drive-01")).not.toBeNull(); + }); + + it("offers no Promote before the batch is completed", async () => { + // Capability-gated, not state-guessed: `PROMOTABLE_STATES` is the kernel's + // and the wire declares it. + await open("annotated", "unannotated"); + expect(screen.queryByTestId("promote-drive-01")).toBeNull(); + }); }); diff --git a/frontend/ui-core/src/screens/ingest.test.tsx b/frontend/ui-core/src/screens/ingest.test.tsx index 84979cfd..a098727f 100644 --- a/frontend/ui-core/src/screens/ingest.test.tsx +++ b/frontend/ui-core/src/screens/ingest.test.tsx @@ -545,8 +545,8 @@ describe("launching a run", () => { status: 200, body: { items: [ - { id: "b1", project_id: PROJECT, name: "open", state: "draft", schema_version: null, asset_count: 4, progress: NO_PROGRESS, allowed_actions: batchActions("draft") }, - { id: "b2", project_id: PROJECT, name: "frozen", state: "in_annotation", schema_version: 1, asset_count: 9, progress: NO_PROGRESS, allowed_actions: batchActions("in_annotation") }, + { id: "b1", project_id: PROJECT, name: "open", state: "draft", schema_version: null, asset_count: 4, progress: NO_PROGRESS, allowed_actions: batchActions("draft"), promoted_asset_count: 0 }, + { id: "b2", project_id: PROJECT, name: "frozen", state: "in_annotation", schema_version: 1, asset_count: 9, progress: NO_PROGRESS, allowed_actions: batchActions("in_annotation"), promoted_asset_count: 0 }, ], total: 2, }, diff --git a/frontend/ui-core/src/screens/navigation.test.tsx b/frontend/ui-core/src/screens/navigation.test.tsx index a4f2358a..8e632d65 100644 --- a/frontend/ui-core/src/screens/navigation.test.tsx +++ b/frontend/ui-core/src/screens/navigation.test.tsx @@ -84,6 +84,7 @@ function answer(path: string): unknown { asset_count: 0, progress: NO_PROGRESS, allowed_actions: batchActions("in_annotation"), + promoted_asset_count: 0, }; } if (path === `/batches/${BATCH}/assets`) return { items: [], total: 0 }; diff --git a/frontend/ui-core/src/screens/overview.test.tsx b/frontend/ui-core/src/screens/overview.test.tsx index 8361296a..6b41f92a 100644 --- a/frontend/ui-core/src/screens/overview.test.tsx +++ b/frontend/ui-core/src/screens/overview.test.tsx @@ -343,6 +343,7 @@ describe("the journey checklist", () => { total: 48, }, allowed_actions: batchActions(state as BatchState), + promoted_asset_count: 0, }; } diff --git a/frontend/ui-core/src/screens/promote.test.tsx b/frontend/ui-core/src/screens/promote.test.tsx new file mode 100644 index 00000000..599a7525 --- /dev/null +++ b/frontend/ui-core/src/screens/promote.test.tsx @@ -0,0 +1,206 @@ +/** + * Promotion, and the three outcomes that used to look identical. + * + * The call always worked. What a person could observe was the word "Promoted" + * and nothing else — and nothing else was structurally possible: promotion is + * not a transition, so the batch stays `completed`, and no read model recorded + * that anything had entered the trunk. So these three were indistinguishable: + * + * 1. promoted 3 of 48 — the shape actually reported, 3 annotated and 45 skipped; + * 2. promoted nothing because it was already there (promotion is a **union**); + * 3. the press did nothing at all. + * + * A user seeing no change concludes (3), which is the only one that was never + * true. `promotionSummary` is the pure part of telling them apart, and it is + * tested on its own because the decision in it is not obvious: **zero promoted + * is not a failure**, and there are two different reasons for it. + */ + +import { QueryClient } from "@tanstack/react-query"; +import { render, screen } from "@testing-library/react"; +import { userEvent } from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { JSX, ReactNode } from "react"; + +import { ApiProvider } from "../data/ApiProvider"; +import { writeToken } from "../data/session"; +import { PromoteButton, promotionSummary } from "./PromoteButton"; +import { batchActions } from "../testing/wire.fixtures.js"; +import type { Batch } from "./queries"; + +describe("what a press promoted, in one sentence", () => { + it("counts what moved, and says where the rest went", () => { + // The founder's real shape, scaled down: 3 promoted out of 48, the other 45 + // skipped. "Promoted 3" over a batch of 48 reads as a failure unless + // somebody says where the other 45 went. + expect(promotionSummary(3, 3, 48)).toContain("Promoted 3 assets"); + expect(promotionSummary(3, 3, 48)).toContain("45 skipped frames stayed out"); + }); + + it("says nothing about exclusions when there were none", () => { + expect(promotionSummary(3, 3, 3)).toBe("Promoted 3 assets to the dataset."); + }); + + it("gets the singular right, because one frame is the common case at the end", () => { + expect(promotionSummary(1, 1, 2)).toContain("Promoted 1 asset "); + expect(promotionSummary(1, 1, 2)).toContain("1 skipped frame stayed out"); + }); + + it("calls a second press already-done rather than nothing-happened", () => { + // Outcome 2, and the one that looked most like a bug: promotion is a union, + // so pressing twice legitimately moves zero. The trunk count is what tells + // this apart from a batch that could never promote anything. + expect(promotionSummary(0, 3, 3)).toMatch(/already in the dataset/i); + }); + + it("explains a batch that has nothing to give, rather than reporting a failure", () => { + // Outcome 3's honest twin: zero moved *and* zero in the trunk means every + // frame was skipped, and `PROMOTABLE_PROGRESS` excludes those on purpose. + expect(promotionSummary(0, 0, 48)).toMatch(/every frame was skipped/i); + }); +}); + +const API = "http://visionset.test"; +const PROJECT = "11111111-1111-4111-8111-111111111111"; +const BATCH = "55555555-5555-4555-8555-555555555555"; + +type Answer = { status: number; body?: unknown }; +let handlers: ((request: Request) => Answer | undefined)[] = []; + +beforeEach(() => { + handlers = []; + writeToken("a-token"); + vi.stubGlobal("fetch", async (request: Request) => { + for (const handler of handlers) { + const answer = handler(request); + if (answer !== undefined) { + return new Response(JSON.stringify(answer.body ?? null), { + status: answer.status, + headers: { "content-type": "application/json" }, + }); + } + } + return new Response(JSON.stringify({ code: "NO_STUB", message: request.url }), { + status: 500, + headers: { "content-type": "application/json" }, + }); + }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + globalThis.sessionStorage.clear(); +}); + +function mount(node: ReactNode): JSX.Element { + return ( + + {node} + + ); +} + +function batch(overrides: Partial = {}): Batch { + return { + id: BATCH, + project_id: PROJECT, + name: "drive-01", + state: "completed", + schema_version: 1, + asset_count: 48, + progress: { + unannotated: 0, + annotated: 3, + skipped: 45, + review_pending: 0, + accepted: 0, + total: 48, + }, + allowed_actions: batchActions("completed"), + promoted_asset_count: 0, + ...overrides, + } as Batch; +} + +function answersPromote(total: number): void { + handlers.push((request) => + request.method === "POST" && request.url.includes("/promote") + ? { status: 200, body: { items: [], total } } + : undefined, + ); +} + +describe("the control", () => { + it("is drawn only where the batch declares promote", () => { + render( + mount( + , + ), + ); + expect(screen.queryByTestId("promote-drive-01")).toBeNull(); + }); + + it("reports what the press did, rather than flipping its own label", async () => { + // The label said "Promoted" and that was the entire feedback. A label flip is + // not a report — and it also made a second press look forbidden when it is + // merely a no-op. + answersPromote(3); + render(mount()); + + await userEvent.click(screen.getByTestId("promote-drive-01")); + + expect((await screen.findByTestId("promoted-drive-01")).textContent).toContain( + "Promoted 3 assets", + ); + expect(screen.getByTestId("promote-drive-01").textContent).toContain("Promote"); + }); + + it("links onward to where the work landed", async () => { + // Promotion's whole evidence lives on the dataset screen, and nothing linked + // there from the place the work was finished. + answersPromote(3); + const opened = vi.fn(); + render(mount()); + + await userEvent.click(screen.getByTestId("promote-drive-01")); + await userEvent.click(await screen.findByTestId("promoted-open-dataset-drive-01")); + + expect(opened).toHaveBeenCalledOnce(); + }); + + it("says what is already in the trunk before anybody presses anything", () => { + // The half that survives a reload: `promoted_asset_count` is derived per + // read, so a session that did not do the promoting still sees it. + render(mount()); + + expect(screen.getByTestId("promoted-count-drive-01").textContent).toBe( + "3 of 48 in the dataset", + ); + }); + + it("says nothing about the trunk when nothing is in it", () => { + render(mount()); + expect(screen.queryByTestId("promoted-count-drive-01")).toBeNull(); + }); + + it("renders a refusal as prose", async () => { + handlers.push((request) => + request.method === "POST" + ? { status: 409, body: { code: "BATCH_NOT_COMPLETE", message: "jobs outstanding" } } + : undefined, + ); + render(mount()); + + await userEvent.click(screen.getByTestId("promote-drive-01")); + + const said = (await screen.findByTestId("promote-error-drive-01")).textContent ?? ""; + expect(said).toContain("still unfinished"); + expect(said).not.toContain("BATCH_NOT_COMPLETE"); + }); +}); diff --git a/frontend/ui-core/src/screens/queries.ts b/frontend/ui-core/src/screens/queries.ts index 0d219c59..fa0c1cc6 100644 --- a/frontend/ui-core/src/screens/queries.ts +++ b/frontend/ui-core/src/screens/queries.ts @@ -1001,7 +1001,25 @@ export function useFormats() { }); } -/** Promote a completed batch into the trunk. Idempotent — a union, not an append. */ +/** + * Promote a completed batch into the trunk. Idempotent — a union, not an append. + * + * **The response is the whole point and was being thrown away** (audit F5). The + * route answers an `AssetPage` of *the assets this press actually promoted*, and + * the screen kept nothing but a button label. What a person could then observe + * was: the word "Promoted", and nothing else — no count, no navigation, and + * structurally nothing else on the row that could move, because promotion is not + * a transition and the batch stays `completed`. + * + * That made three different outcomes identical: "promoted 3 of 48", "promoted + * nothing because it was already done", and "the press did nothing at all". The + * third is what a user concludes, and it is the only one that was never true. + * + * `promoted_asset_count` on `BatchOut` is the other half — the response says what + * *this press* did, the field says what is in the trunk *now*, and only the + * second survives a reload. Both are needed: the first cannot be recovered after + * the fact, and the second cannot distinguish a fresh promotion from an old one. + */ export function usePromoteBatch(projectId: string) { const client = useApiClient(); const queries = useQueryClient(); @@ -1016,6 +1034,11 @@ export function usePromoteBatch(projectId: string) { onSuccess: () => { void queries.invalidateQueries({ queryKey: ["projects", projectId] }); void queries.invalidateQueries({ queryKey: ["datasets"] }); + // The batch's own read, because `promoted_asset_count` moved and nothing + // else on it did. Without this the number a person just changed keeps its + // old value until something unrelated refetches — the declaration-goes- + // stale shape, one field over. + void queries.invalidateQueries({ queryKey: ["batches"] }); }, }); } diff --git a/frontend/ui-core/src/screens/readiness.test.tsx b/frontend/ui-core/src/screens/readiness.test.tsx index d3ad4ba6..22486b79 100644 --- a/frontend/ui-core/src/screens/readiness.test.tsx +++ b/frontend/ui-core/src/screens/readiness.test.tsx @@ -130,6 +130,7 @@ function batchOf(state: string): Record { total: 48, }, allowed_actions: batchActions(state as BatchState), + promoted_asset_count: 0, }; } diff --git a/frontend/ui-core/src/screens/screens.test.tsx b/frontend/ui-core/src/screens/screens.test.tsx index bb001d9c..643a50c9 100644 --- a/frontend/ui-core/src/screens/screens.test.tsx +++ b/frontend/ui-core/src/screens/screens.test.tsx @@ -1177,6 +1177,7 @@ describe("the project header", () => { total: 4, }, allowed_actions: batchActions(options.batchState as BatchState), + promoted_asset_count: 0, }, ], total: 1, diff --git a/openapi.json b/openapi.json index e8b93d24..1d26a538 100644 --- a/openapi.json +++ b/openapi.json @@ -863,6 +863,10 @@ "title": "Project Id", "type": "string" }, + "promoted_asset_count": { + "title": "Promoted Asset Count", + "type": "integer" + }, "schema_version": { "anyOf": [ { @@ -886,7 +890,8 @@ "schema_version", "asset_count", "progress", - "allowed_actions" + "allowed_actions", + "promoted_asset_count" ], "title": "BatchOut", "type": "object" diff --git a/src/visionset/cli/batches.py b/src/visionset/cli/batches.py index 91408e3d..d7aa9530 100644 --- a/src/visionset/cli/batches.py +++ b/src/visionset/cli/batches.py @@ -35,7 +35,13 @@ from visionset.cli._resolve import ProjectOption, resolve_project from visionset.cli._workspace import WorkspaceOption, opened_workspace from visionset.kernel.domain import AssetProgress, BySize, Partition -from visionset.kernel.services import BatchService, DatasetService, JobService +from visionset.kernel.services import ( + BatchService, + DatasetService, + JobService, + ProjectService, + WorkspaceService, +) batch_app = typer.Typer(help="Move batches through the annotation lifecycle.", no_args_is_help=True) @@ -63,6 +69,17 @@ def _echo(batch_id: UUID, state: str, json_out: bool, payload: dict[str, object] typer.echo(str(batch_id)) +def _promoted(service: WorkspaceService, project_id: UUID) -> frozenset[UUID]: + """The trunk's current membership, read once for the whole answer. + + The same cost model REST and MCP use: one query per invocation rather than + one per batch, because ``asset_ids`` is already in hand and the rest is a set + intersection. + """ + dataset = ProjectService(service).get_dataset(project_id) + return DatasetService(service).member_asset_ids(dataset.id) + + @batch_app.command("list") def batch_list( project: ProjectOption, @@ -78,8 +95,13 @@ def batch_list( # does. The counts are the point of the listing: a batch's name and state # do not say whether anybody has started on it. counts = [jobs.batch_progress(batch.id) for batch in batches] + promoted = _promoted(service, resolved.id) if json_out: - document(wire.page([wire.batch(b, c) for b, c in zip(batches, counts, strict=True)])) + document( + wire.page( + [wire.batch(b, c, promoted=promoted) for b, c in zip(batches, counts, strict=True)] + ) + ) return table( _COLUMNS, @@ -129,8 +151,9 @@ def batch_approve( approved = batches.approve(batch, partition) counts = JobService(service).batch_progress(approved.id) job_count = len(batches.jobs(approved.id)) + promoted = _promoted(service, approved.project_id) if json_out: - document(wire.batch(approved, counts)) + document(wire.batch(approved, counts, promoted=promoted)) return note( f"Approved batch {approved.name!r} against schema version " @@ -149,7 +172,8 @@ def batch_start( with opened_workspace(workspace) as service: started = BatchService(service).start(batch) counts = JobService(service).batch_progress(started.id) - _echo(started.id, started.state.value, json_out, wire.batch(started, counts)) + promoted = _promoted(service, started.project_id) + _echo(started.id, started.state.value, json_out, wire.batch(started, counts, promoted=promoted)) @batch_app.command("complete") @@ -166,7 +190,13 @@ def batch_complete( with opened_workspace(workspace) as service: completed = BatchService(service).complete(batch) counts = JobService(service).batch_progress(completed.id) - _echo(completed.id, completed.state.value, json_out, wire.batch(completed, counts)) + promoted = _promoted(service, completed.project_id) + _echo( + completed.id, + completed.state.value, + json_out, + wire.batch(completed, counts, promoted=promoted), + ) @batch_app.command("promote") diff --git a/src/visionset/kernel/services/dataset_service.py b/src/visionset/kernel/services/dataset_service.py index 02b8dd1a..8801c114 100644 --- a/src/visionset/kernel/services/dataset_service.py +++ b/src/visionset/kernel/services/dataset_service.py @@ -97,6 +97,30 @@ def assets(self, dataset_id: UUID) -> list[Asset]: with self._workspace.unit_of_work() as uow: return assets_of(uow, self.require_dataset(uow, dataset_id)) + def member_asset_ids(self, dataset_id: UUID) -> frozenset[UUID]: + """Which assets are in the trunk right now, as a set to test against. + + The cheap half of :meth:`assets`. That one resolves every member to an + ``Asset`` because its callers render them; this one answers *is this in + the trunk*, which needs the id alone — so it skips one lookup per member, + and over a dataset of fifty thousand that is the whole cost of the call. + + A **set**, and returned rather than answered per id, because the caller + that wanted this asks about a batch's worth of assets at once: how much of + a completed batch has reached the trunk is a question about an + intersection, and asking it one id at a time is the shape that turns one + read into N. + + Current membership, not a history. A curator removing an asset takes it + out of this answer, which is right for every question anybody asks it — + "is my work in the dataset" is about now. + + Raises: + DatasetNotFound: no such dataset in this workspace. + """ + with self._workspace.unit_of_work() as uow: + return member_asset_ids_of(uow, self.require_dataset(uow, dataset_id)) + def changes(self, dataset_id: UUID) -> list[DatasetChange]: """The mutation log, oldest entry first. @@ -332,6 +356,19 @@ def assets_of(uow: UnitOfWork, dataset: Dataset) -> list[Asset]: ] +def member_asset_ids_of(uow: UnitOfWork, dataset: Dataset) -> frozenset[UUID]: + """The trunk's membership as a set of ids, inside a caller's own transaction. + + Module-level and public beside :func:`assets_of` for the reason that one is: + a second walk of ``dataset_member`` written somewhere else is a second chance + to disagree with this one. Unlike ``assets_of`` it does **not** resolve the + assets, so it cannot raise ``WorkspaceCorrupt`` — a member naming an asset + that is gone is still a member, and answering "which ids are in" honestly does + not require the rows behind them. + """ + return frozenset(member.asset_id for member in uow.dataset_members.list(dataset.id)) + + def _promotable(batch: Batch, jobs: Iterable[AnnotationJob]) -> list[UUID]: """The batch's assets that earned a place in the trunk, in the batch's order. diff --git a/src/visionset/mcp/batches.py b/src/visionset/mcp/batches.py index 72b26270..569f16a7 100644 --- a/src/visionset/mcp/batches.py +++ b/src/visionset/mcp/batches.py @@ -32,7 +32,13 @@ from visionset import wire from visionset.kernel.domain import BySize, Partition -from visionset.kernel.services import BatchService, DatasetService, JobService, WorkspaceService +from visionset.kernel.services import ( + BatchService, + DatasetService, + JobService, + ProjectService, + WorkspaceService, +) from visionset.mcp._resolve import ProjectRef, identifier, resolve_project from visionset.mcp._workspace import opened_workspace @@ -43,6 +49,17 @@ """Who the dataset change log records for a promotion made by an agent.""" +def _promoted(workspace: WorkspaceService, project_id: UUID) -> frozenset[UUID]: + """The trunk's current membership, read once for the whole answer. + + The same cost model the REST routes use: one query per call rather than one + per batch, because ``asset_ids`` is already in memory and the rest is a set + intersection. + """ + dataset = ProjectService(workspace).get_dataset(project_id) + return DatasetService(workspace).member_asset_ids(dataset.id) + + def _batch_payload(workspace: WorkspaceService, batch_id: UUID) -> dict[str, Any]: """The batch, its progress and its jobs — the shape three tools return.""" batches = BatchService(workspace) @@ -50,7 +67,7 @@ def _batch_payload(workspace: WorkspaceService, batch_id: UUID) -> dict[str, Any counts = JobService(workspace).batch_progress(batch.id) jobs = batches.jobs(batch.id) return { - **wire.batch(batch, counts), + **wire.batch(batch, counts, promoted=_promoted(workspace, batch.project_id)), "jobs": [wire.job(j, batch_id=batch.id, batch_state=batch.state) for j in jobs], } @@ -67,8 +84,9 @@ def list_batches(project: ProjectRef) -> dict[str, Any]: found = BatchService(workspace).list(resolved.id) jobs = JobService(workspace) counts = [jobs.batch_progress(b.id) for b in found] + promoted = _promoted(workspace, resolved.id) return wire.page( - [wire.batch(b, c) for b, c in zip(found, counts, strict=True)], + [wire.batch(b, c, promoted=promoted) for b, c in zip(found, counts, strict=True)], ) diff --git a/src/visionset/server/models.py b/src/visionset/server/models.py index da3e11e4..7adc95f3 100644 --- a/src/visionset/server/models.py +++ b/src/visionset/server/models.py @@ -37,6 +37,7 @@ from __future__ import annotations +from collections.abc import Set as AbstractSet from datetime import datetime from typing import Annotated, Literal, Self from uuid import UUID, uuid4 @@ -614,6 +615,26 @@ def of(cls, counts: dict[AssetProgress, int]) -> Self: # on every read of its name. ``schema_version`` is null exactly while the batch # is a draft — approval is what pins one, and after that it moves only # through an explicit re-pin. +# +# ``promoted_asset_count`` is how many of this batch's assets are in the trunk +# **right now**, and it exists because promotion was otherwise unobservable. +# Promoting is not a transition — the batch stays ``completed`` — and no read +# model recorded that it had happened, so a client could not tell "promoted 3 of +# 48" from "promoted nothing because it was already done" from "the press did +# nothing at all". Every one of those looked identical, which is what made a +# working call read as a broken button. +# +# Current membership rather than a promotion log: a curator removing an asset +# takes it back out, and "how much of this batch is in the dataset" is the +# question anybody looking at a batch is actually asking. It is derived per call +# and never stored — ``Release.asset_count`` is the frozen counterpart, and it +# belongs to the release. +# +# ``promoted`` is passed in rather than read here, and that is the cost model: +# the caller reads the trunk's membership **once per request** and every batch in +# a listing tests against the same set, so a page of twenty batches is one extra +# query rather than twenty. ``batch.asset_ids`` is already in memory — it is what +# ``asset_count`` counts. class BatchOut(BaseModel): """A curated slice of a project's assets that moves through annotation together.""" @@ -625,9 +646,16 @@ class BatchOut(BaseModel): asset_count: int progress: ProgressCounts allowed_actions: list[BatchAction] + promoted_asset_count: int @classmethod - def of(cls, batch: Batch, counts: dict[AssetProgress, int]) -> Self: + def of( + cls, + batch: Batch, + counts: dict[AssetProgress, int], + *, + promoted: AbstractSet[UUID], + ) -> Self: return cls( id=batch.id, project_id=batch.project_id, @@ -637,6 +665,7 @@ def of(cls, batch: Batch, counts: dict[AssetProgress, int]) -> Self: asset_count=len(batch.asset_ids), progress=ProgressCounts.of(counts), allowed_actions=batch_actions(batch.state), + promoted_asset_count=sum(1 for one in batch.asset_ids if one in promoted), ) diff --git a/src/visionset/server/routes/batches.py b/src/visionset/server/routes/batches.py index e2c2250f..a780a2e6 100644 --- a/src/visionset/server/routes/batches.py +++ b/src/visionset/server/routes/batches.py @@ -29,7 +29,7 @@ from uuid import UUID from visionset.kernel.domain import AssetProgress -from visionset.kernel.services import BatchService, DatasetService, JobService +from visionset.kernel.services import BatchService, DatasetService, JobService, ProjectService from visionset.server.dependencies import WorkspaceDep, protected_router from visionset.server.errors import documented from visionset.server.models import ( @@ -51,13 +51,31 @@ router = protected_router(prefix="/batches", tags=["batches"]) +def _promoted(workspace: WorkspaceDep, project_id: UUID) -> frozenset[UUID]: + """The trunk's current membership, read once for the whole response. + + Every ``BatchOut`` needs it and none of them needs a different one, so a + listing of twenty batches costs one query rather than twenty — the batch's + own ``asset_ids`` are already in memory and the rest is a set intersection. + + A project's dataset is 1:1 and created in the same transaction as the + project, so this cannot fail for a project that exists; a project that does + not is already a 404 from whatever resolved it. + """ + dataset = ProjectService(workspace).get_dataset(project_id) + return DatasetService(workspace).member_asset_ids(dataset.id) + + @project_router.get("", responses=documented(404)) def list_batches(workspace: WorkspaceDep, project_id: UUID) -> BatchPage: """Every batch of that project, in the order they were created.""" jobs = JobService(workspace) found = BatchService(workspace).list(project_id) + promoted = _promoted(workspace, project_id) return BatchPage( - items=[BatchOut.of(batch, jobs.batch_progress(batch.id)) for batch in found], + items=[ + BatchOut.of(batch, jobs.batch_progress(batch.id), promoted=promoted) for batch in found + ], total=len(found), ) @@ -72,7 +90,11 @@ def get_batch(workspace: WorkspaceDep, batch_id: UUID) -> BatchOut: pins one, and moves after that only through `repin`. """ batch = BatchService(workspace).get(batch_id) - return BatchOut.of(batch, JobService(workspace).batch_progress(batch.id)) + return BatchOut.of( + batch, + JobService(workspace).batch_progress(batch.id), + promoted=_promoted(workspace, batch.project_id), + ) @router.post("/{batch_id}/approve", responses=documented(404, 409)) @@ -99,14 +121,22 @@ def approve_batch( """ partition = None if body is None else body.to_domain() batch = BatchService(workspace).approve(batch_id, partition) - return BatchOut.of(batch, JobService(workspace).batch_progress(batch.id)) + return BatchOut.of( + batch, + JobService(workspace).batch_progress(batch.id), + promoted=_promoted(workspace, batch.project_id), + ) @router.post("/{batch_id}/start", responses=documented(404, 409)) def start_batch(workspace: WorkspaceDep, batch_id: UUID) -> BatchOut: """Open the batch for annotation. Nothing may be written into it before this.""" batch = BatchService(workspace).start(batch_id) - return BatchOut.of(batch, JobService(workspace).batch_progress(batch.id)) + return BatchOut.of( + batch, + JobService(workspace).batch_progress(batch.id), + promoted=_promoted(workspace, batch.project_id), + ) @router.post("/{batch_id}/repin", responses=documented(404, 409)) @@ -135,7 +165,11 @@ def repin_batch( nothing. Annotations already written keep the version they were stamped with. """ batch = BatchService(workspace).repin(batch_id, allow_destructive=allow_destructive) - return BatchOut.of(batch, JobService(workspace).batch_progress(batch.id)) + return BatchOut.of( + batch, + JobService(workspace).batch_progress(batch.id), + promoted=_promoted(workspace, batch.project_id), + ) @router.post("/{batch_id}/complete", responses=documented(404, 409)) @@ -147,7 +181,11 @@ def complete_batch(workspace: WorkspaceDep, batch_id: UUID) -> BatchOut: what lets its annotated assets be promoted into the project's dataset. """ batch = BatchService(workspace).complete(batch_id) - return BatchOut.of(batch, JobService(workspace).batch_progress(batch.id)) + return BatchOut.of( + batch, + JobService(workspace).batch_progress(batch.id), + promoted=_promoted(workspace, batch.project_id), + ) @router.get("/{batch_id}/jobs", responses=documented(404)) diff --git a/src/visionset/wire/__init__.py b/src/visionset/wire/__init__.py index 09373e87..df56f034 100644 --- a/src/visionset/wire/__init__.py +++ b/src/visionset/wire/__init__.py @@ -47,6 +47,7 @@ from __future__ import annotations from collections.abc import Mapping, Sequence +from collections.abc import Set as AbstractSet from datetime import UTC, datetime from typing import Any from uuid import UUID @@ -314,8 +315,20 @@ def progress_counts(counts: Mapping[AssetProgress, int]) -> dict[str, Any]: } -def batch(value: Batch, counts: Mapping[AssetProgress, int]) -> dict[str, Any]: - """A batch and where its assets have got to. ``asset_ids`` is absent.""" +def batch( + value: Batch, + counts: Mapping[AssetProgress, int], + *, + promoted: AbstractSet[UUID], +) -> dict[str, Any]: + """A batch and where its assets have got to. ``asset_ids`` is absent. + + ``promoted`` is the trunk's current membership, passed in rather than read + here: a listing tests every batch against the same set, so one read covers + the whole answer and ``value.asset_ids`` is already in hand. Keyword-only and + with no default, because a default would report zero promoted for a batch + nobody checked — a number that looks like an answer and is not one. + """ return { "id": str(value.id), "project_id": str(value.project_id), @@ -325,6 +338,7 @@ def batch(value: Batch, counts: Mapping[AssetProgress, int]) -> dict[str, Any]: "asset_count": len(value.asset_ids), "progress": progress_counts(counts), "allowed_actions": [a.value for a in batch_actions(value.state)], + "promoted_asset_count": sum(1 for one in value.asset_ids if one in promoted), } diff --git a/tests/cli/test_json_contract.py b/tests/cli/test_json_contract.py index c669a2cc..ea99e6fd 100644 --- a/tests/cli/test_json_contract.py +++ b/tests/cli/test_json_contract.py @@ -92,7 +92,10 @@ models.BatchAssetOut, ), ("progress_counts", wire.progress_counts(COUNTS), models.ProgressCounts), - ("batch", wire.batch(BATCH, COUNTS), models.BatchOut), + # A promoted set that actually intersects: a count of zero would agree with + # itself even if the intersection were wrong, and this pair exists to catch + # exactly the projection that drifted from its model. + ("batch", wire.batch(BATCH, COUNTS, promoted=frozenset(BATCH.asset_ids[:1])), models.BatchOut), ("job", wire.job(JOB, batch_id=BATCH.id, batch_state=BATCH.state), models.JobOut), ( "asset_progress", diff --git a/tests/kernel/test_dataset_service.py b/tests/kernel/test_dataset_service.py index e5a692f7..dd34dafd 100644 --- a/tests/kernel/test_dataset_service.py +++ b/tests/kernel/test_dataset_service.py @@ -731,3 +731,77 @@ def test_stats_of_an_unknown_dataset_are_refused(tmp_path: Path) -> None: with pytest.raises(DatasetNotFound): fixture.datasets.stats(uuid4()) fixture.close() + + +# --- who is in the trunk, asked cheaply --------------------------------------- +# +# `member_asset_ids` exists because promotion was unobservable: the batch stays +# `completed`, so nothing on a batch read moved when its assets entered the +# dataset, and three different outcomes looked identical to a client. The wire's +# `promoted_asset_count` is an intersection against this set. + + +def test_an_unpromoted_dataset_has_no_members(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + assert fixture.datasets.member_asset_ids(fixture.dataset.id) == frozenset() + + +def test_the_ids_are_exactly_what_promotion_put_there(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + fixture.completed(AssetProgress.ANNOTATED, AssetProgress.SKIPPED, AssetProgress.ANNOTATED) + promoted = fixture.datasets.promote(fixture.batch.id) + + ids = fixture.datasets.member_asset_ids(fixture.dataset.id) + + assert ids == {asset.id for asset in promoted} + # And the skipped one is genuinely out — `PROMOTABLE_PROGRESS` excludes it, + # which is the whole reason a count of 2 over a batch of 3 is not a failure. + assert fixture.assets[1] not in ids + + +def test_it_answers_the_same_set_assets_does_without_resolving_them(tmp_path: Path) -> None: + # The cheap half of `assets`, and it has to stay the same answer: two walks of + # `dataset_member` that disagree is exactly what putting both beside each + # other is meant to prevent. + fixture = Fixture(tmp_path) + fixture.completed(AssetProgress.ANNOTATED, AssetProgress.ANNOTATED, AssetProgress.ACCEPTED) + fixture.datasets.promote(fixture.batch.id) + + assert fixture.datasets.member_asset_ids(fixture.dataset.id) == { + asset.id for asset in fixture.datasets.assets(fixture.dataset.id) + } + + +def test_a_removed_asset_leaves_the_set(tmp_path: Path) -> None: + # Current membership, never a promotion log. "Is my work in the dataset" is a + # question about now, and a curator taking something out has answered it. + fixture = Fixture(tmp_path) + fixture.completed(AssetProgress.ANNOTATED, AssetProgress.ANNOTATED, AssetProgress.ANNOTATED) + fixture.datasets.promote(fixture.batch.id) + fixture.datasets.remove_asset(fixture.dataset.id, fixture.assets[0]) + + ids = fixture.datasets.member_asset_ids(fixture.dataset.id) + + assert fixture.assets[0] not in ids + assert len(ids) == 2 + + +def test_promoting_twice_does_not_double_the_membership(tmp_path: Path) -> None: + # Promotion is a union, so the second press moves nothing — which is the + # outcome a client could not tell from a failure, and the one this set makes + # reportable. + fixture = Fixture(tmp_path) + fixture.completed(AssetProgress.ANNOTATED, AssetProgress.ANNOTATED, AssetProgress.ANNOTATED) + fixture.datasets.promote(fixture.batch.id) + first = fixture.datasets.member_asset_ids(fixture.dataset.id) + + again = fixture.datasets.promote(fixture.batch.id) + + assert again == [] + assert fixture.datasets.member_asset_ids(fixture.dataset.id) == first + + +def test_an_unknown_dataset_is_refused(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + with pytest.raises(DatasetNotFound): + fixture.datasets.member_asset_ids(uuid4()) diff --git a/tests/server/test_batches.py b/tests/server/test_batches.py index ea80300b..4e2af6b9 100644 --- a/tests/server/test_batches.py +++ b/tests/server/test_batches.py @@ -18,9 +18,11 @@ from tests.server._flow import ( LANE, SIGN, + a_box, annotated_batch, asset_ids, batch_from_ingest, + dataset_of, project_with_schema, ) from tests.server._runner import RecordingRunner @@ -546,3 +548,104 @@ def test_a_completed_batchs_pin_is_history( assert response.status_code == 409 assert response.json()["code"] == "INVALID_TRANSITION" + + +# --- promotion, made observable (audit F5/F17) -------------------------------- +# +# Promotion is not a transition: the batch stays `completed` and nothing else on +# its read model moved when its assets entered the trunk. So a client could not +# tell "promoted 3 of 48" from "promoted nothing because it was already done" +# from "the press did nothing", and a working call read as a broken button. + + +def test_a_batch_nobody_promoted_reports_nothing_in_the_dataset( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + _, batch_id = annotated_batch(client, runner, tmp_path) + + body = client.get(f"/batches/{batch_id}").json() + + assert body["promoted_asset_count"] == 0 + assert body["asset_count"] == 3 + + +def test_promoting_moves_the_count_on_the_batch_itself( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + # The half that survives a reload. The response says what *this press* did and + # cannot be recovered afterwards; this says what is in the trunk *now*, and is + # still right in a session that did not do the promoting. + _, batch_id = annotated_batch(client, runner, tmp_path) + client.post(f"/batches/{batch_id}/promote") + + body = client.get(f"/batches/{batch_id}").json() + + assert body["promoted_asset_count"] == 3 + # And the batch has not moved, which is exactly why it needed a number. + assert body["state"] == "completed" + + +def test_the_count_leaves_out_a_frame_that_was_skipped( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + # `PROMOTABLE_PROGRESS` excludes `skipped`, so a count below `asset_count` is + # the ordinary shape rather than a shortfall — and it is the shape the founder + # actually had, at 3 of 48. + project_id = project_with_schema(client) + batch_id = batch_from_ingest(client, runner, tmp_path, project_id, images=3) + client.post(f"/batches/{batch_id}/approve") + client.post(f"/batches/{batch_id}/start") + job_id = client.get(f"/batches/{batch_id}/jobs").json()["items"][0]["id"] + client.post(f"/jobs/{job_id}/start") + assets = asset_ids(client, batch_id) + client.post(f"/jobs/{job_id}/annotations", json=[a_box(assets[0]), a_box(assets[1])]) + client.put(f"/jobs/{job_id}/assets/{assets[2]}/progress", json={"progress": "skipped"}) + client.post(f"/jobs/{job_id}/complete") + client.post(f"/batches/{batch_id}/complete") + client.post(f"/batches/{batch_id}/promote") + + body = client.get(f"/batches/{batch_id}").json() + + assert body["asset_count"] == 3 + assert body["promoted_asset_count"] == 2 + + +def test_promoting_twice_leaves_the_count_where_it_was( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + # The idempotent no-op, which is the outcome that looked most like a failure. + # The second press answers an empty page — and the count says the work is + # there anyway, which is what turns "nothing happened" into "already done". + _, batch_id = annotated_batch(client, runner, tmp_path) + client.post(f"/batches/{batch_id}/promote") + + again = client.post(f"/batches/{batch_id}/promote").json() + + assert again["total"] == 0 + assert client.get(f"/batches/{batch_id}").json()["promoted_asset_count"] == 3 + + +def test_the_listing_reports_it_too( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + # One read of the trunk covers every batch in the page — the reason `promoted` + # is passed into `BatchOut.of` rather than read inside it. + project_id, batch_id = annotated_batch(client, runner, tmp_path) + client.post(f"/batches/{batch_id}/promote") + + items = client.get(f"/projects/{project_id}/batches").json()["items"] + + assert [one["promoted_asset_count"] for one in items] == [3] + + +def test_removing_an_asset_from_the_trunk_takes_it_off_the_count( + client: TestClient, runner: RecordingRunner, tmp_path: Path +) -> None: + # Current membership, never a promotion log. + project_id, batch_id = annotated_batch(client, runner, tmp_path) + client.post(f"/batches/{batch_id}/promote") + dataset_id = dataset_of(client, project_id) + removed = asset_ids(client, batch_id)[0] + client.delete(f"/datasets/{dataset_id}/assets/{removed}") + + assert client.get(f"/batches/{batch_id}").json()["promoted_asset_count"] == 2