From d1a295698910b594c1f8c474d2fb26d75c514b3d Mon Sep 17 00:00:00 2001 From: Gabriel Torres Ruiz Date: Fri, 24 Oct 2025 10:28:17 -0400 Subject: [PATCH 1/7] fix(alerts): implement Slack channel pagination to resolve timeout issues --- .../components/NotificationMethod.test.tsx | 1014 +++++++++-------- .../alerts/components/NotificationMethod.tsx | 258 ++--- superset/reports/api.py | 11 +- superset/reports/schemas.py | 3 + superset/utils/slack.py | 166 ++- tests/integration_tests/reports/api_tests.py | 177 +++ tests/unit_tests/utils/slack_test.py | 516 ++++++++- 7 files changed, 1476 insertions(+), 669 deletions(-) diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx index c4e69ed22084..91b8d4d5f632 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx @@ -62,499 +62,621 @@ const mockSettingSlackV2: NotificationSetting = { ], }; -// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks -describe('NotificationMethod', () => { - beforeEach(() => { - jest.clearAllMocks(); - cleanup(); - }); +beforeEach(() => { + jest.clearAllMocks(); + cleanup(); +}); - test('should render the component', () => { - render( - , - ); +test('NotificationMethod - should render the component', () => { + render( + , + ); + + expect(screen.getByText('Notification Method')).toBeInTheDocument(); + expect(screen.getByText('Email subject name (optional)')).toBeInTheDocument(); + expect(screen.getByText('Email recipients')).toBeInTheDocument(); +}); - expect(screen.getByText('Notification Method')).toBeInTheDocument(); - expect( - screen.getByText('Email subject name (optional)'), - ).toBeInTheDocument(); - expect(screen.getByText('Email recipients')).toBeInTheDocument(); - }); +test('NotificationMethod - should call onRemove when the delete button is clicked', () => { + render( + , + ); + + const deleteButton = document.querySelector('.delete-button'); + if (deleteButton) userEvent.click(deleteButton); + + expect(mockOnRemove).toHaveBeenCalledWith(1); +}); - test('should call onRemove when the delete button is clicked', () => { - render( - , - ); +test('NotificationMethod - should update recipient value when input changes', () => { + render( + , + ); + + const recipientsInput = screen.getByTestId('recipients'); + fireEvent.change(recipientsInput, { + target: { value: 'test1@example.com' }, + }); - const deleteButton = document.querySelector('.delete-button'); - if (deleteButton) userEvent.click(deleteButton); + expect(mockOnUpdate).toHaveBeenCalledWith(0, { + ...mockSetting, + recipients: 'test1@example.com', + }); +}); - expect(mockOnRemove).toHaveBeenCalledWith(1); +test('NotificationMethod - should call onRecipientsChange when the recipients value is changed', () => { + render( + , + ); + + const recipientsInput = screen.getByTestId('recipients'); + fireEvent.change(recipientsInput, { + target: { value: 'test1@example.com' }, }); - test('should update recipient value when input changes', () => { - render( - , - ); + expect(mockOnUpdate).toHaveBeenCalledWith(0, { + ...mockSetting, + recipients: 'test1@example.com', + }); +}); - const recipientsInput = screen.getByTestId('recipients'); - fireEvent.change(recipientsInput, { - target: { value: 'test1@example.com' }, - }); +test('NotificationMethod - should correctly map recipients when method is SlackV2', () => { + const method = 'SlackV2'; + const recipientValue = 'user1,user2'; + const slackOptions: { label: string; value: string }[] = [ + { label: 'User One', value: 'user1' }, + { label: 'User Two', value: 'user2' }, + ]; - expect(mockOnUpdate).toHaveBeenCalledWith(0, { - ...mockSetting, - recipients: 'test1@example.com', - }); - }); + const result = mapSlackValues({ method, recipientValue, slackOptions }); - test('should call onRecipientsChange when the recipients value is changed', () => { - render( - , - ); + expect(result).toEqual([ + { label: 'User One', value: 'user1' }, + { label: 'User Two', value: 'user2' }, + ]); +}); - const recipientsInput = screen.getByTestId('recipients'); - fireEvent.change(recipientsInput, { - target: { value: 'test1@example.com' }, - }); +test('NotificationMethod - should return an empty array when recipientValue is an empty string', () => { + const method = 'SlackV2'; + const recipientValue = ''; + const slackOptions: { label: string; value: string }[] = [ + { label: 'User One', value: 'user1' }, + { label: 'User Two', value: 'user2' }, + ]; - expect(mockOnUpdate).toHaveBeenCalledWith(0, { - ...mockSetting, - recipients: 'test1@example.com', - }); - }); + const result = mapSlackValues({ method, recipientValue, slackOptions }); - test('should correctly map recipients when method is SlackV2', () => { - const method = 'SlackV2'; - const recipientValue = 'user1,user2'; - const slackOptions: { label: string; value: string }[] = [ - { label: 'User One', value: 'user1' }, - { label: 'User Two', value: 'user2' }, - ]; + expect(result).toEqual([]); +}); - const result = mapSlackValues({ method, recipientValue, slackOptions }); +test('NotificationMethod - should correctly map recipients when method is Slack with updated recipient values', () => { + const method = 'Slack'; + const recipientValue = 'User One,User Two'; + const slackOptions: { label: string; value: string }[] = [ + { label: 'User One', value: 'user1' }, + { label: 'User Two', value: 'user2' }, + ]; - expect(result).toEqual([ - { label: 'User One', value: 'user1' }, - { label: 'User Two', value: 'user2' }, - ]); - }); + const result = mapSlackValues({ method, recipientValue, slackOptions }); - test('should return an empty array when recipientValue is an empty string', () => { - const method = 'SlackV2'; - const recipientValue = ''; - const slackOptions: { label: string; value: string }[] = [ - { label: 'User One', value: 'user1' }, - { label: 'User Two', value: 'user2' }, - ]; + expect(result).toEqual([ + { label: 'User One', value: 'user1' }, + { label: 'User Two', value: 'user2' }, + ]); +}); - const result = mapSlackValues({ method, recipientValue, slackOptions }); +test('NotificationMethod - should render CC and BCC fields when method is Email and visibility flags are true', () => { + const defaultProps = { + setting: { + method: NotificationMethodOption.Email, + recipients: 'recipient1@example.com, recipient2@example.com', + cc: 'cc1@example.com', + bcc: 'bcc1@example.com', + options: [NotificationMethodOption.Email, NotificationMethodOption.Slack], + }, + index: 0, + onUpdate: jest.fn(), + onRemove: jest.fn(), + onInputChange: jest.fn(), + email_subject: 'Test Subject', + defaultSubject: 'Default Subject', + setErrorSubject: jest.fn(), + }; + + const { getByTestId } = render(); + + // Check if CC and BCC fields are rendered + expect(getByTestId('cc')).toBeInTheDocument(); + expect(getByTestId('bcc')).toBeInTheDocument(); +}); - expect(result).toEqual([]); - }); +test('NotificationMethod - should render CC and BCC fields with correct values when method is Email', () => { + const defaultProps = { + setting: { + method: NotificationMethodOption.Email, + recipients: 'recipient1@example.com, recipient2@example.com', + cc: 'cc1@example.com', + bcc: 'bcc1@example.com', + options: [NotificationMethodOption.Email, NotificationMethodOption.Slack], + }, + index: 0, + onUpdate: jest.fn(), + onRemove: jest.fn(), + onInputChange: jest.fn(), + email_subject: 'Test Subject', + defaultSubject: 'Default Subject', + setErrorSubject: jest.fn(), + }; + + const { getByTestId } = render(); + + // Check if CC and BCC fields are rendered with correct values + expect(getByTestId('cc')).toHaveValue('cc1@example.com'); + expect(getByTestId('bcc')).toHaveValue('bcc1@example.com'); +}); - test('should correctly map recipients when method is Slack with updated recipient values', () => { - const method = 'Slack'; - const recipientValue = 'User One,User Two'; - const slackOptions: { label: string; value: string }[] = [ - { label: 'User One', value: 'user1' }, - { label: 'User Two', value: 'user2' }, - ]; +test('NotificationMethod - should not render CC and BCC fields when method is not Email', () => { + const defaultProps = { + setting: { + method: NotificationMethodOption.Slack, + recipients: 'recipient1@example.com, recipient2@example.com', + cc: 'cc1@example.com', + bcc: 'bcc1@example.com', + options: [NotificationMethodOption.Email, NotificationMethodOption.Slack], + }, + index: 0, + onUpdate: jest.fn(), + onRemove: jest.fn(), + onInputChange: jest.fn(), + email_subject: 'Test Subject', + defaultSubject: 'Default Subject', + setErrorSubject: jest.fn(), + }; + + const { queryByTestId } = render(); + + // Check if CC and BCC fields are not rendered + expect(queryByTestId('cc')).not.toBeInTheDocument(); + expect(queryByTestId('bcc')).not.toBeInTheDocument(); +}); +// Handle empty recipients list gracefully +test('NotificationMethod - should handle empty recipients list gracefully', () => { + const defaultProps = { + setting: { + method: NotificationMethodOption.Email, + recipients: '', + cc: '', + bcc: '', + options: [NotificationMethodOption.Email, NotificationMethodOption.Slack], + }, + index: 0, + onUpdate: jest.fn(), + onRemove: jest.fn(), + onInputChange: jest.fn(), + email_subject: 'Test Subject', + defaultSubject: 'Default Subject', + setErrorSubject: jest.fn(), + }; + + const { queryByTestId } = render(); + + // Check if CC and BCC fields are not rendered + expect(queryByTestId('cc')).not.toBeInTheDocument(); + expect(queryByTestId('bcc')).not.toBeInTheDocument(); +}); - const result = mapSlackValues({ method, recipientValue, slackOptions }); +test('shows the right combo when ff is false', async () => { + /* should show the div with "Recipients are separated by" + when FeatureFlag.AlertReportSlackV2 is false and fetchSlackChannels errors + */ + // Mock the feature flag to be false + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: false }; - expect(result).toEqual([ - { label: 'User One', value: 'user1' }, - { label: 'User Two', value: 'user2' }, - ]); - }); - test('should render CC and BCC fields when method is Email and visibility flags are true', () => { - const defaultProps = { - setting: { - method: NotificationMethodOption.Email, - recipients: 'recipient1@example.com, recipient2@example.com', - cc: 'cc1@example.com', - bcc: 'bcc1@example.com', - options: [ - NotificationMethodOption.Email, - NotificationMethodOption.Slack, - ], - }, - index: 0, - onUpdate: jest.fn(), - onRemove: jest.fn(), - onInputChange: jest.fn(), - email_subject: 'Test Subject', - defaultSubject: 'Default Subject', - setErrorSubject: jest.fn(), - }; - - const { getByTestId } = render(); - - // Check if CC and BCC fields are rendered - expect(getByTestId('cc')).toBeInTheDocument(); - expect(getByTestId('bcc')).toBeInTheDocument(); - }); - test('should render CC and BCC fields with correct values when method is Email', () => { - const defaultProps = { - setting: { - method: NotificationMethodOption.Email, - recipients: 'recipient1@example.com, recipient2@example.com', - cc: 'cc1@example.com', - bcc: 'bcc1@example.com', - options: [ - NotificationMethodOption.Email, - NotificationMethodOption.Slack, - ], - }, - index: 0, - onUpdate: jest.fn(), - onRemove: jest.fn(), - onInputChange: jest.fn(), - email_subject: 'Test Subject', - defaultSubject: 'Default Subject', - setErrorSubject: jest.fn(), - }; - - const { getByTestId } = render(); - - // Check if CC and BCC fields are rendered with correct values - expect(getByTestId('cc')).toHaveValue('cc1@example.com'); - expect(getByTestId('bcc')).toHaveValue('bcc1@example.com'); + // Mock the SupersetClient.get to simulate an error + jest.spyOn(SupersetClient, 'get').mockImplementation(() => { + throw new Error('Error fetching Slack channels'); }); - test('should not render CC and BCC fields when method is not Email', () => { - const defaultProps = { - setting: { + + render( + ); - - // Check if CC and BCC fields are not rendered - expect(queryByTestId('cc')).not.toBeInTheDocument(); - expect(queryByTestId('bcc')).not.toBeInTheDocument(); - }); - // Handle empty recipients list gracefully - test('should handle empty recipients list gracefully', () => { - const defaultProps = { - setting: { - method: NotificationMethodOption.Email, - recipients: '', - cc: '', - bcc: '', - options: [ - NotificationMethodOption.Email, - NotificationMethodOption.Slack, - ], - }, - index: 0, - onUpdate: jest.fn(), - onRemove: jest.fn(), - onInputChange: jest.fn(), - email_subject: 'Test Subject', - defaultSubject: 'Default Subject', - setErrorSubject: jest.fn(), - }; - - const { queryByTestId } = render(); - - // Check if CC and BCC fields are not rendered - expect(queryByTestId('cc')).not.toBeInTheDocument(); - expect(queryByTestId('bcc')).not.toBeInTheDocument(); + }} + index={0} + onUpdate={mockOnUpdate} + onRemove={mockOnRemove} + onInputChange={mockOnInputChange} + email_subject={mockEmailSubject} + defaultSubject={mockDefaultSubject} + setErrorSubject={mockSetErrorSubject} + />, + ); + + // Wait for the component to handle the error and render the expected div + await waitFor(() => { + expect( + screen.getByText('Recipients are separated by "," or ";"'), + ).toBeInTheDocument(); }); - test('shows the right combo when ff is false', async () => { - /* should show the div with "Recipients are separated by" - when FeatureFlag.AlertReportSlackV2 is false and fetchSlackChannels errors - */ - // Mock the feature flag to be false - window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: false }; - - // Mock the SupersetClient.get to simulate an error - jest.spyOn(SupersetClient, 'get').mockImplementation(() => { - throw new Error('Error fetching Slack channels'); - }); - - render( - , - ); +}); - // Wait for the component to handle the error and render the expected div - await waitFor(() => { - expect( - screen.getByText('Recipients are separated by "," or ";"'), - ).toBeInTheDocument(); - }); - }); - test('shows the textbox when the fetch fails', async () => { - /* should show the div with "Recipients are separated by" +test('shows the textbox when the fetch fails', async () => { + /* should show the div with "Recipients are separated by" when FeatureFlag.AlertReportSlackV2 is true and fetchSlackChannels errors */ - // Mock the feature flag to be false - window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: false }; - - // Mock the SupersetClient.get to simulate an error - jest.spyOn(SupersetClient, 'get').mockImplementation(() => { - throw new Error('Error fetching Slack channels'); - }); - - render( - , - ); + // Mock the feature flag to be false + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: false }; - // Wait for the component to handle the error and render the expected div - await waitFor(() => { - expect( - screen.getByText('Recipients are separated by "," or ";"'), - ).toBeInTheDocument(); - }); + // Mock the SupersetClient.get to simulate an error + jest.spyOn(SupersetClient, 'get').mockImplementation(() => { + throw new Error('Error fetching Slack channels'); }); - test('shows the dropdown when ff is true and slackChannels succeed', async () => { - /* should show the Select channels dropdown + + render( + , + ); + + // Wait for the component to handle the error and render the expected div + await waitFor(() => { + expect( + screen.getByText('Recipients are separated by "," or ";"'), + ).toBeInTheDocument(); + }); +}); + +test('shows the dropdown when ff is true and slackChannels succeed', async () => { + /* should show the Select channels dropdown when FeatureFlag.AlertReportSlackV2 is true and fetchSlackChannels succeeds */ - // Mock the feature flag to be false - window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; - - // Mock the SupersetClient.get to simulate an error - jest - .spyOn(SupersetClient, 'get') - .mockImplementation( - () => - Promise.resolve({ json: { result: [] } }) as unknown as Promise< - Response | JsonResponse | TextResponse - >, - ); - - render( - , + // Mock the feature flag to be false + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + + // Mock the SupersetClient.get to simulate an error + jest + .spyOn(SupersetClient, 'get') + .mockImplementation( + () => + Promise.resolve({ json: { result: [] } }) as unknown as Promise< + Response | JsonResponse | TextResponse + >, ); - // Wait for the component to handle the error and render the expected div - await waitFor(() => { - expect(screen.getByTitle('Slack')).toBeInTheDocument(); - }); + render( + , + ); + + // Wait for the component to handle the error and render the expected div + await waitFor(() => { + expect(screen.getByTitle('Slack')).toBeInTheDocument(); }); - test('shows the textarea when ff is true and slackChannels fail', async () => { - /* should show the Select channels dropdown +}); + +test('shows the textarea when ff is true and slackChannels fail', async () => { + /* should show the Select channels dropdown when FeatureFlag.AlertReportSlackV2 is true and fetchSlackChannels succeeds */ - // Mock the feature flag to be false - window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; - - // Mock the SupersetClient.get to simulate an error - jest.spyOn(SupersetClient, 'get').mockImplementation(() => { - throw new Error('Error fetching Slack channels'); - }); - - render( - , - ); + // Mock the feature flag to be false + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; - // Wait for the component to handle the error and render the expected div - expect( - screen.getByText('Recipients are separated by "," or ";"'), - ).toBeInTheDocument(); + // Mock the SupersetClient.get to simulate an error + jest.spyOn(SupersetClient, 'get').mockImplementation(() => { + throw new Error('Error fetching Slack channels'); }); - test('shows the textarea when ff is true and slackChannels fail and slack is selected', async () => { - /* should show the Select channels dropdown + + render( + , + ); + + // Wait for the component to handle the error and render the expected div + expect( + screen.getByText('Recipients are separated by "," or ";"'), + ).toBeInTheDocument(); +}); + +test('shows the textarea when ff is true and slackChannels fail and slack is selected', async () => { + /* should show the Select channels dropdown when FeatureFlag.AlertReportSlackV2 is true and fetchSlackChannels succeeds */ - // Mock the feature flag to be false - window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; - - // Mock the SupersetClient.get to simulate an error - jest.spyOn(SupersetClient, 'get').mockImplementation(() => { - throw new Error('Error fetching Slack channels'); - }); - - render( - , - ); + // Mock the feature flag to be false + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; - // Wait for the component to handle the error and render the expected div - expect( - screen.getByText('Recipients are separated by "," or ";"'), - ).toBeInTheDocument(); + // Mock the SupersetClient.get to simulate an error + jest.spyOn(SupersetClient, 'get').mockImplementation(() => { + throw new Error('Error fetching Slack channels'); }); - test('shows the textarea when ff is true, slackChannels fail and slack is selected', async () => { - window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; - jest.spyOn(SupersetClient, 'get').mockImplementation(() => { - throw new Error('Error fetching Slack channels'); - }); - - render( - , + render( + , + ); + + // Wait for the component to handle the error and render the expected div + expect( + screen.getByText('Recipients are separated by "," or ";"'), + ).toBeInTheDocument(); +}); + +test('shows the textarea when ff is true, slackChannels fail and slack is selected', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + jest.spyOn(SupersetClient, 'get').mockImplementation(() => { + throw new Error('Error fetching Slack channels'); + }); + + render( + , + ); + + expect( + screen.getByText('Recipients are separated by "," or ";"'), + ).toBeInTheDocument(); +}); + +test('NotificationMethod - AsyncSelect should render for SlackV2 method', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + const mockChannels = [ + { name: 'general', id: 'C001', is_private: false, is_member: true }, + { name: 'random', id: 'C002', is_private: false, is_member: true }, + ]; + + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + } as unknown as Promise); + + render( + , + ); + + // Verify AsyncSelect is rendered for SlackV2 + await waitFor(() => { + expect(screen.getByTitle('Slack')).toBeInTheDocument(); + }); +}); + +test('NotificationMethod - AsyncSelect should handle SlackV2 with valid API response', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + const mockChannels = [ + { + name: 'test-channel', + id: 'C123', + is_private: false, + is_member: true, + }, + ]; + + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + } as unknown as Promise); + + const { container } = render( + , + ); + + // Verify AsyncSelect container is present (not a textarea) + await waitFor(() => { + const textarea = container.querySelector( + 'textarea[data-test="recipients"]', ); + expect(textarea).toBeNull(); // Should NOT have textarea for SlackV2 + }); +}); - expect( - screen.getByText('Recipients are separated by "," or ";"'), - ).toBeInTheDocument(); +test('NotificationMethod - AsyncSelect should render refresh button for SlackV2', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + const mockChannels = [ + { + name: 'test-channel', + id: 'C123', + is_private: false, + is_member: true, + }, + ]; + + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + } as unknown as Promise); + + render( + , + ); + + // Verify refresh button is present (icon only button) + await waitFor(() => { + const refreshButton = screen.getByTestId('refresh-slack-channels'); + expect(refreshButton).toBeInTheDocument(); + expect(refreshButton).toHaveAttribute('title', 'Refresh channels'); + }); +}); + +test('NotificationMethod - AsyncSelect should clear recipients and reload channels when refresh button clicked', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + const mockChannels = [ + { + name: 'test-channel', + id: 'C123', + is_private: false, + is_member: true, + }, + ]; + + jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + } as unknown as Promise); + + render( + , + ); + + // Wait for initial render + await waitFor(() => { + expect(screen.getByTestId('refresh-slack-channels')).toBeInTheDocument(); }); - // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks - describe('RefreshLabel functionality', () => { - test('should call updateSlackOptions with force true when RefreshLabel is clicked', async () => { - // Set feature flag so that SlackV2 branch renders RefreshLabel - window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; - // Spy on SupersetClient.get which is called by updateSlackOptions - const supersetClientSpy = jest - .spyOn(SupersetClient, 'get') - .mockImplementation( - () => - Promise.resolve({ json: { result: [] } }) as unknown as Promise< - Response | JsonResponse | TextResponse - >, - ); - - render( - , - ); - - // Wait for RefreshLabel to be rendered (it may have a tooltip with the provided content) - const refreshLabel = await waitFor(() => screen.getByLabelText('sync')); - // Simulate a click on the RefreshLabel - userEvent.click(refreshLabel); - // Verify that the SupersetClient.get was called indicating that updateSlackOptions executed - await waitFor(() => { - expect(supersetClientSpy).toHaveBeenCalled(); - }); - }); + // Click refresh button + const refreshButton = screen.getByTestId('refresh-slack-channels'); + fireEvent.click(refreshButton); + + // Verify onUpdate was called with empty recipients + await waitFor(() => { + expect(mockOnUpdate).toHaveBeenCalledWith( + 0, + expect.objectContaining({ + recipients: '', + }), + ); }); }); diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx index 228130b32e58..e7efa2646add 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx @@ -20,22 +20,20 @@ import { FunctionComponent, useState, ChangeEvent, - useEffect, useMemo, + useRef, } from 'react'; import rison from 'rison'; import { FeatureFlag, - JsonResponse, SupersetClient, isFeatureEnabled, t, } from '@superset-ui/core'; import { styled, useTheme } from '@apache-superset/core/ui'; import { Icons } from '@superset-ui/core/components/Icons'; -import { Input, Select } from '@superset-ui/core/components'; -import RefreshLabel from '@superset-ui/core/components/RefreshLabel'; +import { Input, Select, AsyncSelect } from '@superset-ui/core/components'; import { NotificationMethodOption, NotificationSetting, @@ -76,6 +74,17 @@ const StyledNotificationMethod = styled.div` margin-left: ${theme.sizeUnit * 2}px; padding-top: ${theme.sizeUnit}px; } + + .refresh-button { + margin-left: ${theme.sizeUnit * 2}px; + cursor: pointer; + color: ${theme.colorTextSecondary}; + + &:hover { + color: ${theme.colorPrimary}; + } + } + .anticon { margin-left: ${theme.sizeUnit}px; } @@ -149,47 +158,6 @@ export const mapSlackValues = ({ .filter(val => !!val) as { label: string; value: string }[]; }; -export const mapChannelsToOptions = (result: SlackChannel[]) => { - const publicChannels: SlackChannel[] = []; - const privateChannels: SlackChannel[] = []; - - result.forEach(channel => { - if (channel.is_private) { - privateChannels.push(channel); - } else { - publicChannels.push(channel); - } - }); - - return [ - { - label: 'Public Channels', - options: publicChannels.map((channel: SlackChannel) => ({ - label: `${channel.name} ${ - channel.is_member ? '' : t('(Bot not in channel)') - }`, - value: channel.id, - key: channel.id, - })), - key: 'public', - }, - { - label: t('Private Channels (Bot in channel)'), - options: privateChannels.map((channel: SlackChannel) => ({ - label: channel.name, - value: channel.id, - key: channel.id, - })), - key: 'private', - }, - ]; -}; - -type SlackOptionsType = { - label: string; - options: { label: string; value: string }[]; -}[]; - export const NotificationMethod: FunctionComponent = ({ setting = null, index, @@ -213,24 +181,16 @@ export const NotificationMethod: FunctionComponent = ({ const [ccValue, setCcValue] = useState(cc || ''); const [bccValue, setBccValue] = useState(bcc || ''); const theme = useTheme(); - const [methodOptionsLoading, setMethodOptionsLoading] = - useState(true); - const [slackOptions, setSlackOptions] = useState([ - { - label: '', - options: [], - }, - ]); - const [useSlackV1, setUseSlackV1] = useState(false); - const [isSlackChannelsLoading, setIsSlackChannelsLoading] = - useState(true); + const cursorRef = useRef>({}); + const [refreshKey, setRefreshKey] = useState(0); + const forceRefreshRef = useRef(false); + const [isRefreshing, setIsRefreshing] = useState(false); const onMethodChange = (selected: { label: string; value: NotificationMethodOption; }) => { - // Since we're swapping the method, reset the recipients setRecipientValue(''); setCcValue(''); setBccValue(''); @@ -248,84 +208,91 @@ export const NotificationMethod: FunctionComponent = ({ } }; - const fetchSlackChannels = async ({ - searchString = '', - types = [], - exactMatch = false, - force = false, - }: { - searchString?: string | undefined; - types?: string[]; - exactMatch?: boolean | undefined; - force?: boolean | undefined; - } = {}): Promise => { - const queryString = rison.encode({ - searchString, - types, - exactMatch, - force, - }); - const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; - return SupersetClient.get({ endpoint }); - }; + const fetchSlackChannels = async ( + search: string, + page: number, + pageSize: number, + ): Promise<{ + data: { label: string; value: string }[]; + totalCount: number; + }> => { + try { + const cacheKey = `${search}:${page}`; + const cursor = page > 0 ? cursorRef.current[cacheKey] : null; + + const params: Record = { + types: ['public_channel', 'private_channel'], + limit: pageSize, + }; - const updateSlackOptions = async ({ - force, - }: { - force?: boolean | undefined; - } = {}) => { - setIsSlackChannelsLoading(true); - fetchSlackChannels({ types: ['public_channel', 'private_channel'], force }) - .then(({ json }) => { - const { result } = json; - const options: SlackOptionsType = mapChannelsToOptions(result); - - setSlackOptions(options); - - if (isFeatureEnabled(FeatureFlag.AlertReportSlackV2)) { - // for edit mode, map existing ids to names for display if slack v2 - // or names to ids if slack v1 - const [publicOptions, privateOptions] = options; - if ( - method && - [ - NotificationMethodOption.SlackV2, - NotificationMethodOption.Slack, - ].includes(method) - ) { - setSlackRecipients( - mapSlackValues({ - method, - recipientValue, - slackOptions: [ - ...publicOptions.options, - ...privateOptions.options, - ], - }), - ); - } - } - }) - .catch(e => { - // Fallback to slack v1 if slack v2 is not compatible - setUseSlackV1(true); - }) - .finally(() => { - setMethodOptionsLoading(false); - setIsSlackChannelsLoading(false); - }); + if (page === 0 && forceRefreshRef.current) { + params.force = true; + forceRefreshRef.current = false; + } + + if (search) { + params.search_string = search; + } + + if (cursor) { + params.cursor = cursor; + } + + const queryString = rison.encode(params); + const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; + const response = await SupersetClient.get({ endpoint }); + + const { result, next_cursor, has_more } = response.json; + + if (next_cursor) { + cursorRef.current[`${search}:${page + 1}`] = next_cursor; + } + + const options = result.map((channel: SlackChannel) => ({ + label: channel.name, + value: channel.id, + })); + + const totalCount = has_more + ? (page + 1) * pageSize + 1 // Fake "has more" + : page * pageSize + options.length; // Last page + + return { + data: options, + totalCount, + }; + } catch (error) { + console.error('Failed to fetch Slack channels:', error); + setUseSlackV1(true); + throw error; + } }; - useEffect(() => { - const slackEnabled = options?.some( - option => - option === NotificationMethodOption.Slack || - option === NotificationMethodOption.SlackV2, - ); - if (slackEnabled && !slackOptions[0]?.options.length) { - updateSlackOptions(); + const handleRefreshSlackChannels = async () => { + setIsRefreshing(true); + + try { + forceRefreshRef.current = true; + cursorRef.current = {}; + + setSlackRecipients([]); + + if (onUpdate && setting) { + onUpdate(index, { + ...setting, + recipients: '', + } as NotificationSetting); + } + + await fetchSlackChannels('', 0, 100); + + setRefreshKey(prev => prev + 1); + } catch (error) { + console.error('Error refreshing channels:', error); + } finally { + setIsRefreshing(false); } - }, []); + }; const methodOptions = useMemo( () => @@ -458,10 +425,8 @@ export const NotificationMethod: FunctionComponent = ({ options={methodOptions} showSearch value={methodOptions.find(option => option.value === method)} - loading={methodOptionsLoading} /> {index !== 0 && !!onRemove ? ( - // eslint-disable-next-line jsx-a11y/control-has-associated-label = ({ ) : ( // for SlackV2
- From 0897c99390d9d4880325953b82df56572faeead8 Mon Sep 17 00:00:00 2001 From: Gabriel Torres Ruiz Date: Sun, 2 Nov 2025 23:19:55 -0400 Subject: [PATCH 5/7] fix(alerts): address new PR comments --- .../src/features/alerts/AlertReportModal.tsx | 8 +- .../components/NotificationMethod.test.tsx | 223 +++++++++++++++-- .../alerts/components/NotificationMethod.tsx | 228 ++++++++++-------- superset/tasks/slack.py | 51 +++- superset/utils/slack.py | 109 +++------ tests/unit_tests/utils/slack_test.py | 208 ++++++++++++---- 6 files changed, 585 insertions(+), 242 deletions(-) diff --git a/superset-frontend/src/features/alerts/AlertReportModal.tsx b/superset-frontend/src/features/alerts/AlertReportModal.tsx index 2d1b216a7b22..60cc94dd2113 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx @@ -1,3 +1,4 @@ +/* eslint-disable camelcase */ /** * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -2586,15 +2587,16 @@ const AlertReportModal: FunctionComponent = ({ children: ( <> {notificationSettings.map((notificationSetting, i) => ( - + { onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -96,7 +96,7 @@ test('NotificationMethod - should call onRemove when the delete button is clicke onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -117,7 +117,7 @@ test('NotificationMethod - should update recipient value when input changes', () onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -143,7 +143,7 @@ test('NotificationMethod - should call onRecipientsChange when the recipients va onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -219,7 +219,7 @@ test('NotificationMethod - should render CC and BCC fields when method is Email onUpdate: jest.fn(), onRemove: jest.fn(), onInputChange: jest.fn(), - email_subject: 'Test Subject', + emailSubject: 'Test Subject', defaultSubject: 'Default Subject', setErrorSubject: jest.fn(), }; @@ -244,7 +244,7 @@ test('NotificationMethod - should render CC and BCC fields with correct values w onUpdate: jest.fn(), onRemove: jest.fn(), onInputChange: jest.fn(), - email_subject: 'Test Subject', + emailSubject: 'Test Subject', defaultSubject: 'Default Subject', setErrorSubject: jest.fn(), }; @@ -269,7 +269,7 @@ test('NotificationMethod - should not render CC and BCC fields when method is no onUpdate: jest.fn(), onRemove: jest.fn(), onInputChange: jest.fn(), - email_subject: 'Test Subject', + emailSubject: 'Test Subject', defaultSubject: 'Default Subject', setErrorSubject: jest.fn(), }; @@ -294,7 +294,7 @@ test('NotificationMethod - should handle empty recipients list gracefully', () = onUpdate: jest.fn(), onRemove: jest.fn(), onInputChange: jest.fn(), - email_subject: 'Test Subject', + emailSubject: 'Test Subject', defaultSubject: 'Default Subject', setErrorSubject: jest.fn(), }; @@ -328,7 +328,7 @@ test('shows the right combo when ff is false', async () => { onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -366,7 +366,7 @@ test('shows the textbox when the fetch fails', async () => { onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -409,7 +409,7 @@ test('shows the dropdown when ff is true and slackChannels succeed', async () => onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -445,7 +445,7 @@ test('shows the textarea when ff is true and slackChannels fail', async () => { onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -481,7 +481,7 @@ test('shows the textarea when ff is true and slackChannels fail and slack is sel onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -510,7 +510,7 @@ test('shows the textarea when ff is true, slackChannels fail and slack is select onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -544,7 +544,7 @@ test('NotificationMethod - AsyncSelect should render for SlackV2 method', async onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -583,7 +583,7 @@ test('NotificationMethod - AsyncSelect should handle SlackV2 with valid API resp onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -625,7 +625,7 @@ test('NotificationMethod - AsyncSelect should render refresh button for SlackV2' onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -669,7 +669,7 @@ test('NotificationMethod - AsyncSelect should reload channels without clearing r onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -743,7 +743,7 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -810,7 +810,7 @@ test('NotificationMethod - should preserve selected channels on refresh', async onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -885,7 +885,7 @@ test('NotificationMethod - should initialize Slack recipients when editing exist onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -990,7 +990,7 @@ test('NotificationMethod - should preserve selected channels during search and p onUpdate={mockOnUpdate} onRemove={mockOnRemove} onInputChange={mockOnInputChange} - email_subject={mockEmailSubject} + emailSubject={mockEmailSubject} defaultSubject={mockDefaultSubject} setErrorSubject={mockSetErrorSubject} addDangerToast={mockAddDangerToast} @@ -1052,3 +1052,182 @@ test('NotificationMethod - should preserve selected channels during search and p getSpy.mockRestore(); }); + +test('NotificationMethod - should support comma-separated channel search', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + + const mockChannelsPage1 = [ + { id: 'C001', name: 'engineering', is_private: false, is_member: true }, + { + id: 'C002', + name: 'engineering-team', + is_private: false, + is_member: true, + }, + ]; + const mockChannelsPage2 = [ + { id: 'C003', name: 'marketing', is_private: false, is_member: true }, + { id: 'C004', name: 'sales', is_private: false, is_member: true }, + ]; + + let callCount = 0; + const getSpy = jest.spyOn(SupersetClient, 'get').mockImplementation(() => { + callCount += 1; + if (callCount === 1) { + return Promise.resolve({ + json: { + result: mockChannelsPage1, + next_cursor: 'cursor_2', + has_more: true, + }, + }) as unknown as Promise; + } + return Promise.resolve({ + json: { + result: mockChannelsPage2, + next_cursor: null, + has_more: false, + }, + }) as unknown as Promise; + }); + + const { getByTestId } = render( + , + ); + + await waitFor( + () => { + expect(getByTestId('recipients')).toBeInTheDocument(); + }, + { timeout: 3000 }, + ); + + await waitFor( + () => { + expect(getSpy).toHaveBeenCalled(); + }, + { timeout: 3000 }, + ); + + const apiCalls = getSpy.mock.calls; + expect(apiCalls.length).toBeGreaterThan(0); + + getSpy.mockRestore(); +}); + +test('NotificationMethod - AsyncSelect should not filter results client-side', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + + const mockChannels = [ + { id: 'C001', name: 'analytics-team', is_private: false, is_member: true }, + { id: 'C002', name: 'analytics-ops', is_private: false, is_member: true }, + { id: 'C003', name: 'engineering', is_private: false, is_member: true }, + ]; + + const getSpy = jest.spyOn(SupersetClient, 'get').mockImplementation( + () => + Promise.resolve({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + }) as unknown as Promise, + ); + + const { getByTestId } = render( + , + ); + + await waitFor( + () => { + expect(getByTestId('recipients')).toBeInTheDocument(); + }, + { timeout: 3000 }, + ); + + await waitFor( + () => { + expect(getSpy).toHaveBeenCalled(); + }, + { timeout: 3000 }, + ); + + const recipientsSelect = getByTestId('recipients'); + expect(recipientsSelect).toBeInTheDocument(); + + getSpy.mockRestore(); +}); + +test('NotificationMethod - AsyncSelect should not use comma as token separator', async () => { + window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true }; + + const mockChannels = [ + { id: 'C001', name: 'engineering', is_private: false, is_member: true }, + { id: 'C002', name: 'marketing', is_private: false, is_member: true }, + ]; + + const getSpy = jest.spyOn(SupersetClient, 'get').mockImplementation( + () => + Promise.resolve({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + }) as unknown as Promise, + ); + + const { getByTestId } = render( + , + ); + + await waitFor( + () => { + expect(getByTestId('recipients')).toBeInTheDocument(); + }, + { timeout: 3000 }, + ); + + await waitFor( + () => { + expect(getSpy).toHaveBeenCalled(); + }, + { timeout: 3000 }, + ); + + const recipientsSelect = getByTestId('recipients'); + expect(recipientsSelect).toBeInTheDocument(); + + getSpy.mockRestore(); +}); diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx index 423b2ca51302..207a7869e8f5 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx @@ -24,7 +24,9 @@ import { useMemo, useRef, useEffect, + useCallback, } from 'react'; +import { debounce } from 'lodash'; import rison from 'rison'; import { @@ -42,7 +44,7 @@ import { SlackChannel, } from '../types'; import { StyledInputContainer } from '../AlertReportModal'; -import { styled, useTheme } from '@apache-superset/core'; +import { styled, useTheme } from '@apache-superset/core/ui'; const StyledNotificationMethod = styled.div` ${({ theme }) => ` @@ -195,10 +197,11 @@ export const NotificationMethod: FunctionComponent = ({ { data: { label: string; value: string }[]; totalCount: number } > >({}); - const [refreshKey, setRefreshKey] = useState(0); const forceRefreshRef = useRef(false); const [isRefreshing, setIsRefreshing] = useState(false); - const hasShownErrorToast = useRef(false); // Track if we've shown the error toast + const hasShownErrorToast = useRef(false); + const [searchGeneration, setSearchGeneration] = useState(0); + const lastSearchValueRef = useRef(''); const onMethodChange = (selected: { label: string; @@ -221,105 +224,120 @@ export const NotificationMethod: FunctionComponent = ({ } }; - const fetchSlackChannels = async ( - search: string, - page: number, - pageSize: number, - ): Promise<{ - data: { label: string; value: string }[]; - totalCount: number; - }> => { - try { - const cacheKey = `${search}:${page}`; - - if (dataCache.current[cacheKey] && !forceRefreshRef.current) { - return dataCache.current[cacheKey]; - } - - const cursor = page > 0 ? cursorRef.current[cacheKey] : null; - - const params: Record = { - types: ['public_channel', 'private_channel'], - limit: pageSize, - }; - - if (page === 0 && forceRefreshRef.current) { - params.force = true; - forceRefreshRef.current = false; - } - - if (search) { - params.search_string = search; - } + const fetchSlackChannels = useCallback( + async ( + search: string, + page: number, + pageSize: number, + ): Promise<{ + data: { label: string; value: string }[]; + totalCount: number; + }> => { + try { + const cacheKey = `${search}:${page}`; + + if (dataCache.current[cacheKey] && !forceRefreshRef.current) { + return dataCache.current[cacheKey]; + } - if (cursor) { - params.cursor = cursor; - } + // Clear cache for previous searches to prevent stale data + if (page === 0) { + Object.keys(dataCache.current).forEach(key => { + if (!key.startsWith(`${search}:`)) { + delete dataCache.current[key]; + delete cursorRef.current[key]; + } + }); + } - const queryString = rison.encode(params); - const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; - const response = await SupersetClient.get({ endpoint }); + const cursor = page > 0 ? cursorRef.current[cacheKey] : null; - const { result, next_cursor, has_more } = response.json; + const params: Record = { + types: ['public_channel', 'private_channel'], + limit: pageSize, + }; - if (next_cursor) { - cursorRef.current[`${search}:${page + 1}`] = next_cursor; - } + if (page === 0 && forceRefreshRef.current) { + params.force = true; + forceRefreshRef.current = false; + } - const options = result.map((channel: SlackChannel) => ({ - label: channel.name, - value: channel.id, - })); + if (search) { + params.search_string = search; + } - const totalCount = has_more - ? (page + 1) * pageSize + 1 - : page * pageSize + options.length; + if (cursor) { + params.cursor = cursor; + } - const responseData = { - data: options, - totalCount, - }; + const queryString = rison.encode(params); + const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; + const response = await SupersetClient.get({ endpoint }); - dataCache.current[cacheKey] = responseData; + const { result, next_cursor, has_more } = response.json; - return responseData; - } catch (error) { - logging.error('Failed to fetch Slack channels:', error); + if (next_cursor) { + cursorRef.current[`${search}:${page + 1}`] = next_cursor; + } - // Show user-friendly error message - if (addDangerToast && !hasShownErrorToast.current) { - addDangerToast( - t( - 'Unable to load Slack channels. Please check your Slack API token configuration. ' + - 'Switching to manual channel input.', - ), - ); - hasShownErrorToast.current = true; - } + const options = result.map((channel: SlackChannel) => ({ + label: channel.name, + value: channel.id, + })); + + const totalCount = has_more + ? (page + 1) * pageSize + 1 + : page * pageSize + options.length; + + const responseData = { + data: options, + totalCount, + }; + + dataCache.current[cacheKey] = responseData; + + return responseData; + } catch (error) { + logging.error('Failed to fetch Slack channels:', error); + + // Show user-friendly error message + if (addDangerToast && !hasShownErrorToast.current) { + addDangerToast( + t( + 'Unable to load Slack channels. Please check your Slack API token configuration. ' + + 'Switching to manual channel input.', + ), + ); + hasShownErrorToast.current = true; + } - // Fallback to Slack v1 without clearing recipients to prevent data loss - setUseSlackV1(true); + // Fallback to Slack v1 without clearing recipients to prevent data loss + setUseSlackV1(true); + + // Auto-switch to Slack V1 in the notification method dropdown + if ( + onUpdate && + setting && + setting.method === NotificationMethodOption.SlackV2 + ) { + onUpdate(index, { + ...setting, + method: NotificationMethodOption.Slack, // Switch from SlackV2 to Slack V1 + }); + } - // Auto-switch to Slack V1 in the notification method dropdown - if ( - onUpdate && - setting && - setting.method === NotificationMethodOption.SlackV2 - ) { - onUpdate(index, { - ...setting, - method: NotificationMethodOption.Slack, // Switch from SlackV2 to Slack V1 - // Keep recipients to preserve user data - }); + return { + data: [], + totalCount: 0, + }; } - - // Return empty result instead of throwing to allow UI to continue - return { - data: [], - totalCount: 0, - }; - } - }; + }, + // Note: searchGeneration is intentionally included even though not used in function body + // Purpose: Trigger function reference change when search changes (after debounce) + // Effect: Forces AsyncSelect to clear internal state and show fresh results + // eslint-disable-next-line react-hooks/exhaustive-deps + [addDangerToast, onUpdate, setting, index, searchGeneration], + ); const handleRefreshSlackChannels = async () => { setIsRefreshing(true); @@ -332,8 +350,6 @@ export const NotificationMethod: FunctionComponent = ({ // Fetch fresh channel list without clearing user selections await fetchSlackChannels('', 0, 100); - - setRefreshKey(prev => prev + 1); } catch (error) { logging.error('Error refreshing channels:', error); @@ -372,14 +388,12 @@ export const NotificationMethod: FunctionComponent = ({ const channel = channelData.data.find( ch => ch.value.toLowerCase() === id.toLowerCase(), ); - // If channel found, use its label; otherwise fallback to ID return channel || { label: id, value: id }; }) .filter(r => r.value); // Filter out empty values setSlackRecipients(mappedRecipients); } catch (error) { - // If fetching fails, use IDs as both label and value const recipientIds = recipients.split(',').map(id => id.trim()); const fallbackRecipients = recipientIds.map(id => ({ label: id, @@ -391,9 +405,29 @@ export const NotificationMethod: FunctionComponent = ({ }; initializeSlackRecipients(); + // Note: fetchSlackChannels and slackRecipients are intentionally omitted + // - fetchSlackChannels: Would cause re-initialization on every search change + // - slackRecipients: Would cause infinite loop (we modify it inside) // eslint-disable-next-line react-hooks/exhaustive-deps }, [method, recipients]); + const debouncedSearchUpdate = useMemo( + () => + debounce((search: string) => { + const trimmedSearch = search.trim(); + if (lastSearchValueRef.current !== trimmedSearch) { + lastSearchValueRef.current = trimmedSearch; + setSearchGeneration(prev => prev + 1); + } + }, 500), + [], + ); + + useEffect( + () => () => debouncedSearchUpdate.cancel(), + [debouncedSearchUpdate], + ); + const methodOptions = useMemo( () => (options || []) @@ -451,6 +485,10 @@ export const NotificationMethod: FunctionComponent = ({ } }; + const handleSlackSearch = (search: string) => { + debouncedSearchUpdate(search); + }; + const onSubjectChange = ( event: ChangeEvent, ) => { @@ -606,18 +644,20 @@ export const NotificationMethod: FunctionComponent = ({ // for SlackV2
true} /> None: + """ + Celery task to warm up the Slack channels cache. + + This task fetches all Slack channels using pagination and stores them in cache. + Useful for large workspaces where the initial channel fetch can be slow. + """ cache_timeout = current_app.config["SLACK_CACHE_TIMEOUT"] retry_count = current_app.config.get("SLACK_API_RATE_LIMIT_RETRY_COUNT", 2) @@ -37,7 +44,45 @@ def cache_channels() -> None: ) try: - get_channels(force=True, cache_timeout=cache_timeout) + all_channels = [] + cursor: Optional[str] = None + page_count = 0 + + while True: + page_count += 1 + + result = get_channels_with_search( + search_string="", + types=list(SlackChannelTypes), + cursor=cursor, + limit=1000, + ) + + page_channels = result["result"] + all_channels.extend(page_channels) + + logger.debug( + "Fetched page %d: %d channels (total: %d)", + page_count, + len(page_channels), + len(all_channels), + ) + + cursor = result.get("next_cursor") + if not cursor or not result.get("has_more"): + break + + logger.info( + "Successfully fetched %d Slack channels in %d pages. Caching results.", + len(all_channels), + page_count, + ) + + cache_key = "slack_conversations_list" + cache_manager.cache.set(cache_key, all_channels, timeout=cache_timeout) + + logger.info("Slack channels cache warm-up completed successfully") + except Exception as ex: logger.exception( "Failed to cache Slack channels: %s. " diff --git a/superset/utils/slack.py b/superset/utils/slack.py index 970cc06c74b6..8d6142a8df06 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -26,9 +26,7 @@ from superset import feature_flag_manager from superset.exceptions import SupersetException -from superset.extensions import cache_manager from superset.reports.schemas import SlackChannelSchema -from superset.utils import cache as cache_util from superset.utils.backports import StrEnum logger = logging.getLogger(__name__) @@ -58,64 +56,6 @@ def get_slack_client() -> WebClient: return client -@cache_util.memoized_func( - key="slack_conversations_list", - cache=cache_manager.cache, -) -def get_channels() -> list[SlackChannelSchema]: - """ - Retrieves a list of all conversations accessible by the bot - from the Slack API, and caches results (to avoid rate limits). - - The Slack API does not provide search so to apply a search use - get_channels_with_search instead. - """ - client = get_slack_client() - channel_schema = SlackChannelSchema() - channels: list[SlackChannelSchema] = [] - extra_params = {"types": ",".join(SlackChannelTypes)} - cursor = None - page_count = 0 - - logger.info("Starting Slack channels fetch") - - try: - while True: - page_count += 1 - - response = client.conversations_list( - limit=999, cursor=cursor, exclude_archived=True, **extra_params - ) - page_channels = response.data["channels"] - channels.extend(channel_schema.load(channel) for channel in page_channels) - - logger.debug( - "Fetched page %d: %d channels (total: %d)", - page_count, - len(page_channels), - len(channels), - ) - - cursor = response.data.get("response_metadata", {}).get("next_cursor") - if not cursor: - break - - logger.info( - "Successfully fetched %d Slack channels in %d pages", - len(channels), - page_count, - ) - return channels - except SlackApiError as ex: - logger.error( - "Failed to fetch Slack channels after %d pages: %s", - page_count, - str(ex), - exc_info=True, - ) - raise - - def _fetch_channels_without_search( client: WebClient, channel_schema: SlackChannelSchema, @@ -126,7 +66,7 @@ def _fetch_channels_without_search( """Fetch channels without search filtering, paginating for large limits.""" channels: list[SlackChannelSchema] = [] slack_cursor = cursor - page_size = min(limit, 200) + page_size = min(limit, 1000) while True: response = client.conversations_list( @@ -165,13 +105,13 @@ def _fetch_channels_with_search( """Fetch channels with search filtering, streaming through pages.""" matches: list[SlackChannelSchema] = [] slack_cursor = cursor - max_pages_to_fetch = 50 - pages_fetched = 0 - search_string_lower = search_string.lower() + search_terms = [ + term.strip().lower() for term in search_string.split(",") if term.strip() + ] - while len(matches) < limit and pages_fetched < max_pages_to_fetch: + while len(matches) < limit: response = client.conversations_list( - limit=200, + limit=1000, cursor=slack_cursor, exclude_archived=True, types=types_param, @@ -182,16 +122,21 @@ def _fetch_channels_with_search( channel_id_lower = channel_data["id"].lower() is_match = False - if exact_match: - is_match = ( - search_string_lower == channel_name_lower - or search_string_lower == channel_id_lower - ) - else: - is_match = ( - search_string_lower in channel_name_lower - or search_string_lower in channel_id_lower - ) + for search_term in search_terms: + if exact_match: + if ( + search_term == channel_name_lower + or search_term == channel_id_lower + ): + is_match = True + break + else: + if ( + search_term in channel_name_lower + or search_term in channel_id_lower + ): + is_match = True + break if is_match: channel = channel_schema.load(channel_data) @@ -204,8 +149,6 @@ def _fetch_channels_with_search( if not slack_cursor: break - pages_fetched += 1 - has_more = bool(slack_cursor) and len(matches) >= limit return { @@ -225,6 +168,15 @@ def get_channels_with_search( """ Fetches Slack channels with pagination and search support. + Args: + search_string: Search term(s). Can be comma-separated for OR logic. + e.g., "engineering,marketing" matches channels containing + "engineering" OR "marketing" + types: Channel types to filter (public_channel, private_channel) + exact_match: If True, search term must exactly match channel name/ID + cursor: Pagination cursor for fetching next page + limit: Maximum number of channels to return + Returns a dict with: - result: list of channels - next_cursor: cursor for next page (None if no more pages) @@ -234,7 +186,6 @@ def get_channels_with_search( We handle two cases: 1. WITHOUT search: Fetch single page from Slack API 2. WITH search: Stream through Slack API pages until we find enough matches - (stops early to prevent timeouts on large workspaces) """ try: client = get_slack_client() diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index 46bfbea43d86..cc1527a8a502 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -429,30 +429,6 @@ def test_streaming_search_pagination(self, mocker): "has_more": False, } - def test_streaming_search_max_pages_safety_limit(self, mocker): - """Test streaming search stops after 50 pages to prevent runaway requests""" - - # Create a response that always has a next cursor (infinite pagination) - mock_data = { - "channels": [ - {"name": "channel", "id": "C1", "is_private": False, "is_member": True}, - ], - "response_metadata": {"next_cursor": "next_page"}, - } - mock_response = MockResponse(mock_data) - - mock_client = mocker.Mock() - mock_client.conversations_list.return_value = mock_response - mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) - - # Search that matches the channel - should stop at 50 pages - result = get_channels_with_search(search_string="channel", limit=100) - - # Should have called conversations_list exactly 50 times (max pages) - assert mock_client.conversations_list.call_count == 50 - # Should return the matches found (50 channels, one per page) - assert len(result["result"]) == 50 - def test_search_with_no_matches(self, mocker): """Test search that finds no matching channels""" @@ -666,9 +642,9 @@ def mock_conversations_list(**kwargs): assert len(result["result"]) == 50 assert result["has_more"] is True - def test_non_search_pagination_over_200_limit(self, mocker): - """Test non-search queries paginate correctly for limits > 200""" - # Create 500 channels + def test_non_search_pagination_over_1000_limit(self, mocker): + """Test non-search queries paginate correctly for limits > 1000""" + # Create 2500 channels all_channels = [ { "name": f"channel-{i}", @@ -676,7 +652,7 @@ def test_non_search_pagination_over_200_limit(self, mocker): "is_private": False, "is_member": True, } - for i in range(500) + for i in range(2500) ] call_count = 0 @@ -686,18 +662,18 @@ def mock_conversations_list(**kwargs): limit = kwargs.get("limit", 100) cursor = kwargs.get("cursor") - # Simulate Slack API pagination (max 200 per page) + # Simulate Slack API pagination (max 1000 per page) if cursor is None: start = 0 - elif cursor == "cursor_200": - start = 200 - elif cursor == "cursor_400": - start = 400 + elif cursor == "cursor_1000": + start = 1000 + elif cursor == "cursor_2000": + start = 2000 else: - start = 600 + start = 3000 - end = min(start + limit, 500) - next_cursor = f"cursor_{end}" if end < 500 else None + end = min(start + limit, 2500) + next_cursor = f"cursor_{end}" if end < 2500 else None call_count += 1 return MockResponse( @@ -711,13 +687,13 @@ def mock_conversations_list(**kwargs): mock_client.conversations_list.side_effect = mock_conversations_list mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) - # Request 300 channels (requires 2 pages of 200 each) - result = get_channels_with_search(limit=300) + # Request 1500 channels (requires 2 pages of 1000 each) + result = get_channels_with_search(limit=1500) - # Should return exactly 300 channels - assert len(result["result"]) == 300 + # Should return exactly 1500 channels + assert len(result["result"]) == 1500 assert result["has_more"] is True - assert result["next_cursor"] == "cursor_400" + assert result["next_cursor"] == "cursor_2000" # Should have made 2 API calls assert call_count == 2 @@ -801,3 +777,153 @@ def test_non_search_empty_result_handling(self, mocker): assert len(result["result"]) == 0 assert result["has_more"] is False assert result["next_cursor"] is None + + def test_comma_separated_search_strings(self, mocker): + """Test search with comma-separated search strings (OR logic)""" + mock_data = { + "channels": [ + { + "name": "engineering", + "id": "C1", + "is_private": False, + "is_member": True, + }, + { + "name": "marketing", + "id": "C2", + "is_private": False, + "is_member": True, + }, + { + "name": "sales", + "id": "C3", + "is_private": False, + "is_member": True, + }, + { + "name": "design", + "id": "C4", + "is_private": False, + "is_member": True, + }, + { + "name": "general", + "id": "C5", + "is_private": False, + "is_member": True, + }, + ], + "response_metadata": {"next_cursor": None}, + } + + mock_response = MockResponse(mock_data) + mock_client = mocker.Mock() + mock_client.conversations_list.return_value = mock_response + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + # Search for "engineering,marketing,sales" + result = get_channels_with_search( + search_string="engineering,marketing,sales", limit=100 + ) + + # Should match 3 channels: engineering, marketing, sales + assert len(result["result"]) == 3 + channel_names = [channel["name"] for channel in result["result"]] + assert "engineering" in channel_names + assert "marketing" in channel_names + assert "sales" in channel_names + assert "design" not in channel_names + assert "general" not in channel_names + + def test_comma_separated_search_with_whitespace(self, mocker): + """Test comma-separated search handles extra whitespace correctly""" + mock_data = { + "channels": [ + { + "name": "engineering-team", + "id": "C1", + "is_private": False, + "is_member": True, + }, + { + "name": "marketing-ops", + "id": "C2", + "is_private": False, + "is_member": True, + }, + { + "name": "general", + "id": "C3", + "is_private": False, + "is_member": True, + }, + ], + "response_metadata": {"next_cursor": None}, + } + + mock_response = MockResponse(mock_data) + mock_client = mocker.Mock() + mock_client.conversations_list.return_value = mock_response + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + # Search with extra whitespace: " engineering , marketing " + result = get_channels_with_search( + search_string=" engineering , marketing ", limit=100 + ) + + # Should match 2 channels, whitespace should be stripped + assert len(result["result"]) == 2 + channel_names = [channel["name"] for channel in result["result"]] + assert "engineering-team" in channel_names + assert "marketing-ops" in channel_names + assert "general" not in channel_names + + def test_comma_separated_exact_match(self, mocker): + """Test comma-separated search with exact_match=True""" + mock_data = { + "channels": [ + { + "name": "engineering", + "id": "C1", + "is_private": False, + "is_member": True, + }, + { + "name": "engineering-team", + "id": "C2", + "is_private": False, + "is_member": True, + }, + { + "name": "sales", + "id": "C3", + "is_private": False, + "is_member": True, + }, + { + "name": "general", + "id": "C4", + "is_private": False, + "is_member": True, + }, + ], + "response_metadata": {"next_cursor": None}, + } + + mock_response = MockResponse(mock_data) + mock_client = mocker.Mock() + mock_client.conversations_list.return_value = mock_response + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + # Exact match search for "engineering,sales" + result = get_channels_with_search( + search_string="engineering,sales", exact_match=True, limit=100 + ) + + # Should match only exact names: engineering and sales (not engineering-team) + assert len(result["result"]) == 2 + channel_names = [channel["name"] for channel in result["result"]] + assert "engineering" in channel_names + assert "sales" in channel_names + assert "engineering-team" not in channel_names + assert "general" not in channel_names From 805c2c608405a6909430b73ab461abee7979e57d Mon Sep 17 00:00:00 2001 From: Gabriel Torres Ruiz Date: Mon, 10 Nov 2025 23:23:55 -0400 Subject: [PATCH 6/7] fix(alerts): adding cache architecture --- .../src/features/alerts/AlertReportModal.tsx | 4 +- .../alerts/components/NotificationMethod.tsx | 134 ++------- .../alerts/hooks/useSlackChannels.test.ts | 280 ++++++++++++++++++ .../features/alerts/hooks/useSlackChannels.ts | 183 ++++++++++++ superset/config.py | 17 ++ superset/tasks/slack.py | 68 ++++- superset/utils/slack.py | 254 ++++++++++++++-- tests/unit_tests/utils/slack_test.py | 165 +++++++++++ 8 files changed, 959 insertions(+), 146 deletions(-) create mode 100644 superset-frontend/src/features/alerts/hooks/useSlackChannels.test.ts create mode 100644 superset-frontend/src/features/alerts/hooks/useSlackChannels.ts diff --git a/superset-frontend/src/features/alerts/AlertReportModal.tsx b/superset-frontend/src/features/alerts/AlertReportModal.tsx index 60cc94dd2113..b982e3aac3b8 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx @@ -2587,9 +2587,7 @@ const AlertReportModal: FunctionComponent = ({ children: ( <> {notificationSettings.map((notificationSetting, i) => ( - + ` @@ -190,19 +179,16 @@ export const NotificationMethod: FunctionComponent = ({ const [ccValue, setCcValue] = useState(cc || ''); const [bccValue, setBccValue] = useState(bcc || ''); const [useSlackV1, setUseSlackV1] = useState(false); - const cursorRef = useRef>({}); - const dataCache = useRef< - Record< - string, - { data: { label: string; value: string }[]; totalCount: number } - > - >({}); - const forceRefreshRef = useRef(false); - const [isRefreshing, setIsRefreshing] = useState(false); const hasShownErrorToast = useRef(false); const [searchGeneration, setSearchGeneration] = useState(0); const lastSearchValueRef = useRef(''); + const { + fetchChannels: fetchSlackChannelsFromHook, + refreshChannels, + isRefreshing, + } = useSlackChannels(addDangerToast); + const onMethodChange = (selected: { label: string; value: NotificationMethodOption; @@ -234,69 +220,11 @@ export const NotificationMethod: FunctionComponent = ({ totalCount: number; }> => { try { - const cacheKey = `${search}:${page}`; - - if (dataCache.current[cacheKey] && !forceRefreshRef.current) { - return dataCache.current[cacheKey]; - } - - // Clear cache for previous searches to prevent stale data - if (page === 0) { - Object.keys(dataCache.current).forEach(key => { - if (!key.startsWith(`${search}:`)) { - delete dataCache.current[key]; - delete cursorRef.current[key]; - } - }); - } - - const cursor = page > 0 ? cursorRef.current[cacheKey] : null; - - const params: Record = { - types: ['public_channel', 'private_channel'], - limit: pageSize, - }; - - if (page === 0 && forceRefreshRef.current) { - params.force = true; - forceRefreshRef.current = false; - } - - if (search) { - params.search_string = search; - } - - if (cursor) { - params.cursor = cursor; - } - - const queryString = rison.encode(params); - const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; - const response = await SupersetClient.get({ endpoint }); - - const { result, next_cursor, has_more } = response.json; - - if (next_cursor) { - cursorRef.current[`${search}:${page + 1}`] = next_cursor; - } - - const options = result.map((channel: SlackChannel) => ({ - label: channel.name, - value: channel.id, - })); + const result = await fetchSlackChannelsFromHook(search, page, pageSize); - const totalCount = has_more - ? (page + 1) * pageSize + 1 - : page * pageSize + options.length; + hasShownErrorToast.current = false; - const responseData = { - data: options, - totalCount, - }; - - dataCache.current[cacheKey] = responseData; - - return responseData; + return result; } catch (error) { logging.error('Failed to fetch Slack channels:', error); @@ -336,33 +264,17 @@ export const NotificationMethod: FunctionComponent = ({ // Purpose: Trigger function reference change when search changes (after debounce) // Effect: Forces AsyncSelect to clear internal state and show fresh results // eslint-disable-next-line react-hooks/exhaustive-deps - [addDangerToast, onUpdate, setting, index, searchGeneration], + [ + fetchSlackChannelsFromHook, + addDangerToast, + onUpdate, + setting, + index, + searchGeneration, + ], ); - const handleRefreshSlackChannels = async () => { - setIsRefreshing(true); - - try { - // Clear cache and cursor to force fresh data fetch - forceRefreshRef.current = true; - cursorRef.current = {}; - dataCache.current = {}; - - // Fetch fresh channel list without clearing user selections - await fetchSlackChannels('', 0, 100); - } catch (error) { - logging.error('Error refreshing channels:', error); - - // Show error toast if available - if (addDangerToast) { - addDangerToast( - t('Failed to refresh Slack channels. Please try again.'), - ); - } - } finally { - setIsRefreshing(false); - } - }; + const handleRefreshSlackChannels = refreshChannels; // Reset error toast flag when method changes useEffect(() => { @@ -405,10 +317,6 @@ export const NotificationMethod: FunctionComponent = ({ }; initializeSlackRecipients(); - // Note: fetchSlackChannels and slackRecipients are intentionally omitted - // - fetchSlackChannels: Would cause re-initialization on every search change - // - slackRecipients: Would cause infinite loop (we modify it inside) - // eslint-disable-next-line react-hooks/exhaustive-deps }, [method, recipients]); const debouncedSearchUpdate = useMemo( diff --git a/superset-frontend/src/features/alerts/hooks/useSlackChannels.test.ts b/superset-frontend/src/features/alerts/hooks/useSlackChannels.test.ts new file mode 100644 index 000000000000..c2cb35465c2e --- /dev/null +++ b/superset-frontend/src/features/alerts/hooks/useSlackChannels.test.ts @@ -0,0 +1,280 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { renderHook } from '@testing-library/react-hooks'; +import { SupersetClient } from '@superset-ui/core'; +import { useSlackChannels } from './useSlackChannels'; + +// Mock SupersetClient +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + SupersetClient: { + get: jest.fn(), + }, + logging: { + error: jest.fn(), + }, +})); + +const mockChannels = [ + { id: 'C001', name: 'general', is_private: false }, + { id: 'C002', name: 'engineering', is_private: false }, + { id: 'C003', name: 'random', is_private: false }, +]; + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('fetches channels successfully', async () => { + (SupersetClient.get as jest.Mock).mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + }); + + const { result } = renderHook(() => useSlackChannels()); + + const channels = await result.current.fetchChannels('', 0, 100); + + expect(channels.data).toHaveLength(3); + expect(channels.data[0]).toEqual({ label: 'general', value: 'C001' }); + expect(channels.totalCount).toBe(3); +}); + +test('caches results and avoids duplicate requests', async () => { + (SupersetClient.get as jest.Mock).mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + }); + + const { result } = renderHook(() => useSlackChannels()); + + // First call + await result.current.fetchChannels('', 0, 100); + + // Second call with same parameters + await result.current.fetchChannels('', 0, 100); + + // Should only call API once + expect(SupersetClient.get).toHaveBeenCalledTimes(1); +}); + +test('deduplicates concurrent requests', async () => { + (SupersetClient.get as jest.Mock).mockImplementation( + () => + new Promise(resolve => + setTimeout( + () => + resolve({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + }), + 100, + ), + ), + ); + + const { result } = renderHook(() => useSlackChannels()); + + // Make concurrent requests + const promise1 = result.current.fetchChannels('', 0, 100); + const promise2 = result.current.fetchChannels('', 0, 100); + + const [result1, result2] = await Promise.all([promise1, promise2]); + + // Should return same data + expect(result1).toEqual(result2); + + // Should only call API once + expect(SupersetClient.get).toHaveBeenCalledTimes(1); +}); + +test('clears cache when search changes', async () => { + (SupersetClient.get as jest.Mock).mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + }); + + const { result } = renderHook(() => useSlackChannels()); + + // First search + await result.current.fetchChannels('eng', 0, 100); + + // Different search + await result.current.fetchChannels('gen', 0, 100); + + // Should call API twice (once for each search) + expect(SupersetClient.get).toHaveBeenCalledTimes(2); +}); + +test('handles pagination with cursors', async () => { + (SupersetClient.get as jest.Mock) + .mockResolvedValueOnce({ + json: { + result: mockChannels.slice(0, 2), + next_cursor: 'cursor_page_2', + has_more: true, + }, + }) + .mockResolvedValueOnce({ + json: { + result: mockChannels.slice(2), + next_cursor: null, + has_more: false, + }, + }); + + const { result } = renderHook(() => useSlackChannels()); + + // First page + const page1 = await result.current.fetchChannels('', 0, 2); + expect(page1.data).toHaveLength(2); + expect(page1.has_more).toBe(true); + + // Second page + const page2 = await result.current.fetchChannels('', 1, 2); + expect(page2.data).toHaveLength(1); +}); + +test('refreshChannels clears cache and refetches', async () => { + (SupersetClient.get as jest.Mock).mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + }); + + const { result } = renderHook(() => useSlackChannels()); + + // Initial fetch + await result.current.fetchChannels('', 0, 100); + + // Cache hit - no new API call + await result.current.fetchChannels('', 0, 100); + expect(SupersetClient.get).toHaveBeenCalledTimes(1); + + // Refresh + await result.current.refreshChannels(); + + // Should call API again + expect(SupersetClient.get).toHaveBeenCalledTimes(2); +}); + +test('isRefreshing state updates correctly', async () => { + (SupersetClient.get as jest.Mock).mockResolvedValue({ + json: { + result: mockChannels, + next_cursor: null, + has_more: false, + }, + }); + + const { result } = renderHook(() => useSlackChannels()); + + expect(result.current.isRefreshing).toBe(false); + + await result.current.refreshChannels(); + + // Should be false after refresh completes + expect(result.current.isRefreshing).toBe(false); +}); + +test('calls onError callback when fetch fails', async () => { + (SupersetClient.get as jest.Mock).mockRejectedValue(new Error('API Error')); + + const onError = jest.fn(); + const { result } = renderHook(() => useSlackChannels(onError)); + + await result.current.fetchChannels('', 0, 100); + + expect(onError).toHaveBeenCalledWith( + expect.stringContaining('Unable to load Slack channels'), + ); +}); + +test('returns empty data on error', async () => { + (SupersetClient.get as jest.Mock).mockRejectedValue(new Error('API Error')); + + const { result } = renderHook(() => useSlackChannels()); + + const channels = await result.current.fetchChannels('', 0, 100); + + expect(channels.data).toEqual([]); + expect(channels.totalCount).toBe(0); +}); + +test('includes search_string in API params when provided', async () => { + (SupersetClient.get as jest.Mock).mockResolvedValue({ + json: { + result: [], + next_cursor: null, + has_more: false, + }, + }); + + const { result } = renderHook(() => useSlackChannels()); + + await result.current.fetchChannels('engineering', 0, 100); + + expect(SupersetClient.get).toHaveBeenCalledWith({ + endpoint: expect.stringContaining('search_string'), + }); +}); + +test('includes cursor in API params for pagination', async () => { + (SupersetClient.get as jest.Mock) + .mockResolvedValueOnce({ + json: { + result: mockChannels.slice(0, 2), + next_cursor: 'test_cursor', + has_more: true, + }, + }) + .mockResolvedValueOnce({ + json: { + result: mockChannels.slice(2), + next_cursor: null, + has_more: false, + }, + }); + + const { result } = renderHook(() => useSlackChannels()); + + // First page + await result.current.fetchChannels('', 0, 2); + + // Second page + await result.current.fetchChannels('', 1, 2); + + expect(SupersetClient.get).toHaveBeenCalledWith({ + endpoint: expect.stringContaining('cursor'), + }); +}); diff --git a/superset-frontend/src/features/alerts/hooks/useSlackChannels.ts b/superset-frontend/src/features/alerts/hooks/useSlackChannels.ts new file mode 100644 index 000000000000..4e911a4a4aa9 --- /dev/null +++ b/superset-frontend/src/features/alerts/hooks/useSlackChannels.ts @@ -0,0 +1,183 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useCallback, useRef, useState } from 'react'; +import { logging, SupersetClient, t } from '@superset-ui/core'; +import rison from 'rison'; +import { SlackChannel } from '../types'; + +export interface SlackChannelOption { + label: string; + value: string; +} + +export interface SlackChannelsResult { + data: SlackChannelOption[]; + totalCount: number; + has_more?: boolean; + next_cursor?: string | null; +} + +export interface UseSlackChannelsResult { + fetchChannels: ( + search: string, + page: number, + pageSize: number, + ) => Promise; + refreshChannels: () => Promise; + isRefreshing: boolean; +} + +/** + * Cache types for managing Slack channel data + */ +type CursorCache = Record; +type DataCache = Record; +type PendingRequestsCache = Record>; + +/** + * Custom hook for managing Slack channels with caching and pagination + */ +export function useSlackChannels( + onError?: (message: string) => void, +): UseSlackChannelsResult { + const cursorRef = useRef({}); + const dataCache = useRef({}); + const pendingRequests = useRef({}); + const [isRefreshing, setIsRefreshing] = useState(false); + + const fetchChannels = useCallback( + async ( + search: string, + page: number, + pageSize: number, + ): Promise => { + const cacheKey = `${search}:${page}`; + + if (dataCache.current[cacheKey]) { + return dataCache.current[cacheKey]; + } + + if (cacheKey in pendingRequests.current) { + return pendingRequests.current[cacheKey]; + } + + const cursor = page > 0 ? cursorRef.current[cacheKey] : null; + + const params: Record = { + types: ['public_channel', 'private_channel'], + limit: pageSize, + }; + + if (search) { + params.search_string = search; + } + + if (cursor) { + params.cursor = cursor; + } + + const queryString = rison.encode(params); + const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; + + const fetchPromise = (async () => { + try { + const response = await SupersetClient.get({ endpoint }); + + const { + result, + next_cursor: nextCursor, + has_more: hasMore, + } = response.json; + + if (nextCursor) { + cursorRef.current[`${search}:${page + 1}`] = nextCursor; + } + + const options = result.map((channel: SlackChannel) => ({ + label: channel.name, + value: channel.id, + })); + + const totalCount = hasMore + ? (page + 1) * pageSize + 1 + : page * pageSize + options.length; + + const responseData = { + data: options, + totalCount, + has_more: hasMore, + next_cursor: nextCursor, + }; + + dataCache.current[cacheKey] = responseData; + + return responseData; + } catch (error) { + logging.error('Failed to fetch Slack channels:', error); + + if (onError) { + onError( + t( + 'Unable to load Slack channels. Please check your Slack API token configuration.', + ), + ); + } + + return { + data: [], + totalCount: 0, + }; + } finally { + delete pendingRequests.current[cacheKey]; + } + })(); + + pendingRequests.current[cacheKey] = fetchPromise; + + return fetchPromise; + }, + [onError], + ); + + const refreshChannels = useCallback(async () => { + setIsRefreshing(true); + + try { + cursorRef.current = {}; + dataCache.current = {}; + pendingRequests.current = {}; + + await fetchChannels('', 0, 100); + } catch (error) { + logging.error('Error refreshing channels:', error); + + if (onError) { + onError(t('Failed to refresh Slack channels. Please try again.')); + } + } finally { + setIsRefreshing(false); + } + }, [fetchChannels, onError]); + + return { + fetchChannels, + refreshChannels, + isRefreshing, + }; +} diff --git a/superset/config.py b/superset/config.py index 4912fc4d954e..1ba1cbcd3e10 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1749,6 +1749,23 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument # noq # For workspaces with 10k+ channels, consider increasing to 10 SLACK_API_RATE_LIMIT_RETRY_COUNT = 2 +# Enable/disable caching for Slack channels list +# When enabled, all channels are cached in memory for fast lookups +# Disable this for very large workspaces (50k+ channels) to reduce memory usage +# Default: True (caching enabled) +SLACK_ENABLE_CACHING: bool = True + +# Maximum number of channels to cache during warmup +# Prevents excessive memory usage in very large workspaces +# The cache warmup task will stop after reaching this limit +# Default: 20000 channels (~100MB cache size) +SLACK_CACHE_MAX_CHANNELS: int = 20000 + +# Celery task timeout for Slack cache warmup (seconds) +# Prevents runaway cache warmup tasks in extremely large workspaces +# Default: 300 seconds (5 minutes) +SLACK_CACHE_WARMUP_TIMEOUT: int = 300 + # The webdriver to use for generating reports. Use one of the following # firefox # Requires: geckodriver and firefox installations diff --git a/superset/tasks/slack.py b/superset/tasks/slack.py index b24a6faf4188..fb3699bf9153 100644 --- a/superset/tasks/slack.py +++ b/superset/tasks/slack.py @@ -20,25 +20,49 @@ from flask import current_app from superset.extensions import cache_manager, celery_app -from superset.utils.slack import get_channels_with_search, SlackChannelTypes +from superset.utils.slack import ( + get_channels_with_search, + SLACK_CHANNELS_CACHE_KEY, + SLACK_CHANNELS_CONTINUATION_CURSOR_KEY, + SlackChannelTypes, +) logger = logging.getLogger(__name__) -@celery_app.task(name="slack.cache_channels") +@celery_app.task( + name="slack.cache_channels", + time_limit=300, # 5 minute hard timeout (via SLACK_CACHE_WARMUP_TIMEOUT) + soft_time_limit=240, # 4 minute warning +) def cache_channels() -> None: """ Celery task to warm up the Slack channels cache. This task fetches all Slack channels using pagination and stores them in cache. - Useful for large workspaces where the initial channel fetch can be slow. + Includes safeguards for very large workspaces (50k+ channels). + + Respects the following config variables: + - SLACK_ENABLE_CACHING: If False, this task does nothing + - SLACK_CACHE_MAX_CHANNELS: Maximum channels to cache (prevents runaway fetches) + - SLACK_CACHE_WARMUP_TIMEOUT: Task timeout in seconds + - SLACK_CACHE_TIMEOUT: How long to keep cached data """ + enable_caching = current_app.config.get("SLACK_ENABLE_CACHING", True) + if not enable_caching: + logger.info( + "Slack caching disabled (SLACK_ENABLE_CACHING=False), skipping cache warmup" + ) + return + cache_timeout = current_app.config["SLACK_CACHE_TIMEOUT"] + max_channels = current_app.config.get("SLACK_CACHE_MAX_CHANNELS", 20000) retry_count = current_app.config.get("SLACK_API_RATE_LIMIT_RETRY_COUNT", 2) logger.info( "Starting Slack channels cache warm-up task " - "(cache_timeout=%ds, retry_count=%d)", + "(max_channels=%d, cache_timeout=%ds, retry_count=%d)", + max_channels, cache_timeout, retry_count, ) @@ -69,17 +93,47 @@ def cache_channels() -> None: ) cursor = result.get("next_cursor") + + # Safety check: stop if we hit the max channel limit + if len(all_channels) >= max_channels: + # Store the continuation cursor so we can resume via API later + if cursor: + cache_manager.cache.set( + SLACK_CHANNELS_CONTINUATION_CURSOR_KEY, + cursor, + timeout=cache_timeout, + ) + logger.warning( + "Reached max channel limit (%d channels in %d pages). " + "Stored continuation cursor for API fallback. " + "Channels beyond limit will be fetched from API on demand.", + len(all_channels), + page_count, + ) + else: + logger.warning( + "Reached max channel limit (%d channels in %d pages). " + "No more channels available.", + len(all_channels), + page_count, + ) + break + if not cursor or not result.get("has_more"): break + cache_size_mb = len(str(all_channels)) / (1024 * 1024) logger.info( - "Successfully fetched %d Slack channels in %d pages. Caching results.", + "Successfully fetched %d Slack channels in %d pages (%.1f MB). " + "Caching results.", len(all_channels), page_count, + cache_size_mb, ) - cache_key = "slack_conversations_list" - cache_manager.cache.set(cache_key, all_channels, timeout=cache_timeout) + cache_manager.cache.set( + SLACK_CHANNELS_CACHE_KEY, all_channels, timeout=cache_timeout + ) logger.info("Slack channels cache warm-up completed successfully") diff --git a/superset/utils/slack.py b/superset/utils/slack.py index 8d6142a8df06..c05ce0854dfc 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -26,11 +26,17 @@ from superset import feature_flag_manager from superset.exceptions import SupersetException +from superset.extensions import cache_manager from superset.reports.schemas import SlackChannelSchema from superset.utils.backports import StrEnum logger = logging.getLogger(__name__) +SLACK_CHANNELS_CACHE_KEY = "slack_conversations_list" +SLACK_CHANNELS_CONTINUATION_CURSOR_KEY = ( + f"{SLACK_CHANNELS_CACHE_KEY}_continuation_cursor" +) + class SlackChannelTypes(StrEnum): PUBLIC = "public_channel" @@ -158,34 +164,149 @@ def _fetch_channels_with_search( } -def get_channels_with_search( - search_string: str = "", - types: Optional[list[SlackChannelTypes]] = None, - exact_match: bool = False, - cursor: Optional[str] = None, - limit: int = 100, -) -> dict[str, Any]: +def _fetch_from_cache( # noqa: C901 + cached_channels: list[SlackChannelSchema], + search_string: str, + types: Optional[list[SlackChannelTypes]], + exact_match: bool, + cursor: Optional[str], + limit: int, +) -> Optional[dict[str, Any]]: """ - Fetches Slack channels with pagination and search support. + Fetch channels from cache with in-memory filtering and pagination. + + This is the fast path - operates entirely in-memory on cached data. + Used when cache is available and warm. Args: - search_string: Search term(s). Can be comma-separated for OR logic. - e.g., "engineering,marketing" matches channels containing - "engineering" OR "marketing" - types: Channel types to filter (public_channel, private_channel) - exact_match: If True, search term must exactly match channel name/ID - cursor: Pagination cursor for fetching next page - limit: Maximum number of channels to return + cached_channels: Complete list of all cached channels + search_string: Search term(s) for filtering + types: Channel types to filter + exact_match: If True, search term must exactly match + cursor: Cache pagination cursor (format: "cache:N") + limit: Maximum channels to return + + Returns: + Dict with filtered and paginated results, or None if pagination + exceeds cached data (signals need to fall back to API) + """ + # Start with all cached channels + channels = list(cached_channels) + + # Filter by channel types if specified + if types and len(types) < len(SlackChannelTypes): + type_set = set(types) + channels = [ + ch + for ch in channels + if ( + (SlackChannelTypes.PRIVATE in type_set and ch.get("is_private")) + or (SlackChannelTypes.PUBLIC in type_set and not ch.get("is_private")) + ) + ] - Returns a dict with: - - result: list of channels - - next_cursor: cursor for next page (None if no more pages) - - has_more: boolean indicating if more pages exist + # Filter by search string with comma-separated OR logic + if search_string: + search_terms = [ + term.strip().lower() for term in search_string.split(",") if term.strip() + ] + filtered = [] - The Slack API is paginated but does not include search. - We handle two cases: - 1. WITHOUT search: Fetch single page from Slack API - 2. WITH search: Stream through Slack API pages until we find enough matches + for ch in channels: + channel_name_lower = ch.get("name", "").lower() + channel_id_lower = ch.get("id", "").lower() + is_match = False + + for search_term in search_terms: + if exact_match: + if ( + search_term == channel_name_lower + or search_term == channel_id_lower + ): + is_match = True + break + else: + if ( + search_term in channel_name_lower + or search_term in channel_id_lower + ): + is_match = True + break + + if is_match: + filtered.append(ch) + + channels = filtered + + # Calculate pagination offset from cursor + offset = 0 + if cursor and cursor.startswith("cache:"): + try: + offset = int(cursor.split(":", 1)[1]) + except (ValueError, IndexError): + offset = 0 + + # Check if we're trying to paginate beyond cached data + # This happens when cache is partial (hit SLACK_CACHE_MAX_CHANNELS limit) + if offset >= len(channels): + logger.info( + "Pagination offset (%d) exceeds cached data (%d channels). " + "Falling back to API.", + offset, + len(channels), + ) + return None + + # Paginate in memory + page = channels[offset : offset + limit] + next_offset = offset + limit + has_more = next_offset < len(channels) + + # Check if we have a continuation cursor (cache was truncated) + continuation_cursor = cache_manager.cache.get( + SLACK_CHANNELS_CONTINUATION_CURSOR_KEY + ) + + # If we've reached the end of cached data but there's a continuation cursor, + # signal that more data exists via API + next_cursor: Optional[str] + if not has_more and continuation_cursor: + # Return special cursor that signals transition to API + next_cursor = "api:continue" + has_more = True + else: + # Use synthetic cursor for cache pagination + next_cursor = f"cache:{next_offset}" if has_more else None + + return { + "result": page, + "next_cursor": next_cursor, + "has_more": has_more, + } + + +def _fetch_from_api( + search_string: str, + types: Optional[list[SlackChannelTypes]], + exact_match: bool, + cursor: Optional[str], + limit: int, +) -> dict[str, Any]: + """ + Fetch from Slack API with pagination. + + This is the original pagination implementation extracted into a helper. + Used when cache is cold or disabled. + + Args: + search_string: Search term(s) for filtering + types: Channel types to filter + exact_match: If True, search term must exactly match + cursor: Real Slack API cursor for pagination + limit: Maximum channels to return + + Returns: + Dict with results from Slack API """ try: client = get_slack_client() @@ -226,6 +347,93 @@ def get_channels_with_search( raise SupersetException(f"Failed to list channels: {ex}") from ex +def get_channels_with_search( + search_string: str = "", + types: Optional[list[SlackChannelTypes]] = None, + exact_match: bool = False, + cursor: Optional[str] = None, + limit: int = 100, +) -> dict[str, Any]: + """ + Fetches Slack channels with pagination and search support. + + Args: + search_string: Search term(s). Can be comma-separated for OR logic. + e.g., "engineering,marketing" matches channels containing + "engineering" OR "marketing" + types: Channel types to filter (public_channel, private_channel) + exact_match: If True, search term must exactly match channel name/ID + cursor: Pagination cursor (format: "cache:N" for cache path, + "api:continue" for transitioning to API continuation, + base64 string for API path, None for first page) + limit: Maximum number of channels to return per page + + Returns a dict with: + - result: list of channels for this page + - next_cursor: cursor for next page (None if no more pages) + - has_more: boolean indicating if more pages exist + """ + enable_caching = app.config.get("SLACK_ENABLE_CACHING", True) + + # Handle transition from cache to API continuation + if cursor == "api:continue": + continuation_cursor = cache_manager.cache.get( + SLACK_CHANNELS_CONTINUATION_CURSOR_KEY + ) + if continuation_cursor: + logger.info("Transitioning from cache to API using continuation cursor") + return _fetch_from_api( + search_string, types, exact_match, continuation_cursor, limit + ) + else: + logger.warning( + "No continuation cursor available, cannot fetch more channels" + ) + return {"result": [], "next_cursor": None, "has_more": False} + + # Try cache path for first page or cache cursor + if enable_caching and (cursor is None or (cursor and cursor.startswith("cache:"))): + cached_channels = cache_manager.cache.get(SLACK_CHANNELS_CACHE_KEY) + if cached_channels: + if cursor is None: + logger.info( + "Using cache path (%d channels available)", len(cached_channels) + ) + + result = _fetch_from_cache( + cached_channels, search_string, types, exact_match, cursor, limit + ) + + # If cache returns None, we've exceeded cache boundary + if result is None: + # Fall back to API using continuation cursor + continuation_cursor = cache_manager.cache.get( + SLACK_CHANNELS_CONTINUATION_CURSOR_KEY + ) + if continuation_cursor: + logger.info( + "Cache boundary exceeded, falling back to API " + "with continuation cursor" + ) + return _fetch_from_api( + search_string, types, exact_match, continuation_cursor, limit + ) + else: + logger.warning( + "Cache boundary exceeded but no continuation cursor available" + ) + return {"result": [], "next_cursor": None, "has_more": False} + + return result + else: + logger.debug("Cache not available, using API path") + cursor = None # Reset cursor for API call + + # API path for real Slack cursors + logger.debug("Using API path") + return _fetch_from_api(search_string, types, exact_match, cursor, limit) + + def should_use_v2_api() -> bool: if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"): return False diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index cc1527a8a502..d9c1d482ce29 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -927,3 +927,168 @@ def test_comma_separated_exact_match(self, mocker): assert "sales" in channel_names assert "engineering-team" not in channel_names assert "general" not in channel_names + + def test_cache_boundary_exceeded_fallback_to_api(self, mocker): + """Test fallback to API when pagination exceeds cached data""" + # Mock cached channels (only 100 channels cached) + cached_channels = [ + { + "name": f"channel-{i}", + "id": f"C{i}", + "is_private": False, + "is_member": True, + } + for i in range(100) + ] + + # Mock continuation channels from API + api_channels = [ + { + "name": f"channel-{i}", + "id": f"C{i}", + "is_private": False, + "is_member": True, + } + for i in range(100, 150) + ] + + def cache_get_side_effect(key): + if key == "slack_conversations_list": + return cached_channels + if key == "slack_conversations_list_continuation_cursor": + return "continuation_cursor_value" + return None + + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + side_effect=cache_get_side_effect, + ) + + mock_data = { + "channels": api_channels, + "response_metadata": {"next_cursor": None}, + } + + mock_response = MockResponse(mock_data) + mock_client = mocker.Mock() + mock_client.conversations_list.return_value = mock_response + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + # Try to paginate beyond cached data (offset 100) + result = get_channels_with_search(cursor="cache:100", limit=50) + + # Should fall back to API and return channels from continuation + assert len(result["result"]) == 50 + assert result["result"][0]["name"] == "channel-100" + + def test_cache_with_continuation_cursor_signals_more_data(self, mocker): + """Test that reaching end of cache with continuation cursor signals more data""" + # Mock cached channels + cached_channels = [ + { + "name": f"channel-{i}", + "id": f"C{i}", + "is_private": False, + "is_member": True, + } + for i in range(100) + ] + + def cache_get_side_effect(key): + if key == "slack_conversations_list": + return cached_channels + if key == "slack_conversations_list_continuation_cursor": + return "continuation_cursor_value" + return None + + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + side_effect=cache_get_side_effect, + ) + + # Get last page of cached data (offset 90, limit 10) + result = get_channels_with_search(cursor="cache:90", limit=10) + + # Should return last 10 channels + assert len(result["result"]) == 10 + assert result["result"][0]["name"] == "channel-90" + + # Should signal more data available via API + assert result["has_more"] is True + assert result["next_cursor"] == "api:continue" + + def test_api_continue_cursor_transitions_to_api(self, mocker): + """Test that 'api:continue' cursor properly transitions to API""" + api_channels = [ + { + "name": f"channel-{i}", + "id": f"C{i}", + "is_private": False, + "is_member": True, + } + for i in range(100, 150) + ] + + def cache_get_side_effect(key): + if key == "slack_conversations_list_continuation_cursor": + return "slack_api_cursor_value" + return None + + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + side_effect=cache_get_side_effect, + ) + + mock_data = { + "channels": api_channels, + "response_metadata": {"next_cursor": "next_slack_cursor"}, + } + + mock_response = MockResponse(mock_data) + mock_client = mocker.Mock() + mock_client.conversations_list.return_value = mock_response + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + # Use api:continue cursor + result = get_channels_with_search(cursor="api:continue", limit=50) + + # Should call API with continuation cursor + mock_client.conversations_list.assert_called_once() + call_args = mock_client.conversations_list.call_args + assert call_args[1]["cursor"] == "slack_api_cursor_value" + + # Should return API results + assert len(result["result"]) == 50 + assert result["next_cursor"] == "next_slack_cursor" + assert result["has_more"] is True + + def test_cache_boundary_without_continuation_cursor(self, mocker): + """Test graceful handling when cache boundary exceeded without cursor""" + cached_channels = [ + { + "name": f"channel-{i}", + "id": f"C{i}", + "is_private": False, + "is_member": True, + } + for i in range(100) + ] + + def cache_get_side_effect(key): + if key == "slack_conversations_list": + return cached_channels + # No continuation cursor available + return None + + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + side_effect=cache_get_side_effect, + ) + + # Try to paginate beyond cached data + result = get_channels_with_search(cursor="cache:100", limit=50) + + # Should return empty result gracefully + assert len(result["result"]) == 0 + assert result["has_more"] is False + assert result["next_cursor"] is None From 4f8a504747a087d8179f9410cc626c3cea0212bf Mon Sep 17 00:00:00 2001 From: Gabriel Torres Ruiz Date: Tue, 18 Nov 2025 23:23:48 -0400 Subject: [PATCH 7/7] fix(alerts): support cache bypass when refreshing Slack channels --- .../alerts/components/NotificationMethod.tsx | 4 +- .../alerts/hooks/useSlackChannels.test.ts | 51 +++++++--------- .../features/alerts/hooks/useSlackChannels.ts | 34 ++++++----- superset/config.py | 11 ---- superset/reports/api.py | 25 +++++++- superset/reports/schemas.py | 2 +- superset/tasks/slack.py | 42 +++----------- superset/utils/slack.py | 8 +-- tests/integration_tests/reports/api_tests.py | 58 +++++++++++++++++++ tests/unit_tests/utils/slack_test.py | 52 +++++++---------- 10 files changed, 156 insertions(+), 131 deletions(-) diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx index f8e400320a57..83c81107ff8c 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx @@ -220,7 +220,7 @@ export const NotificationMethod: FunctionComponent = ({ totalCount: number; }> => { try { - const result = await fetchSlackChannelsFromHook(search, page, pageSize); + const result = await fetchSlackChannelsFromHook({ search, page, pageSize }); hasShownErrorToast.current = false; @@ -562,7 +562,7 @@ export const NotificationMethod: FunctionComponent = ({ allowClear data-test="recipients" fetchOnlyOnSearch={false} - pageSize={100} + pageSize={999} placeholder={t('Select Slack channels')} tokenSeparators={[]} filterOption={() => true} diff --git a/superset-frontend/src/features/alerts/hooks/useSlackChannels.test.ts b/superset-frontend/src/features/alerts/hooks/useSlackChannels.test.ts index c2cb35465c2e..0d043a854283 100644 --- a/superset-frontend/src/features/alerts/hooks/useSlackChannels.test.ts +++ b/superset-frontend/src/features/alerts/hooks/useSlackChannels.test.ts @@ -52,7 +52,7 @@ test('fetches channels successfully', async () => { const { result } = renderHook(() => useSlackChannels()); - const channels = await result.current.fetchChannels('', 0, 100); + const channels = await result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); expect(channels.data).toHaveLength(3); expect(channels.data[0]).toEqual({ label: 'general', value: 'C001' }); @@ -70,11 +70,9 @@ test('caches results and avoids duplicate requests', async () => { const { result } = renderHook(() => useSlackChannels()); - // First call - await result.current.fetchChannels('', 0, 100); + await result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); - // Second call with same parameters - await result.current.fetchChannels('', 0, 100); + await result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); // Should only call API once expect(SupersetClient.get).toHaveBeenCalledTimes(1); @@ -100,9 +98,8 @@ test('deduplicates concurrent requests', async () => { const { result } = renderHook(() => useSlackChannels()); - // Make concurrent requests - const promise1 = result.current.fetchChannels('', 0, 100); - const promise2 = result.current.fetchChannels('', 0, 100); + const promise1 = result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); + const promise2 = result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); const [result1, result2] = await Promise.all([promise1, promise2]); @@ -124,11 +121,9 @@ test('clears cache when search changes', async () => { const { result } = renderHook(() => useSlackChannels()); - // First search - await result.current.fetchChannels('eng', 0, 100); + await result.current.fetchChannels({ search: 'eng', page: 0, pageSize: 100 }); - // Different search - await result.current.fetchChannels('gen', 0, 100); + await result.current.fetchChannels({ search: 'gen', page: 0, pageSize: 100 }); // Should call API twice (once for each search) expect(SupersetClient.get).toHaveBeenCalledTimes(2); @@ -153,17 +148,15 @@ test('handles pagination with cursors', async () => { const { result } = renderHook(() => useSlackChannels()); - // First page - const page1 = await result.current.fetchChannels('', 0, 2); + const page1 = await result.current.fetchChannels({ search: '', page: 0, pageSize: 2 }); expect(page1.data).toHaveLength(2); expect(page1.has_more).toBe(true); - // Second page - const page2 = await result.current.fetchChannels('', 1, 2); + const page2 = await result.current.fetchChannels({ search: '', page: 1, pageSize: 2 }); expect(page2.data).toHaveLength(1); }); -test('refreshChannels clears cache and refetches', async () => { +test('refreshChannels clears cache and refetches with force=true', async () => { (SupersetClient.get as jest.Mock).mockResolvedValue({ json: { result: mockChannels, @@ -174,18 +167,17 @@ test('refreshChannels clears cache and refetches', async () => { const { result } = renderHook(() => useSlackChannels()); - // Initial fetch - await result.current.fetchChannels('', 0, 100); + await result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); - // Cache hit - no new API call - await result.current.fetchChannels('', 0, 100); + await result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); expect(SupersetClient.get).toHaveBeenCalledTimes(1); - // Refresh await result.current.refreshChannels(); - // Should call API again expect(SupersetClient.get).toHaveBeenCalledTimes(2); + const lastCall = (SupersetClient.get as jest.Mock).mock.calls[1][0]; + expect(lastCall.endpoint).toContain('force:!t'); + expect(lastCall.endpoint).toContain('limit:999'); }); test('isRefreshing state updates correctly', async () => { @@ -203,7 +195,6 @@ test('isRefreshing state updates correctly', async () => { await result.current.refreshChannels(); - // Should be false after refresh completes expect(result.current.isRefreshing).toBe(false); }); @@ -213,7 +204,7 @@ test('calls onError callback when fetch fails', async () => { const onError = jest.fn(); const { result } = renderHook(() => useSlackChannels(onError)); - await result.current.fetchChannels('', 0, 100); + await result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); expect(onError).toHaveBeenCalledWith( expect.stringContaining('Unable to load Slack channels'), @@ -225,7 +216,7 @@ test('returns empty data on error', async () => { const { result } = renderHook(() => useSlackChannels()); - const channels = await result.current.fetchChannels('', 0, 100); + const channels = await result.current.fetchChannels({ search: '', page: 0, pageSize: 100 }); expect(channels.data).toEqual([]); expect(channels.totalCount).toBe(0); @@ -242,7 +233,7 @@ test('includes search_string in API params when provided', async () => { const { result } = renderHook(() => useSlackChannels()); - await result.current.fetchChannels('engineering', 0, 100); + await result.current.fetchChannels({ search: 'engineering', page: 0, pageSize: 100 }); expect(SupersetClient.get).toHaveBeenCalledWith({ endpoint: expect.stringContaining('search_string'), @@ -268,11 +259,9 @@ test('includes cursor in API params for pagination', async () => { const { result } = renderHook(() => useSlackChannels()); - // First page - await result.current.fetchChannels('', 0, 2); + await result.current.fetchChannels({ search: '', page: 0, pageSize: 2 }); - // Second page - await result.current.fetchChannels('', 1, 2); + await result.current.fetchChannels({ search: '', page: 1, pageSize: 2 }); expect(SupersetClient.get).toHaveBeenCalledWith({ endpoint: expect.stringContaining('cursor'), diff --git a/superset-frontend/src/features/alerts/hooks/useSlackChannels.ts b/superset-frontend/src/features/alerts/hooks/useSlackChannels.ts index 4e911a4a4aa9..6339f91f5f54 100644 --- a/superset-frontend/src/features/alerts/hooks/useSlackChannels.ts +++ b/superset-frontend/src/features/alerts/hooks/useSlackChannels.ts @@ -33,12 +33,15 @@ export interface SlackChannelsResult { next_cursor?: string | null; } +export interface FetchChannelsParams { + search: string; + page: number; + pageSize: number; + force?: boolean; +} + export interface UseSlackChannelsResult { - fetchChannels: ( - search: string, - page: number, - pageSize: number, - ) => Promise; + fetchChannels: (params: FetchChannelsParams) => Promise; refreshChannels: () => Promise; isRefreshing: boolean; } @@ -62,18 +65,19 @@ export function useSlackChannels( const [isRefreshing, setIsRefreshing] = useState(false); const fetchChannels = useCallback( - async ( - search: string, - page: number, - pageSize: number, - ): Promise => { + async ({ + search, + page, + pageSize, + force = false, + }: FetchChannelsParams): Promise => { const cacheKey = `${search}:${page}`; - if (dataCache.current[cacheKey]) { + if (!force && dataCache.current[cacheKey]) { return dataCache.current[cacheKey]; } - if (cacheKey in pendingRequests.current) { + if (!force && cacheKey in pendingRequests.current) { return pendingRequests.current[cacheKey]; } @@ -92,6 +96,10 @@ export function useSlackChannels( params.cursor = cursor; } + if (force) { + params.force = true; + } + const queryString = rison.encode(params); const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; @@ -163,7 +171,7 @@ export function useSlackChannels( dataCache.current = {}; pendingRequests.current = {}; - await fetchChannels('', 0, 100); + await fetchChannels({ search: '', page: 0, pageSize: 999, force: true }); } catch (error) { logging.error('Error refreshing channels:', error); diff --git a/superset/config.py b/superset/config.py index 1ba1cbcd3e10..6fa61fd6635c 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1755,17 +1755,6 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument # noq # Default: True (caching enabled) SLACK_ENABLE_CACHING: bool = True -# Maximum number of channels to cache during warmup -# Prevents excessive memory usage in very large workspaces -# The cache warmup task will stop after reaching this limit -# Default: 20000 channels (~100MB cache size) -SLACK_CACHE_MAX_CHANNELS: int = 20000 - -# Celery task timeout for Slack cache warmup (seconds) -# Prevents runaway cache warmup tasks in extremely large workspaces -# Default: 300 seconds (5 minutes) -SLACK_CACHE_WARMUP_TIMEOUT: int = 300 - # The webdriver to use for generating reports. Use one of the following # firefox # Requires: geckodriver and firefox installations diff --git a/superset/reports/api.py b/superset/reports/api.py index 8763d87ed9d7..1c441e751079 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -17,7 +17,7 @@ import logging from typing import Any, Optional -from flask import request, Response +from flask import current_app, request, Response from flask_appbuilder.api import expose, permission_name, protect, rison, safe from flask_appbuilder.hooks import before_request from flask_appbuilder.models.sqla.interface import SQLAInterface @@ -41,7 +41,7 @@ from superset.dashboards.filters import DashboardAccessFilter from superset.databases.filters import DatabaseFilter from superset.exceptions import SupersetException -from superset.extensions import event_logger +from superset.extensions import cache_manager, event_logger from superset.reports.filters import ReportScheduleAllTextFilter, ReportScheduleFilter from superset.reports.models import ReportSchedule from superset.reports.schemas import ( @@ -51,7 +51,12 @@ ReportSchedulePostSchema, ReportSchedulePutSchema, ) -from superset.utils.slack import get_channels_with_search +from superset.tasks.slack import cache_channels +from superset.utils.slack import ( + get_channels_with_search, + SLACK_CHANNELS_CACHE_KEY, + SLACK_CHANNELS_CONTINUATION_CURSOR_KEY, +) from superset.views.base_api import ( BaseSupersetModelRestApi, RelatedFieldFilter, @@ -579,6 +584,20 @@ def slack_channels(self, **kwargs: Any) -> Response: exact_match = params.get("exact_match", False) cursor = params.get("cursor") limit = params.get("limit", 100) + force = params.get("force", False) + + # Clear cache if force refresh requested + if force: + cache_manager.cache.delete(SLACK_CHANNELS_CACHE_KEY) + cache_manager.cache.delete( + SLACK_CHANNELS_CONTINUATION_CURSOR_KEY + ) + logger.info("Slack channels cache cleared due to force=True") + + # Trigger async cache warmup if caching is enabled + if current_app.config.get("SLACK_ENABLE_CACHING", True): + cache_channels.delay() + logger.info("Triggered async cache warmup task") channels_data = get_channels_with_search( search_string=search_string, diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index c1dad8a0cbef..6f3c8881e054 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -59,7 +59,7 @@ }, "exact_match": {"type": "boolean"}, "cursor": {"type": ["string", "null"]}, - "limit": {"type": "integer", "default": 100, "minimum": 1, "maximum": 1000}, + "limit": {"type": "integer", "default": 100, "minimum": 1, "maximum": 999}, "force": {"type": "boolean"}, }, } diff --git a/superset/tasks/slack.py b/superset/tasks/slack.py index fb3699bf9153..ed87160ab8e7 100644 --- a/superset/tasks/slack.py +++ b/superset/tasks/slack.py @@ -23,29 +23,28 @@ from superset.utils.slack import ( get_channels_with_search, SLACK_CHANNELS_CACHE_KEY, - SLACK_CHANNELS_CONTINUATION_CURSOR_KEY, SlackChannelTypes, ) logger = logging.getLogger(__name__) +CACHE_WARMUP_TIME_LIMIT = 300 # 5 minutes +CACHE_WARMUP_SOFT_TIME_LIMIT = int(CACHE_WARMUP_TIME_LIMIT * 0.8) # 80% of time_limit + @celery_app.task( name="slack.cache_channels", - time_limit=300, # 5 minute hard timeout (via SLACK_CACHE_WARMUP_TIMEOUT) - soft_time_limit=240, # 4 minute warning + time_limit=CACHE_WARMUP_TIME_LIMIT, + soft_time_limit=CACHE_WARMUP_SOFT_TIME_LIMIT, ) def cache_channels() -> None: """ Celery task to warm up the Slack channels cache. This task fetches all Slack channels using pagination and stores them in cache. - Includes safeguards for very large workspaces (50k+ channels). Respects the following config variables: - SLACK_ENABLE_CACHING: If False, this task does nothing - - SLACK_CACHE_MAX_CHANNELS: Maximum channels to cache (prevents runaway fetches) - - SLACK_CACHE_WARMUP_TIMEOUT: Task timeout in seconds - SLACK_CACHE_TIMEOUT: How long to keep cached data """ enable_caching = current_app.config.get("SLACK_ENABLE_CACHING", True) @@ -56,13 +55,11 @@ def cache_channels() -> None: return cache_timeout = current_app.config["SLACK_CACHE_TIMEOUT"] - max_channels = current_app.config.get("SLACK_CACHE_MAX_CHANNELS", 20000) retry_count = current_app.config.get("SLACK_API_RATE_LIMIT_RETRY_COUNT", 2) logger.info( "Starting Slack channels cache warm-up task " - "(max_channels=%d, cache_timeout=%ds, retry_count=%d)", - max_channels, + "(cache_timeout=%ds, retry_count=%d)", cache_timeout, retry_count, ) @@ -79,7 +76,7 @@ def cache_channels() -> None: search_string="", types=list(SlackChannelTypes), cursor=cursor, - limit=1000, + limit=999, ) page_channels = result["result"] @@ -94,31 +91,6 @@ def cache_channels() -> None: cursor = result.get("next_cursor") - # Safety check: stop if we hit the max channel limit - if len(all_channels) >= max_channels: - # Store the continuation cursor so we can resume via API later - if cursor: - cache_manager.cache.set( - SLACK_CHANNELS_CONTINUATION_CURSOR_KEY, - cursor, - timeout=cache_timeout, - ) - logger.warning( - "Reached max channel limit (%d channels in %d pages). " - "Stored continuation cursor for API fallback. " - "Channels beyond limit will be fetched from API on demand.", - len(all_channels), - page_count, - ) - else: - logger.warning( - "Reached max channel limit (%d channels in %d pages). " - "No more channels available.", - len(all_channels), - page_count, - ) - break - if not cursor or not result.get("has_more"): break diff --git a/superset/utils/slack.py b/superset/utils/slack.py index c05ce0854dfc..f83ea415fac1 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -72,11 +72,10 @@ def _fetch_channels_without_search( """Fetch channels without search filtering, paginating for large limits.""" channels: list[SlackChannelSchema] = [] slack_cursor = cursor - page_size = min(limit, 1000) while True: response = client.conversations_list( - limit=page_size, + limit=999, cursor=slack_cursor, exclude_archived=True, types=types_param, @@ -89,7 +88,7 @@ def _fetch_channels_without_search( slack_cursor = response.data.get("response_metadata", {}).get("next_cursor") - if not slack_cursor or len(page_channels) < page_size or len(channels) >= limit: + if not slack_cursor or len(channels) >= limit: break return { @@ -117,7 +116,7 @@ def _fetch_channels_with_search( while len(matches) < limit: response = client.conversations_list( - limit=1000, + limit=999, cursor=slack_cursor, exclude_archived=True, types=types_param, @@ -247,7 +246,6 @@ def _fetch_from_cache( # noqa: C901 offset = 0 # Check if we're trying to paginate beyond cached data - # This happens when cache is partial (hit SLACK_CACHE_MAX_CHANNELS limit) if offset >= len(channels): logger.info( "Pagination offset (%d) exceeds cached data (%d channels). " diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index cc5058094b7c..92c59fabcc51 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -2261,3 +2261,61 @@ def test_slack_channels_api_error_handling(self, mock_get_channels): data = json.loads(rv.data.decode("utf-8")) assert "Slack API error" in data["message"] + + @patch("superset.reports.api.cache_channels") + @patch("superset.reports.api.cache_manager") + @patch("superset.reports.api.get_channels_with_search") + def test_slack_channels_api_with_force_clears_cache( + self, mock_get_channels, mock_cache_manager, mock_cache_channels + ): + """ + Test /api/v1/report/slack_channels/ endpoint with force=true clears cache + and triggers async cache warmup + """ + self.login(ADMIN_USERNAME) + + mock_get_channels.return_value = { + "result": [{"id": "C123", "name": "general"}], + "next_cursor": None, + "has_more": False, + } + + uri = "api/v1/report/slack_channels/?q=(force:!t)" + rv = self.client.get(uri) + assert rv.status_code == 200 + + # Verify cache was cleared + assert mock_cache_manager.cache.delete.call_count == 2 + + # Verify async cache warmup was triggered + mock_cache_channels.delay.assert_called_once() + + @patch("superset.reports.api.cache_channels") + @patch("superset.reports.api.cache_manager") + @patch("superset.reports.api.get_channels_with_search") + def test_slack_channels_api_force_respects_caching_config( + self, mock_get_channels, mock_cache_manager, mock_cache_channels + ): + """ + Test /api/v1/report/slack_channels/ with force=true + respects SLACK_ENABLE_CACHING + """ + self.login(ADMIN_USERNAME) + + mock_get_channels.return_value = { + "result": [{"id": "C123", "name": "general"}], + "next_cursor": None, + "has_more": False, + } + + # Test with caching disabled + with patch.dict(self.app.config, {"SLACK_ENABLE_CACHING": False}, clear=False): + uri = "api/v1/report/slack_channels/?q=(force:!t)" + rv = self.client.get(uri) + assert rv.status_code == 200 + + # Cache should still be cleared + assert mock_cache_manager.cache.delete.call_count == 2 + + # But async warmup should NOT be triggered + mock_cache_channels.delay.assert_not_called() diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index d9c1d482ce29..04eed3050748 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -317,15 +317,17 @@ def test_filter_channels_by_specified_types( def test_handle_pagination_without_search(self, mocker): """Test pagination returns single page with cursor""" + channels = [ + { + "name": f"channel-{i}", + "id": f"C{i}", + "is_private": False, + "is_member": True, + } + for i in range(150) + ] mock_data_page1 = { - "channels": [ - { - "name": "general", - "id": "C12345", - "is_private": False, - "is_member": True, - } - ], + "channels": channels, "response_metadata": {"next_cursor": "page2_cursor"}, } @@ -335,18 +337,9 @@ def test_handle_pagination_without_search(self, mocker): mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) result = get_channels_with_search(limit=100) - assert result == { - "result": [ - { - "name": "general", - "id": "C12345", - "is_private": False, - "is_member": True, - } - ], - "next_cursor": "page2_cursor", - "has_more": True, - } + assert len(result["result"]) == 100 + assert result["next_cursor"] == "page2_cursor" + assert result["has_more"] is True def test_handle_pagination_with_cursor(self, mocker): """Test pagination with cursor fetches next page""" @@ -659,18 +652,17 @@ def test_non_search_pagination_over_1000_limit(self, mocker): def mock_conversations_list(**kwargs): nonlocal call_count - limit = kwargs.get("limit", 100) + limit = kwargs.get("limit", 999) cursor = kwargs.get("cursor") - # Simulate Slack API pagination (max 1000 per page) if cursor is None: start = 0 - elif cursor == "cursor_1000": - start = 1000 - elif cursor == "cursor_2000": - start = 2000 + elif cursor == "cursor_999": + start = 999 + elif cursor == "cursor_1998": + start = 1998 else: - start = 3000 + start = 2500 end = min(start + limit, 2500) next_cursor = f"cursor_{end}" if end < 2500 else None @@ -687,14 +679,14 @@ def mock_conversations_list(**kwargs): mock_client.conversations_list.side_effect = mock_conversations_list mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) - # Request 1500 channels (requires 2 pages of 1000 each) + # Request 1500 channels (requires 2 pages of 999 each) result = get_channels_with_search(limit=1500) # Should return exactly 1500 channels assert len(result["result"]) == 1500 assert result["has_more"] is True - assert result["next_cursor"] == "cursor_2000" - # Should have made 2 API calls + assert result["next_cursor"] == "cursor_1998" + # Should have made 2 API calls (999 + 501) assert call_count == 2 def test_search_with_exact_match_optimization(self, mocker):