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

(fix) O3-3992: Add safeguards for service queue entry creation to visit form #2014

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

makombe
Copy link
Contributor

@makombe makombe commented Sep 17, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR adds checks for queueLocation, service, and priority as a safeguard to ensure that all necessary information is available before attempting to add a patient to the service queue when starting or editing a visit using the Visit form. Even if the showServiceQueueFields config property is truthy, there might be cases where the user hasn't selected all required queue fields. This check ensures that we only proceed with queue entry creation when all necessary data has been provided.

Screenshots

Related Issue

https://openmrs.atlassian.net/browse/O3-3992

Other

@makombe makombe changed the title O3-3992:Error when patient is not added to Queue during visit start O3-3992:(fix)Error when patient is not added to Queue during visit start Sep 17, 2024
@ojwanganto
Copy link

LGTM. Thanks, @makombe

@denniskigen denniskigen changed the title O3-3992:(fix)Error when patient is not added to Queue during visit start (fix) O3-3992: Add safeguards for service queue entry creation to visit form Sep 18, 2024
@denniskigen
Copy link
Member

denniskigen commented Sep 18, 2024

Tangential, but we should make the logic for operations in the Visit form transactional so that if, say, saving a queue entry fails, all the previous operations, like creating or updating the visit would be rolled back.

@vasharma05
Copy link
Member

Hey @makombe, can you please add screenshots displaying the different scenarios?
Thanks!

@@ -464,7 +464,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
.subscribe({
next: (response) => {
if (response.status === 201) {
if (config.showServiceQueueFields) {
if (config.showServiceQueueFields && queueLocation && service && priority) {
Copy link
Member

@denniskigen denniskigen Sep 18, 2024

Choose a reason for hiding this comment

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

We should extend the validation schema so that when showServiceQueueFields is true, the user is required to fill the specified fields. This will trigger validation error messages if left empty and prevent form submission. Let me know if this approach works for you then we can create a follow-up ticket for implementation @makombe @ojwanganto

Copy link
Member

@denniskigen denniskigen Sep 18, 2024

Choose a reason for hiding this comment

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

That would entail extending the schema like so, for example:

  const visitFormSchema = useMemo(() => {
    const visitAttributes = (config.visitAttributeTypes ?? [])?.reduce(
      (acc, { uuid, required }) => ({
        ...acc,
        [uuid]: required
          ? z
              .string({
                required_error: t('fieldRequired', 'This field is required'),
              })
              .refine((value) => !!value, t('fieldRequired', 'This field is required'))
          : z.string().optional(),
      }),
      {},
    );

    const baseSchema = {
      visitStartDate: z.date().refine(
        (value) => {
          const today = dayjs();
          const startDate = dayjs(value);
          return displayVisitStopDateTimeFields ? true : startDate.isSameOrBefore(today, 'day');
        },
        t('invalidVisitStartDate', 'Start date needs to be on or before {{firstEncounterDatetime}}', {
          firstEncounterDatetime: formatDatetime(new Date()),
          interpolation: {
            escapeValue: false,
          },
        }),
      ),
      visitStartTime: z
        .string()
        .refine((value) => value.match(time12HourFormatRegex), t('invalidTimeFormat', 'Invalid time format')),
      visitStartTimeFormat: z.enum(['PM', 'AM']),
      visitStopDate: displayVisitStopDateTimeFields ? z.date() : z.date().optional(),
      visitStopTime: displayVisitStopDateTimeFields
        ? z
            .string()
            .refine((value) => value.match(time12HourFormatRegex), t('invalidTimeFormat', 'Invalid time format'))
        : z.string().optional(),
      visitStopTimeFormat: displayVisitStopDateTimeFields ? z.enum(['PM', 'AM']) : z.enum(['PM', 'AM']).optional(),
      programType: z.string().optional(),
      visitType: z.string().refine((value) => !!value, t('visitTypeRequired', 'Visit type is required')),
      visitLocation: z.object({
        display: z.string(),
        uuid: z.string(),
      }),
      visitAttributes: z.object(visitAttributes),
    };

    return z.object(config.showServiceQueueFields ? { ...baseSchema, ...serviceQueueFieldsSchema } : baseSchema);
  }, [
    config.visitAttributeTypes,
    config.showServiceQueueFields,
    t,
    displayVisitStopDateTimeFields,
    serviceQueueFieldsSchema,
  ]);

And then we could pass the schema down into the slot as a piece of state:

{config.showServiceQueueFields && (
              <section>
                <div className={styles.sectionTitle}></div>
                <div className={styles.sectionField}>
                  <ExtensionSlot
                    name="visit-form-queue-slot"
                    state={{ setFormFields: setVisitFormFields, schema: serviceQueueFieldsSchema }}
                  />
                </div>
              </section>
            )}

And then we'd need to modify the VisitFormQueueFields component to leverage
RHF and the zod schema prop.

Copy link
Member

Choose a reason for hiding this comment

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

All of this is not strictly necessary for now, though, which is why I proposed addressing it in a subsequent ticket.

Copy link
Contributor Author

@makombe makombe Sep 19, 2024

Choose a reason for hiding this comment

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

Thanks @denniskigen I agree we can do this in a follow up PR. @ojwanganto Whenever queue fields are configured to show on visit form, would it be wise to make it mandatory for them to be filled or there are exceptional cases ? What of retrospective data entry? Queue might not make sense when starting visits retrospectively cc @denniskigen

Copy link

@ojwanganto ojwanganto Sep 19, 2024

Choose a reason for hiding this comment

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

I may not be objective in this. Let's engage the BAs further for a correct and practical position. We can then have in a subsequent PR with the additional enhancements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants