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.

Sign up for GitHub

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

Jump to bottom

refactor: simplify getExploreUrl functions #9831

Merged
merged 5 commits into from May 18, 2020
Merged

refactor: simplify getExploreUrl functions #9831

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16bdb7a
remove payload from return signature
villebro May 15, 2020
52354b2
Rename function and fix tests
villebro May 18, 2020
adc35d2
Lint
villebro May 18, 2020
fe93273
fix tests
villebro May 18, 2020
4e02f4e
Move useLegacyApi inquiry to exploreUtils
villebro May 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions superset-frontend/spec/javascripts/chart/chartActions_spec.js
View file
Open in desktop
Expand Up @@ -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')
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/spec/javascripts/explore/components/SaveModal_spec.jsx
View file
Open in desktop
Expand Up @@ -141,8 +141,8 @@ describe('SaveModal', () => {
describe('saveOrOverwrite', () => {
beforeEach(() => {
sinon
.stub(exploreUtils, 'getExploreUrlAndPayload')
.callsFake(() => ({ url: 'mockURL', payload: defaultProps.form_data }));
.stub(exploreUtils, 'getExploreUrl')
.callsFake(() => 'mockURL');

sinon.stub(defaultProps.actions, 'saveSlice').callsFake(() =>
Promise.resolve({
Expand All @@ -155,7 +155,7 @@ describe('SaveModal', () => {
});

afterEach(() => {
exploreUtils.getExploreUrlAndPayload.restore();
exploreUtils.getExploreUrl.restore();
defaultProps.actions.saveSlice.restore();
});

Expand Down
50 changes: 15 additions & 35 deletions superset-frontend/spec/javascripts/explore/utils_spec.jsx
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ import sinon from 'sinon';

import URI from 'urijs';
import {
getExploreUrlAndPayload,
getExploreUrl,
getExploreLongUrl,
} from 'src/explore/exploreUtils';
import * as hostNamesConfig from 'src/utils/hostNamesConfig';
Expand All @@ -35,33 +35,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,
Expand All @@ -71,10 +69,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,
Expand All @@ -84,10 +81,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,
Expand All @@ -97,10 +93,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,
Expand All @@ -110,10 +105,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,
Expand All @@ -123,20 +117,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);
Comment on lines -126 to -139
Copy link
Member Author

Choose a reason for hiding this comment

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

This was an identical copy of the test case above.

});
});

Expand All @@ -158,7 +138,7 @@ describe('exploreUtils', () => {
});

it('generate url to different domains', () => {
let url = getExploreUrlAndPayload({
let url = getExploreUrl({
formData,
endpointType: 'json',
allowDomainSharding: true,
Expand All @@ -167,36 +147,36 @@ describe('exploreUtils', () => {
// 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;
Expand Down
7 changes: 2 additions & 5 deletions superset-frontend/spec/javascripts/sqllab/ExploreResultsButton_spec.jsx
View file
Open in desktop
Expand Up @@ -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();
Expand Down
8 changes: 4 additions & 4 deletions superset-frontend/src/chart/chartAction.js
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@ import {
import { SupersetClient } from '@superset-ui/connection';
import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
import {
getExploreUrlAndPayload,
getExploreUrl,
getAnnotationJsonUrl,
postForm,
} from '../explore/exploreUtils';
Expand Down Expand Up @@ -213,7 +213,7 @@ function legacyChartDataRequest(
dashboardId,
requestParams,
) {
const { url, payload } = getExploreUrlAndPayload({
const url = getExploreUrl({
formData,
endpointType: 'json',
force,
Expand All @@ -224,7 +224,7 @@ function legacyChartDataRequest(
const querySettings = {
...requestParams,
url,
postPayload: { form_data: payload },
postPayload: { form_data: formData },
};

const clientMethod =
Expand Down Expand Up @@ -400,7 +400,7 @@ export function postChartFormData(

export function redirectSQLLab(formData) {
return dispatch => {
const { url } = getExploreUrlAndPayload({
const url = getExploreUrl({
formData,
endpointType: 'query',
});
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/src/explore/actions/saveModalActions.js
View file
Open in desktop
Expand Up @@ -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) {
Expand Down Expand Up @@ -62,15 +62,15 @@ export function removeSaveModalAlert() {

export function saveSlice(formData, requestParams) {
return dispatch => {
const { url, payload } = getExploreUrlAndPayload({
const url = getExploreUrl({
formData,
endpointType: 'base',
force: false,
curUrl: null,
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()));
};
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/src/explore/components/DisplayQueryButton.jsx
View file
Open in desktop
Expand Up @@ -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';
Expand Down Expand Up @@ -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({
Expand Down
6 changes: 2 additions & 4 deletions superset-frontend/src/explore/components/ExploreViewContainer.jsx
View file
Open in desktop
Expand Up @@ -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';
Expand Down Expand Up @@ -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) {
Expand Down
21 changes: 8 additions & 13 deletions superset-frontend/src/explore/exploreUtils.js
View file
Open in desktop
Expand Up @@ -63,7 +63,7 @@ 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 (
Expand All @@ -84,8 +84,9 @@ export function getExploreLongUrl(
return null;
}


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];
Expand All @@ -107,7 +108,7 @@ export function getExploreLongUrl(
return url;
}

export function getExploreUrlAndPayload({
export function getExploreUrl({
formData,
endpointType = 'base',
force = false,
Expand All @@ -134,7 +135,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);
Expand Down Expand Up @@ -178,13 +179,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') {
Expand Down Expand Up @@ -213,10 +208,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);
}