From 2cd80543581155225f2b538ad8cd5ebc7de5a9ff Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 22 Dec 2021 12:16:04 -0800 Subject: [PATCH] feat: add force option to report screenshots (#17853) * Update existing tests * Add backend test * feat: add force option to report screenshots * Add tests * Rebase fixes * Do not force screenshot on dashboard alerts --- .../src/components/ReportModal/index.tsx | 2 + .../src/views/CRUD/alert/AlertReportModal.tsx | 2 + ..._add_force_screenshot_to_alerts_reports.py | 64 +++++++++++++++++++ superset/models/reports.py | 3 + superset/reports/commands/execute.py | 10 +-- superset/reports/schemas.py | 2 + .../reports/commands_tests.py | 58 +++++++++++++++++ tests/integration_tests/reports/utils.py | 2 + 8 files changed, 135 insertions(+), 8 deletions(-) create mode 100644 superset/migrations/versions/bb38f40aa3ff_add_force_screenshot_to_alerts_reports.py diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index 178e394125746..fc5469d17c174 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -71,6 +71,7 @@ export interface ReportObject { validator_type: string; working_timeout: number; creation_method: string; + force_screenshot: boolean; } interface ChartObject { @@ -227,6 +228,7 @@ const ReportModal: FunctionComponent = ({ active: true, report_format: currentReport?.report_format || defaultNotificationFormat, timezone: currentReport?.timezone, + force_screenshot: false, }; if (isEditMode) { diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx index 5dfeeecfafc9f..84021484fe466 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx +++ b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx @@ -154,6 +154,7 @@ const DEFAULT_ALERT = { sql: '', validator_config_json: {}, validator_type: '', + force_screenshot: false, grace_period: undefined, }; @@ -512,6 +513,7 @@ const AlertReportModal: FunctionComponent = ({ const data: any = { ...currentAlert, type: isReport ? 'Report' : 'Alert', + force_screenshot: contentType === 'chart' && !isReport ? 'true' : 'false', validator_type: conditionNotNull ? 'not null' : 'operator', validator_config_json: conditionNotNull ? {} diff --git a/superset/migrations/versions/bb38f40aa3ff_add_force_screenshot_to_alerts_reports.py b/superset/migrations/versions/bb38f40aa3ff_add_force_screenshot_to_alerts_reports.py new file mode 100644 index 0000000000000..cc698cb8a4987 --- /dev/null +++ b/superset/migrations/versions/bb38f40aa3ff_add_force_screenshot_to_alerts_reports.py @@ -0,0 +1,64 @@ +# 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. +"""Add force_screenshot to alerts/reports + +Revision ID: bb38f40aa3ff +Revises: 31bb738bd1d2 +Create Date: 2021-12-10 19:25:29.802949 + +""" + +# revision identifiers, used by Alembic. +revision = "bb38f40aa3ff" +down_revision = "31bb738bd1d2" + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.ext.declarative import declarative_base + +from superset import db + +Base = declarative_base() + + +class ReportSchedule(Base): + __tablename__ = "report_schedule" + + id = sa.Column(sa.Integer, primary_key=True) + type = sa.Column(sa.String(50), nullable=False) + force_screenshot = sa.Column(sa.Boolean, default=False) + + +def upgrade(): + with op.batch_alter_table("report_schedule") as batch_op: + batch_op.add_column(sa.Column("force_screenshot", sa.Boolean(), default=False)) + + bind = op.get_bind() + session = db.Session(bind=bind) + + for report in session.query(ReportSchedule).all(): + # Update existing alerts that send chart screenshots so that the cache is + # bypassed. We don't turn this one for dashboards because (1) it's currently + # not supported but also because (2) it can be very expensive. + report.force_screenshot = report.type == "Alert" and report.chart_id is not None + + session.commit() + + +def downgrade(): + with op.batch_alter_table("report_schedule") as batch_op: + batch_op.drop_column("force_screenshot") diff --git a/superset/models/reports.py b/superset/models/reports.py index a0dc59917a42c..d8ad8cb8c3467 100644 --- a/superset/models/reports.py +++ b/superset/models/reports.py @@ -148,6 +148,9 @@ class ReportSchedule(Model, AuditMixinNullable): # Store the selected dashboard tabs etc. extra = Column(Text, default="{}") + # (Reports) When generating a screenshot, bypass the cache? + force_screenshot = Column(Boolean, default=False) + def __repr__(self) -> str: return str(self.name) diff --git a/superset/reports/commands/execute.py b/superset/reports/commands/execute.py index 2789bb20fbaa1..687e05c61a004 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -148,13 +148,7 @@ def _get_url( """ # For alerts we always want to send a fresh screenshot, bypassing # the cache. - # TODO (betodealmeida): allow to specify per report if users want - # to bypass the cache as well. - force = ( - "true" - if self._report_schedule.type == ReportScheduleType.ALERT - else "false" - ) + force = "true" if self._report_schedule.force_screenshot else "false" if self._report_schedule.chart: if result_format in { @@ -181,7 +175,7 @@ def _get_url( user_friendly=user_friendly, dashboard_id_or_slug=self._report_schedule.dashboard_id, standalone=DashboardStandaloneMode.REPORT.value, - force=force, + # force=force, TODO (betodealmeida) implement this **kwargs, ) diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 3f2fb4416dbe0..f4c85484aa149 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -202,6 +202,7 @@ class ReportSchedulePostSchema(Schema): default=ReportDataFormat.VISUALIZATION, validate=validate.OneOf(choices=tuple(key.value for key in ReportDataFormat)), ) + force_screenshot = fields.Boolean(default=False) @validates_schema def validate_report_references( # pylint: disable=unused-argument,no-self-use @@ -292,3 +293,4 @@ class ReportSchedulePutSchema(Schema): default=ReportDataFormat.VISUALIZATION, validate=validate.OneOf(choices=tuple(key.value for key in ReportDataFormat)), ) + force_screenshot = fields.Boolean(default=False) diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 0e455b30dd9b4..9aa2ca461bea0 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -135,6 +135,7 @@ def create_report_notification( grace_period: Optional[int] = None, report_format: Optional[ReportDataFormat] = None, name: Optional[str] = None, + force_screenshot: bool = False, ) -> ReportSchedule: report_type = report_type or ReportScheduleType.REPORT target = email_target or slack_channel @@ -174,6 +175,7 @@ def create_report_notification( validator_config_json=validator_config_json, grace_period=grace_period, report_format=report_format or ReportDataFormat.VISUALIZATION, + force_screenshot=force_screenshot, ) return report_schedule @@ -218,6 +220,18 @@ def create_report_email_chart(): cleanup_report_schedule(report_schedule) +@pytest.fixture() +def create_report_email_chart_force_screenshot(): + with app.app_context(): + chart = db.session.query(Slice).first() + report_schedule = create_report_notification( + email_target="target@email.com", chart=chart, force_screenshot=True + ) + yield report_schedule + + cleanup_report_schedule(report_schedule) + + @pytest.fixture() def create_report_email_chart_with_csv(): with app.app_context(): @@ -480,6 +494,7 @@ def create_alert_email_chart(request): validator_config_json=param_config[request.param][ "validator_config_json" ], + force_screenshot=True, ) yield report_schedule @@ -678,6 +693,49 @@ def test_email_chart_report_schedule( assert_log(ReportState.SUCCESS) +@pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", + "create_report_email_chart_force_screenshot", +) +@patch("superset.reports.notifications.email.send_email_smtp") +@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") +def test_email_chart_report_schedule_force_screenshot( + screenshot_mock, email_mock, create_report_email_chart_force_screenshot, +): + """ + ExecuteReport Command: Test chart email report schedule with screenshot + + In this test ``force_screenshot`` is true, and the screenshot URL should + reflect that. + """ + # setup screenshot mock + screenshot_mock.return_value = SCREENSHOT_FILE + + with freeze_time("2020-01-01T00:00:00Z"): + AsyncExecuteReportScheduleCommand( + TEST_ID, create_report_email_chart_force_screenshot.id, datetime.utcnow() + ).run() + + notification_targets = get_target_from_report_schedule( + create_report_email_chart_force_screenshot + ) + # assert that the link sent is correct + assert ( + '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] + # Assert the email inline screenshot + smtp_images = email_mock.call_args[1]["images"] + assert smtp_images[list(smtp_images.keys())[0]] == SCREENSHOT_FILE + # Assert logs are correct + assert_log(ReportState.SUCCESS) + + @pytest.mark.usefixtures( "load_birth_names_dashboard_with_slices", "create_alert_email_chart" ) diff --git a/tests/integration_tests/reports/utils.py b/tests/integration_tests/reports/utils.py index 6cf55a1c1f6ed..2adf9cc3c0ee5 100644 --- a/tests/integration_tests/reports/utils.py +++ b/tests/integration_tests/reports/utils.py @@ -51,6 +51,7 @@ def insert_report_schedule( recipients: Optional[List[ReportRecipients]] = None, report_format: Optional[ReportDataFormat] = None, logs: Optional[List[ReportExecutionLog]] = None, + force_screenshot: bool = False, ) -> ReportSchedule: owners = owners or [] recipients = recipients or [] @@ -75,6 +76,7 @@ def insert_report_schedule( logs=logs, last_state=last_state, report_format=report_format, + force_screenshot=force_screenshot, ) db.session.add(report_schedule) db.session.commit()