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: add permalink to dashboard and explore #19078

Merged
merged 40 commits into from
Mar 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
2693cf4
rename key_value to temporary_cache
villebro Mar 4, 2022
a6cda78
add migration
villebro Mar 4, 2022
3223490
create new key_value package
villebro Mar 4, 2022
f191337
add commands
villebro Mar 8, 2022
dd81dca
lots of new stuff
villebro Mar 9, 2022
14f7bb0
fix schema reference
villebro Mar 9, 2022
4e39cc2
remove redundant filter state from bootstrap data
villebro Mar 9, 2022
e4c808f
add missing license headers
villebro Mar 9, 2022
6ca1862
fix pylint
villebro Mar 9, 2022
3adc45a
fix dashboard permalink access
villebro Mar 9, 2022
ca939cf
use valid json mocks for filter state tests
villebro Mar 10, 2022
1de6b51
fix temporary cache tests
villebro Mar 10, 2022
f4fbad8
add anchors to dashboard state
villebro Mar 10, 2022
b32a473
lint
villebro Mar 10, 2022
d8a4056
fix util test
villebro Mar 10, 2022
6f31dd7
fix url shortlink button tests
villebro Mar 10, 2022
c174e1b
remove legacy shortner
villebro Mar 10, 2022
5efd9de
remove unused imports
villebro Mar 10, 2022
226e5c0
fix js tests
villebro Mar 10, 2022
3f5a444
fix test
villebro Mar 11, 2022
da58de0
add native filter state to anchor link
villebro Mar 11, 2022
85aaa39
add UPDATING.md section
villebro Mar 11, 2022
32cd5dd
address comments
villebro Mar 11, 2022
fe2fba0
address comments
villebro Mar 11, 2022
90381e6
lint
villebro Mar 11, 2022
13c6fe5
fix test
villebro Mar 11, 2022
a317ad0
add utils tests + other test stubs
villebro Mar 14, 2022
cc99cba
add key_value integration tests
villebro Mar 14, 2022
6ea5462
add filter box state to permalink state
villebro Mar 14, 2022
0373492
fully support persisting url parameters
villebro Mar 15, 2022
5708dba
lint, add redirects and a few integration tests
villebro Mar 15, 2022
065c6f4
fix test + clean up trailing comma
villebro Mar 15, 2022
6b6d0c4
Merge branch 'master' into villebro/key-value-permalink
villebro Mar 16, 2022
1c20627
fix anchor bug
villebro Mar 16, 2022
392ea9c
change value to LargeBinary to support persisting binary values
villebro Mar 16, 2022
9dc2405
fix urlParams type and simplify urlencode
villebro Mar 16, 2022
50e7d04
lint
villebro Mar 16, 2022
89cee4b
add optional entry expiration
villebro Mar 16, 2022
6417634
fix incorrect chart id + add test
villebro Mar 16, 2022
4e9b423
Merge branch 'master' into villebro/key-value-permalink
villebro Mar 16, 2022
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
3 changes: 2 additions & 1 deletion UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,12 @@ flag for the legacy datasource editor (DISABLE_LEGACY_DATASOURCE_EDITOR) in conf

### Deprecations

- [19078](https://github.com/apache/superset/pull/19078): Creation of old shorturl links has been deprecated in favor of a new permalink feature that solves the long url problem (old shorturls will still work, though!). By default, new permalinks use UUID4 as the key. However, to use serial ids similar to the old shorturls, add the following to your `superset_config.py`: `PERMALINK_KEY_TYPE = "id"`.
- [18960](https://github.com/apache/superset/pull/18960): Persisting URL params in chart metadata is no longer supported. To set a default value for URL params in Jinja code, use the optional second argument: `url_param("my-param", "my-default-value")`.

### Other

- [17589](https://github.com/apache/incubator-superset/pull/17589): It is now possible to limit access to users' recent activity data by setting the `ENABLE_BROAD_ACTIVITY_ACCESS` config flag to false, or customizing the `raise_for_user_activity_access` method in the security manager.
- [17589](https://github.com/apache/superset/pull/17589): It is now possible to limit access to users' recent activity data by setting the `ENABLE_BROAD_ACTIVITY_ACCESS` config flag to false, or customizing the `raise_for_user_activity_access` method in the security manager.
- [17536](https://github.com/apache/superset/pull/17536): introduced a key-value endpoint to store dashboard filter state. This endpoint is backed by Flask-Caching and the default configuration assumes that the values will be stored in the file system. If you are already using another cache backend like Redis or Memchached, you'll probably want to change this setting in `superset_config.py`. The key is `FILTER_STATE_CACHE_CONFIG` and the available settings can be found in Flask-Caching [docs](https://flask-caching.readthedocs.io/en/latest/).
- [17882](https://github.com/apache/superset/pull/17882): introduced a key-value endpoint to store Explore form data. This endpoint is backed by Flask-Caching and the default configuration assumes that the values will be stored in the file system. If you are already using another cache backend like Redis or Memchached, you'll probably want to change this setting in `superset_config.py`. The key is `EXPLORE_FORM_DATA_CACHE_CONFIG` and the available settings can be found in Flask-Caching [docs](https://flask-caching.readthedocs.io/en/latest/).

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import URLShortLinkButton from 'src/components/URLShortLinkButton';
describe('AnchorLink', () => {
const props = {
anchorLinkId: 'CHART-123',
dashboardId: 10,
};

const globalLocation = window.location;
Expand Down Expand Up @@ -64,8 +65,9 @@ describe('AnchorLink', () => {
expect(wrapper.find(URLShortLinkButton)).toExist();
expect(wrapper.find(URLShortLinkButton)).toHaveProp({ placement: 'right' });

const targetUrl = wrapper.find(URLShortLinkButton).prop('url');
const hash = targetUrl.slice(targetUrl.indexOf('#') + 1);
expect(hash).toBe(props.anchorLinkId);
const anchorLinkId = wrapper.find(URLShortLinkButton).prop('anchorLinkId');
const dashboardId = wrapper.find(URLShortLinkButton).prop('dashboardId');
expect(anchorLinkId).toBe(props.anchorLinkId);
expect(dashboardId).toBe(props.dashboardId);
});
});
11 changes: 4 additions & 7 deletions superset-frontend/src/components/AnchorLink/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ import PropTypes from 'prop-types';
import { t } from '@superset-ui/core';

import URLShortLinkButton from 'src/components/URLShortLinkButton';
import getDashboardUrl from 'src/dashboard/util/getDashboardUrl';
import getLocationHash from 'src/dashboard/util/getLocationHash';

const propTypes = {
anchorLinkId: PropTypes.string.isRequired,
dashboardId: PropTypes.number,
filters: PropTypes.object,
showShortLinkButton: PropTypes.bool,
inFocus: PropTypes.bool,
Expand Down Expand Up @@ -70,17 +70,14 @@ class AnchorLink extends React.PureComponent {
}

render() {
const { anchorLinkId, filters, showShortLinkButton, placement } =
const { anchorLinkId, dashboardId, showShortLinkButton, placement } =
this.props;
return (
<span className="anchor-link-container" id={anchorLinkId}>
{showShortLinkButton && (
<URLShortLinkButton
url={getDashboardUrl({
pathname: window.location.pathname,
filters,
hash: anchorLinkId,
})}
anchorLinkId={anchorLinkId}
dashboardId={dashboardId}
emailSubject={t('Superset chart')}
emailContent={t('Check out this chart in dashboard:')}
placement={placement}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,48 +23,76 @@ import fetchMock from 'fetch-mock';
import URLShortLinkButton from 'src/components/URLShortLinkButton';
import ToastContainer from 'src/components/MessageToasts/ToastContainer';

const fakeUrl = 'http://fakeurl.com';
const DASHBOARD_ID = 10;
const PERMALINK_PAYLOAD = {
key: '123',
url: 'http://fakeurl.com/123',
};
const FILTER_STATE_PAYLOAD = {
value: '{}',
};

fetchMock.post('glob:*/r/shortner/', fakeUrl);
const props = {
dashboardId: DASHBOARD_ID,
};

fetchMock.get(
`glob:*/api/v1/dashboard/${DASHBOARD_ID}/filter_state*`,
FILTER_STATE_PAYLOAD,
);

fetchMock.post(
`glob:*/api/v1/dashboard/${DASHBOARD_ID}/permalink`,
PERMALINK_PAYLOAD,
);

test('renders with default props', () => {
render(<URLShortLinkButton />, { useRedux: true });
render(<URLShortLinkButton {...props} />, { useRedux: true });
expect(screen.getByRole('button')).toBeInTheDocument();
});

test('renders overlay on click', async () => {
render(<URLShortLinkButton />, { useRedux: true });
render(<URLShortLinkButton {...props} />, { useRedux: true });
userEvent.click(screen.getByRole('button'));
expect(await screen.findByRole('tooltip')).toBeInTheDocument();
});

test('obtains short url', async () => {
render(<URLShortLinkButton />, { useRedux: true });
render(<URLShortLinkButton {...props} />, { useRedux: true });
userEvent.click(screen.getByRole('button'));
expect(await screen.findByRole('tooltip')).toHaveTextContent(fakeUrl);
expect(await screen.findByRole('tooltip')).toHaveTextContent(
PERMALINK_PAYLOAD.url,
);
});

test('creates email anchor', async () => {
const subject = 'Subject';
const content = 'Content';

render(<URLShortLinkButton emailSubject={subject} emailContent={content} />, {
useRedux: true,
});
render(
<URLShortLinkButton
{...props}
emailSubject={subject}
emailContent={content}
/>,
{
useRedux: true,
},
);

const href = `mailto:?Subject=${subject}%20&Body=${content}${fakeUrl}`;
const href = `mailto:?Subject=${subject}%20&Body=${content}${PERMALINK_PAYLOAD.url}`;
userEvent.click(screen.getByRole('button'));
expect(await screen.findByRole('link')).toHaveAttribute('href', href);
});

test('renders error message on short url error', async () => {
fetchMock.mock('glob:*/r/shortner/', 500, {
fetchMock.mock(`glob:*/api/v1/dashboard/${DASHBOARD_ID}/permalink`, 500, {
overwriteRoutes: true,
});

render(
<>
<URLShortLinkButton />
<URLShortLinkButton {...props} />
<ToastContainer />
</>,
{ useRedux: true },
Expand Down
27 changes: 20 additions & 7 deletions superset-frontend/src/components/URLShortLinkButton/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,17 @@ import PropTypes from 'prop-types';
import { t } from '@superset-ui/core';
import Popover from 'src/components/Popover';
import CopyToClipboard from 'src/components/CopyToClipboard';
import { getShortUrl } from 'src/utils/urlUtils';
import { getDashboardPermalink, getUrlParam } from 'src/utils/urlUtils';
import withToasts from 'src/components/MessageToasts/withToasts';
import { URL_PARAMS } from 'src/constants';
import { getFilterValue } from 'src/dashboard/components/nativeFilters/FilterBar/keyValue';

const propTypes = {
url: PropTypes.string,
addDangerToast: PropTypes.func.isRequired,
anchorLinkId: PropTypes.string,
dashboardId: PropTypes.number,
Copy link
Member

Choose a reason for hiding this comment

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

Since this is just a 100L component, it may be worth just converting this to TypeScript instead of adding new proptypes

emailSubject: PropTypes.string,
emailContent: PropTypes.string,
addDangerToast: PropTypes.func.isRequired,
placement: PropTypes.oneOf(['right', 'left', 'top', 'bottom']),
};

Expand All @@ -50,9 +53,20 @@ class URLShortLinkButton extends React.Component {

getCopyUrl(e) {
e.stopPropagation();
getShortUrl(this.props.url)
.then(this.onShortUrlSuccess)
.catch(this.props.addDangerToast);
const nativeFiltersKey = getUrlParam(URL_PARAMS.nativeFiltersKey);
if (this.props.dashboardId) {
getFilterValue(this.props.dashboardId, nativeFiltersKey)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe getFilterValue should be renamed to getSavedFilterState for clarify.

We should probably also just pass the full state to URLShortLinkButton itself so we can avoid a round trip to the server for the saved states---because the client states should always be in sync with the URL. If the states already exist somewhere on the client, then we can pass it around for generating the short URL.

By passing full states (an arbitrary JSON object + a base URL template), it also decouples URLShortLinkButton with either dashboard or chart API endpoint, so it can be used in even other places (e.g. SQL queries, etc).

.then(filterState =>
getDashboardPermalink(
String(this.props.dashboardId),
filterState,
this.props.anchorLinkId,
)
.then(this.onShortUrlSuccess)
.catch(this.props.addDangerToast),
)
.catch(this.props.addDangerToast);
}
}

renderPopover() {
Expand Down Expand Up @@ -96,7 +110,6 @@ class URLShortLinkButton extends React.Component {
}

URLShortLinkButton.defaultProps = {
url: window.location.href.substring(window.location.origin.length),
placement: 'left',
emailSubject: '',
emailContent: '',
Expand Down
16 changes: 16 additions & 0 deletions superset-frontend/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,24 @@ export const URL_PARAMS = {
name: 'force',
type: 'boolean',
},
permalinkKey: {
name: 'permalink_key',
type: 'string',
},
} as const;

export const RESERVED_CHART_URL_PARAMS: string[] = [
URL_PARAMS.formDataKey.name,
URL_PARAMS.sliceId.name,
URL_PARAMS.datasetId.name,
];
export const RESERVED_DASHBOARD_URL_PARAMS: string[] = [
URL_PARAMS.nativeFilters.name,
URL_PARAMS.nativeFiltersKey.name,
URL_PARAMS.permalinkKey.name,
URL_PARAMS.preselectFilters.name,
];

/**
* Faster debounce delay for inputs without expensive operation.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ test('should show the share actions', async () => {
};
render(setup(canShareProps));
await openDropdown();
expect(screen.getByText('Copy dashboard URL')).toBeInTheDocument();
expect(screen.getByText('Share dashboard by email')).toBeInTheDocument();
expect(screen.getByText('Copy permalink to clipboard')).toBeInTheDocument();
expect(screen.getByText('Share permalink by email')).toBeInTheDocument();
});

test('should render the "Save Modal" when user can save', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,8 @@ class HeaderActionsDropdown extends React.PureComponent {
{userCanShare && (
<ShareMenuItems
url={url}
copyMenuItemTitle={t('Copy dashboard URL')}
emailMenuItemTitle={t('Share dashboard by email')}
copyMenuItemTitle={t('Copy permalink to clipboard')}
emailMenuItemTitle={t('Share permalink by email')}
emailSubject={emailSubject}
emailBody={emailBody}
addSuccessToast={addSuccessToast}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import moment from 'moment';
import {
Behavior,
getChartMetadataRegistry,
QueryFormData,
styled,
t,
} from '@superset-ui/core';
Expand Down Expand Up @@ -98,7 +99,7 @@ export interface SliceHeaderControlsProps {
isExpanded?: boolean;
updatedDttm: number | null;
isFullSize?: boolean;
formData: { slice_id: number; datasource: string };
formData: Pick<QueryFormData, 'slice_id' | 'datasource'>;
onExploreChart: () => void;

forceRefresh: (sliceId: number, dashboardId: number) => void;
Expand Down Expand Up @@ -309,8 +310,8 @@ class SliceHeaderControls extends React.PureComponent<

{supersetCanShare && (
<ShareMenuItems
copyMenuItemTitle={t('Copy chart URL')}
emailMenuItemTitle={t('Share chart by email')}
copyMenuItemTitle={t('Copy permalink to clipboard')}
Copy link
Member

@kgabryje kgabryje Mar 10, 2022

Choose a reason for hiding this comment

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

I'm not sure if using a word "permalink" brings value to the end user. I feel like it might be confusing, and simply saying "link" or "URL" makes the intention clearer. I'd suggest asking a product manager for an opinion on that.
(applies to changes in ExploreActionButtons too)

Copy link
Member

@michael-s-molina michael-s-molina Mar 10, 2022

Choose a reason for hiding this comment

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

The inspiration here is coming from Github and Shortcut but we could validate the name as you suggested.

Screen Shot 2022-03-08 at 10 02 21 AM

Screen Shot 2022-03-08 at 10 03 35 AM

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - I know @michael-s-molina did some investigative work and found many products use the term. Here's GitHub:
image

One other thing we may want to consider is removing "to clipboard", as it's pretty much self evident.

emailMenuItemTitle={t('Share permalink by email')}
emailSubject={t('Superset chart')}
emailBody={t('Check out this chart: ')}
addSuccessToast={addSuccessToast}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export const RENDER_TAB = 'RENDER_TAB';
export const RENDER_TAB_CONTENT = 'RENDER_TAB_CONTENT';

const propTypes = {
dashboardId: PropTypes.number.isRequired,
id: PropTypes.string.isRequired,
parentId: PropTypes.string.isRequired,
component: componentShape.isRequired,
Expand Down Expand Up @@ -237,6 +238,7 @@ export default class Tab extends React.PureComponent {
{!editMode && (
<AnchorLink
anchorLinkId={component.id}
dashboardId={this.props.dashboardId}
filters={filters}
showShortLinkButton
placement={index >= 5 ? 'left' : 'right'}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const DASHBOARD_ID = '26';
const createProps = () => ({
addDangerToast: jest.fn(),
addSuccessToast: jest.fn(),
url: `/superset/dashboard/${DASHBOARD_ID}/?preselect_filters=%7B%7D`,
url: `/superset/dashboard/${DASHBOARD_ID}`,
copyMenuItemTitle: 'Copy dashboard URL',
emailMenuItemTitle: 'Share dashboard by email',
emailSubject: 'Superset dashboard COVID Vaccine Dashboard',
Expand All @@ -45,10 +45,10 @@ beforeAll((): void => {
// @ts-ignore
delete window.location;
fetchMock.post(
'http://localhost/r/shortner/',
{ body: 'http://localhost:8088/r/3' },
`http://localhost/api/v1/dashboard/${DASHBOARD_ID}/permalink`,
{ key: '123', url: 'http://localhost/superset/dashboard/p/123/' },
{
sendAsJson: false,
sendAsJson: true,
},
);
});
Expand Down Expand Up @@ -104,7 +104,7 @@ test('Click on "Copy dashboard URL" and succeed', async () => {

await waitFor(() => {
expect(spy).toBeCalledTimes(1);
expect(spy).toBeCalledWith('http://localhost:8088/r/3');
expect(spy).toBeCalledWith('http://localhost/superset/dashboard/p/123/');
expect(props.addSuccessToast).toBeCalledTimes(1);
expect(props.addSuccessToast).toBeCalledWith('Copied to clipboard!');
expect(props.addDangerToast).toBeCalledTimes(0);
Expand All @@ -130,7 +130,7 @@ test('Click on "Copy dashboard URL" and fail', async () => {

await waitFor(() => {
expect(spy).toBeCalledTimes(1);
expect(spy).toBeCalledWith('http://localhost:8088/r/3');
expect(spy).toBeCalledWith('http://localhost/superset/dashboard/p/123/');
expect(props.addSuccessToast).toBeCalledTimes(0);
expect(props.addDangerToast).toBeCalledTimes(1);
expect(props.addDangerToast).toBeCalledWith(
Expand Down Expand Up @@ -159,14 +159,14 @@ 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%20dashboard%20COVID%20Vaccine%20Dashboard%20&Body=Check%20out%20this%20dashboard%3A%20http%3A%2F%2Flocalhost%3A8088%2Fr%2F3',
'mailto:?Subject=Superset%20dashboard%20COVID%20Vaccine%20Dashboard%20&Body=Check%20out%20this%20dashboard%3A%20http%3A%2F%2Flocalhost%2Fsuperset%2Fdashboard%2Fp%2F123%2F',
);
});
});

test('Click on "Share dashboard by email" and fail', async () => {
fetchMock.post(
'http://localhost/r/shortner/',
`http://localhost/api/v1/dashboard/${DASHBOARD_ID}/permalink`,
{ status: 404 },
{ overwriteRoutes: true },
);
Expand Down
Loading