From e786f9d21fb0a2576f0ed4cd631356fecf6f770d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 1 Nov 2024 10:27:35 +0100 Subject: [PATCH] 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); }); });