From cd5d4ad1d872cec2a3a823457cd611234594e841 Mon Sep 17 00:00:00 2001 From: Codex Review Integration Date: Wed, 16 Sep 2026 15:18:06 -0300 Subject: [PATCH] fix(work-orders): keep wizard vendor notes bound to the selected company Late roster data no longer overwrites notes typed for the current selection. The notes baseline is keyed by vendor, so a save never patches a previous company. Changing the primary service clears leftover notes, and a save with a missing baseline reads the current roster first. Also drops ticket keys from shipped comments. Co-Authored-By: Claude Opus 5 (1M context) --- .../wizard/use-new-wo-wizard-controller.ts | 5 +- .../wizard/use-wizard-vendor-company-notes.ts | 83 ++++++++++-- .../wizard-step-location-service-select.tsx | 2 +- .../use-save-vendor-company-notes.ts | 7 +- .../utils/build-company-notes-patch.ts | 2 +- .../use-wizard-vendor-company-notes.test.tsx | 118 ++++++++++++++++-- .../wizard-service-clears-vendor.test.tsx | 4 + .../utils/build-company-notes-patch.test.ts | 2 +- 8 files changed, 193 insertions(+), 30 deletions(-) diff --git a/src/app/(protected)/workorders/_components/wizard/use-new-wo-wizard-controller.ts b/src/app/(protected)/workorders/_components/wizard/use-new-wo-wizard-controller.ts index 0f249909..9e0476d3 100644 --- a/src/app/(protected)/workorders/_components/wizard/use-new-wo-wizard-controller.ts +++ b/src/app/(protected)/workorders/_components/wizard/use-new-wo-wizard-controller.ts @@ -65,8 +65,9 @@ export function useNewWoWizardController({ }; const createMutation = useCreateWorkOrderFromWizard((createdDraft) => { - saveCompanyNotes(createdDraft); - onOpenChange(false); + // Close only after the company-notes save attempt settles; its failures are surfaced + // as note-save warnings and must never report the work-order creation as failed. + void saveCompanyNotes(createdDraft).finally(() => onOpenChange(false)); }); const { duplicateRow, setDuplicateRow, handleDuplicateFound, handleCreate, isCreating } = useWizardDuplicateActions({ diff --git a/src/app/(protected)/workorders/_components/wizard/use-wizard-vendor-company-notes.ts b/src/app/(protected)/workorders/_components/wizard/use-wizard-vendor-company-notes.ts index 03db91d0..73625b6a 100644 --- a/src/app/(protected)/workorders/_components/wizard/use-wizard-vendor-company-notes.ts +++ b/src/app/(protected)/workorders/_components/wizard/use-wizard-vendor-company-notes.ts @@ -1,7 +1,14 @@ import { useEffect, useState } from "react"; +import { useQueryClient } from "@tanstack/react-query"; +import { toast } from "react-toastify"; import type { WorkOrderWizardDraft } from "@/domain/work-orders/types/work-order-wizard"; import { useVendorCompanyRoster } from "@/domain/vendors/use-cases/use-vendor-company-roster"; -import { useSaveVendorCompanyNotes } from "@/domain/vendors/use-cases/use-save-vendor-company-notes"; +import { + NOTES_SAVE_FAILED_MESSAGE, + useSaveVendorCompanyNotes, +} from "@/domain/vendors/use-cases/use-save-vendor-company-notes"; +import { vendorCompanyRosterApi } from "@/domain/vendors/api/vendor-company-roster-api"; +import { queryKeys } from "@/infra/query-key/query-key"; import { buildCompanyNotesUpdate, type VendorCompanyNotesBaseline, @@ -9,15 +16,21 @@ import { type SetDraft = (updater: (current: WorkOrderWizardDraft) => WorkOrderWizardDraft) => void; +type VendorNotesBaseline = VendorCompanyNotesBaseline & { vendorId: string }; + /** - * SH-321: the wizard's Vendor Notes field is the vendor company's notes (same field as the - * Vendors page). Picking a technician pre-fills the company's current notes once per company, - * so switching technicians within the same company keeps the dispatcher's edits. + * The wizard's Vendor Notes field is the vendor company's notes (same field as the Vendors + * page). Picking a technician pre-fills the company's current notes once per company, so + * switching technicians within the same company keeps the dispatcher's edits. Notes typed + * for the current selection always win over roster data that arrives late, and a save whose + * baseline is missing or belongs to another selection reads the current roster before + * building the PATCH. */ export function useWizardVendorCompanyNotes(open: boolean, vendorId: string, setDraft: SetDraft) { + const queryClient = useQueryClient(); const { data: roster } = useVendorCompanyRoster(vendorId || undefined); const saveNotes = useSaveVendorCompanyNotes(); - const [baseline, setBaseline] = useState(null); + const [baseline, setBaseline] = useState(null); useEffect(() => { if (open) { @@ -34,24 +47,72 @@ export function useWizardVendorCompanyNotes(open: boolean, vendorId: string, set if (!roster || roster.companyId === null) { return; } - if (baseline && String(baseline.companyId) === String(roster.companyId)) { + if ( + baseline && + baseline.vendorId === vendorId && + String(baseline.companyId) === String(roster.companyId) + ) { return; } setBaseline({ + vendorId, companyId: roster.companyId, rowVersion: roster.rowVersion, notes: roster.notes, }); - setDraft((current) => ({ ...current, vendorNotes: roster.notes })); + setDraft((current) => { + if (current.vendorId !== vendorId) { + return current; + } + // Seed only empty content or content still holding the previous company's unedited + // notes; anything else is the dispatcher's typing and must never be overwritten. + if (current.vendorNotes !== "" && current.vendorNotes !== baseline?.notes) { + return current; + } + return { ...current, vendorNotes: roster.notes }; + }); }, [vendorId, roster, baseline, setDraft]); - function saveCompanyNotes(draft: WorkOrderWizardDraft) { + async function fetchCurrentBaseline(vendorId: string): Promise { + try { + const current = await queryClient.fetchQuery({ + queryKey: queryKeys.vendors.roster("vendorId", vendorId), + queryFn: () => vendorCompanyRosterApi.get({ vendorId }), + }); + if (!current || current.companyId === null) { + return null; + } + return { + vendorId, + companyId: current.companyId, + rowVersion: current.rowVersion, + notes: current.notes, + }; + } catch { + toast.error(NOTES_SAVE_FAILED_MESSAGE); + return null; + } + } + + async function saveCompanyNotes(draft: WorkOrderWizardDraft): Promise { if (!draft.vendorId) { return; } - const update = buildCompanyNotesUpdate(baseline, draft.vendorNotes); - if (update) { - saveNotes.mutate(update); + const active = + baseline && baseline.vendorId === draft.vendorId + ? baseline + : await fetchCurrentBaseline(draft.vendorId); + if (!active) { + return; + } + const update = buildCompanyNotesUpdate(active, draft.vendorNotes); + if (!update) { + return; + } + try { + await saveNotes.mutateAsync(update); + } catch { + // The mutation already surfaces the note-save failure toast. } } diff --git a/src/app/(protected)/workorders/_components/wizard/wizard-step-location-service-select.tsx b/src/app/(protected)/workorders/_components/wizard/wizard-step-location-service-select.tsx index bfefcf5d..b121d3f1 100644 --- a/src/app/(protected)/workorders/_components/wizard/wizard-step-location-service-select.tsx +++ b/src/app/(protected)/workorders/_components/wizard/wizard-step-location-service-select.tsx @@ -29,7 +29,7 @@ export function WizardStepLocationServiceSelect({ onPatch, }: WizardStepLocationServiceSelectProps) { const clearVendorIfPrimaryChanged = (nextPm: string): Partial => - nextPm !== draft.pm ? { vendorId: "", vendorName: "", techPhone: "" } : {}; + nextPm !== draft.pm ? { vendorId: "", vendorName: "", techPhone: "", vendorNotes: "" } : {}; const selectSvc = (p: string) => { onPatch({ diff --git a/src/domain/vendors/use-cases/use-save-vendor-company-notes.ts b/src/domain/vendors/use-cases/use-save-vendor-company-notes.ts index 230f18c3..9e98976b 100644 --- a/src/domain/vendors/use-cases/use-save-vendor-company-notes.ts +++ b/src/domain/vendors/use-cases/use-save-vendor-company-notes.ts @@ -5,7 +5,10 @@ import type { VendorCompanyRoster } from "@/domain/vendors/types/vendor"; import type { VendorCompanyNotesUpdate } from "@/domain/vendors/utils/build-company-notes-patch"; import { queryKeys } from "@/infra/query-key/query-key"; -/** Writes company-level vendor notes (SH-321) and refreshes every vendor view that shows them. */ +export const NOTES_SAVE_FAILED_MESSAGE = + "Work order created, but the vendor notes could not be saved."; + +/** Writes company-level vendor notes and refreshes every vendor view that shows them. */ export function useSaveVendorCompanyNotes(): UseMutationResult< VendorCompanyRoster, unknown, @@ -20,7 +23,7 @@ export function useSaveVendorCompanyNotes(): UseMutationResult< void queryClient.invalidateQueries({ queryKey: queryKeys.vendors.all }); }, onError: () => { - toast.error("Work order created, but the vendor notes could not be saved."); + toast.error(NOTES_SAVE_FAILED_MESSAGE); }, }); } diff --git a/src/domain/vendors/utils/build-company-notes-patch.ts b/src/domain/vendors/utils/build-company-notes-patch.ts index dc2de725..3546a3cc 100644 --- a/src/domain/vendors/utils/build-company-notes-patch.ts +++ b/src/domain/vendors/utils/build-company-notes-patch.ts @@ -12,7 +12,7 @@ export interface VendorCompanyNotesUpdate { } /** - * SH-321: company-level notes edited outside the Vendors page (e.g. the Create WO wizard) + * Company-level notes edited outside the Vendors page (e.g. the Create WO wizard) * are written through the additive roster PATCH, which only touches the fields sent. * Returns null when nothing changed. Clearing notes is not expressible on that PATCH * (blank company fields mean "unchanged"), so an emptied field is not sent. diff --git a/src/test/app/(protected)/workorders/use-wizard-vendor-company-notes.test.tsx b/src/test/app/(protected)/workorders/use-wizard-vendor-company-notes.test.tsx index 924e766e..8801c765 100644 --- a/src/test/app/(protected)/workorders/use-wizard-vendor-company-notes.test.tsx +++ b/src/test/app/(protected)/workorders/use-wizard-vendor-company-notes.test.tsx @@ -3,10 +3,12 @@ import { QueryClientProvider } from "@tanstack/react-query"; import { useState, type ReactNode } from "react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { useWizardVendorCompanyNotes } from "@/app/(protected)/workorders/_components/wizard/use-wizard-vendor-company-notes"; +import { NOTES_SAVE_FAILED_MESSAGE } from "@/domain/vendors/use-cases/use-save-vendor-company-notes"; import { EMPTY_WIZARD_DRAFT, type WorkOrderWizardDraft, } from "@/domain/work-orders/types/work-order-wizard"; +import { toast } from "react-toastify"; import { createTestQueryClient } from "@/test/test-utils"; const rosterGet = vi.fn(); @@ -21,6 +23,8 @@ vi.mock("@/domain/vendors/api/vendor-company-roster-api", () => ({ vi.mock("react-toastify", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); +type Roster = ReturnType; + function roster(companyId: number, notes: string) { return { companyId, @@ -38,6 +42,17 @@ function roster(companyId: number, notes: string) { }; } +function deferredRoster() { + let resolve!: (value: Roster) => void; + const pending = new Promise((res) => { + resolve = res; + }); + return { + promise: () => pending, + resolveAll: (value: Roster) => resolve(value), + }; +} + function useHarness() { const [draft, setDraft] = useState({ ...EMPTY_WIZARD_DRAFT }); const notes = useWizardVendorCompanyNotes(true, draft.vendorId, setDraft); @@ -52,10 +67,11 @@ function renderHarness() { return renderHook(() => useHarness(), { wrapper }); } -describe("useWizardVendorCompanyNotes (SH-321)", () => { +describe("useWizardVendorCompanyNotes", () => { beforeEach(() => { rosterGet.mockReset(); addTechnicians.mockReset(); + vi.mocked(toast.error).mockClear(); rosterGet.mockImplementation(({ vendorId }: { vendorId: string }) => Promise.resolve( vendorId === "31" ? roster(9, "Other company notes") : roster(7, "Call before arrival"), @@ -98,22 +114,98 @@ describe("useWizardVendorCompanyNotes (SH-321)", () => { await waitFor(() => expect(result.current.draft.vendorNotes).toBe("Other company notes")); }); + it("keeps notes typed before the roster resolves and saves them for that company", async () => { + const lateRoster = deferredRoster(); + rosterGet.mockImplementation(({ vendorId }: { vendorId: string }) => + vendorId === "21" ? lateRoster.promise() : Promise.resolve(roster(7, "Call before arrival")), + ); + const { result } = renderHarness(); + act(() => result.current.setDraft((d) => ({ ...d, vendorId: "21" }))); + act(() => result.current.setDraft((d) => ({ ...d, vendorNotes: "Gate code 4411" }))); + + await act(async () => lateRoster.resolveAll(roster(7, "Call before arrival"))); + expect(result.current.draft.vendorNotes).toBe("Gate code 4411"); + + await act(async () => { + await result.current.saveCompanyNotes({ + ...result.current.draft, + vendorNotes: "Gate code 4411", + }); + }); + + expect(addTechnicians).toHaveBeenCalledWith(7, { + rowVersion: "rv-7", + addTechnicians: [], + companyFields: { notes: "Gate code 4411" }, + }); + }); + + it("targets the newly selected company at save time while its roster is still loading", async () => { + const companyB = deferredRoster(); + rosterGet.mockImplementation(({ vendorId }: { vendorId: string }) => + vendorId === "21" ? Promise.resolve(roster(7, "Call before arrival")) : companyB.promise(), + ); + const { result } = renderHarness(); + act(() => result.current.setDraft((d) => ({ ...d, vendorId: "21" }))); + await waitFor(() => expect(result.current.draft.vendorNotes).toBe("Call before arrival")); + + act(() => result.current.setDraft((d) => ({ ...d, vendorId: "31" }))); + act(() => result.current.setDraft((d) => ({ ...d, vendorNotes: "Gate code 4411" }))); + + let savePromise!: Promise; + act(() => { + savePromise = result.current + .saveCompanyNotes({ ...result.current.draft, vendorNotes: "Gate code 4411" }) + .then(() => undefined); + }); + await act(async () => companyB.resolveAll(roster(9, "Other company notes"))); + await act(async () => { + await savePromise; + }); + + expect(addTechnicians).toHaveBeenCalledWith(9, { + rowVersion: "rv-9", + addTechnicians: [], + companyFields: { notes: "Gate code 4411" }, + }); + expect(addTechnicians).not.toHaveBeenCalledWith(7, expect.anything()); + expect(result.current.draft.vendorNotes).toBe("Gate code 4411"); + }); + + it("warns without writing when the save-time roster read fails", async () => { + rosterGet.mockRejectedValue(new Error("roster down")); + const { result } = renderHarness(); + act(() => result.current.setDraft((d) => ({ ...d, vendorId: "21" }))); + act(() => result.current.setDraft((d) => ({ ...d, vendorNotes: "Gate code 4411" }))); + + await act(async () => { + await result.current.saveCompanyNotes({ + ...result.current.draft, + vendorNotes: "Gate code 4411", + }); + }); + + expect(toast.error).toHaveBeenCalledWith(NOTES_SAVE_FAILED_MESSAGE); + expect(addTechnicians).not.toHaveBeenCalled(); + }); + it("writes edited notes to the vendor company when the work order is created", async () => { const { result } = renderHarness(); act(() => result.current.setDraft((d) => ({ ...d, vendorId: "21" }))); await waitFor(() => expect(result.current.draft.vendorNotes).toBe("Call before arrival")); - act(() => - result.current.saveCompanyNotes({ ...result.current.draft, vendorNotes: "Gate code 4411" }), - ); + await act(async () => { + await result.current.saveCompanyNotes({ + ...result.current.draft, + vendorNotes: "Gate code 4411", + }); + }); - await waitFor(() => - expect(addTechnicians).toHaveBeenCalledWith(7, { - rowVersion: "rv-7", - addTechnicians: [], - companyFields: { notes: "Gate code 4411" }, - }), - ); + expect(addTechnicians).toHaveBeenCalledWith(7, { + rowVersion: "rv-7", + addTechnicians: [], + companyFields: { notes: "Gate code 4411" }, + }); }); it("does not write when the notes were left as the company's existing notes", async () => { @@ -121,7 +213,9 @@ describe("useWizardVendorCompanyNotes (SH-321)", () => { act(() => result.current.setDraft((d) => ({ ...d, vendorId: "21" }))); await waitFor(() => expect(result.current.draft.vendorNotes).toBe("Call before arrival")); - act(() => result.current.saveCompanyNotes(result.current.draft)); + await act(async () => { + await result.current.saveCompanyNotes(result.current.draft); + }); expect(addTechnicians).not.toHaveBeenCalled(); }); diff --git a/src/test/app/(protected)/workorders/wizard-service-clears-vendor.test.tsx b/src/test/app/(protected)/workorders/wizard-service-clears-vendor.test.tsx index 3812fb63..9a22a49a 100644 --- a/src/test/app/(protected)/workorders/wizard-service-clears-vendor.test.tsx +++ b/src/test/app/(protected)/workorders/wizard-service-clears-vendor.test.tsx @@ -15,6 +15,7 @@ describe("WizardStepLocationServiceSelect — vendor clear on service change", ( vendorId: "9", vendorName: "Old Vendor", techPhone: "555-0100", + vendorNotes: "Old company notes", }; render( @@ -43,6 +44,7 @@ describe("WizardStepLocationServiceSelect — vendor clear on service change", ( vendorId: "", vendorName: "", techPhone: "", + vendorNotes: "", }), ); }); @@ -56,6 +58,7 @@ describe("WizardStepLocationServiceSelect — vendor clear on service change", ( vendorId: "9", vendorName: "Old Vendor", techPhone: "555-0100", + vendorNotes: "Old company notes", }; render( @@ -83,6 +86,7 @@ describe("WizardStepLocationServiceSelect — vendor clear on service change", ( vendorId: "", vendorName: "", techPhone: "", + vendorNotes: "", }); }); }); diff --git a/src/test/domain/vendors/utils/build-company-notes-patch.test.ts b/src/test/domain/vendors/utils/build-company-notes-patch.test.ts index 11769b1e..824ad1c8 100644 --- a/src/test/domain/vendors/utils/build-company-notes-patch.test.ts +++ b/src/test/domain/vendors/utils/build-company-notes-patch.test.ts @@ -3,7 +3,7 @@ import { buildCompanyNotesUpdate } from "@/domain/vendors/utils/build-company-no const baseline = { companyId: 12, rowVersion: "AAAAAAAAB9E=", notes: "Call before arrival" }; -describe("buildCompanyNotesUpdate (SH-321)", () => { +describe("buildCompanyNotesUpdate", () => { it("builds a notes-only additive patch when the notes changed", () => { expect(buildCompanyNotesUpdate(baseline, " Gate code 4411 ")).toEqual({ companyId: 12,