From 39d3c8afd7f7009d2c88740d18e46b4c7bc7cfd5 Mon Sep 17 00:00:00 2001 From: Peter Holmberg Date: Wed, 20 Apr 2022 09:40:57 +0200 Subject: [PATCH] Alerting: Fix issue with Slack contact point validation (#47559) * secureFields and secureSettings * revert channelIndex * readd lost code * use specific return * register secure fields and use not hard coded index * fix for determineReadOnly * fix lint error * fix test suite Co-authored-by: gillesdemey --- .../ngalert/notifier/available_channels.go | 6 +- .../receivers/form/ChannelOptions.tsx | 10 +--- .../receivers/form/ChannelSubForm.tsx | 6 +- .../receivers/form/fields/OptionField.tsx | 57 ++++++++++++++----- 4 files changed, 53 insertions(+), 26 deletions(-) diff --git a/pkg/services/ngalert/notifier/available_channels.go b/pkg/services/ngalert/notifier/available_channels.go index 65c52ff48e1..317b59db4bb 100644 --- a/pkg/services/ngalert/notifier/available_channels.go +++ b/pkg/services/ngalert/notifier/available_channels.go @@ -383,7 +383,7 @@ func GetAvailableNotifiers() []*alerting.NotifierPlugin { Description: "Specify channel, private group, or IM channel (can be an encoded ID or a name) - required unless you provide a webhook", PropertyName: "recipient", Required: true, - DependsOn: "secureSettings.url", + DependsOn: "url", }, // Logically, this field should be required when not using a webhook, since the Slack API needs a token. // However, since the UI doesn't allow to say that a field is required or not depending on another field, @@ -397,7 +397,7 @@ func GetAvailableNotifiers() []*alerting.NotifierPlugin { PropertyName: "token", Secure: true, Required: true, - DependsOn: "secureSettings.url", + DependsOn: "url", }, { Label: "Username", @@ -463,7 +463,7 @@ func GetAvailableNotifiers() []*alerting.NotifierPlugin { PropertyName: "url", Secure: true, Required: true, - DependsOn: "secureSettings.token", + DependsOn: "token", }, { // New in 8.4. Label: "Endpoint URL", diff --git a/public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx b/public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx index 875cd515e29..0e107f86e4c 100644 --- a/public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx +++ b/public/app/features/alerting/unified/components/receivers/form/ChannelOptions.tsx @@ -47,12 +47,7 @@ export function ChannelOptions({ value="Configured" suffix={ readOnly ? null : ( - ) @@ -74,7 +69,8 @@ export function ChannelOptions({ readOnly={readOnly} key={key} error={error} - pathPrefix={option.secure ? `${pathPrefix}secureSettings.` : `${pathPrefix}settings.`} + pathPrefix={pathPrefix} + pathSuffix={option.secure ? 'secureSettings.' : 'settings.'} option={option} /> ); diff --git a/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx b/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx index 57cad1a84c6..b605088aab2 100644 --- a/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx +++ b/public/app/features/alerting/unified/components/receivers/form/ChannelSubForm.tsx @@ -37,12 +37,15 @@ export function ChannelSubForm({ }: Props): JSX.Element { const styles = useStyles2(getStyles); const name = (fieldName: string) => `${pathPrefix}${fieldName}`; - const { control, watch, register, trigger, formState } = useFormContext(); + const { control, watch, register, trigger, formState, setValue } = useFormContext(); const selectedType = watch(name('type')) ?? defaultValues.type; // nope, setting "default" does not work at all. const { loading: testingReceiver } = useUnifiedAlertingSelector((state) => state.testReceivers); useEffect(() => { register(`${pathPrefix}.__id`); + /* Need to manually register secureFields or else they'll + be lost when testing a contact point */ + register(`${pathPrefix}.secureFields`); }, [register, pathPrefix]); const [_secureFields, setSecureFields] = useState(secureFields ?? {}); @@ -52,6 +55,7 @@ export function ChannelSubForm({ const updatedSecureFields = { ...secureFields }; delete updatedSecureFields[key]; setSecureFields(updatedSecureFields); + setValue(`${pathPrefix}.secureFields`, updatedSecureFields); } }; diff --git a/public/app/features/alerting/unified/components/receivers/form/fields/OptionField.tsx b/public/app/features/alerting/unified/components/receivers/form/fields/OptionField.tsx index 3d6c6df0329..4ed2456f06d 100644 --- a/public/app/features/alerting/unified/components/receivers/form/fields/OptionField.tsx +++ b/public/app/features/alerting/unified/components/receivers/form/fields/OptionField.tsx @@ -1,5 +1,6 @@ import React, { FC, useEffect } from 'react'; import { Checkbox, Field, Input, InputControl, Select, TextArea } from '@grafana/ui'; +import { isEmpty } from 'lodash'; import { NotificationChannelOption } from 'app/types'; import { useFormContext, FieldError, DeepMap } from 'react-hook-form'; import { SubformField } from './SubformField'; @@ -13,11 +14,22 @@ interface Props { option: NotificationChannelOption; invalid?: boolean; pathPrefix: string; + pathSuffix?: string; error?: FieldError | DeepMap; readOnly?: boolean; } -export const OptionField: FC = ({ option, invalid, pathPrefix, error, defaultValue, readOnly = false }) => { +export const OptionField: FC = ({ + option, + invalid, + pathPrefix, + pathSuffix = '', + error, + defaultValue, + readOnly = false, +}) => { + const optionPath = `${pathPrefix}${pathSuffix}`; + if (option.element === 'subform') { return ( = ({ option, invalid, pathPrefix, error, def defaultValue={defaultValue} option={option} errors={error as DeepMap | undefined} - pathPrefix={pathPrefix} + pathPrefix={optionPath} /> ); } @@ -35,7 +47,7 @@ export const OptionField: FC = ({ option, invalid, pathPrefix, error, def readOnly={readOnly} defaultValues={defaultValue} option={option} - pathPrefix={pathPrefix} + pathPrefix={optionPath} errors={error as Array> | undefined} /> ); @@ -48,18 +60,26 @@ export const OptionField: FC = ({ option, invalid, pathPrefix, error, def error={error?.message} > ); }; -const OptionInput: FC = ({ option, invalid, id, pathPrefix = '', readOnly = false }) => { +const OptionInput: FC = ({ + option, + invalid, + id, + pathPrefix = '', + pathIndex = '', + readOnly = false, +}) => { const { control, register, unregister, getValues } = useFormContext(); const name = `${pathPrefix}${option.propertyName}`; @@ -87,11 +107,11 @@ const OptionInput: FC = ({ option, invalid, id, pathPref return ( (option.validationRule !== '' ? validateOption(v, option.validationRule) : true), })} placeholder={option.placeholder} @@ -165,19 +185,26 @@ const validateOption = (value: string, validationRule: string) => { return RegExp(validationRule).test(value) ? true : 'Invalid format'; }; -const determineRequired = (option: NotificationChannelOption, getValues: any) => { +const determineRequired = (option: NotificationChannelOption, getValues: any, pathIndex: string) => { if (!option.dependsOn) { return option.required ? 'Required' : false; } - - const dependentOn = getValues(`items[0].${option.dependsOn}`); - return !dependentOn && option.required ? 'Required' : false; + if (isEmpty(getValues(`${pathIndex}secureFields`))) { + const dependentOn = getValues(`${pathIndex}secureSettings.${option.dependsOn}`); + return !Boolean(dependentOn) && option.required ? 'Required' : false; + } else { + const dependentOn: boolean = getValues(`${pathIndex}secureFields.${option.dependsOn}`); + return !dependentOn && option.required ? 'Required' : false; + } }; -const determineReadOnly = (option: NotificationChannelOption, getValues: any) => { +const determineReadOnly = (option: NotificationChannelOption, getValues: any, pathIndex: string) => { if (!option.dependsOn) { return false; } - - return getValues(`items[0].${option.dependsOn}`); + if (isEmpty(getValues(`${pathIndex}secureFields`))) { + return getValues(`${pathIndex}secureSettings.${option.dependsOn}`); + } else { + return getValues(`${pathIndex}secureFields.${option.dependsOn}`); + } };