Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating Preview Message functionality while setting notifications in detector alerts #1241

Merged
merged 4 commits into from
Dec 21, 2024
Merged

Updating Preview Message functionality while setting notifications in detector alerts #1241

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
beb1a8b
Updated Preview Message experience for notifications in detector trig…
nishtham-amazon Dec 18, 2024
6c24361
Updated detector notification template
nishtham-amazon Dec 19, 2024
00d2e7f
updated code review comments
nishtham-amazon Dec 20, 2024
4da8746
Updated context type being passed to detector notification form
nishtham-amazon Dec 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions public/components/Commons/Constants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/
export const DEFAULT_MESSAGE_SOURCE = {
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
MESSAGE_BODY: `- Triggered alert condition: {{ctx.trigger.name}}
- Severity: {{ctx.trigger.severity}}
- Threat detector: {{ctx.detector.name}}
- Description: {{ctx.detector.description}}
- Detector data sources: {{ctx.detector.datasources}}
`.trim(),
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
MESSAGE_SUBJECT: `Triggered alert condition: {{ctx.trigger.name}} - Severity: {{ctx.trigger.severity}} - Threat detector: {{ctx.detector.name}}
`.trim(),
};
46 changes: 33 additions & 13 deletions public/components/Notifications/NotificationForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
EuiSpacer,
EuiCompressedSwitch,
EuiText,
EuiCompressedTextArea,
EuiCompressedTextArea, EuiCompressedCheckbox,
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
} from '@elastic/eui';
import React, { useState } from 'react';
import { NOTIFICATIONS_HREF } from '../../utils/constants';
Expand All @@ -26,6 +26,7 @@ import {
TriggerAction,
} from '../../../types';
import { getIsNotificationPluginInstalled } from '../../utils/helpers';
import Mustache from 'mustache';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use named imports here and other places too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


export interface NotificationFormProps {
allNotificationChannels: NotificationChannelTypeOptions[];
Expand All @@ -37,10 +38,12 @@ export interface NotificationFormProps {
onMessageBodyChange: (message: string) => void;
onMessageSubjectChange: (subject: string) => void;
onNotificationToggle?: (enabled: boolean) => void;
context: any
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
}

export const NotificationForm: React.FC<NotificationFormProps> = ({
action,
context,
allNotificationChannels,
loadingNotifications,
prepareMessage,
Expand All @@ -53,6 +56,8 @@ export const NotificationForm: React.FC<NotificationFormProps> = ({
const hasNotificationPlugin = getIsNotificationPluginInstalled();
const [shouldSendNotification, setShouldSendNotification] = useState(!!action?.destination_id);
const selectedNotificationChannelOption: NotificationChannelOption[] = [];
const onDisplayPreviewChange = (e) => setDisplayPreview(e.target.checked);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memoize this using useCallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

const [displayPreview, setDisplayPreview] = useState(false);
if (shouldSendNotification && action?.destination_id) {
allNotificationChannels.forEach((typeOption) => {
const matchingChannel = typeOption.options.find(
Expand All @@ -61,6 +66,13 @@ export const NotificationForm: React.FC<NotificationFormProps> = ({
if (matchingChannel) selectedNotificationChannelOption.push(matchingChannel);
});
}
let preview = '';
try {
preview = `${Mustache.render(action?.subject_template.source, context)}\n\n${Mustache.render(action?.message_template.source, context)}`;
} catch (err) {
preview = err.message;
console.error('There was an error rendering mustache template', err);
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
}

return (
<>
Expand Down Expand Up @@ -168,18 +180,26 @@ export const NotificationForm: React.FC<NotificationFormProps> = ({
/>
</EuiCompressedFormRow>
</EuiFlexItem>
{prepareMessage && (
<EuiFlexItem>
<EuiCompressedFormRow>
<EuiSmallButton
fullWidth={false}
onClick={() => prepareMessage(true /* updateMessage */)}
>
Generate message
</EuiSmallButton>
</EuiCompressedFormRow>
</EuiFlexItem>
)}

<EuiFlexItem>
<EuiCompressedCheckbox
id={`checked`}
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
label={'Preview message'}
checked={displayPreview}
onChange={(e) => onDisplayPreviewChange(e)}
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
/>
</EuiFlexItem>
{displayPreview ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We can also do `displayPreview && () instead of the ternary to keep it short

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<EuiCompressedFormRow label="Message preview" style={{ maxWidth: '100%' }}>
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
<EuiCompressedTextArea
placeholder="Preview of mustache template"
fullWidth
value={preview}
readOnly
className="read-only-text-area"
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
/>
</EuiCompressedFormRow>
) : null}
</EuiFlexGroup>
</EuiAccordion>

Expand Down
30 changes: 28 additions & 2 deletions ...eateDetector/components/ConfigureAlerts/components/AlertCondition/AlertConditionPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import {
} from '../../../../../../../types';
import { NotificationForm } from '../../../../../../components/Notifications/NotificationForm';
import { ALERT_SEVERITY_OPTIONS } from '../../../../../../utils/constants';
import { DEFAULT_MESSAGE_SOURCE } from '../../../../../../components/Commons/Constants';
import Mustache from 'mustache';

interface AlertConditionPanelProps extends RouteComponentProps {
alertCondition: AlertCondition;
Expand Down Expand Up @@ -99,6 +101,26 @@ export default class AlertConditionPanel extends Component<
});
}

getTriggerContext = () => {
const lineBreakAndTab = '\n\t';
const { alertCondition, detector } = this.props;
const detectorInput = detector.inputs[0].detector_input;
const detectorIndices = `${lineBreakAndTab}${detectorInput.indices.join(
`,${lineBreakAndTab}`
)}`;
return {
trigger: {
name:alertCondition.name,
severity: parseAlertSeverityToOption(alertCondition.severity)?.label || alertCondition.severity
},
detector: {
name: detector.name,
description: detectorInput.description,
datasources: detectorIndices
}
};
};

// When component mounts, we prepare message but at this point we don't want to emit the
// trigger changed metric since it is not user initiated. So we use the onMount flag to determine that
// and pass it downstream accordingly.
Expand All @@ -113,7 +135,7 @@ export default class AlertConditionPanel extends Component<
parseAlertSeverityToOption(alertCondition.severity)?.label || alertCondition.severity
}`;
const detectorName = `Threat detector: ${detector.name}`;
const defaultSubject = [alertConditionName, alertConditionSeverity, detectorName].join(' - ');
const defaultSubject = DEFAULT_MESSAGE_SOURCE.MESSAGE_SUBJECT;

if (updateMessage || !alertCondition.actions[0]?.subject_template.source)
this.onMessageSubjectChange(defaultSubject, !onMount);
Expand Down Expand Up @@ -157,7 +179,7 @@ export default class AlertConditionPanel extends Component<
if (alertConditionSelections.length)
defaultMessageBody =
defaultMessageBody + lineBreak + lineBreak + alertConditionSelections.join(lineBreak);
this.onMessageBodyChange(defaultMessageBody, !onMount);
this.onMessageBodyChange(DEFAULT_MESSAGE_SOURCE.MESSAGE_BODY, !onMount);
}
};

Expand Down Expand Up @@ -286,6 +308,7 @@ export default class AlertConditionPanel extends Component<
};

render() {
const context = this.getTriggerContext();
const {
alertCondition = getEmptyAlertCondition(),
allNotificationChannels,
Expand Down Expand Up @@ -537,6 +560,9 @@ export default class AlertConditionPanel extends Component<

<NotificationForm
action={alertCondition.actions[0]}
context={{
ctx: context,
}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to match the expected type TriggerContext. It doesn't have ctx as the expected field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

allNotificationChannels={allNotificationChannels}
loadingNotifications={loadingNotifications}
prepareMessage={this.prepareMessage}
Expand Down
64 changes: 26 additions & 38 deletions ...ConfigureAlerts/components/AlertCondition/__snapshots__/AlertConditionPanel.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,28 +838,22 @@ Object {
class="euiFlexItem"
>
<div
class="euiFormRow euiFormRow--compressed"
id="some_html_id-row"
class="euiCheckbox euiCheckbox--compressed"
>
<input
class="euiCheckbox__input"
id="checked"
type="checkbox"
/>
<div
class="euiFormRow__fieldWrapper"
class="euiCheckbox__square"
/>
<label
class="euiCheckbox__label"
for="checked"
>
<button
class="euiButton euiButton--primary euiButton--small"
id="some_html_id"
type="button"
>
<span
class="euiButtonContent euiButton__content"
>
<span
class="euiButton__text"
>
Generate message
</span>
</span>
</button>
</div>
Preview message
</label>
</div>
</div>
</div>
Expand Down Expand Up @@ -1707,28 +1701,22 @@ Object {
class="euiFlexItem"
>
<div
class="euiFormRow euiFormRow--compressed"
id="some_html_id-row"
class="euiCheckbox euiCheckbox--compressed"
>
<input
class="euiCheckbox__input"
id="checked"
type="checkbox"
/>
<div
class="euiFormRow__fieldWrapper"
class="euiCheckbox__square"
/>
<label
class="euiCheckbox__label"
for="checked"
>
<button
class="euiButton euiButton--primary euiButton--small"
id="some_html_id"
type="button"
>
<span
class="euiButtonContent euiButton__content"
>
<span
class="euiButton__text"
>
Generate message
</span>
</span>
</button>
</div>
Preview message
</label>
</div>
</div>
</div>
Expand Down