This is an automated email from the ASF dual-hosted git repository. elizabeth pushed a commit to branch elizabeth/fix-slack-bug in repository https://gitbox.apache.org/repos/asf/superset.git
commit 133ce31957f5cd66cfd9209f574c23fc7e0a2188 Author: Elizabeth Thompson <[email protected]> AuthorDate: Fri Jul 26 15:57:31 2024 -0700 fix slack recipients --- scripts/tests/run.sh | 3 + superset/commands/report/execute.py | 17 ++--- tests/integration_tests/reports/commands_tests.py | 68 +++++++++++++++-- tests/unit_tests/commands/report/execute_test.py | 80 +++++++------------ .../reports/notifications/slack_tests.py | 89 +++++++--------------- 5 files changed, 127 insertions(+), 130 deletions(-) diff --git a/scripts/tests/run.sh b/scripts/tests/run.sh index bf8431caeb..7ba4c5e448 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 b14dfa0027..5d6126016a 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 import create_notification 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 @@ class BaseReportState: 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 @@ class BaseReportState: ) } ) - - 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 @@ class BaseReportState: 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 9aaebf11dd..6701bacaea 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -66,6 +66,7 @@ from superset.models.slice import Slice from superset.reports.models import ( ReportDataFormat, ReportExecutionLog, + ReportRecipientType, ReportSchedule, ReportScheduleType, ReportScheduleValidatorType, @@ -152,7 +153,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)") @@ -1110,6 +1113,63 @@ def test_email_dashboard_report_schedule_force_screenshot( assert_log(ReportState.SUCCESS) [email protected]( + "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" ) @@ -1129,11 +1189,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 index 33c2695df3..b7b545fd4a 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -15,6 +15,8 @@ # 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, @@ -24,9 +26,8 @@ from superset.reports.models import ( from superset.utils.core import HeaderDataType -def test_log_data_with_chart(mocker): - # Mocking the report schedule - mock_report_schedule = mocker.Mock(spec=ReportSchedule) +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 @@ -35,16 +36,13 @@ def test_log_data_with_chart(mocker): mock_report_schedule.owners = [1, 2] mock_report_schedule.recipients = [] - # Initializing the class and setting the report schedule - class_instance = BaseReportState( + class_instance: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" ) class_instance._report_schedule = mock_report_schedule - # Calling the method - result = class_instance._get_log_data() + result: HeaderDataType = class_instance._get_log_data() - # Expected result expected_result: HeaderDataType = { "notification_type": "report_type", "notification_source": ReportSourceFormat.CHART, @@ -55,13 +53,11 @@ def test_log_data_with_chart(mocker): "slack_channels": None, } - # Assertions assert result == expected_result -def test_log_data_with_dashboard(mocker): - # Mocking the report schedule - mock_report_schedule = mocker.Mock(spec=ReportSchedule) +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 @@ -70,16 +66,13 @@ def test_log_data_with_dashboard(mocker): mock_report_schedule.owners = [1, 2] mock_report_schedule.recipients = [] - # Initializing the class and setting the report schedule - class_instance = BaseReportState( + class_instance: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" ) class_instance._report_schedule = mock_report_schedule - # Calling the method - result = class_instance._get_log_data() + result: HeaderDataType = class_instance._get_log_data() - # Expected result expected_result: HeaderDataType = { "notification_type": "report_type", "notification_source": ReportSourceFormat.DASHBOARD, @@ -90,13 +83,11 @@ def test_log_data_with_dashboard(mocker): "slack_channels": None, } - # Assertions assert result == expected_result -def test_log_data_with_email_recipients(mocker): - # Mocking the report schedule - mock_report_schedule = mocker.Mock(spec=ReportSchedule) +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 @@ -109,16 +100,13 @@ def test_log_data_with_email_recipients(mocker): mocker.Mock(type=ReportRecipientType.EMAIL, recipient_config_json="email_2"), ] - # Initializing the class and setting the report schedule - class_instance = BaseReportState( + class_instance: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" ) class_instance._report_schedule = mock_report_schedule - # Calling the method - result = class_instance._get_log_data() + result: HeaderDataType = class_instance._get_log_data() - # Expected result expected_result: HeaderDataType = { "notification_type": "report_type", "notification_source": ReportSourceFormat.DASHBOARD, @@ -129,13 +117,11 @@ def test_log_data_with_email_recipients(mocker): "slack_channels": [], } - # Assertions assert result == expected_result -def test_log_data_with_slack_recipients(mocker): - # Mocking the report schedule - mock_report_schedule = mocker.Mock(spec=ReportSchedule) +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 @@ -148,16 +134,13 @@ def test_log_data_with_slack_recipients(mocker): mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_2"), ] - # Initializing the class and setting the report schedule - class_instance = BaseReportState( + class_instance: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" ) class_instance._report_schedule = mock_report_schedule - # Calling the method - result = class_instance._get_log_data() + result: HeaderDataType = class_instance._get_log_data() - # Expected result expected_result: HeaderDataType = { "notification_type": "report_type", "notification_source": ReportSourceFormat.DASHBOARD, @@ -168,13 +151,11 @@ def test_log_data_with_slack_recipients(mocker): "slack_channels": ["channel_1", "channel_2"], } - # Assertions assert result == expected_result -def test_log_data_no_owners(mocker): - # Mocking the report schedule - mock_report_schedule = mocker.Mock(spec=ReportSchedule) +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 @@ -186,16 +167,13 @@ def test_log_data_no_owners(mocker): mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_2"), ] - # Initializing the class and setting the report schedule - class_instance = BaseReportState( + class_instance: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" ) class_instance._report_schedule = mock_report_schedule - # Calling the method - result = class_instance._get_log_data() + result: HeaderDataType = class_instance._get_log_data() - # Expected result expected_result: HeaderDataType = { "notification_type": "report_type", "notification_source": ReportSourceFormat.DASHBOARD, @@ -206,13 +184,11 @@ def test_log_data_no_owners(mocker): "slack_channels": ["channel_1", "channel_2"], } - # Assertions assert result == expected_result -def test_log_data_with_missing_values(mocker): - # Mocking the report schedule - mock_report_schedule = mocker.Mock(spec=ReportSchedule) +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 @@ -226,16 +202,13 @@ def test_log_data_with_missing_values(mocker): ), ] - # Initializing the class and setting the report schedule - class_instance = BaseReportState( + class_instance: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" ) class_instance._report_schedule = mock_report_schedule - # Calling the method - result = class_instance._get_log_data() + result: HeaderDataType = class_instance._get_log_data() - # Expected result expected_result: HeaderDataType = { "notification_type": "report_type", "notification_source": ReportSourceFormat.DASHBOARD, @@ -246,5 +219,4 @@ def test_log_data_with_missing_values(mocker): "slack_channels": ["channel_1", "channel_2"], } - # Assertions assert result == expected_result diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index 83aa0d2b4d..b7f996631d 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -19,12 +19,27 @@ import uuid 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 + + [email protected] +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],
