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-23105] - Listing Alerts features: Pagination, Ordering, Searching, Filtering #11577

Conversation

santoshp210-akamai
Copy link
Contributor

Description 📝

Added the following features to the Alerts Listing component

  • Pagination
  • Ordering
  • Searching (Alert label)
  • Filtering (Service type, Status)

Changes 🔄

  • Moved the table to AlertListTable from AlertListing component
  • Implemented filtering, searching logic in the AlertListing component
  • Unit Tests for all the Components
  • Moved the Create Alert button from the AlertsLanding component to AlertListing component as per latest UX mockups
  • Minor changes in serverHandler.ts, constants.ts files relevant for functionalities

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 📷

Include a screenshot or screen recording of the change.

🔒 Use the Mask Sensitive Data setting for security.

💡 Use <video src="" /> tag when including recordings in table.

Before After
image image

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.
  • In the Monitor menu, Click on the Alerts tab next to Monitor.

Verification steps

  • The ordering of the Table Columns
  • The pagination of the Table component
  • The filtering of the Alerts with respect to Service Type and Status (individually and combination of both filters)
  • The searching of Alerts with Alert Name(label) working in combination of filters being applied
  • The placement of the Create Alert button and it's functionality
  • Zero-th state Landing Page for the Alerts
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

@santoshp210-akamai santoshp210-akamai requested a review from a team as a code owner January 28, 2025 16:12
@santoshp210-akamai santoshp210-akamai requested review from dwiley-akamai and hasyed-akamai and removed request for a team January 28, 2025 16:12
@santoshp210-akamai santoshp210-akamai self-assigned this Jan 28, 2025
@santoshp210-akamai
Copy link
Contributor Author

This PR introduces changes related to the AlertListing functionality. While the diff exceeds 500 lines due to the scope of the changes, all the updates are interconnected and part of the same feature set , so it made more sense to raise it as a single PR. Hope this is fine and works for the review process. cc: @jaalah-akamai

Copy link

github-actions bot commented Jan 28, 2025

Coverage Report:
Base Coverage: 79.3%
Current Coverage: 79.3%

Comment on lines 31 to 33
await waitFor(() => {
expect(getByText('Error in fetching the alerts')).toBeVisible();
});
Copy link
Member

Choose a reason for hiding this comment

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

I don't think await waitFor(...) is needed. There is nothing async happening in the component to require this

Suggested change
await waitFor(() => {
expect(getByText('Error in fetching the alerts')).toBeVisible();
});
expect(getByText('Error in fetching the alerts')).toBeVisible();

Comment on lines +64 to +66
{formatDate(updated, {
format: 'MMM dd, yyyy, h:mm a',
})}
Copy link
Member

Choose a reason for hiding this comment

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

Should timezone be handled here?

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'm not exactly sure of this comment, do we need to handle it explicitly ? Currently compared to the time we have in mocks and the time showed in the Table the timezone is being handled properly.

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Code review ✅
Ordering of table columns ✅
Pagination ✅
Filtering and searching ✅
Error state when alert-definitions request fails & landing page when there are no definitions ✅

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 1 failing test on test run #4 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
1 Failing487 Passing2 Skipped97m 38s

Details

Failing Tests
SpecTest
linode-config.spec.tsEnd-to-End » Clones a config

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/linode-config.spec.ts"

@santoshp210-akamai
Copy link
Contributor Author

@bnussman-akamai , can you merge the PR.

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Jan 31, 2025
@jaalah-akamai jaalah-akamai merged commit 7511a29 into linode:develop Jan 31, 2025
22 of 23 checks passed
Copy link

cypress bot commented Jan 31, 2025

Cloud Manager E2E    Run #7159

Run Properties:  status check passed Passed #7159  •  git commit 7511a296fd: upcoming: [DI-23105] - Listing Alerts features: Pagination, Ordering, Searching,...
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #7159
Run duration 31m 01s
Commit git commit 7511a296fd: upcoming: [DI-23105] - Listing Alerts features: Pagination, Ordering, Searching,...
Committer santoshp210-akamai
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
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 494
View all changes introduced in this branch ↗︎

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