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 2 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 dashboard COVID Vaccine Dashboard%20&Body=Check out this dashboard: http%3A%2F%2Flocalhost%3A8088%2Fr%2F3',
michael-s-molina marked this conversation as resolved.
Show resolved Hide resolved
);
});
});
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,7 +102,9 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {

async function onShareByEmail() {
try {
const bodyWithLink = `${emailBody}${await generateUrl()}`;
const bodyWithLink = `${emailBody}${encodeURIComponent(
await generateUrl(),
)}`;
window.location.href = `mailto:?Subject=${emailSubject}%20&Body=${bodyWithLink}`;
} catch (error) {
logging.error(error);
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
9 changes: 9 additions & 0 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,15 @@ def explore(
if value:
initial_form_data = json.loads(value)

if not initial_form_data:
slice_id = request.args.get("slice_id")
if slice_id:
initial_form_data["slice_id"] = slice_id

dataset_id = request.args.get("dataset_id")
if dataset_id:
initial_form_data["datasource"] = f"{dataset_id}__table"
michael-s-molina marked this conversation as resolved.
Show resolved Hide resolved
michael-s-molina marked this conversation as resolved.
Show resolved Hide resolved

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