pdf: fix picasso rendering in other timezones than Europe/Zurich #4329
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
This allows to test if the different timezones render the same snapshot. It also makes it easier to review the diff, because the diff only applies to one snapshot. Else if a new snapshot is added or some parts move, the diff is difficult to read.
This allows to run some tests in different timezones. (if dayjs.tz is used instead of dayjs) Also add jsdoc to the export, that we know that the utc and the tz property exist.
To show that it fails. Use the .tz variant in dateHelperVCalendar.js that it is possible to change tz at runtime.
BacLuc
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
BacLuc
changed the title
BacLuc
force-pushed
the
fix-pdf-in-other-timezones
branch
from
d4e32e8
to
e73b988
Compare
BacLuc
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
carlobeltrame
approved these changes
Dec 26, 2023
| ) | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Calculates the visible day start timestamp | ||
| * @param day a day object of the api |
There was a problem hiding this comment.
Maybe keep the prose descriptions from before?
usu
approved these changes
Dec 26, 2023
| @@ -312,13 +329,13 @@ export function positionStyles(scheduleEntries, day, times) { | |||
| const left = leftAndWidth[scheduleEntry.id]?.left || 0 | |||
| const width = leftAndWidth[scheduleEntry.id]?.width || 0 | |||
| const top = positionPercentage( | |||
| utcStringToTimestamp(scheduleEntry.start) - utcStringToTimestamp(day.start), | |||
| dayjs.utc(scheduleEntry.start).valueOf() - dayjs.utc(day.start).valueOf(), | |||
There was a problem hiding this comment.
Instead of resorting to valueOf(), we could also use the the diff function:
dayjs.utc(scheduleEntry.start).diff( dayjs.utc(day.start), 'hours', true)
Then we get the difference in hours directly and don't need to divide by (3600.0 * 1000) again within positionPercentage.
There was a problem hiding this comment.
Yes, but i will leave that for another PR.
| times | ||
| ) | ||
| const bottom = | ||
| 100 - | ||
| positionPercentage( | ||
| utcStringToTimestamp(scheduleEntry.end) - utcStringToTimestamp(day.start), | ||
| dayjs.utc(scheduleEntry.end).valueOf() - dayjs.utc(day.start).valueOf(), |
There was a problem hiding this comment.
Yes, but i will leave that for another PR.
|
|
||
| dayjs.extend(utc) | ||
| dayjs.extend(timezone) |
There was a problem hiding this comment.
Do we really use this extension? Everything should be done with utc now, shouldn't it?
There was a problem hiding this comment.
Not really, but it makes it easier to always use the dayjs.tz object which uses the changed timezone instead of the dayjs object.
so i would leave it here.
BacLuc
force-pushed
the
fix-pdf-in-other-timezones
branch
from
e73b988
to
9337caf
Compare
In the picasso render logic in pdf and in print, we can use the utc dayjs objects directly, instead of converting them to the timestamps which represent the same time in the local timezone. This fixes the render bugs of the picasso in the pdf module if you render it in another timezone. Changed functions: - positionStyles: only used in pdf and print - filterScheduleEntriesByDay: ony used in pdf and print Also remove the other usages in pdf (DayColumn) and print (CalendarDay).
This was only needed to run the tests in multiple timezones. Now this is not needed anymore. (in a later pr we may export dayjs.tz instead of the dayjs object which does not allow to change the timezone between tests).
That nobody has the idea to reuse it in pdf or print. We only need to calculate a timestamp representing the DateTime received in UTC as the same DateTime in the local timezone.
BacLuc
force-pushed
the
fix-pdf-in-other-timezones
branch
from
9337caf
to
a527665
Compare
BacLuc
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
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.
Some time ago i detected that the pdf tests yielded another snapshot in the docker container than when ran locally.
The scary part was that some ScheduleEntries were not displayed.
After a lot of digging a could pin it to the wrong usage of timestampUtcString and utcStringToTimestamp.
In print it was no problem, because there our containers ran anyway in UTC.
But in pdf this is a problem because we use the browser timezone.
Because we don't use vCalendar anymore in print and pdf, we don't need the hack to convert UTC strings into timestamps which represent the same (numeric) time in the local timezone.
The refactoring is done step by step by first reproducing the problem in the test, and then refactoring it.
Finally dateHelperVCalendar.js is moved to the frontend, that we don't reuse it anymore where we don't need it.
It's best to review the commits one by one.
Superseeds #4317
According to comment #4317 (comment)
A simple way to reproduce it is to set the TZ variable in the e2e container, and generate the picasso there.
The frontend seems to work in this timezone, but i could not yet produce a test why.