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 b14dfa0027e0f..5d6126016ac7f 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 @@ -496,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/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 773073d4db0f8..b666a4833814f 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -66,6 +66,7 @@ from superset.reports.models import ( ReportDataFormat, ReportExecutionLog, + ReportRecipientType, ReportSchedule, ReportScheduleType, ReportScheduleValidatorType, @@ -166,7 +167,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)") @@ -1205,6 +1208,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" ) @@ -1224,11 +1284,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: @@ -1301,8 +1359,8 @@ def test_slack_chart_report_schedule_with_errors( ) error_logs = get_error_logs_query(create_report_slack_chart) - # check that we have two logs for each error - assert error_logs.count() == (len(slack_errors) + notification_logs_count) * 2 + # check that we have a log for each error + assert error_logs.count() == len(slack_errors) + notification_logs_count # check that each error has a message assert len([log.error_message for log in error_logs]) == error_logs.count() 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],