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-21694] - Added Resource Select component to the Create-alert form #11331

Conversation

santoshp210-akamai
Copy link
Contributor

Description 📝

  • Added the Resource Multi Select component under General Information section to the Create alert form.

Changes 🔄

  • Added a MultiSelect Autocomplete component for selecting Resources
  • Added Unit Test for the Resource Multi Select component
  • Changed few variables and properties to camelCase

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
Screenshot 2024-11-19 at 21 38 14 Screenshot 2024-11-26 at 18 43 55

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 label of Resources component should change to Clusters if Service is Database

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

@santoshp210-akamai santoshp210-akamai requested a review from a team as a code owner November 26, 2024 13:18
@santoshp210-akamai santoshp210-akamai requested review from coliu-akamai and pmakode-akamai and removed request for a team November 26, 2024 13:18
@santoshp210-akamai santoshp210-akamai self-assigned this Nov 26, 2024
'severity',
]);
// severity has a need for null in the form for edge-cases, so null-checking and returning it as an appropriate type
const severity = formValues.severity!;
return { ...values, severity };
const resourceIds = formValues.resource_ids!;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding resourceIds here as resource_ids are optional as per the API-spec. Using FieldPathByValue on an optional property makes the name prop a never type in the ResourceMultiSelect component. cc: @bnussman-akamai , @jaalah-akamai

@santoshp210-akamai
Copy link
Contributor Author

As discussed with @jaalah-akamai , The properties that use snake_case right now are the the ones used for the API so haven't changed them to camelCase.

Copy link

github-actions bot commented Nov 26, 2024

Coverage Report:
Base Coverage: 86.93%
Current Coverage: 86.93%

Comment on lines 69 to 76
textFieldProps={{
InputProps: {
sx: {
maxHeight: '60px',
overflow: 'auto',
},
},
}}
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 did this for the dashboard filters since that design/layout was a little different. For the alerts, let's omit this and let it grow naturally. I've spoke with UX and we're in the process of coming up with a standard of how to address this, but the overflow UX is really poor and I don't think it's something we should be propagating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, removed that styling.

…sourceMultiSelect and modified and added few properties for the Alert interfaces as per the latest API-spec
Comment on lines +53 to +55
React.useEffect(() => {
setValue(name, []);
}, [region, serviceType, engine, setValue, name]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
React.useEffect(() => {
setValue(name, []);
}, [region, serviceType, engine, setValue, name]);

Copy link
Contributor

@jaalah-akamai jaalah-akamai Nov 27, 2024

Choose a reason for hiding this comment

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

After discussing, it seems this is an actual side effect. We want the resources to reset if region, serviceType, or engine changes. cc: @bnussman-akamai thoughts?

We can keep this, this will eventually get replaced with an entirely different component once that work is done - we can revisit then

…dynamic in the Unit Test, replaced the variable name from serviceWatcher to serviceTypeWatcher, added comment addressing need for useEffect()
@jaalah-akamai jaalah-akamai added Add'tl Approval Needed Waiting on another approval! and removed Requires Changes labels Nov 27, 2024
/>
),
});
expect(getByLabelText('Cluster'));
Copy link
Contributor

@coliu-akamai coliu-akamai Nov 27, 2024

Choose a reason for hiding this comment

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

will need to update this test depending on if label is changed to Clusters ^

and a small nitpick: if this test does need to be updated, could we also put a linebreak between each test while at it for better readability?

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops looks like I submitted this comment early - meant to include it as part of my below review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah @coliu-akamai , Clusters makes sense. I will make the changes and I'll add a linebreak too. Thank you for the approval

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for the additional test coverage!

The label of Resources component should change to Clusters if Service is Database

should it be Cluster or Clusters?
image

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 27, 2024
…l to Clusters in case of Database service type
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 455 passing tests on test run #5 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing455 Passing2 Skipped96m 47s

@jaalah-akamai jaalah-akamai merged commit 0285c7a into linode:develop Nov 27, 2024
21 of 23 checks passed
Copy link

cypress bot commented Nov 27, 2024

Cloud Manager E2E    Run #6896

Run Properties:  status check failed Failed #6896  •  git commit 0285c7a67f: upcoming: [DI-21694] - Added Resource Select component to the Create-alert form ...
Project Cloud Manager E2E
Branch Review develop
Run status status check failed Failed #6896
Run duration 35m 01s
Commit git commit 0285c7a67f: upcoming: [DI-21694] - Added Resource Select component to the Create-alert form ...
Committer santoshp210-akamai
View all properties for this run ↗︎

Test results
Tests that failed  Failures 3
Tests that were flaky  Flaky 6
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 459
View all changes introduced in this branch ↗︎

Tests for review

Failed  switch-linode-state.spec.ts • 1 failed test

View Output Video

Test Artifacts
switch linode state > reboots a linode from landing page Screenshots Video
Failed  update-linode-labels.spec.ts • 1 failed test

View Output Video

Test Artifacts
update linode label > updates a linode label from details page Screenshots Video
Failed  search-linodes.spec.ts • 1 failed test

View Output Video

Test Artifacts
Search Linodes > create a linode and make sure it shows up in the table and is searchable in main search tool Screenshots Video
Flakiness  linodes/switch-linode-state.spec.ts • 1 flaky test

View Output Video

Test Artifacts
switch linode state > reboots a linode from details page Screenshots Video
Flakiness  linodes/clone-linode.spec.ts • 1 flaky test

View Output Video

Test Artifacts
clone linode > can clone a Linode from Linode details page Screenshots Video
Flakiness  linodes/update-linode-labels.spec.ts • 1 flaky test

View Output Video

Test Artifacts
update linode label > updates a linode label from the "Settings" tab Screenshots Video
Flakiness  linodes/backup-linode.spec.ts • 1 flaky test

View Output Video

Test Artifacts
linode backups > can enable backups Screenshots Video
Flakiness  kubernetes/lke-create.spec.ts • 1 flaky test

View Output Video

Test Artifacts
LKE Cluster Creation > can create an LKE cluster Screenshots Video

The first 5 flaky specs are shown, see all 6 specs in Cypress Cloud.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Cloud Pulse - Alerting Cloud Pulse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants