From 4b61c767425911551d276b59f1386b39bf319c5d Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Fri, 28 Jan 2022 17:42:16 -0300 Subject: [PATCH] fix: Explore long URL problem (#18181) * fix: Explore long URL problem * Fixes lint problems * Fixes default value * Removes duplicated test * Fixes share menu items * Fixes tests * Debounces form_data updates * Rewrites debounce function * Moves history update outside the functional component * Mocks lodash function in tests * Fixes Cypress test * Fixes Cypress test #2 --- .../explore/advanced_analytics.test.ts | 5 +- .../cypress/integration/explore/link.test.ts | 12 -- .../spec/helpers/ResizeObserver.ts | 33 +++++ superset-frontend/spec/helpers/shim.ts | 2 + superset-frontend/src/constants.ts | 4 + .../SliceHeader/SliceHeader.test.tsx | 3 +- .../components/SliceHeader/index.tsx | 4 +- .../SliceHeaderControls.test.tsx | 3 +- .../components/SliceHeaderControls/index.tsx | 23 ++- .../components/gridComponents/Chart.jsx | 26 +++- .../ShareMenuItems/ShareMenuItems.test.tsx | 2 +- .../components/menu/ShareMenuItems/index.tsx | 37 +++-- .../explore/components/EmbedCodeButton.jsx | 30 +--- .../components/EmbedCodeButton.test.jsx | 72 ++-------- .../components/ExploreActionButtons.tsx | 15 +- .../components/ExploreChartHeader/index.jsx | 1 - .../explore/components/ExploreChartPanel.jsx | 2 - .../ExploreViewContainer.test.tsx | 112 +++++++++++++++ .../index.jsx} | 102 ++++++++----- .../exploreUtils/exploreUtils.test.jsx | 21 --- .../src/explore/exploreUtils/formData.ts | 61 ++++++++ .../exploreUtils/getExploreLongUrl.test.ts | 134 ------------------ .../src/explore/exploreUtils/index.js | 51 +------ superset/views/core.py | 16 ++- superset/views/utils.py | 6 +- 25 files changed, 377 insertions(+), 400 deletions(-) create mode 100644 superset-frontend/spec/helpers/ResizeObserver.ts create mode 100644 superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx rename superset-frontend/src/explore/components/{ExploreViewContainer.jsx => ExploreViewContainer/index.jsx} (90%) create mode 100644 superset-frontend/src/explore/exploreUtils/formData.ts delete mode 100644 superset-frontend/src/explore/exploreUtils/getExploreLongUrl.test.ts diff --git a/superset-frontend/cypress-base/cypress/integration/explore/advanced_analytics.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/advanced_analytics.test.ts index 0646894fc4..51fd2ce46b 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/advanced_analytics.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/explore/advanced_analytics.test.ts @@ -21,6 +21,8 @@ describe('Advanced analytics', () => { cy.login(); cy.intercept('POST', '/superset/explore_json/**').as('postJson'); cy.intercept('GET', '/superset/explore_json/**').as('getJson'); + cy.intercept('PUT', '/api/v1/explore/**').as('putExplore'); + cy.intercept('GET', '/superset/explore/**').as('getExplore'); }); it('Create custom time compare', () => { @@ -40,12 +42,13 @@ describe('Advanced analytics', () => { cy.get('button[data-test="run-query-button"]').click(); cy.wait('@postJson'); + cy.wait('@putExplore'); cy.reload(); cy.verifySliceSuccess({ waitAlias: '@postJson', chartSelector: 'svg', }); - + cy.wait('@getExplore'); cy.get('.ant-collapse-header').contains('Advanced Analytics').click(); cy.get('[data-test=time_compare]') .find('.ant-select-selector') diff --git a/superset-frontend/cypress-base/cypress/integration/explore/link.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/link.test.ts index e671c57996..027326e021 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/link.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/explore/link.test.ts @@ -48,18 +48,6 @@ describe('Test explore links', () => { }); }); - it('Test if short link is generated', () => { - cy.intercept('POST', 'r/shortner/').as('getShortUrl'); - - cy.visitChartByName('Growth Rate'); - cy.verifySliceSuccess({ waitAlias: '@chartData' }); - - cy.get('[data-test=short-link-button]').click(); - - // explicitly wait for the url response - cy.wait('@getShortUrl'); - }); - it('Test iframe link', () => { cy.visitChartByName('Growth Rate'); cy.verifySliceSuccess({ waitAlias: '@chartData' }); diff --git a/superset-frontend/spec/helpers/ResizeObserver.ts b/superset-frontend/spec/helpers/ResizeObserver.ts new file mode 100644 index 0000000000..ba30a593df --- /dev/null +++ b/superset-frontend/spec/helpers/ResizeObserver.ts @@ -0,0 +1,33 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +class ResizeObserver { + disconnect() { + return null; + } + + observe() { + return null; + } + + unobserve() { + return null; + } +} + +export { ResizeObserver }; diff --git a/superset-frontend/spec/helpers/shim.ts b/superset-frontend/spec/helpers/shim.ts index 9371a00291..8543e98ce8 100644 --- a/superset-frontend/spec/helpers/shim.ts +++ b/superset-frontend/spec/helpers/shim.ts @@ -28,6 +28,7 @@ import Adapter from 'enzyme-adapter-react-16'; import { configure as configureTranslation } from '../../packages/superset-ui-core/src/translation'; import { Worker } from './Worker'; import { IntersectionObserver } from './IntersectionObserver'; +import { ResizeObserver } from './ResizeObserver'; import setupSupersetClient from './setupSupersetClient'; import CacheStorage from './CacheStorage'; @@ -51,6 +52,7 @@ g.window.location = { href: 'about:blank' }; g.window.performance = { now: () => new Date().getTime() }; g.window.Worker = Worker; g.window.IntersectionObserver = IntersectionObserver; +g.window.ResizeObserver = ResizeObserver; g.URL.createObjectURL = () => ''; g.caches = new CacheStorage(); diff --git a/superset-frontend/src/constants.ts b/superset-frontend/src/constants.ts index 996cd7d98c..e27022fe63 100644 --- a/superset-frontend/src/constants.ts +++ b/superset-frontend/src/constants.ts @@ -55,6 +55,10 @@ export const URL_PARAMS = { name: 'show_filters', type: 'boolean', }, + formDataKey: { + name: 'form_data_key', + type: 'string', + }, } as const; /** diff --git a/superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx b/superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx index 6acf88844c..27abbd396f 100644 --- a/superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx @@ -157,7 +157,8 @@ const createProps = () => ({ forceRefresh: jest.fn(), logExploreChart: jest.fn(), exportCSV: jest.fn(), - formData: {}, + onExploreChart: jest.fn(), + formData: { slice_id: 1, datasource: '58__table' }, }); test('Should render', () => { diff --git a/superset-frontend/src/dashboard/components/SliceHeader/index.tsx b/superset-frontend/src/dashboard/components/SliceHeader/index.tsx index a92fcc47a6..88e9a4c69f 100644 --- a/superset-frontend/src/dashboard/components/SliceHeader/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeader/index.tsx @@ -58,7 +58,7 @@ const SliceHeader: FC = ({ updateSliceName = () => ({}), toggleExpandSlice = () => ({}), logExploreChart = () => ({}), - exploreUrl = '#', + onExploreChart, exportCSV = () => ({}), editMode = false, annotationQuery = {}, @@ -171,7 +171,7 @@ const SliceHeader: FC = ({ toggleExpandSlice={toggleExpandSlice} forceRefresh={forceRefresh} logExploreChart={logExploreChart} - exploreUrl={exploreUrl} + onExploreChart={onExploreChart} exportCSV={exportCSV} exportFullCSV={exportFullCSV} supersetCanExplore={supersetCanExplore} diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx index ff8349ee8b..3f78c01439 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx @@ -45,6 +45,7 @@ const createProps = (viz_type = 'sunburst') => ({ forceRefresh: jest.fn(), handleToggleFullSize: jest.fn(), toggleExpandSlice: jest.fn(), + onExploreChart: jest.fn(), slice: { slice_id: 371, slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20371%7D', @@ -90,7 +91,7 @@ const createProps = (viz_type = 'sunburst') => ({ chartStatus: 'rendered', showControls: true, supersetCanShare: true, - formData: {}, + formData: { slice_id: 1, datasource: '58__table' }, }); test('Should render', () => { diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx index d389542b03..f39eda2427 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx @@ -27,8 +27,6 @@ import { import { Menu, NoAnimationDropdown } from 'src/common/components'; import ShareMenuItems from 'src/dashboard/components/menu/ShareMenuItems'; import downloadAsImage from 'src/utils/downloadAsImage'; -import getDashboardUrl from 'src/dashboard/util/getDashboardUrl'; -import { getActiveFilters } from 'src/dashboard/util/activeDashboardFilters'; import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; import CrossFilterScopingModal from 'src/dashboard/components/CrossFilterScopingModal/CrossFilterScopingModal'; import Icons from 'src/components/Icons'; @@ -99,8 +97,8 @@ export interface SliceHeaderControlsProps { isExpanded?: boolean; updatedDttm: number | null; isFullSize?: boolean; - formData: object; - exploreUrl?: string; + formData: { slice_id: number; datasource: string }; + onExploreChart: () => void; forceRefresh: (sliceId: number, dashboardId: number) => void; logExploreChart?: (sliceId: number) => void; @@ -215,13 +213,13 @@ class SliceHeaderControls extends React.PureComponent< const { slice, isFullSize, - componentId, cachedDttm = [], updatedDttm = null, addSuccessToast = () => {}, addDangerToast = () => {}, supersetCanShare = false, isCached = [], + formData, } = this.props; const crossFilterItems = getChartMetadataRegistry().items; const isTable = slice.viz_type === 'table'; @@ -283,10 +281,11 @@ class SliceHeaderControls extends React.PureComponent< )} {this.props.supersetCanExplore && ( - - - {t('View chart in Explore')} - + + {t('View chart in Explore')} )} @@ -309,17 +308,13 @@ class SliceHeaderControls extends React.PureComponent< {supersetCanShare && ( )} diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index 93c6d6ea5d..70679600df 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -19,13 +19,10 @@ import cx from 'classnames'; import React from 'react'; import PropTypes from 'prop-types'; -import { styled } from '@superset-ui/core'; +import { styled, t, logging } from '@superset-ui/core'; import { isEqual } from 'lodash'; -import { - exportChart, - getExploreUrlFromDashboard, -} from 'src/explore/exploreUtils'; +import { exportChart, mountExploreUrl } from 'src/explore/exploreUtils'; import ChartContainer from 'src/chart/ChartContainer'; import { LOG_ACTIONS_CHANGE_DASHBOARD_FILTER, @@ -35,6 +32,8 @@ import { } from 'src/logger/LogUtils'; import { areObjectsEqual } from 'src/reduxUtils'; import { FILTER_BOX_MIGRATION_STATES } from 'src/explore/constants'; +import { postFormData } from 'src/explore/exploreUtils/formData'; +import { URL_PARAMS } from 'src/constants'; import SliceHeader from '../SliceHeader'; import MissingChart from '../MissingChart'; @@ -241,7 +240,20 @@ export default class Chart extends React.Component { }); }; - getChartUrl = () => getExploreUrlFromDashboard(this.props.formData); + onExploreChart = async () => { + try { + const key = await postFormData( + this.props.datasource.id, + this.props.formData, + this.props.slice.id, + ); + const url = mountExploreUrl(null, { [URL_PARAMS.formDataKey.name]: key }); + window.open(url, '_blank', 'noreferrer'); + } catch (error) { + logging.error(error); + this.props.addDangerToast(t('An error occurred while opening Explore')); + } + }; exportCSV(isFullCSV = false) { this.props.logEvent(LOG_ACTIONS_EXPORT_CSV_DASHBOARD_CHART, { @@ -350,7 +362,7 @@ export default class Chart extends React.Component { editMode={editMode} annotationQuery={chart.annotationQuery} logExploreChart={this.logExploreChart} - exploreUrl={this.getChartUrl()} + onExploreChart={this.onExploreChart} exportCSV={this.exportCSV} exportFullCSV={this.exportFullCSV} updateSliceName={updateSliceName} diff --git a/superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx b/superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx index 9e47441fec..43b4188cd9 100644 --- a/superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx +++ b/superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx @@ -134,7 +134,7 @@ test('Click on "Copy dashboard URL" and fail', async () => { expect(props.addSuccessToast).toBeCalledTimes(0); expect(props.addDangerToast).toBeCalledTimes(1); expect(props.addDangerToast).toBeCalledWith( - 'Sorry, your browser does not support copying.', + 'Sorry, something went wrong. Try again later.', ); }); }); diff --git a/superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx b/superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx index 5ef275179c..ed77628bd4 100644 --- a/superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx +++ b/superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx @@ -19,17 +19,19 @@ import React from 'react'; import { useUrlShortener } from 'src/hooks/useUrlShortener'; import copyTextToClipboard from 'src/utils/copy'; -import { t } from '@superset-ui/core'; +import { t, logging } from '@superset-ui/core'; import { Menu } from 'src/common/components'; import { getUrlParam } from 'src/utils/urlUtils'; +import { postFormData } from 'src/explore/exploreUtils/formData'; import { URL_PARAMS } from 'src/constants'; +import { mountExploreUrl } from 'src/explore/exploreUtils'; import { createFilterKey, getFilterValue, } from 'src/dashboard/components/nativeFilters/FilterBar/keyValue'; interface ShareMenuItemProps { - url: string; + url?: string; copyMenuItemTitle: string; emailMenuItemTitle: string; emailSubject: string; @@ -37,6 +39,7 @@ interface ShareMenuItemProps { addDangerToast: Function; addSuccessToast: Function; dashboardId?: string; + formData?: { slice_id: number; datasource: string }; } const ShareMenuItems = (props: ShareMenuItemProps) => { @@ -49,10 +52,11 @@ const ShareMenuItems = (props: ShareMenuItemProps) => { addDangerToast, addSuccessToast, dashboardId, + formData, ...rest } = props; - const getShortUrl = useUrlShortener(url); + const getShortUrl = useUrlShortener(url || ''); async function getCopyUrl() { const risonObj = getUrlParam(URL_PARAMS.nativeFilters); @@ -70,24 +74,37 @@ const ShareMenuItems = (props: ShareMenuItemProps) => { return `${newUrl.pathname}${newUrl.search}`; } + async function generateUrl() { + if (formData) { + const key = await postFormData( + parseInt(formData.datasource.split('_')[0], 10), + formData, + formData.slice_id, + ); + return `${window.location.origin}${mountExploreUrl(null, { + [URL_PARAMS.formDataKey.name]: key, + })}`; + } + const copyUrl = await getCopyUrl(); + return getShortUrl(copyUrl); + } + async function onCopyLink() { try { - const copyUrl = await getCopyUrl(); - const shortUrl = await getShortUrl(copyUrl); - await copyTextToClipboard(shortUrl); + await copyTextToClipboard(await generateUrl()); addSuccessToast(t('Copied to clipboard!')); } catch (error) { - addDangerToast(t('Sorry, your browser does not support copying.')); + logging.error(error); + addDangerToast(t('Sorry, something went wrong. Try again later.')); } } async function onShareByEmail() { try { - const copyUrl = await getCopyUrl(); - const shortUrl = await getShortUrl(copyUrl); - const bodyWithLink = `${emailBody}${shortUrl}`; + const bodyWithLink = `${emailBody}${await generateUrl()}`; window.location.href = `mailto:?Subject=${emailSubject}%20&Body=${bodyWithLink}`; } catch (error) { + logging.error(error); addDangerToast(t('Sorry, something went wrong. Try again later.')); } } diff --git a/superset-frontend/src/explore/components/EmbedCodeButton.jsx b/superset-frontend/src/explore/components/EmbedCodeButton.jsx index 80fc3d5d44..57e6d30de4 100644 --- a/superset-frontend/src/explore/components/EmbedCodeButton.jsx +++ b/superset-frontend/src/explore/components/EmbedCodeButton.jsx @@ -17,7 +17,6 @@ * under the License. */ import React from 'react'; -import PropTypes from 'prop-types'; import { t } from '@superset-ui/core'; import Popover from 'src/components/Popover'; @@ -25,13 +24,7 @@ import { FormLabel } from 'src/components/Form'; import Icons from 'src/components/Icons'; import { Tooltip } from 'src/components/Tooltip'; import CopyToClipboard from 'src/components/CopyToClipboard'; -import { getShortUrl } from 'src/utils/urlUtils'; import { URL_PARAMS } from 'src/constants'; -import { getExploreLongUrl, getURIDirectory } from '../exploreUtils'; - -const propTypes = { - latestQueryFormData: PropTypes.object.isRequired, -}; export default class EmbedCodeButton extends React.Component { constructor(props) { @@ -39,24 +32,8 @@ export default class EmbedCodeButton extends React.Component { this.state = { height: '400', width: '600', - shortUrlId: 0, }; this.handleInputChange = this.handleInputChange.bind(this); - this.getCopyUrl = this.getCopyUrl.bind(this); - this.onShortUrlSuccess = this.onShortUrlSuccess.bind(this); - } - - onShortUrlSuccess(shortUrl) { - const shortUrlId = shortUrl.substring(shortUrl.indexOf('/r/') + 3); - this.setState(() => ({ - shortUrlId, - })); - } - - getCopyUrl() { - return getShortUrl(getExploreLongUrl(this.props.latestQueryFormData)) - .then(this.onShortUrlSuccess) - .catch(this.props.addDangerToast); } handleInputChange(e) { @@ -67,9 +44,7 @@ export default class EmbedCodeButton extends React.Component { } generateEmbedHTML() { - const srcLink = `${window.location.origin + getURIDirectory()}?r=${ - this.state.shortUrlId - }&${URL_PARAMS.standalone.name}=1&height=${this.state.height}`; + const srcLink = `${window.location.href}&${URL_PARAMS.standalone.name}=1&height=${this.state.height}`; return ( ' { - const mockStore = configureStore([]); - const store = mockStore({}); - - const defaultProps = { - latestQueryFormData: { datasource: '107__table' }, - }; - it('renders', () => { - expect(React.isValidElement()).toBe( - true, - ); + expect(React.isValidElement()).toBe(true); }); it('renders overlay trigger', () => { - const wrapper = shallow(); + const wrapper = shallow(); expect(wrapper.find(Popover)).toExist(); }); - it('should create a short, standalone, explore url', () => { - const spy1 = sinon.spy(exploreUtils, 'getExploreLongUrl'); - const spy2 = sinon.spy(urlUtils, 'getShortUrl'); - - const wrapper = mount( - - - , - { - wrappingComponent: Provider, - wrappingComponentProps: { - store, - }, - }, - ).find(EmbedCodeButton); - wrapper.setState({ - height: '1000', - width: '2000', - shortUrlId: 100, - }); - - const trigger = wrapper.find(Popover); - trigger.simulate('click'); - expect(spy1.callCount).toBe(1); - expect(spy2.callCount).toBe(1); - - spy1.restore(); - spy2.restore(); - }); - it('returns correct embed code', () => { - const stub = sinon - .stub(exploreUtils, 'getURIDirectory') - .callsFake(() => 'endpoint_url'); - const wrapper = mount( - - - , - ); + const href = 'http://localhost/explore?form_data_key=xxxxxxxxx'; + Object.defineProperty(window, 'location', { value: { href } }); + const wrapper = mount(); wrapper.find(EmbedCodeButton).setState({ height: '1000', width: '2000', - shortUrlId: 100, }); const embedHTML = `${ @@ -104,13 +49,12 @@ describe('EmbedCodeButton', () => { ' seamless\n' + ' frameBorder="0"\n' + ' scrolling="no"\n' + - ' src="http://localhostendpoint_url?r=100&standalone=' + ` src="${href}&standalone=` }${DashboardStandaloneMode.HIDE_NAV}&height=1000"\n` + `>\n` + ``; expect(wrapper.find(EmbedCodeButton).instance().generateEmbedHTML()).toBe( embedHTML, ); - stub.restore(); }); }); diff --git a/superset-frontend/src/explore/components/ExploreActionButtons.tsx b/superset-frontend/src/explore/components/ExploreActionButtons.tsx index 5d4ac7b5cc..95d3c1470f 100644 --- a/superset-frontend/src/explore/components/ExploreActionButtons.tsx +++ b/superset-frontend/src/explore/components/ExploreActionButtons.tsx @@ -23,9 +23,8 @@ import Icons from 'src/components/Icons'; import { Tooltip } from 'src/components/Tooltip'; import copyTextToClipboard from 'src/utils/copy'; import withToasts from 'src/components/MessageToasts/withToasts'; -import { useUrlShortener } from 'src/hooks/useUrlShortener'; import EmbedCodeButton from './EmbedCodeButton'; -import { exportChart, getExploreLongUrl } from '../exploreUtils'; +import { exportChart } from '../exploreUtils'; import ExploreAdditionalActionsMenu from './ExploreAdditionalActionsMenu'; import { ExportToCSVDropdown } from './ExportToCSVDropdown'; @@ -104,14 +103,12 @@ const ExploreActionButtons = (props: ExploreActionButtonsProps) => { const copyTooltipText = t('Copy chart URL to clipboard'); const [copyTooltip, setCopyTooltip] = useState(copyTooltipText); - const longUrl = getExploreLongUrl(latestQueryFormData); - const getShortUrl = useUrlShortener(longUrl); const doCopyLink = async () => { try { setCopyTooltip(t('Loading...')); - const shortUrl = await getShortUrl(); - await copyTextToClipboard(shortUrl); + const url = window.location.href; + await copyTextToClipboard(url); setCopyTooltip(t('Copied to clipboard!')); addSuccessToast(t('Copied to clipboard!')); } catch (error) { @@ -123,8 +120,8 @@ const ExploreActionButtons = (props: ExploreActionButtonsProps) => { const doShareEmail = async () => { try { const subject = t('Superset Chart'); - const shortUrl = await getShortUrl(); - const body = t('%s%s', 'Check out this chart: ', shortUrl); + const url = window.location.href; + const body = encodeURIComponent(t('%s%s', 'Check out this chart: ', url)); window.location.href = `mailto:?Subject=${subject}%20&Body=${body}`; } catch (error) { addDangerToast(t('Sorry, something went wrong. Try again later.')); @@ -179,7 +176,7 @@ const ExploreActionButtons = (props: ExploreActionButtonsProps) => { tooltip={t('Share chart by email')} onClick={doShareEmail} /> - + } text=".JSON" diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 6f73361dea..bb1fde9c30 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -56,7 +56,6 @@ const CHART_STATUS_MAP = { const propTypes = { actions: PropTypes.object.isRequired, - addHistory: PropTypes.func, can_overwrite: PropTypes.bool.isRequired, can_download: PropTypes.bool.isRequired, dashboardId: PropTypes.number, diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.jsx index 70c62941c1..9df7fdde36 100644 --- a/superset-frontend/src/explore/components/ExploreChartPanel.jsx +++ b/superset-frontend/src/explore/components/ExploreChartPanel.jsx @@ -34,7 +34,6 @@ import { buildV1ChartDataPayload } from '../exploreUtils'; const propTypes = { actions: PropTypes.object.isRequired, - addHistory: PropTypes.func, onQuery: PropTypes.func, can_overwrite: PropTypes.bool.isRequired, can_download: PropTypes.bool.isRequired, @@ -288,7 +287,6 @@ const ExploreChartPanel = props => { ({ + __esModule: true, + useResizeDetector: () => ({ height: 100, width: 100 }), +})); + +jest.mock('lodash/debounce', () => ({ + __esModule: true, + default: (fuc: Function) => fuc, +})); + +fetchMock.post('glob:*/api/v1/explore/form_data*', { key }); +fetchMock.put('glob:*/api/v1/explore/form_data*', { key }); +fetchMock.get('glob:*/api/v1/explore/form_data*', {}); + +const renderWithRouter = (withKey?: boolean) => { + const path = '/superset/explore/'; + const search = withKey ? `?form_data_key=${key}` : ''; + return render( + + + + + , + { useRedux: true, initialState: reduxState }, + ); +}; + +test('generates a new form_data param when none is available', async () => { + const replaceState = jest.spyOn(window.history, 'replaceState'); + await waitFor(() => renderWithRouter()); + expect(replaceState).toHaveBeenCalledWith( + expect.anything(), + undefined, + expect.stringMatching('form_data'), + ); + replaceState.mockRestore(); +}); + +test('generates a different form_data param when one is provided and is mounting', async () => { + const replaceState = jest.spyOn(window.history, 'replaceState'); + await waitFor(() => renderWithRouter(true)); + expect(replaceState).not.toHaveBeenLastCalledWith( + 0, + expect.anything(), + undefined, + expect.stringMatching(key), + ); + replaceState.mockRestore(); +}); + +test('reuses the same form_data param when updating', async () => { + const replaceState = jest.spyOn(window.history, 'replaceState'); + const pushState = jest.spyOn(window.history, 'pushState'); + await waitFor(() => renderWithRouter()); + expect(replaceState.mock.calls.length).toBe(1); + userEvent.click(screen.getByText('Run')); + await waitFor(() => expect(pushState.mock.calls.length).toBe(1)); + expect(replaceState.mock.calls[0]).toEqual(pushState.mock.calls[0]); + replaceState.mockRestore(); + pushState.mockRestore(); +}); diff --git a/superset-frontend/src/explore/components/ExploreViewContainer.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx similarity index 90% rename from superset-frontend/src/explore/components/ExploreViewContainer.jsx rename to superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index c8761ff9af..dc212cebcd 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -21,7 +21,7 @@ import React, { useCallback, useEffect, useMemo, useState } from 'react'; import PropTypes from 'prop-types'; import { bindActionCreators } from 'redux'; import { connect } from 'react-redux'; -import { styled, t, css, useTheme } from '@superset-ui/core'; +import { styled, t, css, useTheme, logging } from '@superset-ui/core'; import { debounce } from 'lodash'; import { Resizable } from 're-resizable'; @@ -37,26 +37,28 @@ import { LocalStorageKeys, } from 'src/utils/localStorageHelpers'; import { URL_PARAMS } from 'src/constants'; +import { getUrlParam } from 'src/utils/urlUtils'; import cx from 'classnames'; import * as chartActions from 'src/chart/chartAction'; import { fetchDatasourceMetadata } from 'src/dashboard/actions/datasources'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import { mergeExtraFormData } from 'src/dashboard/components/nativeFilters/utils'; -import ExploreChartPanel from './ExploreChartPanel'; -import ConnectedControlPanelsContainer from './ControlPanelsContainer'; -import SaveModal from './SaveModal'; -import QueryAndSaveBtns from './QueryAndSaveBtns'; -import DataSourcePanel from './DatasourcePanel'; -import { getExploreLongUrl } from '../exploreUtils'; -import { areObjectsEqual } from '../../reduxUtils'; -import { getFormDataFromControls } from '../controlUtils'; -import * as exploreActions from '../actions/exploreActions'; -import * as saveModalActions from '../actions/saveModalActions'; -import * as logActions from '../../logger/actions'; +import { postFormData, putFormData } from 'src/explore/exploreUtils/formData'; +import ExploreChartPanel from '../ExploreChartPanel'; +import ConnectedControlPanelsContainer from '../ControlPanelsContainer'; +import SaveModal from '../SaveModal'; +import QueryAndSaveBtns from '../QueryAndSaveBtns'; +import DataSourcePanel from '../DatasourcePanel'; +import { mountExploreUrl } from '../../exploreUtils'; +import { areObjectsEqual } from '../../../reduxUtils'; +import { getFormDataFromControls } from '../../controlUtils'; +import * as exploreActions from '../../actions/exploreActions'; +import * as saveModalActions from '../../actions/saveModalActions'; +import * as logActions from '../../../logger/actions'; import { LOG_ACTIONS_MOUNT_EXPLORER, LOG_ACTIONS_CHANGE_EXPLORE_CONTROLS, -} from '../../logger/LogUtils'; +} from '../../../logger/LogUtils'; const propTypes = { ...ExploreChartPanel.propTypes, @@ -161,6 +163,37 @@ function useWindowSize({ delayMs = 250 } = {}) { return size; } +const updateHistory = debounce( + async (formData, datasetId, isReplace, standalone, force, title) => { + const payload = { ...formData }; + const chartId = formData.slice_id; + + try { + let key; + let stateModifier; + if (isReplace) { + key = await postFormData(datasetId, formData, chartId); + stateModifier = 'replaceState'; + } else { + key = getUrlParam(URL_PARAMS.formDataKey); + await putFormData(datasetId, key, formData, chartId); + stateModifier = 'pushState'; + } + const url = mountExploreUrl( + standalone ? URL_PARAMS.standalone.name : null, + { + [URL_PARAMS.formDataKey.name]: key, + }, + force, + ); + window.history[stateModifier](payload, title, url); + } catch (e) { + logging.warn('Failed at altering browser history', e); + } + }, + 1000, +); + function ExploreViewContainer(props) { const dynamicPluginContext = usePluginContext(); const dynamicPlugin = dynamicPluginContext.dynamicPlugins[props.vizType]; @@ -191,39 +224,31 @@ function ExploreViewContainer(props) { }; const addHistory = useCallback( - ({ isReplace = false, title } = {}) => { + async ({ isReplace = false, title } = {}) => { const formData = props.dashboardId ? { ...props.form_data, dashboardId: props.dashboardId, } : props.form_data; - const payload = { ...formData }; - const longUrl = getExploreLongUrl( - formData, - props.standalone ? URL_PARAMS.standalone.name : null, - false, - {}, - props.force, - ); + const datasetId = props.datasource.id; - try { - if (isReplace) { - window.history.replaceState(payload, title, longUrl); - } else { - window.history.pushState(payload, title, longUrl); - } - } catch (e) { - // eslint-disable-next-line no-console - console.warn( - 'Failed at altering browser history', - payload, - title, - longUrl, - ); - } + updateHistory( + formData, + datasetId, + isReplace, + props.standalone, + props.force, + title, + ); }, - [props.form_data, props.standalone, props.force], + [ + props.dashboardId, + props.form_data, + props.datasource.id, + props.standalone, + props.force, + ], ); const handlePopstate = useCallback(() => { @@ -447,7 +472,6 @@ function ExploreViewContainer(props) { {...props} errorMessage={renderErrorMessage()} refreshOverlayVisible={chartIsStale} - addHistory={addHistory} onQuery={onQuery} /> ); diff --git a/superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx b/superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx index 2c1eb036e3..f52b575e2d 100644 --- a/superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx +++ b/superset-frontend/src/explore/exploreUtils/exploreUtils.test.jsx @@ -22,7 +22,6 @@ import URI from 'urijs'; import { buildV1ChartDataPayload, getExploreUrl, - getExploreLongUrl, shouldUseLegacyApi, getSimpleSQLExpression, } from 'src/explore/exploreUtils'; @@ -35,7 +34,6 @@ describe('exploreUtils', () => { const formData = { datasource: '1__table', }; - const sFormData = JSON.stringify(formData); function compareURI(uri1, uri2) { expect(uri1.toString()).toBe(uri2.toString()); } @@ -191,25 +189,6 @@ describe('exploreUtils', () => { }); }); - describe('getExploreLongUrl', () => { - it('generates proper base url with form_data', () => { - compareURI( - URI(getExploreLongUrl(formData, 'base')), - URI('/superset/explore/').search({ form_data: sFormData }), - ); - }); - - it('generates url with standalone', () => { - compareURI( - URI(getExploreLongUrl(formData, 'standalone')), - URI('/superset/explore/').search({ - form_data: sFormData, - standalone: DashboardStandaloneMode.HIDE_NAV, - }), - ); - }); - }); - describe('buildV1ChartDataPayload', () => { it('generate valid request payload despite no registered buildQuery', () => { const v1RequestPayload = buildV1ChartDataPayload({ diff --git a/superset-frontend/src/explore/exploreUtils/formData.ts b/superset-frontend/src/explore/exploreUtils/formData.ts new file mode 100644 index 0000000000..25e77c33fe --- /dev/null +++ b/superset-frontend/src/explore/exploreUtils/formData.ts @@ -0,0 +1,61 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { SupersetClient, JsonObject } from '@superset-ui/core'; + +type Payload = { + dataset_id: number; + form_data: string; + chart_id?: number; +}; + +const assemblePayload = ( + datasetId: number, + form_data: JsonObject, + chartId?: number, +) => { + const payload: Payload = { + dataset_id: datasetId, + form_data: JSON.stringify(form_data), + }; + if (chartId) { + payload.chart_id = chartId; + } + return payload; +}; + +export const postFormData = ( + datasetId: number, + form_data: JsonObject, + chartId?: number, +): Promise => + SupersetClient.post({ + endpoint: 'api/v1/explore/form_data', + jsonPayload: assemblePayload(datasetId, form_data, chartId), + }).then(r => r.json.key); + +export const putFormData = ( + datasetId: number, + key: string, + form_data: JsonObject, + chartId?: number, +): Promise => + SupersetClient.put({ + endpoint: `api/v1/explore/form_data/${key}`, + jsonPayload: assemblePayload(datasetId, form_data, chartId), + }).then(r => r.json.message); diff --git a/superset-frontend/src/explore/exploreUtils/getExploreLongUrl.test.ts b/superset-frontend/src/explore/exploreUtils/getExploreLongUrl.test.ts deleted file mode 100644 index 5672b26187..0000000000 --- a/superset-frontend/src/explore/exploreUtils/getExploreLongUrl.test.ts +++ /dev/null @@ -1,134 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -import { getExploreLongUrl, getExploreUrlFromDashboard } from '.'; - -const createParams = () => ({ - formData: { - datasource: 'datasource', - viz_type: 'viz_type', - }, - endpointType: 'endpointType', - allowOverflow: true, - extraSearch: { same: 'any-string' }, -}); - -test('Should return null if formData.datasource is falsy', () => { - const params = createParams(); - expect( - getExploreLongUrl( - {}, - params.endpointType, - params.allowOverflow, - params.extraSearch, - ), - ).toBeNull(); -}); - -test('Get url when endpointType:standalone', () => { - const params = createParams(); - expect( - getExploreLongUrl( - params.formData, - 'standalone', - params.allowOverflow, - params.extraSearch, - ), - ).toBe( - '/superset/explore/?same=any-string&form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%7D&standalone=1', - ); -}); - -test('Get url when endpointType:standalone and force:true', () => { - const params = createParams(); - expect( - getExploreLongUrl( - params.formData, - 'standalone', - params.allowOverflow, - params.extraSearch, - true, - ), - ).toBe( - '/superset/explore/?same=any-string&form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%7D&force=1&standalone=1', - ); -}); - -test('Get url when endpointType:standalone and allowOverflow:false', () => { - const params = createParams(); - expect( - getExploreLongUrl( - params.formData, - params.endpointType, - false, - params.extraSearch, - ), - ).toBe( - '/superset/explore/?same=any-string&form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%7D', - ); -}); - -test('Get url when endpointType:results', () => { - const params = createParams(); - expect( - getExploreLongUrl( - params.formData, - 'results', - params.allowOverflow, - params.extraSearch, - ), - ).toBe( - '/superset/explore_json/?same=any-string&form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%7D', - ); -}); - -test('Get url when endpointType:results and allowOverflow:false', () => { - const params = createParams(); - expect( - getExploreLongUrl(params.formData, 'results', false, params.extraSearch), - ).toBe( - '/superset/explore_json/?same=any-string&form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%7D', - ); -}); - -test('Get url from a dashboard', () => { - const formData = { - ...createParams().formData, - // these params should get filtered out - extra_form_data: { - filters: { - col: 'foo', - op: 'IN', - val: ['bar'], - }, - }, - dataMask: { - 'NATIVE_FILTER-bqEoUsEPe': { - id: 'NATIVE_FILTER-bqEoUsEPe', - lots: 'of other stuff here too', - }, - }, - url_params: { - native_filters: '(blah)', - standalone: true, - }, - }; - expect(getExploreUrlFromDashboard(formData)).toBe( - '/superset/explore/?form_data=%7B%22datasource%22%3A%22datasource%22%2C%22viz_type%22%3A%22viz_type%22%2C%22extra_form_data%22%3A%7B%22filters%22%3A%7B%22col%22%3A%22foo%22%2C%22op%22%3A%22IN%22%2C%22val%22%3A%5B%22bar%22%5D%7D%7D%7D', - ); -}); diff --git a/superset-frontend/src/explore/exploreUtils/index.js b/superset-frontend/src/explore/exploreUtils/index.js index 9e739351d5..679902243b 100644 --- a/superset-frontend/src/explore/exploreUtils/index.js +++ b/superset-frontend/src/explore/exploreUtils/index.js @@ -18,7 +18,6 @@ */ import { useCallback, useEffect } from 'react'; -import { omit } from 'lodash'; /* eslint camelcase: 0 */ import URI from 'urijs'; import { @@ -37,8 +36,6 @@ import { import { DashboardStandaloneMode } from 'src/dashboard/util/constants'; import { optionLabel } from '../../utils/common'; -const MAX_URL_LENGTH = 8000; - export function getChartKey(explore) { const { slice } = explore; return slice ? slice.slice_id : 0; @@ -91,64 +88,20 @@ export function getURIDirectory(endpointType = 'base') { return '/superset/explore/'; } -/** - * This gets the url of the explore page, with all the form data included explicitly. - * This includes any form data overrides from the dashboard. - */ -export function getExploreLongUrl( - formData, - endpointType, - allowOverflow = true, - extraSearch = {}, - force = false, -) { - if (!formData.datasource) { - return null; - } - +export function mountExploreUrl(endpointType, extraSearch = {}, force = false) { const uri = new URI('/'); const directory = getURIDirectory(endpointType); const search = uri.search(true); Object.keys(extraSearch).forEach(key => { search[key] = extraSearch[key]; }); - search.form_data = safeStringify(formData); if (endpointType === URL_PARAMS.standalone.name) { if (force) { search.force = '1'; } search.standalone = DashboardStandaloneMode.HIDE_NAV; } - const url = uri.directory(directory).search(search).toString(); - if (!allowOverflow && url.length > MAX_URL_LENGTH) { - const minimalFormData = { - datasource: formData.datasource, - viz_type: formData.viz_type, - }; - return getExploreLongUrl( - minimalFormData, - endpointType, - false, - { - URL_IS_TOO_LONG_TO_SHARE: null, - }, - force, - ); - } - return url; -} - -export function getExploreUrlFromDashboard(formData) { - // remove formData params that we don't need in the explore url. - // These are present when generating explore urls from the dashboard page. - // This should be superseded by some sort of "exploration context" system - // where form data and other context is referenced by id. - const trimmedFormData = omit(formData, [ - 'dataMask', - 'url_params', - 'label_colors', - ]); - return getExploreLongUrl(trimmedFormData, null, false); + return uri.directory(directory).search(search).toString(); } export function getChartDataUri({ path, qs, allowDomainSharding = false }) { diff --git a/superset/views/core.py b/superset/views/core.py index 81952e657f..f2368e3441 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -87,6 +87,8 @@ from superset.exceptions import ( SupersetSecurityException, SupersetTimeoutException, ) +from superset.explore.form_data.commands.get import GetFormDataCommand +from superset.explore.form_data.commands.parameters import CommandParameters from superset.extensions import async_query_manager, cache_manager from superset.jinja_context import get_template_processor from superset.models.core import Database, FavStar, Log @@ -730,7 +732,19 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods def explore( self, datasource_type: Optional[str] = None, datasource_id: Optional[int] = None ) -> FlaskResponse: - form_data, slc = get_form_data(use_slice_data=True) + initial_form_data = {} + + form_data_key = request.args.get("form_data_key") + if form_data_key: + parameters = CommandParameters(actor=g.user, key=form_data_key,) + value = GetFormDataCommand(parameters).run() + if value: + initial_form_data = json.loads(value) + + form_data, slc = get_form_data( + use_slice_data=True, initial_form_data=initial_form_data + ) + query_context = request.form.get("query_context") # Flash the SIP-15 message if the slice is owned by the current user and has not # been updated, i.e., is not using the [start, end) interval. diff --git a/superset/views/utils.py b/superset/views/utils.py index 15b312d39d..eda24863e1 100644 --- a/superset/views/utils.py +++ b/superset/views/utils.py @@ -137,9 +137,11 @@ def loads_request_json(request_json_data: str) -> Dict[Any, Any]: def get_form_data( # pylint: disable=too-many-locals - slice_id: Optional[int] = None, use_slice_data: bool = False + slice_id: Optional[int] = None, + use_slice_data: bool = False, + initial_form_data: Optional[Dict[str, Any]] = None, ) -> Tuple[Dict[str, Any], Optional[Slice]]: - form_data: Dict[str, Any] = {} + form_data: Dict[str, Any] = initial_form_data or {} if has_request_context(): # type: ignore # chart data API requests are JSON