[APM] Link preview breaks when editing a custom link (#61053) (#61437)

* refactoring custom link server side * refactoring custom link server side * fixing pr comments * fixing unit test * fixing unit tests * renaming server directory Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
elastic · Mar 26, 2020 · 5a06fc8 · 5a06fc8
1 parent 58a50b7
commit 5a06fc8
Show file tree

Hide file tree

Showing 34 changed files with 556 additions and 397 deletions.
diff --git a/...public/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/FiltersSection.tsx b/...public/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/FiltersSection.tsx
@@ -16,10 +16,12 @@ import {
 import { i18n } from '@kbn/i18n';
 import { isEmpty } from 'lodash';
 import React from 'react';
-import { FilterOptions } from '../../../../../../../../../../plugins/apm/common/custom_link_filter_options';
+import {
+  Filter,
+  FilterKey
+} from '../../../../../../../../../../plugins/apm/common/custom_link/custom_link_types';
 import {
   DEFAULT_OPTION,
-  FilterKeyValue,
   FILTER_SELECT_OPTIONS,
   getSelectOptions
 } from './helper';
@@ -28,12 +30,16 @@ export const FiltersSection = ({
   filters,
   onChangeFilters
 }: {
-  filters: FilterKeyValue[];
-  onChangeFilters: (filters: FilterKeyValue[]) => void;
+  filters: Filter[];
+  onChangeFilters: (filters: Filter[]) => void;
 }) => {
-  const onChangeFilter = (filter: FilterKeyValue, idx: number) => {
+  const onChangeFilter = (
+    key: Filter['key'],
+    value: Filter['value'],
+    idx: number
+  ) => {
     const newFilters = [...filters];
-    newFilters[idx] = filter;
+    newFilters[idx] = { key, value };
     onChangeFilters(newFilters);
   };
 
@@ -45,14 +51,14 @@ export const FiltersSection = ({
     // if there is only one item left it should not be removed
     // but reset to empty
     if (isEmpty(newFilters)) {
-      onChangeFilters([['', '']]);
+      onChangeFilters([{ key: '', value: '' }]);
     } else {
       onChangeFilters(newFilters);
     }
   };
 
   const handleAddFilter = () => {
-    onChangeFilters([...filters, ['', '']]);
+    onChangeFilters([...filters, { key: '', value: '' }]);
   };
 
   return (
@@ -81,7 +87,7 @@ export const FiltersSection = ({
       <EuiSpacer size="s" />
 
       {filters.map((filter, idx) => {
-        const [key, value] = filter;
+        const { key, value } = filter;
         const filterId = `filter-${idx}`;
         const selectOptions = getSelectOptions(filters, key);
         return (
@@ -100,10 +106,7 @@ export const FiltersSection = ({
                   }
                 )}
                 onChange={e =>
-                  onChangeFilter(
-                    [e.target.value as keyof FilterOptions, value],
-                    idx
-                  )
+                  onChangeFilter(e.target.value as FilterKey, value, idx)
                 }
                 isInvalid={
                   !isEmpty(value) &&
@@ -119,7 +122,7 @@ export const FiltersSection = ({
                   'xpack.apm.settings.customizeUI.customLink.flyOut.filters.defaultOption.value',
                   { defaultMessage: 'Value' }
                 )}
-                onChange={e => onChangeFilter([key, e.target.value], idx)}
+                onChange={e => onChangeFilter(key, e.target.value, idx)}
                 value={value}
                 isInvalid={!isEmpty(key) && isEmpty(value)}
               />

diff --git a/...blic/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/LinkPreview.test.tsx b/...blic/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/LinkPreview.test.tsx
@@ -5,7 +5,7 @@
  */
 import React from 'react';
 import { LinkPreview } from '../CustomLinkFlyout/LinkPreview';
-import { render, getNodeText, getByTestId } from '@testing-library/react';
+import { render, getNodeText, getByTestId, act } from '@testing-library/react';
 
 describe('LinkPreview', () => {
   const getElementValue = (container: HTMLElement, id: string) =>
@@ -15,37 +15,47 @@ describe('LinkPreview', () => {
     );
 
   it('shows label and url default values', () => {
-    const { container } = render(
-      <LinkPreview label="" url="" filters={[['', '']]} />
-    );
-    expect(getElementValue(container, 'preview-label')).toEqual('Elastic.co');
-    expect(getElementValue(container, 'preview-url')).toEqual(
-      'https://www.elastic.co'
-    );
+    act(() => {
+      const { container } = render(
+        <LinkPreview label="" url="" filters={[{ key: '', value: '' }]} />
+      );
+      expect(getElementValue(container, 'preview-label')).toEqual('Elastic.co');
+      expect(getElementValue(container, 'preview-url')).toEqual(
+        'https://www.elastic.co'
+      );
+    });
   });
 
   it('shows label and url values', () => {
-    const { container } = render(
-      <LinkPreview label="foo" url="https://baz.co" filters={[['', '']]} />
-    );
-    expect(getElementValue(container, 'preview-label')).toEqual('foo');
-    expect(
-      (getByTestId(container, 'preview-link') as HTMLAnchorElement).text
-    ).toEqual('https://baz.co');
+    act(() => {
+      const { container } = render(
+        <LinkPreview
+          label="foo"
+          url="https://baz.co"
+          filters={[{ key: '', value: '' }]}
+        />
+      );
+      expect(getElementValue(container, 'preview-label')).toEqual('foo');
+      expect(
+        (getByTestId(container, 'preview-link') as HTMLAnchorElement).text
+      ).toEqual('https://baz.co');
+    });
   });
 
   it('shows warning when couldnt replace context variables', () => {
-    const { container } = render(
-      <LinkPreview
-        label="foo"
-        url="https://baz.co?service.name={{invalid}"
-        filters={[['', '']]}
-      />
-    );
-    expect(getElementValue(container, 'preview-label')).toEqual('foo');
-    expect(
-      (getByTestId(container, 'preview-link') as HTMLAnchorElement).text
-    ).toEqual('https://baz.co?service.name={{invalid}');
-    expect(getByTestId(container, 'preview-warning')).toBeInTheDocument();
+    act(() => {
+      const { container } = render(
+        <LinkPreview
+          label="foo"
+          url="https://baz.co?service.name={{invalid}"
+          filters={[{ key: '', value: '' }]}
+        />
+      );
+      expect(getElementValue(container, 'preview-label')).toEqual('foo');
+      expect(
+        (getByTestId(container, 'preview-link') as HTMLAnchorElement).text
+      ).toEqual('https://baz.co?service.name={{invalid}');
+      expect(getByTestId(container, 'preview-warning')).toBeInTheDocument();
+    });
   });
 });
diff --git a/...pm/public/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/LinkPreview.tsx b/...pm/public/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/LinkPreview.tsx
@@ -17,28 +17,22 @@ import {
 } from '@elastic/eui';
 import { i18n } from '@kbn/i18n';
 import { debounce } from 'lodash';
+import { Filter } from '../../../../../../../../../../plugins/apm/common/custom_link/custom_link_types';
 import { Transaction } from '../../../../../../../../../../plugins/apm/typings/es_schemas/ui/transaction';
 import { callApmApi } from '../../../../../../services/rest/createCallApmApi';
-import {
-  FilterKeyValue,
-  convertFiltersToObject,
-  replaceTemplateVariables
-} from './helper';
+import { replaceTemplateVariables, convertFiltersToQuery } from './helper';
 
 interface Props {
   label: string;
   url: string;
-  filters: FilterKeyValue[];
+  filters: Filter[];
 }
 
 const fetchTransaction = debounce(
-  async (
-    filters: FilterKeyValue[],
-    callback: (transaction: Transaction) => void
-  ) => {
+  async (filters: Filter[], callback: (transaction: Transaction) => void) => {
     const transaction = await callApmApi({
       pathname: '/api/apm/settings/custom_links/transaction',
-      params: { query: convertFiltersToObject(filters) }
+      params: { query: convertFiltersToQuery(filters) }
     });
     callback(transaction);
   },
@@ -51,7 +45,20 @@ export const LinkPreview = ({ label, url, filters }: Props) => {
   const [transaction, setTransaction] = useState<Transaction | undefined>();
 
   useEffect(() => {
-    fetchTransaction(filters, setTransaction);
+    /*
+      React throwns "Can't perform a React state update on an unmounted component"
+      It happens when the Custom Link flyout is closed before the return of the api request.
+      To avoid such case, sets the isUnmounted to true when component unmount and check its value before update the transaction.
+    */
+    let isUnmounted = false;
+    fetchTransaction(filters, (_transaction: Transaction) => {
+      if (!isUnmounted) {
+        setTransaction(_transaction);
+      }
+    });
+    return () => {
+      isUnmounted = true;
+    };
   }, [filters]);
 
   const { formattedUrl, error } = replaceTemplateVariables(url, transaction);

diff --git a/...pm/public/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/LinkSection.tsx b/...pm/public/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/LinkSection.tsx
@@ -12,7 +12,7 @@ import {
 } from '@elastic/eui';
 import { i18n } from '@kbn/i18n';
 import React from 'react';
-import { CustomLink } from '../../../../../../../../../../plugins/apm/server/lib/settings/custom_link/custom_link_types';
+import { CustomLink } from '../../../../../../../../../../plugins/apm/common/custom_link/custom_link_types';
 import { Documentation } from './Documentation';
 
 interface InputField {

diff --git a/...apm/public/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/helper.test.ts b/...apm/public/components/app/Settings/CustomizeUI/CustomLink/CustomLinkFlyout/helper.test.ts
@@ -4,79 +4,21 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 import {
-  convertFiltersToArray,
-  convertFiltersToObject,
   getSelectOptions,
   replaceTemplateVariables
 } from '../CustomLinkFlyout/helper';
-import { CustomLink } from '../../../../../../../../../../plugins/apm/server/lib/settings/custom_link/custom_link_types';
 import { Transaction } from '../../../../../../../../../../plugins/apm/typings/es_schemas/ui/transaction';
 
 describe('Custom link helper', () => {
-  describe('convertFiltersToArray', () => {
-    it('returns array of tuple when custom link not defined', () => {
-      expect(convertFiltersToArray()).toEqual([['', '']]);
-    });
-    it('returns filters as array', () => {
-      expect(
-        convertFiltersToArray({
-          'service.name': 'foo',
-          'transaction.type': 'bar'
-        } as CustomLink)
-      ).toEqual([
-        ['service.name', 'foo'],
-        ['transaction.type', 'bar']
-      ]);
-    });
-    it('returns empty when no filter is added', () => {
-      expect(
-        convertFiltersToArray({
-          label: 'foo',
-          url: 'bar'
-        } as CustomLink)
-      ).toEqual([['', '']]);
-    });
-  });
-
-  describe('convertFiltersToObject', () => {
-    it('returns undefined when any filter is added', () => {
-      expect(convertFiltersToObject([['', '']])).toBeUndefined();
-    });
-    it('removes uncompleted filters', () => {
-      expect(
-        convertFiltersToObject([
-          ['service.name', ''],
-          ['', 'foo'],
-          ['transaction.type', 'bar']
-        ])
-      ).toEqual({ 'transaction.type': ['bar'] });
-    });
-    it('splits the value by comma', () => {
-      expect(
-        convertFiltersToObject([
-          ['service.name', 'foo'],
-          ['service.environment', 'foo, bar'],
-          ['transaction.type', 'foo, '],
-          ['transaction.name', 'foo,']
-        ])
-      ).toEqual({
-        'service.name': ['foo'],
-        'service.environment': ['foo', 'bar'],
-        'transaction.type': ['foo'],
-        'transaction.name': ['foo']
-      });
-    });
-  });
-
   describe('getSelectOptions', () => {
     it('returns all available options when no filters were selected', () => {
       expect(
         getSelectOptions(
           [
-            ['', ''],
-            ['', ''],
-            ['', ''],
-            ['', '']
+            { key: '', value: '' },
+            { key: '', value: '' },
+            { key: '', value: '' },
+            { key: '', value: '' }
           ],
           ''
         )
@@ -92,10 +34,10 @@ describe('Custom link helper', () => {
       expect(
         getSelectOptions(
           [
-            ['service.name', 'foo'],
-            ['', ''],
-            ['', ''],
-            ['', '']
+            { key: 'service.name', value: 'foo' },
+            { key: '', value: '' },
+            { key: '', value: '' },
+            { key: '', value: '' }
           ],
           ''
         )
@@ -110,10 +52,10 @@ describe('Custom link helper', () => {
       expect(
         getSelectOptions(
           [
-            ['service.name', 'foo'],
-            ['transaction.name', 'bar'],
-            ['', ''],
-            ['', '']
+            { key: 'service.name', value: 'foo' },
+            { key: 'transaction.name', value: 'bar' },
+            { key: '', value: '' },
+            { key: '', value: '' }
           ],
           'transaction.name'
         )
@@ -128,10 +70,10 @@ describe('Custom link helper', () => {
       expect(
         getSelectOptions(
           [
-            ['service.name', 'foo'],
-            ['transaction.name', 'bar'],
-            ['service.environment', 'baz'],
-            ['transaction.type', 'qux']
+            { key: 'service.name', value: 'foo' },
+            { key: 'transaction.name', value: 'bar' },
+            { key: 'service.environment', value: 'baz' },
+            { key: 'transaction.type', value: 'qux' }
           ],
           ''
         )