Update CreateServer logic so that it ensures a unique human-friendly …

…ID is set

src/app/App.tsx

@@ -50,8 +50,8 @@ const App: FCWithDeps<AppProps, AppDeps> = (
   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();
     }

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<CreateServerProps, CreateServerDeps> = ({ servers
   const [errorImporting, setErrorImporting] = useTimeoutToggle(false, SHOW_IMPORT_MSG_TIME);
   const [isConfirmModalOpen, toggleConfirmModal] = useToggle();
   const [serverData, setServerData] = useState<ServerData>();
-  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);
 

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<ImportServersBtnConnectProps, ImportServersBt
   const [duplicatedServers, setDuplicatedServers] = useState<ServerData[]>([]);
   const [isModalOpen,, showModal, hideModal] = useToggle();
 
-  const importedServersRef = useRef<ServerData[]>([]);
-  const newServersRef = useRef<ServerData[]>([]);
+  const importedServersRef = useRef<ServerWithId[]>([]);
+  const newServersRef = useRef<ServerWithId[]>([]);
 
-  const create = useCallback((serversData: ServerData[]) => {
+  const create = useCallback((serversData: ServerWithId[]) => {
     createServers(serversData);
     onImport();
   }, [createServers, onImport]);
@@ -54,11 +54,11 @@ const ImportServersBtn: FCWithDeps<ImportServersBtnConnectProps, ImportServersBt
         .then((importedServers) => {
           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();

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<ServersMap>(
-    (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;
+}

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',

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<ServersMap>) => ({ ...state, ...newServers }),
     },
   },

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('<ImportServersBtn />', () => {
   });
 
   it.each([
-    ['Save anyway', true],
-    ['Discard', false],
-  ])('creates expected servers depending on selected option in modal', async (btnName, savesDuplicatedServers) => {
-    const existingServer = fromPartial<ServerWithId>({ id: 'abc', url: 'existingUrl', apiKey: 'existingApiKey' });
-    const newServer = fromPartial<ServerWithId>({ 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);
   });
 });