-
Notifications
You must be signed in to change notification settings - Fork 23
Add procedure history management system with encounter type and form #41
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.
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
base: main
Are you sure you want to change the base?
Conversation
…and form JSON schema.
denniskigen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start, @Mwanje! I've left some feedback about a few things we could improve.
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
| "rendering": "select", | ||
| "isSearchable": true, | ||
| "concept": "1651AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", | ||
| "answers": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the plan to eventually include all procedures from the collection as possible answers for this field so we end up with a complete procedure catalog instead of just 4 sample procedures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denniskigen the plan is to include all procedures from the collection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be done in this PR as well or we can do a follow PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an easier way we can maintain the set of answers here, e.g., by referring to a concept set or the existing concept answers? Inevitably for a real-world case this list will get long enough that maintaining it in JSON is likely impractical. @samuelmale Interested in your thoughts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an easier way we can maintain the set of answers here, e.g., by referring to a concept set or the existing concept answers?
Yes! The form-engine infers a field's answers from the associated concept if the questionOptions.answers is empty. (Note that concept sets aren't supported)
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/encountertypes/encountertypes_core-demo.csv
Outdated
Show resolved
Hide resolved
|
@denniskigen have pushed a commit addressing comments |
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Show resolved
Hide resolved
| "rendering": "select", | ||
| "concept": "1732AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", | ||
| "answers": [ | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the current duration units:
- The ordering of units seems random. Could we order them logically (e.g., minutes, hours, days, weeks, months, years), either ascending or descending?
- The validation currently only blocks durations greater than 100 years. Should we consider unit-specific limits (e.g., prevent excessive values in hours or minutes when those units are selected)?
- Referring to this sample procedure list (~50 procedures):
- Which time units are most relevant/commonly used for these procedures?
- What's the optimal ordering for user experience?
- Does the current validation logic fit typical use cases?
- Are months and years realistic for procedure duration tracking, or should we limit to shorter timeframes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think "Number of occurrences" is a completely separate question from "Duration". I.e., if I apply 3 bandages to a patient, that does need to be documented, but the procedure still took, say, 5 minutes regardless.
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
configuration/backend_configuration/ampathforms/procedure-history-form.json
Outdated
Show resolved
Hide resolved
|
Very useful context from @ibacher in the linked PR openmrs/openmrs-esm-patient-chart#2748 (comment). |
| { | ||
| "type": "js_expression", | ||
| "failsWhenExpression": "procedureDurationUnit === '1733AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA' && Number(procedureDuration) > 1440", | ||
| "message": "Duration cannot exceed 1440 minutes.. Change unit or reduce duration." | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we'd want this to be more like 2 days (2880 minutes) rather than 24 hours, unless there's a way to input something like 24 hours + 30 minutes or the like.
| { | ||
| "type": "js_expression", | ||
| "failsWhenExpression": "procedureDurationUnit === '1073AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA' && Number(procedureDuration) > 52", | ||
| "message": "Duration cannot exceed 52 weeks.. Change unit or reduce duration." | ||
| }, | ||
| { | ||
| "type": "js_expression", | ||
| "failsWhenExpression": "procedureDurationUnit === '1074AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA' && Number(procedureDuration) > 12", | ||
| "message": "Duration cannot exceed 12 months.. Change unit or reduce duration." | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we have a good reason for allowing durations in weeks, months or years?
| } | ||
| ] | ||
| }, | ||
| "clearWhenExpression": "Number(procedureDuration) > 99 && (procedureDurationUnit === '1734AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA')", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are other durations handled as validations, but this is a "clearWhenExpression"? It feels like we should use one tool for limits here.
This PR introduces metadata to support the structured documentation and management of patients' procedure history. These include:
Read the Procedure history wiki page to learn more.