diff --git a/superset-frontend/src/explore/ExplorePage.tsx b/superset-frontend/src/explore/ExplorePage.tsx index ce36cfca252c9..6084cee7826e3 100644 --- a/superset-frontend/src/explore/ExplorePage.tsx +++ b/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, }), ); }) diff --git a/superset-frontend/src/explore/actions/exploreActions.ts b/superset-frontend/src/explore/actions/exploreActions.ts index a2e6fde69b349..0e13499b14c43 100644 --- a/superset-frontend/src/explore/actions/exploreActions.ts +++ b/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, diff --git a/superset-frontend/src/explore/actions/hydrateExplore.test.ts b/superset-frontend/src/explore/actions/hydrateExplore.test.ts index e7858b167086d..353a811396d47 100644 --- a/superset-frontend/src/explore/actions/hydrateExplore.test.ts +++ b/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, }, }, }), diff --git a/superset-frontend/src/explore/actions/hydrateExplore.ts b/superset-frontend/src/explore/actions/hydrateExplore.ts index e4bd1bb82f210..1f4f510b5e4e1 100644 --- a/superset-frontend/src/explore/actions/hydrateExplore.ts +++ b/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, }, diff --git a/superset-frontend/src/explore/actions/saveModalActions.js b/superset-frontend/src/explore/actions/saveModalActions.js index 1764a009c3cfc..14bbe09995458 100644 --- a/superset-frontend/src/explore/actions/saveModalActions.js +++ b/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({ diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx index 2b2ef275513c1..74d1764f4a31f 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx +++ b/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(, { 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(, { 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', () => { diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 8e7e5b6e69742..2b68120fb6e80 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/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 = ({
+ {props.isSaveModalVisible && ( + + )} ); } @@ -708,8 +703,16 @@ function ExploreViewContainer(props) { 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))); diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index a4649d8926d8c..dde8d8c04f45e 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -18,20 +18,30 @@ */ /* eslint camelcase: 0 */ import React from 'react'; +import { Dispatch } from 'redux'; +import { SelectValue } from 'antd/lib/select'; +import { connect } from 'react-redux'; +import ReactMarkdown from 'react-markdown'; +import { withRouter, RouteComponentProps } from 'react-router-dom'; +import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls'; +import { + css, + t, + styled, + DatasourceType, + isDefined, + ensureIsArray, +} from '@superset-ui/core'; import { Input } from 'src/components/Input'; import { Form, FormItem } from 'src/components/Form'; import Alert from 'src/components/Alert'; -import { t, styled, DatasourceType } from '@superset-ui/core'; -import ReactMarkdown from 'react-markdown'; import Modal from 'src/components/Modal'; import { Radio } from 'src/components/Radio'; import Button from 'src/components/Button'; import { Select } from 'src/components'; -import { SelectValue } from 'antd/lib/select'; -import { connect } from 'react-redux'; -import { withRouter, RouteComponentProps } from 'react-router-dom'; -import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls'; import Loading from 'src/components/Loading'; +import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; +import { SaveActionType } from 'src/explore/types'; // Session storage key for recent dashboard const SK_DASHBOARD_ID = 'save_chart_recent_dashboard'; @@ -39,7 +49,6 @@ const SELECT_PLACEHOLDER = t('**Select** a dashboard OR **create** a new one'); interface SaveModalProps extends RouteComponentProps { addDangerToast: (msg: string) => void; - onHide: () => void; actions: Record; form_data?: Record; userId: number; @@ -49,17 +58,17 @@ interface SaveModalProps extends RouteComponentProps { slice?: Record; datasource?: Record; dashboardId: '' | number | null; + isVisible: boolean; + dispatch: Dispatch; } -type ActionType = 'overwrite' | 'saveas'; - type SaveModalState = { saveToDashboardId: number | string | null; newSliceName?: string; newDashboardName?: string; datasetName: string; alert: string | null; - action: ActionType; + action: SaveActionType; isLoading: boolean; saveStatus?: string | null; }; @@ -91,6 +100,8 @@ class SaveModal extends React.Component { 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); } isNewDashboard(): boolean { @@ -106,7 +117,10 @@ class SaveModal extends React.Component { componentDidMount() { this.props.actions.fetchDashboards(this.props.userId).then(() => { - const dashboardIds = this.props.dashboards.map( + if (ensureIsArray(this.props.dashboards).length === 0) { + return; + } + const dashboardIds = this.props.dashboards?.map( dashboard => dashboard.value, ); const lastDashboard = sessionStorage.getItem(SK_DASHBOARD_ID); @@ -143,16 +157,17 @@ class SaveModal extends React.Component { this.setState({ saveToDashboardId, newDashboardName }); } - changeAction(action: ActionType) { + changeAction(action: SaveActionType) { this.setState({ action }); } + onHide() { + this.props.dispatch(setSaveChartModalVisibility(false)); + } + async saveOrOverwrite(gotodash: boolean) { this.setState({ alert: null, isLoading: true }); this.props.actions.removeSaveModalAlert(); - // eslint-disable-next-line @typescript-eslint/no-unused-vars - - let promise = Promise.resolve(); // Create or retrieve dashboard type DashboardGetResponse = { @@ -161,12 +176,12 @@ class SaveModal extends React.Component { dashboard_title: string; }; - if (this.props.datasource?.type === DatasourceType.Query) { - const { schema, sql, database } = this.props.datasource; - const { templateParams } = this.props.datasource; - const columns = this.props.datasource?.columns || []; + try { + if (this.props.datasource?.type === DatasourceType.Query) { + const { schema, sql, database } = this.props.datasource; + const { templateParams } = this.props.datasource; + const columns = this.props.datasource?.columns || []; - try { await this.props.actions.saveDataset({ schema, sql, @@ -175,40 +190,31 @@ class SaveModal extends React.Component { datasourceName: this.state.datasetName, columns, }); - } catch { - // Don't continue since server was unable to create dataset - this.setState({ isLoading: false }); - return; } - } - // Get chart dashboards - let sliceDashboards: number[] = []; - if (this.props.slice && this.state.action === 'overwrite') { - promise = promise - .then(() => this.props.actions.getSliceDashboards(this.props.slice)) - .then(dashboards => { - sliceDashboards = dashboards; - }); - } - - let dashboard: DashboardGetResponse | null = null; - if (this.state.newDashboardName || this.state.saveToDashboardId) { - let saveToDashboardId = this.state.saveToDashboardId || null; - if (!this.state.saveToDashboardId) { - promise = promise - .then(() => - this.props.actions.createDashboard(this.state.newDashboardName), - ) - .then((response: { id: number }) => { - saveToDashboardId = response.id; - }); + // Get chart dashboards + let sliceDashboards: number[] = []; + if (this.props.slice && this.state.action === 'overwrite') { + sliceDashboards = await this.props.actions.getSliceDashboards( + this.props.slice, + ); } - promise = promise - .then(() => this.props.actions.getDashboard(saveToDashboardId)) - .then((response: { result: DashboardGetResponse }) => { - dashboard = response.result; + let dashboard: DashboardGetResponse | null = null; + if (this.state.newDashboardName || this.state.saveToDashboardId) { + let saveToDashboardId = this.state.saveToDashboardId || null; + if (!this.state.saveToDashboardId) { + const response = await this.props.actions.createDashboard( + this.state.newDashboardName, + ); + saveToDashboardId = response.id; + } + + const response = await this.props.actions.getDashboard( + saveToDashboardId, + ); + dashboard = response.result; + if (isDefined(dashboard) && isDefined(dashboard?.id)) { sliceDashboards = sliceDashboards.includes(dashboard.id) ? sliceDashboards : [...sliceDashboards, dashboard.id]; @@ -217,13 +223,13 @@ class SaveModal extends React.Component { ...formData, dashboards: sliceDashboards, }); - }); - } + } + } - // Update or create slice - if (this.state.action === 'overwrite') { - promise = promise.then(() => - this.props.actions.updateSlice( + // Update or create slice + let value: { id: number }; + if (this.state.action === 'overwrite') { + value = await this.props.actions.updateSlice( this.props.slice, this.state.newSliceName, sliceDashboards, @@ -233,11 +239,9 @@ class SaveModal extends React.Component { new: !this.state.saveToDashboardId, } : null, - ), - ); - } else { - promise = promise.then(() => - this.props.actions.createSlice( + ); + } else { + value = await this.props.actions.createSlice( this.state.newSliceName, sliceDashboards, dashboard @@ -246,12 +250,9 @@ class SaveModal extends React.Component { new: !this.state.saveToDashboardId, } : null, - ), - ); - } + ); + } - promise.then(((value: { id: number }) => { - // Update recent dashboard if (dashboard) { sessionStorage.setItem(SK_DASHBOARD_ID, `${dashboard.id}`); } else { @@ -271,10 +272,12 @@ class SaveModal extends React.Component { searchParams.set('slice_id', value.id.toString()); } this.props.history.replace(`/explore/?${searchParams.toString()}`); - }) as (value: any) => void); - this.setState({ isLoading: false }); - this.props.onHide(); + this.setState({ isLoading: false }); + this.onHide(); + } finally { + this.setState({ isLoading: false }); + } } renderSaveChartModal = () => { @@ -286,19 +289,8 @@ class SaveModal extends React.Component { {(this.state.alert || this.props.alert) && ( - {this.state.alert || this.props.alert} - - - } + message={this.state.alert || this.props.alert} + onClose={this.removeAlert} /> )} @@ -373,7 +365,7 @@ class SaveModal extends React.Component { renderFooter = () => (
-