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

* Update SaveModal to use v1 API instead of POST /explore.

* Refactor SaveModal tests and remove obsolete ones.

* Fix redirect to /explore on save.

* Add toasts (but they don't work).

* Move logic inside try block, clarify unary-plus use.

* Add tests.

* Fix owners bug in updateSlice, enable navigation to dashboard w/o reload.

* Fix toasts.

* Fix translated strings.

* Fix unintended removal from dashboard bug.

* Fix native filters bug.

* Don't refresh Explore page after saving

* Use JSON payload shorthand.

* Prevent current dashboards being overwritten on viz type change.

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

@@ -464,6 +464,14 @@ function ExploreViewContainer(props) {
     return false;
   }, [lastQueriedControls, props.controls]);
 
+  const saveAction = getUrlParam(URL_PARAMS.saveAction);
+  useChangeEffect(saveAction, () => {
+    if (['saveas', 'overwrite'].includes(saveAction)) {
+      onQuery();
+      addHistory({ isReplace: true });
+    }
+  });
+
   useEffect(() => {
     if (props.ownState !== undefined) {
       onQuery();
@@ -586,6 +594,7 @@ function ExploreViewContainer(props) {
             form_data={props.form_data}
             sliceName={props.sliceName}
             dashboardId={props.dashboardId}
+            sliceDashboards={props.exploreState.sliceDashboards ?? []}
           />
         )}
         <Resizable

superset-frontend/src/explore/components/SaveModal.test.jsx

@@ -29,322 +29,172 @@ import Button from 'src/components/Button';
 import sinon from 'sinon';
 import fetchMock from 'fetch-mock';
 
-import * as exploreUtils from 'src/explore/exploreUtils';
 import * as saveModalActions from 'src/explore/actions/saveModalActions';
 import SaveModal, { StyledModal } from 'src/explore/components/SaveModal';
+import { BrowserRouter } from 'react-router-dom';
 
-describe('SaveModal', () => {
-  const middlewares = [thunk];
-  const mockStore = configureStore(middlewares);
-  const initialState = {
-    chart: {},
-    saveModal: {
-      dashboards: [],
+const middlewares = [thunk];
+const mockStore = configureStore(middlewares);
+const initialState = {
+  chart: {},
+  saveModal: {
+    dashboards: [],
+  },
+  explore: {
+    datasource: {},
+    slice: {
+      slice_id: 1,
+      slice_name: 'title',
+      owners: [1],
     },
-    explore: {
-      datasource: {},
-      slice: {
-        slice_id: 1,
-        slice_name: 'title',
-        owners: [1],
-      },
-      alert: null,
-    },
-    user: {
-      userId: 1,
-    },
-  };
-  const store = mockStore(initialState);
+    alert: null,
+  },
+  user: {
+    userId: 1,
+  },
+};
 
-  const defaultProps = {
-    onHide: () => ({}),
-    actions: bindActionCreators(saveModalActions, arg => {
-      if (typeof arg === 'function') {
-        return arg(jest.fn);
-      }
-      return arg;
-    }),
-    form_data: { datasource: '107__table', url_params: { foo: 'bar' } },
-  };
-  const mockEvent = {
-    target: {
-      value: 'mock event target',
-    },
-    value: 10,
-  };
+const store = mockStore(initialState);
 
-  const mockDashboardData = {
-    pks: ['id'],
-    result: [{ id: 'id', dashboard_title: 'dashboard title' }],
-  };
+const defaultProps = {
+  onHide: () => ({}),
+  actions: bindActionCreators(saveModalActions, arg => {
+    if (typeof arg === 'function') {
+      return arg(jest.fn);
+    }
+    return arg;
+  }),
+  form_data: { datasource: '107__table', url_params: { foo: 'bar' } },
+};
 
-  const saveEndpoint = `glob:*/dashboardasync/api/read?_flt_0_owners=${1}`;
+const mockEvent = {
+  target: {
+    value: 'mock event target',
+  },
+  value: 10,
+};
 
-  beforeAll(() => fetchMock.get(saveEndpoint, mockDashboardData));
+const mockDashboardData = {
+  pks: ['id'],
+  result: [{ id: 'id', dashboard_title: 'dashboard title' }],
+};
 
-  afterAll(() => fetchMock.restore());
+const fetchDashboardsEndpoint = `glob:*/dashboardasync/api/read?_flt_0_owners=${1}`;
 
-  const getWrapper = () =>
-    shallow(<SaveModal {...defaultProps} store={store} />)
-      .dive()
-      .dive();
+beforeAll(() => fetchMock.get(fetchDashboardsEndpoint, mockDashboardData));
 
-  it('renders a Modal with the right set of components', () => {
-    const wrapper = getWrapper();
-    expect(wrapper.find(StyledModal)).toExist();
-    expect(wrapper.find(Radio)).toHaveLength(2);
+afterAll(() => fetchMock.restore());
 
-    const footerWrapper = shallow(wrapper.find(StyledModal).props().footer);
+const getWrapper = () =>
+  shallow(
+    <BrowserRouter>
+      <SaveModal {...defaultProps} store={store} />
+    </BrowserRouter>,
+  )
+    .dive()
+    .dive()
+    .dive()
+    .dive()
+    .dive()
+    .dive()
+    .dive();
 
-    expect(footerWrapper.find(Button)).toHaveLength(3);
-  });
+test('renders a Modal with the right set of components', () => {
+  const wrapper = getWrapper();
+  expect(wrapper.find(StyledModal)).toExist();
+  expect(wrapper.find(Radio)).toHaveLength(2);
 
-  it('renders the right footer buttons when an existing dashboard', () => {
-    const wrapper = getWrapper();
-    const footerWrapper = shallow(wrapper.find(StyledModal).props().footer);
-    const saveAndGoDash = footerWrapper
-      .find('#btn_modal_save_goto_dash')
-      .getElement();
-    const save = footerWrapper.find('#btn_modal_save').getElement();
-    expect(save.props.children).toBe('Save');
-    expect(saveAndGoDash.props.children).toBe('Save & go to dashboard');
-  });
+  const footerWrapper = shallow(wrapper.find(StyledModal).props().footer);
 
-  it('renders the right footer buttons when a new dashboard', () => {
-    const wrapper = getWrapper();
-    wrapper.setState({
-      saveToDashboardId: null,
-      newDashboardName: 'Test new dashboard',
-    });
-    const footerWrapper = shallow(wrapper.find(StyledModal).props().footer);
-    const saveAndGoDash = footerWrapper
-      .find('#btn_modal_save_goto_dash')
-      .getElement();
-    const save = footerWrapper.find('#btn_modal_save').getElement();
-    expect(save.props.children).toBe('Save to new dashboard');
-    expect(saveAndGoDash.props.children).toBe('Save & go to new dashboard');
-  });
-
-  it('overwrite radio button is disabled for new slice', () => {
-    const wrapper = getWrapper();
-    wrapper.setProps({ slice: null });
-    expect(wrapper.find('#overwrite-radio').prop('disabled')).toBe(true);
-  });
-
-  it('disable overwrite option for non-owner', () => {
-    const wrapperForNonOwner = getWrapper();
-    wrapperForNonOwner.setProps({ userId: 2 });
-    const overwriteRadio = wrapperForNonOwner.find('#overwrite-radio');
-    expect(overwriteRadio).toHaveLength(1);
-    expect(overwriteRadio.prop('disabled')).toBe(true);
-  });
-
-  it('saves a new slice', () => {
-    const wrapperForNewSlice = getWrapper();
-    wrapperForNewSlice.setProps({ can_overwrite: false });
-    wrapperForNewSlice.instance().changeAction('saveas');
-    const saveasRadio = wrapperForNewSlice.find('#saveas-radio');
-    saveasRadio.simulate('click');
-    expect(wrapperForNewSlice.state().action).toBe('saveas');
-  });
-
-  it('overwrite a slice', () => {
-    const wrapperForOverwrite = getWrapper();
-    const overwriteRadio = wrapperForOverwrite.find('#overwrite-radio');
-    overwriteRadio.simulate('click');
-    expect(wrapperForOverwrite.state().action).toBe('overwrite');
-  });
-
-  it('componentDidMount', () => {
-    sinon.spy(defaultProps.actions, 'fetchDashboards');
-    mount(
-      <Provider store={store}>
-        <SaveModal {...defaultProps} />
-      </Provider>,
-    );
-    expect(defaultProps.actions.fetchDashboards.calledOnce).toBe(true);
-
-    defaultProps.actions.fetchDashboards.restore();
-  });
-
-  it('onChange', () => {
-    const wrapper = getWrapper();
-    const dashboardId = mockEvent.value;
-
-    wrapper.instance().onSliceNameChange(mockEvent);
-    expect(wrapper.state().newSliceName).toBe(mockEvent.target.value);
-
-    wrapper.instance().onDashboardSelectChange(dashboardId);
-    expect(wrapper.state().saveToDashboardId).toBe(dashboardId);
-  });
-
-  describe('saveOrOverwrite', () => {
-    beforeEach(() => {
-      sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => 'mockURL');
-
-      sinon.stub(defaultProps.actions, 'saveSlice').callsFake(() =>
-        Promise.resolve({
-          dashboard_url: 'http://localhost/mock_dashboard/',
-          slice: { slice_url: '/mock_slice/' },
-        }),
-      );
-    });
-
-    afterEach(() => {
-      exploreUtils.getExploreUrl.restore();
-      defaultProps.actions.saveSlice.restore();
-    });
-
-    it('should save slice without url_params in form_data', () => {
-      const wrapper = getWrapper();
-      wrapper.instance().saveOrOverwrite(true);
-      const { args } = defaultProps.actions.saveSlice.getCall(0);
-      expect(args[0]).toEqual({ datasource: '107__table' });
-    });
-
-    it('existing dashboard', () => {
-      const wrapper = getWrapper();
-      const saveToDashboardId = 100;
-
-      wrapper.setState({ saveToDashboardId });
-      wrapper.instance().saveOrOverwrite(true);
-      const { args } = defaultProps.actions.saveSlice.getCall(0);
-      expect(args[1].save_to_dashboard_id).toBe(saveToDashboardId);
-    });
-
-    it('new dashboard', () => {
-      const wrapper = getWrapper();
-      const newDashboardName = 'new dashboard name';
-
-      wrapper.setState({ newDashboardName });
-      wrapper.instance().saveOrOverwrite(true);
-      const { args } = defaultProps.actions.saveSlice.getCall(0);
-      expect(args[1].new_dashboard_name).toBe(newDashboardName);
-    });
-
-    describe('should always reload or redirect', () => {
-      const originalLocation = window.location;
-      delete window.location;
-      window.location = { assign: jest.fn() };
-      const stub = sinon.stub(window.location, 'assign');
-
-      afterAll(() => {
-        delete window.location;
-        window.location = originalLocation;
-      });
-
-      let wrapper;
-
-      beforeEach(() => {
-        stub.resetHistory();
-        wrapper = getWrapper();
-      });
-
-      it('Save & go to dashboard', () =>
-        new Promise(done => {
-          wrapper.instance().saveOrOverwrite(true);
-          defaultProps.actions.saveSlice().then(() => {
-            expect(window.location.assign.callCount).toEqual(1);
-            expect(window.location.assign.getCall(0).args[0]).toEqual(
-              'http://localhost/mock_dashboard/?foo=bar',
-            );
-            done();
-          });
-        }));
-
-      it('saveas new slice', () =>
-        new Promise(done => {
-          wrapper.setState({
-            action: 'saveas',
-            newSliceName: 'new slice name',
-          });
-          wrapper.instance().saveOrOverwrite(false);
-          defaultProps.actions.saveSlice().then(() => {
-            expect(window.location.assign.callCount).toEqual(1);
-            expect(window.location.assign.getCall(0).args[0]).toEqual(
-              '/mock_slice/?foo=bar',
-            );
-            done();
-          });
-        }));
-
-      it('overwrite original slice', () =>
-        new Promise(done => {
-          wrapper.setState({ action: 'overwrite' });
-          wrapper.instance().saveOrOverwrite(false);
-          defaultProps.actions.saveSlice().then(() => {
-            expect(window.location.assign.callCount).toEqual(1);
-            expect(window.location.assign.getCall(0).args[0]).toEqual(
-              '/mock_slice/?foo=bar',
-            );
-            done();
-          });
-        }));
-    });
-  });
-
-  describe('fetchDashboards', () => {
-    let dispatch;
-    let actionThunk;
-    const userID = 1;
-
-    beforeEach(() => {
-      fetchMock.resetHistory();
-      dispatch = sinon.spy();
-    });
-
-    const makeRequest = () => {
-      actionThunk = saveModalActions.fetchDashboards(userID);
-      return actionThunk(dispatch);
-    };
-
-    it('makes the fetch request', () =>
-      makeRequest().then(() => {
-        expect(fetchMock.calls(saveEndpoint)).toHaveLength(1);
-
-        return Promise.resolve();
-      }));
-
-    it('calls correct actions on success', () =>
-      makeRequest().then(() => {
-        expect(dispatch.callCount).toBe(1);
-        expect(dispatch.getCall(0).args[0].type).toBe(
-          saveModalActions.FETCH_DASHBOARDS_SUCCEEDED,
-        );
-
-        return Promise.resolve();
-      }));
-
-    it('calls correct actions on error', () => {
-      fetchMock.get(
-        saveEndpoint,
-        { throws: 'error' },
-        { overwriteRoutes: true },
-      );
-
-      return makeRequest().then(() => {
-        expect(dispatch.callCount).toBe(1);
-        expect(dispatch.getCall(0).args[0].type).toBe(
-          saveModalActions.FETCH_DASHBOARDS_FAILED,
-        );
-
-        fetchMock.get(saveEndpoint, mockDashboardData, {
-          overwriteRoutes: true,
-        });
-
-        return Promise.resolve();
-      });
-    });
-  });
-
-  it('removeAlert', () => {
-    sinon.spy(defaultProps.actions, 'removeSaveModalAlert');
-    const wrapper = getWrapper();
-    wrapper.setProps({ alert: 'old alert' });
-
-    wrapper.instance().removeAlert();
-    expect(defaultProps.actions.removeSaveModalAlert.callCount).toBe(1);
-    expect(wrapper.state().alert).toBeNull();
-    defaultProps.actions.removeSaveModalAlert.restore();
-  });
+  expect(footerWrapper.find(Button)).toHaveLength(3);
+});
+
+test('renders the right footer buttons when existing dashboard selected', () => {
+  const wrapper = getWrapper();
+  const footerWrapper = shallow(wrapper.find(StyledModal).props().footer);
+  const saveAndGoDash = footerWrapper
+    .find('#btn_modal_save_goto_dash')
+    .getElement();
+  const save = footerWrapper.find('#btn_modal_save').getElement();
+  expect(save.props.children).toBe('Save');
+  expect(saveAndGoDash.props.children).toBe('Save & go to dashboard');
+});
+
+test('renders the right footer buttons when new dashboard selected', () => {
+  const wrapper = getWrapper();
+  wrapper.setState({
+    saveToDashboardId: null,
+    newDashboardName: 'Test new dashboard',
+  });
+  const footerWrapper = shallow(wrapper.find(StyledModal).props().footer);
+  const saveAndGoDash = footerWrapper
+    .find('#btn_modal_save_goto_dash')
+    .getElement();
+  const save = footerWrapper.find('#btn_modal_save').getElement();
+  expect(save.props.children).toBe('Save to new dashboard');
+  expect(saveAndGoDash.props.children).toBe('Save & go to new dashboard');
+});
+
+test('disables overwrite option for new slice', () => {
+  const wrapper = getWrapper();
+  wrapper.setProps({ slice: null });
+  expect(wrapper.find('#overwrite-radio').prop('disabled')).toBe(true);
+});
+
+test('disables overwrite option for non-owner', () => {
+  const wrapperForNonOwner = getWrapper();
+  wrapperForNonOwner.setProps({ userId: 2 });
+  const overwriteRadio = wrapperForNonOwner.find('#overwrite-radio');
+  expect(overwriteRadio).toHaveLength(1);
+  expect(overwriteRadio.prop('disabled')).toBe(true);
+});
+
+test('sets action when saving as new slice', () => {
+  const wrapperForNewSlice = getWrapper();
+  wrapperForNewSlice.setProps({ can_overwrite: false });
+  wrapperForNewSlice.instance().changeAction('saveas');
+  const saveasRadio = wrapperForNewSlice.find('#saveas-radio');
+  saveasRadio.simulate('click');
+  expect(wrapperForNewSlice.state().action).toBe('saveas');
+});
+
+test('sets action when overwriting slice', () => {
+  const wrapperForOverwrite = getWrapper();
+  const overwriteRadio = wrapperForOverwrite.find('#overwrite-radio');
+  overwriteRadio.simulate('click');
+  expect(wrapperForOverwrite.state().action).toBe('overwrite');
+});
+
+test('fetches dashboards on component mount', () => {
+  sinon.spy(defaultProps.actions, 'fetchDashboards');
+  mount(
+    <Provider store={store}>
+      <SaveModal {...defaultProps} />
+    </Provider>,
+  );
+  expect(defaultProps.actions.fetchDashboards.calledOnce).toBe(true);
+
+  defaultProps.actions.fetchDashboards.restore();
+});
+
+test('updates slice name and selected dashboard', () => {
+  const wrapper = getWrapper();
+  const dashboardId = mockEvent.value;
+
+  wrapper.instance().onSliceNameChange(mockEvent);
+  expect(wrapper.state().newSliceName).toBe(mockEvent.target.value);
+
+  wrapper.instance().onDashboardSelectChange(dashboardId);
+  expect(wrapper.state().saveToDashboardId).toBe(dashboardId);
+});
+
+test('removes alert', () => {
+  sinon.spy(defaultProps.actions, 'removeSaveModalAlert');
+  const wrapper = getWrapper();
+  wrapper.setProps({ alert: 'old alert' });
+
+  wrapper.instance().removeAlert();
+  expect(defaultProps.actions.removeSaveModalAlert.callCount).toBe(1);
+  expect(wrapper.state().alert).toBeNull();
+  defaultProps.actions.removeSaveModalAlert.restore();
 });

superset-frontend/src/explore/components/SaveModal.tsx

@@ -21,7 +21,7 @@ import React from 'react';
 import { Input } from 'src/components/Input';
 import { Form, FormItem } from 'src/components/Form';
 import Alert from 'src/components/Alert';
-import { JsonObject, t, styled } from '@superset-ui/core';
+import { t, styled } from '@superset-ui/core';
 import ReactMarkdown from 'react-markdown';
 import Modal from 'src/components/Modal';
 import { Radio } from 'src/components/Radio';
@@ -29,12 +29,13 @@ import Button from 'src/components/Button';
 import { Select } from 'src/components';
 import { SelectValue } from 'antd/lib/select';
 import { connect } from 'react-redux';
+import { withRouter, RouteComponentProps } from 'react-router-dom';
 
 // Session storage key for recent dashboard
 const SK_DASHBOARD_ID = 'save_chart_recent_dashboard';
 const SELECT_PLACEHOLDER = t('**Select** a dashboard OR **create** a new one');
 
-type SaveModalProps = {
+interface SaveModalProps extends RouteComponentProps {
   onHide: () => void;
   actions: Record<string, any>;
   form_data?: Record<string, any>;
@@ -45,7 +46,8 @@ type SaveModalProps = {
   slice?: Record<string, any>;
   datasource?: Record<string, any>;
   dashboardId: '' | number | null;
-};
+  sliceDashboards: number[];
+}
 
 type ActionType = 'overwrite' | 'saveas';
 
@@ -131,39 +133,94 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
   saveOrOverwrite(gotodash: boolean) {
     this.setState({ alert: null });
     this.props.actions.removeSaveModalAlert();
-    const sliceParams: Record<string, any> = {};
-
-    if (this.props.slice && this.props.slice.slice_id) {
-      sliceParams.slice_id = this.props.slice.slice_id;
-    }
-    if (sliceParams.action === 'saveas') {
-      if (this.state.newSliceName === '') {
-        this.setState({ alert: t('Please enter a chart name') });
-        return;
-      }
-    }
-    sliceParams.action = this.state.action;
-    sliceParams.slice_name = this.state.newSliceName;
-    sliceParams.save_to_dashboard_id = this.state.saveToDashboardId;
-    sliceParams.new_dashboard_name = this.state.newDashboardName;
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars
     const { url_params, ...formData } = this.props.form_data || {};
 
-    this.props.actions
-      .saveSlice(formData, sliceParams)
-      .then((data: JsonObject) => {
-        if (data.dashboard_id === null) {
-          sessionStorage.removeItem(SK_DASHBOARD_ID);
-        } else {
-          sessionStorage.setItem(SK_DASHBOARD_ID, data.dashboard_id);
-        }
-        // Go to new slice url or dashboard url
-        let url = gotodash ? data.dashboard_url : data.slice.slice_url;
-        if (url_params) {
-          const prefix = url.includes('?') ? '&' : '?';
-          url = `${url}${prefix}${new URLSearchParams(url_params).toString()}`;
-        }
-        window.location.assign(url);
-      });
+    let promise = Promise.resolve();
+
+    //  Create or retrieve dashboard
+    type DashboardGetResponse = {
+      id: number;
+      url: string;
+      dashboard_title: string;
+    };
+
+    let dashboard: DashboardGetResponse | null = null;
+    if (this.state.newDashboardName || this.state.saveToDashboardId) {
+      let saveToDashboardId = this.state.saveToDashboardId || null;
+      if (!this.state.saveToDashboardId) {
+        promise = promise
+          .then(() =>
+            this.props.actions.createDashboard(this.state.newDashboardName),
+          )
+          .then((response: { id: number }) => {
+            saveToDashboardId = response.id;
+          });
+      }
+
+      promise = promise
+        .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);
+          formData.dashboards = Array.from(dashboards);
+        });
+    }
+
+    //  Update or create slice
+    if (this.state.action === 'overwrite') {
+      promise = promise.then(() =>
+        this.props.actions.updateSlice(
+          this.props.slice,
+          this.state.newSliceName,
+          formData,
+          dashboard
+            ? {
+                title: dashboard.dashboard_title,
+                new: !this.state.saveToDashboardId,
+              }
+            : null,
+        ),
+      );
+    } else {
+      promise = promise.then(() =>
+        this.props.actions.createSlice(
+          this.state.newSliceName,
+          formData,
+          dashboard
+            ? {
+                title: dashboard.dashboard_title,
+                new: !this.state.saveToDashboardId,
+              }
+            : null,
+        ),
+      );
+    }
+
+    promise.then(((value: { id: number }) => {
+      //  Update recent dashboard
+      if (dashboard) {
+        sessionStorage.setItem(SK_DASHBOARD_ID, `${dashboard.id}`);
+      } else {
+        sessionStorage.removeItem(SK_DASHBOARD_ID);
+      }
+
+      // Go to new dashboard url
+      if (gotodash && dashboard) {
+        this.props.history.push(dashboard.url);
+        return;
+      }
+
+      const searchParams = new URLSearchParams(this.props.location.search);
+      searchParams.set('save_action', this.state.action);
+      searchParams.delete('form_data_key');
+      if (this.state.action === 'saveas') {
+        searchParams.set('slice_id', value.id.toString());
+      }
+      this.props.history.replace(`/explore/?${searchParams.toString()}`);
+    }) as (value: any) => void);
+
     this.props.onHide();
   }
 
@@ -296,11 +353,19 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
   }
 }
 
+interface StateProps {
+  datasource: any;
+  slice: any;
+  userId: any;
+  dashboards: any;
+  alert: any;
+}
+
 function mapStateToProps({
   explore,
   saveModal,
   user,
-}: Record<string, any>): Partial<SaveModalProps> {
+}: Record<string, any>): StateProps {
   return {
     datasource: explore.datasource,
     slice: explore.slice,
@@ -310,4 +375,4 @@ function mapStateToProps({
   };
 }
 
-export default connect(mapStateToProps, () => ({}))(SaveModal);
+export default withRouter(connect(mapStateToProps, () => ({}))(SaveModal));