From 542ab02abd528edd0364b910d7b4cf12b151f0f9 Mon Sep 17 00:00:00 2001 From: Grace Date: Thu, 19 Aug 2021 18:50:16 -0700 Subject: [PATCH 1/4] feat: allow company choose alert/report notification methods --- .../src/views/CRUD/alert/AlertReportModal.tsx | 33 +++++++++++++------ .../alert/components/NotificationMethod.tsx | 11 +++---- .../src/views/CRUD/data/database/state.ts | 2 +- superset-frontend/src/views/types.ts | 3 ++ superset/config.py | 2 ++ superset/views/base.py | 1 + 6 files changed, 35 insertions(+), 17 deletions(-) diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx index 6a3001d1a4b7..978ac2c3b8a6 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx +++ b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx @@ -43,8 +43,10 @@ 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 { useCommonConf } from 'src/views/CRUD/data/database/state'; import { AlertReportCronScheduler } from './components/AlertReportCronScheduler'; import { NotificationMethod } from './components/NotificationMethod'; +import { NotificationMethodOption } from '../../types'; import { AlertObject, @@ -78,7 +80,10 @@ interface AlertReportModalProps { show: boolean; } -const NOTIFICATION_METHODS: NotificationMethod[] = ['Email', 'Slack']; +const DEFAULT_NOTIFICATION_METHODS: NotificationMethodOption[] = [ + 'Email', + 'Slack', +]; const DEFAULT_NOTIFICATION_FORMAT = 'PNG'; const CONDITIONS = [ { @@ -381,12 +386,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 +400,11 @@ const AlertReportModal: FunctionComponent = ({ alert = null, isReport = false, }) => { + const conf = useCommonConf(); + const allowedNotificationMethods = + conf?.ALERT_REPORTS_ALERTS_NOTIFICATION_METHODS || + DEFAULT_NOTIFICATION_METHODS; + const [disableSave, setDisableSave] = useState(true); const [ currentAlert, @@ -435,12 +443,14 @@ const AlertReportModal: FunctionComponent = ({ settings.push({ recipients: '', - options: NOTIFICATION_METHODS, // TODO: Need better logic for this + options: allowedNotificationMethods, // TODO: Need better logic for this }); setNotificationSettings(settings); setNotificationAddState( - settings.length === NOTIFICATION_METHODS.length ? 'hidden' : 'disabled', + settings.length === allowedNotificationMethods.length + ? 'hidden' + : 'disabled', ); }; @@ -910,16 +920,18 @@ const AlertReportModal: FunctionComponent = ({ ? JSON.parse(setting.recipient_config_json) : {}; return { - method: setting.type as NotificationMethod, + method: setting.type as NotificationMethodOption, // @ts-ignore: Type not assignable recipients: config.target || setting.recipient_config_json, - options: NOTIFICATION_METHODS as NotificationMethod[], // Need better logic for this + options: allowedNotificationMethods as NotificationMethodOption[], // Need better logic for this }; }); setNotificationSettings(settings); setNotificationAddState( - settings.length === NOTIFICATION_METHODS.length ? 'hidden' : 'active', + settings.length === allowedNotificationMethods.length + ? 'hidden' + : 'active', ); setContentType(resource.chart ? 'chart' : 'dashboard'); setReportFormat( @@ -1309,6 +1321,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 905e85e7e547..0b893b54e263 100644 --- a/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx +++ b/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx @@ -21,6 +21,7 @@ import { styled, t, useTheme } from '@superset-ui/core'; import { Select } from 'src/components'; import Icons from 'src/components/Icons'; import { StyledInputContainer } from '../AlertReportModal'; +import { NotificationMethodOption } from '../../../types'; const StyledNotificationMethod = styled.div` margin-bottom: 10px; @@ -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,7 +125,7 @@ export const NotificationMethod: FunctionComponent = ({ data-test="select-delivery-method" onChange={onMethodChange} placeholder={t('Select Delivery Method')} - options={(options || []).map((method: NotificationMethod) => ({ + options={(options || []).map((method: NotificationMethodOption) => ({ label: method, value: method, }))} diff --git a/superset-frontend/src/views/CRUD/data/database/state.ts b/superset-frontend/src/views/CRUD/data/database/state.ts index 777fdc87b508..9a16371236e2 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 26e0f099c70c..3fedbe593e06 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. */ +export type NotificationMethodOption = 'Email' | 'Slack'; + export interface ViewState { common: { conf: { SQLALCHEMY_DOCS_URL: string; SQLALCHEMY_DISPLAY_TEXT: string; + ALERT_REPORTS_ALERTS_NOTIFICATION_METHODS: NotificationMethodOption[]; }; }; messageToast: Array; diff --git a/superset/config.py b/superset/config.py index c0946740c57f..a91c1df83a92 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1001,6 +1001,8 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # If set to true no notification is sent, the worker will just log a message. # Useful for debugging ALERT_REPORTS_NOTIFICATION_DRY_RUN = False +# Allowed notification methods: Email, Slack +ALERT_REPORTS_ALERTS_NOTIFICATION_METHODS = ["Email", "Slack"] # A custom prefix to use on all Alerts & Reports emails EMAIL_REPORTS_SUBJECT_PREFIX = "[Report] " diff --git a/superset/views/base.py b/superset/views/base.py index 3d4f98825965..db9130750648 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -98,6 +98,7 @@ "SQLALCHEMY_DOCS_URL", "SQLALCHEMY_DISPLAY_TEXT", "GLOBAL_ASYNC_QUERIES_WEBSOCKET_URL", + "ALERT_REPORTS_ALERTS_NOTIFICATION_METHODS", ) logger = logging.getLogger(__name__) From 8d9ce58b6e19bf222043010bf613f00fb4513bb7 Mon Sep 17 00:00:00 2001 From: Grace Date: Mon, 23 Aug 2021 11:27:54 -0700 Subject: [PATCH 2/4] fix: disable Slack as notification method if no api token --- .../src/views/CRUD/alert/AlertReportModal.tsx | 10 +++------- superset-frontend/src/views/types.ts | 2 +- superset/config.py | 2 -- superset/views/base.py | 16 ++++++++++++++-- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx index 978ac2c3b8a6..efce49e311e0 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx +++ b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx @@ -80,10 +80,7 @@ interface AlertReportModalProps { show: boolean; } -const DEFAULT_NOTIFICATION_METHODS: NotificationMethodOption[] = [ - 'Email', - 'Slack', -]; +const DEFAULT_NOTIFICATION_METHODS: NotificationMethodOption[] = ['Email']; const DEFAULT_NOTIFICATION_FORMAT = 'PNG'; const CONDITIONS = [ { @@ -402,8 +399,7 @@ const AlertReportModal: FunctionComponent = ({ }) => { const conf = useCommonConf(); const allowedNotificationMethods = - conf?.ALERT_REPORTS_ALERTS_NOTIFICATION_METHODS || - DEFAULT_NOTIFICATION_METHODS; + conf?.ALERT_REPORTS_NOTIFICATION_METHODS || DEFAULT_NOTIFICATION_METHODS; const [disableSave, setDisableSave] = useState(true); const [ @@ -443,7 +439,7 @@ const AlertReportModal: FunctionComponent = ({ settings.push({ recipients: '', - options: allowedNotificationMethods, // TODO: Need better logic for this + options: allowedNotificationMethods, }); setNotificationSettings(settings); diff --git a/superset-frontend/src/views/types.ts b/superset-frontend/src/views/types.ts index 3fedbe593e06..10b044daad22 100644 --- a/superset-frontend/src/views/types.ts +++ b/superset-frontend/src/views/types.ts @@ -23,7 +23,7 @@ export interface ViewState { conf: { SQLALCHEMY_DOCS_URL: string; SQLALCHEMY_DISPLAY_TEXT: string; - ALERT_REPORTS_ALERTS_NOTIFICATION_METHODS: NotificationMethodOption[]; + ALERT_REPORTS_NOTIFICATION_METHODS: NotificationMethodOption[]; }; }; messageToast: Array; diff --git a/superset/config.py b/superset/config.py index a91c1df83a92..c0946740c57f 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1001,8 +1001,6 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # If set to true no notification is sent, the worker will just log a message. # Useful for debugging ALERT_REPORTS_NOTIFICATION_DRY_RUN = False -# Allowed notification methods: Email, Slack -ALERT_REPORTS_ALERTS_NOTIFICATION_METHODS = ["Email", "Slack"] # A custom prefix to use on all Alerts & Reports emails EMAIL_REPORTS_SUBJECT_PREFIX = "[Report] " diff --git a/superset/views/base.py b/superset/views/base.py index db9130750648..61bd312fb601 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -70,6 +70,7 @@ 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 @@ -98,7 +99,6 @@ "SQLALCHEMY_DOCS_URL", "SQLALCHEMY_DISPLAY_TEXT", "GLOBAL_ASYNC_QUERIES_WEBSOCKET_URL", - "ALERT_REPORTS_ALERTS_NOTIFICATION_METHODS", ) logger = logging.getLogger(__name__) @@ -347,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(), From 5eab082c294d3520ee9e1c76acd50daeaac82aea Mon Sep 17 00:00:00 2001 From: Grace Date: Mon, 23 Aug 2021 12:28:37 -0700 Subject: [PATCH 3/4] fix unit test --- .../src/views/CRUD/alert/AlertReportModal.test.jsx | 3 ++- .../views/CRUD/alert/components/NotificationMethod.tsx | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx index 1509bff49292..7d5e60772d4c 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/components/NotificationMethod.tsx b/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx index 0b893b54e263..0157116c5c40 100644 --- a/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx +++ b/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx @@ -125,10 +125,12 @@ export const NotificationMethod: FunctionComponent = ({ data-test="select-delivery-method" onChange={onMethodChange} placeholder={t('Select Delivery Method')} - options={(options || []).map((method: NotificationMethodOption) => ({ - label: method, - value: method, - }))} + options={(options || []).map( + (method: NotificationMethodOption) => ({ + label: method, + value: method, + }), + )} value={method} /> From ac5a3ad98e1e0875e56163aa020611f15598a097 Mon Sep 17 00:00:00 2001 From: Grace Date: Tue, 24 Aug 2021 16:21:50 -0700 Subject: [PATCH 4/4] improve typing --- .../src/views/CRUD/alert/AlertReportModal.tsx | 15 +++++++-------- .../CRUD/alert/components/NotificationMethod.tsx | 2 +- superset-frontend/src/views/CRUD/alert/types.ts | 4 +++- superset-frontend/src/views/types.ts | 2 +- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx index efce49e311e0..56e1eba7fbd9 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx +++ b/superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx @@ -44,11 +44,8 @@ import withToasts from 'src/messageToasts/enhancers/withToasts'; import Owner from 'src/types/Owner'; import TextAreaControl from 'src/explore/components/controls/TextAreaControl'; import { useCommonConf } from 'src/views/CRUD/data/database/state'; -import { AlertReportCronScheduler } from './components/AlertReportCronScheduler'; -import { NotificationMethod } from './components/NotificationMethod'; -import { NotificationMethodOption } from '../../types'; - import { + NotificationMethodOption, AlertObject, ChartObject, DashboardObject, @@ -56,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 = [ @@ -398,7 +397,7 @@ const AlertReportModal: FunctionComponent = ({ isReport = false, }) => { const conf = useCommonConf(); - const allowedNotificationMethods = + const allowedNotificationMethods: NotificationMethodOption[] = conf?.ALERT_REPORTS_NOTIFICATION_METHODS || DEFAULT_NOTIFICATION_METHODS; const [disableSave, setDisableSave] = useState(true); @@ -916,10 +915,10 @@ const AlertReportModal: FunctionComponent = ({ ? JSON.parse(setting.recipient_config_json) : {}; return { - method: setting.type as NotificationMethodOption, + method: setting.type, // @ts-ignore: Type not assignable recipients: config.target || setting.recipient_config_json, - options: allowedNotificationMethods as NotificationMethodOption[], // Need better logic for this + options: allowedNotificationMethods, }; }); diff --git a/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx b/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx index 0157116c5c40..12b59a7afd2d 100644 --- a/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx +++ b/superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx @@ -20,8 +20,8 @@ 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'; -import { NotificationMethodOption } from '../../../types'; const StyledNotificationMethod = styled.div` margin-bottom: 10px; diff --git a/superset-frontend/src/views/CRUD/alert/types.ts b/superset-frontend/src/views/CRUD/alert/types.ts index f67a6042dfe8..3f939a03bcfa 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/types.ts b/superset-frontend/src/views/types.ts index 10b044daad22..c0ffead6a5d8 100644 --- a/superset-frontend/src/views/types.ts +++ b/superset-frontend/src/views/types.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -export type NotificationMethodOption = 'Email' | 'Slack'; +import { NotificationMethodOption } from './CRUD/alert/types'; export interface ViewState { common: {