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.

Sign up for GitHub

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

Jump to bottom

TimeDimensionDisplay DST day conversion fix #3819

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

TimeDimensionDisplay DST day conversion fix #3819

wants to merge 7 commits into from

Conversation

egor-ryashin
Copy link
Contributor

@egor-ryashin egor-ryashin commented Jan 10, 2024 •
edited

Checklist

  • Manual verification
  • Unit test coverage
  • E2E test coverage
  • Needs manual QA?

Summary

Issue addressed:

#3560 (comment)

Details:

Steps to Verify

  1. Open 311 Operational Metrics dashboard.
  2. Set first_day_of_week: 7
  3. In Chrome Developer Tools -> Sensors modify Location to America/New_York Timezone ID.
  4. Drill into Median Time to Resolve Ticket (Hours) from main dashboard view
  5. Adjust date range to Oct 29 - Nov 30 and make sure that metric trends is set to week
  6. Check that the column header in the TDD shows Nov 5 (Sun) as first day of week.
  7. Select time subrange ending with Nov 5 by dragging the mouse over the chart.
  8. Check the subrange control shows Nov 5.

image

djbarnwal
djbarnwal requested changes Jan 16, 2024
View reviewed changes
Copy link
Member

@djbarnwal djbarnwal left a comment

Choose a reason for hiding this comment

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

This PR breaks charting at multiple places

image

The charts only works if the selected timezone is same as the user's timezone.

web-common/src/features/dashboards/time-series/utils.ts
) {
return original?.map((originalPt, i) => {
const comparisonPt = comparison?.[i];

const ts = adjustOffsetForZone(originalPt.ts, zone);
Copy link
Member

Choose a reason for hiding this comment

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

For the charts, we need to have this offset. Javascript assumes the dates are in the user's timezone. We need to account for that across our charts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you elaborate what is broken in the screenshot?

@nishantmonu51 nishantmonu51 added Type:Bug Something isn't working blocker A release blocker issue that should be resolved before a new release labels Jan 23, 2024
@nishantmonu51 nishantmonu51 removed the blocker A release blocker issue that should be resolved before a new release label Jan 31, 2024
@AndrewRTsao
Copy link
Contributor

@egor-ryashin @djbarnwal @AdityaHegde Do we have an update on this bug fix? I believe it's still an outstanding issue.

@egor-ryashin
Copy link
Contributor Author

The issue is more complex than I expected @djbarnwal is going to take a look

@egor-ryashin
Copy link
Contributor Author

@djbarnwal I wonder if there're any updates?

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

@djbarnwal djbarnwal djbarnwal requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Type:Bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@egor-ryashin @AndrewRTsao @djbarnwal @nishantmonu51