Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Chore] Refactor and improve Discover field summaries #2391

Merged
merged 4 commits into from
Nov 3, 2022
Merged

[Chore] Refactor and improve Discover field summaries #2391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1b824b
[Chore] Refactor and improve field summaries
joshuarrrr Sep 20, 2022
293ece9
[Test] Update functional test
joshuarrrr Oct 12, 2022
6e8cea9
[Chore] Add changelog entry
joshuarrrr Oct 13, 2022
fe59826
[Chore] Refactor columns passing, fix bugs
joshuarrrr Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,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

Expand Down
1 change: 1 addition & 0 deletions src/core/public/mocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ function createCoreSetupMock({
uiSettings: uiSettingsServiceMock.createSetupContract(),
injectedMetadata: {
getInjectedVar: injectedMetadataServiceMock.createSetupContract().getInjectedVar,
getBranding: injectedMetadataServiceMock.createSetupContract().getBranding,
},
};

Expand Down
4 changes: 3 additions & 1 deletion src/plugins/discover/public/application/components/sidebar/discover_field.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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(),
Expand Down
10 changes: 8 additions & 2 deletions src/plugins/discover/public/application/components/sidebar/discover_field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -76,6 +80,7 @@ export interface DiscoverFieldProps {
}

export function DiscoverField({
columns,
field,
indexPattern,
onAddField,
Expand Down Expand Up @@ -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}
/>
)}
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/discover/public/application/components/sidebar/discover_field_bucket.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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>
Expand Down
218 changes: 208 additions & 10 deletions src/plugins/discover/public/application/components/sidebar/discover_field_details.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious what does this act() function do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://reactjs.org/docs/test-utils.html#act

In this case I introduced it to help distinguish between the component state before and after an async hook runs (see L109-113). There are probably other/better ways to test a case like this, but it was an existing pattern in the codebase. Probably something we want to document better for other similar tests.

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 () {
joshuarrrr marked this conversation as resolved.
Show resolved Hide resolved
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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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);
});
});
Loading