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.

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

upcoming: [DI: 21998] - Added Service Type, Engine Option and Region Select component to Create Alert form #11286

Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/api-v4": Added
---

service_type as parameter for the Create Alert POST request ([#11286](https://github.com/linode/manager/pull/11286))
13 changes: 10 additions & 3 deletions packages/api-v4/src/cloudpulse/alerts.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
import { createAlertDefinitionSchema } from '@linode/validation';
import Request, { setURL, setMethod, setData } from '../request';
import { Alert, CreateAlertDefinitionPayload } from './types';
import { Alert, AlertServiceType, CreateAlertDefinitionPayload } from './types';
import { BETA_API_ROOT as API_ROOT } from 'src/constants';

export const createAlertDefinition = (data: CreateAlertDefinitionPayload) =>
export const createAlertDefinition = (
data: CreateAlertDefinitionPayload,
service_type: AlertServiceType
) =>
Request<Alert>(
setURL(`${API_ROOT}/monitor/alert-definitions`),
setURL(
`${API_ROOT}/monitor/services/${encodeURIComponent(
service_type!
)}/alert-definitions`
),
setMethod('POST'),
setData(data, createAlertDefinitionSchema)
);
13 changes: 4 additions & 9 deletions packages/api-v4/src/cloudpulse/types.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
export type AlertSeverityType = 0 | 1 | 2 | 3 | null;
type MetricAggregationType = 'avg' | 'sum' | 'min' | 'max' | 'count' | null;
type MetricOperatorType = 'eq' | 'gt' | 'lt' | 'gte' | 'lte' | null;
export type AlertServiceType = 'linode' | 'dbaas' | null;
Copy link
Member

Choose a reason for hiding this comment

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

Why null? Seems like the should not be nullable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Autocomplete component is controlled, so we have it nullable in the cases where it is being initialized , when user clears an option. We have validations in place so the user won't be able to submit any null value we are making sure appropriate values are being selected.

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case, we should probably be extending the type within Cloud Manager. The @linode/api-v4 package is suppose to be 1:1 with the API

type DimensionFilterOperatorType =
| 'eq'
| 'neq'
| 'startswith'
| 'endswith'
| null;
type AlertDefinitionType = 'default' | 'custom';
type AlertStatusType = 'enabled' | 'disabled';
export type AlertDefinitionType = 'default' | 'custom';
export type AlertStatusType = 'enabled' | 'disabled';
export interface Dashboard {
id: number;
label: string;
Expand Down Expand Up @@ -155,12 +156,6 @@ export interface CreateAlertDefinitionPayload {
triggerCondition: TriggerCondition;
channel_ids: number[];
}
export interface CreateAlertDefinitionForm
extends CreateAlertDefinitionPayload {
region: string;
service_type: string;
engine_type: string;
}
export interface MetricCriteria {
metric: string;
aggregation_type: MetricAggregationType;
Expand All @@ -187,7 +182,7 @@ export interface Alert {
status: AlertStatusType;
type: AlertDefinitionType;
severity: AlertSeverityType;
service_type: string;
service_type: AlertServiceType;
resource_ids: string[];
rule_criteria: {
rules: MetricCriteria[];
Expand Down
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-11286-added-1732032870588.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Added
---

Service, Engine Option, Region components to the Create Alert form ([#11286](https://github.com/linode/manager/pull/11286))
27 changes: 27 additions & 0 deletions packages/manager/src/factories/cloudpulse/alerts.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

We've run into test issues in the past with pickRandom introducing non-determinism in our factories. Because we use factories in our tests, we're expecting them to consistently generate the same values and so our best practice is to recommend against them (docs reference).

Can we avoid random assignment here? (cc @jdamore-linode for visibility)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjac0bs can we use Math.Random to pick out the values ?

Copy link
Member

@bnussman-akamai bnussman-akamai Nov 20, 2024

Choose a reason for hiding this comment

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

We are recommending we don't use any randomness in the factories themselves. If you want randomness in the results returned from the Mock Service Worker, you can implement the randomness at that level rather than at the factory level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a commit to remove pickRandom. The only randomness is the Math.random for the id. Should I remove that too @bnussman-akamai ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@santoshp210-akamai I would believe so, would doing something similar to what you're doing with the label work for the id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaalah-akamai , Not sure. Will try something out.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is better, just be aware when writing tests assertions checking for any specific alert properties that the data is dependent on the alert id, and write the assertion to expect the correct value accordingly (or force a value via mocking in test).

Here's an example of what Banks mentioned about randomness in the mock endpoint vs the factory.

serverHandlers.ts:

  http.post(
    '*/monitor/services/:service_type/alert-definitions',
    async ({ request }) => {
      const types: AlertDefinitionType[] = ['custom', 'default'];
      const status: AlertStatusType[] = ['enabled', 'disabled'];
      const severity: AlertSeverityType[] = [0, 1, 2, 3];
      const users = ['user1', 'user2', 'user3'];
      const serviceTypes: AlertServiceType[] = ['linode', 'dbaas'];

      const reqBody = await request.json();
      const response = alertFactory.build({
        ...(reqBody as CreateAlertDefinitionPayload),
        created_by: pickRandom(users),
        serviceTypes: pickRandom(serviceTypes),
        severity: pickRandom(severity),
        status: pickRandom(status),
        type: pickRandom(types),
        updated_by: pickRandom(users),
      });
      return HttpResponse.json(response);
    }
  ),

factories/cloudpulse/alerts.ts:

export const alertFactory = Factory.Sync.makeFactory<Alert>({
  channels: [],
  created: new Date().toISOString(),
  created_by: 'user1',
  description: '',
  id: Factory.each((i) => i),
  label: Factory.each((id) => `Alert-${id}`),
  resource_ids: ['0', '1', '2', '3'],
  rule_criteria: {
    rules: [],
  },
  service_type: 'linode',
  severity: 0,
  status: 'enabled',
  triggerCondition: {
    evaluation_period_seconds: 0,
    polling_interval_seconds: 0,
    trigger_occurrences: 0,
  },
  type: 'default',
  updated: new Date().toISOString(),
  updated_by: 'user1',
});

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import Factory from 'src/factories/factoryProxy';

import type { Alert } from '@linode/api-v4';

export const alertFactory = Factory.Sync.makeFactory<Alert>({
channels: [],
created: new Date().toISOString(),
created_by: 'user1',
description: '',
id: Factory.each((i) => i),
label: Factory.each((id) => `Alert-${id}`),
resource_ids: ['0', '1', '2', '3'],
rule_criteria: {
rules: [],
},
service_type: 'linode',
severity: 0,
status: 'enabled',
triggerCondition: {
evaluation_period_seconds: 0,
polling_interval_seconds: 0,
trigger_occurrences: 0,
},
type: 'default',
updated: new Date().toISOString(),
updated_by: 'user1',
});
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
import { fireEvent, screen, within } from '@testing-library/react';
import { screen, within } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import * as React from 'react';

import { renderWithTheme } from 'src/utilities/testHelpers';

import { CreateAlertDefinition } from './CreateAlertDefinition';
describe('AlertDefinition Create', () => {
it('should render input components', () => {
it('should render input components', async () => {
const { getByLabelText } = renderWithTheme(<CreateAlertDefinition />);

expect(getByLabelText('Name')).toBeVisible();
expect(getByLabelText('Description (optional)')).toBeVisible();
expect(getByLabelText('Severity')).toBeVisible();
});
it('should be able to enter a value in the textbox', () => {
it('should be able to enter a value in the textbox', async () => {
const { getByLabelText } = renderWithTheme(<CreateAlertDefinition />);
const input = getByLabelText('Name');

fireEvent.change(input, { target: { value: 'text' } });
await userEvent.type(input, 'text');
const specificInput = within(screen.getByTestId('alert-name')).getByTestId(
'textfield-input'
);
Expand All @@ -30,7 +30,9 @@ describe('AlertDefinition Create', () => {

await userEvent.click(submitButton!);

expect(getByText('Name is required')).toBeVisible();
expect(getByText('Severity is required')).toBeVisible();
expect(getByText('Name is required.')).toBeVisible();
expect(getByText('Severity is required.')).toBeVisible();
expect(getByText('Service is required.')).toBeVisible();
expect(getByText('Region is required.')).toBeVisible();
});
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { yupResolver } from '@hookform/resolvers/yup';
import { Paper, TextField, Typography } from '@linode/ui';
import { createAlertDefinitionSchema } from '@linode/validation';
import { useSnackbar } from 'notistack';
import * as React from 'react';
import { Controller, FormProvider, useForm } from 'react-hook-form';
Expand All @@ -11,10 +10,14 @@ import { Breadcrumb } from 'src/components/Breadcrumb/Breadcrumb';
import { useCreateAlertDefinition } from 'src/queries/cloudpulse/alerts';

import { CloudPulseAlertSeveritySelect } from './GeneralInformation/AlertSeveritySelect';
import { EngineOption } from './GeneralInformation/EngineOption';
import { CloudPulseRegionSelect } from './GeneralInformation/RegionSelect';
import { CloudPulseServiceSelect } from './GeneralInformation/ServiceTypeSelect';
import { CreateAlertDefinitionFormSchema } from './schemas';
import { filterFormValues } from './utilities';

import type { CreateAlertDefinitionForm } from './types';
import type {
CreateAlertDefinitionForm,
CreateAlertDefinitionPayload,
MetricCriteria,
TriggerCondition,
} from '@linode/api-v4/lib/cloudpulse/types';
Expand All @@ -35,12 +38,12 @@ const criteriaInitialValues: MetricCriteria[] = [
];
const initialValues: CreateAlertDefinitionForm = {
channel_ids: [],
engine_type: '',
engine_type: null,
label: '',
region: '',
resource_ids: [],
rule_criteria: { rules: criteriaInitialValues },
service_type: '',
service_type: null,
severity: null,
triggerCondition: triggerConditionInitialValues,
};
Expand All @@ -62,19 +65,29 @@ export const CreateAlertDefinition = () => {
const alertCreateExit = () =>
history.push('/monitor/cloudpulse/alerts/definitions');

const formMethods = useForm<CreateAlertDefinitionPayload>({
const formMethods = useForm<CreateAlertDefinitionForm>({
defaultValues: initialValues,
mode: 'onBlur',
resolver: yupResolver(createAlertDefinitionSchema),
resolver: yupResolver(CreateAlertDefinitionFormSchema),
});

const { control, formState, handleSubmit, setError } = formMethods;
const {
control,
formState,
getValues,
handleSubmit,
setError,
watch,
} = formMethods;
const { enqueueSnackbar } = useSnackbar();
const { mutateAsync: createAlert } = useCreateAlertDefinition();
const { mutateAsync: createAlert } = useCreateAlertDefinition(
getValues('service_type')!
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we have validation in place, and service types should be some sort of enum in the future, but to me this is a potential runtime issue. @bnussman-akamai do you have any thoughts on this particularly?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This is tough.

Because the endpoint path includes the service type (POST /monitor/services/{service_type}/alert-definitions) but we are getting the service type from the form data, this could cause issues. I think at the worst, it will just cause an API error, which is fine.

But... Why does the createAlertDefinitionSchema have service_type. Is service_type part of the request body or is it in the path? Some things are not making sense to me here

api-v4 and validation should always be aligned API not UI. If needed we can extend the validation schema inside of Cloud Manager

Copy link
Contributor Author

@santoshp210-akamai santoshp210-akamai Nov 20, 2024

Choose a reason for hiding this comment

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

@bnussman-akamai , It is in the path. The reason why service_type is there in createAlertDefinitionSchema is because we need to validate it as it is mandatory for the selection of resources for the Alert.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea that's definitely an oversight, it should be removed from the validation in the schema and added at the UI level. cc: @santoshp210-akamai

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai Are there any similar Components , to extend the validation schema ? The reason we proceeded this way is because of the need for validation for service_type.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're using a Controller we can utilize the rules prop

Copy link
Member

@bnussman-akamai bnussman-akamai Nov 21, 2024

Choose a reason for hiding this comment

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

You can extend a @linode/validation schema like this:

export const CreateLinodeFromStackScriptSchema = CreateLinodeSchema.concat(
object({
stackscript_id: number().required('You must select a StackScript.'),
})
);

);

const serviceWatcher = watch('service_type');
const onSubmit = handleSubmit(async (values) => {
try {
await createAlert(values);
await createAlert(filterFormValues(values));
enqueueSnackbar('Alert successfully created', {
variant: 'success',
});
Expand Down Expand Up @@ -130,6 +143,9 @@ export const CreateAlertDefinition = () => {
control={control}
name="description"
/>
<CloudPulseServiceSelect name="service_type" />
{serviceWatcher === 'dbaas' && <EngineOption name="engine_type" />}
<CloudPulseRegionSelect name="region" />
<CloudPulseAlertSeveritySelect name="severity" />
mjac0bs marked this conversation as resolved.
Show resolved Hide resolved
<ActionsPanel
primaryButtonProps={{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { fireEvent, screen } from '@testing-library/react';
import { screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import * as React from 'react';

import { renderWithThemeAndHookFormContext } from 'src/utilities/testHelpers';
Expand All @@ -13,20 +14,22 @@ describe('EngineOption component tests', () => {
expect(getByLabelText('Severity')).toBeInTheDocument();
expect(getByTestId('severity')).toBeInTheDocument();
});
it('should render the options happy path', () => {
it('should render the options happy path', async () => {
renderWithThemeAndHookFormContext({
component: <CloudPulseAlertSeveritySelect name="severity" />,
});
fireEvent.click(screen.getByRole('button', { name: 'Open' }));
expect(screen.getByRole('option', { name: 'Info' }));
userEvent.click(screen.getByRole('button', { name: 'Open' }));
expect(await screen.findByRole('option', { name: 'Info' }));
expect(screen.getByRole('option', { name: 'Low' }));
});
it('should be able to select an option', () => {
it('should be able to select an option', async () => {
renderWithThemeAndHookFormContext({
component: <CloudPulseAlertSeveritySelect name="severity" />,
});
fireEvent.click(screen.getByRole('button', { name: 'Open' }));
fireEvent.click(screen.getByRole('option', { name: 'Medium' }));
userEvent.click(screen.getByRole('button', { name: 'Open' }));
await userEvent.click(
await screen.findByRole('option', { name: 'Medium' })
);
expect(screen.getByRole('combobox')).toHaveAttribute('value', 'Medium');
});
it('should render the tooltip text', () => {
Expand All @@ -35,12 +38,12 @@ describe('EngineOption component tests', () => {
});

const severityContainer = container.getByTestId('severity');
fireEvent.click(severityContainer);
userEvent.click(severityContainer);

expect(
screen.getByRole('button', {
name:
'Define a severity level associated with the alert to help you prioritize and manage alerts in the Recent activity tab',
'Define a severity level associated with the alert to help you prioritize and manage alerts in the Recent activity tab.',
})
).toBeVisible();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@ import { Autocomplete } from 'src/components/Autocomplete/Autocomplete';

import { alertSeverityOptions } from '../../constants';

import type {
AlertSeverityType,
CreateAlertDefinitionForm,
} from '@linode/api-v4';
import type { CreateAlertDefinitionForm } from '../types';
import type { AlertSeverityType } from '@linode/api-v4';
import type { FieldPathByValue } from 'react-hook-form';
export interface CloudPulseAlertSeveritySelectProps {
/**
Expand Down Expand Up @@ -41,7 +39,7 @@ export const CloudPulseAlertSeveritySelect = (
}}
textFieldProps={{
labelTooltipText:
'Define a severity level associated with the alert to help you prioritize and manage alerts in the Recent activity tab',
'Define a severity level associated with the alert to help you prioritize and manage alerts in the Recent activity tab.',
}}
value={
field.value !== null
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import * as React from 'react';

import { renderWithThemeAndHookFormContext } from 'src/utilities/testHelpers';

import { EngineOption } from './EngineOption';

describe('EngineOption component tests', () => {
it('should render the component when resource type is dbaas', () => {
const { getByLabelText, getByTestId } = renderWithThemeAndHookFormContext({
component: <EngineOption name={'engine_type'} />,
});
expect(getByLabelText('Engine Option')).toBeInTheDocument();
expect(getByTestId('engine-option')).toBeInTheDocument();
});
it('should render the options happy path', async () => {
const user = userEvent.setup();
renderWithThemeAndHookFormContext({
component: <EngineOption name={'engine_type'} />,
});
user.click(screen.getByRole('button', { name: 'Open' }));
expect(await screen.findByRole('option', { name: 'MySQL' }));
expect(screen.getByRole('option', { name: 'PostgreSQL' }));
});
it('should be able to select an option', async () => {
const user = userEvent.setup();
renderWithThemeAndHookFormContext({
component: <EngineOption name={'engine_type'} />,
});
user.click(screen.getByRole('button', { name: 'Open' }));
await user.click(await screen.findByRole('option', { name: 'MySQL' }));
expect(screen.getByRole('combobox')).toHaveAttribute('value', 'MySQL');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import * as React from 'react';
import { Controller, useFormContext } from 'react-hook-form';

import { Autocomplete } from 'src/components/Autocomplete/Autocomplete';

import { engineTypeOptions } from '../../constants';

import type { CreateAlertDefinitionForm } from '../types';
import type { FieldPathByValue } from 'react-hook-form';

interface EngineOptionProps {
/**
* name used for the component to set in the form
*/
name: FieldPathByValue<CreateAlertDefinitionForm, null | string>;
}
export const EngineOption = (props: EngineOptionProps) => {
const { name } = props;
const { control } = useFormContext<CreateAlertDefinitionForm>();
return (
<Controller
render={({ field, fieldState }) => (
<Autocomplete
onChange={(_, selected: { label: string; value: string }, reason) => {
if (reason === 'selectOption') {
field.onChange(selected.value);
}
if (reason === 'clear') {
field.onChange(null);
}
}}
value={
field.value !== null
? engineTypeOptions.find((option) => option.value === field.value)
: null
}
data-testid="engine-option"
errorText={fieldState.error?.message}
label="Engine Option"
onBlur={field.onBlur}
options={engineTypeOptions}
placeholder="Select an Engine"
/>
)}
control={control}
name={name}
/>
);
};
Loading