fix: Save dataset + chart when Chart source is Query (#20880)

* feat: save dataset savemodal

* fix lint

* add comments

* enable chart power query

* clean up

* added test

* fix overwrite

* add proper error messaging for save datasetModal

* lint

* fix ts lint

* fix

* Disables Save button while network call is in progress, removing second Chart saved toast message

* change naming

* err

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

Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>

* Update SaveModal.tsx

Co-authored-by: Phillip Kelley-Dotson <pkelleydotson@yahoo.com>
Co-authored-by: Eric Briscoe <eric.j.briscoe@gmail.com>
Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com>

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

@@ -56,6 +56,7 @@ import { getFormDataFromControls } from 'src/explore/controlUtils';
 import * as exploreActions from 'src/explore/actions/exploreActions';
 import * as saveModalActions from 'src/explore/actions/saveModalActions';
 import { useTabId } from 'src/hooks/useTabId';
+import withToasts from 'src/components/MessageToasts/withToasts';
 import ExploreChartPanel from '../ExploreChartPanel';
 import ConnectedControlPanelsContainer from '../ControlPanelsContainer';
 import SaveModal from '../SaveModal';
@@ -589,6 +590,7 @@ function ExploreViewContainer(props) {
         />
         {showingModal && (
           <SaveModal
+            addDangerToast={props.addDangerToast}
             onHide={toggleModal}
             actions={props.actions}
             form_data={props.form_data}
@@ -767,4 +769,4 @@ function mapDispatchToProps(dispatch) {
 export default connect(
   mapStateToProps,
   mapDispatchToProps,
-)(ExploreViewContainer);
+)(withToasts(ExploreViewContainer));

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

@@ -54,7 +54,7 @@ const initialState = {
   },
 };
 
-const store = mockStore(initialState);
+const initialStore = mockStore(initialState);
 
 const defaultProps = {
   onHide: () => ({}),
@@ -79,16 +79,36 @@ const mockDashboardData = {
   result: [{ id: 'id', dashboard_title: 'dashboard title' }],
 };
 
+const queryStore = mockStore({
+  chart: {},
+  saveModal: {
+    dashboards: [],
+  },
+  explore: {
+    datasource: { name: 'test', type: 'query' },
+    slice: null,
+    alert: null,
+  },
+  user: {
+    userId: 1,
+  },
+});
+
+const queryDefaultProps = {
+  ...defaultProps,
+  form_data: { datasource: '107__query', url_params: { foo: 'bar' } },
+};
+
 const fetchDashboardsEndpoint = `glob:*/dashboardasync/api/read?_flt_0_owners=${1}`;
 
 beforeAll(() => fetchMock.get(fetchDashboardsEndpoint, mockDashboardData));
 
 afterAll(() => fetchMock.restore());
 
-const getWrapper = () =>
+const getWrapper = (props = defaultProps, store = initialStore) =>
   shallow(
     <BrowserRouter>
-      <SaveModal {...defaultProps} store={store} />
+      <SaveModal {...props} store={store} />
     </BrowserRouter>,
   )
     .dive()
@@ -168,7 +188,7 @@ test('sets action when overwriting slice', () => {
 test('fetches dashboards on component mount', () => {
   sinon.spy(defaultProps.actions, 'fetchDashboards');
   mount(
-    <Provider store={store}>
+    <Provider store={initialStore}>
       <SaveModal {...defaultProps} />
     </Provider>,
   );
@@ -198,3 +218,9 @@ test('removes alert', () => {
   expect(wrapper.state().alert).toBeNull();
   defaultProps.actions.removeSaveModalAlert.restore();
 });
+
+test('set dataset name when chart source is query', () => {
+  const wrapper = getWrapper(queryDefaultProps, queryStore);
+  expect(wrapper.find('[data-test="new-dataset-name"]')).toExist();
+  expect(wrapper.state().datasetName).toBe('test');
+});

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 { t, styled } from '@superset-ui/core';
+import { t, styled, SupersetClient, DatasourceType } from '@superset-ui/core';
 import ReactMarkdown from 'react-markdown';
 import Modal from 'src/components/Modal';
 import { Radio } from 'src/components/Radio';
@@ -30,12 +30,16 @@ import { Select } from 'src/components';
 import { SelectValue } from 'antd/lib/select';
 import { connect } from 'react-redux';
 import { withRouter, RouteComponentProps } from 'react-router-dom';
+import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
+import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+import Loading from 'src/components/Loading';
 
 // 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');
 
 interface SaveModalProps extends RouteComponentProps {
+  addDangerToast: (msg: string) => void;
   onHide: () => void;
   actions: Record<string, any>;
   form_data?: Record<string, any>;
@@ -55,14 +59,22 @@ type SaveModalState = {
   saveToDashboardId: number | string | null;
   newSliceName?: string;
   newDashboardName?: string;
+  datasetName: string;
   alert: string | null;
   action: ActionType;
+  isLoading: boolean;
+  saveStatus?: string | null;
 };
 
 export const StyledModal = styled(Modal)`
   .ant-modal-body {
     overflow: visible;
   }
+  i {
+    position: absolute;
+    top: -${({ theme }) => theme.gridUnit * 5.25}px;
+    left: ${({ theme }) => theme.gridUnit * 26.75}px;
+  }
 `;
 
 class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
@@ -71,8 +83,11 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
     this.state = {
       saveToDashboardId: null,
       newSliceName: props.sliceName,
+      datasetName: props.datasource?.name,
       alert: null,
       action: this.canOverwriteSlice() ? 'overwrite' : 'saveas',
+      isLoading: false,
+      saveStatus: null,
     };
     this.onDashboardSelectChange = this.onDashboardSelectChange.bind(this);
     this.onSliceNameChange = this.onSliceNameChange.bind(this);
@@ -115,6 +130,11 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
     });
   }
 
+  handleDatasetNameChange = (e: React.FormEvent<HTMLInputElement>) => {
+    // @ts-expect-error
+    this.setState({ datasetName: e.target.value });
+  };
+
   onSliceNameChange(event: React.ChangeEvent<HTMLInputElement>) {
     this.setState({ newSliceName: event.target.value });
   }
@@ -130,8 +150,8 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
     this.setState({ action });
   }
 
-  saveOrOverwrite(gotodash: boolean) {
-    this.setState({ alert: null });
+  async saveOrOverwrite(gotodash: boolean) {
+    this.setState({ alert: null, isLoading: true });
     this.props.actions.removeSaveModalAlert();
     // eslint-disable-next-line @typescript-eslint/no-unused-vars
     const { url_params, ...formData } = this.props.form_data || {};
@@ -145,6 +165,43 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
       dashboard_title: string;
     };
 
+    if (this.props.datasource?.type === DatasourceType.Query) {
+      const { schema, sql, database } = this.props.datasource;
+      const { templateParams } = this.props.datasource;
+      const columns = this.props.datasource?.columns || [];
+
+      // Create a dataset object
+      await SupersetClient.post({
+        endpoint: '/superset/sqllab_viz/',
+        postPayload: {
+          data: {
+            schema,
+            sql,
+            dbId: database?.id,
+            templateParams,
+            datasourceName: this.state.datasetName,
+            metrics: [],
+            columns,
+          },
+        },
+      })
+        .then(({ json }) => json)
+        .then(async (data: { table_id: number }) => {
+          // Update form_data to point to new dataset
+          formData.datasource = `${data.table_id}__table`;
+          this.setState({ saveStatus: 'succeed' });
+        })
+        .catch(response =>
+          getClientErrorObject(response).then(e => {
+            this.setState({ isLoading: false, saveStatus: 'failed' });
+            this.props.addDangerToast(e.error);
+          }),
+        );
+    }
+
+    // Don't continue since server was unable to create dataset
+    if (this.state.saveStatus === 'failed') return;
+
     let dashboard: DashboardGetResponse | null = null;
     if (this.state.newDashboardName || this.state.saveToDashboardId) {
       let saveToDashboardId = this.state.saveToDashboardId || null;
@@ -221,9 +278,139 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
       this.props.history.replace(`/explore/?${searchParams.toString()}`);
     }) as (value: any) => void);
 
+    this.setState({ isLoading: false });
     this.props.onHide();
   }
 
+  renderSaveChartModal = () => {
+    const dashboardSelectValue =
+      this.state.saveToDashboardId || this.state.newDashboardName;
+
+    return (
+      <Form data-test="save-modal-body" layout="vertical">
+        {(this.state.alert || this.props.alert) && (
+          <Alert
+            type="warning"
+            message={
+              <>
+                {this.state.alert ? this.state.alert : this.props.alert}
+                <i
+                  role="button"
+                  aria-label="Remove alert"
+                  tabIndex={0}
+                  className="fa fa-close pull-right"
+                  onClick={this.removeAlert.bind(this)}
+                  style={{ cursor: 'pointer' }}
+                />
+              </>
+            }
+          />
+        )}
+        <FormItem data-test="radio-group">
+          <Radio
+            id="overwrite-radio"
+            disabled={!this.canOverwriteSlice()}
+            checked={this.state.action === 'overwrite'}
+            onChange={() => this.changeAction('overwrite')}
+            data-test="save-overwrite-radio"
+          >
+            {t('Save (Overwrite)')}
+          </Radio>
+          <Radio
+            id="saveas-radio"
+            data-test="saveas-radio"
+            checked={this.state.action === 'saveas'}
+            onChange={() => this.changeAction('saveas')}
+          >
+            {t('Save as...')}
+          </Radio>
+        </FormItem>
+        <hr />
+        <FormItem label={t('Chart name')} required>
+          <Input
+            name="new_slice_name"
+            type="text"
+            placeholder="Name"
+            value={this.state.newSliceName}
+            onChange={this.onSliceNameChange}
+            data-test="new-chart-name"
+          />
+        </FormItem>
+        {this.props.datasource?.type === 'query' && (
+          <FormItem label={t('Dataset Name')} required>
+            <InfoTooltipWithTrigger
+              tooltip={t('A reusable dataset will be saved with your chart.')}
+              placement="right"
+            />
+            <Input
+              name="dataset_name"
+              type="text"
+              placeholder="Dataset Name"
+              value={this.state.datasetName}
+              onChange={this.handleDatasetNameChange}
+              data-test="new-dataset-name"
+            />
+          </FormItem>
+        )}
+        <FormItem
+          label={t('Add to dashboard')}
+          data-test="save-chart-modal-select-dashboard-form"
+        >
+          <Select
+            allowClear
+            allowNewOptions
+            ariaLabel={t('Select a dashboard')}
+            options={this.props.dashboards}
+            onChange={this.onDashboardSelectChange}
+            value={dashboardSelectValue || undefined}
+            placeholder={
+              // Using markdown to allow for good i18n
+              <ReactMarkdown
+                source={SELECT_PLACEHOLDER}
+                renderers={{ paragraph: 'span' }}
+              />
+            }
+          />
+        </FormItem>
+      </Form>
+    );
+  };
+
+  renderFooter = () => (
+    <div data-test="save-modal-footer">
+      <Button id="btn_cancel" buttonSize="small" onClick={this.props.onHide}>
+        {t('Cancel')}
+      </Button>
+      <Button
+        id="btn_modal_save_goto_dash"
+        buttonSize="small"
+        disabled={
+          !this.state.newSliceName ||
+          (!this.state.saveToDashboardId && !this.state.newDashboardName)
+        }
+        onClick={() => this.saveOrOverwrite(true)}
+      >
+        {this.isNewDashboard()
+          ? t('Save & go to new dashboard')
+          : t('Save & go to dashboard')}
+      </Button>
+      <Button
+        id="btn_modal_save"
+        buttonSize="small"
+        buttonStyle="primary"
+        onClick={() => this.saveOrOverwrite(false)}
+        disabled={this.state.isLoading || !this.state.newSliceName}
+        data-test="btn-modal-save"
+      >
+        {!this.canOverwriteSlice() && this.props.slice
+          ? t('Save as new chart')
+          : this.isNewDashboard()
+          ? t('Save to new dashboard')
+          : t('Save')}
+      </Button>
+    </div>
+  );
+
   removeAlert() {
     if (this.props.alert) {
       this.props.actions.removeSaveModalAlert();
@@ -232,122 +419,18 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
   }
 
   render() {
-    const dashboardSelectValue =
-      this.state.saveToDashboardId || this.state.newDashboardName;
     return (
       <StyledModal
         show
         onHide={this.props.onHide}
         title={t('Save chart')}
-        footer={
-          <div data-test="save-modal-footer">
-            <Button
-              id="btn_cancel"
-              buttonSize="small"
-              onClick={this.props.onHide}
-            >
-              {t('Cancel')}
-            </Button>
-            <Button
-              id="btn_modal_save_goto_dash"
-              buttonSize="small"
-              disabled={
-                !this.state.newSliceName ||
-                (!this.state.saveToDashboardId && !this.state.newDashboardName)
-              }
-              onClick={() => this.saveOrOverwrite(true)}
-            >
-              {this.isNewDashboard()
-                ? t('Save & go to new dashboard')
-                : t('Save & go to dashboard')}
-            </Button>
-            <Button
-              id="btn_modal_save"
-              buttonSize="small"
-              buttonStyle="primary"
-              onClick={() => this.saveOrOverwrite(false)}
-              disabled={!this.state.newSliceName}
-              data-test="btn-modal-save"
-            >
-              {!this.canOverwriteSlice() && this.props.slice
-                ? t('Save as new chart')
-                : this.isNewDashboard()
-                ? t('Save to new dashboard')
-                : t('Save')}
-            </Button>
-          </div>
-        }
+        footer={this.renderFooter()}
       >
-        <Form data-test="save-modal-body" layout="vertical">
-          {(this.state.alert || this.props.alert) && (
-            <Alert
-              type="warning"
-              message={
-                <>
-                  {this.state.alert ? this.state.alert : this.props.alert}
-                  <i
-                    role="button"
-                    aria-label="Remove alert"
-                    tabIndex={0}
-                    className="fa fa-close pull-right"
-                    onClick={this.removeAlert.bind(this)}
-                    style={{ cursor: 'pointer' }}
-                  />
-                </>
-              }
-            />
-          )}
-          <FormItem data-test="radio-group">
-            <Radio
-              id="overwrite-radio"
-              disabled={!this.canOverwriteSlice()}
-              checked={this.state.action === 'overwrite'}
-              onChange={() => this.changeAction('overwrite')}
-              data-test="save-overwrite-radio"
-            >
-              {t('Save (Overwrite)')}
-            </Radio>
-            <Radio
-              id="saveas-radio"
-              data-test="saveas-radio"
-              checked={this.state.action === 'saveas'}
-              onChange={() => this.changeAction('saveas')}
-            >
-              {t('Save as...')}
-            </Radio>
-          </FormItem>
-          <hr />
-          <FormItem label={t('Chart name')} required>
-            <Input
-              name="new_slice_name"
-              type="text"
-              placeholder="Name"
-              value={this.state.newSliceName}
-              onChange={this.onSliceNameChange}
-              data-test="new-chart-name"
-            />
-          </FormItem>
-          <FormItem
-            label={t('Add to dashboard')}
-            data-test="save-chart-modal-select-dashboard-form"
-          >
-            <Select
-              allowClear
-              allowNewOptions
-              ariaLabel={t('Select a dashboard')}
-              options={this.props.dashboards}
-              onChange={this.onDashboardSelectChange}
-              value={dashboardSelectValue || undefined}
-              placeholder={
-                // Using markdown to allow for good i18n
-                <ReactMarkdown
-                  source={SELECT_PLACEHOLDER}
-                  renderers={{ paragraph: 'span' }}
-                />
-              }
-            />
-          </FormItem>
-        </Form>
+        {this.state.isLoading ? (
+          <Loading position="normal" />
+        ) : (
+          this.renderSaveChartModal()
+        )}
       </StyledModal>
     );
   }

superset-frontend/src/utils/localStorageHelpers.ts

@@ -41,6 +41,7 @@ export enum LocalStorageKeys {
   homepage_dashboard_filter = 'homepage_dashboard_filter',
   homepage_collapse_state = 'homepage_collapse_state',
   homepage_activity_filter = 'homepage_activity_filter',
+  datasetname_set_successful = 'datasetname_set_successful',
   /** END LEGACY LOCAL STORAGE KEYS */
 
   /**
@@ -66,6 +67,7 @@ export type LocalStorageValues = {
   homepage_chart_filter: TableTabTypes;
   homepage_dashboard_filter: TableTabTypes;
   homepage_collapse_state: string[];
+  datasetname_set_successful: boolean;
   homepage_activity_filter: SetTabType | null;
   sqllab__is_autocomplete_enabled: boolean;
   explore__data_table_original_formatted_time_columns: Record<string, string[]>;

superset/views/core.py

@@ -2100,8 +2100,20 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
             .filter_by(database_id=database_id, table_name=table_name)
             .one_or_none()
         )
-        if not table:
-            table = SqlaTable(table_name=table_name, owners=[g.user])
+
+        if table:
+            return json_errors_response(
+                [
+                    SupersetError(
+                        message=f"Dataset [{table_name}] already exists",
+                        error_type=SupersetErrorType.GENERIC_BACKEND_ERROR,
+                        level=ErrorLevel.WARNING,
+                    )
+                ],
+                status=422,
+            )
+
+        table = SqlaTable(table_name=table_name, owners=[g.user])
         table.database = database
         table.schema = data.get("schema")
         table.template_params = data.get("templateParams")