-
Notifications
You must be signed in to change notification settings - Fork 394
upcoming: [DI-29171] - Notification Channel Show Details enhancements #13344
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
base: develop
Are you sure you want to change the base?
upcoming: [DI-29171] - Notification Channel Show Details enhancements #13344
Conversation
| }, | ||
| }; | ||
|
|
||
| hookMocks.useFlags.mockReturnValue(flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| vi.mock('src/hooks/useFlags', () => ({ | |
| useFlags: hookMocks.useFlags, | |
| })); |
Same here
| ); | ||
| } | ||
| return filteredAlerts; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| vi.mock('src/hooks/useFlags', () => ({ | |
| useFlags: hookMocks.useFlags, | |
| })); |
| handleSortClick( | ||
| orderBy, | ||
| handleOrderChange, | ||
| handlePageChange, | ||
| order | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe we could move this to a beforeEach OR outside the tests to reduce duplication
Cloud Manager UI test results🔺 2 failing tests on test run #8 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/clone-linode.spec.ts,cypress/e2e/core/objectStorage/access-key.e2e.spec.ts" |
||||||||||||||||||||
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 ✅