Merge pull request #746 from acelaya-forks/feature/rename-tag-fix

Feature/rename tag fix

src/api/services/ShlinkApiClient.ts

@@ -24,11 +24,9 @@ import { HttpClient } from '../../common/services/HttpClient';
 
 const buildShlinkBaseUrl = (url: string, version: 2 | 3) => `${url}/rest/v${version}`;
 const rejectNilProps = reject(isNil);
-const normalizeOrderByInParams = (params: ShlinkShortUrlsListParams): ShlinkShortUrlsListNormalizedParams => {
-  const { orderBy = {}, ...rest } = params;
-
-  return { ...rest, orderBy: orderToString(orderBy) };
-};
+const normalizeOrderByInParams = (
+  { orderBy = {}, ...rest }: ShlinkShortUrlsListParams,
+): ShlinkShortUrlsListNormalizedParams => ({ ...rest, orderBy: orderToString(orderBy) });
 
 export class ShlinkApiClient {
   private apiVersion: 2 | 3;
@@ -47,7 +45,6 @@ export class ShlinkApiClient {
 
   public readonly createShortUrl = async (options: ShortUrlData): Promise<ShortUrl> => {
     const filteredOptions = reject((value) => isEmpty(value) || isNil(value), options as any);
-
     return this.performRequest<ShortUrl>('/short-urls', 'POST', {}, filteredOptions);
   };
 
@@ -56,31 +53,25 @@ export class ShlinkApiClient {
       .then(({ visits }) => visits);
 
   public readonly getTagVisits = async (tag: string, query?: Omit<ShlinkVisitsParams, 'domain'>): Promise<ShlinkVisits> =>
-    this.performRequest<{ visits: ShlinkVisits }>(`/tags/${tag}/visits`, 'GET', query)
-      .then(({ visits }) => visits);
+    this.performRequest<{ visits: ShlinkVisits }>(`/tags/${tag}/visits`, 'GET', query).then(({ visits }) => visits);
 
   public readonly getDomainVisits = async (domain: string, query?: Omit<ShlinkVisitsParams, 'domain'>): Promise<ShlinkVisits> =>
-    this.performRequest<{ visits: ShlinkVisits }>(`/domains/${domain}/visits`, 'GET', query)
-      .then(({ visits }) => visits);
+    this.performRequest<{ visits: ShlinkVisits }>(`/domains/${domain}/visits`, 'GET', query).then(({ visits }) => visits);
 
   public readonly getOrphanVisits = async (query?: Omit<ShlinkVisitsParams, 'domain'>): Promise<ShlinkVisits> =>
-    this.performRequest<{ visits: ShlinkVisits }>('/visits/orphan', 'GET', query)
-      .then(({ visits }) => visits);
+    this.performRequest<{ visits: ShlinkVisits }>('/visits/orphan', 'GET', query).then(({ visits }) => visits);
 
   public readonly getNonOrphanVisits = async (query?: Omit<ShlinkVisitsParams, 'domain'>): Promise<ShlinkVisits> =>
-    this.performRequest<{ visits: ShlinkVisits }>('/visits/non-orphan', 'GET', query)
-      .then(({ visits }) => visits);
+    this.performRequest<{ visits: ShlinkVisits }>('/visits/non-orphan', 'GET', query).then(({ visits }) => visits);
 
   public readonly getVisitsOverview = async (): Promise<ShlinkVisitsOverview> =>
-    this.performRequest<{ visits: ShlinkVisitsOverview }>('/visits')
-      .then(({ visits }) => visits);
+    this.performRequest<{ visits: ShlinkVisitsOverview }>('/visits').then(({ visits }) => visits);
 
   public readonly getShortUrl = async (shortCode: string, domain?: OptionalString): Promise<ShortUrl> =>
     this.performRequest<ShortUrl>(`/short-urls/${shortCode}`, 'GET', { domain });
 
   public readonly deleteShortUrl = async (shortCode: string, domain?: OptionalString): Promise<void> =>
-    this.performRequest(`/short-urls/${shortCode}`, 'DELETE', { domain })
-      .then(() => {});
+    this.performEmptyRequest(`/short-urls/${shortCode}`, 'DELETE', { domain });
 
   public readonly updateShortUrl = async (
     shortCode: string,
@@ -95,12 +86,10 @@ export class ShlinkApiClient {
       .then(({ data, stats }) => ({ tags: data, stats }));
 
   public readonly deleteTags = async (tags: string[]): Promise<{ tags: string[] }> =>
-    this.performRequest('/tags', 'DELETE', { tags })
-      .then(() => ({ tags }));
+    this.performEmptyRequest('/tags', 'DELETE', { tags }).then(() => ({ tags }));
 
   public readonly editTag = async (oldName: string, newName: string): Promise<{ oldName: string; newName: string }> =>
-    this.performRequest('/tags', 'PUT', {}, { oldName, newName })
-      .then(() => ({ oldName, newName }));
+    this.performEmptyRequest('/tags', 'PUT', {}, { oldName, newName }).then(() => ({ oldName, newName }));
 
   public readonly health = async (): Promise<ShlinkHealth> => this.performRequest<ShlinkHealth>('/health', 'GET');
 
@@ -115,26 +104,35 @@ export class ShlinkApiClient {
   ): Promise<ShlinkDomainRedirects> =>
     this.performRequest<ShlinkDomainRedirects>('/domains/redirects', 'PATCH', {}, domainRedirects);
 
-  private readonly performRequest = async <T>(url: string, method = 'GET', query = {}, body?: object): Promise<T> => {
+  private readonly performRequest = async <T>(url: string, method = 'GET', query = {}, body?: object): Promise<T> =>
+    this.httpClient.fetchJson<T>(...this.toFetchParams(url, method, query, body)).catch(
+      this.handleFetchError(() => this.httpClient.fetchJson<T>(...this.toFetchParams(url, method, query, body))),
+    );
+
+  private readonly performEmptyRequest = async (url: string, method = 'GET', query = {}, body?: object): Promise<void> =>
+    this.httpClient.fetchEmpty(...this.toFetchParams(url, method, query, body)).catch(
+      this.handleFetchError(() => this.httpClient.fetchEmpty(...this.toFetchParams(url, method, query, body))),
+    );
+
+  private readonly toFetchParams = (url: string, method: string, query = {}, body?: object): [string, RequestInit] => {
     const normalizedQuery = stringifyQuery(rejectNilProps(query));
     const stringifiedQuery = isEmpty(normalizedQuery) ? '' : `?${normalizedQuery}`;
 
-    return this.httpClient.fetchJson<T>(
-      `${buildShlinkBaseUrl(this.baseUrl, this.apiVersion)}${url}${stringifiedQuery}`,
-      {
-        method,
-        body: body && JSON.stringify(body),
-        headers: { 'X-Api-Key': this.apiKey },
-      },
-    ).catch((e: unknown) => {
-      if (!isRegularNotFound(parseApiError(e))) {
-        throw e;
-      }
+    return [`${buildShlinkBaseUrl(this.baseUrl, this.apiVersion)}${url}${stringifiedQuery}`, {
+      method,
+      body: body && JSON.stringify(body),
+      headers: { 'X-Api-Key': this.apiKey },
+    }];
+  };
 
-      // If we capture a not found error, let's assume this Shlink version does not support API v3, so we decrease to
-      // v2 and retry
-      this.apiVersion = 2;
-      return this.performRequest(url, method, query, body);
-    });
+  private readonly handleFetchError = (retryFetch: Function) => (e: unknown) => {
+    if (!isRegularNotFound(parseApiError(e))) {
+      throw e;
+    }
+
+    // If we capture a not found error, let's assume this Shlink version does not support API v3, so we decrease to
+    // v2 and retry
+    this.apiVersion = 2;
+    return retryFetch();
   };
 }

src/common/services/HttpClient.ts

@@ -3,19 +3,23 @@ import { Fetch } from '../../utils/types';
 export class HttpClient {
   constructor(private readonly fetch: Fetch) {}
 
-  public fetchJson<T>(url: string, options?: RequestInit): Promise<T> {
-    return this.fetch(url, options).then(async (resp) => {
-      const parsed = await resp.json();
+  public readonly fetchJson = <T>(url: string, options?: RequestInit): Promise<T> =>
+    this.fetch(url, options).then(async (resp) => {
+      const json = await resp.json();
 
       if (!resp.ok) {
-        throw parsed;
+        throw json;
       }
 
-      return parsed as T;
+      return json as T;
     });
-  }
 
-  public fetchBlob(url: string): Promise<Blob> {
-    return this.fetch(url).then((resp) => resp.blob());
-  }
+  public readonly fetchEmpty = (url: string, options?: RequestInit): Promise<void> =>
+    this.fetch(url, options).then(async (resp) => {
+      if (!resp.ok) {
+        throw await resp.json();
+      }
+    });
+
+  public readonly fetchBlob = (url: string): Promise<Blob> => this.fetch(url).then((resp) => resp.blob());
 }

src/index.scss

@@ -39,6 +39,10 @@ a:not(.nav-link):not(.navbar-brand):not(.page-link):not(.highlight-card):not(.bt
   background-color: $mainColor !important;
 }
 
+.bg-warning {
+  color: $lightTextColor;
+}
+
 .card-body,
 .card-header,
 .list-group-item {

src/tags/helpers/EditTagModal.tsx

@@ -27,9 +27,10 @@ export const EditTagModal = ({ getColorForKey }: ColorGenerator) => (
   const [showColorPicker, toggleColorPicker, , hideColorPicker] = useToggle();
   const { editing, error, edited, errorData } = tagEdit;
   const saveTag = handleEventPreventingDefault(
-    async () => editTag({ oldName: tag, newName: newTagName, color })
-      .then(toggle)
-      .catch(() => {}),
+    async () => {
+      await editTag({ oldName: tag, newName: newTagName, color });
+      toggle();
+    },
   );
   const onClosed = pipe(hideColorPicker, () => edited && tagEdited({ oldName: tag, newName: newTagName, color }));
 

src/tags/reducers/tagEdit.ts

@@ -33,35 +33,34 @@ const initialState: TagEdition = {
 
 export const tagEdited = createAction<EditTag>(`${REDUCER_PREFIX}/tagEdited`);
 
-export const tagEditReducerCreator = (buildShlinkApiClient: ShlinkApiClientBuilder, colorGenerator: ColorGenerator) => {
-  const editTag = createAsyncThunk(
-    `${REDUCER_PREFIX}/editTag`,
-    async ({ oldName, newName, color }: EditTag, { getState }): Promise<EditTag> => {
-      await buildShlinkApiClient(getState).editTag(oldName, newName);
-      colorGenerator.setColorForKey(newName, color);
+export const editTag = (
+  buildShlinkApiClient: ShlinkApiClientBuilder,
+  colorGenerator: ColorGenerator,
+) => createAsyncThunk(
+  `${REDUCER_PREFIX}/editTag`,
+  async ({ oldName, newName, color }: EditTag, { getState }): Promise<EditTag> => {
+    await buildShlinkApiClient(getState).editTag(oldName, newName);
+    colorGenerator.setColorForKey(newName, color);
 
-      return { oldName, newName, color };
-    },
-  );
+    return { oldName, newName, color };
+  },
+);
 
-  const { reducer } = createSlice({
-    name: REDUCER_PREFIX,
-    initialState,
-    reducers: {},
-    extraReducers: (builder) => {
-      builder.addCase(editTag.pending, () => ({ editing: true, edited: false, error: false }));
-      builder.addCase(
-        editTag.rejected,
-        (_, { error }) => ({ editing: false, edited: false, error: true, errorData: parseApiError(error) }),
-      );
-      builder.addCase(editTag.fulfilled, (_, { payload }) => ({
-        ...pick(['oldName', 'newName'], payload),
-        editing: false,
-        edited: true,
-        error: false,
-      }));
-    },
-  });
-
-  return { reducer, editTag };
-};
+export const tagEditReducerCreator = (editTagThunk: ReturnType<typeof editTag>) => createSlice({
+  name: REDUCER_PREFIX,
+  initialState,
+  reducers: {},
+  extraReducers: (builder) => {
+    builder.addCase(editTagThunk.pending, () => ({ editing: true, edited: false, error: false }));
+    builder.addCase(
+      editTagThunk.rejected,
+      (_, { error }) => ({ editing: false, edited: false, error: true, errorData: parseApiError(error) }),
+    );
+    builder.addCase(editTagThunk.fulfilled, (_, { payload }) => ({
+      ...pick(['oldName', 'newName'], payload),
+      editing: false,
+      edited: true,
+      error: false,
+    }));
+  },
+});

src/tags/reducers/tagsList.ts

@@ -111,15 +111,19 @@ export const tagsListReducerCreator = (
       { ...initialState, stats: payload.stats, tags: payload.tags, filteredTags: payload.tags }
     ));
 
-    builder.addCase(tagDeleted, (state, { payload: tag }) => ({
-      ...state,
-      tags: rejectTag(state.tags, tag),
-      filteredTags: rejectTag(state.filteredTags, tag),
+    builder.addCase(tagDeleted, ({ tags, filteredTags, ...rest }, { payload: tag }) => ({
+      ...rest,
+      tags: rejectTag(tags, tag),
+      filteredTags: rejectTag(filteredTags, tag),
     }));
-    builder.addCase(tagEdited, (state, { payload }) => ({
-      ...state,
-      tags: state.tags.map(renameTag(payload.oldName, payload.newName)).sort(),
-      filteredTags: state.filteredTags.map(renameTag(payload.oldName, payload.newName)).sort(),
+    builder.addCase(tagEdited, ({ tags, filteredTags, stats, ...rest }, { payload }) => ({
+      ...rest,
+      stats: {
+        ...stats,
+        [payload.newName]: stats[payload.oldName],
+      },
+      tags: tags.map(renameTag(payload.oldName, payload.newName)).sort(),
+      filteredTags: filteredTags.map(renameTag(payload.oldName, payload.newName)).sort(),
     }));
     builder.addCase(createNewVisits, (state, { payload }) => ({
       ...state,

src/tags/services/provideServices.ts

@@ -7,7 +7,7 @@ import { EditTagModal } from '../helpers/EditTagModal';
 import { TagsList } from '../TagsList';
 import { filterTags, listTags, tagsListReducerCreator } from '../reducers/tagsList';
 import { tagDeleted, tagDeleteReducerCreator } from '../reducers/tagDelete';
-import { tagEdited, tagEditReducerCreator } from '../reducers/tagEdit';
+import { editTag, tagEdited, tagEditReducerCreator } from '../reducers/tagEdit';
 import { ConnectDecorator } from '../../container/types';
 import { TagsCards } from '../TagsCards';
 import { TagsTable } from '../TagsTable';
@@ -38,7 +38,7 @@ const provideServices = (bottle: Bottle, connect: ConnectDecorator) => {
   ));
 
   // Reducers
-  bottle.serviceFactory('tagEditReducerCreator', tagEditReducerCreator, 'buildShlinkApiClient', 'ColorGenerator');
+  bottle.serviceFactory('tagEditReducerCreator', tagEditReducerCreator, 'editTag');
   bottle.serviceFactory('tagEditReducer', prop('reducer'), 'tagEditReducerCreator');
 
   bottle.serviceFactory('tagDeleteReducerCreator', tagDeleteReducerCreator, 'buildShlinkApiClient');
@@ -58,7 +58,7 @@ const provideServices = (bottle: Bottle, connect: ConnectDecorator) => {
   bottle.serviceFactory('deleteTag', prop('deleteTag'), 'tagDeleteReducerCreator');
   bottle.serviceFactory('tagDeleted', () => tagDeleted);
 
-  bottle.serviceFactory('editTag', prop('editTag'), 'tagEditReducerCreator');
+  bottle.serviceFactory('editTag', editTag, 'buildShlinkApiClient', 'ColorGenerator');
   bottle.serviceFactory('tagEdited', () => tagEdited);
 };
 

test/api/services/ShlinkApiClient.test.ts

@@ -8,7 +8,8 @@ import { HttpClient } from '../../../src/common/services/HttpClient';
 
 describe('ShlinkApiClient', () => {
   const fetchJson = jest.fn().mockResolvedValue({});
-  const httpClient = Mock.of<HttpClient>({ fetchJson });
+  const fetchEmpty = jest.fn().mockResolvedValue(undefined);
+  const httpClient = Mock.of<HttpClient>({ fetchJson, fetchEmpty });
   const buildApiClient = () => new ShlinkApiClient(httpClient, '', '');
   const shortCodesWithDomainCombinations: [string, OptionalString][] = [
     ['abc123', null],
@@ -196,7 +197,7 @@ describe('ShlinkApiClient', () => {
 
       await deleteTags(tags);
 
-      expect(fetchJson).toHaveBeenCalledWith(
+      expect(fetchEmpty).toHaveBeenCalledWith(
         expect.stringContaining(`/tags?${tags.map((tag) => `tags%5B%5D=${tag}`).join('&')}`),
         expect.objectContaining({ method: 'DELETE' }),
       );
@@ -211,7 +212,7 @@ describe('ShlinkApiClient', () => {
 
       await editTag(oldName, newName);
 
-      expect(fetchJson).toHaveBeenCalledWith(expect.stringContaining('/tags'), expect.objectContaining({
+      expect(fetchEmpty).toHaveBeenCalledWith(expect.stringContaining('/tags'), expect.objectContaining({
         method: 'PUT',
         body: JSON.stringify({ oldName, newName }),
       }));
@@ -225,7 +226,7 @@ describe('ShlinkApiClient', () => {
 
       await deleteShortUrl(shortCode, domain);
 
-      expect(fetchJson).toHaveBeenCalledWith(
+      expect(fetchEmpty).toHaveBeenCalledWith(
         expect.stringContaining(`/short-urls/${shortCode}${expectedQuery}`),
         expect.objectContaining({ method: 'DELETE' }),
       );

test/common/services/HttpClient.test.ts

@@ -7,14 +7,14 @@ describe('HttpClient', () => {
   beforeEach(jest.clearAllMocks);
 
   describe('fetchJson', () => {
-    it('throws json when response is not ok', async () => {
+    it('throws json on success', async () => {
       const theError = { error: true, foo: 'bar' };
       fetch.mockResolvedValue({ json: () => theError, ok: false });
 
       await expect(httpClient.fetchJson('')).rejects.toEqual(theError);
     });
 
-    it('return json when response is ok', async () => {
+    it('return json on failure', async () => {
       const theJson = { foo: 'bar' };
       fetch.mockResolvedValue({ json: () => theJson, ok: true });
 
@@ -24,6 +24,23 @@ describe('HttpClient', () => {
     });
   });
 
+  describe('fetchEmpty', () => {
+    it('returns empty on success', async () => {
+      fetch.mockResolvedValue({ ok: true });
+
+      const result = await httpClient.fetchEmpty('');
+
+      expect(result).not.toBeDefined();
+    });
+
+    it('throws error on failure', async () => {
+      const theError = { error: true, foo: 'bar' };
+      fetch.mockResolvedValue({ json: () => theError, ok: false });
+
+      await expect(httpClient.fetchJson('')).rejects.toEqual(theError);
+    });
+  });
+
   describe('fetchBlob', () => {
     it('returns response as blob', async () => {
       const theBlob = new Blob();

test/tags/reducers/tagEdit.test.ts

@@ -1,5 +1,5 @@
 import { Mock } from 'ts-mockery';
-import { tagEdited, EditTagAction, tagEditReducerCreator } from '../../../src/tags/reducers/tagEdit';
+import { tagEdited, editTag as editTagCreator, EditTagAction, tagEditReducerCreator } from '../../../src/tags/reducers/tagEdit';
 import { ShlinkApiClient } from '../../../src/api/services/ShlinkApiClient';
 import { ColorGenerator } from '../../../src/utils/services/ColorGenerator';
 import { ShlinkState } from '../../../src/container/types';
@@ -11,7 +11,8 @@ describe('tagEditReducer', () => {
   const editTagCall = jest.fn();
   const buildShlinkApiClient = () => Mock.of<ShlinkApiClient>({ editTag: editTagCall });
   const colorGenerator = Mock.of<ColorGenerator>({ setColorForKey: jest.fn() });
-  const { reducer, editTag } = tagEditReducerCreator(buildShlinkApiClient, colorGenerator);
+  const editTag = editTagCreator(buildShlinkApiClient, colorGenerator);
+  const { reducer } = tagEditReducerCreator(editTag);
 
   describe('reducer', () => {
     it('returns loading on EDIT_TAG_START', () => {

test/tags/reducers/tagsList.test.ts

@@ -70,14 +70,30 @@ describe('tagsListReducer', () => {
       const expectedTags = ['foo', 'renamed', 'baz'].sort();
 
       expect(reducer(
-        state({ tags, filteredTags: tags }),
-        {
-          type: tagEdited.toString(),
-          payload: { oldName, newName },
-        },
+        state({
+          tags,
+          filteredTags: tags,
+          stats: {
+            [oldName]: {
+              shortUrlsCount: 35,
+              visitsCount: 35,
+            },
+          },
+        }),
+        { type: tagEdited.toString(), payload: { oldName, newName } },
       )).toEqual({
         tags: expectedTags,
         filteredTags: expectedTags,
+        stats: {
+          [oldName]: {
+            shortUrlsCount: 35,
+            visitsCount: 35,
+          },
+          [newName]: {
+            shortUrlsCount: 35,
+            visitsCount: 35,
+          },
+        },
       });
     });