fix: pass slack recipients correctly (#29721)

pyproject.toml

@@ -230,6 +230,7 @@ module = "tests.*"
 check_untyped_defs = false
 disallow_untyped_calls = false
 disallow_untyped_defs = false
+disable_error_code = "annotation-unchecked"
 
 [tool.tox]
 legacy_tox_ini = """

scripts/tests/run.sh

@@ -53,6 +53,9 @@ function test_init() {
   echo Superset init
   echo --------------------
   superset init
+  echo Load test users
+  echo --------------------
+  superset load-test-users
 }
 
 #

superset/commands/report/execute.py

@@ -15,7 +15,6 @@
 # specific language governing permissions and limitations
 # under the License.
 import logging
-from copy import deepcopy
 from datetime import datetime, timedelta
 from typing import Any, Optional, Union
 from uuid import UUID
@@ -67,6 +66,7 @@ from superset.reports.notifications import create_notification
 from superset.reports.notifications.base import NotificationContent
 from superset.reports.notifications.exceptions import (
     NotificationError,
+    NotificationParamException,
     SlackV1NotificationError,
 )
 from superset.tasks.utils import get_executor
@@ -132,15 +132,13 @@ class BaseReportState:
         V2 uses ids instead of names for channels.
         """
         try:
-            updated_recipients = []
             for recipient in self._report_schedule.recipients:
-                recipient_copy = deepcopy(recipient)
+                if recipient.type == ReportRecipientType.SLACK:
-                if recipient_copy.type == ReportRecipientType.SLACK:
+                    recipient.type = ReportRecipientType.SLACKV2
-                    recipient_copy.type = ReportRecipientType.SLACKV2
+                    slack_recipients = json.loads(recipient.recipient_config_json)
-                    slack_recipients = json.loads(recipient_copy.recipient_config_json)
                     # we need to ensure that existing reports can also fetch
                     # ids from private channels
-                    recipient_copy.recipient_config_json = json.dumps(
+                    recipient.recipient_config_json = json.dumps(
                         {
                             "target": get_channels_with_search(
                                 slack_recipients["target"],
@@ -151,9 +149,6 @@ class BaseReportState:
                             )
                         }
                     )
-
-                updated_recipients.append(recipient_copy)
-            db.session.commit()  # pylint: disable=consider-using-transaction
         except Exception as ex:
             logger.warning(
                 "Failed to update slack recipients to v2: %s", str(ex), exc_info=True
@@ -367,6 +362,7 @@ class BaseReportState:
         chart_id = None
         dashboard_id = None
         report_source = None
+        slack_channels = None
         if self._report_schedule.chart:
             report_source = ReportSourceFormat.CHART
             chart_id = self._report_schedule.chart_id
@@ -374,6 +370,14 @@ class BaseReportState:
             report_source = ReportSourceFormat.DASHBOARD
             dashboard_id = self._report_schedule.dashboard_id
 
+        if self._report_schedule.recipients:
+            slack_channels = [
+                recipient.recipient_config_json
+                for recipient in self._report_schedule.recipients
+                if recipient.type
+                in [ReportRecipientType.SLACK, ReportRecipientType.SLACKV2]
+            ]
+
         log_data: HeaderDataType = {
             "notification_type": self._report_schedule.type,
             "notification_source": report_source,
@@ -381,6 +385,7 @@ class BaseReportState:
             "chart_id": chart_id,
             "dashboard_id": dashboard_id,
             "owners": self._report_schedule.owners,
+            "slack_channels": slack_channels,
         }
         return log_data
 
@@ -486,7 +491,7 @@ class BaseReportState:
                     recipient.type = ReportRecipientType.SLACKV2
                     notification = create_notification(recipient, notification_content)
                     notification.send()
-                except UpdateFailedError as err:
+                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)

superset/utils/core.py

@@ -167,6 +167,7 @@ class HeaderDataType(TypedDict):
     notification_source: str | None
     chart_id: int | None
     dashboard_id: int | None
+    slack_channels: list[str] | None
 
 
 class DatasourceDict(TypedDict):

tests/integration_tests/reports/commands/execute_dashboard_report_tests.py

@@ -112,4 +112,4 @@ def test_report_with_header_data(
         assert header_data.get("notification_format") == report_schedule.report_format
         assert header_data.get("notification_source") == ReportSourceFormat.DASHBOARD
         assert header_data.get("notification_type") == report_schedule.type
-        assert len(send_email_smtp_mock.call_args.kwargs["header_data"]) == 6
+        assert len(send_email_smtp_mock.call_args.kwargs["header_data"]) == 7

tests/integration_tests/reports/commands_tests.py

@@ -67,6 +67,7 @@ from superset.models.slice import Slice
 from superset.reports.models import (
     ReportDataFormat,
     ReportExecutionLog,
+    ReportRecipientType,
     ReportSchedule,
     ReportScheduleType,
     ReportScheduleValidatorType,
@@ -171,7 +172,9 @@ def assert_log(state: str, error_message: Optional[str] = None):
 @contextmanager
 def create_test_table_context(database: Database):
     with database.get_sqla_engine() as engine:
-        engine.execute("CREATE TABLE test_table AS SELECT 1 as first, 2 as second")
+        engine.execute(
+            "CREATE TABLE IF NOT EXISTS test_table AS SELECT 1 as first, 2 as second"
+        )
         engine.execute("INSERT INTO test_table (first, second) VALUES (1, 2)")
         engine.execute("INSERT INTO test_table (first, second) VALUES (3, 4)")
 
@@ -1297,6 +1300,63 @@ 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"
+)
+@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_converts_to_v2(
+    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
+    """
+    # setup screenshot mock
+    screenshot_mock.return_value = SCREENSHOT_FILE
+
+    channel_id = "slack_channel_id"
+
+    get_channels_with_search_mock.return_value = channel_id
+
+    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()
+            ).run()
+
+            assert (
+                slack_client_mock.return_value.files_upload_v2.call_args[1]["channel"]
+                == channel_id
+            )
+            assert (
+                slack_client_mock.return_value.files_upload_v2.call_args[1]["file"]
+                == SCREENSHOT_FILE
+            )
+
+            # Assert that the report recipients were updated
+            assert create_report_slack_chart.recipients[
+                0
+            ].recipient_config_json == json.dumps({"target": channel_id})
+            assert (
+                create_report_slack_chart.recipients[0].type
+                == ReportRecipientType.SLACKV2
+            )
+
+            # Assert logs are correct
+            assert_log(ReportState.SUCCESS)
+            # this will send a warning
+            assert statsd_mock.call_args_list[0] == call(
+                "reports.slack.send.warning", 1
+            )
+            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"
 )
@@ -1316,11 +1376,9 @@ def test_slack_chart_report_schedule_v2(
     """
     # setup screenshot mock
     screenshot_mock.return_value = SCREENSHOT_FILE
-    notification_targets = get_target_from_report_schedule(create_report_slack_chart)
+    channel_id = "slack_channel_id"
 
-    channel_id = notification_targets[0]
+    get_channels_with_search_mock.return_value = channel_id
-
-    get_channels_with_search_mock.return_value = {}
 
     with freeze_time("2020-01-01T00:00:00Z"):
         with patch.object(current_app.config["STATS_LOGGER"], "gauge") as statsd_mock:

tests/unit_tests/commands/report/execute_test.py

@@ -0,0 +1,222 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from pytest_mock import MockerFixture
+
+from superset.commands.report.execute import BaseReportState
+from superset.reports.models import (
+    ReportRecipientType,
+    ReportSchedule,
+    ReportSourceFormat,
+)
+from superset.utils.core import HeaderDataType
+
+
+def test_log_data_with_chart(mocker: MockerFixture) -> None:
+    mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule)
+    mock_report_schedule.chart = True
+    mock_report_schedule.chart_id = 123
+    mock_report_schedule.dashboard_id = None
+    mock_report_schedule.type = "report_type"
+    mock_report_schedule.report_format = "report_format"
+    mock_report_schedule.owners = [1, 2]
+    mock_report_schedule.recipients = []
+
+    class_instance: BaseReportState = BaseReportState(
+        mock_report_schedule, "January 1, 2021", "execution_id_example"
+    )
+    class_instance._report_schedule = mock_report_schedule
+
+    result: HeaderDataType = class_instance._get_log_data()
+
+    expected_result: HeaderDataType = {
+        "notification_type": "report_type",
+        "notification_source": ReportSourceFormat.CHART,
+        "notification_format": "report_format",
+        "chart_id": 123,
+        "dashboard_id": None,
+        "owners": [1, 2],
+        "slack_channels": None,
+    }
+
+    assert result == expected_result
+
+
+def test_log_data_with_dashboard(mocker: MockerFixture) -> None:
+    mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule)
+    mock_report_schedule.chart = False
+    mock_report_schedule.chart_id = None
+    mock_report_schedule.dashboard_id = 123
+    mock_report_schedule.type = "report_type"
+    mock_report_schedule.report_format = "report_format"
+    mock_report_schedule.owners = [1, 2]
+    mock_report_schedule.recipients = []
+
+    class_instance: BaseReportState = BaseReportState(
+        mock_report_schedule, "January 1, 2021", "execution_id_example"
+    )
+    class_instance._report_schedule = mock_report_schedule
+
+    result: HeaderDataType = class_instance._get_log_data()
+
+    expected_result: HeaderDataType = {
+        "notification_type": "report_type",
+        "notification_source": ReportSourceFormat.DASHBOARD,
+        "notification_format": "report_format",
+        "chart_id": None,
+        "dashboard_id": 123,
+        "owners": [1, 2],
+        "slack_channels": None,
+    }
+
+    assert result == expected_result
+
+
+def test_log_data_with_email_recipients(mocker: MockerFixture) -> None:
+    mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule)
+    mock_report_schedule.chart = False
+    mock_report_schedule.chart_id = None
+    mock_report_schedule.dashboard_id = 123
+    mock_report_schedule.type = "report_type"
+    mock_report_schedule.report_format = "report_format"
+    mock_report_schedule.owners = [1, 2]
+    mock_report_schedule.recipients = []
+    mock_report_schedule.recipients = [
+        mocker.Mock(type=ReportRecipientType.EMAIL, recipient_config_json="email_1"),
+        mocker.Mock(type=ReportRecipientType.EMAIL, recipient_config_json="email_2"),
+    ]
+
+    class_instance: BaseReportState = BaseReportState(
+        mock_report_schedule, "January 1, 2021", "execution_id_example"
+    )
+    class_instance._report_schedule = mock_report_schedule
+
+    result: HeaderDataType = class_instance._get_log_data()
+
+    expected_result: HeaderDataType = {
+        "notification_type": "report_type",
+        "notification_source": ReportSourceFormat.DASHBOARD,
+        "notification_format": "report_format",
+        "chart_id": None,
+        "dashboard_id": 123,
+        "owners": [1, 2],
+        "slack_channels": [],
+    }
+
+    assert result == expected_result
+
+
+def test_log_data_with_slack_recipients(mocker: MockerFixture) -> None:
+    mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule)
+    mock_report_schedule.chart = False
+    mock_report_schedule.chart_id = None
+    mock_report_schedule.dashboard_id = 123
+    mock_report_schedule.type = "report_type"
+    mock_report_schedule.report_format = "report_format"
+    mock_report_schedule.owners = [1, 2]
+    mock_report_schedule.recipients = []
+    mock_report_schedule.recipients = [
+        mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_1"),
+        mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_2"),
+    ]
+
+    class_instance: BaseReportState = BaseReportState(
+        mock_report_schedule, "January 1, 2021", "execution_id_example"
+    )
+    class_instance._report_schedule = mock_report_schedule
+
+    result: HeaderDataType = class_instance._get_log_data()
+
+    expected_result: HeaderDataType = {
+        "notification_type": "report_type",
+        "notification_source": ReportSourceFormat.DASHBOARD,
+        "notification_format": "report_format",
+        "chart_id": None,
+        "dashboard_id": 123,
+        "owners": [1, 2],
+        "slack_channels": ["channel_1", "channel_2"],
+    }
+
+    assert result == expected_result
+
+
+def test_log_data_no_owners(mocker: MockerFixture) -> None:
+    mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule)
+    mock_report_schedule.chart = False
+    mock_report_schedule.chart_id = None
+    mock_report_schedule.dashboard_id = 123
+    mock_report_schedule.type = "report_type"
+    mock_report_schedule.report_format = "report_format"
+    mock_report_schedule.owners = []
+    mock_report_schedule.recipients = [
+        mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_1"),
+        mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_2"),
+    ]
+
+    class_instance: BaseReportState = BaseReportState(
+        mock_report_schedule, "January 1, 2021", "execution_id_example"
+    )
+    class_instance._report_schedule = mock_report_schedule
+
+    result: HeaderDataType = class_instance._get_log_data()
+
+    expected_result: HeaderDataType = {
+        "notification_type": "report_type",
+        "notification_source": ReportSourceFormat.DASHBOARD,
+        "notification_format": "report_format",
+        "chart_id": None,
+        "dashboard_id": 123,
+        "owners": [],
+        "slack_channels": ["channel_1", "channel_2"],
+    }
+
+    assert result == expected_result
+
+
+def test_log_data_with_missing_values(mocker: MockerFixture) -> None:
+    mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule)
+    mock_report_schedule.chart = None
+    mock_report_schedule.chart_id = None
+    mock_report_schedule.dashboard_id = None
+    mock_report_schedule.type = "report_type"
+    mock_report_schedule.report_format = "report_format"
+    mock_report_schedule.owners = [1, 2]
+    mock_report_schedule.recipients = [
+        mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_1"),
+        mocker.Mock(
+            type=ReportRecipientType.SLACKV2, recipient_config_json="channel_2"
+        ),
+    ]
+
+    class_instance: BaseReportState = BaseReportState(
+        mock_report_schedule, "January 1, 2021", "execution_id_example"
+    )
+    class_instance._report_schedule = mock_report_schedule
+
+    result: HeaderDataType = class_instance._get_log_data()
+
+    expected_result: HeaderDataType = {
+        "notification_type": "report_type",
+        "notification_source": ReportSourceFormat.DASHBOARD,
+        "notification_format": "report_format",
+        "chart_id": None,
+        "dashboard_id": None,
+        "owners": [1, 2],
+        "slack_channels": ["channel_1", "channel_2"],
+    }
+
+    assert result == expected_result

tests/unit_tests/reports/notifications/email_tests.py

@@ -41,6 +41,7 @@ def test_render_description_with_html() -> None:
             "notification_source": None,
             "chart_id": None,
             "dashboard_id": None,
+            "slack_channels": None,
         },
     )
     email_body = (

tests/unit_tests/reports/notifications/slack_tests.py

@@ -19,12 +19,27 @@ import uuid
 from unittest.mock import MagicMock, patch
 
 import pandas as pd
+import pytest
 from slack_sdk.errors import SlackApiError
 
 from superset.reports.notifications.slackv2 import SlackV2Notification
+from superset.utils.core import HeaderDataType
 
 
-def test_get_channel_with_multi_recipients() -> None:
+@pytest.fixture
+def mock_header_data() -> HeaderDataType:
+    return {
+        "notification_format": "PNG",
+        "notification_type": "Alert",
+        "owners": [1],
+        "notification_source": None,
+        "chart_id": None,
+        "dashboard_id": None,
+        "slack_channels": ["some_channel"],
+    }
+
+
+def test_get_channel_with_multi_recipients(mock_header_data) -> None:
     """
     Test the _get_channel function to ensure it will return a string
     with recipients separated by commas without interstitial spacing
@@ -35,14 +50,7 @@ def test_get_channel_with_multi_recipients() -> None:
 
     content = NotificationContent(
         name="test alert",
-        header_data={
+        header_data=mock_header_data,
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
         embedded_data=pd.DataFrame(
             {
                 "A": [1, 2, 3],
@@ -67,7 +75,7 @@ def test_get_channel_with_multi_recipients() -> None:
     # Test if the recipient configuration JSON is valid when using a SlackV2 recipient type
 
 
-def test_valid_recipient_config_json_slackv2() -> None:
+def test_valid_recipient_config_json_slackv2(mock_header_data) -> None:
     """
     Test if the recipient configuration JSON is valid when using a SlackV2 recipient type
     """
@@ -77,14 +85,7 @@ def test_valid_recipient_config_json_slackv2() -> None:
 
     content = NotificationContent(
         name="test alert",
-        header_data={
+        header_data=mock_header_data,
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
         embedded_data=pd.DataFrame(
             {
                 "A": [1, 2, 3],
@@ -109,7 +110,7 @@ def test_valid_recipient_config_json_slackv2() -> None:
     # Ensure _get_inline_files function returns the correct tuple when content has screenshots
 
 
-def test_get_inline_files_with_screenshots() -> None:
+def test_get_inline_files_with_screenshots(mock_header_data) -> None:
     """
     Test the _get_inline_files function to ensure it will return the correct tuple
     when content has screenshots
@@ -120,14 +121,7 @@ def test_get_inline_files_with_screenshots() -> None:
 
     content = NotificationContent(
         name="test alert",
-        header_data={
+        header_data=mock_header_data,
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
         embedded_data=pd.DataFrame(
             {
                 "A": [1, 2, 3],
@@ -153,7 +147,7 @@ def test_get_inline_files_with_screenshots() -> None:
     # Ensure _get_inline_files function returns None when content has no screenshots or csv
 
 
-def test_get_inline_files_with_no_screenshots_or_csv() -> None:
+def test_get_inline_files_with_no_screenshots_or_csv(mock_header_data) -> None:
     """
     Test the _get_inline_files function to ensure it will return None
     when content has no screenshots or csv
@@ -164,14 +158,7 @@ def test_get_inline_files_with_no_screenshots_or_csv() -> None:
 
     content = NotificationContent(
         name="test alert",
-        header_data={
+        header_data=mock_header_data,
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
         embedded_data=pd.DataFrame(
             {
                 "A": [1, 2, 3],
@@ -201,6 +188,7 @@ def test_send_slackv2(
     slack_client_mock: MagicMock,
     logger_mock: MagicMock,
     flask_global_mock: MagicMock,
+    mock_header_data,
 ) -> None:
     # `superset.models.helpers`, a dependency of following imports,
     # requires app context
@@ -212,14 +200,7 @@ def test_send_slackv2(
     slack_client_mock.return_value.chat_postMessage.return_value = {"ok": True}
     content = NotificationContent(
         name="test alert",
-        header_data={
+        header_data=mock_header_data,
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
         embedded_data=pd.DataFrame(
             {
                 "A": [1, 2, 3],
@@ -269,6 +250,7 @@ def test_send_slack(
     slack_client_mock_util: MagicMock,
     logger_mock: MagicMock,
     flask_global_mock: MagicMock,
+    mock_header_data,
 ) -> None:
     # `superset.models.helpers`, a dependency of following imports,
     # requires app context
@@ -285,14 +267,7 @@ def test_send_slack(
 
     content = NotificationContent(
         name="test alert",
-        header_data={
+        header_data=mock_header_data,
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
         embedded_data=pd.DataFrame(
             {
                 "A": [1, 2, 3],
@@ -343,6 +318,7 @@ def test_send_slack_no_feature_flag(
     slack_client_mock_util: MagicMock,
     logger_mock: MagicMock,
     flask_global_mock: MagicMock,
+    mock_header_data,
 ) -> None:
     # `superset.models.helpers`, a dependency of following imports,
     # requires app context
@@ -360,14 +336,7 @@ def test_send_slack_no_feature_flag(
 
     content = NotificationContent(
         name="test alert",
-        header_data={
+        header_data=mock_header_data,
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
         embedded_data=pd.DataFrame(
             {
                 "A": [1, 2, 3],