diff --git a/superset-frontend/src/components/RefreshLabel/index.tsx b/superset-frontend/src/components/RefreshLabel/index.tsx index 03e0bee13ef..887805d6eb2 100644 --- a/superset-frontend/src/components/RefreshLabel/index.tsx +++ b/superset-frontend/src/components/RefreshLabel/index.tsx @@ -24,9 +24,14 @@ import Icons, { IconType } from 'src/components/Icons'; export interface RefreshLabelProps { onClick: MouseEventHandler; tooltipContent: string; + disabled?: boolean; } -const RefreshLabel = ({ onClick, tooltipContent }: RefreshLabelProps) => { +const RefreshLabel = ({ + onClick, + tooltipContent, + disabled, +}: RefreshLabelProps) => { // eslint-disable-next-line @typescript-eslint/no-unused-vars const IconWithoutRef = forwardRef((props: IconType, ref: any) => ( @@ -36,7 +41,7 @@ const RefreshLabel = ({ onClick, tooltipContent }: RefreshLabelProps) => { ({ cursor: 'pointer', color: theme.colors.grayscale.base, diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx index b0c9186e291..eec2a434f60 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx @@ -52,6 +52,16 @@ const mockSetting: NotificationSetting = { const mockEmailSubject = 'Test Subject'; const mockDefaultSubject = 'Default Subject'; +const mockSettingSlackV2: NotificationSetting = { + method: NotificationMethodOption.SlackV2, + recipients: 'slack-channel', + options: [ + NotificationMethodOption.Email, + NotificationMethodOption.Slack, + NotificationMethodOption.SlackV2, + ], +}; + describe('NotificationMethod', () => { beforeEach(() => { jest.clearAllMocks(); @@ -480,4 +490,71 @@ describe('NotificationMethod', () => { screen.getByText('Recipients are separated by "," or ";"'), ).toBeInTheDocument(); }); + + it('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(); + }); + + describe('RefreshLabel functionality', () => { + it('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('refresh'), + ); + // 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(); + }); + }); + }); }); diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx index 17a78697c9a..e930f53749a 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx @@ -36,6 +36,7 @@ import { } from '@superset-ui/core'; import { Select } from 'src/components'; import Icons from 'src/components/Icons'; +import RefreshLabel from 'src/components/RefreshLabel'; import { NotificationMethodOption, NotificationSetting, @@ -76,6 +77,9 @@ const StyledNotificationMethod = styled.div` margin-left: ${theme.gridUnit * 2}px; padding-top: ${theme.gridUnit}px; } + .anticon { + margin-left: ${theme.gridUnit}px; + } } .ghost-button { @@ -221,6 +225,8 @@ export const NotificationMethod: FunctionComponent = ({ ]); const [useSlackV1, setUseSlackV1] = useState(false); + const [isSlackChannelsLoading, setIsSlackChannelsLoading] = + useState(true); const onMethodChange = (selected: { label: string; @@ -248,16 +254,70 @@ export const NotificationMethod: FunctionComponent = ({ 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 }); + const queryString = rison.encode({ + searchString, + types, + exactMatch, + force, + }); const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; return SupersetClient.get({ endpoint }); }; + 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); + }); + }; + useEffect(() => { const slackEnabled = options?.some( option => @@ -265,44 +325,7 @@ export const NotificationMethod: FunctionComponent = ({ option === NotificationMethodOption.SlackV2, ); if (slackEnabled && !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)) { - // 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); - }); + updateSlackOptions(); } }, []); @@ -518,18 +541,26 @@ export const NotificationMethod: FunctionComponent = ({ ) : ( // for SlackV2 - + updateSlackOptions({ force: true })} + tooltipContent={t('Force refresh Slack channels list')} + disabled={isSlackChannelsLoading} + /> + )} diff --git a/superset/reports/api.py b/superset/reports/api.py index 320eb97c051..a0ebabb2028 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -577,8 +577,12 @@ class ReportScheduleRestApi(BaseSupersetModelRestApi): search_string = params.get("search_string") types = params.get("types", []) exact_match = params.get("exact_match", False) + force = params.get("force", False) channels = get_channels_with_search( - search_string=search_string, types=types, exact_match=exact_match + search_string=search_string, + types=types, + exact_match=exact_match, + force=force, ) return self.response(200, result=channels) except SupersetException as ex: diff --git a/superset/utils/slack.py b/superset/utils/slack.py index 6d51f2765ea..8125a3ac401 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -17,7 +17,7 @@ import logging -from typing import Optional +from typing import Any, Optional from flask import current_app from slack_sdk import WebClient @@ -26,7 +26,9 @@ from slack_sdk.http_retry.builtin_handlers import RateLimitErrorRetryHandler 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 from superset.utils.core import recipients_string_to_list @@ -54,60 +56,82 @@ def get_slack_client() -> WebClient: return client +@cache_util.memoized_func( + key="slack_conversations_list", + cache=cache_manager.cache, +) +def get_channels(limit: int, extra_params: dict[str, Any]) -> 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] = [] + cursor = None + + while True: + response = client.conversations_list( + limit=limit, cursor=cursor, exclude_archived=True, **extra_params + ) + channels.extend( + channel_schema.load(channel) for channel in response.data["channels"] + ) + cursor = response.data.get("response_metadata", {}).get("next_cursor") + if not cursor: + break + + return channels + + def get_channels_with_search( search_string: str = "", limit: int = 999, types: Optional[list[SlackChannelTypes]] = None, exact_match: bool = False, + force: bool = False, ) -> list[SlackChannelSchema]: """ The slack api is paginated but does not include search, so we need to fetch all channels and filter them ourselves This will search by slack name or id """ - + extra_params = {} + extra_params["types"] = ",".join(types) if types else None try: - client = get_slack_client() - channel_schema = SlackChannelSchema() - channels: list[SlackChannelSchema] = [] - cursor = None - extra_params = {} - extra_params["types"] = ",".join(types) if types else None - - while True: - response = client.conversations_list( - limit=limit, cursor=cursor, exclude_archived=True, **extra_params - ) - channels.extend( - channel_schema.load(channel) for channel in response.data["channels"] - ) - cursor = response.data.get("response_metadata", {}).get("next_cursor") - if not cursor: - break - - # The search string can be multiple channels separated by commas - if search_string: - search_array = recipients_string_to_list(search_string) - channels = [ - channel - for channel in channels - if any( - ( - search.lower() == channel["name"].lower() - or search.lower() == channel["id"].lower() - if exact_match - else ( - search.lower() in channel["name"].lower() - or search.lower() in channel["id"].lower() - ) - ) - for search in search_array - ) - ] - return channels + channels = get_channels( + limit=limit, + extra_params=extra_params, + force=force, + cache_timeout=86400, + ) except (SlackClientError, SlackApiError) as ex: raise SupersetException(f"Failed to list channels: {ex}") from ex + # The search string can be multiple channels separated by commas + if search_string: + search_array = recipients_string_to_list(search_string) + channels = [ + channel + for channel in channels + if any( + ( + search.lower() == channel["name"].lower() + or search.lower() == channel["id"].lower() + if exact_match + else ( + search.lower() in channel["name"].lower() + or search.lower() in channel["id"].lower() + ) + ) + for search in search_array + ) + ] + return channels + def should_use_v2_api() -> bool: if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"):