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-23081] - Add filtering, pagination and sorting for resources section in CloudPulse alerts show details page #11541

Conversation

venkymano-akamai
Copy link
Contributor

@venkymano-akamai venkymano-akamai commented Jan 21, 2025

Description 📝

Add filtering, pagination and sorting for Alert Resource section in CloudPulse show details page

Changes 🔄

  1. Added data and filtering logic for resources section in alerts show details page
  2. Added pagination, sorting for resources section in alerts show details page

Target release date 🗓️

25-01-2025

Preview 📷

Before After
Screenshot 2025-01-21 at 9 56 56 PM Screenshot 2025-01-21 at 9 57 13 PM
Screenshot 2025-01-21 at 9 57 31 PM Screenshot 2025-01-21 at 9 57 41 PM
Screenshot 2025-01-21 at 9 57 49 PM Screenshot 2025-01-21 at 9 58 12 PM
Screenshot 2025-01-21 at 9 58 29 PM Screenshot 2025-01-21 at 9 58 41 PM

How to test 🧪

  1. Login as mock, as some of the end points are not available
  2. Navigate to monitor tab and navigate to alerts section
  3. On any of row, click on action menu item show details.
  4. You will be able to see resource section with data and functionality.
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

@venkymano-akamai
Copy link
Contributor Author

This PR is still in progress, Once everything is set will be opening it for review

@venkymano-akamai venkymano-akamai changed the title upcoming: [DI-22838] - Add filtering, pagination and sorting for Alert Resource section in CloudPulse show details page upcoming: [DI-22838] - Add filtering, pagination and sorting for resources section in CloudPulse alerts show details page Jan 21, 2025
@venkymano-akamai venkymano-akamai changed the title upcoming: [DI-22838] - Add filtering, pagination and sorting for resources section in CloudPulse alerts show details page upcoming: [DI-23081] - Add filtering, pagination and sorting for resources section in CloudPulse alerts show details page Jan 22, 2025
@venkymano-akamai venkymano-akamai marked this pull request as ready for review January 23, 2025 10:47
@venkymano-akamai venkymano-akamai requested a review from a team as a code owner January 23, 2025 10:47
@venkymano-akamai venkymano-akamai requested review from hkhalil-akamai and pmakode-akamai and removed request for a team January 23, 2025 10:47
Copy link

github-actions bot commented Jan 23, 2025

Coverage Report:
Base Coverage: 86.69%
Current Coverage: 86.73%

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 474 passing tests on test run #20 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing474 Passing2 Skipped106m 28s

@venkymano-akamai
Copy link
Contributor Author

venkymano-akamai commented Jan 24, 2025

Here OrderBy is not directly used for sorting inside the table, but we have taken sortData function from OrderBy component and reused the same.

Also we have not used usePagination hook as well.

  • We don't need URL history being updated since we are in details context, and our page has multiple sections Overview, criteria, resources and notification channels (upcoming)
  • We also don't need preferences in this context.

Also we have implemented, on any Page size change or sorting change, we move to first page and also we need to scroll to top till Resources section.

@jaalah-akamai , this is done based on our discussion in the sync up.

Thanks.

@venkymano-akamai
Copy link
Contributor Author

@hkhalil-akamai / @pmakode-akamai , This PR is now ready for review

Copy link
Contributor

@pmakode-akamai pmakode-akamai left a comment

Choose a reason for hiding this comment

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

Looks good to me! Left a couple of minor comments

✅ Data & Filtering
✅ Pagination & Sorting
✅ All tests pass

thank you!

Comment on lines +131 to +175
it('should handle sorting correctly', async () => {
const { getByTestId } = renderWithTheme(
<AlertResources alertResourceIds={['1', '2', '3']} serviceType="linode" />
);
const resourceColumn = getByTestId('resource'); // get the resource header column
await userEvent.click(resourceColumn);

const tableBody = getByTestId('alert_resources_content');
let rows = Array.from(tableBody.querySelectorAll('tr'));
expect(
rows
.map(({ textContent }) => textContent)
.every((text, index) => {
return text?.includes(linodes[linodes.length - 1 - index].label);
})
).toBe(true);

await userEvent.click(resourceColumn); // again reverse the sorting
rows = Array.from(tableBody.querySelectorAll('tr'));
expect(
rows
.map(({ textContent }) => textContent)
.every((text, index) => text?.includes(linodes[index].label))
).toBe(true);

const regionColumn = getByTestId('region'); // get the region header column

await userEvent.click(regionColumn); // sort ascending for region
rows = Array.from(tableBody.querySelectorAll('tr')); // refetch
expect(
rows
.map(({ textContent }) => textContent)
.every((text, index) =>
text?.includes(linodes[linodes.length - 1 - index].region)
)
).toBe(true);

await userEvent.click(regionColumn); // reverse the sorting
rows = Array.from(tableBody.querySelectorAll('tr'));
expect(
rows
.map(({ textContent }) => textContent)
.every((text, index) => text?.includes(linodes[index].region)) // validation
).toBe(true);
});
Copy link
Contributor

@pmakode-akamai pmakode-akamai Jan 24, 2025

Choose a reason for hiding this comment

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

Suggested change
it('should handle sorting correctly', async () => {
const { getByTestId } = renderWithTheme(
<AlertResources alertResourceIds={['1', '2', '3']} serviceType="linode" />
);
const resourceColumn = getByTestId('resource'); // get the resource header column
await userEvent.click(resourceColumn);
const tableBody = getByTestId('alert_resources_content');
let rows = Array.from(tableBody.querySelectorAll('tr'));
expect(
rows
.map(({ textContent }) => textContent)
.every((text, index) => {
return text?.includes(linodes[linodes.length - 1 - index].label);
})
).toBe(true);
await userEvent.click(resourceColumn); // again reverse the sorting
rows = Array.from(tableBody.querySelectorAll('tr'));
expect(
rows
.map(({ textContent }) => textContent)
.every((text, index) => text?.includes(linodes[index].label))
).toBe(true);
const regionColumn = getByTestId('region'); // get the region header column
await userEvent.click(regionColumn); // sort ascending for region
rows = Array.from(tableBody.querySelectorAll('tr')); // refetch
expect(
rows
.map(({ textContent }) => textContent)
.every((text, index) =>
text?.includes(linodes[linodes.length - 1 - index].region)
)
).toBe(true);
await userEvent.click(regionColumn); // reverse the sorting
rows = Array.from(tableBody.querySelectorAll('tr'));
expect(
rows
.map(({ textContent }) => textContent)
.every((text, index) => text?.includes(linodes[index].region)) // validation
).toBe(true);
});
it('should handle sorting correctly', () => {
const { getByTestId } = renderWithTheme(
<AlertResources alertResourceIds={['1', '2', '3']} serviceType="linode" />
);
const resourceColumn = getByTestId('resource'); // Get the resource header column
const regionColumn = getByTestId('region'); // Get the region header column
const tableBody = getByTestId('alert_resources_content');
const getRows = () => Array.from(tableBody.querySelectorAll('tr'));
const checkSorting = async (
column: HTMLElement,
dataKey: 'label' | 'region'
) => {
// Test descending order
await userEvent.click(column);
let rows = getRows();
expect(
rows.every((row, index) =>
row.textContent?.includes(
linodes[linodes.length - 1 - index][dataKey]
)
)
).toBe(true);
// Test ascending order
await userEvent.click(column);
rows = getRows();
expect(
rows.every((row, index) =>
row.textContent?.includes(linodes[index][dataKey])
)
).toBe(true);
};
// Test sorting by 'resource' column
checkSorting(resourceColumn, 'label');
// Test sorting by 'region' column
checkSorting(regionColumn, 'region');
});

nit: Just to make it a bit cleaner and more readable

placeholder="Search for a Region or Resource"
value={searchText || ''}
/>
{(isDataLoadingError || alertResourceIds.length) && ( // if there is data loading error display error message with empty table setup
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
{(isDataLoadingError || alertResourceIds.length) && ( // if there is data loading error display error message with empty table setup
{/* If there is a data loading error, display an error message with an empty table setup. */}
{(isDataLoadingError || alertResourceIds.length) && (

nit

expect(queryByText(linodes[0].label)).not.toBeInTheDocument();
expect(getByText(linodes[1].label)).toBeInTheDocument();
});
// clear the search input
Copy link
Contributor

Choose a reason for hiding this comment

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

very small nitpick, but also a personal preference: Since I’m seeing some comments start with a capital letter and others with lowercase, could we make sure to keep the first letter of every comment capitalized throughout for consistency?

@pmakode-akamai pmakode-akamai added the Add'tl Approval Needed Waiting on another approval! label Jan 24, 2025
@jaalah-akamai jaalah-akamai merged commit a0f070d into linode:develop Jan 24, 2025
23 checks passed
@venkymano-akamai venkymano-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Jan 26, 2025
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.

6 participants