Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(explore): Replace url search params only if current page is Explore #20972

Merged
merged 2 commits into from
Aug 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}>
Expand Down Expand Up @@ -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(),
Expand All @@ -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));
Expand All @@ -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();
});
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down