refactor: simplify getExploreUrl functions (#9831)

* remove payload from return signature

* Rename function and fix tests

* Lint

* fix tests

* Move useLegacyApi inquiry to exploreUtils

superset-frontend/spec/javascripts/chart/chartActions_spec.js

@@ -47,8 +47,8 @@ describe('chart actions', () => {
   beforeEach(() => {
     dispatch = sinon.spy();
     urlStub = sinon
-      .stub(exploreUtils, 'getExploreUrlAndPayload')
-      .callsFake(() => ({ url: MOCK_URL, payload: {} }));
+      .stub(exploreUtils, 'getExploreUrl')
+      .callsFake(() => MOCK_URL);
     fakeMetadata = { useLegacyApi: true };
     metadataRegistryStub = sinon
       .stub(chartlib, 'getChartMetadataRegistry')

superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx

@@ -140,9 +140,7 @@ describe('SaveModal', () => {
 
   describe('saveOrOverwrite', () => {
     beforeEach(() => {
-      sinon
-        .stub(exploreUtils, 'getExploreUrlAndPayload')
-        .callsFake(() => ({ url: 'mockURL', payload: defaultProps.form_data }));
+      sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => 'mockURL');
 
       sinon.stub(defaultProps.actions, 'saveSlice').callsFake(() =>
         Promise.resolve({
@@ -155,7 +153,7 @@ describe('SaveModal', () => {
     });
 
     afterEach(() => {
-      exploreUtils.getExploreUrlAndPayload.restore();
+      exploreUtils.getExploreUrl.restore();
       defaultProps.actions.saveSlice.restore();
     });
 

superset-frontend/spec/javascripts/explore/utils_spec.jsx

@@ -19,10 +19,7 @@
 import sinon from 'sinon';
 
 import URI from 'urijs';
-import {
-  getExploreUrlAndPayload,
-  getExploreLongUrl,
-} from 'src/explore/exploreUtils';
+import { getExploreUrl, getExploreLongUrl } from 'src/explore/exploreUtils';
 import * as hostNamesConfig from 'src/utils/hostNamesConfig';
 
 describe('exploreUtils', () => {
@@ -35,33 +32,31 @@ describe('exploreUtils', () => {
     expect(uri1.toString()).toBe(uri2.toString());
   }
 
-  describe('getExploreUrlAndPayload', () => {
+  describe('getExploreUrl', () => {
     it('generates proper base url', () => {
       // This assertion is to show clearly the value of location.href
       // in the context of unit tests.
       expect(location.href).toBe('http://localhost/');
 
-      const { url, payload } = getExploreUrlAndPayload({
+      const url = getExploreUrl({
         formData,
         endpointType: 'base',
         force: false,
         curUrl: 'http://superset.com',
       });
       compareURI(URI(url), URI('/superset/explore/'));
-      expect(payload).toEqual(formData);
     });
     it('generates proper json url', () => {
-      const { url, payload } = getExploreUrlAndPayload({
+      const url = getExploreUrl({
         formData,
         endpointType: 'json',
         force: false,
         curUrl: 'http://superset.com',
       });
       compareURI(URI(url), URI('/superset/explore_json/'));
-      expect(payload).toEqual(formData);
     });
     it('generates proper json forced url', () => {
-      const { url, payload } = getExploreUrlAndPayload({
+      const url = getExploreUrl({
         formData,
         endpointType: 'json',
         force: true,
@@ -71,10 +66,9 @@ describe('exploreUtils', () => {
         URI(url),
         URI('/superset/explore_json/').search({ force: 'true' }),
       );
-      expect(payload).toEqual(formData);
     });
     it('generates proper csv URL', () => {
-      const { url, payload } = getExploreUrlAndPayload({
+      const url = getExploreUrl({
         formData,
         endpointType: 'csv',
         force: false,
@@ -84,10 +78,9 @@ describe('exploreUtils', () => {
         URI(url),
         URI('/superset/explore_json/').search({ csv: 'true' }),
       );
-      expect(payload).toEqual(formData);
     });
     it('generates proper standalone URL', () => {
-      const { url, payload } = getExploreUrlAndPayload({
+      const url = getExploreUrl({
         formData,
         endpointType: 'standalone',
         force: false,
@@ -97,10 +90,9 @@ describe('exploreUtils', () => {
         URI(url),
         URI('/superset/explore/').search({ standalone: 'true' }),
       );
-      expect(payload).toEqual(formData);
     });
     it('preserves main URLs params', () => {
-      const { url, payload } = getExploreUrlAndPayload({
+      const url = getExploreUrl({
         formData,
         endpointType: 'json',
         force: false,
@@ -110,10 +102,9 @@ describe('exploreUtils', () => {
         URI(url),
         URI('/superset/explore_json/').search({ foo: 'bar' }),
       );
-      expect(payload).toEqual(formData);
     });
     it('generate proper save slice url', () => {
-      const { url, payload } = getExploreUrlAndPayload({
+      const url = getExploreUrl({
         formData,
         endpointType: 'json',
         force: false,
@@ -123,20 +114,6 @@ describe('exploreUtils', () => {
         URI(url),
         URI('/superset/explore_json/').search({ foo: 'bar' }),
       );
-      expect(payload).toEqual(formData);
-    });
-    it('generate proper saveas slice url', () => {
-      const { url, payload } = getExploreUrlAndPayload({
-        formData,
-        endpointType: 'json',
-        force: false,
-        curUrl: 'superset.com?foo=bar',
-      });
-      compareURI(
-        URI(url),
-        URI('/superset/explore_json/').search({ foo: 'bar' }),
-      );
-      expect(payload).toEqual(formData);
     });
   });
 
@@ -158,48 +135,48 @@ describe('exploreUtils', () => {
     });
 
     it('generate url to different domains', () => {
-      let url = getExploreUrlAndPayload({
+      let url = getExploreUrl({
         formData,
         endpointType: 'json',
         allowDomainSharding: true,
-      }).url;
+      });
       // skip main domain for fetching chart if domain sharding is enabled
       // to leave main domain free for other calls like fav star, save change, etc.
       expect(url).toMatch(availableDomains[1]);
 
-      url = getExploreUrlAndPayload({
+      url = getExploreUrl({
         formData,
         endpointType: 'json',
         allowDomainSharding: true,
-      }).url;
+      });
       expect(url).toMatch(availableDomains[2]);
 
-      url = getExploreUrlAndPayload({
+      url = getExploreUrl({
         formData,
         endpointType: 'json',
         allowDomainSharding: true,
-      }).url;
+      });
       expect(url).toMatch(availableDomains[3]);
 
       // circle back to first available domain
-      url = getExploreUrlAndPayload({
+      url = getExploreUrl({
         formData,
         endpointType: 'json',
         allowDomainSharding: true,
-      }).url;
+      });
       expect(url).toMatch(availableDomains[1]);
     });
     it('not generate url to different domains without flag', () => {
-      let csvURL = getExploreUrlAndPayload({
+      let csvURL = getExploreUrl({
         formData,
         endpointType: 'csv',
-      }).url;
+      });
       expect(csvURL).toMatch(availableDomains[0]);
 
-      csvURL = getExploreUrlAndPayload({
+      csvURL = getExploreUrl({
         formData,
         endpointType: 'csv',
-      }).url;
+      });
       expect(csvURL).toMatch(availableDomains[0]);
     });
   });

superset-frontend/spec/javascripts/sqllab/ExploreResultsButton_spec.jsx

@@ -177,17 +177,14 @@ describe('ExploreResultsButton', () => {
     fetchMock.post(visualizeEndpoint, visualizationPayload);
 
     beforeEach(() => {
-      sinon.stub(exploreUtils, 'getExploreUrlAndPayload').callsFake(() => ({
-        url: 'mockURL',
-        payload: { datasource: '107__table' },
-      }));
+      sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => 'mockURL');
       sinon.spy(exploreUtils, 'exportChart');
       sinon
         .stub(wrapper.instance(), 'buildVizOptions')
         .callsFake(() => mockOptions);
     });
     afterEach(() => {
-      exploreUtils.getExploreUrlAndPayload.restore();
+      exploreUtils.getExploreUrl.restore();
       exploreUtils.exportChart.restore();
       wrapper.instance().buildVizOptions.restore();
       fetchMock.reset();

superset-frontend/src/chart/chartAction.js

@@ -20,14 +20,12 @@
 /* eslint no-param-reassign: ["error", { "props": false }] */
 import moment from 'moment';
 import { t } from '@superset-ui/translation';
-import {
-  getChartBuildQueryRegistry,
-  getChartMetadataRegistry,
-} from '@superset-ui/chart';
+import { getChartBuildQueryRegistry } from '@superset-ui/chart';
 import { SupersetClient } from '@superset-ui/connection';
 import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
 import {
-  getExploreUrlAndPayload,
+  shouldUseLegacyApi,
+  getExploreUrl,
   getAnnotationJsonUrl,
   postForm,
 } from '../explore/exploreUtils';
@@ -213,7 +211,7 @@ function legacyChartDataRequest(
   dashboardId,
   requestParams,
 ) {
-  const { url, payload } = getExploreUrlAndPayload({
+  const url = getExploreUrl({
     formData,
     endpointType: 'json',
     force,
@@ -224,7 +222,7 @@ function legacyChartDataRequest(
   const querySettings = {
     ...requestParams,
     url,
-    postPayload: { form_data: payload },
+    postPayload: { form_data: formData },
   };
 
   const clientMethod =
@@ -260,7 +258,7 @@ export function exploreJSON(
     const logStart = Logger.getTimestamp();
     const controller = new AbortController();
 
-    const { useLegacyApi } = getChartMetadataRegistry().get(formData.viz_type);
+    const useLegacyApi = shouldUseLegacyApi(formData);
 
     let requestParams = {
       signal: controller.signal,
@@ -400,7 +398,7 @@ export function postChartFormData(
 
 export function redirectSQLLab(formData) {
   return dispatch => {
-    const { url } = getExploreUrlAndPayload({
+    const url = getExploreUrl({
       formData,
       endpointType: 'query',
     });

superset-frontend/src/explore/actions/saveModalActions.js

@@ -17,7 +17,7 @@
  * under the License.
  */
 import { SupersetClient } from '@superset-ui/connection';
-import { getExploreUrlAndPayload } from '../exploreUtils';
+import { getExploreUrl } from '../exploreUtils';
 
 export const FETCH_DASHBOARDS_SUCCEEDED = 'FETCH_DASHBOARDS_SUCCEEDED';
 export function fetchDashboardsSucceeded(choices) {
@@ -62,7 +62,7 @@ export function removeSaveModalAlert() {
 
 export function saveSlice(formData, requestParams) {
   return dispatch => {
-    const { url, payload } = getExploreUrlAndPayload({
+    const url = getExploreUrl({
       formData,
       endpointType: 'base',
       force: false,
@@ -70,7 +70,7 @@ export function saveSlice(formData, requestParams) {
       requestParams,
     });
 
-    return SupersetClient.post({ url, postPayload: { form_data: payload } })
+    return SupersetClient.post({ url, postPayload: { form_data: formData } })
       .then(({ json }) => dispatch(saveSliceSuccess(json)))
       .catch(() => dispatch(saveSliceFailed()));
   };

superset-frontend/src/explore/components/DisplayQueryButton.jsx

@@ -41,7 +41,7 @@ import { SupersetClient } from '@superset-ui/connection';
 
 import getClientErrorObject from '../../utils/getClientErrorObject';
 import CopyToClipboard from './../../components/CopyToClipboard';
-import { getExploreUrlAndPayload } from '../exploreUtils';
+import { getExploreUrl } from '../exploreUtils';
 
 import Loading from '../../components/Loading';
 import ModalTrigger from './../../components/ModalTrigger';
@@ -89,13 +89,13 @@ export class DisplayQueryButton extends React.PureComponent {
   }
   beforeOpen(endpointType) {
     this.setState({ isLoading: true });
-    const { url, payload } = getExploreUrlAndPayload({
+    const url = getExploreUrl({
       formData: this.props.latestQueryFormData,
       endpointType,
     });
     SupersetClient.post({
       url,
-      postPayload: { form_data: payload },
+      postPayload: { form_data: this.props.latestQueryFormData },
     })
       .then(({ json }) => {
         this.setState({

superset-frontend/src/explore/components/ExploreViewContainer.jsx

@@ -27,7 +27,7 @@ import ExploreChartPanel from './ExploreChartPanel';
 import ControlPanelsContainer from './ControlPanelsContainer';
 import SaveModal from './SaveModal';
 import QueryAndSaveBtns from './QueryAndSaveBtns';
-import { getExploreUrlAndPayload, getExploreLongUrl } from '../exploreUtils';
+import { getExploreUrl, getExploreLongUrl } from '../exploreUtils';
 import { areObjectsEqual } from '../../reduxUtils';
 import { getFormDataFromControls } from '../controlUtils';
 import { chartPropShape } from '../../dashboard/util/propShapes';
@@ -231,9 +231,7 @@ class ExploreViewContainer extends React.Component {
   }
 
   addHistory({ isReplace = false, title }) {
-    const { payload } = getExploreUrlAndPayload({
-      formData: this.props.form_data,
-    });
+    const payload = { ...this.props.form_data };
     const longUrl = getExploreLongUrl(this.props.form_data, null, false);
     try {
       if (isReplace) {

superset-frontend/src/explore/exploreUtils.js

@@ -18,6 +18,7 @@
  */
 /* eslint camelcase: 0 */
 import URI from 'urijs';
+import { getChartMetadataRegistry } from '@superset-ui/chart';
 import { availableDomains } from '../utils/hostNamesConfig';
 import { safeStringify } from '../utils/safeStringify';
 
@@ -63,15 +64,14 @@ export function getAnnotationJsonUrl(slice_id, form_data, isNative) {
     .toString();
 }
 
-export function getURIDirectory(formData, endpointType = 'base') {
+export function getURIDirectory(endpointType = 'base') {
   // Building the directory part of the URI
-  let directory = '/superset/explore/';
   if (
     ['json', 'csv', 'query', 'results', 'samples'].indexOf(endpointType) >= 0
   ) {
-    directory = '/superset/explore_json/';
+    return '/superset/explore_json/';
   }
-  return directory;
+  return '/superset/explore/';
 }
 
 export function getExploreLongUrl(
@@ -85,7 +85,7 @@ export function getExploreLongUrl(
   }
 
   const uri = new URI('/');
-  const directory = getURIDirectory(formData, endpointType);
+  const directory = getURIDirectory(endpointType);
   const search = uri.search(true);
   Object.keys(extraSearch).forEach(key => {
     search[key] = extraSearch[key];
@@ -107,7 +107,12 @@ export function getExploreLongUrl(
   return url;
 }
 
-export function getExploreUrlAndPayload({
+export function shouldUseLegacyApi(formData) {
+  const { useLegacyApi } = getChartMetadataRegistry().get(formData.viz_type);
+  return useLegacyApi || false;
+}
+
+export function getExploreUrl({
   formData,
   endpointType = 'base',
   force = false,
@@ -134,7 +139,7 @@ export function getExploreUrlAndPayload({
     uri = URI(URI(curUrl).search());
   }
 
-  const directory = getURIDirectory(formData, endpointType);
+  const directory = getURIDirectory(endpointType);
 
   // Building the querystring (search) part of the URI
   const search = uri.search(true);
@@ -178,13 +183,7 @@ export function getExploreUrlAndPayload({
       }
     });
   }
-  uri = uri.search(search).directory(directory);
-  const payload = { ...formData };
-
-  return {
-    url: uri.toString(),
-    payload,
-  };
+  return uri.search(search).directory(directory).toString();
 }
 
 export function postForm(url, payload, target = '_blank') {
@@ -213,10 +212,10 @@ export function postForm(url, payload, target = '_blank') {
 }
 
 export function exportChart(formData, endpointType) {
-  const { url, payload } = getExploreUrlAndPayload({
+  const url = getExploreUrl({
     formData,
     endpointType,
     allowDomainSharding: false,
   });
-  postForm(url, payload);
+  postForm(url, formData);
 }