Fix bug where sent in the last week email totals were always 0#2826
Fix bug where sent in the last week email totals were always 0#2826
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect “last week” email totals in service template statistics by ensuring billable-units aggregation queries include email notifications (not just SMS) and by defaulting nullable billable-unit sums to 0 for today’s notifications query.
Changes:
- Removed the
notification_type == SMS_TYPEfilter from billable-units aggregation queries so email data is included. - Wrapped
sum(Notification.billable_units)withcoalesce(..., 0)in today’s billable-units stats query.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stats_from_facts = ( | ||
| db.session.query( | ||
| FactNotificationStatus.notification_type.label("notification_type"), | ||
| FactNotificationStatus.notification_status.label("status"), | ||
| *([FactNotificationStatus.template_id.label("template_id")] if by_template else []), | ||
| *([func.sum(FactNotificationStatus.notification_count).label("count")]), | ||
| *([func.sum(FactNotificationStatus.billable_units).label("billable_units")]), | ||
| ) | ||
| .filter( | ||
| FactNotificationStatus.service_id == service_id, | ||
| FactNotificationStatus.bst_date >= start_time, | ||
| FactNotificationStatus.key_type != KEY_TYPE_TEST, | ||
| FactNotificationStatus.notification_type == SMS_TYPE, | ||
| ) |
There was a problem hiding this comment.
The behavior of this query changes to include non-SMS notification types. There are existing unit tests for fetch_notification_billable_units_for_service_for_today_and_7_previous_days, but they currently only cover SMS, so this change isn’t protected against regressions (eg email totals remaining missing/0). Add a test case that creates email (and/or letter) ft_notification_status rows and verifies they are returned/aggregated correctly.
| stats_for_today = ( | ||
| db.session.query( | ||
| Notification.notification_type.cast(db.Text), | ||
| Notification.status, | ||
| *([Notification.template_id] if by_template else []), | ||
| *([func.count().label("count")]), | ||
| *([func.sum(Notification.billable_units).label("billable_units")]), | ||
| *([func.coalesce(func.sum(Notification.billable_units), 0).label("billable_units")]), | ||
| ) | ||
| .filter( | ||
| Notification.created_at >= start_time, | ||
| Notification.service_id == service_id, | ||
| Notification.key_type != KEY_TYPE_TEST, | ||
| Notification.notification_type == SMS_TYPE, | ||
| ) |
There was a problem hiding this comment.
Now that today’s stats include email notifications, add test coverage that exercises the email path end-to-end (including that billable_units comes back as 0 for email rows). Current billable-units tests only create SMS notifications, so they won’t catch regressions in this new aggregation logic.
whabanks
left a comment
whabanks
left a comment
There was a problem hiding this comment.
LGTM, tested with FF_USE_BILLABLE_UNITS both on and off. in Admin/API. Email counts look good, no more 0's 🚀
Summary | Résumé
This PR:
notification_type == SMS_TYPEfrom both billable-units DAO functions so email data is now includedcoalesce(..., 0)aroundsum(Notification.billable_units)in_stats_for_today_with_billable_unitssinceNotification.billable_unitsis nullable for emailsRelated Issues | Cartes liées
https://docs.google.com/document/d/17_hHvjjaF1EBYtteGKX-Qzcg_rqsnvpsRMLaaxybbVc/edit?tab=t.0
Reviewer checklist | Liste de vérification du réviseur