Removing specific filters for heartbeat notifications - #2159
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the specific heartbeat notification filters from the statistics gathering and dashboard rendering functions, as heartbeat notifications are now handled by a dedicated service. Key changes include:
- Removing the filter_heartbeats parameter from get_latest_stats in app/utils.py.
- Updating get_stats_by_month in app/notify_client/service_api_client.py to omit heartbeat filtering.
- Adjusting view calls in app/main/views/index.py to stop passing the filter_heartbeats parameter.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/utils.py | Removed heartbeat filtering in the get_latest_stats function. |
| app/notify_client/service_api_client.py | Updated the API call by removing the filter_heartbeats parameter. |
| app/main/views/index.py | Removed heartbeat filtering when rendering activity and articles. |
Comments suppressed due to low confidence (1)
app/main/views/index.py:291
- Ensure tests for the activity view are updated to confirm consistent statistics reporting after the removal of heartbeat filtering.
return render_template("views/activity.html", **get_latest_stats(get_current_locale(current_app)))
🧪 Review environmenthttps://mlktfdsjdwblyugckmfxulvbje0jarkk.lambda-url.ca-central-1.on.aws/ |
🧪 Review environmenthttps://55rpeq6ctvieef3wltzavlrt2i0gqwue.lambda-url.ca-central-1.on.aws/ |
| emails_total += count | ||
|
|
||
| live_services = get_live_services_count() | ||
| live_services = len(service_api_client.get_live_services_data()["data"]) |
There was a problem hiding this comment.
@andrewleith Was this one an improvement that might have gone after that PR was made (many months ago)? Just want to have your eyes on that and what should be used.
|
|
||
| @cache.memoize(timeout=24 * 60 * 60) | ||
| def get_live_services_count(): | ||
| return len(service_api_client.get_live_services_data({"filter_heartbeats": True})["data"]) |
There was a problem hiding this comment.
Hmm I guess we could keep that one, especially for memoization, and let it be called in the other call site, but we need to remove filter_heartbeats. @andrewleith what do you prefer?
There was a problem hiding this comment.
Yeah I think we should keep the caching here - no need to hit the db often for these generalized statistics!
jimleroyer
left a comment
jimleroyer
left a comment
There was a problem hiding this comment.
LGTM - let's have @andrewleith review as well.
ben851
left a comment
ben851
left a comment
There was a problem hiding this comment.
LGTM, added the caching back
8a7a436 to
979bfbd
Compare
Summary | Résumé
Removing specific filters for heartbeat notifications. This won't be needed as we moved the heartbeat notifications into a dedicated service and after we rework the existing heartbeat in the database so they are associated with the new service.
Related task:
https://app.zenhub.com/workspaces/notify-planning-core-6411dfb7c95fb80014e0cab0/issues/gh/cds-snc/notification-planning-core/560
Test instructions | Instructions pour tester la modification
Test the dashboards that were affected with the filters, make sure these before and after the changes report the same numbers for previous periods of time.