O3-2476 - Remove unnecessary conversion from date to string back to date #836
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -182 B (0%) Total Size: 2.36 MB ℹ️ View Unchanged
|
mseaton
commented
Oct 6, 2023
| @@ -36,7 +38,7 @@ describe('VisitDetailComponent', () => { | |||
| render(<VisitDetailComponent visitUuid={visitUuid} patientUuid={patientUuid} />); | |||
|
|
|||
| expect(screen.getByText(/Some Visit Type/)).toBeInTheDocument(); | |||
| expect(screen.getByText(/29-Jul-2023, 12:34 PM/)).toBeInTheDocument(); | |||
| expect(screen.getByText(formatDate(visitDate))).toBeInTheDocument(); | |||
|
|
|||
There was a problem hiding this comment.
This test started failing after I made the changes described. But this should not have been passing before if I'm understanding right, because a startDatetime with a "Z" at the end indicates UTC, but my computer running these tests is in EDT, so this shows up as 08:34 AM, not 12:34 PM. I've just changed the test to use a locally generated date, so whatever is passed is the same as what is tested. But it seems that this is potentially a fixed bug.
| @@ -63,7 +63,7 @@ const AppointmentDetails: React.FC<AppointmentDetailsProps> = ({ appointment }) | |||
| <span className={styles.historyGridCount}>{appointmentsCount.cancelledAppointments}</span> | |||
| </div> | |||
| <div> | |||
| <p className={styles.historyGridLabel}>{t('upcomming', 'Upcoming')}</p> | |||
| <p className={styles.historyGridLabel}>{t('upcoming', 'Upcoming')}</p> | |||
There was a problem hiding this comment.
Not sure where this came from - this cropped up as message changes when I tried to commit, I noticed this typo, so I fixed it (which enabled us to not lose the existing translations).
| @@ -36,7 +36,7 @@ export async function saveQueueEntry( | |||
| patient: { | |||
| uuid: patientUuid, | |||
| }, | |||
| startedAt: toDateObjectStrict(toOmrsIsoString(new Date())), | |||
| startedAt: new Date(), | |||
There was a problem hiding this comment.
Most of the changes in this PR are this. Find/replace wherever we are converting from date->string->date and just using the original date instead.
ibacher
approved these changes
Oct 10, 2023
Comment on lines
+101
to
107
| startDatetime: new Date( | ||
| dayjs(visitDate).year(), | ||
| dayjs(visitDate).month(), | ||
| dayjs(visitDate).date(), | ||
| hours, | ||
| minutes, | ||
| ), |
There was a problem hiding this comment.
It might be easier to write this as:
Suggested change
| startDatetime: new Date( | |
| dayjs(visitDate).year(), | |
| dayjs(visitDate).month(), | |
| dayjs(visitDate).date(), | |
| hours, | |
| minutes, | |
| ), | |
| startDatetime: dayjs(visitDate).set('hour', hours).set('minute', minutes).startOf('minute').toDate() |
There was a problem hiding this comment.
Agreed that this might be better, but since I haven't tested that, and the current retains the existing approach, I'll leave it for now.
|
@mseaton It would be good to have some kind of description for what this PR does besides just a ticket reference. |
mseaton
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Related Issue
See https://issues.openmrs.org/browse/O3-2476