(feat) HIE-9: Add MPI workflows to OpenMRS frontend #1397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -170 kB (-2.28%) Total Size: 7.28 MB
ℹ️ View Unchanged
|
reagan-meant
changed the title
ibacher
requested changes
Dec 13, 2024
packages/esm-patient-registration-app/src/patient-registration/patient-registration-hooks.ts
Outdated
Show resolved
Hide resolved
Comment on lines
46
to
51
| const { freeTextFieldConceptUuid, fieldConfigurations } = useConfig<RegistrationConfig>(); | ||
| const { isLoading: isLoadingPatientToEdit, patient: patientToEdit } = usePatient(isLocal ? patientUuid : null); | ||
| const { isLoading: isLoadingMpiPatient, patient: mpiPatient } = useMpiPatient(!isLocal ? patientUuid : null); | ||
| const { data: deathInfo, isLoading: isLoadingDeathInfo } = useInitialPersonDeathInfo(isLocal ? patientUuid : null); | ||
| const { data: attributes, isLoading: isLoadingAttributes } = useInitialPersonAttributes(isLocal ? patientUuid : null); | ||
| const { data: identifiers, isLoading: isLoadingIdentifiers } = useInitialPatientIdentifiers( |
There was a problem hiding this comment.
It feels like this hook would be better broken out into a useInitialFormValuesLocal(string) and useInitialFormValueMpi(string).
|
|
||
| useEffect(() => { | ||
| const fetchValues = async () => { | ||
| //check that patient is not dead |
| isLocal, | ||
| ]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This pattern feels off. This feels like it should be split into it's own hook or something an ultimately useSWR() if at all possible.
packages/esm-patient-registration-app/src/patient-registration/patient-registration-utils.ts
Outdated
Show resolved
Hide resolved
| preferred: false, | ||
| address1: '', | ||
| })), | ||
| age: null, |
There was a problem hiding this comment.
We have a birthdate, so we should be able to calculate the age. There's a helper for that in the framework.
There was a problem hiding this comment.
I see... Can we omit it? It's weird, because there's no reasonable numeric representation of "age", i.e., this probably shouldn't be a number at all.
Comment on lines
33
to
35
| dead: fhirPatient.deceasedBoolean, | ||
| deathDate: fhirPatient.deceasedDateTime, |
There was a problem hiding this comment.
In FHIR, only one of these two properties will be set at any given time, so we need to be responsive to that. If deceasedBoolean is set, deceasedDateTime will be null and vice-versa. At the very least, if deceasedDateTime is non-null we can infer that the patient is deceased if the datetime is now or in the past.
Comment on lines
36
to
38
| display: fhirPatient.name[0].text | ||
| ? fhirPatient.name[0].text | ||
| : `${fhirPatient.name[0].family} ${fhirPatient.name[0].given[0]}`, |
There was a problem hiding this comment.
The name format here probably needs to be templateable.
Suggested change
| display: fhirPatient.name[0].text | |
| ? fhirPatient.name[0].text | |
| : `${fhirPatient.name[0].family} ${fhirPatient.name[0].given[0]}`, | |
| display: fhirPatient.name[0].text ?? | |
| `${fhirPatient.name[0].given[0]} ${fhirPatient.name[0].family}`, |
| import { type SearchedPatient } from '../types'; | ||
| import { getCoreTranslation } from '@openmrs/esm-framework'; | ||
| export function inferModeFromSearchParams(searchParams: URLSearchParams) { | ||
| return searchParams.get('mode')?.toLowerCase() === 'external' ? 'external' : 'internal'; |
There was a problem hiding this comment.
These should probably be mpi and just no value, which can be interpretted as local or whatever.
| @@ -43,6 +44,7 @@ const patientSearchCustomRepresentation = `custom:(${patientProperties.join(',') | |||
| */ | |||
| export function useInfinitePatientSearch( | |||
| searchQuery: string, | |||
| searchMode: string, | |||
There was a problem hiding this comment.
Suggested change
| searchMode: string, | |
| searchMode: 'mpi' | null | undefined, |
reagan-meant
commented
Dec 18, 2024
| @@ -407,6 +517,186 @@ describe('Registering a new patient', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('Import an MPI patient record', () => { | |||
There was a problem hiding this comment.
Is there a plan to make the test suite more useful?
samuelmale
reviewed
Dec 18, 2024
| @@ -351,6 +352,21 @@ export const esmPatientRegistrationSchema = { | |||
| }, | |||
| }, | |||
| }, | |||
| identifier: { | |||
There was a problem hiding this comment.
identifierMappings reads better.
| identifier: { | ||
| _type: Type.Array, | ||
| _elements: { | ||
| identifierTypeSystem: { |
There was a problem hiding this comment.
fhirIdentifierSystem reads better.
| _type: Type.String, | ||
| _description: 'Identifier system from the fhir server', | ||
| }, | ||
| identifierTypeUuid: { |
There was a problem hiding this comment.
openmrsIdentifierTypeUuid would be more specific.
| @@ -180,6 +188,64 @@ export function useInitialFormValues(patientUuid: string): [FormValues, Dispatch | |||
| return [initialFormValues, setInitialFormValues]; | |||
| } | |||
|
|
|||
| export function useInitialFormValueMpi(patientUuid: string): [FormValues, Dispatch<FormValues>] { | |||
There was a problem hiding this comment.
Since it returns form values, let's rename this hook to something like: useMpiInitialFormValues or useFormValuesWithMpi or better.
| /* const [sourceRecord, setSourceRecord ] = useState<string | undefined>() | ||
| useDefineAppContext<MPIContext>("sourceRecord", { | ||
| sourceRecord: sourceRecord | ||
| }); */ | ||
|
|
||
| const handleCreatePatientRecord = (externalId: string) => { | ||
| //setSourceRecord(externalId); | ||
| navigate({ | ||
| to: `${window.getOpenmrsSpaBase()}patient-registration?sourceRecord=${externalId}`, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Do you plan on cleaning this up?
| {isMPIPatient && ( | ||
| <div> | ||
| <Tag className={styles.mpiTag} type="blue"> | ||
| 🌐 {'MPI'} |
There was a problem hiding this comment.
I think this tag should be wrapped in a t(...) function to take advantage of translation overrides.
| } | ||
|
|
||
| interface PatientSearchResultsProps { | ||
| searchResults: SearchedPatient[]; | ||
| searchTerm: string; | ||
| searchMode?: 'mpi' | null | undefined; |
There was a problem hiding this comment.
Shouldn't this be a binary type? Something like:
searchMode?: 'local' | 'mpi'; And we default to local.
samuelmale
reviewed
Jan 13, 2025
| } | ||
| })(); | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Would providing all the dependencies cause unnecessary rerenders?
| })(); | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [mpiPatient, isLoadingMpiPatient]); |
There was a problem hiding this comment.
Do we need isLoadingMpiPatient as a dependency or its ripple effect?
| @@ -39,10 +39,14 @@ export const PatientRegistration: React.FC<PatientRegistrationProps> = ({ savePa | |||
| const config = useConfig() as RegistrationConfig; | |||
| const [target, setTarget] = useState<undefined | string>(); | |||
| const { patientUuid: uuidOfPatientToEdit } = useParams(); | |||
| const sourcePatientId = new URLSearchParams(search).get('sourceRecord'); | |||
There was a problem hiding this comment.
@ibacher While we could use the AppContext API, wouldn’t a query parameter be a reasonable fit for passing a patient ID? Or is the concern about exposing identifiers, particularly those from external registries? If that’s not the case, creating a context for this might feel overkill. What do you think?
| setInitialFormValues(initialMPIFormValues); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [initialMPIFormValues, setInitialMPIFormValues]); |
There was a problem hiding this comment.
Is setInitialMPIFormValues a necessary dependency?
| @@ -407,6 +517,186 @@ describe('Registering a new patient', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('Import an MPI patient record', () => { | |||
There was a problem hiding this comment.
Is there a plan to make the test suite more useful?
| return []; | ||
| } | ||
| //Consider patient // https://github.com/openmrs/openmrs-esm-core/blob/main/packages/framework/esm-api/src/types/patient-resource.ts | ||
| const pts: Array<SearchedPatient> = []; |
There was a problem hiding this comment.
Suggested change
| const pts: Array<SearchedPatient> = []; | |
| const omrsPatients: Array<SearchedPatient> = []; |
|
@samuelmale using the appcontext was failing here and so either way we need to be using this for the time being |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Summary
https://openmrs.atlassian.net/browse/HIE-9
Screenshots
Screen.Recording.2024-12-13.at.14.32.28.mov
Related Issue
See background conversation here
Other