From 779f7db1abe9334a4af956624f21c14c83145a99 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Wed, 27 Jul 2022 17:05:42 +0200 Subject: [PATCH 1/6] feat: Use SPA navigation from datasets list to Explore --- .../GenericLink/GenericLink.test.tsx | 48 +++++++++++++++++++ .../components/GenericLink/GenericLink.tsx | 32 +++++++++++++ .../views/CRUD/data/dataset/DatasetList.tsx | 7 ++- 3 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 superset-frontend/src/components/GenericLink/GenericLink.test.tsx create mode 100644 superset-frontend/src/components/GenericLink/GenericLink.tsx diff --git a/superset-frontend/src/components/GenericLink/GenericLink.test.tsx b/superset-frontend/src/components/GenericLink/GenericLink.test.tsx new file mode 100644 index 0000000000000..7857fc2df7694 --- /dev/null +++ b/superset-frontend/src/components/GenericLink/GenericLink.test.tsx @@ -0,0 +1,48 @@ +/** + * 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 { render, screen } from 'spec/helpers/testing-library'; +import { GenericLink } from './GenericLink'; + +test('renders', () => { + render(Link to Explore, { + useRouter: true, + }); + expect(screen.getByText('Link to Explore')).toBeVisible(); +}); + +test('navigates to internal URL', () => { + render(Link to Explore, { + useRouter: true, + }); + const internalLink = screen.getByTestId('internal-link'); + expect(internalLink).toHaveAttribute('href', '/explore'); +}); + +test('navigates to external URL', () => { + render( + + Link to external website + , + { useRouter: true }, + ); + const externalLink = screen.getByTestId('external-link'); + expect(externalLink).toHaveAttribute('href', 'https://superset.apache.org/'); +}); diff --git a/superset-frontend/src/components/GenericLink/GenericLink.tsx b/superset-frontend/src/components/GenericLink/GenericLink.tsx new file mode 100644 index 0000000000000..a241c985bb327 --- /dev/null +++ b/superset-frontend/src/components/GenericLink/GenericLink.tsx @@ -0,0 +1,32 @@ +import React from 'react'; +import { Link, LinkProps } from 'react-router-dom'; + +export const GenericLink = ({ + to, + component, + replace, + innerRef, + children, + ...rest +}: // css prop type check was failing, override with any +LinkProps & { css?: any }) => { + if (typeof to === 'string' && /^https?:\/\//.test(to)) { + return ( + + {children} + + ); + } + return ( + + {children} + + ); +}; diff --git a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx index c5b182871f174..107b072f25781 100644 --- a/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx +++ b/superset-frontend/src/views/CRUD/data/dataset/DatasetList.tsx @@ -60,6 +60,7 @@ import ImportModelsModal from 'src/components/ImportModal/index'; import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import WarningIconWithTooltip from 'src/components/WarningIconWithTooltip'; import { isUserAdmin } from 'src/dashboard/util/permissionUtils'; +import { GenericLink } from 'src/components/GenericLink/GenericLink'; import AddDatasetModal from './AddDatasetModal'; import { @@ -289,7 +290,11 @@ const DatasetList: FunctionComponent = ({ }, }, }: any) => { - const titleLink = {datasetTitle}; + const titleLink = ( + // exploreUrl can be a link to Explore or an external link + // in the first case use SPA routing, else use HTML anchor + {datasetTitle} + ); try { const parsedExtra = JSON.parse(extra); return ( From cbabb4f29bbd04e9e50c23b60911ddfad6420988 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Wed, 27 Jul 2022 17:22:48 +0200 Subject: [PATCH 2/6] fix lint --- .../src/components/GenericLink/GenericLink.test.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/components/GenericLink/GenericLink.test.tsx b/superset-frontend/src/components/GenericLink/GenericLink.test.tsx index 7857fc2df7694..6212f1cc49a5b 100644 --- a/superset-frontend/src/components/GenericLink/GenericLink.test.tsx +++ b/superset-frontend/src/components/GenericLink/GenericLink.test.tsx @@ -22,14 +22,14 @@ import { render, screen } from 'spec/helpers/testing-library'; import { GenericLink } from './GenericLink'; test('renders', () => { - render(Link to Explore, { + render(Link to Explore, { useRouter: true, }); expect(screen.getByText('Link to Explore')).toBeVisible(); }); test('navigates to internal URL', () => { - render(Link to Explore, { + render(Link to Explore, { useRouter: true, }); const internalLink = screen.getByTestId('internal-link'); @@ -38,7 +38,7 @@ test('navigates to internal URL', () => { test('navigates to external URL', () => { render( - + Link to external website , { useRouter: true }, From 15051b16eaba3fce530e755a422d468c397f6ee8 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Wed, 27 Jul 2022 18:42:56 +0200 Subject: [PATCH 3/6] Fix lint --- .../components/GenericLink/GenericLink.tsx | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/superset-frontend/src/components/GenericLink/GenericLink.tsx b/superset-frontend/src/components/GenericLink/GenericLink.tsx index a241c985bb327..bc3e1a0125caf 100644 --- a/superset-frontend/src/components/GenericLink/GenericLink.tsx +++ b/superset-frontend/src/components/GenericLink/GenericLink.tsx @@ -1,3 +1,22 @@ +/** + * 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 { Link, LinkProps } from 'react-router-dom'; From 8581f26a3c6bac37aac9ffe4b632ed61f1b912e5 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Thu, 28 Jul 2022 15:58:54 +0200 Subject: [PATCH 4/6] Fix type --- .../src/components/GenericLink/GenericLink.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/components/GenericLink/GenericLink.tsx b/superset-frontend/src/components/GenericLink/GenericLink.tsx index bc3e1a0125caf..8a27ec526fa45 100644 --- a/superset-frontend/src/components/GenericLink/GenericLink.tsx +++ b/superset-frontend/src/components/GenericLink/GenericLink.tsx @@ -20,15 +20,15 @@ import React from 'react'; import { Link, LinkProps } from 'react-router-dom'; -export const GenericLink = ({ +export const GenericLink = ({ to, component, replace, innerRef, children, ...rest -}: // css prop type check was failing, override with any -LinkProps & { css?: any }) => { +}: React.PropsWithoutRef> & + React.RefAttributes) => { if (typeof to === 'string' && /^https?:\/\//.test(to)) { return ( From ab260129e54f3d7ae335f5951574c10cc9f6f546 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Fri, 29 Jul 2022 14:21:26 +0200 Subject: [PATCH 5/6] Make external links work without protocols --- .../GenericLink/GenericLink.test.tsx | 11 ++++ .../components/GenericLink/GenericLink.tsx | 5 +- superset-frontend/src/utils/urlUtils.test.ts | 54 +++++++++++++++++++ superset-frontend/src/utils/urlUtils.ts | 30 ++++++++++- 4 files changed, 97 insertions(+), 3 deletions(-) create mode 100644 superset-frontend/src/utils/urlUtils.test.ts diff --git a/superset-frontend/src/components/GenericLink/GenericLink.test.tsx b/superset-frontend/src/components/GenericLink/GenericLink.test.tsx index 6212f1cc49a5b..c8f2ba5f5f41e 100644 --- a/superset-frontend/src/components/GenericLink/GenericLink.test.tsx +++ b/superset-frontend/src/components/GenericLink/GenericLink.test.tsx @@ -46,3 +46,14 @@ test('navigates to external URL', () => { const externalLink = screen.getByTestId('external-link'); expect(externalLink).toHaveAttribute('href', 'https://superset.apache.org/'); }); + +test('navigates to external URL without host', () => { + render( + + Link to external website + , + { useRouter: true }, + ); + const externalLink = screen.getByTestId('external-link'); + expect(externalLink).toHaveAttribute('href', '//superset.apache.org/'); +}); diff --git a/superset-frontend/src/components/GenericLink/GenericLink.tsx b/superset-frontend/src/components/GenericLink/GenericLink.tsx index 8a27ec526fa45..2bc111d1b60f4 100644 --- a/superset-frontend/src/components/GenericLink/GenericLink.tsx +++ b/superset-frontend/src/components/GenericLink/GenericLink.tsx @@ -19,6 +19,7 @@ import React from 'react'; import { Link, LinkProps } from 'react-router-dom'; +import { isUrlExternal, parseUrl } from 'src/utils/urlUtils'; export const GenericLink = ({ to, @@ -29,9 +30,9 @@ export const GenericLink = ({ ...rest }: React.PropsWithoutRef> & React.RefAttributes) => { - if (typeof to === 'string' && /^https?:\/\//.test(to)) { + if (typeof to === 'string' && isUrlExternal(to)) { return ( - + {children} ); diff --git a/superset-frontend/src/utils/urlUtils.test.ts b/superset-frontend/src/utils/urlUtils.test.ts new file mode 100644 index 0000000000000..5f413ac59034b --- /dev/null +++ b/superset-frontend/src/utils/urlUtils.test.ts @@ -0,0 +1,54 @@ +/** + * 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 { isUrlExternal, parseUrl } from './urlUtils'; + +test('isUrlExternal', () => { + expect(isUrlExternal('http://google.com')).toBeTruthy(); + expect(isUrlExternal('https://google.com')).toBeTruthy(); + expect(isUrlExternal('//google.com')).toBeTruthy(); + expect(isUrlExternal('google.com')).toBeTruthy(); + expect(isUrlExternal('www.google.com')).toBeTruthy(); + expect(isUrlExternal('mailto:mail@example.com')).toBeTruthy(); + + // treat all urls starting with protocol or hostname as external + // such urls are not handled well by react-router Link component + expect(isUrlExternal('http://localhost:8888/port')).toBeTruthy(); + expect(isUrlExternal('https://localhost/secure')).toBeTruthy(); + expect(isUrlExternal('http://localhost/about')).toBeTruthy(); + expect(isUrlExternal('HTTP://localhost/about')).toBeTruthy(); + expect(isUrlExternal('//localhost/about')).toBeTruthy(); + expect(isUrlExternal('localhost/about')).toBeTruthy(); + + expect(isUrlExternal('/about')).toBeFalsy(); + expect(isUrlExternal('#anchor')).toBeFalsy(); +}); + +test('parseUrl', () => { + expect(parseUrl('http://google.com')).toEqual('http://google.com'); + expect(parseUrl('//google.com')).toEqual('//google.com'); + expect(parseUrl('mailto:mail@example.com')).toEqual( + 'mailto:mail@example.com', + ); + expect(parseUrl('google.com')).toEqual('//google.com'); + expect(parseUrl('www.google.com')).toEqual('//www.google.com'); + + expect(parseUrl('/about')).toEqual('/about'); + expect(parseUrl('#anchor')).toEqual('#anchor'); +}); diff --git a/superset-frontend/src/utils/urlUtils.ts b/superset-frontend/src/utils/urlUtils.ts index 2aac5f3e281a7..25e44b5bb5dd3 100644 --- a/superset-frontend/src/utils/urlUtils.ts +++ b/superset-frontend/src/utils/urlUtils.ts @@ -16,7 +16,12 @@ * specific language governing permissions and limitations * under the License. */ -import { JsonObject, QueryFormData, SupersetClient } from '@superset-ui/core'; +import { + isDefined, + JsonObject, + QueryFormData, + SupersetClient, +} from '@superset-ui/core'; import rison from 'rison'; import { isEmpty } from 'lodash'; import { @@ -175,3 +180,26 @@ export function getDashboardPermalink({ anchor, }); } + +const externalUrlRegex = + /^([^:/?#]+:)?(?:(\/\/)?([^/?#]*))?([^?#]+)?(\?[^#]*)?(#.*)?/; + +export function isUrlExternal(url: string) { + const match = url.match(externalUrlRegex) || []; + return ( + (typeof match[1] === 'string' && match[1].length > 0) || + match[2] === '//' || + (typeof match[3] === 'string' && match[3].length > 0) + ); +} + +export function parseUrl(url: string) { + const match = url.match(externalUrlRegex) || []; + // if url is external but start with protocol or '//', + // it can't be used correctly with element + // in such case, add '//' prefix + if (isUrlExternal(url) && !isDefined(match[1]) && !url.startsWith('//')) { + return `//${url}`; + } + return url; +} From 09db4021db4936150c9c154bfb44c203d424c0d4 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Fri, 29 Jul 2022 14:22:48 +0200 Subject: [PATCH 6/6] add comment --- superset-frontend/src/utils/urlUtils.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/superset-frontend/src/utils/urlUtils.ts b/superset-frontend/src/utils/urlUtils.ts index 25e44b5bb5dd3..24442a2f485b2 100644 --- a/superset-frontend/src/utils/urlUtils.ts +++ b/superset-frontend/src/utils/urlUtils.ts @@ -184,6 +184,9 @@ export function getDashboardPermalink({ const externalUrlRegex = /^([^:/?#]+:)?(?:(\/\/)?([^/?#]*))?([^?#]+)?(\?[^#]*)?(#.*)?/; +// group 1 matches protocol +// group 2 matches '//' +// group 3 matches hostname export function isUrlExternal(url: string) { const match = url.match(externalUrlRegex) || []; return (