-
Notifications
You must be signed in to change notification settings - Fork 283
(feat) O3-4228: RDE support for the order basket #2531
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
base: main
Are you sure you want to change the base?
Conversation
// check if the date is within the bounds of the current visit | ||
const isWithinBounds = isWithinInterval(completeDate, { | ||
start: new Date(currentVisit.startDatetime), | ||
end: currentVisit.stopDatetime ? new Date(currentVisit.stopDatetime) : new Date(), | ||
}); | ||
|
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.
Since this is RDE, I suggest restricting the date selection directly in the UI by setting appropriate boundaries. This will eliminate the need for an additional validation check elsewhere. Specifically, we can set
minDate = startDatetime/ visitStartDate
maxDate = stopDatetime / visitStopDate
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 have added a restriction from the date picker itself. The issue is that sometimes, as much as a user will click on a certain date, the time itself might be out of range. I am not sure the time picker has a way to limit the times natively like the date picker, so that check is for making sure that the user doesn't go out of the visit's bounds.
if (hasRdeDateBoundsError && currentVisit) { | ||
showSnackbar({ | ||
isLowContrast: true, | ||
kind: 'error', | ||
title: t('rdeDateOutOfBounds', 'Retrospective date is out of bounds'), | ||
subtitle: t( | ||
'rdeDateOutOfBoundsMessage', | ||
`The retrospective date must be within {{startDate}} and {{endDate}}.`, | ||
{ | ||
startDate: format(currentVisit.startDatetime, 'PPP hh:mm a'), | ||
endDate: currentVisit.stopDatetime | ||
? format(currentVisit.stopDatetime, 'PPP hh:mm a') | ||
: t('currentDate', 'current date'), | ||
}, |
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.
With setting the maxDate and minDate we will not need this now.
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.
Please refer to the comment above on this issue.
@chibongho remember the bug on |
const isAm = retrospectiveTimeFormat === 'AM'; | ||
const isPm = retrospectiveTimeFormat === 'PM'; | ||
|
||
if (isAm && parseInt(hours, 10) >= 12) { |
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 looks wrong, Won't 12:00am be changed to 12:00pm? The <TimePicker>
below has a regex pattern passed to it. Does it correctly prevent dates that are not in the 12-hour format?
There is also date / time parsing code in visit-date-time.component.tsx
. I think we should consolidate them instead of having separate implementations for different UIs.
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 below has a regex pattern passed to it. Does it correctly prevent dates that are not in the 12-hour format?
I referred to other places the <TimePicker />
component was used , they all seemed to have that pattern to prevent users from entering 24 hour format times but in my UI it seems to be still possible, for other places the time picker is used I haven't verified that. I have added logic to make sure users don't enter invalid times. Thanks for catching that!
let [rawHour, minute] = dateTime.retrospectiveTime.split(':').map(Number); | ||
|
||
// Adjust hour for AM/PM | ||
let hour = rawHour; |
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.
use convertTime12to24
instead of reimplementing its logic
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.
Done!. Simplifies things a lot. I did not know that function existed. Thanks.
Requirements
Summary
This PR adds RDE support to the order basket. When RDE is enabled, the user will now have the option to select a point in time within the bounds of a past visit in which the user can add retrospective data.
Screenshots
order-basket-with-rde.mp4
Related Issue
https://openmrs.atlassian.net/browse/O3-4228
Other