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

kajambiya · 2024-07-31T04:49:26Z

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 allows users to pass max and min dates to the DateObsField via config

Screenshots

Screen.Recording.2024-10-29.at.19.25.09.mov

Related Issue

Other

ibacher

Thanks @kajambiya! A few things need to be cleaned up.

ibacher · 2024-07-31T15:59:25Z

...ages/esm-patient-registration-app/src/patient-registration/field/obs/obs-field.component.tsx

+    return new Date();
+  }
+
+  const date = new Date(dateString);


This would be a great place to use dayjs...

ibacher · 2024-07-31T15:59:46Z

...ages/esm-patient-registration-app/src/patient-registration/field/obs/obs-field.component.tsx

+  if (!isNaN(date.getTime())) {
+    return date;
+  }


Could you explain this a bit better?

This was to check whether the string was successfully converted into a date but since you advised a try...catch, this is no longer needed.

ibacher · 2024-07-31T16:02:41Z

...ages/esm-patient-registration-app/src/patient-registration/field/obs/obs-field.component.tsx

+    return date;
+  }
+
+  return 'Invalid min or max date!';


This is a user-facing string so it must be localizable

Make the message friendlier by passing in as an argument whether this is the min date or max date

Instead of checking the return type, throw an error and use try...catch. It may be better to actually create the error message at that point.

ibacher · 2024-07-31T16:03:49Z

packages/esm-patient-registration-app/src/config-schema.ts

@@ -12,7 +12,8 @@ export interface FieldDefinition {
  label?: string;
  uuid: string;
  placeholder?: string;


May as well remove placeholder here too? Also, these should be removed from the actual config schema and not just the types.

brandones · 2024-09-15T11:44:48Z

Ping @kajambiya . Is it clear to you how to move forward with this?

kajambiya · 2024-09-15T14:23:38Z

Ping @kajambiya . Is it clear to you how to move forward with this?

Yes it is @brandones . Was just busy with some other tasks and had packed this but going to wire a commit tomorrow.

kajambiya · 2024-09-27T11:17:06Z

Thanks @kajambiya! A few things need to be cleaned up.

Hi @ibacher
I've addressed the comments.
Just a quick one, I'm logging the thrown error but I'm also going ahead to pass it as a field error(See video). Is this necessary or the log should be enough?

ibacher · 2024-09-30T14:10:46Z

@kajambiya As a general rule, the most important thing is that if there is an error and it blocks the user from taking an action, that error must be shown to the user in a way they can understand and (hopefully) correct. So the general answer to your question is that logging to the console is a nice-to-have, but not a sufficient implementation of handling an error.

brandones · 2024-10-01T17:06:15Z

@ibacher Does that mean this is in good shape? It looks like the configDateError has been added to the display using the invalidText property of the datepicker.

ibacher · 2024-10-01T17:21:50Z

...ages/esm-patient-registration-app/src/patient-registration/field/obs/obs-field.component.tsx

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


Why are we using translateFrom here instead of t?

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.')

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

ibacher · 2024-10-01T17:22:32Z

...ages/esm-patient-registration-app/src/patient-registration/field/obs/obs-field.component.tsx

 }

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


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?

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

+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.

@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.

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.

@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.

@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'.

ibacher · 2024-10-01T17:25:17Z

...ages/esm-patient-registration-app/src/patient-registration/field/obs/obs-field.component.tsx

+    return null;
+  }
+  if (dateString === 'today') {
+    return dayjs(new Date()).toDate();


Suggested change

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

return new Date();

brandones · 2024-10-01T18:41:32Z

packages/esm-patient-registration-app/src/config-schema.ts

-  placeholder?: string;
-  dateFormat?: string;
+  maxDate?: string;
+  minDate?: string;


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

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.

brandones · 2024-10-14T13:30:39Z

@kajambiya is it clear how to move forward?

kajambiya · 2024-10-15T12:24:22Z

@kajambiya is it clear how to move forward?

@brandones Here's how I think we should proceed. Let me know if you have any thoughts:

Remove the minDate and maxDate.
Introduce two flags: allowFutureDates and allowPastDates.
By default, both flags will be set to true, allowing users to select any date. Implementers can then configure whether to allow or restrict future or past dates as needed.

brandones · 2024-10-15T16:25:26Z

Sounds good, thanks @kajambiya

kajambiya · 2024-10-16T12:13:47Z

Sounds good, thanks @kajambiya

@brandones @ibacher, I've updated the code and the video accordingly. Kindly review and revert.

brandones · 2024-10-17T13:08:08Z

Sorry, I thought we were going to do allowFutureDates and allowPastDates? The reason I think this is more intuitive is because the dates are not being "enabled" or "disabled," their selection is being "allowed" or "disallowed." But "disallow" is uncommon in interfaces, so setting "allowFutureDates" to false is, I think, the intuitive interface for this.

kajambiya · 2024-10-29T19:39:19Z

Sorry, I thought we were going to do allowFutureDates and allowPastDates? The reason I think this is more intuitive is because the dates are not being "enabled" or "disabled," their selection is being "allowed" or "disallowed." But "disallow" is uncommon in interfaces, so setting "allowFutureDates" to false is, I think, the intuitive interface for this.

Hi @brandones Sorry for the late response. I've updated the code and the video accordingly

brandones · 2024-11-03T23:59:28Z

packages/esm-patient-registration-app/src/config-schema.ts

+  allowFutureDates?: boolean;
+  allowPastDates?: boolean;


You have modified the type but you have not modified the actual config schema.

You must modify the actual config schema; there should be a console warning about that visible in the browser console logs. Please use the browser console.

brandones · 2024-11-11T16:15:26Z

Ping @kajambiya

Feedback addressed

kajambiya requested review from brandones, NethmiRodrigo and ibacher July 31, 2024 04:49

ibacher previously requested changes Jul 31, 2024

View reviewed changes

kajambiya added 2 commits September 27, 2024 12:36

Add min and max date validations to obsDate field

e40f4c0

refactor code to address review comments

0cd381b

kajambiya force-pushed the O3-3632 branch from 1b4db62 to 0cd381b Compare September 27, 2024 11:11

denniskigen requested a review from ibacher September 30, 2024 07:47

ibacher changed the title ~~(chore) O3-3632 Add config for maxDate and minDate to DateObsField~~ (feat) O3-3632 Add config for maxDate and minDate to DateObsField Oct 1, 2024

ibacher changed the title ~~(feat) O3-3632 Add config for maxDate and minDate to DateObsField~~ (feat) O3-3632 - Add config for maxDate and minDate to DateObsField Oct 1, 2024

ibacher reviewed Oct 1, 2024

View reviewed changes

brandones requested changes Oct 1, 2024

View reviewed changes

Add flags to disable future and past dates

32486f2

kajambiya force-pushed the O3-3632 branch from fa593f4 to 32486f2 Compare October 16, 2024 11:39

change wording from disable to allow

e1aa33b

brandones requested changes Nov 4, 2024

View reviewed changes

Update config-schema

f95c9bb

brandones approved these changes Nov 13, 2024

View reviewed changes

Merge branch 'main' into O3-3632

9f1979a

brandones merged commit 565a949 into openmrs:main Nov 13, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kajambiya commented Jul 31, 2024 •

edited

Loading

ibacher left a comment

ibacher Jul 31, 2024

ibacher Jul 31, 2024

kajambiya Aug 11, 2024

ibacher Jul 31, 2024

ibacher Jul 31, 2024

brandones commented Sep 15, 2024

kajambiya commented Sep 15, 2024

kajambiya commented Sep 27, 2024 •

edited

Loading

ibacher commented Sep 30, 2024

brandones commented Oct 1, 2024

ibacher Oct 1, 2024

ibacher Oct 1, 2024

ibacher Oct 1, 2024

ibacher Oct 1, 2024

brandones Oct 1, 2024

kajambiya Oct 7, 2024

brandones Oct 10, 2024

mseaton Oct 10, 2024

ibacher Oct 10, 2024

ibacher Oct 1, 2024

brandones Oct 1, 2024

kajambiya Oct 7, 2024

brandones commented Oct 14, 2024

kajambiya commented Oct 15, 2024

brandones commented Oct 15, 2024

kajambiya commented Oct 16, 2024

brandones commented Oct 17, 2024

kajambiya commented Oct 29, 2024

brandones Nov 3, 2024

brandones Nov 4, 2024

kajambiya Nov 13, 2024

brandones commented Nov 11, 2024

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

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

Conversation

kajambiya commented Jul 31, 2024 • edited Loading

Requirements

Summary

Screenshots

Related Issue

Other

ibacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandones commented Sep 15, 2024

kajambiya commented Sep 15, 2024

kajambiya commented Sep 27, 2024 • edited Loading

ibacher commented Sep 30, 2024

brandones commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandones commented Oct 14, 2024

kajambiya commented Oct 15, 2024

brandones commented Oct 15, 2024

kajambiya commented Oct 16, 2024

brandones commented Oct 17, 2024

kajambiya commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandones commented Nov 11, 2024

kajambiya commented Jul 31, 2024 •

edited

Loading

kajambiya commented Sep 27, 2024 •

edited

Loading