-
Notifications
You must be signed in to change notification settings - Fork 18
Change the get notifications for a month to call a new summary table #2740
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
Conversation
smcmurtry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, let's see what the results in staging look like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR optimizes the retrieval of monthly notification statistics by switching from the large ft_notification_status table (28M+ rows) to a new pre-aggregated monthly_notification_stats_summary table. The new table stores monthly aggregates rather than daily data, significantly improving query performance.
Key Changes:
- Modified
fetch_delivered_notification_stats_by_month()to query the newMonthlyNotificationStatsSummarytable instead ofFactNotificationStatus - Updated test fixtures to use
create_monthly_notification_stats_summary()helper instead ofcreate_ft_notification_status() - Added new test helper function
create_monthly_notification_stats_summary()with flexible month parameter handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/dao/fact_notification_status_dao.py | Refactored query to use MonthlyNotificationStatsSummary table with ordering by month (desc) and notification type |
| tests/app/db.py | Added create_monthly_notification_stats_summary() helper function with date/string conversion logic |
| tests/app/dao/test_fact_notification_status_dao.py | Converted tests to use monthly summary data; organized into TestFetchDeliveredNotificationStatsbyMonth class |
| tests/app/service/test_rest.py | Simplified test fixtures to use monthly summary helper instead of creating templates and ft_notification_status records |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| results = fetch_delivered_notification_stats_by_month() | ||
|
|
||
| assert len(results) == 4 | ||
| class TestFetchDeliveredNotificationStatsbyMonth: |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class name has inconsistent capitalization: 'Statsby' should be 'StatsBy' to follow Python naming conventions.
| class TestFetchDeliveredNotificationStatsbyMonth: | |
| class TestFetchDeliveredNotificationStatsByMonth: |
| month_str = month | ||
| else: | ||
| # Convert date object to string format "YYYY-MM-01 00:00:00+00" | ||
| month_str = month.strftime("%Y-%m-01 00:00:00+00") |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The strftime format forces day to '01' regardless of the input date's day. If a date like date(2019, 12, 15) is passed, it will be converted to '2019-12-01 00:00:00+00'. While this may be intentional for normalizing to the first of the month, the function should either document this behavior clearly in the docstring or validate that the input date's day is 1.
| query = ( | ||
| db.session.query( |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out old implementation is extensive (lines 183-207) and makes the function harder to read. Since this is documented as the old approach, consider removing it or moving it to a separate documentation file, leaving only a reference to where the old implementation can be found if needed.
Summary | Résumé
Change the get notifications for a month to call a new summary table
This is because the summary table only stores data for a month, vs day (which is what ft_notification_status does).
This table is a lot faster to answer the question how many notifications have been send by notify.
DONOT MERGE, will test this after code freeze and then merge
We can assume atleast the data in the table is correct, as if I go and query production, I see
which is the same as

which currently calling ft_notification_status
(after this PR is merged, we will be using the new table)