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

santoshp210-akamai · 2024-11-19T16:11:21Z

Description 📝

Added Service, Engine Option, Region Select components to the Create Alert form under General Information.
Added Unit tests for all three components

Changes 🔄

Added Autocomplete components for service type, engine option, region selection for the Create Alert form
Added Unit tests for the components
Modified the Alert Create POST query to include service_type as a parameter of the request
Minor changes to validation schema to improve the error messages
Minor changes to types and interfaces

Target release date 🗓️

Please specify a release date (and environment, if applicable) to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed.

Preview 📷

Before	After

How to test 🧪

Prerequisites

(How to setup test environment)

The tabs are controlled by a featureFlag called aclpAlerting, the flag has been currently disabled. For testing enable the definitions part of the aclpAlerting flag to be true.
The API endpoints are not live yet, so use Legacy MSW Handlers for the mock responses.

Verification steps

(How to verify changes)

In Monitor (once the prerequisites are followed) , Alerts tab should be visible next to Dashboards.
Under that, Definition tab and a Create button should be visible.
On clicking Create, the Form Page should be visible.
The Engine Option component is designed to be visible only when the Service type is Databases, so selecting Databases in Service renders the Engine Option component

As an Author, I have considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support

I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

All unit tests are passing
TypeScript compilation succeeded without errors
Code passes all linting rules

…nt to the Create Alert form and made necessary changes

…ve Memo from RegionSelect component

santoshp210-akamai · 2024-11-19T16:18:23Z

packages/api-v4/src/cloudpulse/types.ts

@@ -158,8 +158,8 @@ export interface CreateAlertDefinitionPayload {
 export interface CreateAlertDefinitionForm
  extends CreateAlertDefinitionPayload {
  region: string;
-  service_type: string;
-  engine_type: string;
+  service_type: string | null;


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

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

santoshp210-akamai · 2024-11-19T16:19:36Z

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/utilities.ts

+  CreateAlertDefinitionPayload,
+} from '@linode/api-v4';
+
+export const getCreateAlertPayload = (


Omitting the props here as we require these attributes for the form but are not part of the payload for the POST request

This name caused me to pause as I reading it in CreateAlertDefinition, I would say we should rename this to include the word filter or omit since that better communicates that the function's purpose is to remove specific fields rather than just "getting" something.

… the changes in error messages

github-actions · 2024-11-19T16:44:29Z

Coverage Report: ✅
Base Coverage: 86.72%
Current Coverage: 86.72%

mjac0bs

Thanks @santoshp210-akamai. Verified the correct form fields appear depending on service type and validation is present.

Left a question about the mocked form submission and a first pass of general feedback.

packages/manager/.changeset/pr-11286-added-1732032870588.md

mjac0bs · 2024-11-19T20:52:28Z

packages/manager/src/factories/cloudpulse/alerts.ts

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)

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

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.

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 ?

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

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

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', });

.../manager/src/features/CloudPulse/Alerts/CreateAlert/GeneralInformation/EngineOption.test.tsx

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/GeneralInformation/RegionSelect.tsx

packages/validation/.changeset/pr-11286-added-1732032964808.md

packages/manager/src/mocks/serverHandlers.ts

mjac0bs · 2024-11-19T21:34:44Z

packages/manager/src/mocks/serverHandlers.ts

-    };
-    return HttpResponse.json(response);
-  }),
+  http.post(


Since we have a mock request here... should we see this firing when the Submit button is clicked? I was not seeing any request in the network tab.

@mjac0bs , The request won't fire as the validation schema is written for all the fields that are there in the entire form. Those components will be added in further PR's.

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/CreateAlertDefinition.tsx

jaalah-akamai · 2024-11-20T01:24:06Z

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/utilities.ts

+  CreateAlertDefinitionPayload,
+} from '@linode/api-v4';
+
+export const getCreateAlertPayload = (


This name caused me to pause as I reading it in CreateAlertDefinition, I would say we should rename this to include the word filter or omit since that better communicates that the function's purpose is to remove specific fields rather than just "getting" something.

jaalah-akamai · 2024-11-20T01:54:25Z

packages/api-v4/src/cloudpulse/types.ts

@@ -158,8 +158,8 @@ export interface CreateAlertDefinitionPayload {
 export interface CreateAlertDefinitionForm
  extends CreateAlertDefinitionPayload {
  region: string;
-  service_type: string;
-  engine_type: string;
+  service_type: string | null;


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

jaalah-akamai · 2024-11-20T02:17:59Z

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/CreateAlertDefinition.tsx

  const { enqueueSnackbar } = useSnackbar();
-  const { mutateAsync: createAlert } = useCreateAlertDefinition();
+  const { mutateAsync: createAlert } = useCreateAlertDefinition(
+    getValues('service_type')!


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?

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

@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.

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

@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.

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

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

manager/packages/manager/src/features/Linodes/LinodeCreate/schemas.ts

Lines 18 to 22 in d8812c4

export const CreateLinodeFromStackScriptSchema = CreateLinodeSchema.concat(

object({

stackscript_id: number().required('You must select a StackScript.'),

})

);

…Payload to filterFormValues and replaced the type of service_typeto a union type of relevant services instead of string

bnussman-akamai · 2024-11-20T16:26:13Z

packages/api-v4/src/cloudpulse/types.ts

@@ -158,8 +159,8 @@ export interface CreateAlertDefinitionPayload {
 export interface CreateAlertDefinitionForm
  extends CreateAlertDefinitionPayload {
  region: string;
-  service_type: string;
-  engine_type: string;
+  service_type: AlertServiceType | null;


It looks like service_type is in the path and not the request body. Should this be removed?

Suggested change

service_type: AlertServiceType | null;

From my understanding this is just for the form itself, it's getting stripped before being sent as part of the request

@jaalah-akamai , Yes that's right.

bnussman-akamai · 2024-11-20T16:27:39Z

packages/api-v4/src/cloudpulse/types.ts

-  service_type: string;
-  engine_type: string;
+  service_type: AlertServiceType | null;
+  engine_type: string | null;


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.

@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.

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.

@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 ?

…eparated the interface from api-v4/types.ts for the non payload fields

jaalah-akamai

Thanks, this is looking much better and I think adding a schema in manager will make this much more organized with how much form work you have to do. 👍

bnussman-akamai · 2024-11-21T15:21:04Z

packages/api-v4/src/cloudpulse/types.ts

@@ -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;


Why null? Seems like the should not be nullable

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.

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

bnussman-akamai · 2024-11-21T15:22:33Z

packages/api-v4/src/cloudpulse/alerts.ts

-export const createAlertDefinition = (data: CreateAlertDefinitionPayload) =>
+export const createAlertDefinition = (
+  data: CreateAlertDefinitionPayload,
+  service_type: string


Optional: Maybe we could use AlertServiceType here.

bnussman-akamai · 2024-11-21T15:26:35Z

packages/manager/src/features/CloudPulse/Alerts/CreateAlert/CreateAlertDefinition.tsx

  const { enqueueSnackbar } = useSnackbar();
-  const { mutateAsync: createAlert } = useCreateAlertDefinition();
+  const { mutateAsync: createAlert } = useCreateAlertDefinition(
+    getValues('service_type')!


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

manager/packages/manager/src/features/Linodes/LinodeCreate/schemas.ts

Lines 18 to 22 in d8812c4

export const CreateLinodeFromStackScriptSchema = CreateLinodeSchema.concat(

object({

stackscript_id: number().required('You must select a StackScript.'),

})

);

mjac0bs

Approving with my initial feedback addressed, pending Banks' last round of feedback is also addressed.

mjac0bs · 2024-11-21T23:47:01Z

packages/api-v4/.changeset/pr-11286-changed-1732032917339.md

+"@linode/api-v4": Changed
+---
+
+Added service_type as parameter for the Create Alert POST request ([#11286](https://github.com/linode/manager/pull/11286))


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

santoshp210-akamai · 2024-11-25T04:44:27Z

@bnussman-akamai , we are extending in a way that's similar to linode validation, I'm not sure what the changes you're requesting here #11286 (comment).

… from string to AlertServiceType wherever applicable, moved randomness to the serverHandler from the alert factory

santoshp210-akamai · 2024-11-25T09:36:28Z

@jaalah-akamai , Thank you for updating the docs with Form Validation Best Practices, examples and references. We are now following the same way to extend additional validation from form validation for our use-cases.

linode-gh-bot · 2024-11-25T10:18:41Z

Cloud Manager UI test results

🔺 4 failing tests on test run #10 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`4 Failing`	`451 Passing`	`2 Skipped`	`114m 30s`

Details

Failing Tests
	Spec	Test
❌	`machine-image-upload.spec.ts`	machine image » uploads machine image, mock finish event
❌	`machine-image-upload.spec.ts`	machine image » uploads machine image, mock upload canceled failed event
❌	`machine-image-upload.spec.ts`	machine image » uploads machine image, mock failed to decompress failed event
❌	`machine-image-upload.spec.ts`	machine image » uploads machine image, mock expired upload event

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/images/machine-image-upload.spec.ts,cypress/e2e/core/images/machine-image-upload.spec.ts,cypress/e2e/core/images/machine-image-upload.spec.ts,cypress/e2e/core/images/machine-image-upload.spec.ts"

santoshp210-akamai added 2 commits November 19, 2024 20:09

upcoming: [DI: 21998] - Added Severity, Region, Engine Option compone…

4617bfb

…nt to the Create Alert form and made necessary changes

upcoming: [DI:21998] - Fixed the ServiceTypeSelect test file and remo…

c2b296e

…ve Memo from RegionSelect component

santoshp210-akamai requested a review from a team as a code owner November 19, 2024 16:11

santoshp210-akamai requested review from mjac0bs and hkhalil-akamai and removed request for a team November 19, 2024 16:11

santoshp210-akamai self-assigned this Nov 19, 2024

upcoming: [DI:21998] - Added changesets

a3e9178

santoshp210-akamai commented Nov 19, 2024

View reviewed changes

mjac0bs requested a review from jaalah-akamai November 19, 2024 16:19

mjac0bs added the CloudPulse label Nov 19, 2024

santoshp210-akamai added 2 commits November 19, 2024 21:51

upcoming: [DI:21998] - Fixed a typo

90b860a

upcoming: [DI:21998] - Modified the Create Alert test file to reflect…

c2fbd5a

… the changes in error messages

mjac0bs reviewed Nov 19, 2024

View reviewed changes

upcoming: [DI-21998] - Addressed the review comments

04e528b

jaalah-akamai reviewed Nov 20, 2024

View reviewed changes

upcoming: [DI-21998] - Changed the name of the utility getCreateAlert…

2533236

…Payload to filterFormValues and replaced the type of service_typeto a union type of relevant services instead of string

bnussman-akamai reviewed Nov 20, 2024

View reviewed changes

santoshp210-akamai added 2 commits November 21, 2024 00:13

upcoming: [DI-21998] - Fixed the type in alerts.ts and removed Random

b91a446

upcoming: [DI-21998] - Extended the createAlertDefinitionSchema and s…

39f2a18

…eparated the interface from api-v4/types.ts for the non payload fields

jaalah-akamai approved these changes Nov 21, 2024

View reviewed changes

bnussman-akamai reviewed Nov 21, 2024

View reviewed changes

mjac0bs approved these changes Nov 21, 2024

View reviewed changes

santoshp210-akamai added 2 commits November 25, 2024 14:29

upcoming: [DI-21998] - Review changes: changeset naming, changed type…

8dfa25d

… from string to AlertServiceType wherever applicable, moved randomness to the serverHandler from the alert factory

Merge commit: Latest linode/develop changes

86960c1

bnussman-akamai approved these changes Nov 25, 2024

View reviewed changes

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

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

Are you sure you want to change the base?

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

Conversation

santoshp210-akamai commented Nov 19, 2024

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author, I have considered 🤔

As an Author, before moving this PR from Draft to Open, I confirmed ✅

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024 • edited Loading

mjac0bs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santoshp210-akamai Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaalah-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

mjac0bs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santoshp210-akamai commented Nov 25, 2024 • edited Loading

santoshp210-akamai commented Nov 25, 2024

linode-gh-bot commented Nov 25, 2024

Cloud Manager UI test results

🔺 4 failing tests on test run #10 ↗︎

Details

Troubleshooting

github-actions bot commented Nov 19, 2024 •

edited

Loading

bnussman-akamai Nov 20, 2024 •

edited

Loading

santoshp210-akamai Nov 20, 2024 •

edited

Loading

bnussman-akamai Nov 21, 2024 •

edited

Loading

bnussman-akamai Nov 21, 2024 •

edited

Loading

santoshp210-akamai commented Nov 25, 2024 •

edited

Loading