Addressing PR feedback

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 get_recipients_list, 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,18 +137,19 @@ 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_list = get_recipients_list(slack_recipients["target"])
-                    channels_list = [channel.lstrip("#") for channel in channels_list]
                     channels = get_channels_with_search(
-                        search_string=channels_list,
+                        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
@@ -158,7 +159,7 @@ def update_report_schedule_slack_v2(self) -> None:
                             f"{', '.join(missing_channels)}"
                         )
                         raise UpdateFailedError(msg)
-                    channel_ids = ", ".join(channel["id"] for channel in channels)
+                    channel_ids = ",".join(channel["id"] for channel in channels)
                     recipient.recipient_config_json = json.dumps(
                         {
                             "target": channel_ids,
@@ -168,7 +169,7 @@ def update_report_schedule_slack_v2(self) -> None:
             # 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.error(msg, exc_info=True)
+            logger.exception(msg)
             raise UpdateFailedError(msg) from ex
 
     def create_log(self, error_message: Optional[str] = None) -> None:
@@ -568,33 +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 mig_err:
-                    notification_errors.append(
-                        SupersetError(
-                            message=mig_err.message,
-                            error_type=SupersetErrorType.REPORT_NOTIFICATION_ERROR,
-                            level=ErrorLevel.ERROR,
-                        )
-                    )
-            except (NotificationError, SupersetException) as ex:
+            except (
+                UpdateFailedError,
+                NotificationParamException,
+                NotificationError,
+                SupersetException,
+            ) as ex:
                 # collect errors but keep processing them
                 notification_errors.append(
                     SupersetError(

superset/reports/api.py

@@ -574,7 +574,7 @@ def slack_channels(self, **kwargs: Any) -> Response:
         """
         try:
             params = kwargs.get("rison", {})
-            search_string = [params.get("search_string", "").strip()]
+            search_string = params.get("search_string")
             types = params.get("types", [])
             exact_match = params.get("exact_match", False)
             channels = get_channels_with_search(

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_recipients_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_recipients_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_recipients_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_recipients_list(recipient_str)
+        return recipients_string_to_list(recipient_str)
 
     def _get_inline_files(
         self,

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_recipients_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_recipients_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_recipients_list(bcc)
+        smtp_mail_bcc = recipients_string_to_list(bcc)
         recipients = recipients + smtp_mail_bcc
 
     msg["Date"] = formatdate(localtime=True)
@@ -811,7 +811,7 @@ def send_mime_email(
     smtp.quit()
 
 
-def get_recipients_list(address_string: str | None) -> list[str]:
+def recipients_string_to_list(address_string: str | None) -> list[str]:
     """
     Returns the list of target recipients for alerts and reports.
 

superset/utils/slack.py

@@ -27,6 +27,7 @@
 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__)
 
@@ -48,7 +49,7 @@ def get_slack_client() -> WebClient:
 
 
 def get_channels_with_search(
-    search_string: list[str] | None = None,
+    search_string: str = "",
     limit: int = 999,
     types: Optional[list[SlackChannelTypes]] = None,
     exact_match: bool = False,
@@ -80,6 +81,7 @@ def get_channels_with_search(
 
         # The search string can be multiple channels separated by commas
         if search_string:
+            search_array = recipients_string_to_list(search_string)
             channels = [
                 channel
                 for channel in channels
@@ -93,7 +95,7 @@ def get_channels_with_search(
                             or search.lower() in channel["id"].lower()
                         )
                     )
-                    for search in search_string
+                    for search in search_array
                 )
             ]
         return channels

tests/integration_tests/utils_tests.py

@@ -52,7 +52,7 @@
     GenericDataType,
     get_form_data_token,
     as_list,
-    get_recipients_list,
+    recipients_string_to_list,
     get_stacktrace,
     merge_extra_filters,
     merge_extra_form_data,
@@ -809,12 +809,12 @@ def test_ssl_certificate_file_creation(self):
         assert expected_filename in path
         assert os.path.exists(path)
 
-    def test_get_recipients_list(self):
-        assert get_recipients_list("a@a") == ["a@a"]
-        assert get_recipients_list(" a@a ") == ["a@a"]
-        assert get_recipients_list("a@a\n") == ["a@a"]
-        assert get_recipients_list(",a@a;") == ["a@a"]
-        assert get_recipients_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",

tests/unit_tests/commands/report/execute_test.py

@@ -521,7 +521,7 @@ def test_update_recipient_to_slack_v2(mocker: MockerFixture):
 
     assert (
         mock_cmmd._report_schedule.recipients[0].recipient_config_json
-        == '{"target": "abc124f, blah_!channel_2"}'
+        == '{"target": "abc124f,blah_!channel_2"}'
     )
     assert mock_cmmd._report_schedule.recipients[0].type == ReportRecipientType.SLACKV2
 

tests/unit_tests/utils/slack_test.py

@@ -60,7 +60,7 @@ def test_handle_empty_search_string(self, mocker):
         mock_client.conversations_list.return_value = mock_response_instance
         mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
 
-        result = get_channels_with_search(search_string=[])
+        result = get_channels_with_search(search_string="")
         assert result == [{"name": "general", "id": "C12345"}]
 
     def test_handle_exact_match_search_string_single_channel(self, mocker):
@@ -81,7 +81,7 @@ def test_handle_exact_match_search_string_single_channel(self, mocker):
         mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
 
         # Call the function with a search string that matches a single channel
-        result = get_channels_with_search(search_string=["general"], exact_match=True)
+        result = get_channels_with_search(search_string="general", exact_match=True)
 
         # Assert that the result is a list with a single channel dictionary
         assert result == [{"name": "general", "id": "C12345"}]
@@ -102,7 +102,7 @@ def test_handle_exact_match_search_string_multiple_channels(self, mocker):
         mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
 
         result = get_channels_with_search(
-            search_string=["general", "random"], exact_match=True
+            search_string="general,random", exact_match=True
         )
         assert result == [
             {"name": "general", "id": "C12345"},
@@ -124,7 +124,7 @@ def test_handle_loose_match_search_string_multiple_channels(self, mocker):
         mock_client.conversations_list.return_value = mock_response_instance
         mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
 
-        result = get_channels_with_search(search_string=["general", "random"])
+        result = get_channels_with_search(search_string="general,random")
         assert result == [
             {"name": "general", "id": "C12345"},
             {"name": "general2", "id": "C13454"},