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.

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

(feat) O3-3316 Add support for encounter diagnosis #298

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

CynthiaKamau
Copy link
Contributor

@CynthiaKamau CynthiaKamau commented May 28, 2024

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 feature attempts to introduce encounter diagnosis in RFE
Current RFE Schema :

{
      "label": "Reason for hospitalization:",
      "id": "hospReason",
      "questionOptions": {
	      "concept": "a8a07a48-1350-11df-a1f1-0026b9348838",
	      "rendering": "problem",
	      "rank": 1
      },
      "type": "diagnosis",
      "validators": []
 }

Proposed schema as in AFE :

{
    "label": "Test Diagnosis",
    "id": "DiagNosIS",
    "type": "diagnosis",
    "questionOptions": {
      "rendering": "repeating",
      "dataSource": "diagnoses",
      "diagnosis": {
        "isConfirmed": "true",
        "rank": 1
      }
    }
 }

Screenshots

Screen.Recording.2024-11-27.at.15.11.12.mov

Related Issue

Other

Copy link

github-actions bot commented May 28, 2024

Size Change: -263 kB (-17.32%) 👏

Total Size: 1.25 MB

Filename Size Change
dist/539.js 0 B -263 kB (removed) 🏆
ℹ️ View Unchanged
Filename Size Change
dist/151.js 379 kB 0 B
dist/225.js 2.57 kB 0 B
dist/254.js 88.7 kB 0 B
dist/277.js 1.85 kB 0 B
dist/300.js 642 B 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/658.js 263 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 +63 B (+0.02%)
dist/openmrs-esm-form-engine-lib.js 3.8 kB +1 B (+0.03%)

compressed-size-action

Comment on lines 527 to 534
const saveDiagnoses = await EncounterFormManager.saveDiagnosis(fields, savedEncounter);
if (saveDiagnoses) {
showSnackbar({
title: t('encounterDiagnosisSaved', 'Encounter diagnosis saved successfully'),
kind: 'success',
isLowContrast: true,
});
}
Copy link
Member

Choose a reason for hiding this comment

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

So the EncounterResource supports diagnoses as a create-able property shouldn't we be creating the diagnoses in a single transaction with the encounter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me add it the same way we handle orders

@brandones
Copy link
Contributor

@CynthiaKamau Ping, are you still working on this? Any blockers?

@CynthiaKamau
Copy link
Contributor Author

@CynthiaKamau Ping, are you still working on this? Any blockers?

Yes, there some fixes that need to be done on the backend to complete the work

@brandones
Copy link
Contributor

@CynthiaKamau Please provide links to the work or issues you're talking about. If there is no linkable issue/PR/talk/Slack, please tell us what work needs to be completed before this can move forward.

@CynthiaKamau
Copy link
Contributor Author

CynthiaKamau commented Jun 19, 2024

@CynthiaKamau Please provide links to the work or issues you're talking about. If there is no linkable issue/PR/talk/Slack, please tell us what work needs to be completed before this can move forward.

This is the link to the backend pr

@brandones
Copy link
Contributor

Ok @CynthiaKamau , looks like that backend PR is merged. Can we move forward with this?

@CynthiaKamau
Copy link
Contributor Author

Ok @CynthiaKamau , looks like that backend PR is merged. Can we move forward with this?

Im working on it now

@CynthiaKamau CynthiaKamau marked this pull request as ready for review September 25, 2024 10:58
Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

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

Nice work @CynthiaKamau! Are you planning on adding some test coverage?

@@ -300,6 +304,27 @@ export async function hydrateRepeatField(
}),
);
}

//handle diagnoses
const unMappedDiagnosis = encounter.diagnoses.filter((diagnosis) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const unMappedDiagnosis = encounter.diagnoses.filter((diagnosis) => {
const unMappedDiagnoses = encounter.diagnoses.filter((diagnosis) => {

@@ -28,6 +29,7 @@ export function prepareEncounter(
const obsForSubmission = [];
prepareObs(obsForSubmission, formFields);
const ordersForSubmission = prepareOrders(formFields);
const diagnosisForSubmission = prepareDiagnosis(formFields);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const diagnosisForSubmission = prepareDiagnosis(formFields);
const diagnosesForSubmission = prepareDiagnosis(formFields);

context: FormProcessorContextProps,
): Promise<any> {
const availableDiagnoses = sourceObject ?? (context.domainObjectValue as OpenmrsEncounter);
const matchedDiagnosis = availableDiagnoses.diagnoses?.find(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const matchedDiagnosis = availableDiagnoses.diagnoses?.find(
const matchedDiagnoses = availableDiagnoses.diagnoses?.find(

@@ -166,6 +166,7 @@ export class EncounterFormProcessor extends FormProcessor {
try {
const { data: savedEncounter } = await saveEncounter(abortController, encounter, encounter.uuid);
const saveOrders = savedEncounter.orders.map((order) => order.orderNumber);
const saveDiagnosis = savedEncounter.diagnoses.map((diagnosis) => diagnosis.display);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const saveDiagnosis = savedEncounter.diagnoses.map((diagnosis) => diagnosis.display);
const savedDiagnoses = savedEncounter.diagnoses.map((diagnosis) => diagnosis.display);

@@ -237,3 +240,19 @@ function handleQuestionsWithObsComments(sectionQuestions: Array<FormField>): Arr

return augmentedQuestions;
}

function handleDiagnosesDataSource(question: FormField) {
Copy link
Member

Choose a reason for hiding this comment

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

I would simply name this: handleDiagnosis(...)

sourceObject: OpenmrsResource,
context: FormProcessorContextProps,
): Promise<any> {
const availableDiagnoses = sourceObject ?? (context.domainObjectValue as OpenmrsEncounter);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const availableDiagnoses = sourceObject ?? (context.domainObjectValue as OpenmrsEncounter);
const encounter = sourceObject ?? (context.domainObjectValue as OpenmrsEncounter);

context: FormProcessorContextProps,
): Promise<any> {
const availableDiagnoses = sourceObject ?? (context.domainObjectValue as OpenmrsEncounter);
const matchedDiagnoses = availableDiagnoses.diagnoses.find(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const matchedDiagnoses = availableDiagnoses.diagnoses.find(
const matchedDiagnosis = availableDiagnoses.diagnoses.find(

},
];

describe('EncounterDiagnosesAdapter', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add test case(s) around:

  • editing a value and the fact that the edited value gets voided

Copy link
Member

Choose a reason for hiding this comment

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

@CynthiaKamau I discovered a bug while testing the entire feature.
Issue: When you delete a diagnosis (using the form repeat controls) and launch the form again in edit mode, the deleted diagnosis still exists, meaning it was never voided. This is partly why I requested the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try retesting now

@@ -166,6 +166,7 @@ export class EncounterFormProcessor extends FormProcessor {
try {
const { data: savedEncounter } = await saveEncounter(abortController, encounter, encounter.uuid);
const saveOrders = savedEncounter.orders.map((order) => order.orderNumber);
const saveDiagnoses = savedEncounter.diagnoses.map((diagnosis) => diagnosis.display);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const saveDiagnoses = savedEncounter.diagnoses.map((diagnosis) => diagnosis.display);
const savedDiagnoses = savedEncounter.diagnoses.map((diagnosis) => diagnosis.display);

(I would also rename saveOrders -> savedOrders)

// handle diagnoses
if (saveDiagnoses.length) {
showSnackbar({
title: translateFn('diagnosisSaved', 'Diagnosis(s) saved successfully'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: translateFn('diagnosisSaved', 'Diagnosis(s) saved successfully'),
title: translateFn('diagnosisSaved', 'Diagnosis(es) saved successfully'),

@@ -300,6 +304,40 @@ export async function hydrateRepeatField(
}),
);
}

const unMappedDiagnoses = encounter.diagnoses.filter((diagnosis) => {
return !assignedDiagnosesIds.includes(diagnosis?.diagnosis?.coded.uuid);
Copy link
Member

Choose a reason for hiding this comment

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

I think the Diagnosis should include the field's ID in it's formFieldPath otherwise we may end up including past diagnoses.

Copy link
Member

Choose a reason for hiding this comment

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

@CynthiaKamau did you see this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you can relook at the pr

sortedDiagnoses
.filter((diagnosis) => !diagnosis.voided)
.map(async (diagnosis) => {
const clone = cloneRepeatField(field, diagnosis, counter++);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a demo where we capture diagnoses with "repeat controls"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@brandones
Copy link
Contributor

Ping @samuelmale

@samuelmale
Copy link
Member

@brandones I left a few more comments for @CynthiaKamau

Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

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

Thanks for the great work @CynthiaKamau!

To make this more generic and practical, we need a way to extend the data source to load concepts from custom concept classes and, optionally, sets.

   questionOptions: {
      diagnosis: {
         rank?: number;
         isConfirmed?: boolean;
         conceptClasses?: Array<string>;
         conceptSet?: string;
      }
   }

cc: @ibacher

diagnosis: {
coded: value,
},
certainty: 'CONFIRMED',
Copy link
Member

Choose a reason for hiding this comment

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

This should default to PROVISIONAL but can be override-able through question options; something like: isConfirmedDiagnosis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially we were to make the feature and schema similar to what is in AFE, has that changed ?

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 not sure about AFE, but HFE and other standard forms in the RefApp, such as the visit note, all default to PROVISIONAL. With the increasing need to use the form engine to replace custom-developed forms, it's important to follow standard patterns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How will confirmed diagnosis be handled ?

Copy link
Member

Choose a reason for hiding this comment

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

From the form-design perspective, the field maybe configured like:

"questionOptions": {
      "diagnosis": {
        "isConfirmed": true;
      }
 }

@@ -182,6 +182,7 @@ export interface FormQuestionOptions {
comment?: string;
orientation?: 'vertical' | 'horizontal';
shownCommentOptions?: { validators?: Array<Record<string, any>>; hide?: { hideWhenExpression: string } };
rank?: number;
Copy link
Member

Choose a reason for hiding this comment

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

We should encapsulate all the supported options:

diagnosis: {
  rank?: number;
  isConfirmed?: boolean
}

@CynthiaKamau
Copy link
Contributor Author

Thanks for the great work @CynthiaKamau!

To make this more generic and practical, we need a way to extend the data source to load concepts from custom concept classes and, optionally, sets.

   questionOptions: {
      diagnosis: {
         rank?: number;
         isConfirmed?: boolean;
         conceptClasses?: Array<string>;
         conceptSet?: string;
      }
   }

cc: @ibacher

Is it supported by the backend ? I remember the question with the type problem had the same challenge

@CynthiaKamau
Copy link
Contributor Author

Thanks for the great work @CynthiaKamau!
To make this more generic and practical, we need a way to extend the data source to load concepts from custom concept classes and, optionally, sets.

   questionOptions: {
      diagnosis: {
         rank?: number;
         isConfirmed?: boolean;
         conceptClasses?: Array<string>;
         conceptSet?: string;
      }
   }

cc: @ibacher

Are the concept classes and sets supported by the backend, or passing more than one concept ? I remember the question with the type problem had the same challenge

@samuelmale
Copy link
Member

Is it supported by the backend ?

The short answer is yes! Through OpenMRS' REST API, it's possible to filter concepts by class (I believe this is what the underlying datasource is doing); Then for the set, you can fetch the provided concept and retrieve the set members.

I remember the question with the type problem had the same challenge

If I remember correctly, this was a pagination issue and the resolution was using a search-based mechanism as opposed to loading all concepts in a single request.

@CynthiaKamau
Copy link
Contributor Author

Is it supported by the backend ?

The short answer is yes! Through OpenMRS' REST API, it's possible to filter concepts by class (I believe this is what the underlying datasource is doing); Then for the set, you can fetch the provided concept and retrieve the set members.

I remember the question with the type problem had the same challenge

If I remember correctly, this was a pagination issue and the resolution was using a search-based mechanism as opposed to loading all concepts in a single request.

I hope it will work for multiple concept classes

@@ -0,0 +1,80 @@
import { type OpenmrsResource } from '@openmrs/esm-framework';
Copy link
Member

Choose a reason for hiding this comment

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

Rename the file name to singular encounter-diagnosis-adapter.ts

@@ -0,0 +1,80 @@
import { type OpenmrsResource } from '@openmrs/esm-framework';
import { type FormFieldValueAdapter, type FormProcessorContextProps } from '..';
Copy link
Member

Choose a reason for hiding this comment

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

These need to be combined with the ../types

@@ -0,0 +1,204 @@
import { type FormContextProps } from '../provider/form-provider';
Copy link
Member

Choose a reason for hiding this comment

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

Rename this file to singular diagnosis as well

@@ -0,0 +1,204 @@
import { type FormContextProps } from '../provider/form-provider';
import { type FormField } from '../types';
import { EncounterDiagnosesAdapter } from './encounter-diagnoses-adapter';
Copy link
Member

Choose a reason for hiding this comment

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

Trickle the changes to other files as well


export let assignedDiagnosesIds: string[] = [];

export const EncounterDiagnosesAdapter: FormFieldValueAdapter = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const EncounterDiagnosesAdapter: FormFieldValueAdapter = {
export const EncounterDiagnosisAdapter: FormFieldValueAdapter = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We agreed to name it in plural since it can be more than one here

Copy link
Member

Choose a reason for hiding this comment

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

Plural makes sense for the variables because you are actually dealing with multiple but singular makes more sense for file names and for this case the function/class. Kinda the same way we have EncounterLocationAdapter and EncounterRoleAdapter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more similar to orders from what i understood, and its in plural, @samuelmale can also weigh in since it was part of his review

Copy link
Member

Choose a reason for hiding this comment

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

I think the singular version (EncounterDiagnosisAdapter) is better because it's more atomic and seems to follow the Single Responsibility Principle. For consistency, we should rename other adapters to align with this pattern.

@@ -61,4 +62,8 @@ export const inbuiltFieldValueAdapters: RegistryItem<FormFieldValueAdapter>[] =
type: 'patientIdentifier',
component: PatientIdentifierAdapter,
},
{
type: 'diagnosis',
component: EncounterDiagnosesAdapter,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
component: EncounterDiagnosesAdapter,
component: EncounterDiagnosisAdapter,

return numberA - numberB; // Sort numerically based on formFieldPath
});

if (field.type === 'diagnosis') {
Copy link
Member

Choose a reason for hiding this comment

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

This might not specific to this PR but shouldn't we add an option to view the diagnosis in the Visit > encounters view.
How does the form look in read-only mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me do a screenshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might not specific to this PR but shouldn't we add an option to view the diagnosis in the Visit > encounters view. How does the form look in read-only mode?

They will be viewable there

Copy link
Member

Choose a reason for hiding this comment

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

They will be viewable there

The screen recording you shared doesn't show it. Maybe it's an old one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They will be viewable there

The screen recording you shared doesn't show it. Maybe it's an old one

Its on the video, if you check the diagnosis on the visits widget and compare it to the open form they are the same, minute 1:32

Copy link
Member

Choose a reason for hiding this comment

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

I think i should have been more clear, i meant the value for the diagnosis but i've also realized it's a similar behavior as the orders. There's a requirement to show the entered values (i.e Infection due to Entamoeba, Mucus in stool, etc) in the visits view but that's out of scope for this PR

Screenshot 2024-11-26 at 19 19 42

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, but i have updated the video to show the diagnosis well, however, on the visits widget, voided diagnosis are also displayed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants