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

Issue 2204 - inaccurate message stats due to nonexistant UTC to local changes #1547

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

Conversation

alexjanousekGSA
Copy link
Contributor

@alexjanousekGSA alexjanousekGSA commented Jan 30, 2025

This is for issue: #2204
Related to PR - 2314

Changes

  • Added support for passing a timezone parameter to /service/{id}/statistics/{start}/{days} to properly adjust dates when querying stats.
  • Default behavior remains the same (UTC) if no timezone is provided.
  • Refactored date calculations for better readability and accuracy, removing the old >= 19 check.
  • Updated methods to properly convert timestamps and return data in the requested timezone.
  • Added and improved test coverage, including edge cases for daylight savings, timezones and large data queries.

@alexjanousekGSA alexjanousekGSA self-assigned this Jan 30, 2025
@alexjanousekGSA alexjanousekGSA changed the title Issue 2204/inaccurate message stats Issue 2204 - inaccurate message stats due to nonexistant UTC to local changes Jan 30, 2025
@@ -1,6 +1,8 @@
import calendar
from datetime import date, datetime, time, timedelta

import pytz
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be using pytz. It alternates between being stable and generating flaky weirdness, depending on the upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

we have removed pytz in api when making changes so this would be putting it back.

Copy link
Contributor

@terrazoon terrazoon left a comment

Choose a reason for hiding this comment

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

I don't think there should be any changes on the API side. It is the job of the front end to pass UTC time to the backend and convert the backend's UTC time to whatever it needs on the front end.

We do not want to do time conversions on the back end because it adds a lot of complexity and generates defects. DB time is always UTC, backend expects to receive UTC, backend sends UTC. Front end does all conversions because only front end knows the local time, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress (WIP: ≤ 3 per person)
Development

Successfully merging this pull request may close these issues.

2 participants