Skip to content

Commit

Permalink
fix: Explore misleading save action (#24862)
Browse files Browse the repository at this point in the history
  • Loading branch information
@michael-s-molina
michael-s-molina authored Aug 2, 2023
1 parent f7e76d0 commit bf1b1a4
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 127 deletions.
5 changes: 0 additions & 5 deletions superset-frontend/src/explore/actions/saveModalActions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]) =>
Expand Down
55 changes: 34 additions & 21 deletions superset-frontend/src/explore/components/SaveModal.test.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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();
Expand Down
205 changes: 107 additions & 98 deletions superset-frontend/src/explore/components/SaveModal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ interface SaveModalProps extends RouteComponentProps {
type SaveModalState = {
newSliceName?: string;
datasetName: string;
alert: string | null;
action: SaveActionType;
isLoading: boolean;
saveStatus?: string | null;
Expand All @@ -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,
Expand All @@ -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);
}

Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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">
Expand All @@ -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"
Expand All @@ -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
Expand Down
3 changes: 0 additions & 3 deletions superset-frontend/src/explore/reducers/saveModalReducer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
},
Expand Down

0 comments on commit bf1b1a4

Please sign in to comment.