display private channels

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) },
         );

superset-frontend/src/components/Select/utils.tsx

@@ -46,6 +46,14 @@ export function isLabeledValue(value: unknown): value is AntdLabeledValue {
 export function getValue(
   option: string | number | AntdLabeledValue | null | undefined,
 ) {
+  console.log(
+    'option:',
+    option,
+    'isLabeledValue(option):',
+    isLabeledValue(option),
+    'option.value:',
+    isLabeledValue(option) ? option.value : option,
+  );
   return isLabeledValue(option) ? option.value : option;
 }
 
@@ -75,6 +83,16 @@ export function hasOption(
   options?: V | LabeledValue | (V | LabeledValue)[],
   checkLabel = false,
 ): boolean {
+  console.log(
+    'value:',
+    value,
+    'options:',
+    options,
+    'checkLabel:',
+    checkLabel,
+    'getOption(value, options, checkLabel):',
+    getOption(value, options, checkLabel),
+  );
   return getOption(value, options, checkLabel) !== undefined;
 }
 

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,27 +92,68 @@ 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(),
       ),
     )
     .filter(val => !!val) as { label: string; value: string }[];
 };
 
+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<NotificationMethodProps> = ({
   setting = null,
   index,
@@ -130,21 +173,15 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
   >([]);
   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<SlackOptionsType>([
+    {
+      label: '',
+      options: [],
+    },
+  ]);
 
   const [useSlackV1, setUseSlackV1] = useState<boolean>(false);
 
-  const mapChannelsToOptions = (result: SlackChannel[]) =>
-    result.map((channel: SlackChannel) => ({
-      label: `${channel.name} ${
-        channel.is_member ? '' : '<i>(Bot not in channel)</i>'
-      }`,
-      value: channel.id,
-    }));
-
   const onMethodChange = (selected: {
     label: string;
     value: NotificationMethodOption;
@@ -162,37 +199,50 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
     }
   };
 
+  const fetchSlackChannels = async ({
+    searchString = '',
+    types = [],
+    exactMatch = false,
+  }: {
+    searchString?: string | undefined;
+    types?: string[];
+    exactMatch?: boolean | undefined;
+  } = {}): Promise<JsonResponse> => {
+    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<NotificationMethodProps> = ({
     }
   }, [method]);
 
-  const formattedOptions = useMemo(
+  const methodOptions = useMemo(
     () =>
       (options || [])
         .filter(
@@ -300,9 +350,9 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
               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 ? (
               <span
@@ -385,12 +435,14 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
                     mode="multiple"
                     name="recipients"
                     value={slackRecipients}
-                    options={slackOptions.data}
+                    options={slackOptions}
                     onChange={onSlackRecipientsChange}
                     allowClear
                     data-test="recipients"
                     allowSelectAll={false}
                     labelInValue
+                    autoClearSearchValue={false}
+                    virtual={false}
                   />
                 )}
               </div>

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(<RecipientIcon type={NotificationMethodOption.Email} />);
+    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(<RecipientIcon type={NotificationMethodOption.Slack} />);
+    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(<RecipientIcon type={NotificationMethodOption.SlackV2} />);
+    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(<RecipientIcon type="unknown" />);
+    const icons = screen.queryByLabelText(/.*/);
+    expect(icons).not.toBeInTheDocument();
+  });
+});

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 = {

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)

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

superset/reports/notifications/email.py

@@ -135,7 +135,7 @@ def _get_content(self) -> EmailContent:
         for msgid in images.keys():
             img_tags.append(
                 f"""<div class="image">
-                    <img width="1000px" src="cid:{msgid}">
+                    <img width="1000" src="cid:{msgid}">
                 </div>
                 """
             )
@@ -153,6 +153,7 @@ def _get_content(self) -> EmailContent:
                   }}
                   .image{{
                       margin-bottom: 18px;
+                      min-width: 1000px;
                   }}
                 </style>
               </head>

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

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"},
     },
 }