Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: move dashboard screenshot standalone logic #23003

Merged
merged 1 commit into from
Feb 15, 2023
Merged

chore: move dashboard screenshot standalone logic #23003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions superset/charts/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)
Expand Down
9 changes: 1 addition & 8 deletions superset/cli/thumbnails.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cache thumbnail functions here take a user id, model Id and optional force arguments. I'm not sure why we were passing url and digest here, but I wrote a test below that asserted that we should be passing the correct args. This might be worth testing to see how this was working before.


if not charts_only:
compute_generic_thumbnail(
Expand Down
3 changes: 0 additions & 3 deletions superset/reports/commands/execute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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,
)
Expand All @@ -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,
)
Expand Down
8 changes: 1 addition & 7 deletions superset/reports/notifications/email.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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>
Expand Down
8 changes: 1 addition & 7 deletions superset/reports/notifications/slack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
NotificationUnprocessableException,
)
from superset.utils.decorators import statsd_gauge
from superset.utils.urls import modify_url_query

logger = logging.getLogger(__name__)

Expand All @@ -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*

Expand All @@ -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,
)

Expand Down
2 changes: 1 addition & 1 deletion superset/tasks/thumbnails.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
20 changes: 19 additions & 1 deletion superset/utils/screenshots.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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)
Expand All @@ -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)
4 changes: 3 additions & 1 deletion superset/utils/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when I passed an integer to this util in a new test it errored here, so I covered this case by converting the value to a string.

)
return urllib.parse.urlunsplit(parts)


Expand Down
5 changes: 5 additions & 0 deletions superset/utils/webdriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = []

Expand Down
24 changes: 23 additions & 1 deletion tests/integration_tests/cli_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
40 changes: 15 additions & 25 deletions tests/integration_tests/reports/commands_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside from removing standalone=0, which doesn't do anything in the page load, the changes also turned slice_id%22%3A%20 into slice_id%22%3A+ but when I tested this url live from a report, it still renders the chart correctly.

)
# Assert the email smtp address
assert email_mock.call_args[0][0] == notification_targets[0]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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"]
)

Expand Down
5 changes: 5 additions & 0 deletions tests/unit_tests/utils/urls_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
[
Expand Down