mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 20:42:43 +00:00
fix(profile): save every valid field even when a sibling is invalid
A profile PUT/PATCH validated as one safeParse and wrote as one transaction, so a single rejected field took every sibling change down with it - editing height and gender together lost both, and onboarding lost a whole baseline step to one field. The schema has no cross-field dependency, so applyProfileUpdate now re-validates each present field against its own schema and writes whatever validates; only a body where nothing validates is refused outright, naming the field that blocked it. The top-level error string also stopped being a raw Zod message (issues[0].message) that could read "Invalid option: expected one of "MALE"|"FEMALE"|"OTHER"" on a person's screen. It's now a person-safe sentence plus a stable errorCode for the web client to localize; the full per-field detail is relocated (not deleted) into details.issues, matching the multi-issue envelope the iOS contract already expects from other write routes.
This commit is contained in:
1 parent
bed634fd67
commit
e49bd866dd
5 files changed
+289
-16
No files matched your search
@@ -1,6 +1,7 @@
|
||||
import {
|
||||
apiSuccess,
|
||||
apiError,
|
||||
apiValidationError,
|
||||
getClientIp,
|
||||
safeJson,
|
||||
} from "@/lib/api-response";
|
||||
@@ -19,7 +20,14 @@ export const PUT = apiHandler(async (request: NextRequest) => {
|
||||
|
||||
const result = await applyProfileUpdate(user.id, body, getClientIp(request));
|
||||
if (!result.ok) {
|
||||
return apiError(result.message, result.status);
|
||||
return result.issues
|
||||
? apiValidationError(
|
||||
result.message,
|
||||
result.issues,
|
||||
result.status,
|
||||
result.errorCode ? { errorCode: result.errorCode } : undefined,
|
||||
)
|
||||
: apiError(result.message, result.status);
|
||||
}
|
||||
|
||||
annotate({ action: { name: "auth.profile.update" } });
|
||||
@@ -37,5 +45,6 @@ export const PUT = apiHandler(async (request: NextRequest) => {
|
||||
insurerName: result.user.insurerName,
|
||||
insurerIkNumber: result.user.insurerIkNumber,
|
||||
hasInsuranceNumber: result.user.hasInsuranceNumber,
|
||||
...(result.rejectedFields ? { rejectedFields: result.rejectedFields } : {}),
|
||||
});
|
||||
});
|
||||
@@ -11,6 +11,7 @@ import { apiHandler, requireAuth } from "@/lib/api-handler";
|
||||
import {
|
||||
apiError,
|
||||
apiSuccess,
|
||||
apiValidationError,
|
||||
getClientIp,
|
||||
safeJson,
|
||||
} from "@/lib/api-response";
|
||||
@@ -101,7 +102,14 @@ export const PATCH = apiHandler(async (request: NextRequest) => {
|
||||
|
||||
const result = await applyProfileUpdate(user.id, body, getClientIp(request));
|
||||
if (!result.ok) {
|
||||
return apiError(result.message, result.status);
|
||||
return result.issues
|
||||
? apiValidationError(
|
||||
result.message,
|
||||
result.issues,
|
||||
result.status,
|
||||
result.errorCode ? { errorCode: result.errorCode } : undefined,
|
||||
)
|
||||
: apiError(result.message, result.status);
|
||||
}
|
||||
|
||||
annotate({ action: { name: "user.profile.update" } });
|
||||
@@ -122,5 +130,6 @@ export const PATCH = apiHandler(async (request: NextRequest) => {
|
||||
insurerName: result.user.insurerName,
|
||||
insurerIkNumber: result.user.insurerIkNumber,
|
||||
hasInsuranceNumber: result.user.hasInsuranceNumber,
|
||||
...(result.rejectedFields ? { rejectedFields: result.rejectedFields } : {}),
|
||||
});
|
||||
});
|
||||
@@ -175,6 +175,32 @@ export function returnAllZodIssues(
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Like {@link returnAllZodIssues}, but for callers that have already
|
||||
* reduced a `ZodError` to a caller-composed, person-safe top-level
|
||||
* `error` sentence and a pre-sanitised issue list (e.g. a shared
|
||||
* write-path helper that validates outside the route handler, such as
|
||||
* `applyProfileUpdate`). The multi-issue envelope (`details.issues`) is
|
||||
* unchanged from `returnAllZodIssues` — this exists so the top-level
|
||||
* string never has to repeat a raw Zod default message (an enum's
|
||||
* `Invalid option: expected one of "A"|"B"|"C"` reaching a person's
|
||||
* screen) while the machine-readable detail is still relocated, not
|
||||
* dropped. `meta.errorCode` lets the web client resolve a localized
|
||||
* sentence via `apiErrors.<errorCode>` (see `localizedApiError`).
|
||||
*/
|
||||
export function apiValidationError(
|
||||
message: string,
|
||||
issues: SanitisedZodIssue[],
|
||||
status: number = 422,
|
||||
meta?: ErrorMeta,
|
||||
): NextResponse {
|
||||
return buildJsonErrorResponse(
|
||||
{ data: null, error: message, details: { issues } },
|
||||
status,
|
||||
meta,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* `meta` is additive — clients that ignore it see the unchanged
|
||||
* `{ data: null, error: <string> }` envelope. New callers can pass
|
||||
|
||||
@@ -45,14 +45,25 @@ describe("applyProfileUpdate timezone validation", () => {
|
||||
expect(prisma.user.update).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("rejects an unknown zone with 422", async () => {
|
||||
it("rejects an unknown zone with 422, naming the field without echoing raw validator prose", async () => {
|
||||
const result = await applyProfileUpdate(USER_ID, {
|
||||
timezone: "Mars/Tharsis",
|
||||
});
|
||||
expect(result.ok).toBe(false);
|
||||
if (!result.ok) {
|
||||
expect(result.status).toBe(422);
|
||||
expect(result.message).toMatch(/Invalid IANA timezone/i);
|
||||
// The specific validator reason is relocated to `issues`, not deleted.
|
||||
expect(result.issues).toEqual([
|
||||
expect.objectContaining({
|
||||
path: "timezone",
|
||||
message: expect.stringMatching(/Invalid IANA timezone/i),
|
||||
}),
|
||||
]);
|
||||
// The top-level message stays a sentence for a person: it names the
|
||||
// field but never repeats the validator's own prose.
|
||||
expect(result.message).toMatch(/nothing was saved/i);
|
||||
expect(result.message).toMatch(/timezone/i);
|
||||
expect(result.message).not.toMatch(/Invalid IANA timezone/i);
|
||||
}
|
||||
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||
});
|
||||
@@ -126,14 +137,20 @@ describe("applyProfileUpdate insurer IK number", () => {
|
||||
expect("insurerIkNumber" in arg.data).toBe(false);
|
||||
});
|
||||
|
||||
it("rejects a non-numeric IKNR with 422", async () => {
|
||||
it("rejects a non-numeric IKNR with 422, relocating the reason to issues", async () => {
|
||||
const result = await applyProfileUpdate(USER_ID, {
|
||||
insurerIkNumber: "12345678X",
|
||||
});
|
||||
expect(result.ok).toBe(false);
|
||||
if (!result.ok) {
|
||||
expect(result.status).toBe(422);
|
||||
expect(result.message).toMatch(/IK number/i);
|
||||
expect(result.issues).toEqual([
|
||||
expect.objectContaining({
|
||||
path: "insurerIkNumber",
|
||||
message: expect.stringMatching(/IK number/i),
|
||||
}),
|
||||
]);
|
||||
expect(result.message).toMatch(/nothing was saved/i);
|
||||
}
|
||||
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||
});
|
||||
@@ -150,3 +167,98 @@ describe("applyProfileUpdate insurer IK number", () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("applyProfileUpdate partial-field save", () => {
|
||||
// Reported regression: the native client sent a lowercase `gender`
|
||||
// alongside a valid `heightCm` in one PUT. Because the whole body was
|
||||
// one `safeParse` and one transaction, the gender rejection took the
|
||||
// height edit down with it — "editing height and gender together lost
|
||||
// both". The profile schema has no cross-field dependency, so the
|
||||
// fix is to salvage every field that DOES validate.
|
||||
it("writes the valid sibling field when another field in the same body is invalid", async () => {
|
||||
const result = await applyProfileUpdate(USER_ID, {
|
||||
heightCm: 180,
|
||||
gender: "male", // wrong case — the client-side half of the bug
|
||||
});
|
||||
expect(result.ok).toBe(true);
|
||||
if (result.ok) {
|
||||
expect(result.rejectedFields).toEqual([
|
||||
expect.objectContaining({ path: "gender" }),
|
||||
]);
|
||||
}
|
||||
expect(prisma.user.update).toHaveBeenCalledTimes(1);
|
||||
const arg = vi.mocked(prisma.user.update).mock.calls[0][0] as {
|
||||
data: Record<string, unknown>;
|
||||
};
|
||||
expect(arg.data.heightCm).toBe(180);
|
||||
expect("gender" in arg.data).toBe(false);
|
||||
});
|
||||
|
||||
it("omits `rejectedFields` entirely when every touched field is valid", async () => {
|
||||
const result = await applyProfileUpdate(USER_ID, {
|
||||
heightCm: 175,
|
||||
gender: "FEMALE",
|
||||
});
|
||||
expect(result.ok).toBe(true);
|
||||
if (result.ok) {
|
||||
expect(result.rejectedFields).toBeUndefined();
|
||||
}
|
||||
});
|
||||
|
||||
it("writes nothing and reports every rejected field when the whole body is invalid", async () => {
|
||||
const result = await applyProfileUpdate(USER_ID, {
|
||||
gender: "male",
|
||||
locale: "fr",
|
||||
});
|
||||
expect(result.ok).toBe(false);
|
||||
if (!result.ok) {
|
||||
expect(result.status).toBe(422);
|
||||
const paths = result.issues?.map((i) => i.path).sort();
|
||||
expect(paths).toEqual(["gender", "locale"]);
|
||||
expect(result.message).toMatch(/nothing was saved/i);
|
||||
}
|
||||
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("applyProfileUpdate top-level error message", () => {
|
||||
// The `locale`/`timeFormat`/`dateFormat` enums carry no custom Zod
|
||||
// `error:` message, unlike `gender` — so an invalid value here is the
|
||||
// sharpest regression check available for "does the raw Zod default
|
||||
// ever reach the top-level string a person reads". Zod v4's default
|
||||
// enum message is shaped like:
|
||||
// Invalid option: expected one of "de"|"en"
|
||||
const RAW_ZOD_ENUM_PATTERN = /expected one of/i;
|
||||
|
||||
it("never lets the raw Zod enum message reach the top-level `message`", async () => {
|
||||
const result = await applyProfileUpdate(USER_ID, { locale: "fr" });
|
||||
expect(result.ok).toBe(false);
|
||||
if (!result.ok) {
|
||||
expect(result.message).not.toMatch(RAW_ZOD_ENUM_PATTERN);
|
||||
expect(result.message).not.toContain('"de"|"en"');
|
||||
// Relocated, not deleted: the same detail is still available for
|
||||
// whoever needs it (the iOS `details.issues` contract), just not
|
||||
// in the string a person reads first.
|
||||
expect(result.issues?.[0]?.path).toBe("locale");
|
||||
}
|
||||
});
|
||||
|
||||
it("never lets the raw Zod enum message reach `message` for timeFormat either", async () => {
|
||||
const result = await applyProfileUpdate(USER_ID, {
|
||||
timeFormat: "12h",
|
||||
});
|
||||
expect(result.ok).toBe(false);
|
||||
if (!result.ok) {
|
||||
expect(result.message).not.toMatch(RAW_ZOD_ENUM_PATTERN);
|
||||
expect(result.issues?.[0]?.path).toBe("timeFormat");
|
||||
}
|
||||
});
|
||||
|
||||
it("carries a stable errorCode so the web client can localize the sentence", async () => {
|
||||
const result = await applyProfileUpdate(USER_ID, { locale: "fr" });
|
||||
expect(result.ok).toBe(false);
|
||||
if (!result.ok) {
|
||||
expect(result.errorCode).toBe("profile.update.nothingSaved");
|
||||
}
|
||||
});
|
||||
});
|
||||
+127
-10
@@ -12,11 +12,17 @@ import {
|
||||
invalidateUserMedications,
|
||||
invalidateUserProfile,
|
||||
} from "@/lib/cache/invalidate";
|
||||
import { sanitiseZodIssues, type SanitisedZodIssue } from "@/lib/api-response";
|
||||
import { z } from "zod/v4";
|
||||
|
||||
const extendedProfileSchema = profileSchema.extend({
|
||||
displayName: z.string().min(1).max(80).nullable().optional(),
|
||||
locale: z.enum(["de", "en"]).nullable().optional(),
|
||||
locale: z
|
||||
.enum(["de", "en"], {
|
||||
error: "Locale must be de or en.",
|
||||
})
|
||||
.nullable()
|
||||
.optional(),
|
||||
timezone: z
|
||||
.string()
|
||||
.min(1)
|
||||
@@ -26,12 +32,58 @@ const extendedProfileSchema = profileSchema.extend({
|
||||
moodReminderEnabled: z.boolean().optional(),
|
||||
// Hour-cycle display preference. AUTO follows the locale convention,
|
||||
// H12 forces AM/PM, H24 forces 24-hour.
|
||||
timeFormat: z.enum(["AUTO", "H12", "H24"]).optional(),
|
||||
timeFormat: z
|
||||
.enum(["AUTO", "H12", "H24"], {
|
||||
error: "Time format must be AUTO, H12 or H24.",
|
||||
})
|
||||
.optional(),
|
||||
// Date-order display preference. AUTO follows the locale convention,
|
||||
// DMY pins day-month-year, MDY month-day-year, YMD ISO yyyy-MM-dd.
|
||||
dateFormat: z.enum(["AUTO", "DMY", "MDY", "YMD"]).optional(),
|
||||
dateFormat: z
|
||||
.enum(["AUTO", "DMY", "MDY", "YMD"], {
|
||||
error: "Date format must be AUTO, DMY, MDY or YMD.",
|
||||
})
|
||||
.optional(),
|
||||
});
|
||||
|
||||
type ExtendedProfileInput = z.infer<typeof extendedProfileSchema>;
|
||||
|
||||
/**
|
||||
* The profile schema has no cross-field `.refine()` — every field is an
|
||||
* independent scalar column. That means a rejected `gender` has no
|
||||
* bearing on whether `heightCm` is safe to write, so a single bad field
|
||||
* no longer has to take every sibling change down with it (the report:
|
||||
* "editing height and gender together lost both").
|
||||
*
|
||||
* Re-validates each key present in `body` against its own field schema
|
||||
* (`extendedProfileSchema.shape[key]`), independent of the others.
|
||||
* Fields that pass land in `valid`; fields that fail land in `issues`
|
||||
* with `path` set to the field name (a field-level `safeParse` reports
|
||||
* an empty path, since it never saw the parent object). Unknown keys
|
||||
* are silently dropped, matching `z.object`'s own default behaviour.
|
||||
*/
|
||||
function splitValidProfileFields(body: Record<string, unknown>): {
|
||||
valid: Partial<ExtendedProfileInput>;
|
||||
issues: SanitisedZodIssue[];
|
||||
} {
|
||||
const shape = extendedProfileSchema.shape as Record<string, z.ZodTypeAny>;
|
||||
const valid: Record<string, unknown> = {};
|
||||
const issues: SanitisedZodIssue[] = [];
|
||||
for (const key of Object.keys(body)) {
|
||||
const fieldSchema = shape[key];
|
||||
if (!fieldSchema) continue;
|
||||
const result = fieldSchema.safeParse(body[key]);
|
||||
if (result.success) {
|
||||
valid[key] = result.data;
|
||||
} else {
|
||||
for (const issue of result.error.issues) {
|
||||
issues.push({ path: key, code: issue.code, message: issue.message });
|
||||
}
|
||||
}
|
||||
}
|
||||
return { valid: valid as Partial<ExtendedProfileInput>, issues };
|
||||
}
|
||||
|
||||
export interface ApplyProfileResult {
|
||||
ok: true;
|
||||
user: {
|
||||
@@ -58,18 +110,53 @@ export interface ApplyProfileResult {
|
||||
insurerIkNumber: string | null;
|
||||
hasInsuranceNumber: boolean;
|
||||
};
|
||||
/**
|
||||
* Present only when the caller sent at least one invalid field
|
||||
* alongside at least one valid one. The fields named here were NOT
|
||||
* written — everything absent from this list landed. Empty/undefined
|
||||
* means every touched field saved cleanly. The web client resolves
|
||||
* `path` against its own field labels to name the rejected field(s);
|
||||
* the message stays machine-readable, never shown verbatim.
|
||||
*/
|
||||
rejectedFields?: SanitisedZodIssue[];
|
||||
}
|
||||
|
||||
export interface ApplyProfileError {
|
||||
ok: false;
|
||||
status: number;
|
||||
/**
|
||||
* A sentence safe for a person to read — never a raw Zod message (no
|
||||
* enum literals, no validator prose). Native/non-i18n callers that
|
||||
* ignore `errorCode` still get something a person can act on.
|
||||
*/
|
||||
message: string;
|
||||
/**
|
||||
* Stable code the web client resolves through `apiErrors.<errorCode>`
|
||||
* for a localized sentence (see `localizedApiError`). Absent for
|
||||
* non-validation failures (e.g. the 409 email-conflict below) that
|
||||
* already carry a safe, specific message.
|
||||
*/
|
||||
errorCode?: string;
|
||||
/**
|
||||
* Full per-field detail, relocated (not deleted) from the top-level
|
||||
* `message`. Matches the `details.issues` multi-issue envelope the
|
||||
* iOS contract already expects from other write routes.
|
||||
*/
|
||||
issues?: SanitisedZodIssue[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate and persist profile updates for `userId`. Returns either an
|
||||
* `ApplyProfileResult` on success or `ApplyProfileError` with a status
|
||||
* code so callers can shape the wire response themselves.
|
||||
*
|
||||
* The profile schema has no cross-field dependency, so an invalid field
|
||||
* never has to block its valid siblings: a full-object parse is tried
|
||||
* first (the common case), and only on failure does the update fall
|
||||
* back to `splitValidProfileFields` to salvage whatever validated. If
|
||||
* NOTHING in the payload validates, nothing is written — an honest
|
||||
* all-or-nothing rejection naming the field that blocked it, rather
|
||||
* than a raw validator string.
|
||||
*/
|
||||
export async function applyProfileUpdate(
|
||||
userId: string,
|
||||
@@ -77,15 +164,44 @@ export async function applyProfileUpdate(
|
||||
ipAddress?: string | null,
|
||||
): Promise<ApplyProfileResult | ApplyProfileError> {
|
||||
const parsed = extendedProfileSchema.safeParse(body);
|
||||
if (!parsed.success) {
|
||||
return {
|
||||
ok: false,
|
||||
status: 422,
|
||||
message: parsed.error.issues[0]?.message ?? "Validation error",
|
||||
};
|
||||
|
||||
let data: Partial<ExtendedProfileInput>;
|
||||
let rejectedFields: SanitisedZodIssue[] = [];
|
||||
|
||||
if (parsed.success) {
|
||||
data = parsed.data;
|
||||
} else {
|
||||
if (typeof body !== "object" || body === null || Array.isArray(body)) {
|
||||
return {
|
||||
ok: false,
|
||||
status: 422,
|
||||
message: "Nothing was saved. The profile update was not an object.",
|
||||
errorCode: "profile.update.invalidBody",
|
||||
issues: sanitiseZodIssues(parsed.error.issues),
|
||||
};
|
||||
}
|
||||
|
||||
const split = splitValidProfileFields(body as Record<string, unknown>);
|
||||
data = split.valid;
|
||||
rejectedFields = split.issues;
|
||||
|
||||
if (Object.keys(data).length === 0) {
|
||||
// Every touched field failed — there is nothing to salvage.
|
||||
// Name the field that blocked it so the person fixes one thing
|
||||
// instead of re-entering everything.
|
||||
const first = rejectedFields[0];
|
||||
return {
|
||||
ok: false,
|
||||
status: 422,
|
||||
message: first
|
||||
? `Nothing was saved. Fix the "${first.path}" field and try again.`
|
||||
: "Nothing was saved. The submitted profile data was invalid.",
|
||||
errorCode: "profile.update.nothingSaved",
|
||||
issues: rejectedFields,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
const data = parsed.data;
|
||||
const normalizedEmail = data.email ? data.email.trim().toLowerCase() : null;
|
||||
|
||||
if (data.email !== undefined && normalizedEmail) {
|
||||
@@ -218,5 +334,6 @@ export async function applyProfileUpdate(
|
||||
insurerIkNumber: updatedUser.insurerIkNumber,
|
||||
hasInsuranceNumber: updatedUser.insuranceNumberEncrypted != null,
|
||||
},
|
||||
...(rejectedFields.length > 0 ? { rejectedFields } : {}),
|
||||
};
|
||||
}
|
||||
Reference in new issue
Block a user