[7.x] [ML] Performance improvements to annotations editing in Single …

…Metric Viewer & buttons placement (#83216) (#83671)
elastic · Nov 18, 2020 · fa52b06 · fa52b06
1 parent 5da99bb
commit fa52b06
Show file tree

Hide file tree

Showing 16 changed files with 244 additions and 119 deletions.
diff --git a/...ic/application/components/annotations/annotation_flyout/__snapshots__/index.test.tsx.snap b/...ic/application/components/annotations/annotation_flyout/__snapshots__/index.test.tsx.snap
diff --git a/x-pack/plugins/ml/public/application/components/annotations/annotation_flyout/index.test.tsx b/x-pack/plugins/ml/public/application/components/annotations/annotation_flyout/index.test.tsx
@@ -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();
   });
 });
diff --git a/x-pack/plugins/ml/public/application/components/annotations/annotation_flyout/index.tsx b/x-pack/plugins/ml/public/application/components/annotations/annotation_flyout/index.tsx
@@ -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';
@@ -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';
@@ -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;
@@ -67,6 +67,7 @@ interface Props {
   };
   detectorIndex: number;
   detectors: ViewableDetector[];
+  annotationUpdatesService: AnnotationUpdatesService;
 }
 
 interface State {
@@ -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,
       });
@@ -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 = () => {
@@ -148,7 +152,10 @@ export class AnnotationFlyoutUI extends Component<CommonProps & Props> {
     }
 
     this.closeDeleteModal();
-    annotation$.next(null);
+
+    const { annotationUpdatesService } = this.props;
+
+    annotationUpdatesService.setValue(null);
     annotationsRefreshed();
   };
 
@@ -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;
     }
@@ -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)
@@ -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
@@ -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"
@@ -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) {
@@ -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">
@@ -441,7 +452,7 @@ export const AnnotationFlyout: FC<any> = (props) => {
           </h2>
         </EuiTitle>
       </EuiFlyoutHeader>
-      <AnnotationFlyoutUI {...props} />
+      <AnnotationFlyoutUI {...props} annotationUpdatesService={annotationUpdatesService} />
     </EuiFlyout>
   );
 };
diff --git a/...ion/components/annotations/annotations_table/__snapshots__/annotations_table.test.js.snap b/...ion/components/annotations/annotations_table/__snapshots__/annotations_table.test.js.snap