chore: Caching the Slack channels list (#32529)

Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>

superset-frontend/src/components/RefreshLabel/index.tsx

@@ -24,9 +24,14 @@ import Icons, { IconType } from 'src/components/Icons';
 export interface RefreshLabelProps {
   onClick: MouseEventHandler<HTMLSpanElement>;
   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) => (
     <Icons.Refresh {...props} />
@@ -36,7 +41,7 @@ const RefreshLabel = ({ onClick, tooltipContent }: RefreshLabelProps) => {
     <Tooltip title={tooltipContent}>
       <IconWithoutRef
         role="button"
-        onClick={onClick}
+        onClick={disabled ? undefined : onClick}
         css={(theme: SupersetTheme) => ({
           cursor: 'pointer',
           color: theme.colors.grayscale.base,

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(
+      <NotificationMethod
+        setting={{
+          ...mockSettingSlackV2,
+          method: NotificationMethodOption.Slack,
+        }}
+        index={0}
+        onUpdate={mockOnUpdate}
+        onRemove={mockOnRemove}
+        onInputChange={mockOnInputChange}
+        email_subject={mockEmailSubject}
+        defaultSubject={mockDefaultSubject}
+        setErrorSubject={mockSetErrorSubject}
+      />,
+    );
+
+    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(
+        <NotificationMethod
+          setting={mockSettingSlackV2}
+          index={0}
+          onUpdate={mockOnUpdate}
+          onRemove={mockOnRemove}
+          onInputChange={mockOnInputChange}
+          email_subject={mockEmailSubject}
+          defaultSubject={mockDefaultSubject}
+          setErrorSubject={mockSetErrorSubject}
+        />,
+      );
+
+      // 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();
+      });
+    });
+  });
 });

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<NotificationMethodProps> = ({
   ]);
 
   const [useSlackV1, setUseSlackV1] = useState<boolean>(false);
+  const [isSlackChannelsLoading, setIsSlackChannelsLoading] =
+    useState<boolean>(true);
 
   const onMethodChange = (selected: {
     label: string;
@@ -248,16 +254,70 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
     searchString = '',
     types = [],
     exactMatch = false,
+    force = false,
   }: {
     searchString?: string | undefined;
     types?: string[];
     exactMatch?: boolean | undefined;
+    force?: boolean | undefined;
   } = {}): Promise<JsonResponse> => {
-    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<NotificationMethodProps> = ({
         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<NotificationMethodProps> = ({
                   </>
                 ) : (
                   // for SlackV2
-                  <Select
-                    ariaLabel={t('Select channels')}
-                    mode="multiple"
-                    name="recipients"
-                    value={slackRecipients}
-                    options={slackOptions}
-                    onChange={onSlackRecipientsChange}
-                    allowClear
-                    data-test="recipients"
-                    allowSelectAll={false}
-                    labelInValue
-                  />
+                  <div className="input-container">
+                    <Select
+                      ariaLabel={t('Select channels')}
+                      mode="multiple"
+                      name="recipients"
+                      value={slackRecipients}
+                      options={slackOptions}
+                      onChange={onSlackRecipientsChange}
+                      allowClear
+                      data-test="recipients"
+                      loading={isSlackChannelsLoading}
+                      allowSelectAll={false}
+                      labelInValue
+                    />
+                    <RefreshLabel
+                      onClick={() => updateSlackOptions({ force: true })}
+                      tooltipContent={t('Force refresh Slack channels list')}
+                      disabled={isSlackChannelsLoading}
+                    />
+                  </div>
                 )}
               </div>
             </StyledInputContainer>

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:

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"):