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: Allow users to bust cache in report dashboard + alerts charts + alert dashboards #18795

Merged
merged 15 commits into from
Mar 4, 2022
6 changes: 4 additions & 2 deletions superset-frontend/src/chart/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

fix these import issues

Copy link
Member

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.

import { getUrlParam } from 'src/utils/urlUtils';
import ChartRenderer from './ChartRenderer';
import { ChartErrorMessage } from './ChartErrorMessage';

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

Choose a reason for hiding this comment

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

what does URL_PARAMS.force do?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 force

this.props.timeout,
this.props.chartId,
this.props.dashboardId,
Expand All @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ export const URL_PARAMS = {
name: 'dataset_id',
type: 'string',
},
force: {
name: 'force',
type: 'boolean',
},
} as const;

/**
Expand Down
18 changes: 18 additions & 0 deletions superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -340,4 +340,22 @@ describe('AlertReportModal', () => {
await waitForComponentToPaint(wrapper);
expect(wrapper.find('textarea[name="recipients"]')).toHaveLength(1);
});

it('renders bypass cache checkbox', async () => {
const bypass = wrapper.find('[data-test="bypass-cache"]');
expect(bypass).toExist();
});

it('renders no bypass cache checkbox when alert', async () => {
const props = {
...mockedProps,
alert: mockData,
isReport: false,
};

const alertWrapper = await mountAndWait(props);

const bypass = alertWrapper.find('[data-test="bypass-cache"]');
expect(bypass).not.toExist();
});
});
25 changes: 13 additions & 12 deletions superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
`;

Expand Down Expand Up @@ -940,7 +941,6 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
(!currentAlert || currentAlert.id || (isHidden && show))
) {
setCurrentAlert({ ...DEFAULT_ALERT });
setForceScreenshot(contentType === 'chart' && !isReport);
setNotificationSettings([]);
setNotificationAddState('active');
}
Expand Down Expand Up @@ -1369,19 +1369,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 || contentType === 'dashboard') && (
<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>
Expand Down
3 changes: 1 addition & 2 deletions superset/reports/commands/execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ def _get_url(
Get the url for this report schedule: chart or dashboard
"""
force = "true" if self._report_schedule.force_screenshot else "false"

if self._report_schedule.chart:
if result_format in {
ChartDataResultFormat.CSV,
Expand All @@ -173,7 +172,7 @@ def _get_url(
user_friendly=user_friendly,
dashboard_id_or_slug=self._report_schedule.dashboard_id,
standalone=DashboardStandaloneMode.REPORT.value,
# force=force, TODO (betodealmeida) implement this
hughhhh marked this conversation as resolved.
Show resolved Hide resolved
force=force,
**kwargs,
)

Expand Down
49 changes: 48 additions & 1 deletion tests/integration_tests/reports/commands_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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="target@email.com", 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():
Expand Down Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand Down