[Chore] Refactor and improve Discover field summaries (opensearch-pro…

…ject#2391) * [Chore] Refactor and improve field summaries * Convert to typescript * Fix types * Add tests Signed-off-by: Josh Romero <rmerqg@amazon.com> * [Test] Update functional test Groups are now naturally sorted by key, which requires selecting a different date filter Signed-off-by: Josh Romero <rmerqg@amazon.com> * [Chore] Add changelog entry Signed-off-by: Josh Romero <rmerqg@amazon.com> * [Chore] Refactor columns passing, fix bugs * pass columns explicitly as props * fix branding in core mocks * fix `toBeUndefined()` usage in tests * remove leftover comment * fix test subject * condense types Signed-off-by: Josh Romero <rmerqg@amazon.com> Signed-off-by: Josh Romero <rmerqg@amazon.com> Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
Arpit-Bandejiya · Jan 13, 2023 · 40a08cb · 40a08cb
1 parent f73ec7b
commit 40a08cb
Show file tree

Hide file tree

Showing 14 changed files with 526 additions and 266 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -68,7 +68,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Adding @zhongnansu as maintainer. ([#2590](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2590))
 
 ### 🪛 Refactoring
-* [MD] Refactor data source error handling ([#2661](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2661))
+
+- [MD] Refactor data source error handling ([#2661](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2661))
+- Refactor and improve Discover field summaries ([#2391](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2391))
 
 ### 🔩 Tests
 

diff --git a/src/core/public/mocks.ts b/src/core/public/mocks.ts
@@ -83,6 +83,7 @@ function createCoreSetupMock({
     uiSettings: uiSettingsServiceMock.createSetupContract(),
     injectedMetadata: {
       getInjectedVar: injectedMetadataServiceMock.createSetupContract().getInjectedVar,
+      getBranding: injectedMetadataServiceMock.createSetupContract().getBranding,
     },
   };
 

diff --git a/src/plugins/discover/public/application/components/sidebar/discover_field.test.tsx b/src/plugins/discover/public/application/components/sidebar/discover_field.test.tsx
@@ -29,6 +29,7 @@
  */
 
 import React from 'react';
+// @ts-ignore
 import { findTestSubject } from '@elastic/eui/lib/test';
 // @ts-ignore
 import stubbedLogstashFields from 'fixtures/logstash_fields';
@@ -99,8 +100,9 @@ function getComponent({
 
   const props = {
     indexPattern,
+    columns: [],
     field: finalField,
-    getDetails: jest.fn(() => ({ buckets: [], error: '', exists: 1, total: true, columns: [] })),
+    getDetails: jest.fn(() => ({ buckets: [], error: '', exists: 1, total: 1 })),
     onAddFilter: jest.fn(),
     onAddField: jest.fn(),
     onRemoveField: jest.fn(),

diff --git a/src/plugins/discover/public/application/components/sidebar/discover_field.tsx b/src/plugins/discover/public/application/components/sidebar/discover_field.tsx
@@ -40,6 +40,10 @@ import { getFieldTypeName } from './lib/get_field_type_name';
 import './discover_field.scss';
 
 export interface DiscoverFieldProps {
+  /**
+   * the selected columns displayed in the doc table in discover
+   */
+  columns: string[];
   /**
    * The displayed field
    */
@@ -76,6 +80,7 @@ export interface DiscoverFieldProps {
 }
 
 export function DiscoverField({
+  columns,
   field,
   indexPattern,
   onAddField,
@@ -228,9 +233,10 @@ export function DiscoverField({
       </EuiPopoverTitle>
       {infoIsOpen && (
         <DiscoverFieldDetails
-          indexPattern={indexPattern}
-          field={field}
+          columns={columns}
           details={getDetails(field)}
+          field={field}
+          indexPattern={indexPattern}
           onAddFilter={onAddFilter}
         />
       )}

diff --git a/src/plugins/discover/public/application/components/sidebar/discover_field_bucket.tsx b/src/plugins/discover/public/application/components/sidebar/discover_field_bucket.tsx
@@ -68,7 +68,7 @@ export function DiscoverFieldBucket({ field, bucket, onAddFilter }: Props) {
                 title={
                   bucket.display === ''
                     ? emptyTxt
-                    : `${bucket.display}: ${bucket.count} (${bucket.percent}%)`
+                    : `${bucket.display}: ${bucket.count} (${bucket.percent.toFixed(1)}%)`
                 }
                 size="xs"
                 className="eui-textTruncate"
@@ -78,7 +78,7 @@ export function DiscoverFieldBucket({ field, bucket, onAddFilter }: Props) {
             </EuiFlexItem>
             <EuiFlexItem grow={false} className="eui-textTruncate">
               <EuiText color="secondary" size="xs" className="eui-textTruncate">
-                {bucket.percent}%
+                {bucket.percent.toFixed(1)}%
               </EuiText>
             </EuiFlexItem>
           </EuiFlexGroup>

diff --git a/src/plugins/discover/public/application/components/sidebar/discover_field_details.test.tsx b/src/plugins/discover/public/application/components/sidebar/discover_field_details.test.tsx
@@ -29,15 +29,26 @@
  */
 
 import React from 'react';
+// @ts-ignore
 import { findTestSubject } from '@elastic/eui/lib/test';
+import { act } from '@testing-library/react';
 // @ts-ignore
 import stubbedLogstashFields from 'fixtures/logstash_fields';
-import { mountWithIntl } from 'test_utils/enzyme_helpers';
+import { mountWithIntl, nextTick } from 'test_utils/enzyme_helpers';
 import { DiscoverFieldDetails } from './discover_field_details';
 import { coreMock } from '../../../../../../core/public/mocks';
 import { IndexPatternField } from '../../../../../data/public';
 import { getStubIndexPattern } from '../../../../../data/public/test_utils';
 
+const mockGetHref = jest.fn();
+const mockGetTriggerCompatibleActions = jest.fn();
+
+jest.mock('../../../opensearch_dashboards_services', () => ({
+  getUiActions: () => ({
+    getTriggerCompatibleActions: mockGetTriggerCompatibleActions,
+  }),
+}));
+
 const indexPattern = getStubIndexPattern(
   'logstash-*',
   (cfg: any) => cfg,
@@ -48,17 +59,187 @@ const indexPattern = getStubIndexPattern(
 
 describe('discover sidebar field details', function () {
   const defaultProps = {
+    columns: [],
+    details: { buckets: [], error: '', exists: 1, total: 1 },
     indexPattern,
-    details: { buckets: [], error: '', exists: 1, total: true, columns: [] },
     onAddFilter: jest.fn(),
   };
 
-  function mountComponent(field: IndexPatternField) {
-    const compProps = { ...defaultProps, field };
+  beforeEach(() => {
+    mockGetHref.mockReturnValue('/foo/bar');
+    mockGetTriggerCompatibleActions.mockReturnValue([
+      {
+        getHref: mockGetHref,
+      },
+    ]);
+  });
+
+  function mountComponent(field: IndexPatternField, props?: Record<string, any>) {
+    const compProps = { ...defaultProps, ...props, field };
     return mountWithIntl(<DiscoverFieldDetails {...compProps} />);
   }
 
-  it('should enable the visualize link for a number field', function () {
+  it('should render buckets if they exist', async function () {
+    const visualizableField = new IndexPatternField(
+      {
+        name: 'bytes',
+        type: 'number',
+        esTypes: ['long'],
+        count: 10,
+        scripted: false,
+        searchable: true,
+        aggregatable: true,
+        readFromDocValues: true,
+      },
+      'bytes'
+    );
+    const buckets = [1, 2, 3].map((n) => ({
+      display: `display-${n}`,
+      value: `value-${n}`,
+      percent: 25,
+      count: 100,
+    }));
+    const comp = mountComponent(visualizableField, {
+      details: { ...defaultProps.details, buckets },
+    });
+    expect(findTestSubject(comp, 'fieldVisualizeError').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualizeBucketContainer').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualizeBucketContainer').children().length).toBe(
+      buckets.length
+    );
+    // Visualize link should not be rendered until async hook update
+    expect(findTestSubject(comp, 'fieldVisualizeLink').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualize-bytes').length).toBe(0);
+
+    // Complete async hook
+    await act(async () => {
+      await nextTick();
+      comp.update();
+    });
+    expect(findTestSubject(comp, 'fieldVisualizeError').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualizeBucketContainer').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualizeBucketContainer').children().length).toBe(
+      buckets.length
+    );
+    expect(findTestSubject(comp, 'fieldVisualizeLink').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualize-bytes').length).toBe(1);
+  });
+
+  it('should only render buckets if they exist', async function () {
+    const visualizableField = new IndexPatternField(
+      {
+        name: 'bytes',
+        type: 'number',
+        esTypes: ['long'],
+        count: 10,
+        scripted: false,
+        searchable: true,
+        aggregatable: true,
+        readFromDocValues: true,
+      },
+      'bytes'
+    );
+    const comp = mountComponent(visualizableField);
+    expect(findTestSubject(comp, 'fieldVisualizeContainer').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualizeError').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualizeBucketContainer').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualizeLink').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualize-bytes').length).toBe(0);
+
+    await act(async () => {
+      await nextTick();
+      comp.update();
+    });
+
+    expect(findTestSubject(comp, 'fieldVisualizeContainer').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualizeError').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualizeBucketContainer').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualizeLink').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualize-bytes').length).toBe(1);
+  });
+
+  it('should render a details error', async function () {
+    const visualizableField = new IndexPatternField(
+      {
+        name: 'bytes',
+        type: 'number',
+        esTypes: ['long'],
+        count: 10,
+        scripted: false,
+        searchable: true,
+        aggregatable: true,
+        readFromDocValues: true,
+      },
+      'bytes'
+    );
+    const errText = 'Some error';
+    const comp = mountComponent(visualizableField, {
+      details: { ...defaultProps.details, error: errText },
+    });
+    expect(findTestSubject(comp, 'fieldVisualizeContainer').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualizeBucketContainer').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualizeError').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualizeError').text()).toBe(errText);
+
+    await act(async () => {
+      await nextTick();
+      comp.update();
+    });
+    expect(findTestSubject(comp, 'fieldVisualizeLink').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualize-bytes').length).toBe(1);
+  });
+
+  it('should handle promise rejection from isFieldVisualizable', async function () {
+    mockGetTriggerCompatibleActions.mockRejectedValue(new Error('Async error'));
+    const visualizableField = new IndexPatternField(
+      {
+        name: 'bytes',
+        type: 'number',
+        esTypes: ['long'],
+        count: 10,
+        scripted: false,
+        searchable: true,
+        aggregatable: true,
+        readFromDocValues: true,
+      },
+      'bytes'
+    );
+    const comp = mountComponent(visualizableField);
+
+    await act(async () => {
+      await nextTick();
+      comp.update();
+    });
+    expect(findTestSubject(comp, 'fieldVisualizeLink').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualize-bytes').length).toBe(0);
+  });
+
+  it('should handle promise rejection from getVisualizeHref', async function () {
+    mockGetHref.mockRejectedValue(new Error('Async error'));
+    const visualizableField = new IndexPatternField(
+      {
+        name: 'bytes',
+        type: 'number',
+        esTypes: ['long'],
+        count: 10,
+        scripted: false,
+        searchable: true,
+        aggregatable: true,
+        readFromDocValues: true,
+      },
+      'bytes'
+    );
+    const comp = mountComponent(visualizableField);
+
+    await act(async () => {
+      await nextTick();
+      comp.update();
+    });
+    expect(findTestSubject(comp, 'fieldVisualizeLink').length).toBe(0);
+    expect(findTestSubject(comp, 'fieldVisualize-bytes').length).toBe(0);
+  });
+
+  it('should enable the visualize link for a number field', async function () {
     const visualizableField = new IndexPatternField(
       {
         name: 'bytes',
@@ -73,10 +254,17 @@ describe('discover sidebar field details', function () {
       'bytes'
     );
     const comp = mountComponent(visualizableField);
-    expect(findTestSubject(comp, 'fieldVisualize-bytes')).toBeTruthy();
+
+    await act(async () => {
+      await nextTick();
+      comp.update();
+    });
+    expect(findTestSubject(comp, 'fieldVisualizeLink').length).toBe(1);
+    expect(findTestSubject(comp, 'fieldVisualize-bytes').length).toBe(1);
   });
 
-  it('should disable the visualize link for an _id field', function () {
+  it('should disable the visualize link for an _id field', async function () {
+    expect.assertions(1);
     const conflictField = new IndexPatternField(
       {
         name: '_id',
@@ -91,10 +279,15 @@ describe('discover sidebar field details', function () {
       'test'
     );
     const comp = mountComponent(conflictField);
-    expect(findTestSubject(comp, 'fieldVisualize-_id')).toEqual({});
+
+    await act(async () => {
+      await nextTick();
+      comp.update();
+    });
+    expect(findTestSubject(comp, 'fieldVisualize-_id').length).toBe(0);
   });
 
-  it('should disable the visualize link for an unknown field', function () {
+  it('should disable the visualize link for an unknown field', async function () {
     const unknownField = new IndexPatternField(
       {
         name: 'test',
@@ -109,6 +302,11 @@ describe('discover sidebar field details', function () {
       'test'
     );
     const comp = mountComponent(unknownField);
-    expect(findTestSubject(comp, 'fieldVisualize-test')).toEqual({});
+
+    await act(async () => {
+      await nextTick();
+      comp.update();
+    });
+    expect(findTestSubject(comp, 'fieldVisualize-test').length).toBe(0);
   });
 });