[Uptime] Switch from EuiFieldNumber to EuiFieldText on settings p…

…age (#66425) (#68044) * Switch from `EuiFieldNumber` to `EuiFieldText` on settings page. * Add unit tests for cert form fields. * Improve validation. * Add additional server-side settings check with tests. * Only create number object for validation when value is string. * Clean up validation function. Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
elastic · Jun 3, 2020 · 0883783 · 0883783
1 parent 884c162
commit 0883783
Show file tree

Hide file tree

Showing 8 changed files with 260 additions and 20 deletions.
diff --git a/x-pack/plugins/uptime/common/translations.ts b/x-pack/plugins/uptime/common/translations.ts
@@ -6,9 +6,13 @@
 
 import { i18n } from '@kbn/i18n';
 
-export const VALUE_MUST_BE_GREATER_THEN_ZEO = i18n.translate(
+export const VALUE_MUST_BE_GREATER_THAN_ZERO = i18n.translate(
   'xpack.uptime.settings.invalid.error',
   {
     defaultMessage: 'Value must be greater than 0.',
   }
 );
+
+export const VALUE_MUST_BE_AN_INTEGER = i18n.translate('xpack.uptime.settings.invalid.nanError', {
+  defaultMessage: 'Value must be an integer.',
+});
diff --git a/x-pack/plugins/uptime/public/components/settings/__tests__/certificate_form.test.tsx b/x-pack/plugins/uptime/public/components/settings/__tests__/certificate_form.test.tsx
@@ -6,7 +6,7 @@
 
 import React from 'react';
 import { CertificateExpirationForm } from '../certificate_form';
-import { shallowWithRouter } from '../../../lib';
+import { shallowWithRouter, mountWithRouter } from '../../../lib';
 
 describe('CertificateForm', () => {
   it('shallow renders expected elements for valid props', () => {
@@ -26,4 +26,110 @@ describe('CertificateForm', () => {
       )
     ).toMatchSnapshot();
   });
+
+  it('submits number values for certs settings fields', () => {
+    const onChangeMock = jest.fn();
+    const wrapper = mountWithRouter(
+      <CertificateExpirationForm
+        loading={false}
+        onChange={onChangeMock}
+        formFields={{
+          heartbeatIndices: 'heartbeat-8*',
+          certExpirationThreshold: 7,
+          certAgeThreshold: 36,
+        }}
+        fieldErrors={null}
+        isDisabled={false}
+      />
+    );
+
+    const inputs = wrapper.find('input');
+
+    expect(inputs).toHaveLength(2);
+
+    // expiration threshold input
+    inputs.at(0).simulate('change', {
+      target: {
+        value: '23',
+      },
+    });
+
+    // age threshold input
+    inputs.at(1).simulate('change', {
+      target: {
+        value: '56',
+      },
+    });
+
+    expect(onChangeMock).toHaveBeenCalledTimes(2);
+
+    expect(onChangeMock.mock.calls[0]).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "certExpirationThreshold": 23,
+        },
+      ]
+    `);
+
+    expect(onChangeMock.mock.calls[1]).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "certAgeThreshold": 56,
+        },
+      ]
+    `);
+  });
+
+  it('submits undefined for NaN values', () => {
+    const onChangeMock = jest.fn();
+    const wrapper = mountWithRouter(
+      <CertificateExpirationForm
+        loading={false}
+        onChange={onChangeMock}
+        formFields={{
+          heartbeatIndices: 'heartbeat-8*',
+          certExpirationThreshold: 7,
+          certAgeThreshold: 36,
+        }}
+        fieldErrors={null}
+        isDisabled={false}
+      />
+    );
+
+    const inputs = wrapper.find('input');
+
+    expect(inputs).toHaveLength(2);
+
+    // expiration threshold input
+    inputs.at(0).simulate('change', {
+      target: {
+        value: 'A',
+      },
+    });
+
+    // age threshold input
+    inputs.at(1).simulate('change', {
+      target: {
+        value: 'g',
+      },
+    });
+
+    expect(onChangeMock).toHaveBeenCalledTimes(2);
+
+    expect(onChangeMock.mock.calls[0]).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "certExpirationThreshold": undefined,
+        },
+      ]
+    `);
+
+    expect(onChangeMock.mock.calls[1]).toMatchInlineSnapshot(`
+      Array [
+        Object {
+          "certAgeThreshold": undefined,
+        },
+      ]
+    `);
+  });
 });
diff --git a/x-pack/plugins/uptime/public/components/settings/certificate_form.tsx b/x-pack/plugins/uptime/public/components/settings/certificate_form.tsx
@@ -10,7 +10,7 @@ import {
   EuiDescribedFormGroup,
   EuiFormRow,
   EuiCode,
-  EuiFieldNumber,
+  EuiFieldText,
   EuiText,
   EuiTitle,
   EuiSpacer,
@@ -80,17 +80,17 @@ export const CertificateExpirationForm: React.FC<SettingsFormProps> = ({
       >
         <EuiFlexGroup>
           <EuiFlexItem grow={2}>
-            <EuiFieldNumber
-              min={1}
+            <EuiFieldText
               aria-label={certificateFormTranslations.expirationInputAriaLabel}
               data-test-subj={`expiration-threshold-input-${loading ? 'loading' : 'loaded'}`}
               fullWidth
               disabled={isDisabled}
+              isInvalid={!!fieldErrors?.expirationThresholdError}
               isLoading={loading}
               value={formFields?.certExpirationThreshold ?? ''}
               onChange={(e) =>
                 onChange({
-                  certExpirationThreshold: Number(e.target.value),
+                  certExpirationThreshold: Number(e.target.value) || undefined,
                 })
               }
             />
@@ -128,17 +128,17 @@ export const CertificateExpirationForm: React.FC<SettingsFormProps> = ({
       >
         <EuiFlexGroup>
           <EuiFlexItem grow={2}>
-            <EuiFieldNumber
-              min={1}
+            <EuiFieldText
               aria-label={certificateFormTranslations.ageInputAriaLabel}
               data-test-subj={`age-threshold-input-${loading ? 'loading' : 'loaded'}`}
               fullWidth
               disabled={isDisabled}
+              isInvalid={!!fieldErrors?.ageThresholdError}
               isLoading={loading}
               value={formFields?.certAgeThreshold ?? ''}
-              onChange={({ currentTarget: { value } }) =>
+              onChange={({ target: { value } }) =>
                 onChange({
-                  certAgeThreshold: Number(value),
+                  certAgeThreshold: Number(value) || undefined,
                 })
               }
             />

diff --git a/x-pack/plugins/uptime/public/pages/__tests__/settings.test.tsx b/x-pack/plugins/uptime/public/pages/__tests__/settings.test.tsx
@@ -0,0 +1,35 @@
+/*
+ * 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 { isValidCertVal } from '../settings';
+
+describe('settings', () => {
+  describe('isValidCertVal', () => {
+    it('handles NaN values', () => {
+      expect(isValidCertVal(NaN)).toMatchInlineSnapshot(`"Must be a number."`);
+    });
+
+    it('handles undefined', () => {
+      expect(isValidCertVal(undefined)).toMatchInlineSnapshot(`"Must be a number."`);
+    });
+
+    it('handles non-integer numbers', () => {
+      expect(isValidCertVal(23.5)).toMatchInlineSnapshot(`"Value must be an integer."`);
+    });
+
+    it('handles values less than 0', () => {
+      expect(isValidCertVal(-1)).toMatchInlineSnapshot(`"Value must be greater than 0."`);
+    });
+
+    it('handles 0', () => {
+      expect(isValidCertVal(0)).toMatchInlineSnapshot(`"Value must be greater than 0."`);
+    });
+
+    it('allows valid integer numbers', () => {
+      expect(isValidCertVal(67)).toBeUndefined();
+    });
+  });
+});
diff --git a/x-pack/plugins/uptime/public/pages/settings.tsx b/x-pack/plugins/uptime/public/pages/settings.tsx
@@ -31,7 +31,10 @@ import {
   OnFieldChangeType,
 } from '../components/settings/certificate_form';
 import * as Translations from './translations';
-import { VALUE_MUST_BE_GREATER_THEN_ZEO } from '../../common/translations';
+import {
+  VALUE_MUST_BE_GREATER_THAN_ZERO,
+  VALUE_MUST_BE_AN_INTEGER,
+} from '../../common/translations';
 
 interface SettingsPageFieldErrors {
   heartbeatIndices: string | '';
@@ -47,12 +50,15 @@ export interface SettingsFormProps {
   isDisabled: boolean;
 }
 
-const isValidCertVal = (val: string | number) => {
-  if (val === '') {
-    return Translations.BLANK_STR;
+export const isValidCertVal = (val?: number): string | undefined => {
+  if (val === undefined || isNaN(val)) {
+    return Translations.settings.mustBeNumber;
+  }
+  if (val <= 0) {
+    return VALUE_MUST_BE_GREATER_THAN_ZERO;
   }
-  if (val === 0) {
-    return VALUE_MUST_BE_GREATER_THEN_ZEO;
+  if (val % 1) {
+    return VALUE_MUST_BE_AN_INTEGER;
   }
 };
 

diff --git a/x-pack/plugins/uptime/public/pages/translations.ts b/x-pack/plugins/uptime/public/pages/translations.ts
@@ -35,6 +35,9 @@ export const settings = {
   returnToOverviewLinkLabel: i18n.translate('xpack.uptime.settings.returnToOverviewLinkLabel', {
     defaultMessage: 'Return to overview',
   }),
+  mustBeNumber: i18n.translate('xpack.uptime.settings.blankNumberField.error', {
+    defaultMessage: 'Must be a number.',
+  }),
 };
 
 export const BLANK_STR = i18n.translate('xpack.uptime.settings.blank.error', {

diff --git a/x-pack/plugins/uptime/server/rest_api/__tests__/dynamic_settings.test.ts b/x-pack/plugins/uptime/server/rest_api/__tests__/dynamic_settings.test.ts
@@ -0,0 +1,77 @@
+/*
+ * 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 { validateCertsValues } from '../dynamic_settings';
+
+describe('dynamic settings', () => {
+  describe('validateCertValues', () => {
+    it(`doesn't allow age threshold values less than 0`, () => {
+      expect(
+        validateCertsValues({
+          certAgeThreshold: -1,
+          certExpirationThreshold: 2,
+          heartbeatIndices: 'foo',
+        })
+      ).toMatchInlineSnapshot(`
+        Object {
+          "certAgeThreshold": "Value must be greater than 0.",
+        }
+      `);
+    });
+
+    it(`doesn't allow non-integer age threshold values`, () => {
+      expect(
+        validateCertsValues({
+          certAgeThreshold: 10.2,
+          certExpirationThreshold: 2,
+          heartbeatIndices: 'foo',
+        })
+      ).toMatchInlineSnapshot(`
+        Object {
+          "certAgeThreshold": "Value must be an integer.",
+        }
+      `);
+    });
+
+    it(`doesn't allow expiration threshold values less than 0`, () => {
+      expect(
+        validateCertsValues({
+          certAgeThreshold: 2,
+          certExpirationThreshold: -1,
+          heartbeatIndices: 'foo',
+        })
+      ).toMatchInlineSnapshot(`
+        Object {
+          "certExpirationThreshold": "Value must be greater than 0.",
+        }
+      `);
+    });
+
+    it(`doesn't allow non-integer expiration threshold values`, () => {
+      expect(
+        validateCertsValues({
+          certAgeThreshold: 2,
+          certExpirationThreshold: 1.23,
+          heartbeatIndices: 'foo',
+        })
+      ).toMatchInlineSnapshot(`
+        Object {
+          "certExpirationThreshold": "Value must be an integer.",
+        }
+      `);
+    });
+
+    it('allows valid values', () => {
+      expect(
+        validateCertsValues({
+          certAgeThreshold: 2,
+          certExpirationThreshold: 13,
+          heartbeatIndices: 'foo',
+        })
+      ).toBeUndefined();
+    });
+  });
+});
diff --git a/x-pack/plugins/uptime/server/rest_api/dynamic_settings.ts b/x-pack/plugins/uptime/server/rest_api/dynamic_settings.ts
@@ -11,7 +11,10 @@ import { UMServerLibs } from '../lib/lib';
 import { DynamicSettings, DynamicSettingsType } from '../../common/runtime_types';
 import { UMRestApiRouteFactory } from '.';
 import { savedObjectsAdapter } from '../lib/saved_objects';
-import { VALUE_MUST_BE_GREATER_THEN_ZEO } from '../../common/translations';
+import {
+  VALUE_MUST_BE_GREATER_THAN_ZERO,
+  VALUE_MUST_BE_AN_INTEGER,
+} from '../../common/translations';
 
 export const createGetDynamicSettingsRoute: UMRestApiRouteFactory = (libs: UMServerLibs) => ({
   method: 'GET',
@@ -24,13 +27,19 @@ export const createGetDynamicSettingsRoute: UMRestApiRouteFactory = (libs: UMSer
   },
 });
 
-const validateCertsValues = (settings: DynamicSettings) => {
+export const validateCertsValues = (
+  settings: DynamicSettings
+): Record<string, string> | undefined => {
   const errors: any = {};
   if (settings.certAgeThreshold <= 0) {
-    errors.certAgeThreshold = VALUE_MUST_BE_GREATER_THEN_ZEO;
+    errors.certAgeThreshold = VALUE_MUST_BE_GREATER_THAN_ZERO;
+  } else if (settings.certAgeThreshold % 1) {
+    errors.certAgeThreshold = VALUE_MUST_BE_AN_INTEGER;
   }
   if (settings.certExpirationThreshold <= 0) {
-    errors.certExpirationThreshold = VALUE_MUST_BE_GREATER_THEN_ZEO;
+    errors.certExpirationThreshold = VALUE_MUST_BE_GREATER_THAN_ZERO;
+  } else if (settings.certExpirationThreshold % 1) {
+    errors.certExpirationThreshold = VALUE_MUST_BE_AN_INTEGER;
   }
   if (errors.certAgeThreshold || errors.certExpirationThreshold) {
     return errors;