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

…ntly (#21920)

superset-frontend/src/explore/ExplorePage.tsx

@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useRef, useState } from 'react';
+import { useDispatch } from 'react-redux';
+import { useLocation } from 'react-router-dom';
+import {
+  getSharedLabelColor,
+  isDefined,
+  JsonObject,
+  makeApi,
+  SharedLabelColorSource,
+  t,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+import { getUrlParam } from 'src/utils/urlUtils';
+import { URL_PARAMS } from 'src/constants';
+import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+import getFormDataWithExtraFilters from 'src/dashboard/util/charts/getFormDataWithExtraFilters';
+import { getAppliedFilterValues } from 'src/dashboard/util/activeDashboardFilters';
+import { getParsedExploreURLParams } from './exploreUtils/getParsedExploreURLParams';
+import { hydrateExplore } from './actions/hydrateExplore';
+import ExploreViewContainer from './components/ExploreViewContainer';
+import { ExploreResponsePayload, SaveActionType } from './types';
+import { fallbackExploreInitialData } from './fixtures';
+import { getItem, LocalStorageKeys } from '../utils/localStorageHelpers';
+import { getFormDataWithDashboardContext } from './controlUtils/getFormDataWithDashboardContext';
+
+const isResult = (rv: JsonObject): rv is ExploreResponsePayload =>
+  rv?.result?.form_data &&
+  rv?.result?.dataset &&
+  isDefined(rv?.result?.dataset?.id);
+
+const fetchExploreData = async (exploreUrlParams: URLSearchParams) => {
+  try {
+    const rv = await makeApi<{}, ExploreResponsePayload>({
+      method: 'GET',
+      endpoint: 'api/v1/explore/',
+    })(exploreUrlParams);
+    if (isResult(rv)) {
+      return rv;
+    }
+    throw new Error(t('Failed to load chart data.'));
+  } catch (err) {
+    // todo: encapsulate the error handler
+    const clientError = await getClientErrorObject(err);
+    throw new Error(
+      clientError.message ||
+        clientError.error ||
+        t('Failed to load chart data.'),
+    );
+  }
+};
+
+const getDashboardPageContext = (pageId?: string | null) => {
+  if (!pageId) {
+    return null;
+  }
+  return (
+    getItem(LocalStorageKeys.dashboard__explore_context, {})[pageId] || null
+  );
+};
+
+const getDashboardContextFormData = () => {
+  const dashboardPageId = getUrlParam(URL_PARAMS.dashboardPageId);
+  const dashboardContext = getDashboardPageContext(dashboardPageId);
+  if (dashboardContext) {
+    const sliceId = getUrlParam(URL_PARAMS.sliceId) || 0;
+    const {
+      labelColors,
+      sharedLabelColors,
+      colorScheme,
+      chartConfiguration,
+      nativeFilters,
+      filterBoxFilters,
+      dataMask,
+      dashboardId,
+    } = dashboardContext;
+    const dashboardContextWithFilters = getFormDataWithExtraFilters({
+      chart: { id: sliceId },
+      filters: getAppliedFilterValues(sliceId, filterBoxFilters),
+      nativeFilters,
+      chartConfiguration,
+      colorScheme,
+      dataMask,
+      labelColors,
+      sharedLabelColors,
+      sliceId,
+      allSliceIds: [sliceId],
+      extraControls: {},
+    });
+    Object.assign(dashboardContextWithFilters, { dashboardId });
+    return dashboardContextWithFilters;
+  }
+  return null;
+};
+
+export default function ExplorePage() {
+  const [isLoaded, setIsLoaded] = useState(false);
+  const isExploreInitialized = useRef(false);
+  const dispatch = useDispatch();
+  const location = useLocation();
+
+  useEffect(() => {
+    const exploreUrlParams = getParsedExploreURLParams(location);
+    const saveAction = getUrlParam(
+      URL_PARAMS.saveAction,
+    ) as SaveActionType | null;
+    const dashboardContextFormData = getDashboardContextFormData();
+    if (!isExploreInitialized.current || !!saveAction) {
+      fetchExploreData(exploreUrlParams)
+        .then(({ result }) => {
+          const formData = dashboardContextFormData
+            ? getFormDataWithDashboardContext(
+                result.form_data,
+                dashboardContextFormData,
+              )
+            : result.form_data;
+          dispatch(
+            hydrateExplore({
+              ...result,
+              form_data: formData,
+              saveAction,
+            }),
+          );
+        })
+        .catch(err => {
+          dispatch(hydrateExplore(fallbackExploreInitialData));
+          dispatch(addDangerToast(err.message));
+        })
+        .finally(() => {
+          setIsLoaded(true);
+          isExploreInitialized.current = true;
+        });
+    }
+    getSharedLabelColor().source = SharedLabelColorSource.explore;
+  }, [dispatch, location]);
+
+  if (!isLoaded) {
+    return <Loading />;
+  }
+  return <ExploreViewContainer />;
+}

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

@@ -30,6 +30,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';
 
@@ -117,6 +118,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

@@ -0,0 +1,212 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { hydrateExplore, HYDRATE_EXPLORE } from './hydrateExplore';
+import { exploreInitialData } from '../fixtures';
+
+test('creates hydrate action from initial data', () => {
+  const dispatch = jest.fn();
+  const getState = jest.fn(() => ({
+    user: {},
+    charts: {},
+    datasources: {},
+    common: {},
+    explore: {},
+  }));
+  // ignore type check - we dont need exact explore state for this test
+  // @ts-ignore
+  hydrateExplore(exploreInitialData)(dispatch, getState);
+  expect(dispatch).toHaveBeenCalledWith(
+    expect.objectContaining({
+      type: HYDRATE_EXPLORE,
+      data: {
+        charts: {
+          371: {
+            id: 371,
+            chartAlert: null,
+            chartStatus: null,
+            chartStackTrace: null,
+            chartUpdateEndTime: null,
+            chartUpdateStartTime: 0,
+            latestQueryFormData: {
+              cache_timeout: undefined,
+              datasource: '8__table',
+              slice_id: 371,
+              url_params: undefined,
+              viz_type: 'table',
+            },
+            sliceFormData: {
+              cache_timeout: undefined,
+              datasource: '8__table',
+              slice_id: 371,
+              url_params: undefined,
+              viz_type: 'table',
+            },
+            queryController: null,
+            queriesResponse: null,
+            triggerQuery: false,
+            lastRendered: 0,
+          },
+        },
+        datasources: {
+          '8__table': exploreInitialData.dataset,
+        },
+        saveModal: {
+          dashboards: [],
+          saveModalAlert: null,
+          isVisible: false,
+        },
+        explore: {
+          can_add: false,
+          can_download: false,
+          can_overwrite: false,
+          isDatasourceMetaLoading: false,
+          isStarred: false,
+          triggerRender: false,
+          datasource: exploreInitialData.dataset,
+          controls: expect.any(Object),
+          form_data: exploreInitialData.form_data,
+          slice: exploreInitialData.slice,
+          standalone: null,
+          force: null,
+          saveAction: null,
+        },
+      },
+    }),
+  );
+});
+
+test('creates hydrate action with existing state', () => {
+  const dispatch = jest.fn();
+  const getState = jest.fn(() => ({
+    user: {},
+    charts: {},
+    datasources: {},
+    common: {},
+    explore: { controlsTransferred: ['all_columns'] },
+  }));
+  // ignore type check - we dont need exact explore state for this test
+  // @ts-ignore
+  hydrateExplore(exploreInitialData)(dispatch, getState);
+  expect(dispatch).toHaveBeenCalledWith(
+    expect.objectContaining({
+      type: HYDRATE_EXPLORE,
+      data: {
+        charts: {
+          371: {
+            id: 371,
+            chartAlert: null,
+            chartStatus: null,
+            chartStackTrace: null,
+            chartUpdateEndTime: null,
+            chartUpdateStartTime: 0,
+            latestQueryFormData: {
+              cache_timeout: undefined,
+              datasource: '8__table',
+              slice_id: 371,
+              url_params: undefined,
+              viz_type: 'table',
+            },
+            sliceFormData: {
+              cache_timeout: undefined,
+              datasource: '8__table',
+              slice_id: 371,
+              url_params: undefined,
+              viz_type: 'table',
+            },
+            queryController: null,
+            queriesResponse: null,
+            triggerQuery: false,
+            lastRendered: 0,
+          },
+        },
+        datasources: {
+          '8__table': exploreInitialData.dataset,
+        },
+        saveModal: {
+          dashboards: [],
+          saveModalAlert: null,
+          isVisible: false,
+        },
+        explore: {
+          can_add: false,
+          can_download: false,
+          can_overwrite: false,
+          isDatasourceMetaLoading: false,
+          isStarred: false,
+          triggerRender: false,
+          datasource: exploreInitialData.dataset,
+          controls: expect.any(Object),
+          controlsTransferred: ['all_columns'],
+          form_data: exploreInitialData.form_data,
+          slice: exploreInitialData.slice,
+          standalone: null,
+          force: null,
+          saveAction: null,
+        },
+      },
+    }),
+  );
+});
+
+test('uses configured default time range if not set', () => {
+  const dispatch = jest.fn();
+  const getState = jest.fn(() => ({
+    user: {},
+    charts: {},
+    datasources: {},
+    common: {
+      conf: {
+        DEFAULT_TIME_FILTER: 'Last year',
+      },
+    },
+    explore: {},
+  }));
+  // @ts-ignore
+  hydrateExplore({ form_data: {}, slice: {}, dataset: {} })(dispatch, getState);
+  expect(dispatch).toHaveBeenCalledWith(
+    expect.objectContaining({
+      data: expect.objectContaining({
+        explore: expect.objectContaining({
+          form_data: expect.objectContaining({
+            time_range: 'Last year',
+          }),
+        }),
+      }),
+    }),
+  );
+  const withTimeRangeSet = {
+    form_data: { time_range: 'Last day' },
+    slice: {},
+    dataset: {},
+  };
+  // @ts-ignore
+  hydrateExplore(withTimeRangeSet)(dispatch, getState);
+  expect(dispatch).toHaveBeenCalledWith(
+    expect.objectContaining({
+      data: expect.objectContaining({
+        explore: expect.objectContaining({
+          form_data: expect.objectContaining({
+            time_range: 'Last day',
+          }),
+        }),
+      }),
+    }),
+  );
+});