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.

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

feat: bypass cache on screenshots for alerts #17695

Merged
merged 4 commits into from
Dec 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions superset-frontend/src/chart/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const propTypes = {
timeout: PropTypes.number,
vizType: PropTypes.string.isRequired,
triggerRender: PropTypes.bool,
force: PropTypes.bool,
isFiltersInitialized: PropTypes.bool,
isDeactivatedViz: PropTypes.bool,
// state
Expand Down Expand Up @@ -84,6 +85,7 @@ const defaultProps = {
dashboardId: null,
chartStackTrace: null,
isDeactivatedViz: false,
force: false,
};

const Styles = styled.div`
Expand Down Expand Up @@ -143,7 +145,7 @@ class Chart extends React.PureComponent {
// Load saved chart with a GET request
this.props.actions.getSavedChart(
this.props.formData,
false,
this.props.force,
this.props.timeout,
this.props.chartId,
this.props.dashboardId,
Expand All @@ -153,7 +155,7 @@ class Chart extends React.PureComponent {
// Create chart with POST request
this.props.actions.postChartFormData(
this.props.formData,
false,
this.props.force,
this.props.timeout,
this.props.chartId,
this.props.dashboardId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const propTypes = {
form_data: PropTypes.object,
ownState: PropTypes.object,
standalone: PropTypes.number,
force: PropTypes.bool,
timeout: PropTypes.number,
refreshOverlayVisible: PropTypes.bool,
chart: chartPropShape,
Expand Down Expand Up @@ -134,7 +135,7 @@ const ExploreChartPanel = props => {
if (slice && slice.query_context === null) {
const queryContext = buildV1ChartDataPayload({
formData: slice.form_data,
force: false,
force: props.force,
resultFormat: 'json',
resultType: 'full',
setDataMask: null,
Expand Down Expand Up @@ -230,6 +231,7 @@ const ExploreChartPanel = props => {
chartId={chart.id}
chartStatus={chart.chartStatus}
triggerRender={props.triggerRender}
force={props.force}
datasource={props.datasource}
errorMessage={props.errorMessage}
formData={props.form_data}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ const propTypes = {
forcedHeight: PropTypes.string,
form_data: PropTypes.object.isRequired,
standalone: PropTypes.number.isRequired,
force: PropTypes.bool,
timeout: PropTypes.number,
impressionId: PropTypes.string,
vizType: PropTypes.string,
Expand Down Expand Up @@ -206,6 +207,8 @@ function ExploreViewContainer(props) {
formData,
props.standalone ? URL_PARAMS.standalone.name : null,
false,
{},
props.force,
);

try {
Expand All @@ -224,7 +227,7 @@ function ExploreViewContainer(props) {
);
}
},
[props.form_data, props.standalone],
[props.form_data, props.standalone, props.force],
);

const handlePopstate = useCallback(() => {
Expand All @@ -233,7 +236,7 @@ function ExploreViewContainer(props) {
props.actions.setExploreControls(formData);
props.actions.postChartFormData(
formData,
false,
props.force,
props.timeout,
props.chart.id,
);
Expand Down Expand Up @@ -643,6 +646,7 @@ function mapStateToProps(state) {
table_name: form_data.datasource_name,
vizType: form_data.viz_type,
standalone: explore.standalone,
force: explore.force,
forcedHeight: explore.forced_height,
chart,
timeout: explore.common.conf.SUPERSET_WEBSERVER_TIMEOUT,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,27 @@ test('Get url when endpointType:standalone', () => {
expect(
getExploreLongUrl(
params.formData,
params.endpointType,
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was wrong.

'standalone',
params.allowOverflow,
params.extraSearch,
),
).toBe(
'/superset/explore/?same=any-string&form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%7D',
'/superset/explore/?same=any-string&form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%7D&standalone=1',
);
});

test('Get url when endpointType:standalone and force:true', () => {
const params = createParams();
expect(
getExploreLongUrl(
params.formData,
'standalone',
params.allowOverflow,
params.extraSearch,
true,
),
).toBe(
'/superset/explore/?same=any-string&form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%7D&force=1&standalone=1',
);
});

Expand Down
16 changes: 13 additions & 3 deletions superset-frontend/src/explore/exploreUtils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ export function getExploreLongUrl(
endpointType,
allowOverflow = true,
extraSearch = {},
force = false,
) {
if (!formData.datasource) {
return null;
Expand All @@ -112,6 +113,9 @@ export function getExploreLongUrl(
});
search.form_data = safeStringify(formData);
if (endpointType === URL_PARAMS.standalone.name) {
if (force) {
search.force = '1';
}
search.standalone = DashboardStandaloneMode.HIDE_NAV;
}
const url = uri.directory(directory).search(search).toString();
Expand All @@ -120,9 +124,15 @@ export function getExploreLongUrl(
datasource: formData.datasource,
viz_type: formData.viz_type,
};
return getExploreLongUrl(minimalFormData, endpointType, false, {
URL_IS_TOO_LONG_TO_SHARE: null,
});
return getExploreLongUrl(
minimalFormData,
endpointType,
false,
{
URL_IS_TOO_LONG_TO_SHARE: null,
},
force,
);
}
return url;
}
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/explore/reducers/getInitialState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export interface ExlorePageBootstrapData extends JsonObject {
form_data: QueryFormData;
slice: Slice | null;
standalone: boolean;
force: boolean;
user: UserWithPermissionsAndRoles;
}

Expand Down
22 changes: 19 additions & 3 deletions superset/reports/commands/execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
DashboardScreenshot,
)
from superset.utils.urls import get_url_path
from superset.utils.webdriver import DashboardStandaloneMode

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -145,6 +146,16 @@ def _get_url(
"""
Get the url for this report schedule: chart or dashboard
"""
# 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"
)

if self._report_schedule.chart:
if result_format in {
ChartDataResultFormat.CSV,
Expand All @@ -155,17 +166,22 @@ def _get_url(
pk=self._report_schedule.chart_id,
format=result_format.value,
type=ChartDataResultType.POST_PROCESSED.value,
force=force,
)
return get_url_path(
"Superset.slice",
"Superset.explore",
user_friendly=user_friendly,
slice_id=self._report_schedule.chart_id,
form_data=json.dumps({"slice_id": self._report_schedule.chart_id}),
standalone="true",
force=force,
**kwargs,
)
return get_url_path(
"Superset.dashboard",
user_friendly=user_friendly,
dashboard_id_or_slug=self._report_schedule.dashboard_id,
standalone=DashboardStandaloneMode.REPORT.value,
force=force,
**kwargs,
)

Expand All @@ -187,7 +203,7 @@ def _get_screenshot(self) -> bytes:
"""
screenshot: Optional[BaseScreenshot] = None
if self._report_schedule.chart:
url = self._get_url(standalone="true")
url = self._get_url()
logger.info("Screenshotting chart at %s", url)
screenshot = ChartScreenshot(
url,
Expand Down
2 changes: 1 addition & 1 deletion superset/utils/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ class ReservedUrlParameters(str, Enum):
@staticmethod
def is_standalone_mode() -> Optional[bool]:
standalone_param = request.args.get(ReservedUrlParameters.STANDALONE.value)
standalone: Optional[bool] = (
standalone: Optional[bool] = bool(
standalone_param and standalone_param != "false" and standalone_param != "0"
)
return standalone
Expand Down
6 changes: 0 additions & 6 deletions superset/utils/webdriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
from typing import Any, Dict, Optional, Tuple, TYPE_CHECKING

from flask import current_app
from requests.models import PreparedRequest
from selenium.common.exceptions import (
StaleElementReferenceException,
TimeoutException,
Expand Down Expand Up @@ -107,11 +106,6 @@ def destroy(driver: WebDriver, tries: int = 2) -> None:
def get_screenshot(
self, url: str, element_name: str, user: "User"
) -> Optional[bytes]:
params = {"standalone": DashboardStandaloneMode.REPORT.value}
req = PreparedRequest()
req.prepare_url(url, params)
url = req.url or ""

driver = self.auth(user)
driver.set_window_size(*self._window)
driver.get(url)
Expand Down
2 changes: 2 additions & 0 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,7 @@ def explore(
query_context,
)
standalone_mode = ReservedUrlParameters.is_standalone_mode()
force = request.args.get("force") in {"force", "1", "true"}
dummy_datasource_data: Dict[str, Any] = {
"type": datasource_type,
"name": datasource_name,
Expand All @@ -866,6 +867,7 @@ def explore(
"datasource_type": datasource_type,
"slice": slc.data if slc else None,
"standalone": standalone_mode,
"force": force,
"user": bootstrap_user_data(g.user, include_perms=True),
"forced_height": request.args.get("height"),
"common": common_bootstrap_payload(),
Expand Down
49 changes: 45 additions & 4 deletions tests/integration_tests/reports/commands_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -647,8 +647,47 @@ def test_email_chart_report_schedule(
)
# assert that the link sent is correct
assert (
f'<a href="http://0.0.0.0:8080/superset/slice/'
f'{create_report_email_chart.chart.id}/">Explore in Superset</a>'
'<a href="http://0.0.0.0:8080/superset/explore/?'
"form_data=%7B%22slice_id%22%3A+"
f"{create_report_email_chart.chart.id}%7D&"
'standalone=true&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]
# 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"
)
@patch("superset.reports.notifications.email.send_email_smtp")
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
def test_email_chart_alert_schedule(
screenshot_mock, email_mock, create_alert_email_chart,
):
"""
ExecuteReport Command: Test chart email alert schedule with screenshot
"""
# setup screenshot mock
screenshot_mock.return_value = SCREENSHOT_FILE

with freeze_time("2020-01-01T00:00:00Z"):
AsyncExecuteReportScheduleCommand(
TEST_ID, create_alert_email_chart.id, datetime.utcnow()
).run()

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/superset/explore/?'
"form_data=%7B%22slice_id%22%3A+"
f"{create_alert_email_chart.chart.id}%7D&"
'standalone=true&force=true">Explore in Superset</a>'
in email_mock.call_args[0][2]
)
# Assert the email smtp address
Expand Down Expand Up @@ -713,8 +752,10 @@ def test_email_chart_report_schedule_with_csv(
)
# assert that the link sent is correct
assert (
f'<a href="http://0.0.0.0:8080/superset/slice/'
f'{create_report_email_chart_with_csv.chart.id}/">Explore in Superset</a>'
'<a href="http://0.0.0.0:8080/superset/explore/?'
"form_data=%7B%22slice_id%22%3A+"
f"{create_report_email_chart_with_csv.chart.id}%7D&"
'standalone=true&force=false">Explore in Superset</a>'
in email_mock.call_args[0][2]
)
# Assert the email smtp address
Expand Down