-
Notifications
You must be signed in to change notification settings - Fork 245
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) O3-2163: Add support for editing end visit data/time on current visits #1712
(feat) O3-2163: Add support for editing end visit data/time on current visits #1712
Conversation
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.
Thanks for this, @usamaidrsk . Can you confirm that the start visit validator on lines 151-154 make sense? I don't really understand why if there is an existing visit then it's fine if the start date is in the future.
(value) => {
const today = dayjs();
const startDate = dayjs(value);
return displayVisitStopDateTimeFields ? true : startDate.isSameOrBefore(today, 'day');
},
Please add unit tests for the end date behavior. The logic of how we decide to show these fields or not is a little funny, so it would be very good to make the intention explicit in tests.
@vasharma05 @usamaidrsk could you guys sync up to figure out how to move this forward? |
Ping @usamaidrsk @vasharma05 |
Ping @usamaidrsk |
1991130
to
a3631a1
Compare
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.
Looks like we have a failing E2E test, and it is being caused by this work. Please fix the E2E test.
f030f2c
to
5d5ac23
Compare
? z | ||
.string() | ||
.refine((value) => value.match(time12HourFormatRegex), t('invalidTimeFormat', 'Invalid time format')) | ||
: z.string().optional(), |
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.
Should this optional one also have the refine
clause?
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, it should have one. Thanks for the catch
93c6325
to
7106630
Compare
Requirements
Summary
This PR provisions the adding of end date to current visits
Screenshots
add-end-visit-field-on-edit-visit-.mp4
Related Issue
O3-2163
Other