diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx index e7fde6cc200f3..593a51c97e033 100644 --- a/superset-frontend/src/components/Select/Select.test.tsx +++ b/superset-frontend/src/components/Select/Select.test.tsx @@ -188,6 +188,20 @@ test('does not add a new option if the value is already in the options', async ( expect(options).toHaveLength(1); }); +test('does not add new options when the value is in a nested/grouped option', async () => { + const options = [ + { + label: 'Group', + options: [OPTIONS[0]], + }, + ]; + render(); await open(); diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx index 9356880ba8a35..bebb788879794 100644 --- a/superset-frontend/src/components/Select/Select.tsx +++ b/superset-frontend/src/components/Select/Select.tsx @@ -182,8 +182,18 @@ const Select = forwardRef( // add selected values to options list if they are not in it const fullSelectOptions = useMemo(() => { + // check to see if selectOptions are grouped + let groupedOptions: SelectOptionsType; + if (selectOptions.some(opt => opt.options)) { + groupedOptions = selectOptions.reduce( + (acc, group) => [...acc, ...group.options], + [] as SelectOptionsType, + ); + } const missingValues: SelectOptionsType = ensureIsArray(selectValue) - .filter(opt => !hasOption(getValue(opt), selectOptions)) + .filter( + opt => !hasOption(getValue(opt), groupedOptions || selectOptions), + ) .map(opt => isLabeledValue(opt) ? opt : { value: opt, label: String(opt) }, ); diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx index 9c344aac31c64..f0c22cfccb246 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx @@ -17,7 +17,7 @@ * under the License. */ import { render, screen, fireEvent } from 'spec/helpers/testing-library'; -import { NotificationMethod, mapSlackOptions } from './NotificationMethod'; +import { NotificationMethod, mapSlackValues } from './NotificationMethod'; import { NotificationMethodOption, NotificationSetting } from '../types'; const mockOnUpdate = jest.fn(); @@ -138,14 +138,12 @@ describe('NotificationMethod', () => { it('should correctly map recipients when method is SlackV2', () => { const method = 'SlackV2'; const recipientValue = 'user1,user2'; - const slackOptions = { - data: [ - { label: 'User One', value: 'user1' }, - { label: 'User Two', value: 'user2' }, - ], - }; + const slackOptions: { label: string; value: string }[] = [ + { label: 'User One', value: 'user1' }, + { label: 'User Two', value: 'user2' }, + ]; - const result = mapSlackOptions({ method, recipientValue, slackOptions }); + const result = mapSlackValues({ method, recipientValue, slackOptions }); expect(result).toEqual([ { label: 'User One', value: 'user1' }, @@ -156,31 +154,25 @@ describe('NotificationMethod', () => { it('should return an empty array when recipientValue is an empty string', () => { const method = 'SlackV2'; const recipientValue = ''; - const slackOptions = { - data: [ - { label: 'User One', value: 'user1' }, - { label: 'User Two', value: 'user2' }, - { label: 'User Three', value: 'user3' }, - ], - }; + const slackOptions: { label: string; value: string }[] = [ + { label: 'User One', value: 'user1' }, + { label: 'User Two', value: 'user2' }, + ]; - const result = mapSlackOptions({ method, recipientValue, slackOptions }); + const result = mapSlackValues({ method, recipientValue, slackOptions }); expect(result).toEqual([]); }); - // Ensure that the mapSlackOptions function correctly maps recipients when the method is Slack with updated recipient values + // Ensure that the mapSlackValues function correctly maps recipients when the method is Slack with updated recipient values it('should correctly map recipients when method is Slack with updated recipient values', () => { const method = 'Slack'; const recipientValue = 'User One,User Two'; - const slackOptions = { - data: [ - { label: 'User One', value: 'user1' }, - { label: 'User Two', value: 'user2' }, - { label: 'User Three', value: 'user3' }, - ], - }; - - const result = mapSlackOptions({ method, recipientValue, slackOptions }); + const slackOptions: { label: string; value: string }[] = [ + { label: 'User One', value: 'user1' }, + { label: 'User Two', value: 'user2' }, + ]; + + const result = mapSlackValues({ method, recipientValue, slackOptions }); expect(result).toEqual([ { label: 'User One', value: 'user1' }, diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx index e8b93580d11cd..92ef454d076e2 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx @@ -23,9 +23,11 @@ import { useEffect, useMemo, } from 'react'; +import rison from 'rison'; import { FeatureFlag, + JsonResponse, SupersetClient, isFeatureEnabled, styled, @@ -90,20 +92,20 @@ const TRANSLATIONS = { ), }; -export const mapSlackOptions = ({ +export const mapSlackValues = ({ method, recipientValue, slackOptions, }: { method: string; recipientValue: string; - slackOptions: { data: { label: string; value: string }[] }; + slackOptions: { label: string; value: string }[]; }) => { const prop = method === NotificationMethodOption.SlackV2 ? 'value' : 'label'; return recipientValue .split(',') .map(recipient => - slackOptions.data.find( + slackOptions.find( option => option[prop].trim().toLowerCase() === recipient.trim().toLowerCase(), ), @@ -111,6 +113,47 @@ export const mapSlackOptions = ({ .filter(val => !!val) as { label: string; value: string }[]; }; +export const mapChannelsToOptions = (result: SlackChannel[]) => { + const publicChannels: SlackChannel[] = []; + const privateChannels: SlackChannel[] = []; + + result.forEach(channel => { + if (channel.is_private) { + privateChannels.push(channel); + } else { + publicChannels.push(channel); + } + }); + + return [ + { + label: 'Public Channels', + options: publicChannels.map((channel: SlackChannel) => ({ + label: `${channel.name} ${ + channel.is_member ? '' : '(Bot not in channel)' + }`, + value: channel.id, + key: channel.id, + })), + key: 'public', + }, + { + label: 'Private Channels (Bot in channel)', + options: privateChannels.map((channel: SlackChannel) => ({ + label: channel.name, + value: channel.id, + key: channel.id, + })), + key: 'private', + }, + ]; +}; + +type SlackOptionsType = { + label: string; + options: { label: string; value: string }[]; +}[]; + export const NotificationMethod: FunctionComponent = ({ setting = null, index, @@ -130,21 +173,15 @@ export const NotificationMethod: FunctionComponent = ({ >([]); const [error, setError] = useState(false); const theme = useTheme(); - const [slackOptions, setSlackOptions] = useState<{ - data: { label: string; value: string }[]; - totalCount: number; - }>({ data: [], totalCount: 0 }); + const [slackOptions, setSlackOptions] = useState([ + { + label: '', + options: [], + }, + ]); const [useSlackV1, setUseSlackV1] = useState(false); - const mapChannelsToOptions = (result: SlackChannel[]) => - result.map((channel: SlackChannel) => ({ - label: `${channel.name} ${ - channel.is_member ? '' : '(Bot not in channel)' - }`, - value: channel.id, - })); - const onMethodChange = (selected: { label: string; value: NotificationMethodOption; @@ -162,37 +199,50 @@ export const NotificationMethod: FunctionComponent = ({ } }; + const fetchSlackChannels = async ({ + searchString = '', + types = [], + exactMatch = false, + }: { + searchString?: string | undefined; + types?: string[]; + exactMatch?: boolean | undefined; + } = {}): Promise => { + const queryString = rison.encode({ searchString, types, exactMatch }); + const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`; + return SupersetClient.get({ endpoint }); + }; + useEffect(() => { - const endpoint = '/api/v1/report/slack_channels/'; if ( method && [ NotificationMethodOption.Slack, NotificationMethodOption.SlackV2, ].includes(method) && - !slackOptions.data.length + !slackOptions[0].options.length ) { - SupersetClient.get({ endpoint }) + fetchSlackChannels({ types: ['public_channel', 'private_channel'] }) .then(({ json }) => { - const { result, count } = json; + const { result } = json; - const options: { label: any; value: any }[] = - mapChannelsToOptions(result); + const options: SlackOptionsType = mapChannelsToOptions(result); - setSlackOptions({ - data: options, - totalCount: (count ?? options.length) as number, - }); + setSlackOptions(options); - // on first load, if the Slack channels api succeeds, - // then convert this to SlackV2 if (isFeatureEnabled(FeatureFlag.AlertReportSlackV2)) { - // convert v1 names to v2 ids + // map existing ids to names for display + // or names to ids if slack v1 + const [publicOptions, privateOptions] = options; + setSlackRecipients( - mapSlackOptions({ + mapSlackValues({ method, recipientValue, - slackOptions: { data: options }, + slackOptions: [ + ...publicOptions.options, + ...privateOptions.options, + ], }), ); if (method === NotificationMethodOption.Slack) { @@ -210,7 +260,7 @@ export const NotificationMethod: FunctionComponent = ({ } }, [method]); - const formattedOptions = useMemo( + const methodOptions = useMemo( () => (options || []) .filter( @@ -300,9 +350,9 @@ export const NotificationMethod: FunctionComponent = ({ labelInValue onChange={onMethodChange} placeholder={t('Select Delivery Method')} - options={formattedOptions} + options={methodOptions} showSearch - value={formattedOptions.find(option => option.value === method)} + value={methodOptions.find(option => option.value === method)} /> {index !== 0 && !!onRemove ? ( = ({ mode="multiple" name="recipients" value={slackRecipients} - options={slackOptions.data} + options={slackOptions} onChange={onSlackRecipientsChange} allowClear data-test="recipients" diff --git a/superset-frontend/src/features/alerts/components/RecipientIcon.test.tsx b/superset-frontend/src/features/alerts/components/RecipientIcon.test.tsx new file mode 100644 index 0000000000000..b3d1ff1edc82e --- /dev/null +++ b/superset-frontend/src/features/alerts/components/RecipientIcon.test.tsx @@ -0,0 +1,32 @@ +import { render, screen } from 'spec/helpers/testing-library'; +import RecipientIcon from './RecipientIcon'; +import { NotificationMethodOption } from '../types'; + +describe('RecipientIcon', () => { + it('should render the email icon when type is Email', () => { + render(); + const regexPattern = new RegExp(NotificationMethodOption.Email, 'i'); + const emailIcon = screen.getByLabelText(regexPattern); + expect(emailIcon).toBeInTheDocument(); + }); + + it('should render the Slack icon when type is Slack', () => { + render(); + const regexPattern = new RegExp(NotificationMethodOption.Slack, 'i'); + const slackIcon = screen.getByLabelText(regexPattern); + expect(slackIcon).toBeInTheDocument(); + }); + + it('should render the Slack icon when type is SlackV2', () => { + render(); + const regexPattern = new RegExp(NotificationMethodOption.Slack, 'i'); + const slackIcon = screen.getByLabelText(regexPattern); + expect(slackIcon).toBeInTheDocument(); + }); + + it('should not render any icon when type is not recognized', () => { + render(); + const icons = screen.queryByLabelText(/.*/); + expect(icons).not.toBeInTheDocument(); + }); +}); diff --git a/superset-frontend/src/features/alerts/types.ts b/superset-frontend/src/features/alerts/types.ts index bf777072d07a4..9c6a2e9c8bf09 100644 --- a/superset-frontend/src/features/alerts/types.ts +++ b/superset-frontend/src/features/alerts/types.ts @@ -57,6 +57,7 @@ export type SlackChannel = { id: string; name: string; is_member: boolean; + is_private: boolean; }; export type Recipient = { diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index e8d79337e56f5..3ec5bdfa97b22 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -76,7 +76,7 @@ from superset.utils.decorators import logs_context, transaction from superset.utils.pdf import build_pdf_from_screenshots from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot -from superset.utils.slack import get_channels_with_search +from superset.utils.slack import get_channels_with_search, SlackChannelTypes from superset.utils.urls import get_url_path logger = logging.getLogger(__name__) @@ -138,8 +138,18 @@ def update_report_schedule_slack_v2(self) -> None: if recipient_copy.type == ReportRecipientType.SLACK: recipient_copy.type = ReportRecipientType.SLACKV2 slack_recipients = json.loads(recipient_copy.recipient_config_json) + # we need to ensure that existing reports can also fetch + # ids from private channels recipient_copy.recipient_config_json = json.dumps( - {"target": get_channels_with_search(slack_recipients["target"])} + { + "target": get_channels_with_search( + slack_recipients["target"], + types=[ + SlackChannelTypes.PRIVATE, + SlackChannelTypes.PUBLIC, + ], + ) + } ) updated_recipients.append(recipient_copy) diff --git a/superset/reports/api.py b/superset/reports/api.py index 4cbe92e283d9a..5ff90165b627f 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -571,8 +571,13 @@ def slack_channels(self, **kwargs: Any) -> Response: $ref: '#/components/responses/500' """ try: - search_string = kwargs.get("rison", {}).get("search_string") - channels = get_channels_with_search(search_string=search_string) + params = kwargs.get("rison", {}) + search_string = params.get("search_string") + types = params.get("types", []) + exact_match = params.get("exact_match", False) + channels = get_channels_with_search( + search_string=search_string, types=types, exact_match=exact_match + ) return self.response(200, result=channels) except SupersetException as ex: logger.error("Error fetching slack channels %s", str(ex)) diff --git a/superset/reports/notifications/email.py b/superset/reports/notifications/email.py index d9ac6dca22a27..d5939f772af07 100644 --- a/superset/reports/notifications/email.py +++ b/superset/reports/notifications/email.py @@ -135,7 +135,7 @@ def _get_content(self) -> EmailContent: for msgid in images.keys(): img_tags.append( f"""
- +
""" ) @@ -153,6 +153,7 @@ def _get_content(self) -> EmailContent: }} .image{{ margin-bottom: 18px; + min-width: 1000px; }} diff --git a/superset/reports/notifications/slackv2.py b/superset/reports/notifications/slackv2.py index 5b449474013f1..8b864f4148603 100644 --- a/superset/reports/notifications/slackv2.py +++ b/superset/reports/notifications/slackv2.py @@ -87,7 +87,6 @@ def send(self) -> None: body = self._get_body(content=self._content) channels = self._get_channels() - logger.info("channels: %s", channels) if not channels: raise NotificationParamException("No recipients saved in the report") diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 20cbff87b8efb..3de34b5b08b84 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -52,7 +52,12 @@ get_slack_channels_schema = { "type": "object", "properties": { - "seach_string": {"type": "string"}, + "search_string": {"type": "string"}, + "types": { + "type": "array", + "items": {"type": "string", "enum": ["public_channel", "private_channel"]}, + }, + "exact_match": {"type": "boolean"}, }, } diff --git a/superset/utils/slack.py b/superset/utils/slack.py index f45ab2e33c530..0d067076890f7 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -17,6 +17,7 @@ import logging +from typing import Optional from flask import current_app from slack_sdk import WebClient @@ -24,10 +25,16 @@ from superset import feature_flag_manager from superset.exceptions import SupersetException +from superset.utils.backports import StrEnum logger = logging.getLogger(__name__) +class SlackChannelTypes(StrEnum): + PUBLIC = "public_channel" + PRIVATE = "private_channel" + + class SlackClientError(Exception): pass @@ -39,7 +46,12 @@ def get_slack_client() -> WebClient: return WebClient(token=token, proxy=current_app.config["SLACK_PROXY"]) -def get_channels_with_search(search_string: str = "", limit: int = 999) -> list[str]: +def get_channels_with_search( + search_string: str = "", + limit: int = 999, + types: Optional[list[SlackChannelTypes]] = None, + exact_match: bool = False, +) -> list[str]: """ The slack api is paginated but does not include search, so we need to fetch all channels and filter them ourselves @@ -50,10 +62,12 @@ def get_channels_with_search(search_string: str = "", limit: int = 999) -> list[ client = get_slack_client() channels = [] 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 + limit=limit, cursor=cursor, exclude_archived=True, **extra_params ) channels.extend(response.data["channels"]) cursor = response.data.get("response_metadata", {}).get("next_cursor") @@ -66,12 +80,21 @@ def get_channels_with_search(search_string: str = "", limit: int = 999) -> list[ search.lower() for search in (search_string.split(",") if search_string else []) ] + print(channels) channels = [ channel for channel in channels if any( - search in channel["name"].lower() or search in channel["id"].lower() + ( + search in channel["name"].lower() + or search in channel["id"].lower() + if exact_match + else ( + search == channel["name"].lower() + or search == channel["id"].lower() + ) + ) for search in search_array ) ]