From e714d750f2fc31ee50db4d347be8900bbef58b2a Mon Sep 17 00:00:00 2001 From: Sonia Aguilar <33540275+soniaAguilarPeiron@users.noreply.github.com> Date: Fri, 23 Sep 2022 14:44:24 +0200 Subject: [PATCH] Alerting: Character / allowed in dashboard folder names, but not in alert folder names (#54395) * Refactor FolderPicker to functional componment and add dissalowSlashes as an optional prop * Update DetailsStep component to use FolderPicker dissalowing slashes in folders * Adds icon that links to the Github issue when showing slashes not allowed warning * Add test for slashed folders warning * Fix merge conflicts with folder creation fix PR * Move slashes filter to useRuleFolderFilter hook, and make folder warning an optional generic prop * Apply PR review suggestions --- .../core/components/Select/FolderPicker.tsx | 68 ++++++++++++++++--- .../alerting/unified/RuleEditor.test.tsx | 31 +++++++-- .../components/rule-editor/DetailsStep.tsx | 11 +-- .../rule-editor/RuleFolderPicker.tsx | 50 +++++++++++++- 4 files changed, 140 insertions(+), 20 deletions(-) diff --git a/public/app/core/components/Select/FolderPicker.tsx b/public/app/core/components/Select/FolderPicker.tsx index 8f50e381094..cfbf20a68f4 100644 --- a/public/app/core/components/Select/FolderPicker.tsx +++ b/public/app/core/components/Select/FolderPicker.tsx @@ -17,6 +17,16 @@ export type FolderPickerFilter = (hits: DashboardSearchHit[]) => DashboardSearch export const ADD_NEW_FOLER_OPTION = '+ Add new'; +export interface FolderWarning { + warningCondition: (value: string) => boolean; + warningComponent: () => JSX.Element; +} + +export interface CustomAdd { + disallowValues: boolean; + isAllowedValue?: (value: string) => boolean; +} + export interface Props { onChange: ($folder: { title: string; id: number }) => void; enableCreateNew?: boolean; @@ -31,7 +41,9 @@ export interface Props { showRoot?: boolean; onClear?: () => void; accessControlMetadata?: boolean; - customAdd?: boolean; + customAdd?: CustomAdd; + folderWarning?: FolderWarning; + /** * Skips loading all folders in order to find the folder matching * the folder where the dashboard is stored. @@ -63,12 +75,17 @@ export function FolderPicker(props: Props) { skipInitialLoad, accessControlMetadata, customAdd, + folderWarning, } = props; - const isClearable = typeof onClear === 'function'; + const [folder, setFolder] = useState(null); const [isCreatingNew, setIsCreatingNew] = useState(false); - const styles = useStyles2(getStyles); const [inputValue, setInputValue] = useState(''); + const [newFolderValue, setNewFolderValue] = useState(folder?.title ?? ''); + + const styles = useStyles2(getStyles); + + const isClearable = typeof onClear === 'function'; const getOptions = useCallback( async (query: string) => { @@ -91,7 +108,7 @@ export function FolderPicker(props: Props) { ) { Boolean(initialTitle) && options.unshift({ label: initialTitle, value: initialFolderId }); } - if (enableCreateNew && customAdd) { + if (enableCreateNew && Boolean(customAdd)) { return [...options, { value: VALUE_FOR_ADD, label: ADD_NEW_FOLER_OPTION, title: query }]; } else { return options; @@ -150,7 +167,7 @@ export function FolderPicker(props: Props) { useEffect(() => { // if this is not the same as our initial value notify parent if (folder && folder.value !== initialFolderId) { - !isCreatingNew && onChange({ id: folder.value!, title: folder.label! }); + !isCreatingNew && folder.value && folder.label && onChange({ id: folder.value, title: folder.label }); } // eslint-disable-next-line react-hooks/exhaustive-deps }, [folder, initialFolderId]); @@ -183,6 +200,7 @@ export function FolderPicker(props: Props) { id: VALUE_FOR_ADD, title: inputValue, }); + setNewFolderValue(inputValue); } else { if (!newFolder) { newFolder = { value: 0, label: rootName }; @@ -202,6 +220,9 @@ export function FolderPicker(props: Props) { const createNewFolder = useCallback( async (folderName: string) => { + if (folderWarning?.warningCondition(folderName)) { + return false; + } const newFolder = await createFolder({ title: folderName }); let folder: SelectableValue = { value: -1, label: 'Not created' }; @@ -217,11 +238,17 @@ export function FolderPicker(props: Props) { return folder; }, - [onFolderChange] + [folderWarning, onFolderChange] ); const onKeyDown = useCallback( (event: React.KeyboardEvent) => { + const dissalowValues = Boolean(customAdd?.disallowValues); + if (event.key === 'Enter' && dissalowValues && !customAdd?.isAllowedValue!(newFolderValue)) { + event.preventDefault(); + return; + } + switch (event.key) { case 'Enter': { createNewFolder(folder?.title!); @@ -234,11 +261,12 @@ export function FolderPicker(props: Props) { } } }, - [createNewFolder, folder, rootName] + [customAdd?.disallowValues, customAdd?.isAllowedValue, newFolderValue, createNewFolder, folder?.title, rootName] ); const onNewFolderChange = (e: FormEvent) => { const value = e.currentTarget.value; + setNewFolderValue(value); setFolder({ id: -1, title: value }); }; @@ -258,25 +286,43 @@ export function FolderPicker(props: Props) { return; }; + const FolderWarningWhenCreating = () => { + if (folderWarning?.warningCondition(newFolderValue)) { + return ; + } else { + return null; + } + }; + + const FolderWarningWhenSearching = () => { + if (folderWarning?.warningCondition(inputValue)) { + return ; + } else { + return null; + } + }; + if (isCreatingNew) { return ( <> + +
Press enter to create the new folder.
-
Press enter to create the new folder.
); } else { return (
+ ({ newFolder: css` color: ${theme.colors.warning.main}; font-size: ${theme.typography.bodySmall.fontSize}; - padding-top: ${theme.spacing(1)}; + padding-bottom: ${theme.spacing(1)}; `, }); diff --git a/public/app/features/alerting/unified/RuleEditor.test.tsx b/public/app/features/alerting/unified/RuleEditor.test.tsx index e78668c5568..3a6f11a16e8 100644 --- a/public/app/features/alerting/unified/RuleEditor.test.tsx +++ b/public/app/features/alerting/unified/RuleEditor.test.tsx @@ -80,8 +80,8 @@ const ui = { alertType: byTestId('alert-type-picker'), dataSource: byTestId('datasource-picker'), folder: byTestId('folder-picker'), - namespace: byTestId('namespace-picker'), folderContainer: byTestId(selectors.components.FolderPicker.containerV2), + namespace: byTestId('namespace-picker'), group: byTestId('group-picker'), annotationKey: (idx: number) => byTestId(`annotation-key-${idx}`), annotationValue: (idx: number) => byTestId(`annotation-value-${idx}`), @@ -244,6 +244,10 @@ describe('RuleEditor', () => { title: 'Folder B', id: 2, }, + { + title: 'Folder / with slash', + id: 2, + }, ] as DashboardSearchHit[]); mocks.api.discoverFeatures.mockResolvedValue({ @@ -411,6 +415,12 @@ describe('RuleEditor', () => { id: 1, }; + const slashedFolder = { + title: 'Folder with /', + uid: 'abcde', + id: 2, + }; + const dataSources = { default: mockDataSource( { @@ -459,7 +469,7 @@ describe('RuleEditor', () => { }, ], }); - mocks.searchFolders.mockResolvedValue([folder] as DashboardSearchHit[]); + mocks.searchFolders.mockResolvedValue([folder, slashedFolder] as DashboardSearchHit[]); await renderRuleEditor(uid); await waitFor(() => expect(mocks.searchFolders).toHaveBeenCalled()); @@ -469,10 +479,23 @@ describe('RuleEditor', () => { // check that it's filled in const nameInput = await ui.inputs.name.find(); expect(nameInput).toHaveValue('my great new rule'); + //check that folder is in the list expect(ui.inputs.folder.get()).toHaveTextContent(new RegExp(folder.title)); expect(ui.inputs.annotationValue(0).get()).toHaveValue('some description'); expect(ui.inputs.annotationValue(1).get()).toHaveValue('some summary'); + //check that slashed folders are not in the list + expect(ui.inputs.folder.get()).toHaveTextContent(new RegExp(folder.title)); + expect(ui.inputs.folder.get()).not.toHaveTextContent(new RegExp(slashedFolder.title)); + + //check that slashes warning is only shown once user search slashes + const folderInput = await ui.inputs.folderContainer.find(); + expect(within(folderInput).queryByText("Folders with '/' character are not allowed.")).not.toBeInTheDocument(); + await userEvent.type(within(folderInput).getByRole('combobox'), 'new slashed //'); + expect(within(folderInput).getByText("Folders with '/' character are not allowed.")).toBeInTheDocument(); + await userEvent.keyboard('{backspace} {backspace}{backspace}'); + expect(within(folderInput).queryByText("Folders with '/' character are not allowed.")).not.toBeInTheDocument(); + // add an annotation await clickSelectOption(ui.inputs.annotationKey(2).get(), /Add new/); await userEvent.type(byRole('textbox').get(ui.inputs.annotationKey(2).get()), 'custom'); @@ -487,10 +510,10 @@ describe('RuleEditor', () => { await waitFor(() => expect(mocks.api.setRulerRuleGroup).toHaveBeenCalled()); //check that '+ Add new' option is in folders drop down even if we don't have values - const folderInput = await ui.inputs.folderContainer.find(); + const emptyFolderInput = await ui.inputs.folderContainer.find(); mocks.searchFolders.mockResolvedValue([] as DashboardSearchHit[]); await renderRuleEditor(uid); - await userEvent.click(within(folderInput).getByRole('combobox')); + await userEvent.click(within(emptyFolderInput).getByRole('combobox')); expect(screen.getByText(ADD_NEW_FOLER_OPTION)).toBeInTheDocument(); expect(mocks.api.setRulerRuleGroup).toHaveBeenCalledWith( diff --git a/public/app/features/alerting/unified/components/rule-editor/DetailsStep.tsx b/public/app/features/alerting/unified/components/rule-editor/DetailsStep.tsx index fd8a95012d4..d79ad03fe9e 100644 --- a/public/app/features/alerting/unified/components/rule-editor/DetailsStep.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/DetailsStep.tsx @@ -15,7 +15,7 @@ import { RuleForm, RuleFormType, RuleFormValues } from '../../types/rule-form'; import AnnotationsField from './AnnotationsField'; import { GroupAndNamespaceFields } from './GroupAndNamespaceFields'; import { RuleEditorSection } from './RuleEditorSection'; -import { RuleFolderPicker, Folder } from './RuleFolderPicker'; +import { RuleFolderPicker, Folder, containsSlashes } from './RuleFolderPicker'; import { checkForPathSeparator } from './util'; const recordingRuleNameValidationPattern = { @@ -117,6 +117,7 @@ export const DetailsStep = ({ initialFolder }: DetailsStepProps) => { enableCreateNew={contextSrv.hasPermission(AccessControlAction.FoldersCreate)} enableReset={true} filter={folderFilter} + dissalowSlashes={true} /> )} name="folder" @@ -165,14 +166,16 @@ const useRuleFolderFilter = (existingRuleForm: RuleForm | null) => { existingRuleForm && hit.folderId === existingRuleForm.id && contextSrv.hasAccessInMetadata(AccessControlAction.AlertingRuleUpdate, hit, rbacDisabledFallback); - return canCreateRuleInFolder || canUpdateInCurrentFolder; }, [existingRuleForm] ); return useCallback( - (folderHits) => folderHits.filter(isSearchHitAvailable), + (folderHits) => + folderHits + .filter(isSearchHitAvailable) + .filter((value: DashboardSearchHit) => !containsSlashes(value.title ?? '')), [isSearchHitAvailable] ); }; @@ -193,6 +196,6 @@ const getStyles = (theme: GrafanaTheme2) => ({ display: flex; flex-direction: row; justify-content: flex-start; - align-items: flex-end; + align-items: end; `, }); diff --git a/public/app/features/alerting/unified/components/rule-editor/RuleFolderPicker.tsx b/public/app/features/alerting/unified/components/rule-editor/RuleFolderPicker.tsx index fee87e60f41..ce320385df5 100644 --- a/public/app/features/alerting/unified/components/rule-editor/RuleFolderPicker.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/RuleFolderPicker.tsx @@ -1,8 +1,13 @@ +import { css } from '@emotion/css'; import React from 'react'; +import { GrafanaTheme2 } from '@grafana/data'; +import { Icon, Stack, Tooltip, useStyles2 } from '@grafana/ui'; import { FolderPicker, Props as FolderPickerProps } from 'app/core/components/Select/FolderPicker'; import { PermissionLevelString } from 'app/types'; +import { FolderWarning, CustomAdd } from '../../../../../core/components/Select/FolderPicker'; + export interface Folder { title: string; id: number; @@ -10,10 +15,38 @@ export interface Folder { export interface RuleFolderPickerProps extends Omit { value?: Folder; + dissalowSlashes: boolean; } +const SlashesWarning = () => { + const styles = useStyles2(getStyles); + const onClick = () => window.open('https://github.com/grafana/grafana/issues/42947', '_blank'); + return ( + +
Folders with '/' character are not allowed.
+ + + + + ); +}; + +export const containsSlashes = (str: string): boolean => str.indexOf('/') !== -1; + export function RuleFolderPicker(props: RuleFolderPickerProps) { const { value } = props; + const warningCondition = (folderName: string) => containsSlashes(folderName); + + const folderWarning: FolderWarning = { + warningCondition: warningCondition, + warningComponent: SlashesWarning, + }; + + const customAdd: CustomAdd = { + disallowValues: true, + isAllowedValue: (value) => !containsSlashes(value), + }; + return ( ); } + +const getStyles = (theme: GrafanaTheme2) => ({ + slashNotAllowed: css` + color: ${theme.colors.warning.main}; + font-size: 12px; + margin-bottom: 2px; + `, + infoIcon: css` + color: ${theme.colors.warning.main}; + font-size: 12px; + margin-bottom: 2px; + cursor: pointer; + `, +});