diff --git a/pyproject.toml b/pyproject.toml index c6e0c16a21af2..d401eee49b234 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -230,6 +230,7 @@ module = "tests.*" check_untyped_defs = false disallow_untyped_calls = false disallow_untyped_defs = false +disable_error_code = "annotation-unchecked" [tool.tox] legacy_tox_ini = """ diff --git a/scripts/tests/run.sh b/scripts/tests/run.sh index bf8431caeb919..7ba4c5e448fee 100755 --- a/scripts/tests/run.sh +++ b/scripts/tests/run.sh @@ -53,6 +53,9 @@ function test_init() { echo Superset init echo -------------------- superset init + echo Load test users + echo -------------------- + superset load-test-users } # diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index bfeb2a678746a..eadf193bf4ec0 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -15,7 +15,6 @@ # specific language governing permissions and limitations # under the License. import logging -from copy import deepcopy from datetime import datetime, timedelta from typing import Any, Optional, Union from uuid import UUID @@ -67,6 +66,7 @@ from superset.reports.notifications.base import NotificationContent from superset.reports.notifications.exceptions import ( NotificationError, + NotificationParamException, SlackV1NotificationError, ) from superset.tasks.utils import get_executor @@ -132,15 +132,13 @@ def update_report_schedule_slack_v2(self) -> None: V2 uses ids instead of names for channels. """ try: - updated_recipients = [] for recipient in self._report_schedule.recipients: - recipient_copy = deepcopy(recipient) - if recipient_copy.type == ReportRecipientType.SLACK: - recipient_copy.type = ReportRecipientType.SLACKV2 - slack_recipients = json.loads(recipient_copy.recipient_config_json) + if recipient.type == ReportRecipientType.SLACK: + recipient.type = ReportRecipientType.SLACKV2 + slack_recipients = json.loads(recipient.recipient_config_json) # we need to ensure that existing reports can also fetch # ids from private channels - recipient_copy.recipient_config_json = json.dumps( + recipient.recipient_config_json = json.dumps( { "target": get_channels_with_search( slack_recipients["target"], @@ -151,9 +149,6 @@ def update_report_schedule_slack_v2(self) -> None: ) } ) - - updated_recipients.append(recipient_copy) - db.session.commit() # pylint: disable=consider-using-transaction except Exception as ex: logger.warning( "Failed to update slack recipients to v2: %s", str(ex), exc_info=True @@ -367,6 +362,7 @@ def _get_log_data(self) -> HeaderDataType: chart_id = None dashboard_id = None report_source = None + slack_channels = None if self._report_schedule.chart: report_source = ReportSourceFormat.CHART chart_id = self._report_schedule.chart_id @@ -374,6 +370,14 @@ def _get_log_data(self) -> HeaderDataType: report_source = ReportSourceFormat.DASHBOARD dashboard_id = self._report_schedule.dashboard_id + if self._report_schedule.recipients: + slack_channels = [ + recipient.recipient_config_json + for recipient in self._report_schedule.recipients + if recipient.type + in [ReportRecipientType.SLACK, ReportRecipientType.SLACKV2] + ] + log_data: HeaderDataType = { "notification_type": self._report_schedule.type, "notification_source": report_source, @@ -381,6 +385,7 @@ def _get_log_data(self) -> HeaderDataType: "chart_id": chart_id, "dashboard_id": dashboard_id, "owners": self._report_schedule.owners, + "slack_channels": slack_channels, } return log_data @@ -486,7 +491,7 @@ def _send( recipient.type = ReportRecipientType.SLACKV2 notification = create_notification(recipient, notification_content) notification.send() - except UpdateFailedError as err: + except (UpdateFailedError, NotificationParamException) as err: # log the error but keep processing the report with SlackV1 logger.warning( "Failed to update slack recipients to v2: %s", str(err) diff --git a/superset/utils/core.py b/superset/utils/core.py index 6a85521388f07..9480e5473330d 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -167,6 +167,7 @@ class HeaderDataType(TypedDict): notification_source: str | None chart_id: int | None dashboard_id: int | None + slack_channels: list[str] | None class DatasourceDict(TypedDict): diff --git a/tests/integration_tests/reports/commands/execute_dashboard_report_tests.py b/tests/integration_tests/reports/commands/execute_dashboard_report_tests.py index 82021a5468eff..318fd4977b16a 100644 --- a/tests/integration_tests/reports/commands/execute_dashboard_report_tests.py +++ b/tests/integration_tests/reports/commands/execute_dashboard_report_tests.py @@ -112,4 +112,4 @@ def test_report_with_header_data( assert header_data.get("notification_format") == report_schedule.report_format assert header_data.get("notification_source") == ReportSourceFormat.DASHBOARD assert header_data.get("notification_type") == report_schedule.type - assert len(send_email_smtp_mock.call_args.kwargs["header_data"]) == 6 + assert len(send_email_smtp_mock.call_args.kwargs["header_data"]) == 7 diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 74268d8be3411..575c8a02b9d46 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -67,6 +67,7 @@ from superset.reports.models import ( ReportDataFormat, ReportExecutionLog, + ReportRecipientType, ReportSchedule, ReportScheduleType, ReportScheduleValidatorType, @@ -171,7 +172,9 @@ def assert_log(state: str, error_message: Optional[str] = None): @contextmanager def create_test_table_context(database: Database): with database.get_sqla_engine() as engine: - engine.execute("CREATE TABLE test_table AS SELECT 1 as first, 2 as second") + engine.execute( + "CREATE TABLE IF NOT EXISTS test_table AS SELECT 1 as first, 2 as second" + ) engine.execute("INSERT INTO test_table (first, second) VALUES (1, 2)") engine.execute("INSERT INTO test_table (first, second) VALUES (3, 4)") @@ -1297,6 +1300,63 @@ def test_email_dashboard_report_schedule_force_screenshot( assert_log(ReportState.SUCCESS) +@pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_report_slack_chart" +) +@patch("superset.commands.report.execute.get_channels_with_search") +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=True) +@patch("superset.reports.notifications.slackv2.get_slack_client") +@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") +def test_slack_chart_report_schedule_converts_to_v2( + screenshot_mock, + slack_client_mock, + slack_should_use_v2_api_mock, + get_channels_with_search_mock, + create_report_slack_chart, +): + """ + ExecuteReport Command: Test chart slack report schedule + """ + # setup screenshot mock + screenshot_mock.return_value = SCREENSHOT_FILE + + channel_id = "slack_channel_id" + + get_channels_with_search_mock.return_value = channel_id + + with freeze_time("2020-01-01T00:00:00Z"): + with patch.object(current_app.config["STATS_LOGGER"], "gauge") as statsd_mock: + AsyncExecuteReportScheduleCommand( + TEST_ID, create_report_slack_chart.id, datetime.utcnow() + ).run() + + assert ( + slack_client_mock.return_value.files_upload_v2.call_args[1]["channel"] + == channel_id + ) + assert ( + slack_client_mock.return_value.files_upload_v2.call_args[1]["file"] + == SCREENSHOT_FILE + ) + + # Assert that the report recipients were updated + assert create_report_slack_chart.recipients[ + 0 + ].recipient_config_json == json.dumps({"target": channel_id}) + assert ( + create_report_slack_chart.recipients[0].type + == ReportRecipientType.SLACKV2 + ) + + # Assert logs are correct + assert_log(ReportState.SUCCESS) + # this will send a warning + assert statsd_mock.call_args_list[0] == call( + "reports.slack.send.warning", 1 + ) + assert statsd_mock.call_args_list[1] == call("reports.slack.send.ok", 1) + + @pytest.mark.usefixtures( "load_birth_names_dashboard_with_slices", "create_report_slack_chartv2" ) @@ -1316,11 +1376,9 @@ def test_slack_chart_report_schedule_v2( """ # setup screenshot mock screenshot_mock.return_value = SCREENSHOT_FILE - notification_targets = get_target_from_report_schedule(create_report_slack_chart) - - channel_id = notification_targets[0] + channel_id = "slack_channel_id" - get_channels_with_search_mock.return_value = {} + get_channels_with_search_mock.return_value = channel_id with freeze_time("2020-01-01T00:00:00Z"): with patch.object(current_app.config["STATS_LOGGER"], "gauge") as statsd_mock: diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py new file mode 100644 index 0000000000000..b7b545fd4a6e5 --- /dev/null +++ b/tests/unit_tests/commands/report/execute_test.py @@ -0,0 +1,222 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from pytest_mock import MockerFixture + +from superset.commands.report.execute import BaseReportState +from superset.reports.models import ( + ReportRecipientType, + ReportSchedule, + ReportSourceFormat, +) +from superset.utils.core import HeaderDataType + + +def test_log_data_with_chart(mocker: MockerFixture) -> None: + mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) + mock_report_schedule.chart = True + mock_report_schedule.chart_id = 123 + mock_report_schedule.dashboard_id = None + mock_report_schedule.type = "report_type" + mock_report_schedule.report_format = "report_format" + mock_report_schedule.owners = [1, 2] + mock_report_schedule.recipients = [] + + class_instance: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + class_instance._report_schedule = mock_report_schedule + + result: HeaderDataType = class_instance._get_log_data() + + expected_result: HeaderDataType = { + "notification_type": "report_type", + "notification_source": ReportSourceFormat.CHART, + "notification_format": "report_format", + "chart_id": 123, + "dashboard_id": None, + "owners": [1, 2], + "slack_channels": None, + } + + assert result == expected_result + + +def test_log_data_with_dashboard(mocker: MockerFixture) -> None: + mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) + mock_report_schedule.chart = False + mock_report_schedule.chart_id = None + mock_report_schedule.dashboard_id = 123 + mock_report_schedule.type = "report_type" + mock_report_schedule.report_format = "report_format" + mock_report_schedule.owners = [1, 2] + mock_report_schedule.recipients = [] + + class_instance: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + class_instance._report_schedule = mock_report_schedule + + result: HeaderDataType = class_instance._get_log_data() + + expected_result: HeaderDataType = { + "notification_type": "report_type", + "notification_source": ReportSourceFormat.DASHBOARD, + "notification_format": "report_format", + "chart_id": None, + "dashboard_id": 123, + "owners": [1, 2], + "slack_channels": None, + } + + assert result == expected_result + + +def test_log_data_with_email_recipients(mocker: MockerFixture) -> None: + mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) + mock_report_schedule.chart = False + mock_report_schedule.chart_id = None + mock_report_schedule.dashboard_id = 123 + mock_report_schedule.type = "report_type" + mock_report_schedule.report_format = "report_format" + mock_report_schedule.owners = [1, 2] + mock_report_schedule.recipients = [] + mock_report_schedule.recipients = [ + mocker.Mock(type=ReportRecipientType.EMAIL, recipient_config_json="email_1"), + mocker.Mock(type=ReportRecipientType.EMAIL, recipient_config_json="email_2"), + ] + + class_instance: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + class_instance._report_schedule = mock_report_schedule + + result: HeaderDataType = class_instance._get_log_data() + + expected_result: HeaderDataType = { + "notification_type": "report_type", + "notification_source": ReportSourceFormat.DASHBOARD, + "notification_format": "report_format", + "chart_id": None, + "dashboard_id": 123, + "owners": [1, 2], + "slack_channels": [], + } + + assert result == expected_result + + +def test_log_data_with_slack_recipients(mocker: MockerFixture) -> None: + mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) + mock_report_schedule.chart = False + mock_report_schedule.chart_id = None + mock_report_schedule.dashboard_id = 123 + mock_report_schedule.type = "report_type" + mock_report_schedule.report_format = "report_format" + mock_report_schedule.owners = [1, 2] + mock_report_schedule.recipients = [] + mock_report_schedule.recipients = [ + mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_1"), + mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_2"), + ] + + class_instance: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + class_instance._report_schedule = mock_report_schedule + + result: HeaderDataType = class_instance._get_log_data() + + expected_result: HeaderDataType = { + "notification_type": "report_type", + "notification_source": ReportSourceFormat.DASHBOARD, + "notification_format": "report_format", + "chart_id": None, + "dashboard_id": 123, + "owners": [1, 2], + "slack_channels": ["channel_1", "channel_2"], + } + + assert result == expected_result + + +def test_log_data_no_owners(mocker: MockerFixture) -> None: + mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) + mock_report_schedule.chart = False + mock_report_schedule.chart_id = None + mock_report_schedule.dashboard_id = 123 + mock_report_schedule.type = "report_type" + mock_report_schedule.report_format = "report_format" + mock_report_schedule.owners = [] + mock_report_schedule.recipients = [ + mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_1"), + mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_2"), + ] + + class_instance: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + class_instance._report_schedule = mock_report_schedule + + result: HeaderDataType = class_instance._get_log_data() + + expected_result: HeaderDataType = { + "notification_type": "report_type", + "notification_source": ReportSourceFormat.DASHBOARD, + "notification_format": "report_format", + "chart_id": None, + "dashboard_id": 123, + "owners": [], + "slack_channels": ["channel_1", "channel_2"], + } + + assert result == expected_result + + +def test_log_data_with_missing_values(mocker: MockerFixture) -> None: + mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) + mock_report_schedule.chart = None + mock_report_schedule.chart_id = None + mock_report_schedule.dashboard_id = None + mock_report_schedule.type = "report_type" + mock_report_schedule.report_format = "report_format" + mock_report_schedule.owners = [1, 2] + mock_report_schedule.recipients = [ + mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_1"), + mocker.Mock( + type=ReportRecipientType.SLACKV2, recipient_config_json="channel_2" + ), + ] + + class_instance: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + class_instance._report_schedule = mock_report_schedule + + result: HeaderDataType = class_instance._get_log_data() + + expected_result: HeaderDataType = { + "notification_type": "report_type", + "notification_source": ReportSourceFormat.DASHBOARD, + "notification_format": "report_format", + "chart_id": None, + "dashboard_id": None, + "owners": [1, 2], + "slack_channels": ["channel_1", "channel_2"], + } + + assert result == expected_result diff --git a/tests/unit_tests/reports/notifications/email_tests.py b/tests/unit_tests/reports/notifications/email_tests.py index 697a9bac40c86..ab3fa8c5afc66 100644 --- a/tests/unit_tests/reports/notifications/email_tests.py +++ b/tests/unit_tests/reports/notifications/email_tests.py @@ -41,6 +41,7 @@ def test_render_description_with_html() -> None: "notification_source": None, "chart_id": None, "dashboard_id": None, + "slack_channels": None, }, ) email_body = ( diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index 83aa0d2b4d625..b7f996631d1a2 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -19,12 +19,27 @@ from unittest.mock import MagicMock, patch import pandas as pd +import pytest from slack_sdk.errors import SlackApiError from superset.reports.notifications.slackv2 import SlackV2Notification +from superset.utils.core import HeaderDataType + + +@pytest.fixture +def mock_header_data() -> HeaderDataType: + return { + "notification_format": "PNG", + "notification_type": "Alert", + "owners": [1], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["some_channel"], + } -def test_get_channel_with_multi_recipients() -> None: +def test_get_channel_with_multi_recipients(mock_header_data) -> None: """ Test the _get_channel function to ensure it will return a string with recipients separated by commas without interstitial spacing @@ -35,14 +50,7 @@ def test_get_channel_with_multi_recipients() -> None: content = NotificationContent( name="test alert", - header_data={ - "notification_format": "PNG", - "notification_type": "Alert", - "owners": [1], - "notification_source": None, - "chart_id": None, - "dashboard_id": None, - }, + header_data=mock_header_data, embedded_data=pd.DataFrame( { "A": [1, 2, 3], @@ -67,7 +75,7 @@ def test_get_channel_with_multi_recipients() -> None: # Test if the recipient configuration JSON is valid when using a SlackV2 recipient type -def test_valid_recipient_config_json_slackv2() -> None: +def test_valid_recipient_config_json_slackv2(mock_header_data) -> None: """ Test if the recipient configuration JSON is valid when using a SlackV2 recipient type """ @@ -77,14 +85,7 @@ def test_valid_recipient_config_json_slackv2() -> None: content = NotificationContent( name="test alert", - header_data={ - "notification_format": "PNG", - "notification_type": "Alert", - "owners": [1], - "notification_source": None, - "chart_id": None, - "dashboard_id": None, - }, + header_data=mock_header_data, embedded_data=pd.DataFrame( { "A": [1, 2, 3], @@ -109,7 +110,7 @@ def test_valid_recipient_config_json_slackv2() -> None: # Ensure _get_inline_files function returns the correct tuple when content has screenshots -def test_get_inline_files_with_screenshots() -> None: +def test_get_inline_files_with_screenshots(mock_header_data) -> None: """ Test the _get_inline_files function to ensure it will return the correct tuple when content has screenshots @@ -120,14 +121,7 @@ def test_get_inline_files_with_screenshots() -> None: content = NotificationContent( name="test alert", - header_data={ - "notification_format": "PNG", - "notification_type": "Alert", - "owners": [1], - "notification_source": None, - "chart_id": None, - "dashboard_id": None, - }, + header_data=mock_header_data, embedded_data=pd.DataFrame( { "A": [1, 2, 3], @@ -153,7 +147,7 @@ def test_get_inline_files_with_screenshots() -> None: # Ensure _get_inline_files function returns None when content has no screenshots or csv -def test_get_inline_files_with_no_screenshots_or_csv() -> None: +def test_get_inline_files_with_no_screenshots_or_csv(mock_header_data) -> None: """ Test the _get_inline_files function to ensure it will return None when content has no screenshots or csv @@ -164,14 +158,7 @@ def test_get_inline_files_with_no_screenshots_or_csv() -> None: content = NotificationContent( name="test alert", - header_data={ - "notification_format": "PNG", - "notification_type": "Alert", - "owners": [1], - "notification_source": None, - "chart_id": None, - "dashboard_id": None, - }, + header_data=mock_header_data, embedded_data=pd.DataFrame( { "A": [1, 2, 3], @@ -201,6 +188,7 @@ def test_send_slackv2( slack_client_mock: MagicMock, logger_mock: MagicMock, flask_global_mock: MagicMock, + mock_header_data, ) -> None: # `superset.models.helpers`, a dependency of following imports, # requires app context @@ -212,14 +200,7 @@ def test_send_slackv2( slack_client_mock.return_value.chat_postMessage.return_value = {"ok": True} content = NotificationContent( name="test alert", - header_data={ - "notification_format": "PNG", - "notification_type": "Alert", - "owners": [1], - "notification_source": None, - "chart_id": None, - "dashboard_id": None, - }, + header_data=mock_header_data, embedded_data=pd.DataFrame( { "A": [1, 2, 3], @@ -269,6 +250,7 @@ def test_send_slack( slack_client_mock_util: MagicMock, logger_mock: MagicMock, flask_global_mock: MagicMock, + mock_header_data, ) -> None: # `superset.models.helpers`, a dependency of following imports, # requires app context @@ -285,14 +267,7 @@ def test_send_slack( content = NotificationContent( name="test alert", - header_data={ - "notification_format": "PNG", - "notification_type": "Alert", - "owners": [1], - "notification_source": None, - "chart_id": None, - "dashboard_id": None, - }, + header_data=mock_header_data, embedded_data=pd.DataFrame( { "A": [1, 2, 3], @@ -343,6 +318,7 @@ def test_send_slack_no_feature_flag( slack_client_mock_util: MagicMock, logger_mock: MagicMock, flask_global_mock: MagicMock, + mock_header_data, ) -> None: # `superset.models.helpers`, a dependency of following imports, # requires app context @@ -360,14 +336,7 @@ def test_send_slack_no_feature_flag( content = NotificationContent( name="test alert", - header_data={ - "notification_format": "PNG", - "notification_type": "Alert", - "owners": [1], - "notification_source": None, - "chart_id": None, - "dashboard_id": None, - }, + header_data=mock_header_data, embedded_data=pd.DataFrame( { "A": [1, 2, 3],