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

(cherry picked from commit d2e0e2b)

Author: Vitor-Avila

superset/commands/report/execute.py

@@ -72,7 +72,7 @@
 )
 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 @@ def update_report_schedule_slack_v2(self) -> None:
                 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 @@ def _send(
         for recipient in recipients:
             notification = create_notification(recipient, notification_content)
             try:
-                if app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"]:
+                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(
-                        "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,
+                        "Attempting to upgrade the report to Slackv2: %s", str(ex)
                     )
-                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:
                     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(

superset/reports/notifications/slack.py

@@ -43,7 +43,7 @@
 )
 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 @@ def _get_channel(self) -> str:
         """
         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,

superset/reports/notifications/slackv2.py

@@ -42,7 +42,7 @@
 )
 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 @@ def _get_channels(self) -> List[str]:
         """  # 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,

superset/reports/schemas.py

@@ -19,7 +19,7 @@
 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 @@ def validate_custom_width(
                     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()

superset/utils/core.py

@@ -700,7 +700,7 @@ def send_email_smtp(  # pylint: disable=invalid-name,too-many-arguments,too-many
         '[email protected]', '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
@@ -711,14 +711,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)
@@ -811,7 +811,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)

superset/utils/slack.py

@@ -25,7 +25,9 @@
 
 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__)
 
@@ -51,7 +53,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 +62,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,29 +72,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