From 9d2b90d42a20da2bc70c912c4ea595e34fc03b89 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 6 Jan 2023 12:28:59 -0800 Subject: [PATCH 1/2] add execution_id to any available report logs --- superset/reports/commands/execute.py | 27 ++++++--- superset/reports/notifications/email.py | 4 +- superset/reports/notifications/slack.py | 5 +- superset/tasks/thumbnails.py | 17 +++++- superset/utils/core.py | 1 + superset/utils/screenshots.py | 56 ++++++++++--------- superset/utils/webdriver.py | 13 +++-- .../reports/commands_tests.py | 14 ++++- tests/integration_tests/thumbnails_tests.py | 54 ++++++++++++++++-- tests/unit_tests/notifications/email_tests.py | 3 + 10 files changed, 142 insertions(+), 52 deletions(-) diff --git a/superset/reports/commands/execute.py b/superset/reports/commands/execute.py index c8edd928b4784..849a987cf4d51 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -73,7 +73,11 @@ from superset.utils.celery import session_scope from superset.utils.core import HeaderDataType, override_user from superset.utils.csv import get_chart_csv_data, get_chart_dataframe -from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot +from superset.utils.screenshots import ( + ChartScreenshot, + DashboardScreenshot, + ScreenshotDetails, +) from superset.utils.urls import get_url_path logger = logging.getLogger(__name__) @@ -204,19 +208,23 @@ def _get_screenshots(self) -> List[bytes]: ) user = security_manager.find_user(username) if self._report_schedule.chart: - screenshot: Union[ChartScreenshot, DashboardScreenshot] = ChartScreenshot( - url, - self._report_schedule.chart.digest, + chart_options = ScreenshotDetails( + execution_id=self._execution_id, window_size=app.config["WEBDRIVER_WINDOW"]["slice"], thumb_size=app.config["WEBDRIVER_WINDOW"]["slice"], ) + screenshot: Union[ChartScreenshot, DashboardScreenshot] = ChartScreenshot( + url, self._report_schedule.chart.digest, chart_options + ) else: - screenshot = DashboardScreenshot( - url, - self._report_schedule.dashboard.digest, + dashboard_options = ScreenshotDetails( + execution_id=self._execution_id, window_size=app.config["WEBDRIVER_WINDOW"]["dashboard"], thumb_size=app.config["WEBDRIVER_WINDOW"]["dashboard"], ) + screenshot = DashboardScreenshot( + url, self._report_schedule.dashboard.digest, dashboard_options + ) try: image = screenshot.get_screenshot(user=user) except SoftTimeLimitExceeded as ex: @@ -324,6 +332,7 @@ def _get_log_data(self) -> HeaderDataType: "chart_id": chart_id, "dashboard_id": dashboard_id, "owners": self._report_schedule.owners, + "execution_id": self._execution_id, } return log_data @@ -453,7 +462,9 @@ def send_error(self, name: str, message: str) -> None: self._execution_id, ) notification_content = NotificationContent( - name=name, text=message, header_data=header_data + name=name, + text=message, + header_data=header_data, ) # filter recipients to recipients who are also owners diff --git a/superset/reports/notifications/email.py b/superset/reports/notifications/email.py index 22b1714f99fed..f2c7fcaadacc1 100644 --- a/superset/reports/notifications/email.py +++ b/superset/reports/notifications/email.py @@ -187,6 +187,7 @@ def _get_to(self) -> str: def send(self) -> None: subject = self._get_subject() content = self._get_content() + execution_id = content.header_data and content.header_data.get("execution_id") to = self._get_to() try: send_email_smtp( @@ -203,7 +204,8 @@ def send(self) -> None: header_data=content.header_data, ) logger.info( - "Report sent to email, notification content is %s", content.header_data + "Report sent to email. Execution_id is %s", + execution_id, ) except SupersetErrorsException as ex: raise NotificationError( diff --git a/superset/reports/notifications/slack.py b/superset/reports/notifications/slack.py index b89a700ef9c3e..14e18910c6d5a 100644 --- a/superset/reports/notifications/slack.py +++ b/superset/reports/notifications/slack.py @@ -180,7 +180,10 @@ def send(self) -> None: ) else: client.chat_postMessage(channel=channel, text=body) - logger.info("Report sent to slack") + logger.info( + "Report sent to slack. Execution id is %s", + self._content.header_data["execution_id"], + ) except ( BotUserAccessError, SlackRequestError, diff --git a/superset/tasks/thumbnails.py b/superset/tasks/thumbnails.py index d76939a07e3a0..4d6da5ccdd0df 100644 --- a/superset/tasks/thumbnails.py +++ b/superset/tasks/thumbnails.py @@ -26,7 +26,11 @@ from superset.extensions import celery_app from superset.tasks.utils import get_executor from superset.utils.core import override_user -from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot +from superset.utils.screenshots import ( + ChartScreenshot, + DashboardScreenshot, + ScreenshotDetails, +) from superset.utils.urls import get_url_path from superset.utils.webdriver import WindowSize @@ -47,6 +51,8 @@ def cache_chart_thumbnail( if not thumbnail_cache: logger.warning("No cache set, refusing to compute") return None + + task_id = None chart = cast(Slice, Slice.get(chart_id)) url = get_url_path("Superset.slice", slice_id=chart.id) logger.info("Caching chart: %s", url) @@ -57,7 +63,8 @@ def cache_chart_thumbnail( ) user = security_manager.find_user(username) with override_user(user): - screenshot = ChartScreenshot(url, chart.digest) + options = ScreenshotDetails(execution_id=task_id) + screenshot = ChartScreenshot(url, chart.digest, options) screenshot.compute_and_cache( user=user, cache=thumbnail_cache, @@ -81,6 +88,8 @@ def cache_dashboard_thumbnail( if not thumbnail_cache: logging.warning("No cache set, refusing to compute") return + + task_id = None dashboard = Dashboard.get(dashboard_id) url = get_url_path("Superset.dashboard", dashboard_id_or_slug=dashboard.id) @@ -91,8 +100,10 @@ def cache_dashboard_thumbnail( current_user=current_user, ) user = security_manager.find_user(username) + task_id = cache_dashboard_thumbnail.request.id with override_user(user): - screenshot = DashboardScreenshot(url, dashboard.digest) + options = ScreenshotDetails(execution_id=task_id) + screenshot = DashboardScreenshot(url, dashboard.digest, options) screenshot.compute_and_cache( user=user, cache=thumbnail_cache, diff --git a/superset/utils/core.py b/superset/utils/core.py index 06f2f63df1797..e15ec5f1f8ff8 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -202,6 +202,7 @@ class HeaderDataType(TypedDict): notification_source: Optional[str] chart_id: Optional[int] dashboard_id: Optional[int] + execution_id: uuid.UUID class DatasourceDict(TypedDict): diff --git a/superset/utils/screenshots.py b/superset/utils/screenshots.py index a904b7dc4384a..5e929ee71bd75 100644 --- a/superset/utils/screenshots.py +++ b/superset/utils/screenshots.py @@ -18,7 +18,8 @@ import logging from io import BytesIO -from typing import Optional, TYPE_CHECKING, Union +from typing import cast, Optional, TYPE_CHECKING, TypedDict, Union +from uuid import UUID from flask import current_app @@ -43,6 +44,12 @@ from flask_caching import Cache +class ScreenshotDetails(TypedDict, total=False): + execution_id: Optional[UUID] + window_size: Optional[WindowSize] + thumb_size: Optional[WindowSize] + + class BaseScreenshot: driver_type = current_app.config["WEBDRIVER_TYPE"] thumbnail_type: str = "" @@ -50,10 +57,11 @@ class BaseScreenshot: window_size: WindowSize = (800, 600) thumb_size: WindowSize = (400, 300) - def __init__(self, url: str, digest: str): + def __init__(self, url: str, digest: str, execution_id: Optional[UUID]): self.digest: str = digest self.url = url self.screenshot: Optional[bytes] = None + self.execution_id = execution_id def driver(self, window_size: Optional[WindowSize] = None) -> WebDriverProxy: window_size = window_size or self.window_size @@ -76,10 +84,17 @@ def cache_key( return md5_sha_from_dict(args) def get_screenshot( - self, user: User, window_size: Optional[WindowSize] = None + self, + user: User, + window_size: Optional[WindowSize] = None, ) -> Optional[bytes]: driver = self.driver(window_size) - self.screenshot = driver.get_screenshot(self.url, self.element, user) + self.screenshot = driver.get_screenshot( + url=self.url, + element_name=self.element, + user=user, + execution_id=self.execution_id, + ) return self.screenshot def get( @@ -204,41 +219,30 @@ class ChartScreenshot(BaseScreenshot): thumbnail_type: str = "chart" element: str = "chart-container" - def __init__( - self, - url: str, - digest: str, - window_size: Optional[WindowSize] = None, - thumb_size: Optional[WindowSize] = None, - ): - # Chart reports are in standalone="true" mode + def __init__(self, url: str, digest: str, *args: ScreenshotDetails): + # Chart reports and thumbnails are in standalone="true" mode url = modify_url_query( url, standalone=ChartStandaloneMode.HIDE_NAV.value, ) - super().__init__(url, digest) - self.window_size = window_size or (800, 600) - self.thumb_size = thumb_size or (800, 600) + options = cast(ScreenshotDetails, {*args}) + super().__init__(url, digest, options.get("execution_id")) + self.window_size = options.get("window_size") or (800, 600) + self.thumb_size = options.get("thumb_size") or (800, 600) class DashboardScreenshot(BaseScreenshot): thumbnail_type: str = "dashboard" element: str = "standalone" - def __init__( - self, - url: str, - digest: str, - window_size: Optional[WindowSize] = None, - thumb_size: Optional[WindowSize] = None, - ): + def __init__(self, url: str, digest: str, *args: ScreenshotDetails): # per the element above, dashboard screenshots # should always capture in standalone url = modify_url_query( url, standalone=DashboardStandaloneMode.REPORT.value, ) - - super().__init__(url, digest) - self.window_size = window_size or (1600, 1200) - self.thumb_size = thumb_size or (800, 600) + options = cast(ScreenshotDetails, {*args}) + super().__init__(url, digest, options.get("execution_id")) + self.window_size = options.get("window_size") or (1600, 1200) + self.thumb_size = options.get("thumb_size") or (800, 600) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index 92a73d347c20a..ebd491defa756 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -21,6 +21,7 @@ from enum import Enum from time import sleep from typing import Any, Dict, List, Optional, Tuple, TYPE_CHECKING +from uuid import UUID from flask import current_app from selenium.common.exceptions import ( @@ -165,7 +166,11 @@ def destroy(driver: WebDriver, tries: int = 2) -> None: pass def get_screenshot( - self, url: str, element_name: str, user: User + self, + url: str, + element_name: str, + user: User, + execution_id: Optional[UUID] = None, ) -> Optional[bytes]: driver = self.auth(user) driver.set_window_size(*self._window) @@ -174,7 +179,6 @@ def get_screenshot( selenium_headstart = current_app.config["SCREENSHOT_SELENIUM_HEADSTART"] logger.debug("Sleeping for %i seconds", selenium_headstart) sleep(selenium_headstart) - try: logger.debug("Wait for the presence of %s", element_name) element = WebDriverWait(driver, self._screenshot_locate_wait).until( @@ -198,10 +202,11 @@ def get_screenshot( ] logger.debug("Wait %i seconds for chart animation", selenium_animation_wait) sleep(selenium_animation_wait) - logger.debug( - "Taking a PNG screenshot of url %s as user %s", + logger.info( + "Taking a PNG screenshot of url %s as user %s with execution_id: %s", url, user.username, + execution_id, ) if current_app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"]: diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 8d6a76c14f67e..70ee90078db23 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -18,8 +18,8 @@ from contextlib import contextmanager from datetime import datetime, timedelta, timezone from typing import List, Optional -from unittest.mock import call, Mock, patch -from uuid import uuid4 +from unittest.mock import ANY, call, Mock, patch +from uuid import UUID, uuid4 import pytest from flask import current_app @@ -697,7 +697,7 @@ def test_email_chart_report_schedule_alpha_owner( # setup screenshot mock username = "" - def _screenshot_side_effect(user: User) -> Optional[bytes]: + def _screenshot_side_effect(user: User, **kwargs) -> Optional[bytes]: nonlocal username username = user.username @@ -781,7 +781,9 @@ def test_email_chart_report_schedule_force_screenshot( ) @patch("superset.reports.notifications.email.send_email_smtp") @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") +@patch("superset.utils.webdriver.logger.debug") def test_email_chart_alert_schedule( + logger_mock, screenshot_mock, email_mock, create_alert_email_chart, @@ -812,6 +814,12 @@ def test_email_chart_alert_schedule( # Assert logs are correct assert_log(ReportState.SUCCESS) + screenshot_mock.assert_called_with( + user=create_alert_email_chart.owners[0], execution_id=ANY + ) + + logger_mock.assert_called_with("foo") + @pytest.mark.usefixtures( "load_birth_names_dashboard_with_slices", "create_report_email_chart" diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index efa0d73cb49f0..b142afcc5a308 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -89,7 +89,9 @@ def test_not_call_find_unexpected_errors_if_feature_disabled( app.config["THUMBNAIL_SELENIUM_USER"] ) url = get_url_path("Superset.dashboard", dashboard_id_or_slug=1) - webdriver_proxy.get_screenshot(url, "grid-container", user=user) + webdriver_proxy.get_screenshot( + url=url, element_name="grid-container", user=user + ) assert not mock_find_unexpected_errors.called @@ -105,7 +107,9 @@ def test_call_find_unexpected_errors_if_feature_enabled( app.config["THUMBNAIL_SELENIUM_USER"] ) url = get_url_path("Superset.dashboard", dashboard_id_or_slug=1) - webdriver_proxy.get_screenshot(url, "grid-container", user=user) + webdriver_proxy.get_screenshot( + url=url, element_name="grid-container", user=user + ) assert mock_find_unexpected_errors.called @@ -155,7 +159,7 @@ def test_screenshot_selenium_headstart( ) url = get_url_path("Superset.slice", slice_id=1, standalone="true") app.config["SCREENSHOT_SELENIUM_HEADSTART"] = 5 - webdriver.get_screenshot(url, "chart-container", user=user) + webdriver.get_screenshot(url=url, element_name="chart-container", user=user) assert mock_sleep.call_args_list[0] == call(5) @patch("superset.utils.webdriver.WebDriverWait") @@ -167,7 +171,7 @@ def test_screenshot_selenium_locate_wait(self, mock_webdriver, mock_webdriver_wa app.config["THUMBNAIL_SELENIUM_USER"] ) url = get_url_path("Superset.slice", slice_id=1, standalone="true") - webdriver.get_screenshot(url, "chart-container", user=user) + webdriver.get_screenshot(url=url, element_name="chart-container", user=user) assert mock_webdriver_wait.call_args_list[0] == call(ANY, 15) @patch("superset.utils.webdriver.WebDriverWait") @@ -179,7 +183,7 @@ def test_screenshot_selenium_load_wait(self, mock_webdriver, mock_webdriver_wait app.config["THUMBNAIL_SELENIUM_USER"] ) url = get_url_path("Superset.slice", slice_id=1, standalone="true") - webdriver.get_screenshot(url, "chart-container", user=user) + webdriver.get_screenshot(url=url, element_name="chart-container", user=user) assert mock_webdriver_wait.call_args_list[2] == call(ANY, 15) @patch("superset.utils.webdriver.WebDriverWait") @@ -194,9 +198,47 @@ def test_screenshot_selenium_animation_wait( ) url = get_url_path("Superset.slice", slice_id=1, standalone="true") app.config["SCREENSHOT_SELENIUM_ANIMATION_WAIT"] = 4 - webdriver.get_screenshot(url, "chart-container", user=user) + webdriver.get_screenshot(url=url, element_name="chart-container", user=user) assert mock_sleep.call_args_list[1] == call(4) + @patch("superset.utils.webdriver.WebDriverWait") + @patch("superset.utils.webdriver.firefox") + @patch("superset.utils.webdriver.logger") + def test_screenshot_logging(self, logger_mock, mock_webdriver, mock_webdriver_wait): + webdriver = WebDriverProxy("firefox") + user = security_manager.get_user_by_username( + app.config["THUMBNAIL_SELENIUM_USER"] + ) + url = get_url_path("Superset.slice", slice_id=1, standalone="true") + webdriver.get_screenshot(url=url, element_name="chart-container", user=user) + logger_mock.info.assert_called_with( + "Taking a PNG screenshot of url %s as user %s with execution_id: %s", + "http://0.0.0.0:8081/superset/slice/1/?standalone=true", + "admin", + None, + ) + + @patch("superset.utils.webdriver.WebDriverWait") + @patch("superset.utils.webdriver.firefox") + @patch("superset.utils.webdriver.logger") + def test_screenshot_logging_with_execution_id( + self, logger_mock, mock_webdriver, mock_webdriver_wait + ): + webdriver = WebDriverProxy("firefox") + user = security_manager.get_user_by_username( + app.config["THUMBNAIL_SELENIUM_USER"] + ) + url = get_url_path("Superset.slice", slice_id=1, standalone="true") + webdriver.get_screenshot( + url=url, element_name="chart-container", user=user, execution_id="12345" + ) + logger_mock.info.assert_called_with( + "Taking a PNG screenshot of url %s as user %s with execution_id: %s", + "http://0.0.0.0:8081/superset/slice/1/?standalone=true", + "admin", + "12345", + ) + class TestThumbnails(SupersetTestCase): diff --git a/tests/unit_tests/notifications/email_tests.py b/tests/unit_tests/notifications/email_tests.py index 4ce34b99cac4d..4d887596fb4de 100644 --- a/tests/unit_tests/notifications/email_tests.py +++ b/tests/unit_tests/notifications/email_tests.py @@ -14,6 +14,8 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +import uuid + import pandas as pd @@ -41,6 +43,7 @@ def test_render_description_with_html() -> None: "notification_source": None, "chart_id": None, "dashboard_id": None, + "execution_id": uuid.uuid4(), }, ) email_body = ( From 3583df1ea0785fd12a457015e5076166b4a58390 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 3 Mar 2023 15:07:02 -0800 Subject: [PATCH 2/2] pass execution_id as extra --- superset/reports/commands/execute.py | 10 ++- superset/reports/notifications/email.py | 4 +- superset/reports/notifications/slack.py | 4 +- superset/utils/screenshots.py | 14 ++-- superset/utils/webdriver.py | 6 +- .../execute_dashboard_report_tests.py | 22 +++-- .../reports/commands_tests.py | 33 +++++++- tests/integration_tests/thumbnails_tests.py | 25 ++++-- tests/unit_tests/notifications/email_tests.py | 43 ++++++++++ tests/unit_tests/notifications/slack_tests.py | 84 +++++++++++++++++++ 10 files changed, 212 insertions(+), 33 deletions(-) create mode 100644 tests/unit_tests/notifications/slack_tests.py diff --git a/superset/reports/commands/execute.py b/superset/reports/commands/execute.py index 849a987cf4d51..72d2f9312d9c9 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -719,9 +719,11 @@ def run(self) -> None: user = security_manager.find_user(username) with override_user(user): logger.info( - "Running report schedule %s as user %s", - self._execution_id, + "Running report schedule as user %s", username, + extra={ + "execution_id": self._execution_id, + }, ) ReportScheduleStateMachine( session, self._execution_id, self._model, self._scheduled_dttm @@ -736,9 +738,9 @@ def validate( # pylint: disable=arguments-differ ) -> None: # Validate/populate model exists logger.info( - "session is validated: id %s, executionid: %s", + "session is validated: id %s", self._model_id, - self._execution_id, + extra={"execution_id": self._execution_id}, ) self._model = ( session.query(ReportSchedule).filter_by(id=self._model_id).one_or_none() diff --git a/superset/reports/notifications/email.py b/superset/reports/notifications/email.py index f2c7fcaadacc1..e002ca1d7729f 100644 --- a/superset/reports/notifications/email.py +++ b/superset/reports/notifications/email.py @@ -204,8 +204,8 @@ def send(self) -> None: header_data=content.header_data, ) logger.info( - "Report sent to email. Execution_id is %s", - execution_id, + "Report sent to email", + extra={"execution_id": execution_id}, ) except SupersetErrorsException as ex: raise NotificationError( diff --git a/superset/reports/notifications/slack.py b/superset/reports/notifications/slack.py index 14e18910c6d5a..12a64c701e2c5 100644 --- a/superset/reports/notifications/slack.py +++ b/superset/reports/notifications/slack.py @@ -181,8 +181,8 @@ def send(self) -> None: else: client.chat_postMessage(channel=channel, text=body) logger.info( - "Report sent to slack. Execution id is %s", - self._content.header_data["execution_id"], + "Report sent to slack", + extra={"execution_id": self._content.header_data["execution_id"]}, ) except ( BotUserAccessError, diff --git a/superset/utils/screenshots.py b/superset/utils/screenshots.py index 5e929ee71bd75..acf5772cb512a 100644 --- a/superset/utils/screenshots.py +++ b/superset/utils/screenshots.py @@ -18,7 +18,7 @@ import logging from io import BytesIO -from typing import cast, Optional, TYPE_CHECKING, TypedDict, Union +from typing import Optional, TYPE_CHECKING, TypedDict, Union from uuid import UUID from flask import current_app @@ -219,13 +219,15 @@ class ChartScreenshot(BaseScreenshot): thumbnail_type: str = "chart" element: str = "chart-container" - def __init__(self, url: str, digest: str, *args: ScreenshotDetails): + def __init__( + self, url: str, digest: str, options: Optional[ScreenshotDetails] = None + ): # Chart reports and thumbnails are in standalone="true" mode url = modify_url_query( url, standalone=ChartStandaloneMode.HIDE_NAV.value, ) - options = cast(ScreenshotDetails, {*args}) + options = options or {} super().__init__(url, digest, options.get("execution_id")) self.window_size = options.get("window_size") or (800, 600) self.thumb_size = options.get("thumb_size") or (800, 600) @@ -235,14 +237,16 @@ class DashboardScreenshot(BaseScreenshot): thumbnail_type: str = "dashboard" element: str = "standalone" - def __init__(self, url: str, digest: str, *args: ScreenshotDetails): + def __init__( + self, url: str, digest: str, options: Optional[ScreenshotDetails] = None + ): # per the element above, dashboard screenshots # should always capture in standalone url = modify_url_query( url, standalone=DashboardStandaloneMode.REPORT.value, ) - options = cast(ScreenshotDetails, {*args}) + options = options or {} super().__init__(url, digest, options.get("execution_id")) self.window_size = options.get("window_size") or (1600, 1200) self.thumb_size = options.get("thumb_size") or (800, 600) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index ebd491defa756..ccb7b4e4ddcbb 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -202,11 +202,11 @@ def get_screenshot( ] logger.debug("Wait %i seconds for chart animation", selenium_animation_wait) sleep(selenium_animation_wait) - logger.info( - "Taking a PNG screenshot of url %s as user %s with execution_id: %s", + logger.debug( + "Taking a PNG screenshot of url %s as user %s", url, user.username, - execution_id, + extra={"execution_id": execution_id}, ) if current_app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"]: diff --git a/tests/integration_tests/reports/commands/execute_dashboard_report_tests.py b/tests/integration_tests/reports/commands/execute_dashboard_report_tests.py index 0027738fd9807..acfb3e495f231 100644 --- a/tests/integration_tests/reports/commands/execute_dashboard_report_tests.py +++ b/tests/integration_tests/reports/commands/execute_dashboard_report_tests.py @@ -53,8 +53,9 @@ def test_report_for_dashboard_with_tabs( name="test report tabbed dashboard", ) as report_schedule: dashboard: Dashboard = report_schedule.dashboard + execution_id = uuid4() AsyncExecuteReportScheduleCommand( - str(uuid4()), report_schedule.id, datetime.utcnow() + str(execution_id), report_schedule.id, datetime.utcnow() ).run() dashboard_state = report_schedule.extra.get("dashboard", {}) permalink_key = CreateDashboardPermalinkCommand( @@ -62,10 +63,15 @@ def test_report_for_dashboard_with_tabs( ).run() assert dashboard_screenshot_mock.call_count == 1 - (url, digest) = dashboard_screenshot_mock.call_args.args + (url, digest, options) = dashboard_screenshot_mock.call_args.args assert url.endswith(f"/superset/dashboard/p/{permalink_key}/") assert digest == dashboard.digest assert send_email_smtp_mock.call_count == 1 + assert options == { + "execution_id": execution_id, + "thumb_size": (1400, 2000), + "window_size": (1400, 2000), + } assert len(send_email_smtp_mock.call_args.kwargs["images"]) == 1 @@ -92,8 +98,9 @@ def test_report_with_header_data( name="test report tabbed dashboard", ) as report_schedule: dashboard: Dashboard = report_schedule.dashboard + execution_id = uuid4() AsyncExecuteReportScheduleCommand( - str(uuid4()), report_schedule.id, datetime.utcnow() + str(execution_id), report_schedule.id, datetime.utcnow() ).run() dashboard_state = report_schedule.extra.get("dashboard", {}) permalink_key = CreateDashboardPermalinkCommand( @@ -101,13 +108,18 @@ def test_report_with_header_data( ).run() assert dashboard_screenshot_mock.call_count == 1 - (url, digest) = dashboard_screenshot_mock.call_args.args + (url, digest, options) = dashboard_screenshot_mock.call_args.args assert url.endswith(f"/superset/dashboard/p/{permalink_key}/") assert digest == dashboard.digest + assert options == { + "execution_id": execution_id, + "thumb_size": (1400, 2000), + "window_size": (1400, 2000), + } assert send_email_smtp_mock.call_count == 1 header_data = send_email_smtp_mock.call_args.kwargs["header_data"] assert header_data.get("dashboard_id") == dashboard.id 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 diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 70ee90078db23..71f11161cd740 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -814,11 +814,36 @@ def test_email_chart_alert_schedule( # Assert logs are correct assert_log(ReportState.SUCCESS) - screenshot_mock.assert_called_with( - user=create_alert_email_chart.owners[0], execution_id=ANY - ) + screenshot_mock.assert_called_with(user=create_alert_email_chart.owners[0]) + - logger_mock.assert_called_with("foo") +@pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_alert_email_chart" +) +@patch("superset.reports.notifications.email.send_email_smtp") +@patch("superset.utils.webdriver.logger.debug") +def test_email_chart_screenshot( + logger_mock, + email_mock, + create_alert_email_chart, +): + """ + ExecuteReport Command: Test chart email alert schedule with screenshot + """ + # setup screenshot mock + from superset.utils.webdriver import WebDriverProxy + + with freeze_time("2020-01-01T00:00:00Z"): + with patch.object(WebDriverProxy, "get_screenshot") as screenshot_mock: + AsyncExecuteReportScheduleCommand( + TEST_ID, create_alert_email_chart.id, datetime.utcnow() + ).run() + screenshot_mock.assert_called_with( + url=f"http://0.0.0.0:8081/explore/?form_data=%7B%22slice_id%22%3A%20{create_alert_email_chart.chart_id}%7D&force=true&standalone=true", + element_name="chart-container", + user=create_alert_email_chart.owners[0], + execution_id=UUID(TEST_ID), + ) @pytest.mark.usefixtures( diff --git a/tests/integration_tests/thumbnails_tests.py b/tests/integration_tests/thumbnails_tests.py index b142afcc5a308..fde75832db259 100644 --- a/tests/integration_tests/thumbnails_tests.py +++ b/tests/integration_tests/thumbnails_tests.py @@ -19,6 +19,7 @@ import json import urllib.request +import uuid from io import BytesIO from typing import Tuple from unittest import skipUnless @@ -210,12 +211,16 @@ def test_screenshot_logging(self, logger_mock, mock_webdriver, mock_webdriver_wa app.config["THUMBNAIL_SELENIUM_USER"] ) url = get_url_path("Superset.slice", slice_id=1, standalone="true") - webdriver.get_screenshot(url=url, element_name="chart-container", user=user) - logger_mock.info.assert_called_with( - "Taking a PNG screenshot of url %s as user %s with execution_id: %s", + webdriver.get_screenshot( + url=url, + element_name="chart-container", + user=user, + ) + logger_mock.debug.assert_called_with( + "Taking a PNG screenshot of url %s as user %s", "http://0.0.0.0:8081/superset/slice/1/?standalone=true", "admin", - None, + extra={"execution_id": None}, ) @patch("superset.utils.webdriver.WebDriverWait") @@ -229,14 +234,18 @@ def test_screenshot_logging_with_execution_id( app.config["THUMBNAIL_SELENIUM_USER"] ) url = get_url_path("Superset.slice", slice_id=1, standalone="true") + execution_id = uuid.uuid4() webdriver.get_screenshot( - url=url, element_name="chart-container", user=user, execution_id="12345" + url=url, + element_name="chart-container", + user=user, + execution_id=execution_id, ) - logger_mock.info.assert_called_with( - "Taking a PNG screenshot of url %s as user %s with execution_id: %s", + logger_mock.debug.assert_called_with( + "Taking a PNG screenshot of url %s as user %s", "http://0.0.0.0:8081/superset/slice/1/?standalone=true", "admin", - "12345", + extra={"execution_id": execution_id}, ) diff --git a/tests/unit_tests/notifications/email_tests.py b/tests/unit_tests/notifications/email_tests.py index 4d887596fb4de..3fae3dde743c0 100644 --- a/tests/unit_tests/notifications/email_tests.py +++ b/tests/unit_tests/notifications/email_tests.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. import uuid +from unittest.mock import MagicMock, patch import pandas as pd @@ -55,3 +56,45 @@ def test_render_description_with_html() -> None: ) assert '

This is a test alert


' in email_body assert '<a href="http://www.example.com">333</a>' in email_body + + +@patch("superset.reports.notifications.email.send_email_smtp") +@patch("superset.reports.notifications.email.logger") +def test_send_email(logger_mock: MagicMock, send_email_mock: MagicMock) -> None: + # `superset.models.helpers`, a dependency of following imports, + # requires app context + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.email import EmailNotification + + execution_id = uuid.uuid4() + content = NotificationContent( + name="test alert", + embedded_data=pd.DataFrame( + { + "A": [1, 2, 3], + "B": [4, 5, 6], + "C": ["111", "222", '333'], + } + ), + description='

This is a test alert


', + header_data={ + "notification_format": "PNG", + "notification_type": "Alert", + "owners": [1], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "execution_id": execution_id, + }, + ) + EmailNotification( + recipient=ReportRecipients( + type=ReportRecipientType.EMAIL, + recipient_config_json='{"target": "foo@bar.com"}', + ), + content=content, + ).send() + logger_mock.info.assert_called_with( + "Report sent to email", extra={"execution_id": execution_id} + ) diff --git a/tests/unit_tests/notifications/slack_tests.py b/tests/unit_tests/notifications/slack_tests.py new file mode 100644 index 0000000000000..6b5cfd2a9ca68 --- /dev/null +++ b/tests/unit_tests/notifications/slack_tests.py @@ -0,0 +1,84 @@ +# 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. +import uuid +from unittest.mock import MagicMock, patch + +import pandas as pd + + +@patch("superset.reports.notifications.slack.logger") +def test_send_slack( + logger_mock: MagicMock, +) -> None: + # `superset.models.helpers`, a dependency of following imports, + # requires app context + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.slack import SlackNotification, WebClient + + execution_id = uuid.uuid4() + content = NotificationContent( + name="test alert", + embedded_data=pd.DataFrame( + { + "A": [1, 2, 3], + "B": [4, 5, 6], + "C": ["111", "222", '333'], + } + ), + description='

This is a test alert


', + header_data={ + "notification_format": "PNG", + "notification_type": "Alert", + "owners": [1], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "execution_id": execution_id, + }, + ) + with patch.object( + WebClient, "chat_postMessage", return_value=True + ) as chat_post_message_mock: + + SlackNotification( + recipient=ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json='{"target": "some_channel"}', + ), + content=content, + ).send() + logger_mock.info.assert_called_with( + "Report sent to slack", extra={"execution_id": execution_id} + ) + chat_post_message_mock.assert_called_with( + channel="some_channel", + text="""*test alert* + +

This is a test alert


+ + + +``` +| | A | B | C | +|---:|----:|----:|:-----------------------------------------| +| 0 | 1 | 4 | 111 | +| 1 | 2 | 5 | 222 | +| 2 | 3 | 6 | 333 | +``` +""", + )