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

fix(explore): fix chart save when dashboard deleted #21497

Merged
merged 2 commits into from
Sep 20, 2022
Merged

fix(explore): fix chart save when dashboard deleted #21497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4686dce
Fix chart save when dashboard deleted.
codyml Sep 20, 2022
cb1f6bd
Fix adding charts to extra dashboards on Save As.
codyml Sep 20, 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
1 change: 0 additions & 1 deletion superset-frontend/src/explore/actions/hydrateExplore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ export const hydrateExplore =
controlsTransferred: explore.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
29 changes: 24 additions & 5 deletions superset-frontend/src/explore/actions/saveModalActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import rison from 'rison';
import { SupersetClient, t } from '@superset-ui/core';
import { addSuccessToast } from 'src/components/MessageToasts/actions';
import { buildV1ChartDataPayload } from '../exploreUtils';
Expand Down Expand Up @@ -66,6 +67,7 @@ export function removeSaveModalAlert() {
export const getSlicePayload = (
sliceName,
formDataWithNativeFilters,
dashboards,
owners,
) => {
const adhocFilters = Object.entries(formDataWithNativeFilters).reduce(
Expand All @@ -78,6 +80,7 @@ export const getSlicePayload = (
const formData = {
...formDataWithNativeFilters,
...adhocFilters,
dashboards,
};

const [datasourceId, datasourceType] = formData.datasource.split('__');
Expand All @@ -87,7 +90,7 @@ export const getSlicePayload = (
viz_type: formData.viz_type,
datasource_id: parseInt(datasourceId, 10),
datasource_type: datasourceType,
dashboards: formData.dashboards,
dashboards,
owners,
query_context: JSON.stringify(
buildV1ChartDataPayload({
Expand Down Expand Up @@ -142,7 +145,7 @@ const addToasts = (isNewSlice, sliceName, addedToDashboard) => {

// Update existing slice
export const updateSlice =
({ slice_id: sliceId, owners }, sliceName, addedToDashboard) =>
({ slice_id: sliceId, owners }, sliceName, dashboards, addedToDashboard) =>
async (dispatch, getState) => {
const {
explore: {
Expand All @@ -152,7 +155,7 @@ export const updateSlice =
try {
const response = await SupersetClient.put({
endpoint: `/api/v1/chart/${sliceId}`,
jsonPayload: getSlicePayload(sliceName, formData, owners),
jsonPayload: getSlicePayload(sliceName, formData, dashboards, owners),
});

dispatch(saveSliceSuccess());
Expand All @@ -166,7 +169,7 @@ export const updateSlice =

// Create new slice
export const createSlice =
(sliceName, addedToDashboard) => async (dispatch, getState) => {
(sliceName, dashboards, addedToDashboard) => async (dispatch, getState) => {
const {
explore: {
form_data: { url_params: _, ...formData },
Expand All @@ -175,7 +178,7 @@ export const createSlice =
try {
const response = await SupersetClient.post({
endpoint: `/api/v1/chart/`,
jsonPayload: getSlicePayload(sliceName, formData),
jsonPayload: getSlicePayload(sliceName, formData, dashboards),
});

dispatch(saveSliceSuccess());
Expand Down Expand Up @@ -215,3 +218,19 @@ export const getDashboard = dashboardId => async dispatch => {
throw error;
}
};

// Get dashboards the slice is added to
export const getSliceDashboards = slice => async dispatch => {
try {
const response = await SupersetClient.get({
endpoint: `/api/v1/chart/${slice.slice_id}?q=${rison.encode({
columns: ['dashboards.id'],
})}`,
});

return response.json.result.dashboards.map(({ id }) => id);
} catch (error) {
dispatch(saveSliceFailed());
throw error;
}
};
61 changes: 52 additions & 9 deletions superset-frontend/src/explore/actions/saveModalActions.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
FETCH_DASHBOARDS_FAILED,
FETCH_DASHBOARDS_SUCCEEDED,
getDashboard,
getSliceDashboards,
SAVE_SLICE_FAILED,
SAVE_SLICE_SUCCESS,
updateSlice,
Expand Down Expand Up @@ -97,10 +98,11 @@ test('updateSlice handles success', async () => {
fetchMock.put(updateSliceEndpoint, sliceResponsePayload);
const dispatch = sinon.spy();
const getState = sinon.spy(() => mockExploreState);
const slice = await updateSlice({ slice_id: sliceId }, sliceName)(
dispatch,
getState,
);
const slice = await updateSlice(
{ slice_id: sliceId },
sliceName,
[],
)(dispatch, getState);

expect(fetchMock.calls(updateSliceEndpoint)).toHaveLength(1);
expect(dispatch.callCount).toBe(2);
Expand All @@ -121,7 +123,7 @@ test('updateSlice handles failure', async () => {
const getState = sinon.spy(() => mockExploreState);
let caughtError;
try {
await updateSlice({ slice_id: sliceId }, sliceName)(dispatch, getState);
await updateSlice({ slice_id: sliceId }, sliceName, [])(dispatch, getState);
} catch (error) {
caughtError = error;
}
Expand All @@ -142,7 +144,7 @@ test('createSlice handles success', async () => {
fetchMock.post(createSliceEndpoint, sliceResponsePayload);
const dispatch = sinon.spy();
const getState = sinon.spy(() => mockExploreState);
const slice = await createSlice(sliceName)(dispatch, getState);
const slice = await createSlice(sliceName, [])(dispatch, getState);
expect(fetchMock.calls(createSliceEndpoint)).toHaveLength(1);
expect(dispatch.callCount).toBe(2);
expect(dispatch.getCall(0).args[0].type).toBe(SAVE_SLICE_SUCCESS);
Expand All @@ -162,7 +164,7 @@ test('createSlice handles failure', async () => {
const getState = sinon.spy(() => mockExploreState);
let caughtError;
try {
await createSlice(sliceName)(dispatch, getState);
await createSlice(sliceName, [])(dispatch, getState);
} catch (error) {
caughtError = error;
}
Expand Down Expand Up @@ -248,7 +250,7 @@ test('updateSlice with add to new dashboard handles success', async () => {
fetchMock.put(updateSliceEndpoint, sliceResponsePayload);
const dispatch = sinon.spy();
const getState = sinon.spy(() => mockExploreState);
const slice = await updateSlice({ slice_id: sliceId }, sliceName, {
const slice = await updateSlice({ slice_id: sliceId }, sliceName, [], {
new: true,
title: dashboardName,
})(dispatch, getState);
Expand All @@ -275,7 +277,7 @@ test('updateSlice with add to existing dashboard handles success', async () => {
fetchMock.put(updateSliceEndpoint, sliceResponsePayload);
const dispatch = sinon.spy();
const getState = sinon.spy(() => mockExploreState);
const slice = await updateSlice({ slice_id: sliceId }, sliceName, {
const slice = await updateSlice({ slice_id: sliceId }, sliceName, [], {
new: false,
title: dashboardName,
})(dispatch, getState);
Expand All @@ -296,3 +298,44 @@ test('updateSlice with add to existing dashboard handles success', async () => {

expect(slice).toEqual(sliceResponsePayload);
});

const slice = { slice_id: 10 };
const dashboardSlicesResponsePayload = {
result: {
dashboards: [{ id: 21 }, { id: 22 }, { id: 23 }],
},
};

const getDashboardSlicesReturnValue = [21, 22, 23];

/**
* Tests getSliceDashboards action
*/

const getSliceDashboardsEndpoint = `glob:*/api/v1/chart/${sliceId}?q=(columns:!(dashboards.id))`;
test('getSliceDashboards with slice handles success', async () => {
fetchMock.reset();
fetchMock.get(getSliceDashboardsEndpoint, dashboardSlicesResponsePayload);
const dispatch = sinon.spy();
const sliceDashboards = await getSliceDashboards(slice)(dispatch);
expect(fetchMock.calls(getSliceDashboardsEndpoint)).toHaveLength(1);
expect(dispatch.callCount).toBe(0);
expect(sliceDashboards).toEqual(getDashboardSlicesReturnValue);
});

test('getSliceDashboards with slice handles failure', async () => {
fetchMock.reset();
fetchMock.get(getSliceDashboardsEndpoint, { throws: sampleError });
const dispatch = sinon.spy();
let caughtError;
try {
await getSliceDashboards(slice)(dispatch);
} catch (error) {
caughtError = error;
}

expect(caughtError).toEqual(sampleError);
expect(fetchMock.calls(getSliceDashboardsEndpoint)).toHaveLength(4);
expect(dispatch.callCount).toBe(1);
expect(dispatch.getCall(0).args[0].type).toBe(SAVE_SLICE_FAILED);
});
1 change: 0 additions & 1 deletion superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,6 @@ function ExploreViewContainer(props) {
form_data={props.form_data}
sliceName={props.sliceName}
dashboardId={props.dashboardId}
sliceDashboards={props.exploreState.sliceDashboards ?? []}
/>
)}
<Resizable
Expand Down
22 changes: 17 additions & 5 deletions superset-frontend/src/explore/components/SaveModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ interface SaveModalProps extends RouteComponentProps {
slice?: Record<string, any>;
datasource?: Record<string, any>;
dashboardId: '' | number | null;
sliceDashboards: number[];
}

type ActionType = 'overwrite' | 'saveas';
Expand Down Expand Up @@ -183,6 +182,16 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
}
}

// Get chart dashboards
let sliceDashboards: number[] = [];
if (this.props.slice && this.state.action === 'overwrite') {
promise = promise
.then(() => this.props.actions.getSliceDashboards(this.props.slice))
.then(dashboards => {
sliceDashboards = dashboards;
});
}

let dashboard: DashboardGetResponse | null = null;
if (this.state.newDashboardName || this.state.saveToDashboardId) {
let saveToDashboardId = this.state.saveToDashboardId || null;
Expand All @@ -200,12 +209,13 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
.then(() => this.props.actions.getDashboard(saveToDashboardId))
.then((response: { result: DashboardGetResponse }) => {
dashboard = response.result;
const dashboards = new Set<number>(this.props.sliceDashboards);
Copy link
Member

Choose a reason for hiding this comment

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

nits:

sliceDashboards = originalDashboards.includes(dashboard.id) ? originalDashboards : [...originalDashboards, dashboard.id],

Copy link
Member Author

Choose a reason for hiding this comment

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

Much cleaner, thanks.

dashboards.add(dashboard.id);
sliceDashboards = sliceDashboards.includes(dashboard.id)
? sliceDashboards
: [...sliceDashboards, dashboard.id];
const { url_params, ...formData } = this.props.form_data || {};
this.props.actions.setFormData({
...formData,
dashboards: Array.from(dashboards),
dashboards: sliceDashboards,
});
});
}
Expand All @@ -216,6 +226,7 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
this.props.actions.updateSlice(
this.props.slice,
this.state.newSliceName,
sliceDashboards,
dashboard
? {
title: dashboard.dashboard_title,
Expand All @@ -228,6 +239,7 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
promise = promise.then(() =>
this.props.actions.createSlice(
this.state.newSliceName,
sliceDashboards,
dashboard
? {
title: dashboard.dashboard_title,
Expand Down Expand Up @@ -276,7 +288,7 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
type="warning"
message={
<>
{this.state.alert ? this.state.alert : this.props.alert}
{this.state.alert || this.props.alert}
<i
role="button"
aria-label="Remove alert"
Expand Down