You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2025/11/15 01:26:38 UTC
(superset) 04/05: fix: PR comments
This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a commit to branch alerts-reports-unable-to-edit-slack-recipients
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 3668f8edb6459514aa4d39a1bee8d6590f4b8730
Author: Gabriel Torres Ruiz <ga...@gmail.com>
AuthorDate: Thu Oct 30 19:00:28 2025 -0400
fix: PR comments
---
.../src/features/alerts/AlertReportModal.tsx | 1 +
.../alerts/components/NotificationMethod.test.tsx | 332 ++++++++++++++++++++-
.../alerts/components/NotificationMethod.tsx | 110 ++++++-
3 files changed, 417 insertions(+), 26 deletions(-)
diff --git a/superset-frontend/src/features/alerts/AlertReportModal.tsx b/superset-frontend/src/features/alerts/AlertReportModal.tsx
index cd923b64a3..b99c3c1088 100644
--- a/superset-frontend/src/features/alerts/AlertReportModal.tsx
+++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx
@@ -2600,6 +2600,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
email_subject={currentAlert?.email_subject || ''}
defaultSubject={emailSubject || ''}
setErrorSubject={handleErrorUpdate}
+ addDangerToast={addDangerToast}
/>
</StyledNotificationMethodWrapper>
))}
diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx
index 7543dfa30b..dfaff1cda2 100644
--- a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx
+++ b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx
@@ -38,6 +38,7 @@ const mockOnUpdate = jest.fn();
const mockOnRemove = jest.fn();
const mockOnInputChange = jest.fn();
const mockSetErrorSubject = jest.fn();
+const mockAddDangerToast = jest.fn();
const mockSetting: NotificationSetting = {
method: NotificationMethodOption.Email,
@@ -78,6 +79,7 @@ test('NotificationMethod - should render the component', () => {
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -97,6 +99,7 @@ test('NotificationMethod - should call onRemove when the delete button is clicke
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -117,6 +120,7 @@ test('NotificationMethod - should update recipient value when input changes', ()
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -142,6 +146,7 @@ test('NotificationMethod - should call onRecipientsChange when the recipients va
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -326,6 +331,7 @@ test('shows the right combo when ff is false', async () => {
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -363,6 +369,7 @@ test('shows the textbox when the fetch fails', async () => {
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -405,6 +412,7 @@ test('shows the dropdown when ff is true and slackChannels succeed', async () =>
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -440,6 +448,7 @@ test('shows the textarea when ff is true and slackChannels fail', async () => {
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -475,6 +484,7 @@ test('shows the textarea when ff is true and slackChannels fail and slack is sel
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -503,6 +513,7 @@ test('shows the textarea when ff is true, slackChannels fail and slack is select
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -536,6 +547,7 @@ test('NotificationMethod - AsyncSelect should render for SlackV2 method', async
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -574,6 +586,7 @@ test('NotificationMethod - AsyncSelect should handle SlackV2 with valid API resp
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -615,6 +628,7 @@ test('NotificationMethod - AsyncSelect should render refresh button for SlackV2'
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -626,7 +640,7 @@ test('NotificationMethod - AsyncSelect should render refresh button for SlackV2'
});
});
-test('NotificationMethod - AsyncSelect should clear recipients and reload channels when refresh button clicked', async () => {
+test('NotificationMethod - AsyncSelect should reload channels without clearing recipients when refresh button clicked', async () => {
window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true };
const mockChannels = [
{
@@ -637,7 +651,7 @@ test('NotificationMethod - AsyncSelect should clear recipients and reload channe
},
];
- jest.spyOn(SupersetClient, 'get').mockResolvedValue({
+ const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({
json: {
result: mockChannels,
next_cursor: null,
@@ -658,6 +672,7 @@ test('NotificationMethod - AsyncSelect should clear recipients and reload channe
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -666,19 +681,25 @@ test('NotificationMethod - AsyncSelect should clear recipients and reload channe
expect(screen.getByTestId('refresh-slack-channels')).toBeInTheDocument();
});
+ const initialCallCount = getSpy.mock.calls.length;
+
// Click refresh button
const refreshButton = screen.getByTestId('refresh-slack-channels');
fireEvent.click(refreshButton);
- // Verify onUpdate was called with empty recipients
+ // Verify the API was called again to reload channels
await waitFor(() => {
- expect(mockOnUpdate).toHaveBeenCalledWith(
- 0,
- expect.objectContaining({
- recipients: '',
- }),
- );
+ expect(getSpy.mock.calls.length).toBeGreaterThan(initialCallCount);
});
+
+ // Verify onUpdate was NOT called with empty recipients (recipients should be preserved)
+ const updateCalls = mockOnUpdate.mock.calls;
+ const callsWithEmptyRecipients = updateCalls.filter(
+ call => call[1]?.recipients === '',
+ );
+ expect(callsWithEmptyRecipients).toHaveLength(0);
+
+ getSpy.mockRestore();
});
test('NotificationMethod - data cache prevents redundant API calls when selecting channels', async () => {
@@ -689,6 +710,16 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin
{ name: 'random', id: 'C002', is_private: false, is_member: true },
];
+ const mockSettingWithoutRecipients: NotificationSetting = {
+ method: NotificationMethodOption.SlackV2,
+ recipients: '', // Empty to avoid initialization call
+ options: [
+ NotificationMethodOption.Email,
+ NotificationMethodOption.Slack,
+ NotificationMethodOption.SlackV2,
+ ],
+ };
+
let callCount = 0;
const getSpy = jest
.spyOn(SupersetClient, 'get')
@@ -707,7 +738,7 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin
const { getByRole } = render(
<NotificationMethod
- setting={mockSettingSlackV2}
+ setting={mockSettingWithoutRecipients}
index={0}
onUpdate={mockOnUpdate}
onRemove={mockOnRemove}
@@ -715,6 +746,7 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin
email_subject={mockEmailSubject}
defaultSubject={mockDefaultSubject}
setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
/>,
);
@@ -736,7 +768,287 @@ test('NotificationMethod - data cache prevents redundant API calls when selectin
await new Promise(resolve => setTimeout(resolve, 200));
+ // Should still be 1 because of caching
expect(callCount).toBe(1);
getSpy.mockRestore();
});
+
+test('NotificationMethod - should preserve selected channels on refresh', async () => {
+ window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true };
+ const mockChannels = [
+ { id: 'C001', name: 'general', is_private: false, is_member: true },
+ { id: 'C002', name: 'random', is_private: false, is_member: true },
+ { id: 'C003', name: 'engineering', is_private: false, is_member: true },
+ ];
+
+ const mockSettingWithRecipients: NotificationSetting = {
+ method: NotificationMethodOption.SlackV2,
+ recipients: 'C001,C002',
+ options: [
+ NotificationMethodOption.Email,
+ NotificationMethodOption.Slack,
+ NotificationMethodOption.SlackV2,
+ ],
+ };
+
+ const getSpy = jest.spyOn(SupersetClient, 'get').mockImplementation(
+ () =>
+ Promise.resolve({
+ json: {
+ result: mockChannels,
+ next_cursor: null,
+ has_more: false,
+ },
+ }) as unknown as Promise<Response | JsonResponse | TextResponse>,
+ );
+
+ const { getByTestId } = render(
+ <NotificationMethod
+ setting={mockSettingWithRecipients}
+ index={0}
+ onUpdate={mockOnUpdate}
+ onRemove={mockOnRemove}
+ onInputChange={mockOnInputChange}
+ email_subject={mockEmailSubject}
+ defaultSubject={mockDefaultSubject}
+ setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
+ />,
+ );
+
+ // Wait for initial load
+ await waitFor(
+ () => {
+ expect(getByTestId('refresh-slack-channels')).toBeInTheDocument();
+ },
+ { timeout: 3000 },
+ );
+
+ const initialCallCount = getSpy.mock.calls.length;
+
+ // Find and click the refresh button
+ const refreshButton = getByTestId('refresh-slack-channels');
+ fireEvent.click(refreshButton);
+
+ // Wait for refresh to complete
+ await waitFor(
+ () => {
+ expect(getSpy.mock.calls.length).toBeGreaterThan(initialCallCount);
+ },
+ { timeout: 3000 },
+ );
+
+ // Verify that recipients are preserved (not cleared to empty string)
+ const updateCalls = mockOnUpdate.mock.calls;
+ const callsWithEmptyRecipients = updateCalls.filter(
+ call => call[1]?.recipients === '',
+ );
+ expect(callsWithEmptyRecipients).toHaveLength(0);
+
+ getSpy.mockRestore();
+});
+
+test('NotificationMethod - should initialize Slack recipients when editing existing alert', async () => {
+ window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true };
+ const mockChannels = [
+ { id: 'C001', name: 'general', is_private: false, is_member: true },
+ { id: 'C002', name: 'random', is_private: false, is_member: true },
+ { id: 'C003', name: 'engineering', is_private: false, is_member: true },
+ ];
+
+ const mockSettingWithRecipients: NotificationSetting = {
+ method: NotificationMethodOption.SlackV2,
+ recipients: 'C001,C002',
+ options: [
+ NotificationMethodOption.Email,
+ NotificationMethodOption.Slack,
+ NotificationMethodOption.SlackV2,
+ ],
+ };
+
+ const getSpy = jest.spyOn(SupersetClient, 'get').mockImplementation(
+ () =>
+ Promise.resolve({
+ json: {
+ result: mockChannels,
+ next_cursor: null,
+ has_more: false,
+ },
+ }) as unknown as Promise<Response | JsonResponse | TextResponse>,
+ );
+
+ const { getByTestId } = render(
+ <NotificationMethod
+ setting={mockSettingWithRecipients}
+ index={0}
+ onUpdate={mockOnUpdate}
+ onRemove={mockOnRemove}
+ onInputChange={mockOnInputChange}
+ email_subject={mockEmailSubject}
+ defaultSubject={mockDefaultSubject}
+ setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
+ />,
+ );
+
+ // Wait for the component to render and recipients dropdown to be available
+ await waitFor(
+ () => {
+ expect(getByTestId('recipients')).toBeInTheDocument();
+ },
+ { timeout: 3000 },
+ );
+
+ // Verify that the component fetched channels for initialization
+ await waitFor(
+ () => {
+ expect(getSpy).toHaveBeenCalled();
+ },
+ { timeout: 3000 },
+ );
+
+ // Verify that the component attempted to initialize recipients
+ expect(getSpy).toHaveBeenCalledWith(
+ expect.objectContaining({
+ endpoint: expect.stringContaining('/api/v1/report/slack_channels/'),
+ }),
+ );
+
+ getSpy.mockRestore();
+});
+
+test('NotificationMethod - should preserve selected channels during search and pagination', async () => {
+ window.featureFlags = { [FeatureFlag.AlertReportSlackV2]: true };
+
+ // Mock channels for different pages and search results
+ const page1Channels = [
+ { id: 'C001', name: 'general', is_private: false, is_member: true },
+ { id: 'C002', name: 'random', is_private: false, is_member: true },
+ { id: 'C003', name: 'engineering', is_private: false, is_member: true },
+ ];
+
+ const page2Channels = [
+ { id: 'C004', name: 'design', is_private: false, is_member: true },
+ { id: 'C005', name: 'product', is_private: false, is_member: true },
+ ];
+
+ const searchChannels = [
+ { id: 'C003', name: 'engineering', is_private: false, is_member: true },
+ { id: 'C006', name: 'eng-team', is_private: false, is_member: true },
+ ];
+
+ // Setting with pre-selected recipients
+ const mockSettingWithRecipients: NotificationSetting = {
+ method: NotificationMethodOption.SlackV2,
+ recipients: 'C001,C002', // Pre-selected: general, random
+ options: [
+ NotificationMethodOption.Email,
+ NotificationMethodOption.Slack,
+ NotificationMethodOption.SlackV2,
+ ],
+ };
+
+ // Mock API to return different results based on search string
+ const getSpy = jest
+ .spyOn(SupersetClient, 'get')
+ .mockImplementation(({ endpoint }) => {
+ const url = endpoint as string;
+ const isSearch = url.includes('search_string');
+ const isPage2 = url.includes('cursor');
+
+ let result;
+ let next_cursor = null;
+ let has_more = false;
+
+ if (isSearch) {
+ // Search returns matching channels
+ result = searchChannels;
+ } else if (isPage2) {
+ // Page 2 returns additional channels
+ result = page2Channels;
+ } else {
+ // Page 1 returns initial channels
+ result = page1Channels;
+ next_cursor = 'page2cursor';
+ has_more = true;
+ }
+
+ return Promise.resolve({
+ json: {
+ result,
+ next_cursor,
+ has_more,
+ },
+ }) as unknown as Promise<Response | JsonResponse | TextResponse>;
+ });
+
+ const { getByTestId } = render(
+ <NotificationMethod
+ setting={mockSettingWithRecipients}
+ index={0}
+ onUpdate={mockOnUpdate}
+ onRemove={mockOnRemove}
+ onInputChange={mockOnInputChange}
+ email_subject={mockEmailSubject}
+ defaultSubject={mockDefaultSubject}
+ setErrorSubject={mockSetErrorSubject}
+ addDangerToast={mockAddDangerToast}
+ />,
+ );
+
+ // Wait for initial page to load
+ await waitFor(
+ () => {
+ expect(getByTestId('recipients')).toBeInTheDocument();
+ },
+ { timeout: 3000 },
+ );
+
+ // Verify initial API call was made
+ await waitFor(
+ () => {
+ expect(getSpy).toHaveBeenCalled();
+ },
+ { timeout: 3000 },
+ );
+
+ const initialCallCount = getSpy.mock.calls.length;
+
+ // Simulate search and pagination scenarios
+ // Note: AsyncSelect is complex to simulate directly, so we verify via API calls
+ // that selections aren't cleared during search and pagination operations
+
+ // Verify no calls to onUpdate cleared the recipients
+ let updateCalls = mockOnUpdate.mock.calls;
+ let callsWithEmptyRecipients = updateCalls.filter(
+ call => call[1]?.recipients === '',
+ );
+ expect(callsWithEmptyRecipients).toHaveLength(0);
+
+ // Simulate pagination by triggering additional API calls
+ // In the real component, this happens when user scrolls in AsyncSelect
+ await waitFor(
+ () => {
+ // The component should have made at least the initial call
+ expect(getSpy.mock.calls.length).toBeGreaterThanOrEqual(initialCallCount);
+ },
+ { timeout: 3000 },
+ );
+
+ // After pagination, verify recipients are still not cleared
+ updateCalls = mockOnUpdate.mock.calls;
+ callsWithEmptyRecipients = updateCalls.filter(
+ call => call[1]?.recipients === '',
+ );
+ expect(callsWithEmptyRecipients).toHaveLength(0);
+
+ // Verify that all onUpdate calls (if any) preserved the recipients
+ // Every call should either have no recipients field touched, or have recipients
+ const updateCallsWithEmptyRecipientsCheck = updateCalls.every(
+ call => !call[1]?.hasOwnProperty('recipients') || call[1].recipients !== '',
+ );
+ expect(updateCallsWithEmptyRecipientsCheck).toBe(true);
+
+ getSpy.mockRestore();
+});
diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx
index 19d336aaa4..1af3521470 100644
--- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx
+++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx
@@ -22,6 +22,7 @@ import {
ChangeEvent,
useMemo,
useRef,
+ useEffect,
} from 'react';
import rison from 'rison';
@@ -29,6 +30,7 @@ import {
FeatureFlag,
SupersetClient,
isFeatureEnabled,
+ logging,
styled,
t,
useTheme,
@@ -136,6 +138,7 @@ interface NotificationMethodProps {
email_subject: string;
defaultSubject: string;
setErrorSubject: (hasError: boolean) => void;
+ addDangerToast?: (msg: string) => void;
}
export const mapSlackValues = ({
@@ -168,6 +171,7 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
email_subject,
defaultSubject,
setErrorSubject,
+ addDangerToast,
}) => {
const { method, recipients, cc, bcc, options } = setting || {};
const [recipientValue, setRecipientValue] = useState<string>(
@@ -193,6 +197,7 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
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 onMethodChange = (selected: {
label: string;
@@ -278,13 +283,40 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
return responseData;
} catch (error) {
- console.error('Failed to fetch Slack channels:', 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);
- onMethodChange({
- label: NotificationMethodOption.Slack,
- value: NotificationMethodOption.Slack,
- });
- throw error;
+
+ // 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 empty result instead of throwing to allow UI to continue
+ return {
+ data: [],
+ totalCount: 0,
+ };
}
};
@@ -292,29 +324,75 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
setIsRefreshing(true);
try {
+ // Clear cache and cursor to force fresh data fetch
forceRefreshRef.current = true;
cursorRef.current = {};
dataCache.current = {};
- setSlackRecipients([]);
-
- if (onUpdate && setting) {
- onUpdate(index, {
- ...setting,
- recipients: '',
- } as NotificationSetting);
- }
-
+ // Fetch fresh channel list without clearing user selections
await fetchSlackChannels('', 0, 100);
setRefreshKey(prev => prev + 1);
} catch (error) {
- console.error('Error refreshing channels:', 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);
}
};
+ // Reset error toast flag when method changes
+ useEffect(() => {
+ hasShownErrorToast.current = false;
+ }, [method]);
+
+ // Initialize slackRecipients when editing an existing alert with SlackV2
+ useEffect(() => {
+ const initializeSlackRecipients = async () => {
+ if (
+ method === NotificationMethodOption.SlackV2 &&
+ recipients &&
+ slackRecipients.length === 0
+ ) {
+ try {
+ // Fetch first page of channels to map IDs to names
+ const channelData = await fetchSlackChannels('', 0, 100);
+ const recipientIds = recipients.split(',').map(id => id.trim());
+
+ // Map recipient IDs to {label, value} format
+ const mappedRecipients = recipientIds
+ .map(id => {
+ 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,
+ value: id,
+ }));
+ setSlackRecipients(fallbackRecipients);
+ }
+ }
+ };
+
+ initializeSlackRecipients();
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [method, recipients]);
+
const methodOptions = useMemo(
() =>
(options || [])