fix(explore): Replace url search params only if current page is Explo…

…re (#20972) * fix(explore): Replace url search params only if current page is Explore * Add unit test
apache · Aug 5, 2022 · 9350bba · 9350bba
1 parent 499a28f
commit 9350bba
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 14 deletions.
diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.jsx
@@ -153,7 +153,7 @@ const ExploreChartPanel = ({
   const [showDatasetModal, setShowDatasetModal] = useState(false);
 
   const metaDataRegistry = getChartMetadataRegistry();
-  const { useLegacyApi } = metaDataRegistry.get(vizType);
+  const { useLegacyApi } = metaDataRegistry.get(vizType) ?? {};
   const vizTypeNeedsDataset =
     useLegacyApi && datasource.type !== DatasourceType.Table;
   // added boolean column to below show boolean so that the errors aren't overlapping

diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
@@ -84,9 +84,21 @@ fetchMock.put('glob:*/api/v1/explore/form_data*', { key });
 fetchMock.get('glob:*/api/v1/explore/form_data*', {});
 fetchMock.get('glob:*/favstar/slice*', { count: 0 });
 
-const renderWithRouter = (withKey?: boolean) => {
-  const path = '/explore/';
+const defaultPath = '/explore/';
+const renderWithRouter = ({
+  withKey,
+  overridePathname,
+}: {
+  withKey?: boolean;
+  overridePathname?: string;
+} = {}) => {
+  const path = overridePathname ?? defaultPath;
   const search = withKey ? `?form_data_key=${key}&dataset_id=1` : '';
+  Object.defineProperty(window, 'location', {
+    get() {
+      return { pathname: path, search };
+    },
+  });
   return render(
     <MemoryRouter initialEntries={[`${path}${search}`]}>
       <Route path={path}>
@@ -123,7 +135,7 @@ test('generates a new form_data param when none is available', async () => {
 
 test('generates a different form_data param when one is provided and is mounting', async () => {
   const replaceState = jest.spyOn(window.history, 'replaceState');
-  await waitFor(() => renderWithRouter(true));
+  await waitFor(() => renderWithRouter({ withKey: true }));
   expect(replaceState).not.toHaveBeenLastCalledWith(
     0,
     expect.anything(),
@@ -144,7 +156,7 @@ test('reuses the same form_data param when updating', async () => {
   });
   const replaceState = jest.spyOn(window.history, 'replaceState');
   const pushState = jest.spyOn(window.history, 'pushState');
-  await waitFor(() => renderWithRouter());
+  await waitFor(() => renderWithRouter({ withKey: true }));
   expect(replaceState.mock.calls.length).toBe(1);
   userEvent.click(screen.getByText('Update chart'));
   await waitFor(() => expect(pushState.mock.calls.length).toBe(1));
@@ -153,3 +165,18 @@ test('reuses the same form_data param when updating', async () => {
   pushState.mockRestore();
   getChartControlPanelRegistry().remove('table');
 });
+
+test('doesnt call replaceState when pathname is not /explore', async () => {
+  getChartMetadataRegistry().registerValue(
+    'table',
+    new ChartMetadata({
+      name: 'fake table',
+      thumbnail: '.png',
+      useLegacyApi: false,
+    }),
+  );
+  const replaceState = jest.spyOn(window.history, 'replaceState');
+  await waitFor(() => renderWithRouter({ overridePathname: '/dashboard' }));
+  expect(replaceState).not.toHaveBeenCalled();
+  replaceState.mockRestore();
+});
diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
@@ -206,15 +206,18 @@ const updateHistory = debounce(
         );
         stateModifier = 'pushState';
       }
-      const url = mountExploreUrl(
-        standalone ? URL_PARAMS.standalone.name : null,
-        {
-          [URL_PARAMS.formDataKey.name]: key,
-          ...additionalParam,
-        },
-        force,
-      );
-      window.history[stateModifier](payload, title, url);
+      // avoid race condition in case user changes route before explore updates the url
+      if (window.location.pathname.startsWith('/explore')) {
+        const url = mountExploreUrl(
+          standalone ? URL_PARAMS.standalone.name : null,
+          {
+            [URL_PARAMS.formDataKey.name]: key,
+            ...additionalParam,
+          },
+          force,
+        );
+        window.history[stateModifier](payload, title, url);
+      }
     } catch (e) {
       logging.warn('Failed at altering browser history', e);
     }