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(dedicated): fix Problem with timezone on graph displays #14430

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

sepiropht
Copy link

@sepiropht sepiropht commented Dec 5, 2024

ref: UXCT-668

Question Answer
Branch? master
Bug fix? yes
New feature? no
Breaking change? no
Tickets Fix #UXCT-668
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated [n/a]
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits [n/a]

Description

Fix graph timezone

Related

@sepiropht sepiropht requested a review from a team as a code owner December 5, 2024 09:47
@github-actions github-actions bot added universe-bare-metal-cloud bug Something isn't working labels Dec 5, 2024
@sepiropht sepiropht force-pushed the fix/UXCT-668 branch 2 times, most recently from aeb38fa to b25c7f3 Compare December 10, 2024 13:44
Copy link
Contributor

@SimonChaumet SimonChaumet left a comment

Choose a reason for hiding this comment

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

name of commit/PR isn't understandable, could you specify what was fixed

after.setTime(this.dateTime);
after = new Date(this.dateTime);
after = new Date(
after.getTime() + after.getTimezoneOffset() * 4 * 60000,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend using date-fns to do this computation, it would be more understandable

You can also separate variables into constant

Personnaly I don't understand what 4 and 60 000 stand for

Copy link
Author

Choose a reason for hiding this comment

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

When accessing the traffic dashboard via URL with a dateTime parameter,
alerts were often not visible due to timezone misalignment.
i just added proper timezone offset handling to ensure alerts appear within the
displayed time window.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't understand what this 4 stands for

Copy link
Contributor

Choose a reason for hiding this comment

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

This part of code is outdate, if you look to the new version of code you could see a constant variable which explain what is this 4.

@qpavy qpavy changed the base branch from master to release/infra-hpc–w1 January 3, 2025 08:06
@qpavy qpavy merged commit f554cc1 into release/infra-hpc–w1 Jan 3, 2025
20 checks passed
@qpavy qpavy deleted the fix/UXCT-668 branch January 3, 2025 08:08
@qpavy qpavy restored the fix/UXCT-668 branch January 3, 2025 08:17
@qpavy qpavy mentioned this pull request Jan 3, 2025
1 task
@anooparveti anooparveti deleted the fix/UXCT-668 branch January 8, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants