fix slack recipients

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
@@ -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

@@ -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:

tests/unit_tests/commands/report/execute_test.py

@@ -15,6 +15,8 @@
 # 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,
@@ -24,9 +26,8 @@
 from superset.utils.core import HeaderDataType
 
 
-def test_log_data_with_chart(mocker):
-    # Mocking the report schedule
-    mock_report_schedule = mocker.Mock(spec=ReportSchedule)
+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
@@ -35,16 +36,13 @@ def test_log_data_with_chart(mocker):
     mock_report_schedule.owners = [1, 2]
     mock_report_schedule.recipients = []
 
-    # Initializing the class and setting the report schedule
-    class_instance = BaseReportState(
+    class_instance: BaseReportState = BaseReportState(
         mock_report_schedule, "January 1, 2021", "execution_id_example"
     )
     class_instance._report_schedule = mock_report_schedule
 
-    # Calling the method
-    result = class_instance._get_log_data()
+    result: HeaderDataType = class_instance._get_log_data()
 
-    # Expected result
     expected_result: HeaderDataType = {
         "notification_type": "report_type",
         "notification_source": ReportSourceFormat.CHART,
@@ -55,13 +53,11 @@ def test_log_data_with_chart(mocker):
         "slack_channels": None,
     }
 
-    # Assertions
     assert result == expected_result
 
 
-def test_log_data_with_dashboard(mocker):
-    # Mocking the report schedule
-    mock_report_schedule = mocker.Mock(spec=ReportSchedule)
+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
@@ -70,16 +66,13 @@ def test_log_data_with_dashboard(mocker):
     mock_report_schedule.owners = [1, 2]
     mock_report_schedule.recipients = []
 
-    # Initializing the class and setting the report schedule
-    class_instance = BaseReportState(
+    class_instance: BaseReportState = BaseReportState(
         mock_report_schedule, "January 1, 2021", "execution_id_example"
     )
     class_instance._report_schedule = mock_report_schedule
 
-    # Calling the method
-    result = class_instance._get_log_data()
+    result: HeaderDataType = class_instance._get_log_data()
 
-    # Expected result
     expected_result: HeaderDataType = {
         "notification_type": "report_type",
         "notification_source": ReportSourceFormat.DASHBOARD,
@@ -90,13 +83,11 @@ def test_log_data_with_dashboard(mocker):
         "slack_channels": None,
     }
 
-    # Assertions
     assert result == expected_result
 
 
-def test_log_data_with_email_recipients(mocker):
-    # Mocking the report schedule
-    mock_report_schedule = mocker.Mock(spec=ReportSchedule)
+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
@@ -109,16 +100,13 @@ def test_log_data_with_email_recipients(mocker):
         mocker.Mock(type=ReportRecipientType.EMAIL, recipient_config_json="email_2"),
     ]
 
-    # Initializing the class and setting the report schedule
-    class_instance = BaseReportState(
+    class_instance: BaseReportState = BaseReportState(
         mock_report_schedule, "January 1, 2021", "execution_id_example"
     )
     class_instance._report_schedule = mock_report_schedule
 
-    # Calling the method
-    result = class_instance._get_log_data()
+    result: HeaderDataType = class_instance._get_log_data()
 
-    # Expected result
     expected_result: HeaderDataType = {
         "notification_type": "report_type",
         "notification_source": ReportSourceFormat.DASHBOARD,
@@ -129,13 +117,11 @@ def test_log_data_with_email_recipients(mocker):
         "slack_channels": [],
     }
 
-    # Assertions
     assert result == expected_result
 
 
-def test_log_data_with_slack_recipients(mocker):
-    # Mocking the report schedule
-    mock_report_schedule = mocker.Mock(spec=ReportSchedule)
+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
@@ -148,16 +134,13 @@ def test_log_data_with_slack_recipients(mocker):
         mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_2"),
     ]
 
-    # Initializing the class and setting the report schedule
-    class_instance = BaseReportState(
+    class_instance: BaseReportState = BaseReportState(
         mock_report_schedule, "January 1, 2021", "execution_id_example"
     )
     class_instance._report_schedule = mock_report_schedule
 
-    # Calling the method
-    result = class_instance._get_log_data()
+    result: HeaderDataType = class_instance._get_log_data()
 
-    # Expected result
     expected_result: HeaderDataType = {
         "notification_type": "report_type",
         "notification_source": ReportSourceFormat.DASHBOARD,
@@ -168,13 +151,11 @@ def test_log_data_with_slack_recipients(mocker):
         "slack_channels": ["channel_1", "channel_2"],
     }
 
-    # Assertions
     assert result == expected_result
 
 
-def test_log_data_no_owners(mocker):
-    # Mocking the report schedule
-    mock_report_schedule = mocker.Mock(spec=ReportSchedule)
+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
@@ -186,16 +167,13 @@ def test_log_data_no_owners(mocker):
         mocker.Mock(type=ReportRecipientType.SLACK, recipient_config_json="channel_2"),
     ]
 
-    # Initializing the class and setting the report schedule
-    class_instance = BaseReportState(
+    class_instance: BaseReportState = BaseReportState(
         mock_report_schedule, "January 1, 2021", "execution_id_example"
     )
     class_instance._report_schedule = mock_report_schedule
 
-    # Calling the method
-    result = class_instance._get_log_data()
+    result: HeaderDataType = class_instance._get_log_data()
 
-    # Expected result
     expected_result: HeaderDataType = {
         "notification_type": "report_type",
         "notification_source": ReportSourceFormat.DASHBOARD,
@@ -206,13 +184,11 @@ def test_log_data_no_owners(mocker):
         "slack_channels": ["channel_1", "channel_2"],
     }
 
-    # Assertions
     assert result == expected_result
 
 
-def test_log_data_with_missing_values(mocker):
-    # Mocking the report schedule
-    mock_report_schedule = mocker.Mock(spec=ReportSchedule)
+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
@@ -226,16 +202,13 @@ def test_log_data_with_missing_values(mocker):
         ),
     ]
 
-    # Initializing the class and setting the report schedule
-    class_instance = BaseReportState(
+    class_instance: BaseReportState = BaseReportState(
         mock_report_schedule, "January 1, 2021", "execution_id_example"
     )
     class_instance._report_schedule = mock_report_schedule
 
-    # Calling the method
-    result = class_instance._get_log_data()
+    result: HeaderDataType = class_instance._get_log_data()
 
-    # Expected result
     expected_result: HeaderDataType = {
         "notification_type": "report_type",
         "notification_source": ReportSourceFormat.DASHBOARD,
@@ -246,5 +219,4 @@ def test_log_data_with_missing_values(mocker):
         "slack_channels": ["channel_1", "channel_2"],
     }
 
-    # Assertions
     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 = (