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
Open
4 changes: 2 additions & 2 deletions .ds.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@
"filename": "tests/app/dao/test_services_dao.py",
"hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8",
"is_verified": false,
"line_number": 289,
"line_number": 290,
"is_secret": false
}
],
Expand Down Expand Up @@ -384,5 +384,5 @@
}
]
},
"generated_at": "2025-01-28T20:24:49Z"
"generated_at": "2025-02-06T14:48:51Z"
}
5 changes: 4 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ static-scan:
clean:
rm -rf node_modules cache target venv .coverage build tests/.cache ${CF_MANIFEST_PATH}


.PHONY: test-single
test-single: export NEW_RELIC_ENVIRONMENT=test
test-single: ## Run a single test file
poetry run pytest $(TEST_FILE)
## DEPLOYMENT

# .PHONY: cf-deploy-failwhale
Expand Down
23 changes: 23 additions & 0 deletions app/dao/date_util.py
Original file line number Diff line number Diff line change
@@ -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.


from app.utils import utc_now


Expand Down Expand Up @@ -93,3 +95,24 @@ def generate_date_range(start_date, end_date=None, days=0):
current_date += timedelta(days=1)
else:
return "An end_date or number of days must be specified"


def build_local_and_utc_date_range(
start_date_str: str,
days: int = 7,
timezone: str = "UTC"
):
"""
Convert date to local range based on timezone
"""

user_timezone = pytz.timezone(timezone)
local_end_date = datetime.strptime(start_date_str, "%Y-%m-%d").replace(tzinfo=user_timezone)
# Subtract (days - 1) so the entire final day is included in the range
local_start_date = local_end_date - timedelta(days=days)

# Convert to UTC for database queries
utc_start_date = local_start_date.astimezone(pytz.utc).replace(hour=0, minute=0, second=0)
utc_end_date = local_end_date.astimezone(pytz.utc).replace(hour=23, minute=59, second=59)

return (local_start_date, utc_start_date, utc_end_date)
63 changes: 41 additions & 22 deletions app/dao/services_dao.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import uuid
from datetime import timedelta
from datetime import datetime, timedelta

import pytz
from flask import current_app
from sqlalchemy import Float, cast, delete, select
from sqlalchemy.orm import joinedload
Expand Down Expand Up @@ -502,7 +503,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date):
select(
NotificationAllTimeView.notification_type,
NotificationAllTimeView.status,
func.date_trunc("day", NotificationAllTimeView.created_at).label("day"),
NotificationAllTimeView.created_at.label("timestamp"),
func.count(NotificationAllTimeView.id).label("count"),
)
.where(
Expand All @@ -514,7 +515,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date):
.group_by(
NotificationAllTimeView.notification_type,
NotificationAllTimeView.status,
func.date_trunc("day", NotificationAllTimeView.created_at),
NotificationAllTimeView.created_at,
)
)

Expand Down Expand Up @@ -563,7 +564,7 @@ def dao_fetch_stats_for_service_from_days_for_user(
select(
NotificationAllTimeView.notification_type,
NotificationAllTimeView.status,
func.date_trunc("day", NotificationAllTimeView.created_at).label("day"),
func.date_trunc("day", NotificationAllTimeView.created_at).label("timestamp"),
func.count(NotificationAllTimeView.id).label("count"),
)
.where(
Expand Down Expand Up @@ -799,31 +800,49 @@ def fetch_notification_stats_for_service_by_month_by_user(


def get_specific_days_stats(
data, start_date, days=None, end_date=None, total_notifications=None
data,
start_date,
days=None,
end_date=None,
timezone="UTC",
total_notifications=None
):
user_timezone = pytz.timezone(timezone or "UTC")

if days is not None and end_date is not None:
raise ValueError("Only set days OR set end_date, not both.")
elif days is not None:
gen_range = generate_date_range(start_date, days=days)
date_range = list(generate_date_range(start_date, days=days))
elif end_date is not None:
gen_range = generate_date_range(start_date, end_date)
date_range = list(generate_date_range(start_date, end_date=end_date))
else:
raise ValueError("Either days or end_date must be set.")
raise ValueError("Either 'days' or 'end_date' must be set.")

grouped_data = {date: [] for date in gen_range} | {
day: [row for row in data if row.day == day]
for day in {item.day for item in data}
}
# Ensure date_range is in correct format
grouped_data = {day.strftime("%Y-%m-%d"): [] for day in date_range}

stats = {
day.strftime("%Y-%m-%d"): statistics.format_statistics(
rows,
total_notifications=(
total_notifications.get(day, 0)
if total_notifications is not None
else None
),
for item in data:
if not isinstance(item.timestamp, datetime):
continue

local_datetime = item.timestamp.astimezone(user_timezone)
local_date_str = local_datetime.strftime("%Y-%m-%d")

if local_date_str in grouped_data:
grouped_data[local_date_str].append(item)

# Build final stats, optionally including total_notifications
stats = {}
for day in date_range:
formatted_day = day.strftime("%Y-%m-%d")
day_data = grouped_data.get(formatted_day, []) # Avoid KeyError
total_for_day = None
if total_notifications and formatted_day in total_notifications:
total_for_day = total_notifications[formatted_day]

stats[formatted_day] = statistics.format_statistics(
day_data,
total_notifications=total_for_day
)
for day, rows in grouped_data.items()
}

return stats
63 changes: 32 additions & 31 deletions app/service/rest.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import itertools
from datetime import datetime, timedelta
from datetime import datetime

from flask import Blueprint, current_app, jsonify, request
from sqlalchemy import select
Expand All @@ -19,7 +19,11 @@
save_model_api_key,
)
from app.dao.dao_utils import dao_rollback, transaction
from app.dao.date_util import get_calendar_year, get_month_start_and_end_date_in_utc
from app.dao.date_util import (
build_local_and_utc_date_range,
get_calendar_year,
get_month_start_and_end_date_in_utc,
)
from app.dao.fact_notification_status_dao import (
fetch_monthly_template_usage_for_service,
fetch_notification_status_for_service_by_month,
Expand Down Expand Up @@ -221,66 +225,63 @@ def get_service_notification_statistics(service_id):

@service_blueprint.route("/<uuid:service_id>/statistics/<string:start>/<int:days>")
def get_service_notification_statistics_by_day(service_id, start, days):
user_timezone = request.args.get("timezone", "UTC")

return jsonify(
data=get_service_statistics_for_specific_days(service_id, start, int(days))
data=get_service_statistics_for_specific_days(service_id, start, int(days), user_timezone)
)


def get_service_statistics_for_specific_days(service_id, start, days=1):
# start and end dates needs to be reversed because
# the end date is today and the start is x days in the past
# a day needs to be substracted to allow for today
end_date = datetime.strptime(start, "%Y-%m-%d")
start_date = end_date - timedelta(days=days - 1)
def get_service_statistics_for_specific_days(service_id, start, days=7, timezone="UTC"):
(
local_start_date,
utc_start_date,
utc_end_date
) = build_local_and_utc_date_range(start, days, timezone)

total_notifications, results = dao_fetch_stats_for_service_from_days(
service_id,
start_date,
end_date,
utc_start_date,
utc_end_date,
)

stats = get_specific_days_stats(
results,
start_date,
data=results,
start_date=local_start_date,
days=days,
total_notifications=total_notifications,
timezone=timezone
)

return stats


@service_blueprint.route(
"/<uuid:service_id>/statistics/user/<uuid:user_id>/<string:start>/<int:days>"
)
def get_service_notification_statistics_by_day_by_user(
service_id, user_id, start, days
):
def get_service_notification_statistics_by_day_by_user(service_id, user_id, start, days):
user_timezone = request.args.get('timezone', 'UTC')
return jsonify(
data=get_service_statistics_for_specific_days_by_user(
service_id, user_id, start, int(days)
service_id, user_id, start, int(days), timezone=user_timezone
)
)


def get_service_statistics_for_specific_days_by_user(
service_id, user_id, start, days=1
service_id, user_id, start, days=1, timezone="UTC"
):
# start and end dates needs to be reversed because
# the end date is today and the start is x days in the past
# a day needs to be substracted to allow for today
end_date = datetime.strptime(start, "%Y-%m-%d")
start_date = end_date - timedelta(days=days - 1)
(
local_start_date,
utc_start_date,
utc_end_date
) = build_local_and_utc_date_range(start_date_str=start, days=days, timezone=timezone)

total_notifications, results = dao_fetch_stats_for_service_from_days_for_user(
service_id, start_date, end_date, user_id
service_id, utc_start_date, utc_end_date, user_id
)

stats = get_specific_days_stats(
results,
start_date,
days=days,
total_notifications=total_notifications,
)
stats = get_specific_days_stats(results, local_start_date, days=days, total_notifications=total_notifications)

return stats


Expand Down
33 changes: 32 additions & 1 deletion tests/app/dao/test_date_utils.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
from datetime import date, datetime
from datetime import date, datetime, timedelta

import pytest
import pytz

from app.dao.date_util import (
build_local_and_utc_date_range,
get_calendar_year,
get_calendar_year_for_datetime,
get_month_start_and_end_date_in_utc,
Expand Down Expand Up @@ -75,3 +77,32 @@ def test_get_month_start_and_end_date_in_utc(month, year, expected_start, expect
)
def test_get_calendar_year_for_datetime(dt, fy):
assert get_calendar_year_for_datetime(dt) == fy


def test_build_local_and_utc_date_range():
local_start, utc_start, utc_end = build_local_and_utc_date_range(
"2025-02-04", 7, "America/New_York"
)
assert local_start.tzinfo
assert utc_start.tzinfo
assert utc_end.tzinfo
assert utc_end > utc_start


def test_build_local_and_utc_7_days_ny():
local_start, utc_start, utc_end = build_local_and_utc_date_range(
"2025-02-10", 7, "America/New_York"
)
diff = local_start + timedelta(days=7)
assert diff == datetime(2025, 2, 10, tzinfo=pytz.timezone("America/New_York"))
assert utc_start < utc_end


def test_build_local_and_utc_1_day_utc():
local_start, utc_start, utc_end = build_local_and_utc_date_range(
"2025-02-10", 1, "UTC"
)
# this should be one day before
assert local_start.isoformat() == "2025-02-09T00:00:00+00:00"
assert utc_start.hour == 0
assert utc_end.hour == 23
Loading