From 818a799cc241a835c53e7d705ab366c6177f6a8f Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Wed, 12 Oct 2022 19:52:36 -0300 Subject: [PATCH] fix: Preserve unknown URL params (#21785) (cherry picked from commit 11d7d6e078b75079c432d8d8028dac45678b2c37) --- .../ExploreViewContainer.test.tsx | 32 +++++++++++++------ .../components/ExploreViewContainer/index.jsx | 4 ++- .../getParsedExploreURLParams.test.ts | 2 +- .../exploreUtils/getParsedExploreURLParams.ts | 5 +-- superset/models/slice.py | 3 +- 5 files changed, 31 insertions(+), 15 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 4fc78594a2970..e382149ee7322 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -67,7 +67,8 @@ const reduxState = { }, }; -const key = 'aWrs7w29sd'; +const KEY = 'aWrs7w29sd'; +const SEARCH = `?form_data_key=${KEY}&dataset_id=1`; jest.mock('react-resize-detector', () => ({ __esModule: true, @@ -79,21 +80,20 @@ jest.mock('lodash/debounce', () => ({ default: (fuc: Function) => fuc, })); -fetchMock.post('glob:*/api/v1/explore/form_data*', { key }); -fetchMock.put('glob:*/api/v1/explore/form_data*', { key }); +fetchMock.post('glob:*/api/v1/explore/form_data*', { key: KEY }); +fetchMock.put('glob:*/api/v1/explore/form_data*', { key: KEY }); fetchMock.get('glob:*/api/v1/explore/form_data*', {}); fetchMock.get('glob:*/favstar/slice*', { count: 0 }); const defaultPath = '/explore/'; const renderWithRouter = ({ - withKey, + search = '', overridePathname, }: { - withKey?: boolean; + search?: string; 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 }; @@ -135,12 +135,12 @@ 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({ withKey: true })); + await waitFor(() => renderWithRouter({ search: SEARCH })); expect(replaceState).not.toHaveBeenLastCalledWith( 0, expect.anything(), undefined, - expect.stringMatching(key), + expect.stringMatching(KEY), ); expect(replaceState).toHaveBeenCalledWith( expect.anything(), @@ -156,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({ withKey: true })); + await waitFor(() => renderWithRouter({ search: SEARCH })); expect(replaceState.mock.calls.length).toBe(1); userEvent.click(screen.getByText('Update chart')); await waitFor(() => expect(pushState.mock.calls.length).toBe(1)); @@ -180,3 +180,17 @@ test('doesnt call replaceState when pathname is not /explore', async () => { expect(replaceState).not.toHaveBeenCalled(); replaceState.mockRestore(); }); + +test('preserves unknown parameters', async () => { + const replaceState = jest.spyOn(window.history, 'replaceState'); + const unknownParam = 'test=123'; + await waitFor(() => + renderWithRouter({ search: `${SEARCH}&${unknownParam}` }), + ); + expect(replaceState).toHaveBeenCalledWith( + expect.anything(), + undefined, + expect.stringMatching(unknownParam), + ); + replaceState.mockRestore(); +}); diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index 7f6a870039bfb..831a37a2f74c9 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -167,7 +167,9 @@ const updateHistory = debounce( ) => { const payload = { ...formData }; const chartId = formData.slice_id; - const additionalParam = {}; + const params = new URLSearchParams(window.location.search); + const additionalParam = Object.fromEntries(params); + if (chartId) { additionalParam[URL_PARAMS.sliceId.name] = chartId; } else { diff --git a/superset-frontend/src/explore/exploreUtils/getParsedExploreURLParams.test.ts b/superset-frontend/src/explore/exploreUtils/getParsedExploreURLParams.test.ts index 5039d3421fb19..58a10c21c151a 100644 --- a/superset-frontend/src/explore/exploreUtils/getParsedExploreURLParams.test.ts +++ b/superset-frontend/src/explore/exploreUtils/getParsedExploreURLParams.test.ts @@ -31,7 +31,7 @@ test('get form_data_key and slice_id from search params - url when moving from d `${EXPLORE_BASE_URL}?form_data_key=yrLXmyE9fmhQ11lM1KgaD1PoPSBpuLZIJfqdyIdw9GoBwhPFRZHeIgeFiNZljbpd&slice_id=56`, ); expect(getParsedExploreURLParams().toString()).toEqual( - 'slice_id=56&form_data_key=yrLXmyE9fmhQ11lM1KgaD1PoPSBpuLZIJfqdyIdw9GoBwhPFRZHeIgeFiNZljbpd', + 'form_data_key=yrLXmyE9fmhQ11lM1KgaD1PoPSBpuLZIJfqdyIdw9GoBwhPFRZHeIgeFiNZljbpd&slice_id=56', ); }); diff --git a/superset-frontend/src/explore/exploreUtils/getParsedExploreURLParams.ts b/superset-frontend/src/explore/exploreUtils/getParsedExploreURLParams.ts index 1e5007875a189..42021381bb7e0 100644 --- a/superset-frontend/src/explore/exploreUtils/getParsedExploreURLParams.ts +++ b/superset-frontend/src/explore/exploreUtils/getParsedExploreURLParams.ts @@ -77,7 +77,7 @@ const EXPLORE_URL_PATH_PARAMS = { // we need to "flatten" the search params to use them with /v1/explore endpoint const getParsedExploreURLSearchParams = (search: string) => { const urlSearchParams = new URLSearchParams(search); - return Object.keys(EXPLORE_URL_SEARCH_PARAMS).reduce((acc, currentParam) => { + return Array.from(urlSearchParams.keys()).reduce((acc, currentParam) => { const paramValue = urlSearchParams.get(currentParam); if (paramValue === null) { return acc; @@ -93,9 +93,10 @@ const getParsedExploreURLSearchParams = (search: string) => { if (typeof parsedParamValue === 'object') { return { ...acc, ...parsedParamValue }; } + const key = EXPLORE_URL_SEARCH_PARAMS[currentParam]?.name || currentParam; return { ...acc, - [EXPLORE_URL_SEARCH_PARAMS[currentParam].name]: parsedParamValue, + [key]: parsedParamValue, }; }, {}); }; diff --git a/superset/models/slice.py b/superset/models/slice.py index de0f3df59684f..244d97aee9e4c 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -335,8 +335,7 @@ def icons(self) -> str: @property def url(self) -> str: - form_data = f"%7B%22slice_id%22%3A%20{self.id}%7D" - return f"/explore/?slice_id={self.id}&form_data={form_data}" + return f"/explore/?slice_id={self.id}" def get_query_context_factory(self) -> QueryContextFactory: if self.query_context_factory is None: