diff --git a/integration_tests/test/testDetailsDialog.spec.ts b/integration_tests/test/testDetailsDialog.spec.ts new file mode 100644 index 00000000..1b362a94 --- /dev/null +++ b/integration_tests/test/testDetailsDialog.spec.ts @@ -0,0 +1,166 @@ +import { expect } from "@playwright/test"; +import { test } from "fixtures"; +import { + TEST_BUILD_FAILED, + TEST_PROJECT, + TEST_RUN_NEW, + TEST_UNRESOLVED, +} from "~client/_test/test.data.helper"; +import { + API_URL, + mockGetBuildDetails, + mockGetBuilds, + mockGetProjects, + mockGetTestRuns, + mockImage, + mockTestRun, +} from "utils/mocks"; + +const project = TEST_PROJECT; +const build = TEST_BUILD_FAILED; + +test.beforeEach(async ({ page }) => { + await mockGetProjects(page, [project]); + await mockGetBuilds(page, project.id, [build]); + await mockGetBuildDetails(page, build); + await mockGetTestRuns(page, build.id, [TEST_UNRESOLVED, TEST_RUN_NEW]); + await mockTestRun(page, TEST_UNRESOLVED); + await mockTestRun(page, TEST_RUN_NEW); + await mockImage(page, "image.png"); + await mockImage(page, "diff.png"); + await mockImage(page, "baseline.png"); + await page.route(`${API_URL}/test-runs/approve?merge=false`, (route) => + route.fulfill({ status: 200, body: "[]" }), + ); +}); + +// The app anchors toasts bottom-centre, which is clear of both paginations on +// the list. The dialog puts its approve/reject bar in exactly that spot, so a +// toast there lands on the buttons and blocks the next screen's approval until +// it times out or is dismissed. +test("raises its toast clear of the approve buttons", async ({ + openProjectPage, + page, +}) => { + const projectPage = await openProjectPage( + project.id, + build.id, + TEST_UNRESOLVED.id, + ); + // by text, not by role name: the tooltip wrapper makes "Hotkey: A" the + // button's accessible name. Anchored so "Approve variations" cannot match. + const approve = page.locator("button").filter({ hasText: /^Approve$/ }); + await expect(approve).toBeVisible(); + const buttons = await approve.boundingBox(); + + await approve.click(); + + await expect(projectPage.notification.message).toBeVisible(); + const toast = await projectPage.notification.message.boundingBox(); + expect(toast.y + toast.height).toBeLessThanOrEqual(buttons.y); +}); + +// A screen now takes a couple of seconds to review and a toast lives five, so +// approving one after another piles them up over the checkpoint's header. +test("replaces its confirmation rather than stacking them up", async ({ + openProjectPage, + page, +}) => { + await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id); + const approve = page.locator("button").filter({ hasText: /^Approve$/ }); + + await approve.click(); + await approve.click(); + await approve.click(); + + await expect(page.getByText("Approved")).toHaveCount(1); +}); + +// notistack's default is five seconds, which outlives the screen the +// confirmation belongs to and leaves one sitting over the header for good. +test("lets the confirmation go before the next screen is reviewed", async ({ + openProjectPage, + page, +}) => { + await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id); + + await page + .locator("button") + .filter({ hasText: /^Approve$/ }) + .click(); + + await expect(page.getByText("Approved")).toBeVisible(); + // comfortably past a two-second toast, comfortably short of a five-second one + await expect(page.getByText("Approved")).toBeHidden({ timeout: 3500 }); +}); + +// Errors are not interchangeable the way the confirmations are: a failure must +// not be swallowed by whatever the reviewer does next. +test("lets an error outlive the confirmation that follows it", async ({ + openProjectPage, + page, +}) => { + await page.route(`${API_URL}/test-runs/reject`, (route) => + route.fulfill({ status: 500, body: JSON.stringify({ message: "nope" }) }), + ); + await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id); + + await page + .locator("button") + .filter({ hasText: /^Reject$/ }) + .click(); + await expect(page.getByText("nope")).toBeVisible(); + await page + .locator("button") + .filter({ hasText: /^Approve$/ }) + .click(); + + await expect(page.getByText("nope")).toBeVisible(); + await expect(page.getByText("Approved")).toBeVisible(); +}); + +// The dialog stays mounted as the reviewer steps between runs, so state that +// belongs to one screen has to be cleared with it. Draw mode was not: it rode +// along, and the next click on a screenshot quietly drew an ignore area there, +// marking a run touched that the reviewer never meant to edit — which then +// blocked the very navigation that would have cleared the flag. +test("leaves draw mode behind when moving to another run", async ({ + openProjectPage, + page, +}) => { + await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id); + // the toggle is an icon with no label, so it is located by the value MUI + // puts on the button element + const drawMode = page.locator('button[value="drawMode"]'); + + await drawMode.click(); + await expect(drawMode).toHaveAttribute("aria-pressed", "true"); + await page.keyboard.press("ArrowRight"); + + await expect(page).toHaveURL(new RegExp(`testId=${TEST_RUN_NEW.id}$`)); + await expect(drawMode).toHaveAttribute("aria-pressed", "false"); +}); + +// A rejection is not a success. Dressing it in the same green tick as an +// approval makes the two indistinguishable at a glance, which matters most +// when they sit next to each other and are pressed in a hurry. +test("does not dress a rejection up as a success", async ({ + openProjectPage, + page, +}) => { + await page.route(`${API_URL}/test-runs/reject`, (route) => + route.fulfill({ status: 200, body: "[]" }), + ); + await openProjectPage(project.id, build.id, TEST_UNRESOLVED.id); + + await page + .locator("button") + .filter({ hasText: /^Reject$/ }) + .click(); + + const toast = page.locator(".notistack-MuiContent", { + hasText: "Rejected", + }); + await expect(toast).toBeVisible(); + await expect(toast).not.toHaveClass(/notistack-MuiContent-success/); +}); diff --git a/src/components/TestDetailsDialog/ApproveRejectButtons.tsx b/src/components/TestDetailsDialog/ApproveRejectButtons.tsx index a222b662..ffef0b07 100644 --- a/src/components/TestDetailsDialog/ApproveRejectButtons.tsx +++ b/src/components/TestDetailsDialog/ApproveRejectButtons.tsx @@ -1,5 +1,5 @@ import { Chip, Button } from "@mui/material"; -import { useSnackbar } from "notistack"; +import { useDialogSnackbar } from "./useDialogSnackbar"; import { useHotkeys } from "react-hotkeys-hook"; import React from "react"; import { testRunService } from "../../services"; @@ -32,7 +32,7 @@ export const ApproveRejectButtons: React.FunctionComponent<{ afterReject?: () => void; onOpenVariations: (mode: MatchingVariationsMode) => void; }> = ({ testRun, afterApprove, afterReject, onOpenVariations }) => { - const { enqueueSnackbar } = useSnackbar(); + const { enqueueSnackbar } = useDialogSnackbar(); const classes = useStyles(); const { selectedProjectId, projectList } = useProjectState(); const { testRuns } = useTestRunState(); @@ -73,8 +73,11 @@ export const ApproveRejectButtons: React.FunctionComponent<{ testRunService .rejectBulk([testRun.id]) .then(() => { + // not a success: a rejection is a deliberate outcome, and the green + // tick made it read as an approval at a glance — the two buttons sit + // next to each other and get pressed in a hurry enqueueSnackbar("Rejected", { - variant: "success", + variant: "info", }); afterReject && afterReject(); }) diff --git a/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx b/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx index e43151de..b901c212 100644 --- a/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx +++ b/src/components/TestDetailsDialog/MatchingVariationsDialog.tsx @@ -23,7 +23,7 @@ import CloseIcon from "@mui/icons-material/Close"; import ChevronLeftIcon from "@mui/icons-material/ChevronLeft"; import ChevronRightIcon from "@mui/icons-material/ChevronRight"; import { makeStyles } from "@mui/styles"; -import { useSnackbar } from "notistack"; +import { useDialogSnackbar } from "./useDialogSnackbar"; import { useNavigate } from "react-router"; import { Tooltip } from "../Tooltip"; import { testRunService, staticService } from "../../services"; @@ -152,7 +152,7 @@ export const MatchingVariationsDialog: React.FunctionComponent<{ onClose: () => void; }> = ({ mode, testRun, groupBy = "customTags", onClose }) => { const classes = useStyles(); - const { enqueueSnackbar } = useSnackbar(); + const { enqueueSnackbar } = useDialogSnackbar(); const navigate = useNavigate(); const { testRuns: allTestRuns, filteredSortedTestRunIds } = useTestRunState(); const { selectedBuild } = useBuildState(); @@ -399,7 +399,9 @@ export const MatchingVariationsDialog: React.FunctionComponent<{ ids.length } variations`, { - variant: "success", + // green for an approval only: a rejection is a deliberate outcome, + // not a success, and must not read as its opposite + variant: mode === "approve" ? "success" : "info", }, ); }) diff --git a/src/components/TestDetailsDialog/TestDetailsModal.tsx b/src/components/TestDetailsDialog/TestDetailsModal.tsx index e614ee5c..8d2f1343 100644 --- a/src/components/TestDetailsDialog/TestDetailsModal.tsx +++ b/src/components/TestDetailsDialog/TestDetailsModal.tsx @@ -47,7 +47,7 @@ import { routes, GO_TO_NEXT_KEY } from "../../constants"; import { useTestRunDispatch, useProjectState } from "../../contexts"; import { DrawArea, ImageStateLoad } from "./DrawArea"; import { CommentsPopper } from "../CommentsPopper"; -import { useSnackbar } from "notistack"; +import { useDialogSnackbar } from "./useDialogSnackbar"; import { ApproveRejectButtons } from "./ApproveRejectButtons"; import { MatchingVariationsDialog, @@ -137,7 +137,7 @@ const TestDetailsModal: React.FunctionComponent = ({ handleClose, }) => { const classes = useStyles(); - const { enqueueSnackbar } = useSnackbar(); + const { enqueueSnackbar } = useDialogSnackbar(); const testRunDispatch = useTestRunDispatch(); const { selectedProjectId, projectList } = useProjectState(); const project = projectList.find((item) => item.id === selectedProjectId); @@ -234,12 +234,19 @@ const TestDetailsModal: React.FunctionComponent = ({ resetPosition(); }; - // the fade and the blend belong to the run being looked at, like the diff - // does: a half-faded image carried onto the next screenshot would hide it + // the fade, the blend and the drawing tools belong to the run being looked + // at, like the diff does: a half-faded image carried onto the next + // screenshot would hide it, and draw mode carried over turned the reviewer's + // next click into an ignore area on a screen they never meant to edit — + // which marked that run touched and then blocked the very navigation that + // would have cleared the flag. This dialog is not remounted between runs, so + // whatever belongs to one screen has to be cleared here. useEffect(() => { setIsDiffShown(!!testRun.diffName); setOverlayOpacity(1); setBlendDifference(false); + setIsDrawMode(false); + setSelectedRectId(undefined); }, [testRun.id, testRun.diffName]); useEffect(() => { diff --git a/src/components/TestDetailsDialog/useDialogSnackbar.ts b/src/components/TestDetailsDialog/useDialogSnackbar.ts new file mode 100644 index 00000000..e9626212 --- /dev/null +++ b/src/components/TestDetailsDialog/useDialogSnackbar.ts @@ -0,0 +1,60 @@ +import { useCallback, useRef } from "react"; +import { + OptionsObject, + SnackbarKey, + SnackbarMessage, + useSnackbar, +} from "notistack"; + +/** + * `useSnackbar` for the details dialog: anchored to the top, and showing one + * confirmation at a time. + * + * The app anchors toasts bottom-centre, which on the test run list is the one + * strip clear of both paginations. This dialog is fullscreen and puts its + * approve/reject bar in exactly that spot, so a toast raised here would land on + * the buttons the reviewer is about to press. + * + * Reviewing a screen now takes a couple of seconds and a toast lives five, so + * approving one after another piled them up over the checkpoint's header. + * Confirmations are interchangeable — the reviewer only needs to know the last + * action went through — so a new one takes the place of the one before it. + * Errors are never replaced: a failure must not be swallowed by whatever the + * reviewer happens to do next. + * + * Callers may still override the anchor per message. + */ +export const useDialogSnackbar = () => { + const { enqueueSnackbar, closeSnackbar } = useSnackbar(); + const lastConfirmation = useRef(null); + + const enqueue = useCallback( + (message: SnackbarMessage, options?: OptionsObject) => { + // anything that is not an error: an approval and a rejection are both + // just "the last action went through", and neither needs to outlive the + // other on screen + const isConfirmation = options?.variant !== "error"; + if (isConfirmation && lastConfirmation.current !== null) { + // a no-op once that toast has timed out on its own + closeSnackbar(lastConfirmation.current); + } + + const key = enqueueSnackbar(message, { + anchorOrigin: { vertical: "top", horizontal: "center" }, + // notistack's five seconds outlives the screen a confirmation belongs + // to, so one sat over the header permanently. Errors keep the default: + // they are worth reading, and there is no next screen waiting on them. + ...(isConfirmation ? { autoHideDuration: 2000 } : {}), + ...options, + }); + + if (isConfirmation) { + lastConfirmation.current = key; + } + return key; + }, + [enqueueSnackbar, closeSnackbar], + ); + + return { enqueueSnackbar: enqueue, closeSnackbar }; +};