Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EDR Workflows] Error message 'the value already exists' shown for duplicate values with the 'is one of' operator on Blocklist tab #196071

Merged
merged 7 commits into from
Oct 23, 2024
Merged

[EDR Workflows] Error message 'the value already exists' shown for duplicate values with the 'is one of' operator on Blocklist tab #196071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
77c6fb9
proper validation handling
szwarckonrad Oct 14, 2024
3e57050
proper validation handling
szwarckonrad Oct 14, 2024
5cbf521
Merge branch 'main' into fix-is-one-of-combo-box-warning
szwarckonrad Oct 14, 2024
b48dbea
Merge branch 'main' into fix-is-one-of-combo-box-warning
szwarckonrad Oct 22, 2024
df292f6
Merge remote-tracking branch 'origin/fix-is-one-of-combo-box-warning'…
szwarckonrad Oct 22, 2024
3318024
unit coverage
szwarckonrad Oct 22, 2024
34494df
Merge branch 'main' into fix-is-one-of-combo-box-warning
szwarckonrad Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ...curity_solution/public/management/pages/blocklist/view/components/blocklist_form.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,18 @@ describe('blocklist form', () => {
expect(screen.queryByText(ERRORS.INVALID_PATH)).toBeTruthy();
});

it('should prevail duplicate value warning on lost focus', async () => {
const item = createItem({
os_types: [OperatingSystem.WINDOWS],
entries: [createEntry('file.Ext.code_signature', ['valid', 'invalid'])],
});
render(createProps({ item }));
await user.type(screen.getByRole('combobox'), 'invalid{enter}');
expect(screen.queryByText(ERRORS.DUPLICATE_VALUE)).toBeTruthy();
await user.click(screen.getByTestId('blocklist-form-os-select'));
expect(screen.queryByText(ERRORS.DUPLICATE_VALUE)).toBeTruthy();
});

it('should warn if single duplicate value entry', async () => {
const hash = 'C3AB8FF13720E8AD9047DD39466B3C8974E592C2FA383D4A3960714CAEF0C4F2';
const item = createItem({
Expand Down
116 changes: 66 additions & 50 deletions ...ns/security_solution/public/management/pages/blocklist/view/components/blocklist_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,8 @@ function isValid(itemValidation: ItemValidation): boolean {
// eslint-disable-next-line react/display-name
export const BlockListForm = memo<ArtifactFormComponentProps>(
({ item, policies, policiesIsLoading, onChange, mode }) => {
const [visited, setVisited] = useState<{ name: boolean; value: boolean }>({
name: false,
value: false,
});
const [nameVisited, setNameVisited] = useState(false);
const [valueVisited, setValueVisited] = useState({ value: false }); // Use object to trigger re-render
Copy link
Member

@joeypoon joeypoon Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So take this with a grain of salt 🧂 because I haven't really touched frontend code in nearly 3 years but this feels a little bit weird. Not a blocker or anything but I do wonder if there's a better approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree with joey here, this doesn't look like a good practice.

i guess it is needed, because warnings and errors are stored in reference values, but i don't see why we need to store those in refs instead of good ol' states. i'd suggest to try to remove the refs, and use states (or maybe one state if they always change together) to improve this file

const warningsRef = useRef<ItemValidation>({ name: {}, value: {} });
const errorsRef = useRef<ItemValidation>({ name: {}, value: {} });
const [selectedPolicies, setSelectedPolicies] = useState<PolicyData[]>([]);
Expand Down Expand Up @@ -269,57 +267,75 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
);
}, [displaySingleValueInput]);

const validateValues = useCallback((nextItem: ArtifactFormComponentProps['item']) => {
const os = ((nextItem.os_types ?? [])[0] as OperatingSystem) ?? OperatingSystem.WINDOWS;
const {
field = 'file.hash.*',
type = ListOperatorTypeEnum.MATCH_ANY,
value = [],
} = (nextItem.entries[0] ?? {}) as BlocklistEntry;

// value can be a string when isOperator is selected
const values = Array.isArray(value) ? value : [value].filter(Boolean);

const newValueWarnings: ItemValidationNodes = {};
const newNameErrors: ItemValidationNodes = {};
const newValueErrors: ItemValidationNodes = {};
// error if name empty
if (!nextItem.name.trim()) {
newNameErrors.NAME_REQUIRED = createValidationMessage(ERRORS.NAME_REQUIRED);
}
const validateValues = useCallback(
(nextItem: ArtifactFormComponentProps['item'], cleanState = false) => {
const os = ((nextItem.os_types ?? [])[0] as OperatingSystem) ?? OperatingSystem.WINDOWS;
const {
field = 'file.hash.*',
type = ListOperatorTypeEnum.MATCH_ANY,
value = [],
} = (nextItem.entries[0] ?? {}) as BlocklistEntry;

// value can be a string when isOperator is selected
const values = Array.isArray(value) ? value : [value].filter(Boolean);

const newValueWarnings: ItemValidationNodes = cleanState
? {}
: { ...warningsRef.current.value };
const newNameErrors: ItemValidationNodes = cleanState ? {} : { ...errorsRef.current.name };
const newValueErrors: ItemValidationNodes = cleanState
? {}
: { ...errorsRef.current.value };

// error if name empty
if (!nextItem.name.trim()) {
newNameErrors.NAME_REQUIRED = createValidationMessage(ERRORS.NAME_REQUIRED);
} else {
delete newNameErrors.NAME_REQUIRED;
}

// error if no values
if (!values.length) {
newValueErrors.VALUE_REQUIRED = createValidationMessage(ERRORS.VALUE_REQUIRED);
}
// error if no values
if (!values.length) {
newValueErrors.VALUE_REQUIRED = createValidationMessage(ERRORS.VALUE_REQUIRED);
} else {
delete newValueErrors.VALUE_REQUIRED;
}

// error if invalid hash
if (field === 'file.hash.*' && values.some((v) => !isValidHash(v))) {
newValueErrors.INVALID_HASH = createValidationMessage(ERRORS.INVALID_HASH);
}
// error if invalid hash
if (field === 'file.hash.*' && values.some((v) => !isValidHash(v))) {
newValueErrors.INVALID_HASH = createValidationMessage(ERRORS.INVALID_HASH);
} else {
delete newValueErrors.INVALID_HASH;
}

const isInvalidPath = values.some((v) => !isPathValid({ os, field, type, value: v }));
// warn if invalid path
if (field !== 'file.hash.*' && isInvalidPath) {
newValueWarnings.INVALID_PATH = createValidationMessage(ERRORS.INVALID_PATH);
}
// warn if duplicates
if (values.length !== uniq(values).length) {
newValueWarnings.DUPLICATE_VALUES = createValidationMessage(ERRORS.DUPLICATE_VALUES);
}
const isInvalidPath = values.some((v) => !isPathValid({ os, field, type, value: v }));
// warn if invalid path
if (field !== 'file.hash.*' && isInvalidPath) {
newValueWarnings.INVALID_PATH = createValidationMessage(ERRORS.INVALID_PATH);
} else {
delete newValueWarnings.INVALID_PATH;
}
// warn if duplicates
if (values.length !== uniq(values).length) {
newValueWarnings.DUPLICATE_VALUES = createValidationMessage(ERRORS.DUPLICATE_VALUES);
} else {
delete newValueWarnings.DUPLICATE_VALUES;
}

warningsRef.current = { ...warningsRef.current, value: newValueWarnings };
errorsRef.current = { name: newNameErrors, value: newValueErrors };
}, []);
warningsRef.current = { ...warningsRef.current, value: newValueWarnings };
errorsRef.current = { name: newNameErrors, value: newValueErrors };
},
[]
);

const handleOnNameBlur = useCallback(() => {
validateValues(item);
setVisited((prevVisited) => ({ ...prevVisited, name: true }));
setNameVisited(true);
}, [item, validateValues]);

const handleOnValueBlur = useCallback(() => {
validateValues(item);
setVisited((prevVisited) => ({ ...prevVisited, value: true }));
setValueVisited({ value: true });
}, [item, validateValues]);

const handleOnNameChange = useCallback(
Expand Down Expand Up @@ -463,7 +479,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
};

// trigger re-render without modifying item
setVisited((prevVisited) => ({ ...prevVisited }));
setValueVisited((prevState) => ({ ...prevState }));
},
[blocklistEntry]
);
Expand Down Expand Up @@ -495,7 +511,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
entries: [{ ...blocklistEntry, value }],
} as ArtifactFormComponentProps['item'];

validateValues(nextItem);
validateValues(nextItem, true);
onChange({
isValid: isValid(errorsRef.current),
item: nextItem,
Expand All @@ -518,7 +534,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
validateValues(nextItem as ArtifactFormComponentProps['item']);
nextItem.entries[0].value = uniq(nextItem.entries[0].value);

setVisited((prevVisited) => ({ ...prevVisited, value: true }));
setValueVisited({ value: true });
onChange({
isValid: isValid(errorsRef.current),
item: nextItem as ArtifactFormComponentProps['item'],
Expand Down Expand Up @@ -563,7 +579,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(

<EuiFormRow
label={NAME_LABEL}
isInvalid={visited.name && !!Object.keys(errorsRef.current.name).length}
isInvalid={nameVisited && !!Object.keys(errorsRef.current.name).length}
error={Object.values(errorsRef.current.name)}
fullWidth
>
Expand All @@ -572,7 +588,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
value={item.name}
onChange={handleOnNameChange}
onBlur={handleOnNameBlur}
required={visited.name}
required={nameVisited}
maxLength={256}
data-test-subj={getTestId('name-input')}
fullWidth
Expand Down Expand Up @@ -651,7 +667,7 @@ export const BlockListForm = memo<ArtifactFormComponentProps>(
</EuiFormRow>
<EuiFormRow
label={valueLabel}
isInvalid={visited.value && !!Object.keys(errorsRef.current.value).length}
isInvalid={valueVisited.value && !!Object.keys(errorsRef.current.value).length}
helpText={Object.values(warningsRef.current.value)}
error={Object.values(errorsRef.current.value)}
fullWidth
Expand Down