fix: avoid validating boolean and undefined parameter values (#1244)

* fix: avoid validating boolean parameter values

* fix: avoid validating undefined parameter values

src/utils/validateParameterValue.test.ts

@@ -1,39 +1,67 @@
 import { validateParameterValue } from 'utils/validateParameterValue';
 
+const createStrategy = (type: string, required: boolean) => {
+    return { type, required };
+};
+
 test('validateParameterValue string', () => {
     const fn = validateParameterValue;
-    expect(fn({ type: 'string', required: false }, '')).toBeUndefined();
-    expect(fn({ type: 'string', required: false }, 'a')).toBeUndefined();
-    expect(fn({ type: 'string', required: true }, '')).not.toBeUndefined();
-    expect(fn({ type: 'string', required: true }, 'a')).toBeUndefined();
+    expect(fn(createStrategy('string', false), undefined)).toBeUndefined();
+    expect(fn(createStrategy('string', false), '')).toBeUndefined();
+    expect(fn(createStrategy('string', true), 'a')).toBeUndefined();
+    expect(fn(createStrategy('string', true), undefined)).not.toBeUndefined();
+    expect(fn(createStrategy('string', true), '')).not.toBeUndefined();
+    expect(fn(createStrategy('string', true), 'a')).toBeUndefined();
 });
 
 test('validateParameterValue list', () => {
     const fn = validateParameterValue;
-    expect(fn({ type: 'list', required: false }, '')).toBeUndefined();
-    expect(fn({ type: 'list', required: false }, 'a,b')).toBeUndefined();
-    expect(fn({ type: 'list', required: true }, '')).not.toBeUndefined();
-    expect(fn({ type: 'list', required: true }, 'a,b')).toBeUndefined();
+    expect(fn(createStrategy('list', false), undefined)).toBeUndefined();
+    expect(fn(createStrategy('list', false), '')).toBeUndefined();
+    expect(fn(createStrategy('list', false), 'a,b')).toBeUndefined();
+    expect(fn(createStrategy('list', true), undefined)).not.toBeUndefined();
+    expect(fn(createStrategy('list', true), '')).not.toBeUndefined();
+    expect(fn(createStrategy('list', true), 'a,b')).toBeUndefined();
 });
 
 test('validateParameterValue number', () => {
     const fn = validateParameterValue;
-    expect(fn({ type: 'number', required: false }, '')).toBeUndefined();
-    expect(fn({ type: 'number', required: false }, 'a')).not.toBeUndefined();
-    expect(fn({ type: 'number', required: false }, '1')).toBeUndefined();
-    expect(fn({ type: 'number', required: true }, '')).not.toBeUndefined();
-    expect(fn({ type: 'number', required: true }, 'a')).not.toBeUndefined();
-    expect(fn({ type: 'number', required: true }, '1')).toBeUndefined();
+    expect(fn(createStrategy('number', false), undefined)).toBeUndefined();
+    expect(fn(createStrategy('number', false), '')).toBeUndefined();
+    expect(fn(createStrategy('number', false), 'a')).not.toBeUndefined();
+    expect(fn(createStrategy('number', false), '0')).toBeUndefined();
+    expect(fn(createStrategy('number', false), '1')).toBeUndefined();
+    expect(fn(createStrategy('number', true), undefined)).not.toBeUndefined();
+    expect(fn(createStrategy('number', true), '')).not.toBeUndefined();
+    expect(fn(createStrategy('number', true), 'a')).not.toBeUndefined();
+    expect(fn(createStrategy('number', true), '0')).toBeUndefined();
+    expect(fn(createStrategy('number', true), '1')).toBeUndefined();
 });
 
 test('validateParameterValue boolean', () => {
     const fn = validateParameterValue;
-    expect(fn({ type: 'boolean', required: false }, '')).toBeUndefined();
-    expect(fn({ type: 'boolean', required: false }, 'true')).toBeUndefined();
-    expect(fn({ type: 'boolean', required: false }, 'false')).toBeUndefined();
-    expect(fn({ type: 'boolean', required: false }, 'a')).not.toBeUndefined();
-    expect(fn({ type: 'boolean', required: true }, '')).toBeUndefined();
-    expect(fn({ type: 'boolean', required: true }, 'true')).toBeUndefined();
-    expect(fn({ type: 'boolean', required: true }, 'false')).toBeUndefined();
-    expect(fn({ type: 'boolean', required: true }, 'a')).not.toBeUndefined();
+    expect(fn(createStrategy('boolean', false), undefined)).toBeUndefined();
+    expect(fn(createStrategy('boolean', false), '')).toBeUndefined();
+    expect(fn(createStrategy('boolean', false), 'true')).toBeUndefined();
+    expect(fn(createStrategy('boolean', false), 'false')).toBeUndefined();
+    expect(fn(createStrategy('boolean', false), 'a')).toBeUndefined();
+    expect(fn(createStrategy('boolean', true), undefined)).toBeUndefined();
+    expect(fn(createStrategy('boolean', true), '')).toBeUndefined();
+    expect(fn(createStrategy('boolean', true), 'true')).toBeUndefined();
+    expect(fn(createStrategy('boolean', true), 'false')).toBeUndefined();
+    expect(fn(createStrategy('boolean', true), 'a')).toBeUndefined();
+});
+
+test('validateParameterValue percentage', () => {
+    const fn = validateParameterValue;
+    expect(fn(createStrategy('percentage', false), undefined)).toBeUndefined();
+    expect(fn(createStrategy('percentage', false), '')).toBeUndefined();
+    expect(fn(createStrategy('percentage', false), 'a')).toBeUndefined();
+    expect(fn(createStrategy('percentage', false), '0')).toBeUndefined();
+    expect(fn(createStrategy('percentage', false), '1')).toBeUndefined();
+    expect(fn(createStrategy('percentage', true), undefined)).toBeUndefined();
+    expect(fn(createStrategy('percentage', true), '')).toBeUndefined();
+    expect(fn(createStrategy('percentage', true), 'a')).toBeUndefined();
+    expect(fn(createStrategy('percentage', true), '0')).toBeUndefined();
+    expect(fn(createStrategy('percentage', true), '1')).toBeUndefined();
 });

src/utils/validateParameterValue.ts

@@ -9,28 +9,27 @@ export const validateParameterValue = (
 ): string | undefined => {
     const { type, required } = definition;
 
-    // The components for booleans and percentages can't yet show error messages.
-    // We should not enforce `required` until these errors can be shown in the UI.
-    const shouldValidateRequired =
-        type === 'string' || type === 'list' || type === 'number';
-    if (shouldValidateRequired && required && value === '') {
-        return 'Field is required';
+    // Some input components in the feature strategy form can't show error messages.
+    // We should not validate those fields until their errors can be shown in the UI.
+    if (type !== 'string' && type !== 'list' && type !== 'number') {
+        return;
     }
 
-    const shouldValidateNumeric = type === 'percentage' || type === 'number';
-    if (shouldValidateNumeric && !isValidNumberOrEmpty(value)) {
-        return 'Not a valid number.';
+    // If we're editing a feature strategy that has a newly added parameter,
+    // the value will be `undefined` until the field has been edited.
+    if (required && (typeof value === 'undefined' || value === '')) {
+        return 'Field is required';
     }
 
-    if (type === 'boolean' && !isValidBooleanOrEmpty(value)) {
-        return 'Not a valid boolean.';
+    if (type === 'number' && !isValidNumberOrEmpty(value)) {
+        return 'Not a valid number.';
     }
 };
 
 const isValidNumberOrEmpty = (value: string | number | undefined): boolean => {
-    return value === '' || /^\d+$/.test(String(value));
-};
-
-const isValidBooleanOrEmpty = (value: string | number | undefined): boolean => {
-    return value === '' || value === 'true' || value === 'false';
+    return (
+        typeof value === 'undefined' ||
+        value === '' ||
+        /^\d+$/.test(String(value))
+    );
 };