Skip to content

Conversation

@Birkbjo
Copy link
Member

@Birkbjo Birkbjo commented Jun 3, 2024

No description provided.

@Birkbjo Birkbjo requested a review from tomzemp June 3, 2024 16:03
Copy link
Member

@tomzemp tomzemp left a comment

Choose a reason for hiding this comment

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

Looks good to me 🎉.

I tested with server in Oslo (CEST) and with browser time set to Bogotá and Vientiane, respectively. Worked as expected.

I still think it makes more sense to have substring(0,23) after toISOString() in getISOFormatLocalTimestamp, in order to get rid of the Z at the end of the string.

With this PR we:

  • select a date with Material UI date picker. Material UI thinks we want the date at 00:00 local time. If we've selected 31 May and we're in Vientiane, that is then 30 May 17:00 UTC.
  • we apply the conversion function ((date.getTime() - date.getTimezoneOffset() * 60000).toISOString(), we get a string back 2024-05-31T00:00:00.000Z
  • we send the date off and the server ignores the Z and just assumes that we want 2024-05-31T00:00:00.000 server time

I guess my argument for removing the Z is that the Z stands for zero offset (from UTC), so with the Z we are saying we are sending the time in UTC, but what we want (and what the backend does because it ignores time zones) is to have the date string representation of the time in the server time zone (without any time zone information).

@Birkbjo
Copy link
Member Author

Birkbjo commented Jun 5, 2024

Thanks for testing it @tomzemp !

still think it makes more sense to have substring(0,23) after toISOString() in getISOFormatLocalTimestamp, in order to get rid of the Z at the end of the string.

Alright, I see what you mean! My worry was that then it's not strictly an isoString anymore, but I guess the name of the function doesn't actually say that either.

Will remove the z and update the comment.

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants