Removing specific filters for heartbeat notifications#2159
Draft
jimleroyer wants to merge 4 commits intomainfrom
Draft
Removing specific filters for heartbeat notifications#2159jimleroyer wants to merge 4 commits intomainfrom
jimleroyer wants to merge 4 commits intomainfrom
Conversation
jimleroyer
requested review from
P0NDER0SA,
ben851,
Copilot,
jzbahrai and
smcmurtry
Contributor
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/ |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not merge yet
We need to perform some data transformations prior to merge this so that numbers are reported the same before and after the change.
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.