[ILM] Surfacing policy error state (elastic#89701)

* added policy top-level callout for error state * added form errors context. errors are sorted by their field path into phases * added data test subject attributes and prevent setErrors from getting called if component is not mounted * update copy * refactored errors context and optimised setting of context value. Also added test for various form error notifications across the collapsed phases * add test for non-phase specific policy validation error * Remove unused import * refactor how errors are listened to, use the new "onError" callback Co-authored-by: Kibana Machine <[email protected]>
jloleysens · Feb 3, 2021 · c64147d · c64147d
1 parent 7d36576
commit c64147d
Show file tree

Hide file tree

Showing 28 changed files with 497 additions and 78 deletions.
diff --git a/...ndex_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx b/...ndex_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx
@@ -197,7 +197,9 @@ export const setup = async (arg?: { appServicesContext: Partial<AppServicesConte
     createFormSetValueAction(`${phase}-selectedNodeAttrs`);
 
   const setReplicas = (phase: Phases) => async (value: string) => {
-    await createFormToggleAction(`${phase}-setReplicasSwitch`)(true);
+    if (!exists(`${phase}-selectedReplicaCount`)) {
+      await createFormToggleAction(`${phase}-setReplicasSwitch`)(true);
+    }
     await createFormSetValueAction(`${phase}-selectedReplicaCount`)(value);
   };
 
@@ -248,8 +250,11 @@ export const setup = async (arg?: { appServicesContext: Partial<AppServicesConte
   return {
     ...testBed,
     actions: {
+      saveAsNewPolicy: createFormToggleAction('saveAsNewSwitch'),
+      setPolicyName: createFormSetValueAction('policyNameField'),
       setWaitForSnapshotPolicy,
       savePolicy,
+      hasGlobalErrorCallout: () => exists('policyFormErrorsCallout'),
       timeline: {
         hasRolloverIndicator: () => exists('timelineHotPhaseRolloverToolTip'),
         hasHotPhase: () => exists('ilmTimelineHotPhase'),
@@ -263,6 +268,7 @@ export const setup = async (arg?: { appServicesContext: Partial<AppServicesConte
         setMaxAge,
         toggleRollover,
         toggleDefaultRollover,
+        hasErrorIndicator: () => exists('phaseErrorIndicator-hot'),
         ...createForceMergeActions('hot'),
         ...createIndexPriorityActions('hot'),
         ...createShrinkActions('hot'),
@@ -276,6 +282,7 @@ export const setup = async (arg?: { appServicesContext: Partial<AppServicesConte
         setDataAllocation: setDataAllocation('warm'),
         setSelectedNodeAttribute: setSelectedNodeAttribute('warm'),
         setReplicas: setReplicas('warm'),
+        hasErrorIndicator: () => exists('phaseErrorIndicator-warm'),
         ...createShrinkActions('warm'),
         ...createForceMergeActions('warm'),
         setReadonly: setReadonly('warm'),
@@ -290,6 +297,7 @@ export const setup = async (arg?: { appServicesContext: Partial<AppServicesConte
         setReplicas: setReplicas('cold'),
         setFreeze,
         freezeExists,
+        hasErrorIndicator: () => exists('phaseErrorIndicator-cold'),
         ...createIndexPriorityActions('cold'),
         ...createSearchableSnapshotActions('cold'),
       },

diff --git a/...ns/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts b/...ns/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts
@@ -29,6 +29,7 @@ window.scrollTo = jest.fn();
 describe('<EditPolicy />', () => {
   let testBed: EditPolicyTestBed;
   const { server, httpRequestsMockHelpers } = setupEnvironment();
+
   afterAll(() => {
     server.restore();
   });
@@ -852,4 +853,132 @@ describe('<EditPolicy />', () => {
       expect(actions.timeline.hasRolloverIndicator()).toBe(false);
     });
   });
+
+  describe('policy error notifications', () => {
+    beforeAll(() => {
+      jest.useFakeTimers();
+    });
+
+    afterAll(() => {
+      jest.useRealTimers();
+    });
+    beforeEach(async () => {
+      httpRequestsMockHelpers.setLoadPolicies([getDefaultHotPhasePolicy('my_policy')]);
+      httpRequestsMockHelpers.setListNodes({
+        nodesByRoles: {},
+        nodesByAttributes: { test: ['123'] },
+        isUsingDeprecatedDataRoleConfig: false,
+      });
+      httpRequestsMockHelpers.setLoadSnapshotPolicies([]);
+
+      await act(async () => {
+        testBed = await setup();
+      });
+
+      const { component } = testBed;
+      component.update();
+    });
+
+    // For new we rely on a setTimeout to ensure that error messages have time to populate
+    // the form object before we look at the form object. See:
+    // x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/form_errors_context.tsx
+    // for where this logic lives.
+    const runTimers = () => {
+      const { component } = testBed;
+      act(() => {
+        jest.runAllTimers();
+      });
+      component.update();
+    };
+
+    test('shows phase error indicators correctly', async () => {
+      // This test simulates a user configuring a policy phase by phase. The flow is the following:
+      // 0. Start with policy with no validation issues present
+      // 1. Configure hot, introducing a validation error
+      // 2. Configure warm, introducing a validation error
+      // 3. Configure cold, introducing a validation error
+      // 4. Fix validation error in hot
+      // 5. Fix validation error in warm
+      // 6. Fix validation error in cold
+      // We assert against each of these progressive states.
+
+      const { actions } = testBed;
+
+      // 0. No validation issues
+      expect(actions.hasGlobalErrorCallout()).toBe(false);
+      expect(actions.hot.hasErrorIndicator()).toBe(false);
+      expect(actions.warm.hasErrorIndicator()).toBe(false);
+      expect(actions.cold.hasErrorIndicator()).toBe(false);
+
+      // 1. Hot phase validation issue
+      await actions.hot.toggleForceMerge(true);
+      await actions.hot.setForcemergeSegmentsCount('-22');
+      runTimers();
+      expect(actions.hasGlobalErrorCallout()).toBe(true);
+      expect(actions.hot.hasErrorIndicator()).toBe(true);
+      expect(actions.warm.hasErrorIndicator()).toBe(false);
+      expect(actions.cold.hasErrorIndicator()).toBe(false);
+
+      // 2. Warm phase validation issue
+      await actions.warm.enable(true);
+      await actions.warm.toggleForceMerge(true);
+      await actions.warm.setForcemergeSegmentsCount('-22');
+      await runTimers();
+      expect(actions.hasGlobalErrorCallout()).toBe(true);
+      expect(actions.hot.hasErrorIndicator()).toBe(true);
+      expect(actions.warm.hasErrorIndicator()).toBe(true);
+      expect(actions.cold.hasErrorIndicator()).toBe(false);
+
+      // 3. Cold phase validation issue
+      await actions.cold.enable(true);
+      await actions.cold.setReplicas('-33');
+      await runTimers();
+      expect(actions.hasGlobalErrorCallout()).toBe(true);
+      expect(actions.hot.hasErrorIndicator()).toBe(true);
+      expect(actions.warm.hasErrorIndicator()).toBe(true);
+      expect(actions.cold.hasErrorIndicator()).toBe(true);
+
+      // 4. Fix validation issue in hot
+      await actions.hot.setForcemergeSegmentsCount('1');
+      await runTimers();
+      expect(actions.hasGlobalErrorCallout()).toBe(true);
+      expect(actions.hot.hasErrorIndicator()).toBe(false);
+      expect(actions.warm.hasErrorIndicator()).toBe(true);
+      expect(actions.cold.hasErrorIndicator()).toBe(true);
+
+      // 5. Fix validation issue in warm
+      await actions.warm.setForcemergeSegmentsCount('1');
+      await runTimers();
+      expect(actions.hasGlobalErrorCallout()).toBe(true);
+      expect(actions.hot.hasErrorIndicator()).toBe(false);
+      expect(actions.warm.hasErrorIndicator()).toBe(false);
+      expect(actions.cold.hasErrorIndicator()).toBe(true);
+
+      // 6. Fix validation issue in cold
+      await actions.cold.setReplicas('1');
+      await runTimers();
+      expect(actions.hasGlobalErrorCallout()).toBe(false);
+      expect(actions.hot.hasErrorIndicator()).toBe(false);
+      expect(actions.warm.hasErrorIndicator()).toBe(false);
+      expect(actions.cold.hasErrorIndicator()).toBe(false);
+    });
+
+    test('global error callout should show if there are any form errors', async () => {
+      const { actions } = testBed;
+
+      expect(actions.hasGlobalErrorCallout()).toBe(false);
+      expect(actions.hot.hasErrorIndicator()).toBe(false);
+      expect(actions.warm.hasErrorIndicator()).toBe(false);
+      expect(actions.cold.hasErrorIndicator()).toBe(false);
+
+      await actions.saveAsNewPolicy(true);
+      await actions.setPolicyName('');
+      await runTimers();
+
+      expect(actions.hasGlobalErrorCallout()).toBe(true);
+      expect(actions.hot.hasErrorIndicator()).toBe(false);
+      expect(actions.warm.hasErrorIndicator()).toBe(false);
+      expect(actions.cold.hasErrorIndicator()).toBe(false);
+    });
+  });
 });
diff --git a/...ctions/edit_policy/components/described_form_row/toggle_field_with_described_form_row.tsx b/...ctions/edit_policy/components/described_form_row/toggle_field_with_described_form_row.tsx
@@ -5,7 +5,7 @@
  */
 import React, { FunctionComponent } from 'react';
 
-import { UseField } from '../../../../../shared_imports';
+import { UseField } from '../../form';
 
 import {
   DescribedFormRow,

diff --git a/...x_lifecycle_management/public/application/sections/edit_policy/components/form_errors.tsx b/...x_lifecycle_management/public/application/sections/edit_policy/components/form_errors.tsx
diff --git a/...cle_management/public/application/sections/edit_policy/components/form_errors_callout.tsx b/...cle_management/public/application/sections/edit_policy/components/form_errors_callout.tsx
@@ -0,0 +1,45 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import React, { FunctionComponent } from 'react';
+import { i18n } from '@kbn/i18n';
+import { EuiCallOut, EuiSpacer } from '@elastic/eui';
+
+import { useFormErrorsContext } from '../form';
+
+const i18nTexts = {
+  callout: {
+    title: i18n.translate('xpack.indexLifecycleMgmt.policyErrorCalloutTitle', {
+      defaultMessage: 'This policy contains errors',
+    }),
+    body: i18n.translate('xpack.indexLifecycleMgmt.policyErrorCalloutDescription', {
+      defaultMessage: 'Please fix all errors before saving the policy.',
+    }),
+  },
+};
+
+export const FormErrorsCallout: FunctionComponent = () => {
+  const {
+    errors: { hasErrors },
+  } = useFormErrorsContext();
+
+  if (!hasErrors) {
+    return null;
+  }
+
+  return (
+    <>
+      <EuiCallOut
+        data-test-subj="policyFormErrorsCallout"
+        color="danger"
+        title={i18nTexts.callout.title}
+      >
+        {i18nTexts.callout.body}
+      </EuiCallOut>
+      <EuiSpacer />
+    </>
+  );
+};
diff --git a/...ns/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts b/...ns/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts
@@ -5,13 +5,13 @@
  */
 
 export { ActiveBadge } from './active_badge';
-export { ErrableFormRow } from './form_errors';
 export { LearnMoreLink } from './learn_more_link';
 export { OptionalLabel } from './optional_label';
 export { PolicyJsonFlyout } from './policy_json_flyout';
 export { DescribedFormRow, ToggleFieldWithDescribedFormRow } from './described_form_row';
 export { FieldLoadingError } from './field_loading_error';
 export { ActiveHighlight } from './active_highlight';
 export { Timeline } from './timeline';
+export { FormErrorsCallout } from './form_errors_callout';
 
 export * from './phases';
diff --git a/...ement/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx b/...ement/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx
@@ -23,6 +23,7 @@ import {
   IndexPriorityField,
   ReplicasField,
 } from '../shared_fields';
+
 import { Phase } from '../phase';
 
 const i18nTexts = {

diff --git a/...t/public/application/sections/edit_policy/components/phases/delete_phase/delete_phase.tsx b/...t/public/application/sections/edit_policy/components/phases/delete_phase/delete_phase.tsx
@@ -9,7 +9,9 @@ import { get } from 'lodash';
 import { FormattedMessage } from '@kbn/i18n/react';
 import { EuiDescribedFormGroup, EuiTextColor, EuiFormRow } from '@elastic/eui';
 
-import { useFormData, UseField, ToggleField } from '../../../../../../shared_imports';
+import { useFormData, ToggleField } from '../../../../../../shared_imports';
+
+import { UseField } from '../../../form';
 
 import { ActiveBadge, LearnMoreLink, OptionalLabel } from '../../index';
 

diff --git a/...agement/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx b/...agement/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx
@@ -19,11 +19,11 @@ import {
   EuiIcon,
 } from '@elastic/eui';
 
-import { useFormData, UseField, SelectField, NumericField } from '../../../../../../shared_imports';
+import { useFormData, SelectField, NumericField } from '../../../../../../shared_imports';
 
 import { i18nTexts } from '../../../i18n_texts';
 
-import { ROLLOVER_EMPTY_VALIDATION, useConfigurationIssues } from '../../../form';
+import { ROLLOVER_EMPTY_VALIDATION, useConfigurationIssues, UseField } from '../../../form';
 
 import { useEditPolicyContext } from '../../../edit_policy_context';
 
@@ -38,8 +38,8 @@ import {
   ReadonlyField,
   ShrinkField,
 } from '../shared_fields';
-
 import { Phase } from '../phase';
+
 import { maxSizeStoredUnits, maxAgeUnits } from './constants';
 
 export const HotPhase: FunctionComponent = () => {

diff --git a/...cycle_management/public/application/sections/edit_policy/components/phases/phase/index.ts b/...cycle_management/public/application/sections/edit_policy/components/phases/phase/index.ts
@@ -0,0 +1,7 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export { Phase } from './phase';