From 197e606bdf812438dbcfe5bb783198a0ea874b52 Mon Sep 17 00:00:00 2001 From: "Eyo O. Eyo" <7893459+eokoneyo@users.noreply.github.com> Date: Mon, 25 Nov 2024 14:35:02 +0100 Subject: [PATCH] fix issue with generating short url when copying share link (#201475) ## Summary Extracted from https://github.com/elastic/kibana/pull/197484, to make it much easier to backport this fix to other releases that require this fix. This PR fixes the issue with generating short URL for the share link. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Elastic Machine --- .../public/components/context/index.test.tsx | 25 +++ .../share/public/components/context/index.tsx | 23 ++- .../public/components/share_tabs.test.tsx | 16 +- .../share/public/components/share_tabs.tsx | 16 +- .../tabs/link/link_content.test.tsx | 191 ++++++++++++++++++ .../components/tabs/link/link_content.tsx | 107 +++++----- 6 files changed, 300 insertions(+), 78 deletions(-) create mode 100644 src/plugins/share/public/components/context/index.test.tsx create mode 100644 src/plugins/share/public/components/tabs/link/link_content.test.tsx diff --git a/src/plugins/share/public/components/context/index.test.tsx b/src/plugins/share/public/components/context/index.test.tsx new file mode 100644 index 0000000000000..162518e505b1d --- /dev/null +++ b/src/plugins/share/public/components/context/index.test.tsx @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { renderHook } from '@testing-library/react-hooks'; +import { useShareTabsContext } from '.'; + +describe('share menu context', () => { + describe('useShareTabsContext', () => { + it('throws an error if used outside of ShareMenuProvider tree', () => { + const { result } = renderHook(() => useShareTabsContext()); + + expect(result.error?.message).toEqual( + expect.stringContaining( + 'Failed to call `useShareTabsContext` because the context from ShareMenuProvider is missing.' + ) + ); + }); + }); +}); diff --git a/src/plugins/share/public/components/context/index.tsx b/src/plugins/share/public/components/context/index.tsx index b75df40aaa41a..13d6138e42a60 100644 --- a/src/plugins/share/public/components/context/index.tsx +++ b/src/plugins/share/public/components/context/index.tsx @@ -9,7 +9,7 @@ import { ThemeServiceSetup } from '@kbn/core-theme-browser'; import { I18nStart } from '@kbn/core/public'; -import { createContext, useContext } from 'react'; +import React, { type PropsWithChildren, createContext, useContext } from 'react'; import { AnonymousAccessServiceContract } from '../../../common'; import type { @@ -35,6 +35,23 @@ export interface IShareContext extends ShareContext { anchorElement?: HTMLElement; } -export const ShareTabsContext = createContext(null); +const ShareTabsContext = createContext(null); -export const useShareTabsContext = () => useContext(ShareTabsContext); +export const ShareMenuProvider = ({ + shareContext, + children, +}: PropsWithChildren<{ shareContext: IShareContext }>) => { + return {children}; +}; + +export const useShareTabsContext = () => { + const context = useContext(ShareTabsContext); + + if (!context) { + throw new Error( + 'Failed to call `useShareTabsContext` because the context from ShareMenuProvider is missing. Ensure the component or React root is wrapped with ShareMenuProvider' + ); + } + + return context; +}; diff --git a/src/plugins/share/public/components/share_tabs.test.tsx b/src/plugins/share/public/components/share_tabs.test.tsx index b4ad92fce84f9..6b04a28304fdd 100644 --- a/src/plugins/share/public/components/share_tabs.test.tsx +++ b/src/plugins/share/public/components/share_tabs.test.tsx @@ -9,7 +9,7 @@ import React from 'react'; import { ShareMenuTabs } from './share_tabs'; -import { ShareTabsContext } from './context'; +import { ShareMenuProvider } from './context'; import { mountWithIntl } from '@kbn/test-jest-helpers'; import { KibanaLocation, LocatorGetUrlParams, UrlService } from '../../common/url_service'; import { @@ -77,9 +77,9 @@ describe('Share modal tabs', () => { }, ]; const wrapper = mountWithIntl( - + - + ); expect(wrapper.find('[data-test-subj="export"]').exists()).toBeTruthy(); }); @@ -92,11 +92,13 @@ describe('Share modal tabs', () => { generateExportUrl: mockGenerateExportUrl, }, ]; + const wrapper = mountWithIntl( - + - + ); + expect(wrapper.find('[data-test-subj="export"]').exists()).toBeFalsy(); }); @@ -116,9 +118,9 @@ describe('Share modal tabs', () => { }, ]; const wrapper = mountWithIntl( - + - + ); expect(wrapper.find('[data-test-subj="export"]').exists()).toBeTruthy(); }); diff --git a/src/plugins/share/public/components/share_tabs.tsx b/src/plugins/share/public/components/share_tabs.tsx index 0ed540a612009..94c4ab8655dca 100644 --- a/src/plugins/share/public/components/share_tabs.tsx +++ b/src/plugins/share/public/components/share_tabs.tsx @@ -8,16 +8,16 @@ */ import React, { type FC } from 'react'; -import { TabbedModal } from '@kbn/shared-ux-tabbed-modal'; +import { TabbedModal, type IModalTabDeclaration } from '@kbn/shared-ux-tabbed-modal'; -import { ShareTabsContext, useShareTabsContext, type IShareContext } from './context'; +import { ShareMenuProvider, useShareTabsContext, type IShareContext } from './context'; import { linkTab, embedTab, exportTab } from './tabs'; export const ShareMenu: FC<{ shareContext: IShareContext }> = ({ shareContext }) => { return ( - + - + ); }; @@ -25,15 +25,9 @@ export const ShareMenu: FC<{ shareContext: IShareContext }> = ({ shareContext }) export const ShareMenuTabs = () => { const shareContext = useShareTabsContext(); - if (!shareContext) { - return null; - } - const { allowEmbed, objectTypeMeta, onClose, shareMenuItems, anchorElement } = shareContext; - const tabs = []; - - tabs.push(linkTab); + const tabs: Array> = [linkTab]; const enabledItems = shareMenuItems.filter(({ shareMenuItem }) => !shareMenuItem?.disabled); diff --git a/src/plugins/share/public/components/tabs/link/link_content.test.tsx b/src/plugins/share/public/components/tabs/link/link_content.test.tsx new file mode 100644 index 0000000000000..77fac4afc8ce0 --- /dev/null +++ b/src/plugins/share/public/components/tabs/link/link_content.test.tsx @@ -0,0 +1,191 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import React, { type ComponentProps } from 'react'; +import { __IntlProvider as IntlProvider } from '@kbn/i18n-react'; +import userEvent from '@testing-library/user-event'; +import { render, screen, waitFor } from '@testing-library/react'; + +import { urlServiceTestSetup } from '../../../../common/url_service/__tests__/setup'; +import { MockLocatorDefinition } from '../../../../common/url_service/mocks'; +import { BrowserShortUrlClientFactory } from '../../../url_service/short_urls/short_url_client_factory'; +import { + BrowserShortUrlClientHttp, + BrowserShortUrlClient, +} from '../../../url_service/short_urls/short_url_client'; +import { BrowserUrlService } from '../../../types'; +import { LinkContent } from './link_content'; + +const renderComponent = (props: ComponentProps) => { + render( + + + + ); +}; + +describe('LinkContent', () => { + const shareableUrl = 'http://localhost:5601/app/dashboards#/view/123'; + + const http: BrowserShortUrlClientHttp = { + basePath: { + get: () => '/xyz', + }, + fetch: jest.fn(async () => { + return {} as any; + }), + }; + + let urlService: BrowserUrlService; + + // @ts-expect-error there is a type because we override the shortUrls implementation + // eslint-disable-next-line prefer-const + ({ service: urlService } = urlServiceTestSetup({ + shortUrls: ({ locators }) => + new BrowserShortUrlClientFactory({ + http, + locators, + }), + })); + + beforeAll(() => { + Object.defineProperty(document, 'execCommand', { + value: jest.fn(() => true), + }); + }); + + it('uses the delegatedShareUrlHandler to generate the shareable URL when it is provided', async () => { + const user = userEvent.setup(); + const objectType = 'dashboard'; + const objectId = '123'; + const isDirty = false; + + const delegatedShareUrlHandler = jest.fn(); + + renderComponent({ + objectType, + objectId, + isDirty, + shareableUrl, + urlService, + allowShortUrl: true, + delegatedShareUrlHandler, + }); + + await user.click(screen.getByTestId('copyShareUrlButton')); + + expect(delegatedShareUrlHandler).toHaveBeenCalled(); + }); + + it('returns the shareable URL when the delegatedShareUrlHandler is not provided and shortURLs are not allowed', async () => { + const user = userEvent.setup(); + const objectType = 'dashboard'; + const objectId = '123'; + const isDirty = false; + + renderComponent({ + objectType, + objectId, + isDirty, + shareableUrl, + urlService, + allowShortUrl: false, + }); + + const copyButton = screen.getByTestId('copyShareUrlButton'); + + await user.click(copyButton); + + waitFor(() => { + expect(copyButton.getAttribute('data-share-url')).toBe(shareableUrl); + }); + }); + + it('invokes the createWithLocator method on the shortURL service if a locator is present when the copy button is clicked', async () => { + const user = userEvent.setup(); + const objectType = 'dashboard'; + const objectId = '123'; + const isDirty = false; + const shareableUrlLocatorParams = { + locator: new MockLocatorDefinition('TEST_LOCATOR'), + params: {}, + }; + + const shortURL = 'http://localhost:5601/xyz/r/s/yellow-orange-tomato'; + + const createWithLocatorSpy = jest.spyOn(BrowserShortUrlClient.prototype, 'createWithLocator'); + + createWithLocatorSpy.mockResolvedValue({ + // @ts-expect-error we only return locator property, as that's all we need for this test + locator: { + getUrl: jest.fn(() => Promise.resolve(shortURL)), + }, + }); + + renderComponent({ + objectType, + objectId, + isDirty, + shareableUrl, + urlService, + allowShortUrl: true, + // @ts-ignore this locator is passed mainly to test the code path that invokes createWithLocator + shareableUrlLocatorParams, + }); + + const copyButton = screen.getByTestId('copyShareUrlButton'); + + const numberOfClicks = 4; + + for (const _click of Array.from({ length: numberOfClicks })) { + await user.click(copyButton); + } + + // should only invoke once no matter how many times the button is clicked + expect(createWithLocatorSpy).toHaveBeenCalledTimes(1); + expect(copyButton.getAttribute('data-share-url')).toBe(shortURL); + }); + + it('invokes the createFromLongUrl method on the shortURL service if a locator is not present when the copy button is clicked', async () => { + const user = userEvent.setup(); + const objectType = 'dashboard'; + const objectId = '123'; + const isDirty = false; + + const shortURL = 'http://localhost:5601/xyz/r/s/yellow-orange-tomato'; + + const createFromLongUrlSpy = jest.spyOn(BrowserShortUrlClient.prototype, 'createFromLongUrl'); + + // @ts-expect-error we only return url property, as that's all we need for this test + createFromLongUrlSpy.mockResolvedValue({ + url: shortURL, + }); + + renderComponent({ + objectType, + objectId, + isDirty, + shareableUrl, + urlService, + allowShortUrl: true, + }); + + const copyButton = screen.getByTestId('copyShareUrlButton'); + + const numberOfClicks = 4; + + for (const _click of Array.from({ length: numberOfClicks })) { + await user.click(copyButton); + } + + // should only invoke once no matter how many times the button is clicked + expect(createFromLongUrlSpy).toHaveBeenCalledTimes(1); + expect(copyButton.getAttribute('data-share-url')).toBe(shortURL); + }); +}); diff --git a/src/plugins/share/public/components/tabs/link/link_content.tsx b/src/plugins/share/public/components/tabs/link/link_content.tsx index 0871599a524ba..6c0d8e6e988ec 100644 --- a/src/plugins/share/public/components/tabs/link/link_content.tsx +++ b/src/plugins/share/public/components/tabs/link/link_content.tsx @@ -20,8 +20,8 @@ import { } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n-react'; -import React, { useCallback, useMemo, useState } from 'react'; -import { IShareContext } from '../../context'; +import React, { useCallback, useState, useRef, useEffect } from 'react'; +import type { IShareContext } from '../../context'; type LinkProps = Pick< IShareContext, @@ -42,87 +42,80 @@ interface UrlParams { } export const LinkContent = ({ - objectType, isDirty, + objectType, shareableUrl, urlService, shareableUrlLocatorParams, allowShortUrl, delegatedShareUrlHandler, }: LinkProps) => { - const [url, setUrl] = useState(''); - const [urlParams] = useState(undefined); + const [snapshotUrl, setSnapshotUrl] = useState(''); const [isTextCopied, setTextCopied] = useState(false); - const [shortUrlCache, setShortUrlCache] = useState(undefined); const [isLoading, setIsLoading] = useState(false); + const urlParamsRef = useRef(undefined); + const urlToCopy = useRef(undefined); + const copiedTextToolTipCleanupIdRef = useRef>(); - const getUrlWithUpdatedParams = useCallback( - (tempUrl: string): string => { - const urlWithUpdatedParams = urlParams - ? Object.keys(urlParams).reduce((urlAccumulator, key) => { - const urlParam = urlParams[key]; - return urlParam - ? Object.keys(urlParam).reduce((queryAccumulator, queryParam) => { - const isQueryParamEnabled = urlParam[queryParam]; - return isQueryParamEnabled - ? queryAccumulator + `&${queryParam}=true` - : queryAccumulator; - }, urlAccumulator) - : urlAccumulator; - }, tempUrl) - : tempUrl; + const getUrlWithUpdatedParams = useCallback((tempUrl: string): string => { + const urlWithUpdatedParams = urlParamsRef.current + ? Object.keys(urlParamsRef.current).reduce((urlAccumulator, key) => { + const urlParam = urlParamsRef.current?.[key]; + return urlParam + ? Object.keys(urlParam).reduce((queryAccumulator, queryParam) => { + const isQueryParamEnabled = urlParam[queryParam]; + return isQueryParamEnabled + ? queryAccumulator + `&${queryParam}=true` + : queryAccumulator; + }, urlAccumulator) + : urlAccumulator; + }, tempUrl) + : tempUrl; - // persist updated url to state - setUrl(urlWithUpdatedParams); - return urlWithUpdatedParams; - }, - [urlParams] - ); + return urlWithUpdatedParams; + }, []); - const getSnapshotUrl = useCallback(() => { - return getUrlWithUpdatedParams(shareableUrl || window.location.href); + useEffect(() => { + setSnapshotUrl(getUrlWithUpdatedParams(shareableUrl || window.location.href)); }, [getUrlWithUpdatedParams, shareableUrl]); const createShortUrl = useCallback(async () => { + const shortUrlService = urlService.shortUrls.get(null); + if (shareableUrlLocatorParams) { - const shortUrls = urlService.shortUrls.get(null); - const shortUrl = await shortUrls.createWithLocator(shareableUrlLocatorParams); - const urlWithLoc = await shortUrl.locator.getUrl(shortUrl.params, { absolute: true }); - setShortUrlCache(urlWithLoc); - return urlWithLoc; + const shortUrl = await shortUrlService.createWithLocator(shareableUrlLocatorParams); + return shortUrl.locator.getUrl(shortUrl.params, { absolute: true }); } else { - const snapshotUrl = getSnapshotUrl(); - const shortUrl = await urlService.shortUrls.get(null).createFromLongUrl(snapshotUrl); - setShortUrlCache(shortUrl.url); - - return shortUrl.url; + return (await shortUrlService.createFromLongUrl(snapshotUrl)).url; } - }, [shareableUrlLocatorParams, urlService.shortUrls, getSnapshotUrl, setShortUrlCache]); + }, [shareableUrlLocatorParams, urlService.shortUrls, snapshotUrl]); const copyUrlHelper = useCallback(async () => { setIsLoading(true); - let urlToCopy = url; - if (!urlToCopy || delegatedShareUrlHandler) { - urlToCopy = delegatedShareUrlHandler - ? delegatedShareUrlHandler?.() + if (!urlToCopy.current) { + urlToCopy.current = delegatedShareUrlHandler + ? delegatedShareUrlHandler() : allowShortUrl ? await createShortUrl() - : getSnapshotUrl(); + : snapshotUrl; } - copyToClipboard(urlToCopy); - setUrl(urlToCopy); - setTextCopied(true); + copyToClipboard(urlToCopy.current); + setTextCopied(() => { + if (copiedTextToolTipCleanupIdRef.current) { + clearInterval(copiedTextToolTipCleanupIdRef.current); + } + + // set up timer to revert copied state to false after specified duration + copiedTextToolTipCleanupIdRef.current = setTimeout(() => setTextCopied(false), 1000); + + // set copied state to true for now + return true; + }); setIsLoading(false); - return urlToCopy; - }, [url, delegatedShareUrlHandler, allowShortUrl, createShortUrl, getSnapshotUrl]); + }, [snapshotUrl, delegatedShareUrlHandler, allowShortUrl, createShortUrl]); - const handleTestUrl = useMemo(() => { - if (objectType !== 'search' || !allowShortUrl) return getSnapshotUrl(); - else if (objectType === 'search' && allowShortUrl) return shortUrlCache; - return copyUrlHelper(); - }, [objectType, getSnapshotUrl, allowShortUrl, shortUrlCache, copyUrlHelper]); return ( <> @@ -157,14 +150,14 @@ export const LinkContent = ({ (objectType === 'lens' && isDirty ? null : setTextCopied(false))} onClick={copyUrlHelper} color={objectType === 'lens' && isDirty ? 'warning' : 'primary'}