fix(Slack): Fix Slack recipients migration to V2 #32336

Vitor-Avila · 2025-02-20T22:13:11Z

SUMMARY

This PR fixes a few things with the Slack method migration from V1 to V2 (required as Slack is fully deprecating files.upload() in March):

get_channels_with_search was returning a list of dictionaries (one per channel found) and we were dumping this directly in the target configuration for a report. The target field expects a comma-separated list of channels IDs for the V2 approach.
I've added a schema that drops any additional fields from each channels to reduce information (some organizations could have thousands of channels).
Enforcing exact_match search during the migration to avoid for example migrating a report from other-channel to another-channel.
Stripping channel names before the search to account for one-channel, second-channel. Also removing leading # to channel names that were supported with the V1 method.
Considering that V1 is reaching EOL and won't work anymore, if the bot has the required scopes for the migration to V2 but the migration fails, the execution now fails with an error that should notify owners for manual fix. The channel names are preserved in the form to make the process easier. Before this PR, the execution was logged as SUCCESS even though the notification was not sent, so this seems more appropriate.

One known issue is that the V1 integration allowed sending reports as a DM (by using the Member ID), and this is no longer possible with V2. The V2 APIs consider a DM as a conversation as well, so the bot would need to parse all Workspace members + public channels + private channels to support this, which would definitely hit the Slack rate limit (20+ requests per minute). We can revisit this once we have a better strategy to avoid rate limits.

Fixes #31936

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Report form when the migration failed

Execution logs when the migration failed

TESTING INSTRUCTIONS

Added testing.

ADDITIONAL INFORMATION

Has associated issue: Failed to update slack recipients to v2 #31936
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

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Non-Specific Error Message ▹ view	✅
	Unsafe String Split Operation ▹ view	✅

Files scanned

File Path	Reviewed
superset/utils/slack.py	✅
superset/reports/schemas.py	✅
superset/commands/report/execute.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ [email protected]

superset/commands/report/execute.py

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Slack Channel Validation Fails with Duplicate Inputs ▹ view	✅
	Invalid Search String Processing ▹ view	✅

Files scanned

File Path	Reviewed
superset/reports/notifications/slackv2.py	✅
superset/reports/notifications/slack.py	✅
superset/utils/slack.py	✅
superset/reports/schemas.py	✅
superset/reports/api.py	✅
superset/commands/report/execute.py	✅
superset/utils/core.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ [email protected]

superset/commands/report/execute.py

superset/reports/api.py

superset/commands/report/execute.py

superset/utils/core.py

eschutho · 2025-03-05T22:42:10Z

superset/commands/report/execute.py

+                            "Could not find the following channels: "
+                            f"{', '.join(missing_channels)}"
+                        )
+                        raise UpdateFailedError(msg)


Do we want to block the conversion to v2 if channel ids can't be found or just log them and continue?

@eschutho from my tests currently (before this PR) what's happening is that the execution itself is listed as successful in the execution logs, but nothing is sent to Slack. I thought about calling send() with the type set to V1 again, but that would cause an infinite loop as we would validate again if there are scopes to migrate, attempt the migration, fail again, etc. Calling it with V2 would also fail as we don't have all IDs.

Considering that the Slack deprecation is next Tuesday, I think it makes sense to consider any issues to migrate to V2 an error that should be fixed asap. Let me know your thoughts -- happy to think of other alternatives.

Ok, this makes sense.

Thanks! Won't resolve this as I think it's good context for someone looking at the changes in the future

superset/commands/report/execute.py

eschutho · 2025-03-05T23:10:46Z

superset/commands/report/execute.py

-                                    SlackChannelTypes.PUBLIC,
-                                ],
-                            )
+                            "target": channel_ids,


Thanks for catching this error/missing step, btw!

eschutho

This looks great. Small formatting suggestions, but good to merge when you're ready!

(cherry picked from commit d2e0e2b)

pull-request-size bot added the size/L label Feb 20, 2025

Vitor-Avila marked this pull request as draft February 20, 2025 22:13

korbit-ai bot reviewed Feb 20, 2025

View reviewed changes

superset/commands/report/execute.py Outdated Show resolved Hide resolved

superset/commands/report/execute.py Outdated Show resolved Hide resolved

rusackas added the review:draft label Feb 21, 2025

github-actions bot added the api Related to the REST API label Feb 28, 2025

Vitor-Avila removed the review:draft label Feb 28, 2025

Vitor-Avila marked this pull request as ready for review February 28, 2025 23:01

korbit-ai bot reviewed Feb 28, 2025

View reviewed changes

superset/commands/report/execute.py Outdated Show resolved Hide resolved

superset/reports/api.py Outdated Show resolved Hide resolved

Vitor-Avila force-pushed the fix/slack-v2-channels-migration branch from f38128c to fb82e8b Compare March 5, 2025 14:51

Vitor-Avila requested review from betodealmeida and eschutho March 5, 2025 20:30

Vitor-Avila added 10 commits March 5, 2025 18:39

fix(Slack): Fix Slack recipients migration to V2

5af66d8

Improving the logic

41cf061

Fixing tests

a4b52c2

More fixes

6b5b242

Error execution when migration fails + more tests

df5a027

Fixing test

14fbb54

Hopefully fixing tests

23d2ce2

Hopefully fixing the test

cdeb989

I think now I fixed the test

afdecd8

I think now I fixed the test (again)

802c0aa

Vitor-Avila force-pushed the fix/slack-v2-channels-migration branch from d934717 to 802c0aa Compare March 5, 2025 21:39

eschutho reviewed Mar 5, 2025

View reviewed changes

superset/reports/api.py Outdated Show resolved Hide resolved

eschutho reviewed Mar 5, 2025

View reviewed changes

superset/commands/report/execute.py Outdated Show resolved Hide resolved

eschutho reviewed Mar 5, 2025

View reviewed changes

superset/commands/report/execute.py Outdated Show resolved Hide resolved

eschutho reviewed Mar 5, 2025

View reviewed changes

superset/utils/core.py Outdated Show resolved Hide resolved

eschutho reviewed Mar 5, 2025

View reviewed changes

superset/commands/report/execute.py Outdated Show resolved Hide resolved

eschutho reviewed Mar 5, 2025

View reviewed changes

eschutho approved these changes Mar 5, 2025

View reviewed changes

github-actions bot removed the api Related to the REST API label Mar 6, 2025

Addressing PR feedback

2c7d8dc

Vitor-Avila force-pushed the fix/slack-v2-channels-migration branch from 7f67895 to 2c7d8dc Compare March 6, 2025 11:36

Vitor-Avila merged commit d2e0e2b into master Mar 6, 2025
40 checks passed

Vitor-Avila deleted the fix/slack-v2-channels-migration branch March 6, 2025 11:52

michael-s-molina added the v5.0 Label added by the release manager to track PRs to be included in the 5.0 branch label Mar 11, 2025

michael-s-molina pushed a commit that referenced this pull request Mar 17, 2025

fix(Slack): Fix Slack recipients migration to V2 (#32336)

405bc26

(cherry picked from commit d2e0e2b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(Slack): Fix Slack recipients migration to V2 #32336

fix(Slack): Fix Slack recipients migration to V2 #32336

Uh oh!

Vitor-Avila commented Feb 20, 2025 •

edited

Loading

Uh oh!

korbit-ai bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

korbit-ai bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eschutho Mar 5, 2025

Uh oh!

Vitor-Avila Mar 5, 2025

Uh oh!

eschutho Mar 5, 2025

Uh oh!

Vitor-Avila Mar 6, 2025

Uh oh!

Uh oh!

eschutho Mar 5, 2025

Uh oh!

eschutho left a comment

Uh oh!

Uh oh!

Uh oh!

fix(Slack): Fix Slack recipients migration to V2 #32336

fix(Slack): Fix Slack recipients migration to V2 #32336

Uh oh!

Conversation

Vitor-Avila commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

korbit-ai bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Interacting with Korbit

Customizing Korbit

Feedback and Support

Uh oh!

Uh oh!

Uh oh!

korbit-ai bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Interacting with Korbit

Customizing Korbit

Feedback and Support

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eschutho Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

Vitor-Avila Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

eschutho Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

Vitor-Avila Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eschutho Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

eschutho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Vitor-Avila commented Feb 20, 2025 •

edited

Loading

korbit-ai bot left a comment •

edited

Loading

korbit-ai bot left a comment •

edited

Loading