Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Explore misleading save action #24862

Merged
merged 2 commits into from
Aug 2, 2023
Merged

fix: Explore misleading save action #24862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a78a85e
fix: Explore misleading save action
michael-s-molina Aug 1, 2023
9063106
Adds test
michael-s-molina Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions superset-frontend/src/explore/actions/saveModalActions.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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 ')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These labels should be a good example of adding unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but I'll focus on the PR changes. This part was only indented.

<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
Open in desktop
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