diff --git a/e2e/vendors/vendors.spec.ts b/e2e/vendors/vendors.spec.ts index f5520d1d..b348fd21 100644 --- a/e2e/vendors/vendors.spec.ts +++ b/e2e/vendors/vendors.spec.ts @@ -475,7 +475,14 @@ test.describe("Vendor directory prototype parity", () => { await expect(page.getByRole("dialog", { name: /Add Vendor/ })).toHaveCount(0); expect(state.patchedCompanyId).toBe("101"); expect(state.patchedBody).toMatchObject({ rowVersion: "rv-1" }); + // Only the field the user actually typed into is transmitted. Address/phone/email were + // populated by selecting the company and never edited, so they must not be sent — otherwise a + // concurrent edit by another user to any of them would be silently reverted on resubmit. expect(state.patchedBody?.companyFields).toEqual({ notes: "Created in browser E2E" }); + expect(state.patchedBody?.companyFields).not.toHaveProperty("companyPhone"); + expect(state.patchedBody?.companyFields).not.toHaveProperty("address"); + expect(state.patchedBody?.companyFields).not.toHaveProperty("email"); + expect(state.patchedBody?.companyFields).not.toHaveProperty("name"); expect(state.patchedBody?.addTechnicians).toEqual([ expect.objectContaining({ contactName: "New Technician", diff --git a/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts b/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts index 61f6f678..96f5e3b7 100644 --- a/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts +++ b/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts @@ -12,8 +12,35 @@ import { buildAdditiveRosterPatch, getSingleStatusOnlyChange, useSaveVendorCompanyRoster, + type EditedCompanyFields, } from "@/domain/vendors/use-cases/use-save-vendor-company-roster"; import { useVendorRosterResolver } from "./use-vendor-roster-resolver"; + +const EDITABLE_COMPANY_FIELDS = [ + "name", + "companyPhone", + "email", + "address", + "city", + "state", + "zip", + "googleMapsUrl", + "notes", +] as const; + +/** + * Company fields the user actually typed into, taken from react-hook-form dirty state. + * Selecting a company calls reset(), so loaded values are not dirty; only genuine edits are. + * The additive PATCH must send these rather than a form-vs-roster diff, which would resend + * stale values for fields another user changed while a conflict was being resolved. + */ +function pickEditedCompanyFields(dirtyFields: Record): EditedCompanyFields { + const edited: EditedCompanyFields = {}; + for (const field of EDITABLE_COMPANY_FIELDS) { + if (dirtyFields[field] === true) edited[field] = true; + } + return edited; +} import { toFormValues, useVendorRosterSelection } from "./use-vendor-roster-selection"; import { useVendorFacets } from "@/domain/vendors/use-cases/use-vendor-facets"; import { isVendorRosterConflictError } from "@/domain/vendors/lib/vendor-roster-conflict"; @@ -106,6 +133,10 @@ export function useVendorRosterForm({ mode: "onChange", }); const { control, handleSubmit, reset, formState } = form; + // react-hook-form exposes formState through a Proxy that only tracks properties read during + // render. Reading dirtyFields inside the submit callback would not subscribe and would come + // back empty, so it is resolved here on every render. + const editedCompanyFields = pickEditedCompanyFields(formState.dirtyFields); const watched = useWatch({ control }); const clearConflict = useCallback(() => setConflict(null), []); @@ -136,7 +167,7 @@ export function useVendorRosterForm({ const values = withoutBlankNewTechnicians(formValues); if (mode === "create" && selection.selectedRoster != null) { const selected = selection.selectedRoster; - if (!buildAdditiveRosterPatch(selected, values, selected.rowVersion)) { + if (!buildAdditiveRosterPatch(selected, values, selected.rowVersion, editedCompanyFields)) { toast.error("Enter at least one technician to add to this vendor."); return; } @@ -147,6 +178,7 @@ export function useVendorRosterForm({ companyId: selected.companyId, rowVersion: selected.rowVersion, originalRoster: selected, + editedCompanyFields, }, { onSuccess: (data) => onSuccess?.(data), diff --git a/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts b/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts index 1d1c4865..ca7fa247 100644 --- a/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts +++ b/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts @@ -16,8 +16,12 @@ export interface SaveVendorCompanyRosterInput { companyId?: string | number | null; rowVersion?: string; originalRoster?: VendorCompanyRoster; + /** Company fields the user actually edited. See buildAdditiveRosterPatch. */ + editedCompanyFields?: EditedCompanyFields; } +export type EditedCompanyFields = Partial>; + export interface SaveVendorCompanyRosterContext { conflict?: unknown; } @@ -92,10 +96,20 @@ function hasTechnicianContent(technician: { ); } +/** + * Builds the additive PATCH body for the Add flow. + * + * `editedCompanyFields` must carry the fields the user actually touched. A plain + * form-vs-roster diff is unsafe here: after a stale-409 the roster is refetched while the form + * keeps the values loaded before the conflict, so a field another user changed in that window + * would look "different" and be sent back with our stale value, silently reverting their edit. + * Only fields the user edited are ever transmitted. + */ export function buildAdditiveRosterPatch( roster: VendorCompanyRoster | null | undefined, values: VendorCompanyRosterFormValues, rowVersion: string, + editedCompanyFields: EditedCompanyFields = {}, ): VendorRosterAdditivePatchInput | null { const addTechnicians = values.technicians.filter( (technician) => technician.id == null && hasTechnicianContent(technician), @@ -103,6 +117,7 @@ export function buildAdditiveRosterPatch( const companyFields: Record = {}; if (roster) { for (const field of COMPANY_FIELDS) { + if (editedCompanyFields[field] !== true) continue; if (values[field] !== roster[field]) companyFields[field] = values[field]; } } @@ -134,6 +149,7 @@ export function useSaveVendorCompanyRoster(): UseMutationResult< companyId, rowVersion, originalRoster, + editedCompanyFields, }: SaveVendorCompanyRosterInput) => { if (mode === "update") { const statusChange = originalRoster @@ -162,7 +178,12 @@ export function useSaveVendorCompanyRoster(): UseMutationResult< throw new Error("Company id is required to add technicians"); } if (!rowVersion) throw new Error("Row version is required to add technicians"); - const patch = buildAdditiveRosterPatch(originalRoster, values, rowVersion); + const patch = buildAdditiveRosterPatch( + originalRoster, + values, + rowVersion, + editedCompanyFields, + ); if (!patch) throw new Error("Nothing to add to this vendor company"); return vendorCompanyRosterApi.addTechnicians(companyId, patch); } diff --git a/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx b/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx index 61c431e6..f03c2a27 100644 --- a/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx +++ b/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx @@ -112,16 +112,45 @@ describe("buildAdditiveRosterPatch", () => { expect(patch?.addTechnicians).toEqual([newTechnician]); }); - it("sends only changed company fields", () => { + it("sends only company fields the user edited", () => { const patch = buildAdditiveRosterPatch( roster, formValues({ notes: "New notes", address: "2 Oak Ave" }), "rv-1", + { notes: true, address: true }, ); expect(patch?.companyFields).toEqual({ notes: "New notes", address: "2 Oak Ave" }); }); + it("never sends a company field the user did not edit, even when it differs from the roster", () => { + // Regression: after a stale-409 the roster is refetched while the form keeps pre-conflict + // values. A form-vs-roster diff would resend our stale value for a field another user changed + // in that window, silently reverting their edit. Only edited fields may be transmitted. + const rosterAfterOtherUserEditedPhone = { ...roster, companyPhone: "314-555-9999" }; + + const patch = buildAdditiveRosterPatch( + rosterAfterOtherUserEditedPhone, + formValues({ companyPhone: roster.companyPhone, notes: "New notes" }), + "rv-2", + { notes: true }, + ); + + expect(patch?.companyFields).toEqual({ notes: "New notes" }); + expect(patch?.companyFields).not.toHaveProperty("companyPhone"); + }); + + it("omits companyFields when values differ but nothing was edited", () => { + const patch = buildAdditiveRosterPatch( + roster, + formValues({ notes: "Drifted under us" }), + "rv-1", + {}, + ); + + expect(patch).not.toHaveProperty("companyFields"); + }); + it("omits companyFields entirely when nothing changed", () => { const patch = buildAdditiveRosterPatch(roster, formValues(), "rv-1");