upcoming: [DI-22217] - Added the Alert Listing page with Alerting Table and added relevant api endpoints #11346

santoshp210-akamai · 2024-12-02T13:10:02Z

Description 📝

Added the Alert Listing component
Added the AlertTableRow component

Changes 🔄

List any change(s) relevant to the reviewer.

Added the AlertListing component with UT
Added the AlertTableRow component with UT
Added GET request to fetch the alert definitions
Changed the AlertDefinitionType to 'system'|'user'
Added the AlertIcon svg for the Empty State for AlertListing component

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

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.
This PR only contains the Table, the ordering, pagination will part of next PR.
The Handlers for the rows in the Table are not yet part of this PR. It is dependent on another PR so it will part of that.

Verification steps

(How to verify changes)

The page to /monitor/alerts/definitions is the AlertListing component
There is a TableRowError component if the API returns an error
There is a TableRowLoading component if the API is still loading

Author Checklists

As an Author, to speed up the review process, I 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

…s, Table rows and added the GET api endpoint and mockServer for fetching alert definitions

…or the AlertListing Table and minor changes

github-actions · 2024-12-02T13:45:45Z

Coverage Report: ✅
Base Coverage: 86.95%
Current Coverage: 86.96%

santoshp210-akamai · 2024-12-02T14:30:36Z

There are still on-going discussions with respect to the columns of the table in the Mockups with UX team, These are the currently agreed upon changes, there's a possibility of couple of columns re ordered or removed.
cc: @jaalah-akamai

jdamore-linode · 2024-12-02T14:36:40Z

packages/manager/.changeset/pr-11346-added-1733145106911.md

+"@linode/manager": Added
+---
+
+AlertListing component and AlertTableRow component with UT ([#11346](https://github.com/linode/manager/pull/11346))


Can we avoid abbreviations like "UT" in the changelogs? Just so each entry is easier to skim if we're trying to find a particular change while troubleshooting a bug, test failure, etc.

Sure, we will follow that from now. I will change this as well.

jdamore-linode · 2024-12-02T14:40:06Z

packages/manager/src/features/CloudPulse/Alerts/AlertsListing/AlertListing.test.tsx

+  it('should render the alert landing table with items', async () => {
+    queryMocks.useAllAlertDefinitionsQuery.mockReturnValue({
+      data: mockResponse,
+      isError: false,
+      isLoading: false,
+      status: 'success',
+    });
+    const { getAllByText } = renderWithTheme(<AlertListing />);
+    getAllByText('Alert Name');
+    getAllByText('Service Type');
+    getAllByText('Severity');
+    getAllByText('Status');
+    getAllByText('Last Modified');
+    getAllByText('Created By');
+  });


Can we improve this test so that it checks that the actual mockResponse data is displayed on the page? Confirming that the column titles are present doesn't get us very far.

I have covered that checks in the AlertTableRow component Unit Tests. So omitted them here. Should I check it here again ?

I think we should at least check that each expected row is present (and that might just involve confirming that the label for each object in the mockResponse array is present, but you could approach it multiple ways e.g. using and checking a data-test-id attribute on each table row or something like that).

The table row tests (which are good!) confirm that an individual table row renders correctly with the right data given some individual Alert object, but for this test we want to know that AlertListing renders an AlertTableRow for each Alert object in the alerts array.

For example (albeit a bit contrived), if there were ever some logic bug where the AlertTableRows weren't being rendered at all, this test wouldn't catch it. Feel free to reach out on Slack or continue the discussion here if I can answer any questions!

Yeah, I got it. I will add another test case to check for the label of each object.

Thanks @santoshp210-akamai!

…ses, unabbreviated UT to Unit Tests in changeset

linode-gh-bot · 2024-12-03T06:23:18Z

Cloud Manager UI test results

🔺 2 failing tests on test run #4 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`2 Failing`	`460 Passing`	`2 Skipped`	`112m 32s`

Details

Failing Tests
	Spec	Test
❌	`create-linode-from-image.spec.ts`	create linode from image, mocked data » creates linode from image on images tab
❌	`create-linode-from-image.spec.ts`	create linode from image, mocked data » creates linode from preselected image on images tab

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/images/create-linode-from-image.spec.ts,cypress/e2e/core/images/create-linode-from-image.spec.ts"

santoshp210-akamai · 2024-12-03T07:14:21Z

packages/manager/src/assets/icons/entityIcons/alert.svg

This file will be replaced before the PR is merged as the icon is not looking good on the UI. There are some confirmations from the UX side.

bnussman-akamai · 2024-12-03T18:43:13Z

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

 import { BETA_API_ROOT as API_ROOT } from 'src/constants';
+import { Params, Filter, ResourcePage } from 'src/types';


Relative imports should be used in all packages (ui, validation, api-v4) except manager. This is because only the manager package has import aliases setup.

Suggested change

import { BETA_API_ROOT as API_ROOT } from 'src/constants';

import { Params, Filter, ResourcePage } from 'src/types';

import { BETA_API_ROOT as API_ROOT } from '../constants';

import { Params, Filter, ResourcePage } from '../types';

bnussman-akamai · 2024-12-03T18:44:26Z

packages/manager/src/features/CloudPulse/Alerts/AlertsLanding/AlertsDefinitionLanding.tsx

 import { CreateAlertDefinition } from '../CreateAlert/CreateAlertDefinition';

 export const AlertDefinitionLanding = () => {
  return (
    <Switch>
      <Route
-        component={AlertDefinition}
+        component={() => <AlertListing />}


I think we can do this here:

Suggested change

component={() => <AlertListing />}

component={AlertListing}

bnussman-akamai · 2024-12-03T18:48:40Z

packages/manager/src/features/CloudPulse/Alerts/AlertsListing/AlertActionMenu.tsx

+/*
+The handlers and alertType are made optional only temporarily, they will be enabled but they are dependent on another feature which will be part of next PR
+*/


For future reference: Currently, this shows up like this:

If you format your comment like this:

Suggested change

/*

The handlers and alertType are made optional only temporarily, they will be enabled but they are dependent on another feature which will be part of next PR

*/

/**

* The handlers and alertType are made optional only temporarily, they will be enabled but they are dependent on another feature which will be part of next PR

*/

Code editors will be able to display the comment in a more readable way

bnussman-akamai · 2024-12-03T18:49:21Z

packages/manager/src/features/CloudPulse/Alerts/AlertsListing/AlertActionMenu.tsx

+}
+
+export interface AlertActionMenuProps {
+  /*


Suggested change

/*

/**

Refer to other comment about code comments

bnussman-akamai · 2024-12-03T18:52:36Z

packages/manager/src/features/CloudPulse/Alerts/AlertsListing/AlertListing.test.tsx

+    await waitFor(() => expect(getByText('Alert Name')).toBeInTheDocument());
+    await waitFor(() => expect(getByText('Service Type')).toBeInTheDocument());
+    await waitFor(() => expect(getByText('Status')).toBeInTheDocument());
+    await waitFor(() => expect(getByText('Last Modified')).toBeInTheDocument());
+    await waitFor(() => expect(getByText('Created By')).toBeInTheDocument());


Because you are using static mocking to mock the return values of useAllAlertDefinitionsQuery, you don't need to await anything

Suggested change

await waitFor(() => expect(getByText('Alert Name')).toBeInTheDocument());

await waitFor(() => expect(getByText('Service Type')).toBeInTheDocument());

await waitFor(() => expect(getByText('Status')).toBeInTheDocument());

await waitFor(() => expect(getByText('Last Modified')).toBeInTheDocument());

await waitFor(() => expect(getByText('Created By')).toBeInTheDocument());

expect(getByText('Alert Name')).toBeInTheDocument();

expect(getByText('Service Type')).toBeInTheDocument();

expect(getByText('Status')).toBeInTheDocument();

expect(getByText('Last Modified')).toBeInTheDocument();

expect(getByText('Created By')).toBeInTheDocument();

bnussman-akamai · 2024-12-03T18:56:17Z

packages/manager/src/queries/cloudpulse/queries.ts

+  alert: {
+    // contextQueries: {
    // This query key is a placeholder , it will be updated once the relevant queries are added
    queryKey: null,
  },
+  alerts: {
+    contextQueries: {
+      all: (params: Params = {}, filter: Filter = {}) => ({
+        queryFn: () => getAllAlertsRequest(params, filter),
+        queryKey: [params, filter],
+      }),
+    },
+    queryKey: null,
+  },


Optional, but we could organize this a bit better by moving alert inside the context of alerts

Suggested change

alert: {

// contextQueries: {

// This query key is a placeholder , it will be updated once the relevant queries are added

queryKey: null,

},

alerts: {

contextQueries: {

all: (params: Params = {}, filter: Filter = {}) => ({

queryFn: () => getAllAlertsRequest(params, filter),

queryKey: [params, filter],

}),

},

queryKey: null,

},

alerts: {

contextQueries: {

alert: {

// contextQueries: {

// This query key is a placeholder , it will be updated once the relevant queries are added

queryKey: null,

},

all: (params: Params = {}, filter: Filter = {}) => ({

queryFn: () => getAllAlertsRequest(params, filter),

queryKey: [params, filter],

}),

},

queryKey: null,

},

bnussman-akamai · 2024-12-03T18:57:28Z

packages/manager/src/features/CloudPulse/Alerts/AlertsListing/AlertListing.tsx

+          </TableRow>
+        </TableHead>
+        <TableBody>
+          {isError === true && (


No need to put === true for a boolean value

Suggested change

{isError === true && (

{isError && (

bnussman-akamai · 2024-12-03T18:57:37Z

packages/manager/src/features/CloudPulse/Alerts/AlertsListing/AlertListing.tsx

+              message={'Error in fetching the alerts.'}
+            />
+          )}
+          {isLoading === true && <TableRowLoading columns={7} />}


Suggested change

{isLoading === true && <TableRowLoading columns={7} />}

{isLoading && <TableRowLoading columns={7} />}

bnussman-akamai · 2024-12-03T18:58:21Z

packages/manager/src/features/CloudPulse/Alerts/AlertsListing/AlertListing.tsx

+              handleClick={() => {
+                'asc';
+              }}


What is going on here?

bnussman-akamai · 2024-12-03T19:00:21Z

packages/manager/src/features/CloudPulse/Alerts/AlertsListing/AlertListing.tsx

+              active={true}
+              direction={order}
+              handleClick={handleOrderChange}
+              label="lastModified"


Options because it doesn't matter right now, but we usually we make these values align with the API response. So in this case, updated. This makes API ordering work easily when it's time to implement it if you are going that route.

Same goes for other table cells

santoshp210-akamai added 2 commits December 2, 2024 15:47

upcoming: [DI-22217] - Added AlertListing component With Table header…

875f278

…s, Table rows and added the GET api endpoint and mockServer for fetching alert definitions

upcoming : [DI-22217] - Added the TableRowError and TableRowLoading f…

65c0a6d

…or the AlertListing Table and minor changes

santoshp210-akamai requested a review from a team as a code owner December 2, 2024 13:10

santoshp210-akamai requested review from bnussman-akamai and coliu-akamai and removed request for a team December 2, 2024 13:10

upcoming: [DI-22217] - Added changesets

80389f5

santoshp210-akamai self-assigned this Dec 2, 2024

jaalah-akamai self-requested a review December 2, 2024 14:18

jaalah-akamai added CloudPulse Ready for Review labels Dec 2, 2024

jdamore-linode reviewed Dec 2, 2024

View reviewed changes

santoshp210-akamai added 2 commits December 2, 2024 22:32

upcoming: [DI-22217] - Review changes: Improving AlertListing test ca…

34acc0a

…ses, unabbreviated UT to Unit Tests in changeset

upcoming: [DI-22217] - Removed unused import

af30200

santoshp210-akamai commented Dec 3, 2024

View reviewed changes

bnussman-akamai requested changes Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [DI-22217] - Added the Alert Listing page with Alerting Table and added relevant api endpoints #11346

upcoming: [DI-22217] - Added the Alert Listing page with Alerting Table and added relevant api endpoints #11346

santoshp210-akamai commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited

Loading

santoshp210-akamai commented Dec 2, 2024

jdamore-linode Dec 2, 2024

santoshp210-akamai Dec 2, 2024

jdamore-linode Dec 2, 2024

santoshp210-akamai Dec 2, 2024 •

edited

Loading

jdamore-linode Dec 2, 2024

santoshp210-akamai Dec 2, 2024

jdamore-linode Dec 2, 2024

linode-gh-bot commented Dec 3, 2024

santoshp210-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

bnussman-akamai Dec 3, 2024

		import { BETA_API_ROOT as API_ROOT } from 'src/constants';
		import { Params, Filter, ResourcePage } from 'src/types';

	{isLoading === true && <TableRowLoading columns={7} />}
	{isLoading && <TableRowLoading columns={7} />}

upcoming: [DI-22217] - Added the Alert Listing page with Alerting Table and added relevant api endpoints #11346

Are you sure you want to change the base?

upcoming: [DI-22217] - Added the Alert Listing page with Alerting Table and added relevant api endpoints #11346

Conversation

santoshp210-akamai commented Dec 2, 2024 • edited Loading

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author, to speed up the review process, I considered 🤔

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

github-actions bot commented Dec 2, 2024 • edited Loading

santoshp210-akamai commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santoshp210-akamai Dec 2, 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

linode-gh-bot commented Dec 3, 2024

Cloud Manager UI test results

🔺 2 failing tests on test run #4 ↗︎

Details

Troubleshooting

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 commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited

Loading

santoshp210-akamai Dec 2, 2024 •

edited

Loading