chore: move dashboard screenshot standalone logic (#23003)

apache · Feb 15, 2023 · 4ddf67f · 4ddf67f
1 parent f6c3044
commit 4ddf67f
Show file tree

Hide file tree

Showing 12 changed files with 76 additions and 56 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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:
               <body>
                 <div>{description}</div>
                 <br>
-                <b><a href="{url}">{call_to_action}</a></b><p></p>
+                <b><a href="{self._content.url}">{call_to_action}</a></b><p></p>
                 {html_table}
                 {img_tag}
               </body>

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -663,11 +663,9 @@ def test_email_chart_report_schedule(
         )
         # assert that the link sent is correct
         assert (
-            '<a href="http://0.0.0.0:8080/explore/?'
-            "form_data=%7B%22slice_id%22%3A%20"
-            f"{create_report_email_chart.chart.id}%7D&"
-            'standalone=0&force=false">Explore in Superset</a>'
-            in email_mock.call_args[0][2]
+            '<a href="http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A+'
+            f"{create_report_email_chart.chart.id}"
+            '%7D&force=false">Explore in Superset</a>' 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 (
-            '<a href="http://0.0.0.0:8080/explore/?'
-            "form_data=%7B%22slice_id%22%3A%20"
-            f"{create_report_email_chart_alpha_owner.chart.id}%7D&"
-            'standalone=0&force=false">Explore in Superset</a>'
-            in email_mock.call_args[0][2]
+            '<a href="http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A+'
+            f"{create_report_email_chart_alpha_owner.chart.id}"
+            '%7D&force=false">Explore in Superset</a>' 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 (
-            '<a href="http://0.0.0.0:8080/explore/?'
-            "form_data=%7B%22slice_id%22%3A%20"
-            f"{create_report_email_chart_force_screenshot.chart.id}%7D&"
-            'standalone=0&force=true">Explore in Superset</a>'
-            in email_mock.call_args[0][2]
+            '<a href="http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A+'
+            f"{create_report_email_chart_force_screenshot.chart.id}"
+            '%7D&force=true">Explore in Superset</a>' 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 (
-            '<a href="http://0.0.0.0:8080/explore/?'
-            "form_data=%7B%22slice_id%22%3A%20"
-            f"{create_alert_email_chart.chart.id}%7D&"
-            'standalone=0&force=true">Explore in Superset</a>'
-            in email_mock.call_args[0][2]
+            '<a href="http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A+'
+            f"{create_alert_email_chart.chart.id}"
+            '%7D&force=true">Explore in Superset</a>' 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 (
-            '<a href="http://0.0.0.0:8080/explore/?'
-            "form_data=%7B%22slice_id%22%3A%20"
+            '<a href="http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A+'
             f"{create_report_email_chart_with_csv.chart.id}%7D&"
-            'standalone=0&force=false">Explore in Superset</a>'
-            in email_mock.call_args[0][2]
+            'force=false">Explore in Superset</a>' 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"<http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A%20{create_report_slack_chart_with_text.chart.id}%7D&standalone=0&force=false|Explore in Superset>"
+            f"<http://0.0.0.0:8080/explore/?form_data=%7B%22slice_id%22%3A+{create_report_slack_chart_with_text.chart.id}%7D&force=false|Explore in Superset>"
             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
@@ -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",
     [