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()