Skip to content

Commit

Permalink
[ML] Performance improvements to annotations editing in Single Metric…
Browse files Browse the repository at this point in the history 
… Viewer & buttons placement (#83216)
  • Loading branch information
@qn895
qn895 authored Nov 18, 2020
1 parent 21c0258 commit 7a7057e
Show file tree
Hide file tree
Showing 16 changed files with 244 additions and 119 deletions.
3 changes: 0 additions & 3 deletions ...ic/application/components/annotations/annotation_flyout/__snapshots__/index.test.tsx.snap
View file

This file was deleted.

122 changes: 83 additions & 39 deletions x-pack/plugins/ml/public/application/components/annotations/annotation_flyout/index.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,58 +5,102 @@
*/

import useObservable from 'react-use/lib/useObservable';

import mockAnnotations from '../annotations_table/__mocks__/mock_annotations.json';

import React from 'react';
import { mountWithIntl, shallowWithIntl } from '@kbn/test/jest';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { IntlProvider } from 'react-intl';

import { Annotation } from '../../../../../common/types/annotations';
import { annotation$ } from '../../../services/annotations_service';
import { AnnotationUpdatesService } from '../../../services/annotations_service';

import { AnnotationFlyout } from './index';
import { MlAnnotationUpdatesContext } from '../../../contexts/ml/ml_annotation_updates_context';

jest.mock('../../../util/dependency_cache', () => ({
getToastNotifications: () => ({ addSuccess: jest.fn(), addDanger: jest.fn() }),
}));

const MlAnnotationUpdatesContextProvider = ({
annotationUpdatesService,
children,
}: {
annotationUpdatesService: AnnotationUpdatesService;
children: React.ReactElement;
}) => {
return (
<MlAnnotationUpdatesContext.Provider value={annotationUpdatesService}>
<IntlProvider>{children}</IntlProvider>
</MlAnnotationUpdatesContext.Provider>
);
};

const ObservableComponent = (props: any) => {
const { annotationUpdatesService } = props;
const annotationProp = useObservable(annotationUpdatesService!.isAnnotationInitialized$());
if (annotationProp === undefined) {
return null;
}
return (
<AnnotationFlyout
annotation={annotationProp}
annotationUpdatesService={annotationUpdatesService}
{...props}
/>
);
};

describe('AnnotationFlyout', () => {
test('Initialization.', () => {
const wrapper = shallowWithIntl(<AnnotationFlyout />);
expect(wrapper).toMatchSnapshot();
let annotationUpdatesService: AnnotationUpdatesService | null = null;
beforeEach(() => {
annotationUpdatesService = new AnnotationUpdatesService();
});

test('Update button is disabled with empty annotation', () => {
test('Update button is disabled with empty annotation', async () => {
const annotation = mockAnnotations[1] as Annotation;
annotation$.next(annotation);

// useObservable wraps the observable in a new component
const ObservableComponent = (props: any) => {
const annotationProp = useObservable(annotation$);
if (annotationProp === undefined) {
return null;
}
return <AnnotationFlyout annotation={annotationProp} {...props} />;
};

const wrapper = mountWithIntl(<ObservableComponent />);
const updateBtn = wrapper.find('EuiButton').first();
expect(updateBtn.prop('isDisabled')).toEqual(true);

annotationUpdatesService!.setValue(annotation);

const { getByTestId } = render(
<MlAnnotationUpdatesContextProvider annotationUpdatesService={annotationUpdatesService!}>
<ObservableComponent annotationUpdatesService={annotationUpdatesService!} />
</MlAnnotationUpdatesContextProvider>
);
const updateBtn = getByTestId('annotationFlyoutUpdateButton');
expect(updateBtn).toBeDisabled();
});

test('Error displayed and update button displayed if annotation text is longer than max chars', () => {
test('Error displayed and update button displayed if annotation text is longer than max chars', async () => {
const annotation = mockAnnotations[2] as Annotation;
annotation$.next(annotation);

// useObservable wraps the observable in a new component
const ObservableComponent = (props: any) => {
const annotationProp = useObservable(annotation$);
if (annotationProp === undefined) {
return null;
}
return <AnnotationFlyout annotation={annotationProp} {...props} />;
};

const wrapper = mountWithIntl(<ObservableComponent />);
const updateBtn = wrapper.find('EuiButton').first();
expect(updateBtn.prop('isDisabled')).toEqual(true);

expect(wrapper.find('EuiFormErrorText')).toHaveLength(1);
annotationUpdatesService!.setValue(annotation);

const { getByTestId } = render(
<MlAnnotationUpdatesContextProvider annotationUpdatesService={annotationUpdatesService!}>
<ObservableComponent annotationUpdatesService={annotationUpdatesService!} />
</MlAnnotationUpdatesContextProvider>
);
const updateBtn = getByTestId('annotationFlyoutUpdateButton');
expect(updateBtn).toBeDisabled();
await waitFor(() => {
const errorText = screen.queryByText(/characters above maximum length/);
expect(errorText).not.toBe(undefined);
});
});

test('Flyout disappears when annotation is updated', async () => {
const annotation = mockAnnotations[0] as Annotation;

annotationUpdatesService!.setValue(annotation);

const { getByTestId } = render(
<MlAnnotationUpdatesContextProvider annotationUpdatesService={annotationUpdatesService!}>
<ObservableComponent annotationUpdatesService={annotationUpdatesService!} />
</MlAnnotationUpdatesContextProvider>
);
const updateBtn = getByTestId('annotationFlyoutUpdateButton');
expect(updateBtn).not.toBeDisabled();
expect(screen.queryByTestId('mlAnnotationFlyout')).toBeInTheDocument();

await fireEvent.click(updateBtn);
expect(screen.queryByTestId('mlAnnotationFlyout')).not.toBeInTheDocument();
});
});
61 changes: 36 additions & 25 deletions x-pack/plugins/ml/public/application/components/annotations/annotation_flyout/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { Component, FC, ReactNode, useCallback } from 'react';
import React, { Component, FC, ReactNode, useCallback, useContext } from 'react';
import useObservable from 'react-use/lib/useObservable';
import * as Rx from 'rxjs';
import { cloneDeep } from 'lodash';
Expand All @@ -28,15 +28,14 @@ import {
import { CommonProps } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import { distinctUntilChanged } from 'rxjs/operators';
import {
ANNOTATION_MAX_LENGTH_CHARS,
ANNOTATION_EVENT_USER,
} from '../../../../../common/constants/annotations';
import {
annotation$,
annotationsRefreshed,
AnnotationState,
AnnotationUpdatesService,
} from '../../../services/annotations_service';
import { AnnotationDescriptionList } from '../annotation_description_list';
import { DeleteAnnotationModal } from '../delete_annotation_modal';
Expand All @@ -48,6 +47,7 @@ import {
} from '../../../../../common/types/annotations';
import { PartitionFieldsType } from '../../../../../common/types/anomalies';
import { PARTITION_FIELDS } from '../../../../../common/constants/anomalies';
import { MlAnnotationUpdatesContext } from '../../../contexts/ml/ml_annotation_updates_context';

interface ViewableDetector {
index: number;
Expand All @@ -67,6 +67,7 @@ interface Props {
};
detectorIndex: number;
detectors: ViewableDetector[];
annotationUpdatesService: AnnotationUpdatesService;
}

interface State {
Expand All @@ -85,7 +86,8 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
public annotationSub: Rx.Subscription | null = null;

componentDidMount() {
this.annotationSub = annotation$.subscribe((v) => {
const { annotationUpdatesService } = this.props;
this.annotationSub = annotationUpdatesService.update$().subscribe((v) => {
this.setState({
annotationState: v,
});
Expand All @@ -100,15 +102,17 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
if (this.state.annotationState === null) {
return;
}
const { annotationUpdatesService } = this.props;

annotation$.next({
annotationUpdatesService.setValue({
...this.state.annotationState,
annotation: e.target.value,
});
};

public cancelEditingHandler = () => {
annotation$.next(null);
const { annotationUpdatesService } = this.props;
annotationUpdatesService.setValue(null);
};

public deleteConfirmHandler = () => {
Expand Down Expand Up @@ -148,7 +152,10 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
}

this.closeDeleteModal();
annotation$.next(null);

const { annotationUpdatesService } = this.props;

annotationUpdatesService.setValue(null);
annotationsRefreshed();
};

Expand Down Expand Up @@ -193,7 +200,8 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {

public saveOrUpdateAnnotation = () => {
const { annotationState: originalAnnotation } = this.state;
const { chartDetails, detectorIndex } = this.props;
const { chartDetails, detectorIndex, annotationUpdatesService } = this.props;

if (originalAnnotation === null) {
return;
}
Expand All @@ -218,8 +226,7 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
}
// Mark the annotation created by `user` if and only if annotation is being created, not updated
annotation.event = annotation.event ?? ANNOTATION_EVENT_USER;

annotation$.next(null);
annotationUpdatesService.setValue(null);

ml.annotations
.indexAnnotation(annotation)
Expand Down Expand Up @@ -356,16 +363,16 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
</EuiFormRow>
</EuiFlyoutBody>
<EuiFlyoutFooter>
<EuiFlexGroup justifyContent="spaceBetween">
<EuiFlexGroup>
<EuiFlexItem grow={false}>
<EuiButtonEmpty iconType="cross" onClick={this.cancelEditingHandler} flush="left">
<EuiButtonEmpty onClick={this.cancelEditingHandler} flush="left">
<FormattedMessage
id="xpack.ml.timeSeriesExplorer.annotationFlyout.cancelButtonLabel"
defaultMessage="Cancel"
/>
</EuiButtonEmpty>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiFlexItem grow={false} style={{ marginLeft: 'auto' }}>
{isExistingAnnotation && (
<EuiButtonEmpty color="danger" onClick={this.deleteConfirmHandler}>
<FormattedMessage
Expand All @@ -376,7 +383,12 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
)}
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButton fill isDisabled={isInvalid === true} onClick={this.saveOrUpdateAnnotation}>
<EuiButton
fill
isDisabled={isInvalid === true}
onClick={this.saveOrUpdateAnnotation}
data-test-subj={'annotationFlyoutUpdateButton'}
>
{isExistingAnnotation ? (
<FormattedMessage
id="xpack.ml.timeSeriesExplorer.annotationFlyout.updateButtonLabel"
Expand All @@ -403,17 +415,11 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
}

export const AnnotationFlyout: FC<any> = (props) => {
const annotationProp = useObservable(
annotation$.pipe(
distinctUntilChanged((prev, curr) => {
// prevent re-rendering
return prev !== null && curr !== null;
})
)
);
const annotationUpdatesService = useContext(MlAnnotationUpdatesContext);
const annotationProp = useObservable(annotationUpdatesService.isAnnotationInitialized$());

const cancelEditingHandler = useCallback(() => {
annotation$.next(null);
annotationUpdatesService.setValue(null);
}, []);

if (annotationProp === undefined || annotationProp === null) {
Expand All @@ -423,7 +429,12 @@ export const AnnotationFlyout: FC<any> = (props) => {
const isExistingAnnotation = typeof annotationProp._id !== 'undefined';

return (
<EuiFlyout onClose={cancelEditingHandler} size="m" aria-labelledby="Add annotation">
<EuiFlyout
onClose={cancelEditingHandler}
size="m"
aria-labelledby="Add annotation"
data-test-subj={'mlAnnotationFlyout'}
>
<EuiFlyoutHeader hasBorder>
<EuiTitle size="s">
<h2 id="mlAnnotationFlyoutTitle">
Expand All @@ -441,7 +452,7 @@ export const AnnotationFlyout: FC<any> = (props) => {
</h2>
</EuiTitle>
</EuiFlyoutHeader>
<AnnotationFlyoutUI {...props} />
<AnnotationFlyoutUI {...props} annotationUpdatesService={annotationUpdatesService} />
</EuiFlyout>
);
};
6 changes: 3 additions & 3 deletions ...ion/components/annotations/annotations_table/__snapshots__/annotations_table.test.js.snap
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 7a7057e

Please sign in to comment.