From bad701ff32c8743bfbc648211e75531516607cf4 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Fri, 24 Jul 2026 01:26:19 -0400 Subject: [PATCH 1/5] Redesign user account page with separated password flow Rename "Preferences" to "Account" in sidebar dropup. Restructure the user page with a profile card (icon, username, role), reordered personal info fields, a dedicated Change Password dialog, phone number min-digit validation, and autofill prevention attributes. Co-Authored-By: Claude Opus 4.6 --- .../src/components/UserDropup/UserDropup.tsx | 4 +- apps/web/src/routes/_app/user.tsx | 251 ++++++++++-------- .../src/utils/__tests__/validation.test.ts | 49 ++++ apps/web/src/utils/validation.ts | 6 + 4 files changed, 198 insertions(+), 112 deletions(-) create mode 100644 apps/web/src/utils/__tests__/validation.test.ts diff --git a/apps/web/src/components/UserDropup/UserDropup.tsx b/apps/web/src/components/UserDropup/UserDropup.tsx index 089d83037..0111639f7 100644 --- a/apps/web/src/components/UserDropup/UserDropup.tsx +++ b/apps/web/src/components/UserDropup/UserDropup.tsx @@ -71,8 +71,8 @@ export const UserDropup = () => { > {t({ - en: 'Preferences', - fr: 'Préférences' + en: 'Account', + fr: 'Compte' })} ; }; +type PasswordFormData = { + confirmPassword: string; + password: string; +}; + const RouteComponent = () => { const currentUser = useAppStore((store) => store.currentUser); - const updateSelfUserMutation = useSelfUpdateUserMutation(); const { resolvedLanguage, t } = useTranslation(); const userInfo = useFindUserQuery(currentUser!.id); + const [isPasswordDialogOpen, setIsPasswordDialogOpen] = useState(false); - const userType: { [key: string]: string } = { - ADMIN: t({ - en: 'Admin', - fr: 'Admin' - }), - GROUP_MANAGER: t({ - en: 'Group Manager', - fr: 'Responsable de groupe' - }), - STANDARD: t({ - en: 'Standard User', - fr: 'Utilisateur standard' - }) + const permissionLabels: { [key: string]: string } = { + ADMIN: t({ en: 'Admin', fr: 'Admin' }), + GROUP_MANAGER: t({ en: 'Group Manager', fr: 'Responsable de groupe' }), + STANDARD: t({ en: 'Standard User', fr: 'Utilisateur standard' }) }; - const userTypes = ['ADMIN', 'GROUP_MANAGER', 'STANDARD']; + const permissionLevel = + typeof userInfo.data.basePermissionLevel === 'string' + ? permissionLabels[userInfo.data.basePermissionLevel] + : undefined; - let fullName: string; - if (userInfo.data.firstName && userInfo.data.lastName) { - fullName = `${userInfo.data.firstName} ${userInfo.data.lastName}`; - } else if (userInfo.data.firstName) { - fullName = userInfo.data.firstName; - } else { - fullName = 'Unnamed User'; - } + const $ProfileFormData = useMemo(() => { + return z.object({ + firstName: z.string().min(1).optional(), + lastName: z.string().min(1).optional(), + dateOfBirth: z.date().optional(), + sex: $Sex.optional(), + email: z.union([z.literal(''), z.email()]).optional(), + phoneNumber: z + .union([ + z.literal(''), + z + .string() + .regex(PHONE_REGEX) + .check((ctx) => { + if (countPhoneDigits(ctx.value) < MIN_PHONE_DIGITS) { + ctx.issues.push({ + code: 'custom', + input: ctx.value, + message: t({ + en: `Phone number must contain at least ${MIN_PHONE_DIGITS} digits`, + fr: `Le numéro de téléphone doit contenir au moins ${MIN_PHONE_DIGITS} chiffres` + }) + }); + } + }) + ]) + .optional() + }) satisfies z.ZodType; + }, [resolvedLanguage]); - const $UpdateUserFormData = useMemo(() => { + const $PasswordFormData = useMemo(() => { return z .object({ - email: z.union([z.literal(''), z.email()]).optional(), - firstName: z.string().min(1).optional(), - - dateOfBirth: z.date().optional(), - lastName: z.string().min(1).optional(), - - confirmPassword: z.string().min(1).optional(), - password: z.string().min(1).optional(), - phoneNumber: z.union([z.literal(''), z.string().regex(PHONE_REGEX)]).optional(), - sex: $Sex.optional() + password: z.string().min(1), + confirmPassword: z.string().min(1) }) .check((ctx) => { - if (ctx.value.password && !estimatePasswordStrength(ctx.value.password).success) { + if (!estimatePasswordStrength(ctx.value.password).success) { ctx.issues.push({ code: 'custom', fatal: true, @@ -94,75 +105,47 @@ const RouteComponent = () => { path: ['confirmPassword'] }); } - }) satisfies z.ZodType; + }) satisfies z.ZodType; }, [resolvedLanguage]); return (
- {t({ - en: 'User Info', - fr: 'Informations utilisateur' - })} + {t({ en: 'Account', fr: 'Compte' })} -
- - {fullName} -

{currentUser?.username ?? fullName}

-

- {userTypes.includes(userInfo.data.basePermissionLevel as string) - ? userType[userInfo.data.basePermissionLevel as string] - : undefined} -

-
+ + +
+ +
+ {currentUser?.username} + {permissionLevel && ( +

+ {t({ en: 'Role', fr: 'Rôle' })}: {permissionLevel} +

+ )} +
+
+
+ +
+ +
{ - return estimatePasswordStrength(password).score; - }, - kind: 'string', - label: t('common.password'), - variant: 'password' - }, - confirmPassword: { - kind: 'string', - label: t('common.confirmPassword'), - variant: 'password' - }, - phoneNumber: { - kind: 'string', - label: t({ - en: 'Phone Number', - fr: 'Numéro de téléphone' - }), - variant: 'input' - } - }, - title: t({ - en: 'Login Credentials', - fr: 'Identifiants de connexion' - }) - }, - { - fields: { - dateOfBirth: { - kind: 'date', - label: t('core.identificationData.dateOfBirth.label') - }, firstName: { kind: 'string', label: t('core.identificationData.firstName.label'), @@ -173,6 +156,10 @@ const RouteComponent = () => { label: t('core.identificationData.lastName.label'), variant: 'input' }, + dateOfBirth: { + kind: 'date', + label: t('core.identificationData.dateOfBirth.label') + }, sex: { kind: 'string', label: t('core.identificationData.sex.label'), @@ -181,6 +168,16 @@ const RouteComponent = () => { MALE: t('core.identificationData.sex.male') }, variant: 'select' + }, + email: { + kind: 'string', + label: t({ en: 'Email', fr: 'Courriel' }), + variant: 'input' + }, + phoneNumber: { + kind: 'string', + label: t({ en: 'Phone Number', fr: 'Numéro de téléphone' }), + variant: 'input' } }, title: t({ @@ -189,31 +186,65 @@ const RouteComponent = () => { }) } ]} + data-form-type="other" + data-lpignore="true" initialValues={{ - dateOfBirth: userInfo.data.dateOfBirth ?? undefined, - email: userInfo.data.email ?? '', firstName: userInfo.data.firstName ?? '', lastName: userInfo.data.lastName ?? '', - phoneNumber: userInfo.data.phoneNumber ?? '', - sex: userInfo.data.sex ?? undefined + dateOfBirth: userInfo.data.dateOfBirth ?? undefined, + sex: userInfo.data.sex ?? undefined, + email: userInfo.data.email ?? '', + phoneNumber: userInfo.data.phoneNumber ?? '' }} key={userInfo.dataUpdatedAt} - validationSchema={$UpdateUserFormData} + submitBtnLabel={t({ en: 'Save', fr: 'Enregistrer' })} + validationSchema={$ProfileFormData} onSubmit={(data) => { - const { confirmPassword, password, ...restData } = data; const filteredData = Object.fromEntries( - Object.entries(restData).filter(([, value]) => value != null && value !== '') + Object.entries(data).filter(([, value]) => value != null && value !== '') ); - void updateSelfUserMutation.mutateAsync({ - data: { - ...filteredData, - ...(password && password === confirmPassword ? { password } : {}) - }, + data: filteredData, id: currentUser!.id }); }} /> + + + + {t({ en: 'Change Password', fr: 'Changer le mot de passe' })} + + + estimatePasswordStrength(password).score, + kind: 'string', + label: t('common.password'), + variant: 'password' + }, + confirmPassword: { + kind: 'string', + label: t('common.confirmPassword'), + variant: 'password' + } + }} + data-form-type="other" + data-lpignore="true" + submitBtnLabel={t({ en: 'Save', fr: 'Enregistrer' })} + validationSchema={$PasswordFormData} + onSubmit={(data) => { + void updateSelfUserMutation + .mutateAsync({ + data: { password: data.password }, + id: currentUser!.id + }) + .then(() => setIsPasswordDialogOpen(false)); + }} + /> + + +
); }; diff --git a/apps/web/src/utils/__tests__/validation.test.ts b/apps/web/src/utils/__tests__/validation.test.ts new file mode 100644 index 000000000..2aceab9f0 --- /dev/null +++ b/apps/web/src/utils/__tests__/validation.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from 'vitest'; + +import { countPhoneDigits, MIN_PHONE_DIGITS, PHONE_REGEX } from '../validation'; + +describe('countPhoneDigits', () => { + it('should count only numeric characters', () => { + expect(countPhoneDigits('+1 (514) 555-1234')).toBe(11); + }); + + it('should return 0 for an empty string', () => { + expect(countPhoneDigits('')).toBe(0); + }); + + it('should ignore dashes, spaces, and parentheses', () => { + expect(countPhoneDigits('(555) 123-4567')).toBe(10); + }); + + it('should count digits in a plain numeric string', () => { + expect(countPhoneDigits('5551234')).toBe(7); + }); +}); + +describe('PHONE_REGEX', () => { + it('should accept a standard North American number', () => { + expect(PHONE_REGEX.test('+15145551234')).toBe(true); + }); + + it('should accept a number with spaces and dashes', () => { + expect(PHONE_REGEX.test('514-555-1234')).toBe(true); + }); + + it('should accept a number with parentheses', () => { + expect(PHONE_REGEX.test('(514) 555-1234')).toBe(true); + }); + + it('should reject a string with fewer than 5 characters', () => { + expect(PHONE_REGEX.test('1234')).toBe(false); + }); + + it('should reject a purely alphabetic string', () => { + expect(PHONE_REGEX.test('abcdefgh')).toBe(false); + }); +}); + +describe('MIN_PHONE_DIGITS', () => { + it('should be 7', () => { + expect(MIN_PHONE_DIGITS).toBe(7); + }); +}); diff --git a/apps/web/src/utils/validation.ts b/apps/web/src/utils/validation.ts index 36fd54d9f..4362e8cb0 100644 --- a/apps/web/src/utils/validation.ts +++ b/apps/web/src/utils/validation.ts @@ -1 +1,7 @@ export const PHONE_REGEX = new RegExp(/^(?=.{5,})\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/); + +export const MIN_PHONE_DIGITS = 7; + +export function countPhoneDigits(phone: string): number { + return phone.replace(/\D/g, '').length; +} From b7f8a7dc9a7977b4b484b6139f079642f43f2236 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Tue, 28 Jul 2026 01:51:18 -0400 Subject: [PATCH 2/5] fix(web): carry translated validation messages and allow clearing contact details A phone or email field declared as `z.union([z.literal(''), ...])` loses the issues raised inside the union member: zod reports a single `invalid_union`, so the field read "Invalid input", in English, instead of saying what was wrong. Both are now one `z.string()` with a check that permits the blank string, built from a shared `blankOr` helper in `utils/validation.ts`. `PHONE_REGEX` is no longer exported, so the format rule and the digit minimum cannot be applied apart from each other. The account page enforced both while the two admin forms enforced only the format, so an admin could save a number the user was then blocked from re-saving. Folding the digit count in also let the regex's overlapping `(?=.{5,})` lookahead go, leaving one rule. `$UpdateUserData` makes `email` and `phoneNumber` nullish so an update can clear them, and both forms send null for a field left blank. Previously the account form filtered empty values out of the payload, so an email once set could never be removed, and the admin sheet sent `''`, which the API rejected. Also from review: `mutate` with `onSuccess` in place of an uncaught `mutateAsync().then()`, the themed primary button instead of a hardcoded `bg-sky-700`, translation keys instead of strings written inline twice, a mapped type over the closed permission-level union, and `user-dropup-account` for the renamed menu entry. Adds unit tests for both schemas and for the nullable update contract, and an e2e page object and spec for /user covering the dropup label, the password dialog, and clearing contact details. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/components/UserDropup/UserDropup.tsx | 6 +- .../src/routes/_app/admin/users/create.tsx | 16 +--- .../web/src/routes/_app/admin/users/index.tsx | 25 ++++-- apps/web/src/routes/_app/user.tsx | 82 ++++++------------- apps/web/src/translations/common.json | 4 + apps/web/src/translations/user.json | 7 +- .../src/utils/__tests__/validation.test.ts | 60 ++++++++++---- apps/web/src/utils/validation.ts | 51 +++++++++++- packages/schemas/src/user/user.test.ts | 28 +++++++ packages/schemas/src/user/user.ts | 5 +- testing/src/pages/_app/user.page.ts | 36 ++++++++ testing/src/specs/admin-management.spec.ts | 22 +++++ testing/src/specs/user-account.spec.ts | 67 +++++++++++++++ testing/src/support/fixtures.ts | 23 ++++-- 14 files changed, 321 insertions(+), 111 deletions(-) create mode 100644 packages/schemas/src/user/user.test.ts create mode 100644 testing/src/pages/_app/user.page.ts create mode 100644 testing/src/specs/user-account.spec.ts diff --git a/apps/web/src/components/UserDropup/UserDropup.tsx b/apps/web/src/components/UserDropup/UserDropup.tsx index 0111639f7..5f212213a 100644 --- a/apps/web/src/components/UserDropup/UserDropup.tsx +++ b/apps/web/src/components/UserDropup/UserDropup.tsx @@ -3,7 +3,7 @@ import { useState } from 'react'; import { ArrowToggle, DropdownMenu } from '@douglasneuroinformatics/libui/components'; import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; import { useNavigate } from '@tanstack/react-router'; -import { Info, LogOutIcon, SchoolIcon, SettingsIcon } from 'lucide-react'; +import { Info, LogOutIcon, SchoolIcon, UserCogIcon } from 'lucide-react'; import { useAppStore } from '@/store'; @@ -64,12 +64,12 @@ export const UserDropup = () => { { void navigate({ to: '/user' }); }} > - + {t({ en: 'Account', fr: 'Compte' diff --git a/apps/web/src/routes/_app/admin/users/create.tsx b/apps/web/src/routes/_app/admin/users/create.tsx index 9a6311023..8cef22df3 100644 --- a/apps/web/src/routes/_app/admin/users/create.tsx +++ b/apps/web/src/routes/_app/admin/users/create.tsx @@ -12,7 +12,7 @@ import { z } from 'zod/v4'; import { PageHeader } from '@/components/PageHeader'; import { useCreateUserMutation } from '@/hooks/useCreateUserMutation'; import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; -import { PHONE_REGEX } from '@/utils/validation'; +import { $PhoneNumber } from '@/utils/validation'; const PASSWORD_ERROR_TRANSLATION_KEYS = { INSUFFICIENT_PASSWORD_STRENGTH: 'common.insufficientPasswordStrength', @@ -199,7 +199,8 @@ const RouteComponent = () => { .extend({ basePermissionLevel: $BasePermissionLevel, groupIds: z.set(z.string()).optional(), - confirmPassword: z.string().min(1) + confirmPassword: z.string().min(1), + phoneNumber: $PhoneNumber(t).optional() }) .check((ctx) => { if (!estimatePasswordStrength(ctx.value.password).success) { @@ -230,17 +231,6 @@ const RouteComponent = () => { path: ['confirmPassword'] }); } - if (ctx.value.phoneNumber && !PHONE_REGEX.test(ctx.value.phoneNumber)) { - ctx.issues.push({ - code: 'custom', - input: ctx.value.phoneNumber, - message: t({ - en: 'Invalid Phone number', - fr: 'Numéro de téléphone invalide' - }), - path: ['phoneNumber'] - }); - } })} onSubmit={(data) => handleSubmit({ ...data, groupIds: Array.from(data.groupIds ?? []) })} /> diff --git a/apps/web/src/routes/_app/admin/users/index.tsx b/apps/web/src/routes/_app/admin/users/index.tsx index 6340f1bc7..ebe82e07f 100644 --- a/apps/web/src/routes/_app/admin/users/index.tsx +++ b/apps/web/src/routes/_app/admin/users/index.tsx @@ -19,7 +19,7 @@ import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; import { useUpdateUserMutation } from '@/hooks/useUpdateUserMutation'; import { usersQueryOptions, useUsersQuery } from '@/hooks/useUsersQuery'; import { useAppStore } from '@/store'; -import { PHONE_REGEX } from '@/utils/validation'; +import { $Email, $PhoneNumber, clearedIfBlank } from '@/utils/validation'; type UpdateUserFormData = { additionalPermissions?: Partial[]; @@ -55,10 +55,10 @@ const UpdateUserForm: React.FC<{ additionalPermissions: z.array($UserPermission.partial()).optional(), confirmPassword: z.string().min(1).optional(), disabled: z.boolean().optional(), - email: z.union([z.literal(''), z.email()]).optional(), + email: $Email(t).optional(), groupIds: z.set(z.string()), password: z.string().min(1).optional(), - phoneNumber: z.union([z.literal(''), z.string().regex(PHONE_REGEX)]).optional() + phoneNumber: $PhoneNumber(t).optional() }) .transform((arg) => { const firstPermission = arg.additionalPermissions?.[0]; @@ -455,12 +455,19 @@ const RouteComponent = () => { deleteUserMutation.mutate({ id: selectedUser!.id }); setSelectedUser(null); }, - onSubmit: ({ confirmPassword: _, groupIds, ...data }) => { - void updateUserMutation - .mutateAsync({ data: { groupIds: Array.from(groupIds), ...data }, id: selectedUser!.id }) - .then(() => { - setSelectedUser(null); - }); + onSubmit: ({ confirmPassword: _, email, groupIds, phoneNumber, ...data }) => { + updateUserMutation.mutate( + { + data: { + ...data, + email: clearedIfBlank(email), + groupIds: Array.from(groupIds), + phoneNumber: clearedIfBlank(phoneNumber) + }, + id: selectedUser!.id + }, + { onSuccess: () => setSelectedUser(null) } + ); } }} /> diff --git a/apps/web/src/routes/_app/user.tsx b/apps/web/src/routes/_app/user.tsx index 8231416fa..a6cbf45d1 100644 --- a/apps/web/src/routes/_app/user.tsx +++ b/apps/web/src/routes/_app/user.tsx @@ -5,6 +5,7 @@ import { estimatePasswordStrength } from '@douglasneuroinformatics/libpasswd'; import { Button, Card, Dialog, Form, Heading } from '@douglasneuroinformatics/libui/components'; import { useTranslation } from '@douglasneuroinformatics/libui/hooks'; import { $Sex } from '@opendatacapture/schemas/subject'; +import type { BasePermissionLevel } from '@opendatacapture/schemas/user'; import { createFileRoute } from '@tanstack/react-router'; import { KeyRoundIcon } from 'lucide-react'; import { z } from 'zod/v4'; @@ -14,7 +15,7 @@ import { UserIcon } from '@/components/UserIcon'; import { useFindUserQuery } from '@/hooks/useFindUserQuery'; import { useSelfUpdateUserMutation } from '@/hooks/useSelfUpdateUserMutation'; import { useAppStore } from '@/store'; -import { countPhoneDigits, MIN_PHONE_DIGITS, PHONE_REGEX } from '@/utils/validation'; +import { $Email, $PhoneNumber, clearedIfBlank } from '@/utils/validation'; type ProfileFormData = { dateOfBirth?: Date | undefined; @@ -37,16 +38,15 @@ const RouteComponent = () => { const userInfo = useFindUserQuery(currentUser!.id); const [isPasswordDialogOpen, setIsPasswordDialogOpen] = useState(false); - const permissionLabels: { [key: string]: string } = { + const permissionLabels: { [K in BasePermissionLevel]: string } = { ADMIN: t({ en: 'Admin', fr: 'Admin' }), GROUP_MANAGER: t({ en: 'Group Manager', fr: 'Responsable de groupe' }), STANDARD: t({ en: 'Standard User', fr: 'Utilisateur standard' }) }; - const permissionLevel = - typeof userInfo.data.basePermissionLevel === 'string' - ? permissionLabels[userInfo.data.basePermissionLevel] - : undefined; + const permissionLevel = userInfo.data.basePermissionLevel + ? permissionLabels[userInfo.data.basePermissionLevel] + : undefined; const $ProfileFormData = useMemo(() => { return z.object({ @@ -54,27 +54,8 @@ const RouteComponent = () => { lastName: z.string().min(1).optional(), dateOfBirth: z.date().optional(), sex: $Sex.optional(), - email: z.union([z.literal(''), z.email()]).optional(), - phoneNumber: z - .union([ - z.literal(''), - z - .string() - .regex(PHONE_REGEX) - .check((ctx) => { - if (countPhoneDigits(ctx.value) < MIN_PHONE_DIGITS) { - ctx.issues.push({ - code: 'custom', - input: ctx.value, - message: t({ - en: `Phone number must contain at least ${MIN_PHONE_DIGITS} digits`, - fr: `Le numéro de téléphone doit contenir au moins ${MIN_PHONE_DIGITS} chiffres` - }) - }); - } - }) - ]) - .optional() + email: $Email(t).optional(), + phoneNumber: $PhoneNumber(t).optional() }) satisfies z.ZodType; }, [resolvedLanguage]); @@ -116,7 +97,7 @@ const RouteComponent = () => { - +
@@ -128,17 +109,10 @@ const RouteComponent = () => { )}
-
- -
+ { }, email: { kind: 'string', - label: t({ en: 'Email', fr: 'Courriel' }), + label: t('common.email'), variant: 'input' }, phoneNumber: { kind: 'string', - label: t({ en: 'Phone Number', fr: 'Numéro de téléphone' }), + label: t('common.phoneNumber'), variant: 'input' } }, @@ -188,6 +162,7 @@ const RouteComponent = () => { ]} data-form-type="other" data-lpignore="true" + data-testid="profile-form" initialValues={{ firstName: userInfo.data.firstName ?? '', lastName: userInfo.data.lastName ?? '', @@ -197,14 +172,11 @@ const RouteComponent = () => { phoneNumber: userInfo.data.phoneNumber ?? '' }} key={userInfo.dataUpdatedAt} - submitBtnLabel={t({ en: 'Save', fr: 'Enregistrer' })} + submitBtnLabel={t('common.save')} validationSchema={$ProfileFormData} - onSubmit={(data) => { - const filteredData = Object.fromEntries( - Object.entries(data).filter(([, value]) => value != null && value !== '') - ); - void updateSelfUserMutation.mutateAsync({ - data: filteredData, + onSubmit={({ email, phoneNumber, ...rest }) => { + updateSelfUserMutation.mutate({ + data: { ...rest, email: clearedIfBlank(email), phoneNumber: clearedIfBlank(phoneNumber) }, id: currentUser!.id }); }} @@ -212,7 +184,7 @@ const RouteComponent = () => { - {t({ en: 'Change Password', fr: 'Changer le mot de passe' })} + {t('user.changePassword')} { }} data-form-type="other" data-lpignore="true" - submitBtnLabel={t({ en: 'Save', fr: 'Enregistrer' })} + submitBtnLabel={t('common.save')} validationSchema={$PasswordFormData} onSubmit={(data) => { - void updateSelfUserMutation - .mutateAsync({ - data: { password: data.password }, - id: currentUser!.id - }) - .then(() => setIsPasswordDialogOpen(false)); + updateSelfUserMutation.mutate( + { data: { password: data.password }, id: currentUser!.id }, + { onSuccess: () => setIsPasswordDialogOpen(false) } + ); }} /> diff --git a/apps/web/src/translations/common.json b/apps/web/src/translations/common.json index e59972ec7..c615fad89 100644 --- a/apps/web/src/translations/common.json +++ b/apps/web/src/translations/common.json @@ -173,6 +173,10 @@ "en": "Research", "fr": "Recherche" }, + "save": { + "en": "Save", + "fr": "Enregistrer" + }, "session": { "en": "Session", "fr": "Session" diff --git a/apps/web/src/translations/user.json b/apps/web/src/translations/user.json index 0967ef424..e840d443d 100644 --- a/apps/web/src/translations/user.json +++ b/apps/web/src/translations/user.json @@ -1 +1,6 @@ -{} +{ + "changePassword": { + "en": "Change Password", + "fr": "Changer le mot de passe" + } +} diff --git a/apps/web/src/utils/__tests__/validation.test.ts b/apps/web/src/utils/__tests__/validation.test.ts index 2aceab9f0..0515e47a5 100644 --- a/apps/web/src/utils/__tests__/validation.test.ts +++ b/apps/web/src/utils/__tests__/validation.test.ts @@ -1,6 +1,18 @@ +import type { TranslateFunction, TranslationKey } from '@douglasneuroinformatics/libui/i18n'; import { describe, expect, it } from 'vitest'; +import { z } from 'zod/v4'; -import { countPhoneDigits, MIN_PHONE_DIGITS, PHONE_REGEX } from '../validation'; +import { $Email, $PhoneNumber, countPhoneDigits, MIN_PHONE_DIGITS } from '../validation'; + +const t: TranslateFunction = (arg) => (typeof arg === 'string' ? arg : (arg.en ?? '')); + +const parseWith = (schema: z.ZodType) => (value: string) => { + const result = schema.safeParse(value); + return { issues: result.error?.issues ?? [], success: result.success }; +}; + +const parseEmail = parseWith($Email(t)); +const parsePhoneNumber = parseWith($PhoneNumber(t)); describe('countPhoneDigits', () => { it('should count only numeric characters', () => { @@ -14,36 +26,50 @@ describe('countPhoneDigits', () => { it('should ignore dashes, spaces, and parentheses', () => { expect(countPhoneDigits('(555) 123-4567')).toBe(10); }); +}); - it('should count digits in a plain numeric string', () => { - expect(countPhoneDigits('5551234')).toBe(7); +describe('$PhoneNumber', () => { + it('should accept a blank value', () => { + expect(parsePhoneNumber('').success).toBe(true); }); -}); -describe('PHONE_REGEX', () => { it('should accept a standard North American number', () => { - expect(PHONE_REGEX.test('+15145551234')).toBe(true); + expect(parsePhoneNumber('+15145551234').success).toBe(true); }); - it('should accept a number with spaces and dashes', () => { - expect(PHONE_REGEX.test('514-555-1234')).toBe(true); + it('should accept a number with spaces, dashes, and parentheses', () => { + expect(parsePhoneNumber('(514) 555-1234').success).toBe(true); }); - it('should accept a number with parentheses', () => { - expect(PHONE_REGEX.test('(514) 555-1234')).toBe(true); + it('should accept a number with exactly the minimum number of digits', () => { + expect(parsePhoneNumber('12-34-567').success).toBe(true); }); - it('should reject a string with fewer than 5 characters', () => { - expect(PHONE_REGEX.test('1234')).toBe(false); + it('should reject a well-formed number with too few digits', () => { + const { issues } = parsePhoneNumber('12-34-56'); + expect(issues).toHaveLength(1); + expect(issues[0]!.message).toBe(`Phone number must contain at least ${MIN_PHONE_DIGITS} digits`); }); - it('should reject a purely alphabetic string', () => { - expect(PHONE_REGEX.test('abcdefgh')).toBe(false); + it('should reject a malformed number with a format message', () => { + const { issues } = parsePhoneNumber('abcdefgh'); + expect(issues).toHaveLength(1); + expect(issues[0]!.message).toBe('Invalid phone number'); }); }); -describe('MIN_PHONE_DIGITS', () => { - it('should be 7', () => { - expect(MIN_PHONE_DIGITS).toBe(7); +describe('$Email', () => { + it('should accept a blank value', () => { + expect(parseEmail('').success).toBe(true); + }); + + it('should accept a well-formed address', () => { + expect(parseEmail('jane.doe@example.org').success).toBe(true); + }); + + it('should reject a malformed address with a translated message', () => { + const { issues } = parseEmail('jane.doe@'); + expect(issues).toHaveLength(1); + expect(issues[0]!.message).toBe('Invalid email address'); }); }); diff --git a/apps/web/src/utils/validation.ts b/apps/web/src/utils/validation.ts index 4362e8cb0..08d399cf6 100644 --- a/apps/web/src/utils/validation.ts +++ b/apps/web/src/utils/validation.ts @@ -1,7 +1,52 @@ -export const PHONE_REGEX = new RegExp(/^(?=.{5,})\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/); +import type { TranslateFunction, TranslationKey } from '@douglasneuroinformatics/libui/i18n'; +import { z } from 'zod/v4'; -export const MIN_PHONE_DIGITS = 7; +const PHONE_REGEX = new RegExp(/^\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/); -export function countPhoneDigits(phone: string): number { +const MIN_PHONE_DIGITS = 7; + +/** + * A field that may be left blank, and is otherwise rejected with the message `findError` returns. + * This is one string schema rather than a union with `z.literal('')` because zod discards the + * issues raised inside a union member, reporting an untranslated `invalid_union` error instead. + */ +function blankOr(findError: (value: string) => null | string) { + return z.string().check((ctx) => { + const message = ctx.value === '' ? null : findError(ctx.value); + if (message) { + ctx.issues.push({ code: 'custom', input: ctx.value, message }); + } + }); +} + +function countPhoneDigits(phone: string): number { return phone.replace(/\D/g, '').length; } + +/** The submit-side counterpart of `blankOr`: a field left blank is cleared, which the API spells null. */ +function clearedIfBlank(value: string | undefined) { + return value === '' ? null : value; +} + +function $Email(t: TranslateFunction) { + return blankOr((value) => + z.email().safeParse(value).success ? null : t({ en: 'Invalid email address', fr: 'Adresse courriel invalide' }) + ); +} + +function $PhoneNumber(t: TranslateFunction) { + return blankOr((value) => { + if (!PHONE_REGEX.test(value)) { + return t({ en: 'Invalid phone number', fr: 'Numéro de téléphone invalide' }); + } + if (countPhoneDigits(value) < MIN_PHONE_DIGITS) { + return t({ + en: `Phone number must contain at least ${MIN_PHONE_DIGITS} digits`, + fr: `Le numéro de téléphone doit contenir au moins ${MIN_PHONE_DIGITS} chiffres` + }); + } + return null; + }); +} + +export { $Email, $PhoneNumber, clearedIfBlank, countPhoneDigits, MIN_PHONE_DIGITS }; diff --git a/packages/schemas/src/user/user.test.ts b/packages/schemas/src/user/user.test.ts new file mode 100644 index 000000000..dcdf9703c --- /dev/null +++ b/packages/schemas/src/user/user.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from 'vitest'; + +import { $CreateUserData, $SelfUpdateUserData } from './user.js'; + +describe('$SelfUpdateUserData', () => { + it('should accept null contact details, so an update can clear them', () => { + expect($SelfUpdateUserData.safeParse({ email: null, phoneNumber: null }).success).toBe(true); + }); + + it('should reject a blank email, which would otherwise be stored as one', () => { + expect($SelfUpdateUserData.safeParse({ email: '' }).success).toBe(false); + }); +}); + +describe('$CreateUserData', () => { + it('should reject null contact details, which are only clearable on update', () => { + const data = { + basePermissionLevel: 'STANDARD', + email: null, + firstName: 'Jane', + groupIds: [], + lastName: 'Doe', + password: 'password', + username: 'jane.doe' + }; + expect($CreateUserData.safeParse(data).success).toBe(false); + }); +}); diff --git a/packages/schemas/src/user/user.ts b/packages/schemas/src/user/user.ts index 19ccf5243..e8078beec 100644 --- a/packages/schemas/src/user/user.ts +++ b/packages/schemas/src/user/user.ts @@ -53,9 +53,12 @@ export const $CreateUserData = $User sex: $Sex.optional() }); +/** Optional contact details are nullable here, and only here, so an update can clear one. */ export type UpdateUserData = z.infer; export const $UpdateUserData = $CreateUserData.partial().extend({ - additionalPermissions: $Permissions.optional() + additionalPermissions: $Permissions.optional(), + email: z.email().nullish(), + phoneNumber: z.string().nullish() }); export type $SelfUpdateUserData = z.infer; diff --git a/testing/src/pages/_app/user.page.ts b/testing/src/pages/_app/user.page.ts new file mode 100644 index 000000000..dc1bc7851 --- /dev/null +++ b/testing/src/pages/_app/user.page.ts @@ -0,0 +1,36 @@ +import type { Locator, Page } from '@playwright/test'; + +import { AppPage } from './route.page'; + +export class UserAccountPage extends AppPage { + readonly changePasswordButton: Locator; + readonly emailInput: Locator; + readonly pageHeader: Locator; + readonly passwordDialog: Locator; + readonly phoneNumberInput: Locator; + readonly profileForm: Locator; + + constructor(page: Page) { + super(page); + this.changePasswordButton = page.getByRole('button', { name: 'Change Password' }); + this.pageHeader = page.getByTestId('page-header'); + this.passwordDialog = page.getByRole('dialog', { name: 'Change Password' }); + this.profileForm = page.getByTestId('profile-form'); + this.emailInput = this.profileForm.getByLabel('Email'); + this.phoneNumberInput = this.profileForm.getByLabel('Phone Number'); + } + + async openPasswordDialog(): Promise { + await this.changePasswordButton.click(); + } + + async saveProfile(): Promise { + await this.profileForm.getByRole('button', { name: 'Submit' }).click(); + } + + async submitNewPassword(password: string): Promise { + await this.passwordDialog.getByLabel('Password', { exact: true }).fill(password); + await this.passwordDialog.getByLabel('Confirm Password').fill(password); + await this.passwordDialog.getByRole('button', { name: 'Submit' }).click(); + } +} diff --git a/testing/src/specs/admin-management.spec.ts b/testing/src/specs/admin-management.spec.ts index ecd67abb1..821103861 100644 --- a/testing/src/specs/admin-management.spec.ts +++ b/testing/src/specs/admin-management.spec.ts @@ -29,4 +29,26 @@ test.describe('admin management', () => { // The admin created during setup is always present. await expect(page.getByTestId('data-table-body')).toContainText('admin'); }); + + test("should clear a user's email from the edit sheet", async ({ api, authenticateAs, page }) => { + const group = await api.createGroup(); + const { user } = await api.createUser({ email: 'contact@example.org', groupIds: [group.id] }); + + await authenticateAs('ADMIN'); + await page.goto('/admin/users'); + // Search so the seeded user is the only row, whichever page it would otherwise land on. + await page.getByTestId('data-table-search-bar').getByRole('searchbox').fill(user.username); + + const editSheet = page.getByTestId('admin-user-edit-sheet'); + const emailInput = editSheet.getByLabel('Email'); + + await page.getByTestId('data-table-row').dblclick(); + await expect(emailInput).toHaveValue('contact@example.org'); + await emailInput.clear(); + await editSheet.getByRole('button', { name: 'Submit' }).click(); + await expect(editSheet).toBeHidden(); + + await page.getByTestId('data-table-row').dblclick(); + await expect(emailInput).toHaveValue(''); + }); }); diff --git a/testing/src/specs/user-account.spec.ts b/testing/src/specs/user-account.spec.ts new file mode 100644 index 000000000..494ad09a0 --- /dev/null +++ b/testing/src/specs/user-account.spec.ts @@ -0,0 +1,67 @@ +import { UserAccountPage } from '../pages/_app/user.page'; +import { SEEDED_USER_PASSWORD } from '../support/constants'; +import { expect, test } from '../support/fixtures'; + +test.describe('user account', () => { + test('should reach the account page from the sidebar dropup @smoke', async ({ getPageModel, page }) => { + const dashboardPage = await getPageModel('/dashboard'); + + await dashboardPage.sidebar.getByTestId('user-dropup-trigger').click(); + const accountItem = page.getByTestId('user-dropup-account'); + await expect(accountItem).toContainText('Account'); + await accountItem.click(); + + const userAccountPage = new UserAccountPage(page); + await expect(page).toHaveURL('/user'); + await expect(userAccountPage.pageHeader).toContainText('Account'); + }); + + test('should keep the dialog open when the new password is too weak', async ({ getPageModel }) => { + const userAccountPage = await getPageModel('/user'); + + await userAccountPage.openPasswordDialog(); + await userAccountPage.submitNewPassword('password'); + + await expect(userAccountPage.passwordDialog.getByTestId('error-message-text')).toContainText( + 'Insufficient password strength' + ); + await expect(userAccountPage.passwordDialog).toBeVisible(); + }); + + test('should close the dialog after the password is changed', async ({ api, authenticateAs, page }) => { + const group = await api.createGroup(); + const { credentials } = await api.createUser({ groupIds: [group.id] }); + await authenticateAs(credentials); + + const userAccountPage = new UserAccountPage(page); + await userAccountPage.goto('/user'); + await userAccountPage.openPasswordDialog(); + await userAccountPage.submitNewPassword(`${SEEDED_USER_PASSWORD}_Changed`); + + await expect(userAccountPage.passwordDialog).toBeHidden(); + }); + + test('should clear contact details that are saved blank', async ({ api, authenticateAs, page }) => { + const group = await api.createGroup(); + const { credentials } = await api.createUser({ + email: 'contact@example.org', + groupIds: [group.id], + phoneNumber: '5145551234' + }); + await authenticateAs(credentials); + + const userAccountPage = new UserAccountPage(page); + await userAccountPage.goto('/user'); + await expect(userAccountPage.emailInput).toHaveValue('contact@example.org'); + await expect(userAccountPage.phoneNumberInput).toHaveValue('5145551234'); + + await userAccountPage.emailInput.clear(); + await userAccountPage.phoneNumberInput.clear(); + await userAccountPage.saveProfile(); + await expect(page.getByRole('heading', { name: 'Success' })).toBeVisible(); + + await page.reload(); + await expect(userAccountPage.emailInput).toHaveValue(''); + await expect(userAccountPage.phoneNumberInput).toHaveValue(''); + }); +}); diff --git a/testing/src/support/fixtures.ts b/testing/src/support/fixtures.ts index 4af68e0d7..4f73d2d03 100644 --- a/testing/src/support/fixtures.ts +++ b/testing/src/support/fixtures.ts @@ -1,5 +1,6 @@ /* eslint-disable no-empty-pattern */ +import type { $LoginCredentials } from '@opendatacapture/schemas/auth'; import { request as apiRequestFactory, test as base, expect } from '@playwright/test'; import type { APIRequestContext } from '@playwright/test'; @@ -9,6 +10,7 @@ import { DatahubPage } from '../pages/_app/datahub/index.page'; import { AccessibleInstrumentsPage } from '../pages/_app/instruments/accessible-instruments.page'; import { RemoteAssignmentPage } from '../pages/_app/session/remote-assignment.page'; import { StartSessionPage } from '../pages/_app/session/start-session.page'; +import { UserAccountPage } from '../pages/_app/user.page'; import { LoginPage } from '../pages/auth/login.page'; import { ApiClient } from './api-client'; import { ADMIN } from './constants'; @@ -24,7 +26,8 @@ const pageModels = { '/datahub/$subjectId/table': SubjectDataTablePage, '/instruments/accessible-instruments': AccessibleInstrumentsPage, '/session/remote-assignment': RemoteAssignmentPage, - '/session/start-session': StartSessionPage + '/session/start-session': StartSessionPage, + '/user': UserAccountPage } satisfies { [K in RouteTo]?: any }; type PageModels = typeof pageModels; @@ -51,11 +54,12 @@ type TestFixtures = { /** First-run gating written to localStorage; override per file with `test.use({ appState })`. */ appState: AppState; /** - * Injects a role's token without navigating, so the test can drive navigation itself. Use this - * (rather than `getPageModel`) when the expected outcome is a redirect, since `getPageModel` - * asserts it landed on the requested route. + * Injects a token without navigating, so the test can drive navigation itself. Use this (rather + * than `getPageModel`) when the expected outcome is a redirect, since `getPageModel` asserts it + * landed on the requested route. Pass a role to reuse the worker's cached user, or credentials + * to act as a user the test seeded itself — necessary when the test mutates that user's login. */ - authenticateAs: (role: Role) => Promise; + authenticateAs: (roleOrCredentials: $LoginCredentials | Role) => Promise; /** Navigates to a route as `actingRole` and returns its page object. */ getPageModel: GetPageModel; /** Short run-unique suffix for naming seeded data in this test. */ @@ -85,9 +89,12 @@ export const test = base.extend({ { scope: 'worker' } ], appState: [{ isDisclaimerAccepted: true, isWalkthroughComplete: true }, { option: true }], - authenticateAs: async ({ appState, page, roleToken }, use) => { - await use(async (role) => { - const accessToken = await roleToken(role); + authenticateAs: async ({ apiRequestContext, appState, page, roleToken }, use) => { + await use(async (roleOrCredentials) => { + const accessToken = + typeof roleOrCredentials === 'string' + ? await roleToken(roleOrCredentials) + : await ApiClient.login(apiRequestContext, roleOrCredentials); await page.addInitScript( (injected) => { window.__PLAYWRIGHT_ACCESS_TOKEN__ = injected.accessToken; From 3bd731e5540ace5d58d61a55c5ffad098ed32052 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Tue, 28 Jul 2026 08:47:13 -0400 Subject: [PATCH 3/5] test(testing): name seeded emails with uniqueId, and document authenticateAs The suite runs fullyParallel against one shared database, so testing/AGENTS.md requires seeded data to be uniquely named. Both new specs seeded a fixed contact@example.org. Also records that authenticateAs now takes credentials as well as a role, so the fixtures table no longer disagrees with the fixture. Co-Authored-By: Claude Opus 5 (1M context) --- testing/AGENTS.md | 22 ++++++++++++---------- testing/src/specs/admin-management.spec.ts | 7 ++++--- testing/src/specs/user-account.spec.ts | 7 ++++--- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/testing/AGENTS.md b/testing/AGENTS.md index fb5dbe6ff..a230104d4 100644 --- a/testing/AGENTS.md +++ b/testing/AGENTS.md @@ -28,7 +28,9 @@ is `.agents/docs/playbooks/add-e2e-test.md`; the tier-by-tier picture is `expect(page).toHaveURL(url)`). When the expected outcome _is_ a redirect, use `authenticateAs` plus a raw `page.goto` instead — see the standard-user cases in `src/specs/authorization.spec.ts`. - **`authenticateAs` works through `page.addInitScript`**, so it must run before the navigation it - is meant to affect. + is meant to affect. Passing a role reuses the worker's cached user; pass `$LoginCredentials` from + `api.createUser()` instead when the test mutates that user's own login, so it cannot invalidate + the cached token every other spec in the worker shares. - **`.env` at the repo root must exist.** `src/support/env.ts` reads `API_DEV_SERVER_PORT`, `GATEWAY_DEV_SERVER_PORT` and `WEB_DEV_SERVER_PORT` and throws while `playwright.config.ts` is loading if any is missing. `./scripts/generate-env.sh` produces it. @@ -57,15 +59,15 @@ A page object is only reachable from a spec once it is registered in the `pageMo ## Fixtures -| Fixture | Scope | Notes | -| ---------------------- | ----------- | -------------------------------------------------------------------------- | -| `getPageModel` | test | Authenticates as `actingRole`, navigates, returns the page object | -| `authenticateAs(role)` | test | Injects a token without navigating | -| `actingRole` | test option | Default `GROUP_MANAGER`; override with `test.use({ actingRole: 'ADMIN' })` | -| `appState` | test option | localStorage first-run gating; both flags default to accepted/complete | -| `uniqueId` | test | Short random suffix for seeded data | -| `api` | worker | `ApiClient` as admin — `createGroup()` / `createUser()` for preconditions | -| `roleToken(role)` | worker | Seeds a group + user per role once, then caches the token | +| Fixture | Scope | Notes | +| --------------------- | ----------- | -------------------------------------------------------------------------- | +| `getPageModel` | test | Authenticates as `actingRole`, navigates, returns the page object | +| `authenticateAs(who)` | test | Injects a token without navigating; takes a role or `$LoginCredentials` | +| `actingRole` | test option | Default `GROUP_MANAGER`; override with `test.use({ actingRole: 'ADMIN' })` | +| `appState` | test option | localStorage first-run gating; both flags default to accepted/complete | +| `uniqueId` | test | Short random suffix for seeded data | +| `api` | worker | `ApiClient` as admin — `createGroup()` / `createUser()` for preconditions | +| `roleToken(role)` | worker | Seeds a group + user per role once, then caches the token | Set up preconditions over the API with the `api` fixture rather than by clicking through the UI; only drive the UI for the behaviour actually under test. diff --git a/testing/src/specs/admin-management.spec.ts b/testing/src/specs/admin-management.spec.ts index 821103861..09717afda 100644 --- a/testing/src/specs/admin-management.spec.ts +++ b/testing/src/specs/admin-management.spec.ts @@ -30,9 +30,10 @@ test.describe('admin management', () => { await expect(page.getByTestId('data-table-body')).toContainText('admin'); }); - test("should clear a user's email from the edit sheet", async ({ api, authenticateAs, page }) => { + test("should clear a user's email from the edit sheet", async ({ api, authenticateAs, page, uniqueId }) => { + const email = `contact-${uniqueId}@example.org`; const group = await api.createGroup(); - const { user } = await api.createUser({ email: 'contact@example.org', groupIds: [group.id] }); + const { user } = await api.createUser({ email, groupIds: [group.id] }); await authenticateAs('ADMIN'); await page.goto('/admin/users'); @@ -43,7 +44,7 @@ test.describe('admin management', () => { const emailInput = editSheet.getByLabel('Email'); await page.getByTestId('data-table-row').dblclick(); - await expect(emailInput).toHaveValue('contact@example.org'); + await expect(emailInput).toHaveValue(email); await emailInput.clear(); await editSheet.getByRole('button', { name: 'Submit' }).click(); await expect(editSheet).toBeHidden(); diff --git a/testing/src/specs/user-account.spec.ts b/testing/src/specs/user-account.spec.ts index 494ad09a0..69937dc86 100644 --- a/testing/src/specs/user-account.spec.ts +++ b/testing/src/specs/user-account.spec.ts @@ -41,10 +41,11 @@ test.describe('user account', () => { await expect(userAccountPage.passwordDialog).toBeHidden(); }); - test('should clear contact details that are saved blank', async ({ api, authenticateAs, page }) => { + test('should clear contact details that are saved blank', async ({ api, authenticateAs, page, uniqueId }) => { + const email = `contact-${uniqueId}@example.org`; const group = await api.createGroup(); const { credentials } = await api.createUser({ - email: 'contact@example.org', + email, groupIds: [group.id], phoneNumber: '5145551234' }); @@ -52,7 +53,7 @@ test.describe('user account', () => { const userAccountPage = new UserAccountPage(page); await userAccountPage.goto('/user'); - await expect(userAccountPage.emailInput).toHaveValue('contact@example.org'); + await expect(userAccountPage.emailInput).toHaveValue(email); await expect(userAccountPage.phoneNumberInput).toHaveValue('5145551234'); await userAccountPage.emailInput.clear(); From 5a7b88e753a2f7c8de9c5cc1d59bbdc06e09a58a Mon Sep 17 00:00:00 2001 From: Thomas Beaudry Date: Tue, 28 Jul 2026 14:05:29 -0400 Subject: [PATCH 4/5] fix(user): consolidate the phone rule in schemas and tidy the account page Addresses the five follow-up items from review on #1444: - Give the password dialog a `Dialog.Description`, so Radix stops warning and the dialog carries an `aria-describedby`. - Key "Account"/"Compte" as `user.account` now that it is used in two places. - Move the phone format and 7-digit minimum into `packages/schemas`, where both tiers read it. `apps/api` had its own regex with no digit rule, so a direct API call could store a number the account page then refused to save. The web helpers now map the returned error code to a translated message rather than restating the rule. - Redeclare `email`/`phoneNumber` on `UpdateUserDto`, which typed them as `string | undefined` while `$UpdateUserData` parses them to `string | null | undefined`. - Use the shared `$Email` on `admin/users/create.tsx`, so typing an address and then clearing it no longer fails with zod's untranslated message, and send blank contact details as absent rather than `''`. Co-Authored-By: Claude Opus 5 --- apps/api/src/users/dto/create-user.dto.ts | 15 +---- apps/api/src/users/dto/update-user.dto.ts | 11 +++- .../src/components/UserDropup/UserDropup.tsx | 7 +-- .../src/routes/_app/admin/users/create.tsx | 13 +++- apps/web/src/routes/_app/user.tsx | 8 ++- apps/web/src/translations/user.json | 4 ++ .../src/utils/__tests__/validation.test.ts | 23 ++++--- apps/web/src/utils/validation.ts | 38 ++++++------ packages/schemas/src/user/user.test.ts | 60 +++++++++++++++---- packages/schemas/src/user/user.ts | 43 ++++++++++++- testing/src/specs/admin-management.spec.ts | 30 ++++++++++ testing/src/specs/user-account.spec.ts | 8 +++ 12 files changed, 195 insertions(+), 65 deletions(-) diff --git a/apps/api/src/users/dto/create-user.dto.ts b/apps/api/src/users/dto/create-user.dto.ts index 6dbd17899..30b4b96fb 100644 --- a/apps/api/src/users/dto/create-user.dto.ts +++ b/apps/api/src/users/dto/create-user.dto.ts @@ -4,23 +4,10 @@ import type { Sex } from '@opendatacapture/schemas/subject'; import { $CreateUserData } from '@opendatacapture/schemas/user'; import type { BasePermissionLevel, CreateUserData } from '@opendatacapture/schemas/user'; -const regex = new RegExp(/^\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/); - // Note: password strength, username-match, and breached-password checks are enforced // centrally in `UsersService.validatePassword` so they apply to every flow that sets a // password (user creation, admin edits, self-service updates, and initial setup). -@ValidationSchema( - $CreateUserData.check((ctx) => { - if (ctx.value.phoneNumber && !regex.test(ctx.value.phoneNumber)) { - ctx.issues.push({ - code: 'custom', - input: ctx.value.phoneNumber, - message: `Invalid phone number`, - path: ['phoneNumber'] - }); - } - }) -) +@ValidationSchema($CreateUserData) export class CreateUserDto implements CreateUserData { @ApiProperty({ description: "Determines the user's base permissions, which may later be modified by an admin", diff --git a/apps/api/src/users/dto/update-user.dto.ts b/apps/api/src/users/dto/update-user.dto.ts index 483fc70d7..95b29921d 100644 --- a/apps/api/src/users/dto/update-user.dto.ts +++ b/apps/api/src/users/dto/update-user.dto.ts @@ -1,8 +1,15 @@ import { ValidationSchema } from '@douglasneuroinformatics/libnest'; -import { PartialType } from '@nestjs/swagger'; +import { ApiProperty, OmitType, PartialType } from '@nestjs/swagger'; import { $UpdateUserData } from '@opendatacapture/schemas/user'; import { CreateUserDto } from './create-user.dto'; +/** Contact details are redeclared because `$UpdateUserData` widens them to null, so an update can clear one. */ @ValidationSchema($UpdateUserData) -export class UpdateUserDto extends PartialType(CreateUserDto) {} +export class UpdateUserDto extends PartialType(OmitType(CreateUserDto, ['email', 'phoneNumber'] as const)) { + @ApiProperty({ description: 'Email, or null to clear the one on record' }) + email?: null | string; + + @ApiProperty({ description: 'Phone Number, or null to clear the one on record' }) + phoneNumber?: null | string; +} diff --git a/apps/web/src/components/UserDropup/UserDropup.tsx b/apps/web/src/components/UserDropup/UserDropup.tsx index 5f212213a..16d78e3de 100644 --- a/apps/web/src/components/UserDropup/UserDropup.tsx +++ b/apps/web/src/components/UserDropup/UserDropup.tsx @@ -17,7 +17,7 @@ export const UserDropup = () => { const navigate = useNavigate(); const [isOpen, setIsOpen] = useState(false); - const { t } = useTranslation('layout'); + const { t } = useTranslation(); return ( @@ -70,10 +70,7 @@ export const UserDropup = () => { }} > - {t({ - en: 'Account', - fr: 'Compte' - })} + {t('user.account')} { }) } ]} + data-testid="create-user-form" initialValues={{ disabled: false }} @@ -200,6 +201,7 @@ const RouteComponent = () => { basePermissionLevel: $BasePermissionLevel, groupIds: z.set(z.string()).optional(), confirmPassword: z.string().min(1), + email: $Email(t).optional(), phoneNumber: $PhoneNumber(t).optional() }) .check((ctx) => { @@ -232,7 +234,14 @@ const RouteComponent = () => { }); } })} - onSubmit={(data) => handleSubmit({ ...data, groupIds: Array.from(data.groupIds ?? []) })} + onSubmit={({ email, groupIds, phoneNumber, ...data }) => + handleSubmit({ + ...data, + email: omittedIfBlank(email), + groupIds: Array.from(groupIds ?? []), + phoneNumber: omittedIfBlank(phoneNumber) + }) + } /> ); diff --git a/apps/web/src/routes/_app/user.tsx b/apps/web/src/routes/_app/user.tsx index a6cbf45d1..26dd6713c 100644 --- a/apps/web/src/routes/_app/user.tsx +++ b/apps/web/src/routes/_app/user.tsx @@ -93,7 +93,7 @@ const RouteComponent = () => {
- {t({ en: 'Account', fr: 'Compte' })} + {t('user.account')} @@ -185,6 +185,12 @@ const RouteComponent = () => { {t('user.changePassword')} + + {t({ + en: 'Enter a new password for your account, then confirm it to save the change.', + fr: 'Saisissez un nouveau mot de passe pour votre compte, puis confirmez-le pour enregistrer la modification.' + })} + = (arg) => (typeof arg === 'string' ? arg : (arg.en ?? '')); @@ -14,17 +15,23 @@ const parseWith = (schema: z.ZodType) => (value: string) => { const parseEmail = parseWith($Email(t)); const parsePhoneNumber = parseWith($PhoneNumber(t)); -describe('countPhoneDigits', () => { - it('should count only numeric characters', () => { - expect(countPhoneDigits('+1 (514) 555-1234')).toBe(11); +describe('clearedIfBlank', () => { + it('should map a blank value to null, so an update clears the field', () => { + expect(clearedIfBlank('')).toBeNull(); }); - it('should return 0 for an empty string', () => { - expect(countPhoneDigits('')).toBe(0); + it('should leave a filled value untouched', () => { + expect(clearedIfBlank('jane.doe@example.org')).toBe('jane.doe@example.org'); + }); +}); + +describe('omittedIfBlank', () => { + it('should map a blank value to undefined, since a create has nothing to clear', () => { + expect(omittedIfBlank('')).toBeUndefined(); }); - it('should ignore dashes, spaces, and parentheses', () => { - expect(countPhoneDigits('(555) 123-4567')).toBe(10); + it('should leave a filled value untouched', () => { + expect(omittedIfBlank('jane.doe@example.org')).toBe('jane.doe@example.org'); }); }); diff --git a/apps/web/src/utils/validation.ts b/apps/web/src/utils/validation.ts index 08d399cf6..c0c6b746a 100644 --- a/apps/web/src/utils/validation.ts +++ b/apps/web/src/utils/validation.ts @@ -1,10 +1,8 @@ import type { TranslateFunction, TranslationKey } from '@douglasneuroinformatics/libui/i18n'; +import { findPhoneNumberError, MIN_PHONE_DIGITS } from '@opendatacapture/schemas/user'; +import type { PhoneNumberErrorCode } from '@opendatacapture/schemas/user'; import { z } from 'zod/v4'; -const PHONE_REGEX = new RegExp(/^\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/); - -const MIN_PHONE_DIGITS = 7; - /** * A field that may be left blank, and is otherwise rejected with the message `findError` returns. * This is one string schema rather than a union with `z.literal('')` because zod discards the @@ -19,15 +17,16 @@ function blankOr(findError: (value: string) => null | string) { }); } -function countPhoneDigits(phone: string): number { - return phone.replace(/\D/g, '').length; -} - -/** The submit-side counterpart of `blankOr`: a field left blank is cleared, which the API spells null. */ +/** The update-side counterpart of `blankOr`: a field left blank is cleared, which the API spells null. */ function clearedIfBlank(value: string | undefined) { return value === '' ? null : value; } +/** The create-side counterpart of `blankOr`: a create has nothing to clear, so a blank field is simply absent. */ +function omittedIfBlank(value: string | undefined) { + return value === '' ? undefined : value; +} + function $Email(t: TranslateFunction) { return blankOr((value) => z.email().safeParse(value).success ? null : t({ en: 'Invalid email address', fr: 'Adresse courriel invalide' }) @@ -35,18 +34,17 @@ function $Email(t: TranslateFunction) { } function $PhoneNumber(t: TranslateFunction) { + const messages: { [K in PhoneNumberErrorCode]: string } = { + INVALID_FORMAT: t({ en: 'Invalid phone number', fr: 'Numéro de téléphone invalide' }), + TOO_FEW_DIGITS: t({ + en: `Phone number must contain at least ${MIN_PHONE_DIGITS} digits`, + fr: `Le numéro de téléphone doit contenir au moins ${MIN_PHONE_DIGITS} chiffres` + }) + }; return blankOr((value) => { - if (!PHONE_REGEX.test(value)) { - return t({ en: 'Invalid phone number', fr: 'Numéro de téléphone invalide' }); - } - if (countPhoneDigits(value) < MIN_PHONE_DIGITS) { - return t({ - en: `Phone number must contain at least ${MIN_PHONE_DIGITS} digits`, - fr: `Le numéro de téléphone doit contenir au moins ${MIN_PHONE_DIGITS} chiffres` - }); - } - return null; + const code = findPhoneNumberError(value); + return code ? messages[code] : null; }); } -export { $Email, $PhoneNumber, clearedIfBlank, countPhoneDigits, MIN_PHONE_DIGITS }; +export { $Email, $PhoneNumber, clearedIfBlank, omittedIfBlank }; diff --git a/packages/schemas/src/user/user.test.ts b/packages/schemas/src/user/user.test.ts index dcdf9703c..41d411cbc 100644 --- a/packages/schemas/src/user/user.test.ts +++ b/packages/schemas/src/user/user.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import { $CreateUserData, $SelfUpdateUserData } from './user.js'; +import { $CreateUserData, $PhoneNumber, $SelfUpdateUserData, $User, MIN_PHONE_DIGITS } from './user.js'; describe('$SelfUpdateUserData', () => { it('should accept null contact details, so an update can clear them', () => { @@ -13,16 +13,54 @@ describe('$SelfUpdateUserData', () => { }); describe('$CreateUserData', () => { + const data = { + basePermissionLevel: 'STANDARD', + firstName: 'Jane', + groupIds: [], + lastName: 'Doe', + password: 'password', + username: 'jane.doe' + }; + it('should reject null contact details, which are only clearable on update', () => { - const data = { - basePermissionLevel: 'STANDARD', - email: null, - firstName: 'Jane', - groupIds: [], - lastName: 'Doe', - password: 'password', - username: 'jane.doe' - }; - expect($CreateUserData.safeParse(data).success).toBe(false); + expect($CreateUserData.safeParse({ ...data, email: null }).success).toBe(false); + }); + + it('should reject a phone number the account page would then refuse to save', () => { + expect($CreateUserData.safeParse({ ...data, phoneNumber: '123' }).success).toBe(false); + }); + + it('should accept a well-formed phone number', () => { + expect($CreateUserData.safeParse({ ...data, phoneNumber: '(514) 555-1234' }).success).toBe(true); + }); +}); + +describe('$PhoneNumber', () => { + it('should accept a number with spaces, dashes, and parentheses', () => { + expect($PhoneNumber.safeParse('(514) 555-1234').success).toBe(true); + }); + + it('should accept a number with exactly the minimum number of digits', () => { + expect($PhoneNumber.safeParse('12-34-567').success).toBe(true); + }); + + it('should reject a well-formed number with too few digits', () => { + const result = $PhoneNumber.safeParse('12-34-56'); + expect(result.error?.issues[0]?.message).toBe(`Phone number must contain at least ${MIN_PHONE_DIGITS} digits`); + }); + + it('should reject a malformed number with a format message', () => { + const result = $PhoneNumber.safeParse('abcdefgh'); + expect(result.error?.issues[0]?.message).toBe('Invalid phone number'); + }); + + it('should reject a blank value, since an absent number is spelled undefined or null', () => { + expect($PhoneNumber.safeParse('').success).toBe(false); + }); +}); + +describe('$User', () => { + it('should accept a stored number predating the digit minimum, so the read model still parses', () => { + expect($User.shape.phoneNumber.safeParse('123').success).toBe(true); }); }); diff --git a/packages/schemas/src/user/user.ts b/packages/schemas/src/user/user.ts index e8078beec..31455a47f 100644 --- a/packages/schemas/src/user/user.ts +++ b/packages/schemas/src/user/user.ts @@ -3,6 +3,40 @@ import { z } from 'zod/v4'; import { $BaseModel, $Permissions } from '../core/core.js'; import { $Sex } from '../subject/subject.js'; +const MIN_PHONE_DIGITS = 7; + +const PHONE_NUMBER_FORMAT = /^\+?\(?\d{1,4}\)?[\s.-]?\d{1,4}[\s.-]?\d{1,9}$/; + +const PHONE_NUMBER_ERROR_MESSAGES = { + INVALID_FORMAT: 'Invalid phone number', + TOO_FEW_DIGITS: `Phone number must contain at least ${MIN_PHONE_DIGITS} digits` +}; + +type PhoneNumberErrorCode = keyof typeof PHONE_NUMBER_ERROR_MESSAGES; + +function countPhoneDigits(value: string): number { + return value.replace(/\D/g, '').length; +} + +/** + * The reason a phone number is rejected, in the same spirit as {@link PASSWORD_ERROR_CODES}: the + * rule lives here so every tier enforces the same one, and `apps/web` maps the code to a translated + * message instead of restating the rule. + */ +function findPhoneNumberError(value: string): null | PhoneNumberErrorCode { + if (!PHONE_NUMBER_FORMAT.test(value)) { + return 'INVALID_FORMAT'; + } + return countPhoneDigits(value) < MIN_PHONE_DIGITS ? 'TOO_FEW_DIGITS' : null; +} + +export const $PhoneNumber = z.string().check((ctx) => { + const code = findPhoneNumberError(ctx.value); + if (code) { + ctx.issues.push({ code: 'custom', input: ctx.value, message: PHONE_NUMBER_ERROR_MESSAGES[code] }); + } +}); + export const $BasePermissionLevel = z.enum(['ADMIN', 'GROUP_MANAGER', 'STANDARD']); export type BasePermissionLevel = z.infer; @@ -30,6 +64,8 @@ export const $User = $BaseModel.extend({ firstName: z.string().min(1), groupIds: z.array(z.string()), lastName: z.string().min(1), + // Not `$PhoneNumber`: this is the read model, which must still parse numbers stored before the + // digit minimum existed. The rule is enforced on the write schemas below. phoneNumber: z.string().nullish(), sex: $Sex.nullish(), username: z.string().min(1) @@ -49,7 +85,7 @@ export const $CreateUserData = $User disabled: z.boolean().optional(), email: z.email().optional(), password: z.string().min(1), - phoneNumber: z.string().optional(), + phoneNumber: $PhoneNumber.optional(), sex: $Sex.optional() }); @@ -58,7 +94,7 @@ export type UpdateUserData = z.infer; export const $UpdateUserData = $CreateUserData.partial().extend({ additionalPermissions: $Permissions.optional(), email: z.email().nullish(), - phoneNumber: z.string().nullish() + phoneNumber: $PhoneNumber.nullish() }); export type $SelfUpdateUserData = z.infer; @@ -73,3 +109,6 @@ export const $SelfUpdateUserData = $UpdateUserData sex: true }) .partial(); + +export { findPhoneNumberError, MIN_PHONE_DIGITS }; +export type { PhoneNumberErrorCode }; diff --git a/testing/src/specs/admin-management.spec.ts b/testing/src/specs/admin-management.spec.ts index 09717afda..79c0d841c 100644 --- a/testing/src/specs/admin-management.spec.ts +++ b/testing/src/specs/admin-management.spec.ts @@ -1,3 +1,4 @@ +import { SEEDED_USER_PASSWORD } from '../support/constants'; import { expect, test } from '../support/fixtures'; test.describe('admin management', () => { @@ -30,6 +31,35 @@ test.describe('admin management', () => { await expect(page.getByTestId('data-table-body')).toContainText('admin'); }); + test('should create a user whose email was typed and then cleared', async ({ authenticateAs, page, uniqueId }) => { + const username = `user_${uniqueId}`; + + await authenticateAs('ADMIN'); + await page.goto('/admin/users/create'); + + const createUserForm = page.getByTestId('create-user-form'); + await createUserForm.getByLabel('Username').fill(username); + await createUserForm.getByLabel('Password', { exact: true }).fill(SEEDED_USER_PASSWORD); + await createUserForm.getByLabel('Confirm Password').fill(SEEDED_USER_PASSWORD); + await createUserForm.getByLabel('First Name').fill('Test'); + await createUserForm.getByLabel('Last Name').fill('User'); + + const emailInput = createUserForm.getByLabel('Email'); + await emailInput.fill(`contact-${uniqueId}@example.org`); + await emailInput.clear(); + + // ADMIN so the form does not also demand a group. + await createUserForm.getByTestId('basePermissionLevel-select-trigger').click(); + await page.getByTestId('basePermissionLevel-select-item-ADMIN').click(); + await createUserForm.getByRole('button', { name: 'Submit' }).click(); + + await expect(page).toHaveURL('/admin/users'); + }); + + test('should reject a phone number with too few digits over the API', async ({ api }) => { + await expect(api.createUser({ phoneNumber: '123' })).rejects.toThrow(/Phone number must contain at least 7 digits/); + }); + test("should clear a user's email from the edit sheet", async ({ api, authenticateAs, page, uniqueId }) => { const email = `contact-${uniqueId}@example.org`; const group = await api.createGroup(); diff --git a/testing/src/specs/user-account.spec.ts b/testing/src/specs/user-account.spec.ts index 69937dc86..2fa9bbf5e 100644 --- a/testing/src/specs/user-account.spec.ts +++ b/testing/src/specs/user-account.spec.ts @@ -16,6 +16,14 @@ test.describe('user account', () => { await expect(userAccountPage.pageHeader).toContainText('Account'); }); + test('should describe the password dialog, so it is announced with more than its title', async ({ getPageModel }) => { + const userAccountPage = await getPageModel('/user'); + + await userAccountPage.openPasswordDialog(); + + await expect(userAccountPage.passwordDialog).toHaveAccessibleDescription(/confirm it to save the change/); + }); + test('should keep the dialog open when the new password is too weak', async ({ getPageModel }) => { const userAccountPage = await getPageModel('/user'); From f14b624736b12177fe8e5537fd24d71dbe12f039 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Tue, 28 Jul 2026 19:31:30 -0400 Subject: [PATCH 5/5] fix(user): keep a legacy phone number from freezing the rest of the form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three items from review. `core.save` already carried the identical English and French, including in `admin/users/index.tsx`, so the `common.save` added here was a second key for one string waiting to drift. Dropped, and both call sites use `core.save`. `UpdateUserDto` derives its two redeclared fields from `UpdateUserData` rather than spelling out `null | string`, so the class cannot fall behind `$UpdateUserData` the way the phone rule fell behind itself. `$User.phoneNumber` still parses a number stored before the digit minimum, but `$UpdateUserData` validates with `$PhoneNumber` and both forms resubmitted the stored value on every save — so a user holding a short number could not change their own first name, and an admin could not edit that user at all. A PATCH should carry only what changed: `omittedIfUnchanged` drops an untouched value from the payload, and `$PhoneNumber` takes the stored value so client-side validation lets it through too. An edited number is still held to the rule, and the write schema is unchanged, so the API still rejects a short number. No e2e covers the legacy case: `$CreateUserData` now enforces the minimum, so such a record cannot be seeded through the API. The unit tests cover it. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/src/users/dto/update-user.dto.ts | 5 ++-- .../web/src/routes/_app/admin/users/index.tsx | 8 +++--- apps/web/src/routes/_app/user.tsx | 16 +++++++---- apps/web/src/translations/common.json | 4 --- .../src/utils/__tests__/validation.test.ts | 28 ++++++++++++++++++- apps/web/src/utils/validation.ts | 22 +++++++++++++-- 6 files changed, 64 insertions(+), 19 deletions(-) diff --git a/apps/api/src/users/dto/update-user.dto.ts b/apps/api/src/users/dto/update-user.dto.ts index 95b29921d..0619436bf 100644 --- a/apps/api/src/users/dto/update-user.dto.ts +++ b/apps/api/src/users/dto/update-user.dto.ts @@ -1,6 +1,7 @@ import { ValidationSchema } from '@douglasneuroinformatics/libnest'; import { ApiProperty, OmitType, PartialType } from '@nestjs/swagger'; import { $UpdateUserData } from '@opendatacapture/schemas/user'; +import type { UpdateUserData } from '@opendatacapture/schemas/user'; import { CreateUserDto } from './create-user.dto'; @@ -8,8 +9,8 @@ import { CreateUserDto } from './create-user.dto'; @ValidationSchema($UpdateUserData) export class UpdateUserDto extends PartialType(OmitType(CreateUserDto, ['email', 'phoneNumber'] as const)) { @ApiProperty({ description: 'Email, or null to clear the one on record' }) - email?: null | string; + email?: UpdateUserData['email']; @ApiProperty({ description: 'Phone Number, or null to clear the one on record' }) - phoneNumber?: null | string; + phoneNumber?: UpdateUserData['phoneNumber']; } diff --git a/apps/web/src/routes/_app/admin/users/index.tsx b/apps/web/src/routes/_app/admin/users/index.tsx index ebe82e07f..66faea57f 100644 --- a/apps/web/src/routes/_app/admin/users/index.tsx +++ b/apps/web/src/routes/_app/admin/users/index.tsx @@ -19,7 +19,7 @@ import { groupsQueryOptions, useGroupsQuery } from '@/hooks/useGroupsQuery'; import { useUpdateUserMutation } from '@/hooks/useUpdateUserMutation'; import { usersQueryOptions, useUsersQuery } from '@/hooks/useUsersQuery'; import { useAppStore } from '@/store'; -import { $Email, $PhoneNumber, clearedIfBlank } from '@/utils/validation'; +import { $Email, $PhoneNumber, clearedIfBlank, omittedIfUnchanged } from '@/utils/validation'; type UpdateUserFormData = { additionalPermissions?: Partial[]; @@ -58,7 +58,7 @@ const UpdateUserForm: React.FC<{ email: $Email(t).optional(), groupIds: z.set(z.string()), password: z.string().min(1).optional(), - phoneNumber: $PhoneNumber(t).optional() + phoneNumber: $PhoneNumber(t, initialValues?.phoneNumber).optional() }) .transform((arg) => { const firstPermission = arg.additionalPermissions?.[0]; @@ -115,7 +115,7 @@ const UpdateUserForm: React.FC<{ }); } }) satisfies z.ZodType; - }, [resolvedLanguage]); + }, [resolvedLanguage, initialValues?.phoneNumber]); return ( @@ -462,7 +462,7 @@ const RouteComponent = () => { ...data, email: clearedIfBlank(email), groupIds: Array.from(groupIds), - phoneNumber: clearedIfBlank(phoneNumber) + phoneNumber: omittedIfUnchanged(phoneNumber, selectedUser!.phoneNumber) }, id: selectedUser!.id }, diff --git a/apps/web/src/routes/_app/user.tsx b/apps/web/src/routes/_app/user.tsx index 26dd6713c..dd808ee7c 100644 --- a/apps/web/src/routes/_app/user.tsx +++ b/apps/web/src/routes/_app/user.tsx @@ -15,7 +15,7 @@ import { UserIcon } from '@/components/UserIcon'; import { useFindUserQuery } from '@/hooks/useFindUserQuery'; import { useSelfUpdateUserMutation } from '@/hooks/useSelfUpdateUserMutation'; import { useAppStore } from '@/store'; -import { $Email, $PhoneNumber, clearedIfBlank } from '@/utils/validation'; +import { $Email, $PhoneNumber, clearedIfBlank, omittedIfUnchanged } from '@/utils/validation'; type ProfileFormData = { dateOfBirth?: Date | undefined; @@ -55,9 +55,9 @@ const RouteComponent = () => { dateOfBirth: z.date().optional(), sex: $Sex.optional(), email: $Email(t).optional(), - phoneNumber: $PhoneNumber(t).optional() + phoneNumber: $PhoneNumber(t, userInfo.data.phoneNumber).optional() }) satisfies z.ZodType; - }, [resolvedLanguage]); + }, [resolvedLanguage, userInfo.data.phoneNumber]); const $PasswordFormData = useMemo(() => { return z @@ -172,11 +172,15 @@ const RouteComponent = () => { phoneNumber: userInfo.data.phoneNumber ?? '' }} key={userInfo.dataUpdatedAt} - submitBtnLabel={t('common.save')} + submitBtnLabel={t('core.save')} validationSchema={$ProfileFormData} onSubmit={({ email, phoneNumber, ...rest }) => { updateSelfUserMutation.mutate({ - data: { ...rest, email: clearedIfBlank(email), phoneNumber: clearedIfBlank(phoneNumber) }, + data: { + ...rest, + email: clearedIfBlank(email), + phoneNumber: omittedIfUnchanged(phoneNumber, userInfo.data.phoneNumber) + }, id: currentUser!.id }); }} @@ -209,7 +213,7 @@ const RouteComponent = () => { }} data-form-type="other" data-lpignore="true" - submitBtnLabel={t('common.save')} + submitBtnLabel={t('core.save')} validationSchema={$PasswordFormData} onSubmit={(data) => { updateSelfUserMutation.mutate( diff --git a/apps/web/src/translations/common.json b/apps/web/src/translations/common.json index c615fad89..e59972ec7 100644 --- a/apps/web/src/translations/common.json +++ b/apps/web/src/translations/common.json @@ -173,10 +173,6 @@ "en": "Research", "fr": "Recherche" }, - "save": { - "en": "Save", - "fr": "Enregistrer" - }, "session": { "en": "Session", "fr": "Session" diff --git a/apps/web/src/utils/__tests__/validation.test.ts b/apps/web/src/utils/__tests__/validation.test.ts index 2a266148a..650083726 100644 --- a/apps/web/src/utils/__tests__/validation.test.ts +++ b/apps/web/src/utils/__tests__/validation.test.ts @@ -3,7 +3,7 @@ import { MIN_PHONE_DIGITS } from '@opendatacapture/schemas/user'; import { describe, expect, it } from 'vitest'; import { z } from 'zod/v4'; -import { $Email, $PhoneNumber, clearedIfBlank, omittedIfBlank } from '../validation'; +import { $Email, $PhoneNumber, clearedIfBlank, omittedIfBlank, omittedIfUnchanged } from '../validation'; const t: TranslateFunction = (arg) => (typeof arg === 'string' ? arg : (arg.en ?? '')); @@ -35,6 +35,24 @@ describe('omittedIfBlank', () => { }); }); +describe('omittedIfUnchanged', () => { + it('should omit a value the user never touched, so the write schema never sees it', () => { + expect(omittedIfUnchanged('123', '123')).toBeUndefined(); + }); + + it('should treat a blank field over an absent value as unchanged', () => { + expect(omittedIfUnchanged('', null)).toBeUndefined(); + }); + + it('should clear a value the user blanked out', () => { + expect(omittedIfUnchanged('', '5145551234')).toBeNull(); + }); + + it('should send an edited value', () => { + expect(omittedIfUnchanged('5145551234', '123')).toBe('5145551234'); + }); +}); + describe('$PhoneNumber', () => { it('should accept a blank value', () => { expect(parsePhoneNumber('').success).toBe(true); @@ -63,6 +81,14 @@ describe('$PhoneNumber', () => { expect(issues).toHaveLength(1); expect(issues[0]!.message).toBe('Invalid phone number'); }); + + it('should accept the number already on the record, even one predating the digit minimum', () => { + expect(parseWith($PhoneNumber(t, '123'))('123').success).toBe(true); + }); + + it('should still reject a short number that is not the one on record', () => { + expect(parseWith($PhoneNumber(t, '123'))('456').success).toBe(false); + }); }); describe('$Email', () => { diff --git a/apps/web/src/utils/validation.ts b/apps/web/src/utils/validation.ts index c0c6b746a..257c9b827 100644 --- a/apps/web/src/utils/validation.ts +++ b/apps/web/src/utils/validation.ts @@ -27,13 +27,28 @@ function omittedIfBlank(value: string | undefined) { return value === '' ? undefined : value; } +/** + * A PATCH carries only what changed, so a value the user never touched is never re-validated by the + * write schema. That matters for a number stored before the digit minimum existed: `$User` still + * parses it so the form can show it, but `$UpdateUserData` would reject it on the way back out and + * block every other field on the form with it. + */ +function omittedIfUnchanged(value: string | undefined, storedValue: null | string | undefined) { + return (value ?? '') === (storedValue ?? '') ? undefined : clearedIfBlank(value); +} + function $Email(t: TranslateFunction) { return blankOr((value) => z.email().safeParse(value).success ? null : t({ en: 'Invalid email address', fr: 'Adresse courriel invalide' }) ); } -function $PhoneNumber(t: TranslateFunction) { +/** + * `storedValue` is the number already on the record, which is accepted as-is: one stored before the + * digit minimum existed would otherwise fail here and leave the rest of the form unsubmittable. + * Pair it with `omittedIfUnchanged` so the value the schema waved through is not then sent. + */ +function $PhoneNumber(t: TranslateFunction, storedValue?: null | string) { const messages: { [K in PhoneNumberErrorCode]: string } = { INVALID_FORMAT: t({ en: 'Invalid phone number', fr: 'Numéro de téléphone invalide' }), TOO_FEW_DIGITS: t({ @@ -42,9 +57,12 @@ function $PhoneNumber(t: TranslateFunction) { }) }; return blankOr((value) => { + if (value === storedValue) { + return null; + } const code = findPhoneNumberError(value); return code ? messages[code] : null; }); } -export { $Email, $PhoneNumber, clearedIfBlank, omittedIfBlank }; +export { $Email, $PhoneNumber, clearedIfBlank, omittedIfBlank, omittedIfUnchanged };