-
Notifications
You must be signed in to change notification settings - Fork 511
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
Enhancement/slack bolt #2767
Enhancement/slack bolt #2767
Conversation
TypeError: __init__() got an unexpected keyword argument 'code' https://github.com/slackapi/bolt-python/blob/main/slack_bolt/response/response.py#L7
* branch off bolt pr and add bookmarks to conversation * branch off bolt pr and add bookmarks to conversation * add types and docstring to set_conversation_bookmark * use existing slack_client functionality instead of bolt to call bookmark.add api * remove deleted bolt function from imports Co-authored-by: Will Sheldon <[email protected]>
result = await client.views_update( | ||
view_id=body["view"]["id"], | ||
trigger_id=body["trigger_id"], | ||
view=modal, | ||
) |
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.
passing in the hash from body
here resulted in a hash conflict and prevented the incident from being created. The hash we got here in body
was associated with the first Report Incident
view and I suspect we want the hash of the Creating incident resources...
view above. It's unclear to me how to send this hash along to this function. I found what looks like a similar issue here: slackapi/bolt-js#787
Nonetheless, removing the hash check resolved the issue, and this function appears to be the odd one out in terms of validating the hash of a view that leverages an ack=
handler/listener.
Current progress for command tests: ✅ |
modal = Modal( | ||
title="Error", close="Close", blocks=[Section(text="Something went wrong...")] | ||
).build() | ||
|
||
if body: |
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.
I think there can be a situation where an internal app error occurs that isn't related Bolt/Slack and this will not trigger (and result in a silent fail). Going to test and revisit.
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.
This ended up being a a lazy listener issue: slackapi/bolt-python#635
Error handling still needs some work to mirror the old implementation though. Primarily around relaying back user friendly error messages through ephemeral responses in Slack.
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.
If possible, we should try and send error messages where the user is. e.g. if a user submits a modal and there is an error, the error should appear in the modal. Sending an ephemeral message in these cases is confusing as the user isn't quite sure which action they took resulted in an error. We can always fall back to ephemeral messages in other when it's not possible.
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.
Agreed, it should be easy to check for a response_url
within the global error handler to mostly get the ideal error flow:
# the user is in a message flow
if body.get("response_url"):
await respond(text=str(error), response_type="ephemeral")
One issue w/ this is that Lazy Listener functions don't seem to surface exceptions within them to the error handler at all and also don't currently log the actual exception, it's just generic:
async def handle_add_timeline_submission_event(
body: dict,
user: DispatchUser,
client: AsyncWebClient,
context: AsyncBoltContext,
form_data: dict,
):
"""Handles the add timeline submission event."""
raise Exception("There was an error!")
ERROR:Failed to run an internal function (There was an error!):/Users/wshel/.pyenv/versions/3.9.7/envs/dispatch/lib/python3.9/site-packages/slack_bolt/lazy_listener/async_internals.py:request_wired_wrapper:30
When this happens the user will be stuck on the intermediary model created by the ack_
function and there will be no indication of an issue. Looking for potential workarounds.
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.
One issue w/ this is that Lazy Listener functions don't seem to surface exceptions within them to the error handler at all and also don't currently log the actual exception, it's just generic.
Perhaps we just handle exceptions (e.g. wrap them in try: catch) in lazy functions (instead of relying on the global handler)?
One known issue is that timeline reaction emoji feature does not work. |
I will tackle that in a follow-up PR. |
This PR refactors the current slack functionality to use the bolt and blockkit libraries.
@mvilanova, this isn't perfect and still needs some more testing. But I would appreciate if you took a look and maybe did some testing if you had some cycles available.
I've been mostly using socket mode as it's a bit easier to add commands in my dev environment.