Clean up validation logic

uyuni-project · Oct 2, 2024 · 71e3797 · 71e3797
1 parent 55afbdc
commit 71e3797
Show file tree

Hide file tree

Showing 14 changed files with 219 additions and 82 deletions.
diff --git a/web/html/src/components/input/InputBase.tsx b/web/html/src/components/input/InputBase.tsx
@@ -168,18 +168,7 @@ export class InputBase<ValueType = string> extends React.Component<InputBaseProp
   componentDidUpdate(prevProps) {
     // Revalidate when changing the following props on-the-fly
     if (this.props.required !== prevProps.required || this.props.disabled !== prevProps.disabled) {
-      const name = this.props.name;
-      if (name instanceof Array) {
-        const values = Object.keys(this.context.model).reduce((filtered, key) => {
-          if (name.includes(key)) {
-            filtered[key] = this.context.model[key];
-          }
-          return filtered;
-        }, {});
-        this.validate(values);
-      } else if (typeof name !== "undefined") {
-        this.validate(this.context.model[name]);
-      }
+      this.validate(this.getModelValue());
     }
   }
 
@@ -200,26 +189,40 @@ export class InputBase<ValueType = string> extends React.Component<InputBaseProp
     });
   };
 
+  getModelValue() {
+    const name = this.props.name;
+    if (Array.isArray(name)) {
+      const values = Object.keys(this.context.model).reduce((filtered, key) => {
+        if (name.includes(key)) {
+          filtered[key] = this.context.model[key];
+        }
+        return filtered;
+      }, {} as ValueType);
+      return values;
+    } else if (typeof name !== "undefined") {
+      return this.context.model[name];
+    }
+  }
+
   isEmptyValue(input: unknown) {
     if (typeof input === "string") {
       return input.trim() === "";
     }
     return _isNil(input);
   }
 
-  // TODO: Move this into the renderer and cache it
-  requiredValidator: Validator = <T extends ValueType>(value: T) => {
+  requiredHint = () => {
+    const value = this.getModelValue();
     const hasNoValue =
       this.isEmptyValue(value) ||
-      // TODO: Fix types
       (Array.isArray(this.props.name) && Object.values(value).filter((v) => !this.isEmptyValue(v)).length === 0);
 
     if (hasNoValue) {
       if (typeof this.props.required === "string") {
         return this.props.required;
       }
 
-      return this.props.label ? t(`${this.props.label} is required.`) : t("required");
+      return this.props.label ? t(`${this.props.label} is required.`) : t("Required");
     }
   };
 
@@ -234,11 +237,6 @@ export class InputBase<ValueType = string> extends React.Component<InputBaseProp
     async <InferredValueType extends unknown = ValueType>(value: InferredValueType): Promise<void> => {
       const validators = Array.isArray(this.props.validate) ? this.props.validate : [this.props.validate] ?? [];
 
-      // TODO: Move this into render so it's always sync and up to date instantly
-      if (!this.props.disabled && this.props.required) {
-        validators.push(this.requiredValidator);
-      }
-
       /**
        * Each validator sets its own result independently, this way we can mix and match different speed async
        * validators without having to wait all of them to finish
@@ -309,10 +307,14 @@ export class InputBase<ValueType = string> extends React.Component<InputBaseProp
       this.validate(value);
     }
 
-    if (this.props.onChange) this.props.onChange(name, value);
+    this.props.onChange?.(name, value);
   };
 
-  pushHint(hints: React.ReactNode[], hint?: ValidationResult) {
+  pushHint(hints: React.ReactNode[], hint?: React.ReactNode) {
+    if (!hint) {
+      return;
+    }
+
     if (Array.isArray(hint)) {
       hint.forEach((item) => this.pushHint(hints, item));
       return;
@@ -330,7 +332,13 @@ export class InputBase<ValueType = string> extends React.Component<InputBaseProp
     const hints: React.ReactNode[] = [];
     this.pushHint(hints, this.props.hint);
     this.state.formErrors?.forEach((error) => this.pushHint(hints, error));
-    this.state.validationErrors.forEach((error) => this.pushHint(hints, error));
+    if (this.state.isTouched) {
+      this.state.validationErrors.forEach((error) => this.pushHint(hints, error));
+
+      if (this.props.required && !this.props.disabled) {
+        this.pushHint(hints, this.requiredHint());
+      }
+    }
 
     const hasError = this.state.isTouched && !this.isValid();
 

diff --git a/web/html/src/components/input/range/Range.test.tsx b/web/html/src/components/input/range/Range.test.tsx
@@ -74,7 +74,7 @@ describe("Range", () => {
           if (!hasValues) {
             return message;
           }
-          // TODO: Replace this with Validate.isInteger or similar
+
           const isInteger = Object.values(value).every((item) => typeof item === "string" && item.match(/^[0-9]+$/));
           if (!isInteger) {
             return message;

diff --git a/web/html/src/components/input/validation/async.stories.tsx b/web/html/src/components/input/validation/async.stories.tsx
@@ -18,7 +18,7 @@ export default () => {
   return (
     <Form model={model} onChange={setModel}>
       <p>Async validation with debounce:</p>
-      <Text name="foo" validate={[asyncValidator]} debounceValidate={500} />
+      <Text name="foo" required validate={asyncValidator} debounceValidate={500} />
     </Form>
   );
 };
diff --git a/web/html/src/components/input/validation/multiple-messages.stories.tsx b/web/html/src/components/input/validation/multiple-messages.stories.tsx
@@ -14,7 +14,7 @@ export default () => {
   return (
     <Form model={model} onChange={setModel}>
       <p>You can return multiple validation errors:</p>
-      <Text name="foo" validate={[validator]} />
+      <Text name="foo" validate={validator} />
     </Form>
   );
 };
diff --git a/web/html/src/components/input/validation/validation.test.ts b/web/html/src/components/input/validation/validation.test.ts
@@ -3,9 +3,21 @@ import { Validation } from "./validation";
 const errorMessage = "error message";
 
 describe("validation", () => {
+  test("matches", () => {
+    const validator = Validation.matches(/foo/, errorMessage);
+
+    expect(validator("")).toEqual(undefined);
+    expect(validator("-")).toEqual(errorMessage);
+    expect(validator("-foo")).toEqual(undefined);
+    expect(validator("-foo-")).toEqual(undefined);
+    expect(validator("-fo-")).toEqual(errorMessage);
+  });
+
   test("minLength string", () => {
     const validator = Validation.minLength(3, errorMessage);
 
+    // Here and elsewhere, if you want the value to be required, set the `required` flag instead
+    expect(validator("")).toEqual(undefined);
     expect(validator("foo")).toEqual(undefined);
     expect(validator("fo")).toEqual(errorMessage);
   });
@@ -33,6 +45,7 @@ describe("validation", () => {
   test("maxLength string", () => {
     const validator = Validation.maxLength(3, errorMessage);
 
+    expect(validator("")).toEqual(undefined);
     expect(validator("foo")).toEqual(undefined);
     expect(validator("fooo")).toEqual(errorMessage);
   });
@@ -58,19 +71,58 @@ describe("validation", () => {
   });
 
   test("isInt", () => {
-    const validator = Validation.isInt();
-
-    // If you want the value to be required, set the `required` flag instead
-    expect(validator("")).toEqual(true);
-    expect(validator("0")).toEqual(true);
-    expect(validator("42")).toEqual(true);
-    expect(validator("42.")).toEqual(false);
-    expect(validator("4.2")).toEqual(false);
-    expect(validator("0x1")).toEqual(false);
-    expect(validator("foo")).toEqual(false);
+    const validator = Validation.isInt(errorMessage);
+
+    expect(validator("")).toEqual(undefined);
+    expect(validator("0")).toEqual(undefined);
+    expect(validator("42")).toEqual(undefined);
+    expect(validator("42.")).toEqual(errorMessage);
+    expect(validator("4.2")).toEqual(errorMessage);
+    expect(validator("0x1")).toEqual(errorMessage);
+    expect(validator("foo")).toEqual(errorMessage);
   });
 
-  test("matches", () => {
-    // TODO: Implement
+  test("min", () => {
+    const validator = Validation.min(7, errorMessage);
+
+    expect(validator("")).toEqual(undefined);
+    expect(validator("6")).toEqual(errorMessage);
+    expect(validator("7")).toEqual(undefined);
+    expect(validator("8")).toEqual(undefined);
+  });
+
+  test("max", () => {
+    const validator = Validation.max(7, errorMessage);
+
+    expect(validator("")).toEqual(undefined);
+    expect(validator("6")).toEqual(undefined);
+    expect(validator("7")).toEqual(undefined);
+    expect(validator("8")).toEqual(errorMessage);
+  });
+
+  test("intRange", () => {
+    const validator = Validation.intRange(3, 5, errorMessage);
+
+    expect(validator("")).toEqual(undefined);
+    expect(validator("1.5")).toEqual(errorMessage);
+    expect(validator("2")).toEqual(errorMessage);
+    expect(validator("3")).toEqual(undefined);
+    expect(validator("4")).toEqual(undefined);
+    expect(validator("4.5")).toEqual(errorMessage);
+    expect(validator("5")).toEqual(undefined);
+    expect(validator("6")).toEqual(errorMessage);
+  });
+
+  test("floatRange", () => {
+    const validator = Validation.floatRange(3, 5, errorMessage);
+
+    expect(validator("")).toEqual(undefined);
+    expect(validator("1.5")).toEqual(errorMessage);
+    expect(validator("2")).toEqual(errorMessage);
+    expect(validator("3")).toEqual(undefined);
+    expect(validator("4")).toEqual(undefined);
+    expect(validator("4.5")).toEqual(undefined);
+    expect(validator("5")).toEqual(undefined);
+    expect(validator("6")).toEqual(errorMessage);
   });
 });
diff --git a/web/html/src/components/input/validation/validation.ts b/web/html/src/components/input/validation/validation.ts
@@ -1,15 +1,18 @@
-// TODO: Add 100% test coverage to this whole file
-
 type OneOrMany<T> = T | T[];
 type SyncOrAsync<T> = T | Promise<T>;
 
-export type ValidationResult = OneOrMany<Exclude<React.ReactNode, boolean> | undefined>;
+export type ValidationResult = OneOrMany<Exclude<JSX.Element | string, boolean> | undefined>;
 export type Validator = (...args: any[]) => SyncOrAsync<ValidationResult>;
 
 /** String must match `regex` */
 const matches =
   (regex: RegExp, message?: string): Validator =>
   (value: string) => {
+    // Here and elsewhere, if you want the value to be required, set the `required` flag instead
+    if (value === "") {
+      return;
+    }
+
     if (!regex.test(value)) {
       return message ?? t("Doesn't match expected format");
     }
@@ -22,11 +25,11 @@ const minLength =
   (value: Record<string, string> | string) => {
     const defaultMessage = t(`Must be at least ${length} characters long`);
     if (typeof value === "object") {
-      const isInvalid = Object.values(value).some((item) => item.length < length);
+      const isInvalid = Object.values(value).some((item) => item.length !== 0 && item.length < length);
       if (isInvalid) {
         return message ?? defaultMessage;
       }
-    } else if (value.length < length) {
+    } else if (value.length !== 0 && value.length < length) {
       return message ?? defaultMessage;
     }
   };
@@ -37,15 +40,16 @@ const maxLength =
   (value: Record<string, string> | string) => {
     const defaultMessage = t(`Must be no more than ${length} characters long`);
     if (typeof value === "object") {
-      const isInvalid = Object.values(value).some((item) => item.length > length);
+      const isInvalid = Object.values(value).some((item) => item.length !== 0 && item.length > length);
       if (isInvalid) {
         return message ?? defaultMessage;
       }
-    } else if (value.length > length) {
+    } else if (value.length !== 0 && value.length > length) {
       return message ?? defaultMessage;
     }
   };
 
+/** String is integer */
 const isInt =
   (message?: string): Validator =>
   (value: string) => {
@@ -59,9 +63,55 @@ const isInt =
     }
   };
 
+/** Value is no smaller than `minValue` */
+const min =
+  (minValue: number, message?: string): Validator =>
+  (value: string) => {
+    if (value === "") {
+      return;
+    }
+
+    const parsed = parseFloat(value);
+    if (isNaN(parsed) || parsed < minValue) {
+      return message ?? t(`Must be larger than ${minValue}`);
+    }
+  };
+
+/** Value is no larger than `maxValue` */
+const max =
+  (maxValue: number, message?: string): Validator =>
+  (value: string) => {
+    if (value === "") {
+      return;
+    }
+
+    const parsed = parseFloat(value);
+    if (isNaN(parsed) || parsed > maxValue) {
+      return message ?? t(`Must be smaller than ${maxValue}`);
+    }
+  };
+
+/** Value is an integer that is no smaller than `minValue` and no larger than `maxValue` */
+const intRange =
+  (minValue: number, maxValue: number, message?: string): Validator =>
+  (value: string) => {
+    return isInt(message)(value) || min(minValue, message)(value) || max(maxValue, message)(value);
+  };
+
+/** Value is a number that is no smaller than `minValue` and no larger than `maxValue` */
+const floatRange =
+  (minValue: number, maxValue: number, message?: string): Validator =>
+  (value: string) => {
+    return min(minValue, message)(value) || max(maxValue, message)(value);
+  };
+
 export const Validation = {
   matches,
   minLength,
   maxLength,
+  min,
+  max,
   isInt,
+  intRange,
+  floatRange,
 };
diff --git a/web/html/src/manager/images/image-profile-edit.tsx b/web/html/src/manager/images/image-profile-edit.tsx
@@ -168,19 +168,22 @@ class CreateImageProfile extends React.Component<Props, State> {
     }
   }
 
-  isLabelValid = (label) => {
+  isLabelValid = async (label: string) => {
     if (this.state.initLabel && this.state.initLabel === label) {
       // Initial state (on edit), always valid.
-      return true;
+      return;
     }
 
     // Should not contain ':' character
-    const isValid = label.indexOf(":") === -1;
+    const matchesFormat = label.indexOf(":") === -1;
 
     // Check for uniqueness
-    return Network.get("/rhn/manager/api/cm/imageprofiles/find/" + label)
-      .then((res) => !res.success && isValid)
+    const isValid = Network.get("/rhn/manager/api/cm/imageprofiles/find/" + label)
+      .then((res) => !res.success && matchesFormat)
       .catch(() => false);
+    if (!isValid) {
+      return t("Label is required and must be a unique string and it cannot include any colons (:).");
+    }
   };
 
   onUpdate = (model) => {
@@ -527,8 +530,7 @@ class CreateImageProfile extends React.Component<Props, State> {
             name="label"
             label={t("Label")}
             required
-            validate={[this.isLabelValid]}
-            invalidHint={t("Label is required and must be a unique string and it cannot include any colons (:).")}
+            validate={this.isLabelValid}
             labelClass="col-md-3"
             divClass="col-md-6"
           />