Skip to content
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

Merged
merged 45 commits into from
Jan 10, 2023
Merged

Enhancement/slack bolt #2767

merged 45 commits into from
Jan 10, 2023

Conversation

kevgliss
Copy link
Contributor

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.

@kevgliss kevgliss added the enhancement New feature or request label Dec 13, 2022
wssheldon and others added 4 commits December 20, 2022 15:03
* 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]>
Comment on lines +1691 to +1695
result = await client.views_update(
view_id=body["view"]["id"],
trigger_id=body["trigger_id"],
view=modal,
)
Copy link
Contributor

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.

@wssheldon
Copy link
Contributor

wssheldon commented Dec 23, 2022

Current progress for command tests:

/dispatch-report-incident
/dispatch-add-timeline-event
/dispatch-list-my-tasks
/dispatch-list-participants
/dispatch-list-resources
/dispatch-assign-role
/dispatch-list-workflows
/dispatch-list-tasks
/dispatch-report-executive
/dispatch-report-tactical
/dispatch-run-workflow
/dispatch-update-incident
/dispatch-notifications-group
/dispatch-update-participant
/dispatch-engage-oncall

src/dispatch/cli.py Outdated Show resolved Hide resolved
src/dispatch/plugins/dispatch_slack/feedback.py Outdated Show resolved Hide resolved
src/dispatch/plugins/dispatch_slack/feedback.py Outdated Show resolved Hide resolved
src/dispatch/plugins/dispatch_slack/feedback.py Outdated Show resolved Hide resolved
src/dispatch/plugins/dispatch_slack/feedback.py Outdated Show resolved Hide resolved
src/dispatch/plugins/dispatch_slack/fields.py Outdated Show resolved Hide resolved
src/dispatch/plugins/dispatch_slack/middleware.py Outdated Show resolved Hide resolved
src/dispatch/plugins/dispatch_slack/middleware.py Outdated Show resolved Hide resolved
modal = Modal(
title="Error", close="Close", blocks=[Section(text="Something went wrong...")]
).build()

if body:
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Screenshot 2023-01-10 at 11 06 23 AM

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.

Copy link
Contributor Author

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)?

@wssheldon
Copy link
Contributor

One known issue is that timeline reaction emoji feature does not work.

@kevgliss
Copy link
Contributor Author

One known issue is that timeline reaction emoji feature does not work.

I will tackle that in a follow-up PR.

@kevgliss kevgliss merged commit 31dbe92 into master Jan 10, 2023
@kevgliss kevgliss deleted the enhancement/slack-bolt branch January 10, 2023 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants