Skip to content
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: send date as string instead of epoch #6532

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

SebastianKrupinski
Copy link
Contributor

@SebastianKrupinski SebastianKrupinski commented Nov 27, 2024

Resolves: #6528

Testing Recommendation

  1. Test with existing appointment schedule and fresh appointment schedule (make sure when creating a new appointment schedule the browser time zone matches the calendar time zone, as there is another bug that will affect this)
  2. Test by selecting different time zones negative (North America/South America), positive (Australia/China/Etc) and matching the appointment schedule
  3. Test by changing browser time zone (either override browser setting or change system time zone). You can check your browser time zone here: https://webbrowsertools.com/timezone/
  4. Test by selecting time zones with an offset greater than the current time. (if the current time is 10.00 select zone that has offset of +11 hours, if the time is 14.00 select timezone that is -11 hours, this would in the past roll the date back one day)
  5. Test by selecting dates close to year end and start
  6. Compare all slot times to a external source I recommend https://www.worldtimebuddy.com/ or https://www.timeanddate.com/worldclock/converter.html you can preset all testing time zones and compare the times

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 23.40%. Comparing base (eb3b186) to head (91d787c).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/services/appointmentService.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6532      +/-   ##
============================================
- Coverage     23.41%   23.40%   -0.01%     
  Complexity      472      472              
============================================
  Files           249      249              
  Lines         11902    11901       -1     
  Branches       2282     2283       +1     
============================================
- Hits           2787     2786       -1     
  Misses         8788     8788              
  Partials        327      327              
Flag Coverage Δ
javascript 14.84% <0.00%> (ø)
php 59.45% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@st3iny st3iny changed the title fix: send date as sting instead of epoch fix: send date as string instead of epoch Nov 27, 2024
st3iny
st3iny previously requested changes Nov 27, 2024
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it seems to make sense. Time zones are ridiculously complicated.

I left a comment regarding the formatting in the frontend.

Comment on lines 191 to 193
const selectedDay = this.selectedDate.getFullYear().toString() + '-' +
(this.selectedDate.getMonth() + 1).toString() + '-' +
this.selectedDate.getDate().toString()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need proper formatting here. Month and date might only have a single digit.

Please use the getYYYYMMDDFromDate(date) from src/utils/date.js which already implements formatting as YYYY-MM-DD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import { getYYYYMMDDFromDate } from '../../utils/date.js'

// [...] further down

const selectedDay = getYYYYMMDDFromDate(this.selectedDate)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay,

2024-1-1 or 2024-01-01 make no difference, its the same date.

getYYYYMMDDFromDate will not work. toISOString shifts the date to UTC which changes the date in some situations, already tried toISOString.

export function getYYYYMMDDFromDate(date) {
	return new Date(date.getTime() - (date.getTimezoneOffset() * 60000))
		.toISOString()
		.split('T')[0]
}

Copy link
Member

@st3iny st3iny Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is why Georg implemented it in a way that reverts the shift to UTC (from the browser's local time zone). See date.getTime() - (date.getTimezoneOffset() * 60000)

I tested it with a fake time zone in Chrome and it should work correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll change it and try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't trust the calculation in getYYYYMMDDFromDate

Tested with:

  • Value In
    Wed Nov 27 2024 14:44:54 GMT-0500 (Eastern Standard Time)

  • Value produced by "new Date(date.getTime() - date.getTimezoneOffset() * 60000)"
    Wed Nov 27 2024 09:44:54 GMT-0500 (Eastern Standard Time)

As you can see it shifted the time by 5 hours. This would have the effect of shifting the date, if the input value time was less then the offset.

E.g.
Input
Wed Nov 27 2024 2:00:00 GMT-0500 (Eastern Standard Time)

Result
Wed Nov 26 2024 21:00:00 GMT-0500 (Eastern Standard Time)

Then toISOString produces this
2024-11-27T14:44:54.790Z

The end result seems correct in my browser and time zone but will this be correct for every time zone and browser. IDK

Copy link
Member

@st3iny st3iny Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that it is confusing. The thing is, that JavaScript's Date is not time zone aware. It always converts to the local time zone, regardless of which time zone you pass to the constructor.

We have unit tests in place to ensure that this implementation works and it is used throughout Calendar.

If it breaks we can fix it. The important thing is that we have one central implementation and not many copied across the code base because this is way harder to maintain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yeah, let's not get pedantic over 3 lines of code.

@st3iny st3iny dismissed their stale review November 28, 2024 07:51

Let's not get pedantic over 3 lines of code.

@ChristophWurst
Copy link
Member

@GVodyanov could you give this a test with the instructions provided? :)

@GVodyanov
Copy link
Contributor

Hey, would you be able to explain this? I'm pretty confused as to why just that Tuesday doesn't work (all other days as far as I can see are fine)

Screenshot from 2024-12-09 19-33-42
Screenshot from 2024-12-09 19-33-28

Kooha-2024-12-09-19-32-48.mp4

@SebastianKrupinski
Copy link
Contributor Author

Hey, would you be able to explain this? I'm pretty confused as to why just that Tuesday doesn't work (all other days as far as I can see are fine)

Do you have an "all day event" on that Tuesday? That would cause the slots to be filtered out as a conflict during that time.

@GVodyanov
Copy link
Contributor

Hey, would you be able to explain this? I'm pretty confused as to why just that Tuesday doesn't work (all other days as far as I can see are fine)

Do you have an "all day event" on that Tuesday? That would cause the slots to be filtered out as a conflict during that time.

I'm dumb

@SebastianKrupinski
Copy link
Contributor Author

Do you have an "all day event" on that Tuesday? That would cause the slots to be filtered out as a conflict during that time.

I'm dumb

🤣 don't worry, I did the same thing

@SebastianKrupinski SebastianKrupinski merged commit 95752cb into main Dec 10, 2024
37 of 39 checks passed
@SebastianKrupinski SebastianKrupinski deleted the fix/issue-6528-ui-epoch branch December 10, 2024 19:05
@ChristophWurst
Copy link
Member

/backport to stable5.0

@backportbot backportbot bot added the backport-request A backport was requested for this pull request label Dec 11, 2024
@backportbot backportbot bot removed the backport-request A backport was requested for this pull request label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
Development

Successfully merging this pull request may close these issues.

Appointment slots are generated for the wrong day due to UI sending date in epoch
4 participants