From 9134d07969f96c80a7cf8d95697ff77f5eee01ad Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 31 Oct 2024 09:30:14 +0100 Subject: [PATCH 01/10] Extract logic to determine if a list of servers contains duplicates --- scripts/docker/servers_from_env.sh | 2 + src/servers/helpers/ImportServersBtn.tsx | 43 +++++++++------- src/servers/helpers/index.ts | 50 +++++++++++++++++++ src/servers/reducers/servers.ts | 15 +----- .../servers/helpers/ImportServersBtn.test.tsx | 25 +++++----- 5 files changed, 90 insertions(+), 45 deletions(-) create mode 100644 src/servers/helpers/index.ts diff --git a/scripts/docker/servers_from_env.sh b/scripts/docker/servers_from_env.sh index 4275f591..8c49f674 100755 --- a/scripts/docker/servers_from_env.sh +++ b/scripts/docker/servers_from_env.sh @@ -4,6 +4,8 @@ set -e ME=$(basename $0) +# In order to allow people to pre-configure a server in their shlink-web-client instance via env vars, this function +# dumps a servers.json file based on the values provided via env vars setup_single_shlink_server() { [ -n "$SHLINK_SERVER_URL" ] || return 0 [ -n "$SHLINK_SERVER_API_KEY" ] || return 0 diff --git a/src/servers/helpers/ImportServersBtn.tsx b/src/servers/helpers/ImportServersBtn.tsx index 161e49cd..f77faba1 100644 --- a/src/servers/helpers/ImportServersBtn.tsx +++ b/src/servers/helpers/ImportServersBtn.tsx @@ -9,6 +9,7 @@ import { componentFactory, useDependencies } from '../../container/utils'; import type { ServerData, ServersMap } from '../data'; import type { ServersImporter } from '../services/ServersImporter'; import { DuplicatedServersModal } from './DuplicatedServersModal'; +import { dedupServers } from './index'; export type ImportServersBtnProps = PropsWithChildren<{ onImport?: () => void; @@ -26,9 +27,6 @@ type ImportServersBtnDeps = { ServersImporter: ServersImporter }; -const serversInclude = (servers: ServerData[], { url, apiKey }: ServerData) => - servers.some((server) => server.url === url && server.apiKey === apiKey); - const ImportServersBtn: FCWithDeps = ({ createServers, servers, @@ -43,7 +41,9 @@ const ImportServersBtn: FCWithDeps([]); const [isModalOpen,, showModal, hideModal] = useToggle(); - const serversToCreate = useRef([]); + const importedServersRef = useRef([]); + const newServersRef = useRef([]); + const create = useCallback((serversData: ServerData[]) => { createServers(serversData); onImport(); @@ -51,22 +51,21 @@ const ImportServersBtn: FCWithDeps) => serversImporter.importServersFromFile(target.files?.[0]) - .then((newServers) => { - serversToCreate.current = newServers; + .then((importedServers) => { + const { duplicatedServers, newServers } = dedupServers(servers, importedServers); - const existingServers = Object.values(servers); - const dupServers = newServers.filter((server) => serversInclude(existingServers, server)); - const hasDuplicatedServers = !!dupServers.length; + importedServersRef.current = importedServers; + newServersRef.current = newServers; - if (!hasDuplicatedServers) { - create(newServers); + if (duplicatedServers.length === 0) { + create(importedServers); } else { - setDuplicatedServers(dupServers); + setDuplicatedServers(duplicatedServers); showModal(); } }) .then(() => { - // Reset input after processing file + // Reset file input after processing file (target as { value: string | null }).value = null; }) .catch(onImportError), @@ -74,13 +73,13 @@ const ImportServersBtn: FCWithDeps { - create(serversToCreate.current); + create(importedServersRef.current); hideModal(); - }, [create, hideModal, serversToCreate]); + }, [create, hideModal]); const createNonDuplicatedServers = useCallback(() => { - create(serversToCreate.current.filter((server) => !serversInclude(duplicatedServers, server))); + create(newServersRef.current); hideModal(); - }, [create, duplicatedServers, hideModal]); + }, [create, hideModal]); return ( <> @@ -91,7 +90,15 @@ const ImportServersBtn: FCWithDepsname, apiKey and url columns. - + ( + (acc, server) => ({ ...acc, [server.id]: server }), + {}, + ); +} + +const serversInclude = (serversList: ServerData[], { url, apiKey }: ServerData) => + serversList.some((server) => server.url === url && server.apiKey === apiKey); + +export type DedupServersResult = { + /** Servers which already exist in the reference list */ + duplicatedServers: ServerData[]; + /** Servers which are new based on a reference list */ + newServers: ServerData[]; +}; + +/** + * Given a list of new servers, checks which of them already exist in a servers map, and which don't + */ +export function dedupServers(servers: ServersMap, serversToAdd: ServerData[]): DedupServersResult { + const serversList = Object.values(servers); + const { duplicatedServers = [], newServers = [] } = groupBy( + serversToAdd, + (server) => serversInclude(serversList, server) ? 'duplicatedServers' : 'newServers', + ); + + return { duplicatedServers, newServers }; +} diff --git a/src/servers/reducers/servers.ts b/src/servers/reducers/servers.ts index fe8220e7..7db3d41d 100644 --- a/src/servers/reducers/servers.ts +++ b/src/servers/reducers/servers.ts @@ -1,7 +1,7 @@ import type { PayloadAction } from '@reduxjs/toolkit'; import { createSlice } from '@reduxjs/toolkit'; -import { randomUUID } from '../../utils/utils'; import type { ServerData, ServersMap, ServerWithId } from '../data'; +import { serversListToMap, serverWithId } from '../helpers'; interface EditServer { serverId: string; @@ -15,19 +15,6 @@ interface SetAutoConnect { const initialState: ServersMap = {}; -const serverWithId = (server: ServerWithId | ServerData): ServerWithId => { - if ('id' in server) { - return server; - } - - return { ...server, id: randomUUID() }; -}; - -const serversListToMap = (servers: ServerWithId[]): ServersMap => servers.reduce( - (acc, server) => ({ ...acc, [server.id]: server }), - {}, -); - export const { actions, reducer } = createSlice({ name: 'shlink/servers', initialState, diff --git a/test/servers/helpers/ImportServersBtn.test.tsx b/test/servers/helpers/ImportServersBtn.test.tsx index b3ba60c5..2a2ddfa0 100644 --- a/test/servers/helpers/ImportServersBtn.test.tsx +++ b/test/servers/helpers/ImportServersBtn.test.tsx @@ -1,4 +1,4 @@ -import { fireEvent, screen, waitFor } from '@testing-library/react'; +import { screen, waitFor } from '@testing-library/react'; import { fromPartial } from '@total-typescript/shoehorn'; import type { ServersMap, ServerWithId } from '../../../src/servers/data'; import type { @@ -9,6 +9,7 @@ import { checkAccessibility } from '../../__helpers__/accessibility'; import { renderWithEvents } from '../../__helpers__/setUpTest'; describe('', () => { + const csvFile = new File([''], 'servers.csv', { type: 'text/csv' }); const onImportMock = vi.fn(); const createServersMock = vi.fn(); const importServersFromFile = vi.fn().mockResolvedValue([]); @@ -54,14 +55,13 @@ describe('', () => { }); it('imports servers when file input changes', async () => { - const { container } = setUp(); - const input = container.querySelector('[type=file]'); + const { user } = setUp(); + + const input = screen.getByTestId('csv-file-input'); + await user.upload(input, csvFile); - if (input) { - fireEvent.change(input, { target: { files: [''] } }); - } expect(importServersFromFile).toHaveBeenCalledTimes(1); - await waitFor(() => expect(createServersMock).toHaveBeenCalledTimes(1)); + expect(createServersMock).toHaveBeenCalledTimes(1); }); it.each([ @@ -70,15 +70,14 @@ describe('', () => { ])('creates expected servers depending on selected option in modal', async (btnName, savesDuplicatedServers) => { const existingServer = fromPartial({ id: 'abc', url: 'existingUrl', apiKey: 'existingApiKey' }); const newServer = fromPartial({ url: 'newUrl', apiKey: 'newApiKey' }); - const { container, user } = setUp({}, { abc: existingServer }); - const input = container.querySelector('[type=file]'); + const { user } = setUp({}, { abc: existingServer }); + const input = screen.getByTestId('csv-file-input'); + importServersFromFile.mockResolvedValue([existingServer, newServer]); expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); - if (input) { - fireEvent.change(input, { target: { files: [''] } }); - } - await waitFor(() => expect(screen.getByRole('dialog')).toBeInTheDocument()); + await user.upload(input, csvFile); + expect(screen.getByRole('dialog')).toBeInTheDocument(); await user.click(screen.getByRole('button', { name: btnName })); expect(createServersMock).toHaveBeenCalledWith(savesDuplicatedServers ? [existingServer, newServer] : [newServer]); From e786f9d21fb0a2576f0ed4cd631356fecf6f770d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 10:27:35 +0100 Subject: [PATCH 02/10] Update CreateServer logic so that it ensures a unique human-friendly ID is set --- src/app/App.tsx | 4 +- src/servers/CreateServer.tsx | 12 ++--- src/servers/helpers/ImportServersBtn.tsx | 18 +++---- src/servers/helpers/index.ts | 47 ++++++++++++++----- src/servers/reducers/remoteServers.ts | 7 ++- src/servers/reducers/servers.ts | 7 +-- .../servers/helpers/ImportServersBtn.test.tsx | 28 +++++++---- 7 files changed, 76 insertions(+), 47 deletions(-) diff --git a/src/app/App.tsx b/src/app/App.tsx index 3c7c540d..aa86063e 100644 --- a/src/app/App.tsx +++ b/src/app/App.tsx @@ -50,8 +50,8 @@ const App: FCWithDeps = ( const isHome = location.pathname === '/'; useEffect(() => { - // Try to fetch the remote servers if the list is empty at first - // We use a ref because we don't care if the servers list becomes empty later + // Try to fetch the remote servers if the list is empty during first render. + // We use a ref because we don't care if the servers list becomes empty later. if (Object.keys(initialServers.current).length === 0) { fetchServers(); } diff --git a/src/servers/CreateServer.tsx b/src/servers/CreateServer.tsx index 6744e127..c40cc6d0 100644 --- a/src/servers/CreateServer.tsx +++ b/src/servers/CreateServer.tsx @@ -8,8 +8,8 @@ import { NoMenuLayout } from '../common/NoMenuLayout'; import type { FCWithDeps } from '../container/utils'; import { componentFactory, useDependencies } from '../container/utils'; import { useGoBack } from '../utils/helpers/hooks'; -import { randomUUID } from '../utils/utils'; import type { ServerData, ServersMap, ServerWithId } from './data'; +import { ensureUniqueIds } from './helpers'; import { DuplicatedServersModal } from './helpers/DuplicatedServersModal'; import type { ImportServersBtnProps } from './helpers/ImportServersBtn'; import { ServerForm } from './helpers/ServerForm'; @@ -44,12 +44,12 @@ const CreateServer: FCWithDeps = ({ servers const [errorImporting, setErrorImporting] = useTimeoutToggle(false, SHOW_IMPORT_MSG_TIME); const [isConfirmModalOpen, toggleConfirmModal] = useToggle(); const [serverData, setServerData] = useState(); - const saveNewServer = useCallback((theServerData: ServerData) => { - const id = randomUUID(); + const saveNewServer = useCallback((newServerData: ServerData) => { + const [newServerWithUniqueId] = ensureUniqueIds(servers, [newServerData]); - createServers([{ ...theServerData, id }]); - navigate(`/server/${id}`); - }, [createServers, navigate]); + createServers([newServerWithUniqueId]); + navigate(`/server/${newServerWithUniqueId.id}`); + }, [createServers, navigate, servers]); const onSubmit = useCallback((newServerData: ServerData) => { setServerData(newServerData); diff --git a/src/servers/helpers/ImportServersBtn.tsx b/src/servers/helpers/ImportServersBtn.tsx index f77faba1..d4e8bee3 100644 --- a/src/servers/helpers/ImportServersBtn.tsx +++ b/src/servers/helpers/ImportServersBtn.tsx @@ -6,10 +6,10 @@ import { useCallback, useRef, useState } from 'react'; import { Button, UncontrolledTooltip } from 'reactstrap'; import type { FCWithDeps } from '../../container/utils'; import { componentFactory, useDependencies } from '../../container/utils'; -import type { ServerData, ServersMap } from '../data'; +import type { ServerData, ServersMap, ServerWithId } from '../data'; import type { ServersImporter } from '../services/ServersImporter'; import { DuplicatedServersModal } from './DuplicatedServersModal'; -import { dedupServers } from './index'; +import { dedupServers, ensureUniqueIds } from './index'; export type ImportServersBtnProps = PropsWithChildren<{ onImport?: () => void; @@ -19,7 +19,7 @@ export type ImportServersBtnProps = PropsWithChildren<{ }>; type ImportServersBtnConnectProps = ImportServersBtnProps & { - createServers: (servers: ServerData[]) => void; + createServers: (servers: ServerWithId[]) => void; servers: ServersMap; }; @@ -41,10 +41,10 @@ const ImportServersBtn: FCWithDeps([]); const [isModalOpen,, showModal, hideModal] = useToggle(); - const importedServersRef = useRef([]); - const newServersRef = useRef([]); + const importedServersRef = useRef([]); + const newServersRef = useRef([]); - const create = useCallback((serversData: ServerData[]) => { + const create = useCallback((serversData: ServerWithId[]) => { createServers(serversData); onImport(); }, [createServers, onImport]); @@ -54,11 +54,11 @@ const ImportServersBtn: FCWithDeps { const { duplicatedServers, newServers } = dedupServers(servers, importedServers); - importedServersRef.current = importedServers; - newServersRef.current = newServers; + importedServersRef.current = ensureUniqueIds(servers, importedServers); + newServersRef.current = ensureUniqueIds(servers, newServers); if (duplicatedServers.length === 0) { - create(importedServers); + create(importedServersRef.current); } else { setDuplicatedServers(duplicatedServers); showModal(); diff --git a/src/servers/helpers/index.ts b/src/servers/helpers/index.ts index 1c71c39e..6385c4d6 100644 --- a/src/servers/helpers/index.ts +++ b/src/servers/helpers/index.ts @@ -6,24 +6,18 @@ import type { ServerData, ServersMap, ServerWithId } from '../data'; * in lowercase and replacing invalid URL characters with hyphens. */ function idForServer(server: ServerData): string { + // TODO Handle invalid URLs. If not valid url, use the value as is const url = new URL(server.url); return `${server.name} ${url.host}`.toLowerCase().replace(/[^a-zA-Z0-9-_.~]/g, '-'); } -export function serverWithId(server: ServerWithId | ServerData): ServerWithId { - if ('id' in server) { - return server; - } - - const id = idForServer(server); - return { ...server, id }; -} - export function serversListToMap(servers: ServerWithId[]): ServersMap { - return servers.reduce( - (acc, server) => ({ ...acc, [server.id]: server }), - {}, - ); + const serversMap: ServersMap = {}; + servers.forEach((server) => { + serversMap[server.id] = server; + }); + + return serversMap; } const serversInclude = (serversList: ServerData[], { url, apiKey }: ServerData) => @@ -48,3 +42,30 @@ export function dedupServers(servers: ServersMap, serversToAdd: ServerData[]): D return { duplicatedServers, newServers }; } + +/** + * Given a servers map and a list of servers, return the same list of servers but all with an ID, ensuring the ID is + * unique both among all those servers and existing ones + */ +export function ensureUniqueIds(existingServers: ServersMap, serversList: ServerData[]): ServerWithId[] { + const existingIds = new Set(Object.keys(existingServers)); + const serversWithId: ServerWithId[] = []; + + serversList.forEach((server) => { + const baseId = idForServer(server); + + let id = baseId; + let iterations = 1; + while (existingIds.has(id)) { + id = `${baseId}-${iterations}`; + iterations++; + } + + serversWithId.push({ id, ...server }); + + // Add this server's ID to the list, so that it is taken into consideration for the next ones + existingIds.add(id); + }); + + return serversWithId; +} diff --git a/src/servers/reducers/remoteServers.ts b/src/servers/reducers/remoteServers.ts index 1ae5496c..37c437d0 100644 --- a/src/servers/reducers/remoteServers.ts +++ b/src/servers/reducers/remoteServers.ts @@ -1,11 +1,14 @@ import type { HttpClient } from '@shlinkio/shlink-js-sdk'; import pack from '../../../package.json'; import { createAsyncThunk } from '../../utils/helpers/redux'; -import type { ServerData } from '../data'; import { hasServerData } from '../data'; +import { ensureUniqueIds } from '../helpers'; import { createServers } from './servers'; -const responseToServersList = (data: any): ServerData[] => (Array.isArray(data) ? data.filter(hasServerData) : []); +const responseToServersList = (data: any) => ensureUniqueIds( + {}, + (Array.isArray(data) ? data.filter(hasServerData) : []), +); export const fetchServers = (httpClient: HttpClient) => createAsyncThunk( 'shlink/remoteServers/fetchServers', diff --git a/src/servers/reducers/servers.ts b/src/servers/reducers/servers.ts index 7db3d41d..ed02fe68 100644 --- a/src/servers/reducers/servers.ts +++ b/src/servers/reducers/servers.ts @@ -1,7 +1,7 @@ import type { PayloadAction } from '@reduxjs/toolkit'; import { createSlice } from '@reduxjs/toolkit'; import type { ServerData, ServersMap, ServerWithId } from '../data'; -import { serversListToMap, serverWithId } from '../helpers'; +import { serversListToMap } from '../helpers'; interface EditServer { serverId: string; @@ -57,10 +57,7 @@ export const { actions, reducer } = createSlice({ }, }, createServers: { - prepare: (servers: ServerData[]) => { - const payload = serversListToMap(servers.map(serverWithId)); - return { payload }; - }, + prepare: (servers: ServerWithId[]) => ({ payload: serversListToMap(servers) }), reducer: (state, { payload: newServers }: PayloadAction) => ({ ...state, ...newServers }), }, }, diff --git a/test/servers/helpers/ImportServersBtn.test.tsx b/test/servers/helpers/ImportServersBtn.test.tsx index 2a2ddfa0..f28618dc 100644 --- a/test/servers/helpers/ImportServersBtn.test.tsx +++ b/test/servers/helpers/ImportServersBtn.test.tsx @@ -1,6 +1,6 @@ import { screen, waitFor } from '@testing-library/react'; import { fromPartial } from '@total-typescript/shoehorn'; -import type { ServersMap, ServerWithId } from '../../../src/servers/data'; +import type { ServerData, ServersMap, ServerWithId } from '../../../src/servers/data'; import type { ImportServersBtnProps } from '../../../src/servers/helpers/ImportServersBtn'; import { ImportServersBtnFactory } from '../../../src/servers/helpers/ImportServersBtn'; @@ -65,22 +65,30 @@ describe('', () => { }); it.each([ - ['Save anyway', true], - ['Discard', false], - ])('creates expected servers depending on selected option in modal', async (btnName, savesDuplicatedServers) => { - const existingServer = fromPartial({ id: 'abc', url: 'existingUrl', apiKey: 'existingApiKey' }); - const newServer = fromPartial({ url: 'newUrl', apiKey: 'newApiKey' }); - const { user } = setUp({}, { abc: existingServer }); - const input = screen.getByTestId('csv-file-input'); + { btnName: 'Save anyway',savesDuplicatedServers: true }, + { btnName: 'Discard', savesDuplicatedServers: false }, + ])('creates expected servers depending on selected option in modal', async ({ btnName, savesDuplicatedServers }) => { + const existingServer: ServerWithId = { + name: 'existingServer', + id: 'existingserver-s.test', + url: 'http://s.test/existingUrl', + apiKey: 'existingApiKey', + }; + const newServer: ServerData = { name: 'newServer', url: 'http://s.test/newUrl', apiKey: 'newApiKey' }; + const { user } = setUp({}, { [existingServer.id]: existingServer }); importServersFromFile.mockResolvedValue([existingServer, newServer]); expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); - await user.upload(input, csvFile); + await user.upload(screen.getByTestId('csv-file-input'), csvFile); expect(screen.getByRole('dialog')).toBeInTheDocument(); await user.click(screen.getByRole('button', { name: btnName })); - expect(createServersMock).toHaveBeenCalledWith(savesDuplicatedServers ? [existingServer, newServer] : [newServer]); + expect(createServersMock).toHaveBeenCalledWith( + savesDuplicatedServers + ? [existingServer, expect.objectContaining(newServer)] + : [expect.objectContaining(newServer)], + ); expect(onImportMock).toHaveBeenCalledTimes(1); }); }); From 44fb07840e268b8efc734f211938371eb49cc71a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 11:59:54 +0100 Subject: [PATCH 03/10] Fix remoteServers test --- src/servers/helpers/index.ts | 2 +- test/servers/reducers/remoteServers.test.ts | 21 ++++++++------------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/servers/helpers/index.ts b/src/servers/helpers/index.ts index 6385c4d6..589b93c7 100644 --- a/src/servers/helpers/index.ts +++ b/src/servers/helpers/index.ts @@ -61,7 +61,7 @@ export function ensureUniqueIds(existingServers: ServersMap, serversList: Server iterations++; } - serversWithId.push({ id, ...server }); + serversWithId.push({ ...server, id }); // Add this server's ID to the list, so that it is taken into consideration for the next ones existingIds.add(id); diff --git a/test/servers/reducers/remoteServers.test.ts b/test/servers/reducers/remoteServers.test.ts index 1026cbcd..1fa8562a 100644 --- a/test/servers/reducers/remoteServers.test.ts +++ b/test/servers/reducers/remoteServers.test.ts @@ -12,27 +12,25 @@ describe('remoteServersReducer', () => { [ [ { - id: '111', name: 'acel.me from servers.json', url: 'https://acel.me', apiKey: '07fb8a96-8059-4094-a24c-80a7d5e7e9b0', }, { - id: '222', name: 'Local from servers.json', url: 'http://localhost:8000', apiKey: '7a531c75-134e-4d5c-86e0-a71b7167b57a', }, ], { - 111: { - id: '111', + 'acel.me-from-servers.json-acel.me': { + id: 'acel.me-from-servers.json-acel.me', name: 'acel.me from servers.json', url: 'https://acel.me', apiKey: '07fb8a96-8059-4094-a24c-80a7d5e7e9b0', }, - 222: { - id: '222', + 'local-from-servers.json-localhost-8000': { + id: 'local-from-servers.json-localhost-8000', name: 'Local from servers.json', url: 'http://localhost:8000', apiKey: '7a531c75-134e-4d5c-86e0-a71b7167b57a', @@ -42,31 +40,28 @@ describe('remoteServersReducer', () => { [ [ { - id: '111', name: 'acel.me from servers.json', url: 'https://acel.me', apiKey: '07fb8a96-8059-4094-a24c-80a7d5e7e9b0', }, { - id: '222', name: 'Invalid', }, { - id: '333', name: 'Local from servers.json', url: 'http://localhost:8000', apiKey: '7a531c75-134e-4d5c-86e0-a71b7167b57a', }, ], { - 111: { - id: '111', + 'acel.me-from-servers.json-acel.me': { + id: 'acel.me-from-servers.json-acel.me', name: 'acel.me from servers.json', url: 'https://acel.me', apiKey: '07fb8a96-8059-4094-a24c-80a7d5e7e9b0', }, - 333: { - id: '333', + 'local-from-servers.json-localhost-8000': { + id: 'local-from-servers.json-localhost-8000', name: 'Local from servers.json', url: 'http://localhost:8000', apiKey: '7a531c75-134e-4d5c-86e0-a71b7167b57a', From 88ad2e7fc29d4109faa25cd05f034665ab3025e7 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 12:01:58 +0100 Subject: [PATCH 04/10] Fix serversReducer test --- test/servers/reducers/servers.test.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/servers/reducers/servers.test.ts b/test/servers/reducers/servers.test.ts index 025a44a1..fcb4642a 100644 --- a/test/servers/reducers/servers.test.ts +++ b/test/servers/reducers/servers.test.ts @@ -105,15 +105,6 @@ describe('serversReducer', () => { expect(payload).toEqual(list); }); - - it('generates an id for every provided server if they do not have it', () => { - const servers = Object.values(list).map(({ name, autoConnect, url, apiKey }) => ( - { name, autoConnect, url, apiKey } - )); - const { payload } = createServers(servers); - - expect(Object.values(payload).every(({ id }) => !!id)).toEqual(true); - }); }); describe('setAutoConnect', () => { From dc8c749212f3aaab58f22846e4d901e0cc78f966 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 12:09:13 +0100 Subject: [PATCH 05/10] Remove dependency on uuid package --- package-lock.json | 19 ------------------- package.json | 1 - src/utils/utils.ts | 3 --- .../servers/helpers/ImportServersBtn.test.tsx | 9 ++++++--- test/servers/reducers/selectedServer.test.ts | 7 +++---- 5 files changed, 9 insertions(+), 30 deletions(-) diff --git a/package-lock.json b/package-lock.json index 2e7f0c22..1ba44e04 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,7 +32,6 @@ "react-router-dom": "^6.27.0", "reactstrap": "^9.2.3", "redux-localstorage-simple": "^2.5.1", - "uuid": "^10.0.0", "workbox-core": "^7.1.0", "workbox-expiration": "^7.1.0", "workbox-precaching": "^7.1.0", @@ -11580,19 +11579,6 @@ "dev": true, "license": "MIT" }, - "node_modules/uuid": { - "version": "10.0.0", - "resolved": "https://registry.npmjs.org/uuid/-/uuid-10.0.0.tgz", - "integrity": "sha512-8XkAphELsDnEGrDxUOHB3RGvXz6TeuYSGEZBOjtTtPm2lwhGBjLgOzLHB63IUWfBpNucQjND6d3AOudO+H3RWQ==", - "funding": [ - "https://github.com/sponsors/broofa", - "https://github.com/sponsors/ctavan" - ], - "license": "MIT", - "bin": { - "uuid": "dist/bin/uuid" - } - }, "node_modules/validate-npm-package-license": { "version": "3.0.4", "resolved": "https://registry.npmjs.org/validate-npm-package-license/-/validate-npm-package-license-3.0.4.tgz", @@ -20190,11 +20176,6 @@ "version": "1.0.2", "dev": true }, - "uuid": { - "version": "10.0.0", - "resolved": "https://registry.npmjs.org/uuid/-/uuid-10.0.0.tgz", - "integrity": "sha512-8XkAphELsDnEGrDxUOHB3RGvXz6TeuYSGEZBOjtTtPm2lwhGBjLgOzLHB63IUWfBpNucQjND6d3AOudO+H3RWQ==" - }, "validate-npm-package-license": { "version": "3.0.4", "resolved": "https://registry.npmjs.org/validate-npm-package-license/-/validate-npm-package-license-3.0.4.tgz", diff --git a/package.json b/package.json index 080a9f54..65bee6d1 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,6 @@ "react-router-dom": "^6.27.0", "reactstrap": "^9.2.3", "redux-localstorage-simple": "^2.5.1", - "uuid": "^10.0.0", "workbox-core": "^7.1.0", "workbox-expiration": "^7.1.0", "workbox-precaching": "^7.1.0", diff --git a/src/utils/utils.ts b/src/utils/utils.ts index d16080cc..b1b11ca0 100644 --- a/src/utils/utils.ts +++ b/src/utils/utils.ts @@ -1,9 +1,6 @@ import type { SyntheticEvent } from 'react'; -import { v4 } from 'uuid'; export const handleEventPreventingDefault = (handler: () => T) => (e: SyntheticEvent) => { e.preventDefault(); handler(); }; - -export const randomUUID = () => v4(); diff --git a/test/servers/helpers/ImportServersBtn.test.tsx b/test/servers/helpers/ImportServersBtn.test.tsx index f28618dc..62a1888c 100644 --- a/test/servers/helpers/ImportServersBtn.test.tsx +++ b/test/servers/helpers/ImportServersBtn.test.tsx @@ -68,12 +68,15 @@ describe('', () => { { btnName: 'Save anyway',savesDuplicatedServers: true }, { btnName: 'Discard', savesDuplicatedServers: false }, ])('creates expected servers depending on selected option in modal', async ({ btnName, savesDuplicatedServers }) => { - const existingServer: ServerWithId = { + const existingServerData: ServerData = { name: 'existingServer', - id: 'existingserver-s.test', url: 'http://s.test/existingUrl', apiKey: 'existingApiKey', }; + const existingServer: ServerWithId = { + ...existingServerData, + id: 'existingserver-s.test', + }; const newServer: ServerData = { name: 'newServer', url: 'http://s.test/newUrl', apiKey: 'newApiKey' }; const { user } = setUp({}, { [existingServer.id]: existingServer }); @@ -86,7 +89,7 @@ describe('', () => { expect(createServersMock).toHaveBeenCalledWith( savesDuplicatedServers - ? [existingServer, expect.objectContaining(newServer)] + ? [expect.objectContaining(existingServerData), expect.objectContaining(newServer)] : [expect.objectContaining(newServer)], ); expect(onImportMock).toHaveBeenCalledTimes(1); diff --git a/test/servers/reducers/selectedServer.test.ts b/test/servers/reducers/selectedServer.test.ts index fac4b708..8c602052 100644 --- a/test/servers/reducers/selectedServer.test.ts +++ b/test/servers/reducers/selectedServer.test.ts @@ -9,7 +9,6 @@ import { selectedServerReducerCreator, selectServer as selectServerCreator, } from '../../../src/servers/reducers/selectedServer'; -import { randomUUID } from '../../../src/utils/utils'; describe('selectedServerReducer', () => { const dispatch = vi.fn(); @@ -41,7 +40,7 @@ describe('selectedServerReducer', () => { ['latest', MAX_FALLBACK_VERSION, 'latest'], ['%invalid_semver%', MIN_FALLBACK_VERSION, '%invalid_semver%'], ])('dispatches proper actions', async (serverVersion, expectedVersion, expectedPrintableVersion) => { - const id = randomUUID(); + const id = crypto.randomUUID(); const getState = createGetStateMock(id); const expectedSelectedServer = { id, @@ -60,7 +59,7 @@ describe('selectedServerReducer', () => { }); it('dispatches error when health endpoint fails', async () => { - const id = randomUUID(); + const id = crypto.randomUUID(); const getState = createGetStateMock(id); const expectedSelectedServer = fromPartial({ id, serverNotReachable: true }); @@ -73,7 +72,7 @@ describe('selectedServerReducer', () => { }); it('dispatches error when server is not found', async () => { - const id = randomUUID(); + const id = crypto.randomUUID(); const getState = vi.fn(() => fromPartial({ servers: {} })); const expectedSelectedServer: NotFoundServer = { serverNotFound: true }; From 9b891c83faa54124087499e8e04960a97d611758 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 12:11:12 +0100 Subject: [PATCH 06/10] Delete unused FormText component --- src/utils/forms/FormText.tsx | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 src/utils/forms/FormText.tsx diff --git a/src/utils/forms/FormText.tsx b/src/utils/forms/FormText.tsx deleted file mode 100644 index f80b5094..00000000 --- a/src/utils/forms/FormText.tsx +++ /dev/null @@ -1,5 +0,0 @@ -import type { FC, PropsWithChildren } from 'react'; - -export const FormText: FC> = ({ children }) => ( - {children} -); From c83abc6e9aecbe8237f487786017f5d12d96ec0b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 12:11:38 +0100 Subject: [PATCH 07/10] Update Bluesky handle --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e2060830..11610708 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ [![GitHub license](https://img.shields.io/github/license/shlinkio/shlink-web-client.svg?style=flat-square)](https://github.com/shlinkio/shlink-web-client/blob/main/LICENSE) [![Mastodon](https://img.shields.io/mastodon/follow/109329425426175098?color=%236364ff&domain=https%3A%2F%2Ffosstodon.org&label=follow&logo=mastodon&logoColor=white&style=flat-square)](https://fosstodon.org/@shlinkio) -[![Bluesky](https://img.shields.io/badge/follow-shlinkio-0285FF.svg?style=flat-square&logo=bluesky&logoColor=white)](https://bsky.app/profile/shlinkio.bsky.social) +[![Bluesky](https://img.shields.io/badge/follow-shlinkio-0285FF.svg?style=flat-square&logo=bluesky&logoColor=white)](https://bsky.app/profile/shlink.io) [![Paypal Donate](https://img.shields.io/badge/Donate-paypal-blue.svg?style=flat-square&logo=paypal&colorA=cccccc)](https://slnk.to/donate) A ReactJS-based progressive web application for [Shlink](https://shlink.io). From 7a9209af0372edc523ad3013aa298a58f50b7ca4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 12:15:15 +0100 Subject: [PATCH 08/10] Use better names for remoteServers test datasets --- test/servers/reducers/remoteServers.test.ts | 31 +++++++++++++-------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/test/servers/reducers/remoteServers.test.ts b/test/servers/reducers/remoteServers.test.ts index 1fa8562a..3c0cf580 100644 --- a/test/servers/reducers/remoteServers.test.ts +++ b/test/servers/reducers/remoteServers.test.ts @@ -9,8 +9,8 @@ describe('remoteServersReducer', () => { const httpClient = fromPartial({ jsonRequest }); it.each([ - [ - [ + { + serversArray: [ { name: 'acel.me from servers.json', url: 'https://acel.me', @@ -22,7 +22,7 @@ describe('remoteServersReducer', () => { apiKey: '7a531c75-134e-4d5c-86e0-a71b7167b57a', }, ], - { + expectedNewServers: { 'acel.me-from-servers.json-acel.me': { id: 'acel.me-from-servers.json-acel.me', name: 'acel.me from servers.json', @@ -36,9 +36,9 @@ describe('remoteServersReducer', () => { apiKey: '7a531c75-134e-4d5c-86e0-a71b7167b57a', }, }, - ], - [ - [ + }, + { + serversArray: [ { name: 'acel.me from servers.json', url: 'https://acel.me', @@ -53,7 +53,7 @@ describe('remoteServersReducer', () => { apiKey: '7a531c75-134e-4d5c-86e0-a71b7167b57a', }, ], - { + expectedNewServers: { 'acel.me-from-servers.json-acel.me': { id: 'acel.me-from-servers.json-acel.me', name: 'acel.me from servers.json', @@ -66,12 +66,19 @@ describe('remoteServersReducer', () => { url: 'http://localhost:8000', apiKey: '7a531c75-134e-4d5c-86e0-a71b7167b57a', }, + }, - ], - ['', {}], - [{}, {}], - ])('tries to fetch servers from remote', async (mockedValue, expectedNewServers) => { - jsonRequest.mockResolvedValue(mockedValue); + }, + { + serversArray: '', + expectedNewServers: {}, + }, + { + serversArray: {}, + expectedNewServers: {}, + }, + ])('tries to fetch servers from remote', async ({ serversArray, expectedNewServers }) => { + jsonRequest.mockResolvedValue(serversArray); const doFetchServers = fetchServers(httpClient); await doFetchServers()(dispatch, vi.fn(), {}); From 645abea72ae72b22ed1207d20d2ac54c2d7f669f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 12:21:42 +0100 Subject: [PATCH 09/10] Add test for servers helpers --- test/servers/helpers/index.test.ts | 43 ++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 test/servers/helpers/index.test.ts diff --git a/test/servers/helpers/index.test.ts b/test/servers/helpers/index.test.ts new file mode 100644 index 00000000..cd1bd5cb --- /dev/null +++ b/test/servers/helpers/index.test.ts @@ -0,0 +1,43 @@ +import { fromPartial } from '@total-typescript/shoehorn'; +import type { ServersMap } from '../../../src/servers/data'; +import { ensureUniqueIds } from '../../../src/servers/helpers'; + +describe('index', () => { + describe('ensureUniqueIds', () => { + const servers: ServersMap = { + 'the-name-example.com': fromPartial({}), + 'another-name-example.com': fromPartial({}), + 'short-domain-s.test': fromPartial({}), + }; + + it('returns expected list of servers when existing IDs conflict', () => { + const result = ensureUniqueIds(servers, [ + fromPartial({ name: 'The name', url: 'https://example.com' }), + fromPartial({ name: 'Short domain', url: 'https://s.test' }), + fromPartial({ name: 'The name', url: 'https://example.com' }), + ]); + + expect(result).toEqual([ + expect.objectContaining({ id: 'the-name-example.com-1' }), + expect.objectContaining({ id: 'short-domain-s.test-1' }), + expect.objectContaining({ id: 'the-name-example.com-2' }), + ]); + }); + + it('returns expected list of servers when IDs conflict in provided list of servers', () => { + const result = ensureUniqueIds(servers, [ + fromPartial({ name: 'Foo', url: 'https://example.com' }), + fromPartial({ name: 'Bar', url: 'https://s.test' }), + fromPartial({ name: 'Foo', url: 'https://example.com' }), + fromPartial({ name: 'Baz', url: 'https://s.test' }), + ]); + + expect(result).toEqual([ + expect.objectContaining({ id: 'foo-example.com' }), + expect.objectContaining({ id: 'bar-s.test' }), + expect.objectContaining({ id: 'foo-example.com-1' }), + expect.objectContaining({ id: 'baz-s.test' }), + ]); + }); + }); +}); From b31949b468d02525aa3fd70ff1a9a75a2c9575b3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 12:48:54 +0100 Subject: [PATCH 10/10] Ensure generating server IDs work even if server URLs are invalid --- CHANGELOG.md | 21 +++++++++++++++++++++ src/servers/helpers/index.ts | 20 +++++++++++++++++--- test/servers/helpers/index.test.ts | 26 ++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e546260..b94d351b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,27 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] +### Added +* [#1360](https://github.com/shlinkio/shlink-web-client/issues/1360) Added ability for server IDs to be generated based on the server name and URL, instead of generating a random UUID. + + This can improve sharing a predefined set of servers cia servers.json, env vars, or simply export and import your servers in some other device, and then be able to share server URLs which continue working. + + All existing servers will keep their generated IDs in existing devices for backwards compatibility, but newly created servers will use the new approach. + +### Changed +* *Nothing* + +### Deprecated +* *Nothing* + +### Removed +* *Nothing* + +### Fixed +* *Nothing* + + ## [4.2.2] - 2024-10-19 ### Added * *Nothing* diff --git a/src/servers/helpers/index.ts b/src/servers/helpers/index.ts index 589b93c7..6a5dca4a 100644 --- a/src/servers/helpers/index.ts +++ b/src/servers/helpers/index.ts @@ -6,9 +6,23 @@ import type { ServerData, ServersMap, ServerWithId } from '../data'; * in lowercase and replacing invalid URL characters with hyphens. */ function idForServer(server: ServerData): string { - // TODO Handle invalid URLs. If not valid url, use the value as is - const url = new URL(server.url); - return `${server.name} ${url.host}`.toLowerCase().replace(/[^a-zA-Z0-9-_.~]/g, '-'); + let urlSegment = server.url; + try { + const { host, pathname } = new URL(urlSegment); + urlSegment = host; + + // Remove leading slash from pathname + const normalizedPathname = pathname.substring(1); + + // Include pathname in the ID, if not empty + if (normalizedPathname.length > 0) { + urlSegment = `${urlSegment} ${normalizedPathname}`; + } + } catch { + // If the server URL is not valid, use the value as is + } + + return `${server.name} ${urlSegment}`.toLowerCase().replace(/[^a-zA-Z0-9-_.~]/g, '-'); } export function serversListToMap(servers: ServerWithId[]): ServersMap { diff --git a/test/servers/helpers/index.test.ts b/test/servers/helpers/index.test.ts index cd1bd5cb..370f6c6c 100644 --- a/test/servers/helpers/index.test.ts +++ b/test/servers/helpers/index.test.ts @@ -39,5 +39,31 @@ describe('index', () => { expect.objectContaining({ id: 'baz-s.test' }), ]); }); + + it('includes server paths when not empty', () => { + const result = ensureUniqueIds({}, [ + fromPartial({ name: 'Foo', url: 'https://example.com' }), + fromPartial({ name: 'Bar', url: 'https://s.test/some/path' }), + fromPartial({ name: 'Baz', url: 'https://s.test/some/other-path-here/123' }), + ]); + + expect(result).toEqual([ + expect.objectContaining({ id: 'foo-example.com' }), + expect.objectContaining({ id: 'bar-s.test-some-path' }), + expect.objectContaining({ id: 'baz-s.test-some-other-path-here-123' }), + ]); + }); + + it('uses server URL verbatim when it is not a valid URL', () => { + const result = ensureUniqueIds({}, [ + fromPartial({ name: 'Foo', url: 'invalid' }), + fromPartial({ name: 'Bar', url: 'this is not a URL' }), + ]); + + expect(result).toEqual([ + expect.objectContaining({ id: 'foo-invalid' }), + expect.objectContaining({ id: 'bar-this-is-not-a-url' }), + ]); + }); }); });