feat: add slackv2 notification #29264

eschutho · 2024-06-15T01:01:28Z

SUMMARY

This PR adds a new Notification SlackV2Notification that uses the Slack upload files v1 method. It is required for slack workspace owners to add the channels:read scope to their workspace in order to use this api. It also fetches all slack channels in order to convert user-inputed names to api-required ids. Because of this, I added a UI to support this.

Per the SIP, the both the api and UI will attempt to convert the notification to SlackV2 if the scopes are in place.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

With feature flag on:

Screen.Recording.2024-07-10.at.4.33.54.PM.mov

With feature flag off (after it has been turned on once, so the existing v2 slack reports do not convert back to v1):

Screen.Recording.2024-07-10.at.4.35.23.PM.mov

TESTING INSTRUCTIONS

Create a new slack application and connect it to Superset.

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

sadpandajoe · 2024-07-03T19:43:44Z

superset/reports/schemas.py

+get_slack_channels_schema = {
+    "type": "object",
+    "properties": {
+        "seach_string": {"type": "string"},


NIT: "search_string" vs "seach_string"?

thanks for catching!

eschutho · 2024-07-11T01:51:01Z

/testenv up

github-actions · 2024-07-11T02:01:28Z

@eschutho Ephemeral environment spinning up at http://35.87.151.148:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

eschutho · 2024-07-15T22:29:01Z

/testenv up FEATURE_ALERT_REPORTS=true

github-actions · 2024-07-15T22:41:39Z

@eschutho Ephemeral environment spinning up at http://35.94.230.149:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

sadpandajoe · 2024-07-16T18:41:46Z

superset/utils/slack.py

+                search.lower()
+                for search in (search_string.split(",") if search_string else [])
+            ]
+            print(channels)


Did you want to print out channels?

Thanks for catching!

sadpandajoe

LGTM

sadpandajoe · 2024-07-16T20:54:45Z

superset/utils/slack.py

+                for search in (search_string.split(",") if search_string else [])
+            ]
+
+            channels = [


I'm just staring at this section of the code more and more, do we have good coverage for this function? Looking a this logic, I could see someone making a change and breaking the channels that are returned.

geido

Thanks for all the hard work on this!

geido · 2024-07-17T12:16:13Z

superset/utils/slack.py

@@ -31,6 +46,80 @@ def get_slack_client() -> WebClient:
    return WebClient(token=token, proxy=current_app.config["SLACK_PROXY"])


+def get_channels_with_search(


So bad that we need this function that seems prone to errors but if Slack does not allow searching, then we are out of luck

Yeah, agree

geido · 2024-07-17T12:16:45Z

superset/tasks/scheduler.py

@@ -94,7 +94,7 @@ def execute(self: Celery.task, report_schedule_id: int) -> None:
        ).run()
    except ReportScheduleUnexpectedError:
        logger.exception(
-            "An unexpected occurred while executing the report: %s", task_id
+            "An unexpected error occurred while executing the report: %s", task_id


geido

A bunch of minor comments from a second look. Still LGTM.

superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx

geido · 2024-07-17T16:36:55Z

superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx

+    );
+
+    const recipientsInput = screen.getByTestId('recipients');
+    fireEvent.change(recipientsInput, {


Can we use userEvent here instead and everywhere else?

I can in two places, but it doesn't work in the others.

superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx

superset-frontend/src/features/alerts/components/NotificationMethod.tsx

geido · 2024-07-17T16:42:19Z

superset/reports/api.py

+            return self.response(200, result=channels)
+        except SupersetException as ex:
+            logger.error("Error fetching slack channels %s", str(ex))
+            return self.response_422(message=str(ex))


Do we want to expose the original error message here?

Yes, because it gives information about how to fix the slack issue. Whether it's missing a scope or the channel doesn't exist. Otherwise, it's difficult to know how to unblock the error.

github-actions · 2024-07-17T22:15:06Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/XL label Jun 15, 2024

github-actions bot added api Related to the REST API packages labels Jun 15, 2024

eschutho mentioned this pull request Jun 15, 2024

[SIP-138] Proposal for Slack file upload V2 integration for Alerts and Reports #29263

Closed

john-bodley added the review:draft label Jun 17, 2024

eschutho force-pushed the elizabeth/slackv2 branch from 745b75d to d1bdaa5 Compare June 17, 2024 20:02

eschutho changed the title ~~[POC] feat: add slackv2 notification~~ feat: add slackv2 notification Jun 17, 2024

github-actions bot added the preset-io label Jun 17, 2024

eschutho force-pushed the elizabeth/slackv2 branch 5 times, most recently from cfbe2ec to c156187 Compare June 19, 2024 01:24

pull-request-size bot added size/XXL and removed size/XL labels Jun 19, 2024

eschutho force-pushed the elizabeth/slackv2 branch from c156187 to c2f6bf8 Compare June 20, 2024 18:32

pull-request-size bot added size/XL and removed size/XXL labels Jun 20, 2024

eschutho force-pushed the elizabeth/slackv2 branch from c2f6bf8 to b1e31fd Compare June 20, 2024 20:55

pull-request-size bot added size/XXL and removed size/XL labels Jun 20, 2024

eschutho force-pushed the elizabeth/slackv2 branch 5 times, most recently from 026ac4c to eccc201 Compare June 21, 2024 00:53

eschutho marked this pull request as ready for review June 21, 2024 15:55

dosubot bot added alert-reports Namespace | Anything related to the Alert & Reports feature change:backend Requires changing the backend change:frontend Requires changing the frontend labels Jun 21, 2024

eschutho force-pushed the elizabeth/slackv2 branch from bd5a20d to ef4581a Compare June 28, 2024 22:42

sadpandajoe mentioned this pull request Jul 3, 2024

Slack API files.upload v1 deprecation #28353

Closed

3 tasks

sadpandajoe reviewed Jul 3, 2024

View reviewed changes

eschutho mentioned this pull request Jul 10, 2024

fix: Unsupported Slack API calls #29532

Closed

9 tasks

eschutho requested review from michael-s-molina, geido and kgabryje as code owners July 10, 2024 23:32

eschutho force-pushed the elizabeth/slackv2 branch 2 times, most recently from 1e048b2 to a0ea944 Compare July 10, 2024 23:58

display private channels

22968c3

eschutho force-pushed the elizabeth/slackv2 branch from a0ea944 to 22968c3 Compare July 11, 2024 01:24

sadpandajoe reviewed Jul 16, 2024

View reviewed changes

sadpandajoe approved these changes Jul 16, 2024

View reviewed changes

add more tests

1b98531

eschutho force-pushed the elizabeth/slackv2 branch from a6bf324 to 1b98531 Compare July 16, 2024 20:20

sadpandajoe reviewed Jul 16, 2024

View reviewed changes

add test

2225a94

geido approved these changes Jul 17, 2024

View reviewed changes

geido reviewed Jul 17, 2024

View reviewed changes

translate strings and improve tests

f7e4a1e

eschutho force-pushed the elizabeth/slackv2 branch from 94c337c to f7e4a1e Compare July 17, 2024 21:39

eschutho merged commit 6dbfe2a into master Jul 17, 2024
34 checks passed

eschutho added a commit that referenced this pull request Jul 24, 2024

feat: add slackv2 notification (#29264)

a0d7a97

PuszekSE mentioned this pull request Jul 26, 2024

Slack Notification - deprecated API #29480

Closed

3 tasks

rusackas deleted the elizabeth/slackv2 branch September 27, 2024 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add slackv2 notification #29264

feat: add slackv2 notification #29264

eschutho commented Jun 15, 2024 •

edited

Loading

sadpandajoe Jul 3, 2024

eschutho Jul 3, 2024

eschutho commented Jul 11, 2024

github-actions bot commented Jul 11, 2024

eschutho commented Jul 15, 2024

github-actions bot commented Jul 15, 2024

sadpandajoe Jul 16, 2024

eschutho Jul 16, 2024

sadpandajoe left a comment

sadpandajoe Jul 16, 2024

geido left a comment

geido Jul 17, 2024

eschutho Jul 17, 2024

geido Jul 17, 2024

geido left a comment

geido Jul 17, 2024

eschutho Jul 17, 2024

geido Jul 17, 2024

eschutho Jul 17, 2024

github-actions bot commented Jul 17, 2024

		@@ -31,6 +46,80 @@ def get_slack_client() -> WebClient:
		return WebClient(token=token, proxy=current_app.config["SLACK_PROXY"])


		def get_channels_with_search(

feat: add slackv2 notification #29264

feat: add slackv2 notification #29264

Conversation

eschutho commented Jun 15, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented Jul 11, 2024

github-actions bot commented Jul 11, 2024

eschutho commented Jul 15, 2024

github-actions bot commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadpandajoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geido 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

geido 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

github-actions bot commented Jul 17, 2024

eschutho commented Jun 15, 2024 •

edited

Loading