-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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.
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
fix: use channel id with new slack api for file uploads #28797
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,12 +17,14 @@ | |
import logging | ||
from collections.abc import Sequence | ||
from io import IOBase | ||
from typing import Union | ||
from typing import List, Union | ||
|
||
import backoff | ||
import pandas as pd | ||
from deprecation import deprecated | ||
from flask import g | ||
from flask_babel import gettext as __ | ||
from slack_sdk import WebClient | ||
from slack_sdk.errors import ( | ||
BotUserAccessError, | ||
SlackApiError, | ||
|
@@ -60,16 +62,25 @@ class SlackNotification(BaseNotification): # pylint: disable=too-few-public-met | |
|
||
type = ReportRecipientType.SLACK | ||
|
||
def _get_channel(self) -> str: | ||
def _get_channels(self, client: WebClient) -> List[str]: | ||
""" | ||
Get the recipient's channel(s). | ||
Note Slack SDK uses "channel" to refer to one or more | ||
channels. Multiple channels are demarcated by a comma. | ||
:returns: The comma separated list of channel(s) | ||
:returns: A list of channel ids: "EID676L" | ||
:raises SlackApiError: If the API call fails | ||
""" | ||
recipient_str = json.loads(self._recipient.recipient_config_json)["target"] | ||
|
||
return ",".join(get_email_address_list(recipient_str)) | ||
channel_recipients: List[str] = get_email_address_list(recipient_str) | ||
|
||
conversations_list_response = client.conversations_list( | ||
types="public_channel,private_channel" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now need to map the list of names to a list of ids. This is the slack sdk method that fetches information that has both the name and id. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this work if the application doesn't have the right scope? I tried testing this here with my token and got this response:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, that's a good point. @Vitor-Avila had a great solution which is to try the deprecated method first and then try the new one, since only new apps are affected right now. I'm also nervous about introducing a breaking change here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume right now we store the names of the channels, which is why we're mapping back to IDs? In the future it would be nice to store the IDs directly, not only it would eliminate the need for this mapping but also reports would keep working when channels are renamed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, correct, since that is what is in the report form. If we stored the ids, then we would need to map then back to the name when someone tries to view/edit the form, but that's prob still better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking about this more @betodealmeida and it would be a breaking change because the existing applications would need the |
||
) | ||
|
||
return [ | ||
c["id"] | ||
for c in conversations_list_response["channels"] | ||
if c["name"] in channel_recipients | ||
] | ||
|
||
def _message_template(self, table: str = "") -> str: | ||
return __( | ||
|
@@ -115,15 +126,19 @@ def _get_body(self) -> str: | |
|
||
# Flatten columns/index so they show up nicely in the table | ||
df.columns = [ | ||
" ".join(str(name) for name in column).strip() | ||
if isinstance(column, tuple) | ||
else column | ||
( | ||
" ".join(str(name) for name in column).strip() | ||
if isinstance(column, tuple) | ||
else column | ||
) | ||
for column in df.columns | ||
] | ||
df.index = [ | ||
" ".join(str(name) for name in index).strip() | ||
if isinstance(index, tuple) | ||
else index | ||
( | ||
" ".join(str(name) for name in index).strip() | ||
if isinstance(index, tuple) | ||
else index | ||
) | ||
for index in df.index | ||
] | ||
|
||
|
@@ -162,29 +177,40 @@ def _get_body(self) -> str: | |
|
||
def _get_inline_files( | ||
self, | ||
) -> tuple[Union[str, None], Sequence[Union[str, IOBase, bytes]]]: | ||
) -> Sequence[Union[str, IOBase, bytes]]: | ||
if self._content.csv: | ||
return ("csv", [self._content.csv]) | ||
return [self._content.csv] | ||
if self._content.screenshots: | ||
return ("png", self._content.screenshots) | ||
return self._content.screenshots | ||
if self._content.pdf: | ||
return ("pdf", [self._content.pdf]) | ||
return (None, []) | ||
return [self._content.pdf] | ||
return [] | ||
|
||
@backoff.on_exception(backoff.expo, SlackApiError, factor=10, base=2, max_tries=5) | ||
@statsd_gauge("reports.slack.send") | ||
def send(self) -> None: | ||
file_type, files = self._get_inline_files() | ||
title = self._content.name | ||
channel = self._get_channel() | ||
body = self._get_body() | ||
global_logs_context = getattr(g, "logs_context", {}) or {} | ||
try: | ||
client = get_slack_client() | ||
# files_upload returns SlackResponse as we run it in sync mode. | ||
if files: | ||
@deprecated(deprecated_in="4.1") | ||
def _deprecated_upload_files( | ||
Comment on lines
+189
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
self, client: WebClient, title: str, body: str | ||
) -> None: | ||
""" | ||
Deprecated method to upload files to slack | ||
Should only be used if the new method fails | ||
To be removed in the next major release | ||
""" | ||
file_type, files = (None, []) | ||
if self._content.csv: | ||
file_type, files = ("csv", [self._content.csv]) | ||
if self._content.screenshots: | ||
file_type, files = ("png", self._content.screenshots) | ||
if self._content.pdf: | ||
file_type, files = ("pdf", [self._content.pdf]) | ||
|
||
recipient_str = json.loads(self._recipient.recipient_config_json)["target"] | ||
|
||
recipients = get_email_address_list(recipient_str) | ||
|
||
for channel in recipients: | ||
if len(files) > 0: | ||
for file in files: | ||
client.files_upload_v2( | ||
client.files_upload( | ||
channels=channel, | ||
file=file, | ||
initial_comment=body, | ||
|
@@ -193,6 +219,46 @@ def send(self) -> None: | |
) | ||
else: | ||
client.chat_postMessage(channel=channel, text=body) | ||
|
||
@backoff.on_exception(backoff.expo, SlackApiError, factor=10, base=2, max_tries=5) | ||
@statsd_gauge("reports.slack.send") | ||
def send(self) -> None: | ||
global_logs_context = getattr(g, "logs_context", {}) or {} | ||
try: | ||
client = get_slack_client() | ||
title = self._content.name | ||
body = self._get_body() | ||
|
||
try: | ||
channels = self._get_channels(client) | ||
except SlackApiError: | ||
logger.warning( | ||
"Slack scope missing. Using deprecated API to get channels. Please update your Slack app to use the new API.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we know what scope is missing here? Can we add it to the error message? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume it will be sent by slack to the error message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We expect |
||
extra={ | ||
"execution_id": global_logs_context.get("execution_id"), | ||
}, | ||
) | ||
self._deprecated_upload_files(client, title, body) | ||
return | ||
|
||
if channels == []: | ||
raise NotificationParamException("No valid channel found") | ||
|
||
files = self._get_inline_files() | ||
|
||
# files_upload returns SlackResponse as we run it in sync mode. | ||
for channel in channels: | ||
if len(files) > 0: | ||
for file in files: | ||
client.files_upload_v2( | ||
channel=channel, | ||
file=file, | ||
initial_comment=body, | ||
title=title, | ||
) | ||
else: | ||
client.chat_postMessage(channel=channel, text=body) | ||
|
||
logger.info( | ||
"Report sent to slack", | ||
extra={ | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
from superset.models.core import Database | ||
from superset.models.dashboard import Dashboard | ||
from superset.models.slice import Slice | ||
from superset.reports.models import ReportSchedule | ||
from superset.utils import json | ||
from superset.utils.core import get_example_default_schema | ||
from superset.utils.database import get_example_database | ||
|
@@ -81,6 +82,7 @@ def load_world_bank_dashboard_with_slices_module_scope(load_world_bank_data): | |
with app.app_context(): | ||
dash_id_to_delete, slices_ids_to_delete = create_dashboard_for_loaded_data() | ||
yield | ||
_cleanup_reports(dash_id_to_delete, slices_ids_to_delete) | ||
_cleanup(dash_id_to_delete, slices_ids_to_delete) | ||
|
||
|
||
|
@@ -143,6 +145,21 @@ def _cleanup(dash_id: int, slices_ids: list[int]) -> None: | |
db.session.commit() | ||
|
||
|
||
def _cleanup_reports(dash_id: int, slices_ids: list[int]) -> None: | ||
reports_with_dash = ( | ||
db.session.query(ReportSchedule).filter_by(dashboard_id=dash_id).all() | ||
) | ||
reports_with_slices = ( | ||
db.session.query(ReportSchedule) | ||
.filter(ReportSchedule.chart_id.in_(slices_ids)) | ||
.all() | ||
) | ||
Comment on lines
+149
to
+156
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can probably combine this into something like (untested): reports = (
db.session.query(ReportSchedule)
.filter(
or_(
ReportSchedule.dashboard_id == dash_id,
ReportSchedule.chart_id.in_(slices_ids),
)
).all()
) |
||
|
||
for report in reports_with_dash + reports_with_slices: | ||
db.session.delete(report) | ||
db.session.commit() | ||
|
||
|
||
def _get_dataframe(database: Database) -> DataFrame: | ||
data = _get_world_bank_data() | ||
df = pd.DataFrame.from_dict(data) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you can use
list[str]
now, given the lowest Python version we support.