upcoming: [DI-29171] - Notification Channel Show Details enhancements#13344
Conversation
| }, | ||
| }; | ||
|
|
||
| hookMocks.useFlags.mockReturnValue(flags); |
There was a problem hiding this comment.
| hookMocks.useFlags.mockReturnValue(flags); |
Rather than mocking the use src/hooks/useFlags module, we should use our built in flags helper if possible, like this:
renderWithTheme(<NotificationChannelAlerts channelId={1} />, {
flags: mockFlags,
});
| renderWithTheme(<NotificationChannelAlerts channelId={1} />); | ||
| expect(screen.getAllByText(/beta/i)).toHaveLength(alerts.length); |
There was a problem hiding this comment.
| renderWithTheme(<NotificationChannelAlerts channelId={1} />); | |
| expect(screen.getAllByText(/beta/i)).toHaveLength(alerts.length); | |
| const { getAllByText } = renderWithTheme(<NotificationChannelAlerts channelId={1} />); | |
| expect(getAllByText(/beta/i)).toHaveLength(alerts.length); |
We generally follow the pattern of destructuring queries from renderWithTheme instead of using the global screen object. Could we update this file (and the other) to align with that convention?
There was a problem hiding this comment.
@pmakode-akamai , We were following the destructuring but after some ESLint upgrade we had the linter warning whenever we destructure and not use the global screen.
If destructing works well, then we will switch to destructuring from now.
There was a problem hiding this comment.
I will try to make these changes in few files where I already have to edit, but in this current PR we can't change all the implementations of global screen in the UTs .
We will take the task up separately
| expect(screen.queryByText('Linode CPU Alert')).not.toBeInTheDocument(); | ||
| expect(screen.queryByText('Linode Memory Alert')).not.toBeInTheDocument(); | ||
| }); | ||
| test('should render the Beta flag for the services in the service column', async () => { |
There was a problem hiding this comment.
| test('should render the Beta flag for the services in the service column', async () => { | |
| it('should render the Beta flag for the services in the service column', async () => { |
| vi.mock('src/hooks/useFlags', () => ({ | ||
| useFlags: queryMocks.useFlags, | ||
| })); | ||
|
|
||
| queryMocks.useFlags.mockReturnValue({ | ||
| aclpServices: aclpServicesFlag, | ||
| }); |
There was a problem hiding this comment.
| vi.mock('src/hooks/useFlags', () => ({ | |
| useFlags: queryMocks.useFlags, | |
| })); | |
| queryMocks.useFlags.mockReturnValue({ | |
| aclpServices: aclpServicesFlag, | |
| }); |
Same here: We should use our built in flags helper option if possible
| vi.mock('src/hooks/useFlags', () => ({ | ||
| useFlags: hookMocks.useFlags, | ||
| })); | ||
|
|
There was a problem hiding this comment.
| vi.mock('src/hooks/useFlags', () => ({ | |
| useFlags: hookMocks.useFlags, | |
| })); |
Same here
| ); | ||
| } | ||
| return filteredAlerts; | ||
| }; |
There was a problem hiding this comment.
Could we add unit tests for the newly added utility functions?
| vi.mock('src/hooks/useFlags', () => ({ | ||
| useFlags: hookMocks.useFlags, | ||
| })); | ||
|
|
There was a problem hiding this comment.
| vi.mock('src/hooks/useFlags', () => ({ | |
| useFlags: hookMocks.useFlags, | |
| })); |
| handleSortClick( | ||
| orderBy, | ||
| handleOrderChange, | ||
| handlePageChange, | ||
| order | ||
| ); |
There was a problem hiding this comment.
Is resetting to page 1 when sorting the expected behavior? I see a different pattern used elsewhere in the codebase where we keep the paginated page persisted when sorting by passing handleOrderChange directly to TableSortCell (e.g., NodeTable).
If resetting to page 1 is intentional, then we shouldn't be relying on functions with 3 / 3+ args. We generally prefer using object params in those cases. That said, if we can simplify this logic, it might be clearer to make it more straightforward like this:
const handleTableSort = (orderBy: string, order?: Order) => {
if (order) {
handleOrderChange(orderBy, order);
handlePageChange(1);
}
};
There was a problem hiding this comment.
From our UX for Alerting we have been following this since AlertsResources component. So for the consistency we follow this behaviour.
But the simplified logic makes sense. Will update that.
| const handlers = { | ||
| handleDelete: vi.fn(), | ||
| handleDetails: vi.fn(), | ||
| handleEdit: vi.fn(), | ||
| }; |
There was a problem hiding this comment.
nit: Maybe we could move this to a beforeEach OR outside the tests to reduce duplication
Cloud Manager UI test results🎉 866 passing tests on test run #10 ↗︎
|
|
Merging this as the cypress failures are not related the PR changes |
4 participants
Description 📝
Enhancements to the Notification Channel Show Details page
Changes 🔄
List any change(s) relevant to the reviewer.
Scope 🚢
Upon production release, changes in this PR will be visible to:
Target release date 🗓️
Feb 10th release
Preview 📷
Include a screenshot
<img src="" />or video<video src="" />of the change.🔒 Use the Mask Sensitive Data setting for security.
💡 For changes requiring multiple steps to validate, prefer a video for clarity.
Before
How to test 🧪
Prerequisites
(How to setup test environment)
Verification steps
(How to verify changes)
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
As an Author, before moving this PR from Draft to Open, I confirmed ✅