diff --git a/e2e/__screenshots__/vendors/vendors.visual.spec.ts/vendor-detail.png b/e2e/__screenshots__/vendors/vendors.visual.spec.ts/vendor-detail.png index 1beb0a15..1993c2c7 100644 Binary files a/e2e/__screenshots__/vendors/vendors.visual.spec.ts/vendor-detail.png and b/e2e/__screenshots__/vendors/vendors.visual.spec.ts/vendor-detail.png differ diff --git a/e2e/vendors/vendors.spec.ts b/e2e/vendors/vendors.spec.ts index b348fd21..d1263828 100644 --- a/e2e/vendors/vendors.spec.ts +++ b/e2e/vendors/vendors.spec.ts @@ -68,6 +68,7 @@ interface MockState { patchedBody?: Record; patchedCompanyId?: string; deletedId?: string; + deleteConfirmedOpenWorkOrders?: boolean; } async function fulfillJson(route: Route, body: unknown, status = 200) { @@ -307,8 +308,9 @@ async function mockVendorApi( }, }), ); - await page.route(/\/api\/vendors\/\d+$/, async (route) => { - const id = route.request().url().split("/").pop() ?? ""; + await page.route(/\/api\/vendors\/\d+(\?.*)?$/, async (route) => { + const requestUrl = new URL(route.request().url()); + const id = requestUrl.pathname.split("/").pop() ?? ""; if (route.request().method() === "PUT") { state.updatedBody = route.request().postDataJSON(); const vendor = vendorRecords.find((item) => String(item.Id) === id); @@ -321,6 +323,8 @@ async function mockVendorApi( return; } if (route.request().method() === "DELETE") { + state.deleteConfirmedOpenWorkOrders = + requestUrl.searchParams.get("confirmOpenWorkOrders") === "true"; if (options.deleteConflict) { await fulfillJson( route, @@ -576,20 +580,28 @@ test.describe("Vendor directory prototype parity", () => { await expect(page.getByRole("button", { name: "Close drawer" })).toHaveCount(0); }); - test("blocks deactivation for linked work orders and preserves the vendor on a raced 409", async ({ + test("confirms deactivation past linked work orders and preserves the vendor on a raced 409", async ({ page, }) => { - const blockedState = await mockVendorApi(page, { deactivationBlocked: true }); + const confirmState = await mockVendorApi(page, { deactivationBlocked: true }); await page.goto("/vendors"); await page.getByRole("button", { name: "Edit vendor Gateway Plumbing" }).click(); await page.getByRole("switch", { name: "Active status" }).click(); - const blockedDialog = page.getByRole("dialog", { name: "Deactivate Vendor" }); - await expect(blockedDialog).toContainText("WO-501 — Emergency boiler repair"); - await expect(blockedDialog.getByRole("button", { name: /^Deactivate$/ })).toBeDisabled(); - expect(blockedState.deletedId).toBeUndefined(); + const dialog = page.getByRole("dialog", { name: "Deactivate this vendor?" }); + await expect(dialog).toContainText("It still has 1 open work order"); + await expect( + dialog.getByRole("link", { name: /WO-501 — Emergency boiler repair/ }), + ).toHaveAttribute("href", "/workorders/501"); + + // SH-254: the open work orders inform the decision, they no longer block it. + const confirm = dialog.getByRole("button", { name: "Deactivate anyway" }); + await expect(confirm).toBeEnabled(); + await confirm.click(); + + await expect.poll(() => confirmState.deletedId).toBe("1"); + expect(confirmState.deleteConfirmedOpenWorkOrders).toBe(true); - await blockedDialog.getByRole("button", { name: "Cancel" }).click(); await page.unrouteAll({ behavior: "wait" }); const racedState = await mockVendorApi(page, { deleteConflict: true }); @@ -597,13 +609,13 @@ test.describe("Vendor directory prototype parity", () => { await page.getByRole("button", { name: "Edit vendor Gateway Plumbing" }).click(); await page.getByRole("switch", { name: "Active status" }).click(); await page - .getByRole("dialog", { name: "Deactivate Vendor" }) + .getByRole("dialog", { name: "Deactivate this vendor?" }) .getByRole("button", { name: /^Deactivate$/, }) .click(); - await expect(page.getByRole("dialog", { name: "Deactivate Vendor" })).toContainText( + await expect(page.getByRole("dialog", { name: "Deactivate this vendor?" })).toContainText( /open work orders|conflict/i, ); expect(racedState.deletedId).toBeUndefined(); diff --git a/src/app/(protected)/vendors/_components/use-vendor-deactivation.ts b/src/app/(protected)/vendors/_components/use-vendor-deactivation.ts index b63291be..5870f7db 100644 --- a/src/app/(protected)/vendors/_components/use-vendor-deactivation.ts +++ b/src/app/(protected)/vendors/_components/use-vendor-deactivation.ts @@ -38,13 +38,18 @@ export function useVendorDeactivation(onSuccess?: () => void): VendorDeactivatio const confirm = () => { if (!target || target.id == null) return; setError(null); - deleteVendor.mutate(target.id, { - onSuccess: () => { - setTarget(null); - onSuccess?.(); + // The dialog has shown whatever open work orders exist, so confirming here is + // the explicit confirmation the API requires to deactivate past them (SH-254). + deleteVendor.mutate( + { id: target.id, confirmOpenWorkOrders: (impact?.openWorkOrders.length ?? 0) > 0 }, + { + onSuccess: () => { + setTarget(null); + onSuccess?.(); + }, + onError: (err: Error) => setError(err.message || "Failed to deactivate vendor"), }, - onError: (err: Error) => setError(err.message || "Failed to deactivate vendor"), - }); + ); }; return { diff --git a/src/app/(protected)/vendors/_components/vendor-deactivation-dialog.tsx b/src/app/(protected)/vendors/_components/vendor-deactivation-dialog.tsx index cf673b6c..a56ca3b5 100644 --- a/src/app/(protected)/vendors/_components/vendor-deactivation-dialog.tsx +++ b/src/app/(protected)/vendors/_components/vendor-deactivation-dialog.tsx @@ -8,11 +8,13 @@ import { DialogContent, DialogContentText, DialogTitle, + Link, List, ListItem, Stack, Typography, } from "@mui/material"; +import { Link as RouterLink } from "react-router"; import type { VendorDeactivationImpact, VendorListItem } from "@/domain/vendors/types/vendor"; interface VendorDeactivationDialogProps { @@ -26,6 +28,15 @@ interface VendorDeactivationDialogProps { onConfirm: () => void; } +function deactivationMessage(companyName: string | undefined, openCount: number): string { + const name = companyName ? `"${companyName}"` : "This vendor"; + if (openCount === 0) { + return `${name} will no longer be selectable for new work orders.`; + } + const plural = openCount === 1 ? "work order" : "work orders"; + return `${name} will no longer be selectable for new work orders. It still has ${openCount} open ${plural} — they'll keep it as-is unless you reassign them.`; +} + export function VendorDeactivationDialog({ target, isLoading, @@ -36,16 +47,16 @@ export function VendorDeactivationDialog({ onClose, onConfirm, }: VendorDeactivationDialogProps) { - const hasBlockingImpact = Boolean(impact && !impact.canDeactivate); + const openWorkOrders = impact?.openWorkOrders ?? []; + const hasOpenWorkOrders = openWorkOrders.length > 0; return ( - Deactivate Vendor + Deactivate this vendor? - Deactivate "{target?.companyName}"? Existing work-order and audit history will - be preserved. + {deactivationMessage(target?.companyName, openWorkOrders.length)} {isLoading && ( @@ -63,19 +74,13 @@ export function VendorDeactivationDialog({ )} - {impact != null && !isLoading && !impact.canDeactivate && ( - - This vendor cannot be deactivated because it still has open work orders. - - )} - - {impact != null && impact.openWorkOrders.length > 0 && ( + {hasOpenWorkOrders && ( - Open work orders ({impact.openWorkOrders.length}) + Open work orders ({openWorkOrders.length}) - {impact.openWorkOrders.map((wo) => ( + {openWorkOrders.map((wo) => ( - + {wo.workOrderNumber ? `${wo.workOrderNumber} — ${wo.workOrderTitle || "Untitled"}` : wo.workOrderTitle || `Work order ${wo.workOrderId}`} - + {Boolean(wo.status) && ( {wo.status} @@ -113,12 +118,12 @@ export function VendorDeactivationDialog({ Cancel diff --git a/src/app/(protected)/vendors/_components/vendor-detail-drawer.tsx b/src/app/(protected)/vendors/_components/vendor-detail-drawer.tsx index 74d6ef02..b1a24c3c 100644 --- a/src/app/(protected)/vendors/_components/vendor-detail-drawer.tsx +++ b/src/app/(protected)/vendors/_components/vendor-detail-drawer.tsx @@ -17,6 +17,7 @@ import { Switch, } from "@mui/material"; import { VendorRosterConflictAlert } from "./vendor-roster-conflict-alert"; +import { VendorStatusBadge } from "./vendor-status-badge"; import { VendorRosterFormFields } from "./vendor-roster-form-fields"; import { VendorRosterLoadErrorAlert } from "./vendor-roster-load-error"; import { useVendorRosterForm } from "./use-vendor-roster-form"; @@ -81,6 +82,13 @@ function DrawerHeader({ technician?: VendorRosterTechnician; onClose: () => void; }) { + const title = technician?.contactName || vendor?.contactName || roster?.name || "Vendor company"; + const company = roster?.name || vendor?.companyName || "Vendor details"; + // The company is a subtitle under the technician's name. When there is no + // technician the title already falls back to the company, and repeating it + // would render the same line twice. + const subtitle = company === title ? null : company; + return ( - {technician?.contactName || vendor?.contactName || roster?.name || "Vendor company"} - - - {roster?.name || vendor?.companyName || "Vendor details"} + {title} + {subtitle !== null && ( + + {subtitle} + + )} @@ -118,17 +128,24 @@ function CompanySection({ roster }: { roster: VendorCompanyRoster }) { - {Boolean(roster.googleMapsUrl) && ( - - - Open in Google Maps - - )} + + + Google Maps + + {roster.googleMapsUrl ? ( + + + Open in Google Maps + + ) : ( + — + )} + ); } @@ -154,23 +171,14 @@ function TechnicianSection({ technician }: { technician: VendorRosterTechnician )} - + {technician.totalJobs ?? 0} Total Jobs - - - Status - - - + @@ -254,10 +262,13 @@ function DrawerEditor({ const selectedIndex = technicians.findIndex( (technician) => technician.id != null && String(technician.id) === String(vendor.id), ); - const selectedTotalJobs = - roster?.technicians.find( - (technician) => technician.id != null && String(technician.id) === String(vendor.id), - )?.totalJobs ?? vendor.totalJobs; + const persistedTechnician = roster?.technicians.find( + (technician) => technician.id != null && String(technician.id) === String(vendor.id), + ); + const selectedTotalJobs = persistedTechnician?.totalJobs ?? vendor.totalJobs; + // Deactivation is a property of the saved vendor, not of the toggle: flipping an + // inactive vendor on and back off without saving must not ask to deactivate it. + const persistedIsActive = persistedTechnician?.isActive ?? vendor.isActive; if (form.isLoading) { return ; @@ -310,7 +321,7 @@ function DrawerEditor({ checked={Boolean(field.value)} slotProps={{ input: { "aria-label": "Active status" } }} onChange={(_event, checked) => { - if (field.value && !checked) onRequestDeactivation(vendor); + if (persistedIsActive && !checked) onRequestDeactivation(vendor); else field.onChange(checked); }} /> diff --git a/src/app/(protected)/vendors/_components/vendor-status-badge.tsx b/src/app/(protected)/vendors/_components/vendor-status-badge.tsx new file mode 100644 index 00000000..7fee8970 --- /dev/null +++ b/src/app/(protected)/vendors/_components/vendor-status-badge.tsx @@ -0,0 +1,20 @@ +import { Box, Stack } from "@mui/material"; +import { Text } from "@/components/ui/text"; + +/** Dot plus text, so the status never reads by colour alone. */ +export function VendorStatusBadge({ isActive }: { isActive: boolean }) { + return ( + + + {isActive ? "Active" : "Inactive"} + + ); +} diff --git a/src/app/(protected)/vendors/_components/vendors-table.tsx b/src/app/(protected)/vendors/_components/vendors-table.tsx index db588cff..e524630c 100644 --- a/src/app/(protected)/vendors/_components/vendors-table.tsx +++ b/src/app/(protected)/vendors/_components/vendors-table.tsx @@ -18,6 +18,7 @@ import { TableRow, Tooltip, } from "@mui/material"; +import { VendorStatusBadge } from "./vendor-status-badge"; import { Text } from "@/components/ui/text"; import type { VendorListItem } from "@/domain/vendors/types/vendor"; @@ -50,23 +51,6 @@ function stopPropagation(event: MouseEvent): void { event.stopPropagation(); } -function VendorStatus({ isActive }: { isActive: boolean }) { - return ( - - - {isActive ? "Active" : "Inactive"} - - ); -} - interface VendorTableRowProps { row: VendorListItem; onOpenDetail: (row: VendorListItem) => void; @@ -177,7 +161,9 @@ function VendorTableRow({ row, onOpenDetail, onOpenEdit }: VendorTableRowProps) {row.totalJobs ?? 0} - + + + => { - await apiDelete(`${API_PATHS.rest.vendors}/${id}`); + // SH-254: confirmOpenWorkOrders tells the API the caller has been shown the + // vendor's open work orders and chose to proceed. Without it the API still blocks. + delete: async (id: string | number, confirmOpenWorkOrders = false): Promise => { + const suffix = confirmOpenWorkOrders ? "?confirmOpenWorkOrders=true" : ""; + await apiDelete(`${API_PATHS.rest.vendors}/${id}${suffix}`); }, getDeactivationImpact: async (id: string | number): Promise => { diff --git a/src/domain/vendors/use-cases/use-delete-vendor.ts b/src/domain/vendors/use-cases/use-delete-vendor.ts index 0457cc7e..17f8ae11 100644 --- a/src/domain/vendors/use-cases/use-delete-vendor.ts +++ b/src/domain/vendors/use-cases/use-delete-vendor.ts @@ -3,11 +3,17 @@ import { toast } from "react-toastify"; import { vendorsApi } from "@/domain/vendors/api/vendors-api"; import { queryKeys } from "@/infra/query-key/query-key"; -export function useDeleteVendor(): UseMutationResult { +export interface DeleteVendorInput { + id: string | number; + confirmOpenWorkOrders?: boolean; +} + +export function useDeleteVendor(): UseMutationResult { const queryClient = useQueryClient(); return useMutation({ - mutationFn: (id: string | number) => vendorsApi.delete(id), + mutationFn: ({ id, confirmOpenWorkOrders = false }: DeleteVendorInput) => + vendorsApi.delete(id, confirmOpenWorkOrders), onSuccess: () => { void queryClient.invalidateQueries({ queryKey: queryKeys.vendors.all }); toast.success("Vendor deactivated"); diff --git a/src/test/app/(protected)/vendors/vendor-detail-drawer.test.tsx b/src/test/app/(protected)/vendors/vendor-detail-drawer.test.tsx index df329a4a..4e52aa7f 100644 --- a/src/test/app/(protected)/vendors/vendor-detail-drawer.test.tsx +++ b/src/test/app/(protected)/vendors/vendor-detail-drawer.test.tsx @@ -1,4 +1,5 @@ import { screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; import { describe, expect, it, vi } from "vitest"; import { VendorDetailDrawer } from "@/app/(protected)/vendors/_components/vendor-detail-drawer"; import { renderWithProviders } from "@/test/test-utils"; @@ -134,3 +135,139 @@ describe("VendorDetailDrawer selected-technician display", () => { expect(screen.queryByRole("switch", { name: "Active status" })).not.toBeInTheDocument(); }); }); + +describe("VendorDetailDrawer design parity", () => { + it("renders the company once when there is no technician name to head the panel", () => { + useVendorCompanyRoster.mockReturnValue(rosterWith([])); + + renderWithProviders( + , + { route: "/vendors", withAuth: false }, + ); + + const heading = screen.getByRole("heading", { level: 2, name: "Gateway Plumbing" }); + expect(heading).toBeInTheDocument(); + // The header block holds the title alone — no subtitle repeating the company. + expect(heading.parentElement?.children).toHaveLength(1); + }); + + it("keeps the company subtitle under a technician name", () => { + useVendorCompanyRoster.mockReturnValue( + rosterWith([{ id: 1, contactName: "Adam", phone: "", isActive: true, totalJobs: 5 }]), + ); + + renderWithProviders(, { + route: "/vendors", + withAuth: false, + }); + + expect(screen.getByRole("heading", { level: 2, name: "Adam" })).toBeInTheDocument(); + expect(screen.getAllByText("Gateway Plumbing").length).toBeGreaterThan(0); + }); + + it("labels the Google Maps field below Address even when no URL is stored", () => { + useVendorCompanyRoster.mockReturnValue( + rosterWith([{ id: 1, contactName: "Adam", phone: "", isActive: true, totalJobs: 5 }]), + ); + + renderWithProviders(, { + route: "/vendors", + withAuth: false, + }); + + expect(screen.getByText("Google Maps")).toBeInTheDocument(); + expect(screen.queryByRole("link", { name: /Open in Google Maps/ })).not.toBeInTheDocument(); + }); + + it("links out to Google Maps when a URL is stored", () => { + const roster = rosterWith([ + { id: 1, contactName: "Adam", phone: "", isActive: true, totalJobs: 5 }, + ]); + roster.data.googleMapsUrl = "https://maps.google.com/?q=1+Industrial+Pkwy"; + useVendorCompanyRoster.mockReturnValue(roster); + + renderWithProviders(, { + route: "/vendors", + withAuth: false, + }); + + expect(screen.getByRole("link", { name: /Open in Google Maps/ })).toHaveAttribute( + "href", + "https://maps.google.com/?q=1+Industrial+Pkwy", + ); + }); + + it("puts the status opposite Total Jobs and drops the redundant Status label", () => { + useVendorCompanyRoster.mockReturnValue( + rosterWith([{ id: 1, contactName: "Adam", phone: "", isActive: true, totalJobs: 5 }]), + ); + + renderWithProviders(, { + route: "/vendors", + withAuth: false, + }); + + const row = screen.getByText("Total Jobs").closest("div")?.parentElement; + expect(row).not.toBeNull(); + expect(getComputedStyle(row as Element).justifyContent).toBe("space-between"); + expect(row).toHaveTextContent("Active"); + expect(screen.queryByText("Status")).not.toBeInTheDocument(); + }); +}); + +describe("VendorDetailDrawer deactivation prompt", () => { + const inactiveVendor: VendorListItem = { ...vendor, isActive: false }; + + it("does not prompt to deactivate a saved-inactive vendor toggled on and back off", async () => { + useVendorCompanyRoster.mockReturnValue( + rosterWith([ + { id: 1, contactName: "Adam", phone: "314-555-0198", isActive: false, totalJobs: 5 }, + ]), + ); + const onRequestDeactivation = vi.fn(); + + renderWithProviders( + , + { route: "/vendors", withAuth: false }, + ); + + const toggle = screen.getByRole("switch", { name: "Active status" }); + await userEvent.click(toggle); + expect(toggle).toBeChecked(); + + await userEvent.click(toggle); + + expect(toggle).not.toBeChecked(); + expect(onRequestDeactivation).not.toHaveBeenCalled(); + }); + + it("prompts to deactivate a saved-active vendor when the toggle is switched off", async () => { + useVendorCompanyRoster.mockReturnValue( + rosterWith([ + { id: 1, contactName: "Adam", phone: "314-555-0198", isActive: true, totalJobs: 5 }, + ]), + ); + const onRequestDeactivation = vi.fn(); + + renderWithProviders( + , + { route: "/vendors", withAuth: false }, + ); + + await userEvent.click(screen.getByRole("switch", { name: "Active status" })); + + expect(onRequestDeactivation).toHaveBeenCalledWith(vendor); + }); +}); diff --git a/src/test/app/(protected)/vendors/vendors-list.test.tsx b/src/test/app/(protected)/vendors/vendors-list.test.tsx index cbfa47f9..ae5a18d3 100644 --- a/src/test/app/(protected)/vendors/vendors-list.test.tsx +++ b/src/test/app/(protected)/vendors/vendors-list.test.tsx @@ -177,7 +177,7 @@ describe("VendorsListPage", () => { expect(screen.getByRole("heading", { level: 2, name: "Adam Whyte" })).toBeInTheDocument(); }); - it("blocks deactivation when the preflight reports open work orders", async () => { + it("offers deactivate-anyway when the preflight reports open work orders", async () => { setupDefaults(); useVendorCompanyRoster.mockReturnValue({ data: activeRoster, @@ -213,13 +213,52 @@ describe("VendorsListPage", () => { await userEvent.click(screen.getByRole("switch", { name: "Active status" })); expect( - screen.getByText(/cannot be deactivated because it still has open work orders/), + screen.getByText( + /will no longer be selectable for new work orders\. It still has 1 open work order —/, + ), ).toBeInTheDocument(); - expect(screen.getByText(/Boiler repair/)).toBeInTheDocument(); - expect(screen.getByRole("button", { name: /^Deactivate$/ })).toBeDisabled(); - expect(mutate).not.toHaveBeenCalled(); + expect(screen.getByRole("link", { name: /Boiler repair/ })).toHaveAttribute( + "href", + "/workorders/101", + ); + + const confirm = screen.getByRole("button", { name: "Deactivate anyway" }); + expect(confirm).toBeEnabled(); + await userEvent.click(confirm); + + expect(mutate).toHaveBeenCalledWith({ id: 1, confirmOpenWorkOrders: true }, expect.anything()); }); + it("deactivates without the confirmation flag when nothing is linked", async () => { + setupDefaults(); + useVendorCompanyRoster.mockReturnValue({ + data: activeRoster, + isLoading: false, + isError: false, + error: null, + refetch: vi.fn(), + }); + useVendorDeactivationImpact.mockReturnValue({ + data: { vendorId: 1, canDeactivate: true, openWorkOrders: [] }, + isLoading: false, + error: null, + }); + useVendorsList.mockImplementation((params: { isActive?: boolean; pageSize?: number }) => { + if (params.pageSize === 1) return result([], 1); + return params.isActive ? result([activeVendor], 1) : result([], 0); + }); + + renderWithProviders(, { route: "/vendors", withAuth: false }); + + await userEvent.click(screen.getByRole("button", { name: "Edit vendor Gateway Plumbing" })); + await userEvent.click(screen.getByRole("switch", { name: "Active status" })); + + expect(screen.queryByText(/It still has/)).not.toBeInTheDocument(); + await userEvent.click(screen.getByRole("button", { name: /^Deactivate$/ })); + + expect(mutate).toHaveBeenCalledWith({ id: 1, confirmOpenWorkOrders: false }, expect.anything()); + }, 10_000); + it("preserves inline edits when deactivation is cancelled", async () => { setupDefaults(); useVendorCompanyRoster.mockReturnValue({ @@ -247,7 +286,7 @@ describe("VendorsListPage", () => { await userEvent.type(company, "Draft Company Name"); await userEvent.click(screen.getByRole("switch", { name: "Active status" })); await userEvent.click( - within(screen.getByRole("dialog", { name: "Deactivate Vendor" })).getByRole("button", { + within(screen.getByRole("dialog", { name: "Deactivate this vendor?" })).getByRole("button", { name: "Cancel", }), );