You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2023/08/04 17:13:44 UTC
[superset] 11/16: fix: Explore misleading save action (#24862)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 3.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit e47377e5764ddb5b9af8e621a0c70afe45ce1a0b
Author: Michael S. Molina <70...@users.noreply.github.com>
AuthorDate: Wed Aug 2 15:46:03 2023 -0300
fix: Explore misleading save action (#24862)
(cherry picked from commit bf1b1a4c46c4de6ed4f0f576fc459d0d5e94e6f3)
---
.../src/explore/actions/saveModalActions.js | 5 -
.../src/explore/components/SaveModal.test.jsx | 55 +++---
.../src/explore/components/SaveModal.tsx | 205 +++++++++++----------
.../src/explore/reducers/saveModalReducer.js | 3 -
4 files changed, 141 insertions(+), 127 deletions(-)
diff --git a/superset-frontend/src/explore/actions/saveModalActions.js b/superset-frontend/src/explore/actions/saveModalActions.js
index de4f7af84c..1a9970ab75 100644
--- 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]) =>
diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx
index d9d6f2983d..bdb93e5429 100644
--- 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();
diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx
index 592bab2f9f..3142d6b758 100644
--- 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
diff --git a/superset-frontend/src/explore/reducers/saveModalReducer.js b/superset-frontend/src/explore/reducers/saveModalReducer.js
index 85983e1ef1..572cabde39 100644
--- 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 };
},