Skip to content

Add Google Chat notification handler; Allow more than one handler to be used at a time; Fix MS teams notification test; Fix failing GitHub build; Prevent a job from failing when coverage step fails; - #108

Merged
Kosma Grochowski (kosmag) merged 3 commits into
getindata:developfrom
kosmag:feat/add-google-chat-notification-handler
Dec 1, 2025

Conversation

@kosmag

@kosmag Kosma Grochowski (kosmag) commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

Add feature - Google Chat notification handler
Resolve bug - allow more than one notification handler to be used at a time
Resolve bug - fix MS Teams notification test
Resolve bug - fix failing build pipeline, prevent pipeline from failing when a coverage step fails

@kosmag
Kosma Grochowski (kosmag) force-pushed the feat/add-google-chat-notification-handler branch 11 times, most recently from 54d7383 to 60fcc89 Compare November 26, 2025 12:40
…be used at a time; Fix MS teams notification test; Fix failing GitHub build; Prevent a job from failing when coverage step fails;
@kosmag
Kosma Grochowski (kosmag) force-pushed the feat/add-google-chat-notification-handler branch from 60fcc89 to 72ecc2b Compare November 26, 2025 12:48
@kosmag Kosma Grochowski (kosmag) changed the title Add Google Chat notification handler; Allow more than one handler to be used at a time; Fix MS Teams notification test; Add Google Chat notification handler; Allow more than one handler to be used at a time; Fix MS teams notification test; Fix failing GitHub build; Prevent a job from failing when coverage step fails; Nov 26, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTMT

@dgniewek

Copy link
Copy Markdown
Collaborator

Code review

Found 3 issues:

  1. Inconsistent template variable naming - The Google Chat handler uses different variable names ({task_id}, {dag_id}, {execution_date}, {log_url}) compared to Slack and MS Teams handlers ({task}, {dag}, {execution_time}, {url}). Since this PR enables multiple handlers to run simultaneously, users will need to maintain different template formats for different notification types.

message_text = handler_definition["message_template"].format(
task_id=task_id,
dag_id=dag_id,
execution_date=context.get("execution_date"),
log_url=logs_url,
)

Compare with Slack handler:

message=handler_definition["message_template"].format(
task=context.get("task_instance").task_id,
dag=context.get("task_instance").dag_id,
execution_time=context.get("execution_date"),
url=context.get("task_instance").log_url,
),

  1. Google Chat test has inadequate verification - The test only verifies that HttpHook was instantiated (mock_http_hook.assert_called_once()), but doesn't verify that the .run() method was called with the correct data, headers, or connection ID. This provides significantly less coverage than the Slack and MS Teams tests.

factory.create_failure_handler(notifications_config)(context)
# then
mock_http_hook.assert_called_once()

Compare with MS Teams test pattern (lines 87-107) which properly verifies the webhook call details.

  1. Test doesn't verify HttpHook.run() was actually called - The mock is on the HttpHook class itself, not the .run() method. The test would pass even if the code was changed to just instantiate HttpHook without calling .run(). Consider mocking HttpHook.run and verifying it was called with the expected JSON data and headers.

@patch("dbt_airflow_factory.notifications.handler.HttpHook")
def test_notification_send_for_google_chat(mock_http_hook, mock_get_connection):
# given
notifications_config = AirflowDagFactory(
path.dirname(path.abspath(__file__)), "notifications_google_chat"
).airflow_config["failure_handlers"]
factory = NotificationHandlersFactory()
context = create_context()
mock_get_connection.return_value = create_google_chat_connection()
# when
factory.create_failure_handler(notifications_config)(context)
# then
mock_http_hook.assert_called_once()

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@kosmag
Kosma Grochowski (kosmag) force-pushed the feat/add-google-chat-notification-handler branch from c685414 to 9dcd565 Compare November 27, 2025 10:28
@kosmag

Copy link
Copy Markdown
Contributor Author

Dominik Gniewek-Węgrzyn (@dgniewek) All fixed

@dgniewek

Copy link
Copy Markdown
Collaborator

Updated Code Review

Thanks for addressing the feedback! All 3 issues have been successfully fixed:

Template variable naming consistency - Now uses {task}, {dag}, {execution_time}, {url} matching Slack/MS Teams
Test properly mocks HttpHook.run - Using @patch on the .run() method directly
Test verifies the webhook call - Includes mock_run.assert_called_once() and validates payload

All requested changes have been addressed. The implementation looks good!

🤖 Generated with Claude Code

@kosmag
Kosma Grochowski (kosmag) merged commit 3b63b83 into getindata:develop Dec 1, 2025
1 check passed
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