-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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: Allow users to bust cache in report dashboard + alerts charts + alert dashboards #18795
Changes from 13 commits
5537e2e
8e41881
a8aa035
6ba541a
843f1bc
d46ee31
54f8a49
8138b0d
c4bc334
97c42ed
5c92769
9c0fdf3
3678612
ac22e03
fd235bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,8 @@ import Loading from 'src/components/Loading'; | |
import { EmptyStateBig } from 'src/components/EmptyState'; | ||
import ErrorBoundary from 'src/components/ErrorBoundary'; | ||
import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils'; | ||
import { URL_PARAMS } from 'src/constants'; | ||
import { getUrlParam } from 'src/utils/urlUtils'; | ||
import ChartRenderer from './ChartRenderer'; | ||
import { ChartErrorMessage } from './ChartErrorMessage'; | ||
|
||
|
@@ -157,7 +159,7 @@ class Chart extends React.PureComponent { | |
// Load saved chart with a GET request | ||
this.props.actions.getSavedChart( | ||
this.props.formData, | ||
this.props.force, | ||
this.props.force || getUrlParam(URL_PARAMS.force), // allow override via url params force=true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does URL_PARAMS.force do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URL_PARAMS is an enum that represents the key for url param we want to pull. In this case it is |
||
this.props.timeout, | ||
this.props.chartId, | ||
this.props.dashboardId, | ||
|
@@ -167,7 +169,7 @@ class Chart extends React.PureComponent { | |
// Create chart with POST request | ||
this.props.actions.postChartFormData( | ||
this.props.formData, | ||
this.props.force, | ||
this.props.force || getUrlParam(URL_PARAMS.force), // allow override via url params force=true | ||
this.props.timeout, | ||
this.props.chartId, | ||
this.props.dashboardId, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -343,6 +343,7 @@ const StyledRadioGroup = styled(Radio.Group)` | |
`; | ||
|
||
const StyledCheckbox = styled(Checkbox)` | ||
margin-top: ${({ theme }) => theme.gridUnit * 2}px; | ||
margin-left: ${({ theme }) => theme.gridUnit * 5.5}px; | ||
`; | ||
|
||
|
@@ -516,11 +517,10 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({ | |
} | ||
}); | ||
|
||
const shouldEnableForceScreenshot = contentType === 'chart' && !isReport; | ||
const data: any = { | ||
...currentAlert, | ||
type: isReport ? 'Report' : 'Alert', | ||
force_screenshot: shouldEnableForceScreenshot || forceScreenshot, | ||
force_screenshot: !isReport || forceScreenshot, // alerts (!isReport) should always bypass cache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait... for alerts with dashboards we don't want to always bypass the cache. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not? Is it because of the perf issue that may arise? |
||
validator_type: conditionNotNull ? 'not null' : 'operator', | ||
validator_config_json: conditionNotNull | ||
? {} | ||
|
@@ -940,7 +940,6 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({ | |
(!currentAlert || currentAlert.id || (isHidden && show)) | ||
) { | ||
setCurrentAlert({ ...DEFAULT_ALERT }); | ||
setForceScreenshot(contentType === 'chart' && !isReport); | ||
setNotificationSettings([]); | ||
setNotificationAddState('active'); | ||
} | ||
|
@@ -1369,19 +1368,20 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({ | |
)} | ||
</StyledRadioGroup> | ||
</div> | ||
{isReport && ( | ||
<div className="inline-container"> | ||
<StyledCheckbox | ||
className="checkbox" | ||
checked={forceScreenshot} | ||
onChange={onForceScreenshotChange} | ||
> | ||
Ignore cache when generating screenshot | ||
</StyledCheckbox> | ||
</div> | ||
)} | ||
</> | ||
)} | ||
{isReport && ( | ||
<div className="inline-container"> | ||
<StyledCheckbox | ||
data-test="bypass-cache" | ||
className="checkbox" | ||
checked={forceScreenshot} | ||
onChange={onForceScreenshotChange} | ||
> | ||
Ignore cache when generating screenshot | ||
</StyledCheckbox> | ||
</div> | ||
)} | ||
<StyledSectionTitle> | ||
<h4>{t('Notification method')}</h4> | ||
<span className="required">*</span> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -290,6 +290,18 @@ def create_report_email_dashboard(): | |
cleanup_report_schedule(report_schedule) | ||
|
||
|
||
@pytest.fixture() | ||
def create_report_email_dashboard_force_screenshot(): | ||
with app.app_context(): | ||
dashboard = db.session.query(Dashboard).first() | ||
report_schedule = create_report_notification( | ||
email_target="[email protected]", dashboard=dashboard, force_screenshot=True | ||
) | ||
yield report_schedule | ||
|
||
cleanup_report_schedule(report_schedule) | ||
|
||
|
||
@pytest.fixture() | ||
def create_report_email_tabbed_dashboard(tabbed_dashboard): | ||
with app.app_context(): | ||
|
@@ -1002,6 +1014,41 @@ def test_email_dashboard_report_schedule( | |
assert_log(ReportState.SUCCESS) | ||
|
||
|
||
@pytest.mark.usefixtures( | ||
"load_birth_names_dashboard_with_slices", | ||
"create_report_email_dashboard_force_screenshot", | ||
) | ||
@patch("superset.reports.notifications.email.send_email_smtp") | ||
@patch("superset.utils.screenshots.DashboardScreenshot.get_screenshot") | ||
def test_email_dashboard_report_schedule_force_screenshot( | ||
screenshot_mock, email_mock, create_report_email_dashboard_force_screenshot | ||
): | ||
""" | ||
ExecuteReport Command: Test dashboard email report schedule | ||
""" | ||
# setup screenshot mock | ||
screenshot_mock.return_value = SCREENSHOT_FILE | ||
|
||
with freeze_time("2020-01-01T00:00:00Z"): | ||
AsyncExecuteReportScheduleCommand( | ||
TEST_ID, | ||
create_report_email_dashboard_force_screenshot.id, | ||
datetime.utcnow(), | ||
).run() | ||
|
||
notification_targets = get_target_from_report_schedule( | ||
create_report_email_dashboard_force_screenshot | ||
) | ||
|
||
# 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_report_slack_chart" | ||
) | ||
|
@@ -1772,7 +1819,7 @@ def test_when_tabs_are_selected_it_takes_screenshots_for_every_tabs( | |
assert dashboard_screenshot_mock.call_count == 2 | ||
for index, tab in enumerate(tabs): | ||
assert dashboard_screenshot_mock.call_args_list[index].args == ( | ||
f"http://0.0.0.0:8080/superset/dashboard/{dashboard.id}/?standalone=3#{tab}", | ||
f"http://0.0.0.0:8080/superset/dashboard/{dashboard.id}/?standalone=3&force=false#{tab}", | ||
f"{dashboard.digest}", | ||
) | ||
assert send_email_smtp_mock.called is True | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix these import issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hughhhh I fixed these real quick so that I could use your branch for testing locally.