Skip to content

Commit a260c44

Browse files
gmmorrisYulNaumenkokibanamachine
authored
[7.x] [Alerting] fixes buggy default message behaviour (#84202) (#84553)
* [Alerting] fixes buggy default message behaviour (#84202) This PR addresses some weird UX we've identified with default values in Action Params components and their inferred defaults when placed inside of an Alerts flyout. Key changes: 1. Typing of these components has been corrected to reflect that we expect these parameters to only be _partial_, as the form is used to set these values (for example, the `message` field of the Server Log action, might or might not be set, so it should be nullable, but in the typing we treated it as the _final_ valid state, which is message not being nullable). 2. When a default message is set by the params components, the are tracked against the value of the default, which means that if the default changes, then so will the value in the field. Custom values provided by the user will not be overridden when the default changes. This has to be handled by the component itself at the moment (hopefully in the future we can make this a concern of the flyout and not each component). 3. The concept of the "Recovered" action group has been removed from these components - that's an Alerting concern, not actions, and shouldn't appear in the action components' code. # Conflicts: # x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.test.tsx * Update server_log_params.test.tsx Removed unused var (caused by bad merge) Co-authored-by: Yuliia Naumenko <yuliia.naumenko@elastic.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
1 parent 0b7ec8b commit a260c44

File tree

9 files changed

+218
-40
lines changed

9 files changed

+218
-40
lines changed

‎x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/email/email_params.test.tsx

+93
Original file line numberDiff line numberDiff line change
@@ -33,4 +33,97 @@ describe('EmailParamsFields renders', () => {
3333
expect(wrapper.find('[data-test-subj="subjectInput"]').length > 0).toBeTruthy();
3434
expect(wrapper.find('[data-test-subj="messageTextArea"]').length > 0).toBeTruthy();
3535
});
36+
37+
test('message param field is rendered with default value if not set', () => {
38+
const actionParams = {
39+
cc: [],
40+
bcc: [],
41+
to: ['test@test.com'],
42+
subject: 'test',
43+
};
44+
45+
const editAction = jest.fn();
46+
mountWithIntl(
47+
<EmailParamsFields
48+
actionParams={actionParams}
49+
errors={{ to: [], cc: [], bcc: [], subject: [], message: [] }}
50+
editAction={editAction}
51+
defaultMessage={'Some default message'}
52+
index={0}
53+
/>
54+
);
55+
56+
expect(editAction).toHaveBeenCalledWith('message', 'Some default message', 0);
57+
});
58+
59+
test('when the default message changes, so is the underlying message if it was set by the previous default', () => {
60+
const actionParams = {
61+
cc: [],
62+
bcc: [],
63+
to: ['test@test.com'],
64+
subject: 'test',
65+
};
66+
67+
const editAction = jest.fn();
68+
const wrapper = mountWithIntl(
69+
<EmailParamsFields
70+
actionParams={actionParams}
71+
errors={{ to: [], cc: [], bcc: [], subject: [], message: [] }}
72+
editAction={editAction}
73+
defaultMessage={'Some default message'}
74+
index={0}
75+
/>
76+
);
77+
78+
expect(editAction).toHaveBeenCalledWith('message', 'Some default message', 0);
79+
80+
wrapper.setProps({
81+
defaultMessage: 'Some different default message',
82+
});
83+
84+
expect(editAction).toHaveBeenCalledWith('message', 'Some different default message', 0);
85+
});
86+
87+
test('when the default message changes, it doesnt change the underlying message if it wasnt set by a previous default', () => {
88+
const actionParams = {
89+
cc: [],
90+
bcc: [],
91+
to: ['test@test.com'],
92+
subject: 'test',
93+
};
94+
95+
const editAction = jest.fn();
96+
const wrapper = mountWithIntl(
97+
<EmailParamsFields
98+
actionParams={actionParams}
99+
errors={{ to: [], cc: [], bcc: [], subject: [], message: [] }}
100+
editAction={editAction}
101+
defaultMessage={'Some default message'}
102+
index={0}
103+
/>
104+
);
105+
106+
expect(editAction).toHaveBeenCalledWith('message', 'Some default message', 0);
107+
108+
// simulate value being updated
109+
const valueToSimulate = 'some new value';
110+
wrapper
111+
.find('[data-test-subj="messageTextArea"]')
112+
.first()
113+
.simulate('change', { target: { value: valueToSimulate } });
114+
expect(editAction).toHaveBeenCalledWith('message', valueToSimulate, 0);
115+
wrapper.setProps({
116+
actionParams: {
117+
...actionParams,
118+
message: valueToSimulate,
119+
},
120+
});
121+
122+
// simulate default changing
123+
wrapper.setProps({
124+
defaultMessage: 'Some different default message',
125+
});
126+
127+
expect(editAction).not.toHaveBeenCalledWith('message', 'Some different default message', 0);
128+
});
36129
});

‎x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/email/email_params.tsx

+9-8
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { ActionParamsProps } from '../../../../types';
1111
import { EmailActionParams } from '../types';
1212
import { TextFieldWithMessageVariables } from '../../text_field_with_message_variables';
1313
import { TextAreaWithMessageVariables } from '../../text_area_with_message_variables';
14-
import { resolvedActionGroupMessage } from '../../../constants';
1514

1615
export const EmailParamsFields = ({
1716
actionParams,
@@ -28,17 +27,19 @@ export const EmailParamsFields = ({
2827
const [addCC, setAddCC] = useState<boolean>(false);
2928
const [addBCC, setAddBCC] = useState<boolean>(false);
3029

30+
const [[isUsingDefault, defaultMessageUsed], setDefaultMessageUsage] = useState<
31+
[boolean, string | undefined]
32+
>([false, defaultMessage]);
3133
useEffect(() => {
32-
if (defaultMessage === resolvedActionGroupMessage) {
33-
editAction('message', defaultMessage, index);
34-
} else if (
35-
(!message || message === resolvedActionGroupMessage) &&
36-
defaultMessage &&
37-
defaultMessage.length > 0
34+
if (
35+
!actionParams?.message ||
36+
(isUsingDefault &&
37+
actionParams?.message === defaultMessageUsed &&
38+
defaultMessageUsed !== defaultMessage)
3839
) {
40+
setDefaultMessageUsage([true, defaultMessage]);
3941
editAction('message', defaultMessage, index);
4042
}
41-
4243
// eslint-disable-next-line react-hooks/exhaustive-deps
4344
}, [defaultMessage]);
4445

‎x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/jira/search_issues.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { useGetSingleIssue } from './use_get_single_issue';
1414
import * as i18n from './translations';
1515

1616
interface Props {
17-
selectedValue: string | null;
17+
selectedValue?: string | null;
1818
http: HttpSetup;
1919
toastNotifications: Pick<
2020
ToastsApi,

‎x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/jira/use_get_fields_by_issue_type.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ interface Props {
2323
ToastsApi,
2424
'get$' | 'add' | 'remove' | 'addSuccess' | 'addWarning' | 'addDanger' | 'addError'
2525
>;
26-
issueType: string;
26+
issueType: string | undefined;
2727
actionConnector?: ActionConnector;
2828
}
2929

‎x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/jira/use_get_single_issue.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ interface Props {
2222
ToastsApi,
2323
'get$' | 'add' | 'remove' | 'addSuccess' | 'addWarning' | 'addDanger' | 'addError'
2424
>;
25-
id: string | null;
25+
id?: string | null;
2626
actionConnector?: ActionConnector;
2727
}
2828

‎x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.test.tsx

+91-8
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ import { ServerLogLevelOptions } from '.././types';
99
import ServerLogParamsFields from './server_log_params';
1010

1111
describe('ServerLogParamsFields renders', () => {
12-
const editAction = jest.fn();
1312
test('all params fields is rendered', () => {
13+
const editAction = jest.fn();
1414
const actionParams = {
1515
level: ServerLogLevelOptions.TRACE,
1616
message: 'test',
@@ -35,20 +35,103 @@ describe('ServerLogParamsFields renders', () => {
3535
test('level param field is rendered with default value if not selected', () => {
3636
const actionParams = {
3737
message: 'test message',
38-
level: ServerLogLevelOptions.INFO,
3938
};
39+
const editAction = jest.fn();
40+
41+
mountWithIntl(
42+
<ServerLogParamsFields
43+
actionParams={actionParams}
44+
errors={{ message: [] }}
45+
editAction={editAction}
46+
index={0}
47+
/>
48+
);
49+
50+
expect(editAction).toHaveBeenCalledWith('level', 'info', 0);
51+
});
52+
53+
test('message param field is rendered with default value if not set', () => {
54+
const actionParams = {
55+
level: ServerLogLevelOptions.TRACE,
56+
};
57+
58+
const editAction = jest.fn();
59+
60+
mountWithIntl(
61+
<ServerLogParamsFields
62+
actionParams={actionParams}
63+
defaultMessage={'Some default message'}
64+
errors={{ message: [] }}
65+
editAction={editAction}
66+
index={0}
67+
/>
68+
);
69+
70+
expect(editAction).toHaveBeenCalledWith('message', 'Some default message', 0);
71+
});
72+
73+
test('when the default message changes, so is the underlying message if it was set by the previous default', () => {
74+
const actionParams = {
75+
level: ServerLogLevelOptions.TRACE,
76+
};
77+
78+
const editAction = jest.fn();
4079
const wrapper = mountWithIntl(
4180
<ServerLogParamsFields
4281
actionParams={actionParams}
82+
defaultMessage={'Some default message'}
4383
errors={{ message: [] }}
44-
editAction={() => {}}
84+
editAction={editAction}
4585
index={0}
4686
/>
4787
);
48-
expect(wrapper.find('[data-test-subj="loggingLevelSelect"]').length > 0).toBeTruthy();
49-
expect(
50-
wrapper.find('[data-test-subj="loggingLevelSelect"]').first().prop('value')
51-
).toStrictEqual('info');
52-
expect(wrapper.find('[data-test-subj="messageTextArea"]').length > 0).toBeTruthy();
88+
89+
expect(editAction).toHaveBeenCalledWith('message', 'Some default message', 0);
90+
91+
wrapper.setProps({
92+
defaultMessage: 'Some different default message',
93+
});
94+
95+
expect(editAction).toHaveBeenCalledWith('message', 'Some different default message', 0);
96+
});
97+
98+
test('when the default message changes, it doesnt change the underlying message if it wasnt set by a previous default', () => {
99+
const actionParams = {
100+
level: ServerLogLevelOptions.TRACE,
101+
};
102+
103+
const editAction = jest.fn();
104+
const wrapper = mountWithIntl(
105+
<ServerLogParamsFields
106+
actionParams={actionParams}
107+
defaultMessage={'Some default message'}
108+
errors={{ message: [] }}
109+
editAction={editAction}
110+
index={0}
111+
/>
112+
);
113+
114+
expect(editAction).toHaveBeenCalledWith('message', 'Some default message', 0);
115+
116+
// simulate value being updated
117+
const valueToSimulate = 'some new value';
118+
wrapper
119+
.find('[data-test-subj="messageTextArea"]')
120+
.first()
121+
.simulate('change', { target: { value: valueToSimulate } });
122+
expect(editAction).toHaveBeenCalledWith('message', valueToSimulate, 0);
123+
wrapper.setProps({
124+
actionParams: {
125+
...actionParams,
126+
message: valueToSimulate,
127+
},
128+
});
129+
130+
// simulate default changing
131+
wrapper.setProps({
132+
defaultMessage: 'Some different default message',
133+
});
134+
135+
expect(editAction).not.toHaveBeenCalledWith('message', 'Some different default message', 0);
53136
});
54137
});

‎x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.tsx

+11-11
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,12 @@
33
* or more contributor license agreements. Licensed under the Elastic License;
44
* you may not use this file except in compliance with the Elastic License.
55
*/
6-
import React, { Fragment, useEffect } from 'react';
6+
import React, { Fragment, useEffect, useState } from 'react';
77
import { i18n } from '@kbn/i18n';
88
import { EuiSelect, EuiFormRow } from '@elastic/eui';
99
import { ActionParamsProps } from '../../../../types';
1010
import { ServerLogActionParams } from '.././types';
1111
import { TextAreaWithMessageVariables } from '../../text_area_with_message_variables';
12-
import { resolvedActionGroupMessage } from '../../../constants';
1312

1413
export const ServerLogParamsFields: React.FunctionComponent<
1514
ActionParamsProps<ServerLogActionParams>
@@ -23,25 +22,26 @@ export const ServerLogParamsFields: React.FunctionComponent<
2322
{ value: 'error', text: 'Error' },
2423
{ value: 'fatal', text: 'Fatal' },
2524
];
26-
2725
useEffect(() => {
28-
if (!actionParams?.level) {
26+
if (!actionParams.level) {
2927
editAction('level', 'info', index);
3028
}
3129
// eslint-disable-next-line react-hooks/exhaustive-deps
3230
}, []);
3331

32+
const [[isUsingDefault, defaultMessageUsed], setDefaultMessageUsage] = useState<
33+
[boolean, string | undefined]
34+
>([false, defaultMessage]);
3435
useEffect(() => {
35-
if (defaultMessage === resolvedActionGroupMessage) {
36-
editAction('message', defaultMessage, index);
37-
} else if (
38-
(!message || message === resolvedActionGroupMessage) &&
39-
defaultMessage &&
40-
defaultMessage.length > 0
36+
if (
37+
!actionParams?.message ||
38+
(isUsingDefault &&
39+
actionParams?.message === defaultMessageUsed &&
40+
defaultMessageUsed !== defaultMessage)
4141
) {
42+
setDefaultMessageUsage([true, defaultMessage]);
4243
editAction('message', defaultMessage, index);
4344
}
44-
4545
// eslint-disable-next-line react-hooks/exhaustive-deps
4646
}, [defaultMessage]);
4747

‎x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/slack/slack_params.tsx

+10-9
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,11 @@
33
* or more contributor license agreements. Licensed under the Elastic License;
44
* you may not use this file except in compliance with the Elastic License.
55
*/
6-
import React, { useEffect } from 'react';
6+
import React, { useEffect, useState } from 'react';
77
import { i18n } from '@kbn/i18n';
88
import { ActionParamsProps } from '../../../../types';
99
import { SlackActionParams } from '../types';
1010
import { TextAreaWithMessageVariables } from '../../text_area_with_message_variables';
11-
import { resolvedActionGroupMessage } from '../../../constants';
1211

1312
const SlackParamsFields: React.FunctionComponent<ActionParamsProps<SlackActionParams>> = ({
1413
actionParams,
@@ -19,17 +18,19 @@ const SlackParamsFields: React.FunctionComponent<ActionParamsProps<SlackActionPa
1918
defaultMessage,
2019
}) => {
2120
const { message } = actionParams;
21+
const [[isUsingDefault, defaultMessageUsed], setDefaultMessageUsage] = useState<
22+
[boolean, string | undefined]
23+
>([false, defaultMessage]);
2224
useEffect(() => {
23-
if (defaultMessage === resolvedActionGroupMessage) {
24-
editAction('message', defaultMessage, index);
25-
} else if (
26-
(!message || message === resolvedActionGroupMessage) &&
27-
defaultMessage &&
28-
defaultMessage.length > 0
25+
if (
26+
!actionParams?.message ||
27+
(isUsingDefault &&
28+
actionParams?.message === defaultMessageUsed &&
29+
defaultMessageUsed !== defaultMessage)
2930
) {
31+
setDefaultMessageUsage([true, defaultMessage]);
3032
editAction('message', defaultMessage, index);
3133
}
32-
3334
// eslint-disable-next-line react-hooks/exhaustive-deps
3435
}, [defaultMessage]);
3536

‎x-pack/plugins/triggers_actions_ui/public/types.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export interface ActionConnectorFieldsProps<TActionConnector> {
4848
}
4949

5050
export interface ActionParamsProps<TParams> {
51-
actionParams: TParams;
51+
actionParams: Partial<TParams>;
5252
index: number;
5353
editAction: (key: string, value: AlertActionParam, index: number) => void;
5454
errors: IErrorObject;

0 commit comments

Comments
 (0)