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

chore(explore): Update chart save to use API endpoints #20498

Merged
4 changes: 4 additions & 0 deletions superset-frontend/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ export const URL_PARAMS = {
name: 'show_database_modal',
type: 'boolean',
},
saveAction: {
name: 'save_action',
type: 'string',
},
} as const;

export const RESERVED_CHART_URL_PARAMS: string[] = [
Expand Down
52 changes: 30 additions & 22 deletions superset-frontend/src/explore/ExplorePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,50 +16,58 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useEffect, useState } from 'react';
import React, { useEffect, useRef, useState } from 'react';
import { useDispatch } from 'react-redux';
import { useLocation } from 'react-router-dom';
import { makeApi, t } from '@superset-ui/core';
import Loading from 'src/components/Loading';
import { addDangerToast } from 'src/components/MessageToasts/actions';
import { isNullish } from 'src/utils/common';
Copy link
Member

Choose a reason for hiding this comment

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

there is a util is isDefined in the @superset-ui/core

import { isDefined } from '@superset-ui/core';

Copy link
Member Author

Choose a reason for hiding this comment

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

@kgabryje I think this was your code.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I wasn't aware of isDefined. Feel free to refactor 🙂

import { getUrlParam } from 'src/utils/urlUtils';
import { URL_PARAMS } from 'src/constants';
import { getParsedExploreURLParams } from './exploreUtils/getParsedExploreURLParams';
import { hydrateExplore } from './actions/hydrateExplore';
import ExploreViewContainer from './components/ExploreViewContainer';
import { ExploreResponsePayload } from './types';
import { fallbackExploreInitialData } from './fixtures';
import { addDangerToast } from '../components/MessageToasts/actions';
import { isNullish } from '../utils/common';

const loadErrorMessage = t('Failed to load chart data.');

const fetchExploreData = () => {
const exploreUrlParams = getParsedExploreURLParams();
return makeApi<{}, ExploreResponsePayload>({
const fetchExploreData = (exploreUrlParams: URLSearchParams) =>
makeApi<{}, ExploreResponsePayload>({
method: 'GET',
endpoint: 'api/v1/explore/',
})(exploreUrlParams);
};

const ExplorePage = () => {
const [isLoaded, setIsLoaded] = useState(false);
const isExploreInitialized = useRef(false);
const dispatch = useDispatch();
const location = useLocation();

useEffect(() => {
fetchExploreData()
.then(({ result }) => {
if (isNullish(result.dataset?.id) && isNullish(result.dataset?.uid)) {
const exploreUrlParams = getParsedExploreURLParams(location);
const isSaveAction = !!getUrlParam(URL_PARAMS.saveAction);
if (!isExploreInitialized.current || isSaveAction) {
fetchExploreData(exploreUrlParams)
.then(({ result }) => {
if (isNullish(result.dataset?.id) && isNullish(result.dataset?.uid)) {
dispatch(hydrateExplore(fallbackExploreInitialData));
dispatch(addDangerToast(loadErrorMessage));
} else {
dispatch(hydrateExplore(result));
}
})
Comment on lines +54 to +60
Copy link
Member

@zhaoyongjie zhaoyongjie Jul 8, 2022

Choose a reason for hiding this comment

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

nits, if isDefined is used.

Suggested change
if (isNullish(result.dataset?.id) && isNullish(result.dataset?.uid)) {
dispatch(hydrateExplore(fallbackExploreInitialData));
dispatch(addDangerToast(loadErrorMessage));
} else {
dispatch(hydrateExplore(result));
}
})
if (isDefined(result.dataset?.id) && isDefined(result.dataset?.uid)) {
dispatch(hydrateExplore(result));
} else {
dispatch(hydrateExplore(fallbackExploreInitialData));
dispatch(addDangerToast(loadErrorMessage));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@codyml codyml Jul 8, 2022

Choose a reason for hiding this comment

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

@zhaoyongjie @kgabryje Not sure I totally understand this suggestion – is there an advantage to using isDefined over isNullish? Also, isn't the suggested code not equivalent to the original code because the original code throws if both id and uid are null/undefined, while the suggested code throws if either are null/undefined?

Copy link
Member

Choose a reason for hiding this comment

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

The same logic, only optimized for readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

But doesn't your suggested logic have a different effect? Or were you suggesting the logic should be different?

.catch(() => {
dispatch(hydrateExplore(fallbackExploreInitialData));
dispatch(addDangerToast(loadErrorMessage));
} else {
dispatch(hydrateExplore(result));
}
})
.catch(() => {
dispatch(hydrateExplore(fallbackExploreInitialData));
dispatch(addDangerToast(loadErrorMessage));
})
.finally(() => {
setIsLoaded(true);
});
}, [dispatch]);
})
.finally(() => {
setIsLoaded(true);
isExploreInitialized.current = true;
});
}
}, [dispatch, location]);

if (!isLoaded) {
return <Loading />;
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/explore/actions/hydrateExplore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export const hydrateExplore =
controlsTransferred: [],
standalone: getUrlParam(URL_PARAMS.standalone),
force: getUrlParam(URL_PARAMS.force),
sliceDashboards: initialFormData.dashboards,
};

// apply initial mapStateToProps for all controls, must execute AFTER
Expand Down
165 changes: 135 additions & 30 deletions superset-frontend/src/explore/actions/saveModalActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@
* specific language governing permissions and limitations
* under the License.
*/
import { SupersetClient } from '@superset-ui/core';
import { buildV1ChartDataPayload, getExploreUrl } from '../exploreUtils';
import { SupersetClient, t } from '@superset-ui/core';
import { addSuccessToast } from 'src/components/MessageToasts/actions';
import { buildV1ChartDataPayload } from '../exploreUtils';

export const FETCH_DASHBOARDS_SUCCEEDED = 'FETCH_DASHBOARDS_SUCCEEDED';
export function fetchDashboardsSucceeded(choices) {
Expand Down Expand Up @@ -60,36 +61,140 @@ export function removeSaveModalAlert() {
return { type: REMOVE_SAVE_MODAL_ALERT };
}

export function saveSlice(formData, requestParams) {
return dispatch => {
let url = getExploreUrl({
formData,
endpointType: 'base',
force: false,
curUrl: null,
requestParams,
export const getSlicePayload = (
sliceName,
formDataWithNativeFilters,
owners,
) => {
const formData = {
...formDataWithNativeFilters,
adhoc_filters: formDataWithNativeFilters.adhoc_filters?.filter(
f => !f.isExtra,
),
};

const [datasourceId, datasourceType] = formData.datasource.split('__');
const payload = {
params: JSON.stringify(formData),
slice_name: sliceName,
viz_type: formData.viz_type,
datasource_id: parseInt(datasourceId, 10),
datasource_type: datasourceType,
dashboards: formData.dashboards,
owners,
query_context: JSON.stringify(
buildV1ChartDataPayload({
formData,
force: false,
resultFormat: 'json',
resultType: 'full',
setDataMask: null,
ownState: null,
}),
),
Comment on lines +85 to +94
Copy link
Member

@zhaoyongjie zhaoyongjie Jul 8, 2022

Choose a reason for hiding this comment

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

nits:

Suggested change
query_context: JSON.stringify(
buildV1ChartDataPayload({
formData,
force: false,
resultFormat: 'json',
resultType: 'full',
setDataMask: null,
ownState: null,
}),
),
query_context: JSON.stringify(buildV1ChartDataPayload({ formData })),

One thing is strange, why does every JSON non-primitive value have to be serialized. i.e. params and query_context.

We serialize every non-primitive JSON value. This may have some potential deserialization errors. When the backend engineer uses the payload, they must remember which param has been serialized.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why non-primitives are serialized, I was mostly just keeping what was there before and I confess I don't really understand what's happening with all of these function calls that generate the payload. Are you sure it's safe to leave out all of those params?

};
return payload;
};

const addToasts = (isNewSlice, sliceName, addedToDashboard) => {
const toasts = [];
if (isNewSlice) {
toasts.push(addSuccessToast(t('Chart [%s] has been saved', sliceName)));
} else {
toasts.push(
addSuccessToast(t('Chart [%s] has been overwritten', sliceName)),
);
}

if (addedToDashboard) {
if (addedToDashboard.new) {
toasts.push(
addSuccessToast(
t(
'Dashboard [%s] just got created and chart [%s] was added to it',
addedToDashboard.title,
sliceName,
),
),
);
} else {
toasts.push(
addSuccessToast(
t(
'Chart [%s] was added to dashboard [%s]',
sliceName,
addedToDashboard.title,
),
),
);
}
}

return toasts;
};

// Update existing slice
export const updateSlice =
codyml marked this conversation as resolved.
Show resolved Hide resolved
({ slice_id: sliceId, owners }, sliceName, formData, addedToDashboard) =>
async dispatch => {
try {
const response = await SupersetClient.put({
endpoint: `/api/v1/chart/${sliceId}`,
jsonPayload: getSlicePayload(sliceName, formData, owners),
});

dispatch(saveSliceSuccess());
addToasts(false, sliceName, addedToDashboard).map(dispatch);
return response.json;
} catch (error) {
dispatch(saveSliceFailed());
throw error;
}
};

// Create new slice
export const createSlice =
(sliceName, formData, addedToDashboard) => async dispatch => {
try {
const response = await SupersetClient.post({
endpoint: `/api/v1/chart/`,
jsonPayload: getSlicePayload(sliceName, formData),
});

dispatch(saveSliceSuccess());
addToasts(true, sliceName, addedToDashboard).map(dispatch);
return response.json;
} catch (error) {
dispatch(saveSliceFailed());
throw error;
}
};

// Create new dashboard
export const createDashboard = dashboardName => async dispatch => {
try {
const response = await SupersetClient.post({
endpoint: `/api/v1/dashboard/`,
jsonPayload: { dashboard_title: dashboardName },
});

// TODO: This will be removed in the next PR that will change the logic to save a slice
url = url.replace('/explore', '/superset/explore');
return response.json;
} catch (error) {
dispatch(saveSliceFailed());
throw error;
}
};

// Save the query context so we can re-generate the data from Python
// for alerts and reports
const queryContext = buildV1ChartDataPayload({
formData,
force: false,
resultFormat: 'json',
resultType: 'full',
// Get existing dashboard from ID
export const getDashboard = dashboardId => async dispatch => {
try {
const response = await SupersetClient.get({
endpoint: `/api/v1/dashboard/${dashboardId}`,
});

return SupersetClient.post({
url,
postPayload: { form_data: formData, query_context: queryContext },
})
.then(response => {
dispatch(saveSliceSuccess(response.json));
return response.json;
})
.catch(() => dispatch(saveSliceFailed()));
};
}
return response.json;
} catch (error) {
dispatch(saveSliceFailed());
throw error;
}
};
Loading