(feat) O3-3367 Add support for person attributes #423

CynthiaKamau · 2024-11-19T10:41:17Z

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

Add support for person attributes

Schema :

{
  "name": "New person attributes  form ",
  "pages": [
    {
      "label": "newPage ",
      "sections": [
        {
          "label": "New section",
          "isExpanded": "true",
          "questions": [
            {
              "label": "Person attribute (String)",
              "type": "personAttribute",
              "required": false,
              "id": "bloodTyping",
              "questionOptions": {
                "rendering": "text",
                "attributeType": "8d871d18-c2cc-11de-8d13-0010c6dffd0f"
              }
            },
            {
              "label": "Person attribute (location drop down)",
              "type": "personAttribute",
              "required": false,
              "id": "bloodTyping1",
              "questionOptions": {
                "rendering": "ui-select-extended",
                "attributeType": "8d87236c-c2cc-11de-8d13-0010c6dffd0f"
              },
              "validators": []
            },
            {
              "label": "Person attribute (concept select drop down)",
              "type": "personAttribute",
              "required": false,
              "id": "bloodTyping2",
              "questionOptions": {
                "rendering": "ui-select-extended",
                "concept": "5272AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
                "attributeType": "8d871f2a-c2cc-11de-8d13-0010c6dffd0f"
              },
              "validators": []
            }
          ]
        }
      ]
    }
  ],
  "processor": "EncounterFormProcessor",
  "encounterType": "dd528487-82a5-4082-9c72-ed246bd49591",
  "referencedForms": [],
  "description": "Form that keeps HIV patient data",
  "version": "2.0",
  "uuid": "9478f8b6-0875-4127-b557-f8d844d1bd69"
}

Screenshots

Screen.Recording.2024-11-21.at.13.54.06.mov

Related Issue

Other

github-actions · 2024-11-19T10:44:12Z

Size Change: -264 kB (-17.31%) 👏

Total Size: 1.26 MB

Filename	Size	Change
`dist/219.js`	0 B	-264 kB (removed)	🏆

ℹ️ View Unchanged

Filename	Size	Change
`dist/151.js`	382 kB	0 B
`dist/225.js`	2.57 kB	0 B
`dist/277.js`	1.85 kB	0 B
`dist/289.js`	88.9 kB	0 B
`dist/300.js`	645 B	0 B
`dist/317.js`	266 kB	0 B
`dist/335.js`	968 B	0 B
`dist/353.js`	3.02 kB	0 B
`dist/41.js`	3.37 kB	0 B
`dist/540.js`	2.63 kB	0 B
`dist/55.js`	758 B	0 B
`dist/585.js`	112 kB	0 B
`dist/635.js`	14.4 kB	0 B
`dist/690.js`	11.5 kB	0 B
`dist/70.js`	483 B	0 B
`dist/979.js`	6.87 kB	0 B
`dist/99.js`	691 B	0 B
`dist/993.js`	3.09 kB	0 B
`dist/main.js`	355 kB	+174 B (+0.05%)
`dist/openmrs-esm-form-engine-lib.js`	3.8 kB	+1 B (+0.03%)

_{compressed-size-action}

ibacher · 2024-11-22T13:04:13Z

So the immediate thing I see here is that we're embedding "format" in the form. I don't really love when we duplicate metadata like this in forms. Just load the format from the backend definition of the attribute type, so we don't face weird issues where changing an attribute type causes forms to break.

pirupius · 2024-11-29T04:03:52Z

src/types/index.ts

@@ -102,7 +102,7 @@ export interface FormSchemaTransformer {
  /**
   * Transforms the raw schema to be compatible with the React Form Engine.
   */
-  transform: (form: FormSchema) => FormSchema;
+  transform: (form: FormSchema) => Promise<FormSchema> | FormSchema;


Do we need the Promise here? i believe the form is resolved before we get here

cc: @samuelmale

I think she explained here: https://github.com/openmrs/openmrs-esm-form-engine-lib/pull/423/files#r1863144624

pirupius · 2024-11-29T04:11:54Z

src/types/schema.ts

+  attribute?: {
+    type?: string;
+  };


Looking at the ampath schema docs for personAtttribute, we could eliminate the need for this being an object for backwards compatibility as well have a single format unless you have reasons for going this route

Let me work on this

pirupius · 2024-11-29T04:17:38Z

src/adapters/person-attributes-adapter.ts

+    }
+    field.meta.submission.newValue = {
+      value: value,
+      attributeType: field.questionOptions?.attribute?.type,


The suggested changes in the schema will trickle down

pirupius · 2024-11-29T04:25:51Z

src/hooks/useFormJson.tsx

  formJson: any,
  schemaTransformers: FormSchemaTransformer[] = [],
  formSessionIntent?: string,
-): FormSchema {
+): Promise<FormSchema> {


I have reservations to the Promise additions to this file because this affects the way the forms load. Maybe you can share why we need this

This adjustment is needed because the format of a person attribute uuid can be a text, location or concept and those details are fetched from the getPersonAttributeTypeFormat endpoint. Based on the response, we transform the question to the right datasource. @ibacher recommended that we get the backend definitions incase they change, we will still always get the right one. You can add suggestions on how to better handle it incase there is a better way to do it.

samuelmale

Nice work @CynthiaKamau! Any chance we can have some integrational test cases covering all supported rendering types?

samuelmale · 2024-12-02T10:01:47Z

src/adapters/person-attributes-adapter.ts

+export const PersonAttributesAdapter: FormFieldValueAdapter = {
+  transformFieldValue: function (field: FormField, value: any, context: FormContextProps) {
+    clearSubmission(field);
+    if (field.meta?.previousValue?.value === value || isEmpty(value)) {


What happens if the user tries to delete an attribute in edit mode?

if (field.meta?.previousValue && isEmpty(value)) { // should we void the attribute? }

samuelmale · 2024-12-02T10:03:41Z

src/adapters/person-attributes-adapter.ts

+import { clearSubmission } from '../utils/common-utils';
+import { isEmpty } from '../validators/form-validator';
+
+export const PersonAttributesAdapter: FormFieldValueAdapter = {


Are you planning on adding some test coverage for this adapter?

samuelmale · 2024-12-02T10:07:13Z

src/hooks/useFormJson.tsx

+  for (let transformer of schemaTransformers) {
+    const draftForm = await transformer.transform(formJson);
+    formJson = draftForm;
+  }


@ibacher Do you see any benefits from executing these sequentially?

Well, merging things would be hard, but this code actually has very different semantics from the previous version. Might be better to preserve the same semantics:

Suggested change

for (let transformer of schemaTransformers) {

const draftForm = await transformer.transform(formJson);

formJson = draftForm;

}

schemaTransformers.reduce((form, transformer) => Promise.resolve(transformer.transform(await form)), Promise.resolve(formJson));

samuelmale · 2024-12-02T10:09:14Z

src/hooks/usePersonAttributes.tsx

+  const [error, setError] = useState(null);
+
+  useEffect(() => {
+    if (patientUuid) {


Can you conditionally load attributes only for forms that support them? (Here is an example https://github.com/openmrs/openmrs-esm-form-engine-lib/blob/main/src/hooks/usePatientPrograms.ts#L12)

samuelmale · 2024-12-02T10:10:23Z

src/processors/encounter/encounter-form-processor.ts

@@ -162,6 +168,27 @@ export class EncounterFormProcessor extends FormProcessor {
      });
    }

+    // save person attributes
+    try {
+      const personattributes = preparePersonAttributes(context.formFields, context.location?.uuid);


Suggested change

const personattributes = preparePersonAttributes(context.formFields, context.location?.uuid);

const personAttributes = preparePersonAttributes(context.formFields, context.location?.uuid);

samuelmale · 2024-12-02T10:10:49Z

src/processors/encounter/encounter-form-processor.ts

+    // save person attributes
+    try {
+      const personattributes = preparePersonAttributes(context.formFields, context.location?.uuid);
+      const savedPrograms = await savePersonAttributes(context.patient, personattributes);


Suggested change

const savedPrograms = await savePersonAttributes(context.patient, personattributes);

const savedAttributes = await savePersonAttributes(context.patient, personAttributes);

samuelmale · 2024-12-02T10:16:26Z

src/types/index.ts

@@ -102,7 +102,7 @@ export interface FormSchemaTransformer {
  /**
   * Transforms the raw schema to be compatible with the React Form Engine.
   */
-  transform: (form: FormSchema) => FormSchema;
+  transform: (form: FormSchema) => Promise<FormSchema> | FormSchema;


I think she explained here: https://github.com/openmrs/openmrs-esm-form-engine-lib/pull/423/files#r1863144624

samuelmale · 2024-12-02T10:18:38Z

src/datasources/person-attribute-datasource.ts

+import { openmrsFetch, restBaseUrl } from '@openmrs/esm-framework';
+import { BaseOpenMRSDataSource } from './data-source';
+
+export class PersonAttributeLocationDataSource extends BaseOpenMRSDataSource {


Suggested change

export class PersonAttributeLocationDataSource extends BaseOpenMRSDataSource {

export class LocationAttributeDataSource extends BaseOpenMRSDataSource {

samuelmale · 2024-12-02T10:29:06Z

src/datasources/person-attribute-datasource.ts

+  async fetchData(searchTerm: string, config?: Record<string, any>, uuid?: string): Promise<any[]> {
+    const rep = 'v=custom:(uuid,display)';
+    const url = `${restBaseUrl}/location?${rep}`;
+    const { data } = await openmrsFetch(searchTerm ? `${url}&q=${searchTerm}` : url);
+
+    return data?.results;
+  }


This data source seems to just load locations (not tied down to the "attribute" model). Anything stopping you from reusing the existing location DS`?

samuelmale · 2024-12-02T10:30:15Z

src/transformers/default-schema-transformer.ts

@@ -267,3 +283,25 @@ function handleQuestionsWithObsComments(sectionQuestions: Array<FormField>): Arr

  return augmentedQuestions;
 }
+
+async function handlePersonAttributeType(question: FormField) {


Are you planning on adding some test coverage?

ibacher · 2024-12-02T18:35:24Z

src/components/inputs/ui-select-extended/ui-select-extended.component.tsx

@@ -149,7 +149,7 @@ const UiSelectExtended: React.FC<FormFieldInputProps> = ({ field, errors, warnin
            selectedItem={selectedItem}
            placeholder={isSearchable ? t('search', 'Search') + '...' : null}
            shouldFilterItem={({ item, inputValue }) => {
-              if (!inputValue) {
+              if (!inputValue || items.find((item) => item.uuid == field.value)) {


Is this required for this PR?

ibacher · 2024-12-02T18:36:45Z

src/datasources/person-attribute-datasource.ts

+  async fetchData(searchTerm: string, config?: Record<string, any>, uuid?: string): Promise<any[]> {
+    const rep = 'v=custom:(uuid,display)';
+    const url = `${restBaseUrl}/location?${rep}`;
+    const { data } = await openmrsFetch(searchTerm ? `${url}&q=${searchTerm}` : url);
+
+    return data?.results;
+  }


ibacher · 2024-12-02T18:36:58Z

src/datasources/select-concept-answers-datasource.ts

@@ -7,7 +7,7 @@ export class SelectConceptAnswersDatasource extends BaseOpenMRSDataSource {
  }

  fetchData(searchTerm: string, config?: Record<string, any>): Promise<any[]> {
-    const apiUrl = this.url.replace('conceptUuid', config.referencedValue || config.concept);
+    const apiUrl = this.url.replace('conceptUuid', config.concept || config.referencedValue);


Why the inversion here?

ibacher · 2024-12-02T18:43:10Z

src/hooks/useFormJson.tsx

+  for (let transformer of schemaTransformers) {
+    const draftForm = await transformer.transform(formJson);
+    formJson = draftForm;
+  }


Well, merging things would be hard, but this code actually has very different semantics from the previous version. Might be better to preserve the same semantics:

Suggested change

for (let transformer of schemaTransformers) {

const draftForm = await transformer.transform(formJson);

formJson = draftForm;

}

schemaTransformers.reduce((form, transformer) => Promise.resolve(transformer.transform(await form)), Promise.resolve(formJson));

ibacher · 2024-12-02T18:45:12Z

src/hooks/usePersonAttributes.tsx

+    if (patientUuid) {
+      openmrsFetch(`${restBaseUrl}/patient/${patientUuid}?v=custom:(attributes)`)
+        .then((response) => {
+          setPersonAttributes(response?.data?.attributes);


Suggested change

setPersonAttributes(response?.data?.attributes);

setPersonAttributes(response.data?.attributes);

Is there some reason we're not using SWR here?

ibacher · 2024-12-02T18:49:25Z

src/registry/inbuilt-components/inbuiltControls.ts

@@ -94,6 +94,6 @@ export const inbuiltControls: Array<RegistryItem<React.ComponentType<FormFieldIn
  },
  ...controlTemplates.map((template) => ({
    name: template.name,
-    component: templateToComponentMap.find((component) => component.name === template.name).baseControlComponent,
+    component: templateToComponentMap.find((component) => component.name === template.name)?.baseControlComponent,


Don't really understand why we need the ? here?

ibacher · 2024-12-02T18:49:45Z

src/registry/registry.ts

@@ -171,7 +171,7 @@ export async function getRegisteredFieldValueAdapter(type: string): Promise<Form
 }

 export async function getRegisteredFormSchemaTransformers(): Promise<FormSchemaTransformer[]> {
-  const transformers = [];
+  const transformers: FormSchemaTransformer[] = [];


Suggested change

const transformers: FormSchemaTransformer[] = [];

const transformers: Array<FormSchemaTransformer> = [];

ibacher · 2024-12-02T18:58:32Z

src/registry/registry.ts

-
-  transformers.push(...inbuiltFormTransformers.map((inbuiltTransformer) => inbuiltTransformer.component));
-
+  const inbuiltTransformersPromises = inbuiltFormTransformers.map(async (inbuiltTransformer) => {
+    const transformer = inbuiltTransformer.component;
+    if (transformer instanceof Promise) {
+      return await transformer;
+    }
+    return transformer;
+  });
+  const resolvedInbuiltTransformers = await Promise.all(inbuiltTransformersPromises);
+  transformers.push(...resolvedInbuiltTransformers);
  transformers.forEach((transformer) => {
    const inbuiltTransformer = inbuiltFormTransformers.find((t) => t.component === transformer);
-    registryCache.formSchemaTransformers[inbuiltTransformer.name] = transformer;
+    if (inbuiltTransformer) {
+      registryCache.formSchemaTransformers[inbuiltTransformer.name] = transformer;
+    }


Could we refactor this? The code is way more complicated than it needs to be and has way more iterations than it needs to. For example:

transformers = transformers.concat(inbuildFormTransformers.map((transformer) => transformer.component ? ({...transformer, component: Promise.resolve(transformer.component)}) : null).filter(Boolean).map(async (transformer) => { const theTransformer = await transformer.component; registryCache.formSchemaTransformers[transformer.name] = theTransformer; return theTransformer; }));

ibacher · 2024-12-02T19:05:01Z

src/transformers/default-schema-transformer.ts

@@ -125,7 +138,7 @@ function setFieldValidators(question: FormField) {
  }
 }

-function transformByType(question: FormField) {
+async function transformByType(question: FormField) {


I actually think there's a good argument to be made to separate this out from the DefaultFormSchemaTransformer. This is inherently async whereas nothing in the default transformer otherwise is. Also, for handling attributes, it would be substantially better to batch them up into a single request... or at least to avoid re-requesting an attribute type already loaded, but that logic requires a different structure than the default class provides.

ibacher · 2024-12-02T19:05:53Z

src/transformers/default-schema-transformer.ts

+  if (question.questionOptions.rendering !== 'text') {
+    question.questionOptions.rendering === 'ui-select-extended';
+  }


I don't think this does what you wanted it to do.

CynthiaKamau marked this pull request as draft November 19, 2024 10:41

CynthiaKamau force-pushed the O3-3367 branch 2 times, most recently from be9d967 to 48be05c Compare November 21, 2024 11:22

CynthiaKamau marked this pull request as ready for review November 21, 2024 11:25

CynthiaKamau force-pushed the O3-3367 branch 2 times, most recently from 7b25040 to ebf119f Compare November 21, 2024 11:42

CynthiaKamau marked this pull request as draft November 21, 2024 11:48

CynthiaKamau force-pushed the O3-3367 branch from ebf119f to f95290b Compare November 22, 2024 07:35

CynthiaKamau marked this pull request as ready for review November 22, 2024 07:43

CynthiaKamau requested review from samuelmale, usamaidrsk and ibacher November 22, 2024 07:43

CynthiaKamau force-pushed the O3-3367 branch 4 times, most recently from 8ac443b to e0f7bcc Compare November 27, 2024 09:15

CynthiaKamau requested a review from pirupius November 28, 2024 06:26

pirupius reviewed Nov 29, 2024

View reviewed changes

CynthiaKamau force-pushed the O3-3367 branch from e0f7bcc to e9aa02e Compare November 29, 2024 09:14

samuelmale requested changes Dec 2, 2024

View reviewed changes

ibacher reviewed Dec 2, 2024

View reviewed changes

(feat) O3-3367 Add support for person attributes

5510d0b

CynthiaKamau force-pushed the O3-3367 branch from 9b355d5 to e967865 Compare December 13, 2024 14:07

code review

cb6a495

CynthiaKamau force-pushed the O3-3367 branch from e967865 to cb6a495 Compare December 16, 2024 12:12

	const personattributes = preparePersonAttributes(context.formFields, context.location?.uuid);
	const personAttributes = preparePersonAttributes(context.formFields, context.location?.uuid);

	const savedPrograms = await savePersonAttributes(context.patient, personattributes);
	const savedAttributes = await savePersonAttributes(context.patient, personAttributes);

	export class PersonAttributeLocationDataSource extends BaseOpenMRSDataSource {
	export class LocationAttributeDataSource extends BaseOpenMRSDataSource {

	setPersonAttributes(response?.data?.attributes);
	setPersonAttributes(response.data?.attributes);

	const transformers: FormSchemaTransformer[] = [];
	const transformers: Array<FormSchemaTransformer> = [];

(feat) O3-3367 Add support for person attributes #423

Are you sure you want to change the base?

(feat) O3-3367 Add support for person attributes #423

Conversation

CynthiaKamau commented Nov 19, 2024 • edited Loading

Requirements

Summary

Screenshots

Related Issue

Other

github-actions bot commented Nov 19, 2024 • edited Loading

ibacher commented Nov 22, 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

CynthiaKamau Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

samuelmale 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

samuelmale Dec 2, 2024 • edited Loading

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

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

CynthiaKamau commented Nov 19, 2024 •

edited

Loading

github-actions bot commented Nov 19, 2024 •

edited

Loading

CynthiaKamau Nov 29, 2024 •

edited

Loading

samuelmale Dec 2, 2024 •

edited

Loading