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.

Sign up for GitHub

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

Jump to bottom

(feat) O3-3632 - Add config for maxDate and minDate to DateObsField #1261

Merged
merged 6 commits into from
Nov 13, 2024
Merged

(feat) O3-3632 - Add config for maxDate and minDate to DateObsField #1261

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e40f4c0
Add min and max date validations to obsDate field
kajambiya Jul 31, 2024
0cd381b
refactor code to address review comments
kajambiya Sep 27, 2024
32486f2
Add flags to disable future and past dates
kajambiya Oct 16, 2024
e1aa33b
change wording from disable to allow
kajambiya Oct 29, 2024
f95c9bb
Update config-schema
kajambiya Nov 13, 2024
9f1979a
Merge branch 'main' into O3-3632
brandones Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/esm-patient-registration-app/src/config-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ export interface FieldDefinition {
type: string;
label?: string;
uuid: string;
placeholder?: string;
dateFormat?: string;
maxDate?: string;
minDate?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

You modified the interface but did not change the actual config schema. Also why did you get rid of placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had removed it because the placeholder for the datepicker is now defined by the openMRSDatepicker component depending on the locale but I've realized that placeholder usage is not for datepicker alone so I'm going to add it back.

showHeading: boolean;
validation?: {
required: boolean;
Expand Down
49 changes: 41 additions & 8 deletions ...s/esm-patient-registration-app/src/patient-registration/field/obs/obs-field.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ import classNames from 'classnames';
import { Field } from 'formik';
import { useTranslation } from 'react-i18next';
import { InlineNotification, Layer, Select, SelectItem } from '@carbon/react';
import { OpenmrsDatePicker, useConfig } from '@openmrs/esm-framework';
import { OpenmrsDatePicker, translateFrom, useConfig } from '@openmrs/esm-framework';
import { type ConceptResponse } from '../../patient-registration.types';
import { type FieldDefinition, type RegistrationConfig } from '../../../config-schema';
import { Input } from '../../input/basic-input/input/input.component';
import { useConcept, useConceptAnswers } from '../field.resource';
import { PatientRegistrationContext } from '../../patient-registration-context';
import styles from './../field.scss';
import dayjs from 'dayjs';
import { moduleName } from '../../../constants';

export interface ObsFieldProps {
fieldDefinition: FieldDefinition;
Expand Down Expand Up @@ -57,8 +59,8 @@ export function ObsField({ fieldDefinition }: ObsFieldProps) {
concept={concept}
label={fieldDefinition.label}
required={fieldDefinition.validation.required}
dateFormat={fieldDefinition.dateFormat}
placeholder={fieldDefinition.placeholder}
minDate={fieldDefinition.minDate}
maxDate={fieldDefinition.maxDate}
/>
);
case 'Coded':
Expand Down Expand Up @@ -159,14 +161,43 @@ interface DateObsFieldProps {
concept: ConceptResponse;
label: string;
required?: boolean;
dateFormat?: string;
placeholder?: string;
minDate?: string;
maxDate?: string;
}

function DateObsField({ concept, label, required, placeholder }: DateObsFieldProps) {
const evaluateConfigDate = (dateString: string, dateType: string): Date | null => {
Copy link
Member

Choose a reason for hiding this comment

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

So basically, the real feature being supported by this PR is the ability to specify today as either the lowest selectable date or the highest selectable date?

Copy link
Member

Choose a reason for hiding this comment

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

Basically, the question here is "why don't we just build that instead of this?"

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, I would support a config interface like, for example,

datepicker: {
  allowPastDates: boolean
  allowFutureDates: boolean
}

does what's actually needed, is easier to maintain and code, and easier to understand and use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibacher & @brandones, my question here would be, don't we anticipate a scenario where someone would pass a fixed date e.g 2024-10-01 as either the min or max date. I don't have a use case per se but I just thought it would be possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I just can't really imagine a use case for that. It is good not to build things no one needs, or to design and build things with no use case. It is valuable to keep our code easy to understand, maintain, and use. If someday a use case comes up, we'll implement it then.

Copy link
Member

Choose a reason for hiding this comment

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

@ibacher / @brandones / @kajambiya please keep in mind that "today" from the perspective of a field on a form often needs to be "as of the encounter date" not "as of the entry date". So it is not sufficient to say we are only going to support past and future as of a "new Date()" object. So this might be an example of where a parent component (eg. a parent form with an encounterDate variable would want to set that as the minDate / maxDate / defaultDate on an obsField it contains). Sorry if this is not relevant, but wanted to mention it in case it is.

Copy link
Member

Choose a reason for hiding this comment

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

@mseaton Yeah, retrospective registration probably requires a redesign of the app. Here, though, we're just passing in a value from the configuration, so the issue is kind of orthogonal to that concern (i.e., we could implement this so that it interprets "today" relative to the user's current selected time for the registration to happen). The issue here is that there's isn't a lot of use for handling strings like '2020-01-05'.

if (!dateString) {
return null;
}
if (dateString === 'today') {
return dayjs(new Date()).toDate();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return dayjs(new Date()).toDate();
return new Date();

}

const parsedDate = dayjs(dateString);
if (!parsedDate.isValid()) {
throw new Error(
translateFrom(moduleName, 'invalidObsDateErrorMessage', `The ${dateType} date value provided is invalid!`),
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why are we using translateFrom here instead of t?
  2. We can't interpolate Min and Max in here like this. It's not translatable. We can do something like t('invalidObsDateMinValue', 'The minimum date value provided is invalid.')

Copy link
Member

Choose a reason for hiding this comment

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

It would, of course, be better to include some example of what is valid.

);
}

return parsedDate.toDate();
};

function DateObsField({ concept, label, required, maxDate, minDate }: DateObsFieldProps) {
const { t } = useTranslation();
const fieldName = `obs.${concept.uuid}`;
const { setFieldValue } = useContext(PatientRegistrationContext);
let evaluatedMinDate = null;
let evaluatedMaxDate = null;
let configDateError = '';

try {
evaluatedMinDate = evaluateConfigDate(minDate, 'Min');
evaluatedMaxDate = evaluateConfigDate(maxDate, 'Max');
} catch (error) {
configDateError = error.message;
console.error(configDateError);
}

const onDateChange = (date: Date) => {
setFieldValue(fieldName, date);
Expand All @@ -185,9 +216,11 @@ function DateObsField({ concept, label, required, placeholder }: DateObsFieldPro
isRequired={required}
onChange={onDateChange}
labelText={label ?? concept.display}
isInvalid={errors[fieldName] && touched[fieldName]}
invalidText={t(meta.error)}
isInvalid={(errors[fieldName] && touched[fieldName]) || configDateError}
invalidText={t(meta.error) || configDateError}
value={field.value}
minDate={evaluatedMinDate}
maxDate={evaluatedMaxDate}
/>
</>
);
Expand Down
Loading