Skip to content

Commit fd4dade

Browse files
dominiqueclarkeyiannisnikolopoulos
dominiqueclarke
and
yiannisnikolopoulos
authored
[Alerting v2] [Rule authoring] Fix alert/recovery delay mode not persisting after save (#260314)
## Summary Recreates #259309, which was closed when the `alerting_v2` branch was merged into `main`. Fixes a multi-layered bug in the Alerting v2 edit rule form where changing the **alert delay** or **recovery delay** and saving would appear to have no effect — the previous mode reappeared the next time the form was opened. Closes: elastic/rna-program#252 ### Root causes 1. **Stale react-hook-form state (primary cause)** — RHF retains field values even after a mode switch. Introduced `stateTransitionAlertDelayMode` / `stateTransitionRecoveryDelayMode` as explicit source-of-truth fields in `FormValues`. `mapStateTransition` now reads these mode fields first and ignores stale numeric values when the mode is `immediate`. 2. **Server-side `null` swallowing** — `nullToUndefionverted `null → undefined` before writing to the saved object. Renamed to `applyNullableUpdate`: `null` is now kept as `null` so the key is present in the SO update and Elasticsearch clears the existing value. 3. **`undefined` instead of `null` for cleared field values** — All cleared fields now use `null` explicitly. 4. **`updateRuleDataSchema` missing explicit `.nullable()`** — `state_transition` in the update schema lacked an explicit `.nullable()` wrapper. 5. **Stale-cache scenario in `FetchedRuleFormPage`** — Added the `!isFetchedAfterMount && isFetching` guard. 6. **`undefined` instead of `null` when removing the recovery base query** — Changed to `null` and widened `RecoveryPolicy.query.base` to `string | null` in `FormValues`. ### Additional clean-ups * `AlertDelayField` and `RecoveryDelayField` now use distinct `EuiButtonGroup` option IDs per group to avoid a11y. * Mode switches now call `setValue('stateTransition', { ...spread })` on the whole object rather than separate nested-path `setValue` calls. * `useEffect`-based default initialisation replaced with `defaultValue` on the RHF `Controller`. * `deriveAlertDelayModeFromStateTransition` / `deriveRecoveryDelayModeFromStateTransition` extracted and exported. * `RecoveryPolicy.query.base` is now `string | null`. ## Test plan * Open the edit form for a rule with no delay configured (Immediate). Switch alert delay to **Breaches**, save. Reopen — verify Breaches is shown with the correct count. * Open the edit form for a rule with Breaches configured. Switch back to **Immediate**, save. Reopen — verify Immediate is shown and no `state_transition` is sent in the PATCH. * Repeat both scenarios for **Duration** mode and for the **Recovery delay** field independently. * Verify that changing alert delay and recovery delay to different modes in the same save round-trips correctly. * Verify that opening the edit form whReact Query has stale cached data shows a spinner until the fresh fetch completes. * Run unit tests: `yarn test:jest <alerting-v2-rule-form package> <alerting_v2 plugin>` --------- Co-authored-by: Yiannis Nikolopoulos <yiannis.nikolopoulos@elastic.co>
1 parent 28af6e7 commit fd4dade

33 files changed

Lines changed: 776 additions & 212 deletions

‎x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.test.tsx‎

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,21 @@
66
*/
77

88
import React from 'react';
9-
import { render, screen, fireEvent } from '@testing-library/react';
9+
import { render, screen, fireEvent, within } from '@testing-library/react';
10+
import { useFormContext } from 'react-hook-form';
1011
import { AlertDelayField } from './alert_delay_field';
12+
import { RecoveryDelayField } from './recovery_delay_field';
13+
import type { FormValues } from '../types';
14+
import { mapFormValuesToUpdateRequest } from '../utils/rule_request_mappers';
1115
import { createFormWrapper } from '../../test_utils';
1216

17+
let getFormValues: (() => FormValues) | undefined;
18+
19+
const CaptureFormGetValues = () => {
20+
getFormValues = useFormContext<FormValues>().getValues;
21+
return null;
22+
};
23+
1324
describe('AlertDelayField', () => {
1425
it('renders the alert delay form row', () => {
1526
render(<AlertDelayField />, {
@@ -36,21 +47,23 @@ describe('AlertDelayField', () => {
3647
expect(screen.getByText('No delay - Alerts on first breach')).toBeInTheDocument();
3748
});
3849

39-
it('derives breaches mode from form state with pendingCount', () => {
50+
it('shows breaches controls when alert delay mode is breaches', () => {
4051
render(<AlertDelayField />, {
4152
wrapper: createFormWrapper({
4253
kind: 'alert',
54+
stateTransitionAlertDelayMode: 'breaches',
4355
stateTransition: { pendingCount: 3 },
4456
}),
4557
});
4658

4759
expect(screen.getByTestId('stateTransitionCountInput')).toBeInTheDocument();
4860
});
4961

50-
it('derives duration mode from form state with pendingTimeframe', () => {
62+
it('shows duration controls when alert delay mode is duration', () => {
5163
render(<AlertDelayField />, {
5264
wrapper: createFormWrapper({
5365
kind: 'alert',
66+
stateTransitionAlertDelayMode: 'duration',
5467
stateTransition: { pendingTimeframe: '10m' },
5568
}),
5669
});
@@ -84,6 +97,7 @@ describe('AlertDelayField', () => {
8497
render(<AlertDelayField />, {
8598
wrapper: createFormWrapper({
8699
kind: 'alert',
100+
stateTransitionAlertDelayMode: 'breaches',
87101
stateTransition: { pendingCount: 3 },
88102
}),
89103
});
@@ -102,4 +116,41 @@ describe('AlertDelayField', () => {
102116

103117
expect(screen.getByTestId('stateTransitionDelayMode')).toBeInTheDocument();
104118
});
119+
120+
it('clears alert delay (pending) when switching to immediate while recovery delay stays on breaches', () => {
121+
getFormValues = undefined;
122+
render(
123+
<>
124+
<CaptureFormGetValues />
125+
<AlertDelayField />
126+
<RecoveryDelayField />
127+
</>,
128+
{
129+
wrapper: createFormWrapper({
130+
kind: 'alert',
131+
stateTransitionAlertDelayMode: 'breaches',
132+
stateTransitionRecoveryDelayMode: 'breaches',
133+
stateTransition: {
134+
pendingCount: 2,
135+
pendingTimeframe: null,
136+
recoveringCount: 3,
137+
recoveringTimeframe: null,
138+
},
139+
}),
140+
}
141+
);
142+
143+
const alertRow = screen.getByTestId('alertDelayFormRow');
144+
fireEvent.click(within(alertRow).getByText('Immediate'));
145+
146+
const values = getFormValues!();
147+
expect(values.stateTransitionAlertDelayMode).toBe('immediate');
148+
expect(values.stateTransition?.pendingCount).toBeNull();
149+
expect(values.stateTransition?.pendingTimeframe).toBeNull();
150+
expect(values.stateTransition?.recoveringCount).toBe(3);
151+
152+
expect(mapFormValuesToUpdateRequest(values).state_transition).toEqual({
153+
recovering_count: 3,
154+
});
155+
});
105156
});

‎x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/alert_delay_field.tsx‎

Lines changed: 62 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -5,81 +5,110 @@
55
* 2.0.
66
*/
77

8-
import React, { useCallback, useState } from 'react';
8+
import React, { useCallback } from 'react';
99
import { EuiButtonGroup, EuiFormRow, EuiSpacer, EuiText } from '@elastic/eui';
1010
import { i18n } from '@kbn/i18n';
1111
import { useFormContext, useWatch } from 'react-hook-form';
1212
import type { FormValues } from '../types';
13+
import { deriveAlertDelayModeFromStateTransition } from '../utils/rule_request_mappers';
1314
import { StateTransitionCountField } from './state_transition_count_field';
1415
import { StateTransitionTimeframeField } from './state_transition_timeframe_field';
1516

1617
type DelayMode = 'immediate' | 'breaches' | 'duration';
1718

19+
const MODE_OPTION_IDS = {
20+
immediate: 'alert_delay_mode_immediate',
21+
breaches: 'alert_delay_mode_breaches',
22+
duration: 'alert_delay_mode_duration',
23+
} as const;
24+
1825
const MODE_OPTIONS = [
1926
{
20-
id: 'immediate' as const,
27+
id: MODE_OPTION_IDS.immediate,
2128
label: i18n.translate('xpack.alertingV2.ruleForm.alertDelay.delayModeImmediate', {
2229
defaultMessage: 'Immediate',
2330
}),
2431
},
2532
{
26-
id: 'breaches' as const,
33+
id: MODE_OPTION_IDS.breaches,
2734
label: i18n.translate('xpack.alertingV2.ruleForm.alertDelay.delayModeBreaches', {
2835
defaultMessage: 'Breaches',
2936
}),
3037
},
3138
{
32-
id: 'duration' as const,
39+
id: MODE_OPTION_IDS.duration,
3340
label: i18n.translate('xpack.alertingV2.ruleForm.alertDelay.delayModeDuration', {
3441
defaultMessage: 'Duration',
3542
}),
3643
},
3744
];
3845

39-
const DEFAULT_PENDING_COUNT = 2;
40-
const DEFAULT_PENDING_TIMEFRAME = '2m';
46+
const modeFromOptionId = (id: string): DelayMode => {
47+
if (id === MODE_OPTION_IDS.immediate) return 'immediate';
48+
if (id === MODE_OPTION_IDS.duration) return 'duration';
49+
return 'breaches';
50+
};
4151

42-
const deriveMode = (stateTransition?: {
43-
pendingTimeframe?: string;
44-
pendingCount?: number;
45-
}): DelayMode => {
46-
if (stateTransition?.pendingTimeframe != null) return 'duration';
47-
if (stateTransition?.pendingCount != null) return 'breaches';
48-
return 'immediate';
52+
const optionIdForMode = (mode: DelayMode): string => {
53+
if (mode === 'immediate') return MODE_OPTION_IDS.immediate;
54+
if (mode === 'duration') return MODE_OPTION_IDS.duration;
55+
return MODE_OPTION_IDS.breaches;
4956
};
5057

58+
const DEFAULT_PENDING_COUNT = 2;
59+
const DEFAULT_PENDING_TIMEFRAME = '2m';
60+
5161
export const AlertDelayField = () => {
52-
const { control, setValue } = useFormContext<FormValues>();
62+
const { control, getValues, setValue } = useFormContext<FormValues>();
5363
const stateTransition = useWatch({ control, name: 'stateTransition' });
54-
const [selectedMode, setSelectedMode] = useState<DelayMode>(deriveMode(stateTransition));
64+
const selectedMode = useWatch({ control, name: 'stateTransitionAlertDelayMode' });
65+
const displayMode: DelayMode =
66+
selectedMode ?? deriveAlertDelayModeFromStateTransition(stateTransition);
5567

5668
const onModeChange = useCallback(
57-
(mode: string) => {
58-
switch (mode as DelayMode) {
69+
(optionId: string) => {
70+
const st = getValues('stateTransition') ?? {};
71+
const nextMode = modeFromOptionId(optionId);
72+
switch (nextMode) {
5973
case 'immediate':
60-
setSelectedMode('immediate');
61-
setValue('stateTransition.pendingCount', undefined);
62-
setValue('stateTransition.pendingTimeframe', undefined);
74+
setValue('stateTransitionAlertDelayMode', 'immediate', { shouldDirty: true });
75+
setValue(
76+
'stateTransition',
77+
{
78+
...st,
79+
pendingCount: null,
80+
pendingTimeframe: null,
81+
},
82+
{ shouldDirty: true, shouldTouch: true }
83+
);
6384
break;
6485
case 'breaches':
65-
setSelectedMode('breaches');
86+
setValue('stateTransitionAlertDelayMode', 'breaches', { shouldDirty: true });
6687
setValue(
67-
'stateTransition.pendingCount',
68-
stateTransition?.pendingCount ?? DEFAULT_PENDING_COUNT
88+
'stateTransition',
89+
{
90+
...st,
91+
pendingCount: st.pendingCount ?? DEFAULT_PENDING_COUNT,
92+
pendingTimeframe: null,
93+
},
94+
{ shouldDirty: true, shouldTouch: true }
6995
);
70-
setValue('stateTransition.pendingTimeframe', undefined);
7196
break;
7297
case 'duration':
73-
setSelectedMode('duration');
74-
setValue('stateTransition.pendingCount', undefined);
98+
setValue('stateTransitionAlertDelayMode', 'duration', { shouldDirty: true });
7599
setValue(
76-
'stateTransition.pendingTimeframe',
77-
stateTransition?.pendingTimeframe ?? DEFAULT_PENDING_TIMEFRAME
100+
'stateTransition',
101+
{
102+
...st,
103+
pendingCount: null,
104+
pendingTimeframe: st.pendingTimeframe ?? DEFAULT_PENDING_TIMEFRAME,
105+
},
106+
{ shouldDirty: true, shouldTouch: true }
78107
);
79108
break;
80109
}
81110
},
82-
[setValue, stateTransition?.pendingCount, stateTransition?.pendingTimeframe]
111+
[getValues, setValue]
83112
);
84113

85114
return (
@@ -97,20 +126,20 @@ export const AlertDelayField = () => {
97126
defaultMessage: 'Alert delay mode',
98127
})}
99128
options={MODE_OPTIONS}
100-
idSelected={selectedMode}
129+
idSelected={optionIdForMode(displayMode)}
101130
onChange={onModeChange}
102131
isFullWidth
103132
data-test-subj="stateTransitionDelayMode"
104133
/>
105134
<EuiSpacer size="s" />
106-
{selectedMode === 'immediate' && (
135+
{displayMode === 'immediate' && (
107136
<EuiText size="xs" color="subdued" data-test-subj="stateTransitionImmediateDescription">
108137
{i18n.translate('xpack.alertingV2.ruleForm.alertDelay.immediateDescription', {
109138
defaultMessage: 'No delay - Alerts on first breach',
110139
})}
111140
</EuiText>
112141
)}
113-
{selectedMode === 'breaches' && (
142+
{displayMode === 'breaches' && (
114143
<StateTransitionCountField
115144
variant="pending"
116145
prependLabel={i18n.translate(
@@ -119,7 +148,7 @@ export const AlertDelayField = () => {
119148
)}
120149
/>
121150
)}
122-
{selectedMode === 'duration' && (
151+
{displayMode === 'duration' && (
123152
<StateTransitionTimeframeField
124153
variant="pending"
125154
numberPrependLabel={i18n.translate(

‎x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/esql_editor_field.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ export interface EsqlEditorFieldProps {
4646
rules?: {
4747
required?: string;
4848
/** Validation function - can be sync or async. Return true for valid, or error string for invalid. */
49-
validate?: (value: string | undefined) => string | boolean | Promise<string | boolean>;
49+
validate?: (value: string | null | undefined) => string | boolean | Promise<string | boolean>;
5050
};
5151
/** Server-side errors to display in the editor */
5252
errors?: Error[];

‎x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/evaluation_query_field.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ export const EvaluationQueryField = ({
2929
required: i18n.translate('xpack.alertingV2.ruleForm.queryRequiredError', {
3030
defaultMessage: 'ES|QL query is required.',
3131
}),
32-
validate: (value: string | undefined) => (value ? validateEsqlQuery(value) ?? true : true),
32+
validate: (value: string | null | undefined) =>
33+
value ? validateEsqlQuery(value) ?? true : true,
3334
}}
3435
/>
3536
);

‎x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_base_and_condition_field.test.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ describe('RecoveryBaseAndConditionField', () => {
377377
await user.click(screen.getByTestId('removeRecoveryBaseQueryButton'));
378378

379379
// Form value should be cleared
380-
expect(formRef?.getValues('recoveryPolicy.query.base')).toBeUndefined();
380+
expect(formRef?.getValues('recoveryPolicy.query.base')).toBeNull();
381381
});
382382

383383
it('hides the base query editor when remove is clicked and validation reflects removal', async () => {

‎x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_base_and_condition_field.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ export const RecoveryBaseAndConditionField = ({
8181
}, [getValues, setValue]);
8282

8383
const handleRemoveBaseQuery = useCallback(() => {
84-
setValue('recoveryPolicy.query.base', undefined);
84+
setValue('recoveryPolicy.query.base', null);
8585
setIsBaseQueryVisible(false);
8686
}, [setValue]);
8787

‎x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/fields/recovery_base_query_field.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ interface RecoveryBaseQueryFieldProps {
2222
/** Validation rules provided by the parent (from useRecoveryValidation hook). */
2323
rules?: {
2424
required?: string;
25-
validate?: (value: string | undefined) => string | boolean | Promise<string | boolean>;
25+
validate?: (value: string | null | undefined) => string | boolean | Promise<string | boolean>;
2626
};
2727
/** Errors to display in the editor (e.g., grouping validation errors). */
2828
errors?: Error[];

0 commit comments

Comments
 (0)