fix: use channel id with new slack api for file uploads #28797
Conversation
| channel_recipients = get_email_address_list(recipient_str) | ||
|
|
||
| conversations_list_response = client.conversations_list( | ||
| types="public_channel,private_channel" |
There was a problem hiding this comment.
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.
Will this work if the application doesn't have the right scope? I tried testing this here with my token and got this response:
{
"ok": false,
"error": "missing_scope",
"needed": "channels:read,groups:read,mpim:read,im:read",
"provided": "commands,chat:write,chat:write.public,channels:history,im:history,mpim:history,groups:history,files:write"
}
There was a problem hiding this comment.
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.
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.
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.
I was thinking about this more @betodealmeida and it would be a breaking change because the existing applications would need the channels:read slack scope for this, but maybe for the next major release, it would a nice improvement to have a multiselect ui for the channel and then also store the id. ❤️
eschutho
force-pushed
the
elizabeth/slack-update
branch
2 times, most recently
from
384b043
to
2e8fb37
Compare
eschutho
force-pushed
the
elizabeth/slack-update
branch
from
2e8fb37
to
b6325bf
Compare
eschutho
force-pushed
the
elizabeth/slack-update
branch
from
67fbddb
to
cb6ac62
Compare
eschutho
force-pushed
the
elizabeth/slack-update
branch
5 times, most recently
from
4f4f0db
to
331c964
Compare
eschutho
force-pushed
the
elizabeth/slack-update
branch
from
331c964
to
1c96045
Compare
| @@ -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]: | |||
There was a problem hiding this comment.
Nit: you can uselist[str] now, given the lowest Python version we support.
| channel_recipients = get_email_address_list(recipient_str) | ||
|
|
||
| conversations_list_response = client.conversations_list( | ||
| types="public_channel,private_channel" |
There was a problem hiding this 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.
| @deprecated(deprecated_in="4.1") | ||
| def _deprecated_upload_files( |
| 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.
Do we know what scope is missing here? Can we add it to the error message?
There was a problem hiding this comment.
I assume it will be sent by slack to the error message.
There was a problem hiding this comment.
We expect channels:read as per the difference between v1 and v2 apis, but it could be anything if they're not set up correctly.
| 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() | ||
| ) |
There was a problem hiding this comment.
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()
)
|
I'll address @betodealmeida's comments in a fast-follow. |
(cherry picked from commit 7253755)
SUMMARY
Fixing changes from #28783 where we needed to update the slack sdk call to the new files_upload_v2 method. The api changes a few things that were missed in the original PR: the
channelsarg is now the channel id instead of name. Also it is recommended to usechannelinstead ofchannels. I also found that the slack util file wasn't being used and cleaned up a few other affected parts.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION