-
Notifications
You must be signed in to change notification settings - Fork 400
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
Add in block_suggestion
interactivity handler support
#1645
Conversation
Codecov Report
@@ Coverage Diff @@
## next-gen #1645 +/- ##
===========================================
Coverage ? 80.70%
===========================================
Files ? 20
Lines ? 1778
Branches ? 505
===========================================
Hits ? 1435
Misses ? 221
Partials ? 122 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -36,36 +36,62 @@ export type KnownOptionsPayloadFromType<T extends string> = Extract<SlackOptions | |||
*/ | |||
export interface BlockSuggestion extends StringIndexed { |
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.
Definitely would love a double check on this schema and thoughts on the below. 👇
Right now the schema is a hybrid between Fil's schema (which I'm guessing might be part of a newer block_suggestions
approach?) and the original Block Suggestions schema added to the options
object, which also contains some legacy things (such as InteractiveMessage
and DialogSuggestion
), about a year and a half ago.
If we use just the new schema, we do get a few errors (ex: the enterprise_id property not being able to be accessed
error thrown in App.ts
that is called out above, as well as some issues with accessing certain properties in the unit tests where the new and old schemas deviate). This is causing concern for me that using just the new schema might accidentally wipe compatibility for how folks are currently using this with Bolt within .options()
, if they are at all. Would love some thoughts on how to approach this, whether it's keeping it as this hybrid approach or trying to migrate everything to the new approach and mitigating any errors that pop up!
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.
At the moment I lean against trying to consume these types, as internally in bolt-js we already have a model of block_suggestion
which needs to be compatible with legacy and next-gen event payloads. I don't want to be using different types internally for blockSuggestion handling vs. .options block suggestion handling events because the underlying events aren't different between next-gen and today's block_suggestion (nor do we want them to drift too much).
It would also add additional dependency for us on deno_slack_sdk to be selectively consuming deno_slack_sdk
function handler types. Currently we're keeping this dependency limited to manifest generation.
Would love to know @filmaj's thoughts on this though since sounds like this could be a candidate for the shared types work he's been prototyping.
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.
The 'original' Block Suggestions schema that @hello-ashleyintech links to looks no longer valid to me. I just played around with this PR and it worked great, but, the block suggestion payloads coming in look more like the type I wrote up in the deno-slack-sdk than the original schema in bolt-js.
As for how to go about that, I leave it to you. Whether you want to update the original schema in bolt or consume the deno one, your call. But, definitely, do some testing on your end to inspect the shape of that payload (in both enterprise and non-enterprise workspaces since the shape changes somewhat based on that. I have access to one that Jim set up, ping me if you want in on that).
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.
Update to this: I ended up adding a new type for Block Suggestions in the next-gen interactivity scenario and left the old Block Suggestions type under options as is! this is to continue legacy support of the original BlockSuggestions while still enabling the next-gen work. See this comment for more context!
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.
So therefore, depending on 1.0 vs. 2.0 context, the payload shapes differ?
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.
yes!
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.
Thanks for confirming! What a headache, though...
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.
Excellent work! Worked like a charm for me!
I left some comments on changes, though. I think we do need to update the type of the block suggestion payload to conform to the new structure we seem to be dispatching from the backend.
@@ -36,36 +36,62 @@ export type KnownOptionsPayloadFromType<T extends string> = Extract<SlackOptions | |||
*/ | |||
export interface BlockSuggestion extends StringIndexed { |
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.
The 'original' Block Suggestions schema that @hello-ashleyintech links to looks no longer valid to me. I just played around with this PR and it worked great, but, the block suggestion payloads coming in look more like the type I wrote up in the deno-slack-sdk than the original schema in bolt-js.
As for how to go about that, I leave it to you. Whether you want to update the original schema in bolt or consume the deno one, your call. But, definitely, do some testing on your end to inspect the shape of that payload (in both enterprise and non-enterprise workspaces since the shape changes somewhat based on that. I have access to one that Jim set up, ping me if you want in on that).
/** | ||
* Block Suggestion payload model for next-gen interactivity | ||
*/ | ||
export interface BlockSuggestionPayload extends StringIndexed { |
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 took the payload from Fil's PR and had to tweak it (make some params optional and add more params) a bit based on what I observed for block_suggestions
payloads in both enterprise and regular workspaces. If anyone is testing this, would love confirmation that the payloads look similar! You can view example shapes of enterprise + non enterprise payloads I was getting in the corresponding test file, src/types/block-suggestion/index.spec.ts
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.
Good work @ashleymcm, things are working nicely on my end on manual tests. A suggestions BlockSuggestionPayload
type:
- We can have
BlockSuggestionPayload
extendFunctionContext
instead of explicitly naming those types to account for thefunction_data
,bot_access_token
and optionalinteractivity
types.
I am okay to proceed with merging for now and followup with any improvements though!
* Add in new Block Suggestion type
Summary
This PR mirrors Fil's work in slackapi/deno-slack-sdk#116 to add interactivity support for
block_suggestions
in Bolt JS. This PR accomplishes this through adding ablockSuggestion
function that can be chained from a SlackFunction object, for example:This PR also mirrors a lot of Sarah's work in #1567!
Testing
ah-add-block-suggestions
slack create -t slack-samples/bolt-js-request-time-off
npm link
in thebolt-js
repo@slack/bolt
package in your test app - runnpm link @slack/bolt
in your test app. Now the code from this branch will be available on your test app! ✨slack create trigger --trigger-def "triggers/link-shortcut.json"
listeners/functions/request-approval.js
file, add the following to the Block Kit within thenotifyApprover
function's call toclient.chat.postMessage
:blockSuggestionHandler
function intorequest-approval.js
:blockSuggestion
function by adding any of the following to therequestApprovalFunc
at the bottom of the file like so:Then, run the trigger. You will receive a message from the application and you will be able to search for quotes using the suggestion dropdown. To pull up valid results, you can type in keywords
and
orlet
. View the demo below:Open TODOs for this PR (other than addressing feedback)
block_suggestions
schema looks right and update any existing unit tests that don't align with this schemaTODOs after this PR
blockActions
function support to create consistency with theblockSuggestion
naming schemaRequirements (place an
x
in each[ ]
)