fix(explore): Chart save modal displays error instead of failing silently

superset-frontend/src/explore/ExplorePage.tsx

@@ -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';
@@ -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
@@ -134,6 +136,7 @@ export default function ExplorePage() {
             hydrateExplore({
               ...result,
               form_data: formData,
+              saveAction,
             }),
           );
         })

superset-frontend/src/explore/actions/exploreActions.ts

@@ -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';
 
@@ -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,

superset-frontend/src/explore/actions/hydrateExplore.test.ts

@@ -70,6 +70,7 @@ test('creates hydrate action from initial data', () => {
         saveModal: {
           dashboards: [],
           saveModalAlert: null,
+          isVisible: false,
         },
         explore: {
           can_add: false,
@@ -84,6 +85,7 @@ test('creates hydrate action from initial data', () => {
           slice: exploreInitialData.slice,
           standalone: null,
           force: null,
+          saveAction: null,
         },
       },
     }),
@@ -140,6 +142,7 @@ test('creates hydrate action with existing state', () => {
         saveModal: {
           dashboards: [],
           saveModalAlert: null,
+          isVisible: false,
         },
         explore: {
           can_add: false,
@@ -155,6 +158,7 @@ test('creates hydrate action with existing state', () => {
           slice: exploreInitialData.slice,
           standalone: null,
           force: null,
+          saveAction: null,
         },
       },
     }),

superset-frontend/src/explore/actions/hydrateExplore.ts

@@ -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();
@@ -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
@@ -174,6 +181,7 @@ export const hydrateExplore =
         saveModal: {
           dashboards: [],
           saveModalAlert: null,
+          isVisible: false,
         },
         explore: exploreState,
       },

superset-frontend/src/explore/actions/saveModalActions.js

@@ -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({

superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx

@@ -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';
@@ -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,
@@ -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', () => {

superset-frontend/src/explore/components/ExploreChartHeader/index.jsx

@@ -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 {
@@ -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';
@@ -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 = {
@@ -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);
 
@@ -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,
@@ -244,7 +253,7 @@ export const ExploreChartHeader = ({
             <div>
               <Button
                 buttonStyle="secondary"
-                onClick={onSaveChart}
+                onClick={showModal}
                 disabled={saveDisabled}
                 data-test="query-save-button"
                 css={saveButtonStyles}
@@ -265,7 +274,7 @@ export const ExploreChartHeader = ({
         <PropertiesModal
           show={isPropertiesModalOpen}
           onHide={closePropertiesModal}
-          onSave={sliceUpdated}
+          onSave={updateSlice}
           slice={slice}
         />
       )}
@@ -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;

superset-frontend/src/explore/components/ExploreViewContainer/index.jsx

@@ -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`
@@ -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();
@@ -338,10 +339,6 @@ function ExploreViewContainer(props) {
     }
   }
 
-  function toggleModal() {
-    setShowingModal(!showingModal);
-  }
-
   function toggleCollapse() {
     setIsCollapsed(!isCollapsed);
   }
@@ -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);
     }
   });
 
@@ -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}
       />
@@ -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);
@@ -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
@@ -757,6 +760,8 @@ function mapStateToProps(state) {
     exploreState: explore,
     reports,
     metadata,
+    saveAction: explore.saveAction,
+    isSaveModalVisible: saveModal.isVisible,
   };
 }
 
@@ -776,4 +781,4 @@ function mapDispatchToProps(dispatch) {
 export default connect(
   mapStateToProps,
   mapDispatchToProps,
-)(withToasts(ExploreViewContainer));
+)(withToasts(React.memo(ExploreViewContainer)));