betodealmeida commented on code in PR #28797:
URL: https://github.com/apache/superset/pull/28797#discussion_r1626638438
##########
superset/reports/notifications/slack.py:
##########
@@ -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]:
Review Comment:
Nit: you can use`list[str]` now, given the lowest Python version we support.
##########
tests/integration_tests/fixtures/world_bank_dashboard.py:
##########
@@ -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()
+ )
Review Comment:
You can probably combine this into something like (untested):
```python
reports = (
db.session.query(ReportSchedule)
.filter(
or_(
ReportSchedule.dashboard_id == dash_id),
ReportSchedule.chart_id.in_(slices_ids),
)
).all()
)
```
##########
superset/reports/notifications/slack.py:
##########
@@ -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.",
Review Comment:
Do we know what scope is missing here? Can we add it to the error message?
##########
superset/reports/notifications/slack.py:
##########
@@ -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(
Review Comment:
Nice!
##########
superset/reports/notifications/slack.py:
##########
@@ -60,16 +61,24 @@ 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"
"""
recipient_str =
json.loads(self._recipient.recipient_config_json)["target"]
- return ",".join(get_email_address_list(recipient_str))
+ channel_recipients = get_email_address_list(recipient_str)
+
+ conversations_list_response = client.conversations_list(
+ types="public_channel,private_channel"
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
