fix(work-orders): address PR review on toast feedback

Scope vendor and save toasts to successful assigned-vendor and slide-over saves.
Bind patch success callbacks per operation instead of shared mutate observers.
This commit is contained in:
Arthur Bassi 2026-08-26 19:30:29 -03:00
parent 97b5e287aa
commit 8d755bcb11
8 changed files with 146 additions and 44 deletions

View file

@ -7,7 +7,7 @@ import {
buildSlideOverPatch, buildSlideOverPatch,
isSlideOverDraftDirty, isSlideOverDraftDirty,
} from "@/domain/work-orders/utils/slide-over-draft"; } from "@/domain/work-orders/utils/slide-over-draft";
import { notifyVendorAssignedIfPatched } from "@/domain/work-orders/utils/work-order-feedback-toasts"; import { notifySlideOverSaveSuccess } from "@/domain/work-orders/utils/work-order-feedback-toasts";
import { import {
isWorkOrderCoreLocked, isWorkOrderCoreLocked,
isWorkOrderFullyLocked, isWorkOrderFullyLocked,
@ -117,7 +117,7 @@ export function useSlideOverEditState({
} }
onSave(row.id, patch, { onSave(row.id, patch, {
onSuccess: () => { onSuccess: () => {
notifyVendorAssignedIfPatched(patch); notifySlideOverSaveSuccess(patch);
setBaseline(activeDraft); setBaseline(activeDraft);
setEditing(false); setEditing(false);
setShowUnsaved(false); setShowUnsaved(false);

View file

@ -159,12 +159,9 @@ export function useWorkOrderTableMutations(
}); });
const patchField: WorkOrderTablePatchFn = (id, patch, options) => { const patchField: WorkOrderTablePatchFn = (id, patch, options) => {
patchMutation.mutate( void patchMutation.mutateAsync({ id, patch }).then(
{ id, patch }, () => options?.onSuccess?.(),
{ (error: Error) => options?.onError?.(error),
onSuccess: () => options?.onSuccess?.(),
onError: (error) => options?.onError?.(error),
},
); );
}; };

View file

@ -16,14 +16,14 @@ export function workOrderCanceledMessage(woNumber: string) {
return `Work order #${woNumber} canceled.`; return `Work order #${woNumber} canceled.`;
} }
/**
* AAP Work Orders board has no vendor-assign toast (bundle audit 2026-08-26).
* SH-119 names that silence as the gap to close; this copy is WO-specific.
*/
export function vendorAssignedMessage() { export function vendorAssignedMessage() {
return "Vendor assigned"; return "Vendor assigned";
} }
export function workOrderSavedMessage() {
return "Work order saved successfully";
}
export function notifyWorkOrderCanceled(woNumber: string) { export function notifyWorkOrderCanceled(woNumber: string) {
toast.warning(workOrderCanceledMessage(woNumber)); toast.warning(workOrderCanceledMessage(woNumber));
} }
@ -32,12 +32,26 @@ export function notifyVendorAssigned() {
toast.success(vendorAssignedMessage()); toast.success(vendorAssignedMessage());
} }
export function isVendorAssignmentPatch(patch: WorkOrderTablePatch): boolean { export function notifyWorkOrderSaved() {
return Object.prototype.hasOwnProperty.call(patch, "vendorId"); toast.success(workOrderSavedMessage());
} }
export function notifyVendorAssignedIfPatched(patch: WorkOrderTablePatch) { function isAssignedVendorId(vendorId: unknown): boolean {
if (isVendorAssignmentPatch(patch)) notifyVendorAssigned(); return vendorId != null && String(vendorId).trim() !== "";
}
export function isVendorAssignmentPatch(patch: WorkOrderTablePatch): boolean {
return (
Object.prototype.hasOwnProperty.call(patch, "vendorId") && isAssignedVendorId(patch.vendorId)
);
}
export function notifySlideOverSaveSuccess(patch: WorkOrderTablePatch) {
if (isVendorAssignmentPatch(patch)) {
notifyVendorAssigned();
return;
}
notifyWorkOrderSaved();
} }
export function patchWorkOrderAsCanceled( export function patchWorkOrderAsCanceled(
@ -53,7 +67,6 @@ export function patchWorkOrderAsCanceled(
); );
} }
/** AAP completes via document generate toast, not a dedicated Complete WO toast. */
export function patchWorkOrderAsCompleted(patchField: PersistPatchFn, id: string | number) { export function patchWorkOrderAsCompleted(patchField: PersistPatchFn, id: string | number) {
patchField(id, { status: "Completed" }); patchField(id, { status: "Completed" });
} }
@ -63,5 +76,9 @@ export function applyVendorTableSave(
id: string | number, id: string | number,
fields: VendorAssignmentFields, fields: VendorAssignmentFields,
) { ) {
onPatch(id, toVendorTablePatch(fields), { onSuccess: notifyVendorAssigned }); onPatch(
id,
toVendorTablePatch(fields),
isAssignedVendorId(fields.vendorId) ? { onSuccess: notifyVendorAssigned } : undefined,
);
} }

View file

@ -3,7 +3,10 @@ import { beforeEach, describe, expect, it, vi } from "vitest";
import { toast } from "react-toastify"; import { toast } from "react-toastify";
import { useSlideOverEditState } from "@/app/(protected)/workorders/_components/detail/use-slide-over-edit-state"; import { useSlideOverEditState } from "@/app/(protected)/workorders/_components/detail/use-slide-over-edit-state";
import type { WorkOrderTableRow } from "@/domain/work-orders/types/work-order-table-row"; import type { WorkOrderTableRow } from "@/domain/work-orders/types/work-order-table-row";
import { vendorAssignedMessage } from "@/domain/work-orders/utils/work-order-feedback-toasts"; import {
vendorAssignedMessage,
workOrderSavedMessage,
} from "@/domain/work-orders/utils/work-order-feedback-toasts";
vi.mock("react-toastify", () => ({ vi.mock("react-toastify", () => ({
toast: { toast: {
@ -138,9 +141,10 @@ describe("useSlideOverEditState completed lock", () => {
options.onSuccess(); options.onSuccess();
}); });
expect(toast.success).toHaveBeenCalledWith(vendorAssignedMessage()); expect(toast.success).toHaveBeenCalledWith(vendorAssignedMessage());
expect(toast.success).toHaveBeenCalledTimes(1);
}); });
it("does not toast on slide-over save when vendorId did not change", () => { it("toasts generic save after a successful non-vendor slide-over patch", () => {
const onSave = vi.fn(); const onSave = vi.fn();
const open = baseRow(); const open = baseRow();
@ -167,6 +171,7 @@ describe("useSlideOverEditState completed lock", () => {
act(() => { act(() => {
options.onSuccess(); options.onSuccess();
}); });
expect(toast.success).not.toHaveBeenCalled(); expect(toast.success).toHaveBeenCalledWith(workOrderSavedMessage());
expect(toast.success).not.toHaveBeenCalledWith(vendorAssignedMessage());
}); });
}); });

View file

@ -77,7 +77,7 @@ import { useWorkOrdersListPage } from "@/app/(protected)/workorders/_hooks/use-w
const row = { id: 77, woNumber: "WO-77" } as WorkOrderTableRow; const row = { id: 77, woNumber: "WO-77" } as WorkOrderTableRow;
describe("useWorkOrdersListPage SH-119 toasts", () => { describe("useWorkOrdersListPage cancel toast", () => {
beforeEach(() => { beforeEach(() => {
vi.clearAllMocks(); vi.clearAllMocks();
}); });

View file

@ -42,7 +42,7 @@ vi.mock("@/app/(protected)/workorders/_components/list/table/cells/vendor-cell",
const row = { id: 7, upliftSummary: { hasUplift: false, pendingCount: 0 } } as WorkOrderTableRow; const row = { id: 7, upliftSummary: { hasUplift: false, pendingCount: 0 } } as WorkOrderTableRow;
describe("WoTableRowServiceCells vendor persist (SH-119)", () => { describe("WoTableRowServiceCells vendor persist", () => {
it("forwards vendor save through patch onSuccess instead of onPatchRow", () => { it("forwards vendor save through patch onSuccess instead of onPatchRow", () => {
const onPatch = vi.fn(); const onPatch = vi.fn();
const onPatchRow = vi.fn(); const onPatchRow = vi.fn();

View file

@ -485,3 +485,55 @@ describe("useWorkOrderTableMutations vendor assignment round-trip", () => {
expect(getClosabilityGaps(tableRowToClosabilityInput(returnedRow))).toContain("Company"); expect(getClosabilityGaps(tableRowToClosabilityInput(returnedRow))).toContain("Company");
}); });
}); });
describe("useWorkOrderTableMutations patchField success callbacks", () => {
beforeEach(() => {
getById.mockReset();
update.mockReset();
patchBoardField.mockReset();
});
it("invokes each onSuccess when two work orders overlap", async () => {
const rows = new Map<string, WorkOrderTableRow>([
["1", { ...BASE_ROW, id: 1, rowVersion: "v1" }],
["2", { ...BASE_ROW, id: 2, rowVersion: "v2" }],
]);
let releaseFirst: () => void = () => undefined;
const firstGate = new Promise<void>((resolve) => {
releaseFirst = resolve;
});
patchBoardField.mockImplementation(async (id: unknown) => {
if (String(id) === "1") await firstGate;
const current = rows.get(String(id));
if (!current) throw new Error("missing row");
const next = { ...current, rowVersion: `${current.rowVersion}-n` };
rows.set(String(id), next);
return next;
});
const onSuccessFirst = vi.fn();
const onSuccessSecond = vi.fn();
const { result } = renderHook(
() =>
useWorkOrderTableMutations({
onPatch: () => undefined,
clearPatch: () => undefined,
getRow: (id) => rows.get(String(id)),
}),
{ wrapper: makeWrapper() },
);
act(() => {
result.current.patchField(1, { dispatcherId: "d2" }, { onSuccess: onSuccessFirst });
result.current.patchField(2, { dispatcherId: "d3" }, { onSuccess: onSuccessSecond });
});
await waitFor(() => expect(onSuccessSecond).toHaveBeenCalledTimes(1));
expect(onSuccessFirst).not.toHaveBeenCalled();
releaseFirst();
await waitFor(() => expect(onSuccessFirst).toHaveBeenCalledTimes(1));
});
});

View file

@ -4,12 +4,13 @@ import { toVendorTablePatch } from "@/domain/work-orders/utils/vendor-assignment
import { import {
applyVendorTableSave, applyVendorTableSave,
isVendorAssignmentPatch, isVendorAssignmentPatch,
notifySlideOverSaveSuccess,
notifyVendorAssigned, notifyVendorAssigned,
notifyVendorAssignedIfPatched,
patchWorkOrderAsCanceled, patchWorkOrderAsCanceled,
patchWorkOrderAsCompleted, patchWorkOrderAsCompleted,
vendorAssignedMessage, vendorAssignedMessage,
workOrderCanceledMessage, workOrderCanceledMessage,
workOrderSavedMessage,
} from "@/domain/work-orders/utils/work-order-feedback-toasts"; } from "@/domain/work-orders/utils/work-order-feedback-toasts";
vi.mock("react-toastify", () => ({ vi.mock("react-toastify", () => ({
@ -20,12 +21,12 @@ vi.mock("react-toastify", () => ({
}, },
})); }));
describe("work-order-feedback-toasts (SH-119)", () => { describe("work-order-feedback-toasts", () => {
beforeEach(() => { beforeEach(() => {
vi.clearAllMocks(); vi.clearAllMocks();
}); });
it("builds the AAP cancel copy with the work order number", () => { it("builds the cancel copy with the work order number", () => {
expect(workOrderCanceledMessage("20260819001")).toBe("Work order #20260819001 canceled."); expect(workOrderCanceledMessage("20260819001")).toBe("Work order #20260819001 canceled.");
}); });
@ -40,20 +41,13 @@ describe("work-order-feedback-toasts (SH-119)", () => {
); );
expect(toast.warning).not.toHaveBeenCalled(); expect(toast.warning).not.toHaveBeenCalled();
const { onSuccess } = patchField.mock.calls[0][2] as { onSuccess: () => void }; const options = patchField.mock.calls[0][2] as { onSuccess: () => void; onError?: unknown };
onSuccess(); expect(options.onError).toBeUndefined();
options.onSuccess();
expect(toast.warning).toHaveBeenCalledWith("Work order #20260819001 canceled."); expect(toast.warning).toHaveBeenCalledWith("Work order #20260819001 canceled.");
expect(toast.success).not.toHaveBeenCalled(); expect(toast.success).not.toHaveBeenCalled();
}); });
it("does not warn when the cancel patch reports onError", () => {
const patchField = vi.fn();
patchWorkOrderAsCanceled(patchField, { id: 42, woNumber: "20260819001" });
const options = patchField.mock.calls[0][2] as { onSuccess?: () => void; onError?: () => void };
options.onError?.(new Error("conflict"));
expect(toast.warning).not.toHaveBeenCalled();
});
it("toasts vendor assignment only after persist onSuccess", () => { it("toasts vendor assignment only after persist onSuccess", () => {
const onPatch = vi.fn(); const onPatch = vi.fn();
const fields = { const fields = {
@ -77,18 +71,55 @@ describe("work-order-feedback-toasts (SH-119)", () => {
expect(toast.warning).not.toHaveBeenCalled(); expect(toast.warning).not.toHaveBeenCalled();
}); });
it("toasts vendor assignment when a slide-over patch includes vendorId", () => { it("persists an empty vendor without an assigned toast", () => {
notifyVendorAssignedIfPatched({ vendorId: "9", dispatcherId: "d1" }); const onPatch = vi.fn();
expect(toast.success).toHaveBeenCalledWith(vendorAssignedMessage()); const fields = {
}); vendorId: "",
company: "",
tech: "",
techPhone: "",
};
applyVendorTableSave(onPatch, 7, fields);
it("does not toast vendor assignment for non-vendor slide-over patches", () => { expect(onPatch).toHaveBeenCalledWith(7, toVendorTablePatch(fields), undefined);
expect(isVendorAssignmentPatch({ dispatcherId: "d1" })).toBe(false);
notifyVendorAssignedIfPatched({ dispatcherId: "d1" });
expect(toast.success).not.toHaveBeenCalled(); expect(toast.success).not.toHaveBeenCalled();
}); });
it("completes a work order without a success toast (AAP has no Complete WO toast)", () => { it("persists a whitespace vendorId without an assigned toast", () => {
const onPatch = vi.fn();
const fields = {
vendorId: " ",
company: "Gateway",
tech: "Pat",
techPhone: "555",
};
applyVendorTableSave(onPatch, 7, fields);
expect(onPatch).toHaveBeenCalledWith(7, toVendorTablePatch(fields), undefined);
expect(toast.success).not.toHaveBeenCalled();
});
it("toasts vendor assignment when a slide-over patch includes a vendor id", () => {
notifySlideOverSaveSuccess({ vendorId: "9", dispatcherId: "d1" });
expect(toast.success).toHaveBeenCalledTimes(1);
expect(toast.success).toHaveBeenCalledWith(vendorAssignedMessage());
});
it("toasts generic save when a slide-over patch has no assigned vendor", () => {
expect(isVendorAssignmentPatch({ dispatcherId: "d1" })).toBe(false);
notifySlideOverSaveSuccess({ dispatcherId: "d1" });
expect(toast.success).toHaveBeenCalledTimes(1);
expect(toast.success).toHaveBeenCalledWith(workOrderSavedMessage());
});
it("does not treat an empty vendorId as an assignment", () => {
expect(isVendorAssignmentPatch({ vendorId: "" })).toBe(false);
notifySlideOverSaveSuccess({ vendorId: "", dispatcherId: "d1" });
expect(toast.success).toHaveBeenCalledWith(workOrderSavedMessage());
expect(toast.success).not.toHaveBeenCalledWith(vendorAssignedMessage());
});
it("completes a work order without a success toast", () => {
const patchField = vi.fn(); const patchField = vi.fn();
patchWorkOrderAsCompleted(patchField, 42); patchWorkOrderAsCompleted(patchField, 42);
expect(patchField).toHaveBeenCalledWith(42, { status: "Completed" }); expect(patchField).toHaveBeenCalledWith(42, { status: "Completed" });