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

dateHelperVCalendar.js: make timezone independent #4317

Conversation

BacLuc
Copy link
Contributor

@BacLuc BacLuc commented Dec 24, 2023

As you can see in https://github.com/BacLuc/ecamp3/actions/runs/7315399347, the pdf tests only work if the TZ is set to UTC.
This is because the .utc got forgotten here.

The further plan is to run the frontend and pdf tests in multiple timezones, that we detect this sooner.
But only after #4316 when i can use matrix jobs without breaking the required status checks.

@BacLuc BacLuc force-pushed the common-make-dateHelperVCalendar-tz-independent branch from 4a1d97b to 982e577 Compare December 24, 2023 16:11
@usu usu added the deploy! Creates a feature branch deployment for this PR label Dec 25, 2023
Copy link

github-actions bot commented Dec 25, 2023

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

@usu
Copy link
Member

usu commented Dec 25, 2023

This is not a bug, this is a feature 😄

The reason is that VCalendar always runs in local timezone (there's no functionality to set the timezone of VCalendar). Because our values in DB are always stored as UTC, we need to have this conversion from UTC -> local and back.

This means that testing the conversion functions is TZ-dependent (by design).

I thought we improved the test experience by overriding the TZ with a known value (#3837), but seems this is not working in that case.

@BacLuc
Copy link
Contributor Author

BacLuc commented Dec 25, 2023

We did do it for the frontend, but not for the pdf module.
Ok, so i have to think a little about it.
The name is a little irritating, because unix timestamps are in UTC per definition (https://en.wikipedia.org/wiki/Unix_time).
Thus there should be no timezone shift in this function converting timestamp to UTC string.
The jsdoc string says that the timestamp is in local timezone, but this makes no sense looking at the standard.

The scary part ist that ScheduleEntries are missing in the picasso of the pdf module depending on the time zone.
It seems i have to dig deeper.

@BacLuc BacLuc marked this pull request as draft December 25, 2023 14:16
@usu
Copy link
Member

usu commented Dec 26, 2023

We did do it for the frontend, but not for the pdf module. Ok, so i have to think a little about it. The name is a little irritating, because unix timestamps are in UTC per definition (https://en.wikipedia.org/wiki/Unix_time). Thus there should be no timezone shift in this function converting timestamp to UTC string. The jsdoc string says that the timestamp is in local timezone, but this makes no sense looking at the standard.

The scary part ist that ScheduleEntries are missing in the picasso of the pdf module depending on the time zone. It seems i have to dig deeper.

Yeah, I see how the naming can be confusing.

If you for example take the 2nd function utcStringToTimestamp, this function is doing 2 things:

  • Convert from string to timestamp format
  • Time shift to the new timezone in order to retain date + time

So a better comment might be:
// converts ISO String format (interpreted in UTC timezone) into a UNIX ms timestamp (retaining date + time, when interpreted in local computer's timezone)

Regarding pdf+print: As they both don't use VCalendar anymore, I think we should refactor them to not use this hack. This should not be necessary anymore.

@BacLuc
Copy link
Contributor Author

BacLuc commented Dec 26, 2023

Superseeded by #4329

@BacLuc BacLuc closed this Dec 26, 2023
@BacLuc BacLuc deleted the common-make-dateHelperVCalendar-tz-independent branch May 26, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy! Creates a feature branch deployment for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants