(feat) HIE-9: Add MPI workflows to OpenMRS frontend #1313
Conversation
|
Size Change: -68.3 kB (-1.01%) Total Size: 6.68 MB
ℹ️ View Unchanged
|
| _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.
| 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.
| 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.
| 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
| 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' |
| @@ -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 |
|
@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. |
This reverts commit e009969.
|
I've wound up reverting this PR because of this regression in the Service queues app search experience: queues-regression.mp4From my initial debugging, the issue stems from this line. It seems as though the React Router context is not available when trying to access let searchParams = new URLSearchParams();
try {
[searchParams] = useSearchParams();
} catch (error) {
console.debug('Router context not available, using default search params');
}I think we need to test this some more before resolving to merge it just to ensure it doesn't subtly break in cases where the expected context is required, or where certain logic that's meant to be optional is marked as so. |
|
Thanks for looking into this, @denniskigen! I have a follow-up question regarding the issue you raised: Do we want to allow other apps that use the search feature to query the MPI as well? My initial thought is that we probably shouldn't, which would mean restricting the external search to only the main search page. @ibacher, do you think we could add an attribute or tag to the patient resource to enable conditional UI rendering, instead of relying on route parameters? |
|
@reagan-meant @denniskigen @ibacher would like to use this work on cross border integration, is there an agreement on how we can have this move this forward. |
In general, querying an MPI is a registration task and should probably be limited to that if that's feasible. |
|
@donaldkibet It's basically what Dennis said. We just need to ensure that we're not breaking search in other contexts. We may need to separate the "MPI search" out into it's own modal or something? |
|
@ibacher @denniskigen here are my suggestions |
|
For:
|
|
Re: 2 something like: export const useSearchParams() {
const [searchParams, setSearchParams] = useState(new URLSearchParams(window.location.search));
useEffect(() => {
const updateSearchParams = () => setSearchParams(new URLSearchParams(window.location.search));
window.addEventListener('popstate', updateSearchParams);
return () => window.removeEventListener('popstate', updateSearchParams);
}, []);
return searchParams;
} |
|
Ping @donaldkibet. I know you have been looking into this as well. |
Requirements
Summary
Screenshots
mpi.mov
Related Issue
https://openmrs.atlassian.net/browse/HIE-9
Other