♻️ ref(slack): fix slack webhook actions typing #77322
Conversation
github-actions
bot
added
the
Scope: Backend
| webhook_client = WebhookClient(response_url) | ||
| try: | ||
| webhook_client.send( | ||
| blocks=json_blocks, | ||
| blocks=response.get("blocks"), |
There was a problem hiding this comment.
don't need to do this, originally that orjson line only converted from single quotes to double quotes (i am assume this was because in pre-slack_sdk world, we needed to. the slack_sdk handles this for us, i verified this with my local setup
| action_data = slack_request.data.get("actions") | ||
| if action_data: |
There was a problem hiding this comment.
don't need this since we have ga'ed blockkit for months now and won't be dealing with non-blockkit interactions.
| @@ -421,7 +423,7 @@ def __init__( | |||
| event: Event | GroupEvent | None = None, | |||
| tags: set[str] | None = None, | |||
| identity: RpcIdentity | None = None, | |||
| actions: Sequence[MessageAction] | None = None, | |||
| actions: Sequence[MessageAction | BlockKitMessageAction] | None = None, | |||
There was a problem hiding this comment.
what is the difference between MessageAction and BlockKitMessageAction? should we always be using one over the other if we don't support non-block kit anymore?
There was a problem hiding this comment.
ideally we should delete MessageAction, but i saw some places in our logic where we create a MessageAction payload to send to slack, so i want to slowly remove it so make sure we don't break changes since ff this is hard
There was a problem hiding this comment.
Yeah, as I mentioned here it seems like BlockKitMessageAction ought to extend MessageAction (assuming we need both).
The "TODO" comment on MessageAction suggests that its bottom four attributes (including block_id, which is confusing) ought to belong to some other subclass. But I agree with delaying that sort of refactoring for later.
I'm okay with using MessageAction | BlockKitMessageAction for type hints temporarily, but would consider changing BlockKitMessageAction to extend MessageAction instead.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #77322 +/- ##
==========================================
+ Coverage 78.12% 78.14% +0.01%
==========================================
Files 6996 6967 -29
Lines 310293 309641 -652
Branches 50765 50658 -107
==========================================
- Hits 242415 241963 -452
- Misses 56156 61259 +5103
+ Partials 11722 6419 -5303 |
iamrajjoshi
force-pushed
the
raj/webhook-actions-typing
branch
from
28bfc32
to
f6ca079
Compare
| if isinstance(error.detail, dict): | ||
| text = list(*error.detail.values())[0] | ||
| else: | ||
| text = str(error.detail[0]) |
There was a problem hiding this comment.
Technically the type definition for error.detail allows lists and/or dictionaries to be nested recursively. It might be more correct to do something like the helper function I sketched out here.
There was a problem hiding this comment.
i yoinked that helper into this pr 😄
fixing the typing for Slack webhook actions. i also removed some old non slack blockkit logic since we have GA'ed for a while and won't have to deal with interactions with the old messages.
fixing the typing for Slack webhook actions. i also removed some old non slack blockkit logic since we have GA'ed for a while and won't have to deal with interactions with the old messages.
fixing the typing for Slack webhook actions. i also removed some old non slack blockkit logic since we have GA'ed for a while and won't have to deal with interactions with the old messages.