sadpandajoe commented on code in PR #35832:
URL: https://github.com/apache/superset/pull/35832#discussion_r3826557839
##########
superset/utils/slack.py:
##########
@@ -59,80 +62,275 @@ 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()
+def _fetch_channels_without_search(
+ client: WebClient,
+ channel_schema: SlackChannelSchema,
+ types_param: str,
+ cursor: Optional[str],
+ limit: int,
+) -> dict[str, Any]:
+ """Fetch channels without search filtering, paginating for large limits."""
channels: list[SlackChannelSchema] = []
- extra_params = {"types": ",".join(SlackChannelTypes)}
- cursor = None
- page_count = 0
+ slack_cursor = cursor
+
+ while True:
+ response = client.conversations_list(
+ limit=999,
+ cursor=slack_cursor,
+ exclude_archived=True,
+ types=types_param,
+ )
+
+ page_channels = [
+ channel_schema.load(channel) for channel in
response.data["channels"]
+ ]
+ channels.extend(page_channels)
+
+ slack_cursor = response.data.get("response_metadata",
{}).get("next_cursor")
+
+ if not slack_cursor or len(channels) >= limit:
+ break
+
+ return {
+ "result": channels[:limit],
Review Comment:
Returning Slack's page cursor after slicing this response makes part of the
page unreachable: with `limit=100`, a Slack page containing 150 channels
returns only the first 100 and the next request resumes after all 150. Could
the API retain the in-page remainder or request no more than the caller's
remaining limit?
##########
superset/reports/api.py:
##########
@@ -577,14 +582,31 @@ def slack_channels(self, **kwargs: Any) -> Response:
search_string = params.get("search_string")
types = params.get("types", [])
exact_match = params.get("exact_match", False)
+ cursor = params.get("cursor")
+ limit = params.get("limit", 100)
force = params.get("force", False)
- channels = get_channels_with_search(
+
+ # 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,
types=types,
exact_match=exact_match,
- force=force,
+ cursor=cursor,
+ limit=limit,
)
- return self.response(200, result=channels)
+ return self.response(200, **channels_data)
Review Comment:
The endpoint now returns `next_cursor` and `has_more`, but the response
schema above still documents only `result`; generated clients therefore have no
documented way to paginate. Could the OpenAPI response (and generated artifact,
if applicable) include the new fields?
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -540,24 +551,33 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
) : (
// for SlackV2
<div className="input-container">
- <Select
+ <AsyncSelect
ariaLabel={t('Select channels')}
mode="multiple"
name="recipients"
value={slackRecipients}
- options={slackOptions}
+ options={fetchSlackChannels}
onChange={onSlackRecipientsChange}
+ onSearch={handleSlackSearch}
allowClear
data-test="recipients"
- loading={isSlackChannelsLoading}
- allowSelectAll={false}
- labelInValue
- />
- <RefreshLabel
- onClick={() => updateSlackOptions({ force: true })}
- tooltipContent={t('Force refresh Slack channels list')}
- disabled={isSlackChannelsLoading}
+ fetchOnlyOnSearch={false}
+ pageSize={999}
+ placeholder={t('Select Slack channels')}
+ tokenSeparators={[]}
+ filterOption={() => true}
/>
+ <span
+ role="button"
Review Comment:
This focusable `span` only handles clicks, so pressing Enter or Space does
nothing for keyboard users. Could this use the existing button/refresh control
or add equivalent keyboard activation?
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -248,84 +210,131 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
}
};
- const fetchSlackChannels = async ({
- 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,
- 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,
- ],
- }),
- );
- }
+ const fetchSlackChannels = useCallback(
+ async (
+ search: string,
+ page: number,
+ pageSize: number,
+ ): Promise<{
+ data: { label: string; value: string }[];
+ totalCount: number;
+ }> => {
+ try {
+ const result = await fetchSlackChannelsFromHook({ search, page,
pageSize });
+
+ hasShownErrorToast.current = false;
+
+ return result;
+ } 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;
}
- })
- .catch(e => {
- // Fallback to slack v1 if slack v2 is not compatible
+
+ // Fallback to Slack v1 without clearing recipients to prevent data
loss
setUseSlackV1(true);
- })
- .finally(() => {
- setMethodOptionsLoading(false);
- setIsSlackChannelsLoading(false);
- });
- };
+ // 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
+ });
+ }
+
+ 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
+ [
+ fetchSlackChannelsFromHook,
+ addDangerToast,
+ onUpdate,
+ setting,
+ index,
+ searchGeneration,
+ ],
+ );
+
+ const handleRefreshSlackChannels = refreshChannels;
Review Comment:
This refresh only updates the hook cache; the AsyncSelect keeps its own
fetched options because the `options` callback identity does not change. After
Slack changes, reopening an already loaded dropdown can still show the
pre-refresh list. Could refresh also invalidate or remount the selector's query
cache?
##########
superset/utils/slack.py:
##########
@@ -59,80 +62,275 @@ 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()
+def _fetch_channels_without_search(
+ client: WebClient,
+ channel_schema: SlackChannelSchema,
+ types_param: str,
+ cursor: Optional[str],
+ limit: int,
+) -> dict[str, Any]:
+ """Fetch channels without search filtering, paginating for large limits."""
channels: list[SlackChannelSchema] = []
- extra_params = {"types": ",".join(SlackChannelTypes)}
- cursor = None
- page_count = 0
+ slack_cursor = cursor
+
+ while True:
+ response = client.conversations_list(
+ limit=999,
+ cursor=slack_cursor,
+ exclude_archived=True,
+ types=types_param,
+ )
+
+ page_channels = [
+ channel_schema.load(channel) for channel in
response.data["channels"]
+ ]
+ channels.extend(page_channels)
+
+ slack_cursor = response.data.get("response_metadata",
{}).get("next_cursor")
+
+ if not slack_cursor or len(channels) >= limit:
+ break
+
+ return {
+ "result": channels[:limit],
+ "next_cursor": slack_cursor,
+ "has_more": bool(slack_cursor),
+ }
+
+
+def _fetch_channels_with_search(
+ client: WebClient,
+ channel_schema: SlackChannelSchema,
+ types_param: str,
+ search_string: str,
+ exact_match: bool,
+ cursor: Optional[str],
+ limit: int,
+) -> dict[str, Any]:
+ """Fetch channels with search filtering, streaming through pages."""
+ matches: list[SlackChannelSchema] = []
+ slack_cursor = cursor
+ search_terms = [
+ term.strip().lower() for term in search_string.split(",") if
term.strip()
Review Comment:
This no longer accepts the separator formats already supported for stored
Slack recipients. For example, migration of `alerts;ops` produces one search
term here but two names in `recipients_string_to_list`, so it fails with
missing channels. Could this reuse that parser rather than only splitting
commas?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
