-
Notifications
You must be signed in to change notification settings - Fork 248
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
(feat) Allow privileged users to edit visit details and delete empty visits #1451
Conversation
…ient-chart into feat/edit-delete-visit
Size Change: +10.2 kB (0%) Total Size: 10.7 MB
ℹ️ View Unchanged
|
ab18ebe
to
1500767
Compare
packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
Outdated
Show resolved
Hide resolved
{Object.entries(errors) | ||
.filter(([key]) => !key) | ||
.map(([_, value]) => ( | ||
<InlineNotification | ||
kind={'error'} | ||
className={styles.inlineNotification} | ||
title={t('error', 'Error')} | ||
subtitle={value.message} | ||
/> | ||
))} |
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.
The errors from the superRefine
above is not having the key name, hence I had to filter them out.
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.
I don't see any checks to ensure that a visit is empty when it's deleted. In fact, there's logic in here to cascade visit deletes to also delete visit queue entries.
For preference, we would remove the delete stuff, but at the very least the functionality should be accurately described.
packages/esm-patient-chart-app/src/visit/visit-prompt/cancel-visit-dialog.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visit-prompt/cancel-visit-dialog.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visits-widget/visit.resource.tsx
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visits-widget/visit.resource.tsx
Outdated
Show resolved
Hide resolved
@@ -11,5 +11,5 @@ export const convertTime12to24 = (time12h, timeFormat: amPm) => { | |||
hours = hours === '12' ? hours : parseInt(hours, 10) + 12; | |||
} | |||
|
|||
return [hours, minutes]; | |||
return [hours, minutes] as [number, number]; |
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.
The typing here is wrong. It returns [ string | number, string ]
. I actually think the intention here is to return [ string, string ]
(so that we can represent midnight as '00'
and 11 PM as '23'
).
return [hours, minutes] as [number, number]; | |
return [hours, minutes]; |
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.
I'm also confused as to why this function is modified 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.
It was done because the the return value was shown as [any, any]
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.
Right, because time12h
is inferred as any
, but clearly should be string
, since we're expecting it to split on a character. Change that and the inferred type should be correct.
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.
Hi @ibacher , it was expected that the function will return number, because when I fixed the type as suggested by you, there were TS errors from other apps.
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.
@vasharma05 My point wasn't about what the function should return, just what it actually returns. Casting to as [number, number]
probably gets around Typescript reporting the issue, but it results in unsound types. In that case, the best thing to do would be to explicitly convert the string to a number, e.g.,
return [typeof hours === 'string' ? parseInt(hours) : hours, parseInt(minutes)];
...es/esm-patient-chart-app/src/visit/visit-action-items/delete-visit-action-item.component.tsx
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visit-prompt/cancel-visit-dialog.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visit-prompt/cancel-visit-dialog.test.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visits-widget/visit.resource.tsx
Show resolved
Hide resolved
actionButtonLabel: t('undo', 'Undo'), | ||
onActionButtonClick: restoreDeletedVisit, |
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 used?
Hi @ibacher , yes, so after the visit is voided
, the notification with an action button Undo
appears and if the user clicks the Undo
button, the restoreDeletedVisit
is called.
@@ -11,5 +11,5 @@ export const convertTime12to24 = (time12h, timeFormat: amPm) => { | |||
hours = hours === '12' ? hours : parseInt(hours, 10) + 12; | |||
} | |||
|
|||
return [hours, minutes]; | |||
return [hours, minutes] as [number, number]; |
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.
It was done because the the return value was shown as [any, any]
…ient-chart into feat/edit-delete-visit
Hi @ibacher ! |
…ient-chart into feat/edit-delete-visit
…ient-chart into feat/edit-delete-visit
@vasharma05 Could you resolve the conflicts? I think this is good to go. |
…ient-chart into feat/edit-delete-visit
Requirements
Summary
This PR allows the users with appropriate privelege to edit visit details and delete empty visits.
Editing visit's details
Editing past visit's details
Editing active visit's details
Deleting empty visit
Only visits with no encounters associated will be allowed to be deleted.
Related Issue
Other