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 3 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": Changed
---

Added service_type as parameter for the Create Alert POST request ([#11286](https://github.com/linode/manager/pull/11286))
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be better as an 'Added' changeset, and without the 'Added' at the beginning of the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it

11 changes: 9 additions & 2 deletions packages/api-v4/src/cloudpulse/alerts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,16 @@ import Request, { setURL, setMethod, setData } from '../request';
import { Alert, 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: string
Copy link
Member

Choose a reason for hiding this comment

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

Optional: Maybe we could use AlertServiceType here.

) =>
Request<Alert>(
setURL(`${API_ROOT}/monitor/alert-definitions`),
setURL(
`${API_ROOT}/monitor/services/${encodeURIComponent(
service_type
)}/alert-definitions`
),
setMethod('POST'),
setData(data, createAlertDefinitionSchema)
);
4 changes: 2 additions & 2 deletions packages/api-v4/src/cloudpulse/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,8 @@ export interface CreateAlertDefinitionPayload {
export interface CreateAlertDefinitionForm
extends CreateAlertDefinitionPayload {
region: string;
service_type: string;
engine_type: string;
service_type: string | null;
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 type for the service_type has been kept string as we're getting the data from the API. Once we're aware of the possible service_types , we will use specific strings as a type for it

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we had an understanding as to what these will be: enum['linode','lke','aclb','dbaas']?

engine_type: string | null;
Copy link
Member

Choose a reason for hiding this comment

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

Is this reflected in the API spec?

Also wondering if the API really needs this. Don't all Databases regardless of engine have unique IDs? Maybe the backend could determine this without us having to pass it.

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 This is a good question, in addition we may want to do the same when it comes to the schema and add it at the UI level if it's something we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not there as part of the request, so we are not sending these attributes but we have this interface extended so we can have this part of the form for the to take the user's input and for validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@santoshp210-akamai This is a good question, in addition we may want to do the same when it comes to the schema and add it at the UI level if it's something we need.

So just for these components, we just add it in UI and then validate separately ?

}
export interface MetricCriteria {
metric: string;
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
---

Add Service, Engine Option, Region components to the Create Alert form ([#11286](https://github.com/linode/manager/pull/11286))
mjac0bs marked this conversation as resolved.
Show resolved Hide resolved
28 changes: 28 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,28 @@
import Factory from 'src/factories/factoryProxy';
import { pickRandom } from 'src/utilities/random';

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

export const alertFactory = Factory.Sync.makeFactory<Alert>({
channels: [],
created: new Date().toISOString(),
created_by: Factory.each(() => pickRandom(['user1', 'user2', 'user3'])),
description: '',
id: Factory.each(() => Math.floor(Math.random() * 1000000)),
label: Factory.each((id) => `Alert-${id}`),
resource_ids: ['1', '2', '3'],
rule_criteria: {
rules: [],
},
service_type: Factory.each(() => pickRandom(['linode', 'dbaas'])),
severity: Factory.each(() => pickRandom([0, 1, 2, 3])),
status: Factory.each(() => pickRandom(['enabled', 'disabled'])),
triggerCondition: {
evaluation_period_seconds: 0,
polling_interval_seconds: 0,
trigger_occurrences: 0,
},
type: Factory.each(() => pickRandom(['default', 'custom'])),
updated: new Date().toISOString(),
updated_by: Factory.each(() => pickRandom(['user1', 'user2', 'user3'])),
});
Original file line number Diff line number Diff line change
Expand Up @@ -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 type is required.')).toBeVisible();
expect(getByText('Region is required.')).toBeVisible();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ import { Typography } from 'src/components/Typography';
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 { getCreateAlertPayload } from './utilities';

import type {
CreateAlertDefinitionForm,
CreateAlertDefinitionPayload,
MetricCriteria,
TriggerCondition,
} from '@linode/api-v4/lib/cloudpulse/types';
Expand All @@ -37,12 +40,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 @@ -64,19 +67,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),
});

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(getCreateAlertPayload(values));
enqueueSnackbar('Alert successfully created', {
variant: 'success',
});
Expand Down Expand Up @@ -132,6 +145,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
@@ -0,0 +1,32 @@
import { fireEvent, screen } from '@testing-library/react';
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', () => {
renderWithThemeAndHookFormContext({
component: <EngineOption name={'engine_type'} />,
});
fireEvent.click(screen.getByRole('button', { name: 'Open' }));
mjac0bs marked this conversation as resolved.
Show resolved Hide resolved
expect(screen.getByRole('option', { name: 'MySQL' }));
expect(screen.getByRole('option', { name: 'PostgreSQL' }));
});
it('should be able to select an option', () => {
renderWithThemeAndHookFormContext({
component: <EngineOption name={'engine_type'} />,
});
fireEvent.click(screen.getByRole('button', { name: 'Open' }));
fireEvent.click(screen.getByRole('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 '@linode/api-v4';
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}
/>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import * as React from 'react';

import * as regions from 'src/queries/regions/regions';
import { renderWithThemeAndHookFormContext } from 'src/utilities/testHelpers';

import { CloudPulseRegionSelect } from './RegionSelect';

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

describe('RegionSelect', () => {
vi.spyOn(regions, 'useRegionsQuery').mockReturnValue({
data: Array<Region>(),
} as ReturnType<typeof regions.useRegionsQuery>);

it('should render a RegionSelect component', () => {
const { getByTestId } = renderWithThemeAndHookFormContext({
component: <CloudPulseRegionSelect name="region" />,
});
expect(getByTestId('region-select')).toBeInTheDocument();
});
it('should render a Region Select component with proper error message on api call failure', () => {
vi.spyOn(regions, 'useRegionsQuery').mockReturnValue({
data: undefined,
isError: true,
isLoading: false,
} as ReturnType<typeof regions.useRegionsQuery>);
const { getByText } = renderWithThemeAndHookFormContext({
component: <CloudPulseRegionSelect name="region" />,
});

expect(getByText('Failed to fetch Region.'));
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import * as React from 'react';
import { Controller, useFormContext } from 'react-hook-form';

import { RegionSelect } from 'src/components/RegionSelect/RegionSelect';
import { useRegionsQuery } from 'src/queries/regions/regions';

export interface CloudViewRegionSelectProps {
/**
* name used for the component to set in the form
*/
name: string;
}

export const CloudPulseRegionSelect = (props: CloudViewRegionSelectProps) => {
const { name } = props;
const { data: regions, isError, isLoading } = useRegionsQuery();
const { control } = useFormContext();
return (
<Controller
render={({ field, fieldState }) => (
<RegionSelect
errorText={
fieldState.error?.message ??
(isError ? 'Failed to fetch Region.' : '')
}
onChange={(_, value) => {
field.onChange(value?.id);
}}
currentCapability={undefined}
disableClearable={false}
mjac0bs marked this conversation as resolved.
Show resolved Hide resolved
fullWidth
label="Region"
loading={isLoading}
placeholder="Select a Region"
regions={regions ?? []}
textFieldProps={{ onBlur: field.onBlur }}
value={field.value}
/>
)}
control={control}
name={name}
/>
);
};
Loading