[ILM] Revisit searchable snapshot field after new redesign (#90793) (#…

…90937) * moved searchable snapshot field out of cold phase accordian * refactor styling to padding top and bottom to get advanced settings drop down to sit flush with side of panel * Error clearing fix and cosmetic changes - the error state of the form would not clear correctly if the erroring field was unmounted. The logic for clearing form errors was also incorrectly using "keys" instead of "values". - updated the width of wait for snapshot policy field to be the same as other fields * fix hook dependency causing clearError to be called * slight improvement to component integration test * re-add singleSelection to snapshot policiy field config * refactored Phase component API and fixed typo in comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
elastic · Feb 15, 2021 · ce9ca8e · ce9ca8e
1 parent 8acf580
commit ce9ca8e
Show file tree

Hide file tree

Showing 9 changed files with 76 additions and 21 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
@@ -237,7 +237,9 @@ export const setup = async (arg?: { appServicesContext: Partial<AppServicesConte
  exists(`${fieldSelector}.searchableSnapshotDisabledDueToLicense`),
  toggleSearchableSnapshot,
  setSearchableSnapshot: async (value: string) => {
- await toggleSearchableSnapshot(true);
+ if (!exists(`searchableSnapshotField-${phase}.searchableSnapshotCombobox`)) {
+ await toggleSearchableSnapshot(true);
+ }
  act(() => {
  find(`searchableSnapshotField-${phase}.searchableSnapshotCombobox`).simulate('change', [
  { label: value },

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
@@ -916,7 +916,7 @@ describe('<EditPolicy />', () => {
  await actions.warm.enable(true);
  await actions.warm.toggleForceMerge(true);
  await actions.warm.setForcemergeSegmentsCount('-22');
- await runTimers();
+ runTimers();
  expect(actions.hasGlobalErrorCallout()).toBe(true);
  expect(actions.hot.hasErrorIndicator()).toBe(true);
  expect(actions.warm.hasErrorIndicator()).toBe(true);
@@ -925,31 +925,31 @@ describe('<EditPolicy />', () => {
  // 3. Cold phase validation issue
  await actions.cold.enable(true);
  await actions.cold.setReplicas('-33');
- await runTimers();
+ 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();
+ 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();
+ 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();
+ runTimers();
  expect(actions.hasGlobalErrorCallout()).toBe(false);
  expect(actions.hot.hasErrorIndicator()).toBe(false);
  expect(actions.warm.hasErrorIndicator()).toBe(false);
@@ -966,11 +966,36 @@ describe('<EditPolicy />', () => {
 
  await actions.saveAsNewPolicy(true);
  await actions.setPolicyName('');
- await runTimers();
+ runTimers();
+
+ expect(actions.hasGlobalErrorCallout()).toBe(true);
+ expect(actions.hot.hasErrorIndicator()).toBe(false);
+ expect(actions.warm.hasErrorIndicator()).toBe(false);
+ expect(actions.cold.hasErrorIndicator()).toBe(false);
+ });
+
+ test('clears all error indicators if last erroring field is unmounted', async () => {
+ const { actions } = testBed;
+
+ await actions.cold.enable(true);
+ // introduce validation error
+ await actions.cold.setSearchableSnapshot('');
+ runTimers();
+
+ await actions.savePolicy();
+ runTimers();
 
  expect(actions.hasGlobalErrorCallout()).toBe(true);
  expect(actions.hot.hasErrorIndicator()).toBe(false);
  expect(actions.warm.hasErrorIndicator()).toBe(false);
+ expect(actions.cold.hasErrorIndicator()).toBe(true);
+
+ // unmount the field
+ await actions.cold.toggleSearchableSnapshot(false);
+
+ expect(actions.hasGlobalErrorCallout()).toBe(false);
+ expect(actions.hot.hasErrorIndicator()).toBe(false);
+ expect(actions.warm.hasErrorIndicator()).toBe(false);
  expect(actions.cold.hasErrorIndicator()).toBe(false);
  });
  });

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
@@ -51,10 +51,8 @@ export const ColdPhase: FunctionComponent = () => {
  const showReplicasField = get(formData, formFieldPaths.searchableSnapshot) == null;
 
  return (
- <Phase phase={'cold'}>
- <SearchableSnapshotField phase={'cold'} />
-
- {showReplicasField && <ReplicasField phase={'cold'} />}
+ <Phase phase="cold" topLevelSettings={<SearchableSnapshotField phase="cold" />}>
+ {showReplicasField && <ReplicasField phase="cold" />}
 
  {/* Freeze section */}
  {!isUsingSearchableSnapshotInHotPhase && (
@@ -90,10 +88,10 @@ export const ColdPhase: FunctionComponent = () => {
  {/* Data tier allocation section */}
  <DataTierAllocationField
  description={i18nTexts.dataTierAllocation.description}
- phase={'cold'}
+ phase="cold"
  />
 
- <IndexPriorityField phase={'cold'} />
+ <IndexPriorityField phase="cold" />
  </Phase>
  );
 };
diff --git a/...cle_management/public/application/sections/edit_policy/components/phases/phase/phase.scss b/...cle_management/public/application/sections/edit_policy/components/phases/phase/phase.scss
@@ -9,7 +9,8 @@
  }
  .ilmSettingsButton {
  color: $euiColorPrimary;
- padding: $euiSizeS;
+ padding-top: $euiSizeS;
+ padding-bottom: $euiSizeS;
  }
  .euiCommentTimeline {
  padding-top: $euiSize;

diff --git a/...ycle_management/public/application/sections/edit_policy/components/phases/phase/phase.tsx b/...ycle_management/public/application/sections/edit_policy/components/phases/phase/phase.tsx
@@ -37,9 +37,13 @@ import './phase.scss';
 
 interface Props {
  phase: PhasesExceptDelete;
+ /**
+ * Settings that should always be visible on the phase when it is enabled.
+ */
+ topLevelSettings?: React.ReactNode;
 }
 
-export const Phase: FunctionComponent<Props> = ({ children, phase }) => {
+export const Phase: FunctionComponent<Props> = ({ children, topLevelSettings, phase }) => {
  const enabledPath = `_meta.${phase}.enabled`;
  const [formData] = useFormData<FormInternal>({
  watch: [enabledPath],
@@ -102,7 +106,15 @@ export const Phase: FunctionComponent<Props> = ({ children, phase }) => {
 
  {enabled && (
  <>
- <EuiSpacer size="m" />
+ {!!topLevelSettings ? (
+ <>
+ <EuiSpacer />
+ {topLevelSettings}
+ </>
+ ) : (
+ <EuiSpacer size="m" />
+ )}
+
  <EuiAccordion
  id={`${phase}-settingsSwitch`}
  buttonContent={

diff --git a/...y/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx b/...y/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx
@@ -185,6 +185,7 @@ export const SearchableSnapshotField: FunctionComponent<Props> = ({ phase }) =>
  <div className="ilmSearchableSnapshotField">
  <UseField<string>
  config={{
+ label: i18nTexts.editPolicy.searchableSnapshotsFieldLabel,
  defaultValue: cloud?.isCloudEnabled ? CLOUD_DEFAULT_REPO : undefined,
  validations: [
  {
@@ -209,6 +210,7 @@ export const SearchableSnapshotField: FunctionComponent<Props> = ({ phase }) =>
  value: singleSelectionArray,
  } as any
  }
+ label={field.label}
  fullWidth={false}
  euiFieldProps={{
  'data-test-subj': 'searchableSnapshotCombobox',

diff --git a/...lication/sections/edit_policy/components/phases/shared_fields/snapshot_policies_field.tsx b/...lication/sections/edit_policy/components/phases/shared_fields/snapshot_policies_field.tsx
@@ -194,6 +194,7 @@ export const SnapshotPoliciesField: React.FunctionComponent = () => {
  }
  euiFieldProps={{
  'data-test-subj': 'snapshotPolicyCombobox',
+ fullWidth: false,
  options: policies,
  singleSelection: { asPlainText: true },
  isLoading,

diff --git a/...management/public/application/sections/edit_policy/form/components/enhanced_use_field.tsx b/...management/public/application/sections/edit_policy/form/components/enhanced_use_field.tsx
@@ -70,5 +70,14 @@ export const EnhancedUseField = <T, F = FormData, I = T>(
  };
  }, []);
 
+ // Make sure to clear error message if the field is unmounted.
+ useEffect(() => {
+ return () => {
+ if (isMounted.current === false) {
+ clearError(phase, path);
+ }
+ };
+ }, [phase, path, clearError]);
+
  return <UseField {...props} onError={onError} />;
 };
diff --git a/...lifecycle_management/public/application/sections/edit_policy/form/form_errors_context.tsx b/...lifecycle_management/public/application/sections/edit_policy/form/form_errors_context.tsx
@@ -54,6 +54,8 @@ export const FormErrorsProvider: FunctionComponent = ({ children }) => {
  const [errors, setErrors] = useState<Errors>(createEmptyErrors);
  const form = useFormContext<FormInternal>();
 
+ const { getErrors: getFormErrors } = form;
+
  const addError: ContextValue['addError'] = useCallback(
  (phase, fieldPath, errorMessages) => {
  setErrors((previousErrors) => ({
@@ -70,28 +72,31 @@ export const FormErrorsProvider: FunctionComponent = ({ children }) => {
 
  const clearError: ContextValue['clearError'] = useCallback(
  (phase, fieldPath) => {
- if (form.getErrors().length) {
+ if (getFormErrors().length) {
  setErrors((previousErrors) => {
  const {
  [phase]: { [fieldPath]: fieldErrorToOmit, ...restOfPhaseErrors },
+ hasErrors,
  ...otherPhases
  } = previousErrors;
 
- const hasErrors =
+ const nextHasErrors =
  Object.keys(restOfPhaseErrors).length === 0 &&
- Object.keys(otherPhases).some((phaseErrors) => !!Object.keys(phaseErrors).length);
+ Object.values(otherPhases).some((phaseErrors) => {
+ return !!Object.keys(phaseErrors).length;
+ });
 
  return {
  ...previousErrors,
- hasErrors,
+ hasErrors: nextHasErrors,
  [phase]: restOfPhaseErrors,
  };
  });
  } else {
  setErrors(createEmptyErrors);
  }
  },
- [form, setErrors]
+ [getFormErrors, setErrors]
  );
 
  return (