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: Improves key expiration handling in Explore #18624

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
8 changes: 8 additions & 0 deletions superset-frontend/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ export const URL_PARAMS = {
name: 'form_data_key',
type: 'string',
},
sliceId: {
name: 'slice_id',
type: 'string',
},
datasetId: {
name: 'dataset_id',
type: 'string',
},
} as const;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,12 @@ export default class Chart extends React.Component {
const key = await postFormData(
this.props.datasource.id,
this.props.formData,
this.props.slice.id,
this.props.slice.slice_id,
michael-s-molina marked this conversation as resolved.
Show resolved Hide resolved
);
const url = mountExploreUrl(null, { [URL_PARAMS.formDataKey.name]: key });
const url = mountExploreUrl(null, {
[URL_PARAMS.formDataKey.name]: key,
[URL_PARAMS.sliceId.name]: this.props.slice.slice_id,
});
window.open(url, '_blank', 'noreferrer');
} catch (error) {
logging.error(error);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ test('Click on "Share dashboard by email" and succeed', async () => {
await waitFor(() => {
expect(props.addDangerToast).toBeCalledTimes(0);
expect(window.location.href).toBe(
'mailto:?Subject=Superset dashboard COVID Vaccine Dashboard%20&Body=Check out this dashboard: http://localhost:8088/r/3',
'mailto:?Subject=Superset%20dashboard%20COVID%20Vaccine%20Dashboard%20&Body=Check%20out%20this%20dashboard%3A%20http%3A%2F%2Flocalhost%3A8088%2Fr%2F3',
);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {
);
return `${window.location.origin}${mountExploreUrl(null, {
[URL_PARAMS.formDataKey.name]: key,
[URL_PARAMS.sliceId.name]: formData.slice_id,
})}`;
}
const copyUrl = await getCopyUrl();
Expand All @@ -101,8 +102,11 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {

async function onShareByEmail() {
try {
const bodyWithLink = `${emailBody}${await generateUrl()}`;
window.location.href = `mailto:?Subject=${emailSubject}%20&Body=${bodyWithLink}`;
const encodedBody = encodeURIComponent(
`${emailBody}${await generateUrl()}`,
);
const encodedSubject = encodeURIComponent(emailSubject);
window.location.href = `mailto:?Subject=${encodedSubject}%20&Body=${encodedBody}`;
} catch (error) {
logging.error(error);
addDangerToast(t('Sorry, something went wrong. Try again later.'));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const reduxState = {
common: { conf: { SUPERSET_WEBSERVER_TIMEOUT: 60 } },
controls: { datasource: { value: '1__table' } },
datasource: {
id: 1,
type: 'table',
columns: [{ is_dttm: false }],
metrics: [{ id: 1, metric_name: 'count' }],
Expand Down Expand Up @@ -65,7 +66,7 @@ fetchMock.get('glob:*/api/v1/explore/form_data*', {});

const renderWithRouter = (withKey?: boolean) => {
const path = '/superset/explore/';
const search = withKey ? `?form_data_key=${key}` : '';
const search = withKey ? `?form_data_key=${key}&dataset_id=1` : '';
return render(
<MemoryRouter initialEntries={[`${path}${search}`]}>
<Route path={path}>
Expand All @@ -82,7 +83,12 @@ test('generates a new form_data param when none is available', async () => {
expect(replaceState).toHaveBeenCalledWith(
expect.anything(),
undefined,
expect.stringMatching('form_data'),
expect.stringMatching('form_data_key'),
);
expect(replaceState).toHaveBeenCalledWith(
expect.anything(),
undefined,
expect.stringMatching('dataset_id'),
);
replaceState.mockRestore();
});
Expand All @@ -96,6 +102,11 @@ test('generates a different form_data param when one is provided and is mounting
undefined,
expect.stringMatching(key),
);
expect(replaceState).toHaveBeenCalledWith(
expect.anything(),
undefined,
expect.stringMatching('dataset_id'),
);
replaceState.mockRestore();
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,12 @@ const updateHistory = debounce(
async (formData, datasetId, isReplace, standalone, force, title) => {
const payload = { ...formData };
const chartId = formData.slice_id;
const additionalParam = {};
if (chartId) {
additionalParam[URL_PARAMS.sliceId.name] = chartId;
} else {
additionalParam[URL_PARAMS.datasetId.name] = datasetId;
}

try {
let key;
Expand All @@ -183,6 +189,7 @@ const updateHistory = debounce(
standalone ? URL_PARAMS.standalone.name : null,
{
[URL_PARAMS.formDataKey.name]: key,
...additionalParam,
},
force,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import React from 'react';
import PropTypes from 'prop-types';
import { t, styled, supersetTheme } from '@superset-ui/core';
import { getUrlParam } from 'src/utils/urlUtils';

import { Dropdown, Menu } from 'src/common/components';
import { Tooltip } from 'src/components/Tooltip';
Expand All @@ -32,6 +33,7 @@ import { postForm } from 'src/explore/exploreUtils';
import Button from 'src/components/Button';
import ErrorAlert from 'src/components/ErrorMessage/ErrorAlert';
import WarningIconWithTooltip from 'src/components/WarningIconWithTooltip';
import { URL_PARAMS } from 'src/constants';

const propTypes = {
actions: PropTypes.object.isRequired,
Expand Down Expand Up @@ -182,6 +184,14 @@ class DatasourceControl extends React.PureComponent {
const { showChangeDatasourceModal, showEditDatasourceModal } = this.state;
const { datasource, onChange } = this.props;
const isMissingDatasource = datasource.id == null;
let isMissingParams = false;
if (isMissingDatasource) {
const datasetId = getUrlParam(URL_PARAMS.datasetId);
const sliceId = getUrlParam(URL_PARAMS.sliceId);
if (!datasetId && !sliceId) {
isMissingParams = true;
}
}

const isSqlSupported = datasource.type === 'table';

Expand Down Expand Up @@ -244,7 +254,25 @@ class DatasourceControl extends React.PureComponent {
</Dropdown>
</div>
{/* missing dataset */}
{isMissingDatasource && (
{isMissingDatasource && isMissingParams && (
<div className="error-alert">
<ErrorAlert
level="warning"
title={t('Missing URL parameters')}
source="explore"
subtitle={
<>
<p>
{t(
'The URL is missing the dataset_id or slice_id parameters.',
)}
</p>
</>
}
/>
</div>
)}
{isMissingDatasource && !isMissingParams && (
<div className="error-alert">
<ErrorAlert
level="warning"
Expand Down
10 changes: 10 additions & 0 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,16 @@ def explore(
if value:
initial_form_data = json.loads(value)

if not initial_form_data:
slice_id = request.args.get("slice_id")
dataset_id = request.args.get("dataset_id")
if slice_id:
initial_form_data["slice_id"] = slice_id
flash(_("Form data not found in cache, reverting to chart metadata."))
elif dataset_id:
initial_form_data["datasource"] = f"{dataset_id}__table"
flash(_("Form data not found in cache, reverting to dataset metadata."))

form_data, slc = get_form_data(
use_slice_data=True, initial_form_data=initial_form_data
)
Expand Down