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
This commit is contained in:
Sonia Aguilar
2022-09-23 14:44:24 +02:00
committed by GitHub
parent d0d8544ded
commit e714d750f2
4 changed files with 140 additions and 20 deletions

View File

@@ -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<SelectedFolder | null>(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<number> = { 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<HTMLInputElement>) => {
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 <folderWarning.warningComponent />;
} else {
return null;
}
};
const FolderWarningWhenSearching = () => {
if (folderWarning?.warningCondition(inputValue)) {
return <folderWarning.warningComponent />;
} else {
return null;
}
};
if (isCreatingNew) {
return (
<>
<FolderWarningWhenCreating />
<div className={styles.newFolder}>Press enter to create the new folder.</div>
<Input
aria-label={'aria-label'}
width={30}
autoFocus={true}
value={folder?.title || ''}
value={newFolderValue}
onChange={onNewFolderChange}
onKeyDown={onKeyDown}
placeholder="Press enter to confirm new folder."
onBlur={onBlur}
/>
<div className={styles.newFolder}>Press enter to create the new folder.</div>
</>
);
} else {
return (
<div data-testid={selectors.components.FolderPicker.containerV2}>
<FolderWarningWhenSearching />
<AsyncSelect
inputId={inputId}
aria-label={selectors.components.FolderPicker.input}
@@ -286,7 +332,7 @@ export function FolderPicker(props: Props) {
inputValue={inputValue}
onInputChange={onInputChange}
value={folder}
allowCustomValue={enableCreateNew && !customAdd}
allowCustomValue={enableCreateNew && !Boolean(customAdd)}
loadOptions={debouncedSearch}
onChange={onFolderChange}
onCreateOption={createNewFolder}
@@ -324,6 +370,6 @@ const getStyles = (theme: GrafanaTheme2) => ({
newFolder: css`
color: ${theme.colors.warning.main};
font-size: ${theme.typography.bodySmall.fontSize};
padding-top: ${theme.spacing(1)};
padding-bottom: ${theme.spacing(1)};
`,
});

View File

@@ -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(

View File

@@ -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<FolderPickerFilter>(
(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;
`,
});

View File

@@ -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<FolderPickerProps, 'initialTitle' | 'initialFolderId'> {
value?: Folder;
dissalowSlashes: boolean;
}
const SlashesWarning = () => {
const styles = useStyles2(getStyles);
const onClick = () => window.open('https://github.com/grafana/grafana/issues/42947', '_blank');
return (
<Stack gap={0.5}>
<div className={styles.slashNotAllowed}>Folders with &apos;/&apos; character are not allowed.</div>
<Tooltip placement="top" content={'Link to the Github issue'} theme="info">
<Icon name="info-circle" size="xs" className={styles.infoIcon} onClick={onClick} />
</Tooltip>
</Stack>
);
};
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 (
<FolderPicker
showRoot={false}
@@ -23,7 +56,22 @@ export function RuleFolderPicker(props: RuleFolderPickerProps) {
accessControlMetadata
{...props}
permissionLevel={PermissionLevelString.View}
customAdd={true}
customAdd={customAdd}
folderWarning={folderWarning}
/>
);
}
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;
`,
});