Skip to content

Conversation

@jimleroyer
Copy link
Member

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.

Copy link
Contributor

Copilot AI left a 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 removes heartbeat‐specific filtering from both the API endpoints and the corresponding DAO and test functions, as heartbeat notifications are now handled by a dedicated service. The key changes include:

  • Removal of the heartbeat filtering logic from REST endpoints (app/service/rest.py) and data access functions (app/dao/services_dao.py, app/dao/fact_notification_status_dao.py).
  • Deletion and refactoring of tests that previously validated heartbeat filtering behavior.
  • Cleanup of unused imports (e.g. set_config) in tests.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/app/service/test_rest.py Removed unused import and a test related to heartbeat filtering.
tests/app/dao/test_services_dao.py Deleted heartbeat filtering tests and updated the live services data test.
tests/app/dao/test_fact_notification_status_dao.py Removed the heartbeat filtering test for delivered notification stats.
app/service/rest.py Eliminated the extraction and passing of filter_heartbeats from the API endpoints.
app/dao/services_dao.py Updated function signature and logic by removing heartbeat filtering clauses.
app/dao/fact_notification_status_dao.py Removed heartbeat filtering logic within the delivered notification stats function.
Comments suppressed due to low confidence (4)

tests/app/service/test_rest.py:70

  • Removed unused import 'set_config'; please verify that no tests rely on this configuration helper.
from tests.conftest import set_config

tests/app/service/test_rest.py:252

  • [nitpick] The removal of the heartbeat filtering test requires confirming that the overall test coverage remains sufficient for delivered notification stats.
def test_get_delivered_notification_stats_by_month_data_without_heartbeat(notify_api, admin_request, sample_service):

app/service/rest.py:205

  • [nitpick] The removal of request argument parsing for 'filter_heartbeats' simplifies the endpoint; verify that the API clients have been updated to match this change.
def get_live_services_data():

app/dao/fact_notification_status_dao.py:176

  • Heartbeat filtering logic has been removed from delivered notification stats; please confirm that dependent systems are adjusted accordingly.
def fetch_delivered_notification_stats_by_month():



def dao_fetch_live_services_data(filter_heartbeats=None):
def dao_fetch_live_services_data():
Copy link

Copilot AI Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signature has been updated to remove the filter_heartbeats parameter; ensure that all consumers of this API no longer require heartbeat filtering.

Suggested change
def dao_fetch_live_services_data():
def dao_fetch_live_services_data():
# Ensure that the function no longer relies on the removed `filter_heartbeats` parameter.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants