(feat) HIE-9: Add MPI workflows to OpenMRS frontend #1313
Merged
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: -68.3 kB (-1.01%) Total Size: 6.68 MB
ℹ️ View Unchanged
|
ibacher
requested changes
Sep 9, 2024
| _default: null, | ||
| _description: 'Identifier type uuid of OpenMRS to map the identifier system', | ||
| }, | ||
| identifierTypeName: { |
There was a problem hiding this comment.
This should just be fetched from the backend, not part of the configuration.
| const [patient, setPatient] = useState<fhir.Patient>(null); | ||
| const [isLoading, setIsLoading] = useState(false); | ||
|
|
||
| let url = `${fhirBaseUrl}/Patient/${patientId}/$cr`; |
There was a problem hiding this comment.
I'd just calculate this in the useEffect() hook where you use it. E.g.,
useEffect(() => {
if (patientId) {
const url = `${fhirBaseUrl}/Patient/${patientId}/$cr`;
setIsLoading(true);
openmrsFetch(url).then((response) => {
if (response.data) {
setPatient(response.data);
setIsLoading(false);
}
});
}
}, [patientId, url]);| if (patientId && url) { | ||
| setIsLoading(true); | ||
| openmrsFetch(url).then((response) => { | ||
| if (response.status == 200 && response.data) { |
There was a problem hiding this comment.
You don't need to check the response status. openmrsFetch does this for you.
| useEffect(() => { | ||
| if (patientId && url) { | ||
| setIsLoading(true); | ||
| openmrsFetch(url).then((response) => { |
There was a problem hiding this comment.
It's not necessary, but you might consider using using SWR instead of doing all of this. That handles most of the isLoading and can cache results.
| if (response.status == 200 && response.data) { | ||
| setPatient(response.data); | ||
| setIsLoading(false); | ||
| } |
There was a problem hiding this comment.
I think setIsLoading() needs to be unconditionally set to false once the request is done; otherwise you'll display a loading spinner endlessly if the request isn't successful.
Comment on lines
195
to
198
| export function getIdentifierFieldValuesFromFhirPatient( | ||
| patient: fhir.Patient, | ||
| identifierConfig, | ||
| ): { [key: string]: any } { |
There was a problem hiding this comment.
This could be better typed than it is like this.
| patient: fhir.Patient, | ||
| identifierConfig, | ||
| ): { [key: string]: any } { | ||
| const identifiers: { [key: string]: any } = {}; |
There was a problem hiding this comment.
This should also be an explicit type. Don't. use any if you can avoid it.
| @@ -0,0 +1,45 @@ | |||
| import capitalize from 'lodash-es/capitalize'; | |||
There was a problem hiding this comment.
Suggested change
| import capitalize from 'lodash-es/capitalize'; | |
| import { capitalize } from 'lodash-es'; |
| import capitalize from 'lodash-es/capitalize'; | ||
|
|
||
| export function inferModeFromSearchParams(searchParams: URLSearchParams) { | ||
| return searchParams?.get('mode')?.toLowerCase() == 'external' ? 'external' : 'internal'; |
There was a problem hiding this comment.
Suggested change
| return searchParams?.get('mode')?.toLowerCase() == 'external' ? 'external' : 'internal'; | |
| return searchParams.get('mode')?.toLowerCase() === 'external' ? 'external' : 'internal'; |
| return searchParams?.get('mode')?.toLowerCase() == 'external' ? 'external' : 'internal'; | ||
| } | ||
|
|
||
| export function mapToOpenMRSPatient(fhirPatients: any): any { |
There was a problem hiding this comment.
This could also benefit from explicit types on both sides. Presumable fhirPatients is a fhir.Bundle and each entry is a fhir.Patient or similar.
There was a problem hiding this comment.
I think there's also an OpenMRS patient type in core that can be the return type.
reagan-meant
force-pushed
the
HIE-9
branch
2 times, most recently
from
b14aa33
to
31fc392
Compare
ibacher
approved these changes
Oct 3, 2024
| age: null, | ||
| birthdate: fhirPatient.birthDate, | ||
| gender: capitalize(fhirPatient.gender), | ||
| dead: !fhirPatient.active, |
There was a problem hiding this comment.
This seems wrong or, at the very least, very specific. dead corresponds to FHIR's deceasedBoolean (and deathDate with deceasedDateTime). active === false just means "this patient record is no longer actively used in this system".
| dead: !fhirPatient.active, | ||
| deathDate: '', | ||
| personName: { | ||
| display: `${fhirPatient.name[0].family} ${fhirPatient.name[0].given[0]}`, |
There was a problem hiding this comment.
I always dislike inferring what the display should be for a name, since this is very location-sensitive, e.g., how its written, how it's formatted etc. I'd prefer that we either pull from the name[0].text first.
| render(<PatientSearchButton />); | ||
|
|
||
| render( | ||
| <BrowserRouter> |
There was a problem hiding this comment.
It's probably better to use a MemoryBrowser in tests.
| @@ -5,6 +5,8 @@ import AdvancedPatientSearchComponent from '../patient-search-page/advanced-pati | |||
| import Overlay from '../ui-components/overlay'; | |||
| import PatientSearchBar from '../patient-search-bar/patient-search-bar.component'; | |||
| import { type PatientSearchConfig } from '../config-schema'; | |||
| import { inferModeFromSearchParams } from '../mpi/utils'; | |||
| import { useSearchParams } from 'react-router-dom'; | |||
There was a problem hiding this comment.
Very minor, but this import should come before all relative imports.
| kind="ghost" | ||
| renderIcon={'Search'} | ||
| onClick={(e) => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
Do we need the preventDefault() call here?
|
Do we expect users to know what "MPI" means? I would think "Check for national chart" or "Search health exchange" would be easier for providers/users to understand. |
|
@bmamlin How about 'Search External Registry' |
ibacher
reviewed
Oct 10, 2024
ibacher
approved these changes
Oct 21, 2024
| @@ -35,6 +36,8 @@ export const patientSearchBar = getSyncLifecycle(patientSearchBarComponent, opti | |||
| export function startupApp() { | |||
| defineConfigSchema(moduleName, configSchema); | |||
|
|
|||
| registerFeatureFlag('mpiFlag', 'MPI Service', 'Enables the Master Patient Index workflows'); | |||
There was a problem hiding this comment.
Probably should've said this: our preferred means of registering feature flags is via routes.json. Basically add the same properties, but to a featureFlag definition there.
|
Could you move this along, @reagan-meant? |
|
@denniskigen are you available to review? |
|
@reagan-meant , could you fix the conflicts? Looks like @ibacher has approved. Once you fix the conflicts I'll give it a once over and merge. |
|
Thanks for your efforts on this, @reagan-meant. I’ll merge it in soon. There are a few areas that need tweaking, but that can happen once this gets checked in. How prepared is the backend to support this work? Can we get that support set up reasonably soon? I’m excited to see how this evolves! |
|
@denniskigen here is the relevant backend module |
3 tasks
|
@reagan-meant @ibacher I have some questions and suggestions about the module dependencies:
I can take care of adding |
|
We'd like to get it in reasonably soon, but I'm not sure it's going to be enabled for, e.g., the dev3 / test3 / o3 stack. This is maybe something we should discuss on the engineering call. Basically, I'm thinking about spinning up an integrated MPI demo that has, e.g., a demo CR linked to it and I think that's a separate stack in the sense that it wouldn't make sense in the RefApp. However, I am concerned that having functionality not activated in dev3 would make it very easy for that functionality to break. That's probably something we can solve with e2e tests, but we'd need a strategy to include some kind of dummy CR in the e2e tests. |
|
+1 for discussing it on the eng leads call. Thanks. |
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
Screenshots
mpi.mov
Related Issue
https://openmrs.atlassian.net/browse/HIE-9
Other