-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(imaging): create basic imaging module #2250
Conversation
… profile dropdown
…nslation key more clearly fix HospitalRun#2236
…/AlexTan331/hospitalrun-frontend into display-user-in-profile-dropdown
add permissions to request new imaging and view all imagings; add links to sidebar and navbar for creating new imaging and viewing imagings
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hospitalrun/hospitalrun-frontend/82kmapk97 |
src/HospitalRun.tsx
Outdated
@@ -53,6 +54,7 @@ const HospitalRun = () => { | |||
<Route path="/labs" component={Labs} /> | |||
<Route path="/incidents" component={Incidents} /> | |||
<Route path="/settings" component={Settings} /> | |||
<Route path="/imagings" component={Imagings} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just go with /imaging
for the route name
useEffect(() => { | ||
setSearchFilter('all' as ImagingFilter) | ||
}, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this useEffect
needed? it seems like it might not be since we initialize the state to be all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is to display all the imaging with any status (requested
, canceled
, and completed
). And this method can be used for searching
imaging with specific status in the future.
src/imagings/ViewImagings.tsx
Outdated
label: t('imagings.imaging.requestedBy'), | ||
key: 'requestedBy', | ||
formatter: (row) => extractUsername(row.requestedBy), | ||
}, // need to be formated later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the comment, there appears like there may be some work left? if not, remove comment.
const store = mockStore({ | ||
title: '', | ||
user: { permissions: [Permissions.ViewImagings, Permissions.RequestLab] }, | ||
imagings: { imagings: [] }, | ||
} as any) | ||
titleSpy = jest.spyOn(titleUtil, 'default') | ||
jest.spyOn(ImagingRepository, 'findAll').mockResolvedValue([]) | ||
await act(async () => { | ||
await mount( | ||
<Provider store={store}> | ||
<Router history={createMemoryHistory()}> | ||
<ViewImagings /> | ||
</Router> | ||
</Provider>, | ||
) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lately, we have been putting this kind of setup in a setup
function. I think we should continue this pattern since it makes it easy to make sure each test is not dependent on a previous test.
Each describe can share the setup
function. i.e. one setup function for the whole test suite.
wrapper = mount( | ||
<Provider store={store}> | ||
<Router history={history}> | ||
<NewImagingRequest /> | ||
</Router> | ||
</Provider>, | ||
) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use a setup function here as well.
It would like something like:
const setup = () => {
const store = mockStore({ title: '', imaging: { status: 'loading', error: {} } } as any)
history.push('/imagings/new')
wrapper = mount(
<Provider store={store}>
<Router history={history}>
<NewImagingRequest />
</Router>
</Provider>,
)
return { wrapper: wrapper as ReactWrapper, history, store }
}
This will eliminate the need for global variables in this file.
@AlexTan331 this is awesome work! let me know if you have any questions about getting the tests passing or any of the code review feedback. |
…ace route name to 'imaging' fix HospitalRun#2242
Fixes #2242
Changes proposed in this pull request:
No new dependency added
Side note: