This is an automated email from the ASF dual-hosted git repository. vavila pushed a commit to branch fix/slack-v2-channels-migration in repository https://gitbox.apache.org/repos/asf/superset.git
commit 206a73e4e18bbfd035d213d1e91ce5d9a4d84297 Author: Vitor Avila <[email protected]> AuthorDate: Thu Feb 20 19:04:33 2025 -0300 fix(Slack): Fix Slack recipients migration to V2 --- superset/commands/report/execute.py | 20 +++--- superset/reports/schemas.py | 16 ++++- superset/utils/slack.py | 12 ++-- tests/unit_tests/commands/report/execute_test.py | 77 ++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 12 deletions(-) diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 0c57d55bb5..839c1496f8 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -139,15 +139,21 @@ class BaseReportState: slack_recipients = json.loads(recipient.recipient_config_json) # we need to ensure that existing reports can also fetch # ids from private channels + current_target_count = slack_recipients["target"].split(",") + channels = get_channels_with_search( + slack_recipients["target"], + types=[ + SlackChannelTypes.PRIVATE, + SlackChannelTypes.PUBLIC, + ], + exact_match=True, + ) + if len(current_target_count) != len(channels): + raise UpdateFailedError("Not all channels could be found") + 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: diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 7078970b38..cfccc579bc 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/slack.py b/superset/utils/slack.py index 468429fb60..b788d96630 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -25,6 +25,7 @@ from slack_sdk.errors import SlackApiError from superset import feature_flag_manager from superset.exceptions import SupersetException +from superset.reports.schemas import SlackChannelSchema from superset.utils.backports import StrEnum logger = logging.getLogger(__name__) @@ -51,7 +52,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 @@ -60,7 +61,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 @@ -69,7 +71,9 @@ 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 @@ -77,7 +81,7 @@ def get_channels_with_search( # The search string can be multiple channels separated by commas if search_string: search_array = [ - search.lower() + search.strip().lower() for search in (search_string.split(",") if search_string else []) ] diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index 7c04f2f8ab..69bcc03074 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()
