From 0afb1eadcc48126b7f0605696f07804952549605 Mon Sep 17 00:00:00 2001
From: olav <mail@olav.io>
Date: Mon, 22 Aug 2022 16:35:51 +0200
Subject: [PATCH] fix: relax validation for required params (#1238)

* fix: relax validation for required params

* refactor: inline parameter type checkers
---
 .../src/utils/validateParameterValue.test.ts  | 80 ++++++++-----------
 frontend/src/utils/validateParameterValue.ts  | 19 ++++-
 2 files changed, 48 insertions(+), 51 deletions(-)

diff --git a/frontend/src/utils/validateParameterValue.test.ts b/frontend/src/utils/validateParameterValue.test.ts
index 81909bf18e..0186607fec 100644
--- a/frontend/src/utils/validateParameterValue.test.ts
+++ b/frontend/src/utils/validateParameterValue.test.ts
@@ -1,55 +1,39 @@
 import { validateParameterValue } from 'utils/validateParameterValue';
 
 test('validateParameterValue string', () => {
-    expect(
-        validateParameterValue(
-            { type: 'string', name: 'a', description: 'b', required: false },
-            ''
-        )
-    ).toBeUndefined();
-    expect(
-        validateParameterValue(
-            { type: 'string', name: 'a', description: 'b', required: false },
-            'a'
-        )
-    ).toBeUndefined();
-    expect(
-        validateParameterValue(
-            { type: 'string', name: 'a', description: 'b', required: true },
-            ''
-        )
-    ).not.toBeUndefined();
-    expect(
-        validateParameterValue(
-            { type: 'string', name: 'a', description: 'b', required: true },
-            'b'
-        )
-    ).toBeUndefined();
+    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();
+});
+
+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();
 });
 
 test('validateParameterValue number', () => {
-    expect(
-        validateParameterValue(
-            { type: 'number', name: 'a', description: 'b', required: false },
-            ''
-        )
-    ).toBeUndefined();
-    expect(
-        validateParameterValue(
-            { type: 'number', name: 'a', description: 'b', required: false },
-            'a'
-        )
-    ).not.toBeUndefined();
-    expect(
-        validateParameterValue(
-            { type: 'number', name: 'a', description: 'b', required: true },
-            ''
-        )
-    ).not.toBeUndefined();
-    expect(
-        validateParameterValue(
-            { type: 'number', name: 'a', description: 'b', required: true },
-            '1'
-        )
-    ).toBeUndefined();
+    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();
+});
+
+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();
 });
diff --git a/frontend/src/utils/validateParameterValue.ts b/frontend/src/utils/validateParameterValue.ts
index 2bb6295d40..6f011d93d7 100644
--- a/frontend/src/utils/validateParameterValue.ts
+++ b/frontend/src/utils/validateParameterValue.ts
@@ -4,20 +4,33 @@ import {
 } from 'interfaces/strategy';
 
 export const validateParameterValue = (
-    definition: IStrategyParameter,
+    definition: Pick<IStrategyParameter, 'type' | 'required'>,
     value: IFeatureStrategyParameters[string]
 ): string | undefined => {
     const { type, required } = definition;
 
-    if (required && value === '') {
+    // 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';
     }
 
-    if (type === 'number' && !isValidNumberOrEmpty(value)) {
+    const shouldValidateNumeric = type === 'percentage' || type === 'number';
+    if (shouldValidateNumeric && !isValidNumberOrEmpty(value)) {
         return 'Not a valid number.';
     }
+
+    if (type === 'boolean' && !isValidBooleanOrEmpty(value)) {
+        return 'Not a valid boolean.';
+    }
 };
 
 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';
+};