QT/superset2
1
0
Fork 0
mirror of https://github.com/apache/superset.git synced 2026-04-19 08:04:53 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(Slack): Fix Slack recipients migration to V2 (#32336)

Browse Source
This commit is contained in:
Vitor Avila
2025-03-06 08:52:15 -03:00
committed by GitHub
parent 05409d51da
commit d2e0e2b79c
11 changed files with 318 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files

80
superset/commands/report/execute.py
View File

@@ -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(

4
superset/reports/notifications/slack.py
View File

@@ -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,

4
superset/reports/notifications/slackv2.py
View File

@@ -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,

16
superset/reports/schemas.py
View File

@@ -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()

14
superset/utils/core.py
View File

@@ -702,7 +702,7 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many
'test@example.com', 'foo', '<b>Foo</b> 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)

25
superset/utils/slack.py
View File

@@ -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

140
tests/integration_tests/reports/commands_tests.py
View File

@@ -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"

6
tests/integration_tests/reports/utils.py
View File

@@ -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,

14
tests/integration_tests/utils_tests.py
View File

@@ -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",

77
tests/unit_tests/commands/report/execute_test.py
View File

@@ -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()

16
tests/unit_tests/utils/slack_test.py
View File

@@ -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 = {