From 4ddf67fc143f585d00a99510a6a914885dfd9a89 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Wed, 15 Feb 2023 14:35:08 -0800 Subject: [PATCH] chore: move dashboard screenshot standalone logic (#23003) --- superset/charts/api.py | 4 +- superset/cli/thumbnails.py | 9 +---- superset/reports/commands/execute.py | 3 -- superset/reports/notifications/email.py | 8 +--- superset/reports/notifications/slack.py | 8 +--- superset/tasks/thumbnails.py | 2 +- superset/utils/screenshots.py | 20 +++++++++- superset/utils/urls.py | 4 +- superset/utils/webdriver.py | 5 +++ tests/integration_tests/cli_tests.py | 24 ++++++++++- .../reports/commands_tests.py | 40 +++++++------------ tests/unit_tests/utils/urls_tests.py | 5 +++ 12 files changed, 76 insertions(+), 56 deletions(-) diff --git a/superset/charts/api.py b/superset/charts/api.py index e9ba4feddb8b8..7dc6d5e1e8d93 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -560,7 +560,7 @@ def cache_screenshot(self, pk: int, **kwargs: Any) -> WerkzeugResponse: if not chart: return self.response_404() - chart_url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") + chart_url = get_url_path("Superset.slice", slice_id=chart.id) screenshot_obj = ChartScreenshot(chart_url, chart.digest) cache_key = screenshot_obj.cache_key(window_size, thumb_size) image_url = get_url_path( @@ -683,7 +683,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) -> WerkzeugResponse: return self.response_404() current_user = get_current_user() - url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") + url = get_url_path("Superset.slice", slice_id=chart.id) if kwargs["rison"].get("force", False): logger.info( "Triggering thumbnail compute (chart id: %s) ASYNC", str(chart.id) diff --git a/superset/cli/thumbnails.py b/superset/cli/thumbnails.py index 5556cff92f620..276d9981c1ec2 100755 --- a/superset/cli/thumbnails.py +++ b/superset/cli/thumbnails.py @@ -22,7 +22,6 @@ from flask.cli import with_appcontext from superset.extensions import db -from superset.utils.urls import get_url_path logger = logging.getLogger(__name__) @@ -94,13 +93,7 @@ def compute_generic_thumbnail( action = "Processing" msg = f'{action} {friendly_type} "{model}" ({i+1}/{count})' click.secho(msg, fg="green") - if friendly_type == "chart": - url = get_url_path( - "Superset.slice", slice_id=model.id, standalone="true" - ) - else: - url = get_url_path("Superset.dashboard", dashboard_id_or_slug=model.id) - func(url, model.digest, force=force) + func(None, model.id, force=force) if not charts_only: compute_generic_thumbnail( diff --git a/superset/reports/commands/execute.py b/superset/reports/commands/execute.py index 8133cec29b971..c8edd928b4784 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -75,7 +75,6 @@ from superset.utils.csv import get_chart_csv_data, get_chart_dataframe from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot from superset.utils.urls import get_url_path -from superset.utils.webdriver import DashboardStandaloneMode logger = logging.getLogger(__name__) @@ -172,7 +171,6 @@ def _get_url( "ExploreView.root", user_friendly=user_friendly, form_data=json.dumps({"slice_id": self._report_schedule.chart_id}), - standalone="true", force=force, **kwargs, ) @@ -190,7 +188,6 @@ def _get_url( "Superset.dashboard", user_friendly=user_friendly, dashboard_id_or_slug=self._report_schedule.dashboard_id, - standalone=DashboardStandaloneMode.REPORT.value, force=force, **kwargs, ) diff --git a/superset/reports/notifications/email.py b/superset/reports/notifications/email.py index 4b061c8ef6b69..22b1714f99fed 100644 --- a/superset/reports/notifications/email.py +++ b/superset/reports/notifications/email.py @@ -32,7 +32,6 @@ from superset.reports.notifications.exceptions import NotificationError from superset.utils.core import HeaderDataType, send_email_smtp from superset.utils.decorators import statsd_gauge -from superset.utils.urls import modify_url_query logger = logging.getLogger(__name__) @@ -129,11 +128,6 @@ def _get_content(self) -> EmailContent: html_table = "" call_to_action = __(app.config["EMAIL_REPORTS_CTA"]) - url = ( - modify_url_query(self._content.url, standalone="0") - if self._content.url is not None - else "" - ) img_tags = [] for msgid in images.keys(): img_tags.append( @@ -162,7 +156,7 @@ def _get_content(self) -> EmailContent:
{description}

- {call_to_action}

+ {call_to_action}

{html_table} {img_tag} diff --git a/superset/reports/notifications/slack.py b/superset/reports/notifications/slack.py index 91470c5b02736..b89a700ef9c3e 100644 --- a/superset/reports/notifications/slack.py +++ b/superset/reports/notifications/slack.py @@ -44,7 +44,6 @@ NotificationUnprocessableException, ) from superset.utils.decorators import statsd_gauge -from superset.utils.urls import modify_url_query logger = logging.getLogger(__name__) @@ -63,11 +62,6 @@ def _get_channel(self) -> str: return json.loads(self._recipient.recipient_config_json)["target"] def _message_template(self, table: str = "") -> str: - url = ( - modify_url_query(self._content.url, standalone="0") - if self._content.url is not None - else "" - ) return __( """*%(name)s* @@ -79,7 +73,7 @@ def _message_template(self, table: str = "") -> str: """, name=self._content.name, description=self._content.description or "", - url=url, + url=self._content.url, table=table, ) diff --git a/superset/tasks/thumbnails.py b/superset/tasks/thumbnails.py index c03d13b0bdbeb..d76939a07e3a0 100644 --- a/superset/tasks/thumbnails.py +++ b/superset/tasks/thumbnails.py @@ -48,7 +48,7 @@ def cache_chart_thumbnail( logger.warning("No cache set, refusing to compute") return None chart = cast(Slice, Slice.get(chart_id)) - url = get_url_path("Superset.slice", slice_id=chart.id, standalone="true") + url = get_url_path("Superset.slice", slice_id=chart.id) logger.info("Caching chart: %s", url) _, username = get_executor( executor_types=current_app.config["THUMBNAIL_EXECUTE_AS"], diff --git a/superset/utils/screenshots.py b/superset/utils/screenshots.py index 741bc9ea5fa93..a904b7dc4384a 100644 --- a/superset/utils/screenshots.py +++ b/superset/utils/screenshots.py @@ -23,7 +23,13 @@ from flask import current_app from superset.utils.hashing import md5_sha_from_dict -from superset.utils.webdriver import WebDriverProxy, WindowSize +from superset.utils.urls import modify_url_query +from superset.utils.webdriver import ( + ChartStandaloneMode, + DashboardStandaloneMode, + WebDriverProxy, + WindowSize, +) logger = logging.getLogger(__name__) @@ -205,6 +211,11 @@ def __init__( window_size: Optional[WindowSize] = None, thumb_size: Optional[WindowSize] = None, ): + # Chart reports 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) @@ -221,6 +232,13 @@ def __init__( window_size: Optional[WindowSize] = None, thumb_size: Optional[WindowSize] = None, ): + # 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) diff --git a/superset/utils/urls.py b/superset/utils/urls.py index c31bfb1a5103c..31fbd89337af4 100644 --- a/superset/utils/urls.py +++ b/superset/utils/urls.py @@ -48,7 +48,9 @@ def modify_url_query(url: str, **kwargs: Any) -> str: v = [v] params[k] = v - parts[3] = "&".join(f"{k}={urllib.parse.quote(v[0])}" for k, v in params.items()) + parts[3] = "&".join( + f"{k}={urllib.parse.quote(str(v[0]))}" for k, v in params.items() + ) return urllib.parse.urlunsplit(parts) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index b1ba784281082..e678587029dbc 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -51,6 +51,11 @@ class DashboardStandaloneMode(Enum): REPORT = 3 +class ChartStandaloneMode(Enum): + HIDE_NAV = "true" + SHOW_NAV = 0 + + def find_unexpected_errors(driver: WebDriver) -> List[str]: error_messages = [] diff --git a/tests/integration_tests/cli_tests.py b/tests/integration_tests/cli_tests.py index f69efc6253943..aaa682bee0f3e 100644 --- a/tests/integration_tests/cli_tests.py +++ b/tests/integration_tests/cli_tests.py @@ -27,7 +27,9 @@ from freezegun import freeze_time import superset.cli.importexport -from superset import app +import superset.cli.thumbnails +from superset import app, db +from superset.models.dashboard import Dashboard from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, load_birth_names_data, @@ -495,3 +497,23 @@ def test_failing_import_datasets_versioned_export( ) assert_cli_fails_properly(response, caplog) + + +@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") +@mock.patch("superset.tasks.thumbnails.cache_dashboard_thumbnail") +def test_compute_thumbnails(thumbnail_mock, app_context, fs): + + thumbnail_mock.return_value = None + runner = app.test_cli_runner() + dashboard = db.session.query(Dashboard).filter_by(slug="births").first() + response = runner.invoke( + superset.cli.thumbnails.compute_thumbnails, + ["-d", "-i", dashboard.id], + ) + + thumbnail_mock.assert_called_with( + None, + dashboard.id, + force=False, + ) + assert response.exit_code == 0 diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index eb606ffec3ee8..8d6a76c14f67e 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -663,11 +663,9 @@ def test_email_chart_report_schedule( ) # assert that the link sent is correct assert ( - 'Explore in Superset' - in email_mock.call_args[0][2] + 'Explore in Superset' in email_mock.call_args[0][2] ) # Assert the email smtp address assert email_mock.call_args[0][0] == notification_targets[0] @@ -720,11 +718,9 @@ def _screenshot_side_effect(user: User) -> Optional[bytes]: # assert that the link sent is correct assert ( - 'Explore in Superset' - in email_mock.call_args[0][2] + 'Explore in Superset' in email_mock.call_args[0][2] ) # Assert the email smtp address assert email_mock.call_args[0][0] == notification_targets[0] @@ -767,11 +763,9 @@ def test_email_chart_report_schedule_force_screenshot( ) # assert that the link sent is correct assert ( - 'Explore in Superset' - in email_mock.call_args[0][2] + 'Explore in Superset' in email_mock.call_args[0][2] ) # Assert the email smtp address assert email_mock.call_args[0][0] == notification_targets[0] @@ -806,11 +800,9 @@ def test_email_chart_alert_schedule( notification_targets = get_target_from_report_schedule(create_alert_email_chart) # assert that the link sent is correct assert ( - 'Explore in Superset' - in email_mock.call_args[0][2] + 'Explore in Superset' in email_mock.call_args[0][2] ) # Assert the email smtp address assert email_mock.call_args[0][0] == notification_targets[0] @@ -880,11 +872,9 @@ def test_email_chart_report_schedule_with_csv( ) # assert that the link sent is correct assert ( - 'Explore in Superset' - in email_mock.call_args[0][2] + 'force=false">Explore in Superset' in email_mock.call_args[0][2] ) # Assert the email smtp address assert email_mock.call_args[0][0] == notification_targets[0] @@ -1313,7 +1303,7 @@ def test_slack_chart_report_schedule_with_text( | 1 | c21 | c22 | c23 |""" assert table_markdown in post_message_mock.call_args[1]["text"] assert ( - f"" + f"" in post_message_mock.call_args[1]["text"] ) diff --git a/tests/unit_tests/utils/urls_tests.py b/tests/unit_tests/utils/urls_tests.py index a3893953b8ba1..208d6caea4375 100644 --- a/tests/unit_tests/utils/urls_tests.py +++ b/tests/unit_tests/utils/urls_tests.py @@ -37,6 +37,11 @@ def test_convert_dashboard_link() -> None: assert test_url == "http://localhost:9000/superset/dashboard/3/?standalone=0" +def test_convert_dashboard_link_with_integer() -> None: + test_url = modify_url_query(EXPLORE_DASHBOARD_LINK, standalone=0) + assert test_url == "http://localhost:9000/superset/dashboard/3/?standalone=0" + + @pytest.mark.parametrize( "url,is_safe", [