From a0de712bcaab72eb69bda3b4e78e61343f89455c Mon Sep 17 00:00:00 2001 From: Sonia Aguilar <33540275+soniaAguilarPeiron@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:13:27 +0100 Subject: [PATCH] Alerting: Fix simplified query step (#97046) * Fix simplified query step * remove is advancedMode and use editorSettings.simplifiedQueryEditor * remove unnecessary Boolean conversion * fix when feature toggle is disabled * fix test * simplify code * fix when not ff is not enabled --- .../components/rule-editor/QueryWrapper.tsx | 4 +- .../alert-rule-form/AlertRuleForm.tsx | 55 ++++++++++++++---- .../QueryAndExpressionsStep.tsx | 41 +++++++------- .../determineAdvancedMode.test.ts | 56 +------------------ .../useAdvancedMode.ts | 32 ++--------- 5 files changed, 77 insertions(+), 111 deletions(-) diff --git a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx b/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx index 62f2ea6159a..0989fada424 100644 --- a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx @@ -14,6 +14,7 @@ import { RelativeTimeRange, ThresholdsConfig, } from '@grafana/data'; +import { config } from '@grafana/runtime'; import { DataQuery } from '@grafana/schema'; import { GraphThresholdsStyleMode, Icon, InlineField, Input, Stack, Tooltip, useStyles2 } from '@grafana/ui'; import { logInfo } from 'app/features/alerting/unified/Analytics'; @@ -81,7 +82,8 @@ export const QueryWrapper = ({ const defaults = dsInstance?.getDefaultQuery ? dsInstance.getDefaultQuery(CoreApp.UnifiedAlerting) : {}; const { getValues } = useFormContext(); - const isAdvancedMode = getValues('editorSettings.simplifiedQueryEditor') !== true; + const isSwitchModeEnabled = config.featureToggles.alertingQueryAndExpressionsStepMode ?? false; + const isAdvancedMode = isSwitchModeEnabled ? getValues('editorSettings.simplifiedQueryEditor') !== true : true; const queryWithDefaults = { ...defaults, diff --git a/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/AlertRuleForm.tsx b/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/AlertRuleForm.tsx index 185406b26ef..6de19d58e7d 100644 --- a/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/AlertRuleForm.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/AlertRuleForm.tsx @@ -67,7 +67,11 @@ import { GrafanaFolderAndLabelsStep } from '../GrafanaFolderAndLabelsStep'; import { NotificationsStep } from '../NotificationsStep'; import { RecordingRulesNameSpaceAndGroupStep } from '../RecordingRulesNameSpaceAndGroupStep'; import { RuleInspector } from '../RuleInspector'; -import { QueryAndExpressionsStep } from '../query-and-alert-condition/QueryAndExpressionsStep'; +import { + areQueriesTransformableToSimpleCondition, + isExpressionQueryInAlert, + QueryAndExpressionsStep, +} from '../query-and-alert-condition/QueryAndExpressionsStep'; import { translateRouteParamToRuleType } from '../util'; type Props = { @@ -386,15 +390,17 @@ function formValuesFromQueryParams(ruleDefinition: string, type: RuleFormType): }; } - return setInstantOrRange( - ignoreHiddenQueries({ - ...getDefaultFormValues(), - ...ruleFromQueryParams, - annotations: normalizeDefaultAnnotations(ruleFromQueryParams.annotations ?? []), - queries: ruleFromQueryParams.queries ?? getDefaultQueries(), - type: type || RuleFormType.grafana, - evaluateEvery: DEFAULT_GROUP_EVALUATION_INTERVAL, - }) + return setQueryEditorSettings( + setInstantOrRange( + ignoreHiddenQueries({ + ...getDefaultFormValues(), + ...ruleFromQueryParams, + annotations: normalizeDefaultAnnotations(ruleFromQueryParams.annotations ?? []), + queries: ruleFromQueryParams.queries ?? getDefaultQueries(), + type: type || RuleFormType.grafana, + evaluateEvery: DEFAULT_GROUP_EVALUATION_INTERVAL, + }) + ) ); } @@ -405,6 +411,35 @@ function formValuesFromPrefill(rule: Partial): RuleFormValues { }); } +function setQueryEditorSettings(values: RuleFormValues): RuleFormValues { + const isQuerySwitchModeEnabled = config.featureToggles.alertingQueryAndExpressionsStepMode ?? false; + + if (!isQuerySwitchModeEnabled) { + return { + ...values, + editorSettings: { + simplifiedQueryEditor: false, + simplifiedNotificationEditor: true, // actually it doesn't matter in this case + }, + }; + } + + // data queries only + const dataQueries = values.queries.filter((query) => !isExpressionQuery(query.model)); + + // expression queries only + const expressionQueries = values.queries.filter((query) => isExpressionQueryInAlert(query)); + + const queryParamsAreTransformable = areQueriesTransformableToSimpleCondition(dataQueries, expressionQueries); + return { + ...values, + editorSettings: { + simplifiedQueryEditor: queryParamsAreTransformable, + simplifiedNotificationEditor: true, + }, + }; +} + function setInstantOrRange(values: RuleFormValues): RuleFormValues { return { ...values, diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx index 7d0eaeb5b97..8185e12f65c 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx @@ -32,7 +32,6 @@ import { import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto'; import { useRulesSourcesWithRuler } from '../../../hooks/useRuleSourcesWithRuler'; -import { useURLSearchParams } from '../../../hooks/useURLSearchParams'; import { RuleFormType, RuleFormValues } from '../../../types/rule-form'; import { getDefaultOrFirstCompatibleDataSource } from '../../../utils/datasource'; import { isPromOrLokiQuery, PromOrLokiQuery } from '../../../utils/rule-form'; @@ -134,9 +133,7 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange }: P } = useFormContext(); const { queryPreviewData, runQueries, cancelQueries, isPreviewLoading, clearPreviewData } = useAlertQueryRunner(); - const [queryParams] = useURLSearchParams(); const isSwitchModeEnabled = config.featureToggles.alertingQueryAndExpressionsStepMode ?? false; - const isNewFromQueryParams = queryParams.has('defaults') && !editingExistingRule; const initialState = { queries: getValues('queries'), @@ -167,20 +164,23 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange }: P const isCloudAlertRuleType = isCloudAlertingRuleByType(type); const [showResetModeModal, setShowResetModal] = useState(false); - const { isAdvancedMode, simpleCondition, setSimpleCondition } = useAdvancedMode( - editorSettings, + const simplifiedQueryInForm = editorSettings?.simplifiedQueryEditor; + + const { simpleCondition, setSimpleCondition } = useAdvancedMode( + simplifiedQueryInForm, isGrafanaAlertingType, - isNewFromQueryParams, dataQueries, expressionQueries ); + const simplifiedQueryStep = isSwitchModeEnabled ? getValues('editorSettings.simplifiedQueryEditor') : false; + // If we switch to simple mode we need to update the simple condition with the data in the queries reducer useEffect(() => { - if (!isAdvancedMode && isGrafanaAlertingType) { + if (simplifiedQueryStep && isGrafanaAlertingType) { setSimpleCondition(getSimpleConditionFromExpressions(expressionQueries)); } - }, [isAdvancedMode, expressionQueries, isGrafanaAlertingType, setSimpleCondition]); + }, [simplifiedQueryStep, expressionQueries, isGrafanaAlertingType, setSimpleCondition]); const { rulesSourcesWithRuler } = useRulesSourcesWithRuler(); @@ -192,14 +192,14 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange }: P return; } // we need to be sure the condition is set once we switch to simple mode - if (!isAdvancedMode) { + if (simplifiedQueryStep) { setValue('condition', SimpleConditionIdentifier.thresholdId); runQueries(getValues('queries'), SimpleConditionIdentifier.thresholdId); } else { runQueries(getValues('queries'), condition || (getValues('condition') ?? '')); } }, - [isCloudAlertRuleType, runQueries, getValues, isAdvancedMode, setValue] + [isCloudAlertRuleType, runQueries, getValues, simplifiedQueryStep, setValue] ); // whenever we update the queries we have to update the form too @@ -472,13 +472,12 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange }: P if (!type) { return null; } - const switchMode = isGrafanaAlertingType && isSwitchModeEnabled ? { - isAdvancedMode, + isAdvancedMode: !simplifiedQueryStep, setAdvancedMode: (isAdvanced: boolean) => { - if (!isAdvanced) { + if (!getValues('editorSettings.simplifiedQueryEditor')) { if (!areQueriesTransformableToSimpleCondition(dataQueries, expressionQueries)) { setShowResetModal(true); return; @@ -573,7 +572,7 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange }: P condition={condition} onSetCondition={handleSetCondition} /> - {isAdvancedMode && ( + {!simplifiedQueryStep && ( @@ -688,7 +687,7 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange }: P confirmText="Deactivate" icon="exclamation-triangle" onConfirm={() => { - setValue('editorSettings.simplifiedNotificationEditor', true); + setValue('editorSettings.simplifiedQueryEditor', true); setShowResetModal(false); dispatch(resetToSimpleCondition()); }} @@ -768,7 +767,7 @@ const useSetExpressionAndDataSource = () => { }; }; -function isExpressionQueryInAlert( +export function isExpressionQueryInAlert( query: AlertQuery ): query is AlertQuery { return isExpressionQuery(query.model); diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/determineAdvancedMode.test.ts b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/determineAdvancedMode.test.ts index e82f1497399..f296957161e 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/determineAdvancedMode.test.ts +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/determineAdvancedMode.test.ts @@ -1,76 +1,26 @@ -import { produce } from 'immer'; - -import { dataQuery, reduceExpression, thresholdExpression } from '../../../mocks'; - import { determineAdvancedMode } from './useAdvancedMode'; -const dataQueries = [dataQuery]; -const expressionQueries = [reduceExpression, thresholdExpression]; - describe('determineAdvancedMode', () => { it('should return true if simplifiedQueryEditor is false', () => { - const editorSettings = { simplifiedQueryEditor: false, simplifiedNotificationEditor: true }; const isGrafanaAlertingType = true; - const isNewFromQueryParams = false; - const result = determineAdvancedMode( - editorSettings, - isGrafanaAlertingType, - isNewFromQueryParams, - dataQueries, - expressionQueries - ); + const result = determineAdvancedMode(false, isGrafanaAlertingType); expect(result).toBe(true); }); it('should return true if isGrafanaAlertingType is false', () => { - const editorSettings = { simplifiedQueryEditor: true, simplifiedNotificationEditor: true }; const isGrafanaAlertingType = false; - const isNewFromQueryParams = false; - const result = determineAdvancedMode( - editorSettings, - isGrafanaAlertingType, - isNewFromQueryParams, - dataQueries, - expressionQueries - ); - - expect(result).toBe(true); - }); - - const editorSettings = { simplifiedQueryEditor: true, simplifiedNotificationEditor: true }; - it('should return true if isNewFromQueryParams is true and queries are not transformable', () => { - const isGrafanaAlertingType = true; - const isNewFromQueryParams = true; - - const newQuery = produce(dataQuery, (draft) => { - draft.refId = 'whatever'; - }); - - const result = determineAdvancedMode( - editorSettings, - isGrafanaAlertingType, - isNewFromQueryParams, - [newQuery], - expressionQueries - ); + const result = determineAdvancedMode(true, isGrafanaAlertingType); expect(result).toBe(true); }); it('should return false if all conditions are false', () => { const isGrafanaAlertingType = true; - const isNewFromQueryParams = false; - const result = determineAdvancedMode( - editorSettings, - isGrafanaAlertingType, - isNewFromQueryParams, - dataQueries, - expressionQueries - ); + const result = determineAdvancedMode(true, isGrafanaAlertingType); expect(result).toBe(false); }); diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/useAdvancedMode.ts b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/useAdvancedMode.ts index 3fbf4f91874..5708efba7cb 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/useAdvancedMode.ts +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/useAdvancedMode.ts @@ -5,8 +5,6 @@ import { EvalFunction } from 'app/features/alerting/state/alertDef'; import { ExpressionQuery } from 'app/features/expressions/types'; import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto'; -import { SimplifiedEditor } from '../../../types/rule-form'; - import { areQueriesTransformableToSimpleCondition } from './QueryAndExpressionsStep'; import { getSimpleConditionFromExpressions, SimpleCondition } from './SimpleCondition'; @@ -27,19 +25,8 @@ function initializeSimpleCondition( }; } } -export function determineAdvancedMode( - editorSettings: SimplifiedEditor | undefined, - isGrafanaAlertingType: boolean, - isNewFromQueryParams: boolean, - dataQueries: Array>, - expressionQueries: Array> -) { - const queryParamsAreTransformable = areQueriesTransformableToSimpleCondition(dataQueries, expressionQueries); - return ( - Boolean(editorSettings?.simplifiedQueryEditor) === false || - !isGrafanaAlertingType || - (isNewFromQueryParams && !queryParamsAreTransformable) - ); +export function determineAdvancedMode(simplifiedQueryEditor: boolean | undefined, isGrafanaAlertingType: boolean) { + return simplifiedQueryEditor === false || !isGrafanaAlertingType; } /* @@ -47,29 +34,22 @@ export function determineAdvancedMode( depending on the editor settings, the alert type, and the queries. */ export const useAdvancedMode = ( - editorSettings: SimplifiedEditor | undefined, + simplifiedQueryEditor: boolean | undefined, isGrafanaAlertingType: boolean, - isNewFromQueryParams: boolean, dataQueries: Array>, expressionQueries: Array> ) => { - const isAdvancedMode = determineAdvancedMode( - editorSettings, - isGrafanaAlertingType, - isNewFromQueryParams, - dataQueries, - expressionQueries - ); + const isAdvancedMode = determineAdvancedMode(simplifiedQueryEditor, isGrafanaAlertingType); const [simpleCondition, setSimpleCondition] = useState( initializeSimpleCondition(isGrafanaAlertingType, dataQueries, expressionQueries) ); useEffect(() => { - if (!isAdvancedMode && isGrafanaAlertingType) { + if (isGrafanaAlertingType && !isAdvancedMode) { setSimpleCondition(getSimpleConditionFromExpressions(expressionQueries)); } }, [isAdvancedMode, expressionQueries, isGrafanaAlertingType]); - return { isAdvancedMode, simpleCondition, setSimpleCondition }; + return { simpleCondition, setSimpleCondition }; };