Adding Message Metadata Support to Bolt JS #1508

dannyhostetler · 2022-06-30T17:47:53Z

Summary

The PR introduces support for Message Metadata events within the Bolt framework. This should be paired together with slackapi/node-slack-sdk#1504 as the "MessageMetadata" is defined within.

Support for MessageMetadataPostedEvent, MessageMetadataUpdated and MessageMetadataDeletedEvent are introduced within this PR.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

dannyhostetler · 2022-06-30T17:54:25Z

Regarding Issue #1507

seratch

Basically looks great to me but I have one additional change request 🙇

seratch · 2022-07-01T01:59:43Z

src/types/events/message-events.ts

+  bot_id?: string;
+  user_id: string;
+  team_id: string;
+  message_ts: string;


Can you add deleted_ts: string too?

Another good catch, this is done 👍

CLAassistant · 2022-07-01T04:47:20Z

All committers have signed the CLA.

seratch · 2022-07-01T06:34:30Z

As the CI build failure shows, this pull request requires a new version release of @slack/types. Let me convert this PR to draft mode for now.

dannyhostetler · 2022-07-01T13:36:44Z

I'm assuming we would wait for this milestone before consider merging this PR, right?

seratch · 2022-07-04T04:49:05Z

@dannyhostetler Thanks for checking the milestone. Since other tasks that were listed there need more time, I've moved them to other milestones. We will release a new package version shortly.

seratch · 2022-07-04T05:09:45Z

types v2.6.0 is now available https://github.com/slackapi/node-slack-sdk/releases/tag/%40slack%2Ftypes%402.6.0

seratch · 2022-07-04T05:12:35Z

Can you fix the test failure detected by GitHub Actions jobs?

TSError: ⨯ Unable to compile TypeScript:
Error: src/types/events/message-events.spec.ts(230,11): error TS2322: Type '{ type: "message_metadata_deleted"; channel_id: string; event_ts: string; previous_metadata: { event_type: string; event_payload: { key: string; }; }; app_id: string; bot_id: string; user_id: string; team_id: string; message_ts: string; }' is not assignable to type 'MessageMetadataEvent'.
  Property 'deleted_ts' is missing in type '{ type: "message_metadata_deleted"; channel_id: string; event_ts: string; previous_metadata: { event_type: string; event_payload: { key: string; }; }; app_id: string; bot_id: string; user_id: string; team_id: string; message_ts: string; }' but required in type 'MessageMetadataDeletedEvent'.

…yload

codecov · 2022-07-05T14:46:36Z

Codecov Report

Merging #1508 (7a7c82b) into main (7eec438) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 7a7c82b differs from pull request most recent head 43beefa. Consider uploading reports for the commit 43beefa to get more accurate results

@@            Coverage Diff             @@
##             main    #1508      +/-   ##
==========================================
+ Coverage   82.00%   82.07%   +0.06%     
==========================================
  Files          18       18              
  Lines        1495     1495              
  Branches      435      435              
==========================================
+ Hits         1226     1227       +1     
  Misses        172      172              
+ Partials       97       96       -1

Impacted Files	Coverage Δ
src/types/middleware.ts	`100.00% <ø> (ø)`
src/App.ts	`84.11% <100.00%> (+0.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7eec438...43beefa. Read the comment docs.

filmaj

Hey, this looks good to me!

I did have one question and one suggestion.

Suggestion: what about adding a very simple example to the examples/ directory that includes showing an app listening to metadata events as well as using these types? I think that would be helpful to a lot of people, as well as for us the maintainers, whom have to quickly reproduce different scenarios for how people build apps. I always use the examples a lot to help with triaging new incoming issues.

Question: do we need to take these metadata events into account in the event / middleware processing chain in bolt? In particular, the conversation ID extraction logic. This method has different logic to pull out the conversation ID based on the kind of event being received. Do we need to add some more logic to take into account the message metadata event structure here?

Thanks a lot for taking the time to send these pull requests, it is very much appreciated 🙏

seratch · 2022-07-05T22:54:28Z

@filmaj

Question: do we need to take these metadata events into account in the event / middleware processing chain in bolt? In particular, the conversation ID extraction logic. This method has different logic to pull out the conversation ID based on the kind of event being received. Do we need to add some more logic to take into account the message metadata event structure here?

No, I don't think so. The conversation ID is supposed to be used only by https://slack.dev/bolt-js/concepts#conversation-store and the convo store feature is associated with message-type events. If a developer intentionally use metadata events only, I am sure that the developer does not expect convo store to work with convo store well.

Also, in reality, I think that the convo store is not used a lot. The feature was added to provide an alternative for hubot's brain: https://slack.dev/bolt-js/tutorial/hubot-migration#migrating-the-brain-to-the-conversation-store There is no downside to keep the feature so far but, in future major versions, we may consider deprecating the feature or removing the feature from the core module.

dannyhostetler · 2022-07-06T01:13:37Z

@filmaj

Suggestion: what about adding a very simple example to the examples/ directory that includes showing an app listening to metadata events as well as using these types? I think that would be helpful to a lot of people, as well as for us the maintainers, whom have to quickly reproduce different scenarios for how people build apps. I always use the examples a lot to help with triaging new incoming issues.

Fantastic suggestion, and thanks for the feedback! Agreed, I myself am always referring to these examples. I'm working on mocking up a simple example RE: Message Metadata events and I'll get it pushed within the next day or so.

filmaj · 2022-07-06T12:53:15Z

Ah, thanks for educating me on that @seratch !

seratch · 2022-07-12T08:47:57Z

examples/message-metadata/app.js

+
+app.event('message_metadata_posted', async ({ event, say }) => {
+  const { message_ts: thread_ts } = event;
+  say({


Suggested change

say({

await say({

My mistake, thanks for catching. This is fixed.

seratch · 2022-07-12T08:48:38Z

examples/message-metadata/app.js

+// Post a message with Message Metadata
+app.command('/post', async ({ ack, command, say }) => {
+  await ack();
+  say({


Suggested change

say({

await say({

My mistake, thanks for catching. This is fixed.

filmaj · 2022-07-12T14:01:55Z

Hey @dannyhostetler ! One thing I noticed trying to get the example running by linking up the bolt-js root repo first is I get this error:

➜ npm link
npm ERR! code 2
npm ERR! path /Users/fmaj/src/bolt-js
npm ERR! command failed
npm ERR! command sh -c npm run build
npm ERR! > @slack/[email protected] build
npm ERR! > tsc
npm ERR!
npm ERR! src/types/events/message-events.ts(1,48): error TS2305: Module '"@slack/types"' has no exported member 'MessageMetadata'.

I think that's because @slack/types didn't introduce support for MessageMetadata until v2.6.0. So I think we need to bump the @slack/types in package.json to be at least 2.6.0.

Odd that the CI didn't pick this up... I guess because the semver specifier for @slack/types in package.json is ^2.4.0, and CI installs a fresh copy of the types, so it gets the latest version just fine. But I guess on my system I had an earlier version installed locally and thus the error.

dannyhostetler · 2022-07-12T18:47:12Z

Hey @dannyhostetler ! One thing I noticed trying to get the example running by linking up the bolt-js root repo first is I get this error:
➜ npm link
npm ERR! code 2
npm ERR! path /Users/fmaj/src/bolt-js
npm ERR! command failed
npm ERR! command sh -c npm run build
npm ERR! > @slack/[email protected] build
npm ERR! > tsc
npm ERR!
npm ERR! src/types/events/message-events.ts(1,48): error TS2305: Module '"@slack/types"' has no exported member 'MessageMetadata'.
I think that's because @slack/types didn't introduce support for MessageMetadata until v2.6.0. So I think we need to bump the @slack/types in package.json to be at least 2.6.0.

Odd that the CI didn't pick this up... I guess because the semver specifier for @slack/types in package.json is ^2.4.0, and CI installs a fresh copy of the types, so it gets the latest version just fine. But I guess on my system I had an earlier version installed locally and thus the error.

@filmaj Think this should be good now. For what ever reason I explicitly removed that from the package.json because @slack\types is a dependency package of bolt and didn't think it was needed. Anywho, give it a try again and see if this works for you.

filmaj · 2022-07-12T19:21:47Z

@dannyhostetler Oh, I meant in the top-level package.json in this repo!

dannyhostetler · 2022-07-12T19:27:15Z

@dannyhostetler Oh, I meant in the top-level package.json in this repo!

Oh ok, gotcha. Bumped to ^2.7.0. Thanks for keeping me inline on this one 🤝

filmaj · 2022-07-12T19:45:04Z

OK, got the package building! However, I can't seem to trigger the message_metadata_posted event in the app 🤔 I added the /post slash command to my app definition on api.slack.com/apps, as well as subscribed the app to the message_metadata_posted event. The /post works, and the bot answers "Message Metadata Posting", but can't seem to get the "Message Metadata Posted" event to trigger...

I'll try to dig in more tomorrow to see what's going on.

dannyhostetler · 2022-07-12T20:23:24Z

@filmaj I'm willing to bet it's because you're missing metadata_subscriptions in your manifest.

"event_subscriptions": { "bot_events": [ "message_metadata_deleted", "message_metadata_posted", "message_metadata_updated" ], "metadata_subscriptions": [ { "app_id": "[your app id]", "event_type": "my_event" } ] },

filmaj · 2022-07-13T13:56:57Z

You would have won that bet @dannyhostetler !

Last request for this PR: can we add a basic README.md to the example directory that details the setup instructions for this app? Maybe as simple as "create an app on api.slack.com/apps from scratch, note down the App ID, open up the manifest editor on api.slack.com/apps/, paste in the contents of app-manifest.json, update the reference to your App ID in there, save it. Install to a workspace. Export the your app and bot tokens as env vars. Run the app with node app.js. In your workspace run the /post slash command. Voila!'. Just something along the lines of the examples/oauth/README.md, maybe?

dannyhostetler · 2022-07-13T14:05:13Z

@filmaj Yeah, the metadata subscription is a bit tricky (and I don't personally like how you have to specify the app id, I think should it be able to dynamically determine 🤷). No biggie on the README, I should have actually added one earlier (probably just being lazy). I'm traveling over the next day or so, so I'll get this pushed sometime this week.

filmaj · 2022-07-13T14:06:48Z

Yes I agree with you re: entering the App ID, seems like us-at-Slack should be able to infer that 🤨 I'll raise that internally to see if that can be improved.

And thank you for following up on all my requests 🙏 really appreciate your collaboration on this 🙇

dannyhostetler · 2022-07-13T14:51:10Z

@filmaj Actually just went ahead and did this now. Added a basic README, hope this looks good

filmaj

Yes!!! 🎉 Thanks a lot, looks great, dropping an approval here, as soon as the CI passes will merge.

adding message metadata support

86f1ee8

seratch added this to the 3.12.0 milestone Jul 1, 2022

seratch requested review from srajiang, seratch and filmaj July 1, 2022 01:57

seratch added the enhancement M-T: A feature request for new functionality label Jul 1, 2022

seratch requested changes Jul 1, 2022

View reviewed changes

seratch marked this pull request as draft July 1, 2022 06:34

adding deleted_ts to MessageMetadataDeletedEvent

e2d1229

seratch marked this pull request as ready for review July 4, 2022 05:09

adding missing prop deleted_ts to support message_metadata_deleted pa…

5ff4e3d

…yload

dannyhostetler requested a review from seratch July 4, 2022 13:42

filmaj reviewed Jul 5, 2022

View reviewed changes

dannyhostetler added 2 commits July 9, 2022 06:52

adding message metadata simple example

a2b8f64

fixing line formatting

48de091

dannyhostetler requested a review from filmaj July 9, 2022 12:04

seratch reviewed Jul 12, 2022

View reviewed changes

adding missing await statement for say promise

8279b5f

dannyhostetler requested a review from seratch July 12, 2022 11:09

bumping @slack\types in package.json

2fea552

bumping @slack\types in top-level package.json

8bdc8f9

minor tweak to package.json

cd68b23

adding README

43beefa

filmaj approved these changes Jul 13, 2022

View reviewed changes

filmaj merged commit fef721f into slackapi:main Jul 13, 2022

This was referenced Aug 15, 2022

[WIP] Add app_id and metadata to generic message event #1065

Closed

Message event types to include metadata #1064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Message Metadata Support to Bolt JS #1508

Adding Message Metadata Support to Bolt JS #1508

dannyhostetler commented Jun 30, 2022 •

edited

Loading

dannyhostetler commented Jun 30, 2022

seratch left a comment

seratch Jul 1, 2022

dannyhostetler Jul 1, 2022

CLAassistant commented Jul 1, 2022 •

edited

Loading

seratch commented Jul 1, 2022

dannyhostetler commented Jul 1, 2022

seratch commented Jul 4, 2022

seratch commented Jul 4, 2022

seratch commented Jul 4, 2022

codecov bot commented Jul 5, 2022 •

edited

Loading

filmaj left a comment

seratch commented Jul 5, 2022 •

edited

Loading

dannyhostetler commented Jul 6, 2022

filmaj commented Jul 6, 2022

seratch Jul 12, 2022

dannyhostetler Jul 12, 2022

seratch Jul 12, 2022

dannyhostetler Jul 12, 2022

filmaj commented Jul 12, 2022

dannyhostetler commented Jul 12, 2022

filmaj commented Jul 12, 2022

dannyhostetler commented Jul 12, 2022

filmaj commented Jul 12, 2022

dannyhostetler commented Jul 12, 2022

filmaj commented Jul 13, 2022 •

edited

Loading

dannyhostetler commented Jul 13, 2022 •

edited

Loading

filmaj commented Jul 13, 2022

dannyhostetler commented Jul 13, 2022

filmaj left a comment

Adding Message Metadata Support to Bolt JS #1508

Adding Message Metadata Support to Bolt JS #1508

Conversation

dannyhostetler commented Jun 30, 2022 • edited Loading

Summary

Requirements (place an x in each [ ])

dannyhostetler commented Jun 30, 2022

seratch left a comment

Choose a reason for hiding this comment

seratch Jul 1, 2022

Choose a reason for hiding this comment

dannyhostetler Jul 1, 2022

Choose a reason for hiding this comment

CLAassistant commented Jul 1, 2022 • edited Loading

seratch commented Jul 1, 2022

dannyhostetler commented Jul 1, 2022

seratch commented Jul 4, 2022

seratch commented Jul 4, 2022

seratch commented Jul 4, 2022

codecov bot commented Jul 5, 2022 • edited Loading

Codecov Report

filmaj left a comment

Choose a reason for hiding this comment

seratch commented Jul 5, 2022 • edited Loading

dannyhostetler commented Jul 6, 2022

filmaj commented Jul 6, 2022

seratch Jul 12, 2022

Choose a reason for hiding this comment

dannyhostetler Jul 12, 2022

Choose a reason for hiding this comment

seratch Jul 12, 2022

Choose a reason for hiding this comment

dannyhostetler Jul 12, 2022

Choose a reason for hiding this comment

filmaj commented Jul 12, 2022

dannyhostetler commented Jul 12, 2022

filmaj commented Jul 12, 2022

dannyhostetler commented Jul 12, 2022

filmaj commented Jul 12, 2022

dannyhostetler commented Jul 12, 2022

filmaj commented Jul 13, 2022 • edited Loading

dannyhostetler commented Jul 13, 2022 • edited Loading

filmaj commented Jul 13, 2022

dannyhostetler commented Jul 13, 2022

filmaj left a comment

Choose a reason for hiding this comment

dannyhostetler commented Jun 30, 2022 •

edited

Loading

Requirements (place an `x` in each `[ ]`)

CLAassistant commented Jul 1, 2022 •

edited

Loading

codecov bot commented Jul 5, 2022 •

edited

Loading

seratch commented Jul 5, 2022 •

edited

Loading

filmaj commented Jul 13, 2022 •

edited

Loading

dannyhostetler commented Jul 13, 2022 •

edited

Loading