Skip to content

Commit

Permalink
fix(explore): Chart save modal displays error instead of failing sile…
Browse files Browse the repository at this point in the history
…ntly (#21920)
  • Loading branch information
kgabryje authored Oct 27, 2022
1 parent fb8231b commit 9d25453
Show file tree
Hide file tree
Showing 12 changed files with 189 additions and 132 deletions.
9 changes: 6 additions & 3 deletions superset-frontend/src/explore/ExplorePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import { getAppliedFilterValues } from 'src/dashboard/util/activeDashboardFilter
import { getParsedExploreURLParams } from './exploreUtils/getParsedExploreURLParams';
import { hydrateExplore } from './actions/hydrateExplore';
import ExploreViewContainer from './components/ExploreViewContainer';
import { ExploreResponsePayload } from './types';
import { ExploreResponsePayload, SaveActionType } from './types';
import { fallbackExploreInitialData } from './fixtures';
import { getItem, LocalStorageKeys } from '../utils/localStorageHelpers';
import { getFormDataWithDashboardContext } from './controlUtils/getFormDataWithDashboardContext';
Expand Down Expand Up @@ -119,9 +119,11 @@ export default function ExplorePage() {

useEffect(() => {
const exploreUrlParams = getParsedExploreURLParams(location);
const isSaveAction = !!getUrlParam(URL_PARAMS.saveAction);
const saveAction = getUrlParam(
URL_PARAMS.saveAction,
) as SaveActionType | null;
const dashboardContextFormData = getDashboardContextFormData();
if (!isExploreInitialized.current || isSaveAction) {
if (!isExploreInitialized.current || !!saveAction) {
fetchExploreData(exploreUrlParams)
.then(({ result }) => {
const formData = dashboardContextFormData
Expand All @@ -134,6 +136,7 @@ export default function ExplorePage() {
hydrateExplore({
...result,
form_data: formData,
saveAction,
}),
);
})
Expand Down
6 changes: 6 additions & 0 deletions superset-frontend/src/explore/actions/exploreActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
toastActions,
} from 'src/components/MessageToasts/actions';
import { Slice } from 'src/types/Chart';
import { SaveActionType } from 'src/explore/types';

const FAVESTAR_BASE_URL = '/superset/favstar/slice';

Expand Down Expand Up @@ -114,6 +115,11 @@ export function updateChartTitle(sliceName: string) {
return { type: UPDATE_CHART_TITLE, sliceName };
}

export const SET_SAVE_ACTION = 'SET_SAVE_ACTION';
export function setSaveAction(saveAction: SaveActionType | null) {
return { type: SET_SAVE_ACTION, saveAction };
}

export const CREATE_NEW_SLICE = 'CREATE_NEW_SLICE';
export function createNewSlice(
can_add: boolean,
Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/src/explore/actions/hydrateExplore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ test('creates hydrate action from initial data', () => {
saveModal: {
dashboards: [],
saveModalAlert: null,
isVisible: false,
},
explore: {
can_add: false,
Expand All @@ -84,6 +85,7 @@ test('creates hydrate action from initial data', () => {
slice: exploreInitialData.slice,
standalone: null,
force: null,
saveAction: null,
},
},
}),
Expand Down Expand Up @@ -140,6 +142,7 @@ test('creates hydrate action with existing state', () => {
saveModal: {
dashboards: [],
saveModalAlert: null,
isVisible: false,
},
explore: {
can_add: false,
Expand All @@ -155,6 +158,7 @@ test('creates hydrate action with existing state', () => {
slice: exploreInitialData.slice,
standalone: null,
force: null,
saveAction: null,
},
},
}),
Expand Down
10 changes: 9 additions & 1 deletion superset-frontend/src/explore/actions/hydrateExplore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,13 @@ enum ColorSchemeType {

export const HYDRATE_EXPLORE = 'HYDRATE_EXPLORE';
export const hydrateExplore =
({ form_data, slice, dataset, metadata }: ExplorePageInitialData) =>
({
form_data,
slice,
dataset,
metadata,
saveAction = null,
}: ExplorePageInitialData) =>
(dispatch: Dispatch, getState: () => ExplorePageState) => {
const { user, datasources, charts, sliceEntities, common, explore } =
getState();
Expand Down Expand Up @@ -129,6 +135,7 @@ export const hydrateExplore =
standalone: getUrlParam(URL_PARAMS.standalone),
force: getUrlParam(URL_PARAMS.force),
metadata,
saveAction,
};

// apply initial mapStateToProps for all controls, must execute AFTER
Expand Down Expand Up @@ -174,6 +181,7 @@ export const hydrateExplore =
saveModal: {
dashboards: [],
saveModalAlert: null,
isVisible: false,
},
explore: exploreState,
},
Expand Down
6 changes: 6 additions & 0 deletions superset-frontend/src/explore/actions/saveModalActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ export function fetchDashboardsFailed(userId) {
return { type: FETCH_DASHBOARDS_FAILED, userId };
}

export const SET_SAVE_CHART_MODAL_VISIBILITY =
'SET_SAVE_CHART_MODAL_VISIBILITY';
export function setSaveChartModalVisibility(isVisible) {
return { type: SET_SAVE_CHART_MODAL_VISIBILITY, isVisible };
}

export function fetchDashboards(userId) {
return function fetchDashboardsThunk(dispatch) {
return SupersetClient.get({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { render, screen, waitFor } from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import fetchMock from 'fetch-mock';
import * as chartAction from 'src/components/Chart/chartAction';
import * as saveModalActions from 'src/explore/actions/saveModalActions';
import * as downloadAsImage from 'src/utils/downloadAsImage';
import * as exploreUtils from 'src/explore/exploreUtils';
import { FeatureFlag } from '@superset-ui/core';
Expand Down Expand Up @@ -114,7 +115,6 @@ const createProps = (additionalProps = {}) => ({
changed_by: 'John Doe',
dashboards: [{ id: 1, dashboard_title: 'Test' }],
},
onSaveChart: jest.fn(),
canOverwrite: false,
canDownload: false,
isStarred: false,
Expand Down Expand Up @@ -178,19 +178,29 @@ test('does not render the metadata bar when not saved', async () => {
});

test('Save chart', async () => {
const setSaveChartModalVisibility = jest.spyOn(
saveModalActions,
'setSaveChartModalVisibility',
);
const props = createProps();
render(<ExploreHeader {...props} />, { useRedux: true });
expect(await screen.findByText('Save')).toBeInTheDocument();
userEvent.click(screen.getByText('Save'));
expect(props.onSaveChart).toHaveBeenCalled();
expect(setSaveChartModalVisibility).toHaveBeenCalledWith(true);
setSaveChartModalVisibility.mockClear();
});

test('Save disabled', async () => {
const setSaveChartModalVisibility = jest.spyOn(
saveModalActions,
'setSaveChartModalVisibility',
);
const props = createProps();
render(<ExploreHeader {...props} saveDisabled />, { useRedux: true });
expect(await screen.findByText('Save')).toBeInTheDocument();
userEvent.click(screen.getByText('Save'));
expect(props.onSaveChart).not.toHaveBeenCalled();
expect(setSaveChartModalVisibility).not.toHaveBeenCalled();
setSaveChartModalVisibility.mockClear();
});

describe('Additional actions tests', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useEffect, useMemo, useState } from 'react';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import { useDispatch } from 'react-redux';
import PropTypes from 'prop-types';
import { Tooltip } from 'src/components/Tooltip';
import {
Expand All @@ -28,7 +27,6 @@ import {
SupersetClient,
t,
} from '@superset-ui/core';
import { toggleActive, deleteActiveReport } from 'src/reports/actions/reports';
import { chartPropShape } from 'src/dashboard/util/propShapes';
import AlteredSliceTag from 'src/components/AlteredSliceTag';
import Button from 'src/components/Button';
Expand All @@ -37,6 +35,7 @@ import PropertiesModal from 'src/explore/components/PropertiesModal';
import { sliceUpdated } from 'src/explore/actions/exploreActions';
import { PageHeaderWithActions } from 'src/components/PageHeaderWithActions';
import MetadataBar, { MetadataType } from 'src/components/MetadataBar';
import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions';
import { useExploreAdditionalActionsMenu } from '../useExploreAdditionalActionsMenu';

const propTypes = {
Expand Down Expand Up @@ -82,12 +81,11 @@ export const ExploreChartHeader = ({
canOverwrite,
canDownload,
isStarred,
sliceUpdated,
sliceName,
onSaveChart,
saveDisabled,
metadata,
}) => {
const dispatch = useDispatch();
const { latestQueryFormData, sliceFormData } = chart;
const [isPropertiesModalOpen, setIsPropertiesModalOpen] = useState(false);

Expand Down Expand Up @@ -141,6 +139,17 @@ export const ExploreChartHeader = ({
setIsPropertiesModalOpen(false);
};

const showModal = useCallback(() => {
dispatch(setSaveChartModalVisibility(true));
}, [dispatch]);

const updateSlice = useCallback(
slice => {
dispatch(sliceUpdated(slice));
},
[dispatch],
);

const [menu, isDropdownVisible, setIsDropdownVisible] =
useExploreAdditionalActionsMenu(
latestQueryFormData,
Expand Down Expand Up @@ -244,7 +253,7 @@ export const ExploreChartHeader = ({
<div>
<Button
buttonStyle="secondary"
onClick={onSaveChart}
onClick={showModal}
disabled={saveDisabled}
data-test="query-save-button"
css={saveButtonStyles}
Expand All @@ -265,7 +274,7 @@ export const ExploreChartHeader = ({
<PropertiesModal
show={isPropertiesModalOpen}
onHide={closePropertiesModal}
onSave={sliceUpdated}
onSave={updateSlice}
slice={slice}
/>
)}
Expand All @@ -275,11 +284,4 @@ export const ExploreChartHeader = ({

ExploreChartHeader.propTypes = propTypes;

function mapDispatchToProps(dispatch) {
return bindActionCreators(
{ sliceUpdated, toggleActive, deleteActiveReport },
dispatch,
);
}

export default connect(null, mapDispatchToProps)(ExploreChartHeader);
export default ExploreChartHeader;
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ const propTypes = {
timeout: PropTypes.number,
impressionId: PropTypes.string,
vizType: PropTypes.string,
saveAction: PropTypes.string,
isSaveModalVisible: PropTypes.bool,
};

const ExploreContainer = styled.div`
Expand Down Expand Up @@ -240,7 +242,6 @@ function ExploreViewContainer(props) {
props.controls,
);

const [showingModal, setShowingModal] = useState(false);
const [isCollapsed, setIsCollapsed] = useState(false);
const [shouldForceUpdate, setShouldForceUpdate] = useState(-1);
const tabId = useTabId();
Expand Down Expand Up @@ -338,10 +339,6 @@ function ExploreViewContainer(props) {
}
}

function toggleModal() {
setShowingModal(!showingModal);
}

function toggleCollapse() {
setIsCollapsed(!isCollapsed);
}
Expand Down Expand Up @@ -471,11 +468,11 @@ function ExploreViewContainer(props) {
return false;
}, [lastQueriedControls, props.controls]);

const saveAction = getUrlParam(URL_PARAMS.saveAction);
useChangeEffect(saveAction, () => {
if (['saveas', 'overwrite'].includes(saveAction)) {
useChangeEffect(props.saveAction, () => {
if (['saveas', 'overwrite'].includes(props.saveAction)) {
onQuery();
addHistory({ isReplace: true });
props.actions.setSaveAction(null);
}
});

Expand Down Expand Up @@ -566,7 +563,6 @@ function ExploreViewContainer(props) {
ownState={props.ownState}
user={props.user}
reports={props.reports}
onSaveChart={toggleModal}
saveDisabled={errorMessage || props.chart.chartStatus === 'loading'}
metadata={props.metadata}
/>
Expand Down Expand Up @@ -595,16 +591,6 @@ function ExploreViewContainer(props) {
}
`}
/>
{showingModal && (
<SaveModal
addDangerToast={props.addDangerToast}
onHide={toggleModal}
actions={props.actions}
form_data={props.form_data}
sliceName={props.sliceName}
dashboardId={props.dashboardId}
/>
)}
<Resizable
onResizeStop={(evt, direction, ref, d) => {
setShouldForceUpdate(d?.width);
Expand Down Expand Up @@ -701,15 +687,32 @@ function ExploreViewContainer(props) {
{renderChartContainer()}
</div>
</ExplorePanelContainer>
{props.isSaveModalVisible && (
<SaveModal
addDangerToast={props.addDangerToast}
actions={props.actions}
form_data={props.form_data}
sliceName={props.sliceName}
dashboardId={props.dashboardId}
/>
)}
</ExploreContainer>
);
}

ExploreViewContainer.propTypes = propTypes;

function mapStateToProps(state) {
const { explore, charts, common, impressionId, dataMask, reports, user } =
state;
const {
explore,
charts,
common,
impressionId,
dataMask,
reports,
user,
saveModal,
} = state;
const { controls, slice, datasource, metadata } = explore;
const form_data = getFormDataFromControls(controls);
const slice_id = form_data.slice_id ?? slice?.slice_id ?? 0; // 0 - unsaved chart
Expand Down Expand Up @@ -757,6 +760,8 @@ function mapStateToProps(state) {
exploreState: explore,
reports,
metadata,
saveAction: explore.saveAction,
isSaveModalVisible: saveModal.isVisible,
};
}

Expand All @@ -776,4 +781,4 @@ function mapDispatchToProps(dispatch) {
export default connect(
mapStateToProps,
mapDispatchToProps,
)(withToasts(ExploreViewContainer));
)(withToasts(React.memo(ExploreViewContainer)));
Loading

0 comments on commit 9d25453

Please sign in to comment.