fix: pass slack recipients correctly (apache#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.base import NotificationContent
 from superset.reports.notifications.exceptions import (
     NotificationError,
+    NotificationParamException,
     SlackV1NotificationError,
 )
 from superset.tasks.utils import get_executor
@@ -132,15 +132,13 @@ def update_report_schedule_slack_v2(self) -> None:
         V2 uses ids instead of names for channels.
         """
         try:
-            updated_recipients = []
             for recipient in self._report_schedule.recipients:
-                recipient_copy = deepcopy(recipient)
-                if recipient_copy.type == ReportRecipientType.SLACK:
-                    recipient_copy.type = ReportRecipientType.SLACKV2
-                    slack_recipients = json.loads(recipient_copy.recipient_config_json)
+                if recipient.type == ReportRecipientType.SLACK:
+                    recipient.type = ReportRecipientType.SLACKV2
+                    slack_recipients = json.loads(recipient.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 @@ def update_report_schedule_slack_v2(self) -> None:
                             )
                         }
                     )
-
-                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,20 +362,30 @@ def _get_log_data(self) -> HeaderDataType:
         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
         else:
             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,
             "notification_format": self._report_schedule.report_format,
             "chart_id": chart_id,
             "dashboard_id": dashboard_id,
             "owners": self._report_schedule.owners,
+            "slack_channels": slack_channels,
         }
         return log_data
 
@@ -486,7 +491,7 @@ def _send(
                     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.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 = notification_targets[0]
+    channel_id = "slack_channel_id"
 
-    get_channels_with_search_mock.return_value = {}
+    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: