From f798c07fdecb39be11c00e0bce15c7645b76a009 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Mon, 18 Nov 2024 20:13:25 +0100 Subject: [PATCH 1/3] Validation Adds a model-level validation capability to our validation library. --- .../FieldMultiInput/FieldMultiInput.story.tsx | 22 +- .../FieldMultiInput/FieldMultiInput.test.tsx | 54 ++++- .../FieldMultiInput/FieldMultiInput.tsx | 36 +++- .../FieldSelect/FieldSelect.story.tsx | 5 +- .../components/Validation/Validation.jsx | 105 ---------- .../components/Validation/Validation.test.tsx | 101 +++++++--- .../components/Validation/Validation.tsx | 190 ++++++++++++++++++ .../components/Validation/rules.test.ts | 59 ++++++ .../shared/components/Validation/rules.ts | 81 ++++++++ .../shared/components/Validation/useRule.js | 8 +- .../LabelsInput/LabelsInput.test.tsx | 119 +++++++++++ .../components/LabelsInput/LabelsInput.tsx | 71 ++++++- 12 files changed, 697 insertions(+), 154 deletions(-) delete mode 100644 web/packages/shared/components/Validation/Validation.jsx create mode 100644 web/packages/shared/components/Validation/Validation.tsx diff --git a/web/packages/shared/components/FieldMultiInput/FieldMultiInput.story.tsx b/web/packages/shared/components/FieldMultiInput/FieldMultiInput.story.tsx index 5362236a8b24d..2f798d4d923d1 100644 --- a/web/packages/shared/components/FieldMultiInput/FieldMultiInput.story.tsx +++ b/web/packages/shared/components/FieldMultiInput/FieldMultiInput.story.tsx @@ -20,6 +20,12 @@ import React, { useState } from 'react'; import Box from 'design/Box'; +import { Button } from 'design/Button'; + +import Validation from 'shared/components/Validation'; + +import { arrayOf, requiredField } from '../Validation/rules'; + import { FieldMultiInput } from './FieldMultiInput'; export default { @@ -30,7 +36,21 @@ export function Story() { const [items, setItems] = useState([]); return ( - + + {({ validator }) => ( + <> + + + + )} + ); } diff --git a/web/packages/shared/components/FieldMultiInput/FieldMultiInput.test.tsx b/web/packages/shared/components/FieldMultiInput/FieldMultiInput.test.tsx index ce023a071053a..89b191e1e5b2d 100644 --- a/web/packages/shared/components/FieldMultiInput/FieldMultiInput.test.tsx +++ b/web/packages/shared/components/FieldMultiInput/FieldMultiInput.test.tsx @@ -19,20 +19,36 @@ import userEvent from '@testing-library/user-event'; import React, { useState } from 'react'; -import { render, screen } from 'design/utils/testing'; +import { act, render, screen } from 'design/utils/testing'; + +import Validation, { Validator } from 'shared/components/Validation'; + +import { arrayOf, requiredField } from '../Validation/rules'; import { FieldMultiInput, FieldMultiInputProps } from './FieldMultiInput'; const TestFieldMultiInput = ({ onChange, + refValidator, ...rest -}: Partial) => { +}: Partial & { + refValidator?: (v: Validator) => void; +}) => { const [items, setItems] = useState([]); const handleChange = (it: string[]) => { setItems(it); onChange?.(it); }; - return ; + return ( + + {({ validator }) => { + refValidator?.(validator); + return ( + + ); + }} + + ); }; test('adding, editing, and removing items', async () => { @@ -69,3 +85,35 @@ test('keyboard handling', async () => { await user.keyboard('{Enter}bananas'); expect(onChange).toHaveBeenLastCalledWith(['apples', 'bananas', 'oranges']); }); + +test('validation', async () => { + const user = userEvent.setup(); + let validator: Validator; + render( + { + validator = v; + }} + rule={arrayOf(requiredField('required'))} + /> + ); + + act(() => validator.validate()); + expect(validator.state.valid).toBe(true); + expect(screen.getByRole('textbox')).toHaveAccessibleDescription(''); + + await user.click(screen.getByRole('button', { name: 'Add More' })); + await user.type(screen.getAllByRole('textbox')[1], 'foo'); + act(() => validator.validate()); + expect(validator.state.valid).toBe(false); + expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription( + 'required' + ); + expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription(''); + + await user.type(screen.getAllByRole('textbox')[0], 'foo'); + act(() => validator.validate()); + expect(validator.state.valid).toBe(true); + expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription(''); + expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription(''); +}); diff --git a/web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx b/web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx index e1dbace8c97d5..f323ec7dd268d 100644 --- a/web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx +++ b/web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx @@ -21,15 +21,35 @@ import { ButtonSecondary } from 'design/Button'; import ButtonIcon from 'design/ButtonIcon'; import Flex from 'design/Flex'; import * as Icon from 'design/Icon'; -import Input from 'design/Input'; import { useRef } from 'react'; import styled, { useTheme } from 'styled-components'; +import { + precomputed, + Rule, + ValidationResult, +} from 'shared/components/Validation/rules'; +import { useRule } from 'shared/components/Validation'; + +import FieldInput from '../FieldInput'; + +type StringListValidationResult = ValidationResult & { + /** + * A list of validation results, one per label. Note: items are optional just + * because `useRule` by default returns only `ValidationResult`. For the + * actual validation, it's not optional; if it's undefined, or there are + * fewer items in this list than the labels, the corresponding items will be + * treated as valid. + */ + items?: ValidationResult[]; +}; + export type FieldMultiInputProps = { label?: string; value: string[]; disabled?: boolean; onChange?(val: string[]): void; + rule?: Rule; }; /** @@ -45,7 +65,13 @@ export function FieldMultiInput({ value, disabled, onChange, + rule = defaultRule, }: FieldMultiInputProps) { + // It's important to first validate, and then treat an empty array as a + // single-item list with an empty string, since this "synthetic" empty + // string is technically not a part of the model and should not be + // validated. + const validationResult: StringListValidationResult = useRule(rule(value)); if (value.length === 0) { value = ['']; } @@ -90,8 +116,11 @@ export function FieldMultiInput({ // procedure whose complexity would probably outweigh the benefits). - onChange?.( @@ -99,6 +128,7 @@ export function FieldMultiInput({ ) } onKeyDown={e => handleKeyDown(i, e)} + mb={0} /> () => ({ valid: true }); + const Fieldset = styled.fieldset` border: none; margin: 0; diff --git a/web/packages/shared/components/FieldSelect/FieldSelect.story.tsx b/web/packages/shared/components/FieldSelect/FieldSelect.story.tsx index cecfa4e84dac0..dde0dc2b97dce 100644 --- a/web/packages/shared/components/FieldSelect/FieldSelect.story.tsx +++ b/web/packages/shared/components/FieldSelect/FieldSelect.story.tsx @@ -53,7 +53,10 @@ export function Default() { return ( {({ validator }) => { - validator.validate(); + // Prevent rendering loop. + if (!validator.state.validating) { + validator.validate(); + } return ( . - */ - -import React from 'react'; - -import { isObject } from 'shared/utils/highbar'; - -import Logger from '../../libs/logger'; - -const logger = Logger.create('validation'); - -// Validator handles input validation -export default class Validator { - valid = true; - - constructor() { - // store subscribers - this._subs = []; - } - - // adds a callback to the list of subscribers - subscribe(cb) { - this._subs.push(cb); - } - - // removes a callback from the list of subscribers - unsubscribe(cb) { - const index = this._subs.indexOf(cb); - if (index > -1) { - this._subs.splice(index, 1); - } - } - - addResult(result) { - // result can be a boolean value or an object - let isValid = false; - if (isObject(result)) { - isValid = result.valid; - } else { - logger.error(`rule should return a valid object`); - } - - this.valid = this.valid && Boolean(isValid); - } - - reset() { - this.valid = true; - this.validating = false; - } - - validate() { - this.reset(); - this.validating = true; - this._subs.forEach(cb => { - try { - cb(); - } catch (err) { - logger.error(err); - } - }); - - return this.valid; - } -} - -const ValidationContext = React.createContext({}); - -export function Validation(props) { - const [validator] = React.useState(() => new Validator()); - // handle render functions - const children = - typeof props.children === 'function' - ? props.children({ validator }) - : props.children; - - return ( - - {children} - - ); -} - -export function useValidation() { - const value = React.useContext(ValidationContext); - if (!(value instanceof Validator)) { - logger.warn('Missing Validation Context declaration'); - } - - return value; -} diff --git a/web/packages/shared/components/Validation/Validation.test.tsx b/web/packages/shared/components/Validation/Validation.test.tsx index 19f40a8d44986..a933e9d916af9 100644 --- a/web/packages/shared/components/Validation/Validation.test.tsx +++ b/web/packages/shared/components/Validation/Validation.test.tsx @@ -17,32 +17,22 @@ */ import React from 'react'; +import { render, fireEvent, screen, act } from 'design/utils/testing'; -import { render, fireEvent, screen } from 'design/utils/testing'; +import Validator, { Result, Validation, useValidation } from './Validation'; -import Logger from '../../libs/logger'; - -import Validator, { Validation, useValidation } from './Validation'; - -jest.mock('../../libs/logger', () => { - const mockLogger = { - error: jest.fn(), - warn: jest.fn(), - }; - - return { - create: () => mockLogger, - }; +afterEach(() => { + jest.restoreAllMocks(); }); -test('methods of Validator: sub, unsub, validate', () => { +test('methods of Validator: addRuleCallback, removeRuleCallback, validate', () => { const mockCb1 = jest.fn(); const mockCb2 = jest.fn(); const validator = new Validator(); // test suscribe - validator.subscribe(mockCb1); - validator.subscribe(mockCb2); + validator.addRuleCallback(mockCb1); + validator.addRuleCallback(mockCb2); // test validate runs all subscribed cb's expect(validator.validate()).toBe(true); @@ -51,42 +41,42 @@ test('methods of Validator: sub, unsub, validate', () => { jest.clearAllMocks(); // test unsubscribe method removes correct cb - validator.unsubscribe(mockCb2); + validator.removeRuleCallback(mockCb2); expect(validator.validate()).toBe(true); expect(mockCb1).toHaveBeenCalledTimes(1); expect(mockCb2).toHaveBeenCalledTimes(0); }); test('methods of Validator: addResult, reset', () => { + const consoleError = jest.spyOn(console, 'error').mockImplementation(); const validator = new Validator(); // test addResult for nil object const result = null; validator.addResult(result); - expect(Logger.create().error).toHaveBeenCalledTimes(1); + expect(consoleError).toHaveBeenCalledTimes(1); // test addResult for boolean validator.addResult(true); - expect(validator.valid).toBe(false); + expect(validator.state.valid).toBe(false); // test addResult with incorrect object - let resultObj = {}; - validator.addResult(resultObj); - expect(validator.valid).toBe(false); + validator.addResult({} as Result); + expect(validator.state.valid).toBe(false); // test addResult with correct object with "valid" prop from prior test set to false - resultObj = { valid: true }; + let resultObj = { valid: true }; validator.addResult(resultObj); - expect(validator.valid).toBe(false); + expect(validator.state.valid).toBe(false); // test reset validator.reset(); - expect(validator.valid).toBe(true); - expect(validator.validating).toBe(false); + expect(validator.state.valid).toBe(true); + expect(validator.state.validating).toBe(false); // test addResult with correct object with "valid" prop reset to true validator.addResult(resultObj); - expect(validator.valid).toBe(true); + expect(validator.state.valid).toBe(true); }); test('trigger validation via useValidation hook', () => { @@ -102,7 +92,7 @@ test('trigger validation via useValidation hook', () => { ); fireEvent.click(screen.getByRole('button')); - expect(validator.validating).toBe(true); + expect(validator.state.validating).toBe(true); }); test('trigger validation via render function', () => { @@ -122,5 +112,56 @@ test('trigger validation via render function', () => { ); fireEvent.click(screen.getByRole('button')); - expect(validator.validating).toBe(true); + expect(validator.state.validating).toBe(true); +}); + +test('rendering validation result via useValidation hook', () => { + let validator: Validator; + const TestComponent = () => { + validator = useValidation(); + return ( + <> +
Validating: {String(validator.state.validating)}
+
Valid: {String(validator.state.valid)}
+ + ); + }; + render( + + + + ); + validator.addRuleCallback(() => validator.addResult({ valid: false })); + + expect(screen.getByText('Validating: false')).toBeInTheDocument(); + expect(screen.getByText('Valid: true')).toBeInTheDocument(); + + act(() => validator.validate()); + expect(screen.getByText('Validating: true')).toBeInTheDocument(); + expect(screen.getByText('Valid: false')).toBeInTheDocument(); +}); + +test('rendering validation result via render function', () => { + let validator: Validator; + render( + + {props => { + validator = props.validator; + return ( + <> +
Validating: {String(validator.state.validating)}
+
Valid: {String(validator.state.valid)}
+ + ); + }} + + ); + validator.addRuleCallback(() => validator.addResult({ valid: false })); + + expect(screen.getByText('Validating: false')).toBeInTheDocument(); + expect(screen.getByText('Valid: true')).toBeInTheDocument(); + + act(() => validator.validate()); + expect(screen.getByText('Validating: true')).toBeInTheDocument(); + expect(screen.getByText('Valid: false')).toBeInTheDocument(); }); diff --git a/web/packages/shared/components/Validation/Validation.tsx b/web/packages/shared/components/Validation/Validation.tsx new file mode 100644 index 0000000000000..b032f452f4dba --- /dev/null +++ b/web/packages/shared/components/Validation/Validation.tsx @@ -0,0 +1,190 @@ +/* + * Teleport + * Copyright (C) 2023 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import React from 'react'; + +import { Logger } from 'design/logger'; + +import { isObject } from 'shared/utils/highbar'; +import { Store, useStore } from 'shared/libs/stores'; + +import { ValidationResult } from './rules'; + +const logger = new Logger('validation'); + +/** A per-rule callback that will be executed during validation. */ +type RuleCallback = () => void; + +export type Result = ValidationResult | boolean; + +type ValidatorState = { + /** Indicates whether the last validation was successful. */ + valid: boolean; + /** + * Indicates whether the validator has been activated by a call to + * `validate`. + */ + validating: boolean; +}; + +/** A store that handles input validation and makes its results accessible. */ +export default class Validator extends Store { + state = { + valid: true, + validating: false, + }; + + /** + * @deprecated For temporary Enterprise compatibility only. Use {@link state} + * instead. + */ + valid = true; + + /** Callbacks that will be executed upon validation. */ + private ruleCallbacks: RuleCallback[] = []; + + /** Adds a rule callback that will be executed upon validation. */ + addRuleCallback(cb: RuleCallback) { + this.ruleCallbacks.push(cb); + } + + /** Removes a rule callback. */ + removeRuleCallback(cb: RuleCallback) { + const index = this.ruleCallbacks.indexOf(cb); + if (index > -1) { + this.ruleCallbacks.splice(index, 1); + } + } + + addResult(result: Result) { + // result can be a boolean value or an object + let isValid = false; + if (isObject(result)) { + isValid = result.valid; + } else { + logger.error(`rule should return a valid object`); + } + + const valid = this.state.valid && Boolean(isValid); + this.setState({ valid }); + this.valid = valid; + } + + reset() { + this.setState({ + valid: true, + validating: false, + }); + this.valid = true; + } + + validate() { + this.reset(); + this.setState({ validating: true }); + for (const cb of this.ruleCallbacks) { + try { + cb(); + } catch (err) { + logger.error(err); + } + } + + return this.state.valid; + } +} + +const ValidationContext = React.createContext(undefined); + +type ValidationRenderFunction = (arg: { validator: Validator }) => any; + +/** + * Installs a validation context that provides a {@link Validator} store. The + * store can be retrieved either through {@link useValidation} hook or by a + * render callback, e.g.: + * + * ``` + * function Component() { + * return ( + * + * {({validator}) => ( + * <> + * (...) + * + * + * )} + * + * ); + * } + * ``` + * + * The simplest way to use validation is validating on the view layer: just use + * a `rule` prop with `FieldInput` or a similar component and pass a rule like + * `requiredField`. + * + * Unfortunately, due to architectural limitations, this will not work well in + * scenarios where information about validity about given field or group of + * fields is required outside that field. In cases like this, the best option + * is to validate the model during render time on the top level (for example, + * execute an entire set of rules on a model using `runRules`). The result of + * model validation will then contain information about the validity of each + * field. It can then be used wherever it's needed, and also attached to + * appropriate inputs with a `precomputed` validation rule. Example: + * + * ``` + * function Component(model: Model) { + * const rules = { + * name: requiredField('required'), + * email: requiredEmailLike, + * } + * const validationResult = runRules(model, rules); + * } + * ``` + * + * Note that, as this example shows clearly, the validator itself, despite its + * name, doesn't really validate anything -- it merely aggregates validation + * results. Also it's worth mentioning that the validator will not do it + * without our help -- each validated field needs to be actually attached to a + * field, even if using a `precomputed` rule, for this to work. The validation + * callbacks registered by validation rules on the particular fields are the + * actual points where the errors are consumed by the validator. + */ +export function Validation(props: { + children?: React.ReactNode | ValidationRenderFunction; +}) { + const [validator] = React.useState(() => new Validator()); + useStore(validator); + // handle render functions + const children = + typeof props.children === 'function' + ? props.children({ validator }) + : props.children; + + return ( + + {children} + + ); +} + +export function useValidation(): Validator | undefined { + const validator = React.useContext(ValidationContext); + if (!validator) { + logger.warn('Missing Validation Context declaration'); + } + return useStore(validator); +} diff --git a/web/packages/shared/components/Validation/rules.test.ts b/web/packages/shared/components/Validation/rules.test.ts index a07b16fb7aaa7..1ea9f5a15a541 100644 --- a/web/packages/shared/components/Validation/rules.test.ts +++ b/web/packages/shared/components/Validation/rules.test.ts @@ -25,6 +25,8 @@ import { requiredEmailLike, requiredIamRoleName, requiredPort, + runRules, + arrayOf, } from './rules'; describe('requiredField', () => { @@ -153,3 +155,60 @@ describe('requiredPort', () => { expect(requiredPort(port)()).toEqual(expected); }); }); + +test('runRules', () => { + expect( + runRules( + { foo: 'val1', bar: 'val2', irrelevant: undefined }, + { foo: requiredField('no foo'), bar: requiredField('no bar') } + ) + ).toEqual({ + valid: true, + fields: { + foo: { valid: true, message: '' }, + bar: { valid: true, message: '' }, + }, + }); + + expect( + runRules( + { foo: '', bar: 'val2', irrelevant: undefined }, + { foo: requiredField('no foo'), bar: requiredField('no bar') } + ) + ).toEqual({ + valid: false, + fields: { + foo: { valid: false, message: 'no foo' }, + bar: { valid: true, message: '' }, + }, + }); +}); + +test.each([ + { + name: 'invalid', + items: ['a', '', 'c'], + expected: { + valid: false, + items: [ + { valid: true, message: '' }, + { valid: false, message: 'required' }, + { valid: true, message: '' }, + ], + }, + }, + { + name: 'valid', + items: ['a', 'b', 'c'], + expected: { + valid: true, + items: [ + { valid: true, message: '' }, + { valid: true, message: '' }, + { valid: true, message: '' }, + ], + }, + }, +])('arrayOf: $name', ({ items, expected }) => { + expect(arrayOf(requiredField('required'))(items)()).toEqual(expected); +}); diff --git a/web/packages/shared/components/Validation/rules.ts b/web/packages/shared/components/Validation/rules.ts index 52063d67fce99..57b07f062a5af 100644 --- a/web/packages/shared/components/Validation/rules.ts +++ b/web/packages/shared/components/Validation/rules.ts @@ -31,6 +31,8 @@ export interface ValidationResult { */ export type Rule = (value: T) => () => R; +type RuleResult = ReturnType>; + /** * requiredField checks for empty strings and arrays. * @@ -280,6 +282,83 @@ const requiredAll = return { valid: true }; }; +/** A result of the {@link arrayOf} validation rule. */ +export type ArrayValidationResult = ValidationResult & { + /** Results of validating each separate item. */ + items: R[]; +}; + +/** Validates an array by executing given rule on each of its elements. */ +const arrayOf = + ( + elementRule: Rule + ): Rule> => + (values: T[]) => + () => { + const results = values.map(v => elementRule(v)()); + return { items: results, valid: results.every(r => r.valid) }; + }; + +/** + * Passes a precomputed validation result instead of computing it inside the + * rule. + * + * This rule is a hacky way to allow the validation engine to operate with + * validation results computed outside of the validator's validation cycle. See + * the `Validation` component's documentation for more information about where + * this is useful and a detailed usage example. + */ +const precomputed = + (res: ValidationResult): Rule => + () => + () => + res; + +/** + * A set of rules to be executed using `runRules` on a model object. The rule + * set contains a subset of keys of the object. + */ +export type RuleSet = Record< + K, + Rule +>; + +/** A result of executing a set of rules on a model object. */ +export type RuleSetValidationResult> = { + valid: boolean; + /** + * Each member of the `fields` object corresponds to a rule from within the + * rule set and contains the result of validating a model field of the same + * name. + */ + fields: { [k in keyof R]: RuleResult }; // Record; +}; + +/** + * Executes a set of rules on a model object, producing a precomputed + * validation result that can be used with `precomputed` rule to inject to + * field components, but also allows for consuming the validation data outside + * these fields. + * + * `K` is the subset of model field names. + * `M` is the validated model. + */ +export const runRules = >( + model: M, + rules: RuleSet +): RuleSetValidationResult> => { + const fields = {} as { + [k in keyof RuleSet]: RuleResult[k]>; + }; + let valid = true; + for (const key in rules) { + const modelValue = model[key]; + fields[key] = rules[key](modelValue)(); + valid &&= fields[key].valid; + } + return { fields, valid }; +}; + export { requiredToken, requiredPassword, @@ -292,4 +371,6 @@ export { requiredMatchingRoleNameAndRoleArn, validAwsIAMRoleName, requiredPort, + arrayOf, + precomputed, }; diff --git a/web/packages/shared/components/Validation/useRule.js b/web/packages/shared/components/Validation/useRule.js index ad0ca82157cbf..e8d2a77e391ae 100644 --- a/web/packages/shared/components/Validation/useRule.js +++ b/web/packages/shared/components/Validation/useRule.js @@ -39,7 +39,7 @@ export default function useRule(cb) { // register to validation context to be called on cb() React.useEffect(() => { function onValidate() { - if (validator.validating) { + if (validator.state.validating) { const result = cb(); validator.addResult(result); rerender({}); @@ -47,18 +47,18 @@ export default function useRule(cb) { } // subscribe to store changes - validator.subscribe(onValidate); + validator.addRuleCallback(onValidate); // unsubscribe on unmount function cleanup() { - validator.unsubscribe(onValidate); + validator.removeRuleCallback(onValidate); } return cleanup; }, [cb]); // if validation has been requested, cb right away. - if (validator.validating) { + if (validator.state.validating) { return cb(); } diff --git a/web/packages/teleport/src/components/LabelsInput/LabelsInput.test.tsx b/web/packages/teleport/src/components/LabelsInput/LabelsInput.test.tsx index eaee3b29c7ea6..6697f3d5f163b 100644 --- a/web/packages/teleport/src/components/LabelsInput/LabelsInput.test.tsx +++ b/web/packages/teleport/src/components/LabelsInput/LabelsInput.test.tsx @@ -17,7 +17,10 @@ */ import { render, fireEvent, screen } from 'design/utils/testing'; +import Validation, { Validator } from 'shared/components/Validation'; +import { act } from '@testing-library/react'; +import { Label, LabelsInput, LabelsRule, nonEmptyLabels } from './LabelsInput'; import { Default, Custom, @@ -102,3 +105,119 @@ test('removing last label is not possible due to requiring labels', async () => expect(screen.getByPlaceholderText('label key')).toBeInTheDocument(); expect(screen.getByPlaceholderText('label value')).toBeInTheDocument(); }); + +describe('validation rules', () => { + function setup(labels: Label[], rule: LabelsRule) { + let validator: Validator; + render( + + {({ validator: v }) => { + validator = v; + return ( + {}} rule={rule} /> + ); + }} + + ); + return { validator }; + } + + describe.each([ + { name: 'explicitly enforced standard rule', rule: nonEmptyLabels }, + { name: 'implicit standard rule', rule: undefined }, + ])('$name', ({ rule }) => { + test('invalid', () => { + const { validator } = setup( + [ + { name: '', value: 'foo' }, + { name: 'bar', value: '' }, + { name: 'asdf', value: 'qwer' }, + ], + rule + ); + act(() => validator.validate()); + expect(validator.state.valid).toBe(false); + expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription( + 'required' + ); // '' + expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription(''); // 'foo' + expect(screen.getAllByRole('textbox')[2]).toHaveAccessibleDescription(''); // 'bar' + expect(screen.getAllByRole('textbox')[3]).toHaveAccessibleDescription( + 'required' + ); // '' + expect(screen.getAllByRole('textbox')[4]).toHaveAccessibleDescription(''); // 'asdf' + expect(screen.getAllByRole('textbox')[5]).toHaveAccessibleDescription(''); // 'qwer' + }); + + test('valid', () => { + const { validator } = setup( + [ + { name: '', value: 'foo' }, + { name: 'bar', value: '' }, + { name: 'asdf', value: 'qwer' }, + ], + rule + ); + act(() => validator.validate()); + expect(validator.state.valid).toBe(false); + expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription( + 'required' + ); // '' + expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription(''); // 'foo' + expect(screen.getAllByRole('textbox')[2]).toHaveAccessibleDescription(''); // 'bar' + expect(screen.getAllByRole('textbox')[3]).toHaveAccessibleDescription( + 'required' + ); // '' + expect(screen.getAllByRole('textbox')[4]).toHaveAccessibleDescription(''); // 'asdf' + expect(screen.getAllByRole('textbox')[5]).toHaveAccessibleDescription(''); // 'qwer' + }); + }); + + const nameNotFoo: LabelsRule = (labels: Label[]) => () => { + const results = labels.map(label => ({ + name: + label.name === 'foo' + ? { valid: false, message: 'no foo please' } + : { valid: true }, + value: { valid: true }, + })); + return { + valid: results.every(r => r.name.valid && r.value.valid), + items: results, + }; + }; + + test('custom rule, invalid', async () => { + const { validator } = setup( + [ + { name: 'foo', value: 'bar' }, + { name: 'bar', value: 'foo' }, + ], + nameNotFoo + ); + act(() => validator.validate()); + expect(validator.state.valid).toBe(false); + expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription( + 'no foo please' + ); // 'foo' key + expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription(''); + expect(screen.getAllByRole('textbox')[2]).toHaveAccessibleDescription(''); + expect(screen.getAllByRole('textbox')[3]).toHaveAccessibleDescription(''); + }); + + test('custom rule, valid', async () => { + const { validator } = setup( + [ + { name: 'xyz', value: 'bar' }, + { name: 'bar', value: 'foo' }, + ], + nameNotFoo + ); + act(() => validator.validate()); + expect(validator.state.valid).toBe(true); + expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription(''); + expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription(''); + expect(screen.getAllByRole('textbox')[2]).toHaveAccessibleDescription(''); + expect(screen.getAllByRole('textbox')[3]).toHaveAccessibleDescription(''); + }); +}); diff --git a/web/packages/teleport/src/components/LabelsInput/LabelsInput.tsx b/web/packages/teleport/src/components/LabelsInput/LabelsInput.tsx index f163d7df0e0de..c4256375325b7 100644 --- a/web/packages/teleport/src/components/LabelsInput/LabelsInput.tsx +++ b/web/packages/teleport/src/components/LabelsInput/LabelsInput.tsx @@ -19,8 +19,17 @@ import styled from 'styled-components'; import { Flex, Box, ButtonSecondary, ButtonIcon } from 'design'; import FieldInput from 'shared/components/FieldInput'; -import { Validator, useValidation } from 'shared/components/Validation'; -import { requiredField } from 'shared/components/Validation/rules'; +import { + Validator, + useRule, + useValidation, +} from 'shared/components/Validation'; +import { + precomputed, + requiredField, + Rule, + ValidationResult, +} from 'shared/components/Validation/rules'; import * as Icons from 'design/Icon'; import { inputGeometry } from 'design/Input/Input'; @@ -34,6 +43,24 @@ export type LabelInputTexts = { placeholder: string; }; +type LabelListValidationResult = ValidationResult & { + /** + * A list of validation results, one per label. Note: items are optional just + * because `useRule` by default returns only `ValidationResult`. For the + * actual validation, it's not optional; if it's undefined, or there are + * fewer items in this list than the labels, a default validation rule will + * be used instead. + */ + items?: LabelValidationResult[]; +}; + +type LabelValidationResult = { + name: ValidationResult; + value: ValidationResult; +}; + +export type LabelsRule = Rule; + export function LabelsInput({ labels = [], setLabels, @@ -44,6 +71,7 @@ export function LabelsInput({ labelKey = { fieldName: 'Key', placeholder: 'label key' }, labelVal = { fieldName: 'Value', placeholder: 'label value' }, inputWidth = 200, + rule = defaultRule, }: { labels: Label[]; setLabels(l: Label[]): void; @@ -57,8 +85,15 @@ export function LabelsInput({ * Makes it so at least one label is required */ areLabelsRequired?: boolean; + /** + * A rule for validating the list of labels as a whole. Note that contrary to + * other input fields, the labels input will default to validating every + * input as required if this property is undefined. + */ + rule?: LabelsRule; }) { const validator = useValidation() as Validator; + const validationResult: LabelListValidationResult = useRule(rule(labels)); function addLabel() { setLabels([...labels, { name: '', value: '' }]); @@ -92,11 +127,8 @@ export function LabelsInput({ setLabels(newList); }; - const requiredUniqueKey = value => () => { + const requiredKey = value => () => { // Check for empty length and duplicate key. - // TODO(bl-nero): This function doesn't really check for uniqueness; it - // needs to be fixed. This control should probably be merged with - // `LabelsCreater`, which has this feature working correctly. let notValid = !value || value.length === 0; return { @@ -121,12 +153,18 @@ export function LabelsInput({ )} {labels.map((label, index) => { + const validationItem: LabelValidationResult | undefined = + validationResult.items?.[index]; return ( () => ({ valid: true }); + const SmallText = styled.span` font-size: ${p => p.theme.fontSizes[1]}px; font-weight: lighter; `; + +export const nonEmptyLabels: LabelsRule = labels => () => { + const results = labels.map(label => ({ + name: requiredField('required')(label.name)(), + value: requiredField('required')(label.value)(), + })); + return { + valid: results.every(r => r.name.valid && r.value.valid), + items: results, + }; +}; From 22be0160065be5cc8dba01fafe4a271a0de09393 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Tue, 3 Dec 2024 16:12:08 +0100 Subject: [PATCH 2/3] review --- .../components/FieldMultiInput/FieldMultiInput.tsx | 14 +++++++------- .../shared/components/Validation/Validation.tsx | 6 ++++-- .../shared/components/Validation/rules.test.ts | 4 ++-- web/packages/shared/components/Validation/rules.ts | 4 ++-- .../components/LabelsInput/LabelsInput.test.tsx | 2 +- .../src/components/LabelsInput/LabelsInput.tsx | 6 +++--- 6 files changed, 19 insertions(+), 17 deletions(-) diff --git a/web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx b/web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx index f323ec7dd268d..48a12d403f313 100644 --- a/web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx +++ b/web/packages/shared/components/FieldMultiInput/FieldMultiInput.tsx @@ -35,13 +35,13 @@ import FieldInput from '../FieldInput'; type StringListValidationResult = ValidationResult & { /** - * A list of validation results, one per label. Note: items are optional just - * because `useRule` by default returns only `ValidationResult`. For the - * actual validation, it's not optional; if it's undefined, or there are - * fewer items in this list than the labels, the corresponding items will be - * treated as valid. + * A list of validation results, one per list item. Note: results are + * optional just because `useRule` by default returns only + * `ValidationResult`. For the actual validation, it's not optional; if it's + * undefined, or there are fewer results in this list than the list items, + * the corresponding items will be treated as valid. */ - items?: ValidationResult[]; + results?: ValidationResult[]; }; export type FieldMultiInputProps = { @@ -119,7 +119,7 @@ export function FieldMultiInput({ diff --git a/web/packages/shared/components/Validation/Validation.tsx b/web/packages/shared/components/Validation/Validation.tsx index b032f452f4dba..e319d061164da 100644 --- a/web/packages/shared/components/Validation/Validation.tsx +++ b/web/packages/shared/components/Validation/Validation.tsx @@ -110,7 +110,9 @@ export default class Validator extends Store { const ValidationContext = React.createContext(undefined); -type ValidationRenderFunction = (arg: { validator: Validator }) => any; +type ValidationRenderFunction = (arg: { + validator: Validator; +}) => React.ReactNode; /** * Installs a validation context that provides a {@link Validator} store. The @@ -184,7 +186,7 @@ export function Validation(props: { export function useValidation(): Validator | undefined { const validator = React.useContext(ValidationContext); if (!validator) { - logger.warn('Missing Validation Context declaration'); + throw new Error('useValidation() called without a validation context'); } return useStore(validator); } diff --git a/web/packages/shared/components/Validation/rules.test.ts b/web/packages/shared/components/Validation/rules.test.ts index 1ea9f5a15a541..07ee1bf434d01 100644 --- a/web/packages/shared/components/Validation/rules.test.ts +++ b/web/packages/shared/components/Validation/rules.test.ts @@ -190,7 +190,7 @@ test.each([ items: ['a', '', 'c'], expected: { valid: false, - items: [ + results: [ { valid: true, message: '' }, { valid: false, message: 'required' }, { valid: true, message: '' }, @@ -202,7 +202,7 @@ test.each([ items: ['a', 'b', 'c'], expected: { valid: true, - items: [ + results: [ { valid: true, message: '' }, { valid: true, message: '' }, { valid: true, message: '' }, diff --git a/web/packages/shared/components/Validation/rules.ts b/web/packages/shared/components/Validation/rules.ts index 57b07f062a5af..545f28a348fce 100644 --- a/web/packages/shared/components/Validation/rules.ts +++ b/web/packages/shared/components/Validation/rules.ts @@ -285,7 +285,7 @@ const requiredAll = /** A result of the {@link arrayOf} validation rule. */ export type ArrayValidationResult = ValidationResult & { /** Results of validating each separate item. */ - items: R[]; + results: R[]; }; /** Validates an array by executing given rule on each of its elements. */ @@ -296,7 +296,7 @@ const arrayOf = (values: T[]) => () => { const results = values.map(v => elementRule(v)()); - return { items: results, valid: results.every(r => r.valid) }; + return { results: results, valid: results.every(r => r.valid) }; }; /** diff --git a/web/packages/teleport/src/components/LabelsInput/LabelsInput.test.tsx b/web/packages/teleport/src/components/LabelsInput/LabelsInput.test.tsx index 6697f3d5f163b..8f8c07ea95d0c 100644 --- a/web/packages/teleport/src/components/LabelsInput/LabelsInput.test.tsx +++ b/web/packages/teleport/src/components/LabelsInput/LabelsInput.test.tsx @@ -183,7 +183,7 @@ describe('validation rules', () => { })); return { valid: results.every(r => r.name.valid && r.value.valid), - items: results, + results: results, }; }; diff --git a/web/packages/teleport/src/components/LabelsInput/LabelsInput.tsx b/web/packages/teleport/src/components/LabelsInput/LabelsInput.tsx index c4256375325b7..eee6025249817 100644 --- a/web/packages/teleport/src/components/LabelsInput/LabelsInput.tsx +++ b/web/packages/teleport/src/components/LabelsInput/LabelsInput.tsx @@ -51,7 +51,7 @@ type LabelListValidationResult = ValidationResult & { * fewer items in this list than the labels, a default validation rule will * be used instead. */ - items?: LabelValidationResult[]; + results?: LabelValidationResult[]; }; type LabelValidationResult = { @@ -154,7 +154,7 @@ export function LabelsInput({ {labels.map((label, index) => { const validationItem: LabelValidationResult | undefined = - validationResult.items?.[index]; + validationResult.results?.[index]; return ( @@ -246,6 +246,6 @@ export const nonEmptyLabels: LabelsRule = labels => () => { })); return { valid: results.every(r => r.name.valid && r.value.valid), - items: results, + results: results, }; }; From 5e020d9010cb036085d997eb8eeb0fbe1047f638 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Wed, 4 Dec 2024 15:41:42 +0100 Subject: [PATCH 3/3] Never return undefined from useValidation() --- web/packages/shared/components/Validation/Validation.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/packages/shared/components/Validation/Validation.tsx b/web/packages/shared/components/Validation/Validation.tsx index e319d061164da..6450c2915a61d 100644 --- a/web/packages/shared/components/Validation/Validation.tsx +++ b/web/packages/shared/components/Validation/Validation.tsx @@ -183,7 +183,7 @@ export function Validation(props: { ); } -export function useValidation(): Validator | undefined { +export function useValidation(): Validator { const validator = React.useContext(ValidationContext); if (!validator) { throw new Error('useValidation() called without a validation context');