ref(slack): use sdk client to get channel id #72752
Conversation
github-actions
bot
added
the
Scope: Backend
|
had to update some other tests because of the feature flag check 😢 |
iamrajjoshi
force-pushed
the
raj/mes-ref/slack/sdk-2.10
branch
from
8e1e158
to
beb202c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #72752 +/- ##
==========================================
- Coverage 78.03% 78.01% -0.02%
==========================================
Files 6632 6632
Lines 296032 296094 +62
Branches 50994 51002 +8
==========================================
- Hits 230994 230993 -1
- Misses 58754 58814 +60
- Partials 6284 6287 +3
|
| "types": "public_channel,private_channel", | ||
| } | ||
| try: | ||
| users_list = client.users_list(limit=SLACK_GET_CHANNEL_ID_PAGE_SIZE, kwargs=payload) |
There was a problem hiding this comment.
will replace this after cathy's refactor pr gets merged
| @@ -1,6 +1,5 @@ | |||
| __all__ = ( | |||
There was a problem hiding this comment.
I am planning to add a task to clean these files at the end of the refactoring
| SLACK_DEFAULT_TIMEOUT = 10 | ||
| MEMBER_PREFIX = "@" | ||
| CHANNEL_PREFIX = "#" | ||
| strip_channel_chars = "".join([MEMBER_PREFIX, CHANNEL_PREFIX]) | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
It can't be because i modify the instance in the function.
| if time.time() > time_to_quit: | ||
| break |
There was a problem hiding this comment.
(i'm wondering what the timeout is for? i don't see this being kicked off as an async task. maybe some API calls have a timeout)
should we return SlackChannelIdData with timed_out=True for the timeout here?
There was a problem hiding this comment.
There is a timeout in case the member list is extremely long, in which case the function will return early if it can't find a user that matches in time. I should return early here as you said.
There was a problem hiding this comment.
i guess i'm wondering what is necessitating the timeout in terms of the call stack, it's probably some upstream caller then
| :return: a tuple of three values | ||
| 1. prefix: string (`"#"` or `"@"`) | ||
| 2. channel_id: string or `None` | ||
| 3. timed_out: boolean (whether we hit our self-imposed time limit) |
There was a problem hiding this comment.
It's no longer returning a tuple. If these field details need to be documented, we should move them to the dataclass definition.
| id_data.prefix = "#" | ||
| except SlackApiError: | ||
| logger.exception("rule.slack.channel_check_error", extra=logger_params) | ||
| return id_data |
There was a problem hiding this comment.
We should document the meaning of returning SlackChannelIdData(prefix="", channel_id=None, timed_out=False) in the name is None case. It adds a case where prefix is not "#" or "@" as promised in the docstring, for instance. Do we expect the caller to check for this? Would it be appropriate to return None instead?
There was a problem hiding this comment.
i have looked through callers, most of them complain loudly when the channel_id is None. its a good call to document this case.
| id_data.channel_id = check_for_channel(client, name) | ||
| id_data.prefix = "#" |
There was a problem hiding this comment.
Let's refactor this flow to make SlackChannelIdData immutable. We could use local variables here and only construct the object when we're ready to return.
|
|
||
| # We need to check for individual users if we don't have a channel | ||
| users = get_users_sdk(client, integration) | ||
| for user in users: |
There was a problem hiding this comment.
This for-loop has enough going on (iterating over users and trying to find a match on a best-effort basis before timing out) that I think it's worth breaking out into a private helper method.
| # The "name" field is unique (this is the username for users) | ||
| # so we return immediately if we find a match. | ||
| # convert to lower case since all names in Slack are lowercase | ||
| if name and str(user["name"]).lower() == name.lower(): |
There was a problem hiding this comment.
It's better to call .casefold() than .lower() to avoid problems with obscure Unicode letters. https://stackoverflow.com/a/319435
| } | ||
| logger.exception("slack.chat_scheduleMessage.error", extra=logger_params) | ||
| if "channel_not_found" in str(e): | ||
| return None |
There was a problem hiding this comment.
Is this an expected condition? Will it happen often enough that we don't want to call logger.exception every time?
There was a problem hiding this comment.
Essentially this channel checks for either a channel or a particular user. if we get a channel not found, we attempt to find a user with the particular name, which is why we have to end up checking for this error
iamrajjoshi
force-pushed
the
raj/mes-ref/slack/sdk-2.10
branch
from
8ad4ff8
to
87cc3c5
Compare
| _channel_id = None | ||
| _prefix = "" |
There was a problem hiding this comment.
just wondering why you chose to have underscores here? these aren't global variables?
There was a problem hiding this comment.
i did that cause intellisense kept suggesting the variables defined in the previous function were in scope here.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Adds feature-flagged usage of Slack SDK client to check channel id by schedule-sending a message and then immediately deleting. Also adds tests for the code area.