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

graceguo-supercat · 2021-08-20T02:36:12Z

SUMMARY

If system admin did not set SLACK_API_TOKEN config, we should not show Slack as notification method.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI and manual test

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

cc @john-bodley @m-ajay @betodealmeida

codecov · 2021-08-20T02:55:49Z

Codecov Report

Merging #16367 (28535d9) into master (81241b6) will decrease coverage by 0.22%.
The diff coverage is 88.23%.

❗ Current head 28535d9 differs from pull request most recent head 8a60d5f. Consider uploading reports for the commit 8a60d5f to get more accurate results

@@            Coverage Diff             @@
##           master   #16367      +/-   ##
==========================================
- Coverage   76.53%   76.31%   -0.23%     
==========================================
  Files        1000     1000              
  Lines       53500    53511      +11     
  Branches     6814     6818       +4     
==========================================
- Hits        40946    40836     -110     
- Misses      12316    12436     +120     
- Partials      238      239       +1

Flag	Coverage Δ
hive	`?`
javascript	`70.64% <83.33%> (-0.01%)`	⬇️
mysql	`81.53% <100.00%> (+<0.01%)`	⬆️
postgres	`81.51% <100.00%> (+<0.01%)`	⬆️
presto	`?`
python	`81.64% <100.00%> (-0.43%)`	⬇️
sqlite	`81.20% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...frontend/src/views/CRUD/alert/AlertReportModal.tsx	`61.13% <75.00%> (-0.26%)`	⬇️
...views/CRUD/alert/components/NotificationMethod.tsx	`73.68% <100.00%> (+0.71%)`	⬆️
...set-frontend/src/views/CRUD/data/database/state.ts	`100.00% <100.00%> (ø)`
superset/views/base.py	`76.43% <100.00%> (+0.40%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-82.15%)`	⬇️
superset/db_engine_specs/hive.py	`69.80% <0.00%> (-16.87%)`	⬇️
superset/db_engine_specs/presto.py	`83.47% <0.00%> (-6.91%)`	⬇️
superset/views/database/mixins.py	`81.03% <0.00%> (-1.73%)`	⬇️
superset/connectors/sqla/models.py	`88.04% <0.00%> (-1.66%)`	⬇️
superset/db_engine_specs/base.py	`88.00% <0.00%> (-0.39%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81241b6...8a60d5f. Read the comment docs.

john-bodley · 2021-08-20T16:38:22Z

superset/config.py

@@ -994,6 +994,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"]


@graceguo-supercat I think it would be preferable to reuse this enum. Also what happens if you make this an empty list?

I agree with John, should we disable the feature if the list is empty?

Based on the new implementation, we will always has 'Email' option, so i don't handle empty case here.

betodealmeida · 2021-08-21T00:54:33Z

Alternatively, we can show the Slack option only if SLACK_API_TOKEN is defined in the config.

graceguo-supercat · 2021-08-23T18:37:11Z

Alternatively, we can show the Slack option only if SLACK_API_TOKEN is defined in the config.

I actually think this is a good idea:

We should not show Slack if api token is None. Right now we show Slack as notification method even no API token set, and later user will get error message. This is not a correct behavior.
We could create ALERT_REPORTS_NOTIFICATION_METHODS internally (not in config.py) based on Slack settings, and pass it to front-end, it could avoid circular dependency. Otherwise, move those constants will touch about 20 files which makes me feel the changes are risky.

graceguo-supercat · 2021-08-24T16:12:39Z

ping @betodealmeida and @john-bodley in last 24 ours i have to resolve conflicts twice. please take a look.

etr2460 · 2021-08-24T18:47:32Z

superset/views/base.py

+        frontend_config["ALERT_REPORTS_NOTIFICATION_METHODS"] = [
+            ReportRecipientType.EMAIL,
+            ReportRecipientType.SLACK,
+        ]
+    else:
+        frontend_config["ALERT_REPORTS_NOTIFICATION_METHODS"] = [
+            ReportRecipientType.EMAIL,
+        ]


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.

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

etr2460

stamping with a couple nits.

One question i had while looking through this code @dpgaspar was why we couldn't get the possible options for the crud view from the validator in FAB? It seems like we don't do that today, likely because we wouldn't have the validator when creating a new object, but maybe we should consider unifying the validator logic with the logic that generates options for selectors, that way we can only have one source of truth for all these configs

etr2460 · 2021-08-24T19:49:45Z

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


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

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

etr2460 · 2021-08-24T19:50:47Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

@@ -397,6 +397,10 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
  alert = null,
  isReport = false,
 }) => {
+  const conf = useCommonConf();
+  const allowedNotificationMethods =


should we cast this as NotificationMethodOption[] here so that we don't need to cast it multiple times below?

etr2460 · 2021-08-24T19:51:40Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

@@ -78,7 +80,7 @@ interface AlertReportModalProps {
  show: boolean;
 }

-const NOTIFICATION_METHODS: NotificationMethod[] = ['Email', 'Slack'];
+const DEFAULT_NOTIFICATION_METHODS: NotificationMethodOption[] = ['Email'];


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

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.

* upstream/master: (25 commits) chore(ci): bump pylint to 2.10.2 (apache#16463) fix: prevent page crash when chart can't render (apache#16464) chore: fixed slack invite link (apache#16466) fix(native-filters): handle null values in value filter (apache#16460) feat: add function list to auto-complete to Clickhouse datasource (apache#16234) refactor(explore): improve typing for Dnd controls (apache#16362) fix(explore): update overwrite button on perm change (apache#16437) feat: Draggable and Resizable Modal (apache#16394) refactor: sql_json view endpoint (apache#16441) fix(dashboard): undo and redo buttons weird alignment (apache#16417) fix: setupPlugin in chart list page (apache#16413) fix: Disable Slack notification method if no api token (apache#16367) feat: add Shillelagh DB engine spec (apache#16416) fix: copy to Clipboard order (apache#16299) docs: make FEATURE_FLAGS.md reference a link (apache#16415) chore(viz): bump superset-ui to 0.17.87 (apache#16420) feat: add activate command (apache#16404) Revert "fix(explore): let admin overwrite slice (apache#16290)" (apache#16408) fix(explore): retain chart ownership on query context update (apache#16419) chore: Removes the TODOs and uses the default page size (apache#16422) ...

* feat: allow company choose alert/report notification methods * fix: disable Slack as notification method if no api token * fix unit test * improve typing

pull-request-size bot added the size/M label Aug 20, 2021

graceguo-supercat force-pushed the gg-AddSlackAsConfig branch from d6890ef to 3788e46 Compare August 20, 2021 06:03

graceguo-supercat requested review from betodealmeida, riahk and lilykuang August 20, 2021 06:10

john-bodley reviewed Aug 20, 2021

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Aug 21, 2021

graceguo-supercat marked this pull request as draft August 21, 2021 06:19

graceguo-supercat force-pushed the gg-AddSlackAsConfig branch from a92cdd9 to a07c7ec Compare August 23, 2021 17:46

pull-request-size bot added size/M and removed size/L labels Aug 23, 2021

graceguo-supercat marked this pull request as ready for review August 23, 2021 17:47

graceguo-supercat changed the title ~~feat: allow company admin choose alert and report notification methods~~ fix: Disable Slack notification method if no api token Aug 23, 2021

graceguo-supercat force-pushed the gg-AddSlackAsConfig branch 2 times, most recently from 8c655f7 to 959f573 Compare August 23, 2021 18:32

graceguo-supercat force-pushed the gg-AddSlackAsConfig branch from 959f573 to 10ad1fd Compare August 23, 2021 18:41

graceguo-supercat requested a review from john-bodley August 23, 2021 18:46

graceguo-supercat force-pushed the gg-AddSlackAsConfig branch 2 times, most recently from 4e6edb9 to b2c5901 Compare August 23, 2021 22:39

Grace added 2 commits August 24, 2021 09:38

feat: allow company choose alert/report notification methods

542ab02

fix: disable Slack as notification method if no api token

8d9ce58

graceguo-supercat force-pushed the gg-AddSlackAsConfig branch from b2c5901 to d2179a0 Compare August 24, 2021 16:49

fix unit test

5eab082

graceguo-supercat force-pushed the gg-AddSlackAsConfig branch from d2179a0 to 5eab082 Compare August 24, 2021 17:12

etr2460 reviewed Aug 24, 2021

View reviewed changes

etr2460 approved these changes Aug 24, 2021

View reviewed changes

improve typing

ac5a3ad

graceguo-supercat force-pushed the gg-AddSlackAsConfig branch from 8a60d5f to ac5a3ad Compare August 24, 2021 23:41

graceguo-supercat merged commit 78d8089 into apache:master Aug 25, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

graceguo-supercat commented Aug 20, 2021 •

edited

codecov bot commented Aug 20, 2021 •

edited

john-bodley Aug 20, 2021 •

edited

betodealmeida Aug 21, 2021

graceguo-supercat Aug 23, 2021

betodealmeida commented Aug 21, 2021

graceguo-supercat commented Aug 23, 2021 •

edited

graceguo-supercat commented Aug 24, 2021

etr2460 Aug 24, 2021 •

edited

graceguo-supercat Aug 25, 2021

etr2460 left a comment

etr2460 Aug 24, 2021

graceguo-supercat Aug 24, 2021

etr2460 Aug 24, 2021

etr2460 Aug 24, 2021

graceguo-supercat Aug 24, 2021

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

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

Conversation

graceguo-supercat commented Aug 20, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Aug 20, 2021 • edited

Codecov Report

john-bodley Aug 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida commented Aug 21, 2021

graceguo-supercat commented Aug 23, 2021 • edited

graceguo-supercat commented Aug 24, 2021

etr2460 Aug 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented Aug 20, 2021 •

edited

codecov bot commented Aug 20, 2021 •

edited

john-bodley Aug 20, 2021 •

edited

graceguo-supercat commented Aug 23, 2021 •

edited

etr2460 Aug 24, 2021 •

edited