From 0c3aa7d8fe18cca19c6a164ae7deb927fb1843bb Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Tue, 6 Aug 2024 15:40:30 -0700 Subject: [PATCH] fix: load slack channels earlier (#29846) --- .../components/NotificationMethod.test.tsx | 302 +++++++++++++++++- .../alerts/components/NotificationMethod.tsx | 58 ++-- 2 files changed, 330 insertions(+), 30 deletions(-) diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx index 292e8d9afc4c1..39d9b2625012f 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx @@ -16,9 +16,21 @@ * specific language governing permissions and limitations * under the License. */ -import { fireEvent, render, screen } from 'spec/helpers/testing-library'; +import { + cleanup, + fireEvent, + render, + screen, + waitFor, +} from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; +import { + FeatureFlag, + JsonResponse, + SupersetClient, + TextResponse, +} from '@superset-ui/core'; import { NotificationMethod, mapSlackValues } from './NotificationMethod'; import { NotificationMethodOption, NotificationSetting } from '../types'; @@ -43,6 +55,7 @@ const mockDefaultSubject = 'Default Subject'; describe('NotificationMethod', () => { beforeEach(() => { jest.clearAllMocks(); + cleanup(); }); it('should render the component', () => { @@ -180,4 +193,291 @@ describe('NotificationMethod', () => { { label: 'User Two', value: 'user2' }, ]); }); + it('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(); + }); + it('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'); + }); + it('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 + it('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(); + }); + it('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(); + }); + }); + it('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( + , + ); + + // Wait for the component to handle the error and render the expected div + await waitFor(() => { + expect( + screen.getByText('Recipients are separated by "," or ";"'), + ).toBeInTheDocument(); + }); + }); + it('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( + , + ); + + // Wait for the component to handle the error and render the expected div + await waitFor(() => { + expect(screen.getByTitle('Slack')).toBeInTheDocument(); + }); + }); + it('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( + , + ); + + // Wait for the component to handle the error and render the expected div + expect( + screen.getByText('Recipients are separated by "," or ";"'), + ).toBeInTheDocument(); + }); + it('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( + , + ); + + // Wait for the component to handle the error and render the expected div + expect( + screen.getByText('Recipients are separated by "," or ";"'), + ).toBeInTheDocument(); + }); }); diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx index 5028c14f13c22..dff2b9e96fea3 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx @@ -211,6 +211,8 @@ 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: '', @@ -257,51 +259,47 @@ export const NotificationMethod: FunctionComponent = ({ }; useEffect(() => { - if ( - method && - [ - NotificationMethodOption.Slack, - NotificationMethodOption.SlackV2, - ].includes(method) && - !slackOptions[0]?.options.length - ) { + if (!slackOptions[0]?.options.length) { fetchSlackChannels({ types: ['public_channel', 'private_channel'] }) .then(({ json }) => { const { result } = json; - const options: SlackOptionsType = mapChannelsToOptions(result); setSlackOptions(options); if (isFeatureEnabled(FeatureFlag.AlertReportSlackV2)) { - // map existing ids to names for display + // for edit mode, map existing ids to names for display if slack v2 // or names to ids if slack v1 const [publicOptions, privateOptions] = options; - - setSlackRecipients( - mapSlackValues({ - method, - recipientValue, - slackOptions: [ - ...publicOptions.options, - ...privateOptions.options, - ], - }), - ); - if (method === NotificationMethodOption.Slack) { - onMethodChange({ - label: NotificationMethodOption.Slack, - value: NotificationMethodOption.SlackV2, - }); + if ( + method && + [ + NotificationMethodOption.SlackV2, + NotificationMethodOption.Slack, + ].includes(method) + ) { + setSlackRecipients( + mapSlackValues({ + method, + recipientValue, + slackOptions: [ + ...publicOptions.options, + ...privateOptions.options, + ], + }), + ); } } }) - .catch(() => { + .catch(e => { // Fallback to slack v1 if slack v2 is not compatible setUseSlackV1(true); + }) + .finally(() => { + setMethodOptionsLoading(false); }); } - }, [method]); + }, []); const methodOptions = useMemo( () => @@ -323,7 +321,7 @@ export const NotificationMethod: FunctionComponent = ({ : method, value: method, })), - [options], + [options, useSlackV1], ); if (!setting) { @@ -434,8 +432,10 @@ 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