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..0619436bf 100644 --- a/apps/api/src/users/dto/update-user.dto.ts +++ b/apps/api/src/users/dto/update-user.dto.ts @@ -1,8 +1,16 @@ import { ValidationSchema } from '@douglasneuroinformatics/libnest'; -import { PartialType } from '@nestjs/swagger'; +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'; +/** 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?: UpdateUserData['email']; + + @ApiProperty({ description: 'Phone Number, or null to clear the one on record' }) + phoneNumber?: UpdateUserData['phoneNumber']; +} diff --git a/apps/web/src/components/UserDropup/UserDropup.tsx b/apps/web/src/components/UserDropup/UserDropup.tsx index 089d83037..16d78e3de 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'; @@ -17,7 +17,7 @@ export const UserDropup = () => { const navigate = useNavigate(); const [isOpen, setIsOpen] = useState(false); - const { t } = useTranslation('layout'); + const { t } = useTranslation(); return ( @@ -64,16 +64,13 @@ export const UserDropup = () => { { void navigate({ to: '/user' }); }} > - - {t({ - en: 'Preferences', - fr: 'Préférences' - })} + + {t('user.account')} { }) } ]} + data-testid="create-user-form" initialValues={{ disabled: false }} @@ -199,7 +200,9 @@ const RouteComponent = () => { .extend({ basePermissionLevel: $BasePermissionLevel, groupIds: z.set(z.string()).optional(), - confirmPassword: z.string().min(1) + confirmPassword: z.string().min(1), + email: $Email(t).optional(), + phoneNumber: $PhoneNumber(t).optional() }) .check((ctx) => { if (!estimatePasswordStrength(ctx.value.password).success) { @@ -230,19 +233,15 @@ 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 ?? []) })} + 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/admin/users/index.tsx b/apps/web/src/routes/_app/admin/users/index.tsx index 6340f1bc7..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 { PHONE_REGEX } from '@/utils/validation'; +import { $Email, $PhoneNumber, clearedIfBlank, omittedIfUnchanged } 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, 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 ( @@ -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: omittedIfUnchanged(phoneNumber, selectedUser!.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 eb2832b45..dd808ee7c 100644 --- a/apps/web/src/routes/_app/user.tsx +++ b/apps/web/src/routes/_app/user.tsx @@ -1,11 +1,13 @@ /* eslint-disable perfectionist/sort-objects */ -import { useMemo } from 'react'; +import { useMemo, useState } from 'react'; import { estimatePasswordStrength } from '@douglasneuroinformatics/libpasswd'; -import { Form, Heading } from '@douglasneuroinformatics/libui/components'; +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'; import { PageHeader } from '@/components/PageHeader'; @@ -13,68 +15,58 @@ import { UserIcon } from '@/components/UserIcon'; import { useFindUserQuery } from '@/hooks/useFindUserQuery'; import { useSelfUpdateUserMutation } from '@/hooks/useSelfUpdateUserMutation'; import { useAppStore } from '@/store'; -import { PHONE_REGEX } from '@/utils/validation'; +import { $Email, $PhoneNumber, clearedIfBlank, omittedIfUnchanged } from '@/utils/validation'; -type UpdateUserFormData = { - confirmPassword?: string | undefined; +type ProfileFormData = { dateOfBirth?: Date | undefined; email?: string | undefined; firstName?: string | undefined; lastName?: string | undefined; - password?: string | undefined; phoneNumber?: string | undefined; sex?: undefined | z.infer; }; +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: { [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 userTypes = ['ADMIN', 'GROUP_MANAGER', 'STANDARD']; + const permissionLevel = userInfo.data.basePermissionLevel + ? 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: $Email(t).optional(), + phoneNumber: $PhoneNumber(t, userInfo.data.phoneNumber).optional() + }) satisfies z.ZodType; + }, [resolvedLanguage, userInfo.data.phoneNumber]); - 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 +86,40 @@ const RouteComponent = () => { path: ['confirmPassword'] }); } - }) satisfies z.ZodType; + }) satisfies z.ZodType; }, [resolvedLanguage]); return (
- {t({ - en: 'User Info', - fr: 'Informations utilisateur' - })} + {t('user.account')} -
- - {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 +130,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 +142,16 @@ const RouteComponent = () => { MALE: t('core.identificationData.sex.male') }, variant: 'select' + }, + email: { + kind: 'string', + label: t('common.email'), + variant: 'input' + }, + phoneNumber: { + kind: 'string', + label: t('common.phoneNumber'), + variant: 'input' } }, title: t({ @@ -189,31 +160,71 @@ const RouteComponent = () => { }) } ]} + data-form-type="other" + data-lpignore="true" + data-testid="profile-form" 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} - onSubmit={(data) => { - const { confirmPassword, password, ...restData } = data; - const filteredData = Object.fromEntries( - Object.entries(restData).filter(([, value]) => value != null && value !== '') - ); - - void updateSelfUserMutation.mutateAsync({ + submitBtnLabel={t('core.save')} + validationSchema={$ProfileFormData} + onSubmit={({ email, phoneNumber, ...rest }) => { + updateSelfUserMutation.mutate({ data: { - ...filteredData, - ...(password && password === confirmPassword ? { password } : {}) + ...rest, + email: clearedIfBlank(email), + phoneNumber: omittedIfUnchanged(phoneNumber, userInfo.data.phoneNumber) }, id: currentUser!.id }); }} /> + + + + {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.' + })} + + + + 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('core.save')} + validationSchema={$PasswordFormData} + onSubmit={(data) => { + updateSelfUserMutation.mutate( + { data: { password: data.password }, id: currentUser!.id }, + { onSuccess: () => setIsPasswordDialogOpen(false) } + ); + }} + /> + + +
); }; diff --git a/apps/web/src/translations/user.json b/apps/web/src/translations/user.json index 0967ef424..c33ce7281 100644 --- a/apps/web/src/translations/user.json +++ b/apps/web/src/translations/user.json @@ -1 +1,10 @@ -{} +{ + "account": { + "en": "Account", + "fr": "Compte" + }, + "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 new file mode 100644 index 000000000..650083726 --- /dev/null +++ b/apps/web/src/utils/__tests__/validation.test.ts @@ -0,0 +1,108 @@ +import type { TranslateFunction, TranslationKey } from '@douglasneuroinformatics/libui/i18n'; +import { MIN_PHONE_DIGITS } from '@opendatacapture/schemas/user'; +import { describe, expect, it } from 'vitest'; +import { z } from 'zod/v4'; + +import { $Email, $PhoneNumber, clearedIfBlank, omittedIfBlank, omittedIfUnchanged } 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('clearedIfBlank', () => { + it('should map a blank value to null, so an update clears the field', () => { + expect(clearedIfBlank('')).toBeNull(); + }); + + 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 leave a filled value untouched', () => { + expect(omittedIfBlank('jane.doe@example.org')).toBe('jane.doe@example.org'); + }); +}); + +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); + }); + + it('should accept a standard North American number', () => { + expect(parsePhoneNumber('+15145551234').success).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 exactly the minimum number of digits', () => { + expect(parsePhoneNumber('12-34-567').success).toBe(true); + }); + + 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 malformed number with a format message', () => { + const { issues } = parsePhoneNumber('abcdefgh'); + 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', () => { + 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 36fd54d9f..257c9b827 100644 --- a/apps/web/src/utils/validation.ts +++ b/apps/web/src/utils/validation.ts @@ -1 +1,68 @@ -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 { findPhoneNumberError, MIN_PHONE_DIGITS } from '@opendatacapture/schemas/user'; +import type { PhoneNumberErrorCode } from '@opendatacapture/schemas/user'; +import { z } from 'zod/v4'; + +/** + * 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 }); + } + }); +} + +/** 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; +} + +/** + * 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' }) + ); +} + +/** + * `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({ + 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 (value === storedValue) { + return null; + } + const code = findPhoneNumberError(value); + return code ? messages[code] : null; + }); +} + +export { $Email, $PhoneNumber, clearedIfBlank, omittedIfBlank, omittedIfUnchanged }; diff --git a/packages/schemas/src/user/user.test.ts b/packages/schemas/src/user/user.test.ts new file mode 100644 index 000000000..41d411cbc --- /dev/null +++ b/packages/schemas/src/user/user.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from 'vitest'; + +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', () => { + 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', () => { + 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', () => { + 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 19ccf5243..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,13 +85,16 @@ 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() }); +/** 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: $PhoneNumber.nullish() }); export type $SelfUpdateUserData = z.infer; @@ -70,3 +109,6 @@ export const $SelfUpdateUserData = $UpdateUserData sex: true }) .partial(); + +export { findPhoneNumberError, MIN_PHONE_DIGITS }; +export type { PhoneNumberErrorCode }; diff --git a/testing/AGENTS.md b/testing/AGENTS.md index 6d353efb3..e2bf07ecc 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 | -| `roleAccount(role)` | worker | Seeds a group + user per role once, then caches its token and username | +| 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 | +| `roleAccount(role)` | worker | Seeds a group + user per role once, then caches its token and username | 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/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..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', () => { @@ -29,4 +30,56 @@ test.describe('admin management', () => { // The admin created during setup is always present. 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(); + const { user } = await api.createUser({ email, 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(email); + 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..2fa9bbf5e --- /dev/null +++ b/testing/src/specs/user-account.spec.ts @@ -0,0 +1,76 @@ +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 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'); + + 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, uniqueId }) => { + const email = `contact-${uniqueId}@example.org`; + const group = await api.createGroup(); + const { credentials } = await api.createUser({ + email, + groupIds: [group.id], + phoneNumber: '5145551234' + }); + await authenticateAs(credentials); + + const userAccountPage = new UserAccountPage(page); + await userAccountPage.goto('/user'); + await expect(userAccountPage.emailInput).toHaveValue(email); + 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 c9a0fd032..f99e4a6d0 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'; @@ -10,6 +11,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'; @@ -26,7 +28,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; @@ -53,11 +56,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. */ @@ -87,9 +91,12 @@ export const test = base.extend({ { scope: 'worker' } ], appState: [{ isDisclaimerAccepted: true, isWalkthroughComplete: true }, { option: true }], - authenticateAs: async ({ appState, page, roleAccount }, use) => { - await use(async (role) => { - const { accessToken } = await roleAccount(role); + authenticateAs: async ({ apiRequestContext, appState, page, roleAccount }, use) => { + await use(async (roleOrCredentials) => { + const accessToken = + typeof roleOrCredentials === 'string' + ? (await roleAccount(roleOrCredentials)).accessToken + : await ApiClient.login(apiRequestContext, roleOrCredentials); await page.addInitScript( (injected) => { window.__PLAYWRIGHT_ACCESS_TOKEN__ = injected.accessToken;