-
Notifications
You must be signed in to change notification settings - Fork 366
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
Changes from 9 commits
4617bfb
c2b296e
a3e9178
90b860a
c2fbd5a
04e528b
2533236
b91a446
39f2a18
8dfa25d
86960c1
7d562ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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)) | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: Maybe we could use |
||
) => | ||
Request<Alert>( | ||
setURL(`${API_ROOT}/monitor/alert-definitions`), | ||
setURL( | ||
`${API_ROOT}/monitor/services/${encodeURIComponent( | ||
service_type | ||
)}/alert-definitions` | ||
), | ||
setMethod('POST'), | ||
setData(data, createAlertDefinitionSchema) | ||
); |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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; | ||
|
@@ -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; | ||
|
@@ -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[]; | ||
|
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)) |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've run into test issues in the past with Can we avoid random assignment here? (cc @jdamore-linode for visibility) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mjac0bs can we use Math.Random to pick out the values ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have pushed a commit to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jaalah-akamai , Not sure. Will try something out. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import Factory from 'src/factories/factoryProxy'; | ||
|
||
import type { | ||
Alert, | ||
AlertDefinitionType, | ||
AlertServiceType, | ||
AlertSeverityType, | ||
AlertStatusType, | ||
} from '@linode/api-v4'; | ||
|
||
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']; | ||
export const alertFactory = Factory.Sync.makeFactory<Alert>({ | ||
channels: [], | ||
created: new Date().toISOString(), | ||
created_by: Factory.each((i) => users[i % users.length]), | ||
description: '', | ||
id: Factory.each((i) => i), | ||
label: Factory.each((id) => `Alert-${id}`), | ||
resource_ids: ['0', '1', '2', '3'], | ||
rule_criteria: { | ||
rules: [], | ||
}, | ||
service_type: Factory.each((i) => serviceTypes[i % serviceTypes.length]), | ||
severity: Factory.each((i) => severity[i % severity.length]), | ||
status: Factory.each((i) => status[i % status.length]), | ||
triggerCondition: { | ||
evaluation_period_seconds: 0, | ||
polling_interval_seconds: 0, | ||
trigger_occurrences: 0, | ||
}, | ||
type: Factory.each((i) => types[i % types.length]), | ||
updated: new Date().toISOString(), | ||
updated_by: Factory.each((i) => users[(i + 3) % users.length]), | ||
}); |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,5 @@ | ||||||||||||
import { yupResolver } from '@hookform/resolvers/yup'; | ||||||||||||
import { Paper } 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'; | ||||||||||||
|
@@ -13,10 +12,14 @@ 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 { 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'; | ||||||||||||
|
@@ -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, | ||||||||||||
}; | ||||||||||||
|
@@ -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), | ||||||||||||
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')! | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. This is tough. Because the endpoint path includes the service type ( But... Why does the api-v4 and validation should always be aligned API not UI. If needed we can extend the validation schema inside of Cloud Manager There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bnussman-akamai , It is in the path. The reason why There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're using a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can extend a manager/packages/manager/src/features/Linodes/LinodeCreate/schemas.ts Lines 18 to 22 in d8812c4
|
||||||||||||
); | ||||||||||||
|
||||||||||||
const serviceWatcher = watch('service_type'); | ||||||||||||
const onSubmit = handleSubmit(async (values) => { | ||||||||||||
try { | ||||||||||||
await createAlert(values); | ||||||||||||
await createAlert(filterFormValues(values)); | ||||||||||||
enqueueSnackbar('Alert successfully created', { | ||||||||||||
variant: 'success', | ||||||||||||
}); | ||||||||||||
|
@@ -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={{ | ||||||||||||
|
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} | ||
/> | ||
); | ||
}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it