display private channels

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(<Select {...defaultProps} options={options} value={OPTIONS[0]} />);
+  await open();
+  expect(await findSelectOption(OPTIONS[0].label)).toBeInTheDocument();
+  const selectOptions = await findAllSelectOptions();
+  expect(selectOptions).toHaveLength(1);
+});
+
 test('inverts the selection', async () => {
   render(<Select {...defaultProps} invertSelection />);
   await open();

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/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' },

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 }[];
 };
 
+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<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,7 +435,7 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
                     mode="multiple"
                     name="recipients"
                     value={slackRecipients}
-                    options={slackOptions.data}
+                    options={slackOptions}
                     onChange={onSlackRecipientsChange}
                     allowClear
                     data-test="recipients"

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)