diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 0c57d55bb59..2e94b8c4b80 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -72,7 +72,7 @@ from superset.reports.notifications.exceptions import ( ) from superset.tasks.utils import get_executor from superset.utils import json -from superset.utils.core import HeaderDataType, override_user +from superset.utils.core import HeaderDataType, override_user, recipients_string_to_list from superset.utils.csv import get_chart_csv_data, get_chart_dataframe from superset.utils.decorators import logs_context, transaction from superset.utils.pdf import build_pdf_from_screenshots @@ -137,24 +137,40 @@ class BaseReportState: if recipient.type == ReportRecipientType.SLACK: recipient.type = ReportRecipientType.SLACKV2 slack_recipients = json.loads(recipient.recipient_config_json) + # V1 method allowed to use leading `#` in the channel name + channel_names = (slack_recipients["target"] or "").replace("#", "") # we need to ensure that existing reports can also fetch # ids from private channels + channels = get_channels_with_search( + search_string=channel_names, + types=[ + SlackChannelTypes.PRIVATE, + SlackChannelTypes.PUBLIC, + ], + exact_match=True, + ) + channels_list = recipients_string_to_list(channel_names) + if len(channels_list) != len(channels): + missing_channels = set(channels_list) - { + channel["name"] for channel in channels + } + msg = ( + "Could not find the following channels: " + f"{', '.join(missing_channels)}" + ) + raise UpdateFailedError(msg) + channel_ids = ",".join(channel["id"] for channel in channels) recipient.recipient_config_json = json.dumps( { - "target": get_channels_with_search( - slack_recipients["target"], - types=[ - SlackChannelTypes.PRIVATE, - SlackChannelTypes.PUBLIC, - ], - ) + "target": channel_ids, } ) except Exception as ex: - logger.warning( - "Failed to update slack recipients to v2: %s", str(ex), exc_info=True - ) - raise UpdateFailedError from ex + # Revert to v1 to preserve configuration (requires manual fix) + recipient.type = ReportRecipientType.SLACK + msg = f"Failed to update slack recipients to v2: {str(ex)}" + logger.exception(msg) + raise UpdateFailedError(msg) from ex def create_log(self, error_message: Optional[str] = None) -> None: """ @@ -553,30 +569,32 @@ class BaseReportState: for recipient in recipients: notification = create_notification(recipient, notification_content) try: - if app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"]: - logger.info( - "Would send notification for alert %s, to %s. " - "ALERT_REPORTS_NOTIFICATION_DRY_RUN is enabled, " - "set it to False to send notifications.", - self._report_schedule.name, - recipient.recipient_config_json, - ) - else: - notification.send() - except SlackV1NotificationError as ex: - # The slack notification should be sent with the v2 api - logger.info("Attempting to upgrade the report to Slackv2: %s", str(ex)) try: + if app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"]: + logger.info( + "Would send notification for alert %s, to %s. " + "ALERT_REPORTS_NOTIFICATION_DRY_RUN is enabled, " + "set it to False to send notifications.", + self._report_schedule.name, + recipient.recipient_config_json, + ) + else: + notification.send() + except SlackV1NotificationError as ex: + # The slack notification should be sent with the v2 api + logger.info( + "Attempting to upgrade the report to Slackv2: %s", str(ex) + ) self.update_report_schedule_slack_v2() recipient.type = ReportRecipientType.SLACKV2 notification = create_notification(recipient, notification_content) notification.send() - 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) - ) - except (NotificationError, SupersetException) as ex: + except ( + UpdateFailedError, + NotificationParamException, + NotificationError, + SupersetException, + ) as ex: # collect errors but keep processing them notification_errors.append( SupersetError( diff --git a/superset/reports/notifications/slack.py b/superset/reports/notifications/slack.py index 4622a456069..589fddb9aad 100644 --- a/superset/reports/notifications/slack.py +++ b/superset/reports/notifications/slack.py @@ -43,7 +43,7 @@ from superset.reports.notifications.exceptions import ( ) from superset.reports.notifications.slack_mixin import SlackMixin from superset.utils import json -from superset.utils.core import get_email_address_list +from superset.utils.core import recipients_string_to_list from superset.utils.decorators import statsd_gauge from superset.utils.slack import ( get_slack_client, @@ -70,7 +70,7 @@ class SlackNotification(SlackMixin, BaseNotification): # pylint: disable=too-fe """ recipient_str = json.loads(self._recipient.recipient_config_json)["target"] - return ",".join(get_email_address_list(recipient_str)) + return ",".join(recipients_string_to_list(recipient_str)) def _get_inline_files( self, diff --git a/superset/reports/notifications/slackv2.py b/superset/reports/notifications/slackv2.py index 824d4bd3265..19b5d98e00c 100644 --- a/superset/reports/notifications/slackv2.py +++ b/superset/reports/notifications/slackv2.py @@ -42,7 +42,7 @@ from superset.reports.notifications.exceptions import ( ) from superset.reports.notifications.slack_mixin import SlackMixin from superset.utils import json -from superset.utils.core import get_email_address_list +from superset.utils.core import recipients_string_to_list from superset.utils.decorators import statsd_gauge from superset.utils.slack import get_slack_client @@ -64,7 +64,7 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too- """ # noqa: E501 recipient_str = json.loads(self._recipient.recipient_config_json)["target"] - return get_email_address_list(recipient_str) + return recipients_string_to_list(recipient_str) def _get_inline_files( self, diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 7078970b381..cfccc579bc0 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -19,7 +19,7 @@ from typing import Any, Optional, Union from croniter import croniter from flask import current_app from flask_babel import gettext as _ -from marshmallow import fields, Schema, validate, validates, validates_schema +from marshmallow import EXCLUDE, fields, Schema, validate, validates, validates_schema from marshmallow.validate import Length, Range, ValidationError from pytz import all_timezones @@ -399,3 +399,17 @@ class ReportSchedulePutSchema(Schema): max=max_width, ) ) + + +class SlackChannelSchema(Schema): + """ + Schema to load Slack channels, set to ignore any fields not used by Superset. + """ + + class Meta: + unknown = EXCLUDE + + id = fields.String() + name = fields.String() + is_member = fields.Boolean() + is_private = fields.Boolean() diff --git a/superset/utils/core.py b/superset/utils/core.py index 2aaf9122297..9e1bee5ecb5 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -702,7 +702,7 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many 'test@example.com', 'foo', 'Foo bar',['/dev/null'], dryrun=True) """ smtp_mail_from = config["SMTP_MAIL_FROM"] - smtp_mail_to = get_email_address_list(to) + smtp_mail_to = recipients_string_to_list(to) msg = MIMEMultipart(mime_subtype) msg["Subject"] = subject @@ -713,14 +713,14 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many recipients = smtp_mail_to if cc: - smtp_mail_cc = get_email_address_list(cc) + smtp_mail_cc = recipients_string_to_list(cc) msg["Cc"] = ", ".join(smtp_mail_cc) recipients = recipients + smtp_mail_cc smtp_mail_bcc = [] if bcc: # don't add bcc in header - smtp_mail_bcc = get_email_address_list(bcc) + smtp_mail_bcc = recipients_string_to_list(bcc) recipients = recipients + smtp_mail_bcc msg["Date"] = formatdate(localtime=True) @@ -813,7 +813,13 @@ def send_mime_email( smtp.quit() -def get_email_address_list(address_string: str) -> list[str]: +def recipients_string_to_list(address_string: str | None) -> list[str]: + """ + Returns the list of target recipients for alerts and reports. + + Strips values and converts a comma/semicolon separated + string into a list. + """ address_string_list: list[str] = [] if isinstance(address_string, str): address_string_list = re.split(r",|\s|;", address_string) diff --git a/superset/utils/slack.py b/superset/utils/slack.py index b4c8018ffc7..6d51f2765ea 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -26,7 +26,9 @@ from slack_sdk.http_retry.builtin_handlers import RateLimitErrorRetryHandler from superset import feature_flag_manager from superset.exceptions import SupersetException +from superset.reports.schemas import SlackChannelSchema from superset.utils.backports import StrEnum +from superset.utils.core import recipients_string_to_list logger = logging.getLogger(__name__) @@ -57,7 +59,7 @@ def get_channels_with_search( limit: int = 999, types: Optional[list[SlackChannelTypes]] = None, exact_match: bool = False, -) -> list[str]: +) -> list[SlackChannelSchema]: """ The slack api is paginated but does not include search, so we need to fetch all channels and filter them ourselves @@ -66,7 +68,8 @@ def get_channels_with_search( try: client = get_slack_client() - channels = [] + channel_schema = SlackChannelSchema() + channels: list[SlackChannelSchema] = [] cursor = None extra_params = {} extra_params["types"] = ",".join(types) if types else None @@ -75,29 +78,27 @@ def get_channels_with_search( response = client.conversations_list( limit=limit, cursor=cursor, exclude_archived=True, **extra_params ) - channels.extend(response.data["channels"]) + channels.extend( + channel_schema.load(channel) for channel in response.data["channels"] + ) cursor = response.data.get("response_metadata", {}).get("next_cursor") if not cursor: break # The search string can be multiple channels separated by commas if search_string: - search_array = [ - search.lower() - for search in (search_string.split(",") if search_string else []) - ] - + search_array = recipients_string_to_list(search_string) channels = [ channel for channel in channels if any( ( - search == channel["name"].lower() - or search == channel["id"].lower() + search.lower() == channel["name"].lower() + or search.lower() == channel["id"].lower() if exact_match else ( - search in channel["name"].lower() - or search in channel["id"].lower() + search.lower() in channel["name"].lower() + or search.lower() in channel["id"].lower() ) ) for search in search_array diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 7b22b38fc0c..d3798d05ec8 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -308,7 +308,10 @@ def create_report_slack_chart(): def create_report_slack_chartv2(): chart = db.session.query(Slice).first() report_schedule = create_report_notification( - slack_channel="slack_channel_id", chart=chart, name="report_slack_chartv2" + slack_channel="slack_channel_id", + chart=chart, + name="report_slack_chartv2", + use_slack_v2=True, ) yield report_schedule @@ -1300,9 +1303,7 @@ 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" -) +@pytest.mark.usefixtures("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") @@ -1316,13 +1317,19 @@ def test_slack_chart_report_schedule_converts_to_v2( ): """ ExecuteReport Command: Test chart slack report schedule + while converting the recipients list to SlackV2. """ # setup screenshot mock screenshot_mock.return_value = SCREENSHOT_FILE - channel_id = "slack_channel_id" - - get_channels_with_search_mock.return_value = channel_id + get_channels_with_search_mock.return_value = [ + { + "id": channel_id, + "name": "slack_channel", + "is_member": True, + "is_private": False, + }, + ] with freeze_time("2020-01-01T00:00:00Z"): with patch.object(current_app.config["STATS_LOGGER"], "gauge") as statsd_mock: @@ -1357,33 +1364,40 @@ def test_slack_chart_report_schedule_converts_to_v2( 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" -) @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_v2( +def test_slack_chart_report_schedule_converts_to_v2_channel_with_hash( 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 + ExecuteReport Command: Test converting a Slack report to v2 when + the channel name includes the leading hash (supported in v1). """ # setup screenshot mock screenshot_mock.return_value = SCREENSHOT_FILE channel_id = "slack_channel_id" - - get_channels_with_search_mock.return_value = channel_id + chart = db.session.query(Slice).first() + report_schedule = create_report_notification( + slack_channel="#slack_channel", chart=chart + ) + get_channels_with_search_mock.return_value = [ + { + "id": channel_id, + "name": "slack_channel", + "is_member": True, + "is_private": False, + }, + ] 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() + TEST_ID, report_schedule.id, datetime.utcnow() ).run() assert ( @@ -1395,6 +1409,12 @@ def test_slack_chart_report_schedule_v2( == SCREENSHOT_FILE ) + # Assert that the report recipients were updated + assert report_schedule.recipients[0].recipient_config_json == json.dumps( + {"target": channel_id} + ) + assert report_schedule.recipients[0].type == ReportRecipientType.SLACKV2 + # Assert logs are correct assert_log(ReportState.SUCCESS) # this will send a warning @@ -1403,6 +1423,94 @@ def test_slack_chart_report_schedule_v2( ) assert statsd_mock.call_args_list[1] == call("reports.slack.send.ok", 1) + cleanup_report_schedule(report_schedule) + + +@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_fails_to_converts_to_v2( + screenshot_mock, + slack_client_mock, + slack_should_use_v2_api_mock, + get_channels_with_search_mock, +): + """ + ExecuteReport Command: Test converting a Slack report to v2 fails. + """ + # setup screenshot mock + screenshot_mock.return_value = SCREENSHOT_FILE + channel_id = "slack_channel_id" + chart = db.session.query(Slice).first() + report_schedule = create_report_notification( + slack_channel="#slack_channel,my_member_ID", chart=chart + ) + get_channels_with_search_mock.return_value = [ + { + "id": channel_id, + "name": "slack_channel", + "is_member": True, + "is_private": False, + }, + ] + + with pytest.raises(ReportScheduleSystemErrorsException): + AsyncExecuteReportScheduleCommand( + TEST_ID, report_schedule.id, datetime.utcnow() + ).run() + + # Assert failuer with proper log + expected_message = ( + "Failed to update slack recipients to v2: " + "Could not find the following channels: my_member_ID" + ) + assert_log(ReportState.ERROR, error_message=expected_message) + + # Assert that previous configuration was kept for manual correction + assert report_schedule.recipients[0].recipient_config_json == json.dumps( + {"target": "#slack_channel,my_member_ID"} + ) + assert report_schedule.recipients[0].type == ReportRecipientType.SLACK + + cleanup_report_schedule(report_schedule) + + +@pytest.mark.usefixtures("create_report_slack_chartv2") +@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_v2( + screenshot_mock, + slack_client_mock, + slack_should_use_v2_api_mock, + create_report_slack_chartv2, +): + """ + ExecuteReport Command: Test chart slack report schedule using Slack v2. + """ + # setup screenshot mock + screenshot_mock.return_value = SCREENSHOT_FILE + + 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_chartv2.id, datetime.utcnow() + ).run() + + assert ( + slack_client_mock.return_value.files_upload_v2.call_args[1]["channel"] + == "slack_channel_id" + ) + assert ( + slack_client_mock.return_value.files_upload_v2.call_args[1]["file"] + == SCREENSHOT_FILE + ) + + # Assert logs are correct + assert_log(ReportState.SUCCESS) + assert statsd_mock.call_args_list[0] == call("reports.slack.send.ok", 1) + @pytest.mark.usefixtures( "load_birth_names_dashboard_with_slices", "create_report_slack_chart" diff --git a/tests/integration_tests/reports/utils.py b/tests/integration_tests/reports/utils.py index 3ab5c46c4eb..e11ecc653b1 100644 --- a/tests/integration_tests/reports/utils.py +++ b/tests/integration_tests/reports/utils.py @@ -119,6 +119,7 @@ def create_report_notification( owners: Optional[list[User]] = None, ccTarget: Optional[str] = None, # noqa: N803 bccTarget: Optional[str] = None, # noqa: N803 + use_slack_v2: bool = False, ) -> ReportSchedule: if not owners: owners = [ @@ -130,8 +131,11 @@ def create_report_notification( ] if slack_channel: + type = ( + ReportRecipientType.SLACKV2 if use_slack_v2 else ReportRecipientType.SLACK + ) recipient = ReportRecipients( - type=ReportRecipientType.SLACK, + type=type, recipient_config_json=json.dumps( { "target": slack_channel, diff --git a/tests/integration_tests/utils_tests.py b/tests/integration_tests/utils_tests.py index ca49d65fc09..8a09a23f9f8 100644 --- a/tests/integration_tests/utils_tests.py +++ b/tests/integration_tests/utils_tests.py @@ -52,7 +52,7 @@ from superset.utils.core import ( GenericDataType, get_form_data_token, as_list, - get_email_address_list, + recipients_string_to_list, get_stacktrace, merge_extra_filters, merge_extra_form_data, @@ -809,12 +809,12 @@ class TestUtils(SupersetTestCase): assert expected_filename in path assert os.path.exists(path) - def test_get_email_address_list(self): - assert get_email_address_list("a@a") == ["a@a"] - assert get_email_address_list(" a@a ") == ["a@a"] - assert get_email_address_list("a@a\n") == ["a@a"] - assert get_email_address_list(",a@a;") == ["a@a"] - assert get_email_address_list(",a@a; b@b c@c a-c@c; d@d, f@f") == [ + def test_recipients_string_to_list(self): + assert recipients_string_to_list("a@a") == ["a@a"] + assert recipients_string_to_list(" a@a ") == ["a@a"] + assert recipients_string_to_list("a@a\n") == ["a@a"] + assert recipients_string_to_list(",a@a;") == ["a@a"] + assert recipients_string_to_list(",a@a; b@b c@c a-c@c; d@d, f@f") == [ "a@a", "b@b", "c@c", diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index 7c04f2f8ab2..0f245777297 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -24,9 +24,11 @@ import pytest from pytest_mock import MockerFixture from superset.app import SupersetApp +from superset.commands.exceptions import UpdateFailedError from superset.commands.report.execute import BaseReportState from superset.dashboards.permalink.types import DashboardPermalinkState from superset.reports.models import ( + ReportRecipients, ReportRecipientType, ReportSchedule, ReportScheduleType, @@ -480,3 +482,78 @@ def test_screenshot_width_calculation( f"Test {test_id}: Expected width {expected_width}, " f"but got {kwargs['window_size'][0]}" ) + + +def test_update_recipient_to_slack_v2(mocker: MockerFixture): + """ + Test converting a Slack recipient to Slack v2 format. + """ + mocker.patch( + "superset.commands.report.execute.get_channels_with_search", + return_value=[ + { + "id": "abc124f", + "name": "channel-1", + "is_member": True, + "is_private": False, + }, + { + "id": "blah_!channel_2", + "name": "Channel_2", + "is_member": True, + "is_private": False, + }, + ], + ) + mock_report_schedule = ReportSchedule( + recipients=[ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "Channel-1, Channel_2"}), + ), + ], + ) + + mock_cmmd: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + mock_cmmd.update_report_schedule_slack_v2() + + assert ( + mock_cmmd._report_schedule.recipients[0].recipient_config_json + == '{"target": "abc124f,blah_!channel_2"}' + ) + assert mock_cmmd._report_schedule.recipients[0].type == ReportRecipientType.SLACKV2 + + +def test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture): + """ + Test converting a Slack recipient to Slack v2 format raises an error + in case it can't find all channels. + """ + mocker.patch( + "superset.commands.report.execute.get_channels_with_search", + return_value=[ + { + "id": "blah_!channel_2", + "name": "Channel 2", + "is_member": True, + "is_private": False, + }, + ], + ) + mock_report_schedule = ReportSchedule( + name="Test Report", + recipients=[ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "Channel 1, Channel 2"}), + ), + ], + ) + + mock_cmmd: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + with pytest.raises(UpdateFailedError): + mock_cmmd.update_report_schedule_slack_v2() diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index 42c3d3d4ce8..ed7a82c220c 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -153,7 +153,12 @@ The server responded with: missing scope: channels:read""" def test_filter_channels_by_specified_types(self, mocker): mock_data = { "channels": [ - {"name": "general", "id": "C12345", "type": "public"}, + { + "id": "C12345", + "name": "general", + "is_member": False, + "is_private": False, + }, ], "response_metadata": {"next_cursor": None}, } @@ -164,7 +169,14 @@ The server responded with: missing scope: channels:read""" mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) result = get_channels_with_search(types=["public"]) - assert result == [{"name": "general", "id": "C12345", "type": "public"}] + assert result == [ + { + "id": "C12345", + "name": "general", + "is_member": False, + "is_private": False, + } + ] def test_handle_pagination_multiple_pages(self, mocker): mock_data_page1 = {