fix: Explore misleading save action (#24862)

2023-08-02 15:46:03 -03:00 · 2023-08-02 15:46:03 -03:00 · bf1b1a4c46
parent f7e76d02b7
commit bf1b1a4c46
4 changed files with 141 additions and 127 deletions
--- a/superset-frontend/src/explore/actions/saveModalActions.js
+++ b/superset-frontend/src/explore/actions/saveModalActions.js
@ -49,11 +49,6 @@ export function saveSliceSuccess(data) {
  return { type: SAVE_SLICE_SUCCESS, data };
 }

-export const REMOVE_SAVE_MODAL_ALERT = 'REMOVE_SAVE_MODAL_ALERT';
-export function removeSaveModalAlert() {
-  return { type: REMOVE_SAVE_MODAL_ALERT };
-}
-
 const extractAddHocFiltersFromFormData = formDataToHandle =>
  Object.entries(formDataToHandle).reduce(
    (acc, [key, value]) =>
--- a/superset-frontend/src/explore/components/SaveModal.test.jsx
+++ b/superset-frontend/src/explore/components/SaveModal.test.jsx
@ -24,7 +24,6 @@ import { bindActionCreators } from 'redux';
 import { shallow } from 'enzyme';
 import { Radio } from 'src/components/Radio';
 import Button from 'src/components/Button';
-import sinon from 'sinon';
 import fetchMock from 'fetch-mock';

 import * as saveModalActions from 'src/explore/actions/saveModalActions';
@ -131,7 +130,7 @@ test('renders a Modal with the right set of components', () => {
  expect(footerWrapper.find(Button)).toHaveLength(3);
 });

-test('renders the right footer buttons when existing dashboard selected', () => {
+test('renders the right footer buttons', () => {
  const wrapper = getWrapper();
  const footerWrapper = shallow(wrapper.find(StyledModal).props().footer);
  const saveAndGoDash = footerWrapper
@ -142,18 +141,43 @@ test('renders the right footer buttons when existing dashboard selected', () =>
  expect(saveAndGoDash.props.children).toBe('Save & go to dashboard');
 });

-test('renders the right footer buttons when new dashboard selected', () => {
+test('does not render a message when overriding', () => {
+  const wrapper = getWrapper();
+  wrapper.setState({
+    action: 'overwrite',
+  });
+  expect(
+    wrapper.find('[message="A new chart will be created."]'),
+  ).not.toExist();
+});
+
+test('renders a message when saving as', () => {
+  const wrapper = getWrapper();
+  wrapper.setState({
+    action: 'saveas',
+  });
+  expect(wrapper.find('[message="A new chart will be created."]')).toExist();
+});
+
+test('renders a message when a new dashboard is selected', () => {
  const wrapper = getWrapper();
  wrapper.setState({
    dashboard: { label: 'Test new dashboard', value: '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');
+  expect(
+    wrapper.find('[message="A new dashboard will be created."]'),
+  ).toExist();
+});
+
+test('renders a message when saving as with new dashboard', () => {
+  const wrapper = getWrapper();
+  wrapper.setState({
+    action: 'saveas',
+    dashboard: { label: 'Test new dashboard', value: 'Test new dashboard' },
+  });
+  expect(
+    wrapper.find('[message="A new chart and dashboard will be created."]'),
+  ).toExist();
 });

 test('disables overwrite option for new slice', () => {
@ -197,17 +221,6 @@ test('updates slice name and selected dashboard', () => {
  expect(wrapper.state().dashboard.value).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();
-});
-
 test('set dataset name when chart source is query', () => {
  const wrapper = getWrapper(queryDefaultProps, queryStore);
  expect(wrapper.find('[data-test="new-dataset-name"]')).toExist();
--- a/superset-frontend/src/explore/components/SaveModal.tsx
+++ b/superset-frontend/src/explore/components/SaveModal.tsx
@ -67,7 +67,6 @@ interface SaveModalProps extends RouteComponentProps {
 type SaveModalState = {
  newSliceName?: string;
  datasetName: string;
-  alert: string | null;
  action: SaveActionType;
  isLoading: boolean;
  saveStatus?: string | null;
@ -92,7 +91,6 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
    this.state = {
      newSliceName: props.sliceName,
      datasetName: props.datasource?.name,
-      alert: null,
      action: this.canOverwriteSlice() ? 'overwrite' : 'saveas',
      isLoading: false,
      vizType: props.form_data?.viz_type,
@ -103,7 +101,6 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
    this.changeAction = this.changeAction.bind(this);
    this.saveOrOverwrite = this.saveOrOverwrite.bind(this);
    this.isNewDashboard = this.isNewDashboard.bind(this);
-    this.removeAlert = this.removeAlert.bind(this);
    this.onHide = this.onHide.bind(this);
  }

@ -163,8 +160,7 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
  }

  async saveOrOverwrite(gotodash: boolean) {
-    this.setState({ alert: null, isLoading: true });
-    this.props.actions.removeSaveModalAlert();
+    this.setState({ isLoading: true });

    //  Create or retrieve dashboard
    type DashboardGetResponse = {
@ -324,89 +320,115 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
    };
  };

-  renderSaveChartModal = () => (
-    <Form data-test="save-modal-body" layout="vertical">
-      {(this.state.alert || this.props.alert) && (
-        <Alert
-          type="warning"
-          message={this.state.alert || this.props.alert}
-          onClose={this.removeAlert}
-        />
-      )}
-      <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"
-          />
+  renderSaveChartModal = () => {
+    const info = this.info();
+    return (
+      <Form data-test="save-modal-body" layout="vertical">
+        <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="dataset_name"
+            name="new_slice_name"
            type="text"
-            placeholder="Dataset Name"
-            value={this.state.datasetName}
-            onChange={this.handleDatasetNameChange}
-            data-test="new-dataset-name"
+            placeholder="Name"
+            value={this.state.newSliceName}
+            onChange={this.onSliceNameChange}
+            data-test="new-chart-name"
          />
        </FormItem>
-      )}
-      {!(
-        isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) &&
-        this.state.vizType === 'filter_box'
-      ) && (
-        <FormItem
-          label={t('Add to dashboard')}
-          data-test="save-chart-modal-select-dashboard-form"
-        >
-          <AsyncSelect
-            allowClear
-            allowNewOptions
-            ariaLabel={t('Select a dashboard')}
-            options={this.loadDashboards}
-            onChange={this.onDashboardChange}
-            value={this.state.dashboard}
-            placeholder={
-              <div>
-                <b>{t('Select')}</b>
-                {t(' a dashboard OR ')}
-                <b>{t('create')}</b>
-                {t(' a new one')}
-              </div>
-            }
+        {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>
+        )}
+        {!(
+          isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) &&
+          this.state.vizType === 'filter_box'
+        ) && (
+          <FormItem
+            label={t('Add to dashboard')}
+            data-test="save-chart-modal-select-dashboard-form"
+          >
+            <AsyncSelect
+              allowClear
+              allowNewOptions
+              ariaLabel={t('Select a dashboard')}
+              options={this.loadDashboards}
+              onChange={this.onDashboardChange}
+              value={this.state.dashboard}
+              placeholder={
+                <div>
+                  <b>{t('Select')}</b>
+                  {t(' a dashboard OR ')}
+                  <b>{t('create')}</b>
+                  {t(' a new one')}
+                </div>
+              }
+            />
+          </FormItem>
+        )}
+        {info && <Alert type="info" message={info} closable={false} />}
+        {this.props.alert && (
+          <Alert
+            css={{ marginTop: info ? 16 : undefined }}
+            type="warning"
+            message={this.props.alert}
+            closable={false}
          />
-        </FormItem>
-      )}
-    </Form>
-  );
+        )}
+      </Form>
+    );
+  };
+
+  info = () => {
+    const isNewDashboard = this.isNewDashboard();
+    let chartWillBeCreated = false;
+    if (
+      this.props.slice &&
+      (this.state.action !== 'overwrite' || !this.canOverwriteSlice())
+    ) {
+      chartWillBeCreated = true;
+    }
+    if (chartWillBeCreated && isNewDashboard) {
+      return t('A new chart and dashboard will be created.');
+    }
+    if (chartWillBeCreated) {
+      return t('A new chart will be created.');
+    }
+    if (isNewDashboard) {
+      return t('A new dashboard will be created.');
+    }
+    return null;
+  };

  renderFooter = () => (
    <div data-test="save-modal-footer">
@ -426,9 +448,7 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
        }
        onClick={() => this.saveOrOverwrite(true)}
      >
-        {this.isNewDashboard()
-          ? t('Save & go to new dashboard')
-          : t('Save & go to dashboard')}
+        {t('Save & go to dashboard')}
      </Button>
      <Button
        id="btn_modal_save"
@ -443,22 +463,11 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
        }
        data-test="btn-modal-save"
      >
-        {!this.canOverwriteSlice() && this.props.slice
-          ? t('Save as new chart')
-          : this.isNewDashboard()
-          ? t('Save to new dashboard')
-          : t('Save')}
+        {t('Save')}
      </Button>
    </div>
  );

-  removeAlert() {
-    if (this.props.alert) {
-      this.props.actions.removeSaveModalAlert();
-    }
-    this.setState({ alert: null });
-  }
-
  render() {
    return (
      <StyledModal
--- a/superset-frontend/src/explore/reducers/saveModalReducer.js
+++ b/superset-frontend/src/explore/reducers/saveModalReducer.js
@ -40,9 +40,6 @@ export default function saveModalReducer(state = {}, action) {
    [actions.SAVE_SLICE_SUCCESS](data) {
      return { ...state, data };
    },
-    [actions.REMOVE_SAVE_MODAL_ALERT]() {
-      return { ...state, saveModalAlert: null };
-    },
    [HYDRATE_EXPLORE]() {
      return { ...action.data.saveModal };
    },