(feat) O3-4071: Improve Start Visit Form to support "Visit Locations" #2103
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
(feat) O3-4072: Improve Start Visit Form to Support Default Visit Type
# Conflicts: # yarn.lock
mogoodrich
commented
Nov 13, 2024
| @@ -8,8 +8,7 @@ export const esmPatientChartSchema = { | |||
| }, | |||
| disableChangingVisitLocation: { | |||
| _type: Type.Boolean, | |||
| _description: | |||
| "Whether the visit location field in the Start Visit form should be view-only. If so, the visit location will always be set to the user's login location.", | |||
| _description: 'Whether the visit location field in the Start Visit form should be view-only.', | |||
There was a problem hiding this comment.
The second half of this statement is no longer true, so I just removed it.
| _type: Type.Boolean, | ||
| _description: | ||
| 'On the start visit form, whether to restrict the visit location to locations with the Visit Location tag', | ||
| _default: false, |
There was a problem hiding this comment.
There's an argument to be made that this should default to 'true', though assumedly this would potentially break some existing implementations.
There was a problem hiding this comment.
Maybe we should mention that this requires the EMR API to be installed on the backend? (And maybe even provide a warning if it's set to true without it installed)
There was a problem hiding this comment.
And yes, good point, we should definitely note that, will add.
| @@ -2,7 +2,7 @@ import { type FetchResponse, openmrsFetch, useConfig } from '@openmrs/esm-framew | |||
| import useSWRImmutable from 'swr/immutable'; | |||
| import { type ChartConfig } from '../../config-schema'; | |||
|
|
|||
| export const useDefaultLoginLocation = () => { | |||
| export const useDefaultFacilityLocation = () => { | |||
There was a problem hiding this comment.
Changed this to a more accurate name (IMO)
|
|
||
| interface EmrApiConfigurationResponse { | ||
| atFacilityVisitType: OpenmrsResource; | ||
| // TODO: extract this into Core and include all fields (see Ward app) |
There was a problem hiding this comment.
This hook now existing in two places, here and in the Ward app... I can ticket adding this hook to Core.
| @@ -1,27 +0,0 @@ | |||
| import { type FetchResponse, type Location, openmrsFetch, restBaseUrl } from '@openmrs/esm-framework'; | |||
There was a problem hiding this comment.
No longer needed, we are switching to using the useLocations hook provided by core.
| ? locations | ||
| : selectedSessionLocation | ||
| ? [selectedSessionLocation] | ||
| : []; |
There was a problem hiding this comment.
This was making sure that if there were no valid locations, the selected session location still appears in the list... which seemed incorrect.
|
|
||
| visitType: visitToEdit?.visitType?.uuid, | ||
| visitLocation: visitToEdit?.location ?? sessionLocation ?? {}, | ||
| visitType: visitToEdit?.visitType?.uuid ?? emrConfiguration?.atFacilityVisitType?.uuid, |
There was a problem hiding this comment.
Set the visit type to the "atFacilityVisitType" as defined by the EMR API module, if present.
| @@ -664,14 +672,6 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({ | |||
| const minVisitStopDatetimeFallback = Date.parse(visitStartDate.toLocaleString()); | |||
| minVisitStopDatetime = minVisitStopDatetime || minVisitStopDatetimeFallback; | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
I'm no sure this was doing much of anything.
| </div> | ||
| </section> | ||
| {/* Lists available visit types if no atFacilityVisitType enabled. The content switcher only gets shown when recommended visit types are enabled */} | ||
| {!emrConfiguration?.atFacilityVisitType && ( |
There was a problem hiding this comment.
Hide the visit type selector if atFacilityVisitType has been specified.
| const requireActual = jest.requireActual('../hooks/useDefaultLocation'); | ||
| jest.mock('../hooks/useEmrConfiguration', () => ({ | ||
| useEmrConfiguration: jest.fn(() => ({})), | ||
| })); |
There was a problem hiding this comment.
According to best practices, we shouldn't do this anymore because the "jest.mocked(useEmrConfiguration)" should handle this... but it doesn't, and in mocked objects in this file it appears to both use the "jest.mocked" pattern and then use the jest.mock('../hooks...')... if someone other than me can understand what is going on here, happy to fix.
There was a problem hiding this comment.
We don't need partial mocks for hooks from core anymore, but we still need them for hooks defined within an app.
For core hooks, I think the magic comes from these places: jest.config.js, esm-core's mock.tsx and esm-react-utils's mock.tsx. It'd be nice if we have something similar for individual apps, or at least a standardized file to add partial mocks to so we don't have to do that in every *test.tsx file
There was a problem hiding this comment.
Thanks! So just to confirm, the takeaway here is what I did is fine, correct?
mogoodrich
requested review from
denniskigen,
ibacher,
mseaton,
chibongho and
vasharma05
|
And it looks like the build is failing because it's attempting to modify the yarn.lock file to set some of the OpenMRS versions back to next... I had to update to the latest version of ESM Core, did I screw something up? @denniskigen @ibacher ? Thanks... (I can look into this more tomorrow as well) |
|
Run |
Ah right, thanks @ibacher , done! |
|
Size Change: -125 kB (-0.78%) Total Size: 15.9 MB
ℹ️ View Unchanged
|
chibongho
reviewed
Nov 13, 2024
| * @param restrictByVisitLocationTag | ||
| */ | ||
| export function useDefaultVisitLocation(location: Location, restrictByVisitLocationTag: boolean) { | ||
| let url = `${restBaseUrl}/emrapi/locationThatSupportsVisits?location=${location?.uuid}`; |
There was a problem hiding this comment.
I think we need to check the feature flag for whether emrapi is installed
There was a problem hiding this comment.
Good point... I was going this would be "buyer beware" and the implementer would need to know that the EMR API module needed to be installed, but no reason not to add this check as well.
| _type: Type.Boolean, | ||
| _description: | ||
| 'On the start visit form, whether to restrict the visit location to locations with the Visit Location tag', | ||
| _default: false, |
There was a problem hiding this comment.
Maybe we should mention that this requires the EMR API to be installed on the backend? (And maybe even provide a warning if it's set to true without it installed)
| const requireActual = jest.requireActual('../hooks/useDefaultLocation'); | ||
| jest.mock('../hooks/useEmrConfiguration', () => ({ | ||
| useEmrConfiguration: jest.fn(() => ({})), | ||
| })); |
There was a problem hiding this comment.
We don't need partial mocks for hooks from core anymore, but we still need them for hooks defined within an app.
For core hooks, I think the magic comes from these places: jest.config.js, esm-core's mock.tsx and esm-react-utils's mock.tsx. It'd be nice if we have something similar for individual apps, or at least a standardized file to add partial mocks to so we don't have to do that in every *test.tsx file
(feat) O3-4072: Improve Start Visit Form to Support Default Visit Type
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.
(feat) O3-4071: Improve Start Visit Form to support "Visit Locations"
(feat) O3-4072: Improve Start Visit Form to Support Default Visit Type
Requirements
Summary
Screenshots
Example with restrictByVisitLocationTag set to true and emr.atFacilitiyVisitType specified... note that visit selector is hidden and only locations that are visit locations (KGH and Wellbody) appear in the location dropdown.
Related Issue
https://openmrs.atlassian.net/browse/O3-4071
https://openmrs.atlassian.net/browse/O3-4072
Other