(feat) O3-1911: Reference Mappings instead of Concept UUIDs #86
Conversation
(feat) O3-2129: Form Engine: Support Default Labels form obs questions and answers
(feat) O3-2129: Form Engine: Support Default Labels form obs questions and answers
|
Size Change: -34.5 kB (-6%) ✅ Total Size: 548 kB
ℹ️ View Unchanged
|
|
@samuelmale @ibacher @denniskigen @donaldkibet I put this PR in draft form but it's not ready for merge yet, but I'd still be interested in your review, because as I told @ibacher I'm currently in the state where I know "just enough to be dangerous" and would love your thoughts on the approach. This is meant to handle the linked tickets and support:
You can see in the code, but when rendering a form, we now pull out all concept references from the form schema and fetch those concepts from the backend, and the swap out any form references and labels as neeeded when setting up the fields. It is working in my simple testing, but I should do some more advanced tests, and also should look into creating unit tests around this functionality. Some high-level questions that I had:
|
| 'custom:(uuid,display,conceptMappings:(conceptReferenceTerm:(conceptSource:(name),code)))'; | ||
|
|
||
| export function useConcepts(references: Array<string>) { | ||
| // TODO: handle paging (ie when number of concepts greater than default limit per page) |
There was a problem hiding this comment.
Any thoughts on this? Is there any existing pattern/code for paging through the results if necessary?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
mogoodrich
requested review from
samuelmale,
donaldkibet,
ibacher and
denniskigen
There was a problem hiding this comment.
Is this library currently in active use, or is it still a work-in-progress?
The library is actively in use, and there has been a production rollout reported by ICAP last year.
I haven't looked into how the engine renders previously entered forms/encounters
When it comes to rendering previously entered forms or encounters, the data binding is handled by the useInitialValues() hook. However, the actual value mapping is determined by the configured value adapter handler (See here). To elude from breaking things, you want to look at the ObsSubmissionHandler#getInitialValue which maps obs to fields primarily by the formFieldPath (see FormRecordable.java) and then fallsback to the concept based mapping which is currently assumed to be a UUID.
What is our policy on creating typescript types for OpenMRS objects?
It is preferable to reference typings from ESM Core if they already exist.
| const flattenedFieldsTemp = []; | ||
| const conceptReferencesTemp = []; |
There was a problem hiding this comment.
Given the high probability of duplicated entries, do we want to use an instance of a Set instead?
src/utils/ohri-form-helper.ts
Outdated
| */ | ||
| export function findConceptByReference(reference: string, concepts) { | ||
| // we only currently support uuids and reference term pairs in the format "SOURCE:TERM", so if no ":" return null | ||
| if (!reference.includes(':')) { |
There was a problem hiding this comment.
I think this function should expect both reference by uuid and concept mappings:
- The logic that loads concepts doesn't filter out
uuidtype references. - Additionally, we would like to load display strings that can serve as labels for questions lacking a label, where the concept reference is represented by a
uuid.
There was a problem hiding this comment.
Oh, good point. When I originally wrote this method it was just about replacing mappings with uuids, but, yes, we should also match on uuid now so that we can load display strings in the uuid case as well, i will fix.
| 'custom:(uuid,display,conceptMappings:(conceptReferenceTerm:(conceptSource:(name),code)))'; | ||
|
|
||
| export function useConcepts(references: Array<string>) { | ||
| // TODO: handle paging (ie when number of concepts greater than default limit per page) |
There was a problem hiding this comment.
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.
| 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, | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
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.
@mogoodrich Now that we are fetching concepts upfront, we don't need to do an extra fetch at the question level.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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?
Oh, great, thanks @samuelmale , I will check that out... I was hoping we were using formFieldPath, great to hear that we are. |
|
Thanks @samuelmale ... this is all very helpful in helping me move forward... I will continue working on this this week and let you know when I have a new PR ready. |
Great. taking a look into this closer @samuelmale I think that this should work fine... but Is there any way in the current UI to open a form in "edit" mode so I can test further? I can't figure it out for some reason. |
(feat) O3-2129: Form Engine: Support Default Labels form obs questions and answers
|
|
||
| /** | ||
| * Given a reference to a concept (either the uuid, or the source and reference term, ie "CIEL:1234") and a set of concepts, return matching concept, if any | ||
| * |
There was a problem hiding this comment.
Changed to support fetching concept by uuid
|
Okay, I added some tests and addressed a couple of the PR suggestions... still a couple outstanding questions to resolve, but I think this is close to being read y to merge in @samuelmale , let me know your thoughts. |
@mogoodrich Feel free to visit our demo site at, direct link to a patient's DB: https://ohri-demo.globalhealthapp.net/openmrs/spa/patient/2216e3ec-1984-4db0-8f03-4db848a86ab4/chart/Covid%20Assessments "View" or "Edit" an encounter through "Actions" Logins: admin, Admin123 |
|
So I was able to do some testing entering and editing forms this evening and things seem to be working well. @samuelmale what do you think about merging this in and I can ticket the outstanding tasks (linking the full concept to the field, and handling the case where the number of concepts on a form exceeds the page limit set for the REST endpoint) just to keep this moving along? Are there any E2E tests I should be aware of to make sure I haven't broken anything? :) Thanks! |
We currently don't support any E2E workflow. However, earlier this month I got a chance to meet @jayasanka-sack in Addis and we briefly hinted at the need of setting up an E2E pipeline for the forms engine. |
|
Thanks @samuelmale ! I ticketed the two issues we discussed above: https://issues.openmrs.org/browse/O3-2167 I also still need to update the documentation, thanks! |
(feat) O3-1911: Reference Mappings instead of Concept UUIDs
(feat) O3-2129: Form Engine: Support Default Labels form obs questions and answers
Requirements
Summary
Screenshots
Related Issue
Other