fix slack recipients

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

tests/integration_tests/reports/commands_tests.py

@@ -66,6 +66,7 @@
 from superset.reports.models import (
     ReportDataFormat,
     ReportExecutionLog,
+    ReportRecipientType,
     ReportSchedule,
     ReportScheduleType,
     ReportScheduleValidatorType,
@@ -166,7 +167,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)")
 
@@ -1205,6 +1208,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"
 )
@@ -1224,11 +1284,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:
@@ -1301,8 +1359,8 @@ def test_slack_chart_report_schedule_with_errors(
     )
     error_logs = get_error_logs_query(create_report_slack_chart)
 
-    # check that we have two logs for each error
-    assert error_logs.count() == (len(slack_errors) + notification_logs_count) * 2
+    # check that we have a log for each error
+    assert error_logs.count() == len(slack_errors) + notification_logs_count
 
     # check that each error has a message
     assert len([log.error_message for log in error_logs]) == error_logs.count()

tests/unit_tests/reports/notifications/slack_tests.py

@@ -19,12 +19,27 @@
 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
+
+
+@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() -> None:
+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={
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
+        header_data=mock_header_data,
         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={
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
+        header_data=mock_header_data,
         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={
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
+        header_data=mock_header_data,
         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={
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
+        header_data=mock_header_data,
         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={
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
+        header_data=mock_header_data,
         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={
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
+        header_data=mock_header_data,
         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={
-            "notification_format": "PNG",
-            "notification_type": "Alert",
-            "owners": [1],
-            "notification_source": None,
-            "chart_id": None,
-            "dashboard_id": None,
-        },
+        header_data=mock_header_data,
         embedded_data=pd.DataFrame(
             {
                 "A": [1, 2, 3],