From 78d8089b1895ed60a38e767d5f63ef24ad27294f Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Tue, 24 Aug 2021 17:28:50 -0700 Subject: [PATCH] fix: Disable Slack notification method if no api token (#16367) * feat: allow company choose alert/report notification methods * fix: disable Slack as notification method if no api token * fix unit test * improve typing --- .../CRUD/alert/AlertReportModal.test.jsx | 3 +- .../src/views/CRUD/alert/AlertReportModal.tsx | 36 +++++++++++-------- .../alert/components/NotificationMethod.tsx | 19 +++++----- .../src/views/CRUD/alert/types.ts | 4 ++- .../src/views/CRUD/data/database/state.ts | 2 +- superset-frontend/src/views/types.ts | 3 ++ superset/views/base.py | 15 +++++++- 7 files changed, 55 insertions(+), 27 deletions(-) diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx index 1509bff492..7d5e60772d 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx +++ b/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx @@ -326,9 +326,10 @@ describe('AlertReportModal', () => { }); await waitForComponentToPaint(wrapper); + // use default config: only show Email as notification option. expect( wrapper.find('[data-test="notification-add"]').props().status, - ).toEqual('disabled'); + ).toEqual('hidden'); act(() => { wrapper .find('[data-test="select-delivery-method"]') diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx index 6a3001d1a4..56e1eba7fb 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx +++ b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx @@ -43,10 +43,9 @@ import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; import withToasts from 'src/messageToasts/enhancers/withToasts'; import Owner from 'src/types/Owner'; import TextAreaControl from 'src/explore/components/controls/TextAreaControl'; -import { AlertReportCronScheduler } from './components/AlertReportCronScheduler'; -import { NotificationMethod } from './components/NotificationMethod'; - +import { useCommonConf } from 'src/views/CRUD/data/database/state'; import { + NotificationMethodOption, AlertObject, ChartObject, DashboardObject, @@ -54,7 +53,9 @@ import { MetaObject, Operator, Recipient, -} from './types'; +} from 'src/views/CRUD/alert/types'; +import { AlertReportCronScheduler } from './components/AlertReportCronScheduler'; +import { NotificationMethod } from './components/NotificationMethod'; const TIMEOUT_MIN = 1; const TEXT_BASED_VISUALIZATION_TYPES = [ @@ -78,7 +79,7 @@ interface AlertReportModalProps { show: boolean; } -const NOTIFICATION_METHODS: NotificationMethod[] = ['Email', 'Slack']; +const DEFAULT_NOTIFICATION_METHODS: NotificationMethodOption[] = ['Email']; const DEFAULT_NOTIFICATION_FORMAT = 'PNG'; const CONDITIONS = [ { @@ -381,12 +382,10 @@ const NotificationMethodAdd: FunctionComponent = ({ ); }; -type NotificationMethod = 'Email' | 'Slack'; - type NotificationSetting = { - method?: NotificationMethod; + method?: NotificationMethodOption; recipients: string; - options: NotificationMethod[]; + options: NotificationMethodOption[]; }; const AlertReportModal: FunctionComponent = ({ @@ -397,6 +396,10 @@ const AlertReportModal: FunctionComponent = ({ alert = null, isReport = false, }) => { + const conf = useCommonConf(); + const allowedNotificationMethods: NotificationMethodOption[] = + conf?.ALERT_REPORTS_NOTIFICATION_METHODS || DEFAULT_NOTIFICATION_METHODS; + const [disableSave, setDisableSave] = useState(true); const [ currentAlert, @@ -435,12 +438,14 @@ const AlertReportModal: FunctionComponent = ({ settings.push({ recipients: '', - options: NOTIFICATION_METHODS, // TODO: Need better logic for this + options: allowedNotificationMethods, }); setNotificationSettings(settings); setNotificationAddState( - settings.length === NOTIFICATION_METHODS.length ? 'hidden' : 'disabled', + settings.length === allowedNotificationMethods.length + ? 'hidden' + : 'disabled', ); }; @@ -910,16 +915,18 @@ const AlertReportModal: FunctionComponent = ({ ? JSON.parse(setting.recipient_config_json) : {}; return { - method: setting.type as NotificationMethod, + method: setting.type, // @ts-ignore: Type not assignable recipients: config.target || setting.recipient_config_json, - options: NOTIFICATION_METHODS as NotificationMethod[], // Need better logic for this + options: allowedNotificationMethods, }; }); setNotificationSettings(settings); setNotificationAddState( - settings.length === NOTIFICATION_METHODS.length ? 'hidden' : 'active', + settings.length === allowedNotificationMethods.length + ? 'hidden' + : 'active', ); setContentType(resource.chart ? 'chart' : 'dashboard'); setReportFormat( @@ -1309,6 +1316,7 @@ const AlertReportModal: FunctionComponent = ({ diff --git a/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx b/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx index 905e85e7e5..12b59a7afd 100644 --- a/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx +++ b/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx @@ -20,6 +20,7 @@ import React, { FunctionComponent, useState } from 'react'; import { styled, t, useTheme } from '@superset-ui/core'; import { Select } from 'src/components'; import Icons from 'src/components/Icons'; +import { NotificationMethodOption } from 'src/views/CRUD/alert/types'; import { StyledInputContainer } from '../AlertReportModal'; const StyledNotificationMethod = styled.div` @@ -49,12 +50,10 @@ const StyledNotificationMethod = styled.div` } `; -type NotificationMethod = 'Email' | 'Slack'; - type NotificationSetting = { - method?: NotificationMethod; + method?: NotificationMethodOption; recipients: string; - options: NotificationMethod[]; + options: NotificationMethodOption[]; }; interface NotificationMethodProps { @@ -80,7 +79,7 @@ export const NotificationMethod: FunctionComponent = ({ return null; } - const onMethodChange = (method: NotificationMethod) => { + const onMethodChange = (method: NotificationMethodOption) => { // Since we're swapping the method, reset the recipients setRecipientValue(''); if (onUpdate) { @@ -126,10 +125,12 @@ export const NotificationMethod: FunctionComponent = ({ data-test="select-delivery-method" onChange={onMethodChange} placeholder={t('Select Delivery Method')} - options={(options || []).map((method: NotificationMethod) => ({ - label: method, - value: method, - }))} + options={(options || []).map( + (method: NotificationMethodOption) => ({ + label: method, + value: method, + }), + )} value={method} /> diff --git a/superset-frontend/src/views/CRUD/alert/types.ts b/superset-frontend/src/views/CRUD/alert/types.ts index f67a6042df..3f939a03bc 100644 --- a/superset-frontend/src/views/CRUD/alert/types.ts +++ b/superset-frontend/src/views/CRUD/alert/types.ts @@ -40,11 +40,13 @@ export type DatabaseObject = { id: number; }; +export type NotificationMethodOption = 'Email' | 'Slack'; + export type Recipient = { recipient_config_json: { target: string; }; - type: string; + type: NotificationMethodOption; }; export type MetaObject = { diff --git a/superset-frontend/src/views/CRUD/data/database/state.ts b/superset-frontend/src/views/CRUD/data/database/state.ts index 777fdc87b5..9a16371236 100644 --- a/superset-frontend/src/views/CRUD/data/database/state.ts +++ b/superset-frontend/src/views/CRUD/data/database/state.ts @@ -21,5 +21,5 @@ import { useSelector } from 'react-redux'; import { ViewState } from 'src/views/types'; export function useCommonConf() { - return useSelector((state: ViewState) => state.common.conf); + return useSelector((state: ViewState) => state?.common?.conf); } diff --git a/superset-frontend/src/views/types.ts b/superset-frontend/src/views/types.ts index 26e0f099c7..c0ffead6a5 100644 --- a/superset-frontend/src/views/types.ts +++ b/superset-frontend/src/views/types.ts @@ -16,11 +16,14 @@ * specific language governing permissions and limitations * under the License. */ +import { NotificationMethodOption } from './CRUD/alert/types'; + export interface ViewState { common: { conf: { SQLALCHEMY_DOCS_URL: string; SQLALCHEMY_DISPLAY_TEXT: string; + ALERT_REPORTS_NOTIFICATION_METHODS: NotificationMethodOption[]; }; }; messageToast: Array; diff --git a/superset/views/base.py b/superset/views/base.py index 3d4f988259..61bd312fb6 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -70,6 +70,7 @@ from superset.exceptions import ( SupersetSecurityException, ) from superset.models.helpers import ImportExportMixin +from superset.models.reports import ReportRecipientType from superset.translations.utils import get_language_pack from superset.typing import FlaskResponse from superset.utils import core as utils @@ -346,9 +347,21 @@ def common_bootstrap_payload() -> Dict[str, Any]: messages = get_flashed_messages(with_categories=True) locale = str(get_locale()) + # should not expose API TOKEN to frontend + frontend_config = {k: conf.get(k) for k in FRONTEND_CONF_KEYS} + if conf.get("SLACK_API_TOKEN"): + frontend_config["ALERT_REPORTS_NOTIFICATION_METHODS"] = [ + ReportRecipientType.EMAIL, + ReportRecipientType.SLACK, + ] + else: + frontend_config["ALERT_REPORTS_NOTIFICATION_METHODS"] = [ + ReportRecipientType.EMAIL, + ] + bootstrap_data = { "flash_messages": messages, - "conf": {k: conf.get(k) for k in FRONTEND_CONF_KEYS}, + "conf": frontend_config, "locale": locale, "language_pack": get_language_pack(locale), "feature_flags": get_feature_flags(),