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-1911: Reference Mappings instead of Concept UUIDs #86

Merged
merged 3 commits into from
Jun 2, 2023
Merged

(feat) O3-1911: Reference Mappings instead of Concept UUIDs #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab5d858
(feat) O3-1911: Reference Mappings instead of Concept UUIDs
mogoodrich May 26, 2023
2f26e49
(feat) O3-1911: Reference Mappings instead of Concept UUIDs
mogoodrich May 26, 2023
1a2470f
(feat) O3-1911: Reference Mappings instead of Concept UUIDs
mogoodrich May 30, 2023
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
140 changes: 140 additions & 0 deletions __mocks__/concepts.mock.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
{
"results": [
{
"uuid": "3cd6f600-26fe-102b-80cb-0017a47871b2",
"display": "Yes",
"conceptMappings": [
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "SNOMED CT"
},
"code": "373066001"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "PIH"
},
"code": "YES"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "CIEL"
},
"code": "1065"
}
}
]
},
{
"uuid": "3cd6f86c-26fe-102b-80cb-0017a47871b2",
"display": "No",
"conceptMappings": [
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "PIH"
},
"code": "NO"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "CIEL"
},
"code": "1066"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "SNOMED CT"
},
"code": "373067005"
}
}
]
},
{
"uuid": "3cccf632-26fe-102b-80cb-0017a47871b2",
"display": "Cough",
"conceptMappings": [
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "SNOMED CT"
},
"code": "263731006"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "AMPATH"
},
"code": "5956"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "SNOMED CT"
},
"code": "49727002"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "PIH"
},
"code": "COUGH"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "PIH"
},
"code": "107"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "CIEL"
},
"code": "143264"
}
}
]
},
{
"uuid": "f8134959-62d2-4f94-af6c-3580312b07a0",
"display": "Occurrence of trauma",
"conceptMappings": [
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "PIH"
},
"code": "8848"
}
},
{
"conceptReferenceTerm": {
"conceptSource": {
"name": "PIH"
},
"code": "Occurrence of trauma"
}
}
]
}
]
}
54 changes: 54 additions & 0 deletions __mocks__/forms/ohri-forms/reference-by-mapping-form.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
{
"encounterType": "92fd09b4-5335-4f7e-9f63-b2a663fd09a6",
"name": "Reference By Mapping Form",
"processor": "EncounterFormProcessor",
"referencedForms": [],
"uuid": "3e360508-2357-4098-a3d8-48d793a08fa0",
"version": "1.0",
"pages": [
{
"label": "First Page",
"sections": [
{
"label": "Another Section",
"isExpanded": "true",
"questions": [
{
"type": "obs",
"questionOptions": {
"rendering": "radio",
"concept": "PIH:Occurrence of trauma",
"answers": [
{
"concept": "PIH:Yes"
},
{
"concept": "PIH:No"
}
]
},
"id": "traumaQuestion"
},
{
"type": "obs",
"questionOptions": {
"rendering": "radio",
"concept": "PIH:COUGH",
"answers": [
{
"concept": "PIH:Yes"
},
{
"concept": "PIH:No"
}
]
},
"id": "coughQuestion"
}
]
}
]
}
],
"description": "comment"
}
1 change: 0 additions & 1 deletion src/api/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ export interface OHRIFormQuestionOptions {
maxLength?: string;
minLength?: string;
showDate?: string;
conceptMappings?: Array<Record<any, any>>;
samuelmale marked this conversation as resolved.
Show resolved Hide resolved
answers?: Array<Record<any, any>>;
weeksList?: string;
locationTag?: string;
Expand Down
43 changes: 40 additions & 3 deletions src/components/encounter/ohri-encounter-form.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { OHRIFormContext } from '../../ohri-form-context';
import {
cascadeVisibityToChildFields,
evaluateFieldReadonlyProp,
findConceptByReference,
findPagesWithErrors,
voidObsValueOnFieldHidden,
} from '../../utils/ohri-form-helper';
Expand All @@ -28,6 +29,7 @@ import { scrollIntoView } from '../../utils/ohri-sidebar';
import { useEncounter } from '../../hooks/useEncounter';
import { useInitialValues } from '../../hooks/useInitialValues';
import { useEncounterRole } from '../../hooks/useEncounterRole';
import { useConcepts } from '../../hooks/useConcepts';

interface OHRIEncounterFormProps {
formJson: OHRIFormSchema;
Expand Down Expand Up @@ -102,8 +104,10 @@ export const OHRIEncounterForm: React.FC<OHRIEncounterFormProps> = ({
);
const { encounterRole } = useEncounterRole();

const flattenedFields = useMemo(() => {
// given the form, flatten the fields and pull out all concept references
const [flattenedFields, conceptReferences] = useMemo(() => {
const flattenedFieldsTemp = [];
const conceptReferencesTemp = new Set<string>();
samuelmale marked this conversation as resolved.
Show resolved Hide resolved
form.pages?.forEach(page =>
page.sections?.forEach(section => {
section.questions?.forEach(question => {
Expand All @@ -128,7 +132,19 @@ export const OHRIEncounterForm: React.FC<OHRIEncounterFormProps> = ({
});
}),
);
return flattenedFieldsTemp;
flattenedFieldsTemp.forEach(field => {
if (field.questionOptions?.concept) {
conceptReferencesTemp.add(field.questionOptions.concept);
}
if (field.questionOptions?.answers) {
field.questionOptions.answers.forEach(answer => {
if (answer.concept) {
conceptReferencesTemp.add(answer.concept);
}
});
}
});
return [flattenedFieldsTemp, conceptReferencesTemp];
}, []);

const { initialValues: tempInitialValues, isFieldEncounterBindingComplete } = useInitialValues(
Expand All @@ -137,6 +153,9 @@ export const OHRIEncounterForm: React.FC<OHRIEncounterFormProps> = ({
encounterContext,
);

// look up concepts via their references
const { concepts } = useConcepts(conceptReferences);

const addScrollablePages = useCallback(() => {
formJson.pages.forEach(page => {
if (!page.isSubform) {
Expand Down Expand Up @@ -211,6 +230,24 @@ export const OHRIEncounterForm: React.FC<OHRIEncounterFormProps> = ({
},
);
}

// for each question and answer, see if we find a matching concept, and, if so:
// 1) replace the concept reference with uuid (for the case when the form references the concept by mapping)
// 2) use the concept display as the label if no label specified
const matchingConcept = findConceptByReference(field.questionOptions.concept, concepts);
field.questionOptions.concept = matchingConcept ? matchingConcept.uuid : field.questionOptions.concept;
field.label = field.label ? field.label : matchingConcept?.display;
if (field.questionOptions.answers) {
field.questionOptions.answers = field.questionOptions.answers.map(answer => {
const matchingAnswerConcept = findConceptByReference(answer.concept, concepts);
return {
...answer,
concept: matchingAnswerConcept ? matchingAnswerConcept.uuid : answer.concept,
label: answer.label ? answer.label : matchingAnswerConcept?.display,
};
});
}
Comment on lines +237 to +249
Copy link
Member

Choose a reason for hiding this comment

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

Can we link the matching concept to the field object for future references?

Background: We have this rarely used feature where we annotate the field with the concept name represented through a tooltip. Currently, we handle the concept fetch at the question level (see link). One important point to note is that this feature is only available in the "view" mode, this may not be the most ideal approach but it's something we intend to keep supporting.

Screenshot 2023-05-29 at 14 07 47

@mogoodrich Now that we are fetching concepts upfront, we don't need to do an extra fetch at the question level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks @samuelmale for pointing that fetch out, I hadn't noticed it. I can see if I we can replace it with the new fetch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry @samuelmale I just read this quickly the first time, but digging deeper I understand what you mean and I 100% agree that it makes sense to link the whole concept to the field. What do you think the best naming convention would be? We currently already have a "concept" on the field, but that is currently a string. We could swap out the concept string with a concept object when/if a match is found, but we'd need to go through and make sure that in all places where we reference "concept" we handle both possibilities... basically, any place where it expects "concept" to be a string we should use concept.uuid if concept is an object.

Copy link
Member

Choose a reason for hiding this comment

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

I'm terrible at naming things but I was thinking introducing a meta (for the lack of a better name) property at the question root level. This can be used to hold a map of arbitrary objects associated to a field.

I don't object using the existing field.questionOptions.concept property but I think we need a meta-like property to keep things in one place.

Copy link
Member Author

@mogoodrich mogoodrich May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So I liked the idea of meta a lot, but when I started to think about designing it out, I had doubts. Are you thinking adding something like this:

export interface OHRIFormQuestionOptions {
  **meta?: object;**
  extensionId?: string;
  extensionSlotName?: string;
  rendering: RenderType;
  concept?: string;
  /**
   * max and min are used to validate number field values
   */
  max?: string;
  min?: string;
  /**
   * maxLength and maxLength are used to validate text field length
   */
  maxLength?: string;
  minLength?: string;
  showDate?: string;
  answers?: Array<Record<any, any>>;
  weeksList?: string;
  locationTag?: string;
  rows?: number;
  toggleOptions?: { labelTrue: string; labelFalse: string };
  repeatOptions?: { addText?: string; limit?: string; limitExpression?: string };
  defaultValue?: any;
  calculate?: {
    calculateExpression: string;
  };
  isDateTime?: { labelTrue: boolean; labelFalse: boolean };
  usePreviousValueDisabled?: boolean;
}

When I started to map this out, the I realized that aren't a lot of the existing fields stuff we'd consider "metadata" so it might get confusing as to what lives in "meta" and what doesn't? Also, if we store all the concepts for the questions and answers within this "meta" section, we still need to figure out a way to map the answer concepts to the specific answers they go with?

Which led me to think... maybe worth starting a broader discussion about this? Is there a place where we are having Form Engine design discussions? Either Talk, or we could look into the RPC model they were using for other O3 design discussion?

Copy link
Member

Choose a reason for hiding this comment

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

Also, if we store all the concepts for the questions and answers within this "meta" section, we still need to figure out a way to map the answer concepts to the specific answers they go with?

I didn't think of a use case for linking the concept answers; if you asked me, I would say it's less likely to be desired. That being said, mapping ceases to be an issue.

I see in the proposed typings above you included meta to the question options, I was inclined to have it at the root level instead:

export interface OHRIFormField {
  label: string;
  type: string;
  questionOptions: OHRIFormQuestionOptions;
  id: string;
  // other props...
  meta: Record<string, any>;
}

PS: I'm honestly not so dogmatic about how we should approach this. I will let others share their thoughts; I just don't want to drag this.. cc: @ibacher, @denniskigen etc..

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Samuel... yes, let's see what Ian and Dennis think... that being said, if it becomes a dragged out discussion, I don't think blocks this PR? We could merge this in and handle adding "meta" as a future ticket?


return field;
}),
);
Expand All @@ -237,7 +274,7 @@ export const OHRIEncounterForm: React.FC<OHRIEncounterFormProps> = ({
setIsFieldInitializationComplete(true);
}
}
}, [tempInitialValues]);
}, [tempInitialValues, concepts]);

useEffect(() => {
if (sessionMode == 'enter' && !isTrue(formJson.formOptions?.usePreviousValueDisabled)) {
Expand Down
14 changes: 14 additions & 0 deletions src/hooks/useConcepts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import useSWRImmutable from 'swr/immutable';
import { openmrsFetch, OpenmrsResource } from '@openmrs/esm-framework';

const conceptRepresentation =
'custom:(uuid,display,conceptMappings:(conceptReferenceTerm:(conceptSource:(name),code)))';

export function useConcepts(references: Set<string>) {
// TODO: handle paging (ie when number of concepts greater than default limit per page)
Copy link
Member Author

Choose a reason for hiding this comment

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

Any thoughts on this? Is there any existing pattern/code for paging through the results if necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the question about existing patterns or code for paging through results, the forms-engine library does not currently define any standard patterns for pagination. However, I have a related question: Should we consider establishing a limit for the number of questions and sections allowed per page? This could help establish a standard for determining the page size.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be inclined to have a recommended max per page, not a strict max. Also, the way the code I added works, I believe it fetches all the concepts across all pages at the start, so I'm unsure if that would help in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking @samuelmale we can leave this as-is for now and I can create a future ticket about figuring out how to handle forms with lots of concepts?

const { data, error, isLoading } = useSWRImmutable<{ data: { results: Array<OpenmrsResource> } }, Error>(
`/ws/rest/v1/concept?references=${Array.from(references).join(',')}&v=${conceptRepresentation}`,
openmrsFetch,
);
return { concepts: data?.data.results, error, isLoading };
}
28 changes: 27 additions & 1 deletion src/ohri-form.component.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import next_visit_form from '../__mocks__/forms/ohri-forms/next-visit-test-form.
import months_on_art_form from '../__mocks__/forms/ohri-forms/months-on-art-form.json';
import age_validation_form from '../__mocks__/forms/ohri-forms/age-validation-form.json';
import viral_load_status_form from '../__mocks__/forms/ohri-forms/viral-load-status-form.json';
import reference_by_mapping_form from '../__mocks__/forms/ohri-forms/reference-by-mapping-form.json';
import external_data_source_form from '../__mocks__/forms/ohri-forms/external_data_source_form.json';
import mock_concepts from '../__mocks__/concepts.mock.json';
import { mockPatient } from '../__mocks__/patient.mock';
import { mockSessionDataResponse } from '../__mocks__/session.mock';
import demoHtsOpenmrsForm from '../__mocks__/forms/omrs-forms/demo_hts-form.json';
Expand All @@ -20,11 +22,12 @@ import demoHtsOhriForm from '../__mocks__/forms/ohri-forms/demo_hts-form.json';
import {
assertFormHasAllFields,
findMultiSelectInput,
findNumberInput,
findNumberInput, findRadioGroupInput, findRadioGroupMember,
findSelectInput,
findTextOrDateInput,
} from './utils/test-utils';
import { mockVisit } from '../__mocks__/visit.mock';
import {async} from "rxjs";

//////////////////////////////////////////
////// Base setup
Expand Down Expand Up @@ -320,6 +323,29 @@ describe('OHRI Forms:', () => {
});
});

describe("Concept references", () => {
const conceptResourcePath = when((url: string) => url.includes('/ws/rest/v1/concept?references=PIH:Occurrence of trauma,PIH:Yes,PIH:No,PIH:COUGH'));

when(mockOpenmrsFetch)
.calledWith(conceptResourcePath)
.mockReturnValue({ data: mock_concepts });

it('should add default labels based on concept display and substitute mapping references with uuids', async () => {
await act(async () => renderForm(null, reference_by_mapping_form));

const yes = await screen.findAllByRole('radio', { name: 'Yes' }) as Array<HTMLInputElement>;
const no = await screen.findAllByRole('radio', { name: 'No' }) as Array<HTMLInputElement>;
await assertFormHasAllFields(screen, [
{ fieldName: 'Cough', fieldType: 'radio' },
{ fieldName: 'Occurrence of trauma', fieldType: 'radio' },
]);
await act(async () => expect(no[0].value).toBe("3cd6f86c-26fe-102b-80cb-0017a47871b2"))
await act(async () => expect(no[1].value).toBe("3cd6f86c-26fe-102b-80cb-0017a47871b2"))
await act(async () => expect(yes[0].value).toBe("3cd6f600-26fe-102b-80cb-0017a47871b2"))
await act(async () => expect(yes[1].value).toBe("3cd6f600-26fe-102b-80cb-0017a47871b2"))
})
})

function renderForm(formUUID, formJson, intent?: string) {
return act(() => {
render(
Expand Down
Loading