fix(chart): chart updates are not retained (#23627)

superset-frontend/cypress-base/cypress/e2e/explore/utils.ts

@@ -44,7 +44,10 @@ export function interceptExploreJson() {
 }
 
 export function interceptExploreGet() {
-  cy.intercept('GET', `/api/v1/explore/?slice_id=**`).as('getExplore');
+  cy.intercept({
+    method: 'GET',
+    url: /api\/v1\/explore\/\?(form_data_key|dashboard_page_id|slice_id)=.*/,
+  }).as('getExplore');
 }
 
 export function setFilter(filter: string, option: string) {

superset-frontend/spec/fixtures/mockDashboardFormData.ts

@@ -0,0 +1,79 @@
+/**
+ * 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.
+ */
+/* eslint-disable theme-colors/no-literal-colors */
+import { JsonObject } from '@superset-ui/core';
+
+export const getDashboardFormData = (overrides: JsonObject = {}) => ({
+  label_colors: {
+    Girls: '#FF69B4',
+    Boys: '#ADD8E6',
+    girl: '#FF69B4',
+    boy: '#ADD8E6',
+  },
+  shared_label_colors: {
+    boy: '#ADD8E6',
+    girl: '#FF69B4',
+  },
+  color_scheme: 'd3Category20b',
+  extra_filters: [
+    {
+      col: '__time_range',
+      op: '==',
+      val: 'No filter',
+    },
+    {
+      col: '__time_grain',
+      op: '==',
+      val: 'P1D',
+    },
+    {
+      col: '__time_col',
+      op: '==',
+      val: 'ds',
+    },
+  ],
+  extra_form_data: {
+    filters: [
+      {
+        col: 'name',
+        op: 'IN',
+        val: ['Aaron'],
+      },
+      {
+        col: 'num_boys',
+        op: '<=',
+        val: 10000,
+      },
+      {
+        col: {
+          sqlExpression: 'totally viable sql expression',
+          expressionType: 'SQL',
+          label: 'My column',
+        },
+        op: 'IN',
+        val: ['Value1', 'Value2'],
+      },
+    ],
+    granularity_sqla: 'ds',
+    time_range: 'Last month',
+    time_grain_sqla: 'PT1S',
+  },
+  dashboardId: 2,
+  ...overrides,
+});

superset-frontend/spec/fixtures/mockExploreFormData.ts

@@ -0,0 +1,87 @@
+/**
+ * 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 { JsonObject } from '@superset-ui/core';
+
+export const getExploreFormData = (overrides: JsonObject = {}) => ({
+  adhoc_filters: [
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SIMPLE' as const,
+      operator: 'IN' as const,
+      subject: 'gender',
+      comparator: ['boys'],
+      filterOptionName: '123',
+    },
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "name = 'John'",
+      filterOptionName: '456',
+    },
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "city = 'Warsaw'",
+      filterOptionName: '567',
+    },
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SIMPLE' as const,
+      operator: 'TEMPORAL_RANGE' as const,
+      subject: 'ds',
+      comparator: 'No filter',
+      filterOptionName: '678',
+    },
+  ],
+  adhoc_filters_b: [
+    {
+      clause: 'WHERE' as const,
+      expressionType: 'SQL' as const,
+      operator: null,
+      subject: null,
+      comparator: null,
+      sqlExpression: "country = 'Poland'",
+      filterOptionName: '789',
+    },
+  ],
+  applied_time_extras: {},
+  color_scheme: 'supersetColors',
+  datasource: '2__table',
+  granularity_sqla: 'ds',
+  groupby: ['gender'],
+  metric: {
+    aggregate: 'SUM',
+    column: {
+      column_name: 'num',
+      type: 'BIGINT',
+    },
+    expressionType: 'SIMPLE',
+    label: 'Births',
+  },
+  slice_id: 46,
+  time_range: '100 years ago : now',
+  viz_type: 'pie',
+  ...overrides,
+});

superset-frontend/src/explore/components/SaveModal.tsx

@@ -266,7 +266,9 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
 
       const searchParams = new URLSearchParams(window.location.search);
       searchParams.set('save_action', this.state.action);
-      searchParams.delete('form_data_key');
+      if (this.state.action !== 'overwrite') {
+        searchParams.delete('form_data_key');
+      }
       if (this.state.action === 'saveas') {
         searchParams.set('slice_id', value.id.toString());
       }

superset-frontend/src/explore/controlUtils/getFormDataFromDashboardContext.test.ts

@@ -18,135 +18,10 @@
  */
 
 import { JsonObject } from '@superset-ui/core';
+import { getExploreFormData } from 'spec/fixtures/mockExploreFormData';
+import { getDashboardFormData } from 'spec/fixtures/mockDashboardFormData';
 import { getFormDataWithDashboardContext } from './getFormDataWithDashboardContext';
 
-const getExploreFormData = (overrides: JsonObject = {}) => ({
-  adhoc_filters: [
-    {
-      clause: 'WHERE' as const,
-      expressionType: 'SIMPLE' as const,
-      operator: 'IN' as const,
-      subject: 'gender',
-      comparator: ['boys'],
-      filterOptionName: '123',
-    },
-    {
-      clause: 'WHERE' as const,
-      expressionType: 'SQL' as const,
-      operator: null,
-      subject: null,
-      comparator: null,
-      sqlExpression: "name = 'John'",
-      filterOptionName: '456',
-    },
-    {
-      clause: 'WHERE' as const,
-      expressionType: 'SQL' as const,
-      operator: null,
-      subject: null,
-      comparator: null,
-      sqlExpression: "city = 'Warsaw'",
-      filterOptionName: '567',
-    },
-    {
-      clause: 'WHERE' as const,
-      expressionType: 'SIMPLE' as const,
-      operator: 'TEMPORAL_RANGE' as const,
-      subject: 'ds',
-      comparator: 'No filter',
-      filterOptionName: '678',
-    },
-  ],
-  adhoc_filters_b: [
-    {
-      clause: 'WHERE' as const,
-      expressionType: 'SQL' as const,
-      operator: null,
-      subject: null,
-      comparator: null,
-      sqlExpression: "country = 'Poland'",
-      filterOptionName: '789',
-    },
-  ],
-  applied_time_extras: {},
-  color_scheme: 'supersetColors',
-  datasource: '2__table',
-  granularity_sqla: 'ds',
-  groupby: ['gender'],
-  metric: {
-    aggregate: 'SUM',
-    column: {
-      column_name: 'num',
-      type: 'BIGINT',
-    },
-    expressionType: 'SIMPLE',
-    label: 'Births',
-  },
-  slice_id: 46,
-  time_range: '100 years ago : now',
-  viz_type: 'pie',
-  ...overrides,
-});
-
-const getDashboardFormData = (overrides: JsonObject = {}) => ({
-  label_colors: {
-    Girls: '#FF69B4',
-    Boys: '#ADD8E6',
-    girl: '#FF69B4',
-    boy: '#ADD8E6',
-  },
-  shared_label_colors: {
-    boy: '#ADD8E6',
-    girl: '#FF69B4',
-  },
-  color_scheme: 'd3Category20b',
-  extra_filters: [
-    {
-      col: '__time_range',
-      op: '==',
-      val: 'No filter',
-    },
-    {
-      col: '__time_grain',
-      op: '==',
-      val: 'P1D',
-    },
-    {
-      col: '__time_col',
-      op: '==',
-      val: 'ds',
-    },
-  ],
-  extra_form_data: {
-    filters: [
-      {
-        col: 'name',
-        op: 'IN',
-        val: ['Aaron'],
-      },
-      {
-        col: 'num_boys',
-        op: '<=',
-        val: 10000,
-      },
-      {
-        col: {
-          sqlExpression: 'totally viable sql expression',
-          expressionType: 'SQL',
-          label: 'My column',
-        },
-        op: 'IN',
-        val: ['Value1', 'Value2'],
-      },
-    ],
-    granularity_sqla: 'ds',
-    time_range: 'Last month',
-    time_grain_sqla: 'PT1S',
-  },
-  dashboardId: 2,
-  ...overrides,
-});
-
 const getExpectedResultFormData = (overrides: JsonObject = {}) => ({
   adhoc_filters: [
     {

superset-frontend/src/pages/Chart/Chart.test.tsx

@@ -0,0 +1,189 @@
+/**
+ * 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 from 'react';
+import fetchMock from 'fetch-mock';
+import { Link } from 'react-router-dom';
+import {
+  render,
+  waitFor,
+  screen,
+  fireEvent,
+} from 'spec/helpers/testing-library';
+import { getExploreFormData } from 'spec/fixtures/mockExploreFormData';
+import { getDashboardFormData } from 'spec/fixtures/mockDashboardFormData';
+import { LocalStorageKeys } from 'src/utils/localStorageHelpers';
+import getFormDataWithExtraFilters from 'src/dashboard/util/charts/getFormDataWithExtraFilters';
+import { URL_PARAMS } from 'src/constants';
+import { JsonObject } from '@superset-ui/core';
+
+import ChartPage from '.';
+
+jest.mock('re-resizable', () => ({
+  Resizable: () => <div data-test="mock-re-resizable" />,
+}));
+jest.mock(
+  'src/explore/components/ExploreChartPanel',
+  () =>
+    ({ exploreState }: { exploreState: JsonObject }) =>
+      (
+        <div data-test="mock-explore-chart-panel">
+          {JSON.stringify(exploreState)}
+        </div>
+      ),
+);
+jest.mock('src/dashboard/util/charts/getFormDataWithExtraFilters');
+
+describe('ChartPage', () => {
+  afterEach(() => {
+    fetchMock.reset();
+  });
+
+  test('fetches metadata on mount', async () => {
+    const exploreApiRoute = 'glob:*/api/v1/explore/*';
+    const exploreFormData = getExploreFormData({
+      viz_type: 'table',
+      show_cell_bars: true,
+    });
+    fetchMock.get(exploreApiRoute, {
+      result: { dataset: { id: 1 }, form_data: exploreFormData },
+    });
+    const { getByTestId } = render(<ChartPage />, {
+      useRouter: true,
+      useRedux: true,
+    });
+    await waitFor(() =>
+      expect(fetchMock.calls(exploreApiRoute).length).toBe(1),
+    );
+    expect(getByTestId('mock-explore-chart-panel')).toBeInTheDocument();
+    expect(getByTestId('mock-explore-chart-panel')).toHaveTextContent(
+      JSON.stringify({ show_cell_bars: true }).slice(1, -1),
+    );
+  });
+
+  describe('with dashboardContextFormData', () => {
+    const dashboardPageId = 'mockPageId';
+
+    beforeEach(() => {
+      localStorage.setItem(
+        LocalStorageKeys.dashboard__explore_context,
+        JSON.stringify({
+          [dashboardPageId]: {},
+        }),
+      );
+    });
+
+    afterEach(() => {
+      localStorage.clear();
+      (getFormDataWithExtraFilters as jest.Mock).mockClear();
+    });
+
+    test('overrides the form_data with dashboardContextFormData', async () => {
+      const dashboardFormData = getDashboardFormData();
+      (getFormDataWithExtraFilters as jest.Mock).mockReturnValue(
+        dashboardFormData,
+      );
+      const exploreApiRoute = 'glob:*/api/v1/explore/*';
+      const exploreFormData = getExploreFormData();
+      fetchMock.get(exploreApiRoute, {
+        result: { dataset: { id: 1 }, form_data: exploreFormData },
+      });
+      window.history.pushState(
+        {},
+        '',
+        `/?${URL_PARAMS.dashboardPageId.name}=${dashboardPageId}`,
+      );
+      const { getByTestId } = render(<ChartPage />, {
+        useRouter: true,
+        useRedux: true,
+      });
+      await waitFor(() =>
+        expect(fetchMock.calls(exploreApiRoute).length).toBe(1),
+      );
+      expect(getByTestId('mock-explore-chart-panel')).toHaveTextContent(
+        JSON.stringify({ color_scheme: dashboardFormData.color_scheme }).slice(
+          1,
+          -1,
+        ),
+      );
+    });
+
+    test('overrides the form_data with exploreFormData when location is updated', async () => {
+      const dashboardFormData = {
+        ...getDashboardFormData(),
+        viz_type: 'table',
+        show_cell_bars: true,
+      };
+      (getFormDataWithExtraFilters as jest.Mock).mockReturnValue(
+        dashboardFormData,
+      );
+      const exploreApiRoute = 'glob:*/api/v1/explore/*';
+      const exploreFormData = getExploreFormData({
+        viz_type: 'table',
+        show_cell_bars: true,
+      });
+      fetchMock.get(exploreApiRoute, {
+        result: { dataset: { id: 1 }, form_data: exploreFormData },
+      });
+      window.history.pushState(
+        {},
+        '',
+        `/?${URL_PARAMS.dashboardPageId.name}=${dashboardPageId}`,
+      );
+      const { getByTestId } = render(
+        <>
+          <Link
+            to={`/?${URL_PARAMS.dashboardPageId.name}=${dashboardPageId}&${URL_PARAMS.saveAction.name}=overwrite`}
+          >
+            Change route
+          </Link>
+          <ChartPage />
+        </>,
+        {
+          useRouter: true,
+          useRedux: true,
+        },
+      );
+      await waitFor(() =>
+        expect(fetchMock.calls(exploreApiRoute).length).toBe(1),
+      );
+      expect(getByTestId('mock-explore-chart-panel')).toHaveTextContent(
+        JSON.stringify({
+          show_cell_bars: dashboardFormData.show_cell_bars,
+        }).slice(1, -1),
+      );
+      const updatedExploreFormData = {
+        ...exploreFormData,
+        show_cell_bars: false,
+      };
+      fetchMock.reset();
+      fetchMock.get(exploreApiRoute, {
+        result: { dataset: { id: 1 }, form_data: updatedExploreFormData },
+      });
+      fireEvent.click(screen.getByText('Change route'));
+      await waitFor(() =>
+        expect(fetchMock.calls(exploreApiRoute).length).toBe(1),
+      );
+      expect(getByTestId('mock-explore-chart-panel')).toHaveTextContent(
+        JSON.stringify({
+          show_cell_bars: updatedExploreFormData.show_cell_bars,
+        }).slice(1, -1),
+      );
+    });
+  });
+});

superset-frontend/src/pages/Chart/index.tsx

@@ -129,12 +129,13 @@ export default function ExplorePage() {
     if (!isExploreInitialized.current || !!saveAction) {
       fetchExploreData(exploreUrlParams)
         .then(({ result }) => {
-          const formData = dashboardContextFormData
-            ? getFormDataWithDashboardContext(
-                result.form_data,
-                dashboardContextFormData,
-              )
-            : result.form_data;
+          const formData =
+            !isExploreInitialized.current && dashboardContextFormData
+              ? getFormDataWithDashboardContext(
+                  result.form_data,
+                  dashboardContextFormData,
+                )
+              : result.form_data;
           dispatch(
             hydrateExplore({
               ...result,