Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(refactor) O3-4221: Replace validation with zod #1413

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"devDependencies": {
"@babel/core": "^7.11.6",
"@carbon/react": "^1.71.0",
"@openmrs/esm-framework": "next",
"@openmrs/esm-framework": "^6.0.1-pre.2553",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this back to next. Read here - https://o3-docs.openmrs.org/docs/frontend-modules/development#i-see-errors-about-missing-apis-when-i-run-the-app on instructions on how to update the core and framework, and importantly the step about checking out package.json when doing so.

"@openmrs/esm-patient-common-lib": "next",
"@playwright/test": "1.48.2",
"@swc/core": "^1.2.165",
Expand Down Expand Up @@ -65,7 +65,7 @@
"jest-cli": "^29.7.0",
"jest-environment-jsdom": "^29.7.0",
"lint-staged": "^15.2.1",
"openmrs": "next",
"openmrs": "^6.0.1-pre.2553",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

"prettier": "^3.1.1",
"react": "^18.1.0",
"react-dom": "^18.1.0",
Expand Down
5 changes: 4 additions & 1 deletion packages/esm-service-queues-app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@
},
"dependencies": {
"@carbon/react": "^1.71.0",
"lodash-es": "^4.17.15"
"@hookform/resolvers": "^3.9.1",
"lodash-es": "^4.17.15",
"react-hook-form": "^7.54.0",
"zod": "^3.24.1"
},
"peerDependencies": {
"@openmrs/esm-framework": "5.x",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ import { render, screen } from '@testing-library/react';
import { useLayoutType } from '@openmrs/esm-framework';
import QueueServiceForm from './queue-service-form.workspace';

jest.mock('react-i18next', () => ({
useTranslation: () => ({
t: (key: string, defaultValue: string) => defaultValue,
}),
}));

Comment on lines +7 to +12
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to mock this?

const defaultProps = {
closeWorkspace: jest.fn(),
promptBeforeClosing: jest.fn(),
Expand Down Expand Up @@ -32,9 +38,22 @@ jest.mock('../create-queue-entry/hooks/useQueueLocations', () => ({
}),
}));

jest.mock('@openmrs/esm-framework', () => {
return {
showSnackbar: jest.fn(),
restBaseUrl: '/ws/rest/v1',
useLayoutType: jest.fn(),

useExtensionSlot: jest.requireActual('@openmrs/esm-framework').useExtensionSlot,

importDynamic: jest.fn(),
};
});

Comment on lines +41 to +52
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this PR about partial mocks - openmrs/openmrs-esm-patient-chart#1933. But I'm confused as to why these require mocking for this test case, like the restBaseUrl?

Suggested change
jest.mock('@openmrs/esm-framework', () => {
return {
showSnackbar: jest.fn(),
restBaseUrl: '/ws/rest/v1',
useLayoutType: jest.fn(),
useExtensionSlot: jest.requireActual('@openmrs/esm-framework').useExtensionSlot,
importDynamic: jest.fn(),
};
});
jest.mock('@openmrs/esm-framework', () => ({
...jest.requireActual('@openmrs/esm-framework'),
showSnackbar: jest.fn(),
restBaseUrl: '/ws/rest/v1',
useLayoutType: jest.fn(),
importDynamic: jest.fn(),
}));

describe('QueueServiceForm', () => {
beforeEach(() => {
mockUseLayoutType.mockReturnValue('tablet');
jest.clearAllMocks();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to do this, since this is jest does this as its set in the config by default-

Suggested change
jest.clearAllMocks();

});

it('should display required error messages when form is submitted with missing fields', async () => {
Expand All @@ -60,6 +79,8 @@ describe('QueueServiceForm', () => {
await user.selectOptions(serviceSelect, '6f017eb0-b035-4acd-b284-da45f5067502');
await user.selectOptions(locationSelect, '34567eb0-b035-4acd-b284-da45f5067502');

const submitButton = screen.getByText('Save');
await user.click(submitButton);
expect(queueNameInput).toHaveValue('Test Queue');
expect(serviceSelect).toHaveValue('6f017eb0-b035-4acd-b284-da45f5067502');
expect(locationSelect).toHaveValue('34567eb0-b035-4acd-b284-da45f5067502');
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import React, { useCallback, useState } from 'react';
import { useTranslation } from 'react-i18next';
import { useForm, Controller } from 'react-hook-form';
import * as z from 'zod';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import * as z from 'zod';
import { z } from 'zod';

import { zodResolver } from '@hookform/resolvers/zod';

import {
Button,
ButtonSet,
Expand All @@ -18,85 +22,84 @@ import { saveQueue, useServiceConcepts } from './queue-service.resource';
import { useQueueLocations } from '../create-queue-entry/hooks/useQueueLocations';
import styles from './queue-service-form.scss';

const QueueServiceSchema = z.object({
queueName: z.string().min(1, { message: 'Queue name is required' }),
queueConcept: z.string().min(1, { message: 'Queue concept is required' }),
userLocation: z.string().min(1, { message: 'Queue location is required' }),
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion would require moving this inside the component definition itself so that it could use the t function.
@denniskigen looking at the other places where we've used zod in patient-management, we've done this in different ways, so I don't know if what I've suggested is the best way to go about this.

Suggested change
const QueueServiceSchema = z.object({
queueName: z.string().min(1, { message: 'Queue name is required' }),
queueConcept: z.string().min(1, { message: 'Queue concept is required' }),
userLocation: z.string().min(1, { message: 'Queue location is required' }),
});
const QueueServiceSchema = z.object({
queueName: z.string({
required_error: t('queueNameRequired', 'Queue name is required'),
}),
queueConcept: z.string({
required_error: t('queueConceptRequired', 'Queue concept is required'),
}),
userLocation: z.string({
required_error: t('queueLocationRequired', 'Queue location is required'),
}),
});

type QueueServiceFormData = z.infer<typeof QueueServiceSchema>;

const QueueServiceForm: React.FC<DefaultWorkspaceProps> = ({ closeWorkspace }) => {
const { t } = useTranslation();
const { queueConcepts } = useServiceConcepts();
const { queueLocations } = useQueueLocations();
const [queueName, setQueueName] = useState('');
const [queueConcept, setQueueConcept] = useState('');
const [isMissingName, setMissingName] = useState(false);
const [isMissingQueue, setMissingQueue] = useState(false);
const [isMissingLocation, setMissingLocation] = useState(false);
const [userLocation, setUserLocation] = useState('');

const createQueue = useCallback(
(event) => {
event.preventDefault();

if (!queueName) {
setMissingName(true);
return;
}
if (!queueConcept) {
setMissingQueue(true);
return;
}
if (!userLocation) {
setMissingLocation(true);
return;
}

setMissingName(false);
setMissingQueue(false);
setMissingLocation(false);
const {
control,
register,
handleSubmit,
formState: { errors },
} = useForm<QueueServiceFormData>({
resolver: zodResolver(QueueServiceSchema),
defaultValues: {
queueName: '',
queueConcept: '',
userLocation: '',
},
});

saveQueue(queueName, queueConcept, queueName, userLocation).then(
({ status }) => {
if (status === 201) {
showSnackbar({
title: t('addQueue', 'Add queue'),
kind: 'success',
subtitle: t('queueAddedSuccessfully', 'Queue added successfully'),
});
closeWorkspace();
mutate(`${restBaseUrl}/queue?${userLocation}`);
mutate(`${restBaseUrl}/queue?location=${userLocation}`);
}
},
(error) => {
const createQueue = (data: QueueServiceFormData) => {
saveQueue(data.queueName, data.queueConcept, data.queueName, data.userLocation).then(
({ status }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.then and showing the snackbar for success a .catch to catch the error afterwards should be sufficient, rather than checking for the status and doing an error inside the .then

if (status === 201) {
showSnackbar({
title: t('errorAddingQueue', 'Error adding queue'),
kind: 'error',
isLowContrast: false,
subtitle: error?.message,
title: t('addQueue', 'Add queue'),
kind: 'success',
subtitle: t('queueAddedSuccessfully', 'Queue added successfully'),
});
},
);
},
[queueName, queueConcept, userLocation, t, closeWorkspace],
);
closeWorkspace();
mutate(`${restBaseUrl}/queue?${data.userLocation}`);
mutate(`${restBaseUrl}/queue?location=${data.userLocation}`);
}
},
(error) => {
showSnackbar({
title: t('errorAddingQueue', 'Error adding queue'),
kind: 'error',
isLowContrast: false,
subtitle: error?.message,
});
},
);
};

return (
<Form onSubmit={createQueue} className={styles.form}>
<Form onSubmit={handleSubmit(createQueue)} className={styles.form}>
<Stack gap={4} className={styles.grid}>
<Column>
<Layer className={styles.input}>
<TextInput
id="queueName"
invalidText="Required"
labelText={t('queueName', 'Queue name')}
onChange={(event) => setQueueName(event.target.value)}
value={queueName}
<Controller
name="queueName"
control={control}
render={({ field }) => (
<TextInput
{...field}
id="queueName"
invalidText={errors.queueName?.message}
invalid={!!errors.queueName}
labelText={t('queueName', 'Queue name')}
/>
)}
/>
{isMissingName && (
{errors.queueName && (
<section>
{/* TODO: Use a zod schema instead of this */}
<InlineNotification
style={{ margin: '0', minWidth: '100%' }}
kind="error"
lowContrast
title={t('missingQueueName', 'Missing queue name')}
subtitle={t('addQueueName', 'Please add a queue name')}
subtitle={errors.queueName.message}
/>
</section>
)}
Expand All @@ -105,31 +108,37 @@ const QueueServiceForm: React.FC<DefaultWorkspaceProps> = ({ closeWorkspace }) =

<Column>
<Layer className={styles.input}>
<Select
labelText={t('selectServiceType', 'Select a service type')}
id="queueConcept"
invalidText="Required"
onChange={(event) => setQueueConcept(event.target.value)}
value={queueConcept}>
{!queueConcept && <SelectItem text={t('selectServiceType', 'Select a service type')} value="" />}
{queueConcepts.length === 0 && (
<SelectItem text={t('noServicesAvailable', 'No services available')} value="" />
<Controller
name="queueConcept"
control={control}
render={({ field }) => (
<Select
{...field}
labelText={t('selectServiceType', 'Select a service type')}
id="queueConcept"
invalidText={errors.queueConcept?.message}>
{!field.value && <SelectItem text={t('selectServiceType', 'Select a service type')} value="" />}
{queueConcepts.length === 0 && (
<SelectItem text={t('noServicesAvailable', 'No services available')} value="" />
)}
{queueConcepts?.length > 0 &&
queueConcepts.map((concept) => (
<SelectItem key={concept.uuid} text={concept.display} value={concept.uuid}>
{concept.display}
</SelectItem>
))}
</Select>
)}
{queueConcepts?.length > 0 &&
queueConcepts.map((concept) => (
<SelectItem key={concept.uuid} text={concept.display} value={concept.uuid}>
{concept.display}
</SelectItem>
))}
</Select>
{isMissingQueue && (
/>

{errors.queueConcept && (
<section>
<InlineNotification
style={{ margin: '0', minWidth: '100%' }}
kind="error"
lowContrast
title={t('missingService', 'Missing service')}
subtitle={t('selectServiceType', 'Select a service type')}
subtitle={errors.queueConcept.message}
/>
</section>
)}
Expand All @@ -138,31 +147,37 @@ const QueueServiceForm: React.FC<DefaultWorkspaceProps> = ({ closeWorkspace }) =

<Column>
<Layer className={styles.input}>
<Select
id="location"
invalidText="Required"
labelText={t('selectALocation', 'Select a location')}
onChange={(event) => setUserLocation(event.target.value)}
value={userLocation}>
{!userLocation && <SelectItem text={t('selectALocation', 'Select a location')} value="" />}
{queueLocations.length === 0 && (
<SelectItem text={t('noLocationsAvailable', 'No locations available')} value="" />
<Controller
name="userLocation"
control={control}
render={({ field }) => (
<Select
{...field}
id="location"
invalidText="Required"
labelText={t('selectALocation', 'Select a location')}>
{!field.value && <SelectItem text={t('selectALocation', 'Select a location')} value="" />}
{queueLocations.length === 0 && (
<SelectItem text={t('noLocationsAvailable', 'No locations available')} value="" />
)}
{queueLocations?.length > 0 &&
queueLocations.map((location) => (
<SelectItem key={location.id} text={location.name} value={location.id}>
{location.name}
</SelectItem>
))}
</Select>
)}
{queueLocations?.length > 0 &&
queueLocations.map((location) => (
<SelectItem key={location.id} text={location.name} value={location.id}>
{location.name}
</SelectItem>
))}
</Select>
{isMissingLocation && (
/>

{errors.userLocation && (
<section>
<InlineNotification
style={{ margin: '0', minWidth: '100%' }}
kind="error"
lowContrast
title={t('missingLocation', 'Missing location')}
subtitle={t('pleaseSelectLocation', 'Please select a location')}
subtitle={errors.userLocation.message}
/>
</section>
)}
Expand Down
2 changes: 0 additions & 2 deletions packages/esm-service-queues-app/translations/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"addPatientToQueue": "Add patient to queue",
"addProviderQueueRoom": "Add provider queue room",
"addQueue": "Add queue",
"addQueueName": "Please add a queue name",
"addQueueRoom": "Add queue room",
"addQueueRoomName": "Please add a queue room name",
"addQueueRoomService": "Please add a queue room service",
Expand Down Expand Up @@ -129,7 +128,6 @@
"patients": "Patients",
"patientsCurrentlyInQueue": "Patients currently in queue",
"phoneNumber": "Phone number",
"pleaseSelectLocation": "Please select a location",
"previousPage": "Previous page",
"previousVisit": "Previous visit",
"priority": "Priority",
Expand Down
Loading
Loading