Skip to content

fix: Disable Slack notification method if no api token #16367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
graceguo-supercat merged 4 commits into from
Aug 25, 2021
Merged

fix: Disable Slack notification method if no api token #16367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
542ab02
feat: allow company choose alert/report notification methods
Aug 20, 2021
8d9ce58
fix: disable Slack as notification method if no api token
Aug 23, 2021
5eab082
fix unit test
Aug 23, 2021
ac5a3ad
improve typing
Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]')
Expand Down
36 changes: 22 additions & 14 deletions superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,19 @@ 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,
DatabaseObject,
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 = [
Expand All @@ -78,7 +79,7 @@ interface AlertReportModalProps {
show: boolean;
}

const NOTIFICATION_METHODS: NotificationMethod[] = ['Email', 'Slack'];
const DEFAULT_NOTIFICATION_METHODS: NotificationMethodOption[] = ['Email'];
Copy link
Copy Markdown
Member

@etr2460 etr2460 Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if changing the default is a breaking change, but since we should probably never hit this code path (can the page render without the config options actually here?) it's likely fine

Copy link
Copy Markdown
Author

@graceguo-supercat graceguo-supercat Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if system admin didn't set slack API token in config, user did saw Slack option before, but they can't see report in slack.

const DEFAULT_NOTIFICATION_FORMAT = 'PNG';
const CONDITIONS = [
{
Expand Down Expand Up @@ -381,12 +382,10 @@ const NotificationMethodAdd: FunctionComponent<NotificationMethodAddProps> = ({
);
};

type NotificationMethod = 'Email' | 'Slack';

type NotificationSetting = {
method?: NotificationMethod;
method?: NotificationMethodOption;
recipients: string;
options: NotificationMethod[];
options: NotificationMethodOption[];
};

const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
Expand All @@ -397,6 +396,10 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
alert = null,
isReport = false,
}) => {
const conf = useCommonConf();
const allowedNotificationMethods: NotificationMethodOption[] =
conf?.ALERT_REPORTS_NOTIFICATION_METHODS || DEFAULT_NOTIFICATION_METHODS;

const [disableSave, setDisableSave] = useState<boolean>(true);
const [
currentAlert,
Expand Down Expand Up @@ -435,12 +438,14 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({

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

Expand Down Expand Up @@ -910,16 +915,18 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
? 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(
Expand Down Expand Up @@ -1309,6 +1316,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
<NotificationMethod
setting={notificationSetting}
index={i}
key={`NotificationMethod-${i}`}
onUpdate={updateNotificationSetting}
onRemove={removeNotificationSetting}
/>
Expand Down
19 changes: 10 additions & 9 deletions superset-frontend/src/views/CRUD/alert/components/NotificationMethod.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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 {
Expand All @@ -80,7 +79,7 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
return null;
}

const onMethodChange = (method: NotificationMethod) => {
const onMethodChange = (method: NotificationMethodOption) => {
// Since we're swapping the method, reset the recipients
setRecipientValue('');
if (onUpdate) {
Expand Down Expand Up @@ -126,10 +125,12 @@ export const NotificationMethod: FunctionComponent<NotificationMethodProps> = ({
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}
/>
</div>
Expand Down
4 changes: 3 additions & 1 deletion superset-frontend/src/views/CRUD/alert/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/data/database/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Copy Markdown
Member

@etr2460 etr2460 Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did we need to add ? here? seems like it worked fine before?

Copy link
Copy Markdown
Author

@graceguo-supercat graceguo-supercat Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will break unit test AlertReportModal.test.jsx: it didn't read conf before this PR.

}
3 changes: 3 additions & 0 deletions superset-frontend/src/views/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object>;
Expand Down
15 changes: 14 additions & 1 deletion superset/views/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
]
Comment on lines +353 to +360
Copy link
Copy Markdown
Member

@etr2460 etr2460 Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this need to live in the frontend config/bootstrap data? i'm not super familiar with the reports/alerts code, but it seems like they might call apis on load, which would mean we could just get this configuration via an api request instead of bundling it in the bootstrap data on every superset page.

Copy link
Copy Markdown
Author

@graceguo-supercat graceguo-supercat Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bootstrap_data is used by all entry point, includes spa.html. I think populate it with configs not a bad idea.


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(),
Expand Down