-
Notifications
You must be signed in to change notification settings - Fork 232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(fix) O3-3661: Admission Requests List should include both Admission and Transfer requests #1255
Conversation
…and Transfer requests
@@ -6,7 +6,10 @@ import useWardLocation from './useWardLocation'; | |||
const defaultRep = | |||
'custom:(dispositionLocation,dispositionType,disposition,dispositionEncounter:full,patient:default,dispositionObsGroup,visit)'; | |||
|
|||
export function useInpatientRequest(dispositionType: Array<DispositionType> = ['ADMIT'], rep: string = defaultRep) { | |||
export function useInpatientRequest( | |||
dispositionType: Array<DispositionType> = ['ADMIT', 'TRANSFER'], |
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.
technically I didn't need to change the default value, but if we are going to have a default value seemed to make sense to have it equal to the only value we are currently passing in.
import { Loading } from '@carbon/react'; | ||
import { InlineNotification } from '@carbon/react'; | ||
import { useTranslation } from 'react-i18next'; | ||
|
||
const AdmissionRequestsWorkspace: React.FC<DefaultWorkspaceProps> = () => { | ||
const { t } = useTranslation(); | ||
const { inpatientRequests, isLoading, error } = useInpatientRequest(); | ||
const admissionRequests = inpatientRequests?.filter((request) => request.dispositionType == 'ADMIT'); |
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 shouldn't have to filter here if we are only fetching types we want?
Size Change: +23 B (0%) Total Size: 6.13 MB ℹ️ View Unchanged
|
Hi @mogoodrich , please don't delete the PR template. Could you please add a screenshot or video? It's hard to evaluate the work without that. |
Sorry about deleting the PR template @brandones . RE: screenshot, I realized after I made the changes that this won't be fully testable (and therefore demoable) until we implement https://openmrs.atlassian.net/issues/O3-3247?filter=10648 , but I just added a screenshot to the PR showing what this PR should change. |
@@ -29,7 +27,7 @@ const AdmissionRequestsWorkspace: React.FC<DefaultWorkspaceProps> = () => { | |||
|
|||
return ( | |||
<div className={styles.admissionRequestsWorkspace}> | |||
{admissionRequests.map((admissionRequest, indx) => ( | |||
{inpatientRequests.map((admissionRequest, indx) => ( | |||
<AdmissionRequestCard key={indx} patient={admissionRequest.patient} /> |
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 there something in the response we could use as a key here?
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.
Or just like inpatient-request-workspace-card-${indx}
.
@vasharma05 sorry we didn't catch this in the original PR with this change, but you can't just use an integer as a React key; it is very easy to wind up with duplicate keys that way. Duplicate keys can cause really bizarre and confusing bugs.
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.
<AdmissionRequestCard key={indx} patient={admissionRequest.patient} /> | |
<AdmissionRequestCard key={`inpatient-request-workspace-card-${indx}`} patient={admissionRequest.patient} /> |
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.
LGTM
…and Transfer requests (#1255) (follow-on update after merging)
https://openmrs.atlassian.net/browse/O3-3661
The terminology is a little confusing, but according to @Fiona Anderson the admissions request list should should those awaiting admission to the ward, and those awaiting transfer into the ward from another ward.
There isn't a currently a way to set up transfer questions on our demo server (that will come with https://openmrs.atlassian.net/issues/O3-3247?filter=10648, so this won't be fully testable until then) but this PR expands the admission request view to dsplay both admission and transfer requests for the current ward in the following view.