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

apache · Sep 20, 2022 · 6644a84 · 6644a84
1 parent 2224ebe
commit 6644a84
Show file tree

Hide file tree

Showing 5 changed files with 93 additions and 21 deletions.
diff --git a/superset-frontend/src/explore/actions/hydrateExplore.ts b/superset-frontend/src/explore/actions/hydrateExplore.ts
@@ -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

diff --git a/superset-frontend/src/explore/actions/saveModalActions.js b/superset-frontend/src/explore/actions/saveModalActions.js
@@ -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';
@@ -66,6 +67,7 @@ export function removeSaveModalAlert() {
 export const getSlicePayload = (
   sliceName,
   formDataWithNativeFilters,
+  dashboards,
   owners,
 ) => {
   const adhocFilters = Object.entries(formDataWithNativeFilters).reduce(
@@ -78,6 +80,7 @@ export const getSlicePayload = (
   const formData = {
     ...formDataWithNativeFilters,
     ...adhocFilters,
+    dashboards,
   };
 
   const [datasourceId, datasourceType] = formData.datasource.split('__');
@@ -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({
@@ -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: {
@@ -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());
@@ -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 },
@@ -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());
@@ -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;
+  }
+};
diff --git a/superset-frontend/src/explore/actions/saveModalActions.test.js b/superset-frontend/src/explore/actions/saveModalActions.test.js
@@ -27,6 +27,7 @@ import {
   FETCH_DASHBOARDS_FAILED,
   FETCH_DASHBOARDS_SUCCEEDED,
   getDashboard,
+  getSliceDashboards,
   SAVE_SLICE_FAILED,
   SAVE_SLICE_SUCCESS,
   updateSlice,
@@ -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);
@@ -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;
   }
@@ -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);
@@ -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;
   }
@@ -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);
@@ -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);
@@ -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);
+});
diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
@@ -599,7 +599,6 @@ function ExploreViewContainer(props) {
             form_data={props.form_data}
             sliceName={props.sliceName}
             dashboardId={props.dashboardId}
-            sliceDashboards={props.exploreState.sliceDashboards ?? []}
           />
         )}
         <Resizable

diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx
@@ -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';
@@ -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;
@@ -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);
-          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,
           });
         });
     }
@@ -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,
@@ -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,
@@ -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"